Re: [EXT] Re: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Greg Ungerer <gerg@linux-m68k.org>]

Hi Andy,

On 21/10/20 12:19 pm, Andy Duan wrote:
> From: Greg Ungerer <gerg@linux-m68k.org> Sent: Wednesday, October 21, 2020 9:52 AM
>> Hi Andrew,
>>
>> Thanks for the quick response.
>>
>>
>> On 20/10/20 12:40 pm, Andrew Lunn wrote:
>>> On Tue, Oct 20, 2020 at 12:14:04PM +1000, Greg Ungerer wrote:
>>>> Hi Andrew,
>>>>
>>>> Commit f166f890c8f0 ("[PATCH] net: ethernet: fec: Replace interrupt
>>>> driven MDIO with polled IO") breaks the FEC driver on at least one of
>>>> the ColdFire platforms (the 5208). Maybe others, that is all I have
>>>> tested on so far.
>>>>
>>>> Specifically the driver no longer finds any PHY devices when it
>>>> probes the MDIO bus at kernel start time.
>>>>
>>>> I have pinned the problem down to this one specific change in this commit:
>>>>
>>>>> @@ -2143,8 +2142,21 @@ static int fec_enet_mii_init(struct
>> platform_device *pdev)
>>>>>      if (suppress_preamble)
>>>>>              fep->phy_speed |= BIT(7);
>>>>> +   /* Clear MMFR to avoid to generate MII event by writing MSCR.
>>>>> +    * MII event generation condition:
>>>>> +    * - writing MSCR:
>>>>> +    *      - mmfr[31:0]_not_zero & mscr[7:0]_is_zero &
>>>>> +    *        mscr_reg_data_in[7:0] != 0
>>>>> +    * - writing MMFR:
>>>>> +    *      - mscr[7:0]_not_zero
>>>>> +    */
>>>>> +   writel(0, fep->hwp + FEC_MII_DATA);
>>>>
>>>> At least by removing this I get the old behavior back and everything
>>>> works as it did before.
>>>>
>>>> With that write of the FEC_MII_DATA register in place it seems that
>>>> subsequent MDIO operations return immediately (that is FEC_IEVENT is
>>>> set) - even though it is obvious the MDIO transaction has not completed yet.
>>>>
>>>> Any ideas?
>>>
>>> Hi Greg
>>>
>>> This has come up before, but the discussion fizzled out without a
>>> final patch fixing the issue. NXP suggested this
>>>
>>> writel(0, fep->hwp + FEC_MII_DATA);
>>>
>>> Without it, some other FEC variants break because they do generate an
>>> interrupt at the wrong time causing all following MDIO transactions to
>>> fail.
>>>
>>> At the moment, we don't seem to have a clear understanding of the
>>> different FEC versions, and how their MDIO implementations vary.
>>
>> Based on Andy and Chris' comments is something like the attached patch what
>> we need?
> 
> Greg, imx28 platform also requires the flag.

Got it, thanks. I will update the patch. I won't resend a v2 just yet, 
wait and see if there is any other comments.

Regards
Greg