Re: [PATCH net-next v1 1/1] net: dsa: microchip: ksz8: add MTU configuration support

[Arun.Ramadoss]

Hi Oleksij,

On Thu, 2022-11-03 at 10:13 +0100, Oleksij Rempel wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> Make MTU configurable on KSZ87xx and KSZ88xx series of switches.
> 
> Before this patch, pre-configured behavior was different on different
> switch series, due to opposite meaning of the same bit:
> - KSZ87xx: Reg 4, Bit 1 - if 1, max frame size is 1532; if 0 - 1514
> - KSZ88xx: Reg 4, Bit 1 - if 1, max frame size is 1514; if 0 - 1532
> 
> Since the code was telling "... SW_LEGAL_PACKET_DISABLE, true)", I
> assume, the idea was to set max frame size to 1532.
> 
> With this patch, by setting MTU size 1500, both switch series will be
> configured to the 1532 frame limit.
> 
> This patch was tested on KSZ8873.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>
> ---
>  drivers/net/dsa/microchip/ksz8.h        |  2 +
>  drivers/net/dsa/microchip/ksz8795.c     | 74
> ++++++++++++++++++++++++-
>  drivers/net/dsa/microchip/ksz8795_reg.h |  9 +++
>  drivers/net/dsa/microchip/ksz_common.c  |  2 +
>  4 files changed, 85 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz8.h
> b/drivers/net/dsa/microchip/ksz8.h
> index 8582b4b67d98..027b92f5fa73 100644
> --- a/drivers/net/dsa/microchip/ksz8.h
> +++ b/drivers/net/dsa/microchip/ksz8.h
> @@ -57,5 +57,7 @@ int ksz8_reset_switch(struct ksz_device *dev);
>  int ksz8_switch_detect(struct ksz_device *dev);
>  int ksz8_switch_init(struct ksz_device *dev);
>  void ksz8_switch_exit(struct ksz_device *dev);
> +int ksz8_change_mtu(struct ksz_device *dev, int port, int mtu);
> +int ksz8_max_mtu(struct ksz_device *dev, int port);
> 
>  #endif
> diff --git a/drivers/net/dsa/microchip/ksz8795.c
> b/drivers/net/dsa/microchip/ksz8795.c
> index bd3b133e7085..fd2539aabb2c 100644
> --- a/drivers/net/dsa/microchip/ksz8795.c
> +++ b/drivers/net/dsa/microchip/ksz8795.c
> @@ -76,6 +76,78 @@ int ksz8_reset_switch(struct ksz_device *dev)
>         return 0;
>  }
> 
> +static int ksz8863_change_mtu(struct ksz_device *dev, int port, int
> max_frame)
> +{
> +       u8 ctrl2 = 0;
> +
> +       if (max_frame <= KSZ8863_LEGAL_PACKET_SIZE)
> +               ctrl2 |= KSZ8863_LEGAL_PACKET_ENABLE;
> +       else if (max_frame > KSZ8863_NORMAL_PACKET_SIZE)
> +               ctrl2 |= KSZ8863_HUGE_PACKET_ENABLE;
> +
> +       return regmap_update_bits(dev->regmap[0], REG_SW_CTRL_2,
> +                                 KSZ8863_LEGAL_PACKET_ENABLE
> +                                 | KSZ8863_HUGE_PACKET_ENABLE,
> ctrl2);
> +}
Suggestion:
regmap_update_bits, can you create a macro like ksz_rmw8 similar to
what we have ksz_prmw8 in ksz_common.h. So that it can used across the
ksz and lan937x switches.

> +
> +static int ksz8795_change_mtu(struct ksz_device *dev, int port, int
> max_frame)
> +{
> +       u8 ctrl1 = 0, ctrl2 = 0;
> +       int ret;
> +
> +       if (max_frame > KSZ8863_LEGAL_PACKET_SIZE)
> +               ctrl2 |= SW_LEGAL_PACKET_DISABLE;
> +       else if (max_frame > KSZ8863_NORMAL_PACKET_SIZE)
> +               ctrl1 |= SW_HUGE_PACKET;
> +
> +       ret = regmap_update_bits(dev->regmap[0], REG_SW_CTRL_1,
> +                                SW_HUGE_PACKET, ctrl1);
> +       if (ret)
> +               return ret;
> +
> +       return regmap_update_bits(dev->regmap[0], REG_SW_CTRL_2,
> +                                SW_LEGAL_PACKET_DISABLE, ctrl2);
> +}
> +
> +int ksz8_change_mtu(struct ksz_device *dev, int port, int mtu)
> +{
> +       u16 frame_size, max_frame = 0;
> +       int i;
> +
> +       frame_size = mtu + VLAN_ETH_HLEN + ETH_FCS_LEN;
> +
> +       /* Cache the per-port MTU setting */
> +       dev->ports[port].max_frame = frame_size;
> +
> +       for (i = 0; i < dev->info->port_cnt; i++)
> +               max_frame = max(max_frame, dev->ports[i].max_frame);

This port caching snippet is present in ksz9477_change_mtu function
also. It can be moved to ksz_change_mtu and remove it in
ksz9477_change_mtu. 

> +
> +       switch (dev->chip_id) {
> +       case KSZ8795_CHIP_ID:
> +       case KSZ8794_CHIP_ID:
> +       case KSZ8765_CHIP_ID:
> +               return ksz8795_change_mtu(dev, port, max_frame);
> +       case KSZ8830_CHIP_ID:
> +               return ksz8863_change_mtu(dev, port, max_frame);
> +       }
> +
> +       return -EOPNOTSUPP;
> +}
> +
> +int ksz8_max_mtu(struct ksz_device *dev, int port)
> +{
> +       switch (dev->chip_id) {
> +       case KSZ8795_CHIP_ID:
> +       case KSZ8794_CHIP_ID:
> +       case KSZ8765_CHIP_ID:
> +               return KSZ8795_HUGE_PACKET_SIZE - VLAN_ETH_HLEN -
> ETH_FCS_LEN;
> +       case KSZ8830_CHIP_ID:
> +               return KSZ8863_HUGE_PACKET_SIZE - VLAN_ETH_HLEN -
> ETH_FCS_LEN;
> +       }
> +
> +       return -EOPNOTSUPP;
> +}
> +

Intially KSZ9477 had the max_mtu which I used for the LAN937x. Since
there is no much thing to do in this function, why can't we move it
ksz_common.c and add chip id for ksz9 and lan937x & remove
ksz9477_max_mtu. It will reduce the max_mtu functioin pointer in
ksz_dev_ops.

> 
> diff --git a/drivers/net/dsa/microchip/ksz_common.c
> b/drivers/net/dsa/microchip/ksz_common.c
> index d612181b3226..40fd78951bf8 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -171,6 +171,8 @@ static const struct ksz_dev_ops ksz8_dev_ops = {
>         .reset = ksz8_reset_switch,
>         .init = ksz8_switch_init,
>         .exit = ksz8_switch_exit,
> +       .change_mtu = ksz8_change_mtu,
> +       .max_mtu = ksz8_max_mtu,
>  };
> 
>  static void ksz9477_phylink_mac_link_up(struct ksz_device *dev, int
> port,
> -
> 2.30.2
>