Received-SPF: pass (google.com: domain of linux-kernel+bounces-14248-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <162e12c7-c7c1-3bf9-48ec-05ee3c30e784@ti.com>
Date: Tue, 2 Jan 2024 16:03:11 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
CC: "davem@davemloft.net" <davem@davemloft.net>,
        "edumazet@google.com"
	<edumazet@google.com>,
        "kuba@kernel.org" <kuba@kernel.org>,
        "pabeni@redhat.com" <pabeni@redhat.com>,
        "netdev@vger.kernel.org"
	<netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>,
        <s-vadapalli@ti.com>
Subject: Re: [PATCH 3/3] net: ethernet: ti: am65-cpsw: Add device tree
 property to set max MTU
Content-Language: en-US
To: =?UTF-8?B?U2FuanXDoW4gR2FyY8OtYSwgSm9yZ2U=?=
	<Jorge.SanjuanGarcia@duagon.com>
References: <20240102081825.14635-1-jorge.sanjuangarcia@duagon.com>
 <c025f2f9-ca2c-4fdb-adb1-803745fded0c.a613f387-0b3b-49fd-9401-3a0ed0c1f80e.20d242d9-3999-48e2-8a9e-cd0f9ba3351c@emailsignatures365.codetwo.com>
 <20240102081825.14635-4-jorge.sanjuangarcia@duagon.com>
From: Siddharth Vadapalli <s-vadapalli@ti.com>
In-Reply-To: <20240102081825.14635-4-jorge.sanjuangarcia@duagon.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit

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.