Re: [PATCH net-next 3/3] net: dsa: microchip: add other KSZ9477 switch variants

[Andrew Lunn <andrew@lunn.ch>]

On Tue, Feb 26, 2019 at 03:37:45PM -0800, Tristram.Ha@microchip.com wrote:
> From: Tristram Ha <Tristram.Ha@microchip.com>
> 
> Add other switches in KSZ9477 family.
> 
> KSZ9896 is a switch with 6 ports; the last one is typically used to
> connect to MAC.
> KSZ9567 is same as KSZ9897 but with 1588 PTP capability.
> KSZ8567 is same as KSZ9567 but without gigabit capability.
> KSZ9563 is same as KSZ9893 but with 1588 PTP capability.
> KSZ8563 is same as KSZ9563 but without gigabit capability.
> KSZ8565 is a switch with 5 ports; however, port 7 has to be used to
> connect to MAC.  This chip can only be set through device tree.
> 
> Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
> ---
>  drivers/net/dsa/microchip/ksz9477.c     | 93 ++++++++++++++++++++++++++++++++-
>  drivers/net/dsa/microchip/ksz9477_spi.c |  3 ++
>  drivers/net/dsa/microchip/ksz_common.c  |  4 ++
>  3 files changed, 98 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
> index 3bb548a..81e7c2f 100644
> --- a/drivers/net/dsa/microchip/ksz9477.c
> +++ b/drivers/net/dsa/microchip/ksz9477.c
> @@ -1264,6 +1264,32 @@ static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
>  		ksz_pread16(dev, port, REG_PORT_PHY_INT_ENABLE, &data16);
>  }
>  
> +#define KSZ_CHIP_NAME_SIZE		18
> +
> +static char *ksz9477_chip_names[KSZ_CHIP_NAME_SIZE] = {
> +	"Microchip KSZ9477",

This looks wrong. You are defining an array of 18 elements, not an
array of pointers to 18 byte chars.


> +	"Microchip KSZ9897",
> +	"Microchip KSZ9896",
> +	"Microchip KSZ9567",
> +	"Microchip KSZ8567",
> +	"Microchip KSZ8565",
> +	"Microchip KSZ9893",
> +	"Microchip KSZ9563",
> +	"Microchip KSZ8563",
> +};
> +
> +enum {
> +	KSZ9477_SW_CHIP,
> +	KSZ9897_SW_CHIP,
> +	KSZ9896_SW_CHIP,
> +	KSZ9567_SW_CHIP,
> +	KSZ8567_SW_CHIP,
> +	KSZ8565_SW_CHIP,
> +	KSZ9893_SW_CHIP,
> +	KSZ9563_SW_CHIP,
> +	KSZ8563_SW_CHIP,
> +};

There should be a one-to-one mapping between this enum and the array
above. You can make this clear using Designated Initializers.

> +
>  static void ksz9477_config_cpu_port(struct dsa_switch *ds)
>  {
>  	struct ksz_device *dev = ds->priv;
> @@ -1314,7 +1340,8 @@ static void ksz9477_config_cpu_port(struct dsa_switch *ds)
>  		p->vid_member = (1 << i);
>  		p->member = dev->port_mask;
>  		ksz9477_port_stp_state_set(ds, i, BR_STATE_DISABLED);
> -		p->on = 1;
> +		if (!dsa_is_unused_port(ds, i))
> +			p->on = 1;
>  		if (i < dev->phy_port_cnt)
>  			p->phy = 1;
>  		if (dev->chip_id == 0x00947700 && i == 6) {
> @@ -1406,6 +1433,7 @@ static u32 ksz9477_get_port_addr(int port, int offset)
>  
>  static int ksz9477_switch_detect(struct ksz_device *dev)
>  {
> +	int chip = -1;
>  	u8 data8;
>  	u8 id_hi;
>  	u8 id_lo;
> @@ -1448,6 +1476,12 @@ static int ksz9477_switch_detect(struct ksz_device *dev)
>  			dev->features &= ~GBIT_SUPPORT;
>  		dev->mib_port_cnt = 3;
>  		dev->phy_port_cnt = 2;
> +		if (!(data8 & SW_AVB_ABLE))
> +			chip = KSZ9893_SW_CHIP;
> +		else if (data8 & SW_QW_ABLE)
> +			chip = KSZ8563_SW_CHIP;
> +		else
> +			chip = KSZ9563_SW_CHIP;
>  	} else {
>  		/* Chip uses new XMII register definitions. */
>  		dev->features |= NEW_XMII;
> @@ -1455,6 +1489,37 @@ static int ksz9477_switch_detect(struct ksz_device *dev)
>  		/* Chip does not support gigabit. */
>  		if (!(data8 & SW_GIGABIT_ABLE))
>  			dev->features &= ~GBIT_SUPPORT;
> +		if ((id_lo & 0xf) == 6)
> +			dev->mib_port_cnt = 6;

> +		if (id_hi == FAMILY_ID_94)
> +			chip = KSZ9477_SW_CHIP;
> +		else if (id_hi == FAMILY_ID_98 && id_lo == CHIP_ID_97)
> +			chip = KSZ9897_SW_CHIP;
> +		else if (id_hi == FAMILY_ID_98 && id_lo == CHIP_ID_96)
> +			chip = KSZ9896_SW_CHIP;
> +		else if (id_hi == FAMILY_ID_95 && id_lo == CHIP_ID_67)
> +			chip = KSZ9567_SW_CHIP;
> +		else if (id_hi == FAMILY_ID_85 && id_lo == CHIP_ID_67)
> +			chip = KSZ8567_SW_CHIP;
> +		if (id_lo == CHIP_ID_67) {
> +			id_hi = FAMILY_ID_98;
> +			id_lo = CHIP_ID_97;
> +		} else if (id_lo == CHIP_ID_66) {
> +			id_hi = FAMILY_ID_98;
> +			id_lo = CHIP_ID_96;
> +		}

Maybe add a mask to ksz_chip_data table, so you can mask id_low,
id_high and then compare against chip_id?

> @@ -1495,6 +1564,15 @@ struct ksz_chip_data {
>  		.port_cnt = 7,		/* total physical port count */
>  	},
>  	{
> +		.chip_id = 0x00989600,
> +		.dev_name = "KSZ9896",
> +		.num_vlans = 4096,
> +		.num_alus = 4096,
> +		.num_statics = 16,
> +		.cpu_ports = 0x3F,	/* can be configured as cpu port */
> +		.port_cnt = 6,		/* total port count */
> +	},

  Andrew