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

[Maxime Ripard <maxime@cerno.tech>]

[-- Attachment #1.1: Type: text/plain, Size: 4970 bytes --]

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?

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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