[v2,7/9] dt-bindings: usb: qcom,dwc3: add wakeup-source property

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>
---

Changes in v2
 - disallow 'wakeup-source' in child node (Krzysztof)

 Documentation/devicetree/bindings/usb/qcom,dwc3.yaml | 5 +++++
 1 file changed, 5 insertions(+)

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 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