All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH v2 0/6] Extend mv88e61xx driver to support 88E6071
@ 2019-07-09 23:22 Anatolij Gustschin
  2019-07-09 23:22 ` [U-Boot] [PATCH v2 1/6] net: phy: mv88e61xx: rework to enable detection of 88E6071 devices Anatolij Gustschin
                   ` (6 more replies)
  0 siblings, 7 replies; 25+ messages in thread
From: Anatolij Gustschin @ 2019-07-09 23:22 UTC (permalink / raw)
  To: u-boot

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(-)

-- 
2.17.1

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 1/6] net: phy: mv88e61xx: rework to enable detection of 88E6071 devices
  2019-07-09 23:22 [U-Boot] [PATCH v2 0/6] Extend mv88e61xx driver to support 88E6071 Anatolij Gustschin
@ 2019-07-09 23:22 ` Anatolij Gustschin
  2019-07-23  4:00   ` Joe Hershberger
  2019-07-09 23:22 ` [U-Boot] [PATCH v2 2/6] net: phy: mv88e61xx: add CPU port parameter init for 88E6071 Anatolij Gustschin
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 25+ messages in thread
From: Anatolij Gustschin @ 2019-07-09 23:22 UTC (permalink / raw)
  To: u-boot

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;
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 2/6] net: phy: mv88e61xx: add CPU port parameter init for 88E6071
  2019-07-09 23:22 [U-Boot] [PATCH v2 0/6] Extend mv88e61xx driver to support 88E6071 Anatolij Gustschin
  2019-07-09 23:22 ` [U-Boot] [PATCH v2 1/6] net: phy: mv88e61xx: rework to enable detection of 88E6071 devices Anatolij Gustschin
@ 2019-07-09 23:22 ` Anatolij Gustschin
  2019-07-23  4:05   ` Joe Hershberger
  2019-07-09 23:22 ` [U-Boot] [PATCH v2 3/6] net: phy: mv88E61xx: fix ENERGY_DET init for mv88E6071 Anatolij Gustschin
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 25+ messages in thread
From: Anatolij Gustschin @ 2019-07-09 23:22 UTC (permalink / raw)
  To: u-boot

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);
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 3/6] net: phy: mv88E61xx: fix ENERGY_DET init for mv88E6071
  2019-07-09 23:22 [U-Boot] [PATCH v2 0/6] Extend mv88e61xx driver to support 88E6071 Anatolij Gustschin
  2019-07-09 23:22 ` [U-Boot] [PATCH v2 1/6] net: phy: mv88e61xx: rework to enable detection of 88E6071 devices Anatolij Gustschin
  2019-07-09 23:22 ` [U-Boot] [PATCH v2 2/6] net: phy: mv88e61xx: add CPU port parameter init for 88E6071 Anatolij Gustschin
@ 2019-07-09 23:22 ` Anatolij Gustschin
  2019-07-23  4:11   ` Joe Hershberger
  2019-07-09 23:22 ` [U-Boot] [PATCH v2 4/6] net: phy: mv88E61xx: add config option for mv88E6071 support Anatolij Gustschin
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 25+ messages in thread
From: Anatolij Gustschin @ 2019-07-09 23:22 UTC (permalink / raw)
  To: u-boot

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);
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 4/6] net: phy: mv88E61xx: add config option for mv88E6071 support
  2019-07-09 23:22 [U-Boot] [PATCH v2 0/6] Extend mv88e61xx driver to support 88E6071 Anatolij Gustschin
                   ` (2 preceding siblings ...)
  2019-07-09 23:22 ` [U-Boot] [PATCH v2 3/6] net: phy: mv88E61xx: fix ENERGY_DET init for mv88E6071 Anatolij Gustschin
@ 2019-07-09 23:22 ` Anatolij Gustschin
  2019-07-23  4:26   ` Joe Hershberger
  2019-07-09 23:22 ` [U-Boot] [PATCH v2 5/6] net: phy: mv88e61xx: register phy_driver struct for 88E6071 Anatolij Gustschin
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 25+ messages in thread
From: Anatolij Gustschin @ 2019-07-09 23:22 UTC (permalink / raw)
  To: 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"
 
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 5/6] net: phy: mv88e61xx: register phy_driver struct for 88E6071
  2019-07-09 23:22 [U-Boot] [PATCH v2 0/6] Extend mv88e61xx driver to support 88E6071 Anatolij Gustschin
                   ` (3 preceding siblings ...)
  2019-07-09 23:22 ` [U-Boot] [PATCH v2 4/6] net: phy: mv88E61xx: add config option for mv88E6071 support Anatolij Gustschin
@ 2019-07-09 23:22 ` Anatolij Gustschin
  2019-07-23  4:29   ` Joe Hershberger
  2019-07-09 23:22 ` [U-Boot] [PATCH v2 6/6] net: phy: fix switch vendor name Anatolij Gustschin
  2019-07-10  9:04 ` [U-Boot] [PATCH v2 0/6] Extend mv88e61xx driver to support 88E6071 Chris Packham
  6 siblings, 1 reply; 25+ messages in thread
From: Anatolij Gustschin @ 2019-07-09 23:22 UTC (permalink / raw)
  To: u-boot

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;
 }
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 6/6] net: phy: fix switch vendor name
  2019-07-09 23:22 [U-Boot] [PATCH v2 0/6] Extend mv88e61xx driver to support 88E6071 Anatolij Gustschin
                   ` (4 preceding siblings ...)
  2019-07-09 23:22 ` [U-Boot] [PATCH v2 5/6] net: phy: mv88e61xx: register phy_driver struct for 88E6071 Anatolij Gustschin
@ 2019-07-09 23:22 ` Anatolij Gustschin
  2019-07-23  4:29   ` Joe Hershberger
  2019-07-10  9:04 ` [U-Boot] [PATCH v2 0/6] Extend mv88e61xx driver to support 88E6071 Chris Packham
  6 siblings, 1 reply; 25+ messages in thread
From: Anatolij Gustschin @ 2019-07-09 23:22 UTC (permalink / raw)
  To: u-boot

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
 
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 0/6] Extend mv88e61xx driver to support 88E6071
  2019-07-09 23:22 [U-Boot] [PATCH v2 0/6] Extend mv88e61xx driver to support 88E6071 Anatolij Gustschin
                   ` (5 preceding siblings ...)
  2019-07-09 23:22 ` [U-Boot] [PATCH v2 6/6] net: phy: fix switch vendor name Anatolij Gustschin
@ 2019-07-10  9:04 ` Chris Packham
  2019-07-10  9:14   ` Anatolij Gustschin
  6 siblings, 1 reply; 25+ messages in thread
From: Chris Packham @ 2019-07-10  9:04 UTC (permalink / raw)
  To: u-boot

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>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 0/6] Extend mv88e61xx driver to support 88E6071
  2019-07-10  9:04 ` [U-Boot] [PATCH v2 0/6] Extend mv88e61xx driver to support 88E6071 Chris Packham
@ 2019-07-10  9:14   ` Anatolij Gustschin
  0 siblings, 0 replies; 25+ messages in thread
From: Anatolij Gustschin @ 2019-07-10  9:14 UTC (permalink / raw)
  To: u-boot

Hi Chris,

On Wed, 10 Jul 2019 21:04:42 +1200
Chris Packham judge.packham at gmail.com wrote:
...
> Tested-by: Chris Packham <judge.packham@gmail.com>

Thanks for review and testing!

--
Anatolij

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 1/6] net: phy: mv88e61xx: rework to enable detection of 88E6071 devices
  2019-07-09 23:22 ` [U-Boot] [PATCH v2 1/6] net: phy: mv88e61xx: rework to enable detection of 88E6071 devices Anatolij Gustschin
@ 2019-07-23  4:00   ` Joe Hershberger
  2019-07-23  4:07     ` Joe Hershberger
  2019-07-25 21:40     ` Anatolij Gustschin
  0 siblings, 2 replies; 25+ messages in thread
From: Joe Hershberger @ 2019-07-23  4:00 UTC (permalink / raw)
  To: u-boot

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 at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 2/6] net: phy: mv88e61xx: add CPU port parameter init for 88E6071
  2019-07-09 23:22 ` [U-Boot] [PATCH v2 2/6] net: phy: mv88e61xx: add CPU port parameter init for 88E6071 Anatolij Gustschin
@ 2019-07-23  4:05   ` Joe Hershberger
  2019-07-25 21:40     ` Anatolij Gustschin
  0 siblings, 1 reply; 25+ messages in thread
From: Joe Hershberger @ 2019-07-23  4:05 UTC (permalink / raw)
  To: u-boot

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 at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 1/6] net: phy: mv88e61xx: rework to enable detection of 88E6071 devices
  2019-07-23  4:00   ` Joe Hershberger
@ 2019-07-23  4:07     ` Joe Hershberger
  2019-07-26 14:48       ` Anatolij Gustschin
  2019-07-25 21:40     ` Anatolij Gustschin
  1 sibling, 1 reply; 25+ messages in thread
From: Joe Hershberger @ 2019-07-23  4:07 UTC (permalink / raw)
  To: 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 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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 3/6] net: phy: mv88E61xx: fix ENERGY_DET init for mv88E6071
  2019-07-09 23:22 ` [U-Boot] [PATCH v2 3/6] net: phy: mv88E61xx: fix ENERGY_DET init for mv88E6071 Anatolij Gustschin
@ 2019-07-23  4:11   ` Joe Hershberger
  0 siblings, 0 replies; 25+ messages in thread
From: Joe Hershberger @ 2019-07-23  4:11 UTC (permalink / raw)
  To: u-boot

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 at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 4/6] net: phy: mv88E61xx: add config option for mv88E6071 support
  2019-07-09 23:22 ` [U-Boot] [PATCH v2 4/6] net: phy: mv88E61xx: add config option for mv88E6071 support Anatolij Gustschin
@ 2019-07-23  4:26   ` Joe Hershberger
  2019-07-25 21:41     ` Anatolij Gustschin
  0 siblings, 1 reply; 25+ messages in thread
From: Joe Hershberger @ 2019-07-23  4:26 UTC (permalink / raw)
  To: u-boot

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."
> +       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.

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 at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 5/6] net: phy: mv88e61xx: register phy_driver struct for 88E6071
  2019-07-09 23:22 ` [U-Boot] [PATCH v2 5/6] net: phy: mv88e61xx: register phy_driver struct for 88E6071 Anatolij Gustschin
@ 2019-07-23  4:29   ` Joe Hershberger
  2019-07-25 21:41     ` Anatolij Gustschin
  0 siblings, 1 reply; 25+ messages in thread
From: Joe Hershberger @ 2019-07-23  4:29 UTC (permalink / raw)
  To: u-boot

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 at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 6/6] net: phy: fix switch vendor name
  2019-07-09 23:22 ` [U-Boot] [PATCH v2 6/6] net: phy: fix switch vendor name Anatolij Gustschin
@ 2019-07-23  4:29   ` Joe Hershberger
  0 siblings, 0 replies; 25+ messages in thread
From: Joe Hershberger @ 2019-07-23  4:29 UTC (permalink / raw)
  To: u-boot

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>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 1/6] net: phy: mv88e61xx: rework to enable detection of 88E6071 devices
  2019-07-23  4:00   ` Joe Hershberger
  2019-07-23  4:07     ` Joe Hershberger
@ 2019-07-25 21:40     ` Anatolij Gustschin
  1 sibling, 0 replies; 25+ messages in thread
From: Anatolij Gustschin @ 2019-07-25 21:40 UTC (permalink / raw)
  To: u-boot

On Tue, 23 Jul 2019 04:08:23 +0000
Joe Hershberger joe.hershberger at 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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 2/6] net: phy: mv88e61xx: add CPU port parameter init for 88E6071
  2019-07-23  4:05   ` Joe Hershberger
@ 2019-07-25 21:40     ` Anatolij Gustschin
  0 siblings, 0 replies; 25+ messages in thread
From: Anatolij Gustschin @ 2019-07-25 21:40 UTC (permalink / raw)
  To: u-boot

On Tue, 23 Jul 2019 04:05:56 +0000
Joe Hershberger joe.hershberger at 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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 5/6] net: phy: mv88e61xx: register phy_driver struct for 88E6071
  2019-07-23  4:29   ` Joe Hershberger
@ 2019-07-25 21:41     ` Anatolij Gustschin
  0 siblings, 0 replies; 25+ messages in thread
From: Anatolij Gustschin @ 2019-07-25 21:41 UTC (permalink / raw)
  To: u-boot

On Tue, 23 Jul 2019 04:29:05 +0000
Joe Hershberger joe.hershberger at 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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 4/6] net: phy: mv88E61xx: add config option for mv88E6071 support
  2019-07-23  4:26   ` Joe Hershberger
@ 2019-07-25 21:41     ` Anatolij Gustschin
  2019-07-29 23:18       ` Joe Hershberger
  0 siblings, 1 reply; 25+ messages in thread
From: Anatolij Gustschin @ 2019-07-25 21:41 UTC (permalink / raw)
  To: u-boot

On Tue, 23 Jul 2019 04:26:17 +0000
Joe Hershberger joe.hershberger at ni.com wrote:
...
> > +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.  
> 
> 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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 1/6] net: phy: mv88e61xx: rework to enable detection of 88E6071 devices
  2019-07-23  4:07     ` Joe Hershberger
@ 2019-07-26 14:48       ` Anatolij Gustschin
  2019-07-26 18:51         ` Joe Hershberger
  0 siblings, 1 reply; 25+ messages in thread
From: Anatolij Gustschin @ 2019-07-26 14:48 UTC (permalink / raw)
  To: u-boot

On Tue, 23 Jul 2019 04:07:04 +0000
Joe Hershberger joe.hershberger at 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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 1/6] net: phy: mv88e61xx: rework to enable detection of 88E6071 devices
  2019-07-26 14:48       ` Anatolij Gustschin
@ 2019-07-26 18:51         ` Joe Hershberger
  0 siblings, 0 replies; 25+ messages in thread
From: Joe Hershberger @ 2019-07-26 18:51 UTC (permalink / raw)
  To: u-boot

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 at 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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 4/6] net: phy: mv88E61xx: add config option for mv88E6071 support
  2019-07-25 21:41     ` Anatolij Gustschin
@ 2019-07-29 23:18       ` Joe Hershberger
  2019-09-03 22:00         ` Joe Hershberger
  0 siblings, 1 reply; 25+ messages in thread
From: Joe Hershberger @ 2019-07-29 23:18 UTC (permalink / raw)
  To: u-boot

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 at ni.com wrote:
> ...
> > > +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.
> >
> > 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 at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 4/6] net: phy: mv88E61xx: add config option for mv88E6071 support
  2019-07-29 23:18       ` Joe Hershberger
@ 2019-09-03 22:00         ` Joe Hershberger
  2019-10-26 23:15           ` Anatolij Gustschin
  0 siblings, 1 reply; 25+ messages in thread
From: Joe Hershberger @ 2019-09-03 22:00 UTC (permalink / raw)
  To: 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 at ni.com wrote:
> > ...
> > > > +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.
> > >
> > > 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 at lists.denx.de
> > https://lists.denx.de/listinfo/u-boot

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 4/6] net: phy: mv88E61xx: add config option for mv88E6071 support
  2019-09-03 22:00         ` Joe Hershberger
@ 2019-10-26 23:15           ` Anatolij Gustschin
  0 siblings, 0 replies; 25+ messages in thread
From: Anatolij Gustschin @ 2019-10-26 23:15 UTC (permalink / raw)
  To: u-boot

Hi Joe,

On Tue, 3 Sep 2019 22:00:08 +0000
Joe Hershberger joe.hershberger at 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 at ni.com wrote:
> > > ...  
> > > > > +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.  
> > > >
> > > > 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

^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2019-10-26 23:15 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-09 23:22 [U-Boot] [PATCH v2 0/6] Extend mv88e61xx driver to support 88E6071 Anatolij Gustschin
2019-07-09 23:22 ` [U-Boot] [PATCH v2 1/6] net: phy: mv88e61xx: rework to enable detection of 88E6071 devices Anatolij Gustschin
2019-07-23  4:00   ` Joe Hershberger
2019-07-23  4:07     ` Joe Hershberger
2019-07-26 14:48       ` Anatolij Gustschin
2019-07-26 18:51         ` Joe Hershberger
2019-07-25 21:40     ` Anatolij Gustschin
2019-07-09 23:22 ` [U-Boot] [PATCH v2 2/6] net: phy: mv88e61xx: add CPU port parameter init for 88E6071 Anatolij Gustschin
2019-07-23  4:05   ` Joe Hershberger
2019-07-25 21:40     ` Anatolij Gustschin
2019-07-09 23:22 ` [U-Boot] [PATCH v2 3/6] net: phy: mv88E61xx: fix ENERGY_DET init for mv88E6071 Anatolij Gustschin
2019-07-23  4:11   ` Joe Hershberger
2019-07-09 23:22 ` [U-Boot] [PATCH v2 4/6] net: phy: mv88E61xx: add config option for mv88E6071 support Anatolij Gustschin
2019-07-23  4:26   ` Joe Hershberger
2019-07-25 21:41     ` Anatolij Gustschin
2019-07-29 23:18       ` Joe Hershberger
2019-09-03 22:00         ` Joe Hershberger
2019-10-26 23:15           ` Anatolij Gustschin
2019-07-09 23:22 ` [U-Boot] [PATCH v2 5/6] net: phy: mv88e61xx: register phy_driver struct for 88E6071 Anatolij Gustschin
2019-07-23  4:29   ` Joe Hershberger
2019-07-25 21:41     ` Anatolij Gustschin
2019-07-09 23:22 ` [U-Boot] [PATCH v2 6/6] net: phy: fix switch vendor name Anatolij Gustschin
2019-07-23  4:29   ` Joe Hershberger
2019-07-10  9:04 ` [U-Boot] [PATCH v2 0/6] Extend mv88e61xx driver to support 88E6071 Chris Packham
2019-07-10  9:14   ` Anatolij Gustschin

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.