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

[Pawel Laszczak]

>
>On 26-08-12 08:13:08, Pawel Laszczak wrote:
>>
>> Hi Peter,
>>
>> Sorry about that. Yeah, I know. I think b4 added that address
>> automatically And I didn't notice.
>
>Probably you work at old kernel?
>
>>
>> >
>> >> 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.
>
>Add Gary to confirm if it affects CIX P1 platform.
>
>> >>  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.
>>
>
>It is not so simple, it needs to co-work with wakeup irq. See cdns_wakeup_irq
>and cdns_drd_irq please. By the way, why you add above code?
>

I removed it in v2.
It turned out that the code was not needed.

Thanks,
Pawel
>--
>
>Thanks,
>Peter Chen