Message-ID: <CAPTae5+xF0B64AhT5fjMU9tcW8cT9smO5eUD=Wpsiw2CKAhDAQ@mail.gmail.com>
Date: Wed, 5 Feb 2025 10:19:09 -0800
From: Badhri Jagan Sridharan <badhri@...gle.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Thinh.Nguyen@...opsys.com, gregkh@...uxfoundation.org, 
	felipe.balbi@...ux.intel.com, robh@...nel.org, krzk+dt@...nel.org, 
	conor+dt@...nel.org, johnyoun@...opsys.com, linux-usb@...r.kernel.org, 
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org, jameswei@...gle.com, 
	stable@...nel.org
Subject: Re: [PATCH v1 1/2] dt-bindings: usb: snps,dwc3: Add property for imod

On Wed, Feb 5, 2025 at 5:50 AM Krzysztof Kozlowski <krzk@...nel.org> wrote:
>
> On 05/02/2025 10:07, Badhri Jagan Sridharan wrote:
> > .
> >
> > On Sun, Feb 2, 2025 at 6:11 AM Krzysztof Kozlowski <krzk@...nel.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@...nel.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@...gle.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

Powered by blists - more mailing lists