Re: [PATCH v1 net] net: dsa: microchip: correct KSZ8795 static MAC table access

[Horatiu Vultur <horatiu.vultur@microchip.com>]

The 07/10/2023 17:10, Tristram.Ha@microchip.com wrote:
> From: Tristram Ha <Tristram.Ha@microchip.com>

Hi Tristram,

It looks like you forgot again to add all the maintainers to the email
thread. Was it something wrong with the command
./scripts/get_maintainer.pl?

> 
> The KSZ8795 driver code was modified to use on KSZ8863/73, which has
> different register definitions.  Some of the new KSZ8795 register
> information are wrong compared to previous code.
> 
> KSZ8795 also behaves differently in that the STATIC_MAC_TABLE_USE_FID
> and STATIC_MAC_TABLE_FID bits are off by 1 when doing MAC table reading
> than writing.  To compensate that a special code was added to shift the
> register value by 1 before applying those bits.  This is wrong when the
> code is running on KSZ8863, so this special code is only executed when
> KSZ8795 is detected.
> 
> Fixes: 4b20a07e103f ("net: dsa: microchip: ksz8795: add support for ksz88xx chips")
> Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>

Other than what I mentioned aboved, it looks OK.
Reviewed-by: Horatiu Vultur <horatiu.vultur@microchip.com>

> ---
> v1
> - Use correct commit for fixes
> 
>  drivers/net/dsa/microchip/ksz8795.c    | 8 +++++++-
>  drivers/net/dsa/microchip/ksz_common.c | 8 ++++----
>  drivers/net/dsa/microchip/ksz_common.h | 7 +++++++
>  3 files changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
> index 84d502589f8e..91aba470fb2f 100644
> --- a/drivers/net/dsa/microchip/ksz8795.c
> +++ b/drivers/net/dsa/microchip/ksz8795.c
> @@ -506,7 +506,13 @@ static int ksz8_r_sta_mac_table(struct ksz_device *dev, u16 addr,
>  		(data_hi & masks[STATIC_MAC_TABLE_FWD_PORTS]) >>
>  			shifts[STATIC_MAC_FWD_PORTS];
>  	alu->is_override = (data_hi & masks[STATIC_MAC_TABLE_OVERRIDE]) ? 1 : 0;
> -	data_hi >>= 1;
> +
> +	/* KSZ8795 family switches have STATIC_MAC_TABLE_USE_FID and
> +	 * STATIC_MAC_TABLE_FID definitions off by 1 when doing read on the
> +	 * static MAC table compared to doing write.
> +	 */
> +	if (ksz_is_ksz87xx(dev))
> +		data_hi >>= 1;
>  	alu->is_static = true;
>  	alu->is_use_fid = (data_hi & masks[STATIC_MAC_TABLE_USE_FID]) ? 1 : 0;
>  	alu->fid = (data_hi & masks[STATIC_MAC_TABLE_FID]) >>
> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 813b91a816bb..b18cd170ec06 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -331,13 +331,13 @@ static const u32 ksz8795_masks[] = {
>  	[STATIC_MAC_TABLE_VALID]	= BIT(21),
>  	[STATIC_MAC_TABLE_USE_FID]	= BIT(23),
>  	[STATIC_MAC_TABLE_FID]		= GENMASK(30, 24),
> -	[STATIC_MAC_TABLE_OVERRIDE]	= BIT(26),
> -	[STATIC_MAC_TABLE_FWD_PORTS]	= GENMASK(24, 20),
> +	[STATIC_MAC_TABLE_OVERRIDE]	= BIT(22),
> +	[STATIC_MAC_TABLE_FWD_PORTS]	= GENMASK(20, 16),
>  	[DYNAMIC_MAC_TABLE_ENTRIES_H]	= GENMASK(6, 0),
> -	[DYNAMIC_MAC_TABLE_MAC_EMPTY]	= BIT(8),
> +	[DYNAMIC_MAC_TABLE_MAC_EMPTY]	= BIT(7),
>  	[DYNAMIC_MAC_TABLE_NOT_READY]	= BIT(7),
>  	[DYNAMIC_MAC_TABLE_ENTRIES]	= GENMASK(31, 29),
> -	[DYNAMIC_MAC_TABLE_FID]		= GENMASK(26, 20),
> +	[DYNAMIC_MAC_TABLE_FID]		= GENMASK(22, 16),
>  	[DYNAMIC_MAC_TABLE_SRC_PORT]	= GENMASK(26, 24),
>  	[DYNAMIC_MAC_TABLE_TIMESTAMP]	= GENMASK(28, 27),
>  	[P_MII_TX_FLOW_CTRL]		= BIT(5),
> diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
> index 28444e5924f9..a4de58847dea 100644
> --- a/drivers/net/dsa/microchip/ksz_common.h
> +++ b/drivers/net/dsa/microchip/ksz_common.h
> @@ -601,6 +601,13 @@ static inline void ksz_regmap_unlock(void *__mtx)
>  	mutex_unlock(mtx);
>  }
>  
> +static inline bool ksz_is_ksz87xx(struct ksz_device *dev)
> +{
> +	return dev->chip_id == KSZ8795_CHIP_ID ||
> +	       dev->chip_id == KSZ8794_CHIP_ID ||
> +	       dev->chip_id == KSZ8765_CHIP_ID;
> +}
> +
>  static inline bool ksz_is_ksz88x3(struct ksz_device *dev)
>  {
>  	return dev->chip_id == KSZ8830_CHIP_ID;
> -- 
> 2.17.1
> 

-- 
/Horatiu