Re: [PATCH] net: fec: fix probing of fec1 when fec0 is not probed yet

[Heiko Schocher]

Hello Wei,

On 27.11.23 07:55, Wei Fang wrote:
>> -----Original Message-----
>> From: Heiko Schocher <hs@xxxxxxx>
>> Sent: 2023年11月24日 13:19
>> To: Wei Fang <wei.fang@xxxxxxx>; Andrew Lunn <andrew@xxxxxxx>; Heiko
>> Schocher <heiko.schocher@xxxxxxxxx>
>> Cc: netdev@xxxxxxxxxxxxxxx; Clark Wang <xiaoning.wang@xxxxxxx>; David S.
>> Miller <davem@xxxxxxxxxxxxx>; Eric Dumazet <edumazet@xxxxxxxxxx>;
>> Jakub Kicinski <kuba@xxxxxxxxxx>; dl-linux-imx <linux-imx@xxxxxxx>; Paolo
>> Abeni <pabeni@xxxxxxxxxx>; Shenwei Wang <shenwei.wang@xxxxxxx>;
>> linux-kernel@xxxxxxxxxxxxxxx; Alexander Sverdlin
>> <alexander.sverdlin@xxxxxxxxxxx>
>> Subject: Re: [PATCH] net: fec: fix probing of fec1 when fec0 is not probed yet
>>
>> Hello Wei,
>>
>> On 24.11.23 03:33, Wei Fang wrote:
>>>> -----Original Message-----
>>>> From: Andrew Lunn <andrew@xxxxxxx>
>>>> Sent: 2023年11月24日 4:09
>>>> To: Heiko Schocher <heiko.schocher@xxxxxxxxx>
>>>> Cc: netdev@xxxxxxxxxxxxxxx; Heiko Schocher <hs@xxxxxxx>; Clark Wang
>>>> <xiaoning.wang@xxxxxxx>; David S. Miller <davem@xxxxxxxxxxxxx>; Eric
>>>> Dumazet <edumazet@xxxxxxxxxx>; Jakub Kicinski <kuba@xxxxxxxxxx>;
>>>> dl-linux-imx <linux-imx@xxxxxxx>; Paolo Abeni <pabeni@xxxxxxxxxx>;
>>>> Shenwei Wang <shenwei.wang@xxxxxxx>; Wei Fang
>> <wei.fang@xxxxxxx>;
>>>> linux-kernel@xxxxxxxxxxxxxxx
>>>> Subject: Re: [PATCH] net: fec: fix probing of fec1 when fec0 is not
>>>> probed yet
>>>>
>>>> On Thu, Nov 23, 2023 at 02:27:43PM +0100, Heiko Schocher wrote:
>>>>> it is possible that fec1 is probed before fec0. On SoCs with
>>>>> FEC_QUIRK_SINGLE_MDIO set (which means fec1 uses mii from fec0) init
>>>>> of mii fails for fec1 when fec0 is not yet probed, as fec0 setups
>>>>> mii bus. In this case fec_enet_mii_init for fec1 returns with
>>>>> -ENODEV, and so fec1 never comes up.
>>>>>
>>>>> Return here with -EPROBE_DEFER so interface gets later probed again.
>>>>>
>>>>> Found this on imx8qxp based board, using 2 ethernet interfaces, and
>>>>> from time to time, fec1 interface came not up.
>>>>>
>>>>> Signed-off-by: Heiko Schocher <hs@xxxxxxx>
>>>>> ---
>>>>>
>>>>>  drivers/net/ethernet/freescale/fec_main.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/net/ethernet/freescale/fec_main.c
>>>>> b/drivers/net/ethernet/freescale/fec_main.c
>>>>> index c3b7694a7485..d956f95e7a65 100644
>>>>> --- a/drivers/net/ethernet/freescale/fec_main.c
>>>>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>>>>> @@ -2445,7 +2445,7 @@ static int fec_enet_mii_init(struct
>>>> platform_device *pdev)
>>>>>  			mii_cnt++;
>>>>>  			return 0;
>>>>>  		}
>>>>> -		return -ENOENT;
>>>>> +		return -EPROBE_DEFER;
>>>>
>>>> I think this has been tried before.
>>>>
>>> Yes, there was indeed a similar patch [1] before. but this issue seems
>>> to only exist in downstream tree, because there is a local patch which is not
>> in the upstream.
>>>
>>>         /* board only enable one mii bus in default */
>>>         if (!of_get_property(np, "fsl,mii-exclusive", NULL))
>>>                 fep->quirks |= FEC_QUIRK_SINGLE_MDIO;
>>
>> Yes, sorry for overseeing this ... but there are SoCs in upstream which set
>> FEC_QUIRK_SINGLE_MDIO and so mii bus from for example fec0 is used on
>> other fecX devices, correct?
>>
> Yes, i.MX28 still uses FEC_QUIRK_SINGLE_MDIO flag in the upstream, But the
> current patch doesn't seem to completely solve this issue.
> 
> If fec1 is probed before fec0, I think its fep->dev_id should be equal to 0, so it will
> not go to the following statement.

Indeed... and than fep->dev_id is interchanged too... I wonder why not using:

	fep->dev_id = of_alias_get_id(np, "ethernet");

in fec_probe function for getting the dev_id. With that no problems with
"wrong" dev_id and also my change makes sense...

But you are correct, currently patch is useless.

>  if ((fep->quirks & FEC_QUIRK_SINGLE_MDIO) && fep->dev_id > 0) {
> 		/* fec1 uses fec0 mii_bus */
> 		if (mii_cnt && fec0_mii_bus) {
> 			fep->mii_bus = fec0_mii_bus;
> 			mii_cnt++;
> 			return 0;
> 		}
> 		return -ENOENT;
> }
> 
> I'm considering removing the FEC_QUIRK_SINGLE_MDIO flag from upstream
> in the future as it's not necessary. But not now as I am busy with other projects.

Hmm.. and what is on platforms which have this usecase?

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs@xxxxxxx