From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id D63BAC7619A for ; Sat, 15 Apr 2023 12:44:07 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229766AbjDOMoA (ORCPT ); Sat, 15 Apr 2023 08:44:00 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:52066 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229481AbjDOMn7 (ORCPT ); Sat, 15 Apr 2023 08:43:59 -0400 Received: from fudo.makrotopia.org (fudo.makrotopia.org [IPv6:2a07:2ec0:3002::71]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 572B74487 for ; Sat, 15 Apr 2023 05:43:51 -0700 (PDT) Received: from local by fudo.makrotopia.org with esmtpsa (TLS1.3:TLS_AES_256_GCM_SHA384:256) (Exim 4.96) (envelope-from ) id 1pnfFp-00023W-02; Sat, 15 Apr 2023 14:43:45 +0200 Date: Sat, 15 Apr 2023 13:43:36 +0100 From: Daniel Golle To: =?utf-8?B?QXLEsW7DpyDDnE5BTA==?= Cc: Vladimir Oltean , Frank Wunderlich , netdev , erkin.bozoglu@xeront.com Subject: Re: mt7530: dsa_switch_parse_of() fails, causes probe code to run twice Message-ID: References: <896514df-af33-6408-8b33-d8fd06e671ef@arinc9.com> <5e10f823-88f1-053a-d691-6bc900bd85a6@arinc9.com> <01fe9c85-f1e0-107a-6fb7-e643fb76544e@arinc9.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <01fe9c85-f1e0-107a-6fb7-e643fb76544e@arinc9.com> Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Sat, Apr 15, 2023 at 10:53:10AM +0300, Arınç ÜNAL wrote: > 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 Tested on BPi-R64 (MT7622A+MT7531BE), BPi-R3 (MT7986A+MT7531AE) and MT7988A reference board. All working just fine. For BPi-R2 (MT7623N+MT7530) I made sure to disable the already enabled regulators in the error path and hence made the WARN_ON no longer trigger, see: https://github.com/dangowrt/linux/commit/55035b5ac739914166ed4f026262d0fc9b17bc76 > > 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. What's bad about having the hardware setup in mt7530_setup()? I think it even makes more sense to have it there and *not* in mt7530_probe() for exactly such reasons. Maybe we can even also move the reset function there and really do *all* of the hardware setup there and let the probe function really just parse DTS, probe the hardware, allocate memory and initialize data-structures like it is supposed to be. That being said, I thought that having PLL actication in mt7530_probe() would make things easier (as in: not require a function pointer to the sgmii_create function), but looking at it now this is not even true, we now require void mt7530_core_write(struct mt7530_priv *priv, u32 reg, u32 val); void mt7530_core_set(struct mt7530_priv *priv, u32 reg, u32 val); void mt7530_core_clear(struct mt7530_priv *priv, u32 reg, u32 val); void mt7530_write(struct mt7530_priv *priv, u32 reg, u32 val); u32 _mt7530_read(struct mt7530_dummy_poll *p); u32 mt7530_read(struct mt7530_priv *priv, u32 reg); being either exported or to be inline functions in mt7530.h which previously wasn't needed... I may miss something here and would like to understand your perspective: What exactly is the argument for moving all of the setup to the probe function? > 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ç