[PATCH net-next 0/2] net: stmmac: TBS support for platform driver

[PATCH net-next 0/2] net: stmmac: TBS support for platform driver

[Rohan G Thomas]

Hi,
This patchset enable Time Based Scheduling(TBS) support for Tx queues
through the stmmac platform driver. For this a new per-queue
tx-config property, tbs-enabled is added to the devicetree.

Commit 7eadf57290ec ("net: stmmac: pci: Enable TBS on GMAC5 IPK PCI
entry") enables similar support for the stmmac pci driver.

Rohan G Thomas (2):
  dt-bindings: net: snps,dwmac: Time Based Scheduling
  net: stmmac: TBS support for platform driver

 .../devicetree/bindings/net/snps,dwmac.yaml   |  8 ++++++
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 25 +++++++++++++++----
 .../ethernet/stmicro/stmmac/stmmac_platform.c |  4 +++
 3 files changed, 32 insertions(+), 5 deletions(-)

-- 
2.26.2

[PATCH net-next 1/2] dt-bindings: net: snps,dwmac: Time Based Scheduling

[Rohan G Thomas]

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 <rohan.g.thomas@intel.com>
---
 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

Re: [PATCH net-next 1/2] dt-bindings: net: snps,dwmac: Time Based Scheduling

[Rob Herring]

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 <rohan.g.thomas@intel.com>
> ---
>  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
> 

Re: [PATCH net-next 1/2] dt-bindings: net: snps,dwmac: Time Based Scheduling

[rohan.g.thomas]

From: Rohan G Thomas <rohan.g.thomas@intel.com>

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 <rohan.g.thomas@intel.com>
>> ---
>>  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
>> 

Re: [PATCH net-next 1/2] dt-bindings: net: snps,dwmac: Time Based Scheduling

[Esben Haabendal]

rohan.g.thomas@intel.com writes:

> From: Rohan G Thomas <rohan.g.thomas@intel.com>
>
> 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 <rohan.g.thomas@intel.com>
>>> ---
>>>  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
>>> 

RE: [PATCH net-next 1/2] dt-bindings: net: snps,dwmac: Time Based Scheduling

[rohan.g.thomas]

From: Rohan G Thomas <rohan.g.thomas@intel.com>

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

Re: [PATCH net-next 1/2] dt-bindings: net: snps,dwmac: Time Based Scheduling

[Jakub Kicinski]

On Sat, 27 Jan 2024 01:36:34 +0800 rohan.g.thomas@intel.com 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.

RE: [PATCH net-next 1/2] dt-bindings: net: snps,dwmac: Time Based Scheduling

[Rohan G Thomas]

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

[PATCH net-next 2/2] net: stmmac: TBS support for platform driver

[Rohan G Thomas]

Enable Time Based Scheduling(TBS) support for Tx queues through the
stmmac platform driver. For this a new per-queue tx-config property,
tbs-enabled is added to the devicetree.

Commit 7eadf57290ec ("net: stmmac: pci: Enable TBS on GMAC5 IPK PCI
entry") enables similar support for the stmmac pci driver.

Also add check whether TBS support is available for a Tx DMA channel
before enabling TBS support for that Tx DMA channel.

Signed-off-by: Rohan G Thomas <rohan.g.thomas@intel.com>
---
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 25 +++++++++++++++----
 .../ethernet/stmicro/stmmac/stmmac_platform.c |  4 +++
 2 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 81b6f3ecdf92..7333f0640b3d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -3773,12 +3773,18 @@ stmmac_setup_dma_desc(struct stmmac_priv *priv, unsigned int mtu)
 		dma_conf->dma_rx_size = DMA_DEFAULT_RX_SIZE;
 
 	/* Earlier check for TBS */
-	for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++) {
-		struct stmmac_tx_queue *tx_q = &dma_conf->tx_queue[chan];
-		int tbs_en = priv->plat->tx_queues_cfg[chan].tbs_en;
+	if (priv->dma_cap.tbssel) {
+		/* TBS is available only for tbs_ch_num of Tx DMA channels,
+		 * starting from the highest Tx DMA channel.
+		 */
+		chan = priv->dma_cap.number_tx_channel - priv->dma_cap.tbs_ch_num;
+		for (; chan < priv->plat->tx_queues_to_use; chan++) {
+			struct stmmac_tx_queue *tx_q = &dma_conf->tx_queue[chan];
+			int tbs_en = priv->plat->tx_queues_cfg[chan].tbs_en;
 
-		/* Setup per-TXQ tbs flag before TX descriptor alloc */
-		tx_q->tbs |= tbs_en ? STMMAC_TBS_AVAIL : 0;
+			/* Setup per-TXQ tbs flag before TX descriptor alloc */
+			tx_q->tbs |= tbs_en ? STMMAC_TBS_AVAIL : 0;
+		}
 	}
 
 	ret = alloc_dma_desc_resources(priv, dma_conf);
@@ -7505,6 +7511,15 @@ int stmmac_dvr_probe(struct device *device,
 		}
 	}
 
+	/* If TBS feature is supported(i.e. tbssel is true), then at least 1 Tx
+	 * DMA channel supports TBS. So if tbs_ch_num is 0 and tbssel is true,
+	 * assume all Tx DMA channels support TBS. TBS_CH field, which gives
+	 * number of Tx DMA channels with TBS support is only available only for
+	 * DW xGMAC IP. For other DWMAC IPs all Tx DMA channels can support TBS.
+	 */
+	if (priv->dma_cap.tbssel && !priv->dma_cap.tbs_ch_num)
+		priv->dma_cap.tbs_ch_num = priv->dma_cap.number_tx_channel;
+
 	ndev->features |= ndev->hw_features | NETIF_F_HIGHDMA;
 	ndev->watchdog_timeo = msecs_to_jiffies(watchdog);
 #ifdef STMMAC_VLAN_TAG_USED
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index 843bd8804bfa..6c0191c84071 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -279,6 +279,10 @@ static int stmmac_mtl_setup(struct platform_device *pdev,
 		plat->tx_queues_cfg[queue].coe_unsupported =
 			of_property_read_bool(q_node, "snps,coe-unsupported");
 
+		/* Select TBS for supported queues */
+		plat->tx_queues_cfg[queue].tbs_en =
+			of_property_read_bool(q_node, "snps,tbs-enabled");
+
 		queue++;
 	}
 	if (queue != plat->tx_queues_to_use) {
-- 
2.26.2

Re: [PATCH net-next 2/2] net: stmmac: TBS support for platform driver

[Abhishek Chauhan (ABC)]

Qualcomm had similar discussions with respect to enabling of TBS for a particular queue. 
We had similar discussion on these terms yesterday with Redhat. Adding Andrew from Redhat here 

What we discovered as part of the discussions is listed below. 

1. Today upstream stmmac code is designed in such a way that TBS flag is put as
part of queue configurations(see below snippet) and as well know that stmmac queue 
configuration comes from the dtsi file. 

//ndo_open => stmmac_open
int tbs_en = priv->plat->tx_queues_cfg[chan].tbs_en;(comes from tx_queues_cfg)

/* Setup per-TXQ tbs flag before TX descriptor alloc */
tx_q->tbs |= tbs_en ? STMMAC_TBS_AVAIL : 0;

2. There is a no way to do this dynamically from user space because we don't have any 
API exposed which can do it from user space and also TBS rely on special descriptors 
aka enhanced desc this cannot be done run time and stmmac has to be aware of it before 
we do DMA/MAC/MTL start. To do this dynamically would only mean stopping DMA/MAC/MTL 
realloc resources for enhanced desc and the starting MAC/DMA/MTL. This means we are 
disrupting other traffic(By stopping MAC block).

3. I dont think there is a way we can enable this dynamically today. I would like upstream 
community to share your thoughts as well.

4. I agree with Rohan's patch here and want upstream community to accept it. This will allow
use to configure the queues where TBS needs to be enabled as hardcoding in the code unless upstream
has better way to this using userspace.

Please let us know if you think otherwise. 


On 9/27/2023 6:09 AM, Rohan G Thomas wrote:
> Enable Time Based Scheduling(TBS) support for Tx queues through the
> stmmac platform driver. For this a new per-queue tx-config property,
> tbs-enabled is added to the devicetree.
> 
> Commit 7eadf57290ec ("net: stmmac: pci: Enable TBS on GMAC5 IPK PCI
> entry") enables similar support for the stmmac pci driver.
> 
> Also add check whether TBS support is available for a Tx DMA channel
> before enabling TBS support for that Tx DMA channel.
> 
> Signed-off-by: Rohan G Thomas <rohan.g.thomas@intel.com>
> ---
>  .../net/ethernet/stmicro/stmmac/stmmac_main.c | 25 +++++++++++++++----
>  .../ethernet/stmicro/stmmac/stmmac_platform.c |  4 +++
>  2 files changed, 24 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 81b6f3ecdf92..7333f0640b3d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -3773,12 +3773,18 @@ stmmac_setup_dma_desc(struct stmmac_priv *priv, unsigned int mtu)
>  		dma_conf->dma_rx_size = DMA_DEFAULT_RX_SIZE;
>  
>  	/* Earlier check for TBS */
> -	for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++) {
> -		struct stmmac_tx_queue *tx_q = &dma_conf->tx_queue[chan];
> -		int tbs_en = priv->plat->tx_queues_cfg[chan].tbs_en;
> +	if (priv->dma_cap.tbssel) {
> +		/* TBS is available only for tbs_ch_num of Tx DMA channels,
> +		 * starting from the highest Tx DMA channel.
> +		 */
> +		chan = priv->dma_cap.number_tx_channel - priv->dma_cap.tbs_ch_num;
> +		for (; chan < priv->plat->tx_queues_to_use; chan++) {
> +			struct stmmac_tx_queue *tx_q = &dma_conf->tx_queue[chan];
> +			int tbs_en = priv->plat->tx_queues_cfg[chan].tbs_en;
>  
> -		/* Setup per-TXQ tbs flag before TX descriptor alloc */
> -		tx_q->tbs |= tbs_en ? STMMAC_TBS_AVAIL : 0;
> +			/* Setup per-TXQ tbs flag before TX descriptor alloc */
> +			tx_q->tbs |= tbs_en ? STMMAC_TBS_AVAIL : 0;
> +		}
>  	}
>  
>  	ret = alloc_dma_desc_resources(priv, dma_conf);
> @@ -7505,6 +7511,15 @@ int stmmac_dvr_probe(struct device *device,
>  		}
>  	}
>  
> +	/* If TBS feature is supported(i.e. tbssel is true), then at least 1 Tx
> +	 * DMA channel supports TBS. So if tbs_ch_num is 0 and tbssel is true,
> +	 * assume all Tx DMA channels support TBS. TBS_CH field, which gives
> +	 * number of Tx DMA channels with TBS support is only available only for
> +	 * DW xGMAC IP. For other DWMAC IPs all Tx DMA channels can support TBS.
> +	 */
> +	if (priv->dma_cap.tbssel && !priv->dma_cap.tbs_ch_num)
> +		priv->dma_cap.tbs_ch_num = priv->dma_cap.number_tx_channel;
> +
>  	ndev->features |= ndev->hw_features | NETIF_F_HIGHDMA;
>  	ndev->watchdog_timeo = msecs_to_jiffies(watchdog);
>  #ifdef STMMAC_VLAN_TAG_USED
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> index 843bd8804bfa..6c0191c84071 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> @@ -279,6 +279,10 @@ static int stmmac_mtl_setup(struct platform_device *pdev,
>  		plat->tx_queues_cfg[queue].coe_unsupported =
>  			of_property_read_bool(q_node, "snps,coe-unsupported");
>  
> +		/* Select TBS for supported queues */
> +		plat->tx_queues_cfg[queue].tbs_en =
> +			of_property_read_bool(q_node, "snps,tbs-enabled");
> +
>  		queue++;
>  	}
>  	if (queue != plat->tx_queues_to_use) {

Re: [PATCH net-next 2/2] net: stmmac: TBS support for platform driver

[Rohan G Thomas]

On Wed, 10 Jan 2024 20:20:39 +0000 : Abhishek Chauhan (ABC) wrote:

> Qualcomm had similar discussions with respect to enabling of TBS for a
> particular queue.
> We had similar discussion on these terms yesterday with Redhat. Adding
> Andrew from Redhat here
> 
> What we discovered as part of the discussions is listed below.
> 
> 1. Today upstream stmmac code is designed in such a way that TBS flag is put
> as part of queue configurations(see below snippet) and as well know that
> stmmac queue configuration comes from the dtsi file.
> 
> //ndo_open => stmmac_open
> int tbs_en = priv->plat->tx_queues_cfg[chan].tbs_en;(comes from
> tx_queues_cfg)
> 
> /* Setup per-TXQ tbs flag before TX descriptor alloc */ tx_q->tbs |= tbs_en ?
> STMMAC_TBS_AVAIL : 0;
> 
> 2. There is a no way to do this dynamically from user space because we don't
> have any API exposed which can do it from user space and also TBS rely on
> special descriptors aka enhanced desc this cannot be done run time and
> stmmac has to be aware of it before we do DMA/MAC/MTL start. To do this
> dynamically would only mean stopping DMA/MAC/MTL realloc resources for
> enhanced desc and the starting MAC/DMA/MTL. This means we are disrupting
> other traffic(By stopping MAC block).
> 
> 3. I dont think there is a way we can enable this dynamically today. I would like
> upstream community to share your thoughts as well.
> 
> 4. I agree with Rohan's patch here and want upstream community to accept it.
> This will allow use to configure the queues where TBS needs to be enabled as
> hardcoding in the code unless upstream has better way to this using
> userspace.
> 
> Please let us know if you think otherwise.
> 
> 

Hi Abhishek,

Thanks for bringing this to discussion and a better explanation from
your side. I agree with you.

Best Regards,
Rohan

Re: [PATCH net-next 2/2] net: stmmac: TBS support for platform driver

[Esben Haabendal]

"Abhishek Chauhan (ABC)" <quic_abchauha@quicinc.com> writes:

> Qualcomm had similar discussions with respect to enabling of TBS for a
> particular queue. We had similar discussion on these terms yesterday
> with Redhat. Adding Andrew from Redhat here
>
> What we discovered as part of the discussions is listed below.
>
> 1. Today upstream stmmac code is designed in such a way that TBS flag
> is put as part of queue configurations(see below snippet) and as well
> know that stmmac queue configuration comes from the dtsi file.
>
> //ndo_open => stmmac_open
> int tbs_en = priv->plat->tx_queues_cfg[chan].tbs_en;(comes from tx_queues_cfg)
>
> /* Setup per-TXQ tbs flag before TX descriptor alloc */
> tx_q->tbs |= tbs_en ? STMMAC_TBS_AVAIL : 0;
>
> 2. There is a no way to do this dynamically from user space because we don't have any 
> API exposed which can do it from user space

Not now. But why not extend ethtool API to allow enabling TBS for
supported controllers?

> and also TBS rely on special descriptors aka enhanced desc this cannot
> be done run time and stmmac has to be aware of it before we do
> DMA/MAC/MTL start.

Isn't this somewhat similar to changing the RX/TX ring parameters, which
I believe also is quite difficult to do at run time, and ethtool
therefore requires the interface to be down in oroer to change them?

> To do this dynamically would only mean stopping DMA/MAC/MTL realloc
> resources for enhanced desc and the starting MAC/DMA/MTL. This means
> we are disrupting other traffic(By stopping MAC block).

Yes. But you would be disrupting traffic less than by requiring a
complete reboot of the target which is needed if the devicetree must be
changed.

> 3. I dont think there is a way we can enable this dynamically today. I
> would like upstream community to share your thoughts as well.

Hereby done. Could we investigate the possibility of using ethtool to
change TBS enable/disable "run-time"?

> 4. I agree with Rohan's patch here and want upstream community to
> accept it. This will allow use to configure the queues where TBS needs
> to be enabled as hardcoding in the code unless upstream has better way
> to this using userspace.
>
> Please let us know if you think otherwise. 

/Esben

Re: [PATCH net-next 2/2] net: stmmac: TBS support for platform driver

[Abhishek Chauhan (ABC)]

Hi Esben,
	My responses are inline to the queries. I hope i have justified my problem statement.  

On 1/26/2024 12:43 AM, Esben Haabendal wrote:
> "Abhishek Chauhan (ABC)" <quic_abchauha@quicinc.com> writes:
> 
>> Qualcomm had similar discussions with respect to enabling of TBS for a
>> particular queue. We had similar discussion on these terms yesterday
>> with Redhat. Adding Andrew from Redhat here
>>
>> What we discovered as part of the discussions is listed below.
>>
>> 1. Today upstream stmmac code is designed in such a way that TBS flag
>> is put as part of queue configurations(see below snippet) and as well
>> know that stmmac queue configuration comes from the dtsi file.
>>
>> //ndo_open => stmmac_open
>> int tbs_en = priv->plat->tx_queues_cfg[chan].tbs_en;(comes from tx_queues_cfg)
>>
>> /* Setup per-TXQ tbs flag before TX descriptor alloc */
>> tx_q->tbs |= tbs_en ? STMMAC_TBS_AVAIL : 0;
>>
>> 2. There is a no way to do this dynamically from user space because we don't have any 
>> API exposed which can do it from user space
> 
> Not now. But why not extend ethtool API to allow enabling TBS for
> supported controllers?
> 
ethtool API can be implemented but that still doesn't solve the problem of stopping the 
entire MAC block because of enhanced desc allocation. 
1. We can either allocate enhanced desc for all channels at bootup and then choose 
to switch to enable TBS mode at runtime  (Additional memory usage)
2. Live with the disruption of traffic for a brief duration of time. 
Which is not a good solution for priority and critical traffic. 
>> and also TBS rely on special descriptors aka enhanced desc this cannot
>> be done run time and stmmac has to be aware of it before we do
>> DMA/MAC/MTL start.
> 
> Isn't this somewhat similar to changing the RX/TX ring parameters, which
> I believe also is quite difficult to do at run time, and ethtool
> therefore requires the interface to be down in oroer to change them?
> 
>> To do this dynamically would only mean stopping DMA/MAC/MTL realloc
>> resources for enhanced desc and the starting MAC/DMA/MTL. This means
>> we are disrupting other traffic(By stopping MAC block).
> 
> Yes. But you would be disrupting traffic less than by requiring a
> complete reboot of the target which is needed if the devicetree must be
> changed.
> 
any DTS solution today anyway requires completely loading the boot image 
and rebooting the device, but once the device is functional,
End user can activate TBS, as he knows the exact usecase and requirements. 
I understand the solution is not scalable, but at this point we don't have a solution 
to activate TBS  at runtime. 
>> 3. I dont think there is a way we can enable this dynamically today. I
>> would like upstream community to share your thoughts as well.
> 
> Hereby done. Could we investigate the possibility of using ethtool to
> change TBS enable/disable "run-time"?
> 
We can either allocate enhanced desc for all channels at bootup
and then choose to switch to enable TBS mode at runtime
>> 4. I agree with Rohan's patch here and want upstream community to
>> accept it. This will allow use to configure the queues where TBS needs
>> to be enabled as hardcoding in the code unless upstream has better way
>> to this using userspace.
>>
>> Please let us know if you think otherwise. 
> 
> /Esben
Best Regards
ABC

Re: [PATCH net-next 2/2] net: stmmac: TBS support for platform driver

[Esben Haabendal]

"Abhishek Chauhan (ABC)" <quic_abchauha@quicinc.com> writes:
> On 1/26/2024 12:43 AM, Esben Haabendal wrote:
>> "Abhishek Chauhan (ABC)" <quic_abchauha@quicinc.com> writes:
>>
>>> Qualcomm had similar discussions with respect to enabling of TBS for a
>>> particular queue. We had similar discussion on these terms yesterday
>>> with Redhat. Adding Andrew from Redhat here
>>>
>>> What we discovered as part of the discussions is listed below.
>>>
>>> 1. Today upstream stmmac code is designed in such a way that TBS flag
>>> is put as part of queue configurations(see below snippet) and as well
>>> know that stmmac queue configuration comes from the dtsi file.
>>>
>>> //ndo_open => stmmac_open
>>> int tbs_en = priv->plat->tx_queues_cfg[chan].tbs_en;(comes from tx_queues_cfg)
>>>
>>> /* Setup per-TXQ tbs flag before TX descriptor alloc */
>>> tx_q->tbs |= tbs_en ? STMMAC_TBS_AVAIL : 0;
>>>
>>> 2. There is a no way to do this dynamically from user space because
>>> we don't have any API exposed which can do it from user space
>>
>> Not now. But why not extend ethtool API to allow enabling TBS for
>> supported controllers?
>
> ethtool API can be implemented but that still doesn't solve the
> problem of stopping the entire MAC block because of enhanced desc
> allocation.

I am not sure what you exact point is here.

If you look at the implementation of ethtool API for changing ring
parameters, you have stmmac_set_ringparam() which calls
stmmac_reinit_ringparam(), which again calls stmmac_release() if the
interface is up (stopping the entire MAC), and then stmmac_open() which
reinitializes everything.

The same pattern could be applied to changes to enable enhanced
descriptor allocation.

I don't see why that will not be acceptable. Why would anyone have to do
that while critical traffic is flowing? In your system you should be
able to know which queues needs enhanced descriptors before starting
communication.

> 1. We can either allocate enhanced desc for all channels at bootup and
> then choose to switch to enable TBS mode at runtime (Additional memory
> usage)

A good default would IMHO be to enable enhanced descriptors for all but
TX queue 0. This will allow use of TBS without needing to change
anything. If the rather minimal extra memory usage is disturbing anyone,
then they can tune it at boot time before bringing the interface up.

> 2. Live with the disruption of traffic for a brief duration of time.
> Which is not a good solution for priority and critical traffic.

As mentioned above, I don't see why anyone would need to modify the
descriptor allocation while critical traffic is flowing.

If you are able put this information in your device tree, you definitely
will be able to put it in an boot script in some form.

>>> and also TBS rely on special descriptors aka enhanced desc this
>>> cannot be done run time and stmmac has to be aware of it before we
>>> do DMA/MAC/MTL start.
>>
>> Isn't this somewhat similar to changing the RX/TX ring parameters,
>> which I believe also is quite difficult to do at run time, and
>> ethtool therefore requires the interface to be down in oroer to
>> change them?
>>
>>> To do this dynamically would only mean stopping DMA/MAC/MTL realloc
>>> resources for enhanced desc and the starting MAC/DMA/MTL. This means
>>> we are disrupting other traffic(By stopping MAC block).
>>
>> Yes. But you would be disrupting traffic less than by requiring a
>> complete reboot of the target which is needed if the devicetree must be
>> changed.
>>
> any DTS solution today anyway requires completely loading the boot
> image and rebooting the device, but once the device is functional, End
> user can activate TBS, as he knows the exact usecase and requirements.
> I understand the solution is not scalable, but at this point we don't
> have a solution to activate TBS at runtime.

Exactly. We are discussing a solution to activate enhanced descriptors
at "runtime". But I propose to do it in a similar way as changing ring
parameters, so it is in runtime seen from a CPU perspective, but the
interface will be shortly brought down when changing it.

>>> 3. I dont think there is a way we can enable this dynamically today. I
>>> would like upstream community to share your thoughts as well.
>>
>> Hereby done. Could we investigate the possibility of using ethtool to
>> change TBS enable/disable "run-time"?
>>
> We can either allocate enhanced desc for all channels at bootup
> and then choose to switch to enable TBS mode at runtime.

I think we should do something like this:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=3b12ec8f618e

for all glue drivers, so a sane default is established that allows using
TBS from boot up.

But in addition to that, I think it would make sense to create an
ethtool API to change it from that default. And it will bring down the
interface while applying the change.

/Esben

Re: [PATCH net-next 2/2] net: stmmac: TBS support for platform driver

[Abhishek Chauhan (ABC)]

On 2/1/2024 12:26 AM, Esben Haabendal wrote:
> "Abhishek Chauhan (ABC)" <quic_abchauha@quicinc.com> writes:
>> On 1/26/2024 12:43 AM, Esben Haabendal wrote:
>>> "Abhishek Chauhan (ABC)" <quic_abchauha@quicinc.com> writes:
>>>
>>>> Qualcomm had similar discussions with respect to enabling of TBS for a
>>>> particular queue. We had similar discussion on these terms yesterday
>>>> with Redhat. Adding Andrew from Redhat here
>>>>
>>>> What we discovered as part of the discussions is listed below.
>>>>
>>>> 1. Today upstream stmmac code is designed in such a way that TBS flag
>>>> is put as part of queue configurations(see below snippet) and as well
>>>> know that stmmac queue configuration comes from the dtsi file.
>>>>
>>>> //ndo_open => stmmac_open
>>>> int tbs_en = priv->plat->tx_queues_cfg[chan].tbs_en;(comes from tx_queues_cfg)
>>>>
>>>> /* Setup per-TXQ tbs flag before TX descriptor alloc */
>>>> tx_q->tbs |= tbs_en ? STMMAC_TBS_AVAIL : 0;
>>>>
>>>> 2. There is a no way to do this dynamically from user space because
>>>> we don't have any API exposed which can do it from user space
>>>
>>> Not now. But why not extend ethtool API to allow enabling TBS for
>>> supported controllers?
>>
>> ethtool API can be implemented but that still doesn't solve the
>> problem of stopping the entire MAC block because of enhanced desc
>> allocation.
> 
> I am not sure what you exact point is here.
> 
> If you look at the implementation of ethtool API for changing ring
> parameters, you have stmmac_set_ringparam() which calls
> stmmac_reinit_ringparam(), which again calls stmmac_release() if the
> interface is up (stopping the entire MAC), and then stmmac_open() which
> reinitializes everything.
> 
> The same pattern could be applied to changes to enable enhanced
> descriptor allocation.
> 
> I don't see why that will not be acceptable. Why would anyone have to do
> that while critical traffic is flowing? In your system you should be
> able to know which queues needs enhanced descriptors before starting
> communication.
> 
>> 1. We can either allocate enhanced desc for all channels at bootup and
>> then choose to switch to enable TBS mode at runtime (Additional memory
>> usage)
> 
> A good default would IMHO be to enable enhanced descriptors for all but
> TX queue 0. This will allow use of TBS without needing to change
> anything. If the rather minimal extra memory usage is disturbing anyone,
> then they can tune it at boot time before bringing the interface up.
> 
>> 2. Live with the disruption of traffic for a brief duration of time.
>> Which is not a good solution for priority and critical traffic.
> 
> As mentioned above, I don't see why anyone would need to modify the
> descriptor allocation while critical traffic is flowing.
> 
> If you are able put this information in your device tree, you definitely
> will be able to put it in an boot script in some form.
> 
>>>> and also TBS rely on special descriptors aka enhanced desc this
>>>> cannot be done run time and stmmac has to be aware of it before we
>>>> do DMA/MAC/MTL start.
>>>
>>> Isn't this somewhat similar to changing the RX/TX ring parameters,
>>> which I believe also is quite difficult to do at run time, and
>>> ethtool therefore requires the interface to be down in oroer to
>>> change them?
>>>
>>>> To do this dynamically would only mean stopping DMA/MAC/MTL realloc
>>>> resources for enhanced desc and the starting MAC/DMA/MTL. This means
>>>> we are disrupting other traffic(By stopping MAC block).
>>>
>>> Yes. But you would be disrupting traffic less than by requiring a
>>> complete reboot of the target which is needed if the devicetree must be
>>> changed.
>>>
>> any DTS solution today anyway requires completely loading the boot
>> image and rebooting the device, but once the device is functional, End
>> user can activate TBS, as he knows the exact usecase and requirements.
>> I understand the solution is not scalable, but at this point we don't
>> have a solution to activate TBS at runtime.
> 
> Exactly. We are discussing a solution to activate enhanced descriptors
> at "runtime". But I propose to do it in a similar way as changing ring
> parameters, so it is in runtime seen from a CPU perspective, but the
> interface will be shortly brought down when changing it.
> 
>>>> 3. I dont think there is a way we can enable this dynamically today. I
>>>> would like upstream community to share your thoughts as well.
>>>
>>> Hereby done. Could we investigate the possibility of using ethtool to
>>> change TBS enable/disable "run-time"?
>>>
>> We can either allocate enhanced desc for all channels at bootup
>> and then choose to switch to enable TBS mode at runtime.
> 
> I think we should do something like this:
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=3b12ec8f618e
> 
> for all glue drivers, so a sane default is established that allows using
> TBS from boot up.
> 
> But in addition to that, I think it would make sense to create an
> ethtool API to change it from that default. And it will bring down the
> interface while applying the change.
> 

Agreed. Okay, We can go with this approach. I will evaluate the changes from ethtool perspective.
ethtool approach will be favorable to everyone as most of the users rely on ethtool to configure 
the NIC parameters. 

For now glue drivers can exclude queue0 for tbs and have all other queues to have tbs enabled.  

> /Esben

Re: [PATCH net-next 2/2] net: stmmac: TBS support for platform driver

[Esben Haabendal]

Rohan G Thomas <rohan.g.thomas@intel.com> writes:

> Enable Time Based Scheduling(TBS) support for Tx queues through the
> stmmac platform driver. For this a new per-queue tx-config property,
> tbs-enabled is added to the devicetree.
>
> Commit 7eadf57290ec ("net: stmmac: pci: Enable TBS on GMAC5 IPK PCI
> entry") enables similar support for the stmmac pci driver.
>
> Also add check whether TBS support is available for a Tx DMA channel
> before enabling TBS support for that Tx DMA channel.
>
> Signed-off-by: Rohan G Thomas <rohan.g.thomas@intel.com>
> ---
>  .../net/ethernet/stmicro/stmmac/stmmac_main.c | 25 +++++++++++++++----
>  .../ethernet/stmicro/stmmac/stmmac_platform.c |  4 +++
>  2 files changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 81b6f3ecdf92..7333f0640b3d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -3773,12 +3773,18 @@ stmmac_setup_dma_desc(struct stmmac_priv *priv, unsigned int mtu)
>  		dma_conf->dma_rx_size = DMA_DEFAULT_RX_SIZE;
>  
>  	/* Earlier check for TBS */
> -	for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++) {
> -		struct stmmac_tx_queue *tx_q = &dma_conf->tx_queue[chan];
> -		int tbs_en = priv->plat->tx_queues_cfg[chan].tbs_en;
> +	if (priv->dma_cap.tbssel) {
> +		/* TBS is available only for tbs_ch_num of Tx DMA channels,
> +		 * starting from the highest Tx DMA channel.
> +		 */
> +		chan = priv->dma_cap.number_tx_channel - priv->dma_cap.tbs_ch_num;
> +		for (; chan < priv->plat->tx_queues_to_use; chan++) {
> +			struct stmmac_tx_queue *tx_q = &dma_conf->tx_queue[chan];
> +			int tbs_en = priv->plat->tx_queues_cfg[chan].tbs_en;
>  
> -		/* Setup per-TXQ tbs flag before TX descriptor alloc */
> -		tx_q->tbs |= tbs_en ? STMMAC_TBS_AVAIL : 0;
> +			/* Setup per-TXQ tbs flag before TX descriptor alloc */
> +			tx_q->tbs |= tbs_en ? STMMAC_TBS_AVAIL : 0;
> +		}
>  	}
>  
>  	ret = alloc_dma_desc_resources(priv, dma_conf);
> @@ -7505,6 +7511,15 @@ int stmmac_dvr_probe(struct device *device,
>  		}
>  	}
>  
> +	/* If TBS feature is supported(i.e. tbssel is true), then at least 1 Tx
> +	 * DMA channel supports TBS. So if tbs_ch_num is 0 and tbssel is true,
> +	 * assume all Tx DMA channels support TBS. TBS_CH field, which gives
> +	 * number of Tx DMA channels with TBS support is only available only for
> +	 * DW xGMAC IP. For other DWMAC IPs all Tx DMA channels can support TBS.

The Ethernet QOS controllers found in various i.MX socs does not support
TBS on TX queue 0. I believe this patch would break the dwmac driver for
these platforms.

> +	 */
> +	if (priv->dma_cap.tbssel && !priv->dma_cap.tbs_ch_num)
> +		priv->dma_cap.tbs_ch_num = priv->dma_cap.number_tx_channel;
> +
>  	ndev->features |= ndev->hw_features | NETIF_F_HIGHDMA;
>  	ndev->watchdog_timeo = msecs_to_jiffies(watchdog);
>  #ifdef STMMAC_VLAN_TAG_USED
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> index 843bd8804bfa..6c0191c84071 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> @@ -279,6 +279,10 @@ static int stmmac_mtl_setup(struct platform_device *pdev,
>  		plat->tx_queues_cfg[queue].coe_unsupported =
>  			of_property_read_bool(q_node, "snps,coe-unsupported");
>  
> +		/* Select TBS for supported queues */
> +		plat->tx_queues_cfg[queue].tbs_en =
> +			of_property_read_bool(q_node, "snps,tbs-enabled");
> +
>  		queue++;
>  	}
>  	if (queue != plat->tx_queues_to_use) {

/Esben

RE: [PATCH net-next 2/2] net: stmmac: TBS support for platform driver

[rohan.g.thomas]

From: Rohan G Thomas <rohan.g.thomas@intel.com>

On Fri, 26 Jan 2024 09:35:01 +0100, Esben Haabendal wrote:
Hi Esben,

Thanks for your comments. Please find my reply inline.

> > +	/* If TBS feature is supported(i.e. tbssel is true), then at least 1 Tx
> > +	 * DMA channel supports TBS. So if tbs_ch_num is 0 and tbssel is true,
> > +	 * assume all Tx DMA channels support TBS. TBS_CH field, which gives
> > +	 * number of Tx DMA channels with TBS support is only available only
> for
> > +	 * DW xGMAC IP. For other DWMAC IPs all Tx DMA channels can
> support TBS.
> 
> The Ethernet QOS controllers found in various i.MX socs does not support TBS
> on TX queue 0. I believe this patch would break the dwmac driver for these
> platforms.

AFAIU from Synopsys DWMAC5 Databook, all queues support TBS. But TBS
cannot coexist with TSO. So all glue drivers enabling TBS feature are
avoiding queue 0 to support TSO. Also packets requesting TSO are
always directed to queue 0 by stmmac driver.

> 
> > +	 */
> > +	if (priv->dma_cap.tbssel && !priv->dma_cap.tbs_ch_num)
> > +		priv->dma_cap.tbs_ch_num = priv-
> >dma_cap.number_tx_channel;
> > +

I don't think updating tbs_ch_num with number_tx_channel would break
the dwmac driver for i.MX socs. tbs_ch_num is used by 'Earlier check
for TBS' loop to set tbs flag. 

> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -3773,12 +3773,18 @@ stmmac_setup_dma_desc(struct stmmac_priv
> *priv, unsigned int mtu)
> >  		dma_conf->dma_rx_size = DMA_DEFAULT_RX_SIZE;
> >
> >  	/* Earlier check for TBS */
> > -	for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++) {
> > -		struct stmmac_tx_queue *tx_q = &dma_conf-
> >tx_queue[chan];
> > -		int tbs_en = priv->plat->tx_queues_cfg[chan].tbs_en;
> > +	if (priv->dma_cap.tbssel) {
> > +		/* TBS is available only for tbs_ch_num of Tx DMA channels,
> > +		 * starting from the highest Tx DMA channel.
> > +		 */
> > +		chan = priv->dma_cap.number_tx_channel - priv-
> >dma_cap.tbs_ch_num;

For IPs which don't have tbs_ch_num, this loop goes from 0 to
number_tx_channel to check if tbs_enable is set by glue driver.
Existing logic is also the same. Unless you set tbs_en flag of
queue 0 from the glue driver or dts configuration this patch doesn't
set tbs flag for queue 0. This is a sanity check to avoid wrong
configuration for IPs which support tbs only in a few number of
queues.

> > +		for (; chan < priv->plat->tx_queues_to_use; chan++) {
> > +			struct stmmac_tx_queue *tx_q = &dma_conf-
> >tx_queue[chan];
> > +			int tbs_en = priv->plat->tx_queues_cfg[chan].tbs_en;
> >
> > -		/* Setup per-TXQ tbs flag before TX descriptor alloc */
> > -		tx_q->tbs |= tbs_en ? STMMAC_TBS_AVAIL : 0;
> > +			/* Setup per-TXQ tbs flag before TX descriptor alloc
> */
> > +			tx_q->tbs |= tbs_en ? STMMAC_TBS_AVAIL : 0;
> > +		}
> >  	}

Please correct me if I've misstated anything.

BR,
Rohan

Re: [PATCH net-next 2/2] net: stmmac: TBS support for platform driver

[Esben Haabendal]

rohan.g.thomas@intel.com writes:

> From: Rohan G Thomas <rohan.g.thomas@intel.com>
>
> On Fri, 26 Jan 2024 09:35:01 +0100, Esben Haabendal wrote:
>
>> > +	/* If TBS feature is supported(i.e. tbssel is true), then at least 1 Tx
>> > +	 * DMA channel supports TBS. So if tbs_ch_num is 0 and tbssel is true,
>> > +	 * assume all Tx DMA channels support TBS. TBS_CH field, which gives
>> > +	 * number of Tx DMA channels with TBS support is only available only
>> for
>> > +	 * DW xGMAC IP. For other DWMAC IPs all Tx DMA channels can
>> support TBS.
>> 
>> The Ethernet QOS controllers found in various i.MX socs does not support TBS
>> on TX queue 0. I believe this patch would break the dwmac driver for these
>> platforms.
>
> AFAIU from Synopsys DWMAC5 Databook, all queues support TBS. But TBS
> cannot coexist with TSO. So all glue drivers enabling TBS feature are
> avoiding queue 0 to support TSO. Also packets requesting TSO are
> always directed to queue 0 by stmmac driver.

After re-reading the i.MX8MP documentation, and making a few
experiments, I have to agree with you. Enabling TBS (enhanced
descriptors) for Q0 should be ok on i.MX.

>> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> > @@ -3773,12 +3773,18 @@ stmmac_setup_dma_desc(struct stmmac_priv
>> *priv, unsigned int mtu)
>> >  		dma_conf->dma_rx_size = DMA_DEFAULT_RX_SIZE;
>> >
>> >  	/* Earlier check for TBS */
>> > -	for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++) {
>> > -		struct stmmac_tx_queue *tx_q = &dma_conf-
>> >tx_queue[chan];
>> > -		int tbs_en = priv->plat->tx_queues_cfg[chan].tbs_en;
>> > +	if (priv->dma_cap.tbssel) {
>> > +		/* TBS is available only for tbs_ch_num of Tx DMA channels,
>> > +		 * starting from the highest Tx DMA channel.
>> > +		 */
>> > +		chan = priv->dma_cap.number_tx_channel - priv-
>> >dma_cap.tbs_ch_num;
>
> For IPs which don't have tbs_ch_num, this loop goes from 0 to
> number_tx_channel to check if tbs_enable is set by glue driver.
> Existing logic is also the same. Unless you set tbs_en flag of
> queue 0 from the glue driver or dts configuration this patch doesn't
> set tbs flag for queue 0. This is a sanity check to avoid wrong
> configuration for IPs which support tbs only in a few number of
> queues.

Sounds good.

>> > +		for (; chan < priv->plat->tx_queues_to_use; chan++) {
>> > +			struct stmmac_tx_queue *tx_q = &dma_conf-
>> >tx_queue[chan];
>> > +			int tbs_en = priv->plat->tx_queues_cfg[chan].tbs_en;
>> >
>> > -		/* Setup per-TXQ tbs flag before TX descriptor alloc */
>> > -		tx_q->tbs |= tbs_en ? STMMAC_TBS_AVAIL : 0;
>> > +			/* Setup per-TXQ tbs flag before TX descriptor alloc
>> */
>> > +			tx_q->tbs |= tbs_en ? STMMAC_TBS_AVAIL : 0;
>> > +		}
>> >  	}
>
> Please correct me if I've misstated anything.

No corrections for now :)

/Esben