Messages in this thread | | | Date | Fri, 17 Mar 2023 11:46:29 +0200 | From | Vladimir Oltean <> | Subject | Re: [RFC/RFT PATCH net-next 2/4] net: dsa: microchip: partial conversion to regfields API for KSZ8795 (WIP) |
| |
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; >
| |