[PATCH v3 1/6] net: introduce DSA class for Ethernet switches

[Alexandru Marginean]

On 12/17/2019 10:41 AM, Vladimir Oltean wrote:
> On Tue, 17 Dec 2019 at 09:18, Alexandru Marginean
> <alexm.osslist at gmail.com> wrote:
>>
>> On 12/15/2019 2:08 PM, Vladimir Oltean wrote:
>>> On Tue, 3 Dec 2019 at 17:32, Alex Marginean <alexandru.marginean at nxp.com> wrote:
>>>> +/**
>>>> + * This function deals with additional devices around the switch as these should
>>>> + * have been bound to drivers by now.
>>>> + * TODO: pick up references to other switch devices here, if we're cascaded.
>>>> + */
>>>> +static int dm_dsa_pre_probe(struct udevice *dev)
>>>> +{
>>>> +       struct dsa_perdev_platdata *platdata = dev_get_platdata(dev);
>>>> +       int i;
>>>> +
>>>> +       if (!platdata)
>>>> +               return -EINVAL;
>>>> +
>>>> +       if (ofnode_valid(platdata->master_node))
>>>> +               uclass_find_device_by_ofnode(UCLASS_ETH, platdata->master_node,
>>>> +                                            &platdata->master_dev);
>>>> +
>>>> +       for (i = 0; i < platdata->num_ports; i++) {
>>>> +               struct dsa_port_platdata *port = &platdata->port[i];
>>>> +
>>>> +               if (port->dev) {
>>>> +                       port->dev->priv = port;
>>>> +                       port->phy = dm_eth_phy_connect(port->dev);
>>>
>>> Fixed-link interfaces don't work with DM_MDIO. That is somewhat
>>> natural as there is no MDIO bus for a fixed-link. However the legacy
>>> phy_connect function can be made rather easily to work with
>>> fixed-link, since it has the necessary code for dealing with it
>>> already. I am not, however, sure how it ever worked in the absence of
>>> an MDIO bus.
>>>
>>> commit 1b7e23cc7e6d0dc3fe7ae9c55390b40717ff3c3a
>>> Author: Vladimir Oltean <olteanv at gmail.com>
>>> Date:   Sat Dec 14 23:25:40 2019 +0200
>>>
>>>       phy: make phy_connect_fixed work with a null mdio bus
>>>
>>>       It is utterly pointless to require an MDIO bus pointer for a fixed PHY
>>>       device. The fixed.c implementation does not require it, only
>>>       phy_device_create. Fix that.
>>>
>>>       Signed-off-by: Vladimir Oltean <olteanv at gmail.com>
>>>
>>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>>> index 80a7664e4978..8ea5c9005291 100644
>>> --- a/drivers/net/phy/phy.c
>>> +++ b/drivers/net/phy/phy.c
>>> @@ -658,7 +658,7 @@ static struct phy_device *phy_device_create(struct
>>> mii_dev *bus, int addr,
>>>           dev = malloc(sizeof(*dev));
>>>           if (!dev) {
>>>                   printf("Failed to allocate PHY device for %s:%d\n",
>>> -                      bus->name, addr);
>>> +                      bus ? bus->name : "(null bus)", addr);
>>>                   return NULL;
>>>           }
>>>
>>> @@ -686,7 +686,7 @@ static struct phy_device *phy_device_create(struct
>>> mii_dev *bus, int addr,
>>>                   return NULL;
>>>           }
>>>
>>> -       if (addr >= 0 && addr < PHY_MAX_ADDR)
>>> +       if (addr >= 0 && addr < PHY_MAX_ADDR && phy_id != PHY_FIXED_ID)
>>>                   bus->phymap[addr] = dev;
>>>
>>>           return dev;
>>>
>>> With the patch above in place, fixed-link can also be made to work
>>> with some logic similar to what can be seen below:
>>>
>>>       if (ofnode_valid(ofnode_find_subnode(port->dev->node, "fixed-link")))
>>>           port->phy = phy_connect(NULL, 0, port->dev, phy_mode); //
>>> phy_mode needs to be pre-parsed somewhere else as well
>>>       else
>>>           port->phy = dm_eth_phy_connect(port->dev);
>>>
>>> How would you see fixed-link interfaces being treated? My question so
>>> far is in the context of front-panel ports but I am interested in your
>>> view of the CPU port situation as well.
>>
>> I was thinking turning dm_eth_phy_connect into a more generic helper
>> that also deals with fixed links, which it does not yet.  That would
> 
> How would you do that? Just put my snippet above in a higher-level
> wrapper over dm_eth_phy_connect and phy_connect? Otherwise I think
> it's pretty difficult and hacky to pass a null mdiodev pointer through
> dm_mdio_phy_connect.

It wouldn't go through mdio_phy_connect, dm_eth_phy_connect would need 
to figure out if it's a fixed link or PHY and then call the proper 
function, like in your snippet.
The MDIO NULL topic is a bit more complicated I think.  Some of the Eth 
drivers just deal with fixed link internally and don't create a phy 
device at all.  If there is a PHY device I think we need to have a MDIO 
bus for it, and we shouldn't piggy-back on real MDIO buses.  I'm 
thinking we could have a dummy MDIO bus created automatically by 
ETH/MDIO uclass code for fixed link PHY devices.

Thoughts?
Alex

> 
>> move the "fixed-link" if out of the driver code.  Ideally the driver
>> should be able to call a single helper and, if the device has a DT node,
>> it would get back a PHY handle to either a proper PHY or to a fixed link
>> (from phy_connect_fixed).
>>
>> Alex
>>
>>>
>>>> +               }
>>>> +       }
>>>> +
>>>> +       return 0;
>>>> +}
>>>
>>> Thanks,
>>> -Vladimir
>>>