[U-Boot] [PATCH v2 1/6] net: phy: mv88e61xx: rework to enable detection of 88E6071 devices
Joe Hershberger
joe.hershberger at ni.com
Tue Jul 23 04:07:04 UTC 2019
On Mon, Jul 22, 2019 at 11:00 PM Joe Hershberger <joe.hershberger at ni.com> wrote:
>
> On Tue, Jul 9, 2019 at 6:23 PM Anatolij Gustschin <agust at denx.de> wrote:
> >
> > Extend the driver to init switch register offsets from variables
> > instead of compile time macros and enable 88E6071 detection.
> > Ethernet transfer (e.g. tftp) does not work yet, so enable the
> > registration of the 'indirect mii' bus for easier PHY register
> > access by 'mii' command.
> >
> > Signed-off-by: Anatolij Gustschin <agust at denx.de>
> > ---
> > drivers/net/phy/mv88e61xx.c | 95 +++++++++++++++++++++++++++++--------
> > 1 file changed, 76 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/net/phy/mv88e61xx.c b/drivers/net/phy/mv88e61xx.c
> > index c1e2860329..8f5da9486c 100644
> > --- a/drivers/net/phy/mv88e61xx.c
> > +++ b/drivers/net/phy/mv88e61xx.c
> > @@ -39,15 +39,12 @@
> >
> > #define PHY_AUTONEGOTIATE_TIMEOUT 5000
> >
> > -#define PORT_COUNT 11
> > -#define PORT_MASK ((1 << PORT_COUNT) - 1)
> > +#define PORT_MASK(port_count) ((1 << (port_count)) - 1)
> >
> > /* Device addresses */
> > #define DEVADDR_PHY(p) (p)
> > -#define DEVADDR_PORT(p) (0x10 + (p))
> > +#define DEVADDR_PORT(p) (priv->port_reg_base + (p))
>
> This is an obtuse macro. It should not reference a local variable
> internally. If you want to use priv, pass it as a parameter.
>
> > #define DEVADDR_SERDES 0x0F
> > -#define DEVADDR_GLOBAL_1 0x1B
> > -#define DEVADDR_GLOBAL_2 0x1C
> >
> > /* SMI indirection registers for multichip addressing mode */
> > #define SMI_CMD_REG 0x00
> > @@ -188,11 +185,16 @@
> > #define PORT_SWITCH_ID_6176 0x1760
> > #define PORT_SWITCH_ID_6240 0x2400
> > #define PORT_SWITCH_ID_6352 0x3520
> > +#define PORT_SWITCH_ID_6071 0x0710
> >
> > struct mv88e61xx_phy_priv {
> > struct mii_dev *mdio_bus;
> > int smi_addr;
> > int id;
> > + int port_count;
> > + int port_reg_base;
> > + u8 global1;
> > + u8 global2;
>
> I think this could stand some commenting. global what? Why 2?
>
> > };
> >
> > static inline int smi_cmd(int cmd, int addr, int reg)
> > @@ -329,11 +331,12 @@ static int mv88e61xx_reg_write(struct phy_device *phydev, int dev, int reg,
> >
> > static int mv88e61xx_phy_wait(struct phy_device *phydev)
> > {
> > + struct mv88e61xx_phy_priv *priv = phydev->priv;
> > int val;
> > u32 timeout = 100;
> >
> > do {
> > - val = mv88e61xx_reg_read(phydev, DEVADDR_GLOBAL_2,
> > + val = mv88e61xx_reg_read(phydev, priv->global2,
>
> Probably just use phydev->priv->global2 instead of the local variable.
>
> > GLOBAL2_REG_PHY_CMD);
> > if (val >= 0 && (val & SMI_BUSY) == 0)
> > return 0;
> > @@ -347,13 +350,15 @@ static int mv88e61xx_phy_wait(struct phy_device *phydev)
> > static int mv88e61xx_phy_read_indirect(struct mii_dev *smi_wrapper, int dev,
> > int devad, int reg)
> > {
> > + struct mv88e61xx_phy_priv *priv;
> > struct phy_device *phydev;
> > int res;
> >
> > phydev = (struct phy_device *)smi_wrapper->priv;
> > + priv = phydev->priv;
> >
> > /* Issue command to read */
> > - res = mv88e61xx_reg_write(phydev, DEVADDR_GLOBAL_2,
> > + res = mv88e61xx_reg_write(phydev, priv->global2,
> > GLOBAL2_REG_PHY_CMD,
> > smi_cmd_read(dev, reg));
> >
> > @@ -363,25 +368,27 @@ static int mv88e61xx_phy_read_indirect(struct mii_dev *smi_wrapper, int dev,
> > return res;
> >
> > /* Read retrieved data */
> > - return mv88e61xx_reg_read(phydev, DEVADDR_GLOBAL_2,
> > + return mv88e61xx_reg_read(phydev, priv->global2,
> > GLOBAL2_REG_PHY_DATA);
> > }
> >
> > static int mv88e61xx_phy_write_indirect(struct mii_dev *smi_wrapper, int dev,
> > int devad, int reg, u16 data)
> > {
> > + struct mv88e61xx_phy_priv *priv;
> > struct phy_device *phydev;
> > int res;
> >
> > phydev = (struct phy_device *)smi_wrapper->priv;
> > + priv = phydev->priv;
> >
> > /* Set the data to write */
> > - res = mv88e61xx_reg_write(phydev, DEVADDR_GLOBAL_2,
> > + res = mv88e61xx_reg_write(phydev, priv->global2,
> > GLOBAL2_REG_PHY_DATA, data);
> > if (res < 0)
> > return res;
> > /* Issue the write command */
> > - res = mv88e61xx_reg_write(phydev, DEVADDR_GLOBAL_2,
> > + res = mv88e61xx_reg_write(phydev, priv->global2,
> > GLOBAL2_REG_PHY_CMD,
> > smi_cmd_write(dev, reg));
> > if (res < 0)
> > @@ -408,12 +415,14 @@ static int mv88e61xx_phy_write(struct phy_device *phydev, int phy,
> >
> > static int mv88e61xx_port_read(struct phy_device *phydev, u8 port, u8 reg)
> > {
> > + struct mv88e61xx_phy_priv *priv = phydev->priv;
>
> Huh? When casually read, this seems useless.
>
> > return mv88e61xx_reg_read(phydev, DEVADDR_PORT(port), reg);
> > }
> >
> > static int mv88e61xx_port_write(struct phy_device *phydev, u8 port, u8 reg,
> > u16 val)
> > {
> > + struct mv88e61xx_phy_priv *priv = phydev->priv;
>
> Huh? Ditto.
>
> > return mv88e61xx_reg_write(phydev, DEVADDR_PORT(port), reg, val);
> > }
> >
> > @@ -515,12 +524,13 @@ static int mv88e61xx_parse_status(struct phy_device *phydev)
> >
> > static int mv88e61xx_switch_reset(struct phy_device *phydev)
> > {
> > + struct mv88e61xx_phy_priv *priv = phydev->priv;
> > int time;
> > int val;
> > u8 port;
> >
> > /* Disable all ports */
> > - for (port = 0; port < PORT_COUNT; port++) {
> > + for (port = 0; port < priv->port_count; port++) {
> > val = mv88e61xx_port_read(phydev, port, PORT_REG_CTRL);
> > if (val < 0)
> > return val;
> > @@ -536,18 +546,18 @@ static int mv88e61xx_switch_reset(struct phy_device *phydev)
> > udelay(2000);
> >
> > /* Reset switch */
> > - val = mv88e61xx_reg_read(phydev, DEVADDR_GLOBAL_1, GLOBAL1_CTRL);
> > + val = mv88e61xx_reg_read(phydev, priv->global1, GLOBAL1_CTRL);
> > if (val < 0)
> > return val;
> > val |= GLOBAL1_CTRL_SWRESET;
> > - val = mv88e61xx_reg_write(phydev, DEVADDR_GLOBAL_1,
> > + val = mv88e61xx_reg_write(phydev, priv->global1,
> > GLOBAL1_CTRL, val);
> > if (val < 0)
> > return val;
> >
> > /* Wait up to 1 second for switch reset complete */
> > for (time = 1000; time; time--) {
> > - val = mv88e61xx_reg_read(phydev, DEVADDR_GLOBAL_1,
> > + val = mv88e61xx_reg_read(phydev, priv->global1,
> > GLOBAL1_CTRL);
> > if (val >= 0 && ((val & GLOBAL1_CTRL_SWRESET) == 0))
> > break;
> > @@ -732,22 +742,23 @@ static int mv88e61xx_fixed_port_setup(struct phy_device *phydev, u8 port)
> >
> > static int mv88e61xx_set_cpu_port(struct phy_device *phydev)
> > {
> > + struct mv88e61xx_phy_priv *priv = phydev->priv;
> > int val;
> >
> > /* Set CPUDest */
> > - val = mv88e61xx_reg_read(phydev, DEVADDR_GLOBAL_1, GLOBAL1_MON_CTRL);
> > + val = mv88e61xx_reg_read(phydev, priv->global1, GLOBAL1_MON_CTRL);
> > if (val < 0)
> > return val;
> > val = bitfield_replace(val, GLOBAL1_MON_CTRL_CPUDEST_SHIFT,
> > GLOBAL1_MON_CTRL_CPUDEST_WIDTH,
> > CONFIG_MV88E61XX_CPU_PORT);
> > - val = mv88e61xx_reg_write(phydev, DEVADDR_GLOBAL_1,
> > + val = mv88e61xx_reg_write(phydev, priv->global1,
> > GLOBAL1_MON_CTRL, val);
> > if (val < 0)
> > return val;
> >
> > /* Allow CPU to route to any port */
> > - val = PORT_MASK & ~(1 << CONFIG_MV88E61XX_CPU_PORT);
> > + val = PORT_MASK(priv->port_count) & ~(1 << CONFIG_MV88E61XX_CPU_PORT);
> > val = mv88e61xx_port_set_vlan(phydev, CONFIG_MV88E61XX_CPU_PORT, val);
> > if (val < 0)
> > return val;
> > @@ -856,6 +867,24 @@ static int mv88e61xx_phy_config_port(struct phy_device *phydev, u8 phy)
> > return 0;
> > }
> >
> > +static void mv88e61xx_priv_reg_offs_pre_init(struct mv88e61xx_phy_priv *priv)
> > +{
> > + /*
> > + * Initial 'port_reg_base' value must be in the port register,
> > + * map and the global_N register offsets must be correct,
>
> They are globalN, not global_N. What is the map that this referenced?
> Please reword.
>
> > + * otherwise detection of switch ID won't work!
>
> That seems likely... but this looks very magic. Please document it a
> little better and if possible reference any public datasheets or
> reference manuals.
>
> > + */
> > +#ifndef CONFIG_MV88E61XX_88E6020_FAMILY
> > + priv->global1 = 0x1B;
> > + priv->global2 = 0x1C;
> > + priv->port_reg_base = 0x10;
> > +#else
> > + priv->global1 = 0x0F;
> > + priv->global2 = 0x07;
> > + priv->port_reg_base = 0x08;
> > +#endif
>
> I think it would be much cleaner if the contents of this function and
> the following switch case in the next function were implemented as
> driver data selections in the udevice_id structure and then used from
> there.
Argh... I forgot phy is not using DM yet. Nvm.
> > +}
> > +
> > static int mv88e61xx_probe(struct phy_device *phydev)
> > {
> > struct mii_dev *smi_wrapper;
> > @@ -910,13 +939,38 @@ static int mv88e61xx_probe(struct phy_device *phydev)
> >
> > phydev->priv = priv;
> >
> > + mv88e61xx_priv_reg_offs_pre_init(priv);
> > +
> > priv->id = mv88e61xx_get_switch_id(phydev);
> > + debug("%s ID 0x%x\n", __func__, priv->id);
> > +
> > + switch (priv->id) {
> > + case PORT_SWITCH_ID_6096:
> > + case PORT_SWITCH_ID_6097:
> > + case PORT_SWITCH_ID_6172:
> > + case PORT_SWITCH_ID_6176:
> > + case PORT_SWITCH_ID_6240:
> > + case PORT_SWITCH_ID_6352:
> > + priv->port_count = 11;
> > + break;
> > + case PORT_SWITCH_ID_6071:
> > + priv->port_count = 7;
> > + break;
> > + default:
> > + free(priv);
> > + return -ENODEV;
> > + }
> > +
> > + res = mdio_register(smi_wrapper);
> > + if (res)
> > + printf("Failed to register SMI bus\n");
> >
> > return 0;
> > }
> >
> > static int mv88e61xx_phy_config(struct phy_device *phydev)
> > {
> > + struct mv88e61xx_phy_priv *priv = phydev->priv;
> > int res;
> > int i;
> > int ret = -1;
> > @@ -925,7 +979,7 @@ static int mv88e61xx_phy_config(struct phy_device *phydev)
> > if (res < 0)
> > return res;
> >
> > - for (i = 0; i < PORT_COUNT; i++) {
> > + for (i = 0; i < priv->port_count; i++) {
> > if ((1 << i) & CONFIG_MV88E61XX_PHY_PORTS) {
> > phydev->addr = i;
> >
> > @@ -988,13 +1042,14 @@ static int mv88e61xx_phy_is_connected(struct phy_device *phydev)
> >
> > static int mv88e61xx_phy_startup(struct phy_device *phydev)
> > {
> > + struct mv88e61xx_phy_priv *priv = phydev->priv;
> > int i;
> > int link = 0;
> > int res;
> > int speed = phydev->speed;
> > int duplex = phydev->duplex;
> >
> > - for (i = 0; i < PORT_COUNT; i++) {
> > + for (i = 0; i < priv->port_count; i++) {
> > if ((1 << i) & CONFIG_MV88E61XX_PHY_PORTS) {
> > phydev->addr = i;
> > if (!mv88e61xx_phy_is_connected(phydev))
> > @@ -1068,6 +1123,8 @@ int get_phy_id(struct mii_dev *bus, int smi_addr, int devad, u32 *phy_id)
> > temp_phy.priv = &temp_priv;
> > temp_mii.priv = &temp_phy;
> >
> > + mv88e61xx_priv_reg_offs_pre_init(&temp_priv);
>
> Ick.
>
> > +
> > val = mv88e61xx_phy_read_indirect(&temp_mii, 0, devad, MII_PHYSID1);
> > if (val < 0)
> > return -EIO;
> > --
> > 2.17.1
> >
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot at lists.denx.de
> > https://lists.denx.de/listinfo/u-boot
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
More information about the U-Boot
mailing list