On Thu, Jun 27, 2024 at 7:07 PM Andrew Halaney <ahalaney@xxxxxxxxxx> wrote: > > On Thu, Jun 27, 2024 at 01:39:47PM GMT, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx> > > > > 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@xxxxxxxxxx> > > 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@xxxxxxxxxxxxxxxxxxxxx/ > 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 >
