Messages in this thread | | | From | Sanjuán García, Jorge <> | Subject | Re: [PATCH 3/3] net: ethernet: ti: am65-cpsw: Add device tree property to set max MTU | Date | Tue, 2 Jan 2024 15:27:26 +0000 |
| |
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
| |