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

[Michael Grzeschik]

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.

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 at 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.

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

>>  	usbphyc_phy->active = true;
>>
>>  	return 0;
>

Regards,
Michael

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20230213/ee314fc7/attachment.sig>