[PATCH v2 0/4] usb: dwc3: Introduce refclk lpm

[PATCH v2 0/4] usb: dwc3: Introduce refclk lpm

[Thinh Nguyen]

This patch series introduce a new feature in DWC_usb31 with more
aggressive low power management using reference clock.

Changes in v2:
- Revise commit messages to correctly describe the feature
- Split previous patch "usb: dwc3: Add reference clock properties" to 2
  separate patches
- Remove reference clock period validation in since DWC_usb3
  can support more periods than DWC_usb31
- Rename property snps,enable-refclk-lpm to snps,enable-refclk-sof


Thinh Nguyen (4):
  usb: dwc3: Add property snps,refclk-period-ns
  usb: dwc3: Set the reference clock period
  usb: dwc3: Add property snps,enable-refclk-sof
  usb: dwc3: Enable frame number tracking based on reference clock

 Documentation/devicetree/bindings/usb/dwc3.txt |  5 +++
 drivers/usb/dwc3/core.c                        | 60 ++++++++++++++++++++++++++
 drivers/usb/dwc3/core.h                        | 16 +++++++
 3 files changed, 81 insertions(+)

-- 
2.11.0

[PATCH v2 1/4] usb: dwc3: Add property snps,refclk-period-ns

[Thinh Nguyen]

This patch introduces property "snps,refclk-period-ns" to inform the
controller of the reference clock period. If the reference clock period
is different from the default Core Consultant setting, then this
property can be set to the reference clock period.

This property does not control the reference clock rate. The controller
uses this value to perform internal timing calculations that are based
on the reference clock.

Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
Changes in v2:
- Split from "usb: dwc3: Add reference clock properties"
- Revise commit message and property description

 Documentation/devicetree/bindings/usb/dwc3.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
index 8e5265e9f658..b7e67edff9b2 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -99,6 +99,8 @@ Optional properties:
 			this and tx-thr-num-pkt-prd to a valid, non-zero value
 			1-16 (DWC_usb31 programming guide section 1.2.3) to
 			enable periodic ESS TX threshold.
+ - snps,refclk-period-ns: if set, this value informs the controller of the
+			reference clock period in nanoseconds.
 
  - <DEPRECATED> tx-fifo-resize: determines if the FIFO *has* to be reallocated.
  - snps,incr-burst-type-adjustment: Value for INCR burst type of GSBUSCFG0
-- 
2.11.0

Re: [PATCH v2 1/4] usb: dwc3: Add property snps,refclk-period-ns

[Rob Herring]

On Fri, Dec 07, 2018 at 06:27:30PM -0800, Thinh Nguyen wrote:
> This patch introduces property "snps,refclk-period-ns" to inform the
> controller of the reference clock period. If the reference clock period
> is different from the default Core Consultant setting, then this
> property can be set to the reference clock period.
> 
> This property does not control the reference clock rate. The controller
> uses this value to perform internal timing calculations that are based
> on the reference clock.
> 
> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
> ---
> Changes in v2:
> - Split from "usb: dwc3: Add reference clock properties"
> - Revise commit message and property description
> 
>  Documentation/devicetree/bindings/usb/dwc3.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> index 8e5265e9f658..b7e67edff9b2 100644
> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> @@ -99,6 +99,8 @@ Optional properties:
>  			this and tx-thr-num-pkt-prd to a valid, non-zero value
>  			1-16 (DWC_usb31 programming guide section 1.2.3) to
>  			enable periodic ESS TX threshold.
> + - snps,refclk-period-ns: if set, this value informs the controller of the
> +			reference clock period in nanoseconds.

Shouldn't you be able to retrieve the refclk frequency and then 
calculate the period?

Rob

Re: [PATCH v2 1/4] usb: dwc3: Add property snps,refclk-period-ns

[Thinh Nguyen]

Hi Rob,

On 12/18/2018 8:39 AM, Rob Herring wrote:
> On Fri, Dec 07, 2018 at 06:27:30PM -0800, Thinh Nguyen wrote:
>> This patch introduces property "snps,refclk-period-ns" to inform the
>> controller of the reference clock period. If the reference clock period
>> is different from the default Core Consultant setting, then this
>> property can be set to the reference clock period.
>>
>> This property does not control the reference clock rate. The controller
>> uses this value to perform internal timing calculations that are based
>> on the reference clock.
>>
>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
>> ---
>> Changes in v2:
>> - Split from "usb: dwc3: Add reference clock properties"
>> - Revise commit message and property description
>>
>>  Documentation/devicetree/bindings/usb/dwc3.txt | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>> index 8e5265e9f658..b7e67edff9b2 100644
>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>> @@ -99,6 +99,8 @@ Optional properties:
>>  			this and tx-thr-num-pkt-prd to a valid, non-zero value
>>  			1-16 (DWC_usb31 programming guide section 1.2.3) to
>>  			enable periodic ESS TX threshold.
>> + - snps,refclk-period-ns: if set, this value informs the controller of the
>> +			reference clock period in nanoseconds.
> Shouldn't you be able to retrieve the refclk frequency and then 
> calculate the period?

The thing is we cannot determine the ref_clk frequency for some devices
that don't specify their clocks. So I think we should have an option to
inform the controller of the ref_clk period for those devices.

Thanks for the the review,
Thinh

Re: [PATCH v2 1/4] usb: dwc3: Add property snps,refclk-period-ns

[Rob Herring]

On Tue, Dec 18, 2018 at 6:22 PM Thinh Nguyen <thinh.nguyen@synopsys.com> wrote:
>
> Hi Rob,
>
> On 12/18/2018 8:39 AM, Rob Herring wrote:
> > On Fri, Dec 07, 2018 at 06:27:30PM -0800, Thinh Nguyen wrote:
> >> This patch introduces property "snps,refclk-period-ns" to inform the
> >> controller of the reference clock period. If the reference clock period
> >> is different from the default Core Consultant setting, then this
> >> property can be set to the reference clock period.
> >>
> >> This property does not control the reference clock rate. The controller
> >> uses this value to perform internal timing calculations that are based
> >> on the reference clock.
> >>
> >> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
> >> ---
> >> Changes in v2:
> >> - Split from "usb: dwc3: Add reference clock properties"
> >> - Revise commit message and property description
> >>
> >>  Documentation/devicetree/bindings/usb/dwc3.txt | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> >> index 8e5265e9f658..b7e67edff9b2 100644
> >> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> >> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> >> @@ -99,6 +99,8 @@ Optional properties:
> >>                      this and tx-thr-num-pkt-prd to a valid, non-zero value
> >>                      1-16 (DWC_usb31 programming guide section 1.2.3) to
> >>                      enable periodic ESS TX threshold.
> >> + - snps,refclk-period-ns: if set, this value informs the controller of the
> >> +                    reference clock period in nanoseconds.
> > Shouldn't you be able to retrieve the refclk frequency and then
> > calculate the period?
>
> The thing is we cannot determine the ref_clk frequency for some devices
> that don't specify their clocks. So I think we should have an option to
> inform the controller of the ref_clk period for those devices.

Specifying the clock should be mandatory (if you want/need this
feature). It just requires a fixed-clock node at a minimum.

Rob

Re: [PATCH v2 1/4] usb: dwc3: Add property snps,refclk-period-ns

[Thinh Nguyen]

Hi Rob,

On 12/19/2018 5:18 AM, Rob Herring wrote:
> On Tue, Dec 18, 2018 at 6:22 PM Thinh Nguyen <thinh.nguyen@synopsys.com> wrote:
>> Hi Rob,
>>
>> On 12/18/2018 8:39 AM, Rob Herring wrote:
>>> On Fri, Dec 07, 2018 at 06:27:30PM -0800, Thinh Nguyen wrote:
>>>> This patch introduces property "snps,refclk-period-ns" to inform the
>>>> controller of the reference clock period. If the reference clock period
>>>> is different from the default Core Consultant setting, then this
>>>> property can be set to the reference clock period.
>>>>
>>>> This property does not control the reference clock rate. The controller
>>>> uses this value to perform internal timing calculations that are based
>>>> on the reference clock.
>>>>
>>>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
>>>> ---
>>>> Changes in v2:
>>>> - Split from "usb: dwc3: Add reference clock properties"
>>>> - Revise commit message and property description
>>>>
>>>>  Documentation/devicetree/bindings/usb/dwc3.txt | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>> index 8e5265e9f658..b7e67edff9b2 100644
>>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>> @@ -99,6 +99,8 @@ Optional properties:
>>>>                      this and tx-thr-num-pkt-prd to a valid, non-zero value
>>>>                      1-16 (DWC_usb31 programming guide section 1.2.3) to
>>>>                      enable periodic ESS TX threshold.
>>>> + - snps,refclk-period-ns: if set, this value informs the controller of the
>>>> +                    reference clock period in nanoseconds.
>>> Shouldn't you be able to retrieve the refclk frequency and then
>>> calculate the period?
>> The thing is we cannot determine the ref_clk frequency for some devices
>> that don't specify their clocks. So I think we should have an option to
>> inform the controller of the ref_clk period for those devices.
> Specifying the clock should be mandatory (if you want/need this
> feature). It just requires a fixed-clock node at a minimum.

Depending on the design of the controller, the ref_clk frequency is not
something that the OS can read/control. So we cannot make it mandatory
for every device to have a clock node.

Thinh

Re: [PATCH v2 1/4] usb: dwc3: Add property snps,refclk-period-ns

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2546 bytes --]


Hi,

Thinh Nguyen <thinh.nguyen@synopsys.com> writes:
>>> On 12/18/2018 8:39 AM, Rob Herring wrote:
>>>> On Fri, Dec 07, 2018 at 06:27:30PM -0800, Thinh Nguyen wrote:
>>>>> This patch introduces property "snps,refclk-period-ns" to inform the
>>>>> controller of the reference clock period. If the reference clock period
>>>>> is different from the default Core Consultant setting, then this
>>>>> property can be set to the reference clock period.
>>>>>
>>>>> This property does not control the reference clock rate. The controller
>>>>> uses this value to perform internal timing calculations that are based
>>>>> on the reference clock.
>>>>>
>>>>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
>>>>> ---
>>>>> Changes in v2:
>>>>> - Split from "usb: dwc3: Add reference clock properties"
>>>>> - Revise commit message and property description
>>>>>
>>>>>  Documentation/devicetree/bindings/usb/dwc3.txt | 2 ++
>>>>>  1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>> index 8e5265e9f658..b7e67edff9b2 100644
>>>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>> @@ -99,6 +99,8 @@ Optional properties:
>>>>>                      this and tx-thr-num-pkt-prd to a valid, non-zero value
>>>>>                      1-16 (DWC_usb31 programming guide section 1.2.3) to
>>>>>                      enable periodic ESS TX threshold.
>>>>> + - snps,refclk-period-ns: if set, this value informs the controller of the
>>>>> +                    reference clock period in nanoseconds.
>>>> Shouldn't you be able to retrieve the refclk frequency and then
>>>> calculate the period?
>>> The thing is we cannot determine the ref_clk frequency for some devices
>>> that don't specify their clocks. So I think we should have an option to
>>> inform the controller of the ref_clk period for those devices.
>> Specifying the clock should be mandatory (if you want/need this
>> feature). It just requires a fixed-clock node at a minimum.
>
> Depending on the design of the controller, the ref_clk frequency is not
> something that the OS can read/control. So we cannot make it mandatory
> for every device to have a clock node.

We can make it mandatory to everyone who wants to use the feature. It's
no different than making snps,refclk-period-ns mandatory to everyone who
wants to use the feature you're introducing.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v2 1/4] usb: dwc3: Add property snps,refclk-period-ns

[Thinh Nguyen]

Hi,

On 12/19/2018 10:48 PM, Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <thinh.nguyen@synopsys.com> writes:
>>>> On 12/18/2018 8:39 AM, Rob Herring wrote:
>>>>> On Fri, Dec 07, 2018 at 06:27:30PM -0800, Thinh Nguyen wrote:
>>>>>> This patch introduces property "snps,refclk-period-ns" to inform the
>>>>>> controller of the reference clock period. If the reference clock period
>>>>>> is different from the default Core Consultant setting, then this
>>>>>> property can be set to the reference clock period.
>>>>>>
>>>>>> This property does not control the reference clock rate. The controller
>>>>>> uses this value to perform internal timing calculations that are based
>>>>>> on the reference clock.
>>>>>>
>>>>>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
>>>>>> ---
>>>>>> Changes in v2:
>>>>>> - Split from "usb: dwc3: Add reference clock properties"
>>>>>> - Revise commit message and property description
>>>>>>
>>>>>>  Documentation/devicetree/bindings/usb/dwc3.txt | 2 ++
>>>>>>  1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>>> index 8e5265e9f658..b7e67edff9b2 100644
>>>>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>>> @@ -99,6 +99,8 @@ Optional properties:
>>>>>>                      this and tx-thr-num-pkt-prd to a valid, non-zero value
>>>>>>                      1-16 (DWC_usb31 programming guide section 1.2.3) to
>>>>>>                      enable periodic ESS TX threshold.
>>>>>> + - snps,refclk-period-ns: if set, this value informs the controller of the
>>>>>> +                    reference clock period in nanoseconds.
>>>>> Shouldn't you be able to retrieve the refclk frequency and then
>>>>> calculate the period?
>>>> The thing is we cannot determine the ref_clk frequency for some devices
>>>> that don't specify their clocks. So I think we should have an option to
>>>> inform the controller of the ref_clk period for those devices.
>>> Specifying the clock should be mandatory (if you want/need this
>>> feature). It just requires a fixed-clock node at a minimum.
>> Depending on the design of the controller, the ref_clk frequency is not
>> something that the OS can read/control. So we cannot make it mandatory
>> for every device to have a clock node.
> We can make it mandatory to everyone who wants to use the feature. It's
> no different than making snps,refclk-period-ns mandatory to everyone who
> wants to use the feature you're introducing.
>

But not every design has access to the clock with an actual address for
the OS to read. Only the developers will know the frequency of the
ref_clk, and they can inform the controller via this property.

We cannot force the developers to change their design requirement simply
to inform the controller of the ref_clk period.

Thinh

Re: [PATCH v2 1/4] usb: dwc3: Add property snps,refclk-period-ns

[Rob Herring]

On Thu, Dec 20, 2018 at 6:22 PM Thinh Nguyen <thinh.nguyen@synopsys.com> wrote:
>
> Hi,
>
> On 12/19/2018 10:48 PM, Felipe Balbi wrote:
> > Hi,
> >
> > Thinh Nguyen <thinh.nguyen@synopsys.com> writes:
> >>>> On 12/18/2018 8:39 AM, Rob Herring wrote:
> >>>>> On Fri, Dec 07, 2018 at 06:27:30PM -0800, Thinh Nguyen wrote:
> >>>>>> This patch introduces property "snps,refclk-period-ns" to inform the
> >>>>>> controller of the reference clock period. If the reference clock period
> >>>>>> is different from the default Core Consultant setting, then this
> >>>>>> property can be set to the reference clock period.
> >>>>>>
> >>>>>> This property does not control the reference clock rate. The controller
> >>>>>> uses this value to perform internal timing calculations that are based
> >>>>>> on the reference clock.
> >>>>>>
> >>>>>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
> >>>>>> ---
> >>>>>> Changes in v2:
> >>>>>> - Split from "usb: dwc3: Add reference clock properties"
> >>>>>> - Revise commit message and property description
> >>>>>>
> >>>>>>  Documentation/devicetree/bindings/usb/dwc3.txt | 2 ++
> >>>>>>  1 file changed, 2 insertions(+)
> >>>>>>
> >>>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> >>>>>> index 8e5265e9f658..b7e67edff9b2 100644
> >>>>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> >>>>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> >>>>>> @@ -99,6 +99,8 @@ Optional properties:
> >>>>>>                      this and tx-thr-num-pkt-prd to a valid, non-zero value
> >>>>>>                      1-16 (DWC_usb31 programming guide section 1.2.3) to
> >>>>>>                      enable periodic ESS TX threshold.
> >>>>>> + - snps,refclk-period-ns: if set, this value informs the controller of the
> >>>>>> +                    reference clock period in nanoseconds.
> >>>>> Shouldn't you be able to retrieve the refclk frequency and then
> >>>>> calculate the period?
> >>>> The thing is we cannot determine the ref_clk frequency for some devices
> >>>> that don't specify their clocks. So I think we should have an option to
> >>>> inform the controller of the ref_clk period for those devices.
> >>> Specifying the clock should be mandatory (if you want/need this
> >>> feature). It just requires a fixed-clock node at a minimum.
> >> Depending on the design of the controller, the ref_clk frequency is not
> >> something that the OS can read/control. So we cannot make it mandatory
> >> for every device to have a clock node.
> > We can make it mandatory to everyone who wants to use the feature. It's
> > no different than making snps,refclk-period-ns mandatory to everyone who
> > wants to use the feature you're introducing.
> >
>
> But not every design has access to the clock with an actual address for
> the OS to read. Only the developers will know the frequency of the
> ref_clk, and they can inform the controller via this property.

Then you use the flxed-clock binding.

Rob

Re: [PATCH v2 1/4] usb: dwc3: Add property snps,refclk-period-ns

[Thinh Nguyen]

Hi,

On 12/21/2018 9:12 AM, Rob Herring wrote:
> On Thu, Dec 20, 2018 at 6:22 PM Thinh Nguyen <thinh.nguyen@synopsys.com> wrote:
>> Hi,
>>
>> On 12/19/2018 10:48 PM, Felipe Balbi wrote:
>>> Hi,
>>>
>>> Thinh Nguyen <thinh.nguyen@synopsys.com> writes:
>>>>>> On 12/18/2018 8:39 AM, Rob Herring wrote:
>>>>>>> On Fri, Dec 07, 2018 at 06:27:30PM -0800, Thinh Nguyen wrote:
>>>>>>>> This patch introduces property "snps,refclk-period-ns" to inform the
>>>>>>>> controller of the reference clock period. If the reference clock period
>>>>>>>> is different from the default Core Consultant setting, then this
>>>>>>>> property can be set to the reference clock period.
>>>>>>>>
>>>>>>>> This property does not control the reference clock rate. The controller
>>>>>>>> uses this value to perform internal timing calculations that are based
>>>>>>>> on the reference clock.
>>>>>>>>
>>>>>>>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
>>>>>>>> ---
>>>>>>>> Changes in v2:
>>>>>>>> - Split from "usb: dwc3: Add reference clock properties"
>>>>>>>> - Revise commit message and property description
>>>>>>>>
>>>>>>>>  Documentation/devicetree/bindings/usb/dwc3.txt | 2 ++
>>>>>>>>  1 file changed, 2 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>>>>> index 8e5265e9f658..b7e67edff9b2 100644
>>>>>>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>>>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>>>>> @@ -99,6 +99,8 @@ Optional properties:
>>>>>>>>                      this and tx-thr-num-pkt-prd to a valid, non-zero value
>>>>>>>>                      1-16 (DWC_usb31 programming guide section 1.2.3) to
>>>>>>>>                      enable periodic ESS TX threshold.
>>>>>>>> + - snps,refclk-period-ns: if set, this value informs the controller of the
>>>>>>>> +                    reference clock period in nanoseconds.
>>>>>>> Shouldn't you be able to retrieve the refclk frequency and then
>>>>>>> calculate the period?
>>>>>> The thing is we cannot determine the ref_clk frequency for some devices
>>>>>> that don't specify their clocks. So I think we should have an option to
>>>>>> inform the controller of the ref_clk period for those devices.
>>>>> Specifying the clock should be mandatory (if you want/need this
>>>>> feature). It just requires a fixed-clock node at a minimum.
>>>> Depending on the design of the controller, the ref_clk frequency is not
>>>> something that the OS can read/control. So we cannot make it mandatory
>>>> for every device to have a clock node.
>>> We can make it mandatory to everyone who wants to use the feature. It's
>>> no different than making snps,refclk-period-ns mandatory to everyone who
>>> wants to use the feature you're introducing.
>>>
>> But not every design has access to the clock with an actual address for
>> the OS to read. Only the developers will know the frequency of the
>> ref_clk, and they can inform the controller via this property.
> Then you use the flxed-clock binding.

Ah.. I didn't know that. Sure, I'll check and revise the change so it
can be done that way.

Thanks,
Thinh

[PATCH v2 3/4] usb: dwc3: Add property snps,enable-refclk-sof

[Thinh Nguyen]

This patch adds a property to enable the controller to track the
frame number based on the reference clock.

When operating in USB 2.0 mode, the peripheral controller uses the USB2
PHY clocks to track the frame number. This prevents the controller from
suspending the USB2 PHY when the device goes into low power. Version
1.80a of the DWC_usb31 peripheral controller introduces a way to track
frame number based on the reference clock instead. This feature allows
the controller to suspend the USB2 PHY when the device goes into low
power. This improves power saving for devices that have isochronous
endpoints.

Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
Changes in v2:
- Revise property description
- Rename property from snps,enable-refclk-lpm to snps,enable-refclk-sof

 Documentation/devicetree/bindings/usb/dwc3.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
index b7e67edff9b2..01b948fff0eb 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -101,6 +101,9 @@ Optional properties:
 			enable periodic ESS TX threshold.
  - snps,refclk-period-ns: if set, this value informs the controller of the
 			reference clock period in nanoseconds.
+ - snps,enable-refclk-sof: set to enable reference clock based frame number
+			tracking while in low power, allowing the controller to
+			suspend the PHY during low power states.
 
  - <DEPRECATED> tx-fifo-resize: determines if the FIFO *has* to be reallocated.
  - snps,incr-burst-type-adjustment: Value for INCR burst type of GSBUSCFG0
-- 
2.11.0

Re: [PATCH v2 3/4] usb: dwc3: Add property snps,enable-refclk-sof

[Rob Herring]

On Fri, Dec 07, 2018 at 06:27:43PM -0800, Thinh Nguyen wrote:
> This patch adds a property to enable the controller to track the
> frame number based on the reference clock.
> 
> When operating in USB 2.0 mode, the peripheral controller uses the USB2
> PHY clocks to track the frame number. This prevents the controller from
> suspending the USB2 PHY when the device goes into low power. Version
> 1.80a of the DWC_usb31 peripheral controller introduces a way to track
> frame number based on the reference clock instead. This feature allows
> the controller to suspend the USB2 PHY when the device goes into low
> power. This improves power saving for devices that have isochronous
> endpoints.
> 
> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
> ---
> Changes in v2:
> - Revise property description
> - Rename property from snps,enable-refclk-lpm to snps,enable-refclk-sof
> 
>  Documentation/devicetree/bindings/usb/dwc3.txt | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> index b7e67edff9b2..01b948fff0eb 100644
> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> @@ -101,6 +101,9 @@ Optional properties:
>  			enable periodic ESS TX threshold.
>   - snps,refclk-period-ns: if set, this value informs the controller of the
>  			reference clock period in nanoseconds.
> + - snps,enable-refclk-sof: set to enable reference clock based frame number
> +			tracking while in low power, allowing the controller to
> +			suspend the PHY during low power states.

This should be implied by the compatible string.

>  
>   - <DEPRECATED> tx-fifo-resize: determines if the FIFO *has* to be reallocated.
>   - snps,incr-burst-type-adjustment: Value for INCR burst type of GSBUSCFG0
> -- 
> 2.11.0
> 

Re: [PATCH v2 3/4] usb: dwc3: Add property snps,enable-refclk-sof

[Thinh Nguyen]

Hi Rob,

On 12/18/2018 8:41 AM, Rob Herring wrote:
> On Fri, Dec 07, 2018 at 06:27:43PM -0800, Thinh Nguyen wrote:
>> This patch adds a property to enable the controller to track the
>> frame number based on the reference clock.
>>
>> When operating in USB 2.0 mode, the peripheral controller uses the USB2
>> PHY clocks to track the frame number. This prevents the controller from
>> suspending the USB2 PHY when the device goes into low power. Version
>> 1.80a of the DWC_usb31 peripheral controller introduces a way to track
>> frame number based on the reference clock instead. This feature allows
>> the controller to suspend the USB2 PHY when the device goes into low
>> power. This improves power saving for devices that have isochronous
>> endpoints.
>>
>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
>> ---
>> Changes in v2:
>> - Revise property description
>> - Rename property from snps,enable-refclk-lpm to snps,enable-refclk-sof
>>
>>  Documentation/devicetree/bindings/usb/dwc3.txt | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>> index b7e67edff9b2..01b948fff0eb 100644
>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>> @@ -101,6 +101,9 @@ Optional properties:
>>  			enable periodic ESS TX threshold.
>>   - snps,refclk-period-ns: if set, this value informs the controller of the
>>  			reference clock period in nanoseconds.
>> + - snps,enable-refclk-sof: set to enable reference clock based frame number
>> +			tracking while in low power, allowing the controller to
>> +			suspend the PHY during low power states.
> This should be implied by the compatible string.

Can you clarify further how that will work? (I think I may misunderstand
what you mean)

Thanks for the review!
Thinh

Re: [PATCH v2 3/4] usb: dwc3: Add property snps,enable-refclk-sof

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2359 bytes --]


Hi,

Rob Herring <robh@kernel.org> writes:
> On Fri, Dec 07, 2018 at 06:27:43PM -0800, Thinh Nguyen wrote:
>> This patch adds a property to enable the controller to track the
>> frame number based on the reference clock.
>> 
>> When operating in USB 2.0 mode, the peripheral controller uses the USB2
>> PHY clocks to track the frame number. This prevents the controller from
>> suspending the USB2 PHY when the device goes into low power. Version
>> 1.80a of the DWC_usb31 peripheral controller introduces a way to track
>> frame number based on the reference clock instead. This feature allows
>> the controller to suspend the USB2 PHY when the device goes into low
>> power. This improves power saving for devices that have isochronous
>> endpoints.
>> 
>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
>> ---
>> Changes in v2:
>> - Revise property description
>> - Rename property from snps,enable-refclk-lpm to snps,enable-refclk-sof
>> 
>>  Documentation/devicetree/bindings/usb/dwc3.txt | 3 +++
>>  1 file changed, 3 insertions(+)
>> 
>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>> index b7e67edff9b2..01b948fff0eb 100644
>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>> @@ -101,6 +101,9 @@ Optional properties:
>>  			enable periodic ESS TX threshold.
>>   - snps,refclk-period-ns: if set, this value informs the controller of the
>>  			reference clock period in nanoseconds.
>> + - snps,enable-refclk-sof: set to enable reference clock based frame number
>> +			tracking while in low power, allowing the controller to
>> +			suspend the PHY during low power states.
>
> This should be implied by the compatible string.

Two problems with this:

1) Won't work for PCI-based systems

2) If we start having many users of this we will end up with:

	if (of_device_is_compatible("a") ||
        	of_device_is_compatible("b") ||
        	of_device_is_compatible("c") ||
                of_device_is_compatible("d") ||
                ...)
		foo();

Conversely, if we just pass a flag, this branch will never change. We
won't need changes to the kernel because a new platform needing refclk
based frame number tracking is, now, supported upstream.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v2 3/4] usb: dwc3: Add property snps,enable-refclk-sof

[Rob Herring]

On Thu, Dec 20, 2018 at 12:52 AM Felipe Balbi <balbi@kernel.org> wrote:
>
>
> Hi,
>
> Rob Herring <robh@kernel.org> writes:
> > On Fri, Dec 07, 2018 at 06:27:43PM -0800, Thinh Nguyen wrote:
> >> This patch adds a property to enable the controller to track the
> >> frame number based on the reference clock.
> >>
> >> When operating in USB 2.0 mode, the peripheral controller uses the USB2
> >> PHY clocks to track the frame number. This prevents the controller from
> >> suspending the USB2 PHY when the device goes into low power. Version
> >> 1.80a of the DWC_usb31 peripheral controller introduces a way to track
> >> frame number based on the reference clock instead. This feature allows
> >> the controller to suspend the USB2 PHY when the device goes into low
> >> power. This improves power saving for devices that have isochronous
> >> endpoints.
> >>
> >> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
> >> ---
> >> Changes in v2:
> >> - Revise property description
> >> - Rename property from snps,enable-refclk-lpm to snps,enable-refclk-sof
> >>
> >>  Documentation/devicetree/bindings/usb/dwc3.txt | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> >> index b7e67edff9b2..01b948fff0eb 100644
> >> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> >> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> >> @@ -101,6 +101,9 @@ Optional properties:
> >>                      enable periodic ESS TX threshold.
> >>   - snps,refclk-period-ns: if set, this value informs the controller of the
> >>                      reference clock period in nanoseconds.
> >> + - snps,enable-refclk-sof: set to enable reference clock based frame number
> >> +                    tracking while in low power, allowing the controller to
> >> +                    suspend the PHY during low power states.
> >
> > This should be implied by the compatible string.
>
> Two problems with this:
>
> 1) Won't work for PCI-based systems

For PCI, you would decide this based on VID/PID, wouldn't you?
Compatible is basically equivalent to that.

> 2) If we start having many users of this we will end up with:
>
>         if (of_device_is_compatible("a") ||
>                 of_device_is_compatible("b") ||
>                 of_device_is_compatible("c") ||
>                 of_device_is_compatible("d") ||
>                 ...)
>                 foo();
>
> Conversely, if we just pass a flag, this branch will never change. We
> won't need changes to the kernel because a new platform needing refclk
> based frame number tracking is, now, supported upstream.

Things are implied based on compatible frequently and this is not how
the code ends up looking like. Normally, match data would have the
flag setting and that variable will get passed to whatever needs it.

USB blocks and their integration quirks are complicated enough that
I'm not really worried about needing to update the kernel for a new
platform. If a new platform is so similar to an existing one, then a
fallback compatible can be used and the kernel doesn't need a change.

A property like this makes more sense when it is board-specific, not
SoC specific. I may be wrong, but this looked more like a SoC
integration decision than board level.

Rob