[PATCH 0/3] net: ethernet: ti: am65-cpsw: Allow for MTU values

[PATCH 0/3] net: ethernet: ti: am65-cpsw: Allow for MTU values

[Sanjuán García, Jorge]

The am65-cpsw-nuss driver has a fixed definition for the maximum ethernet
frame length of 1522 bytes (AM65_CPSW_MAX_PACKET_SIZE). This limits the switch
ports to only operate at a maximum MTU of 1500 bytes. When combining this CPSW
switch with a DSA switch connected to one of its ports this limitation shows up.
The extra 8 bytes the DSA subsystem adds internally to the ethernet frame
create resulting frames bigger than 1522 bytes (1518 for non VLAN + 8 for DSA
stuff) so they get dropped by the switch.

One of the issues with the the am65-cpsw-nuss driver is that the network device
max_mtu was being set to the same fixed value defined for the max total frame
length (1522 bytes). This makes the DSA subsystem believe that the MTU of the
interface can be set to 1508 bytes to make room for the extra 8 bytes of the DSA
headers. However, all packages created assuming the 1500 bytes payload get
dropped by the switch as oversized.

This series offers a solution to this problem. First, the max_mtu advertised on
the network device and the actual max frame size configured on the switch
registers are made consistent by letting the extra room needed for the ethernet
headers and the frame checksum (22 bytes including VLAN). Also, make the max
frame length configurable via a devicetree property to allow for MTU values
greater than 1500 bytes.

Jorge Sanjuan Garcia (3):
  net: ethernet: ti: am65-cpsw: Fix max mtu to fit ethernet frames
  net: ethernet: ti: am65-cpsw: Introduce rx_packet_max member
  net: ethernet: ti: am65-cpsw: Add device tree property to set max MTU

 drivers/net/ethernet/ti/am65-cpsw-nuss.c | 26 ++++++++++++++++++------
 drivers/net/ethernet/ti/am65-cpsw-nuss.h |  3 +++
 2 files changed, 23 insertions(+), 6 deletions(-)

-- 
2.34.1

[PATCH 1/3] net: ethernet: ti: am65-cpsw: Fix max mtu to fit ethernet frames

[Sanjuán García, Jorge]

The value of AM65_CPSW_MAX_PACKET_SIZE represents the maximum length
of a received frame. This value is written to the register
AM65_CPSW_PORT_REG_RX_MAXLEN.

The maximum MTU configured on the network device should then leave
some room for the ethernet headers and frame check. Otherwise, if
the network interface is configured to its maximum mtu possible,
the frames will be larger than AM65_CPSW_MAX_PACKET_SIZE and will
get dropped as oversized.

Fixes: 93a76530316a ("net: ethernet: ti: introduce am65x/j721e gigabit eth subsystem driver")
Signed-off-by: Jorge Sanjuan Garcia <jorge.sanjuangarcia@duagon.com>
---
 drivers/net/ethernet/ti/am65-cpsw-nuss.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index 7651f90f51f2..378d69b8cb14 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -2196,7 +2196,8 @@ am65_cpsw_nuss_init_port_ndev(struct am65_cpsw_common *common, u32 port_idx)
 	eth_hw_addr_set(port->ndev, port->slave.mac_addr);
 
 	port->ndev->min_mtu = AM65_CPSW_MIN_PACKET_SIZE;
-	port->ndev->max_mtu = AM65_CPSW_MAX_PACKET_SIZE;
+	port->ndev->max_mtu = AM65_CPSW_MAX_PACKET_SIZE -
+			      (VLAN_ETH_HLEN + ETH_FCS_LEN);
 	port->ndev->hw_features = NETIF_F_SG |
 				  NETIF_F_RXCSUM |
 				  NETIF_F_HW_CSUM |
-- 
2.34.1

Re: [PATCH 1/3] net: ethernet: ti: am65-cpsw: Fix max mtu to fit ethernet frames

[Siddharth Vadapalli]

Hello,

On 02-01-2024 13:49, Sanjuán García, Jorge wrote:
> The value of AM65_CPSW_MAX_PACKET_SIZE represents the maximum length
> of a received frame. This value is written to the register
> AM65_CPSW_PORT_REG_RX_MAXLEN.
> 
> The maximum MTU configured on the network device should then leave
> some room for the ethernet headers and frame check. Otherwise, if
> the network interface is configured to its maximum mtu possible,
> the frames will be larger than AM65_CPSW_MAX_PACKET_SIZE and will
> get dropped as oversized.
> 
> Fixes: 93a76530316a ("net: ethernet: ti: introduce am65x/j721e gigabit eth 
> subsystem driver")
> Signed-off-by: Jorge Sanjuan Garcia <jorge.sanjuangarcia@duagon.com>

Please use [PATCH net] as the subject prefix for patches which are
posted as Bug Fixes. Apart from that, this patch looks good to me.

Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com>

> ---
>   drivers/net/ethernet/ti/am65-cpsw-nuss.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c 
> b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> index 7651f90f51f2..378d69b8cb14 100644
> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> @@ -2196,7 +2196,8 @@ am65_cpsw_nuss_init_port_ndev(struct am65_cpsw_common 
> *common, u32 port_idx)
>           eth_hw_addr_set(port->ndev, port->slave.mac_addr);
> 
>           port->ndev->min_mtu = AM65_CPSW_MIN_PACKET_SIZE;
> -       port->ndev->max_mtu = AM65_CPSW_MAX_PACKET_SIZE;
> +       port->ndev->max_mtu = AM65_CPSW_MAX_PACKET_SIZE -
> +                             (VLAN_ETH_HLEN + ETH_FCS_LEN);
>           port->ndev->hw_features = NETIF_F_SG |
>                                     NETIF_F_RXCSUM |
>                                     NETIF_F_HW_CSUM |

...

-- 
Regards,
Siddharth.

[PATCH 2/3] net: ethernet: ti: am65-cpsw: Introduce rx_packet_max member

[Sanjuán García, Jorge]

The value written to the register AM65_CPSW_PORT_REG_RX_MAXLEN
is currently fixed to what AM65_CPSW_MAX_PACKET_SIZE defines. This
patch prepares the driver so that the maximum received frame length
can be configured in future updates.

Signed-off-by: Jorge Sanjuan Garcia <jorge.sanjuangarcia@duagon.com>
---
 drivers/net/ethernet/ti/am65-cpsw-nuss.c | 13 ++++++++-----
 drivers/net/ethernet/ti/am65-cpsw-nuss.h |  2 ++
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index 378d69b8cb14..a920146d7a60 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -151,9 +151,11 @@ static void am65_cpsw_port_set_sl_mac(struct am65_cpsw_port *slave,
 
 static void am65_cpsw_sl_ctl_reset(struct am65_cpsw_port *port)
 {
+	struct am65_cpsw_common *common = port->common;
+
 	cpsw_sl_reset(port->slave.mac_sl, 100);
 	/* Max length register has to be restored after MAC SL reset */
-	writel(AM65_CPSW_MAX_PACKET_SIZE,
+	writel(common->rx_packet_max,
 	       port->port_base + AM65_CPSW_PORT_REG_RX_MAXLEN);
 }
 
@@ -455,7 +457,7 @@ static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common)
 	       AM65_CPSW_CTL_VLAN_AWARE | AM65_CPSW_CTL_P0_RX_PAD,
 	       common->cpsw_base + AM65_CPSW_REG_CTL);
 	/* Max length register */
-	writel(AM65_CPSW_MAX_PACKET_SIZE,
+	writel(common->rx_packet_max,
 	       host_p->port_base + AM65_CPSW_PORT_REG_RX_MAXLEN);
 	/* set base flow_id */
 	writel(common->rx_flow_id_base,
@@ -507,7 +509,7 @@ static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common)
 
 	for (i = 0; i < common->rx_chns.descs_num; i++) {
 		skb = __netdev_alloc_skb_ip_align(NULL,
-						  AM65_CPSW_MAX_PACKET_SIZE,
+						  common->rx_packet_max,
 						  GFP_KERNEL);
 		if (!skb) {
 			ret = -ENOMEM;
@@ -848,7 +850,7 @@ static int am65_cpsw_nuss_rx_packets(struct am65_cpsw_common *common,
 
 	k3_cppi_desc_pool_free(rx_chn->desc_pool, desc_rx);
 
-	new_skb = netdev_alloc_skb_ip_align(ndev, AM65_CPSW_MAX_PACKET_SIZE);
+	new_skb = netdev_alloc_skb_ip_align(ndev, common->rx_packet_max);
 	if (new_skb) {
 		ndev_priv = netdev_priv(ndev);
 		am65_cpsw_nuss_set_offload_fwd_mark(skb, ndev_priv->offload_fwd_mark);
@@ -2196,7 +2198,7 @@ am65_cpsw_nuss_init_port_ndev(struct am65_cpsw_common *common, u32 port_idx)
 	eth_hw_addr_set(port->ndev, port->slave.mac_addr);
 
 	port->ndev->min_mtu = AM65_CPSW_MIN_PACKET_SIZE;
-	port->ndev->max_mtu = AM65_CPSW_MAX_PACKET_SIZE -
+	port->ndev->max_mtu = common->rx_packet_max -
 			      (VLAN_ETH_HLEN + ETH_FCS_LEN);
 	port->ndev->hw_features = NETIF_F_SG |
 				  NETIF_F_RXCSUM |
@@ -2926,6 +2928,7 @@ static int am65_cpsw_nuss_probe(struct platform_device *pdev)
 		return -ENOENT;
 
 	common->rx_flow_id_base = -1;
+	common->rx_packet_max = AM65_CPSW_MAX_PACKET_SIZE;
 	init_completion(&common->tdown_complete);
 	common->tx_ch_num = AM65_CPSW_DEFAULT_TX_CHNS;
 	common->pf_p0_rx_ptype_rrobin = false;
diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.h b/drivers/net/ethernet/ti/am65-cpsw-nuss.h
index f3dad2ab9828..141160223d73 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.h
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.h
@@ -130,6 +130,8 @@ struct am65_cpsw_common {
 	u32			tx_ch_rate_msk;
 	u32			rx_flow_id_base;
 
+	int			rx_packet_max;
+
 	struct am65_cpsw_tx_chn	tx_chns[AM65_CPSW_MAX_TX_QUEUES];
 	struct completion	tdown_complete;
 	atomic_t		tdown_cnt;
-- 
2.34.1

Re: [PATCH 2/3] net: ethernet: ti: am65-cpsw: Introduce rx_packet_max member

[Siddharth Vadapalli]

Hello,

On 02-01-2024 13:49, Sanjuán García, Jorge wrote:
> The value written to the register AM65_CPSW_PORT_REG_RX_MAXLEN
> is currently fixed to what AM65_CPSW_MAX_PACKET_SIZE defines. This
> patch prepares the driver so that the maximum received frame length
> can be configured in future updates.
> 
> Signed-off-by: Jorge Sanjuan Garcia <jorge.sanjuangarcia@duagon.com>
> ---

For patches which add new features, please use the subject prefix
[PATCH net-next].

>   drivers/net/ethernet/ti/am65-cpsw-nuss.c | 13 ++++++++-----
>   drivers/net/ethernet/ti/am65-cpsw-nuss.h |  2 ++
>   2 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c 
> b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> index 378d69b8cb14..a920146d7a60 100644
> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> @@ -151,9 +151,11 @@ static void am65_cpsw_port_set_sl_mac(struct am65_cpsw_port 
> *slave,
> 
>   static void am65_cpsw_sl_ctl_reset(struct am65_cpsw_port *port)
>   {
> +       struct am65_cpsw_common *common = port->common;
> +
>           cpsw_sl_reset(port->slave.mac_sl, 100);
>           /* Max length register has to be restored after MAC SL reset */
> -       writel(AM65_CPSW_MAX_PACKET_SIZE,
> +       writel(common->rx_packet_max,
>                  port->port_base + AM65_CPSW_PORT_REG_RX_MAXLEN);

Prior to this patch, since the RX Packet size was hard-coded, it was
being set to the same value across all ports. However, please note that
there are per-port registers:
port->port_base + AM65_CPSW_PORT_REG_RX_MAXLEN
So, wouldn't it be better to define the "rx_packet_max" member within
"struct am65_cpsw_port", rather than "struct am65_cpsw_common"?
The same question applies to the following sections as well.

I went through patch 3/3 of this series and the device-tree property:
max-frame-size
is being used to fetch the value for "rx_packet_max". But, isn't
max-frame-size a port specific parameter? Shouldn't it then be stored as
a member of "struct am65_cpsw_port".

>   }
> 
> @@ -455,7 +457,7 @@ static int am65_cpsw_nuss_common_open(struct 
> am65_cpsw_common *common)
>                  AM65_CPSW_CTL_VLAN_AWARE | AM65_CPSW_CTL_P0_RX_PAD,
>                  common->cpsw_base + AM65_CPSW_REG_CTL);
>           /* Max length register */
> -       writel(AM65_CPSW_MAX_PACKET_SIZE,
> +       writel(common->rx_packet_max,
>                  host_p->port_base + AM65_CPSW_PORT_REG_RX_MAXLEN);
>           /* set base flow_id */
>           writel(common->rx_flow_id_base,
> @@ -507,7 +509,7 @@ static int am65_cpsw_nuss_common_open(struct 
> am65_cpsw_common *common)
> 
>           for (i = 0; i < common->rx_chns.descs_num; i++) {
>                   skb = __netdev_alloc_skb_ip_align(NULL,
> -                                                 AM65_CPSW_MAX_PACKET_SIZE,
> +                                                 common->rx_packet_max,
>                                                     GFP_KERNEL);
>                   if (!skb) {
>                           ret = -ENOMEM;
> @@ -848,7 +850,7 @@ static int am65_cpsw_nuss_rx_packets(struct am65_cpsw_common 
> *common,
> 
>           k3_cppi_desc_pool_free(rx_chn->desc_pool, desc_rx);
> 
> -       new_skb = netdev_alloc_skb_ip_align(ndev, AM65_CPSW_MAX_PACKET_SIZE);
> +       new_skb = netdev_alloc_skb_ip_align(ndev, common->rx_packet_max);
>           if (new_skb) {
>                   ndev_priv = netdev_priv(ndev);
>                   am65_cpsw_nuss_set_offload_fwd_mark(skb, 
> ndev_priv->offload_fwd_mark);
> @@ -2196,7 +2198,7 @@ am65_cpsw_nuss_init_port_ndev(struct am65_cpsw_common 
> *common, u32 port_idx)
>           eth_hw_addr_set(port->ndev, port->slave.mac_addr);
> 
>           port->ndev->min_mtu = AM65_CPSW_MIN_PACKET_SIZE;
> -       port->ndev->max_mtu = AM65_CPSW_MAX_PACKET_SIZE -
> +       port->ndev->max_mtu = common->rx_packet_max -
>                                 (VLAN_ETH_HLEN + ETH_FCS_LEN);
>           port->ndev->hw_features = NETIF_F_SG |
>                                     NETIF_F_RXCSUM |
> @@ -2926,6 +2928,7 @@ static int am65_cpsw_nuss_probe(struct platform_device *pdev)
>                   return -ENOENT;
> 
>           common->rx_flow_id_base = -1;
> +       common->rx_packet_max = AM65_CPSW_MAX_PACKET_SIZE;
>           init_completion(&common->tdown_complete);
>           common->tx_ch_num = AM65_CPSW_DEFAULT_TX_CHNS;
>           common->pf_p0_rx_ptype_rrobin = false;
> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.h 
> b/drivers/net/ethernet/ti/am65-cpsw-nuss.h
> index f3dad2ab9828..141160223d73 100644
> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.h
> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.h
> @@ -130,6 +130,8 @@ struct am65_cpsw_common {
>           u32                     tx_ch_rate_msk;
>           u32                     rx_flow_id_base;
> 
> +       int                     rx_packet_max;
> +
>           struct am65_cpsw_tx_chn tx_chns[AM65_CPSW_MAX_TX_QUEUES];
>           struct completion       tdown_complete;
>           atomic_t                tdown_cnt;

...

-- 
Regards,
Siddharth.

[PATCH 3/3] net: ethernet: ti: am65-cpsw: Add device tree property to set max MTU

[Sanjuán García, Jorge]

The switch supports ethernet frame sizes between 64 and 2024 bytes
(including VLAN) as stated in the technical reference manual.

This patch adds a new devicetree property so the switch ports can
be configured with an MTU higher than the standar 1500 bytes, making
the max frame length configured on the registers and the max_mtu
advertised on the network device consistent.

Signed-off-by: Jorge Sanjuan Garcia <jorge.sanjuangarcia@duagon.com>
---
 drivers/net/ethernet/ti/am65-cpsw-nuss.c | 18 ++++++++++++++----
 drivers/net/ethernet/ti/am65-cpsw-nuss.h |  1 +
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index a920146d7a60..6a5c8b6e03f4 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -56,7 +56,7 @@
 #define AM65_CPSW_MAX_PORTS	8
 
 #define AM65_CPSW_MIN_PACKET_SIZE	VLAN_ETH_ZLEN
-#define AM65_CPSW_MAX_PACKET_SIZE	(VLAN_ETH_FRAME_LEN + ETH_FCS_LEN)
+#define AM65_CPSW_MAX_PACKET_SIZE	2024
 
 #define AM65_CPSW_REG_CTL		0x004
 #define AM65_CPSW_REG_STAT_PORT_EN	0x014
@@ -2198,8 +2198,7 @@ am65_cpsw_nuss_init_port_ndev(struct am65_cpsw_common *common, u32 port_idx)
 	eth_hw_addr_set(port->ndev, port->slave.mac_addr);
 
 	port->ndev->min_mtu = AM65_CPSW_MIN_PACKET_SIZE;
-	port->ndev->max_mtu = common->rx_packet_max -
-			      (VLAN_ETH_HLEN + ETH_FCS_LEN);
+	port->ndev->max_mtu = common->max_mtu;
 	port->ndev->hw_features = NETIF_F_SG |
 				  NETIF_F_RXCSUM |
 				  NETIF_F_HW_CSUM |
@@ -2927,8 +2926,19 @@ static int am65_cpsw_nuss_probe(struct platform_device *pdev)
 	if (common->port_num < 1 || common->port_num > AM65_CPSW_MAX_PORTS)
 		return -ENOENT;
 
+	common->max_mtu = VLAN_ETH_DATA_LEN;
+	of_property_read_u32(dev->of_node, "max-frame-size", &common->max_mtu);
+
+	common->rx_packet_max = common->max_mtu + VLAN_ETH_HLEN + ETH_FCS_LEN;
+	if (common->rx_packet_max > AM65_CPSW_MAX_PACKET_SIZE) {
+		common->rx_packet_max = AM65_CPSW_MAX_PACKET_SIZE;
+		common->max_mtu = AM65_CPSW_MAX_PACKET_SIZE -
+				  (VLAN_ETH_HLEN + ETH_FCS_LEN);
+	}
+
+	dev_info(common->dev, "Max RX packet size set to %d\n", common->rx_packet_max);
+
 	common->rx_flow_id_base = -1;
-	common->rx_packet_max = AM65_CPSW_MAX_PACKET_SIZE;
 	init_completion(&common->tdown_complete);
 	common->tx_ch_num = AM65_CPSW_DEFAULT_TX_CHNS;
 	common->pf_p0_rx_ptype_rrobin = false;
diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.h b/drivers/net/ethernet/ti/am65-cpsw-nuss.h
index 141160223d73..3bb0ff94a46a 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.h
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.h
@@ -130,6 +130,7 @@ struct am65_cpsw_common {
 	u32			tx_ch_rate_msk;
 	u32			rx_flow_id_base;
 
+	int			max_mtu;
 	int			rx_packet_max;
 
 	struct am65_cpsw_tx_chn	tx_chns[AM65_CPSW_MAX_TX_QUEUES];
-- 
2.34.1

Re: [PATCH 3/3] net: ethernet: ti: am65-cpsw: Add device tree property to set max MTU

[Siddharth Vadapalli]

Hello,

On 02-01-2024 13:49, Sanjuán García, Jorge wrote:
> The switch supports ethernet frame sizes between 64 and 2024 bytes
> (including VLAN) as stated in the technical reference manual.

Could you please share the source for the "2024 bytes" mentioned above?
In J7200 SoC's TRM, I see support for up to 9604 bytes (including VLAN)
in the "CPSW_PN_RX_MAXLEN_REG_k" register description for CPSW5G
instance of CPSW.

> 
> This patch adds a new devicetree property so the switch ports can
> be configured with an MTU higher than the standar 1500 bytes, making

nitpick: standar/standard.

> the max frame length configured on the registers and the max_mtu
> advertised on the network device consistent.
> 
> Signed-off-by: Jorge Sanjuan Garcia <jorge.sanjuangarcia@duagon.com>
> ---

For patches which add new features, please use the subject prefix
[PATCH net-next].

>   drivers/net/ethernet/ti/am65-cpsw-nuss.c | 18 ++++++++++++++----
>   drivers/net/ethernet/ti/am65-cpsw-nuss.h |  1 +
>   2 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c 
> b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> index a920146d7a60..6a5c8b6e03f4 100644
> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> @@ -56,7 +56,7 @@
>   #define AM65_CPSW_MAX_PORTS     8
> 
>   #define AM65_CPSW_MIN_PACKET_SIZE       VLAN_ETH_ZLEN
> -#define AM65_CPSW_MAX_PACKET_SIZE      (VLAN_ETH_FRAME_LEN + ETH_FCS_LEN)
> +#define AM65_CPSW_MAX_PACKET_SIZE      2024
> 
>   #define AM65_CPSW_REG_CTL               0x004
>   #define AM65_CPSW_REG_STAT_PORT_EN      0x014
> @@ -2198,8 +2198,7 @@ am65_cpsw_nuss_init_port_ndev(struct am65_cpsw_common 
> *common, u32 port_idx)
>           eth_hw_addr_set(port->ndev, port->slave.mac_addr);
> 
>           port->ndev->min_mtu = AM65_CPSW_MIN_PACKET_SIZE;
> -       port->ndev->max_mtu = common->rx_packet_max -
> -                             (VLAN_ETH_HLEN + ETH_FCS_LEN);
> +       port->ndev->max_mtu = common->max_mtu;

This seems to be modifying what was added in just the previous patch.
Isn't it better to merge these changes into a single patch?

>           port->ndev->hw_features = NETIF_F_SG |
>                                     NETIF_F_RXCSUM |
>                                     NETIF_F_HW_CSUM |
> @@ -2927,8 +2926,19 @@ static int am65_cpsw_nuss_probe(struct platform_device *pdev)
>           if (common->port_num < 1 || common->port_num > AM65_CPSW_MAX_PORTS)
>                   return -ENOENT;
> 
> +       common->max_mtu = VLAN_ETH_DATA_LEN;
> +       of_property_read_u32(dev->of_node, "max-frame-size", &common->max_mtu);

The device-tree property "max-frame-size" is a port-specific property.
Therefore, it is wrong to expect the property to be present at the CPSW
node level instead of being present within each port in the
"ethernet-ports" node. This section should be moved into the function:
am65_cpsw_nuss_init_slave_ports()
which parses the device-tree nodes for each port. The "max-frame-size"
property can be stored there on a per-port basis within a newly added
member in "struct am65_cpsw_port" as mentioned in my previous mail for
patch 2/3.

> +
> +       common->rx_packet_max = common->max_mtu + VLAN_ETH_HLEN + ETH_FCS_LEN;
> +       if (common->rx_packet_max > AM65_CPSW_MAX_PACKET_SIZE) {
> +               common->rx_packet_max = AM65_CPSW_MAX_PACKET_SIZE;
> +               common->max_mtu = AM65_CPSW_MAX_PACKET_SIZE -
> +                                 (VLAN_ETH_HLEN + ETH_FCS_LEN);
> +       }
> +
> +       dev_info(common->dev, "Max RX packet size set to %d\n", 
> common->rx_packet_max);
> +
>           common->rx_flow_id_base = -1;
> -       common->rx_packet_max = AM65_CPSW_MAX_PACKET_SIZE;
>           init_completion(&common->tdown_complete);
>           common->tx_ch_num = AM65_CPSW_DEFAULT_TX_CHNS;
>           common->pf_p0_rx_ptype_rrobin = false;
> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.h 
> b/drivers/net/ethernet/ti/am65-cpsw-nuss.h
> index 141160223d73..3bb0ff94a46a 100644
> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.h
> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.h
> @@ -130,6 +130,7 @@ struct am65_cpsw_common {
>           u32                     tx_ch_rate_msk;
>           u32                     rx_flow_id_base;
> 
> +       int                     max_mtu;
>           int                     rx_packet_max;
> 
>           struct am65_cpsw_tx_chn tx_chns[AM65_CPSW_MAX_TX_QUEUES];

...

-- 
Regards,
Siddharth.

Re: [PATCH 3/3] net: ethernet: ti: am65-cpsw: Add device tree property to set max MTU

[Sanjuán García, Jorge]

On Tue, 2024-01-02 at 16:03 +0530, Siddharth Vadapalli wrote:
> [No suele recibir correo electrónico de s-vadapalli@ti.com. Descubra
> por qué esto es importante en
> https://aka.ms/LearnAboutSenderIdentification ]
> 
> Hello,

Hello,

First of all thanks for the quick review. Some comments bellow:

> 
> On 02-01-2024 13:49, Sanjuán García, Jorge wrote:
> > The switch supports ethernet frame sizes between 64 and 2024 bytes
> > (including VLAN) as stated in the technical reference manual.
> 
> Could you please share the source for the "2024 bytes" mentioned
> above?
> In J7200 SoC's TRM, I see support for up to 9604 bytes (including
> VLAN)
> in the "CPSW_PN_RX_MAXLEN_REG_k" register description for CPSW5G
> instance of CPSW.
> 

The 2024 bytes as max value I got it from the AM6442 TRF which is the
SoC I have been working on. At least for port 0, the register
CPSW_P0_RX_MAXLEN_REG is documented as: "The maximum value is
9604 (including VLAN) when fifo_blk_size = 4. When fifo_blk_size = 1
the maximum value is 2024 (including VLAN)". It is not clear to me how
the fifo_blk_size should work from the reference manual so I kept it
safe to those 2024 bytes. Please let me know if something else should
be considered.

> > 
> > This patch adds a new devicetree property so the switch ports can
> > be configured with an MTU higher than the standar 1500 bytes,
> > making
> 
> nitpick: standar/standard.

Oops.

> 
> > the max frame length configured on the registers and the max_mtu
> > advertised on the network device consistent.
> > 
> > Signed-off-by: Jorge Sanjuan Garcia
> > <jorge.sanjuangarcia@duagon.com>
> > ---
> 
> For patches which add new features, please use the subject prefix
> [PATCH net-next].
> 
> >   drivers/net/ethernet/ti/am65-cpsw-nuss.c | 18 ++++++++++++++----
> >   drivers/net/ethernet/ti/am65-cpsw-nuss.h |  1 +
> >   2 files changed, 15 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> > b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> > index a920146d7a60..6a5c8b6e03f4 100644
> > --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> > +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> > @@ -56,7 +56,7 @@
> >   #define AM65_CPSW_MAX_PORTS     8
> > 
> >   #define AM65_CPSW_MIN_PACKET_SIZE       VLAN_ETH_ZLEN
> > -#define AM65_CPSW_MAX_PACKET_SIZE      (VLAN_ETH_FRAME_LEN +
> > ETH_FCS_LEN)
> > +#define AM65_CPSW_MAX_PACKET_SIZE      2024
> > 
> >   #define AM65_CPSW_REG_CTL               0x004
> >   #define AM65_CPSW_REG_STAT_PORT_EN      0x014
> > @@ -2198,8 +2198,7 @@ am65_cpsw_nuss_init_port_ndev(struct
> > am65_cpsw_common
> > *common, u32 port_idx)
> >           eth_hw_addr_set(port->ndev, port->slave.mac_addr);
> > 
> >           port->ndev->min_mtu = AM65_CPSW_MIN_PACKET_SIZE;
> > -       port->ndev->max_mtu = common->rx_packet_max -
> > -                             (VLAN_ETH_HLEN + ETH_FCS_LEN);
> > +       port->ndev->max_mtu = common->max_mtu;
> 
> This seems to be modifying what was added in just the previous patch.
> Isn't it better to merge these changes into a single patch?

Yeah. I was not sure about whether it would be best to split the new
struct member and the device tree parsing into two patches. I'll merge
patches 2/3 and 3/3 of this series as one patch with the updates.

> 
> >           port->ndev->hw_features = NETIF_F_SG |
> >                                     NETIF_F_RXCSUM |
> >                                     NETIF_F_HW_CSUM |
> > @@ -2927,8 +2926,19 @@ static int am65_cpsw_nuss_probe(struct
> > platform_device *pdev)
> >           if (common->port_num < 1 || common->port_num >
> > AM65_CPSW_MAX_PORTS)
> >                   return -ENOENT;
> > 
> > +       common->max_mtu = VLAN_ETH_DATA_LEN;
> > +       of_property_read_u32(dev->of_node, "max-frame-size",
> > &common->max_mtu);
> 
> The device-tree property "max-frame-size" is a port-specific
> property.
> Therefore, it is wrong to expect the property to be present at the
> CPSW
> node level instead of being present within each port in the
> "ethernet-ports" node. This section should be moved into the
> function:
> am65_cpsw_nuss_init_slave_ports()
> which parses the device-tree nodes for each port. The "max-frame-
> size"
> property can be stored there on a per-port basis within a newly added
> member in "struct am65_cpsw_port" as mentioned in my previous mail
> for
> patch 2/3.
> 

That makes sense. I agree this should be a per slave port property.
I'll start putting together a version 2 doing it that way. I need to
think about what we should do with port 0's max frame size.

> > +
> > +       common->rx_packet_max = common->max_mtu + VLAN_ETH_HLEN +
> > ETH_FCS_LEN;
> > +       if (common->rx_packet_max > AM65_CPSW_MAX_PACKET_SIZE) {
> > +               common->rx_packet_max = AM65_CPSW_MAX_PACKET_SIZE;
> > +               common->max_mtu = AM65_CPSW_MAX_PACKET_SIZE -
> > +                                 (VLAN_ETH_HLEN + ETH_FCS_LEN);
> > +       }
> > +
> > +       dev_info(common->dev, "Max RX packet size set to %d\n",
> > common->rx_packet_max);
> > +
> >           common->rx_flow_id_base = -1;
> > -       common->rx_packet_max = AM65_CPSW_MAX_PACKET_SIZE;
> >           init_completion(&common->tdown_complete);
> >           common->tx_ch_num = AM65_CPSW_DEFAULT_TX_CHNS;
> >           common->pf_p0_rx_ptype_rrobin = false;
> > diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.h
> > b/drivers/net/ethernet/ti/am65-cpsw-nuss.h
> > index 141160223d73..3bb0ff94a46a 100644
> > --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.h
> > +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.h
> > @@ -130,6 +130,7 @@ struct am65_cpsw_common {
> >           u32                     tx_ch_rate_msk;
> >           u32                     rx_flow_id_base;
> > 
> > +       int                     max_mtu;
> >           int                     rx_packet_max;
> > 
> >           struct am65_cpsw_tx_chn tx_chns[AM65_CPSW_MAX_TX_QUEUES];
> 
> ...
> 
> --
> Regards,
> Siddharth.

Regards,
Jorge

Re: [PATCH 3/3] net: ethernet: ti: am65-cpsw: Add device tree property to set max MTU

[Andrew Lunn]

On Tue, Jan 02, 2024 at 08:19:15AM +0000, Sanjuán García, Jorge wrote:
> The switch supports ethernet frame sizes between 64 and 2024 bytes
> (including VLAN) as stated in the technical reference manual.
> 
> This patch adds a new devicetree property so the switch ports can
> be configured with an MTU higher than the standar 1500 bytes, making
> the max frame length configured on the registers and the max_mtu
> advertised on the network device consistent.

Why do you need a device tree property for this? How many other
drivers have a device tree property like this? Why not set
ndev->max_mtu to 2024 minus overheads?

	Andrew

Re: [PATCH 3/3] net: ethernet: ti: am65-cpsw: Add device tree property to set max MTU

[Sanjuán García, Jorge]

On Wed, 2024-01-03 at 02:42 +0100, Andrew Lunn wrote:
> [No suele recibir correo electrónico de andrew@lunn.ch. Descubra por
> qué esto es importante en
> https://aka.ms/LearnAboutSenderIdentification ]
> 
> On Tue, Jan 02, 2024 at 08:19:15AM +0000, Sanjuán García, Jorge
> wrote:
> > The switch supports ethernet frame sizes between 64 and 2024 bytes
> > (including VLAN) as stated in the technical reference manual.
> > 
> > This patch adds a new devicetree property so the switch ports can
> > be configured with an MTU higher than the standar 1500 bytes,
> > making
> > the max frame length configured on the registers and the max_mtu
> > advertised on the network device consistent.
> 
> Why do you need a device tree property for this? How many other
> drivers have a device tree property like this? Why not set
> ndev->max_mtu to 2024 minus overheads?

Hi Andrew,

There are a few drivers that set the max_mtu based on "max_frame_size"
parsed from device tree. Here is a list:

driver/net/ethernet/
  stmicro/stmmac/stmmac_platform.c
  altera/altera_tse_main.c
  socionext/netsec.c
  ibm/emac/core.c

I also considered hardcoding this to the maximum capabilities of the HW
but I ended making this a configurable frame size. I beleive this way
it is more stable as I don't know whether there may be any performance
issues if we default the max frame size on the swith registers to be
something different than the standard 1522 bytes. I need it for my use
case where there is a DSA switch connected to one port and I need some
extra room for the DSA headers.

Regards,
Jorge

> 
>         Andrew

Re: [PATCH 3/3] net: ethernet: ti: am65-cpsw: Add device tree property to set max MTU

[Andrew Lunn]

> There are a few drivers that set the max_mtu based on "max_frame_size"
> parsed from device tree. Here is a list:
> 
> driver/net/ethernet/
>   stmicro/stmmac/stmmac_platform.c
>   altera/altera_tse_main.c
>   socionext/netsec.c
>   ibm/emac/core.c

So not many.

> I also considered hardcoding this to the maximum capabilities of the HW
> but I ended making this a configurable frame size. I beleive this way
> it is more stable as I don't know whether there may be any performance
> issues if we default the max frame size on the swith registers to be
> something different than the standard 1522 bytes. I need it for my use
> case where there is a DSA switch connected to one port and I need some
> extra room for the DSA headers.

Generally, you just set max_mtu should not have any performance
impacts, since the MTU will still default to 1500. The user has to
take an action to change the MTU and only then could it have any
impact, if implemented correctly.

DT should describe hardware, not configuration of the hardware, and
this is clearly configuration of hardware.

	Andrew