From mboxrd@z Thu Jan 1 00:00:00 1970 From: Joe Hershberger Date: Tue, 23 Jul 2019 04:07:04 +0000 Subject: [U-Boot] [PATCH v2 1/6] net: phy: mv88e61xx: rework to enable detection of 88E6071 devices In-Reply-To: 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 Mon, Jul 22, 2019 at 11:00 PM Joe Hershberger wrote: > > 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. 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