Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 19 Oct 2021 16:12:40 +0200
From:   Andrew Lunn <andrew@lunn.ch>
To:     Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
Cc:     Joakim Zhang <qiangqing.zhang@nxp.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] net: fec: defer probe if PHY on external MDIO bus is not
 available
Message-ID: <YW7SWKiUy8LfvSkl@lunn.ch>
References: <20211014113043.3518-1-matthias.schiffer@ew.tq-group.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211014113043.3518-1-matthias.schiffer@ew.tq-group.com>
Precedence: bulk

On Thu, Oct 14, 2021 at 01:30:43PM +0200, Matthias Schiffer wrote:
> On some SoCs like i.MX6UL it is common to use the same MDIO bus for PHYs
> on both Ethernet controllers. Currently device trees for such setups
> have to make assumptions regarding the probe order of the controllers:
> 
> For example in imx6ul-14x14-evk.dtsi, the MDIO bus of fec2 is used for
> the PHYs of both fec1 and fec2. The reason is that fec2 has a lower
> address than fec1 and is thus loaded first, so the bus is already
> available when fec1 is probed.
> 
> Besides being confusing, this limitation also makes it impossible to use
> the same device tree for variants of the i.MX6UL with one Ethernet
> controller (which have to use the MDIO of fec1, as fec2 does not exist)
> and variants with two controllers (which have to use fec2 because of the
> load order).
> 
> To fix this, defer the probe of the Ethernet controller when the PHY is
> not on our own MDIO bus and not available.
> 
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 47a6fc702ac7..dc070dd216e8 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -3820,7 +3820,28 @@ fec_probe(struct platform_device *pdev)
>  		goto failed_stop_mode;
>  
>  	phy_node = of_parse_phandle(np, "phy-handle", 0);
> -	if (!phy_node && of_phy_is_fixed_link(np)) {
> +	if (phy_node) {
> +		struct device_node *mdio_parent =
> +			of_get_next_parent(of_get_parent(phy_node));
> +
> +		ret = 0;
> +
> +		/* Skip PHY availability check for our own MDIO bus to avoid
> +		 * cyclic dependency
> +		 */
> +		if (mdio_parent != np) {
> +			struct phy_device *phy = of_phy_find_device(phy_node);
> +
> +			if (phy)
> +				put_device(&phy->mdio.dev);
> +			else
> +				ret = -EPROBE_DEFER;
> +		}

I've not looked at the details yet, just back from vacation. But this
seems wrong. I would of expected phylib to of returned -EPRODE_DEFER
at some point, when asked for a PHY which does not exist yet. All the
driver should need to do is make sure it returns the
-EPRODE_DEFER.

       Andrew