Message-ID: <CAMRc=MfznDaaNcfvRBg1wpiOkyTE=Ks-_nx=aCY1MR5-50Ka+A@mail.gmail.com>
Date: Thu, 27 Jun 2024 20:35:16 +0200
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Andrew Halaney <ahalaney@...hat.com>, Russell King <linux@...linux.org.uk>
Cc: Vinod Koul <vkoul@...nel.org>, Alexandre Torgue <alexandre.torgue@...s.st.com>, 
	Jose Abreu <joabreu@...opsys.com>, "David S . Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Maxime Coquelin <mcoquelin.stm32@...il.com>, netdev@...r.kernel.org, 
	linux-arm-msm@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [PATCH v2 net-next 2/2] net: stmmac: qcom-ethqos: add a DMA-reset
 quirk for sa8775p-ride

On Thu, Jun 27, 2024 at 7:07 PM Andrew Halaney <ahalaney@...hat.com> wrote:
>
> On Thu, Jun 27, 2024 at 01:39:47PM GMT, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
> >
> > On sa8775p-ride the RX clocks from the AQR115C PHY are not available at
> > the time of the DMA reset so we need to loop TX clocks to RX and then
> > disable loopback after link-up. Use the existing callbacks to do it just
> > for this board.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
>
> Sorry, not being very helpful but trying to understand these changes
> and the general cleanup of stmmac... so I'll point out that I'm still
> confused by this based on Russell's last response:
> https://lore.kernel.org/netdev/ZnQLED%2FC3Opeim5q@shell.armlinux.org.uk/
>

I realized Russell's email didn't pop up in get_maintainers.pl for
stmmac. Adding him now.

> Quote:
>
>     If you're using true Cisco SGMII, there are _no_ clocks transferred
>     between the PHY and PCS/MAC. There are two balanced pairs of data
>     lines and that is all - one for transmit and one for receive. So this
>     explanation doesn't make sense to me.
>
>
> <snip>
>
> > +}
> > +
> >  static void ethqos_set_func_clk_en(struct qcom_ethqos *ethqos)
> >  {
> > +     qcom_ethqos_set_sgmii_loopback(ethqos, true);
> >       rgmii_updatel(ethqos, RGMII_CONFIG_FUNC_CLK_EN,
> >                     RGMII_CONFIG_FUNC_CLK_EN, RGMII_IO_MACRO_CONFIG);
> >  }
> <snip>
> > @@ -682,6 +702,7 @@ static void ethqos_fix_mac_speed(void *priv, unsigned int speed, unsigned int mo
> >  {
> >       struct qcom_ethqos *ethqos = priv;
> >
> > +     qcom_ethqos_set_sgmii_loopback(ethqos, false);
>
> I'm trying to map out when the loopback is currently enabled/disabled
> due to Russell's prior concerns.
>
> Quote:
>
>     So you enable loopback at open time, and disable it when the link comes
>     up. This breaks inband signalling (should stmmac ever use that) because
>     enabling loopback prevents the PHY sending the SGMII result to the PCS
>     to indicate that the link has come up... thus phylink won't call
>     mac_link_up().
>
>     So no, I really hate this proposed change.
>
>     What I think would be better is if there were hooks at the appropriate
>     places to handle the lack of clock over _just_ the period that it needs
>     to be handled, rather than hacking the driver as this proposal does,
>     abusing platform callbacks because there's nothing better.
>
> looks like currently you'd:
>     qcom_ethqos_probe()
>         ethqos_clks_config(ethqos, true)
>             ethqos_set_func_clk_en(ethqos)
>                 qcom_ethqos_set_sgmii_loopback(ethqos, true) // loopback enabled
>         ethqos_set_func_clk_en(ethqos)
>             qcom_ethqos_set_sgmii_loopback(ethqos, true) // no change in loopback
>     devm_stmmac_pltfr_probe()
>         stmmac_pltfr_probe()
>             stmmac_drv_probe()
>                 pm_runtime_put()
>     // Eventually runtime PM will then do below
>     stmmac_stmmac_runtime_suspend()
>         stmmac_bus_clks_config(priv, false)
>             ethqos_clks_config(ethqos, false) // pointless branch but proving to myself
>                                               // that pm_runtime isn't getting in the way here
>     __stmmac_open()
>         stmmac_runtime_resume()
>             ethqos_clks_config(ethqos, true)
>                 ethqos_set_func_clk_en(ethqos)
>                     qcom_ethqos_set_sgmii_loopback(ethqos, true) // no change in loopback
>     stmmac_mac_link_up()
>         ethqos_fix_mac_speed()
>             qcom_ethqos_set_sgmii_loopback(ethqos, false); // loopback disabled
>
> Good chance I foobared tracing that... but!
> That seems to still go against Russell's comment, i.e. its on at probe
> and remains on until a link is up. This doesn't add anymore stmmac wide
> platform callbacks at least, but I'm still concerned based on his prior
> comments.
>
> Its not clear to me though if the "2500basex" mentioned here supports
> any in-band signalling from a Qualcomm SoC POV (not just the Aquantia
> phy its attached to, but in general). So maybe in that case its not a
> concern?
>
> Although, this isn't tied to _just_ 2500basex here. If I boot the
> sa8775p-ride (r2 version, with a marvell 88ea1512 phy attached via
> sgmii, not indicating 2500basex) wouldn't all this get exercised? Right
> now the devicetree doesn't indicate inband signalling, but I tried that
> over here with Russell's clean up a week or two ago and things at least
> came up ok (which made me think all the INTEGRATED_PCS stuff wasn't needed,
> and I'm not totally positive my test proved inband signalling worked,
> but I thought it did...):
>

Am I getting this right? You added `managed = "in-band-status"' to
Rev2 DTS and it still worked?

>     https://lore.kernel.org/netdev/zzevmhmwxrhs5yfv5srvcjxrue2d7wu7vjqmmoyd5mp6kgur54@jvmuv7bxxhqt/
>
> based on Russell's comments, I feel if I was to use his series over
> there, add 'managed = "in-band-status"' to the dts, and then apply this
> series, the link would not come up anymore.
>

Because I can confirm that it doesn't on Rev 3. :(

So to explain myself: I tried to follow Andrew Lunn's suggestion about
unifying this and the existing ethqos_set_func_clk_en() bits as they
seem to address a similar issue.

I'm working with limited information here as well regarding this issue
so I figured this could work but you're right - if we ever need
in-band signalling, then it won't work. It's late here so let me get
back to it tomorrow.

> Total side note, but I'm wondering if the sa8775p-ride dts should
> specify 'managed = "in-band-status"'.
>

I'll check this at the source.

Bart

> Thanks,
> Andrew
>

Powered by blists - more mailing lists