RE: [PATCH v5 3/6] net: dsa: microchip: ksz8795: move register offsets and shifts to separate struct

[<Tristram.Ha@microchip.com>]

> In order to get this driver used with other switches the functions need
> to use different offsets and register shifts. This patch changes the
> direct use of the register defines to register description structures,
> which can be set depending on the chips register layout.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> 
> ---
> v1 -> v4: - extracted this change from bigger previous patch
> v4 -> v5: - added missing variables in ksz8_r_vlan_entries
>           - moved shifts, masks and registers to arrays indexed by enums
>           - using unsigned types where possible
> ---
>  drivers/net/dsa/microchip/ksz8.h        |  69 +++++++
>  drivers/net/dsa/microchip/ksz8795.c     | 261 +++++++++++++++++-------
>  drivers/net/dsa/microchip/ksz8795_reg.h |  85 --------
>  3 files changed, 253 insertions(+), 162 deletions(-)
>  create mode 100644 drivers/net/dsa/microchip/ksz8.h

Sorry for not respond to these patches sooner.

There are 3 older KSZ switch families: KSZ8863/73, KSZ8895/64, and KSZ8795/94.
The newer KSZ8795 is not considered an upgrade for KSZ8895, so some of
these switch registers are moved around and some features are dropped.

It is best to have one driver to support all 3 switches, but some operations are
Incompatible so it may be better to keep the drivers separate for now.

For basic operations those issues may not occur so it seems simple to have
one driver handling all 3 switches.  I will come up with a list of those
incompatibilities.

The tail tag format of KSZ8863 is different from KSZ8895 and KSZ8795, but
because of the DSA driver implementation that issue never comes up.

> -static void ksz8_from_vlan(u16 vlan, u8 *fid, u8 *member, u8 *valid)
> +static void ksz8_from_vlan(struct ksz_device *dev, u32 vlan, u8 *fid,
> +                          u8 *member, u8 *valid)
>  {
> -       *fid = vlan & VLAN_TABLE_FID;
> -       *member = (vlan & VLAN_TABLE_MEMBERSHIP) >>
> VLAN_TABLE_MEMBERSHIP_S;
> -       *valid = !!(vlan & VLAN_TABLE_VALID);
> +       struct ksz8 *ksz8 = dev->priv;
> +       const u32 *masks = ksz8->masks;
> +       const u8 *shifts = ksz8->shifts;
> +
> +       *fid = vlan & masks[VLAN_TABLE_FID];
> +       *member = (vlan & masks[VLAN_TABLE_MEMBERSHIP]) >>
> +                       shifts[VLAN_TABLE_MEMBERSHIP_S];
> +       *valid = !!(vlan & masks[VLAN_TABLE_VALID]);
>  }
> 
> -static void ksz8_to_vlan(u8 fid, u8 member, u8 valid, u16 *vlan)
> +static void ksz8_to_vlan(struct ksz_device *dev, u8 fid, u8 member, u8
> valid,
> +                        u32 *vlan)
>  {
> +       struct ksz8 *ksz8 = dev->priv;
> +       const u32 *masks = ksz8->masks;
> +       const u8 *shifts = ksz8->shifts;
> +
>         *vlan = fid;
> -       *vlan |= (u16)member << VLAN_TABLE_MEMBERSHIP_S;
> +       *vlan |= (u16)member << shifts[VLAN_TABLE_MEMBERSHIP_S];
>         if (valid)
> -               *vlan |= VLAN_TABLE_VALID;
> +               *vlan |= masks[VLAN_TABLE_VALID];
>  }
> 
>  static void ksz8_r_vlan_entries(struct ksz_device *dev, u16 addr)
>  {
> +       struct ksz8 *ksz8 = dev->priv;
> +       const u8 *shifts = ksz8->shifts;
>         u64 data;
>         int i;
> 
> @@ -418,7 +509,7 @@ static void ksz8_r_vlan_entries(struct ksz_device
> *dev, u16 addr)
>         addr *= dev->phy_port_cnt;
>         for (i = 0; i < dev->phy_port_cnt; i++) {
>                 dev->vlan_cache[addr + i].table[0] = (u16)data;
> -               data >>= VLAN_TABLE_S;
> +               data >>= shifts[VLAN_TABLE];
>         }
>  }
> 
> @@ -454,6 +545,8 @@ static void ksz8_w_vlan_table(struct ksz_device *dev,
> u16 vid, u32 vlan)
> 

The VLAN table operation in KSZ8863 is completely different from KSZ8795.

> -/**
> - * VLAN_TABLE_FID                      00-007F007F-007F007F
> - * VLAN_TABLE_MEMBERSHIP               00-0F800F80-0F800F80
> - * VLAN_TABLE_VALID                    00-10001000-10001000
> - */
> -
> -#define VLAN_TABLE_FID                 0x007F
> -#define VLAN_TABLE_MEMBERSHIP          0x0F80
> -#define VLAN_TABLE_VALID               0x1000
> -
> -#define VLAN_TABLE_MEMBERSHIP_S                7
> -#define VLAN_TABLE_S                   16
> -

In KSZ8795 you can use all 4096 VLAN id.  Each entry in the table contains
4 VID.  The FID is actually used for lookup and there is a limit, so you need
to convert VID to FID when programming the VLAN table.

The only effect of using FID is the same MAC address can be used in
different VLANs.

In KSZ8863 there are only 16 entries in the VLAN table.  Just like static
MAC table each entry is programmed using an index.  The entry contains
the VID, which does not have any relationship with the index unlike in
KSZ8795.

The number of FID is also limited to 16.  So the maximum VLAN used is 16.

>  /**
>   * MIB_COUNTER_VALUE                   00-00000000-3FFFFFFF
>   * MIB_TOTAL_BYTES                     00-0000000F-FFFFFFFF
> @@ -956,9 +874,6 @@
>   * MIB_COUNTER_OVERFLOW                        00-00000040-00000000
>   */
> 
> -#define MIB_COUNTER_OVERFLOW           BIT(6)
> -#define MIB_COUNTER_VALID              BIT(5)
> -
>  #define MIB_COUNTER_VALUE              0x3FFFFFFF

The MIB counters are also a little different in KSZ8863 and KSZ8795.
KSZ8863 may have smaller total bytes.  In normal operation this issue may
not come up.