On Wed, Feb 5, 2025 at 5:50 AM Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote: > > On 05/02/2025 10:07, Badhri Jagan Sridharan wrote: > > . > > > > On Sun, Feb 2, 2025 at 6:11 AM Krzysztof Kozlowski <krzk@xxxxxxxxxx> 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@xxxxxxxxxx > >>> 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@xxxxxxxxxx> > >>> --- > >>> .../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@xxxxxxxxxxxxxx/T/#m392cc1fe604499984c61ac07dacc840616194efe is the first patch which introduces IMOD as board specific property for XHCI based controllers. Thanks, Badhri > > > Best regards, > Krzysztof
