diff mbox series

[v2,5/5] usb: dwc3-am62: add workaround for Errata i2409

Message ID 20240205141221.56076-6-rogerq@kernel.org
State New
Headers
Series usb: dwc3-am62: module removal and errata fixes |

Commit Message

Roger Quadros Feb. 5, 2024, 2:12 p.m. UTC 
  All AM62 devices have Errata i2409 [1] due to which
USB2 PHY may lock up due to short suspend.

Workaround involves setting bit 5 and 4 PLL_REG12
in PHY2 register space after USB controller is brought
out of LPSC reset but before controller initialization.

Handle this workaround.

[1] - https://www.ti.com/lit/er/sprz487d/sprz487d.pdf

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
Cc: Conor Dooley <conor+dt@kernel.org>
Signed-off-by: Roger Quadros <rogerq@kernel.org>
---

Notes:
    Changelog:
    
    v2:
    - don't add phy read/write helpers or add phy to private data
    
    v1: https://lore.kernel.org/all/20240201121220.5523-5-rogerq@kernel.org/

 drivers/usb/dwc3/dwc3-am62.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

Comments

Andrew Davis Feb. 5, 2024, 6:07 p.m. UTC | #1 
On 2/5/24 8:12 AM, Roger Quadros wrote:
> All AM62 devices have Errata i2409 [1] due to which
> USB2 PHY may lock up due to short suspend.
> 
> Workaround involves setting bit 5 and 4 PLL_REG12
> in PHY2 register space after USB controller is brought
> out of LPSC reset but before controller initialization.
> 
> Handle this workaround.
> 
> [1] - https://www.ti.com/lit/er/sprz487d/sprz487d.pdf
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> Cc: Conor Dooley <conor+dt@kernel.org>
> Signed-off-by: Roger Quadros <rogerq@kernel.org>
> ---
> 
> Notes:
>      Changelog:
>      
>      v2:
>      - don't add phy read/write helpers or add phy to private data
>      
>      v1: https://lore.kernel.org/all/20240201121220.5523-5-rogerq@kernel.org/
> 
>   drivers/usb/dwc3/dwc3-am62.c | 21 ++++++++++++++++++++-
>   1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-am62.c b/drivers/usb/dwc3/dwc3-am62.c
> index af1ce934e7fb..5ae5c3087b0f 100644
> --- a/drivers/usb/dwc3/dwc3-am62.c
> +++ b/drivers/usb/dwc3/dwc3-am62.c
> @@ -101,6 +101,11 @@
>   #define PHY_CORE_VOLTAGE_MASK	BIT(31)
>   #define PHY_PLL_REFCLK_MASK	GENMASK(3, 0)
>   
> +/* USB PHY2 register offsets */
> +#define	USB_PHY_PLL_REG12		0x130
> +#define	USB_PHY_PLL_LDO_REF_EN		BIT(5)
> +#define	USB_PHY_PLL_LDO_REF_EN_EN	BIT(4)
> +
>   #define DWC3_AM62_AUTOSUSPEND_DELAY	100
>   
>   struct dwc3_am62 {
> @@ -184,8 +189,9 @@ static int dwc3_ti_probe(struct platform_device *pdev)
>   	struct device *dev = &pdev->dev;
>   	struct device_node *node = pdev->dev.of_node;
>   	struct dwc3_am62 *am62;
> -	int i, ret;
>   	unsigned long rate;
> +	void __iomem *phy;
> +	int i, ret;
>   	u32 reg;
>   
>   	am62 = devm_kzalloc(dev, sizeof(*am62), GFP_KERNEL);
> @@ -201,6 +207,12 @@ static int dwc3_ti_probe(struct platform_device *pdev)
>   		return PTR_ERR(am62->usbss);
>   	}
>   
> +	phy = devm_platform_ioremap_resource(pdev, 1);
> +	if (IS_ERR(phy)) {
> +		dev_err(dev, "can't map PHY IOMEM resource. Won't apply i2409 fix.\n");
> +		phy = NULL;
> +	}

Why not move this down to where you use it below, then just have
it be an if/else with the work around in the if (!IS_ERR(phy))
and the warning in the else.

Andrew

> +
>   	am62->usb2_refclk = devm_clk_get(dev, "ref");
>   	if (IS_ERR(am62->usb2_refclk)) {
>   		dev_err(dev, "can't get usb2_refclk\n");
> @@ -227,6 +239,13 @@ static int dwc3_ti_probe(struct platform_device *pdev)
>   	if (ret)
>   		return ret;
>   
> +	/* Workaround Errata i2409 */
> +	if (phy) {
> +		reg = readl(phy + USB_PHY_PLL_REG12);
> +		reg |= USB_PHY_PLL_LDO_REF_EN | USB_PHY_PLL_LDO_REF_EN_EN;
> +		writel(reg, phy + USB_PHY_PLL_REG12);
> +	}
> +
>   	/* VBUS divider select */
>   	am62->vbus_divider = device_property_read_bool(dev, "ti,vbus-divider");
>   	reg = dwc3_ti_readl(am62, USBSS_PHY_CONFIG);
Roger Quadros Feb. 8, 2024, 12:20 p.m. UTC | #2 
On 05/02/2024 20:07, Andrew Davis wrote:
> On 2/5/24 8:12 AM, Roger Quadros wrote:
>> All AM62 devices have Errata i2409 [1] due to which
>> USB2 PHY may lock up due to short suspend.
>>
>> Workaround involves setting bit 5 and 4 PLL_REG12
>> in PHY2 register space after USB controller is brought
>> out of LPSC reset but before controller initialization.
>>
>> Handle this workaround.
>>
>> [1] - https://www.ti.com/lit/er/sprz487d/sprz487d.pdf
>>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
>> Cc: Conor Dooley <conor+dt@kernel.org>
>> Signed-off-by: Roger Quadros <rogerq@kernel.org>
>> ---
>>
>> Notes:
>>      Changelog:
>>           v2:
>>      - don't add phy read/write helpers or add phy to private data
>>           v1: https://lore.kernel.org/all/20240201121220.5523-5-rogerq@kernel.org/
>>
>>   drivers/usb/dwc3/dwc3-am62.c | 21 ++++++++++++++++++++-
>>   1 file changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/dwc3/dwc3-am62.c b/drivers/usb/dwc3/dwc3-am62.c
>> index af1ce934e7fb..5ae5c3087b0f 100644
>> --- a/drivers/usb/dwc3/dwc3-am62.c
>> +++ b/drivers/usb/dwc3/dwc3-am62.c
>> @@ -101,6 +101,11 @@
>>   #define PHY_CORE_VOLTAGE_MASK    BIT(31)
>>   #define PHY_PLL_REFCLK_MASK    GENMASK(3, 0)
>>   +/* USB PHY2 register offsets */
>> +#define    USB_PHY_PLL_REG12        0x130
>> +#define    USB_PHY_PLL_LDO_REF_EN        BIT(5)
>> +#define    USB_PHY_PLL_LDO_REF_EN_EN    BIT(4)
>> +
>>   #define DWC3_AM62_AUTOSUSPEND_DELAY    100
>>     struct dwc3_am62 {
>> @@ -184,8 +189,9 @@ static int dwc3_ti_probe(struct platform_device *pdev)
>>       struct device *dev = &pdev->dev;
>>       struct device_node *node = pdev->dev.of_node;
>>       struct dwc3_am62 *am62;
>> -    int i, ret;
>>       unsigned long rate;
>> +    void __iomem *phy;
>> +    int i, ret;
>>       u32 reg;
>>         am62 = devm_kzalloc(dev, sizeof(*am62), GFP_KERNEL);
>> @@ -201,6 +207,12 @@ static int dwc3_ti_probe(struct platform_device *pdev)
>>           return PTR_ERR(am62->usbss);
>>       }
>>   +    phy = devm_platform_ioremap_resource(pdev, 1);
>> +    if (IS_ERR(phy)) {
>> +        dev_err(dev, "can't map PHY IOMEM resource. Won't apply i2409 fix.\n");
>> +        phy = NULL;
>> +    }
> 
> Why not move this down to where you use it below, then just have
> it be an if/else with the work around in the if (!IS_ERR(phy))
> and the warning in the else.

Seems reasonable. Will fix.

> 
> Andrew
> 
>> +
>>       am62->usb2_refclk = devm_clk_get(dev, "ref");
>>       if (IS_ERR(am62->usb2_refclk)) {
>>           dev_err(dev, "can't get usb2_refclk\n");
>> @@ -227,6 +239,13 @@ static int dwc3_ti_probe(struct platform_device *pdev)
>>       if (ret)
>>           return ret;
>>   +    /* Workaround Errata i2409 */
>> +    if (phy) {
>> +        reg = readl(phy + USB_PHY_PLL_REG12);
>> +        reg |= USB_PHY_PLL_LDO_REF_EN | USB_PHY_PLL_LDO_REF_EN_EN;
>> +        writel(reg, phy + USB_PHY_PLL_REG12);
>> +    }
>> +
>>       /* VBUS divider select */
>>       am62->vbus_divider = device_property_read_bool(dev, "ti,vbus-divider");
>>       reg = dwc3_ti_readl(am62, USBSS_PHY_CONFIG);
Francesco Dolcini Feb. 11, 2024, 4:18 p.m. UTC | #3 
On Mon, Feb 05, 2024 at 04:12:21PM +0200, Roger Quadros wrote:
> All AM62 devices have Errata i2409 [1] due to which
> USB2 PHY may lock up due to short suspend.

Is there any visible log trace when we have this phy lock up situation?
Eventually it would be nice to have this in the commit message.

Francesco
Roger Quadros Feb. 14, 2024, 9:40 a.m. UTC | #4 
On 11/02/2024 18:18, Francesco Dolcini wrote:
> On Mon, Feb 05, 2024 at 04:12:21PM +0200, Roger Quadros wrote:
>> All AM62 devices have Errata i2409 [1] due to which
>> USB2 PHY may lock up due to short suspend.
> 
> Is there any visible log trace when we have this phy lock up situation?
> Eventually it would be nice to have this in the commit message.
> 

I have not been able to reproduce this issue here so no log trace.
diff mbox series

Patch

diff --git a/drivers/usb/dwc3/dwc3-am62.c b/drivers/usb/dwc3/dwc3-am62.c
index af1ce934e7fb..5ae5c3087b0f 100644
--- a/drivers/usb/dwc3/dwc3-am62.c
+++ b/drivers/usb/dwc3/dwc3-am62.c
@@ -101,6 +101,11 @@ 
 #define PHY_CORE_VOLTAGE_MASK	BIT(31)
 #define PHY_PLL_REFCLK_MASK	GENMASK(3, 0)
 
+/* USB PHY2 register offsets */
+#define	USB_PHY_PLL_REG12		0x130
+#define	USB_PHY_PLL_LDO_REF_EN		BIT(5)
+#define	USB_PHY_PLL_LDO_REF_EN_EN	BIT(4)
+
 #define DWC3_AM62_AUTOSUSPEND_DELAY	100
 
 struct dwc3_am62 {
@@ -184,8 +189,9 @@  static int dwc3_ti_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct device_node *node = pdev->dev.of_node;
 	struct dwc3_am62 *am62;
-	int i, ret;
 	unsigned long rate;
+	void __iomem *phy;
+	int i, ret;
 	u32 reg;
 
 	am62 = devm_kzalloc(dev, sizeof(*am62), GFP_KERNEL);
@@ -201,6 +207,12 @@  static int dwc3_ti_probe(struct platform_device *pdev)
 		return PTR_ERR(am62->usbss);
 	}
 
+	phy = devm_platform_ioremap_resource(pdev, 1);
+	if (IS_ERR(phy)) {
+		dev_err(dev, "can't map PHY IOMEM resource. Won't apply i2409 fix.\n");
+		phy = NULL;
+	}
+
 	am62->usb2_refclk = devm_clk_get(dev, "ref");
 	if (IS_ERR(am62->usb2_refclk)) {
 		dev_err(dev, "can't get usb2_refclk\n");
@@ -227,6 +239,13 @@  static int dwc3_ti_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	/* Workaround Errata i2409 */
+	if (phy) {
+		reg = readl(phy + USB_PHY_PLL_REG12);
+		reg |= USB_PHY_PLL_LDO_REF_EN | USB_PHY_PLL_LDO_REF_EN_EN;
+		writel(reg, phy + USB_PHY_PLL_REG12);
+	}
+
 	/* VBUS divider select */
 	am62->vbus_divider = device_property_read_bool(dev, "ti,vbus-divider");
 	reg = dwc3_ti_readl(am62, USBSS_PHY_CONFIG);