Date:   Sat, 15 Apr 2023 10:53:10 +0300
From:   Arınç ÜNAL <arinc.unal@...nc9.com>
To:     Daniel Golle <daniel@...rotopia.org>
Cc:     Vladimir Oltean <olteanv@...il.com>,
        Frank Wunderlich <frank-w@...lic-files.de>,
        netdev <netdev@...r.kernel.org>, erkin.bozoglu@...ont.com
Subject: Re: mt7530: dsa_switch_parse_of() fails, causes probe code to run
 twice

On 15.04.2023 03:52, Daniel Golle wrote:
> On Sat, Apr 15, 2023 at 03:28:55AM +0300, Arınç ÜNAL wrote:
>> On 15.04.2023 02:53, Daniel Golle wrote:
>>> On Sat, Apr 15, 2023 at 02:23:16AM +0300, Arınç ÜNAL wrote:
>>>> On 15.04.2023 01:48, Daniel Golle wrote:
>>>>> On Sat, Apr 15, 2023 at 01:41:07AM +0300, Arınç ÜNAL wrote:
>>>>>> Hey there,
>>>>>>
>>>>>> I've been working on the MT7530 DSA subdriver. While doing some tests, I
>>>>>> realised mt7530_probe() runs twice. I moved enabling the regulators from
>>>>>> mt7530_setup() to mt7530_probe(). Enabling the regulators there ends up
>>>>>> with exception warnings on the first time. It works fine when
>>>>>> mt7530_probe() is run again.
>>>>>>
>>>>>> This should not be an expected behaviour, right? Any ideas how we can make
>>>>>> it work the first time?
>>>>>
>>>>> Can you share the patch or work-in-progress tree which will allow me
>>>>> to reproduce this problem?
>>>>
>>>> I tested this on vanilla 6.3-rc6. There's just the diff below that is
>>>> applied. I encountered it on the standalone MT7530 on my Bananapi BPI-R2. I
>>>> haven't tried it on MCM MT7530 on MT7621 SoC yet.
>>>>
>>>>>
>>>>> It can of course be that regulator driver has not yet been loaded on
>>>>> the first run and -EPROBE_DEFER is returned in that case. Knowing the
>>>>> value of 'err' variable below would hence be valuable information.
>>>>
>>>> Regardless of enabling the regulator on either mt7530_probe() or
>>>> mt7530_setup(), dsa_switch_parse_of() always fails.
>>>
>>> So dsa_switch_parse_of() can return -EPROBE_DEFER if the ethernet
>>> driver responsible for the CPU port has not yet been loaded.
>>>
>>> See net/dsa/dsa.c (inside function dsa_port_parse_of):
>>> [...]
>>> 1232)                master = of_find_net_device_by_node(ethernet);
>>> 1233)                of_node_put(ethernet);
>>> 1234)                if (!master)
>>> 1235)                        return -EPROBE_DEFER;
>>> [...]
>>>
>>> Hence it would be important to include the value of 'err' in your
>>> debugging printf output, as -EPROBE_DEFER can be an expected and
>>> implicitely intended reality and nothing is wrong then.
>>
>> Thanks Daniel. I can't do more tests soon but this is probably what's going
>> on as the logs already indicate that the MediaTek ethernet driver was yet to
>> load.
>>
>> As acknowledged, since running the MT7530 DSA subdriver from scratch is
>> expected if the ethernet driver is not loaded yet, there's not really a
>> problem. Though the switch is reset twice in a short amount of time. I don't
>> think that's very great.
> 
> That's true, and we should try to avoid that.
> 
>>
>> The driver initialisation seems serialised (at least for the drivers built
>> into the kernel) as I tried sleeping for 5 seconds on mt7530_probe() but no
>> other driver was loaded in the meantime so I got the same behaviour.
>>
>> The regulator code will cause a long and nasty exception the first time.
>> Though there's nothing wrong as it does what it's supposed to do on the
>> second run. I'm not sure if that's negligible.
>>
>> Could we at least somehow make the MT7530 DSA subdriver wait until the
>> regulator driver is loaded?
> 
> I assume the regulator-related stackdump is unrelated, but caused by
> cpufreq changes, which had now been fixed by commit 0883426fd07e
> ("cpufreq: mediatek: Raise proc and sram max voltage for MT7622/7623").
> 
> If you are using v6.3-rc6 this commit is still missing there, but
> manually picking it from linux-next should fix it.

I did one better and just did the test on the current linux-next, I get 
exceptions that seem to be identical. I also made sure this commit was 
actually there.

> 
> Let me know if I can help with testing on my farm of MediaTek boards.
> I'm a bit nervous about fixing MT7531BE soon, so deciding if we move
> PLL activation to mt7530_probe() would be essential as it makes the
> fix much easier...

Can you test this branch on MT7531AE, MT7531BE and the switch on MT7988 
SoC? I just need to complete the patch logs, the code won't change much.

https://github.com/arinc9/linux/commits/for-netnext

I'm thinking if we can -EPROBE_DEFER right at the start of 
mt7530_probe(), it should prevent the reset code from running twice, and 
enabling the regulator will run without any exceptions.

I think I can just keep enabling the regulator on mt7530_setup() if I 
can't figure that out. On MT7623NI, the switch stops working after 35 
seconds with these logs. As long as the regulator is enabled before 
this, everything keeps working.

[   35.037200] vusb: disabling
[   35.040089] vmc: disabling
[   35.042856] vmch: disabling
[   35.045709] vgp1: disabling
[   35.049010] vcamaf: disabling

Arınç

Powered by blists - more mailing lists