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@xxxxxxxxxx> 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@xxxxxxxxxxxxxx/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