| Message ID | 20230601100005.2216345-7-lukma@denx.de |
|---|---|
| State | Changes Requested |
| Delegated to: | Ramon Fried |
| Headers | show |
| Series | Provide support for mv88e6020 Marvell switch | expand |
On 6/1/23 12:00, Lukasz Majewski wrote: > Some devices, when configured in bootstrap to 'no cpu' mode require PHY > manual reset to get them operational and responding to reading their ID > registers. > > Without this step - the PHYLIB probing will fail. > > In more details - the bootstrap configuration from switch must be read. > The value of CONFIG Data1 (0x71) of Scratch and Misc register is read > to check if 'no_cpu' and 'addr4' bits were set. > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > Reviewed-by: Ramon Fried <rfried.dev@gmail.com> > > --- > > drivers/net/phy/mv88e61xx.c | 63 +++++++++++++++++++++++++++++++++++-- > 1 file changed, 61 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/phy/mv88e61xx.c b/drivers/net/phy/mv88e61xx.c > index 69a87bead469..cf8f5e833e82 100644 > --- a/drivers/net/phy/mv88e61xx.c > +++ b/drivers/net/phy/mv88e61xx.c > @@ -194,6 +194,17 @@ struct mv88e61xx_phy_priv { > u8 phy_ctrl1_en_det_width; /* Width of 'EDet' bit field */ > u8 phy_ctrl1_en_det_ctrl; /* 'EDet' control value */ > u8 direct_access; /* Access switch device directly */ > + /* > + * Bootstrap configuration: > + * > + * If addr4 = 1 device is accessible from 0x10 address on MDIO bus. > + */ > + u8 addr4; > + /* > + * If no_cpu = 1 switch is automatically setup, otherwise PHY reset is > + * required from CPU for normal operation. > + */ > + u8 no_cpu; > }; > > static inline int smi_cmd(int cmd, int addr, int reg) > @@ -1218,6 +1229,33 @@ U_BOOT_PHY_DRIVER(mv88e6071) = { > .shutdown = &genphy_shutdown, > }; > > +static int mv88e61xx_read_bootstrap(struct phy_device *phydev) > +{ > + struct mv88e61xx_phy_priv *priv = phydev->priv; > + struct mii_dev *mdio_bus = priv->mdio_bus; > + int val; > + > + /* mv88e6020 - ID = 0x0200 (REG 3 on non PHY port) */ > + if (priv->id == PORT_SWITCH_ID_6020) { > + /* Prepare to read scratch and misc register */ > + mdio_bus->write(mdio_bus, priv->global2, 0, > + 0x1a /*MV_SCRATCH_MISC*/, > + (0x71 /*MV_CONFIG_DATA1*/ << 8)); Introduce macros for these magic values. > + val = mdio_bus->read(mdio_bus, priv->global2, 0, > + 0x1a /*MV_SCRATCH_MISC*/); > + > + if (val & (1 << 0)) > + priv->no_cpu = 1; > + if (val & (1 << 4)) Macros, and also BIT() [..]
Hi Marek, > On 6/1/23 12:00, Lukasz Majewski wrote: > > Some devices, when configured in bootstrap to 'no cpu' mode require > > PHY manual reset to get them operational and responding to reading > > their ID registers. > > > > Without this step - the PHYLIB probing will fail. > > > > In more details - the bootstrap configuration from switch must be > > read. The value of CONFIG Data1 (0x71) of Scratch and Misc register > > is read to check if 'no_cpu' and 'addr4' bits were set. > > > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > > Reviewed-by: Ramon Fried <rfried.dev@gmail.com> > > > > --- > > > > drivers/net/phy/mv88e61xx.c | 63 > > +++++++++++++++++++++++++++++++++++-- 1 file changed, 61 > > insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/phy/mv88e61xx.c > > b/drivers/net/phy/mv88e61xx.c index 69a87bead469..cf8f5e833e82 > > 100644 --- a/drivers/net/phy/mv88e61xx.c > > +++ b/drivers/net/phy/mv88e61xx.c > > @@ -194,6 +194,17 @@ struct mv88e61xx_phy_priv { > > u8 phy_ctrl1_en_det_width; /* Width of 'EDet' bit field */ > > u8 phy_ctrl1_en_det_ctrl; /* 'EDet' control value */ > > u8 direct_access; /* Access switch device > > directly */ > > + /* > > + * Bootstrap configuration: > > + * > > + * If addr4 = 1 device is accessible from 0x10 address on > > MDIO bus. > > + */ > > + u8 addr4; > > + /* > > + * If no_cpu = 1 switch is automatically setup, otherwise > > PHY reset is > > + * required from CPU for normal operation. > > + */ > > + u8 no_cpu; > > }; > > > > static inline int smi_cmd(int cmd, int addr, int reg) > > @@ -1218,6 +1229,33 @@ U_BOOT_PHY_DRIVER(mv88e6071) = { > > .shutdown = &genphy_shutdown, > > }; > > > > +static int mv88e61xx_read_bootstrap(struct phy_device *phydev) > > +{ > > + struct mv88e61xx_phy_priv *priv = phydev->priv; > > + struct mii_dev *mdio_bus = priv->mdio_bus; > > + int val; > > + > > + /* mv88e6020 - ID = 0x0200 (REG 3 on non PHY port) */ > > + if (priv->id == PORT_SWITCH_ID_6020) { > > + /* Prepare to read scratch and misc register */ > > + mdio_bus->write(mdio_bus, priv->global2, 0, > > + 0x1a /*MV_SCRATCH_MISC*/, > > + (0x71 /*MV_CONFIG_DATA1*/ << 8)); > > Introduce macros for these magic values. Frankly speaking, this is more readable than macros as it is in sync with vendor's documentation. In other words - it is easier to find this information in documentation when presented like above. > > > + val = mdio_bus->read(mdio_bus, priv->global2, 0, > > + 0x1a /*MV_SCRATCH_MISC*/); > > + > > + if (val & (1 << 0)) > > + priv->no_cpu = 1; > > + if (val & (1 << 4)) > > Macros, and also BIT() > > [..] Ok. Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On 6/1/23 14:13, Lukasz Majewski wrote: > Hi Marek, > >> On 6/1/23 12:00, Lukasz Majewski wrote: >>> Some devices, when configured in bootstrap to 'no cpu' mode require >>> PHY manual reset to get them operational and responding to reading >>> their ID registers. >>> >>> Without this step - the PHYLIB probing will fail. >>> >>> In more details - the bootstrap configuration from switch must be >>> read. The value of CONFIG Data1 (0x71) of Scratch and Misc register >>> is read to check if 'no_cpu' and 'addr4' bits were set. >>> >>> Signed-off-by: Lukasz Majewski <lukma@denx.de> >>> Reviewed-by: Ramon Fried <rfried.dev@gmail.com> >>> >>> --- >>> >>> drivers/net/phy/mv88e61xx.c | 63 >>> +++++++++++++++++++++++++++++++++++-- 1 file changed, 61 >>> insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/net/phy/mv88e61xx.c >>> b/drivers/net/phy/mv88e61xx.c index 69a87bead469..cf8f5e833e82 >>> 100644 --- a/drivers/net/phy/mv88e61xx.c >>> +++ b/drivers/net/phy/mv88e61xx.c >>> @@ -194,6 +194,17 @@ struct mv88e61xx_phy_priv { >>> u8 phy_ctrl1_en_det_width; /* Width of 'EDet' bit field */ >>> u8 phy_ctrl1_en_det_ctrl; /* 'EDet' control value */ >>> u8 direct_access; /* Access switch device >>> directly */ >>> + /* >>> + * Bootstrap configuration: >>> + * >>> + * If addr4 = 1 device is accessible from 0x10 address on >>> MDIO bus. >>> + */ >>> + u8 addr4; >>> + /* >>> + * If no_cpu = 1 switch is automatically setup, otherwise >>> PHY reset is >>> + * required from CPU for normal operation. >>> + */ >>> + u8 no_cpu; >>> }; >>> >>> static inline int smi_cmd(int cmd, int addr, int reg) >>> @@ -1218,6 +1229,33 @@ U_BOOT_PHY_DRIVER(mv88e6071) = { >>> .shutdown = &genphy_shutdown, >>> }; >>> >>> +static int mv88e61xx_read_bootstrap(struct phy_device *phydev) >>> +{ >>> + struct mv88e61xx_phy_priv *priv = phydev->priv; >>> + struct mii_dev *mdio_bus = priv->mdio_bus; >>> + int val; >>> + >>> + /* mv88e6020 - ID = 0x0200 (REG 3 on non PHY port) */ >>> + if (priv->id == PORT_SWITCH_ID_6020) { >>> + /* Prepare to read scratch and misc register */ >>> + mdio_bus->write(mdio_bus, priv->global2, 0, >>> + 0x1a /*MV_SCRATCH_MISC*/, >>> + (0x71 /*MV_CONFIG_DATA1*/ << 8)); >> >> Introduce macros for these magic values. > > Frankly speaking, this is more readable than macros as it is in sync > with vendor's documentation. Sorry, I am not buying this argument. Random constant 0x123 is NOT more readable than the standard that the rest of the U-Boot code base uses which is: #define MV_SCRATCH_MISC 0x1a and then call(MV_SCRATCH_MISC...) . Hard-coding random magic register offsets into the code is just sloppy and leads to duplication. The mv88e61xx driver does not do it either, so this patch is violating its coding style, see the top 200 or so lines of the driver. Do not do it. To make it clear, until this is fixed, NAK . > In other words - it is easier to find this information in documentation > when presented like above. No
diff --git a/drivers/net/phy/mv88e61xx.c b/drivers/net/phy/mv88e61xx.c index 69a87bead469..cf8f5e833e82 100644 --- a/drivers/net/phy/mv88e61xx.c +++ b/drivers/net/phy/mv88e61xx.c @@ -194,6 +194,17 @@ struct mv88e61xx_phy_priv { u8 phy_ctrl1_en_det_width; /* Width of 'EDet' bit field */ u8 phy_ctrl1_en_det_ctrl; /* 'EDet' control value */ u8 direct_access; /* Access switch device directly */ + /* + * Bootstrap configuration: + * + * If addr4 = 1 device is accessible from 0x10 address on MDIO bus. + */ + u8 addr4; + /* + * If no_cpu = 1 switch is automatically setup, otherwise PHY reset is + * required from CPU for normal operation. + */ + u8 no_cpu; }; static inline int smi_cmd(int cmd, int addr, int reg) @@ -1218,6 +1229,33 @@ U_BOOT_PHY_DRIVER(mv88e6071) = { .shutdown = &genphy_shutdown, }; +static int mv88e61xx_read_bootstrap(struct phy_device *phydev) +{ + struct mv88e61xx_phy_priv *priv = phydev->priv; + struct mii_dev *mdio_bus = priv->mdio_bus; + int val; + + /* mv88e6020 - ID = 0x0200 (REG 3 on non PHY port) */ + if (priv->id == PORT_SWITCH_ID_6020) { + /* Prepare to read scratch and misc register */ + mdio_bus->write(mdio_bus, priv->global2, 0, + 0x1a /*MV_SCRATCH_MISC*/, + (0x71 /*MV_CONFIG_DATA1*/ << 8)); + + val = mdio_bus->read(mdio_bus, priv->global2, 0, + 0x1a /*MV_SCRATCH_MISC*/); + + if (val & (1 << 0)) + priv->no_cpu = 1; + if (val & (1 << 4)) + priv->addr4 = 1; + debug("mv88e6020: no_cpu=%d addr4=%d\n", priv->no_cpu, + priv->addr4); + } + + return 0; +} + /* * Overload weak get_phy_id definition since we need non-standard functions * to read PHY registers @@ -1257,13 +1295,34 @@ int get_phy_id(struct mii_dev *bus, int smi_addr, int devad, u32 *phy_id) if (val < 0) return val; - val = mv88e61xx_phy_read_indirect(&temp_mii, 0, devad, MII_PHYSID1); + mv88e61xx_read_bootstrap(&temp_phy); + + /* + * When switch is configured to work with CPU (i.e. NO_CPU == 0), PHYs + * require reset (to at least single one) to have its registers + * accessible. + */ + if (!temp_priv.no_cpu && temp_priv.id == PORT_SWITCH_ID_6020) { + /* Reset PHY */ + val = mv88e61xx_phy_read_indirect(&temp_mii, temp_phy.addr, + devad, MII_BMCR); + if (val & BMCR_PDOWN) + val &= ~BMCR_PDOWN; + + mv88e61xx_phy_write_indirect(&temp_mii, temp_phy.addr, devad, + MII_BMCR, val); + } + + /* Read PHY_ID */ + val = mv88e61xx_phy_read_indirect(&temp_mii, temp_phy.addr, devad, + MII_PHYSID1); if (val < 0) return -EIO; *phy_id = val << 16; - val = mv88e61xx_phy_read_indirect(&temp_mii, 0, devad, MII_PHYSID2); + val = mv88e61xx_phy_read_indirect(&temp_mii, temp_phy.addr, devad, + MII_PHYSID2); if (val < 0) return -EIO;