| Message ID | 20230814185043.9252-3-quic_eserrao@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Support dwc3 runtime suspend during bus suspend | expand |
On Mon, 14 Aug 2023 11:50:42 -0700, 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@quicinc.com> > --- > Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 5 +++++ > 1 file changed, 5 insertions(+) > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/usb/snps,dwc3.yaml: properties:snps,runtime-suspend-on-usb-suspend: 'oneOf' conditional failed, one must be fixed: 'type' is a required property hint: A vendor boolean property can use "type: boolean" /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/usb/snps,dwc3.yaml: properties:snps,runtime-suspend-on-usb-suspend: 'oneOf' conditional failed, one must be fixed: 'enum' is a required property 'const' is a required property hint: A vendor string property with exact values has an implicit type from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml# /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/usb/snps,dwc3.yaml: properties:snps,runtime-suspend-on-usb-suspend: 'oneOf' conditional failed, one must be fixed: '$ref' is a required property 'allOf' is a required property hint: A vendor property needs a $ref to types.yaml from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml# hint: Vendor specific properties must have a type and description unless they have a defined, common suffix. from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml# doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230814185043.9252-3-quic_eserrao@quicinc.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
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@quicinc.com> > --- > 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. Best regards, Krzysztof
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@quicinc.com> >> --- >> 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 Hi Thinh,Roger Please let me know your opinion on 'snps,skip-dwc3-halt-on-usb-suspend' as the quirk name. Thanks Elson
On Fri, Aug 18, 2023, 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@quicinc.com> > > > --- > > > 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 > > Hi Thinh,Roger > Please let me know your opinion on 'snps,skip-dwc3-halt-on-usb-suspend' as > the quirk name. > I don't consider this a quirk. If there's a quirk, something's broken. It's a behavior of the driver depending on the hardware capability. So, this name should reflect the capability property of the phy(s). I'm not great with naming either. Perhaps this? snps,delegate-wakeup-interrupt Thanks, Thinh
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@quicinc.com> >>> --- >>> 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. Best regards, Krzysztof
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@quicinc.com> >>>> --- >>>> 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) 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. The goal of this dt property is for the dwc3 driver to allow handling of the bus suspend interrupt when such a capability exists on a given HW platform. When this property is 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. 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 Elson
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@quicinc.com> >>>>> --- >>>>> 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. > 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. Best regards, Krzysztof
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@quicinc.com> >>>>>> --- >>>>>> 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. > > Best regards, > Krzysztof >
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@quicinc.com> > >>>>>> --- > >>>>>> 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
On 26/08/2023 03:53, Thinh Nguyen wrote: >>>> 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. That's not the purpose of bindings. Sorry. > > Regardless, what Krzysztof said is valid. Perhaps we can look into > enhancing dwc3 to maintain shared behavior based on compatible instead? Best regards, Krzysztof
On 8/26/2023 1:39 AM, Krzysztof Kozlowski wrote: > On 26/08/2023 03:53, Thinh Nguyen wrote: >>>>> 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. > > That's not the purpose of bindings. Sorry. > >> >> Regardless, what Krzysztof said is valid. Perhaps we can look into >> enhancing dwc3 to maintain shared behavior based on compatible instead? > > Thank you Krzysztof, Thinh, Roger for your comments and feedback. I will upload v5 making this change compatible-specifc. Best Regards Elson
On 8/29/2023 6:37 PM, Thinh Nguyen wrote: > Just want to clarify, there are dwc3 properties and there are dt binding > properties. Often the case that dt binding matches 1-to-1 with dwc3 > driver property. Now, we need to enhance the checkers so that the dwc3 > driver property to match cases where it is platform specific and through > compatible string. > Thank you for the clarification Thinh. To confirm, we would need to modify the driver to parse a new compatible string (say "snps,dwc3-ext-wakeup") and add .data field so that the driver is aware that this particular platform supports external wakeup detection.Right ? Regards Elson
On 30/08/2023 06:31, Elson Serrao wrote: > > > On 8/29/2023 6:37 PM, Thinh Nguyen wrote: >> Just want to clarify, there are dwc3 properties and there are dt binding >> properties. Often the case that dt binding matches 1-to-1 with dwc3 >> driver property. Now, we need to enhance the checkers so that the dwc3 >> driver property to match cases where it is platform specific and through >> compatible string. >> > > Thank you for the clarification Thinh. > To confirm, we would need to modify the driver to parse a new compatible > string (say "snps,dwc3-ext-wakeup") and add .data field so that the > driver is aware that this particular platform supports external wakeup > detection.Right ? No, it's not then platform specific. You said it depends on each platform. Platform is Qualcomm SM8450 for example. Best regards, Krzysztof
On Thu, Sep 21, 2023, Elson Serrao wrote: > > > On 8/30/2023 11:29 PM, Krzysztof Kozlowski wrote: > > On 31/08/2023 05:01, Thinh Nguyen wrote: > > > On Wed, Aug 30, 2023, Krzysztof Kozlowski wrote: > > > > On 30/08/2023 06:31, Elson Serrao wrote: > > > > > > > > > > > > > > > On 8/29/2023 6:37 PM, Thinh Nguyen wrote: > > > > > > Just want to clarify, there are dwc3 properties and there are dt binding > > > > > > properties. Often the case that dt binding matches 1-to-1 with dwc3 > > > > > > driver property. Now, we need to enhance the checkers so that the dwc3 > > > > > > driver property to match cases where it is platform specific and through > > > > > > compatible string. > > > > > > > > > > > > > > > > Thank you for the clarification Thinh. > > > > > To confirm, we would need to modify the driver to parse a new compatible > > > > > string (say "snps,dwc3-ext-wakeup") and add .data field so that the > > > > > driver is aware that this particular platform supports external wakeup > > > > > detection.Right ? > > > > > > > > No, it's not then platform specific. You said it depends on each > > > > platform. Platform is Qualcomm SM8450 for example. > > > > > > > > > > Hi Elson, > > > > > > Use the compatible string of your platform. > > > > > > e.g. > > > if (dev->of_node) { > > > struct device_node *parent = of_get_parent(dev->of_node); > > > > > > dwc->no_disconnect_on_usb_suspend = > > > of_device_is_compatible(parent, "qcom,your-compatible-string") || > > > of_device_is_compatible(parent, "some-other-platform"); > > > } > > > > > > You need to enhance dwc3_get_properties(). This may get big as dwc3 adds > > > more properties. Perhaps you can help come up with ideas to keep this > > > clean. Perhaps we can separate this out of dwc3 core.c? > > HI Thinh > > Apologies for the delayed response. > Series https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-usb/cover/1655094654-24052-1-git-send-email-quic_kriskura@quicinc.com/__;!!A4F2R9G_pg!YGlVy7No98zfEM-X5iWRhIUJ-gJEJn_gbTR4k12avzENV1TXf7cwJLZUezYzAU-rnHIbbqA1UWM0IE0R-t5SMMTJLwLZ$ > from Krishna K, introduced a dt property 'wakeup-source' which indicates a > platforms capability to handle wakeup interrupts. Based on this property, > glue drivers can inform dwc3 core that the device is wakeup capable through > device_init_wakeup(). For example dwc3-qcom driver informs it like below as > per the implementation done in the above series > > wakeup_source = of_property_read_bool(dev->of_node, "wakeup-source"); > device_init_wakeup(&pdev->dev, wakeup_source); > device_init_wakeup(&qcom->dwc3->dev, wakeup_source); > > The dwc3 core now can access this info through device_may_wakeup(dwc->dev) > while checking for bus suspend scenario to know whether the platform is > capable of detecting wakeup. > > Please let me know your thoughts on this approach. > Hi Elson, I think that it may not work for everyone. Some platforms may indicate wakeup-source but should only be applicable in selected scenarios. (e.g. Roger's platform was only intended to keep connect on suspend) Also, how will you disable it for certain platforms? Probably will need to use compatible string then too. Thanks, Thinh
>> HI Thinh >> >> Apologies for the delayed response. >> Series https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-usb/cover/1655094654-24052-1-git-send-email-quic_kriskura@quicinc.com/__;!!A4F2R9G_pg!YGlVy7No98zfEM-X5iWRhIUJ-gJEJn_gbTR4k12avzENV1TXf7cwJLZUezYzAU-rnHIbbqA1UWM0IE0R-t5SMMTJLwLZ$ >> from Krishna K, introduced a dt property 'wakeup-source' which indicates a >> platforms capability to handle wakeup interrupts. Based on this property, >> glue drivers can inform dwc3 core that the device is wakeup capable through >> device_init_wakeup(). For example dwc3-qcom driver informs it like below as >> per the implementation done in the above series >> >> wakeup_source = of_property_read_bool(dev->of_node, "wakeup-source"); >> device_init_wakeup(&pdev->dev, wakeup_source); >> device_init_wakeup(&qcom->dwc3->dev, wakeup_source); >> >> The dwc3 core now can access this info through device_may_wakeup(dwc->dev) >> while checking for bus suspend scenario to know whether the platform is >> capable of detecting wakeup. >> >> Please let me know your thoughts on this approach. >> > > Hi Elson, > > I think that it may not work for everyone. Some platforms may indicate > wakeup-source but should only be applicable in selected scenarios. > (e.g. Roger's platform was only intended to keep connect on suspend) > > Also, how will you disable it for certain platforms? Probably will need > to use compatible string then too. > Hi Thinh Thank you for your feedback. As an alternative approach, how about exposing an API from dwc3 core that glue drivers can call to enable runtime suspend during bus suspend feature ( i.e this API sets dwc->runtime_suspend_on_usb_suspend field). Only the platforms that need this feature to be enabled, can call this API after the child (dwc3 core) probe. Thanks Elson
On Tue, Oct 17, 2023, Elson Serrao wrote: > > > > > HI Thinh > > > > > > Apologies for the delayed response. > > > Series https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-usb/cover/1655094654-24052-1-git-send-email-quic_kriskura@quicinc.com/__;!!A4F2R9G_pg!YGlVy7No98zfEM-X5iWRhIUJ-gJEJn_gbTR4k12avzENV1TXf7cwJLZUezYzAU-rnHIbbqA1UWM0IE0R-t5SMMTJLwLZ$ > > > from Krishna K, introduced a dt property 'wakeup-source' which indicates a > > > platforms capability to handle wakeup interrupts. Based on this property, > > > glue drivers can inform dwc3 core that the device is wakeup capable through > > > device_init_wakeup(). For example dwc3-qcom driver informs it like below as > > > per the implementation done in the above series > > > > > > wakeup_source = of_property_read_bool(dev->of_node, "wakeup-source"); > > > device_init_wakeup(&pdev->dev, wakeup_source); > > > device_init_wakeup(&qcom->dwc3->dev, wakeup_source); > > > > > > The dwc3 core now can access this info through device_may_wakeup(dwc->dev) > > > while checking for bus suspend scenario to know whether the platform is > > > capable of detecting wakeup. > > > > > > Please let me know your thoughts on this approach. > > > > > > > Hi Elson, > > > > I think that it may not work for everyone. Some platforms may indicate > > wakeup-source but should only be applicable in selected scenarios. > > (e.g. Roger's platform was only intended to keep connect on suspend) > > > > Also, how will you disable it for certain platforms? Probably will need > > to use compatible string then too. > > > > Hi Thinh > > Thank you for your feedback. As an alternative approach, how about exposing > an API from dwc3 core that glue drivers can call to enable runtime suspend > during bus suspend feature ( i.e this API sets > dwc->runtime_suspend_on_usb_suspend field). > > Only the platforms that need this feature to be enabled, can call this API > after the child (dwc3 core) probe. > You mean after Bjorn's updates? That sounds good to me. Please make it generic so that we can set other driver properties too. Thanks, Thinh
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. + unevaluatedProperties: false required:
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@quicinc.com> --- Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 5 +++++ 1 file changed, 5 insertions(+)