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

[PATCH net-next] 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.

Ensure that of_phy_connect() does not suceed by removing any phandle
reference for the PHY we need to divert. This forces the DSA code to use
the DSA slave_mii_bus that we register and ensures the MDIO diversion is
being used.

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] net: dsa: bcm_sf2: Ensure that MDIO diversion is used

[kernel test robot]

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

Hi Florian,

I love your patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Florian-Fainelli/net-dsa-bcm_sf2-Ensure-that-MDIO-diversion-is-used/20200903-050536
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git dc1a9bf2c8169d9f607502162af1858a73a18cb8
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sh 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "of_remove_property" [drivers/net/dsa/bcm-sf2.ko] undefined!
ERROR: modpost: "__delay" [drivers/net/mdio/mdio-cavium.ko] undefined!

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 52749 bytes --]

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

[Andrew Lunn]

On Wed, Sep 02, 2020 at 02:03:27PM -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.
> 
> Ensure that of_phy_connect() does not suceed by removing any phandle
> reference for the PHY we need to divert. This forces the DSA code to use
> the DSA slave_mii_bus that we register and ensures the MDIO diversion is
> being used.

Hi Florian

Sorry, i don't get this explanation. Can you point me towards a device
tree i can look at to maybe understand what is going on.

     Andrew

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

[Florian Fainelli]

On 9/2/2020 6:13 PM, Andrew Lunn wrote:
> On Wed, Sep 02, 2020 at 02:03:27PM -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.
>>
>> Ensure that of_phy_connect() does not suceed by removing any phandle
>> reference for the PHY we need to divert. This forces the DSA code to use
>> the DSA slave_mii_bus that we register and ensures the MDIO diversion is
>> being used.
> 
> Hi Florian
> 
> Sorry, i don't get this explanation. Can you point me towards a device
> tree i can look at to maybe understand what is going on.
The firmware provides the Device Tree but here is the relevant section 
for you pasted below. The problematic device is a particular revision of 
the silicon (D0) which got later fixed (E0) however the Device Tree was 
created after the fixed platform, not the problematic one. Both 
revisions of the silicon are in production.

There should have been an internal MDIO bus created for that chip 
revision such that we could have correctly parented phy@0 (bcm53125 
below) as child node of the internal MDIO bus, but you have to realize 
that this was done back in 2014 when DSA was barely revived as an active 
subsystem. The BCM53125 node should have have been converted to an 
actual switch node at some point, I use a mdio_boardinfo overlay 
downstream to support the switch as a proper b53/DSA switch, anyway.

The problem is that of_phy_connect() for port@1 will 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. 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.

Hope this clarifies the mess :)


		switch_top@f0b00000 {
			#address-cells = <0x01>;
			#size-cells = <0x01>;
			compatible = "brcm,bcm7445-switch-top-v2.0\0simple-bus";
			ranges = <0x00 0xf0b00000 0x40804>;

			ethernet_switch@0 {
				#address-cells = <0x02>;
				#size-cells = <0x00>;
				brcm,num-acb-queues = <0x40>;
				brcm,num-gphy = <0x01>;
				brcm,num-rgmii-ports = <0x02>;
				compatible = "brcm,bcm7445-switch-v4.0\0brcm,bcm53012";
				dsa,ethernet = <0x16>;
				dsa,mii-bus = <0x17>;
				resets = <0x18 0x1a>;
				reset-names = "switch";
				reg = <0x00 0x40000 0x40000 0x110 0x40340 0x30 0x40380 0x30 0x40400 
0x34 0x40600 0x208>;
				reg-names = "core\0reg\0intrl2_0\0intrl2_1\0fcb\0acb";
				interrupts = <0x00 0x18 0x04 0x00 0x19 0x04>;
				interrupt-names = "switch_0\0switch_1";
				brcm,fcb-pause-override;
				brcm,acb-packets-inflight;
				clocks = <0x0a 0x6d 0x0a 0x76>;
				clock-names = "sw_switch\0sw_switch_mdiv";

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

					port@0 {
						phy-mode = "internal";
						phy-handle = <0x29>;
						linux,phandle = <0x2a>;
						phandle = <0x2a>;
						reg = <0x00>;
						label = "gphy";
					};

					port@1 {
						phy-mode = "rgmii-txid";
						phy-handle = <0x2c>;
						linux,phandle = <0x2d>;
						phandle = <0x2d>;
						reg = <0x01>;
						label = "rgmii_1";
					};

					port@2 {
						phy-mode = "rgmii-txid";
						fixed-link = <0x02 0x01 0x3e8 0x00 0x00>;
						linux,phandle = <0x2f>;
						phandle = <0x2f>;
						reg = <0x02>;
						label = "rgmii_2";
					};

					port@7 {
						phy-mode = "moca";
						fixed-link = <0x07 0x01 0x3e8 0x00 0x00>;
						linux,phandle = <0x31>;
						phandle = <0x31>;
						reg = <0x07>;
						label = "moca";
					};

					port@8 {
						linux,phandle = <0x33>;
						phandle = <0x33>;
						reg = <0x08>;
						label = "cpu";
						ethernet = <0x16>;
					};
				};
			};

			mdio@403c0 {
				reg = <0x403c0 0x08 0x40300 0x18>;
				#address-cells = <0x01>;
				#size-cells = <0x00>;
				compatible = "brcm,bcm7445-mdio-v4.0\0brcm,unimac-mdio";
				reg-names = "mdio\0mdio_indir_rw";
				clocks = <0x0a 0x6d>;
				clock-names = "sw_switch";
				linux,phandle = <0x17>;
				phandle = <0x17>;

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

				ethernet-phy@5 {
					linux,phandle = <0x29>;
					phandle = <0x29>;
					clock-names = "sw_gphy";
					clocks = <0x0a 0x63>;
					device_type = "ethernet-phy";
					max-speed = <0x3e8>;
					reg = <0x05>;
					compatible = "brcm,28nm-gphy\0ethernet-phy-ieee802.3-c22";
				};
			};
		};
-- 
Florian

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

[Andrew Lunn]

> The firmware provides the Device Tree but here is the relevant section for
> you pasted below. The problematic device is a particular revision of the
> silicon (D0) which got later fixed (E0) however the Device Tree was created
> after the fixed platform, not the problematic one. Both revisions of the
> silicon are in production.
> 
> There should have been an internal MDIO bus created for that chip revision
> such that we could have correctly parented phy@0 (bcm53125 below) as child
> node of the internal MDIO bus, but you have to realize that this was done
> back in 2014 when DSA was barely revived as an active subsystem. The
> BCM53125 node should have have been converted to an actual switch node at
> some point, I use a mdio_boardinfo overlay downstream to support the switch
> as a proper b53/DSA switch, anyway.

I was expecting something like that. I think this patch needs a
comment in the code explaining it is a workaround for a DT blob which
cannot be changed. Maybe also make it conditional on the board
compatible string?

Thanks
	Andrew

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

[Florian Fainelli]

On 9/3/2020 3:03 PM, Andrew Lunn wrote:
>> The firmware provides the Device Tree but here is the relevant section for
>> you pasted below. The problematic device is a particular revision of the
>> silicon (D0) which got later fixed (E0) however the Device Tree was created
>> after the fixed platform, not the problematic one. Both revisions of the
>> silicon are in production.
>>
>> There should have been an internal MDIO bus created for that chip revision
>> such that we could have correctly parented phy@0 (bcm53125 below) as child
>> node of the internal MDIO bus, but you have to realize that this was done
>> back in 2014 when DSA was barely revived as an active subsystem. The
>> BCM53125 node should have have been converted to an actual switch node at
>> some point, I use a mdio_boardinfo overlay downstream to support the switch
>> as a proper b53/DSA switch, anyway.
> 
> I was expecting something like that. I think this patch needs a
> comment in the code explaining it is a workaround for a DT blob which
> cannot be changed. Maybe also make it conditional on the board
> compatible string?

It is already targeted at the Broadcom pseudo PHY address (30) which is 
the one that needs diversion, I will update the patch description 
accordingly though.
-- 
Florian

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

[Andrew Lunn]

On Thu, Sep 03, 2020 at 09:00:13PM -0700, Florian Fainelli wrote:
> 
> 
> On 9/3/2020 3:03 PM, Andrew Lunn wrote:
> > > The firmware provides the Device Tree but here is the relevant section for
> > > you pasted below. The problematic device is a particular revision of the
> > > silicon (D0) which got later fixed (E0) however the Device Tree was created
> > > after the fixed platform, not the problematic one. Both revisions of the
> > > silicon are in production.
> > > 
> > > There should have been an internal MDIO bus created for that chip revision
> > > such that we could have correctly parented phy@0 (bcm53125 below) as child
> > > node of the internal MDIO bus, but you have to realize that this was done
> > > back in 2014 when DSA was barely revived as an active subsystem. The
> > > BCM53125 node should have have been converted to an actual switch node at
> > > some point, I use a mdio_boardinfo overlay downstream to support the switch
> > > as a proper b53/DSA switch, anyway.
> > 
> > I was expecting something like that. I think this patch needs a
> > comment in the code explaining it is a workaround for a DT blob which
> > cannot be changed. Maybe also make it conditional on the board
> > compatible string?
> 
> It is already targeted at the Broadcom pseudo PHY address (30) which is the
> one that needs diversion, I will update the patch description accordingly
> though.

O.K, looking at the patch, it was not clear to me it was already
restricted. 

	    Andrew