Re: [PATCH v2 1/1] phy: sun4i-usb: fix phy write on H3 and newer

[Evgeny Boger <boger@wirenboard.com>]

Hi Maxime!
12.01.2022 11:42, Maxime Ripard пишет:
> Hi,
>
> On Tue, Jan 11, 2022 at 07:51:53PM +0300, Evgeny Boger wrote:
>> We noticed that USB hosts won't reliably enumerate USB HS (480Mb/s) devices
>> in our custom A40i-based design. What we observe is that after several
>> attempts USB device would fail to enumerate on EHCI port (HS) and will be
>> enumerated instead on OHCI (FS, 12Mb/s) only:
>>
>> [    6.368009] usb 1-1: new high-speed USB device number 5 using ehci-platform
>> [    6.818008] usb 1-1: device not accepting address 5, error -71
>> [    6.823868] usb usb1-port1: unable to enumerate USB device
>> [    7.308013] usb 3-1: new full-speed USB device number 2 using ohci-platform
>> [    7.575045] usb 3-1: not running at top speed; connect to a high speed hub
>>
>> On some boards one of the ports would work in high-speed mode, but
>> on most of the boards all three USB ports would only work in FS mode.
>>
>> At the same time, USB work flawlessly in high-speed mode in vendor kernel.
>>
>> Looking for the differences in USB code, we found the issue with USB PHY
>> register initialization. Basically, USB PHY driver sets a couple of
>> internal undocumented PHY registers to the predefined constants. These PHY
>> registers are accessed in a very (and I mean VERY) weird way by shifting
>> register addresses and values bit-by-bit. This access method was slightly
>> changed starting from H3 SoC, according to the BSP source for different
>> SoCs.
>>
>> As a result, mainline PHY driver won't set these PHY registers properly
>> resulting in unreliable enumeration in high-speed mode.
>>
>> We don't know whether this issue will result in broken HS mode on all
>> affected SoCs or instead the A40i is an unfortunate exception. What we
>> indeed verified, is that BSPs for all affected SoCs write these registers
>> properly while mainline kernel don't. We also were able to reproduce the
>> USB issue on a couple of A40i boards from other vendors, so we are pretty
>> sure these registers have to always be properly set, regardlress of
>> a hardware layout.
>>
>> The proposed patch is tested on A40i-based Wiren Board 7 building
>> automation controller. More details are below.
>>
>> On older SoCs (prior to H3) PHY register are accessed by manipulating
>> the common register for all PHYs. PHY index is specified by pulsing
>> usbc bit.
>>
>> Newer SoCs leave the access procedure mostly unchanged, the
>> difference being that the latch registers are separate for each PHY.
>>
>> Additionally, accessing USB PHY registers is only possible if phy0 is
>> routed to musb IP instead of HCI.
>>
>> Introduce phy_reg_access_v2 cfg flag for H3 (H2+, H5),
>> R40 (V40, A40i, T3), V3s (V3, S3) and A64 SoCs.
>>
>> On A83t, H6, H616, T507 and probably on more recent hardware,
>> these PHY registers are not used in vendor BSP.
>> So don't set v2 flag for these even newer SoCs as a precaution.
>>
>> Signed-off-by: Evgeny Boger <boger@wirenboard.com>
> This should probably be sent to stable, and have a Fixes tag?

well, phy register writes never worked on H3 SoC and newer, so there is 
no specific commit which broke it. So nothing to put to Fixes: tag.

I'm not sure it qualifies for stable too. Citing guidelines:

 >It must be obviously correct and tested

I was only able to test it on handful of A40i boards and on one A20 board

 >It must fix a real bug that bothers people

this bug for certain bothers *us*, but our users won't benefit from 
porting this fix to stable.
I didn't find reports on this bug from other people.
I think unfortunately not many people are using mainline kernel on these 
SoCs.


>
>> ---
>>   drivers/phy/allwinner/phy-sun4i-usb.c | 44 ++++++++++++++++++++++++---
>>   1 file changed, 39 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/phy/allwinner/phy-sun4i-usb.c b/drivers/phy/allwinner/phy-sun4i-usb.c
>> index 788dd5cdbb7d..cf10e385f199 100644
>> --- a/drivers/phy/allwinner/phy-sun4i-usb.c
>> +++ b/drivers/phy/allwinner/phy-sun4i-usb.c
>> @@ -119,6 +119,7 @@ struct sun4i_usb_phy_cfg {
>>   	bool dedicated_clocks;
>>   	bool enable_pmu_unk1;
>>   	bool phy0_dual_route;
>> +	bool phy_reg_access_v2;
>>   	int missing_phys;
>>   };
>>   
>> @@ -192,13 +193,38 @@ static void sun4i_usb_phy_write(struct sun4i_usb_phy *phy, u32 addr, u32 data,
>>   				int len)
>>   {
>>   	struct sun4i_usb_phy_data *phy_data = to_sun4i_usb_phy_data(phy);
>> -	u32 temp, usbc_bit = BIT(phy->index * 2);
>> +	u32 otgctl_val, temp, usbc_bit;
>>   	void __iomem *phyctl = phy_data->base + phy_data->cfg->phyctl_offset;
>> +	void __iomem *phyctl_latch;
>>   	unsigned long flags;
>>   	int i;
>>   
>>   	spin_lock_irqsave(&phy_data->reg_lock, flags);
>>   
>> +	/* On older SoCs (prior to H3) PHY register are accessed by manipulating the
>> +	 * common register for all PHYs. PHY index is specified by pulsing usbc bit.
>> +	 * Newer SoCs leave the access procedure mostly unchanged, the difference
>> +	 * being that the latch registers are separate for each PHY.
>> +	 */
> Multi-line comments need to start with an empty line
>
>> +	if (phy_data->cfg->phy_reg_access_v2) {
>> +		if (phy->index == 0)
>> +			phyctl_latch = phy_data->base + phy_data->cfg->phyctl_offset;
>> +		else
>> +			phyctl_latch = phy->pmu + phy_data->cfg->phyctl_offset;
>> +		usbc_bit = 1;
>> +
>> +		/* Accessing USB PHY registers is only possible if phy0 is routed to musb.
>> +		 * As it's not clear whether is this related to actual PHY
>> +		 * routing or rather the hardware is just reusing the same bit,
>> +		 * don't check phy0_dual_route here.
>> +		 */
> Ditto
>
> Thanks!
> Maxime


-- 
С уважением,
     Евгений Богер / Evgeny Boger
     CTO, Wiren Board Team
     https://wirenboard.com/ru
     +7 495 150 66 19 (# 33)


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