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

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?

-- 

Thanks,
Peter Chen