[U-Boot] [PATCH v4 0/3] Add MMD PHY helpers

[U-Boot] [PATCH v4 0/3] Add MMD PHY helpers

[Carlo Caione]

Introduce phy_(read|write)_mmd() generic 802.3 clause 45 register
accessors for Clause 22 PHYs, using the indirect method. Allow this
behaviour to be overriden by PHY drivers where necessary.

Carlo Caione (3):
  net: phy: Add support for accessing MMD PHY registers
  net: phy: ti: use generic helpers to access MMD registers
  cmd: mdio: Add new parameter to access MMD PHY registers

 cmd/mdio.c            |  52 ++++++++++++-----
 drivers/net/phy/phy.c |   4 ++
 drivers/net/phy/ti.c  | 130 ++++++++----------------------------------
 include/phy.h         |  52 +++++++++++++++++
 4 files changed, 118 insertions(+), 120 deletions(-)

-- 
2.19.1

[U-Boot] [PATCH v4 1/3] net: phy: Add support for accessing MMD PHY registers

[Carlo Caione]

Two new helper functions (phy_read_mmd() and phy_write_mmd()) are added
to allow access to the MMD PHY registers.

The MMD PHY registers can be accessed by two means:

1. Using two new MMD access function hooks in the PHY driver. These
functions can be implemented when the PHY driver does not support the
standard IEEE Compatible clause 45 access mechanism described in clause
22 or if the PHY uses its own non-standard access mechanism.

2. The standard clause 45 access extensions to the MMD registers through
the indirection registers (clause 22) in all the other cases.

Signed-off-by: Carlo Caione <ccaione@baylibre.com>
Acked-by: Joe Hershberger <joe.hershberger@ni.com>
---
 drivers/net/phy/phy.c |  4 ++++
 include/phy.h         | 52 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index cda4caa803..6769047407 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -549,6 +549,10 @@ int phy_register(struct phy_driver *drv)
 		drv->readext += gd->reloc_off;
 	if (drv->writeext)
 		drv->writeext += gd->reloc_off;
+	if (drv->read_mmd)
+		drv->read_mmd += gd->reloc_off;
+	if (drv->write_mmd)
+		drv->write_mmd += gd->reloc_off;
 #endif
 	return 0;
 }
diff --git a/include/phy.h b/include/phy.h
index b86fdfb2ce..440013d000 100644
--- a/include/phy.h
+++ b/include/phy.h
@@ -101,6 +101,14 @@ struct phy_driver {
 	int (*readext)(struct phy_device *phydev, int addr, int devad, int reg);
 	int (*writeext)(struct phy_device *phydev, int addr, int devad, int reg,
 			u16 val);
+
+	/* Phy specific driver override for reading a MMD register */
+	int (*read_mmd)(struct phy_device *phydev, int devad, int reg);
+
+	/* Phy specific driver override for writing a MMD register */
+	int (*write_mmd)(struct phy_device *phydev, int devad, int reg,
+			 u16 val);
+
 	struct list_head list;
 };
 
@@ -164,6 +172,50 @@ static inline int phy_write(struct phy_device *phydev, int devad, int regnum,
 	return bus->write(bus, phydev->addr, devad, regnum, val);
 }
 
+static inline void phy_mmd_start_indirect(struct phy_device *phydev, int devad,
+					  int regnum)
+{
+	/* Write the desired MMD Devad */
+	phy_write(phydev, MDIO_DEVAD_NONE, MII_MMD_CTRL, devad);
+
+	/* Write the desired MMD register address */
+	phy_write(phydev, MDIO_DEVAD_NONE, MII_MMD_DATA, regnum);
+
+	/* Select the Function : DATA with no post increment */
+	phy_write(phydev, MDIO_DEVAD_NONE, MII_MMD_CTRL,
+		  (devad | MII_MMD_CTRL_NOINCR));
+}
+
+static inline int phy_read_mmd(struct phy_device *phydev, int devad,
+			       int regnum)
+{
+	if (regnum > (u16)~0 || devad > 32)
+		return -EINVAL;
+
+	if (phydev->drv->read_mmd)
+		return phydev->drv->read_mmd(phydev, devad, regnum);
+
+	phy_mmd_start_indirect(phydev, devad, regnum);
+
+	/* Read the content of the MMD's selected register */
+	return phy_read(phydev, MDIO_DEVAD_NONE, MII_MMD_DATA);
+}
+
+static inline int phy_write_mmd(struct phy_device *phydev, int devad,
+				int regnum, u16 val)
+{
+	if (regnum > (u16)~0 || devad > 32)
+		return -EINVAL;
+
+	if (phydev->drv->write_mmd)
+		return phydev->drv->write_mmd(phydev, devad, regnum, val);
+
+	phy_mmd_start_indirect(phydev, devad, regnum);
+
+	/* Write the data into MMD's selected register */
+	return phy_write(phydev, MDIO_DEVAD_NONE, MII_MMD_DATA, val);
+}
+
 #ifdef CONFIG_PHYLIB_10G
 extern struct phy_driver gen10g_driver;
 
-- 
2.19.1

[U-Boot] [PATCH v4 2/3] net: phy: ti: use generic helpers to access MMD registers

[Carlo Caione]

Now that generic helpers are available, use those instead of relying on
ti specific functions.

Signed-off-by: Carlo Caione <ccaione@baylibre.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Acked-by: Joe Hershberger <joe.hershberger@ni.com>
---
 drivers/net/phy/ti.c | 130 +++++++++----------------------------------
 1 file changed, 25 insertions(+), 105 deletions(-)

diff --git a/drivers/net/phy/ti.c b/drivers/net/phy/ti.c
index 6db6edd0d0..6ac890a7f5 100644
--- a/drivers/net/phy/ti.c
+++ b/drivers/net/phy/ti.c
@@ -73,16 +73,6 @@
 #define MII_DP83867_CFG2_SPEEDOPT_INTLOW	0x2000
 #define MII_DP83867_CFG2_MASK			0x003F
 
-#define MII_MMD_CTRL	0x0d /* MMD Access Control Register */
-#define MII_MMD_DATA	0x0e /* MMD Access Data Register */
-
-/* MMD Access Control register fields */
-#define MII_MMD_CTRL_DEVAD_MASK	0x1f /* Mask MMD DEVAD*/
-#define MII_MMD_CTRL_ADDR	0x0000 /* Address */
-#define MII_MMD_CTRL_NOINCR	0x4000 /* no post increment */
-#define MII_MMD_CTRL_INCR_RDWT	0x8000 /* post increment on reads & writes */
-#define MII_MMD_CTRL_INCR_ON_WT	0xC000 /* post increment on writes only */
-
 /* User setting - can be taken from DTS */
 #define DEFAULT_RX_ID_DELAY	DP83867_RGMIIDCTL_2_25_NS
 #define DEFAULT_TX_ID_DELAY	DP83867_RGMIIDCTL_2_75_NS
@@ -116,88 +106,20 @@ struct dp83867_private {
 	int clk_output_sel;
 };
 
-/**
- * phy_read_mmd_indirect - reads data from the MMD registers
- * @phydev: The PHY device bus
- * @prtad: MMD Address
- * @devad: MMD DEVAD
- * @addr: PHY address on the MII bus
- *
- * Description: it reads data from the MMD registers (clause 22 to access to
- * clause 45) of the specified phy address.
- * To read these registers we have:
- * 1) Write reg 13 // DEVAD
- * 2) Write reg 14 // MMD Address
- * 3) Write reg 13 // MMD Data Command for MMD DEVAD
- * 3) Read  reg 14 // Read MMD data
- */
-int phy_read_mmd_indirect(struct phy_device *phydev, int prtad,
-			  int devad, int addr)
-{
-	int value = -1;
-
-	/* Write the desired MMD Devad */
-	phy_write(phydev, addr, MII_MMD_CTRL, devad);
-
-	/* Write the desired MMD register address */
-	phy_write(phydev, addr, MII_MMD_DATA, prtad);
-
-	/* Select the Function : DATA with no post increment */
-	phy_write(phydev, addr, MII_MMD_CTRL, (devad | MII_MMD_CTRL_NOINCR));
-
-	/* Read the content of the MMD's selected register */
-	value = phy_read(phydev, addr, MII_MMD_DATA);
-	return value;
-}
-
-/**
- * phy_write_mmd_indirect - writes data to the MMD registers
- * @phydev: The PHY device
- * @prtad: MMD Address
- * @devad: MMD DEVAD
- * @addr: PHY address on the MII bus
- * @data: data to write in the MMD register
- *
- * Description: Write data from the MMD registers of the specified
- * phy address.
- * To write these registers we have:
- * 1) Write reg 13 // DEVAD
- * 2) Write reg 14 // MMD Address
- * 3) Write reg 13 // MMD Data Command for MMD DEVAD
- * 3) Write reg 14 // Write MMD data
- */
-void phy_write_mmd_indirect(struct phy_device *phydev, int prtad,
-			    int devad, int addr, u32 data)
-{
-	/* Write the desired MMD Devad */
-	phy_write(phydev, addr, MII_MMD_CTRL, devad);
-
-	/* Write the desired MMD register address */
-	phy_write(phydev, addr, MII_MMD_DATA, prtad);
-
-	/* Select the Function : DATA with no post increment */
-	phy_write(phydev, addr, MII_MMD_CTRL, (devad | MII_MMD_CTRL_NOINCR));
-
-	/* Write the data into MMD's selected register */
-	phy_write(phydev, addr, MII_MMD_DATA, data);
-}
-
 static int dp83867_config_port_mirroring(struct phy_device *phydev)
 {
 	struct dp83867_private *dp83867 =
 		(struct dp83867_private *)phydev->priv;
 	u16 val;
 
-	val = phy_read_mmd_indirect(phydev, DP83867_CFG4, DP83867_DEVADDR,
-				    phydev->addr);
+	val = phy_read_mmd(phydev, DP83867_DEVADDR, DP83867_CFG4);
 
 	if (dp83867->port_mirroring == DP83867_PORT_MIRRORING_EN)
 		val |= DP83867_CFG4_PORT_MIRROR_EN;
 	else
 		val &= ~DP83867_CFG4_PORT_MIRROR_EN;
 
-	phy_write_mmd_indirect(phydev, DP83867_CFG4, DP83867_DEVADDR,
-			       phydev->addr, val);
+	phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_CFG4, val);
 
 	return 0;
 }
@@ -257,13 +179,13 @@ static int dp83867_of_init(struct phy_device *phydev)
 
 	/* Clock output selection if muxing property is set */
 	if (dp83867->clk_output_sel != DP83867_CLK_O_SEL_REF_CLK) {
-		val = phy_read_mmd_indirect(phydev, DP83867_IO_MUX_CFG,
-					    DP83867_DEVADDR, phydev->addr);
+		val = phy_read_mmd(phydev, DP83867_DEVADDR,
+				   DP83867_IO_MUX_CFG);
 		val &= ~DP83867_IO_MUX_CFG_CLK_O_SEL_MASK;
 		val |= (dp83867->clk_output_sel <<
 			DP83867_IO_MUX_CFG_CLK_O_SEL_SHIFT);
-		phy_write_mmd_indirect(phydev, DP83867_IO_MUX_CFG,
-				       DP83867_DEVADDR, phydev->addr, val);
+		phy_write_mmd(phydev, DP83867_DEVADDR,
+			      DP83867_IO_MUX_CFG, val);
 	}
 
 	return 0;
@@ -308,11 +230,11 @@ static int dp83867_config(struct phy_device *phydev)
 
 	/* Mode 1 or 2 workaround */
 	if (dp83867->rxctrl_strap_quirk) {
-		val = phy_read_mmd_indirect(phydev, DP83867_CFG4,
-					    DP83867_DEVADDR, phydev->addr);
+		val = phy_read_mmd(phydev, DP83867_DEVADDR,
+				   DP83867_CFG4);
 		val &= ~BIT(7);
-		phy_write_mmd_indirect(phydev, DP83867_CFG4,
-				       DP83867_DEVADDR, phydev->addr, val);
+		phy_write_mmd(phydev, DP83867_DEVADDR,
+			      DP83867_CFG4, val);
 	}
 
 	if (phy_interface_is_rgmii(phydev)) {
@@ -332,8 +254,8 @@ static int dp83867_config(struct phy_device *phydev)
 		 * register's bit 11 (marked as RESERVED).
 		 */
 
-		bs = phy_read_mmd_indirect(phydev, DP83867_STRAP_STS1,
-					   DP83867_DEVADDR, phydev->addr);
+		bs = phy_read_mmd(phydev, DP83867_DEVADDR,
+				  DP83867_STRAP_STS1);
 		val = phy_read(phydev, MDIO_DEVAD_NONE, MII_DP83867_PHYCTRL);
 		if (bs & DP83867_STRAP_STS1_RESERVED) {
 			val &= ~DP83867_PHYCR_RESERVED_MASK;
@@ -354,8 +276,8 @@ static int dp83867_config(struct phy_device *phydev)
 			 MII_DP83867_CFG2_SPEEDOPT_INTLOW);
 		phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83867_CFG2, cfg2);
 
-		phy_write_mmd_indirect(phydev, DP83867_RGMIICTL,
-				       DP83867_DEVADDR, phydev->addr, 0x0);
+		phy_write_mmd(phydev, DP83867_DEVADDR,
+			      DP83867_RGMIICTL, 0x0);
 
 		phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83867_PHYCTRL,
 			  DP83867_PHYCTRL_SGMIIEN |
@@ -367,8 +289,8 @@ static int dp83867_config(struct phy_device *phydev)
 	}
 
 	if (phy_interface_is_rgmii(phydev)) {
-		val = phy_read_mmd_indirect(phydev, DP83867_RGMIICTL,
-					    DP83867_DEVADDR, phydev->addr);
+		val = phy_read_mmd(phydev, DP83867_DEVADDR,
+				   DP83867_RGMIICTL);
 
 		if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
 			val |= (DP83867_RGMII_TX_CLK_DELAY_EN |
@@ -380,26 +302,24 @@ static int dp83867_config(struct phy_device *phydev)
 		if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID)
 			val |= DP83867_RGMII_RX_CLK_DELAY_EN;
 
-		phy_write_mmd_indirect(phydev, DP83867_RGMIICTL,
-				       DP83867_DEVADDR, phydev->addr, val);
+		phy_write_mmd(phydev, DP83867_DEVADDR,
+			      DP83867_RGMIICTL, val);
 
 		delay = (dp83867->rx_id_delay |
 			 (dp83867->tx_id_delay << DP83867_RGMII_TX_CLK_DELAY_SHIFT));
 
-		phy_write_mmd_indirect(phydev, DP83867_RGMIIDCTL,
-				       DP83867_DEVADDR, phydev->addr, delay);
+		phy_write_mmd(phydev, DP83867_DEVADDR,
+			      DP83867_RGMIIDCTL, delay);
 
 		if (dp83867->io_impedance >= 0) {
-			val = phy_read_mmd_indirect(phydev,
-						    DP83867_IO_MUX_CFG,
-						    DP83867_DEVADDR,
-						    phydev->addr);
+			val = phy_read_mmd(phydev,
+					   DP83867_DEVADDR,
+					   DP83867_IO_MUX_CFG);
 			val &= ~DP83867_IO_MUX_CFG_IO_IMPEDANCE_CTRL;
 			val |= dp83867->io_impedance &
 			       DP83867_IO_MUX_CFG_IO_IMPEDANCE_CTRL;
-			phy_write_mmd_indirect(phydev, DP83867_IO_MUX_CFG,
-					       DP83867_DEVADDR, phydev->addr,
-					       val);
+			phy_write_mmd(phydev, DP83867_DEVADDR,
+				      DP83867_IO_MUX_CFG, val);
 		}
 	}
 
-- 
2.19.1

[U-Boot] [PATCH v4 3/3] cmd: mdio: Add new parameter to access MMD PHY registers

[Carlo Caione]

Two new parameters (rmmd and wmmd) are added to allow the `mdio` command
to access the content of the MMD PHY registers.

Signed-off-by: Carlo Caione <ccaione@baylibre.com>
Acked-by: Joe Hershberger <joe.hershberger@ni.com>
---
 cmd/mdio.c | 52 +++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 37 insertions(+), 15 deletions(-)

diff --git a/cmd/mdio.c b/cmd/mdio.c
index 184868063a..5138db505a 100644
--- a/cmd/mdio.c
+++ b/cmd/mdio.c
@@ -43,7 +43,7 @@ static int mdio_write_ranges(struct phy_device *phydev, struct mii_dev *bus,
 			     int addrlo,
 			     int addrhi, int devadlo, int devadhi,
 			     int reglo, int reghi, unsigned short data,
-			     int extended)
+			     int extended, int mmd)
 {
 	int addr, devad, reg;
 	int err = 0;
@@ -51,12 +51,15 @@ static int mdio_write_ranges(struct phy_device *phydev, struct mii_dev *bus,
 	for (addr = addrlo; addr <= addrhi; addr++) {
 		for (devad = devadlo; devad <= devadhi; devad++) {
 			for (reg = reglo; reg <= reghi; reg++) {
-				if (!extended)
-					err = bus->write(bus, addr, devad,
-							 reg, data);
-				else
+				if (mmd)
+					err = phy_write_mmd(phydev, devad, reg,
+							    data);
+				else if (extended)
 					err = phydev->drv->writeext(phydev,
 							addr, devad, reg, data);
+				else
+					err = bus->write(bus, addr, devad,
+							 reg, data);
 
 				if (err)
 					goto err_out;
@@ -71,7 +74,7 @@ err_out:
 static int mdio_read_ranges(struct phy_device *phydev, struct mii_dev *bus,
 			    int addrlo,
 			    int addrhi, int devadlo, int devadhi,
-			    int reglo, int reghi, int extended)
+			    int reglo, int reghi, int extended, int mmd)
 {
 	int addr, devad, reg;
 
@@ -83,11 +86,13 @@ static int mdio_read_ranges(struct phy_device *phydev, struct mii_dev *bus,
 			for (reg = reglo; reg <= reghi; reg++) {
 				int val;
 
-				if (!extended)
-					val = bus->read(bus, addr, devad, reg);
-				else
+				if (mmd)
+					val = phy_read_mmd(phydev, devad, reg);
+				else if (extended)
 					val = phydev->drv->readext(phydev, addr,
 						devad, reg);
+				else
+					val = bus->read(bus, addr, devad, reg);
 
 				if (val < 0) {
 					printf("Error\n");
@@ -189,6 +194,7 @@ static int do_mdio(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	struct mii_dev *bus;
 	struct phy_device *phydev = NULL;
 	int extended = 0;
+	int mmd = 0;
 
 	if (argc < 2)
 		return CMD_RET_USAGE;
@@ -222,14 +228,26 @@ static int do_mdio(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 				bus = phydev->bus;
 				extended = 1;
 			} else {
-				return -1;
+				return CMD_RET_FAILURE;
 			}
 
 			if (!phydev->drv ||
 			    (!phydev->drv->writeext && (op[0] == 'w')) ||
 			    (!phydev->drv->readext && (op[0] == 'r'))) {
 				puts("PHY does not have extended functions\n");
-				return -1;
+				return CMD_RET_FAILURE;
+			}
+		}
+		if (op[1] == 'm') {
+			phydev = mdio_phydev_for_ethname(argv[2]);
+
+			if (phydev) {
+				addrlo = phydev->addr;
+				addrhi = addrlo;
+				bus = phydev->bus;
+				mmd = 1;
+			} else {
+				return CMD_RET_FAILURE;
 			}
 		}
 	}
@@ -242,13 +260,13 @@ static int do_mdio(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		if (pos > 1)
 			if (extract_reg_range(argv[pos--], &devadlo, &devadhi,
 					      &reglo, &reghi))
-				return -1;
+				return CMD_RET_FAILURE;
 
 	default:
 		if (pos > 1)
 			if (extract_phy_range(&argv[2], pos - 1, &bus,
 					      &phydev, &addrlo, &addrhi))
-				return -1;
+				return CMD_RET_FAILURE;
 
 		break;
 	}
@@ -265,12 +283,12 @@ static int do_mdio(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	switch (op[0]) {
 	case 'w':
 		mdio_write_ranges(phydev, bus, addrlo, addrhi, devadlo, devadhi,
-				  reglo, reghi, data, extended);
+				  reglo, reghi, data, extended, mmd);
 		break;
 
 	case 'r':
 		mdio_read_ranges(phydev, bus, addrlo, addrhi, devadlo, devadhi,
-				 reglo, reghi, extended);
+				 reglo, reghi, extended, mmd);
 		break;
 	}
 
@@ -303,6 +321,10 @@ U_BOOT_CMD(
 		"read PHY's extended register at <devad>.<reg>\n"
 	"mdio wx <phydev> [<devad>.]<reg> <data> - "
 		"write PHY's extended register at <devad>.<reg>\n"
+	"mdio rmmd <phydev> [<devad>.]<reg> - "
+		"read PHY's extended register at <devad>.<reg>\n"
+	"mdio wmmd <phydev> [<devad>.]<reg> <data> - "
+		"write PHY's extended register at <devad>.<reg>\n"
 	"<phydev> may be:\n"
 	"   <busname>  <addr>\n"
 	"   <addr>\n"
-- 
2.19.1

[U-Boot] [PATCH v4 3/3] cmd: mdio: Add new parameter to access MMD PHY registers

[Vladimir Oltean]

On 1/24/19 10:56 AM, Carlo Caione wrote:
> Two new parameters (rmmd and wmmd) are added to allow the `mdio` command
> to access the content of the MMD PHY registers.
> 
> Signed-off-by: Carlo Caione <ccaione@baylibre.com>
> Acked-by: Joe Hershberger <joe.hershberger@ni.com>
> ---
>   cmd/mdio.c | 52 +++++++++++++++++++++++++++++++++++++---------------
>   1 file changed, 37 insertions(+), 15 deletions(-)
> 
> diff --git a/cmd/mdio.c b/cmd/mdio.c
> index 184868063a..5138db505a 100644
> --- a/cmd/mdio.c
> +++ b/cmd/mdio.c
> @@ -43,7 +43,7 @@ static int mdio_write_ranges(struct phy_device *phydev, struct mii_dev *bus,
>   			     int addrlo,
>   			     int addrhi, int devadlo, int devadhi,
>   			     int reglo, int reghi, unsigned short data,
> -			     int extended)
> +			     int extended, int mmd)
>   {
>   	int addr, devad, reg;
>   	int err = 0;
> @@ -51,12 +51,15 @@ static int mdio_write_ranges(struct phy_device *phydev, struct mii_dev *bus,
>   	for (addr = addrlo; addr <= addrhi; addr++) {
>   		for (devad = devadlo; devad <= devadhi; devad++) {
>   			for (reg = reglo; reg <= reghi; reg++) {
> -				if (!extended)
> -					err = bus->write(bus, addr, devad,
> -							 reg, data);
> -				else
> +				if (mmd)
> +					err = phy_write_mmd(phydev, devad, reg,
> +							    data);
> +				else if (extended)
>   					err = phydev->drv->writeext(phydev,
>   							addr, devad, reg, data);
> +				else
> +					err = bus->write(bus, addr, devad,
> +							 reg, data);
>   
>   				if (err)
>   					goto err_out;
> @@ -71,7 +74,7 @@ err_out:
>   static int mdio_read_ranges(struct phy_device *phydev, struct mii_dev *bus,
>   			    int addrlo,
>   			    int addrhi, int devadlo, int devadhi,
> -			    int reglo, int reghi, int extended)
> +			    int reglo, int reghi, int extended, int mmd)
>   {
>   	int addr, devad, reg;
>   
> @@ -83,11 +86,13 @@ static int mdio_read_ranges(struct phy_device *phydev, struct mii_dev *bus,
>   			for (reg = reglo; reg <= reghi; reg++) {
>   				int val;
>   
> -				if (!extended)
> -					val = bus->read(bus, addr, devad, reg);
> -				else
> +				if (mmd)
> +					val = phy_read_mmd(phydev, devad, reg);
> +				else if (extended)
>   					val = phydev->drv->readext(phydev, addr,
>   						devad, reg);
> +				else
> +					val = bus->read(bus, addr, devad, reg);
>   
>   				if (val < 0) {
>   					printf("Error\n");
> @@ -189,6 +194,7 @@ static int do_mdio(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>   	struct mii_dev *bus;
>   	struct phy_device *phydev = NULL;
>   	int extended = 0;
> +	int mmd = 0;
>   
>   	if (argc < 2)
>   		return CMD_RET_USAGE;
> @@ -222,14 +228,26 @@ static int do_mdio(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>   				bus = phydev->bus;
>   				extended = 1;
>   			} else {
> -				return -1;
> +				return CMD_RET_FAILURE;
>   			}
>   
>   			if (!phydev->drv ||
>   			    (!phydev->drv->writeext && (op[0] == 'w')) ||
>   			    (!phydev->drv->readext && (op[0] == 'r'))) {
>   				puts("PHY does not have extended functions\n");
> -				return -1;
> +				return CMD_RET_FAILURE;
> +			}
> +		}
> +		if (op[1] == 'm') {
> +			phydev = mdio_phydev_for_ethname(argv[2]);
> +
> +			if (phydev) {
> +				addrlo = phydev->addr;
> +				addrhi = addrlo;
> +				bus = phydev->bus;
> +				mmd = 1;
> +			} else {
> +				return CMD_RET_FAILURE;
>   			}
>   		}
>   	}
> @@ -242,13 +260,13 @@ static int do_mdio(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>   		if (pos > 1)
>   			if (extract_reg_range(argv[pos--], &devadlo, &devadhi,
>   					      &reglo, &reghi))
> -				return -1;
> +				return CMD_RET_FAILURE;
>   
>   	default:
>   		if (pos > 1)
>   			if (extract_phy_range(&argv[2], pos - 1, &bus,
>   					      &phydev, &addrlo, &addrhi))
> -				return -1;
> +				return CMD_RET_FAILURE;
>   
>   		break;
>   	}
> @@ -265,12 +283,12 @@ static int do_mdio(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>   	switch (op[0]) {
>   	case 'w':
>   		mdio_write_ranges(phydev, bus, addrlo, addrhi, devadlo, devadhi,
> -				  reglo, reghi, data, extended);
> +				  reglo, reghi, data, extended, mmd);
>   		break;
>   
>   	case 'r':
>   		mdio_read_ranges(phydev, bus, addrlo, addrhi, devadlo, devadhi,
> -				 reglo, reghi, extended);
> +				 reglo, reghi, extended, mmd);
>   		break;
>   	}
>   
> @@ -303,6 +321,10 @@ U_BOOT_CMD(
>   		"read PHY's extended register at <devad>.<reg>\n"
>   	"mdio wx <phydev> [<devad>.]<reg> <data> - "
>   		"write PHY's extended register at <devad>.<reg>\n"
> +	"mdio rmmd <phydev> [<devad>.]<reg> - "
> +		"read PHY's extended register at <devad>.<reg>\n"
> +	"mdio wmmd <phydev> [<devad>.]<reg> <data> - "
> +		"write PHY's extended register at <devad>.<reg>\n"
>   	"<phydev> may be:\n"
>   	"   <busname>  <addr>\n"
>   	"   <addr>\n"
> 

It works for me, but I do have a question.
Is there any limitation preventing you to add this functionality via the 
standard "mdio read x.y" instead of "mdio rmmd x.y" if the PHY is known 
to be C22?

-Vladimir

[U-Boot] [PATCH v4 3/3] cmd: mdio: Add new parameter to access MMD PHY registers

[Carlo Caione]

On 24/01/2019 19:56, Vladimir Oltean wrote:
> On 1/24/19 10:56 AM, Carlo Caione wrote:

> It works for me, but I do have a question.
> Is there any limitation preventing you to add this functionality via the
> standard "mdio read x.y" instead of "mdio rmmd x.y" if the PHY is known
> to be C22?

You can used the standard "mdio read" but it's more verbose and hard to 
recall:

mdio write 0 0.d 0x3
mdio write 0 0.e 0x1
mdio write 0 0.d 0x4003
mdio read 0 0.e

vs

mdio rmmd 3.1

--
Carlo Caione

[U-Boot] [PATCH v4 3/3] cmd: mdio: Add new parameter to access MMD PHY registers

[Vladimir Oltean]

On 1/24/19 10:01 PM, Carlo Caione wrote:
> On 24/01/2019 19:56, Vladimir Oltean wrote:
>> On 1/24/19 10:56 AM, Carlo Caione wrote:
> 
>> It works for me, but I do have a question.
>> Is there any limitation preventing you to add this functionality via the
>> standard "mdio read x.y" instead of "mdio rmmd x.y" if the PHY is known
>> to be C22?
> 
> You can used the standard "mdio read" but it's more verbose and hard to
> recall:
> 
> mdio write 0 0.d 0x3
> mdio write 0 0.e 0x1
> mdio write 0 0.d 0x4003
> mdio read 0 0.e
> 
> vs
> 
> mdio rmmd 3.1
> 
> --
> Carlo Caione
> 

No, I mean instead of doing "mdio rmmd 3.1" to do "mdio read 3.1" 
(basically not define a new command).

[U-Boot] [PATCH v4 3/3] cmd: mdio: Add new parameter to access MMD PHY registers

[Carlo Caione]

On 24/01/2019 20:04, Vladimir Oltean wrote:
> On 1/24/19 10:01 PM, Carlo Caione wrote:
>> On 24/01/2019 19:56, Vladimir Oltean wrote:
>>> On 1/24/19 10:56 AM, Carlo Caione wrote:
> 
> No, I mean instead of doing "mdio rmmd 3.1" to do "mdio read 3.1"
> (basically not define a new command).

Ooooh, I think you can do that only on C45 PHYs, not on C22.
(tested on my board and it didn't work FWIW).

Cheers,

--
Carlo Caione

[U-Boot] [PATCH v4 3/3] cmd: mdio: Add new parameter to access MMD PHY registers

[Vladimir Oltean]

On 1/24/19 10:08 PM, Carlo Caione wrote:
> On 24/01/2019 20:04, Vladimir Oltean wrote:
>> On 1/24/19 10:01 PM, Carlo Caione wrote:
>>> On 24/01/2019 19:56, Vladimir Oltean wrote:
>>>> On 1/24/19 10:56 AM, Carlo Caione wrote:
>>
>> No, I mean instead of doing "mdio rmmd 3.1" to do "mdio read 3.1"
>> (basically not define a new command).
> 
> Ooooh, I think you can do that only on C45 PHYs, not on C22.
> (tested on my board and it didn't work FWIW).
> 
> Cheers,
> 
> --
> Carlo Caione
> 
> 

I still think I haven't successfully made my point.
If "mdio read 3.1" is a C45-only thing, "mdio read 1" is a C22-only 
thing, then why do you need a new command "mdio rmmd 3.1" to do C45 
emulation over C22? Is there any overlap I'm missing that mandates a new 
syntax to differentiate? Can the command not simply see whether the PHY 
is C22, and if it is and the MMD address is non-zero, just emulate it 
via 0xd and 0xe writes?


-Vladimir

[U-Boot] [PATCH v4 3/3] cmd: mdio: Add new parameter to access MMD PHY registers

[Carlo Caione]

On 24/01/2019 20:12, Vladimir Oltean wrote:
> 
> I still think I haven't successfully made my point.
> If "mdio read 3.1" is a C45-only thing, "mdio read 1" is a C22-only
> thing, then why do you need a new command "mdio rmmd 3.1" to do C45
> emulation over C22? Is there any overlap I'm missing that mandates a new
> syntax to differentiate? Can the command not simply see whether the PHY
> is C22, and if it is and the MMD address is non-zero, just emulate it
> via 0xd and 0xe writes?

Ok, I got your point now. I didn't give much thought on this TBH, good 
question.

Do we currently have a way in U-Boot to differentiate between C22 and 
C45 PHYs? The generic_10g PHY should be C45 but is that the only one 
currently supported?

--
Carlo Caione

[U-Boot] [PATCH v4 3/3] cmd: mdio: Add new parameter to access MMD PHY registers

[Vladimir Oltean]

On 1/24/19 10:19 PM, Carlo Caione wrote:
> On 24/01/2019 20:12, Vladimir Oltean wrote:
>>
>> I still think I haven't successfully made my point.
>> If "mdio read 3.1" is a C45-only thing, "mdio read 1" is a C22-only
>> thing, then why do you need a new command "mdio rmmd 3.1" to do C45
>> emulation over C22? Is there any overlap I'm missing that mandates a new
>> syntax to differentiate? Can the command not simply see whether the PHY
>> is C22, and if it is and the MMD address is non-zero, just emulate it
>> via 0xd and 0xe writes?
> 
> Ok, I got your point now. I didn't give much thought on this TBH, good
> question.
> 
> Do we currently have a way in U-Boot to differentiate between C22 and
> C45 PHYs? The generic_10g PHY should be C45 but is that the only one
> currently supported?
> 
> --
> Carlo Caione
> 

I can't completely answer that, TBH I don't even know who is supposed to 
make that distinction.
For Freescale parts that is a call for the MDIO bus driver to make, for 
good or bad (see drivers/net/fm/memac_phy.c where dev_addr is compared 
to MDIO_DEVAD_NONE).
And in your patch, phy_write_mmd is only a wrapper over bus->write in 
the end, with some more logic to handle C22 indirection.
So my question of unifying "mdio rmmd" with "mdio read" translates into: 
Does it make sense to also handle the check with MDIO_DEVAD_NONE in 
phy_write_mmd, instead of jumping straight ahead to perform indirection? 
The goal would then be to just call phy_write_mmd from cmd/mdio.c 
regardless of the target PHY's clause.
-Vladimir

[U-Boot] [PATCH v4 3/3] cmd: mdio: Add new parameter to access MMD PHY registers

[Carlo Caione]

On 24/01/2019 20:48, Vladimir Oltean wrote:
> On 1/24/19 10:19 PM, Carlo Caione wrote:
>> On 24/01/2019 20:12, Vladimir Oltean wrote:
>>>
> 
> I can't completely answer that, TBH I don't even know who is supposed to
> make that distinction.

In the kernel that distinction is made by the driver itself, hence my 
question. See [0].

> For Freescale parts that is a call for the MDIO bus driver to make, for
> good or bad (see drivers/net/fm/memac_phy.c where dev_addr is compared
> to MDIO_DEVAD_NONE).

> And in your patch, phy_write_mmd is only a wrapper over bus->write in
> the end, with some more logic to handle C22 indirection.
> So my question of unifying "mdio rmmd" with "mdio read" translates into:

> Does it make sense to also handle the check with MDIO_DEVAD_NONE in
> phy_write_mmd, instead of jumping straight ahead to perform indirection?

Honestly I'm not quite sure of all the possible implications here IMO 
the safest bet here is just to follow what's done by the kernel. Maybe 
Joe can step in about this.

In general we have 3 possible cases:

1) your driver is doing something non-standard when accessing the MMDs 
and we deal with that using the PHY driver hooks
2) your PHY is C22 and you have to use the indirect method
3) your PHY is C45 and you can use the direct register reading (mangling 
a bit the address apparently)

The kernel is dealing with all the cases, U-Boot is only dealing with 
C22 PHYs (cases 1 and 2) because AFAICT there isn't yet a generic way to 
detect if the PHY is C22 or C45.

I'm not sure if the indirect method works also for C45 PHYs.

> The goal would then be to just call phy_write_mmd from cmd/mdio.c
> regardless of the target PHY's clause.

Again I wrote that patch only assuming that we were going to deal with 
C22 PHYs. At this point I wonder if the C22 indirect method works also 
for C45 PHYs. If that's the case than the phy_write_mmd should already 
work regardless of the target PHY clause.

Cheers.

[0] 
https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy-core.c#L296

[U-Boot] [PATCH v4 3/3] cmd: mdio: Add new parameter to access MMD PHY registers

[Vladimir Oltean]

On 25.01.2019 12:12, Carlo Caione wrote:
> On 24/01/2019 20:48, Vladimir Oltean wrote:
>> On 1/24/19 10:19 PM, Carlo Caione wrote:
>>> On 24/01/2019 20:12, Vladimir Oltean wrote:
>>>>
>>
>> I can't completely answer that, TBH I don't even know who is supposed to
>> make that distinction.
> 
> In the kernel that distinction is made by the driver itself, hence my
> question. See [0].
> 
>> For Freescale parts that is a call for the MDIO bus driver to make, for
>> good or bad (see drivers/net/fm/memac_phy.c where dev_addr is compared
>> to MDIO_DEVAD_NONE).
> 
>> And in your patch, phy_write_mmd is only a wrapper over bus->write in
>> the end, with some more logic to handle C22 indirection.
>> So my question of unifying "mdio rmmd" with "mdio read" translates into:
> 
>> Does it make sense to also handle the check with MDIO_DEVAD_NONE in
>> phy_write_mmd, instead of jumping straight ahead to perform indirection?
> 
> Honestly I'm not quite sure of all the possible implications here IMO
> the safest bet here is just to follow what's done by the kernel. Maybe
> Joe can step in about this.
> 
> In general we have 3 possible cases:
> 
> 1) your driver is doing something non-standard when accessing the MMDs
> and we deal with that using the PHY driver hooks
> 2) your PHY is C22 and you have to use the indirect method
> 3) your PHY is C45 and you can use the direct register reading (mangling
> a bit the address apparently)
> 
> The kernel is dealing with all the cases, U-Boot is only dealing with
> C22 PHYs (cases 1 and 2) because AFAICT there isn't yet a generic way to
> detect if the PHY is C22 or C45.
> 
> I'm not sure if the indirect method works also for C45 PHYs.
> 
>> The goal would then be to just call phy_write_mmd from cmd/mdio.c
>> regardless of the target PHY's clause.
> 
> Again I wrote that patch only assuming that we were going to deal with
> C22 PHYs. At this point I wonder if the C22 indirect method works also
> for C45 PHYs. If that's the case than the phy_write_mmd should already
> work regardless of the target PHY clause.
> 
> Cheers.
> 
> [0]
> https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy-core.c#L296
> 

I'm not suggesting to use C22 indirection if the PHY already supports 
native C45 addressing. Even if that worked, it would be a pointless 
exercise in all but a few cases (like the MDIO controller does not 
support C22, but the PHY does support both C22 and C45).
I was just wondering out loud whether the introduction of the "mdio 
rmmd" command is justified or not. I now understand that using e.g. 
"mdio read 1.3" will confuse the command for clause 45 PHY's because it 
won't know whether it should access the PHY via native C45 or via 
indirect C22 (obviously it shouldn't do the latter). So in lack of a 
clear distinction mechanism, I now think that a new command truly is 
necessary for performing indirect C45 access on C22.
What I am still not convinced of, however, is whether those commands 
should be called "rmmd" and "wmmd". It is not immediately obvious from 
the command description that this is what they are for, and a user may 
attempt to use them for C45 PHY's as well, which will probably not yield 
the intended result.

-Vladimir

[U-Boot] [PATCH v4 3/3] cmd: mdio: Add new parameter to access MMD PHY registers

[Joe Hershberger]

On Fri, Jan 25, 2019 at 7:12 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> On 25.01.2019 12:12, Carlo Caione wrote:
> > On 24/01/2019 20:48, Vladimir Oltean wrote:
> >> On 1/24/19 10:19 PM, Carlo Caione wrote:
> >>> On 24/01/2019 20:12, Vladimir Oltean wrote:
> >>>>
> >>
> >> I can't completely answer that, TBH I don't even know who is supposed to
> >> make that distinction.
> >
> > In the kernel that distinction is made by the driver itself, hence my
> > question. See [0].
> >
> >> For Freescale parts that is a call for the MDIO bus driver to make, for
> >> good or bad (see drivers/net/fm/memac_phy.c where dev_addr is compared
> >> to MDIO_DEVAD_NONE).
> >
> >> And in your patch, phy_write_mmd is only a wrapper over bus->write in
> >> the end, with some more logic to handle C22 indirection.
> >> So my question of unifying "mdio rmmd" with "mdio read" translates into:
> >
> >> Does it make sense to also handle the check with MDIO_DEVAD_NONE in
> >> phy_write_mmd, instead of jumping straight ahead to perform indirection?
> >
> > Honestly I'm not quite sure of all the possible implications here IMO
> > the safest bet here is just to follow what's done by the kernel. Maybe
> > Joe can step in about this.
> >
> > In general we have 3 possible cases:
> >
> > 1) your driver is doing something non-standard when accessing the MMDs
> > and we deal with that using the PHY driver hooks
> > 2) your PHY is C22 and you have to use the indirect method
> > 3) your PHY is C45 and you can use the direct register reading (mangling
> > a bit the address apparently)
> >
> > The kernel is dealing with all the cases, U-Boot is only dealing with
> > C22 PHYs (cases 1 and 2) because AFAICT there isn't yet a generic way to
> > detect if the PHY is C22 or C45.
> >
> > I'm not sure if the indirect method works also for C45 PHYs.
> >
> >> The goal would then be to just call phy_write_mmd from cmd/mdio.c
> >> regardless of the target PHY's clause.
> >
> > Again I wrote that patch only assuming that we were going to deal with
> > C22 PHYs. At this point I wonder if the C22 indirect method works also
> > for C45 PHYs. If that's the case than the phy_write_mmd should already
> > work regardless of the target PHY clause.
> >
> > Cheers.
> >
> > [0]
> > https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy-core.c#L296
> >
>
> I'm not suggesting to use C22 indirection if the PHY already supports
> native C45 addressing. Even if that worked, it would be a pointless
> exercise in all but a few cases (like the MDIO controller does not
> support C22, but the PHY does support both C22 and C45).
> I was just wondering out loud whether the introduction of the "mdio
> rmmd" command is justified or not. I now understand that using e.g.
> "mdio read 1.3" will confuse the command for clause 45 PHY's because it
> won't know whether it should access the PHY via native C45 or via
> indirect C22 (obviously it shouldn't do the latter). So in lack of a
> clear distinction mechanism, I now think that a new command truly is
> necessary for performing indirect C45 access on C22.
> What I am still not convinced of, however, is whether those commands
> should be called "rmmd" and "wmmd". It is not immediately obvious from
> the command description that this is what they are for, and a user may
> attempt to use them for C45 PHY's as well, which will probably not yield
> the intended result.

I agree. The MMD in the register name is simply "MDIO Manageable
Devices"... i.e. the phys.

I think the commands should be "iread" and "iwrite" to denote the
indirect access in use.

Thanks,
-Joe

> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH v4 3/3] cmd: mdio: Add new parameter to access MMD PHY registers

[Vladimir Oltean]

On 2/5/19 1:28 AM, Joe Hershberger wrote:
> On Fri, Jan 25, 2019 at 7:12 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>>
>> On 25.01.2019 12:12, Carlo Caione wrote:
>>> On 24/01/2019 20:48, Vladimir Oltean wrote:
>>>> On 1/24/19 10:19 PM, Carlo Caione wrote:
>>>>> On 24/01/2019 20:12, Vladimir Oltean wrote:
>>>>>>
>>>>
>>>> I can't completely answer that, TBH I don't even know who is supposed to
>>>> make that distinction.
>>>
>>> In the kernel that distinction is made by the driver itself, hence my
>>> question. See [0].
>>>
>>>> For Freescale parts that is a call for the MDIO bus driver to make, for
>>>> good or bad (see drivers/net/fm/memac_phy.c where dev_addr is compared
>>>> to MDIO_DEVAD_NONE).
>>>
>>>> And in your patch, phy_write_mmd is only a wrapper over bus->write in
>>>> the end, with some more logic to handle C22 indirection.
>>>> So my question of unifying "mdio rmmd" with "mdio read" translates into:
>>>
>>>> Does it make sense to also handle the check with MDIO_DEVAD_NONE in
>>>> phy_write_mmd, instead of jumping straight ahead to perform indirection?
>>>
>>> Honestly I'm not quite sure of all the possible implications here IMO
>>> the safest bet here is just to follow what's done by the kernel. Maybe
>>> Joe can step in about this.
>>>
>>> In general we have 3 possible cases:
>>>
>>> 1) your driver is doing something non-standard when accessing the MMDs
>>> and we deal with that using the PHY driver hooks
>>> 2) your PHY is C22 and you have to use the indirect method
>>> 3) your PHY is C45 and you can use the direct register reading (mangling
>>> a bit the address apparently)
>>>
>>> The kernel is dealing with all the cases, U-Boot is only dealing with
>>> C22 PHYs (cases 1 and 2) because AFAICT there isn't yet a generic way to
>>> detect if the PHY is C22 or C45.
>>>
>>> I'm not sure if the indirect method works also for C45 PHYs.
>>>
>>>> The goal would then be to just call phy_write_mmd from cmd/mdio.c
>>>> regardless of the target PHY's clause.
>>>
>>> Again I wrote that patch only assuming that we were going to deal with
>>> C22 PHYs. At this point I wonder if the C22 indirect method works also
>>> for C45 PHYs. If that's the case than the phy_write_mmd should already
>>> work regardless of the target PHY clause.
>>>
>>> Cheers.
>>>
>>> [0]
>>> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Flatest%2Fsource%2Fdrivers%2Fnet%2Fphy%2Fphy-core.c%23L296&amp;data=02%7C01%7Cvladimir.oltean%40nxp.com%7C826fd741578446f6f36908d68af87b27%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636849197162094704&amp;sdata=r9AlGZbzGtLC2z7u2HgKKZt17Cl1OcHncjeY00xlVWE%3D&amp;reserved=0
>>>
>>
>> I'm not suggesting to use C22 indirection if the PHY already supports
>> native C45 addressing. Even if that worked, it would be a pointless
>> exercise in all but a few cases (like the MDIO controller does not
>> support C22, but the PHY does support both C22 and C45).
>> I was just wondering out loud whether the introduction of the "mdio
>> rmmd" command is justified or not. I now understand that using e.g.
>> "mdio read 1.3" will confuse the command for clause 45 PHY's because it
>> won't know whether it should access the PHY via native C45 or via
>> indirect C22 (obviously it shouldn't do the latter). So in lack of a
>> clear distinction mechanism, I now think that a new command truly is
>> necessary for performing indirect C45 access on C22.
>> What I am still not convinced of, however, is whether those commands
>> should be called "rmmd" and "wmmd". It is not immediately obvious from
>> the command description that this is what they are for, and a user may
>> attempt to use them for C45 PHY's as well, which will probably not yield
>> the intended result.
> 
> I agree. The MMD in the register name is simply "MDIO Manageable
> Devices"... i.e. the phys.
> 
> I think the commands should be "iread" and "iwrite" to denote the
> indirect access in use.
> 

Which brings me to my next point.
If we can't properly make the distinction between an indirect C22 MMD 
access and a proper C45 MMD access, and hence not keeping proper API 
compatibility with Linux kernel, aren't we better off going back to 
square 1 and using phy_read_mmd_indirect and phy_write_mmd_indirect?

-Vladimir

[U-Boot] [PATCH v4 3/3] cmd: mdio: Add new parameter to access MMD PHY registers

[Joe Hershberger]

On Mon, Feb 4, 2019 at 5:39 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> On 2/5/19 1:28 AM, Joe Hershberger wrote:
> > On Fri, Jan 25, 2019 at 7:12 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> >>
> >> On 25.01.2019 12:12, Carlo Caione wrote:
> >>> On 24/01/2019 20:48, Vladimir Oltean wrote:
> >>>> On 1/24/19 10:19 PM, Carlo Caione wrote:
> >>>>> On 24/01/2019 20:12, Vladimir Oltean wrote:
> >>>>>>
> >>>>
> >>>> I can't completely answer that, TBH I don't even know who is supposed to
> >>>> make that distinction.
> >>>
> >>> In the kernel that distinction is made by the driver itself, hence my
> >>> question. See [0].
> >>>
> >>>> For Freescale parts that is a call for the MDIO bus driver to make, for
> >>>> good or bad (see drivers/net/fm/memac_phy.c where dev_addr is compared
> >>>> to MDIO_DEVAD_NONE).
> >>>
> >>>> And in your patch, phy_write_mmd is only a wrapper over bus->write in
> >>>> the end, with some more logic to handle C22 indirection.
> >>>> So my question of unifying "mdio rmmd" with "mdio read" translates into:
> >>>
> >>>> Does it make sense to also handle the check with MDIO_DEVAD_NONE in
> >>>> phy_write_mmd, instead of jumping straight ahead to perform indirection?
> >>>
> >>> Honestly I'm not quite sure of all the possible implications here IMO
> >>> the safest bet here is just to follow what's done by the kernel. Maybe
> >>> Joe can step in about this.
> >>>
> >>> In general we have 3 possible cases:
> >>>
> >>> 1) your driver is doing something non-standard when accessing the MMDs
> >>> and we deal with that using the PHY driver hooks
> >>> 2) your PHY is C22 and you have to use the indirect method
> >>> 3) your PHY is C45 and you can use the direct register reading (mangling
> >>> a bit the address apparently)
> >>>
> >>> The kernel is dealing with all the cases, U-Boot is only dealing with
> >>> C22 PHYs (cases 1 and 2) because AFAICT there isn't yet a generic way to
> >>> detect if the PHY is C22 or C45.
> >>>
> >>> I'm not sure if the indirect method works also for C45 PHYs.
> >>>
> >>>> The goal would then be to just call phy_write_mmd from cmd/mdio.c
> >>>> regardless of the target PHY's clause.
> >>>
> >>> Again I wrote that patch only assuming that we were going to deal with
> >>> C22 PHYs. At this point I wonder if the C22 indirect method works also
> >>> for C45 PHYs. If that's the case than the phy_write_mmd should already
> >>> work regardless of the target PHY clause.
> >>>
> >>> Cheers.
> >>>
> >>> [0]
> >>> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Flatest%2Fsource%2Fdrivers%2Fnet%2Fphy%2Fphy-core.c%23L296&amp;data=02%7C01%7Cvladimir.oltean%40nxp.com%7C826fd741578446f6f36908d68af87b27%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636849197162094704&amp;sdata=r9AlGZbzGtLC2z7u2HgKKZt17Cl1OcHncjeY00xlVWE%3D&amp;reserved=0
> >>>
> >>
> >> I'm not suggesting to use C22 indirection if the PHY already supports
> >> native C45 addressing. Even if that worked, it would be a pointless
> >> exercise in all but a few cases (like the MDIO controller does not
> >> support C22, but the PHY does support both C22 and C45).
> >> I was just wondering out loud whether the introduction of the "mdio
> >> rmmd" command is justified or not. I now understand that using e.g.
> >> "mdio read 1.3" will confuse the command for clause 45 PHY's because it
> >> won't know whether it should access the PHY via native C45 or via
> >> indirect C22 (obviously it shouldn't do the latter). So in lack of a
> >> clear distinction mechanism, I now think that a new command truly is
> >> necessary for performing indirect C45 access on C22.
> >> What I am still not convinced of, however, is whether those commands
> >> should be called "rmmd" and "wmmd". It is not immediately obvious from
> >> the command description that this is what they are for, and a user may
> >> attempt to use them for C45 PHY's as well, which will probably not yield
> >> the intended result.
> >
> > I agree. The MMD in the register name is simply "MDIO Manageable
> > Devices"... i.e. the phys.
> >
> > I think the commands should be "iread" and "iwrite" to denote the
> > indirect access in use.
> >
>
> Which brings me to my next point.
> If we can't properly make the distinction between an indirect C22 MMD
> access and a proper C45 MMD access, and hence not keeping proper API
> compatibility with Linux kernel, aren't we better off going back to
> square 1 and using phy_read_mmd_indirect and phy_write_mmd_indirect?

I think we can and should make the new wrapper functions remain named
phy_*_mmd_indirect and the names of the override functions in the phy
driver ops should be *_mmd_indirect. The override is still for an
indirect access of c45 registers, just an apparently non-standard one.
It is this way in Linux as well.

-Joe


> -Vladimir
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH v4 3/3] cmd: mdio: Add new parameter to access MMD PHY registers

[Carlo Caione]

On 05/02/2019 00:15, Joe Hershberger wrote:
> On Mon, Feb 4, 2019 at 5:39 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:

/cut
>> Which brings me to my next point.
>> If we can't properly make the distinction between an indirect C22 MMD
>> access and a proper C45 MMD access, and hence not keeping proper API
>> compatibility with Linux kernel, aren't we better off going back to
>> square 1 and using phy_read_mmd_indirect and phy_write_mmd_indirect?
> 
> I think we can and should make the new wrapper functions remain named
> phy_*_mmd_indirect and the names of the override functions in the phy
> driver ops should be *_mmd_indirect. The override is still for an
> indirect access of c45 registers, just an apparently non-standard one.
> It is this way in Linux as well.

Alright then. I'll prepare a V5.

A couple on notes:

1. I'd prefer the parameters of the "mdio" command to be name "rimmd" 
and "wimmd" for "r/w indirect MMD" to keep the (twisted) logic of the 
mdio command code of differentiating the parameters according to 
argv[1][1] and r/w according to argv[1][0]

2. Since [0] needs a respin as well after the requested changes, I'm 
going to embedded that patch into this patchset.

Cheers

[0] https://lists.denx.de/pipermail/u-boot/2019-January/356019.html

--
Carlo Caione

[U-Boot] [PATCH v4 3/3] cmd: mdio: Add new parameter to access MMD PHY registers

[Joe Hershberger]

On Tue, Feb 5, 2019 at 9:20 AM Carlo Caione <ccaione@baylibre.com> wrote:
>
> On 05/02/2019 00:15, Joe Hershberger wrote:
> > On Mon, Feb 4, 2019 at 5:39 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> /cut
> >> Which brings me to my next point.
> >> If we can't properly make the distinction between an indirect C22 MMD
> >> access and a proper C45 MMD access, and hence not keeping proper API
> >> compatibility with Linux kernel, aren't we better off going back to
> >> square 1 and using phy_read_mmd_indirect and phy_write_mmd_indirect?
> >
> > I think we can and should make the new wrapper functions remain named
> > phy_*_mmd_indirect and the names of the override functions in the phy
> > driver ops should be *_mmd_indirect. The override is still for an
> > indirect access of c45 registers, just an apparently non-standard one.
> > It is this way in Linux as well.
>
> Alright then. I'll prepare a V5.
>
> A couple on notes:
>
> 1. I'd prefer the parameters of the "mdio" command to be name "rimmd"
> and "wimmd" for "r/w indirect MMD" to keep the (twisted) logic of the
> mdio command code of differentiating the parameters according to
> argv[1][1] and r/w according to argv[1][0]

Is there a reason you want to keep the mmd in there? It seems implied
by doing any access using the mdio command.

Maybe wi and ri or windirect and rindirect or wind and rind?

> 2. Since [0] needs a respin as well after the requested changes, I'm
> going to embedded that patch into this patchset.

Sounds good.

>
> Cheers
>
> [0] https://lists.denx.de/pipermail/u-boot/2019-January/356019.html
>
> --
> Carlo Caione
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH v4 3/3] cmd: mdio: Add new parameter to access MMD PHY registers

[Vladimir Oltean]

On 2/6/19 12:10 AM, Joe Hershberger wrote:
> On Tue, Feb 5, 2019 at 9:20 AM Carlo Caione <ccaione@baylibre.com> wrote:
>>
>> On 05/02/2019 00:15, Joe Hershberger wrote:
>>> On Mon, Feb 4, 2019 at 5:39 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>>
>> /cut
>>>> Which brings me to my next point.
>>>> If we can't properly make the distinction between an indirect C22 MMD
>>>> access and a proper C45 MMD access, and hence not keeping proper API
>>>> compatibility with Linux kernel, aren't we better off going back to
>>>> square 1 and using phy_read_mmd_indirect and phy_write_mmd_indirect?
>>>
>>> I think we can and should make the new wrapper functions remain named
>>> phy_*_mmd_indirect and the names of the override functions in the phy
>>> driver ops should be *_mmd_indirect. The override is still for an
>>> indirect access of c45 registers, just an apparently non-standard one.
>>> It is this way in Linux as well.

I guess it is just me who is still unclear on this?
Since Russell King's patch "3b85d8d net: phy: remove the indirect MMD 
read/write methods", the Linux API is no longer like that (the 
phy_driver pointers phy_read_mmd and phy_read_mmd_indirect were merged 
into one).
Just want to make sure that everybody is on the same page and we agreed 
on API compatibility with pre-3b85d8d Linux.

>>
>> Alright then. I'll prepare a V5.
>>
>> A couple on notes:
>>
>> 1. I'd prefer the parameters of the "mdio" command to be name "rimmd"
>> and "wimmd" for "r/w indirect MMD" to keep the (twisted) logic of the
>> mdio command code of differentiating the parameters according to
>> argv[1][1] and r/w according to argv[1][0]
> 
> Is there a reason you want to keep the mmd in there? It seems implied
> by doing any access using the mdio command.
> 
> Maybe wi and ri or windirect and rindirect or wind and rind?
> 

What about exposing the indirect read as
"mii read   <addr> [<indirect_devad>.]<reg>"?
It should be clear to most people (and if it isn't, it should be 
clarified) that the legacy "mii" command is clause 22 only, therefore 
the "<mmd>.<reg>" syntactic sugar must logically mean that indirect 
access is what's going on when applied to "mii". The implementation can 
freely call phy_read_mmd_indirect if it parses such syntax.

Just my 2c. Either way, exposing an explicit command for indirect access 
means that U-boot commits long-term to not trying to implicitly know 
about, and populate, phydev->is_c45.

-Vladimir

[U-Boot] [PATCH v4 3/3] cmd: mdio: Add new parameter to access MMD PHY registers

[Joe Hershberger]

On Tue, Feb 5, 2019 at 7:37 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> On 2/6/19 12:10 AM, Joe Hershberger wrote:
> > On Tue, Feb 5, 2019 at 9:20 AM Carlo Caione <ccaione@baylibre.com> wrote:
> >>
> >> On 05/02/2019 00:15, Joe Hershberger wrote:
> >>> On Mon, Feb 4, 2019 at 5:39 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> >>
> >> /cut
> >>>> Which brings me to my next point.
> >>>> If we can't properly make the distinction between an indirect C22 MMD
> >>>> access and a proper C45 MMD access, and hence not keeping proper API
> >>>> compatibility with Linux kernel, aren't we better off going back to
> >>>> square 1 and using phy_read_mmd_indirect and phy_write_mmd_indirect?
> >>>
> >>> I think we can and should make the new wrapper functions remain named
> >>> phy_*_mmd_indirect and the names of the override functions in the phy
> >>> driver ops should be *_mmd_indirect. The override is still for an
> >>> indirect access of c45 registers, just an apparently non-standard one.
> >>> It is this way in Linux as well.
>
> I guess it is just me who is still unclear on this?
> Since Russell King's patch "3b85d8d net: phy: remove the indirect MMD
> read/write methods", the Linux API is no longer like that (the
> phy_driver pointers phy_read_mmd and phy_read_mmd_indirect were merged
> into one).
> Just want to make sure that everybody is on the same page and we agreed
> on API compatibility with pre-3b85d8d Linux.

Argh. I was looking at the patch that Carlo referenced and did not
look to see that it further changed.

But looking at 3b85d8d I don't see how the concerns that you and Carlo
discussed about determining what to do if c45 is also supported. Do
you know how Linux handles this or should I do some research?

> >>
> >> Alright then. I'll prepare a V5.
> >>
> >> A couple on notes:
> >>
> >> 1. I'd prefer the parameters of the "mdio" command to be name "rimmd"
> >> and "wimmd" for "r/w indirect MMD" to keep the (twisted) logic of the
> >> mdio command code of differentiating the parameters according to
> >> argv[1][1] and r/w according to argv[1][0]
> >
> > Is there a reason you want to keep the mmd in there? It seems implied
> > by doing any access using the mdio command.
> >
> > Maybe wi and ri or windirect and rindirect or wind and rind?
> >
>
> What about exposing the indirect read as
> "mii read   <addr> [<indirect_devad>.]<reg>"?
>
> It should be clear to most people (and if it isn't, it should be
> clarified) that the legacy "mii" command is clause 22 only, therefore
> the "<mmd>.<reg>" syntactic sugar must logically mean that indirect
> access is what's going on when applied to "mii". The implementation can
> freely call phy_read_mmd_indirect if it parses such syntax.

While it is clear, I would prefer to not encourage further use of the
mii command. I would rather add the ability to explicitly specify the
clause in the mdio command.

Perhaps the default can be to attempt to auto select, but if it is
ambiguous, require the explicit specification. It could follow a
similar approach to the "md" command.  We can add the ability to add
".22" and ".45" to the mdio command to explicitly select.

> Just my 2c. Either way, exposing an explicit command for indirect access
> means that U-boot commits long-term to not trying to implicitly know
> about, and populate, phydev->is_c45.

While using either mdio rindirect or mii / mdio.22 read they are
effectively explicit commands to select "indirect", so I'm not sure
what point you are making here.

-Joe

>
> -Vladimir
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH v4 3/3] cmd: mdio: Add new parameter to access MMD PHY registers

[Carlo Caione]

On 06/02/2019 03:31, Joe Hershberger wrote:

/cut
> Perhaps the default can be to attempt to auto select, but if it is
> ambiguous, require the explicit specification. It could follow a
> similar approach to the "md" command.  We can add the ability to add
> ".22" and ".45" to the mdio command to explicitly select.

What about we go back to have a generic phy_{r|w}_mmd() and (in this order):

1) If the PHY driver is defining a generic {r|w}_mmd() hook we use that.
2) We do direct C45 access if (phy_driver->features & PHY_10G_FEATURES)
3) We do direct C22 access if (devad == MDIO_DEVAD_NONE)
4) We do indirect C22 access to C45 in all the other cases

Se we can have one single "mdio" command for all the cases.

--
Carlo Caione

[U-Boot] [PATCH v4 3/3] cmd: mdio: Add new parameter to access MMD PHY registers

[Vladimir Oltean]

On 2/6/19 11:35 AM, Carlo Caione wrote:
> On 06/02/2019 03:31, Joe Hershberger wrote:
> 
> /cut
>> Perhaps the default can be to attempt to auto select, but if it is
>> ambiguous, require the explicit specification. It could follow a
>> similar approach to the "md" command.  We can add the ability to add
>> ".22" and ".45" to the mdio command to explicitly select.
> 
> What about we go back to have a generic phy_{r|w}_mmd() and (in this order):
> 
> 1) If the PHY driver is defining a generic {r|w}_mmd() hook we use that.
> 2) We do direct C45 access if (phy_driver->features & PHY_10G_FEATURES)
> 3) We do direct C22 access if (devad == MDIO_DEVAD_NONE)
> 4) We do indirect C22 access to C45 in all the other cases
> 
> Se we can have one single "mdio" command for all the cases.
> 

This approximation sounds like it could actually work. And especially if 
you could make sure that even if the approximation is wrong, it is 
possible to correct it by implementing a driver-level override without 
duplicating too much code, then I think there's no practical issue.
I think I do have a few boards with various C45 PHYs that I can 
volunteer to test your v5 patchset on, and report back if I see any 
glaring problems.

> --
> Carlo Caione
> 

On 2/6/19 5:31 AM, Joe Hershberger wrote:
 > On Tue, Feb 5, 2019 at 7:37 PM Vladimir Oltean wrote:
 >> Just my 2c. Either way, exposing an explicit command for indirect access
 >> means that U-boot commits long-term to not trying to implicitly know
 >> about, and populate, phydev->is_c45.
 >
 > While using either mdio rindirect or mii / mdio.22 read they are
 > effectively explicit commands to select "indirect", so I'm not sure
 > what point you are making here.
 >
 > -Joe
 >

I mean that if U-boot were ever able to imply PHY compatibility with 
clause 45 (aka the API can be made to only expose phy_read_mmd and 
phy_write_mmd), then there shouldn't be a command exposed to the user 
for requesting an MMD access that is explicitly direct or indirect.
What should the PHY driver do if it knows it's C22 but an explicit C45 
MMD access is requested, or the other way around?

Carlo's proposal sounds good and also solves the problem of "mdio read" 
semantics without over-engineering things.

-Vladimir