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

[Pawel Laszczak]

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

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