[U-Boot] [PATCH v2 0/6] Extend mv88e61xx driver to support 88E6071
This series adds support for 88E6071 switches in the mv88e61xx driver.
Changes in v2: - fix port init for 6096/6097 devices in patch 1/6 - update commit description in patch 4/6
Anatolij Gustschin (6): net: phy: mv88e61xx: rework to enable detection of 88E6071 devices net: phy: mv88e61xx: add CPU port parameter init for 88E6071 net: phy: mv88E61xx: fix ENERGY_DET init for mv88E6071 net: phy: mv88E61xx: add config option for mv88E6071 support net: phy: mv88e61xx: register phy_driver struct for 88E6071 net: phy: fix switch vendor name
drivers/net/phy/Kconfig | 9 +- drivers/net/phy/mv88e61xx.c | 168 +++++++++++++++++++++++++++--------- 2 files changed, 137 insertions(+), 40 deletions(-)
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@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)) #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; };
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, 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; 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; 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, + * otherwise detection of switch ID won't work! + */ +#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 +} + 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); + val = mv88e61xx_phy_read_indirect(&temp_mii, 0, devad, MII_PHYSID1); if (val < 0) return -EIO;
On Tue, Jul 9, 2019 at 6:23 PM Anatolij Gustschin agust@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@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.
+}
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@lists.denx.de https://lists.denx.de/listinfo/u-boot
On Mon, Jul 22, 2019 at 11:00 PM Joe Hershberger joe.hershberger@ni.com wrote:
On Tue, Jul 9, 2019 at 6:23 PM Anatolij Gustschin agust@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@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@lists.denx.de https://lists.denx.de/listinfo/u-boot
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
On Tue, 23 Jul 2019 04:07:04 +0000 Joe Hershberger joe.hershberger@ni.com wrote:
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.
No, I'll stick with local variable. priv is void*, we will get
+drivers/net/phy/mv88e61xx.c:338:48: error: dereferencing 'void *' pointer [-Werror] + val = mv88e61xx_reg_read(phydev, phydev->priv->global2,
-- Anatolij
On Fri, Jul 26, 2019 at 9:48 AM Anatolij Gustschin agust@denx.de wrote:
On Tue, 23 Jul 2019 04:07:04 +0000 Joe Hershberger joe.hershberger@ni.com wrote:
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.
No, I'll stick with local variable. priv is void*, we will get
+drivers/net/phy/mv88e61xx.c:338:48: error: dereferencing 'void *' pointer [-Werror]
- val = mv88e61xx_reg_read(phydev, phydev->priv->global2,
Ah, ok.
-Joe
On Tue, 23 Jul 2019 04:08:23 +0000 Joe Hershberger joe.hershberger@ni.com wrote: ...
-#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.
Okay.
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?
I'll add some comments here.
...
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.
Done in v3.
...
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.
Okay, I'll remove DEVADDR_PORT() macro.
...
static int mv88e61xx_port_write(struct phy_device *phydev, u8 port, u8 reg, u16 val) {
struct mv88e61xx_phy_priv *priv = phydev->priv;Huh? Ditto.
Will fix it.
...
+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.
Okay, done in v3.
* 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.
Will add more comments here. The datasheets or manuals are all under NDA, I can't add references here.
*/+#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.
The problem is that this offset pre-initialisation is also required before the actual driver probing (i.e. calls of get_phy_id() from PHY framework).
...
@@ -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.
will add a comment why it is required here.
-- Anatolij
On 88E6071 chip the port status register bit field offsets for duplex and link bits differ. Extend the driver to use 88E6071 specific offset values. The width of bit fields for speed status differ, too. Adapt for proper port speed detection on 88E6071.
Signed-off-by: Anatolij Gustschin agust@denx.de --- drivers/net/phy/mv88e61xx.c | 41 +++++++++++++++++++++++++------------ 1 file changed, 28 insertions(+), 13 deletions(-)
diff --git a/drivers/net/phy/mv88e61xx.c b/drivers/net/phy/mv88e61xx.c index 8f5da9486c..d38109b797 100644 --- a/drivers/net/phy/mv88e61xx.c +++ b/drivers/net/phy/mv88e61xx.c @@ -85,9 +85,6 @@ #define GLOBAL1_MON_CTRL_CPUDEST_SHIFT 4 #define GLOBAL1_MON_CTRL_CPUDEST_WIDTH 4
-#define PORT_REG_STATUS_LINK BIT(11) -#define PORT_REG_STATUS_DUPLEX BIT(10) - #define PORT_REG_STATUS_SPEED_SHIFT 8 #define PORT_REG_STATUS_SPEED_WIDTH 2 #define PORT_REG_STATUS_SPEED_10 0 @@ -108,6 +105,7 @@ #define PORT_REG_PHYS_CTRL_DUPLEX_VALUE BIT(3) #define PORT_REG_PHYS_CTRL_DUPLEX_FORCE BIT(2) #define PORT_REG_PHYS_CTRL_SPD1000 BIT(1) +#define PORT_REG_PHYS_CTRL_SPD100 BIT(0) #define PORT_REG_PHYS_CTRL_SPD_MASK (BIT(1) | BIT(0))
#define PORT_REG_CTRL_PSTATE_SHIFT 0 @@ -193,6 +191,9 @@ struct mv88e61xx_phy_priv { int id; int port_count; int port_reg_base; + u16 port_stat_link_mask; + u16 port_stat_dup_mask; + u8 port_stat_speed_width; u8 global1; u8 global2; }; @@ -638,6 +639,7 @@ static int mv88e61xx_port_set_vlan(struct phy_device *phydev, u8 port,
static int mv88e61xx_read_port_config(struct phy_device *phydev, u8 port) { + struct mv88e61xx_phy_priv *priv = phydev->priv; int res; int val; bool forced = false; @@ -645,7 +647,7 @@ static int mv88e61xx_read_port_config(struct phy_device *phydev, u8 port) val = mv88e61xx_port_read(phydev, port, PORT_REG_STATUS); if (val < 0) return val; - if (!(val & PORT_REG_STATUS_LINK)) { + if (!(val & priv->port_stat_link_mask)) { /* Temporarily force link to read port configuration */ u32 timeout = 100; forced = true; @@ -668,7 +670,7 @@ static int mv88e61xx_read_port_config(struct phy_device *phydev, u8 port) res = -EIO; goto unforce; } - if (val & PORT_REG_STATUS_LINK) + if (val & priv->port_stat_link_mask) break; } while (--timeout);
@@ -678,13 +680,13 @@ static int mv88e61xx_read_port_config(struct phy_device *phydev, u8 port) } }
- if (val & PORT_REG_STATUS_DUPLEX) + if (val & priv->port_stat_dup_mask) phydev->duplex = DUPLEX_FULL; else phydev->duplex = DUPLEX_HALF;
val = bitfield_extract(val, PORT_REG_STATUS_SPEED_SHIFT, - PORT_REG_STATUS_SPEED_WIDTH); + priv->port_stat_speed_width); switch (val) { case PORT_REG_STATUS_SPEED_1000: phydev->speed = SPEED_1000; @@ -717,6 +719,7 @@ unforce:
static int mv88e61xx_fixed_port_setup(struct phy_device *phydev, u8 port) { + struct mv88e61xx_phy_priv *priv = phydev->priv; int val;
val = mv88e61xx_port_read(phydev, port, PORT_REG_PHYS_CTRL); @@ -724,13 +727,19 @@ static int mv88e61xx_fixed_port_setup(struct phy_device *phydev, u8 port) return val;
val &= ~(PORT_REG_PHYS_CTRL_SPD_MASK | - PORT_REG_PHYS_CTRL_FC_VALUE); - val |= PORT_REG_PHYS_CTRL_PCS_AN_EN | - PORT_REG_PHYS_CTRL_PCS_AN_RST | - PORT_REG_PHYS_CTRL_FC_FORCE | + PORT_REG_PHYS_CTRL_FC_VALUE | + PORT_REG_PHYS_CTRL_FC_FORCE); + val |= PORT_REG_PHYS_CTRL_FC_FORCE | PORT_REG_PHYS_CTRL_DUPLEX_VALUE | - PORT_REG_PHYS_CTRL_DUPLEX_FORCE | - PORT_REG_PHYS_CTRL_SPD1000; + PORT_REG_PHYS_CTRL_DUPLEX_FORCE; + + if (priv->id == PORT_SWITCH_ID_6071) { + val |= PORT_REG_PHYS_CTRL_SPD100; + } else { + val |= PORT_REG_PHYS_CTRL_PCS_AN_EN | + PORT_REG_PHYS_CTRL_PCS_AN_RST | + PORT_REG_PHYS_CTRL_SPD1000; + }
if (port == CONFIG_MV88E61XX_CPU_PORT) val |= PORT_REG_PHYS_CTRL_LINK_VALUE | @@ -952,9 +961,15 @@ static int mv88e61xx_probe(struct phy_device *phydev) case PORT_SWITCH_ID_6240: case PORT_SWITCH_ID_6352: priv->port_count = 11; + priv->port_stat_link_mask = BIT(11); + priv->port_stat_dup_mask = BIT(10); + priv->port_stat_speed_width = 2; break; case PORT_SWITCH_ID_6071: priv->port_count = 7; + priv->port_stat_link_mask = BIT(12); + priv->port_stat_dup_mask = BIT(9); + priv->port_stat_speed_width = 1; break; default: free(priv);
On Tue, Jul 9, 2019 at 6:25 PM Anatolij Gustschin agust@denx.de wrote:
On 88E6071 chip the port status register bit field offsets for duplex and link bits differ. Extend the driver to use 88E6071 specific offset values. The width of bit fields for speed status differ, too. Adapt for proper port speed detection on 88E6071.
Signed-off-by: Anatolij Gustschin agust@denx.de
drivers/net/phy/mv88e61xx.c | 41 +++++++++++++++++++++++++------------ 1 file changed, 28 insertions(+), 13 deletions(-)
diff --git a/drivers/net/phy/mv88e61xx.c b/drivers/net/phy/mv88e61xx.c index 8f5da9486c..d38109b797 100644 --- a/drivers/net/phy/mv88e61xx.c +++ b/drivers/net/phy/mv88e61xx.c @@ -85,9 +85,6 @@ #define GLOBAL1_MON_CTRL_CPUDEST_SHIFT 4 #define GLOBAL1_MON_CTRL_CPUDEST_WIDTH 4
-#define PORT_REG_STATUS_LINK BIT(11) -#define PORT_REG_STATUS_DUPLEX BIT(10)
#define PORT_REG_STATUS_SPEED_SHIFT 8 #define PORT_REG_STATUS_SPEED_WIDTH 2
Surprised this didn't go away.
#define PORT_REG_STATUS_SPEED_10 0 @@ -108,6 +105,7 @@ #define PORT_REG_PHYS_CTRL_DUPLEX_VALUE BIT(3) #define PORT_REG_PHYS_CTRL_DUPLEX_FORCE BIT(2) #define PORT_REG_PHYS_CTRL_SPD1000 BIT(1) +#define PORT_REG_PHYS_CTRL_SPD100 BIT(0) #define PORT_REG_PHYS_CTRL_SPD_MASK (BIT(1) | BIT(0))
#define PORT_REG_CTRL_PSTATE_SHIFT 0 @@ -193,6 +191,9 @@ struct mv88e61xx_phy_priv { int id; int port_count; int port_reg_base;
u16 port_stat_link_mask;u16 port_stat_dup_mask;u8 port_stat_speed_width; u8 global1; u8 global2;}; @@ -638,6 +639,7 @@ static int mv88e61xx_port_set_vlan(struct phy_device *phydev, u8 port,
static int mv88e61xx_read_port_config(struct phy_device *phydev, u8 port) {
struct mv88e61xx_phy_priv *priv = phydev->priv; int res; int val; bool forced = false;@@ -645,7 +647,7 @@ static int mv88e61xx_read_port_config(struct phy_device *phydev, u8 port) val = mv88e61xx_port_read(phydev, port, PORT_REG_STATUS); if (val < 0) return val;
if (!(val & PORT_REG_STATUS_LINK)) {
if (!(val & priv->port_stat_link_mask)) { /* Temporarily force link to read port configuration */ u32 timeout = 100; forced = true;@@ -668,7 +670,7 @@ static int mv88e61xx_read_port_config(struct phy_device *phydev, u8 port) res = -EIO; goto unforce; }
if (val & PORT_REG_STATUS_LINK)
if (val & priv->port_stat_link_mask) break; } while (--timeout);@@ -678,13 +680,13 @@ static int mv88e61xx_read_port_config(struct phy_device *phydev, u8 port) } }
if (val & PORT_REG_STATUS_DUPLEX)
if (val & priv->port_stat_dup_mask) phydev->duplex = DUPLEX_FULL; else phydev->duplex = DUPLEX_HALF; val = bitfield_extract(val, PORT_REG_STATUS_SPEED_SHIFT,
PORT_REG_STATUS_SPEED_WIDTH);
priv->port_stat_speed_width); switch (val) { case PORT_REG_STATUS_SPEED_1000: phydev->speed = SPEED_1000;@@ -717,6 +719,7 @@ unforce:
static int mv88e61xx_fixed_port_setup(struct phy_device *phydev, u8 port) {
struct mv88e61xx_phy_priv *priv = phydev->priv; int val; val = mv88e61xx_port_read(phydev, port, PORT_REG_PHYS_CTRL);@@ -724,13 +727,19 @@ static int mv88e61xx_fixed_port_setup(struct phy_device *phydev, u8 port) return val;
val &= ~(PORT_REG_PHYS_CTRL_SPD_MASK |
PORT_REG_PHYS_CTRL_FC_VALUE);val |= PORT_REG_PHYS_CTRL_PCS_AN_EN |PORT_REG_PHYS_CTRL_PCS_AN_RST |PORT_REG_PHYS_CTRL_FC_FORCE |
PORT_REG_PHYS_CTRL_FC_VALUE |PORT_REG_PHYS_CTRL_FC_FORCE);val |= PORT_REG_PHYS_CTRL_FC_FORCE | PORT_REG_PHYS_CTRL_DUPLEX_VALUE |
PORT_REG_PHYS_CTRL_DUPLEX_FORCE |PORT_REG_PHYS_CTRL_SPD1000;
PORT_REG_PHYS_CTRL_DUPLEX_FORCE;if (priv->id == PORT_SWITCH_ID_6071) {val |= PORT_REG_PHYS_CTRL_SPD100;} else {val |= PORT_REG_PHYS_CTRL_PCS_AN_EN |PORT_REG_PHYS_CTRL_PCS_AN_RST |PORT_REG_PHYS_CTRL_SPD1000;} if (port == CONFIG_MV88E61XX_CPU_PORT) val |= PORT_REG_PHYS_CTRL_LINK_VALUE |@@ -952,9 +961,15 @@ static int mv88e61xx_probe(struct phy_device *phydev) case PORT_SWITCH_ID_6240: case PORT_SWITCH_ID_6352: priv->port_count = 11;
priv->port_stat_link_mask = BIT(11);priv->port_stat_dup_mask = BIT(10);priv->port_stat_speed_width = 2; break; case PORT_SWITCH_ID_6071: priv->port_count = 7;priv->port_stat_link_mask = BIT(12);priv->port_stat_dup_mask = BIT(9);priv->port_stat_speed_width = 1; break; default: free(priv);-- 2.17.1
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
On Tue, 23 Jul 2019 04:05:56 +0000 Joe Hershberger joe.hershberger@ni.com wrote: ...
-#define PORT_REG_STATUS_LINK BIT(11) -#define PORT_REG_STATUS_DUPLEX BIT(10)
#define PORT_REG_STATUS_SPEED_SHIFT 8 #define PORT_REG_STATUS_SPEED_WIDTH 2
Surprised this didn't go away.
Removed in v3, thanks.
-- Anatolij
On mv88E6071 the 'EDet' field offset, width and sense control bits are different, adjust the driver to init the PHY control register as needed. This fixes not working link detection and tftp transfers.
Signed-off-by: Anatolij Gustschin agust@denx.de --- drivers/net/phy/mv88e61xx.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/drivers/net/phy/mv88e61xx.c b/drivers/net/phy/mv88e61xx.c index d38109b797..165bc261e4 100644 --- a/drivers/net/phy/mv88e61xx.c +++ b/drivers/net/phy/mv88e61xx.c @@ -119,16 +119,12 @@
#define SERDES_REG_CTRL_1_FORCE_LINK BIT(10)
-#define PHY_REG_CTRL1_ENERGY_DET_SHIFT 8 -#define PHY_REG_CTRL1_ENERGY_DET_WIDTH 2 - /* Field values */ #define PORT_REG_CTRL_PSTATE_DISABLED 0 #define PORT_REG_CTRL_PSTATE_FORWARD 3
#define PHY_REG_CTRL1_ENERGY_DET_OFF 0 #define PHY_REG_CTRL1_ENERGY_DET_SENSE_ONLY 2 -#define PHY_REG_CTRL1_ENERGY_DET_SENSE_XMIT 3
/* PHY Status Register */ #define PHY_REG_STATUS1_SPEED 0xc000 @@ -194,6 +190,9 @@ struct mv88e61xx_phy_priv { u16 port_stat_link_mask; u16 port_stat_dup_mask; u8 port_stat_speed_width; + u8 phy_ctrl1_en_det_shift; + u8 phy_ctrl1_en_det_width; + u8 phy_ctrl1_en_det_sense_xmit; u8 global1; u8 global2; }; @@ -841,6 +840,7 @@ static int mv88e61xx_phy_enable(struct phy_device *phydev, u8 phy)
static int mv88e61xx_phy_setup(struct phy_device *phydev, u8 phy) { + struct mv88e61xx_phy_priv *priv = phydev->priv; int val;
/* @@ -850,9 +850,9 @@ static int mv88e61xx_phy_setup(struct phy_device *phydev, u8 phy) val = mv88e61xx_phy_read(phydev, phy, PHY_REG_CTRL1); if (val < 0) return val; - val = bitfield_replace(val, PHY_REG_CTRL1_ENERGY_DET_SHIFT, - PHY_REG_CTRL1_ENERGY_DET_WIDTH, - PHY_REG_CTRL1_ENERGY_DET_SENSE_XMIT); + val = bitfield_replace(val, priv->phy_ctrl1_en_det_shift, + priv->phy_ctrl1_en_det_width, + priv->phy_ctrl1_en_det_sense_xmit); val = mv88e61xx_phy_write(phydev, phy, PHY_REG_CTRL1, val); if (val < 0) return val; @@ -964,12 +964,18 @@ static int mv88e61xx_probe(struct phy_device *phydev) priv->port_stat_link_mask = BIT(11); priv->port_stat_dup_mask = BIT(10); priv->port_stat_speed_width = 2; + priv->phy_ctrl1_en_det_shift = 8; + priv->phy_ctrl1_en_det_width = 2; + priv->phy_ctrl1_en_det_sense_xmit = 3; break; case PORT_SWITCH_ID_6071: priv->port_count = 7; priv->port_stat_link_mask = BIT(12); priv->port_stat_dup_mask = BIT(9); priv->port_stat_speed_width = 1; + priv->phy_ctrl1_en_det_shift = 14; + priv->phy_ctrl1_en_det_width = 1; + priv->phy_ctrl1_en_det_sense_xmit = 1; break; default: free(priv);
On Tue, Jul 9, 2019 at 6:24 PM Anatolij Gustschin agust@denx.de wrote:
On mv88E6071 the 'EDet' field offset, width and sense control bits are different, adjust the driver to init the PHY control register as needed. This fixes not working link detection and tftp transfers.
Good grief... kind of obnoxious how much is changed seemingly arbitrarily. Hmm. Not friendly to software.
Signed-off-by: Anatolij Gustschin agust@denx.de
drivers/net/phy/mv88e61xx.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/drivers/net/phy/mv88e61xx.c b/drivers/net/phy/mv88e61xx.c index d38109b797..165bc261e4 100644 --- a/drivers/net/phy/mv88e61xx.c +++ b/drivers/net/phy/mv88e61xx.c @@ -119,16 +119,12 @@
#define SERDES_REG_CTRL_1_FORCE_LINK BIT(10)
-#define PHY_REG_CTRL1_ENERGY_DET_SHIFT 8 -#define PHY_REG_CTRL1_ENERGY_DET_WIDTH 2
/* Field values */ #define PORT_REG_CTRL_PSTATE_DISABLED 0 #define PORT_REG_CTRL_PSTATE_FORWARD 3
#define PHY_REG_CTRL1_ENERGY_DET_OFF 0 #define PHY_REG_CTRL1_ENERGY_DET_SENSE_ONLY 2 -#define PHY_REG_CTRL1_ENERGY_DET_SENSE_XMIT 3
/* PHY Status Register */ #define PHY_REG_STATUS1_SPEED 0xc000 @@ -194,6 +190,9 @@ struct mv88e61xx_phy_priv { u16 port_stat_link_mask; u16 port_stat_dup_mask; u8 port_stat_speed_width;
u8 phy_ctrl1_en_det_shift;u8 phy_ctrl1_en_det_width;u8 phy_ctrl1_en_det_sense_xmit; u8 global1; u8 global2;}; @@ -841,6 +840,7 @@ static int mv88e61xx_phy_enable(struct phy_device *phydev, u8 phy)
static int mv88e61xx_phy_setup(struct phy_device *phydev, u8 phy) {
struct mv88e61xx_phy_priv *priv = phydev->priv; int val; /*@@ -850,9 +850,9 @@ static int mv88e61xx_phy_setup(struct phy_device *phydev, u8 phy) val = mv88e61xx_phy_read(phydev, phy, PHY_REG_CTRL1); if (val < 0) return val;
val = bitfield_replace(val, PHY_REG_CTRL1_ENERGY_DET_SHIFT,PHY_REG_CTRL1_ENERGY_DET_WIDTH,PHY_REG_CTRL1_ENERGY_DET_SENSE_XMIT);
val = bitfield_replace(val, priv->phy_ctrl1_en_det_shift,priv->phy_ctrl1_en_det_width,priv->phy_ctrl1_en_det_sense_xmit); val = mv88e61xx_phy_write(phydev, phy, PHY_REG_CTRL1, val); if (val < 0) return val;@@ -964,12 +964,18 @@ static int mv88e61xx_probe(struct phy_device *phydev) priv->port_stat_link_mask = BIT(11); priv->port_stat_dup_mask = BIT(10); priv->port_stat_speed_width = 2;
priv->phy_ctrl1_en_det_shift = 8;priv->phy_ctrl1_en_det_width = 2;priv->phy_ctrl1_en_det_sense_xmit = 3; break; case PORT_SWITCH_ID_6071: priv->port_count = 7; priv->port_stat_link_mask = BIT(12); priv->port_stat_dup_mask = BIT(9); priv->port_stat_speed_width = 1;priv->phy_ctrl1_en_det_shift = 14;priv->phy_ctrl1_en_det_width = 1;priv->phy_ctrl1_en_det_sense_xmit = 1; break; default: free(priv);-- 2.17.1
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
MV88E61XX_88E6020_FAMILY option enables support for switch devices 6250/6220/6020/6070/6071.
Signed-off-by: Anatolij Gustschin agust@denx.de --- drivers/net/phy/Kconfig | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index 2a3da068c9..097b499ba3 100644 --- a/drivers/net/phy/Kconfig +++ b/drivers/net/phy/Kconfig @@ -50,6 +50,13 @@ config MV88E61XX_SWITCH
if MV88E61XX_SWITCH
+config MV88E61XX_88E6020_FAMILY + bool "Marvell MV88E6020 family support." + help + The driver supports 6172/6176/6240/6352 devices in the + default configuration. Select this option to enable support + for 6250/6220/6020/6070/6071 switches. + config MV88E61XX_CPU_PORT int "CPU Port"
On Tue, Jul 9, 2019 at 6:26 PM Anatolij Gustschin agust@denx.de wrote:
MV88E61XX_88E6020_FAMILY option enables support for switch devices 6250/6220/6020/6070/6071.
Signed-off-by: Anatolij Gustschin agust@denx.de
drivers/net/phy/Kconfig | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index 2a3da068c9..097b499ba3 100644 --- a/drivers/net/phy/Kconfig +++ b/drivers/net/phy/Kconfig @@ -50,6 +50,13 @@ config MV88E61XX_SWITCH
if MV88E61XX_SWITCH
+config MV88E61XX_88E6020_FAMILY
bool "Marvell MV88E6020 family support."helpThe driver supports 6172/6176/6240/6352 devices in thedefault configuration. Select this option to enable supportfor 6250/6220/6020/6070/6071 switches.
Is there a rhyme or reason to the model numbers here? This option seems oddly named, especially since the help doesn't have the model numbers in numerical order. Can you make it a choice instead?
E.g...
choice prompt "MV88E61XX device selection" default MV88E61XX_88E6172
config MV88E61XX_88E6020 bool "MV88E6020"
[ ... ]
config MV88E61XX_88E6172 bool "MV88E6172"
[ ... ]
config MV88E61XX_88E6352 bool "MV88E6352"
help The driver supports 6172/6176/6240/6352 devices in the default configuration. Select the device to enable support for (e.g. 6020/6070/6071/6220/6250).
endchoice
config MV88E61XX_CPU_PORT int "CPU Port"
-- 2.17.1
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
On Tue, 23 Jul 2019 04:26:17 +0000 Joe Hershberger joe.hershberger@ni.com wrote: ...
+config MV88E61XX_88E6020_FAMILY
bool "Marvell MV88E6020 family support."helpThe driver supports 6172/6176/6240/6352 devices in thedefault configuration. Select this option to enable supportfor 6250/6220/6020/6070/6071 switches.Is there a rhyme or reason to the model numbers here? This option seems oddly named, especially since the help doesn't have the model numbers in numerical order. Can you make it a choice instead?
We want to be able to use single U-Boot images which support different switches, a choice will make it not possible.
-- Anatolij
On Thu, Jul 25, 2019 at 4:42 PM Anatolij Gustschin agust@denx.de wrote:
On Tue, 23 Jul 2019 04:26:17 +0000 Joe Hershberger joe.hershberger@ni.com wrote: ...
+config MV88E61XX_88E6020_FAMILY
bool "Marvell MV88E6020 family support."helpThe driver supports 6172/6176/6240/6352 devices in thedefault configuration. Select this option to enable supportfor 6250/6220/6020/6070/6071 switches.Is there a rhyme or reason to the model numbers here? This option seems oddly named, especially since the help doesn't have the model numbers in numerical order. Can you make it a choice instead?
We want to be able to use single U-Boot images which support different switches, a choice will make it not possible.
I don't see how choice is any different than what you have here in that respect. You have a hard-coded selection among families. If you have a need to run the same binary on multiple switches, then this option should be removed and the pre-init should be detecting the target.
-Joe
-- Anatolij _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
On Mon, Jul 29, 2019 at 6:17 PM Joe Hershberger joe.hershberger@ni.com wrote:
On Thu, Jul 25, 2019 at 4:42 PM Anatolij Gustschin agust@denx.de wrote:
On Tue, 23 Jul 2019 04:26:17 +0000 Joe Hershberger joe.hershberger@ni.com wrote: ...
+config MV88E61XX_88E6020_FAMILY
bool "Marvell MV88E6020 family support."helpThe driver supports 6172/6176/6240/6352 devices in thedefault configuration. Select this option to enable supportfor 6250/6220/6020/6070/6071 switches.Is there a rhyme or reason to the model numbers here? This option seems oddly named, especially since the help doesn't have the model numbers in numerical order. Can you make it a choice instead?
We want to be able to use single U-Boot images which support different switches, a choice will make it not possible.
I don't see how choice is any different than what you have here in that respect. You have a hard-coded selection among families. If you have a need to run the same binary on multiple switches, then this option should be removed and the pre-init should be detecting the target.
Any response here?
-Joe
-- Anatolij _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Hi Joe,
On Tue, 3 Sep 2019 22:00:08 +0000 Joe Hershberger joe.hershberger@ni.com wrote:
On Mon, Jul 29, 2019 at 6:17 PM Joe Hershberger joe.hershberger@ni.com wrote:
On Thu, Jul 25, 2019 at 4:42 PM Anatolij Gustschin agust@denx.de wrote:
On Tue, 23 Jul 2019 04:26:17 +0000 Joe Hershberger joe.hershberger@ni.com wrote: ...
+config MV88E61XX_88E6020_FAMILY
bool "Marvell MV88E6020 family support."helpThe driver supports 6172/6176/6240/6352 devices in thedefault configuration. Select this option to enable supportfor 6250/6220/6020/6070/6071 switches.Is there a rhyme or reason to the model numbers here? This option seems oddly named, especially since the help doesn't have the model numbers in numerical order. Can you make it a choice instead?
We want to be able to use single U-Boot images which support different switches, a choice will make it not possible.
I don't see how choice is any different than what you have here in that respect. You have a hard-coded selection among families. If you have a need to run the same binary on multiple switches, then this option should be removed and the pre-init should be detecting the target.
Any response here?
Sorry for delay, I've been too busy with other stuff. v4 patch series was reworked to drop this config option. Thanks!
-- Anatolij
Support probing and init for 88E6071 switch.
Signed-off-by: Anatolij Gustschin agust@denx.de --- drivers/net/phy/mv88e61xx.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/drivers/net/phy/mv88e61xx.c b/drivers/net/phy/mv88e61xx.c index 165bc261e4..d827ecc1bc 100644 --- a/drivers/net/phy/mv88e61xx.c +++ b/drivers/net/phy/mv88e61xx.c @@ -1116,10 +1116,22 @@ static struct phy_driver mv88e609x_driver = { .shutdown = &genphy_shutdown, };
+static struct phy_driver mv88e6071_driver = { + .name = "Marvell MV88E6071", + .uid = 0x1410db0, + .mask = 0xfffffff0, + .features = PHY_BASIC_FEATURES | SUPPORTED_MII, + .probe = mv88e61xx_probe, + .config = mv88e61xx_phy_config, + .startup = mv88e61xx_phy_startup, + .shutdown = &genphy_shutdown, +}; + int phy_mv88e61xx_init(void) { phy_register(&mv88e61xx_driver); phy_register(&mv88e609x_driver); + phy_register(&mv88e6071_driver);
return 0; }
On Tue, Jul 9, 2019 at 6:25 PM Anatolij Gustschin agust@denx.de wrote:
Support probing and init for 88E6071 switch.
Signed-off-by: Anatolij Gustschin agust@denx.de
drivers/net/phy/mv88e61xx.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/drivers/net/phy/mv88e61xx.c b/drivers/net/phy/mv88e61xx.c index 165bc261e4..d827ecc1bc 100644 --- a/drivers/net/phy/mv88e61xx.c +++ b/drivers/net/phy/mv88e61xx.c @@ -1116,10 +1116,22 @@ static struct phy_driver mv88e609x_driver = { .shutdown = &genphy_shutdown, };
+static struct phy_driver mv88e6071_driver = {
.name = "Marvell MV88E6071",
Is it really just the 6071? What about all the other ones mentioned in the Kconfig?
.uid = 0x1410db0,.mask = 0xfffffff0,.features = PHY_BASIC_FEATURES | SUPPORTED_MII,.probe = mv88e61xx_probe,.config = mv88e61xx_phy_config,.startup = mv88e61xx_phy_startup,.shutdown = &genphy_shutdown,+};
int phy_mv88e61xx_init(void) { phy_register(&mv88e61xx_driver); phy_register(&mv88e609x_driver);
phy_register(&mv88e6071_driver); return 0;}
2.17.1
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
On Tue, 23 Jul 2019 04:29:05 +0000 Joe Hershberger joe.hershberger@ni.com wrote: ...
+static struct phy_driver mv88e6071_driver = {
.name = "Marvell MV88E6071",Is it really just the 6071? What about all the other ones mentioned in the Kconfig?
Yes, only 6071 for now. I do not have other compatible devices to get the IDs for adding them in the driver.
-- Anatolij
Fix vendor name in MV88E61xx option description.
Signed-off-by: Anatolij Gustschin agust@denx.de --- drivers/net/phy/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index 097b499ba3..94386b74ba 100644 --- a/drivers/net/phy/Kconfig +++ b/drivers/net/phy/Kconfig @@ -46,7 +46,7 @@ config B53_PHY_PORTS endif # B53_SWITCH
config MV88E61XX_SWITCH - bool "Marvel MV88E61xx Ethernet switch PHY support." + bool "Marvell MV88E61xx Ethernet switch PHY support."
if MV88E61XX_SWITCH
On Tue, Jul 9, 2019 at 6:25 PM Anatolij Gustschin agust@denx.de wrote:
Fix vendor name in MV88E61xx option description.
Signed-off-by: Anatolij Gustschin agust@denx.de
Acked-by: Joe Hershberger joe.hershberger@ni.com
On Wed, Jul 10, 2019 at 11:23 AM Anatolij Gustschin agust@denx.de wrote:
This series adds support for 88E6071 switches in the mv88e61xx driver.
Changes in v2:
- fix port init for 6096/6097 devices in patch 1/6
- update commit description in patch 4/6
Anatolij Gustschin (6): net: phy: mv88e61xx: rework to enable detection of 88E6071 devices net: phy: mv88e61xx: add CPU port parameter init for 88E6071 net: phy: mv88E61xx: fix ENERGY_DET init for mv88E6071 net: phy: mv88E61xx: add config option for mv88E6071 support net: phy: mv88e61xx: register phy_driver struct for 88E6071 net: phy: fix switch vendor name
For the series
Reviewed-by: Chris Packham judge.packham@gmail.com
And in terms of regression testing on 88E6097
Tested-by: Chris Packham judge.packham@gmail.com
Hi Chris,
On Wed, 10 Jul 2019 21:04:42 +1200 Chris Packham judge.packham@gmail.com wrote: ...
Tested-by: Chris Packham judge.packham@gmail.com
Thanks for review and testing!
-- Anatolij
participants (3)
-
Anatolij Gustschin -
Chris Packham -
Joe Hershberger