Re: [PATCH 3/6] net: bcmgenet: enable automatic phy discovery

[Jeremy Linton]

Hi,

On 2/1/20 9:25 AM, Andrew Lunn wrote:
> On Sat, Feb 01, 2020 at 01:46:22AM -0600, Jeremy Linton wrote:
> > The unimac mdio driver falls back to scanning the
> > entire bus if its given an appropriate mask. In ACPI
> > mode we expect that the system is well behaved and
> > conforms to recent versions of the specification.
> >
> > We then utilize phy_find_first(), and
> > phy_connect_direct() to find and attach to the
> > discovered phy during net_device open.
> >
> > Signed-off-by: Jeremy Linton <jeremy.linton@xxxxxxx>
> > ---
> >   drivers/net/ethernet/broadcom/genet/bcmmii.c | 40 +++++++++++++++++---
> >   1 file changed, 34 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
> > index 2049f8218589..f3271975b375 100644
> > --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
> > +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
> > @@ -5,7 +5,7 @@
> >    * Copyright (c) 2014-2017 Broadcom
> >    */
> >   
> > -
> > +#include <linux/acpi.h>
> >   #include <linux/types.h>
> >   #include <linux/delay.h>
> >   #include <linux/wait.h>
> > @@ -311,7 +311,9 @@ int bcmgenet_mii_config(struct net_device *dev, bool init)
> >   int bcmgenet_mii_probe(struct net_device *dev)
> >   {
> >   	struct bcmgenet_priv *priv = netdev_priv(dev);
> > -	struct device_node *dn = priv->pdev->dev.of_node;
> > +	struct device *kdev = &priv->pdev->dev;
> > +	struct device_node *dn = kdev->of_node;
> > +
> >   	struct phy_device *phydev;
> >   	u32 phy_flags = 0;
> >   	int ret;
> > @@ -334,7 +336,27 @@ int bcmgenet_mii_probe(struct net_device *dev)
> >   			return -ENODEV;
> >   		}
> >   	} else {
> > -		phydev = dev->phydev;
> > +		if (has_acpi_companion(kdev)) {
> > +			char mdio_bus_id[MII_BUS_ID_SIZE];
> > +			struct mii_bus *unimacbus;
> > +
> > +			snprintf(mdio_bus_id, MII_BUS_ID_SIZE, "%s-%d",
> > +				 UNIMAC_MDIO_DRV_NAME, priv->pdev->id);
> > +
> > +			unimacbus = mdio_find_bus(mdio_bus_id);
> > +			if (!unimacbus) {
> > +				pr_err("Unable to find mii\n");
> > +				return -ENODEV;
> > +			}
> > +			phydev = phy_find_first(unimacbus);
> > +			put_device(&unimacbus->dev);
> > +			if (!phydev) {
> > +				pr_err("Unable to find PHY\n");
> > +				return -ENODEV;
>
> Hi Jeremy
>
> phy_find_first() is not recommended. Only use it if you have no other
> option. If the hardware is more complex, two PHYs on one bus, you are
> going to have a problem. So i suggest this is used only for PCI cards
> where the hardware is very fixed, and there is only ever one MAC and
> PHY on the PCI card. When you do have this split between MAC and MDIO
> bus, each being independent devices, it is more likely that you do
> have multiple PHYs on one shared MDIO bus.
>
> In the DT world, you use a phy-handle to point to the PHY node in the
> device tree. Does ACPI have the same concept, a pointer to some other
> device in ACPI?


I though I should clarify the direct question here about ACPI. ACPI does 
have the ability to do what you describe, but it a more rigorous way. If 
you look at the ACPI GenericSerialBus abstraction you will see how ACPI 
would likely handle this situation. I've been considering making a 
similar comment in that large fwnode patch set posted the other day.