[net-next] dsa: slave: support phy devices on external MII bus

[net-next] dsa: slave: support phy devices on external MII bus

[Andrew Lunn]

Hi Martin

Sorry for starting a new thread. I deleted the patchset from my mailbox.

Florian said:

> The logic goes like this:
> 
> - try to connect to the PHY via phy-handle
> - if we have a PHY we are connecting via phy-handle but we need to
>   divert MDIO reads/writes connect using its address on the diverted
>   bus
> - connect using a fixed PHY
> - finally try using the DSA slave MII bus which would connect to the switch internal PHYs

This is not quite correct. Looking at the code:

        phy_dn = of_parse_phandle(port_dn, "phy-handle", 0);
 ...

        if (phy_dn) {
                int phy_id = of_mdio_parse_addr(&slave_dev->dev, phy_dn);

                /* If this PHY address is part of phys_mii_mask, which means
                 * that we need to divert reads and writes to/from it, then we
                 * want to bind this device using the slave MII bus created by
                 * DSA to make that happen.
                 */
                if (!phy_is_fixed && phy_id >= 0 &&
                    (ds->phys_mii_mask & (1 << phy_id))) {
                        ret = dsa_slave_phy_connect(p, slave_dev, phy_id);
                        if (ret) {
                                netdev_err(slave_dev, "failed to connect to phy%d: %d\n", phy_id, ret);
                                of_node_put(phy_dn);
                                return ret;
                        }
                } else {
                        p->phy = of_phy_connect(slave_dev, phy_dn,

The first point really is:

- try to connect to the PHY via phy-handle, if the phy_id is not
valid, or if the phy_id does not map to a phy that the switch says
does not exist.

In your case, all these points are true, so it uses
dsa_slave_phy_connect().  But we actually want it to use
of_phy_connect(), which will use the correct bus.

For some Marvell chips, you cannot actually go on ds->phys_mii_mask.
These devices can have in built PHYs and SERDES interfaces which can
be assigned to ports. These SERDES interfaces could have external PHYs
connected to them, and be on the external MDIO bus. So
ds->phys_mii_mask indicates there is a PHY, but the phy-handle points
to a different phy.

So i think this code block needs to change. If we have a phy-handle,
use it.  i.e. what Florian _thinks_ it should be doing. If not, then
use dsa_slave_phy_connect().

    Andrew

Re: [net-next] dsa: slave: support phy devices on external MII bus

[Florian Fainelli]

On 10/18/2017 09:21 AM, Andrew Lunn wrote:
> Hi Martin
> 
> Sorry for starting a new thread. I deleted the patchset from my mailbox.
> 
> Florian said:
> 
>> The logic goes like this:
>>
>> - try to connect to the PHY via phy-handle
>> - if we have a PHY we are connecting via phy-handle but we need to
>>   divert MDIO reads/writes connect using its address on the diverted
>>   bus
>> - connect using a fixed PHY
>> - finally try using the DSA slave MII bus which would connect to the switch internal PHYs
> 
> This is not quite correct. Looking at the code:
> 
>         phy_dn = of_parse_phandle(port_dn, "phy-handle", 0);
>  ...
> 
>         if (phy_dn) {
>                 int phy_id = of_mdio_parse_addr(&slave_dev->dev, phy_dn);
> 
>                 /* If this PHY address is part of phys_mii_mask, which means
>                  * that we need to divert reads and writes to/from it, then we
>                  * want to bind this device using the slave MII bus created by
>                  * DSA to make that happen.
>                  */
>                 if (!phy_is_fixed && phy_id >= 0 &&
>                     (ds->phys_mii_mask & (1 << phy_id))) {
>                         ret = dsa_slave_phy_connect(p, slave_dev, phy_id);
>                         if (ret) {
>                                 netdev_err(slave_dev, "failed to connect to phy%d: %d\n", phy_id, ret);
>                                 of_node_put(phy_dn);
>                                 return ret;
>                         }
>                 } else {
>                         p->phy = of_phy_connect(slave_dev, phy_dn,
> 
> The first point really is:
> 
> - try to connect to the PHY via phy-handle, if the phy_id is not
> valid, or if the phy_id does not map to a phy that the switch says
> does not exist.
> 
> In your case, all these points are true, so it uses
> dsa_slave_phy_connect().  But we actually want it to use
> of_phy_connect(), which will use the correct bus.
> 
> For some Marvell chips, you cannot actually go on ds->phys_mii_mask.
> These devices can have in built PHYs and SERDES interfaces which can
> be assigned to ports. These SERDES interfaces could have external PHYs
> connected to them, and be on the external MDIO bus. So
> ds->phys_mii_mask indicates there is a PHY, but the phy-handle points
> to a different phy.
> 
> So i think this code block needs to change. If we have a phy-handle,
> use it.  i.e. what Florian _thinks_ it should be doing. If not, then
> use dsa_slave_phy_connect().

I see what you mean now, the logic above gets defeated because it does
not concern itself with the MDIO controller parent of the PHY node
pointed to by phy-handle. So if like Martin you have two MDIO busses,
but both happen to have MDIO addresses that are valid for both busses,
the logic above gets defeated and we wrongly try to attach to the switch
internal MDIO bus under ds->slave_mii_bus.

The easiest fix would certainly to lookup the parent MDIO bus and do
that only if ds->slave_mii_bus->of_node and the parent of the node
pointed to 'phy-handle' match.

Does that work for you?
-- 
Florian

Re: [net-next] dsa: slave: support phy devices on external MII bus

[Martin Hundebøll]

On 2017-10-18 18:51, Florian Fainelli wrote:
> On 10/18/2017 09:21 AM, Andrew Lunn wrote:
>> Hi Martin
>>
>> Sorry for starting a new thread. I deleted the patchset from my mailbox.
>>
>> Florian said:
>>
>>> The logic goes like this:
>>>
>>> - try to connect to the PHY via phy-handle
>>> - if we have a PHY we are connecting via phy-handle but we need to
>>>    divert MDIO reads/writes connect using its address on the diverted
>>>    bus
>>> - connect using a fixed PHY
>>> - finally try using the DSA slave MII bus which would connect to the switch internal PHYs
>>
>> This is not quite correct. Looking at the code:
>>
>>          phy_dn = of_parse_phandle(port_dn, "phy-handle", 0);
>>   ...
>>
>>          if (phy_dn) {
>>                  int phy_id = of_mdio_parse_addr(&slave_dev->dev, phy_dn);
>>
>>                  /* If this PHY address is part of phys_mii_mask, which means
>>                   * that we need to divert reads and writes to/from it, then we
>>                   * want to bind this device using the slave MII bus created by
>>                   * DSA to make that happen.
>>                   */
>>                  if (!phy_is_fixed && phy_id >= 0 &&
>>                      (ds->phys_mii_mask & (1 << phy_id))) {
>>                          ret = dsa_slave_phy_connect(p, slave_dev, phy_id);
>>                          if (ret) {
>>                                  netdev_err(slave_dev, "failed to connect to phy%d: %d\n", phy_id, ret);
>>                                  of_node_put(phy_dn);
>>                                  return ret;
>>                          }
>>                  } else {
>>                          p->phy = of_phy_connect(slave_dev, phy_dn,
>>
>> The first point really is:
>>
>> - try to connect to the PHY via phy-handle, if the phy_id is not
>> valid, or if the phy_id does not map to a phy that the switch says
>> does not exist.
>>
>> In your case, all these points are true, so it uses
>> dsa_slave_phy_connect().  But we actually want it to use
>> of_phy_connect(), which will use the correct bus.
>>
>> For some Marvell chips, you cannot actually go on ds->phys_mii_mask.
>> These devices can have in built PHYs and SERDES interfaces which can
>> be assigned to ports. These SERDES interfaces could have external PHYs
>> connected to them, and be on the external MDIO bus. So
>> ds->phys_mii_mask indicates there is a PHY, but the phy-handle points
>> to a different phy.
>>
>> So i think this code block needs to change. If we have a phy-handle,
>> use it.  i.e. what Florian _thinks_ it should be doing. If not, then
>> use dsa_slave_phy_connect().
> 
> I see what you mean now, the logic above gets defeated because it does
> not concern itself with the MDIO controller parent of the PHY node
> pointed to by phy-handle. So if like Martin you have two MDIO busses,
> but both happen to have MDIO addresses that are valid for both busses,
> the logic above gets defeated and we wrongly try to attach to the switch
> internal MDIO bus under ds->slave_mii_bus.
> 
> The easiest fix would certainly to lookup the parent MDIO bus and do
> that only if ds->slave_mii_bus->of_node and the parent of the node
> pointed to 'phy-handle' match.
> 
> Does that work for you?
> 

As Andrew implies, I think we should rewrite the entire block to make it 
more intuitive.

Are these the cases that should be handled?

0) Fixed link
Register using of_phy_register_fixed_link().

1) No phy-handle
Use dsa_slave_phy_connect() to connect on internal MDIO bus with phy 
address from index/port-reg property.

2) Valid phy handle
Use of_phy_connect() to connect using parent MDIO bus handle.


I am most certainly missing some corner cases here, so please educate me!

// Martin

Re: [net-next] dsa: slave: support phy devices on external MII bus

[Florian Fainelli]

On 10/18/2017 10:30 AM, Martin Hundebøll wrote:
> 
> 
> On 2017-10-18 18:51, Florian Fainelli wrote:
>> On 10/18/2017 09:21 AM, Andrew Lunn wrote:
>>> Hi Martin
>>>
>>> Sorry for starting a new thread. I deleted the patchset from my mailbox.
>>>
>>> Florian said:
>>>
>>>> The logic goes like this:
>>>>
>>>> - try to connect to the PHY via phy-handle
>>>> - if we have a PHY we are connecting via phy-handle but we need to
>>>>    divert MDIO reads/writes connect using its address on the diverted
>>>>    bus
>>>> - connect using a fixed PHY
>>>> - finally try using the DSA slave MII bus which would connect to the
>>>> switch internal PHYs
>>>
>>> This is not quite correct. Looking at the code:
>>>
>>>          phy_dn = of_parse_phandle(port_dn, "phy-handle", 0);
>>>   ...
>>>
>>>          if (phy_dn) {
>>>                  int phy_id = of_mdio_parse_addr(&slave_dev->dev,
>>> phy_dn);
>>>
>>>                  /* If this PHY address is part of phys_mii_mask,
>>> which means
>>>                   * that we need to divert reads and writes to/from
>>> it, then we
>>>                   * want to bind this device using the slave MII bus
>>> created by
>>>                   * DSA to make that happen.
>>>                   */
>>>                  if (!phy_is_fixed && phy_id >= 0 &&
>>>                      (ds->phys_mii_mask & (1 << phy_id))) {
>>>                          ret = dsa_slave_phy_connect(p, slave_dev,
>>> phy_id);
>>>                          if (ret) {
>>>                                  netdev_err(slave_dev, "failed to
>>> connect to phy%d: %d\n", phy_id, ret);
>>>                                  of_node_put(phy_dn);
>>>                                  return ret;
>>>                          }
>>>                  } else {
>>>                          p->phy = of_phy_connect(slave_dev, phy_dn,
>>>
>>> The first point really is:
>>>
>>> - try to connect to the PHY via phy-handle, if the phy_id is not
>>> valid, or if the phy_id does not map to a phy that the switch says
>>> does not exist.
>>>
>>> In your case, all these points are true, so it uses
>>> dsa_slave_phy_connect().  But we actually want it to use
>>> of_phy_connect(), which will use the correct bus.
>>>
>>> For some Marvell chips, you cannot actually go on ds->phys_mii_mask.
>>> These devices can have in built PHYs and SERDES interfaces which can
>>> be assigned to ports. These SERDES interfaces could have external PHYs
>>> connected to them, and be on the external MDIO bus. So
>>> ds->phys_mii_mask indicates there is a PHY, but the phy-handle points
>>> to a different phy.
>>>
>>> So i think this code block needs to change. If we have a phy-handle,
>>> use it.  i.e. what Florian _thinks_ it should be doing. If not, then
>>> use dsa_slave_phy_connect().
>>
>> I see what you mean now, the logic above gets defeated because it does
>> not concern itself with the MDIO controller parent of the PHY node
>> pointed to by phy-handle. So if like Martin you have two MDIO busses,
>> but both happen to have MDIO addresses that are valid for both busses,
>> the logic above gets defeated and we wrongly try to attach to the switch
>> internal MDIO bus under ds->slave_mii_bus.
>>
>> The easiest fix would certainly to lookup the parent MDIO bus and do
>> that only if ds->slave_mii_bus->of_node and the parent of the node
>> pointed to 'phy-handle' match.
>>
>> Does that work for you?
>>
> 
> As Andrew implies, I think we should rewrite the entire block to make it
> more intuitive.

I don't particularly care what the fate of that code is, as long as it
does not break on my systems, you break it, you fix it.

> 
> Are these the cases that should be handled?
> 
> 0) Fixed link
> Register using of_phy_register_fixed_link().

Yes.

> 
> 1) No phy-handle
> Use dsa_slave_phy_connect() to connect on internal MDIO bus with phy
> address from index/port-reg property.

Yes.

> 
> 2) Valid phy handle
> Use of_phy_connect() to connect using parent MDIO bus handle.

Yes, but with the caveat already covered today: there is a possible
problem with having to divert MDIO accesses of a PHY pointed by
phy-handle towards the internal switch bus because of specific problems
such as those explained in drivers/net/bcm_sf2.c, I don't mind trying to
do things smarter or in a different way, but that needs to be something
possible.
-- 
Florian

Re: [net-next] dsa: slave: support phy devices on external MII bus

[Andrew Lunn]

> Yes, but with the caveat already covered today: there is a possible
> problem with having to divert MDIO accesses of a PHY pointed by
> phy-handle towards the internal switch bus because of specific problems
> such as those explained in drivers/net/bcm_sf2.c, I don't mind trying to
> do things smarter or in a different way, but that needs to be something
> possible.

Hi Florian

Are you referring to:

        /* Include the pseudo-PHY address to divert reads towards our
         * workaround. This is only required for 7445D0, since 7445E0
         * disconnects the internal switch pseudo-PHY such that we can use the
         * regular SWITCH_MDIO master controller instead.
         *
         * Here we flag the pseudo PHY as needing special treatment and would
         * otherwise make all other PHY read/writes go to the master MDIO bus
         * controller that comes with this switch backed by the "mdio-unimac"
         * driver.
         */
?

Do you have an example device tree fragment, just to make it clearer?

Thanks
	Andrew

Re: [net-next] dsa: slave: support phy devices on external MII bus

[Florian Fainelli]

On 10/18/2017 12:09 PM, Andrew Lunn wrote:
>> Yes, but with the caveat already covered today: there is a possible
>> problem with having to divert MDIO accesses of a PHY pointed by
>> phy-handle towards the internal switch bus because of specific problems
>> such as those explained in drivers/net/bcm_sf2.c, I don't mind trying to
>> do things smarter or in a different way, but that needs to be something
>> possible.
> 
> Hi Florian
> 
> Are you referring to:
> 
>         /* Include the pseudo-PHY address to divert reads towards our
>          * workaround. This is only required for 7445D0, since 7445E0
>          * disconnects the internal switch pseudo-PHY such that we can use the
>          * regular SWITCH_MDIO master controller instead.
>          *
>          * Here we flag the pseudo PHY as needing special treatment and would
>          * otherwise make all other PHY read/writes go to the master MDIO bus
>          * controller that comes with this switch backed by the "mdio-unimac"
>          * driver.
>          */
> ?

Yes.

> 
> Do you have an example device tree fragment, just to make it clearer?


Sure:

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>;
                        };

			port@1 {
				label = "rgmii_1";
				reg = <1>;
				/* connects to the external BCM53125 switch below */
				fixed-link {
					speed = <1000>;
					full-duplex;
				}
			};
                };
        };

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

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

		/* External BCM53125 switch that requires the workaround above */
		ethernet-switch@0 {
			reg = <0>;
			compatible = "brcm,bcm53125";

			ports {
				...
			};
	};
};

In fact, this now makes me think that I had to put this workaround in
place solely because at some point I modeled the external BCM53125
switch as a PHY device, and so I had to have phy_connect() work correctly!

Now that we have a proper MDIO device class, this may actually no longer
be necessary at all within net/dsa/slave.c::dsa_slave_phy_setup() since
the probe paths would work completely differently and the offending port
(port 1) would have a fixed-link property. I still need my local
workaround in bcm_sf2.c of course.

I guess, thanks for making me post this example and realize this is
probably no longer necessary :)
-- 
Florian

Re: [net-next] dsa: slave: support phy devices on external MII bus

[Andrew Lunn]

> I guess, thanks for making me post this example and realize this is
> probably no longer necessary :)

Hi Florian

You are welcome. Please test and report back. This code is complex, so
removing part of it would be great.

   Andrew