RE: [PATCH] usb: cdnsp: Fix issue with resuming from L1

[Pawel Laszczak]

>-----Original Message-----
>From: Pawel Laszczak
>Sent: Friday, April 11, 2025 11:40 AM
>To: gregkh@xxxxxxxxxxxxxxxxxxx
>Cc: peter.chen@xxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; linux-
>kernel@xxxxxxxxxxxxxxx; stable@xxxxxxxxxxxxxxx
>Subject: RE: [PATCH] usb: cdnsp: Fix issue with resuming from L1
>
>>
>>
>>On Thu, Apr 10, 2025 at 07:34:16AM +0000, Pawel Laszczak wrote:
>>> Subject: [PATCH] usb: cdnsp: Fix issue with resuming from L1
>>
>>Why is the subject line duplicated here?  Can you fix up your git
>>send-email process to not do that?
>>
>>> In very rare cases after resuming controller from L1 to L0 it reads
>>> registers before the clock has been enabled and as the result driver
>>> reads incorrect value.
>>> To fix this issue driver increases APB timeout value.
>>>
>>> Probably this issue occurs only on Cadence platform but fix should
>>> have no impact for other existing platforms.
>>
>>If this is the case, shouldn't you just handle this for
>>Cadence-specific hardware and add the check for that to this change?
>
>This fix will not have negative impact for other platforms, but I'm not sure
>whether other platforms are free from this issue.
>It is very hard to recreate and debug this issue.

Sorry, you have right should be platform specific. Other platform with Cadence
USB controller may require this time even longer.

>
>Thanks,
>Pawel
>>
>>>
>>> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence
>>> USBSSP DRD Driver")
>>> cc: stable@xxxxxxxxxxxxxxx
>>> Signed-off-by: Pawel Laszczak <pawell@xxxxxxxxxxx>
>>> ---
>>>  drivers/usb/cdns3/cdnsp-gadget.c | 22 ++++++++++++++++++++++
>>> drivers/usb/cdns3/cdnsp-gadget.h |  4 ++++
>>>  2 files changed, 26 insertions(+)
>>>
>>> diff --git a/drivers/usb/cdns3/cdnsp-gadget.c
>>> b/drivers/usb/cdns3/cdnsp-gadget.c
>>> index 87f310841735..b12581b94567 100644
>>> --- a/drivers/usb/cdns3/cdnsp-gadget.c
>>> +++ b/drivers/usb/cdns3/cdnsp-gadget.c
>>> @@ -139,6 +139,21 @@ static void cdnsp_clear_port_change_bit(struct
>>cdnsp_device *pdev,
>>>  	       (portsc & PORT_CHANGE_BITS), port_regs);  }
>>>
>>> +static void cdnsp_set_apb_timeout_value(struct cdnsp_device *pdev) {
>>> +	__le32 __iomem *reg;
>>> +	void __iomem *base;
>>> +	u32 offset = 0;
>>> +	u32 val;
>>> +
>>> +	base = &pdev->cap_regs->hc_capbase;
>>> +	offset = cdnsp_find_next_ext_cap(base, offset, D_XEC_PRE_REGS_CAP);
>>> +	reg = base + offset + REG_CHICKEN_BITS_3_OFFSET;
>>> +
>>> +	val  = le32_to_cpu(readl(reg));
>>> +	writel(cpu_to_le32(CHICKEN_APB_TIMEOUT_SET(val)), reg);
>>
>>Do you need to do a read to ensure that the write is flushed to the
>>device before continuing?
>>
>>> +}
>>> +
>>>  static void cdnsp_set_chicken_bits_2(struct cdnsp_device *pdev, u32
>>> bit)  {
>>>  	__le32 __iomem *reg;
>>> @@ -1798,6 +1813,13 @@ static int cdnsp_gen_setup(struct cdnsp_device
>>*pdev)
>>>  	pdev->hci_version = HC_VERSION(pdev->hcc_params);
>>>  	pdev->hcc_params = readl(&pdev->cap_regs->hcc_params);
>>>
>>> +	/* In very rare cases after resuming controller from L1 to L0 it reads
>>> +	 * registers before the clock has been enabled and as the result driver
>>> +	 * reads incorrect value.
>>> +	 * To fix this issue driver increases APB timeout value.
>>> +	 */
>>
>>Nit, please use the "normal" kernel comment style.
>>
>>thanks,
>>
>>greg k-h