Re: [PATCH v6 2/5] usb: cdns3-ti: run HW init at resume() if HW was reset

[Roger Quadros <rogerq@kernel.org>]

On 13/12/2024 17:28, Théo Lebrun wrote:
> On Thu Dec 12, 2024 at 1:18 PM CET, Roger Quadros wrote:
>> On 10/12/2024 19:13, Théo Lebrun wrote:
>>> At runtime_resume(), read the W1 (Wrapper Register 1) register to detect
>>> if an hardware reset occurred. If it did, run the hardware init sequence.
>>>
>>> This callback will be called at system-wide resume. Previously, if a
>>> reset occurred during suspend, we would crash. The wrapper config had
>>> not been written, leading to invalid register accesses inside cdns3.
>>>
>>
>> Did I understand right that the Controller reset can happen only at
>> system suspend and never at runtime suspend?
> 
> J7200 + upstream kernel => if the power domain is cut off (it is
> implicitly at runtime PM) then it resets. This is 100% board dependent.
> We just never let it go into runtime suspend, for the moment.
> 
>> If so do you really need the runtime suspend/resume hooks?
>> you should have different system suspend/resume hooks than runtime suspend/resume
>> hooks and deal with the re-initialization in system resume hook.
> 
> The patch series works in the current setup with the wrapper that is
> never shut off. But it would also work if someone decides to use RPM on
> the wrapper.

But will it work? if we let it runtime suspend and controller looses power,
even though we restore the wrapper, who is restoring XHCI controller on
runtime resume?

Also we would be interested in wakeup events at runtime suspend
and by loosing power it doesn't look like it will ever wake up with any USB
event.

> 
> Overall, the current kernel-wide strategy is to minimise
> suspend/resume-specific code. Having only the concept of "runtime PM"
> and triggering that at system-wide suspend/resume is easier to reason
> about. It unifies concepts and reduces the states a device can be in.

or we just focus on system suspend/resume if the driver can't do
meaningful runtime suspend/resume?

> 
> We could even imagine a future where ->suspend|resume() callbacks
> are pm_runtime_force_suspend|resume() by default.
> That'd be the dream, at least.
> 
> Thanks,
> 
> --
> Théo Lebrun, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> 

-- 
cheers,
-roger