[PATCH net-next v5 0/2] net: stmmac: Tx coe sw fallback

[PATCH net-next v5 0/2] net: stmmac: Tx coe sw fallback

[Rohan G Thomas]

Hi,
Some DWMAC IPs support tx coe only for a few initial tx queues,
starting from tx queue 0. This patchset adds support for tx coe sw
fallback for those queues that don't support tx coe. Also, add binding
for snps,tx-queues-with-coe property.

changelog v5:
* As rightly suggested by Serge, reworked redundant code.

changelog v4: 
* Replaced tx_q_coe_lmt with bit flag.

changelog v3: 
* Resend with complete email list.

changelog v2: 
* Reformed binding description.
* Minor grammatical corrections in comments and commit messages.

Rohan G Thomas (2):
  dt-bindings: net: snps,dwmac: Tx queues with coe
  net: stmmac: Tx coe sw fallback

 .../devicetree/bindings/net/snps,dwmac.yaml    |  3 +++
 .../net/ethernet/stmicro/stmmac/stmmac_main.c  | 18 ++++++++++++++++++
 .../ethernet/stmicro/stmmac/stmmac_platform.c  |  4 ++++
 include/linux/stmmac.h                         |  1 +
 4 files changed, 26 insertions(+)

-- 
2.19.0

[PATCH net-next v5 1/2] dt-bindings: net: snps,dwmac: Tx queues with coe

[Rohan G Thomas]

Add dt-bindings for the number of tx queues with coe support. Some
dwmac IPs support tx queues only for a few initial tx queues,
starting from tx queue 0.

Signed-off-by: Rohan G Thomas <rohan.g.thomas@intel.com>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
---
 Documentation/devicetree/bindings/net/snps,dwmac.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
index ddf9522a5dc2..0c6431c10cf9 100644
--- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
+++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
@@ -313,6 +313,9 @@ properties:
       snps,tx-queues-to-use:
         $ref: /schemas/types.yaml#/definitions/uint32
         description: number of TX queues to be used in the driver
+      snps,tx-queues-with-coe:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        description: number of TX queues that support TX checksum offloading
       snps,tx-sched-wrr:
         type: boolean
         description: Weighted Round Robin
-- 
2.19.0

Re: [PATCH net-next v5 1/2] dt-bindings: net: snps,dwmac: Tx queues with coe

[Serge Semin]

On Sat, Aug 19, 2023 at 10:31:31AM +0800, Rohan G Thomas wrote:
> Add dt-bindings for the number of tx queues with coe support. Some
> dwmac IPs support tx queues only for a few initial tx queues,
> starting from tx queue 0.
> 
> Signed-off-by: Rohan G Thomas <rohan.g.thomas@intel.com>
> Acked-by: Conor Dooley <conor.dooley@microchip.com>

Reviewed-by: Serge Semin <fancer.lancer@gmail.com>

-Serge(y)

> ---
>  Documentation/devicetree/bindings/net/snps,dwmac.yaml | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> index ddf9522a5dc2..0c6431c10cf9 100644
> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> @@ -313,6 +313,9 @@ properties:
>        snps,tx-queues-to-use:
>          $ref: /schemas/types.yaml#/definitions/uint32
>          description: number of TX queues to be used in the driver
> +      snps,tx-queues-with-coe:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description: number of TX queues that support TX checksum offloading
>        snps,tx-sched-wrr:
>          type: boolean
>          description: Weighted Round Robin
> -- 
> 2.19.0
> 

Re: [PATCH net-next v5 1/2] dt-bindings: net: snps,dwmac: Tx queues with coe

[Jakub Kicinski]

On Sat, 19 Aug 2023 10:31:31 +0800 Rohan G Thomas wrote:
> +      snps,tx-queues-with-coe:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description: number of TX queues that support TX checksum offloading

Is it going to be obvious that if not present all queues support
checksum offload? I think we should document the default.
-- 
pw-bot: cr

Re: [PATCH net-next v5 1/2] dt-bindings: net: snps,dwmac: Tx queues with coe

[Rohan G Thomas]

On Sat, 19 Aug 2023 10:31:31 +0800 Rohan G Thomas wrote:
>> +      snps,tx-queues-with-coe:
>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +        description: number of TX queues that support TX checksum 
>> + offloading
>
>Is it going to be obvious that if not present all queues support checksum offload? I think we should document the default.

Agreed. Will add this in the next version.

BR,
Rohan

Re: [PATCH net-next v5 1/2] dt-bindings: net: snps,dwmac: Tx queues with coe

[Serge Semin]

On Tue, Aug 22, 2023 at 05:15:25PM -0700, Jakub Kicinski wrote:
> On Sat, 19 Aug 2023 10:31:31 +0800 Rohan G Thomas wrote:
> > +      snps,tx-queues-with-coe:
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +        description: number of TX queues that support TX checksum offloading
> 

> Is it going to be obvious that if not present all queues support
> checksum offload? I think we should document the default.

This question is debatable:
1. By default the DW xGMAC and DW QoS Eth IP-cores are
synthesized with only the very first Tx queue having Tx COE enabled.
2. If TSO is disabled then the Tx COE can be individually enabled
for each queue available on DW QoS Eth controller and for the very
first N queues on DW xGMAC controller.
3. If TSO is enabled then the Tx COE will be automatically and always
enabled for as many first queues as there are TSO-capable
DMA-channels.
4. At the current state the STMMAC driver assumes that all Tx Queues
support Tx COE.

The entry 4 can't be changed since we'll risk to catch regressions on
the platforms with no property specified. On the other hand it partly
contradicts to the rest of the entries. I don't know what would be a
correct way to specify the default value in this case. Most likely
just keep the entry 4 and be done with it.

BTW I just noticed that but the suggested "snps,tx-queues-with-coe"
property semantic will only cover a DW XGMAC-part of the case 2. DW
QoS Eth can be synthesized with Tx COE individually enabled for a
particular queue if TSO is unavailable.

-Serge(y)

> -- 
> pw-bot: cr

Re: [PATCH net-next v5 1/2] dt-bindings: net: snps,dwmac: Tx queues with coe

[Rohan G Thomas]

>On Tue, Aug 22, 2023 at 05:15:25PM -0700, Jakub Kicinski wrote:
>> On Sat, 19 Aug 2023 10:31:31 +0800 Rohan G Thomas wrote:
>> > +      snps,tx-queues-with-coe:
>> > +        $ref: /schemas/types.yaml#/definitions/uint32
>> > +        description: number of TX queues that support TX checksum offloading
>> 
>
>> Is it going to be obvious that if not present all queues support
>> checksum offload? I think we should document the default.
>
>This question is debatable:
>1. By default the DW xGMAC and DW QoS Eth IP-cores are
>synthesized with only the very first Tx queue having Tx COE enabled.
>2. If TSO is disabled then the Tx COE can be individually enabled
>for each queue available on DW QoS Eth controller and for the very
>first N queues on DW xGMAC controller.
>3. If TSO is enabled then the Tx COE will be automatically and always
>enabled for as many first queues as there are TSO-capable
>DMA-channels.
>4. At the current state the STMMAC driver assumes that all Tx Queues
>support Tx COE.
>
>The entry 4 can't be changed since we'll risk to catch regressions on
>the platforms with no property specified. On the other hand it partly
>contradicts to the rest of the entries. I don't know what would be a
>correct way to specify the default value in this case. Most likely
>just keep the entry 4 and be done with it.
>
>BTW I just noticed that but the suggested "snps,tx-queues-with-coe"
>property semantic will only cover a DW XGMAC-part of the case 2. DW
>QoS Eth can be synthesized with Tx COE individually enabled for a
>particular queue if TSO is unavailable.

Hi Serge,

Didn't know about a different IP configuration supported by DW QoS Eth IP. If
this is the case, I think we can have a flag 'coe-unsupported' for any TX
queue subnode as below.

+          snps,coe-unsupported:
+            $ref: /schemas/types.yaml#/definitions/flag
+            description:
+              TX checksum offload is unsupported by the TX queue. If TX checksum
+              offload is requested for a packet to be transmitted through this
+              TX queue then have a software fallback in the driver for checksum
+              calculation.

If this is okay, I can rework the patch based on this. Covers both DW QoS Eth IP
and DW XGMAC IP cases.

>
>-Serge(y)
>
>> -- 
>> pw-bot: cr

BR,
Rohan

Re: [PATCH net-next v5 1/2] dt-bindings: net: snps,dwmac: Tx queues with coe

[Serge Semin]

On Thu, Aug 24, 2023 at 01:10:04AM +0800, Rohan G Thomas wrote:
> >On Tue, Aug 22, 2023 at 05:15:25PM -0700, Jakub Kicinski wrote:
> >> On Sat, 19 Aug 2023 10:31:31 +0800 Rohan G Thomas wrote:
> >> > +      snps,tx-queues-with-coe:
> >> > +        $ref: /schemas/types.yaml#/definitions/uint32
> >> > +        description: number of TX queues that support TX checksum offloading
> >> 
> >
> >> Is it going to be obvious that if not present all queues support
> >> checksum offload? I think we should document the default.
> >
> >This question is debatable:
> >1. By default the DW xGMAC and DW QoS Eth IP-cores are
> >synthesized with only the very first Tx queue having Tx COE enabled.
> >2. If TSO is disabled then the Tx COE can be individually enabled
> >for each queue available on DW QoS Eth controller and for the very
> >first N queues on DW xGMAC controller.
> >3. If TSO is enabled then the Tx COE will be automatically and always
> >enabled for as many first queues as there are TSO-capable
> >DMA-channels.
> >4. At the current state the STMMAC driver assumes that all Tx Queues
> >support Tx COE.
> >
> >The entry 4 can't be changed since we'll risk to catch regressions on
> >the platforms with no property specified. On the other hand it partly
> >contradicts to the rest of the entries. I don't know what would be a
> >correct way to specify the default value in this case. Most likely
> >just keep the entry 4 and be done with it.
> >
> >BTW I just noticed that but the suggested "snps,tx-queues-with-coe"
> >property semantic will only cover a DW XGMAC-part of the case 2. DW
> >QoS Eth can be synthesized with Tx COE individually enabled for a
> >particular queue if TSO is unavailable.
> 
> Hi Serge,
> 
> Didn't know about a different IP configuration supported by DW QoS Eth IP. If
> this is the case, I think we can have a flag 'coe-unsupported' for any TX
> queue subnode as below.
> 
> +          snps,coe-unsupported:

> +            $ref: /schemas/types.yaml#/definitions/flag

AFAIR tKrzysztof preferred to use type: boolean for the flags.

> +            description:
> +              TX checksum offload is unsupported by the TX queue. 

> +              If TX checksum
> +              offload is requested for a packet to be transmitted through this
> +              TX queue then have a software fallback in the driver for checksum
> +              calculation.

This is redundant in the HW description.

> 

> If this is okay, I can rework the patch based on this. Covers both DW QoS Eth IP
> and DW XGMAC IP cases.

I guess that's the only choice we have seeing the driver already
expects all the Tx queues having the COE supported.

-Serge(y)

> 
> >
> >-Serge(y)
> >
> >> -- 
> >> pw-bot: cr
> 
> BR,
> Rohan

[PATCH net-next v5 2/2] net: stmmac: Tx coe sw fallback

[Rohan G Thomas]

Add sw fallback of tx checksum calculation for those tx queues that
don't support tx checksum offloading. Because, some DWMAC IPs support
tx checksum offloading only for a few initial tx queues, starting
from tx queue 0.

Signed-off-by: Rohan G Thomas <rohan.g.thomas@intel.com>
---
 .../net/ethernet/stmicro/stmmac/stmmac_main.c  | 18 ++++++++++++++++++
 .../ethernet/stmicro/stmmac/stmmac_platform.c  |  4 ++++
 include/linux/stmmac.h                         |  1 +
 3 files changed, 23 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 733b5e900817..3ffef45a2861 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -4409,6 +4409,16 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 	WARN_ON(tx_q->tx_skbuff[first_entry]);
 
 	csum_insertion = (skb->ip_summed == CHECKSUM_PARTIAL);
+	/* Some DWMAC IPs support tx coe only for a few initial tx queues,
+	 * starting from tx queue 0. So checksum offloading for those queues
+	 * that don't support tx coe needs to fallback to software checksum
+	 * calculation.
+	 */
+	if (csum_insertion && queue >= priv->plat->tx_queues_with_coe) {
+		if (unlikely(skb_checksum_help(skb)))
+			goto dma_map_err;
+		csum_insertion = !csum_insertion;
+	}
 
 	if (likely(priv->extend_desc))
 		desc = (struct dma_desc *)(tx_q->dma_etx + entry);
@@ -7401,6 +7411,14 @@ int stmmac_dvr_probe(struct device *device,
 		dev_info(priv->device, "SPH feature enabled\n");
 	}
 
+	if (priv->plat->tx_coe && !priv->plat->tx_queues_with_coe)
+		priv->plat->tx_queues_with_coe = priv->plat->tx_queues_to_use;
+	else if (!priv->plat->tx_coe)
+		priv->plat->tx_queues_with_coe = 0;
+	else if (priv->plat->tx_queues_with_coe < priv->plat->tx_queues_to_use)
+		dev_info(priv->device, "TX COE only available for %u queues\n",
+			 priv->plat->tx_queues_with_coe);
+
 	/* Ideally our host DMA address width is the same as for the
 	 * device. However, it may differ and then we have to use our
 	 * host DMA width for allocation and the device DMA width for
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index be8e79c7aa34..0138b7c9c7ab 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -225,6 +225,10 @@ static int stmmac_mtl_setup(struct platform_device *pdev,
 				 &plat->tx_queues_to_use))
 		plat->tx_queues_to_use = 1;
 
+	if (of_property_read_u32(tx_node, "snps,tx-queues-with-coe",
+				 &plat->tx_queues_with_coe))
+		plat->tx_queues_with_coe = plat->tx_queues_to_use;
+
 	if (of_property_read_bool(tx_node, "snps,tx-sched-wrr"))
 		plat->tx_sched_algorithm = MTL_TX_ALGORITHM_WRR;
 	else if (of_property_read_bool(tx_node, "snps,tx-sched-wfq"))
diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
index 784277d666eb..cb508164eaea 100644
--- a/include/linux/stmmac.h
+++ b/include/linux/stmmac.h
@@ -252,6 +252,7 @@ struct plat_stmmacenet_data {
 	u32 host_dma_width;
 	u32 rx_queues_to_use;
 	u32 tx_queues_to_use;
+	u32 tx_queues_with_coe;
 	u8 rx_sched_algorithm;
 	u8 tx_sched_algorithm;
 	struct stmmac_rxq_cfg rx_queues_cfg[MTL_MAX_RX_QUEUES];
-- 
2.19.0

Re: [PATCH net-next v5 2/2] net: stmmac: Tx coe sw fallback

[Serge Semin]

On Sat, Aug 19, 2023 at 10:31:32AM +0800, Rohan G Thomas wrote:
> Add sw fallback of tx checksum calculation for those tx queues that
> don't support tx checksum offloading. Because, some DWMAC IPs support
> tx checksum offloading only for a few initial tx queues, starting
> from tx queue 0.
> 
> Signed-off-by: Rohan G Thomas <rohan.g.thomas@intel.com>

LGTM,
Reviewed-by: Serge Semin <fancer.lancer@gmail.com>

-Serge(y)

> ---
>  .../net/ethernet/stmicro/stmmac/stmmac_main.c  | 18 ++++++++++++++++++
>  .../ethernet/stmicro/stmmac/stmmac_platform.c  |  4 ++++
>  include/linux/stmmac.h                         |  1 +
>  3 files changed, 23 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 733b5e900817..3ffef45a2861 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -4409,6 +4409,16 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>  	WARN_ON(tx_q->tx_skbuff[first_entry]);
>  
>  	csum_insertion = (skb->ip_summed == CHECKSUM_PARTIAL);
> +	/* Some DWMAC IPs support tx coe only for a few initial tx queues,
> +	 * starting from tx queue 0. So checksum offloading for those queues
> +	 * that don't support tx coe needs to fallback to software checksum
> +	 * calculation.
> +	 */
> +	if (csum_insertion && queue >= priv->plat->tx_queues_with_coe) {
> +		if (unlikely(skb_checksum_help(skb)))
> +			goto dma_map_err;
> +		csum_insertion = !csum_insertion;
> +	}
>  
>  	if (likely(priv->extend_desc))
>  		desc = (struct dma_desc *)(tx_q->dma_etx + entry);
> @@ -7401,6 +7411,14 @@ int stmmac_dvr_probe(struct device *device,
>  		dev_info(priv->device, "SPH feature enabled\n");
>  	}
>  
> +	if (priv->plat->tx_coe && !priv->plat->tx_queues_with_coe)
> +		priv->plat->tx_queues_with_coe = priv->plat->tx_queues_to_use;
> +	else if (!priv->plat->tx_coe)
> +		priv->plat->tx_queues_with_coe = 0;
> +	else if (priv->plat->tx_queues_with_coe < priv->plat->tx_queues_to_use)
> +		dev_info(priv->device, "TX COE only available for %u queues\n",
> +			 priv->plat->tx_queues_with_coe);
> +
>  	/* Ideally our host DMA address width is the same as for the
>  	 * device. However, it may differ and then we have to use our
>  	 * host DMA width for allocation and the device DMA width for
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> index be8e79c7aa34..0138b7c9c7ab 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> @@ -225,6 +225,10 @@ static int stmmac_mtl_setup(struct platform_device *pdev,
>  				 &plat->tx_queues_to_use))
>  		plat->tx_queues_to_use = 1;
>  
> +	if (of_property_read_u32(tx_node, "snps,tx-queues-with-coe",
> +				 &plat->tx_queues_with_coe))
> +		plat->tx_queues_with_coe = plat->tx_queues_to_use;
> +
>  	if (of_property_read_bool(tx_node, "snps,tx-sched-wrr"))
>  		plat->tx_sched_algorithm = MTL_TX_ALGORITHM_WRR;
>  	else if (of_property_read_bool(tx_node, "snps,tx-sched-wfq"))
> diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
> index 784277d666eb..cb508164eaea 100644
> --- a/include/linux/stmmac.h
> +++ b/include/linux/stmmac.h
> @@ -252,6 +252,7 @@ struct plat_stmmacenet_data {
>  	u32 host_dma_width;
>  	u32 rx_queues_to_use;
>  	u32 tx_queues_to_use;
> +	u32 tx_queues_with_coe;
>  	u8 rx_sched_algorithm;
>  	u8 tx_sched_algorithm;
>  	struct stmmac_rxq_cfg rx_queues_cfg[MTL_MAX_RX_QUEUES];
> -- 
> 2.19.0
>