From mboxrd@z Thu Jan 1 00:00:00 1970 From: Joe Hershberger Date: Tue, 23 Jul 2019 04:00:14 +0000 Subject: [U-Boot] [PATCH v2 1/6] net: phy: mv88e61xx: rework to enable detection of 88E6071 devices In-Reply-To: <20190709232251.31746-2-agust@denx.de> References: <20190709232251.31746-1-agust@denx.de> <20190709232251.31746-2-agust@denx.de> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Tue, Jul 9, 2019 at 6:23 PM Anatolij Gustschin 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 > --- > 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. > +} > + > 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