[PATCH v1 1/2] dt-bindings: usb: snps,dwc3: Add property for imod

[PATCH v1 1/2] dt-bindings: usb: snps,dwc3: Add property for imod

[Badhri Jagan Sridharan]

This change adds `snps,device-mode-intrpt-mod-interval`
which allows enabling interrupt moderation through
snps,dwc3 node.

`snps,device-mode-intrpt-mod-interval`specifies the
minimum inter-interrupt interval in 250ns increments
during device mode operation. A value of 0 disables
the interrupt throttling logic and interrupts are
generated immediately if event count becomes non-zero.
Otherwise, the interrupt is signaled when all of the
following conditons are met which are, EVNT_HANDLER_BUSY
is 0, event count is non-zero and at least 250ns increments
of this value has elapsed since the last time interrupt
was de-asserted.

Cc: stable@kernel.org
Fixes: cf40b86b6ef6 ("usb: dwc3: Implement interrupt moderation")
Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
---
 .../devicetree/bindings/usb/snps,dwc3-common.yaml   | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
index c956053fd036..3957f1dac3c4 100644
--- a/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
+++ b/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
@@ -375,6 +375,19 @@ properties:
     items:
       enum: [1, 4, 8, 16, 32, 64, 128, 256]
 
+  snps,device-mode-intrpt-mod-interval:
+    description:
+      Specifies the minimum inter-interrupt interval in 250ns increments
+      during device mode operation. A value of 0 disables the interrupt
+      throttling logic and interrupts are generated immediately if event
+      count becomes non-zero. Otherwise, the interrupt is signaled when
+      all of the following conditons are met which are, EVNT_HANDLER_BUSY
+      is 0, event count is non-zero and at least 250ns increments of this
+      value has elapsed since the last time interrupt was de-asserted.
+    $ref: /schemas/types.yaml#/definitions/uint16
+    minimum: 0
+    maximum: 255
+
   num-hc-interrupters:
     maximum: 8
     default: 1

base-commit: 72deda0abee6e705ae71a93f69f55e33be5bca5c
-- 
2.48.1.362.g079036d154-goog

[PATCH v1 2/2] usb: dwc3: core: Obtain imod_interval from device tree

[Badhri Jagan Sridharan]

Although commit cf40b86b6ef6 ("usb: dwc3: Implement interrupt
moderation") adds support for interrupt moderation in device
mode, it does not add an option to enable interrupt moderation
unless the version of the controller is 3.00a where the
interrupt moderation is automatically enabled. This patch
reads the interrupt moderation interval counter value from
device tree so that the interrupt moderation can be enabled
through the device tree.

The explicit initialization to 0 can be avoided as the dwc3
struct is kzalloc'ed.

Cc: stable@kernel.org
Fixes: cf40b86b6ef6 ("usb: dwc3: Implement interrupt moderation")
Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
---
 drivers/usb/dwc3/core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index dfa1b5fe48dc..be0d302bbab7 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1818,6 +1818,9 @@ static void dwc3_get_properties(struct dwc3 *dwc)
 	dwc->dis_split_quirk = device_property_read_bool(dev,
 				"snps,dis-split-quirk");
 
+	device_property_read_u16(dev, "snps,device-mode-intrpt-mod-interval",
+				 &dwc->imod_interval);
+
 	dwc->lpm_nyet_threshold = lpm_nyet_threshold;
 	dwc->tx_de_emphasis = tx_de_emphasis;
 
@@ -1835,8 +1838,6 @@ static void dwc3_get_properties(struct dwc3 *dwc)
 	dwc->tx_thr_num_pkt_prd = tx_thr_num_pkt_prd;
 	dwc->tx_max_burst_prd = tx_max_burst_prd;
 
-	dwc->imod_interval = 0;
-
 	dwc->tx_fifo_resize_max_num = tx_fifo_resize_max_num;
 }
 
-- 
2.48.1.362.g079036d154-goog

Re: [PATCH v1 1/2] dt-bindings: usb: snps,dwc3: Add property for imod

[Krzysztof Kozlowski]

On Sun, Feb 02, 2025 at 03:50:59AM +0000, Badhri Jagan Sridharan wrote:
> This change adds `snps,device-mode-intrpt-mod-interval`

Thank you for your patch. There is something to discuss/improve.

> which allows enabling interrupt moderation through
> snps,dwc3 node.
> 
> `snps,device-mode-intrpt-mod-interval`specifies the
> minimum inter-interrupt interval in 250ns increments
> during device mode operation. A value of 0 disables
> the interrupt throttling logic and interrupts are
> generated immediately if event count becomes non-zero.
> Otherwise, the interrupt is signaled when all of the
> following conditons are met which are, EVNT_HANDLER_BUSY
> is 0, event count is non-zero and at least 250ns increments
> of this value has elapsed since the last time interrupt
> was de-asserted.

Please wrap commit message according to Linux coding style / submission
process (neither too early nor over the limit):
https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597

> 
> Cc: stable@kernel.org
> Fixes: cf40b86b6ef6 ("usb: dwc3: Implement interrupt moderation")

I don't understand what are you fixing here.  Above commit does not
introduce that property.


> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> ---
>  .../devicetree/bindings/usb/snps,dwc3-common.yaml   | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
> index c956053fd036..3957f1dac3c4 100644
> --- a/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
> @@ -375,6 +375,19 @@ properties:
>      items:
>        enum: [1, 4, 8, 16, 32, 64, 128, 256]
>  
> +  snps,device-mode-intrpt-mod-interval:
> +    description:
> +      Specifies the minimum inter-interrupt interval in 250ns increments

Then use proper property unit suffix.

> +      during device mode operation. A value of 0 disables the interrupt
> +      throttling logic and interrupts are generated immediately if event
> +      count becomes non-zero. Otherwise, the interrupt is signaled when
> +      all of the following conditons are met which are, EVNT_HANDLER_BUSY
> +      is 0, event count is non-zero and at least 250ns increments of this
> +      value has elapsed since the last time interrupt was de-asserted.

Why is this property of a board? Why different boards would wait
different amount of time?

> +    $ref: /schemas/types.yaml#/definitions/uint16

Drop, use proper name suffix.

Best regards,
Krzysztof

Re: [PATCH v1 1/2] dt-bindings: usb: snps,dwc3: Add property for imod

[Krzysztof Kozlowski]

On 02/02/2025 15:11, Krzysztof Kozlowski wrote:
> On Sun, Feb 02, 2025 at 03:50:59AM +0000, Badhri Jagan Sridharan wrote:
>> This change adds `snps,device-mode-intrpt-mod-interval`
> 
> Thank you for your patch. There is something to discuss/improve.


Also one more note:

Please do not use "This commit/patch/change", but imperative mood. See
longer explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95


Best regards,
Krzysztof

Re: [PATCH v1 2/2] usb: dwc3: core: Obtain imod_interval from device tree

[Thinh Nguyen]

On Sun, Feb 02, 2025, Badhri Jagan Sridharan wrote:
> Although commit cf40b86b6ef6 ("usb: dwc3: Implement interrupt
> moderation") adds support for interrupt moderation in device
> mode, it does not add an option to enable interrupt moderation
> unless the version of the controller is 3.00a where the
> interrupt moderation is automatically enabled. This patch
> reads the interrupt moderation interval counter value from
> device tree so that the interrupt moderation can be enabled
> through the device tree.
> 
> The explicit initialization to 0 can be avoided as the dwc3
> struct is kzalloc'ed.
> 
> Cc: stable@kernel.org
> Fixes: cf40b86b6ef6 ("usb: dwc3: Implement interrupt moderation")
> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> ---
>  drivers/usb/dwc3/core.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index dfa1b5fe48dc..be0d302bbab7 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1818,6 +1818,9 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>  	dwc->dis_split_quirk = device_property_read_bool(dev,
>  				"snps,dis-split-quirk");
>  
> +	device_property_read_u16(dev, "snps,device-mode-intrpt-mod-interval",
> +				 &dwc->imod_interval);
> +

This value will get overwritten in dwc3_check_params() for DWC_usb3
v3.00a.

>  	dwc->lpm_nyet_threshold = lpm_nyet_threshold;
>  	dwc->tx_de_emphasis = tx_de_emphasis;
>  
> @@ -1835,8 +1838,6 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>  	dwc->tx_thr_num_pkt_prd = tx_thr_num_pkt_prd;
>  	dwc->tx_max_burst_prd = tx_max_burst_prd;
>  
> -	dwc->imod_interval = 0;
> -
>  	dwc->tx_fifo_resize_max_num = tx_fifo_resize_max_num;
>  }
>  
> -- 
> 2.48.1.362.g079036d154-goog
> 

Do you need a property to adjust the IMOD interval? If not, just enable
it for all capable devices (dwc3_has_imod) with IMODI of 1 for now. It
should be good to have it enabled for all capable devices by default.

BR,
Thinh

Re: [PATCH v1 2/2] usb: dwc3: core: Obtain imod_interval from device tree

[Badhri Jagan Sridharan]

On Mon, Feb 3, 2025 at 4:46 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
>
> On Sun, Feb 02, 2025, Badhri Jagan Sridharan wrote:
> > Although commit cf40b86b6ef6 ("usb: dwc3: Implement interrupt
> > moderation") adds support for interrupt moderation in device
> > mode, it does not add an option to enable interrupt moderation
> > unless the version of the controller is 3.00a where the
> > interrupt moderation is automatically enabled. This patch
> > reads the interrupt moderation interval counter value from
> > device tree so that the interrupt moderation can be enabled
> > through the device tree.
> >
> > The explicit initialization to 0 can be avoided as the dwc3
> > struct is kzalloc'ed.
> >
> > Cc: stable@kernel.org
> > Fixes: cf40b86b6ef6 ("usb: dwc3: Implement interrupt moderation")
> > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> > ---
> >  drivers/usb/dwc3/core.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index dfa1b5fe48dc..be0d302bbab7 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -1818,6 +1818,9 @@ static void dwc3_get_properties(struct dwc3 *dwc)
> >       dwc->dis_split_quirk = device_property_read_bool(dev,
> >                               "snps,dis-split-quirk");
> >
> > +     device_property_read_u16(dev, "snps,device-mode-intrpt-mod-interval",
> > +                              &dwc->imod_interval);
> > +
>
> This value will get overwritten in dwc3_check_params() for DWC_usb3
> v3.00a.
>
> >       dwc->lpm_nyet_threshold = lpm_nyet_threshold;
> >       dwc->tx_de_emphasis = tx_de_emphasis;
> >
> > @@ -1835,8 +1838,6 @@ static void dwc3_get_properties(struct dwc3 *dwc)
> >       dwc->tx_thr_num_pkt_prd = tx_thr_num_pkt_prd;
> >       dwc->tx_max_burst_prd = tx_max_burst_prd;
> >
> > -     dwc->imod_interval = 0;
> > -
> >       dwc->tx_fifo_resize_max_num = tx_fifo_resize_max_num;
> >  }
> >
> > --
> > 2.48.1.362.g079036d154-goog
> >
>
> Do you need a property to adjust the IMOD interval? If not, just enable
> it for all capable devices (dwc3_has_imod) with IMODI of 1 for now. It
> should be good to have it enabled for all capable devices by default.
>

Yes, IMHO it would make sense to have a property to adjust IMOD
interval for two reasons:
1. This would be then identical to the "imod-interval-ns" property
that's used for XHCI based host mode controllers.
2. Also, potentially allowing the controller to interrupt once every
250ns might not be desirable for all platforms and should be left as
platform tunable to fully realize the capability of interrupt
moderation which the controller offers again similar to
"imod-interval-ns" property in host mode.

Along with this I will also set the default value to 1 in my v2 which
I will send out shortly.

Thanks,
Badhri

> BR,
> Thinh

Re: [PATCH v1 1/2] dt-bindings: usb: snps,dwc3: Add property for imod

[Badhri Jagan Sridharan]

.

On Sun, Feb 2, 2025 at 6:11 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Sun, Feb 02, 2025 at 03:50:59AM +0000, Badhri Jagan Sridharan wrote:
> > This change adds `snps,device-mode-intrpt-mod-interval`
>
> Thank you for your patch. There is something to discuss/improve.

Hi Krzysztof,

Thanks for taking the time to review ! Happy to address them.

>
> > which allows enabling interrupt moderation through
> > snps,dwc3 node.
> >
> > `snps,device-mode-intrpt-mod-interval`specifies the
> > minimum inter-interrupt interval in 250ns increments
> > during device mode operation. A value of 0 disables
> > the interrupt throttling logic and interrupts are
> > generated immediately if event count becomes non-zero.
> > Otherwise, the interrupt is signaled when all of the
> > following conditons are met which are, EVNT_HANDLER_BUSY
> > is 0, event count is non-zero and at least 250ns increments
> > of this value has elapsed since the last time interrupt
> > was de-asserted.
>
> Please wrap commit message according to Linux coding style / submission
> process (neither too early nor over the limit):
> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597

Ack! will do in V2 of this patch.

>
> >
> > Cc: stable@kernel.org
> > Fixes: cf40b86b6ef6 ("usb: dwc3: Implement interrupt moderation")
>
> I don't understand what are you fixing here.  Above commit does not
> introduce that property.

Although the above commit does not add this property, it has
implemented the entire feature except for the property so thought
sending this with "Fixes:" while CCing  stable@ will allow the
backport.  I am interested in having this patch in older kernel
versions as well where imod support has been added. Wondering what
would be the right way to achieve this. Eager to know your thoughts !

>
>
> > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> > ---
> >  .../devicetree/bindings/usb/snps,dwc3-common.yaml   | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
> > index c956053fd036..3957f1dac3c4 100644
> > --- a/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
> > +++ b/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
> > @@ -375,6 +375,19 @@ properties:
> >      items:
> >        enum: [1, 4, 8, 16, 32, 64, 128, 256]
> >
> > +  snps,device-mode-intrpt-mod-interval:
> > +    description:
> > +      Specifies the minimum inter-interrupt interval in 250ns increments
>
> Then use proper property unit suffix.

Ack ! changing to snps,device-mode-intrpt-mod-interval-ns in V2.

>
> > +      during device mode operation. A value of 0 disables the interrupt
> > +      throttling logic and interrupts are generated immediately if event
> > +      count becomes non-zero. Otherwise, the interrupt is signaled when
> > +      all of the following conditons are met which are, EVNT_HANDLER_BUSY
> > +      is 0, event count is non-zero and at least 250ns increments of this
> > +      value has elapsed since the last time interrupt was de-asserted.
>
> Why is this property of a board? Why different boards would wait
> different amount of time?

Interrupt moderation allows batch processing of events reported by the
controller.
A very low value of snps,device-mode-intrpt-mod-interval-ns implies
that the controller will interrupt more often to make the host
processor process a smaller set of events Vs a larger value will wake
up the host processor at longer intervals to process events (likely
more). So depending what the board is designed for this can be tuned
to achieve the needed outcome.

This is very similar to the "imod-interval-ns" in
https://elixir.bootlin.com/linux/v6.13.1/source/Documentation/devicetree/bindings/usb/usb-xhci.yaml
except that in this case the Synopsys DWC3 controller supports this
for the device mode operation as well.

>
> > +    $ref: /schemas/types.yaml#/definitions/uint16
>
> Drop, use proper name suffix.

Ack, will drop in V2.

Thanks,
Badhri

>
> Best regards,
> Krzysztof
>

Re: [PATCH v1 1/2] dt-bindings: usb: snps,dwc3: Add property for imod

[Krzysztof Kozlowski]

On 05/02/2025 10:07, Badhri Jagan Sridharan wrote:
> .
> 
> On Sun, Feb 2, 2025 at 6:11 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On Sun, Feb 02, 2025 at 03:50:59AM +0000, Badhri Jagan Sridharan wrote:
>>> This change adds `snps,device-mode-intrpt-mod-interval`
>>
>> Thank you for your patch. There is something to discuss/improve.
> 
> Hi Krzysztof,
> 
> Thanks for taking the time to review ! Happy to address them.
> 
>>
>>> which allows enabling interrupt moderation through
>>> snps,dwc3 node.
>>>
>>> `snps,device-mode-intrpt-mod-interval`specifies the
>>> minimum inter-interrupt interval in 250ns increments
>>> during device mode operation. A value of 0 disables
>>> the interrupt throttling logic and interrupts are
>>> generated immediately if event count becomes non-zero.
>>> Otherwise, the interrupt is signaled when all of the
>>> following conditons are met which are, EVNT_HANDLER_BUSY
>>> is 0, event count is non-zero and at least 250ns increments
>>> of this value has elapsed since the last time interrupt
>>> was de-asserted.
>>
>> Please wrap commit message according to Linux coding style / submission
>> process (neither too early nor over the limit):
>> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597
> 
> Ack! will do in V2 of this patch.
> 
>>
>>>
>>> Cc: stable@kernel.org
>>> Fixes: cf40b86b6ef6 ("usb: dwc3: Implement interrupt moderation")
>>
>> I don't understand what are you fixing here.  Above commit does not
>> introduce that property.
> 
> Although the above commit does not add this property, it has
> implemented the entire feature except for the property so thought
> sending this with "Fixes:" while CCing  stable@ will allow the
> backport.  I am interested in having this patch in older kernel

Not implementing DT bindings is not a bug. Otherwise provide any sort of
proof that this was not intentional.

I can easily provide you proof why this was intentional: negative review
maintainers.


> versions as well where imod support has been added. Wondering what
> would be the right way to achieve this. Eager to know your thoughts !

So again, downstream and forks... NAK, you cannot push things to stable
just because you want them backported by Greg.

This is not acceptable.

> 
>>
>>
>>> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
>>> ---
>>>  .../devicetree/bindings/usb/snps,dwc3-common.yaml   | 13 +++++++++++++
>>>  1 file changed, 13 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
>>> index c956053fd036..3957f1dac3c4 100644
>>> --- a/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
>>> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
>>> @@ -375,6 +375,19 @@ properties:
>>>      items:
>>>        enum: [1, 4, 8, 16, 32, 64, 128, 256]
>>>
>>> +  snps,device-mode-intrpt-mod-interval:
>>> +    description:
>>> +      Specifies the minimum inter-interrupt interval in 250ns increments
>>
>> Then use proper property unit suffix.
> 
> Ack ! changing to snps,device-mode-intrpt-mod-interval-ns in V2.
> 
>>
>>> +      during device mode operation. A value of 0 disables the interrupt
>>> +      throttling logic and interrupts are generated immediately if event
>>> +      count becomes non-zero. Otherwise, the interrupt is signaled when
>>> +      all of the following conditons are met which are, EVNT_HANDLER_BUSY
>>> +      is 0, event count is non-zero and at least 250ns increments of this
>>> +      value has elapsed since the last time interrupt was de-asserted.
>>
>> Why is this property of a board? Why different boards would wait
>> different amount of time?
> 
> Interrupt moderation allows batch processing of events reported by the
> controller.
> A very low value of snps,device-mode-intrpt-mod-interval-ns implies
> that the controller will interrupt more often to make the host
> processor process a smaller set of events Vs a larger value will wake
> up the host processor at longer intervals to process events (likely
> more). So depending what the board is designed for this can be tuned
> to achieve the needed outcome.

I do not see dependency on the board. Host has the same CPU always, so
it processes with the same speed.


Best regards,
Krzysztof

Re: [PATCH v1 1/2] dt-bindings: usb: snps,dwc3: Add property for imod

[Badhri Jagan Sridharan]

On Wed, Feb 5, 2025 at 5:50 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 05/02/2025 10:07, Badhri Jagan Sridharan wrote:
> > .
> >
> > On Sun, Feb 2, 2025 at 6:11 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>
> >> On Sun, Feb 02, 2025 at 03:50:59AM +0000, Badhri Jagan Sridharan wrote:
> >>> This change adds `snps,device-mode-intrpt-mod-interval`
> >>
> >> Thank you for your patch. There is something to discuss/improve.
> >
> > Hi Krzysztof,
> >
> > Thanks for taking the time to review ! Happy to address them.
> >
> >>
> >>> which allows enabling interrupt moderation through
> >>> snps,dwc3 node.
> >>>
> >>> `snps,device-mode-intrpt-mod-interval`specifies the
> >>> minimum inter-interrupt interval in 250ns increments
> >>> during device mode operation. A value of 0 disables
> >>> the interrupt throttling logic and interrupts are
> >>> generated immediately if event count becomes non-zero.
> >>> Otherwise, the interrupt is signaled when all of the
> >>> following conditons are met which are, EVNT_HANDLER_BUSY
> >>> is 0, event count is non-zero and at least 250ns increments
> >>> of this value has elapsed since the last time interrupt
> >>> was de-asserted.
> >>
> >> Please wrap commit message according to Linux coding style / submission
> >> process (neither too early nor over the limit):
> >> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597
> >
> > Ack! will do in V2 of this patch.
> >
> >>
> >>>
> >>> Cc: stable@kernel.org
> >>> Fixes: cf40b86b6ef6 ("usb: dwc3: Implement interrupt moderation")
> >>
> >> I don't understand what are you fixing here.  Above commit does not
> >> introduce that property.
> >
> > Although the above commit does not add this property, it has
> > implemented the entire feature except for the property so thought
> > sending this with "Fixes:" while CCing  stable@ will allow the
> > backport.  I am interested in having this patch in older kernel
>
> Not implementing DT bindings is not a bug. Otherwise provide any sort of
> proof that this was not intentional.



>
> I can easily provide you proof why this was intentional: negative review
> maintainers.
>
>
> > versions as well where imod support has been added. Wondering what
> > would be the right way to achieve this. Eager to know your thoughts !
>
> So again, downstream and forks... NAK, you cannot push things to stable
> just because you want them backported by Greg.
>
> This is not acceptable.

Hi Krzysztof,

I totally agree that this is not a bug but the intention here is to
not call it a bug but rather to land this in older versions of the
kernel as well which I wasn't sure how to do and I was perceiving
"Fixes:" while CCing  stable@ as a way to do that.
I will drop "Fixes:" and the CC to  stable@.  Came across the following
https://docs.kernel.org/process/backporting.html#submitting-backports-to-stable.I
will follow this and explicitly submit backports. Let me know if that
isn't reasonable

>
> >
> >>
> >>
> >>> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> >>> ---
> >>>  .../devicetree/bindings/usb/snps,dwc3-common.yaml   | 13 +++++++++++++
> >>>  1 file changed, 13 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
> >>> index c956053fd036..3957f1dac3c4 100644
> >>> --- a/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
> >>> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
> >>> @@ -375,6 +375,19 @@ properties:
> >>>      items:
> >>>        enum: [1, 4, 8, 16, 32, 64, 128, 256]
> >>>
> >>> +  snps,device-mode-intrpt-mod-interval:
> >>> +    description:
> >>> +      Specifies the minimum inter-interrupt interval in 250ns increments
> >>
> >> Then use proper property unit suffix.
> >
> > Ack ! changing to snps,device-mode-intrpt-mod-interval-ns in V2.
> >
> >>
> >>> +      during device mode operation. A value of 0 disables the interrupt
> >>> +      throttling logic and interrupts are generated immediately if event
> >>> +      count becomes non-zero. Otherwise, the interrupt is signaled when
> >>> +      all of the following conditons are met which are, EVNT_HANDLER_BUSY
> >>> +      is 0, event count is non-zero and at least 250ns increments of this
> >>> +      value has elapsed since the last time interrupt was de-asserted.
> >>
> >> Why is this property of a board? Why different boards would wait
> >> different amount of time?
> >
> > Interrupt moderation allows batch processing of events reported by the
> > controller.
> > A very low value of snps,device-mode-intrpt-mod-interval-ns implies
> > that the controller will interrupt more often to make the host
> > processor process a smaller set of events Vs a larger value will wake
> > up the host processor at longer intervals to process events (likely
> > more). So depending what the board is designed for this can be tuned
> > to achieve the needed outcome.
>
> I do not see dependency on the board. Host has the same CPU always, so
> it processes with the same speed.

 By "host processor", I am referring to the CPU which is scheduling
the TRBs and responding to the events reported by the Synopsys DWC3
controller. In a nutshell, the CPU which is driving the  Synopsys DWC3
controller. The Synopsys DWC3 controller could be paired with any CPU
configuration and therefore is "Host has the same CPU always" a fair
assumption to be made ?

As a reference,
https://lore.kernel.org/linux-arm-kernel/9cb2e5fc-1bec-c19c-c04e-fe56e5c1bc16@codeaurora.org/T/#m392cc1fe604499984c61ac07dacc840616194efe
is the first patch which introduces IMOD as board specific property
for XHCI based controllers.

Thanks,
Badhri

>
>
> Best regards,
> Krzysztof

Re: [PATCH v1 1/2] dt-bindings: usb: snps,dwc3: Add property for imod

[Krzysztof Kozlowski]

On 05/02/2025 19:19, Badhri Jagan Sridharan wrote:
>>>>
>>>>> +      during device mode operation. A value of 0 disables the interrupt
>>>>> +      throttling logic and interrupts are generated immediately if event
>>>>> +      count becomes non-zero. Otherwise, the interrupt is signaled when
>>>>> +      all of the following conditons are met which are, EVNT_HANDLER_BUSY
>>>>> +      is 0, event count is non-zero and at least 250ns increments of this
>>>>> +      value has elapsed since the last time interrupt was de-asserted.
>>>>
>>>> Why is this property of a board? Why different boards would wait
>>>> different amount of time?
>>>
>>> Interrupt moderation allows batch processing of events reported by the
>>> controller.
>>> A very low value of snps,device-mode-intrpt-mod-interval-ns implies
>>> that the controller will interrupt more often to make the host
>>> processor process a smaller set of events Vs a larger value will wake
>>> up the host processor at longer intervals to process events (likely
>>> more). So depending what the board is designed for this can be tuned
>>> to achieve the needed outcome.
>>
>> I do not see dependency on the board. Host has the same CPU always, so
>> it processes with the same speed.
> 
>  By "host processor", I am referring to the CPU which is scheduling
> the TRBs and responding to the events reported by the Synopsys DWC3
> controller. In a nutshell, the CPU which is driving the  Synopsys DWC3
> controller. The Synopsys DWC3 controller could be paired with any CPU
> configuration and therefore is "Host has the same CPU always" a fair
> assumption to be made ?

Not really, this is part of a SoC, so DWC3 controller is always here
fixed per given setup with given CPU. You claim that this is independent
of SoC, but so far arguments do not support that statement. This is
related to given SoC, so no need for this property and you can deduce
everything from SoC.

You push this as a property because you (or vendor) do not want to
upstream your SoC. That's common pattern, seen here many times. BTW,
good counter argument would be to show me patches for upstream DTS users
of this. Actually that would be very easy way to finish discussion...
but there are no patches, right? Why? Because it is not upstream and it
is done for downstream solution. Sorry, no. Develop code how upstream
develops, not downstream.

Best regards,
Krzysztof

Re: [PATCH v1 1/2] dt-bindings: usb: snps,dwc3: Add property for imod

[Badhri Jagan Sridharan]

On Wed, Feb 5, 2025 at 11:35 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 05/02/2025 19:19, Badhri Jagan Sridharan wrote:
> >>>>
> >>>>> +      during device mode operation. A value of 0 disables the interrupt
> >>>>> +      throttling logic and interrupts are generated immediately if event
> >>>>> +      count becomes non-zero. Otherwise, the interrupt is signaled when
> >>>>> +      all of the following conditons are met which are, EVNT_HANDLER_BUSY
> >>>>> +      is 0, event count is non-zero and at least 250ns increments of this
> >>>>> +      value has elapsed since the last time interrupt was de-asserted.
> >>>>
> >>>> Why is this property of a board? Why different boards would wait
> >>>> different amount of time?
> >>>
> >>> Interrupt moderation allows batch processing of events reported by the
> >>> controller.
> >>> A very low value of snps,device-mode-intrpt-mod-interval-ns implies
> >>> that the controller will interrupt more often to make the host
> >>> processor process a smaller set of events Vs a larger value will wake
> >>> up the host processor at longer intervals to process events (likely
> >>> more). So depending what the board is designed for this can be tuned
> >>> to achieve the needed outcome.
> >>
> >> I do not see dependency on the board. Host has the same CPU always, so
> >> it processes with the same speed.
> >
> >  By "host processor", I am referring to the CPU which is scheduling
> > the TRBs and responding to the events reported by the Synopsys DWC3
> > controller. In a nutshell, the CPU which is driving the  Synopsys DWC3
> > controller. The Synopsys DWC3 controller could be paired with any CPU
> > configuration and therefore is "Host has the same CPU always" a fair
> > assumption to be made ?
>
> Not really, this is part of a SoC, so DWC3 controller is always here
> fixed per given setup with given CPU. You claim that this is independent
> of SoC, but so far arguments do not support that statement. This is
> related to given SoC, so no need for this property and you can deduce
> everything from SoC.
>
> You push this as a property because you (or vendor) do not want to
> upstream your SoC. That's common pattern, seen here many times. BTW,
> good counter argument would be to show me patches for upstream DTS users
> of this. Actually that would be very easy way to finish discussion...
> but there are no patches, right? Why? Because it is not upstream and it
> is done for downstream solution. Sorry, no. Develop code how upstream
> develops, not downstream.

Thanks for persisting in explaining this Krzysztof ! Much appreciated
! I finally understand your perspective.
What I missed to understand is that when
https://lore.kernel.org/linux-arm-kernel/9cb2e5fc-1bec-c19c-c04e-fe56e5c1bc16@codeaurora.org/T/#m392cc1fe604499984c61ac07dacc840616194efe
added "imod-interval-ns", it also added how the property was used. I
will look into upstreaming the SOC driver which makes use of the
property as well.
We can drop this patch till then.

>>> +++ b/Documentation/devicetree/bindings/usb/mediatek,mtk-xhci.txt
>>> @@ -46,6 +46,7 @@ Optional properties:
>>>    - pinctrl-names : a pinctrl state named "default" must be defined
>>>    - pinctrl-0 : pin control group
>>>   See: Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
>>> + - imod-interval-ns: default interrupt moderation interval is 5000ns
>>>
>>>   Example:
>>>   usb30: usb at 11270000 {
>>> @@ -66,6 +67,7 @@ usb30: usb at 11270000 {
>>>   usb3-lpm-capable;
>>>   mediatek,syscon-wakeup = <&pericfg>;
>>>   mediatek,wakeup-src = <1>;
>>> + imod-interval-ns = <10000>;
>>>   };


Thanks,
Badhri

>
> Best regards,
> Krzysztof