Re: [PATCH v4 2/3] dt-bindings: usb: snps,dwc3: Add runtime-suspend-on-usb-suspend property

[Thinh Nguyen]

On Wed, Aug 23, 2023, Roger Quadros wrote:
> 
> 
> On 23/08/2023 09:34, Krzysztof Kozlowski wrote:
> > On 23/08/2023 01:58, Elson Serrao wrote:
> >>
> >>
> >> On 8/19/2023 2:35 AM, Krzysztof Kozlowski wrote:
> >>> On 18/08/2023 21:16, Elson Serrao wrote:
> >>>>
> >>>>
> >>>> On 8/15/2023 10:44 PM, Krzysztof Kozlowski wrote:
> >>>>> On 14/08/2023 20:50, Elson Roy Serrao wrote:
> >>>>>> This property allows dwc3 runtime suspend when bus suspend interrupt
> >>>>>> is received even with cable connected. This would allow the dwc3
> >>>>>> controller to enter low power mode during bus suspend scenario.
> >>>>>>
> >>>>>> This property would particularly benefit dwc3 IPs where hibernation is
> >>>>>> not enabled and the dwc3 low power mode entry/exit is handled by the
> >>>>>> glue driver. The assumption here is that the platform using this dt
> >>>>>> property is capable of detecting resume events to bring the controller
> >>>>>> out of suspend.
> >>>>>>
> >>>>>> Signed-off-by: Elson Roy Serrao <quic_eserrao@xxxxxxxxxxx>
> >>>>>> ---
> >>>>>>    Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 5 +++++
> >>>>>>    1 file changed, 5 insertions(+)
> >>>>>>
> >>>>>> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> >>>>>> index a696f23730d3..e19a60d06d2b 100644
> >>>>>> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> >>>>>> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> >>>>>> @@ -403,6 +403,11 @@ properties:
> >>>>>>        description:
> >>>>>>          Enable USB remote wakeup.
> >>>>>>    
> >>>>>> +  snps,runtime-suspend-on-usb-suspend:
> >>>>>> +    description:
> >>>>>> +      If True then dwc3 runtime suspend is allowed during bus suspend
> >>>>>> +      case even with the USB cable connected.
> >>>>>
> >>>>> This was no tested... but anyway, this is no a DT property but OS
> >>>>> policy. There is no such thing as "runtime suspend" in the hardware,
> >>>>> because you describe one particular OS.
> >>>>>
> >>>>> Sorry, no a DT property, drop the change entirely.
> >>>>>
> >>>>>
> >>>> Hi Krzysztof
> >>>>
> >>>> Sorry my local dt checker had some issue and it did not catch these
> >>>> errors. I have rectified it now.
> >>>>
> >>>> This dt property is mainly for skipping dwc3 controller halt when a USB
> >>>> suspend interrupt is received with usb cable connected, so that we dont
> >>>> trigger a DISCONNECT event. Perhaps a better name would reflect the true
> >>>> usage of this?
> >>>>
> >>>> Something like snps,skip-dwc3-halt-on-usb-suspend. dwc3 cores where
> >>>> hibernation feature is not enabled/supported can use this property
> >>>
> >>> So this is specific to DWC3 core, thus should be just implied by compatible.
> >>>
> >>
> >> Hi Krzysztof
> >>
> >> Apologies for not being clear. Below is the reasoning behind this dt entry.
> >>
> >> When bus suspend interrupt is received and if usb cable is connected, 
> >> dwc3 driver does not suspend. The aim of this series is to handle this 
> >> interrupt when USB cable is connected to achieve power savings. OEMs 
> >> might have their own implementation in their glue driver to turn off 
> >> clocks and other resources when USB is not in use, thus saving power. 
> >> But since glue layer has dependency on dwc3 driver (parent-child 
> >> relationship) we need to allow dwc3 driver to NOT ignore the bus suspend 
> >> interrupt and let the dwc3 driver suspend (so that glue driver can 
> >> suspend as well)
> > 
> > All this describes current OS implementation so has nothing to do with
> > bindings.
> > 
> >>
> >> Now it is the responsibility of glue driver to detect USB wakeup signal 
> >> from the host during resume (since dwc3 driver is suspended at this 
> >> point and cannot handle interrupts). Every OEM may not have the 
> >> capability to detect wakeup signal. 
> > 
> > Again, driver architecture.
> 
> This is not driver architecture but SoC (hardware) architecture.
> Most SoCs have some kind of Wrapper h/w logic around the USB h/w module
> where they implement such logic like power/clocking/wake-up handling etc.
> So this information (whether wake-up is supported or not)
> should be already known to the respective Wrapper driver.
> 
> Now the missing part is how to convey this information to the USB (DWC3)
> driver so it behaves in the correct way.
> 
> > 
> >> The goal of this dt property is for 
> >> the dwc3 driver to allow handling of the bus suspend interrupt when such 
> > 
> > DT properties are not "for the drivers".
> > 
> >> a capability exists on a given HW platform. When this property is
> > 
> > Each platform has this capability. If not, then it is
> > compatible-related, as I said before which you did not address.
> > 
> > 
> >> defined dwc3 driver knows that the low power mode entry/exit is 
> >> controlled by the glue driver and thus it can allow the suspend 
> >> operation when bus suspend interrupt is received.
> >>
> >> For example on Qualcomm platforms there is a phy sideband signalling 
> >> which detects the wakeup signal when resume is initiated by the host.
> > 
> > So compatible-specific.
> > 
> >> Thus qcom platforms can benefit from this feature by defining this dt 
> >> property. (in a parallel discussion with Thinh N to come up with a 
> >> better name for this dt entry).
> > 
> > Thanks, with quite a long message you at the end admitted this is
> > compatible-specific. Exactly what I wrote it one sentence previously.
> > 

Various dwc3 platforms often share a common capability that can be
shared with a common dt property. If we dedicate a property such as in
this case, it helps designers enable a certain feature without updating
the driver every time a new platform is introduced. It also helps keep
the driver a bit simpler on the compatible checks.

Regardless, what Krzysztof said is valid. Perhaps we can look into
enhancing dwc3 to maintain shared behavior based on compatible instead?

Thanks,
Thinh