diff mbox series

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

Message ID 20240102081825.14635-4-jorge.sanjuangarcia@duagon.com
State New
Headers
Series net: ethernet: ti: am65-cpsw: Allow for MTU values |

Commit Message

Sanjuán García, Jorge Jan. 2, 2024, 8:19 a.m. UTC 
  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(-)

Comments

Siddharth Vadapalli Jan. 2, 2024, 10:33 a.m. UTC | #1 
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];

...
Sanjuán García, Jorge Jan. 2, 2024, 3:27 p.m. UTC | #2 
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
Andrew Lunn Jan. 3, 2024, 1:42 a.m. UTC | #3 
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
Sanjuán García, Jorge Jan. 3, 2024, 10:11 a.m. UTC | #4 
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
Andrew Lunn Jan. 3, 2024, 1:39 p.m. UTC | #5 
> 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
diff mbox series

Patch

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];