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

2017-04-13 Thread 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@f0b0 {
>> compatible = "simple-bus";
>> #size-cells = <1>;
>> #address-cells = <1>;
>> ranges = <0 0xf0b0 0x40804>;
>>
>> ethernet_switch@0 {
>> compatible = "brcm,bcm7445-switch-v4.0";
>> #size-cells = <0>;
>> #address-cells = <1>;
>> reg = <0x0 0x4
>> 0x4 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

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

2017-04-13 Thread 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@f0b0 {
> compatible = "simple-bus";
> #size-cells = <1>;
> #address-cells = <1>;
> ranges = <0 0xf0b0 0x40804>;
> 
> ethernet_switch@0 {
> compatible = "brcm,bcm7445-switch-v4.0";
> #size-cells = <0>;
> #address-cells = <1>;
> reg = <0x0 0x4
> 0x4 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

2017-04-13 Thread David Miller
From: Florian Fainelli 
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 

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.


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

2017-04-12 Thread 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@f0b0 {
compatible = "simple-bus";
#size-cells = <1>;
#address-cells = <1>;
ranges = <0 0xf0b0 0x40804>;

ethernet_switch@0 {
compatible = "brcm,bcm7445-switch-v4.0";
#size-cells = <0>;
#address-cells = <1>;
reg = <0x0 0x4
0x4 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 (!pr

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

2017-04-12 Thread 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


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

2017-04-12 Thread 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


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

2017-04-11 Thread 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


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

2017-04-11 Thread 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


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

2017-04-11 Thread 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 
> ---
> 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


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

2017-04-10 Thread 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 
---
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