Re: [RFC/RFT PATCH net-next 2/4] net: dsa: microchip: partial conversion to regfields API for KSZ8795 (WIP)

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Thu, Mar 16, 2023 at 06:12:48PM +0200, Vladimir Oltean wrote:
> During review, Russell King has pointed out that the current way in
> which the KSZ common driver performs register access is error-prone.
> 
> Based on the KSZ9477 software model where we have the XMII Port Control
> 0 Register (at offset 0xN300) and XMII Port Control 1 Register
> (at offset 0xN301), this driver also holds P_XMII_CTRL_0 and P_XMII_CTRL_1.
> 
> However, on some switches, fields accessed through P_XMII_CTRL_0 (for
> example P_MII_100MBIT_M or P_MII_DUPLEX_M) and fields accessed through
> P_XMII_CTRL_1 (for example P_MII_SEL_M) may end up accessing the same
> hardware register. Or at least, that was certainly the impression after
> commit
> https://patchwork.kernel.org/project/netdevbpf/patch/20230315231916.2998480-1-vladimir.oltean@nxp.com/
> (sorry, can't reference the sha1sum of an unmerged commit), because for
> ksz8795_regs[], P_XMII_CTRL_0 and P_XMII_CTRL_1 now have the same value.
> 
> But the reality is far more interesting. Opening a KSZ8795 datasheet, I
> see that out of the register fields accessed via P_XMII_CTRL_0:
> - what the driver names P_MII_SEL_M *is* actually "GMII/MII Mode Select"
>   (bit 2) of the Port 5 Interface Control 6, address 0x56 (all good here)
> - what the driver names P_MII_100MBIT_M is actually "Switch SW5-MII/RMII
>   Speed" (bit 4) of the Global Control 4 register, address 0x06.
> 
> That is a huge problem, because the driver cannot access this register
> for KSZ8795 in its current form, even if that register exists. This
> creates an even stronger motivation to try to do something to normalize
> the way in which this driver abstracts away register field movement from
> one switch family to another.
> 
> As I had proposed in that thread, reg_fields from regmap propose to
> solve exactly this problem. This patch contains a COMPLETELY UNTESTED
> rework of the driver, so that accesses done through the following
> registers (for demonstration purposes):
> - REG_IND_BYTE - a global register
> - REG_IND_CTRL_0 - another global register
> - P_LOCAL_CTRL - a port register
> - P_FORCE_CTRL - another port register
> - P_XMII_CTRL_0 and P_XMII_CTRL_1 - either port register, or global
>   registers, depending on which manual you read!
> 
> are converted to the regfields API.
> 
> !! WARNING !! I only attempted to add a ksz_reg_fields structure for
> KSZ8795. The other switch families will currently crash!
> 
> For easier partial migration, I have renamed the "REG_" or "P_" prefixes
> of the enum ksz_regs values into a common "RF_" (for reg field) prefix
> for a new enum type: ksz_rf. Eventually, enum ksz_regs, as well as the
> masks, should disappear completely, being replaced by reg fields.
> 
> Link: https://lore.kernel.org/netdev/Y%2FYPfxg8Ackb8zmW@shell.armlinux.org.uk/
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> diff --git a/drivers/net/dsa/microchip/ksz8863_smi.c b/drivers/net/dsa/microchip/ksz8863_smi.c
> index 5871f27451cb..f9d22a444146 100644
> --- a/drivers/net/dsa/microchip/ksz8863_smi.c
> +++ b/drivers/net/dsa/microchip/ksz8863_smi.c
> @@ -136,11 +136,16 @@ static const struct regmap_config ksz8863_regmap_config[] = {
>  
>  static int ksz8863_smi_probe(struct mdio_device *mdiodev)
>  {
> +	const struct ksz_chip_data *chip;
>  	struct regmap_config rc;
>  	struct ksz_device *dev;
>  	int ret;
>  	int i;
>  
> +	chip = device_get_match_data(ddev);

s/ddev/&mdiodev->dev/

> +	if (!chip)
> +		return -EINVAL;
> +
>  	dev = ksz_switch_alloc(&mdiodev->dev, mdiodev);
>  	if (!dev)
>  		return -ENOMEM;
> @@ -159,6 +164,10 @@ static int ksz8863_smi_probe(struct mdio_device *mdiodev)
>  		}
>  	}
>  
> +	ret = ksz_regfields_init(dev, chip);
> +	if (ret)
> +		return ret;
> +
>  	if (mdiodev->dev.platform_data)
>  		dev->pdata = mdiodev->dev.platform_data;
>  
> diff --git a/drivers/net/dsa/microchip/ksz9477_i2c.c b/drivers/net/dsa/microchip/ksz9477_i2c.c
> index 497be833f707..2cbd76aed974 100644
> --- a/drivers/net/dsa/microchip/ksz9477_i2c.c
> +++ b/drivers/net/dsa/microchip/ksz9477_i2c.c
> @@ -16,10 +16,15 @@ KSZ_REGMAP_TABLE(ksz9477, not_used, 16, 0, 0);
>  
>  static int ksz9477_i2c_probe(struct i2c_client *i2c)
>  {
> +	const struct ksz_chip_data *chip;
>  	struct regmap_config rc;
>  	struct ksz_device *dev;
>  	int i, ret;
>  
> +	chip = device_get_match_data(ddev);

s/ddev/&i2c->dev/

> +	if (!chip)
> +		return -EINVAL;
> +
>  	dev = ksz_switch_alloc(&i2c->dev, i2c);
>  	if (!dev)
>  		return -ENOMEM;
> @@ -35,6 +40,10 @@ static int ksz9477_i2c_probe(struct i2c_client *i2c)
>  		}
>  	}
>  
> +	ret = ksz_regfields_init(dev, chip);
> +	if (ret)
> +		return ret;
> +
>  	if (i2c->dev.platform_data)
>  		dev->pdata = i2c->dev.platform_data;
>