[PATCH v1 0/6] Provide support for mv88e6020 Marvell switch

[PATCH v1 0/6] Provide support for mv88e6020 Marvell switch

[Lukasz Majewski]

This patch set provides support for mv88e6020 switch. It is a dual chip
device, with direct access to port register on SMI bus. However, PHYs
are accessed indirectly.

I've rebased this series (unmodified) on v2023.07-rcX
(SHA1: cb4437e530ec1ff3deae85754010344afab8bcc5)  and added reviewed
by tags.


Lukasz Majewski (6):
  net: mv88e61xx: Add support for checking addressing mode
  net: mv88e61xx: Configure PHY ports to also pass packets between them
  net: mv88e61xx: Clear temporary structs before using them in
    get_phy_id()
  net: mv88e61xx: Directly access the switch chip
  net: mv88e61xx: Set proper offset when R0_LED/ADDR4 is set on
    bootstrap
  net: mv88e61xx: Reset switch PHYs when bootstrapped to !NO_CPU

 drivers/net/phy/mv88e61xx.c | 158 ++++++++++++++++++++++++++++++++----
 1 file changed, 142 insertions(+), 16 deletions(-)

-- 
2.30.2

[PATCH v1 1/6] net: mv88e61xx: Add support for checking addressing mode

[Lukasz Majewski]

Some Marvell switch devices are dual chip ones, like mv88e6020, which
use direct MDIO addressing to access its ports' registers. Such approach
allows connecting two such devices in a single MDIO bus with simple
addressing scheme.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>
---

 drivers/net/phy/mv88e61xx.c | 42 +++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/net/phy/mv88e61xx.c b/drivers/net/phy/mv88e61xx.c
index 85778106eddc..31f9b57456d6 100644
--- a/drivers/net/phy/mv88e61xx.c
+++ b/drivers/net/phy/mv88e61xx.c
@@ -194,6 +194,7 @@ struct mv88e61xx_phy_priv {
 	u8 phy_ctrl1_en_det_shift; /* 'EDet' bit field offset */
 	u8 phy_ctrl1_en_det_width; /* Width of 'EDet' bit field */
 	u8 phy_ctrl1_en_det_ctrl;  /* 'EDet' control value */
+	u8 direct_access;          /* Access switch device directly */
 };
 
 static inline int smi_cmd(int cmd, int addr, int reg)
@@ -920,6 +921,40 @@ static int mv88e61xx_priv_reg_offs_pre_init(struct phy_device *phydev)
 	return -ENODEV;
 }
 
+static int mv88e61xx_check_addressing(struct phy_device *phydev)
+{
+	if (!CONFIG_IS_ENABLED(OF_CONTROL))
+		return 0;
+
+	/*
+	 * Some devices - like mv88e6020 are dual chip - i.e. two
+	 * such devices can be directly accessed via SMI bus.
+	 * The addressing depends on R0_LED/ADDR4 pin value duing
+	 * bootstrap.
+	 *
+	 * This means that there is no need for indirect access.
+	 */
+	struct mv88e61xx_phy_priv *priv = phydev->priv;
+
+	/*
+	 * As this function is called very early and hence the phydev
+	 * is not yet initialized we use aliast and DTS to asses if
+	 * device shall be directly accessed or not.
+	 */
+	ofnode sw0;
+	int ret;
+
+	sw0 = ofnode_get_aliases_node("switch0");
+	if (!ofnode_valid(sw0))
+		return -ENODEV;
+
+	ret = ofnode_device_is_compatible(sw0, "marvell,mv88e6020");
+	if (ret)
+		priv->direct_access = 1;
+
+	return 0;
+}
+
 static int mv88e61xx_probe(struct phy_device *phydev)
 {
 	struct mii_dev *smi_wrapper;
@@ -974,6 +1009,8 @@ static int mv88e61xx_probe(struct phy_device *phydev)
 
 	phydev->priv = priv;
 
+	mv88e61xx_check_addressing(phydev);
+
 	res = mv88e61xx_priv_reg_offs_pre_init(phydev);
 	if (res < 0)
 		return res;
@@ -1180,6 +1217,11 @@ 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_check_addressing(&temp_phy);
+	/* For direct access the phy address equals to smi_addr */
+	if (temp_priv.direct_access)
+		temp_phy.addr = smi_addr;
+
 	/*
 	 * get_phy_id() can be called by framework before mv88e61xx driver
 	 * probing, in this case the global register offsets are not
-- 
2.30.2

[PATCH v1 2/6] net: mv88e61xx: Configure PHY ports to also pass packets between them

[Lukasz Majewski]

After this change PHY ports are able to pass packets between them (and
also to CPU port).

The Kconfig variable - CONFIG_MV88E61XX_PHY_PORTS - is used to get the
PHY ports of the switch and generate proper mask.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>
---

 drivers/net/phy/mv88e61xx.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/mv88e61xx.c b/drivers/net/phy/mv88e61xx.c
index 31f9b57456d6..4aee83551beb 100644
--- a/drivers/net/phy/mv88e61xx.c
+++ b/drivers/net/phy/mv88e61xx.c
@@ -865,14 +865,19 @@ static int mv88e61xx_phy_setup(struct phy_device *phydev, u8 phy)
 
 static int mv88e61xx_phy_config_port(struct phy_device *phydev, u8 phy)
 {
+	struct mv88e61xx_phy_priv *priv = phydev->priv;
+	u16 port_mask;
 	int val;
 
 	val = mv88e61xx_port_enable(phydev, phy);
 	if (val < 0)
 		return val;
 
-	val = mv88e61xx_port_set_vlan(phydev, phy,
-			1 << CONFIG_MV88E61XX_CPU_PORT);
+	port_mask = PORT_MASK(priv->port_count) & CONFIG_MV88E61XX_PHY_PORTS;
+	port_mask &= ~(1 << phy);
+	port_mask |= (1 << CONFIG_MV88E61XX_CPU_PORT);
+
+	val = mv88e61xx_port_set_vlan(phydev, phy, port_mask);
 	if (val < 0)
 		return val;
 
-- 
2.30.2

[PATCH v1 3/6] net: mv88e61xx: Clear temporary structs before using them in get_phy_id()

[Lukasz Majewski]

Those automatically created structures can have random value.
However, mv88e61xx driver assumes that those are zeroed.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>
---

 drivers/net/phy/mv88e61xx.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/phy/mv88e61xx.c b/drivers/net/phy/mv88e61xx.c
index 4aee83551beb..c19c3dfa8b6d 100644
--- a/drivers/net/phy/mv88e61xx.c
+++ b/drivers/net/phy/mv88e61xx.c
@@ -1213,6 +1213,10 @@ int get_phy_id(struct mii_dev *bus, int smi_addr, int devad, u32 *phy_id)
 	struct mii_dev temp_mii;
 	int val;
 
+	memset(&temp_phy, 0, sizeof(temp_phy));
+	memset(&temp_priv, 0, sizeof(temp_priv));
+	memset(&temp_mii, 0, sizeof(temp_mii));
+
 	/*
 	 * Buid temporary data structures that the chip reading code needs to
 	 * read the ID
-- 
2.30.2

[PATCH v1 4/6] net: mv88e61xx: Directly access the switch chip

[Lukasz Majewski]

The mv88e6020 is accessed in a direct way (i.e. with direct read and
write to mdio bus). The only necessary indirection is required when
accessing its PHY registers.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>
---

 drivers/net/phy/mv88e61xx.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/mv88e61xx.c b/drivers/net/phy/mv88e61xx.c
index c19c3dfa8b6d..c9917953f3d7 100644
--- a/drivers/net/phy/mv88e61xx.c
+++ b/drivers/net/phy/mv88e61xx.c
@@ -261,8 +261,11 @@ static int mv88e61xx_reg_read(struct phy_device *phydev, int dev, int reg)
 	int smi_addr = priv->smi_addr;
 	int res;
 
-	/* In single-chip mode, the device can be addressed directly */
-	if (smi_addr == 0)
+	/*
+	 * In single-chip or dual-chip (like mv88e6020) mode, the device can
+	 * be addressed directly.
+	 */
+	if (smi_addr == 0 || priv->direct_access)
 		return mdio_bus->read(mdio_bus, dev, MDIO_DEVAD_NONE, reg);
 
 	/* Wait for the bus to become free */
@@ -298,11 +301,13 @@ static int mv88e61xx_reg_write(struct phy_device *phydev, int dev, int reg,
 	int smi_addr = priv->smi_addr;
 	int res;
 
-	/* In single-chip mode, the device can be addressed directly */
-	if (smi_addr == 0) {
+	/*
+	 * In single-chip or dual-chip (like mv88e6020) mode, the device can
+	 * be addressed directly.
+	 */
+	if (smi_addr == 0 || priv->direct_access)
 		return mdio_bus->write(mdio_bus, dev, MDIO_DEVAD_NONE, reg,
 				val);
-	}
 
 	/* Wait for the bus to become free */
 	res = mv88e61xx_smi_wait(mdio_bus, smi_addr);
-- 
2.30.2

[PATCH v1 5/6] net: mv88e61xx: Set proper offset when R0_LED/ADDR4 is set on bootstrap

[Lukasz Majewski]

The mv88e61xx driver need to be adjusted to handle situation when
switch MDIO addresses are switched by offset (0x10 in this case).

Signed-off-by: Lukasz Majewski <lukma@denx.de>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>
---

 drivers/net/phy/mv88e61xx.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/net/phy/mv88e61xx.c b/drivers/net/phy/mv88e61xx.c
index c9917953f3d7..69a87bead469 100644
--- a/drivers/net/phy/mv88e61xx.c
+++ b/drivers/net/phy/mv88e61xx.c
@@ -45,7 +45,6 @@
 #define PORT_MASK(port_count)		((1 << (port_count)) - 1)
 
 /* Device addresses */
-#define DEVADDR_PHY(p)			(p)
 #define DEVADDR_SERDES			0x0F
 
 /* SMI indirection registers for multichip addressing mode */
@@ -406,7 +405,7 @@ static int mv88e61xx_phy_write_indirect(struct mii_dev *smi_wrapper, int dev,
 /* Wrapper function to make calls to phy_read_indirect simpler */
 static int mv88e61xx_phy_read(struct phy_device *phydev, int phy, int reg)
 {
-	return mv88e61xx_phy_read_indirect(phydev->bus, DEVADDR_PHY(phy),
+	return mv88e61xx_phy_read_indirect(phydev->bus, phydev->addr,
 					   MDIO_DEVAD_NONE, reg);
 }
 
@@ -414,7 +413,7 @@ static int mv88e61xx_phy_read(struct phy_device *phydev, int phy, int reg)
 static int mv88e61xx_phy_write(struct phy_device *phydev, int phy,
 		int reg, u16 val)
 {
-	return mv88e61xx_phy_write_indirect(phydev->bus, DEVADDR_PHY(phy),
+	return mv88e61xx_phy_write_indirect(phydev->bus, phydev->addr,
 					    MDIO_DEVAD_NONE, reg, val);
 }
 
@@ -918,12 +917,21 @@ static int mv88e61xx_priv_reg_offs_pre_init(struct phy_device *phydev)
 	/*
 	 * Now try via port registers with device address 0x08
 	 * (88E6020 and compatible switches).
+	 *
+	 * When R0_LED/ADDR4 is set during bootstrap, one needs
+	 * to add 0x10 offset to switch addresses.
+	 *
+	 * The phydev->addr is set according to device tree address
+	 * of MDIO accessible device:
+	 *
+	 * When on board RO_LED/ADDR4 = 1 -> 0x10
+	 *                              0 -> 0x0
 	 */
-	priv->port_reg_base = 0x08;
+	priv->port_reg_base = 0x08 + phydev->addr;
 	priv->id = mv88e61xx_get_switch_id(phydev);
 	if (priv->id != 0xfff0) {
-		priv->global1 = 0x0F;
-		priv->global2 = 0x07;
+		priv->global1 = 0x0F + phydev->addr;
+		priv->global2 = 0x07 + phydev->addr;
 		return 0;
 	}
 
@@ -1082,7 +1090,10 @@ static int mv88e61xx_phy_config(struct phy_device *phydev)
 
 	for (i = 0; i < priv->port_count; i++) {
 		if ((1 << i) & CONFIG_MV88E61XX_PHY_PORTS) {
-			phydev->addr = i;
+			if (phydev->addr)
+				phydev->addr += i;
+			else
+				phydev->addr = i;
 
 			res = mv88e61xx_phy_enable(phydev, i);
 			if (res < 0) {
-- 
2.30.2

[PATCH v1 6/6] net: mv88e61xx: Reset switch PHYs when bootstrapped to !NO_CPU

[Lukasz Majewski]

Some devices, when configured in bootstrap to 'no cpu' mode require PHY
manual reset to get them operational and responding to reading their ID
registers.

Without this step - the PHYLIB probing will fail.

In more details - the bootstrap configuration from switch must be read.
The value of CONFIG Data1 (0x71) of Scratch and Misc register is read
to check if 'no_cpu' and 'addr4' bits were set.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

---

 drivers/net/phy/mv88e61xx.c | 63 +++++++++++++++++++++++++++++++++++--
 1 file changed, 61 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/mv88e61xx.c b/drivers/net/phy/mv88e61xx.c
index 69a87bead469..cf8f5e833e82 100644
--- a/drivers/net/phy/mv88e61xx.c
+++ b/drivers/net/phy/mv88e61xx.c
@@ -194,6 +194,17 @@ struct mv88e61xx_phy_priv {
 	u8 phy_ctrl1_en_det_width; /* Width of 'EDet' bit field */
 	u8 phy_ctrl1_en_det_ctrl;  /* 'EDet' control value */
 	u8 direct_access;          /* Access switch device directly */
+	/*
+	 * Bootstrap configuration:
+	 *
+	 * If addr4 = 1 device is accessible from 0x10 address on MDIO bus.
+	 */
+	u8 addr4;
+	/*
+	 * If no_cpu = 1 switch is automatically setup, otherwise PHY reset is
+	 * required from CPU for normal operation.
+	 */
+	u8 no_cpu;
 };
 
 static inline int smi_cmd(int cmd, int addr, int reg)
@@ -1218,6 +1229,33 @@ U_BOOT_PHY_DRIVER(mv88e6071) = {
 	.shutdown = &genphy_shutdown,
 };
 
+static int mv88e61xx_read_bootstrap(struct phy_device *phydev)
+{
+	struct mv88e61xx_phy_priv *priv = phydev->priv;
+	struct mii_dev *mdio_bus = priv->mdio_bus;
+	int val;
+
+	/* mv88e6020 - ID = 0x0200 (REG 3 on non PHY port) */
+	if (priv->id == PORT_SWITCH_ID_6020) {
+		/* Prepare to read scratch and misc register */
+		mdio_bus->write(mdio_bus, priv->global2, 0,
+				0x1a /*MV_SCRATCH_MISC*/,
+				(0x71 /*MV_CONFIG_DATA1*/ << 8));
+
+		val = mdio_bus->read(mdio_bus, priv->global2, 0,
+				     0x1a /*MV_SCRATCH_MISC*/);
+
+		if (val & (1 << 0))
+			priv->no_cpu = 1;
+		if (val & (1 << 4))
+			priv->addr4 = 1;
+		debug("mv88e6020: no_cpu=%d addr4=%d\n", priv->no_cpu,
+		      priv->addr4);
+	}
+
+	return 0;
+}
+
 /*
  * Overload weak get_phy_id definition since we need non-standard functions
  * to read PHY registers
@@ -1257,13 +1295,34 @@ int get_phy_id(struct mii_dev *bus, int smi_addr, int devad, u32 *phy_id)
 	if (val < 0)
 		return val;
 
-	val = mv88e61xx_phy_read_indirect(&temp_mii, 0, devad, MII_PHYSID1);
+	mv88e61xx_read_bootstrap(&temp_phy);
+
+	/*
+	 * When switch is configured to work with CPU (i.e. NO_CPU == 0), PHYs
+	 * require reset (to at least single one) to have its registers
+	 * accessible.
+	 */
+	if (!temp_priv.no_cpu && temp_priv.id == PORT_SWITCH_ID_6020) {
+		/* Reset PHY */
+		val = mv88e61xx_phy_read_indirect(&temp_mii, temp_phy.addr,
+						  devad, MII_BMCR);
+		if (val & BMCR_PDOWN)
+			val &= ~BMCR_PDOWN;
+
+		mv88e61xx_phy_write_indirect(&temp_mii, temp_phy.addr, devad,
+					     MII_BMCR, val);
+	}
+
+	/* Read PHY_ID */
+	val = mv88e61xx_phy_read_indirect(&temp_mii, temp_phy.addr, devad,
+					  MII_PHYSID1);
 	if (val < 0)
 		return -EIO;
 
 	*phy_id = val << 16;
 
-	val = mv88e61xx_phy_read_indirect(&temp_mii, 0, devad, MII_PHYSID2);
+	val = mv88e61xx_phy_read_indirect(&temp_mii, temp_phy.addr, devad,
+					  MII_PHYSID2);
 	if (val < 0)
 		return -EIO;
 
-- 
2.30.2

Re: [PATCH v1 2/6] net: mv88e61xx: Configure PHY ports to also pass packets between them

[Marek Vasut]

On 6/1/23 12:00, Lukasz Majewski wrote:
> After this change PHY ports are able to pass packets between them (and
> also to CPU port).
> 
> The Kconfig variable - CONFIG_MV88E61XX_PHY_PORTS - is used to get the
> PHY ports of the switch and generate proper mask.
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

Was there a V0 before ? Or where did these RB tags come from ?

> ---
> 
>   drivers/net/phy/mv88e61xx.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/mv88e61xx.c b/drivers/net/phy/mv88e61xx.c
> index 31f9b57456d6..4aee83551beb 100644
> --- a/drivers/net/phy/mv88e61xx.c
> +++ b/drivers/net/phy/mv88e61xx.c
> @@ -865,14 +865,19 @@ static int mv88e61xx_phy_setup(struct phy_device *phydev, u8 phy)
>   
>   static int mv88e61xx_phy_config_port(struct phy_device *phydev, u8 phy)
>   {
> +	struct mv88e61xx_phy_priv *priv = phydev->priv;
> +	u16 port_mask;
>   	int val;
>   
>   	val = mv88e61xx_port_enable(phydev, phy);
>   	if (val < 0)
>   		return val;
>   
> -	val = mv88e61xx_port_set_vlan(phydev, phy,
> -			1 << CONFIG_MV88E61XX_CPU_PORT);
> +	port_mask = PORT_MASK(priv->port_count) & CONFIG_MV88E61XX_PHY_PORTS;
> +	port_mask &= ~(1 << phy);
> +	port_mask |= (1 << CONFIG_MV88E61XX_CPU_PORT);

BIT() ?

Re: [PATCH v1 3/6] net: mv88e61xx: Clear temporary structs before using them in get_phy_id()

[Marek Vasut]

On 6/1/23 12:00, Lukasz Majewski wrote:
> Those automatically created structures can have random value.
> However, mv88e61xx driver assumes that those are zeroed.
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> Reviewed-by: Ramon Fried <rfried.dev@gmail.com>
> ---
> 
>   drivers/net/phy/mv88e61xx.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/phy/mv88e61xx.c b/drivers/net/phy/mv88e61xx.c
> index 4aee83551beb..c19c3dfa8b6d 100644
> --- a/drivers/net/phy/mv88e61xx.c
> +++ b/drivers/net/phy/mv88e61xx.c
> @@ -1213,6 +1213,10 @@ int get_phy_id(struct mii_dev *bus, int smi_addr, int devad, u32 *phy_id)
>   	struct mii_dev temp_mii;
>   	int val;
>   
> +	memset(&temp_phy, 0, sizeof(temp_phy));
> +	memset(&temp_priv, 0, sizeof(temp_priv));
> +	memset(&temp_mii, 0, sizeof(temp_mii));

struct mii_dev temp_mii = { 0 }; etc

should work all the same, no need for memset.

Re: [PATCH v1 6/6] net: mv88e61xx: Reset switch PHYs when bootstrapped to !NO_CPU

[Marek Vasut]

On 6/1/23 12:00, Lukasz Majewski wrote:
> Some devices, when configured in bootstrap to 'no cpu' mode require PHY
> manual reset to get them operational and responding to reading their ID
> registers.
> 
> Without this step - the PHYLIB probing will fail.
> 
> In more details - the bootstrap configuration from switch must be read.
> The value of CONFIG Data1 (0x71) of Scratch and Misc register is read
> to check if 'no_cpu' and 'addr4' bits were set.
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> Reviewed-by: Ramon Fried <rfried.dev@gmail.com>
> 
> ---
> 
>   drivers/net/phy/mv88e61xx.c | 63 +++++++++++++++++++++++++++++++++++--
>   1 file changed, 61 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/mv88e61xx.c b/drivers/net/phy/mv88e61xx.c
> index 69a87bead469..cf8f5e833e82 100644
> --- a/drivers/net/phy/mv88e61xx.c
> +++ b/drivers/net/phy/mv88e61xx.c
> @@ -194,6 +194,17 @@ struct mv88e61xx_phy_priv {
>   	u8 phy_ctrl1_en_det_width; /* Width of 'EDet' bit field */
>   	u8 phy_ctrl1_en_det_ctrl;  /* 'EDet' control value */
>   	u8 direct_access;          /* Access switch device directly */
> +	/*
> +	 * Bootstrap configuration:
> +	 *
> +	 * If addr4 = 1 device is accessible from 0x10 address on MDIO bus.
> +	 */
> +	u8 addr4;
> +	/*
> +	 * If no_cpu = 1 switch is automatically setup, otherwise PHY reset is
> +	 * required from CPU for normal operation.
> +	 */
> +	u8 no_cpu;
>   };
>   
>   static inline int smi_cmd(int cmd, int addr, int reg)
> @@ -1218,6 +1229,33 @@ U_BOOT_PHY_DRIVER(mv88e6071) = {
>   	.shutdown = &genphy_shutdown,
>   };
>   
> +static int mv88e61xx_read_bootstrap(struct phy_device *phydev)
> +{
> +	struct mv88e61xx_phy_priv *priv = phydev->priv;
> +	struct mii_dev *mdio_bus = priv->mdio_bus;
> +	int val;
> +
> +	/* mv88e6020 - ID = 0x0200 (REG 3 on non PHY port) */
> +	if (priv->id == PORT_SWITCH_ID_6020) {
> +		/* Prepare to read scratch and misc register */
> +		mdio_bus->write(mdio_bus, priv->global2, 0,
> +				0x1a /*MV_SCRATCH_MISC*/,
> +				(0x71 /*MV_CONFIG_DATA1*/ << 8));

Introduce macros for these magic values.

> +		val = mdio_bus->read(mdio_bus, priv->global2, 0,
> +				     0x1a /*MV_SCRATCH_MISC*/);
> +
> +		if (val & (1 << 0))
> +			priv->no_cpu = 1;
> +		if (val & (1 << 4))

Macros, and also BIT()

[..]

Re: [PATCH v1 2/6] net: mv88e61xx: Configure PHY ports to also pass packets between them

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 1861 bytes --]

Hi Marek,

> On 6/1/23 12:00, Lukasz Majewski wrote:
> > After this change PHY ports are able to pass packets between them
> > (and also to CPU port).
> > 
> > The Kconfig variable - CONFIG_MV88E61XX_PHY_PORTS - is used to get
> > the PHY ports of the switch and generate proper mask.
> > 
> > Signed-off-by: Lukasz Majewski <lukma@denx.de>
> > Reviewed-by: Ramon Fried <rfried.dev@gmail.com>  
> 
> Was there a V0 before ? Or where did these RB tags come from ?
> 

There was v1 in 2021 - The patman is not allowing "v1 RESEND" tag
adding.

> > ---
> > 
> >   drivers/net/phy/mv88e61xx.c | 9 +++++++--
> >   1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/phy/mv88e61xx.c
> > b/drivers/net/phy/mv88e61xx.c index 31f9b57456d6..4aee83551beb
> > 100644 --- a/drivers/net/phy/mv88e61xx.c
> > +++ b/drivers/net/phy/mv88e61xx.c
> > @@ -865,14 +865,19 @@ static int mv88e61xx_phy_setup(struct
> > phy_device *phydev, u8 phy) 
> >   static int mv88e61xx_phy_config_port(struct phy_device *phydev,
> > u8 phy) {
> > +	struct mv88e61xx_phy_priv *priv = phydev->priv;
> > +	u16 port_mask;
> >   	int val;
> >   
> >   	val = mv88e61xx_port_enable(phydev, phy);
> >   	if (val < 0)
> >   		return val;
> >   
> > -	val = mv88e61xx_port_set_vlan(phydev, phy,
> > -			1 << CONFIG_MV88E61XX_CPU_PORT);
> > +	port_mask = PORT_MASK(priv->port_count) &
> > CONFIG_MV88E61XX_PHY_PORTS;
> > +	port_mask &= ~(1 << phy);
> > +	port_mask |= (1 << CONFIG_MV88E61XX_CPU_PORT);  
> 
> BIT() ?

I've kept the "style" from the rest of the file.


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 2/6] net: mv88e61xx: Configure PHY ports to also pass packets between them

[Marek Vasut]

On 6/1/23 13:02, Lukasz Majewski wrote:
> Hi Marek,
> 
>> On 6/1/23 12:00, Lukasz Majewski wrote:
>>> After this change PHY ports are able to pass packets between them
>>> (and also to CPU port).
>>>
>>> The Kconfig variable - CONFIG_MV88E61XX_PHY_PORTS - is used to get
>>> the PHY ports of the switch and generate proper mask.
>>>
>>> Signed-off-by: Lukasz Majewski <lukma@denx.de>
>>> Reviewed-by: Ramon Fried <rfried.dev@gmail.com>
>>
>> Was there a V0 before ? Or where did these RB tags come from ?
>>
> 
> There was v1 in 2021 - The patman is not allowing "v1 RESEND" tag
> adding.

I think after two years, it would be good to drop the RB tags and do 
another round of reviews.

Re: [PATCH v1 6/6] net: mv88e61xx: Reset switch PHYs when bootstrapped to !NO_CPU

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 3023 bytes --]

Hi Marek,

> On 6/1/23 12:00, Lukasz Majewski wrote:
> > Some devices, when configured in bootstrap to 'no cpu' mode require
> > PHY manual reset to get them operational and responding to reading
> > their ID registers.
> > 
> > Without this step - the PHYLIB probing will fail.
> > 
> > In more details - the bootstrap configuration from switch must be
> > read. The value of CONFIG Data1 (0x71) of Scratch and Misc register
> > is read to check if 'no_cpu' and 'addr4' bits were set.
> > 
> > Signed-off-by: Lukasz Majewski <lukma@denx.de>
> > Reviewed-by: Ramon Fried <rfried.dev@gmail.com>
> > 
> > ---
> > 
> >   drivers/net/phy/mv88e61xx.c | 63
> > +++++++++++++++++++++++++++++++++++-- 1 file changed, 61
> > insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/phy/mv88e61xx.c
> > b/drivers/net/phy/mv88e61xx.c index 69a87bead469..cf8f5e833e82
> > 100644 --- a/drivers/net/phy/mv88e61xx.c
> > +++ b/drivers/net/phy/mv88e61xx.c
> > @@ -194,6 +194,17 @@ struct mv88e61xx_phy_priv {
> >   	u8 phy_ctrl1_en_det_width; /* Width of 'EDet' bit field */
> >   	u8 phy_ctrl1_en_det_ctrl;  /* 'EDet' control value */
> >   	u8 direct_access;          /* Access switch device
> > directly */
> > +	/*
> > +	 * Bootstrap configuration:
> > +	 *
> > +	 * If addr4 = 1 device is accessible from 0x10 address on
> > MDIO bus.
> > +	 */
> > +	u8 addr4;
> > +	/*
> > +	 * If no_cpu = 1 switch is automatically setup, otherwise
> > PHY reset is
> > +	 * required from CPU for normal operation.
> > +	 */
> > +	u8 no_cpu;
> >   };
> >   
> >   static inline int smi_cmd(int cmd, int addr, int reg)
> > @@ -1218,6 +1229,33 @@ U_BOOT_PHY_DRIVER(mv88e6071) = {
> >   	.shutdown = &genphy_shutdown,
> >   };
> >   
> > +static int mv88e61xx_read_bootstrap(struct phy_device *phydev)
> > +{
> > +	struct mv88e61xx_phy_priv *priv = phydev->priv;
> > +	struct mii_dev *mdio_bus = priv->mdio_bus;
> > +	int val;
> > +
> > +	/* mv88e6020 - ID = 0x0200 (REG 3 on non PHY port) */
> > +	if (priv->id == PORT_SWITCH_ID_6020) {
> > +		/* Prepare to read scratch and misc register */
> > +		mdio_bus->write(mdio_bus, priv->global2, 0,
> > +				0x1a /*MV_SCRATCH_MISC*/,
> > +				(0x71 /*MV_CONFIG_DATA1*/ << 8));  
> 
> Introduce macros for these magic values.

Frankly speaking, this is more readable than macros as it is in sync
with vendor's documentation.

In other words - it is easier to find this information in documentation
when presented like above.

> 
> > +		val = mdio_bus->read(mdio_bus, priv->global2, 0,
> > +				     0x1a /*MV_SCRATCH_MISC*/);
> > +
> > +		if (val & (1 << 0))
> > +			priv->no_cpu = 1;
> > +		if (val & (1 << 4))  
> 
> Macros, and also BIT()
> 
> [..]

Ok.


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 6/6] net: mv88e61xx: Reset switch PHYs when bootstrapped to !NO_CPU

[Marek Vasut]

On 6/1/23 14:13, Lukasz Majewski wrote:
> Hi Marek,
> 
>> On 6/1/23 12:00, Lukasz Majewski wrote:
>>> Some devices, when configured in bootstrap to 'no cpu' mode require
>>> PHY manual reset to get them operational and responding to reading
>>> their ID registers.
>>>
>>> Without this step - the PHYLIB probing will fail.
>>>
>>> In more details - the bootstrap configuration from switch must be
>>> read. The value of CONFIG Data1 (0x71) of Scratch and Misc register
>>> is read to check if 'no_cpu' and 'addr4' bits were set.
>>>
>>> Signed-off-by: Lukasz Majewski <lukma@denx.de>
>>> Reviewed-by: Ramon Fried <rfried.dev@gmail.com>
>>>
>>> ---
>>>
>>>    drivers/net/phy/mv88e61xx.c | 63
>>> +++++++++++++++++++++++++++++++++++-- 1 file changed, 61
>>> insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/phy/mv88e61xx.c
>>> b/drivers/net/phy/mv88e61xx.c index 69a87bead469..cf8f5e833e82
>>> 100644 --- a/drivers/net/phy/mv88e61xx.c
>>> +++ b/drivers/net/phy/mv88e61xx.c
>>> @@ -194,6 +194,17 @@ struct mv88e61xx_phy_priv {
>>>    	u8 phy_ctrl1_en_det_width; /* Width of 'EDet' bit field */
>>>    	u8 phy_ctrl1_en_det_ctrl;  /* 'EDet' control value */
>>>    	u8 direct_access;          /* Access switch device
>>> directly */
>>> +	/*
>>> +	 * Bootstrap configuration:
>>> +	 *
>>> +	 * If addr4 = 1 device is accessible from 0x10 address on
>>> MDIO bus.
>>> +	 */
>>> +	u8 addr4;
>>> +	/*
>>> +	 * If no_cpu = 1 switch is automatically setup, otherwise
>>> PHY reset is
>>> +	 * required from CPU for normal operation.
>>> +	 */
>>> +	u8 no_cpu;
>>>    };
>>>    
>>>    static inline int smi_cmd(int cmd, int addr, int reg)
>>> @@ -1218,6 +1229,33 @@ U_BOOT_PHY_DRIVER(mv88e6071) = {
>>>    	.shutdown = &genphy_shutdown,
>>>    };
>>>    
>>> +static int mv88e61xx_read_bootstrap(struct phy_device *phydev)
>>> +{
>>> +	struct mv88e61xx_phy_priv *priv = phydev->priv;
>>> +	struct mii_dev *mdio_bus = priv->mdio_bus;
>>> +	int val;
>>> +
>>> +	/* mv88e6020 - ID = 0x0200 (REG 3 on non PHY port) */
>>> +	if (priv->id == PORT_SWITCH_ID_6020) {
>>> +		/* Prepare to read scratch and misc register */
>>> +		mdio_bus->write(mdio_bus, priv->global2, 0,
>>> +				0x1a /*MV_SCRATCH_MISC*/,
>>> +				(0x71 /*MV_CONFIG_DATA1*/ << 8));
>>
>> Introduce macros for these magic values.
> 
> Frankly speaking, this is more readable than macros as it is in sync
> with vendor's documentation.

Sorry, I am not buying this argument. Random constant 0x123 is NOT more 
readable than the standard that the rest of the U-Boot code base uses 
which is:

#define MV_SCRATCH_MISC 0x1a

and then call(MV_SCRATCH_MISC...) .

Hard-coding random magic register offsets into the code is just sloppy 
and leads to duplication. The mv88e61xx driver does not do it either, so 
this patch is violating its coding style, see the top 200 or so lines of 
the driver. Do not do it.

To make it clear, until this is fixed, NAK .

> In other words - it is easier to find this information in documentation
> when presented like above.

No

Re: [PATCH v1 2/6] net: mv88e61xx: Configure PHY ports to also pass packets between them

[Vladimir Oltean]

Hi Lukasz,

On Thu, Jun 01, 2023 at 01:44:30PM +0200, Marek Vasut wrote:
> I think after two years, it would be good to drop the RB tags and do another
> round of reviews.

To expand on Marek's point.

In those past 2 years, Tim Harvey has put in a considerable amount of
effort to add another driver for mv88e6xxx that uses DM_DSA. I believe
the current "PHY" driver for the same hardware should be considered
obsolete until all platforms are converted to DM_DSA, then it can be
deleted. So, no new features for it.

Then, there's also the question of the sanity of the proposed change itself.

I believe that we need to be humble enough to recognize that the U-Boot
network stack is not competent enough to handle the switching capabilities
of a switch, not even enough for it to be safe. It doesn't handle STP
(Spanning Tree Protocol), for one thing. So it will never be capable of
detecting switching loops, such as to block one of its ports in order to
not kill the network.

In principle, I would say: as long as there is no plan to handle STP,
there should be no plan to allow autonomous packet forwarding from U-Boot.
The U-Boot network stack is there so that you can TFTP a kernel and boot it,
which is also the only use case behind DM_DSA.

But you may say: I'm never going to allow packet forwarding from U-Boot
in a network with loops!

Okay, but your patch suggests otherwise. Which ports allow forwarding is
a compile-time option, which... is by definition contrary to any runtime
network topology determinations.

Maybe enabling forwarding between switch ports through a CLI command
that communicates with DM_DSA would be tolerable - assuming that users
are smart enough to not use it in a network with STP. But again, I'm not
really sure what's the use case.

Re: [PATCH v1 2/6] net: mv88e61xx: Configure PHY ports to also pass packets between them

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 3018 bytes --]

Hi Vladimir,

> Hi Lukasz,
> 
> On Thu, Jun 01, 2023 at 01:44:30PM +0200, Marek Vasut wrote:
> > I think after two years, it would be good to drop the RB tags and
> > do another round of reviews.  
> 
> To expand on Marek's point.
> 
> In those past 2 years, Tim Harvey has put in a considerable amount of
> effort to add another driver for mv88e6xxx that uses DM_DSA. I believe
> the current "PHY" driver for the same hardware should be considered
> obsolete until all platforms are converted to DM_DSA, then it can be
> deleted. So, no new features for it.

Ok. Thanks for the clarification.

> 
> Then, there's also the question of the sanity of the proposed change
> itself.
> 
> I believe that we need to be humble enough to recognize that the
> U-Boot network stack is not competent enough to handle the switching
> capabilities of a switch, not even enough for it to be safe. It
> doesn't handle STP (Spanning Tree Protocol), for one thing. So it
> will never be capable of detecting switching loops, such as to block
> one of its ports in order to not kill the network.
> 
> In principle, I would say: as long as there is no plan to handle STP,
> there should be no plan to allow autonomous packet forwarding from
> U-Boot. The U-Boot network stack is there so that you can TFTP a
> kernel and boot it, which is also the only use case behind DM_DSA.
> 
> But you may say: I'm never going to allow packet forwarding from
> U-Boot in a network with loops!
> 
> Okay, but your patch suggests otherwise. Which ports allow forwarding
> is a compile-time option, which... is by definition contrary to any
> runtime network topology determinations.
> 
> Maybe enabling forwarding between switch ports through a CLI command
> that communicates with DM_DSA would be tolerable - assuming that users
> are smart enough to not use it in a network with STP.

No - adding extra command line option is not planned.

> But again, I'm
> not really sure what's the use case.

This is a simple use case - the board on which I do work has two LAN
ports, so the device is "attached" to the single LAN cable.

When booting the device (even in u-boot) - packet's shall be forwarded
between LAN ports to keep the ETH connection.

However, I don't want to add STP for the u-boot network stack.

I've also looked to Tim's patch set [1] (also in mainline), and I do
believe that some extra features for this driver can be added; like
ADDR4 (so the address is 'shifted' to 0x10) and !NO_CPU bootstraps
(so ports need to be reset at power up).

Those features are common for all mv88e6xxx devices.


Links:

[1] -
https://patchwork.ozlabs.org/project/uboot/cover/20221130174251.82087-1-tharvey@gateworks.com/


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v1 4/6] net: mv88e61xx: Directly access the switch chip

[Ramon Fried]

On Wed, Mar 17, 2021 at 4:14 PM Lukasz Majewski <lukma@denx.de> wrote:
>
> The mv88e6020 is accessed in a direct way (i.e. with direct read and
> write to mdio bus). The only necessary indirection is required when
> accessing its PHY registers.
>
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> ---
>
>  drivers/net/phy/mv88e61xx.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/phy/mv88e61xx.c b/drivers/net/phy/mv88e61xx.c
> index d8116530700d..3d846b89fd12 100644
> --- a/drivers/net/phy/mv88e61xx.c
> +++ b/drivers/net/phy/mv88e61xx.c
> @@ -269,8 +269,11 @@ static int mv88e61xx_reg_read(struct phy_device *phydev, int dev, int reg)
>         int smi_addr = priv->smi_addr;
>         int res;
>
> -       /* In single-chip mode, the device can be addressed directly */
> -       if (smi_addr == 0)
> +       /*
> +        * In single-chip or dual-chip (like mv88e6020) mode, the device can
> +        * be addressed directly.
> +        */
> +       if (smi_addr == 0 || priv->direct_access)
>                 return mdio_bus->read(mdio_bus, dev, MDIO_DEVAD_NONE, reg);
>
>         /* Wait for the bus to become free */
> @@ -306,11 +309,13 @@ static int mv88e61xx_reg_write(struct phy_device *phydev, int dev, int reg,
>         int smi_addr = priv->smi_addr;
>         int res;
>
> -       /* In single-chip mode, the device can be addressed directly */
> -       if (smi_addr == 0) {
> +       /*
> +        * In single-chip or dual-chip (like mv88e6020) mode, the device can
> +        * be addressed directly.
> +        */
> +       if (smi_addr == 0 || priv->direct_access)
>                 return mdio_bus->write(mdio_bus, dev, MDIO_DEVAD_NONE, reg,
>                                 val);
> -       }
>
>         /* Wait for the bus to become free */
>         res = mv88e61xx_smi_wait(mdio_bus, smi_addr);
> --
> 2.20.1
>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

[PATCH v1 4/6] net: mv88e61xx: Directly access the switch chip

[Lukasz Majewski]

The mv88e6020 is accessed in a direct way (i.e. with direct read and
write to mdio bus). The only necessary indirection is required when
accessing its PHY registers.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---

 drivers/net/phy/mv88e61xx.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/mv88e61xx.c b/drivers/net/phy/mv88e61xx.c
index d8116530700d..3d846b89fd12 100644
--- a/drivers/net/phy/mv88e61xx.c
+++ b/drivers/net/phy/mv88e61xx.c
@@ -269,8 +269,11 @@ static int mv88e61xx_reg_read(struct phy_device *phydev, int dev, int reg)
 	int smi_addr = priv->smi_addr;
 	int res;
 
-	/* In single-chip mode, the device can be addressed directly */
-	if (smi_addr == 0)
+	/*
+	 * In single-chip or dual-chip (like mv88e6020) mode, the device can
+	 * be addressed directly.
+	 */
+	if (smi_addr == 0 || priv->direct_access)
 		return mdio_bus->read(mdio_bus, dev, MDIO_DEVAD_NONE, reg);
 
 	/* Wait for the bus to become free */
@@ -306,11 +309,13 @@ static int mv88e61xx_reg_write(struct phy_device *phydev, int dev, int reg,
 	int smi_addr = priv->smi_addr;
 	int res;
 
-	/* In single-chip mode, the device can be addressed directly */
-	if (smi_addr == 0) {
+	/*
+	 * In single-chip or dual-chip (like mv88e6020) mode, the device can
+	 * be addressed directly.
+	 */
+	if (smi_addr == 0 || priv->direct_access)
 		return mdio_bus->write(mdio_bus, dev, MDIO_DEVAD_NONE, reg,
 				val);
-	}
 
 	/* Wait for the bus to become free */
 	res = mv88e61xx_smi_wait(mdio_bus, smi_addr);
-- 
2.20.1