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

[Krzysztof Kozlowski <krzk@kernel.org>]

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