RE: [PATCH] usb: cdnsp: fix wakeup from S3 after controller context loss

>
>On 26-07-23 13:28:27, Pawel Laszczak via B4 Relay wrote:
>> From: Pawel Laszczak <[email protected]>
>>
>
>Hi Pawel,
>
>I have already left nxp six years ago, please change my email
>as [email protected].

Hi Peter,

Sorry about that. Yeah, I know. I think b4 added that address automatically
And I didn't notice.

>
>> CDNSP controller loses its runtime register programming across S3
>> suspend/resume.
>
>It is not always true, it depends on SoC implementation. For some
>SoCs (like CIX P1), the controller power still exists when the
>system at S3.
>
>> After resume the operational and interrupter registers
>> may contain reset values, which prevents the gadget side from recovering
>> correctly and breaks wakeup from S3.
>>
>> Fix this by reprogramming the controller state required for normal
>> operation after resume, including the command ring, DCBAA pointer,
>> doorbell base, event ring, ERST base/size and event ring dequeue
>> pointer.
>>
>> Move the basic controller register programming out of the one-time
>memory
>> initialization path and make it reusable from the resume path. Also
>> separate ring allocation from ring initialization so that rings can be
>> reinitialized without reallocating DMA memory.
>>
>> This fixes S3 resume on systems where the controller register context is
>> lost while keeping the existing DMA allocations intact.
>
>With your fix, does device mode wakeup still work, does affect the SoC
>like I mentioned CIX P1 which keeps power at S3?
>

Yes, device mode should still works correctly with this fix, and the
patch does not break platforms like CIX P1 that keep power during S3,
but I have verified this only on my FPGA platform.
We cannot test on CIX P1 directly. but the logic is sound based on
the hardware register behavior.

The resume path uses CFG_U1_PIPE_CLK_GATE_EN (bit 0 of
XEC_CFG_3XPORT_MODE_2) to detect whether the controller lost its
register context:

- On platforms where power is lost during S3 (like ours), the
  controller undergoes a Power-On Reset, which sets this bit back to
  its reset value of 1. The driver detects this and takes the full
  reinitialization path. Wakeup is not signaled in this case, as the
  USB link has been dropped.

- On platforms where power is retained during S3 (like CIX P1), this
  bit remains cleared (it was cleared by the driver as part of a
   previous fix). The driver detects bit = 0, takes the CRS (Controller
  Restore State) path, restores saved registers, and then calls
  __cdnsp_gadget_wakeup() if the link was in U3. Device mode wakeup
  works correctly in this case.

>> @@ -1575,9 +1576,13 @@ irqreturn_t cdnsp_thread_irq_handler(int irq,
>void *data)
>>  irqreturn_t cdnsp_irq_handler(int irq, void *priv)
>>  {
>>  	struct cdnsp_device *pdev = (struct cdnsp_device *)priv;
>> +	struct cdns *cdns = dev_get_drvdata(pdev->dev);
>>  	u32 irq_pending;
>>  	u32 status;
>>
>> +	if (cdns->in_lpm)
>> +		return IRQ_NONE;
>> +
>
>If the interrupt happens before cdns->in_lpm is clear, there will be no one
>handles interrupt issue.

I will change it to IRQ_HANDLED. 
The controller is stopped during suspend so no level-triggered
storm can occur. IRQ_HANDLED properly acknowledges the interrupt to
the kernel preventing it from disabling the IRQ line due to spurious
interrupt detection.

Thanks,
Pawel 

>
>--
>
>Thanks,
>Peter Chen