[RFC net-next] of: mdio: Honor hints from MDIO bus drivers

[RFC net-next] of: mdio: Honor hints from MDIO bus drivers

[Florian Fainelli]

A MDIO bus driver can set phy_mask to indicate which PHYs should be
probed and which should not. Right now, of_mdiobus_register() always
sets mdio->phy_mask to ~0 which means: don't probe anything yourself,
and let the Device Tree scanning do it based on the availability of
child nodes.

When MDIO buses are stacked together (on purpose, as is done by DSA), we
run into possible double probing which is, at best unnecessary, and at
worse, can cause problems if that's not expected (e.g: during probe
deferral).

Fix this by remember the original mdio->phy_mask, and make sure that if
it was set to all 0xF, we set it to zero internally in order not to
influence how the child PHY/MDIO device registration is going to behave.
When the original mdio->phy_mask is set to something non-zero, we honor
this value and utilize it as a hint to register only the child nodes
that we have both found, and indicated to be necessary.

Signed-off-by: Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
Sending this as RFC because a quick look at the current tree makes
me think we are fine, but I would appreciate some review/feedback
before this gets merged.

Thank you!

 drivers/of/of_mdio.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index 0b2979816dbf..6bfbf00623cb 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -209,6 +209,7 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
 {
 	struct device_node *child;
 	bool scanphys = false;
+	u32 orig_phy_mask;
 	int addr, rc;
 
 	/* Do not continue if the node is disabled */
@@ -217,8 +218,15 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
 
 	/* Mask out all PHYs from auto probing.  Instead the PHYs listed in
 	 * the device tree are populated after the bus has been registered */
+	orig_phy_mask = mdio->phy_mask;
 	mdio->phy_mask = ~0;
 
+	/* If the original phy_mask was all 0xf, we make it zero here in order
+	 * to get child Device Tree nodes to be probed successfully
+	 */
+	if (orig_phy_mask == mdio->phy_mask)
+		orig_phy_mask = 0;
+
 	mdio->dev.of_node = np;
 
 	/* Register the MDIO bus */
@@ -234,6 +242,10 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
 			continue;
 		}
 
+		/* Honor hints from the mdio bus */
+		if (orig_phy_mask & BIT(addr))
+			continue;
+
 		if (of_mdiobus_child_is_phy(child))
 			of_mdiobus_register_phy(mdio, child, addr);
 		else
-- 
2.9.3

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC net-next] of: mdio: Honor hints from MDIO bus drivers

[Florian Fainelli]

On 04/10/2017 02:42 PM, Florian Fainelli wrote:
> A MDIO bus driver can set phy_mask to indicate which PHYs should be
> probed and which should not. Right now, of_mdiobus_register() always
> sets mdio->phy_mask to ~0 which means: don't probe anything yourself,
> and let the Device Tree scanning do it based on the availability of
> child nodes.
> 
> When MDIO buses are stacked together (on purpose, as is done by DSA), we
> run into possible double probing which is, at best unnecessary, and at
> worse, can cause problems if that's not expected (e.g: during probe
> deferral).
> 
> Fix this by remember the original mdio->phy_mask, and make sure that if
> it was set to all 0xF, we set it to zero internally in order not to
> influence how the child PHY/MDIO device registration is going to behave.
> When the original mdio->phy_mask is set to something non-zero, we honor
> this value and utilize it as a hint to register only the child nodes
> that we have both found, and indicated to be necessary.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> Sending this as RFC because a quick look at the current tree makes
> me think we are fine, but I would appreciate some review/feedback
> before this gets merged.

To give some more background and rational for this change.

On a platform where we have a parent MDIO bus, backed by the
mdio-bcm-unimac.c driver, we also register a slave MII bus (through
net/dsa/dsa2.c) which is parented to this UniMAC MDIO bus through an
assignment of of_node. This slave MII bus is created in order to
intercept reads/writes to problematic addresses (e.g: that clashes with
another piece of hardware).

This means that the slave DSA MII bus inherits all child nodes from the
originating master MII bus. This also means that when the slave MII bus
is probed via of_mdiobus_register(), we probe the same devices twice:
once through the master, another time through the slave.

With this change, we avoid double probing, because when creating the
slave MDIO bus, we carefully set phy_mask to intentionally restrict the
child PHY/MDIO device's creation to the relevant devices.

> 
> Thank you!
> 
>  drivers/of/of_mdio.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
> index 0b2979816dbf..6bfbf00623cb 100644
> --- a/drivers/of/of_mdio.c
> +++ b/drivers/of/of_mdio.c
> @@ -209,6 +209,7 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
>  {
>  	struct device_node *child;
>  	bool scanphys = false;
> +	u32 orig_phy_mask;
>  	int addr, rc;
>  
>  	/* Do not continue if the node is disabled */
> @@ -217,8 +218,15 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
>  
>  	/* Mask out all PHYs from auto probing.  Instead the PHYs listed in
>  	 * the device tree are populated after the bus has been registered */
> +	orig_phy_mask = mdio->phy_mask;
>  	mdio->phy_mask = ~0;
>  
> +	/* If the original phy_mask was all 0xf, we make it zero here in order
> +	 * to get child Device Tree nodes to be probed successfully
> +	 */
> +	if (orig_phy_mask == mdio->phy_mask)
> +		orig_phy_mask = 0;
> +
>  	mdio->dev.of_node = np;
>  
>  	/* Register the MDIO bus */
> @@ -234,6 +242,10 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
>  			continue;
>  		}
>  
> +		/* Honor hints from the mdio bus */
> +		if (orig_phy_mask & BIT(addr))
> +			continue;
> +
>  		if (of_mdiobus_child_is_phy(child))
>  			of_mdiobus_register_phy(mdio, child, addr);
>  		else
> 


-- 
Florian

Re: [RFC net-next] of: mdio: Honor hints from MDIO bus drivers

[Andrew Lunn]

> To give some more background and rational for this change.
> 
> On a platform where we have a parent MDIO bus, backed by the
> mdio-bcm-unimac.c driver, we also register a slave MII bus (through
> net/dsa/dsa2.c) which is parented to this UniMAC MDIO bus through an
> assignment of of_node. This slave MII bus is created in order to
> intercept reads/writes to problematic addresses (e.g: that clashes with
> another piece of hardware).
> 
> This means that the slave DSA MII bus inherits all child nodes from the
> originating master MII bus. This also means that when the slave MII bus
> is probed via of_mdiobus_register(), we probe the same devices twice:
> once through the master, another time through the slave.

Ah, O.K. This makes more sense. On the hardware i have, we get three
deep in MDIO busses. We have the FEC mdio bus. On top of that we have
a gpio-mux-mdio, and on top of that we have the mv88e6xxx mdio
bus. And i've never seen issues.

So your real problem here is you have two mdio busses using the same
device tree properties. I would actually say that is just plain
broken.

	Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC net-next] of: mdio: Honor hints from MDIO bus drivers

[Florian Fainelli]

On 04/11/2017 04:14 PM, Andrew Lunn wrote:
>> To give some more background and rational for this change.
>>
>> On a platform where we have a parent MDIO bus, backed by the
>> mdio-bcm-unimac.c driver, we also register a slave MII bus (through
>> net/dsa/dsa2.c) which is parented to this UniMAC MDIO bus through an
>> assignment of of_node. This slave MII bus is created in order to
>> intercept reads/writes to problematic addresses (e.g: that clashes with
>> another piece of hardware).
>>
>> This means that the slave DSA MII bus inherits all child nodes from the
>> originating master MII bus. This also means that when the slave MII bus
>> is probed via of_mdiobus_register(), we probe the same devices twice:
>> once through the master, another time through the slave.
> 
> Ah, O.K. This makes more sense. On the hardware i have, we get three
> deep in MDIO busses. We have the FEC mdio bus. On top of that we have
> a gpio-mux-mdio, and on top of that we have the mv88e6xxx mdio
> bus. And i've never seen issues.
> 
> So your real problem here is you have two mdio busses using the same
> device tree properties. I would actually say that is just plain
> broken.

>From a Device Tree/HW representation perspective, we do have the
external BCM53125 switch physically attached to the 7445/7278
SWITCH_MDIO bus (backed by mdio-bcm-unimac) so in that regard the
representation is correct. There is also an integrated Gigabit PHY
(bcm7xxx) which is attached to that bus.

>From a SW perspective though, we want to talk to the integrated Gigabit
PHY using mdio-bcm-unimac but talk to the external BCM53125 switch using
the slave MII bus created by the bcm_sf2 driver in order to create an
isolation. We need to inherit some of the parent (mdio-bcm-unimac) child
DT nodes (such as the BCM53125), but not the GPHY. The easiest solution
I found was to use this patch.

Using mdiobus_register() instead of of_mdiobus_register() was
considered, but then, the child BCM53125 has no more "visbility" into
the OF world at all, and it matters, because this switch is also driven
via a DSA switch driver and its Ethernet data-path is connected to one
port of the bcm_sf2 switch..

Thankfully the HW bug was fixed eventually ;)
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC net-next] of: mdio: Honor hints from MDIO bus drivers

[Florian Fainelli]

On 04/11/2017 04:23 PM, Florian Fainelli wrote:
> On 04/11/2017 04:14 PM, Andrew Lunn wrote:
>>> To give some more background and rational for this change.
>>>
>>> On a platform where we have a parent MDIO bus, backed by the
>>> mdio-bcm-unimac.c driver, we also register a slave MII bus (through
>>> net/dsa/dsa2.c) which is parented to this UniMAC MDIO bus through an
>>> assignment of of_node. This slave MII bus is created in order to
>>> intercept reads/writes to problematic addresses (e.g: that clashes with
>>> another piece of hardware).
>>>
>>> This means that the slave DSA MII bus inherits all child nodes from the
>>> originating master MII bus. This also means that when the slave MII bus
>>> is probed via of_mdiobus_register(), we probe the same devices twice:
>>> once through the master, another time through the slave.
>>
>> Ah, O.K. This makes more sense. On the hardware i have, we get three
>> deep in MDIO busses. We have the FEC mdio bus. On top of that we have
>> a gpio-mux-mdio, and on top of that we have the mv88e6xxx mdio
>> bus. And i've never seen issues.
>>
>> So your real problem here is you have two mdio busses using the same
>> device tree properties. I would actually say that is just plain
>> broken.
> 
> From a Device Tree/HW representation perspective, we do have the
> external BCM53125 switch physically attached to the 7445/7278
> SWITCH_MDIO bus (backed by mdio-bcm-unimac) so in that regard the
> representation is correct. There is also an integrated Gigabit PHY
> (bcm7xxx) which is attached to that bus.
> 
> From a SW perspective though, we want to talk to the integrated Gigabit
> PHY using mdio-bcm-unimac but talk to the external BCM53125 switch using
> the slave MII bus created by the bcm_sf2 driver in order to create an
> isolation. We need to inherit some of the parent (mdio-bcm-unimac) child
> DT nodes (such as the BCM53125), but not the GPHY. The easiest solution
> I found was to use this patch.
> 
> Using mdiobus_register() instead of of_mdiobus_register() was
> considered, but then, the child BCM53125 has no more "visbility" into
> the OF world at all, and it matters, because this switch is also driven
> via a DSA switch driver and its Ethernet data-path is connected to one
> port of the bcm_sf2 switch..
> 
> Thankfully the HW bug was fixed eventually ;)

In fact, all I need is to flag the internal Gigabit PHY for the slave
MII bus node with something that makes it appear as "disabled" which I
can presumably do with of_update_property() and putting a status =
"disabled" property in there. Let me do something like that and see how
big of a hack this becomes.
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC net-next] of: mdio: Honor hints from MDIO bus drivers

[Andrew Lunn]

> >>> To give some more background and rational for this change.
> >>>
> >>> On a platform where we have a parent MDIO bus, backed by the
> >>> mdio-bcm-unimac.c driver, we also register a slave MII bus (through
> >>> net/dsa/dsa2.c) which is parented to this UniMAC MDIO bus through an
> >>> assignment of of_node. This slave MII bus is created in order to
> >>> intercept reads/writes to problematic addresses (e.g: that clashes with
> >>> another piece of hardware).
> >>>
> >>> This means that the slave DSA MII bus inherits all child nodes from the
> >>> originating master MII bus. This also means that when the slave MII bus
> >>> is probed via of_mdiobus_register(), we probe the same devices twice:
> >>> once through the master, another time through the slave.
> >>
> >> Ah, O.K. This makes more sense. On the hardware i have, we get three
> >> deep in MDIO busses. We have the FEC mdio bus. On top of that we have
> >> a gpio-mux-mdio, and on top of that we have the mv88e6xxx mdio
> >> bus. And i've never seen issues.
> >>
> >> So your real problem here is you have two mdio busses using the same
> >> device tree properties. I would actually say that is just plain
> >> broken.
> > 
> > From a Device Tree/HW representation perspective, we do have the
> > external BCM53125 switch physically attached to the 7445/7278
> > SWITCH_MDIO bus (backed by mdio-bcm-unimac) so in that regard the
> > representation is correct. There is also an integrated Gigabit PHY
> > (bcm7xxx) which is attached to that bus.

This is made harder by you talking about a board which does not appear
to have its DT file in mainline. So i'm having to guess what it looks
like.

So what i think we are talking about is this bit of code:

static int bcm_sf2_mdio_register(struct dsa_switch *ds)
{
        struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
        struct device_node *dn;
        static int index;
        int err;

        /* Find our integrated MDIO bus node */
        dn = of_find_compatible_node(NULL, NULL, "brcm,unimac-mdio");
        priv->master_mii_bus = of_mdio_find_bus(dn);
        if (!priv->master_mii_bus)
                return -EPROBE_DEFER;

        get_device(&priv->master_mii_bus->dev);
        priv->master_mii_dn = dn;

        priv->slave_mii_bus = devm_mdiobus_alloc(ds->dev);
        if (!priv->slave_mii_bus)
                return -ENOMEM;

        priv->slave_mii_bus->priv = priv;
        priv->slave_mii_bus->name = "sf2 slave mii";
        priv->slave_mii_bus->read = bcm_sf2_sw_mdio_read;
        priv->slave_mii_bus->write = bcm_sf2_sw_mdio_write;
        snprintf(priv->slave_mii_bus->id, MII_BUS_ID_SIZE, "sf2-%d",
                 index++);
        priv->slave_mii_bus->dev.of_node = dn;

If i get you right, your switch is hanging off the MDIO bus
"brcm,unimac-mdio" you find the dn for. You then register another MDIO
bus using the exact same node? How does that make any sense? Isn't it
a physical separate MDIO bus? So it should have its own set of nodes
in the device tree. This is how we do it for the Marvell switches. See
Documentation/devicetree/binding/net/dsa/marvell.txt and
arch/arm/boot/dts/vf610-zii-dev-rev-b.dts. That DT blob uses
phy-handle to link the switch ports to the phys on the mdio bus.

	   Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC net-next] of: mdio: Honor hints from MDIO bus drivers

[Florian Fainelli]

On 04/12/2017 03:10 PM, Andrew Lunn wrote:
>>>>> To give some more background and rational for this change.
>>>>>
>>>>> On a platform where we have a parent MDIO bus, backed by the
>>>>> mdio-bcm-unimac.c driver, we also register a slave MII bus (through
>>>>> net/dsa/dsa2.c) which is parented to this UniMAC MDIO bus through an
>>>>> assignment of of_node. This slave MII bus is created in order to
>>>>> intercept reads/writes to problematic addresses (e.g: that clashes with
>>>>> another piece of hardware).
>>>>>
>>>>> This means that the slave DSA MII bus inherits all child nodes from the
>>>>> originating master MII bus. This also means that when the slave MII bus
>>>>> is probed via of_mdiobus_register(), we probe the same devices twice:
>>>>> once through the master, another time through the slave.
>>>>
>>>> Ah, O.K. This makes more sense. On the hardware i have, we get three
>>>> deep in MDIO busses. We have the FEC mdio bus. On top of that we have
>>>> a gpio-mux-mdio, and on top of that we have the mv88e6xxx mdio
>>>> bus. And i've never seen issues.
>>>>
>>>> So your real problem here is you have two mdio busses using the same
>>>> device tree properties. I would actually say that is just plain
>>>> broken.
>>>
>>> From a Device Tree/HW representation perspective, we do have the
>>> external BCM53125 switch physically attached to the 7445/7278
>>> SWITCH_MDIO bus (backed by mdio-bcm-unimac) so in that regard the
>>> representation is correct. There is also an integrated Gigabit PHY
>>> (bcm7xxx) which is attached to that bus.
> 
> This is made harder by you talking about a board which does not appear
> to have its DT file in mainline. So i'm having to guess what it looks
> like.

The DT binding is in tree and provides an example of how the switch
looks like, below is the example, but I am also adding the MDIO bus and
the PHYs just so you can see how things wind up:

switch_top@f0b00000 {
        compatible = "simple-bus";
        #size-cells = <1>;
        #address-cells = <1>;
        ranges = <0 0xf0b00000 0x40804>;

        ethernet_switch@0 {
                compatible = "brcm,bcm7445-switch-v4.0";
                #size-cells = <0>;
                #address-cells = <1>;
                reg = <0x0 0x40000
                        0x40000 0x110
                        0x40340 0x30
                        0x40380 0x30
                        0x40400 0x34
                        0x40600 0x208>;
                reg-names = "core", "reg", intrl2_0", "intrl2_1",
                            "fcb, "acb";
                interrupts = <0 0x18 0
                                0 0x19 0>;
                brcm,num-gphy = <1>;
                brcm,num-rgmii-ports = <2>;
                brcm,fcb-pause-override;
                brcm,acb-packets-inflight;

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

                        port@0 {
                                label = "gphy";
                                reg = <0>;
				phy-handle = <&phy5>;
                        };

			sw0port1: port@1 {
				label = "rgmii_1";
				reg = <1>;
				phy-mode = "rgmii";
				fixed-link {
					speed = <1000>;
					full-duplex;
				};
			}
                };
        };

	mdio@403c0 {
		reg = <0x403c0 0x8 0x40300 0x18>;
		#address-cells = <0x1>;
		#size-cells = <0x0>;
		compatible = "brcm,unimac-mdio";
		reg-names = "mdio", "mdio_indir_rw";

		switch: switch@0 {
			broken-turn-around;
			reg = <0x0>;
			compatible = "brcm,bcm53125";
			#address-cells = <1>;
			#size-cells = <0>;

			ports {
				..
				port@8 {
					ethernet = <&sw0port1>;
				};
				...
			};
		};

		phy5: ethernet-phy@5 {
			reg = <0x5>;
			compatible = "ethernet-phy-ieee802.3-c22";
		};
	};
};


> 
> So what i think we are talking about is this bit of code:
> 
> static int bcm_sf2_mdio_register(struct dsa_switch *ds)
> {
>         struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
>         struct device_node *dn;
>         static int index;
>         int err;
> 
>         /* Find our integrated MDIO bus node */
>         dn = of_find_compatible_node(NULL, NULL, "brcm,unimac-mdio");
>         priv->master_mii_bus = of_mdio_find_bus(dn);
>         if (!priv->master_mii_bus)
>                 return -EPROBE_DEFER;
> 
>         get_device(&priv->master_mii_bus->dev);
>         priv->master_mii_dn = dn;
> 
>         priv->slave_mii_bus = devm_mdiobus_alloc(ds->dev);
>         if (!priv->slave_mii_bus)
>                 return -ENOMEM;
> 
>         priv->slave_mii_bus->priv = priv;
>         priv->slave_mii_bus->name = "sf2 slave mii";
>         priv->slave_mii_bus->read = bcm_sf2_sw_mdio_read;
>         priv->slave_mii_bus->write = bcm_sf2_sw_mdio_write;
>         snprintf(priv->slave_mii_bus->id, MII_BUS_ID_SIZE, "sf2-%d",
>                  index++);
>         priv->slave_mii_bus->dev.of_node = dn;
> 
> If i get you right, your switch is hanging off the MDIO bus
> "brcm,unimac-mdio" you find the dn for. You then register another MDIO
> bus using the exact same node? How does that make any sense? Isn't it
> a physical separate MDIO bus?

First, the main switch is memory mapped into the 7445 SoC's register
space. This switch has an external MDIO bus which connects to an
integrated Gigabit PHY at MDIO address 5, but also to a BCM53125 switch
at address 30.

Because of a bug in the D0 revision of the 7445, programming the
BCM53125 switch through MDIO ends-up programming the 7445 memory mapped
switch as well because the integrated 7445 switch has its pseudo-PHY
snooping accesses to the MDIO bus! What was done to work-around this is
to create a slave MII bus through DSA, and divert the reads/writes
from/to the BCM53125 by instead using internal 7445 switch registers
which isolate its pseudo-PHY from the MDIO bus, thus no double
programming anymore.

Since the BCM53125 switch is a) physically attached to the mdio@403c0
node, and b) needs to have visibility in the OF world for DSA to probe
it, this is what I did here.

The slave MII bus is using the same node here because that's the
simplest way to make this bus inherit the devices of interest from the
parent bus.

> So it should have its own set of nodes
> in the device tree. This is how we do it for the Marvell switches. See
> Documentation/devicetree/binding/net/dsa/marvell.txt and
> arch/arm/boot/dts/vf610-zii-dev-rev-b.dts. That DT blob uses
> phy-handle to link the switch ports to the phys on the mdio bus.

>From a pure HW representation, this is not quite correct, because the
switch is physically attached to mdio@403c0, but since we are
pathologically broken, we need something different here...
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC net-next] of: mdio: Honor hints from MDIO bus drivers

[Andrew Lunn]

> The DT binding is in tree and provides an example of how the switch
> looks like, below is the example, but I am also adding the MDIO bus and
> the PHYs just so you can see how things wind up:
> 
> switch_top@f0b00000 {
>         compatible = "simple-bus";
>         #size-cells = <1>;
>         #address-cells = <1>;
>         ranges = <0 0xf0b00000 0x40804>;
> 
>         ethernet_switch@0 {
>                 compatible = "brcm,bcm7445-switch-v4.0";
>                 #size-cells = <0>;
>                 #address-cells = <1>;
>                 reg = <0x0 0x40000
>                         0x40000 0x110
>                         0x40340 0x30
>                         0x40380 0x30
>                         0x40400 0x34
>                         0x40600 0x208>;
>                 reg-names = "core", "reg", intrl2_0", "intrl2_1",
>                             "fcb, "acb";
>                 interrupts = <0 0x18 0
>                                 0 0x19 0>;
>                 brcm,num-gphy = <1>;
>                 brcm,num-rgmii-ports = <2>;
>                 brcm,fcb-pause-override;
>                 brcm,acb-packets-inflight;
> 
>                 ports {
>                         #address-cells = <1>;
>                         #size-cells = <0>;
> 
>                         port@0 {
>                                 label = "gphy";
>                                 reg = <0>;
> 				phy-handle = <&phy5>;
>                         };
> 
> 			sw0port1: port@1 {
> 				label = "rgmii_1";
> 				reg = <1>;
> 				phy-mode = "rgmii";
> 				fixed-link {
> 					speed = <1000>;
> 					full-duplex;
> 				};
> 			}
>                 };
>         };
> 
> 	mdio@403c0 {
> 		reg = <0x403c0 0x8 0x40300 0x18>;
> 		#address-cells = <0x1>;
> 		#size-cells = <0x0>;
> 		compatible = "brcm,unimac-mdio";
> 		reg-names = "mdio", "mdio_indir_rw";
> 
> 		switch: switch@0 {
> 			broken-turn-around;
> 			reg = <0x0>;
> 			compatible = "brcm,bcm53125";
> 			#address-cells = <1>;
> 			#size-cells = <0>;
> 
> 			ports {
> 				..
> 				port@8 {
> 					ethernet = <&sw0port1>;
> 				};
> 				...
> 			};
> 		};
> 
> 		phy5: ethernet-phy@5 {
> 			reg = <0x5>;
> 			compatible = "ethernet-phy-ieee802.3-c22";
> 		};
> 	};
> };

So phy5 is connected to the internal switch with a phy-handle. But
because of your double usage of this node, it also can be mapped into
the external switches port 5?

Is that your problem?

It seems like you should add an mdio node inside your switch node, and
list your external switch internal/external phys there if needed.

   Andrew

Re: [RFC net-next] of: mdio: Honor hints from MDIO bus drivers

[Florian Fainelli]

On 04/13/2017 02:51 PM, Andrew Lunn wrote:
>> The DT binding is in tree and provides an example of how the switch
>> looks like, below is the example, but I am also adding the MDIO bus and
>> the PHYs just so you can see how things wind up:
>>
>> switch_top@f0b00000 {
>>         compatible = "simple-bus";
>>         #size-cells = <1>;
>>         #address-cells = <1>;
>>         ranges = <0 0xf0b00000 0x40804>;
>>
>>         ethernet_switch@0 {
>>                 compatible = "brcm,bcm7445-switch-v4.0";
>>                 #size-cells = <0>;
>>                 #address-cells = <1>;
>>                 reg = <0x0 0x40000
>>                         0x40000 0x110
>>                         0x40340 0x30
>>                         0x40380 0x30
>>                         0x40400 0x34
>>                         0x40600 0x208>;
>>                 reg-names = "core", "reg", intrl2_0", "intrl2_1",
>>                             "fcb, "acb";
>>                 interrupts = <0 0x18 0
>>                                 0 0x19 0>;
>>                 brcm,num-gphy = <1>;
>>                 brcm,num-rgmii-ports = <2>;
>>                 brcm,fcb-pause-override;
>>                 brcm,acb-packets-inflight;
>>
>>                 ports {
>>                         #address-cells = <1>;
>>                         #size-cells = <0>;
>>
>>                         port@0 {
>>                                 label = "gphy";
>>                                 reg = <0>;
>> 				phy-handle = <&phy5>;
>>                         };
>>
>> 			sw0port1: port@1 {
>> 				label = "rgmii_1";
>> 				reg = <1>;
>> 				phy-mode = "rgmii";
>> 				fixed-link {
>> 					speed = <1000>;
>> 					full-duplex;
>> 				};
>> 			}
>>                 };
>>         };
>>
>> 	mdio@403c0 {
>> 		reg = <0x403c0 0x8 0x40300 0x18>;
>> 		#address-cells = <0x1>;
>> 		#size-cells = <0x0>;
>> 		compatible = "brcm,unimac-mdio";
>> 		reg-names = "mdio", "mdio_indir_rw";
>>
>> 		switch: switch@0 {
>> 			broken-turn-around;
>> 			reg = <0x0>;
>> 			compatible = "brcm,bcm53125";
>> 			#address-cells = <1>;
>> 			#size-cells = <0>;
>>
>> 			ports {
>> 				..
>> 				port@8 {
>> 					ethernet = <&sw0port1>;
>> 				};
>> 				...
>> 			};
>> 		};
>>
>> 		phy5: ethernet-phy@5 {
>> 			reg = <0x5>;
>> 			compatible = "ethernet-phy-ieee802.3-c22";
>> 		};
>> 	};
>> };
> 
> So phy5 is connected to the internal switch with a phy-handle. But
> because of your double usage of this node, it also can be mapped into
> the external switches port 5?
> 
> Is that your problem?

Kind of, it does translate into an invalid mapping by virtue of the PHY
being in a bad state, see below.

The mapping per-se is not the problem, but the fact that the PHY driver
is probed twice is the original problem that I have. The double probing
comes from the switch driver being probed first (drivers/net/dsa/ comes
before drivers/net/ethernet) and depends on the master netdev to be running.

We need to turn on the Gigabit PHY clock in order to be able to read its
PHY OUI and map it to a driver (yes a workaround could be to put its
exact compatible string in DT, that way, no need for get_phy_id()). We
have a local change in mdio-bcm-unimac.c which does exactly that (using
the clock framework), and then, to avoid artificially bumping the clock
reference count, the BCM7xxx PHY driver in its ->probe() function checks
whether the clock is enabled (yes, using __clk_is_enabled while it
probably should not) and keep the clock turned on for the MDIO layer to
successfully read/write from the PHY. The BCM7xxx PHY driver does
properly manage the clock though, and turns it off upon ->remove(). We
got probed and removed once, no more clock enabled because of the first
probe deferral.

The second time around, when the slave MII bus probes us again, we go
through the BCM7xxx ->probe() and ->remove() callbacks again, but the
clock was already turned off due to first probe that got deferred.

When the bcm_sf2 driver finally gets initialized, we try to attach to
this Gigabit PHY, the driver is there, good, but the clock is turned off
already, so the PHY does not respond correctly at all anymore and we
end-up reading garbage.

> 
> It seems like you should add an mdio node inside your switch node, and
> list your external switch internal/external phys there if needed.

I think I am going to keep this hack local and think about solving this
differently on an upstream kernel, since I am convinced now this is not
necessarily the right approach.

Thanks for your time and consideration!
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC net-next] of: mdio: Honor hints from MDIO bus drivers

[David Miller]

From: Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Date: Mon, 10 Apr 2017 14:42:58 -0700

> A MDIO bus driver can set phy_mask to indicate which PHYs should be
> probed and which should not. Right now, of_mdiobus_register() always
> sets mdio->phy_mask to ~0 which means: don't probe anything yourself,
> and let the Device Tree scanning do it based on the availability of
> child nodes.
> 
> When MDIO buses are stacked together (on purpose, as is done by DSA), we
> run into possible double probing which is, at best unnecessary, and at
> worse, can cause problems if that's not expected (e.g: during probe
> deferral).
> 
> Fix this by remember the original mdio->phy_mask, and make sure that if
> it was set to all 0xF, we set it to zero internally in order not to
> influence how the child PHY/MDIO device registration is going to behave.
> When the original mdio->phy_mask is set to something non-zero, we honor
> this value and utilize it as a hint to register only the child nodes
> that we have both found, and indicated to be necessary.
> 
> Signed-off-by: Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

I don't think it's valid to have a unique OF node appear twice in the
device tree hiearchy.

Even if you can somehow hack this situation into working, you are
asking for all kinds of problems in the long run by doing things that
way.

If you have to, instantiate a new dummy device (perhaps a
platform_device, which thus can have private attributes you can store
in a structure whose layout you control) to act as the placeholder for
operation interception and property duplication.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html