Add new property tbs-enabled to enable Time Based Scheduling(TBS)
support per Tx queues. TBS feature can be enabled later using ETF
qdisc but for only those queues that have TBS support enabled.
Commit 7eadf57290ec ("net: stmmac: pci: Enable TBS on GMAC5 IPK PCI
entry") enables similar support from the stmmac pci driver.
Signed-off-by: Rohan G Thomas <[email protected]>
---
Documentation/devicetree/bindings/net/snps,dwmac.yaml | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
index 5c2769dc689a..db1eb0997602 100644
--- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
+++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
@@ -399,6 +399,14 @@ properties:
type: boolean
description: TX checksum offload is unsupported by the TX queue.
+ snps,tbs-enabled:
+ type: boolean
+ description:
+ Enable Time Based Scheduling(TBS) support for the TX queue. TSO and
+ TBS cannot be supported by a queue at the same time. If TSO support
+ is enabled, then default TX queue 0 for TSO and in that case don't
+ enable TX queue 0 for TBS.
+
allOf:
- if:
required:
--
2.26.2
On Wed, Sep 27, 2023 at 09:09:18PM +0800, Rohan G Thomas wrote:
> Add new property tbs-enabled to enable Time Based Scheduling(TBS)
That's not the property you added.
> support per Tx queues. TBS feature can be enabled later using ETF
> qdisc but for only those queues that have TBS support enabled.
This property defines capable or enabled?
Seems like OS configuration and policy.
Doesn't eh DWMAC have capability registers for supported features? Or
did they forget per queue capabilities?
>
> Commit 7eadf57290ec ("net: stmmac: pci: Enable TBS on GMAC5 IPK PCI
> entry") enables similar support from the stmmac pci driver.
Why does unconditionally enabling TBS work there, but not here?
>
> Signed-off-by: Rohan G Thomas <[email protected]>
> ---
> Documentation/devicetree/bindings/net/snps,dwmac.yaml | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> index 5c2769dc689a..db1eb0997602 100644
> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> @@ -399,6 +399,14 @@ properties:
> type: boolean
> description: TX checksum offload is unsupported by the TX queue.
>
> + snps,tbs-enabled:
> + type: boolean
> + description:
> + Enable Time Based Scheduling(TBS) support for the TX queue. TSO and
> + TBS cannot be supported by a queue at the same time. If TSO support
> + is enabled, then default TX queue 0 for TSO and in that case don't
> + enable TX queue 0 for TBS.
> +
> allOf:
> - if:
> required:
> --
> 2.26.2
>
From: Rohan G Thomas <[email protected]>
On Wed, Sep 27, 2023 at 09:09:18PM +0800, Rohan G Thomas wrote:
>> Add new property tbs-enabled to enable Time Based Scheduling(TBS)
>
>That's not the property you added.
>
>> support per Tx queues. TBS feature can be enabled later using ETF
>> qdisc but for only those queues that have TBS support enabled.
>
>This property defines capable or enabled?
This property is to enable TBS support for any Tx queue. Why this
added is because I think TBS need not be enabled for all capable
Tx queues(Tx DMA channels) because of the following hw limitations.
1. As per DWMAC QoS and DWXGMAC databooks, TBS cannot coexist with
TSO. So TBS cannot be enabled for a Tx queue which is for TSO.
2. Also as per DWXGMAC databook, "Do not enable time-based scheduling
(or enhanced descriptors) for the channel for which TSO or transmit
timestamp or one-step timestamping control correction feature is
enabled".
3. As per DWXGMAC databook, "Use separate channel (without TBS
enabled) for time critical traffic. Mixing such traffic with TBS
enabled traffic can cause delays in transmitting time critical
traffic."
More explanation below...
>
>Seems like OS configuration and policy.
Tx queues need to be configured for TBS during hw setup itself as
special enhanced descriptors are used by the hw for TBS support
enabled queues. Switching between enhanced and normal descriptors
on run is not feasible. So this flag is for enabling "Enhanced
Descriptors for Time Based Scheduling". This I think is a hw specific
requirement.
>
>Doesn't eh DWMAC have capability registers for supported features? Or
>did they forget per queue capabilities?
Yes, capability registers are available. For DWMAC5 IP, if TBSSEL bit
is set, then TBS is supported by all Tx queues. For DWXGMAC IP, if
TBSSEL bit is set, then TBS is supported by TBS_CH number of Tx
queues starting from the highest Tx queue. But because of the hw
limitations mentioned above, TBS cannot be enabled for all capable
queues.
>
>>
>> Commit 7eadf57290ec ("net: stmmac: pci: Enable TBS on GMAC5 IPK PCI
>> entry") enables similar support from the stmmac pci driver.
>
>Why does unconditionally enabling TBS work there, but not here?
There, Tx queue 0 is not enabled for TBS as it is used for TSO.
>
>>
>> Signed-off-by: Rohan G Thomas <[email protected]>
>> ---
>> Documentation/devicetree/bindings/net/snps,dwmac.yaml | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/>devicetree/bindings/net/snps,dwmac.yaml
>> index 5c2769dc689a..db1eb0997602 100644
>> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>> @@ -399,6 +399,14 @@ properties:
>> type: boolean
>> description: TX checksum offload is unsupported by the TX queue.
>>
>> + snps,tbs-enabled:
>> + type: boolean
>> + description:
>> + Enable Time Based Scheduling(TBS) support for the TX queue. TSO and
>> + TBS cannot be supported by a queue at the same time. If TSO support
>> + is enabled, then default TX queue 0 for TSO and in that case don't
>> + enable TX queue 0 for TBS.
>> +
>> allOf:
>> - if:
>> required:
>> --
>> 2.26.2
>>
[email protected] writes:
> From: Rohan G Thomas <[email protected]>
>
> On Wed, Sep 27, 2023 at 09:09:18PM +0800, Rohan G Thomas wrote:
>>> Add new property tbs-enabled to enable Time Based Scheduling(TBS)
>>
>>That's not the property you added.
>>
>>> support per Tx queues. TBS feature can be enabled later using ETF
>>> qdisc but for only those queues that have TBS support enabled.
>>
>>This property defines capable or enabled?
>
> This property is to enable TBS support for any Tx queue. Why this
> added is because I think TBS need not be enabled for all capable
> Tx queues(Tx DMA channels) because of the following hw limitations.
> 1. As per DWMAC QoS and DWXGMAC databooks, TBS cannot coexist with
> TSO. So TBS cannot be enabled for a Tx queue which is for TSO.
> 2. Also as per DWXGMAC databook, "Do not enable time-based scheduling
> (or enhanced descriptors) for the channel for which TSO or transmit
> timestamp or one-step timestamping control correction feature is
> enabled".
> 3. As per DWXGMAC databook, "Use separate channel (without TBS
> enabled) for time critical traffic. Mixing such traffic with TBS
> enabled traffic can cause delays in transmitting time critical
> traffic."
> More explanation below...
>
>>
>>Seems like OS configuration and policy.
>
> Tx queues need to be configured for TBS during hw setup itself as
> special enhanced descriptors are used by the hw for TBS support
> enabled queues. Switching between enhanced and normal descriptors on
> run is not feasible. So this flag is for enabling "Enhanced
> Descriptors for Time Based Scheduling". This I think is a hw specific
> requirement.
Support for enhanced descriptors is definitely hardware specific.
Enabling the use of enhanced descriptors is a configuration choice.
The tricky part here is that the whole devicetree bindings story for the
stmmac driver is filled with such configuration choices. As such, it is
only natural to add the property you are suggesting here. I completely
agree. But you can also argue that it is "wrong", because it does not
just describe the hardware, but also a configuration choice.
>>Doesn't eh DWMAC have capability registers for supported features? Or
>>did they forget per queue capabilities?
>
> Yes, capability registers are available. For DWMAC5 IP, if TBSSEL bit
> is set, then TBS is supported by all Tx queues.
Not true. Some NXP imx8 and imx9 chips support Synopsys MAC 5.10a IP,
and does not support TBS for queue 0. And they have TBSSEL bit set, but
no TBS_CH support.
> For DWXGMAC IP, if TBSSEL bit is set, then TBS is supported by TBS_CH
> number of Tx queues starting from the highest Tx queue. But because of
> the hw limitations mentioned above, TBS cannot be enabled for all
> capable queues.
>
>>
>>>
>>> Commit 7eadf57290ec ("net: stmmac: pci: Enable TBS on GMAC5 IPK PCI
>>> entry") enables similar support from the stmmac pci driver.
>>
>>Why does unconditionally enabling TBS work there, but not here?
>
> There, Tx queue 0 is not enabled for TBS as it is used for TSO.
>
>>
>>>
>>> Signed-off-by: Rohan G Thomas <[email protected]>
>>> ---
>>> Documentation/devicetree/bindings/net/snps,dwmac.yaml | 8 ++++++++
>>> 1 file changed, 8 insertions(+)
>>>
>> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/>devicetree/bindings/net/snps,dwmac.yaml
>>> index 5c2769dc689a..db1eb0997602 100644
>>> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>>> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>>> @@ -399,6 +399,14 @@ properties:
>>> type: boolean
>>> description: TX checksum offload is unsupported by the TX queue.
>>>
>>> + snps,tbs-enabled:
>>> + type: boolean
>>> + description:
>>> + Enable Time Based Scheduling(TBS) support for the TX queue. TSO and
>>> + TBS cannot be supported by a queue at the same time. If TSO support
>>> + is enabled, then default TX queue 0 for TSO and in that case don't
>>> + enable TX queue 0 for TBS.
>>> +
>>> allOf:
>>> - if:
>>> required:
>>> --
>>> 2.26.2
>>>
From: Rohan G Thomas <[email protected]>
On Fri, 26 Jan 2024 09:52:40 +0100, Esben Haabendal wrote:
Hi Esben,
Thanks for your comments. Like to get some clarification on a few
things.
> >>
> >>Seems like OS configuration and policy.
> >
> > Tx queues need to be configured for TBS during hw setup itself as
> > special enhanced descriptors are used by the hw for TBS support
> > enabled queues. Switching between enhanced and normal descriptors on
> > run is not feasible. So this flag is for enabling "Enhanced
> > Descriptors for Time Based Scheduling". This I think is a hw specific
> > requirement.
>
> Support for enhanced descriptors is definitely hardware specific.
> Enabling the use of enhanced descriptors is a configuration choice.
>
> The tricky part here is that the whole devicetree bindings story for the
> stmmac driver is filled with such configuration choices. As such, it is
> only natural to add the property you are suggesting here. I completely
> agree. But you can also argue that it is "wrong", because it does not
> just describe the hardware, but also a configuration choice.
Isn't this requirement of using enhanced tx desc instead of normal tx
desc to support TBS is specific to Synopsys IP? Switching from
normal desc to enhanced desc at the time of tc-etf qdisc offload
cannot be done without traffic disruption, which I don't think is
acceptable. Since this behavior is IP specific, can we consider
this as an OS configuration choice?
Agreed that this feature(use of enhanced desc) can be enabled from
glue drivers. But I added this dt property, thinking this feature is
specific and common to DWMAC core and we can enable this feature for
stmmac platform driver without a glue driver. If this is not
acceptable, I can think of doing this from the glue driver.
> >>Doesn't eh DWMAC have capability registers for supported features? Or
> >>did they forget per queue capabilities?
> >
> > Yes, capability registers are available. For DWMAC5 IP, if TBSSEL bit
> > is set, then TBS is supported by all Tx queues.
>
> Not true. Some NXP imx8 and imx9 chips support Synopsys MAC 5.10a IP,
> and does not support TBS for queue 0. And they have TBSSEL bit set, but
> no TBS_CH support.
AFAIU from Synopsys DWMAC5 Databook, all queues support TBS. But TBS
cannot coexist with TSO. So all glue drivers enabling TBS feature
avoid queue 0 to support TSO. Please correct me if I'm wrong.
>
> > For DWXGMAC IP, if TBSSEL bit is set, then TBS is supported by TBS_CH
> > number of Tx queues starting from the highest Tx queue. But because of
> > the hw limitations mentioned above, TBS cannot be enabled for all
> > capable queues.
> >
BR,
Rohan
On Sat, 27 Jan 2024 01:36:34 +0800 [email protected] wrote:
> > The tricky part here is that the whole devicetree bindings story for the
> > stmmac driver is filled with such configuration choices. As such, it is
> > only natural to add the property you are suggesting here. I completely
> > agree. But you can also argue that it is "wrong", because it does not
> > just describe the hardware, but also a configuration choice.
>
> Isn't this requirement of using enhanced tx desc instead of normal tx
> desc to support TBS is specific to Synopsys IP? Switching from
> normal desc to enhanced desc at the time of tc-etf qdisc offload
> cannot be done without traffic disruption, which I don't think is
> acceptable. Since this behavior is IP specific, can we consider
> this as an OS configuration choice?
Why is traffic disruption not acceptable when TBS gets turned on?
User has to install the right qdisc to enable TBS, I presume,
installing qdiscs destroys the old ones which also leads to packet
drops.
On Fri, 26 Jan 2024 12:19:28 -0800, Jakub Kicinski wrote:
> > > The tricky part here is that the whole devicetree bindings story for the
> > > stmmac driver is filled with such configuration choices. As such, it is
> > > only natural to add the property you are suggesting here. I completely
> > > agree. But you can also argue that it is "wrong", because it does not
> > > just describe the hardware, but also a configuration choice.
> >
> > Isn't this requirement of using enhanced tx desc instead of normal tx
> > desc to support TBS is specific to Synopsys IP? Switching from
> > normal desc to enhanced desc at the time of tc-etf qdisc offload
> > cannot be done without traffic disruption, which I don't think is
> > acceptable. Since this behavior is IP specific, can we consider
> > this as an OS configuration choice?
>
> Why is traffic disruption not acceptable when TBS gets turned on?
> User has to install the right qdisc to enable TBS, I presume,
> installing qdiscs destroys the old ones which also leads to packet
> drops.
Hi Jakub,
Agreed that packet loss is acceptable during qdisc install.
Sorry, I'm a little out of context in the above statements.
Switching between normal and enhanced desc is really not needed as
enhanced desc can support transmission of packets that don't have any
launch time also. So for any tbs supported queues we can enable
enhanced desc for tbs during stmmac_open itself.
As I mentioned in my previous reply:
> > Agreed that this feature(use of enhanced desc) can be enabled from
> > glue drivers. But I added this dt property, thinking this feature is
> > specific and common to DWMAC core and we can enable this feature for
> > stmmac platform driver without a glue driver. If this is not
> > acceptable, I can think of doing this from the glue driver.
Like Esben mentioned I think enabling tbs_en flag explicitly from the
glue driver is the way to enable tbs support for a tx-queue right now.
BR,
Rohan