Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sat, 1 Feb 2020 16:25:18 +0100
From:   Andrew Lunn <andrew@lunn.ch>
To:     Jeremy Linton <jeremy.linton@arm.com>
Cc:     netdev@vger.kernel.org, opendmb@gmail.com, f.fainelli@gmail.com,
        davem@davemloft.net, bcm-kernel-feedback-list@broadcom.com,
        linux-kernel@vger.kernel.org, wahrenst@gmx.net,
        hkallweit1@gmail.com
Subject: Re: [PATCH 3/6] net: bcmgenet: enable automatic phy discovery
Message-ID: <20200201152518.GI9639@lunn.ch>
References: <20200201074625.8698-1-jeremy.linton@arm.com>
 <20200201074625.8698-4-jeremy.linton@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200201074625.8698-4-jeremy.linton@arm.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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@arm.com>
> ---
>  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?

Thanks
	Andrew