Re: [PATCH] phy: stm32-usphyc: add mdelay(1) to fix timeout on some machines

[Fabrice Gasnier <fabrice.gasnier@foss.st.com>]

On 2/13/23 15:59, Michael Grzeschik wrote:
> Hi Fabrice,
> 
> On Wed, Feb 01, 2023 at 05:20:11PM +0100, Fabrice Gasnier wrote:
>> On 1/24/23 21:45, Michael Grzeschik wrote:
>>> An mdelay of 1 seems to be necessary on some machines, since
>>
>> Hi Michael,
>>
>> Could you precise on which board ?
> 
> It was reproducible on one of our lxa-mc1 boards.
> 
>>> the monsel status does not seem to be accurate. On rare occasions just
>>> working with the phy after this pll check lead to no functional usb.
>>
>> Could you elaborate on the symptoms (provide some error logs) ?
> 
> The symptom was, that the dwc2 controller was not resuming from polling
> on the soft reset done bit and so ran into a timeout. (dwc2:
> GRSTCTL_CSFTRST(GRSTCTL_CSFTRST_DONE))
> 
> While debugging we found that the stm vendor uboot phy driver
> was just adding an predefined wait of 200us after setting
> the pll.
> 
> https://github.com/STMicroelectronics/u-boot/blob/v2021.10-stm32mp/drivers/phy/phy-stm32-usbphyc.c#L257
> 
> The comment for the PLL_INIT_TIME_US says that the value is based on the
> possible max pll lock latency and an additional phy init delay:
> 
>  /* max 100 us for PLL lock and 100 us for PHY init */
>  #define PLL_INIT_TIME_US    200
> 
> Reading the datasheet of the stm32mp1, the 100 us sure come from
> tLOCK in Table 41. USB_PLL characteristics. (Page 159)
> 
> https://www.st.com/resource/en/datasheet/stm32mp157c.pdf
> 
> 
> In the mainline Kernel we have the set of lock monitor
> bits that are actually undefined in any register specs
> we read so far.
> 
>  /* STM32_USBPHYC_MONITOR bit fields */
>  #define STM32_USBPHYC_MON_OUT   GENMASK(3, 0)
>  #define STM32_USBPHYC_MON_SEL   GENMASK(8, 4)
>  #define STM32_USBPHYC_MON_SEL_LOCKP 0x1F
>  #define STM32_USBPHYC_MON_OUT_LOCKP BIT(3)
> 
> So this will bring the maximum lock delay to its real
> delay.


Hi Michael,

Thanks for all detailed explanation.
I asked some improvements in the documentations in this area.

> 
> But for the additional phy init delay, we are unsure where the 100us are
> coming from. So we could not really tell if the value makes sense at
> all. The assumption was to increase the delay, just to make sure that
> this will solve the issue for good.
> 
>>> With this short mdelay this issue was not reported again.
>>>
>>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>>> ---
>>>  drivers/phy/st/phy-stm32-usbphyc.c | 9 +++++++++
>>>  1 file changed, 9 insertions(+)
>>>
>>> diff --git a/drivers/phy/st/phy-stm32-usbphyc.c
>>> b/drivers/phy/st/phy-stm32-usbphyc.c
>>> index 5bb9647b078f12..c452a0caceb9fa 100644
>>> --- a/drivers/phy/st/phy-stm32-usbphyc.c
>>> +++ b/drivers/phy/st/phy-stm32-usbphyc.c
>>> @@ -353,6 +353,15 @@ static int stm32_usbphyc_phy_init(struct phy *phy)
>>>          goto pll_disable;
>>>      }
>>>
>>> +    /* This mdelay seems to be necessary on some machines, since the
>>> +     * monsel status does not seem to be accurate. On rare occasions
>>> +     * just working with the phy after this pll check the usb
>>> +     * peripheral (e.g. on the dwc2) run into timeout issues and
>>> +     * leading to no functional usb. With this short mdelay this
>>> +     * issue was not reported again.
>>> +     */
>>> +    mdelay(1);
>>> +
>>
>> The USBPHYC provides two PHY interfaces:
>> - PHY port#1 is for USBH
>> - PHY port#2 can be assigned to USBH or DWC2.
>>
>> Adding some delay here, we'll hit twice this penalty (e.g. for USBH +
>> DWC2 or dual USBH) on all MP1 (MP13 & MP15) platforms.
>>
>> Could you try to narrow down the issue by adding some debug log in:
>> - stm32_usbphyc_phy_init
>> - stm32_usbphyc_clk48_prepare
> 
> The whole issue is actually very easy to reproduce in the bootloader.
> So we already mainlined this patch to the barebox bootloader.
> 
> So this patch is actually a port from the bootloader to make sure
> that the kernel does not run into the issue as well.

Kernel and u-boot drivers are both registering:
- a clock provider
  clk_prepare ... -> stm32_usbphyc_clk48_prepare()
  Currently, there's no wait loop NOR delay here

- a PHY provider (2 PHYs, e.g. 2 ports)
  additional mdelay(1) will be hit twice, one time for DWC2, one for
EHCI/OHCI

Both in kernel and u-boot the clk_prepare() likely happens 1st, then
phy_init().

This is still unclear to me why an additional delay in PHY init, would
be necessary in your environment, with the kernel. Is there a real issue
in kernel ? Or is it more a caution, theoretical port from barebox ?

Still, If needed, I'd advice the delay to be applied only once, e.g. in
stm32_usbphyc_pll_enable(), just after PLLEN bit has been set (as in
u-boot). And rely on MON bits for each PHY init.

Regarding mdelay(): It would also be better to adopt usleep_range()
instead of busy loop.

 	stm32_usbphyc_set_bits(pll_reg, PLLEN);

+	/* Wait for maximum lock time */
+	usleep_range(200, 300);
+
 	return 0;

This way, delay will be hit upon clk_prepare, then polling the MON bits
in phy_init() may become a no-op. So, the delay would be hit only once
(instead of two times). If ever the phy_init() comes first in the
future, only the 1st call to one of the APIs will hit this delay.

Could you check if this solves the issue at your end ?

I had a look at barebox driver, same logic could apply there even if no
clock provider is registered.

> 
> I bet your team at st can figure out a more reasonable value for the
> delay.

I'd recommend 200µs (min) as in u-boot.

Regards,
Fabrice
> 
>>>      usbphyc_phy->active = true;
>>>
>>>      return 0;
>>
> 
> Regards,
> Michael
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel