* [PATCH 2/3] dt-bindings: net: snps,dwmac: Add time-based-scheduling property [not found] <b365dc6f756a3fad4dfaa2675c98f4078aba8a55.1706105494.git.esben@geanix.com> @ 2024-01-24 14:33 ` Esben Haabendal 2024-01-24 16:07 ` Conor Dooley 2024-01-30 21:39 ` Rob Herring 0 siblings, 2 replies; 8+ messages in thread From: Esben Haabendal @ 2024-01-24 14:33 UTC (permalink / raw) To: devicetree, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Alexandre Torgue, Giuseppe Cavallaro, Jose Abreu Cc: netdev, linux-kernel Time Based Scheduling can be enabled per TX queue, if supported by the controller. Signed-off-by: Esben Haabendal <esben@geanix.com> --- Documentation/devicetree/bindings/net/snps,dwmac.yaml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml index 5c2769dc689a..301e9150ecc3 100644 --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml @@ -399,6 +399,12 @@ properties: type: boolean description: TX checksum offload is unsupported by the TX queue. + snps,time-based-scheduling: + type: boolean + description: + Time Based Scheduling will be enabled for TX queue. + This is typically not supported for TX queue 0. + allOf: - if: required: -- 2.43.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] dt-bindings: net: snps,dwmac: Add time-based-scheduling property 2024-01-24 14:33 ` [PATCH 2/3] dt-bindings: net: snps,dwmac: Add time-based-scheduling property Esben Haabendal @ 2024-01-24 16:07 ` Conor Dooley 2024-01-25 9:10 ` esben 2024-01-30 21:39 ` Rob Herring 1 sibling, 1 reply; 8+ messages in thread From: Conor Dooley @ 2024-01-24 16:07 UTC (permalink / raw) To: Esben Haabendal Cc: devicetree, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Alexandre Torgue, Giuseppe Cavallaro, Jose Abreu, netdev, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1522 bytes --] On Wed, Jan 24, 2024 at 03:33:06PM +0100, Esben Haabendal wrote: > Time Based Scheduling can be enabled per TX queue, if supported by the > controller. If time based scheduling is not supported by the controller, then the property should not be present! The presence of a property like this should mean that the feature is supported, using it is up to the operating system. That said, why is this a property that should be in DT? If support is per controller is it not sufficient to use the compatible to determine if this is supported? Thanks, Conor. > > Signed-off-by: Esben Haabendal <esben@geanix.com> > --- > Documentation/devicetree/bindings/net/snps,dwmac.yaml | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml > index 5c2769dc689a..301e9150ecc3 100644 > --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml > +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml > @@ -399,6 +399,12 @@ properties: > type: boolean > description: TX checksum offload is unsupported by the TX queue. > > + snps,time-based-scheduling: > + type: boolean > + description: > + Time Based Scheduling will be enabled for TX queue. > + This is typically not supported for TX queue 0. > + > allOf: > - if: > required: > -- > 2.43.0 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] dt-bindings: net: snps,dwmac: Add time-based-scheduling property 2024-01-24 16:07 ` Conor Dooley @ 2024-01-25 9:10 ` esben 2024-01-25 9:19 ` Krzysztof Kozlowski 0 siblings, 1 reply; 8+ messages in thread From: esben @ 2024-01-25 9:10 UTC (permalink / raw) To: Conor Dooley Cc: devicetree, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Alexandre Torgue, Giuseppe Cavallaro, Jose Abreu, netdev, linux-kernel Conor Dooley <conor@kernel.org> writes: > On Wed, Jan 24, 2024 at 03:33:06PM +0100, Esben Haabendal wrote: >> Time Based Scheduling can be enabled per TX queue, if supported by the >> controller. > > If time based scheduling is not supported by the controller, then the > property should not be present! The presence of a property like this > should mean that the feature is supported, using it is up to the > operating system. > > That said, why is this a property that should be in DT? It is added to the tx-queues-config object of snps,dwmac bindings. This entire object is about configuration of the ethernet controller, which is also what the purpose of the snps,time-based-scheduling. So yes, it is not specifically about describing what the hardware is capable of, but how the hardware is configured. It is a continuation of the current driver design. > If support is per controller is it not sufficient to use the > compatible to determine if this is supported? Are you suggesting to include the mapping from all supported compatible controllers to which TX queues supports TBS in the driver code? What would the benefit of that compared to describing it explicitly in the binding? And for the purpose of the above question, I am talking about it as if the binding was describing the hardware capability and not the configuration. /Esben >> Signed-off-by: Esben Haabendal <esben@geanix.com> >> --- >> Documentation/devicetree/bindings/net/snps,dwmac.yaml | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml >> index 5c2769dc689a..301e9150ecc3 100644 >> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml >> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml >> @@ -399,6 +399,12 @@ properties: >> type: boolean >> description: TX checksum offload is unsupported by the TX queue. >> >> + snps,time-based-scheduling: >> + type: boolean >> + description: >> + Time Based Scheduling will be enabled for TX queue. >> + This is typically not supported for TX queue 0. >> + >> allOf: >> - if: >> required: >> -- >> 2.43.0 >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] dt-bindings: net: snps,dwmac: Add time-based-scheduling property 2024-01-25 9:10 ` esben @ 2024-01-25 9:19 ` Krzysztof Kozlowski 2024-01-25 11:55 ` esben 0 siblings, 1 reply; 8+ messages in thread From: Krzysztof Kozlowski @ 2024-01-25 9:19 UTC (permalink / raw) To: esben, Conor Dooley Cc: devicetree, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Alexandre Torgue, Giuseppe Cavallaro, Jose Abreu, netdev, linux-kernel On 25/01/2024 10:10, esben@geanix.com wrote: > Conor Dooley <conor@kernel.org> writes: > >> On Wed, Jan 24, 2024 at 03:33:06PM +0100, Esben Haabendal wrote: >>> Time Based Scheduling can be enabled per TX queue, if supported by the >>> controller. >> >> If time based scheduling is not supported by the controller, then the >> property should not be present! The presence of a property like this >> should mean that the feature is supported, using it is up to the >> operating system. >> >> That said, why is this a property that should be in DT? > > It is added to the tx-queues-config object of snps,dwmac bindings. This > entire object is about configuration of the ethernet controller, which > is also what the purpose of the snps,time-based-scheduling. > So yes, it is not specifically about describing what the hardware is > capable of, but how the hardware is configured. It is a continuation of > the current driver design. > >> If support is per controller is it not sufficient to use the >> compatible to determine if this is supported? > > Are you suggesting to include the mapping from all supported compatible > controllers to which TX queues supports TBS in the driver code? What > would the benefit of that compared to describing it explicitly in the > binding? The benefit is complying with DT bindings rules, saying that bindings describe hardware pieces, not drivers. > And for the purpose of the above question, I am talking about it as if > the binding was describing the hardware capability and not the > configuration. "if"? You wrote it is for driver design... Best regards, Krzysztof ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] dt-bindings: net: snps,dwmac: Add time-based-scheduling property 2024-01-25 9:19 ` Krzysztof Kozlowski @ 2024-01-25 11:55 ` esben 2024-01-25 17:14 ` Conor Dooley 0 siblings, 1 reply; 8+ messages in thread From: esben @ 2024-01-25 11:55 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Conor Dooley, devicetree, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Alexandre Torgue, Giuseppe Cavallaro, Jose Abreu, netdev, linux-kernel Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes: > On 25/01/2024 10:10, esben@geanix.com wrote: >> Conor Dooley <conor@kernel.org> writes: >> >>> On Wed, Jan 24, 2024 at 03:33:06PM +0100, Esben Haabendal wrote: >>>> Time Based Scheduling can be enabled per TX queue, if supported by the >>>> controller. >>> >>> If time based scheduling is not supported by the controller, then the >>> property should not be present! The presence of a property like this >>> should mean that the feature is supported, using it is up to the >>> operating system. >>> >>> That said, why is this a property that should be in DT? >> >> It is added to the tx-queues-config object of snps,dwmac bindings. This >> entire object is about configuration of the ethernet controller, which >> is also what the purpose of the snps,time-based-scheduling. >> So yes, it is not specifically about describing what the hardware is >> capable of, but how the hardware is configured. It is a continuation of >> the current driver design. >> >>> If support is per controller is it not sufficient to use the >>> compatible to determine if this is supported? >> >> Are you suggesting to include the mapping from all supported compatible >> controllers to which TX queues supports TBS in the driver code? What >> would the benefit of that compared to describing it explicitly in the >> binding? > > The benefit is complying with DT bindings rules, saying that bindings > describe hardware pieces, not drivers. Understood. >> And for the purpose of the above question, I am talking about it as if >> the binding was describing the hardware capability and not the >> configuration. > > "if"? You wrote it is for driver design... If you look at the current driver, all the devicetree bindings under rx-queues-config and tx-queues-config are violating the DT binding rules. Cleaning up that requires quite some work and I guess will break backwards compatibility to some extend. But that is another story. I will respin the patch according to Conor's suggestion. /Esben ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] dt-bindings: net: snps,dwmac: Add time-based-scheduling property 2024-01-25 11:55 ` esben @ 2024-01-25 17:14 ` Conor Dooley 0 siblings, 0 replies; 8+ messages in thread From: Conor Dooley @ 2024-01-25 17:14 UTC (permalink / raw) To: esben Cc: Krzysztof Kozlowski, devicetree, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Alexandre Torgue, Giuseppe Cavallaro, Jose Abreu, netdev, linux-kernel [-- Attachment #1: Type: text/plain, Size: 2297 bytes --] On Thu, Jan 25, 2024 at 12:55:12PM +0100, esben@geanix.com wrote: > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes: > > > On 25/01/2024 10:10, esben@geanix.com wrote: > >> Conor Dooley <conor@kernel.org> writes: > >> > >>> On Wed, Jan 24, 2024 at 03:33:06PM +0100, Esben Haabendal wrote: > >>>> Time Based Scheduling can be enabled per TX queue, if supported by the > >>>> controller. > >>> > >>> If time based scheduling is not supported by the controller, then the > >>> property should not be present! The presence of a property like this > >>> should mean that the feature is supported, using it is up to the > >>> operating system. > >>> > >>> That said, why is this a property that should be in DT? > >> > >> It is added to the tx-queues-config object of snps,dwmac bindings. This > >> entire object is about configuration of the ethernet controller, which > >> is also what the purpose of the snps,time-based-scheduling. > >> So yes, it is not specifically about describing what the hardware is > >> capable of, but how the hardware is configured. It is a continuation of > >> the current driver design. > >> > >>> If support is per controller is it not sufficient to use the > >>> compatible to determine if this is supported? > >> > >> Are you suggesting to include the mapping from all supported compatible > >> controllers to which TX queues supports TBS in the driver code? What > >> would the benefit of that compared to describing it explicitly in the > >> binding? > > > > The benefit is complying with DT bindings rules, saying that bindings > > describe hardware pieces, not drivers. > > Understood. > > >> And for the purpose of the above question, I am talking about it as if > >> the binding was describing the hardware capability and not the > >> configuration. > > > > "if"? You wrote it is for driver design... > > If you look at the current driver, all the devicetree bindings under > rx-queues-config and tx-queues-config are violating the DT binding > rules. > Cleaning up that requires quite some work and I guess will break > backwards compatibility to some extend. Let bygones be bygones. If something undesirable got in previously, breaking backwards compatibility there is not justified IMO. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] dt-bindings: net: snps,dwmac: Add time-based-scheduling property 2024-01-24 14:33 ` [PATCH 2/3] dt-bindings: net: snps,dwmac: Add time-based-scheduling property Esben Haabendal 2024-01-24 16:07 ` Conor Dooley @ 2024-01-30 21:39 ` Rob Herring 2024-01-31 7:31 ` Esben Haabendal 1 sibling, 1 reply; 8+ messages in thread From: Rob Herring @ 2024-01-30 21:39 UTC (permalink / raw) To: Esben Haabendal Cc: devicetree, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Krzysztof Kozlowski, Conor Dooley, Alexandre Torgue, Giuseppe Cavallaro, Jose Abreu, netdev, linux-kernel On Wed, Jan 24, 2024 at 03:33:06PM +0100, Esben Haabendal wrote: > Time Based Scheduling can be enabled per TX queue, if supported by the > controller. > > Signed-off-by: Esben Haabendal <esben@geanix.com> > --- > Documentation/devicetree/bindings/net/snps,dwmac.yaml | 6 ++++++ > 1 file changed, 6 insertions(+) This is not v1 which you are aware. Where's the justification or do I need to ask the same questions again? Here's the last discussion[1]. I'm still not clear on why this is needed. Seems like the combination of TBS and TSO capabilities provides enough information. If TSO is enabled for a queue, then don't enable TBS. This binding is already such a mess of properties, I'm inclined to say "what's one more", but it's death by 1000 cuts. Part of the problem is this binding is for not 1 IP block, but something that's evolved over at least 15 years. The question on configuration properties really comes down to who would configure things and when. If it's one time for the life of given h/w, then DT makes sense. If every user wants/needs to tweak the setting, then definitely shouldn't be in DT. Somewhere in the middle? Judgement call. > diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml > index 5c2769dc689a..301e9150ecc3 100644 > --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml > +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml > @@ -399,6 +399,12 @@ properties: > type: boolean > description: TX checksum offload is unsupported by the TX queue. > > + snps,time-based-scheduling: > + type: boolean > + description: > + Time Based Scheduling will be enabled for TX queue. > + This is typically not supported for TX queue 0. Make the property name clear it is an enable, not a capability. > + [1] https://lore.kernel.org/all/20230929051758.21492-1-rohan.g.thomas@intel.com/ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] dt-bindings: net: snps,dwmac: Add time-based-scheduling property 2024-01-30 21:39 ` Rob Herring @ 2024-01-31 7:31 ` Esben Haabendal 0 siblings, 0 replies; 8+ messages in thread From: Esben Haabendal @ 2024-01-31 7:31 UTC (permalink / raw) To: Rob Herring Cc: devicetree, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Krzysztof Kozlowski, Conor Dooley, Alexandre Torgue, Giuseppe Cavallaro, Jose Abreu, netdev, linux-kernel Rob Herring <robh@kernel.org> writes: > On Wed, Jan 24, 2024 at 03:33:06PM +0100, Esben Haabendal wrote: >> Time Based Scheduling can be enabled per TX queue, if supported by the >> controller. >> >> Signed-off-by: Esben Haabendal <esben@geanix.com> >> --- >> Documentation/devicetree/bindings/net/snps,dwmac.yaml | 6 ++++++ >> 1 file changed, 6 insertions(+) > > This is not v1 which you are aware. Where's the justification or do I > need to ask the same questions again? Here's the last discussion[1]. Yes, I am aware. I must admit I only spotted the last discussion you are referring to after submitting my version of it. Sorry about that. > I'm still not clear on why this is needed. Seems like the combination > of TBS and TSO capabilities provides enough information. If TSO is > enabled for a queue, then don't enable TBS. > This binding is already such a mess of properties, I'm inclined to say > "what's one more", but it's death by 1000 cuts. Part of the problem is > this binding is for not 1 IP block, but something that's evolved over > at least 15 years. It definitely is a mess. A lot of these properties are not the type of properties that I think would be accepted today, as there is a lot of configuration like properties there. > The question on configuration properties really comes down to who > would configure things and when. If it's one time for the life of > given h/w, then DT makes sense. If every user wants/needs to tweak the > setting, then definitely shouldn't be in DT. Somewhere in the middle? > Judgement call. Some of the existsing configuration properties in there is something that users will need to tweak, such as the selection of queue scheduling and priority algorithms. The TBS vs TSO is probably somewhere in the middle. It might just be that choosing TSO for TX queue 0, and TBS for the remaining ones are something that everybody can agree on. But I am not really sure about that. I think we should drop this binding. I have found another simple solution for i.MX, which does not involve new bindings. https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=3b12ec8f618e Improving on that, I think we should make switching between TSO and TBS a run-time configurable thing, instead of creating binding for it. But I am unsure if that is really worth it. /Esben > >> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml >> index 5c2769dc689a..301e9150ecc3 100644 >> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml >> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml >> @@ -399,6 +399,12 @@ properties: >> type: boolean >> description: TX checksum offload is unsupported by the TX queue. >> >> + snps,time-based-scheduling: >> + type: boolean >> + description: >> + Time Based Scheduling will be enabled for TX queue. >> + This is typically not supported for TX queue 0. > > Make the property name clear it is an enable, not a capability. > >> + > > [1] https://lore.kernel.org/all/20230929051758.21492-1-rohan.g.thomas@intel.com/ ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-01-31 7:31 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <b365dc6f756a3fad4dfaa2675c98f4078aba8a55.1706105494.git.esben@geanix.com>
2024-01-24 14:33 ` [PATCH 2/3] dt-bindings: net: snps,dwmac: Add time-based-scheduling property Esben Haabendal
2024-01-24 16:07 ` Conor Dooley
2024-01-25 9:10 ` esben
2024-01-25 9:19 ` Krzysztof Kozlowski
2024-01-25 11:55 ` esben
2024-01-25 17:14 ` Conor Dooley
2024-01-30 21:39 ` Rob Herring
2024-01-31 7:31 ` Esben Haabendal
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).