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
