| Message ID | 20250202035100.31235-1-badhri@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/2] dt-bindings: usb: snps,dwc3: Add property for imod | expand |
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
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
. 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 >
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
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(+) base-commit: 72deda0abee6e705ae71a93f69f55e33be5bca5c