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

[Siddharth Vadapalli <s-vadapalli@ti.com>]

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.