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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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