Re: [RFC Patch net-next v2 02/15] net: dsa: microchip: move switch chip_id detection to ksz_common

[<Arun.Ramadoss@microchip.com>]

Hi Vladimir,
Thanks for the comment. 

On Mon, 2022-06-13 at 12:18 +0300, Vladimir Oltean wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Mon, May 30, 2022 at 04:12:44PM +0530, Arun Ramadoss wrote:
> > KSZ87xx and KSZ88xx have chip_id representation at reg location 0.
> > And
> > KSZ9477 compatible switch and LAN937x switch have same chip_id
> > detection
> > at location 0x01 and 0x02. To have the common switch detect
> > functionality for ksz switches, ksz_switch_detect function is
> > introduced.
> > 
> > Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com>
> > ---
> >  drivers/net/dsa/microchip/ksz8795.c     | 46 ---------------
> >  drivers/net/dsa/microchip/ksz8795_reg.h | 13 -----
> >  drivers/net/dsa/microchip/ksz9477.c     | 21 -------
> >  drivers/net/dsa/microchip/ksz9477_reg.h |  1 -
> >  drivers/net/dsa/microchip/ksz_common.c  | 78
> > +++++++++++++++++++++++--
> >  drivers/net/dsa/microchip/ksz_common.h  | 19 +++++-
> >  6 files changed, 92 insertions(+), 86 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/microchip/ksz8795.c
> > b/drivers/net/dsa/microchip/ksz8795.c
> > index 12a599d5e61a..927db57d02db 100644
> > --- a/drivers/net/dsa/microchip/ksz8795.c
> > +++ b/drivers/net/dsa/microchip/ksz8795.c
> > @@ -1424,51 +1424,6 @@ static u32 ksz8_get_port_addr(int port, int
> > offset)
> >       return PORT_CTRL_ADDR(port, offset);
> >  }
> > 
> > -static int ksz8_switch_detect(struct ksz_device *dev)
> > -{
> > -     u8 id1, id2;
> > -     u16 id16;
> > -     int ret;
> > -
> > -     /* read chip id */
> > -     ret = ksz_read16(dev, REG_CHIP_ID0, &id16);
> > -     if (ret)
> > -             return ret;
> > -
> > -     id1 = id16 >> 8;
> > -     id2 = id16 & SW_CHIP_ID_M;
> > -
> > -     switch (id1) {
> > -     case KSZ87_FAMILY_ID:
> > -             if ((id2 != CHIP_ID_94 && id2 != CHIP_ID_95))
> > -                     return -ENODEV;
> > -
> > -             if (id2 == CHIP_ID_95) {
> > -                     u8 val;
> > -
> > -                     id2 = 0x95;
> > -                     ksz_read8(dev, REG_PORT_STATUS_0, &val);
> 
> Could you replace all remaining occurrences of REG_PORT_STATUS_0 and
> PORT_FIBER_MODE with KSZ8_PORT_STATUS_0 and KSZ8_PORT_FIBER_MODE?
> It would be good to not have multiple definitions for the same thing.

Ok. I will update the macro unique for KSZ8 Switches.

> > -                     if (val & PORT_FIBER_MODE)
> > -                             id2 = 0x65;
> > -             } else if (id2 == CHIP_ID_94) {
> > -                     id2 = 0x94;
> > -             }
> > -             break;
> > -     case KSZ88_FAMILY_ID:
> > -             if (id2 != CHIP_ID_63)
> > -                     return -ENODEV;
> > -             break;
> > -     default:
> > -             dev_err(dev->dev, "invalid family id: %d\n", id1);
> > -             return -ENODEV;
> > -     }
> > -     id16 &= ~0xff;
> > -     id16 |= id2;
> > -     dev->chip_id = id16;
> > -
> > -     return 0;
> > -}
> > -
> > 
> > diff --git a/drivers/net/dsa/microchip/ksz8795_reg.h
> > b/drivers/net/dsa/microchip/ksz8795_reg.h
> > index 4109433b6b6c..50cdc2a09f5a 100644
> > --- a/drivers/net/dsa/microchip/ksz8795_reg.h
> > +++ b/drivers/net/dsa/microchip/ksz8795_reg.h
> > @@ -14,23 +14,10 @@
> >  #define KS_PRIO_M                    0x3
> >  #define KS_PRIO_S                    2
> > 
> > -#define REG_CHIP_ID0                 0x00
> > -
> > -#define KSZ87_FAMILY_ID                      0x87
> > -#define KSZ88_FAMILY_ID                      0x88
> > -
> > -#define REG_CHIP_ID1                 0x01
> > -
> > -#define SW_CHIP_ID_M                 0xF0
> > -#define SW_CHIP_ID_S                 4
> >  #define SW_REVISION_M                        0x0E
> >  #define SW_REVISION_S                        1
> >  #define SW_START                     0x01
> > 
> > -#define CHIP_ID_94                   0x60
> > -#define CHIP_ID_95                   0x90
> > -#define CHIP_ID_63                   0x30
> > -
> >  #define KSZ8863_REG_SW_RESET         0x43
> > 
> >  #define KSZ8863_GLOBAL_SOFTWARE_RESET        BIT(4)
> > diff --git a/drivers/net/dsa/microchip/ksz9477.c
> > b/drivers/net/dsa/microchip/ksz9477.c
> > index 7afc06681c02..7d3c8f6908b6 100644
> > --- a/drivers/net/dsa/microchip/ksz9477.c
> > +++ b/drivers/net/dsa/microchip/ksz9477.c
> > @@ -1360,23 +1360,6 @@ static u32 ksz9477_get_port_addr(int port,
> > int offset)
> >       return PORT_CTRL_ADDR(port, offset);
> >  }
> > 
> > 
> > diff --git a/drivers/net/dsa/microchip/ksz9477_reg.h
> > b/drivers/net/dsa/microchip/ksz9477_reg.h
> > index 7a2c8d4767af..077e35ab11b5 100644
> > --- a/drivers/net/dsa/microchip/ksz9477_reg.h
> > +++ b/drivers/net/dsa/microchip/ksz9477_reg.h
> > @@ -25,7 +25,6 @@
> > 
> >  #define REG_CHIP_ID2__1                      0x0002
> > 
> > -#define CHIP_ID_63                   0x63
> >  #define CHIP_ID_66                   0x66
> >  #define CHIP_ID_67                   0x67
> >  #define CHIP_ID_77                   0x77
> > diff --git a/drivers/net/dsa/microchip/ksz_common.c
> > b/drivers/net/dsa/microchip/ksz_common.c
> > index 9ca8c8d7740f..9057cdb5971c 100644
> > --- a/drivers/net/dsa/microchip/ksz_common.c
> > +++ b/drivers/net/dsa/microchip/ksz_common.c
> > @@ -930,6 +930,72 @@ void ksz_port_stp_state_set(struct dsa_switch
> > *ds, int port,
> >  }
> >  EXPORT_SYMBOL_GPL(ksz_port_stp_state_set);
> > 
> > 
> > diff --git a/drivers/net/dsa/microchip/ksz_common.h
> > b/drivers/net/dsa/microchip/ksz_common.h
> > index 8500eaedad67..d16c095cdefb 100644
> > --- a/drivers/net/dsa/microchip/ksz_common.h
> > +++ b/drivers/net/dsa/microchip/ksz_common.h
> > @@ -90,6 +90,7 @@ struct ksz_device {
> > 
> >       /* chip specific data */
> >       u32 chip_id;
> > +     u8 chip_rev;
> >       int cpu_port;                   /* port connected to CPU */
> >       int phy_port_cnt;
> >       phy_interface_t compat_interface;
> > @@ -182,7 +183,6 @@ struct ksz_dev_ops {
> >       void (*freeze_mib)(struct ksz_device *dev, int port, bool
> > freeze);
> >       void (*port_init_cnt)(struct ksz_device *dev, int port);
> >       int (*shutdown)(struct ksz_device *dev);
> > -     int (*detect)(struct ksz_device *dev);
> >       int (*init)(struct ksz_device *dev);
> >       void (*exit)(struct ksz_device *dev);
> >  };
> > @@ -353,6 +353,23 @@ static inline void ksz_regmap_unlock(void
> > *__mtx)
> >  #define PORT_RX_ENABLE                       BIT(1)
> >  #define PORT_LEARN_DISABLE           BIT(0)
> > 
> > +/* Switch ID Defines */
> > +#define REG_CHIP_ID0                 0x00
> > +
> > +#define SW_FAMILY_ID_M                       GENMASK(15, 8)
> > +#define KSZ87_FAMILY_ID                      0x87
> > +#define KSZ88_FAMILY_ID                      0x88
> > +
> > +#define KSZ8_PORT_STATUS_0           0x08
> > +#define KSZ8_PORT_FIBER_MODE         BIT(7)
> > +
> > +#define SW_CHIP_ID_M                 GENMASK(7, 4)
> > +#define CHIP_ID_94                   0x6
> > +#define CHIP_ID_95                   0x9
> 
> KSZ87_CHIP_ID_xxx maybe?

Ok. I will update.

> 
> > +#define CHIP_ID_63                   0x3
> 
> And KSZ88_CHIP_ID_63.

I will rename it this. 

> 
> > +
> > +#define SW_REV_ID_M                  GENMASK(7, 4)
> > +
> >  /* Regmap tables generation */
> >  #define KSZ_SPI_OP_RD                3
> >  #define KSZ_SPI_OP_WR                2
> > --
> > 2.36.1
> > 
> 
>