| Message ID | 20220804151001.23612-8-johan+linaro@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 416b61893860d45484d4717599c828411cee9c7f |
| Headers | show |
| Series | usb: dwc3: qcom: fix wakeup implementation | expand |
On Thu, Aug 04, 2022 at 05:09:59PM +0200, Johan Hovold wrote: > Add a wakeup-source property to the binding to describe whether the > wakeup interrupts can wake the system from suspend. > > Acked-by: Rob Herring <robh@kernel.org> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> So this is based on the fact that Qcom glue wrapper is supplying the wakeup interrupts. But isn't it possible that on other platform, the DWC IP can supply wakeup interrupts? In the driver, the wakeup-source parsing has been moved to the Qcom glue driver. But this contradicts with the binding. Thanks, Mani > --- > > Changes in v2 > - disallow 'wakeup-source' in child node (Krzysztof) > > Documentation/devicetree/bindings/usb/qcom,dwc3.yaml | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml > index fea3e7092ace..d5959bdea63e 100644 > --- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml > +++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml > @@ -108,12 +108,17 @@ properties: > HS/FS/LS modes are supported. > type: boolean > > + wakeup-source: true > + > # Required child node: > > patternProperties: > "^usb@[0-9a-f]+$": > $ref: snps,dwc3.yaml# > > + properties: > + wakeup-source: false > + > required: > - compatible > - reg > -- > 2.35.1 >
On Sat, Aug 06, 2022 at 08:38:48PM +0530, Manivannan Sadhasivam wrote: > On Thu, Aug 04, 2022 at 05:09:59PM +0200, Johan Hovold wrote: > > Add a wakeup-source property to the binding to describe whether the > > wakeup interrupts can wake the system from suspend. > > > > Acked-by: Rob Herring <robh@kernel.org> > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > > So this is based on the fact that Qcom glue wrapper is supplying the wakeup > interrupts. But isn't it possible that on other platform, the DWC IP can supply > wakeup interrupts? Yeah, possibly, and that's why Rob suggested keeping the 'wakeup-source' property also in the core node. > In the driver, the wakeup-source parsing has been moved to the Qcom glue driver. > But this contradicts with the binding. That's irrelevant. The core driver does not implement wakeup support. It was just added as a hack for the Qualcomm driver, and you won't get wakeup-capability for other platforms by just parsing the property in the core driver. When/if wakeup support for such a platform is added, then the core driver may need to look at the property again. Johan
On Sat, Aug 06, 2022 at 06:41:37PM +0200, Johan Hovold wrote: > On Sat, Aug 06, 2022 at 08:38:48PM +0530, Manivannan Sadhasivam wrote: > > On Thu, Aug 04, 2022 at 05:09:59PM +0200, Johan Hovold wrote: > > > Add a wakeup-source property to the binding to describe whether the > > > wakeup interrupts can wake the system from suspend. > > > > > > Acked-by: Rob Herring <robh@kernel.org> > > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > > > > So this is based on the fact that Qcom glue wrapper is supplying the wakeup > > interrupts. But isn't it possible that on other platform, the DWC IP can supply > > wakeup interrupts? > > Yeah, possibly, and that's why Rob suggested keeping the 'wakeup-source' > property also in the core node. > > > In the driver, the wakeup-source parsing has been moved to the Qcom glue driver. > > But this contradicts with the binding. > > That's irrelevant. The core driver does not implement wakeup support. It > was just added as a hack for the Qualcomm driver, and you won't get > wakeup-capability for other platforms by just parsing the property in > the core driver. > > When/if wakeup support for such a platform is added, then the core > driver may need to look at the property again. > My point is, the platform drivers are free to add "wakeup-source" property in the DWC node. Then in that case, the DWC driver should handle the capability, isn't it? I know it is broken currently, but moving the wakeup parsing code is not helping either. And... I'm aware of the fact that the binding should describe the hardware and not the limitation of the driver. So perhaps we should document it in the driver as a TODO or something? Thanks, Mani > Johan
On Sat, Aug 06, 2022 at 10:22:38PM +0530, Manivannan Sadhasivam wrote: > On Sat, Aug 06, 2022 at 06:41:37PM +0200, Johan Hovold wrote: > > On Sat, Aug 06, 2022 at 08:38:48PM +0530, Manivannan Sadhasivam wrote: > > > On Thu, Aug 04, 2022 at 05:09:59PM +0200, Johan Hovold wrote: > > > > Add a wakeup-source property to the binding to describe whether the > > > > wakeup interrupts can wake the system from suspend. > > > > > > > > Acked-by: Rob Herring <robh@kernel.org> > > > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > > > > > > So this is based on the fact that Qcom glue wrapper is supplying the wakeup > > > interrupts. But isn't it possible that on other platform, the DWC IP can supply > > > wakeup interrupts? > > > > Yeah, possibly, and that's why Rob suggested keeping the 'wakeup-source' > > property also in the core node. > > > > > In the driver, the wakeup-source parsing has been moved to the Qcom glue driver. > > > But this contradicts with the binding. > > > > That's irrelevant. The core driver does not implement wakeup support. It > > was just added as a hack for the Qualcomm driver, and you won't get > > wakeup-capability for other platforms by just parsing the property in > > the core driver. > > > > When/if wakeup support for such a platform is added, then the core > > driver may need to look at the property again. > > > > My point is, the platform drivers are free to add "wakeup-source" property in > the DWC node. Then in that case, the DWC driver should handle the capability, > isn't it? No, not really. They wouldn't violate the current binding, but it would arguably still be wrong to do so unless that platform actually supports wakeup without involvement from a glue layer. Perhaps we should reconsider reverting the binding update adding this property to the core node and only add it selectively for the platforms for which is actually applies (if they even exist). > I know it is broken currently, but moving the wakeup parsing code is not > helping either. It's not even broken. It has never even been implemented. Just because someone added a hack that should probably never have been merged in the first place, doesn't mean we should somehow pretend that we support it. > And... I'm aware of the fact that the binding should describe the hardware and > not the limitation of the driver. So perhaps we should document it in the > driver as a TODO or something? I'd rather just revert the binding update to avoid having discussions like this. We don't even know if it's possible to support on any platform yet (and remember that none of this has even been in an rc release yet). Johan
On Sat, Aug 06, 2022 at 07:09:44PM +0200, Johan Hovold wrote: > On Sat, Aug 06, 2022 at 10:22:38PM +0530, Manivannan Sadhasivam wrote: > > On Sat, Aug 06, 2022 at 06:41:37PM +0200, Johan Hovold wrote: > > > On Sat, Aug 06, 2022 at 08:38:48PM +0530, Manivannan Sadhasivam wrote: > > > > On Thu, Aug 04, 2022 at 05:09:59PM +0200, Johan Hovold wrote: > > > > > Add a wakeup-source property to the binding to describe whether the > > > > > wakeup interrupts can wake the system from suspend. > > > > > > > > > > Acked-by: Rob Herring <robh@kernel.org> > > > > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > > > > > > > > So this is based on the fact that Qcom glue wrapper is supplying the wakeup > > > > interrupts. But isn't it possible that on other platform, the DWC IP can supply > > > > wakeup interrupts? > > > > > > Yeah, possibly, and that's why Rob suggested keeping the 'wakeup-source' > > > property also in the core node. > > > > > > > In the driver, the wakeup-source parsing has been moved to the Qcom glue driver. > > > > But this contradicts with the binding. > > > > > > That's irrelevant. The core driver does not implement wakeup support. It > > > was just added as a hack for the Qualcomm driver, and you won't get > > > wakeup-capability for other platforms by just parsing the property in > > > the core driver. > > > > > > When/if wakeup support for such a platform is added, then the core > > > driver may need to look at the property again. > > > > > > > My point is, the platform drivers are free to add "wakeup-source" property in > > the DWC node. Then in that case, the DWC driver should handle the capability, > > isn't it? > > No, not really. They wouldn't violate the current binding, but it would > arguably still be wrong to do so unless that platform actually supports > wakeup without involvement from a glue layer. > > Perhaps we should reconsider reverting the binding update adding this > property to the core node and only add it selectively for the platforms > for which is actually applies (if they even exist). > That sounds right to me. > > I know it is broken currently, but moving the wakeup parsing code is not > > helping either. > > It's not even broken. It has never even been implemented. > > Just because someone added a hack that should probably never have been > merged in the first place, doesn't mean we should somehow pretend that > we support it. > > > And... I'm aware of the fact that the binding should describe the hardware and > > not the limitation of the driver. So perhaps we should document it in the > > driver as a TODO or something? > > I'd rather just revert the binding update to avoid having discussions > like this. We don't even know if it's possible to support on any > platform yet (and remember that none of this has even been in an rc > release yet). > Okay. Thanks, Mani > Johan
diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml index fea3e7092ace..d5959bdea63e 100644 --- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml +++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml @@ -108,12 +108,17 @@ properties: HS/FS/LS modes are supported. type: boolean + wakeup-source: true + # Required child node: patternProperties: "^usb@[0-9a-f]+$": $ref: snps,dwc3.yaml# + properties: + wakeup-source: false + required: - compatible - reg