[PATCH net-next v2 0/2] net: dsa: bcm_sf2: Ensure MDIO diversion is used

[PATCH net-next v2 0/2] net: dsa: bcm_sf2: Ensure MDIO diversion is used

[Florian Fainelli]

Changes in v2:

- export of_update_property() to permit building bcm_sf2 as a module
- provided a better explanation of the problem being solved after
  explaining it to Andrew during the v1 review

Florian Fainelli (2):
  of: Export of_remove_property() to modules
  net: dsa: bcm_sf2: Ensure that MDIO diversion is used

 drivers/net/dsa/bcm_sf2.c | 31 +++++++++++++++++++++++++++++--
 drivers/of/base.c         |  1 +
 include/linux/of.h        |  5 +++++
 3 files changed, 35 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH net-next v2 1/2] of: Export of_remove_property() to modules

[Florian Fainelli]

We will need to remove some OF properties in drivers/net/dsa/bcm_sf2.c
with a subsequent commit. Export of_remove_property() to modules so we
can keep bcm_sf2 modular and provide an empty stub for when CONFIG_OF is
disabled to maintain the ability to compile test.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/of/base.c  | 1 +
 include/linux/of.h | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index ea44fea99813..161a23631472 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1869,6 +1869,7 @@ int of_remove_property(struct device_node *np, struct property *prop)
 
 	return rc;
 }
+EXPORT_SYMBOL_GPL(of_remove_property);
 
 int __of_update_property(struct device_node *np, struct property *newprop,
 		struct property **oldpropp)
diff --git a/include/linux/of.h b/include/linux/of.h
index 5cf7ae0465d1..481ec0467285 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -929,6 +929,11 @@ static inline int of_machine_is_compatible(const char *compat)
 	return 0;
 }
 
+static inline int of_remove_property(struct device_node *np, struct property *prop)
+{
+	return 0;
+}
+
 static inline bool of_console_check(const struct device_node *dn, const char *name, int index)
 {
 	return false;
-- 
2.25.1

[PATCH net-next v2 2/2] net: dsa: bcm_sf2: Ensure that MDIO diversion is used

[Florian Fainelli]

Registering our slave MDIO bus outside of the OF infrastructure is
necessary in order to avoid creating double references of the same
Device Tree nodes, however it is not sufficient to guarantee that the
MDIO bus diversion is used because of_phy_connect() will still resolve
to a valid PHY phandle and it will connect to the PHY using its parent
MDIO bus which is still the SF2 master MDIO bus. The reason for that is
because BCM7445 systems were already shipped with a Device Tree blob
looking like this (irrelevant parts omitted for simplicity):

	ports {
		#address-cells = <1>;
		#size-cells = <0>;

		port@1 {
			phy-mode = "rgmii-txid";
			phy-handle = <&phy0>;
                        reg = <1>;
			label = "rgmii_1";
		};
	...

	mdio@403c0 {
		...

		phy0: ethernet-phy@0 {
			broken-turn-around;
			device_type = "ethernet-phy";
			max-speed = <0x3e8>;
			reg = <0>;
			compatible = "brcm,bcm53125", "ethernet-phy-ieee802.3-c22";
		};
	};

There is a hardware issue with chip revisions (Dx) that lead to the
development of the following commits:

461cd1b03e32 ("net: dsa: bcm_sf2: Register our slave MDIO bus")
536fab5bf582 ("net: dsa: bcm_sf2: Do not register slave MDIO bus with OF")
b8c6cd1d316f ("net: dsa: bcm_sf2: do not use indirect reads and writes for 7445E0")

There should have been an internal MDIO bus node created for the chip
revision (Dx) that suffers from this problem, but it did not happen back
then.

Had that happen, that we should have correctly parented phy@0 (bcm53125
below) as child node of the internal MDIO bus, but the production Device
Tree blob that was shipped with the firmware targeted the fixed version
of the chip, despite both the affected and corrected chips being shipped
into production.

The problem is that of_phy_connect() for port@1 will happily resolve the
'phy-handle' from the mdio@403c0 node, which bypasses the diversion
completely. This results in this double programming that the diversion
refers to and aims to avoid. In order to force of_phy_connect() to fail,
and have DSA call to dsa_slave_phy_connect(), we must deactivate
ethernet-phy@0 from mdio@403c0, and the best way to do that is by
removing the phandle property completely.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/bcm_sf2.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index 1c7fbb6f0447..8e215c148487 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -489,9 +489,11 @@ static void bcm_sf2_identify_ports(struct bcm_sf2_priv *priv,
 static int bcm_sf2_mdio_register(struct dsa_switch *ds)
 {
 	struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
-	struct device_node *dn;
+	struct device_node *dn, *child;
+	struct phy_device *phydev;
+	struct property *prop;
 	static int index;
-	int err;
+	int err, reg;
 
 	/* Find our integrated MDIO bus node */
 	dn = of_find_compatible_node(NULL, NULL, "brcm,unimac-mdio");
@@ -534,6 +536,31 @@ static int bcm_sf2_mdio_register(struct dsa_switch *ds)
 	priv->slave_mii_bus->parent = ds->dev->parent;
 	priv->slave_mii_bus->phy_mask = ~priv->indir_phy_mask;
 
+	/* We need to make sure that of_phy_connect() will not work by
+	 * removing the 'phandle' and 'linux,phandle' properties and
+	 * unregister the existing PHY device that was already registered.
+	 */
+	for_each_available_child_of_node(dn, child) {
+		if (of_property_read_u32(child, "reg", &reg) ||
+		    reg >= PHY_MAX_ADDR)
+			continue;
+
+		if (!(priv->indir_phy_mask & BIT(reg)))
+			continue;
+
+		prop = of_find_property(child, "phandle", NULL);
+		if (prop)
+			of_remove_property(child, prop);
+
+		prop = of_find_property(child, "linux,phandle", NULL);
+		if (prop)
+			of_remove_property(child, prop);
+
+		phydev = of_phy_find_device(child);
+		if (phydev)
+			phy_device_remove(phydev);
+	}
+
 	err = mdiobus_register(priv->slave_mii_bus);
 	if (err && dn)
 		of_node_put(dn);
-- 
2.25.1

Re: [PATCH net-next v2 1/2] of: Export of_remove_property() to modules

[Rob Herring]

On Fri, Sep 4, 2020 at 3:37 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> We will need to remove some OF properties in drivers/net/dsa/bcm_sf2.c
> with a subsequent commit. Export of_remove_property() to modules so we
> can keep bcm_sf2 modular and provide an empty stub for when CONFIG_OF is
> disabled to maintain the ability to compile test.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/of/base.c  | 1 +
>  include/linux/of.h | 5 +++++
>  2 files changed, 6 insertions(+)

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH net-next v2 2/2] net: dsa: bcm_sf2: Ensure that MDIO diversion is used

[Andrew Lunn]

On Fri, Sep 04, 2020 at 02:37:30PM -0700, Florian Fainelli wrote:
> Registering our slave MDIO bus outside of the OF infrastructure is
> necessary in order to avoid creating double references of the same
> Device Tree nodes, however it is not sufficient to guarantee that the
> MDIO bus diversion is used because of_phy_connect() will still resolve
> to a valid PHY phandle and it will connect to the PHY using its parent
> MDIO bus which is still the SF2 master MDIO bus. The reason for that is
> because BCM7445 systems were already shipped with a Device Tree blob
> looking like this (irrelevant parts omitted for simplicity):

Hi Florian

Thanks for the extended commit message.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v2 0/2] net: dsa: bcm_sf2: Ensure MDIO diversion is used

[Jakub Kicinski]

On Fri,  4 Sep 2020 14:37:28 -0700 Florian Fainelli wrote:
> Changes in v2:
> 
> - export of_update_property() to permit building bcm_sf2 as a module
> - provided a better explanation of the problem being solved after
>   explaining it to Andrew during the v1 review

Applied, thanks everyone!