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/ 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...): 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. Total side note, but I'm wondering if the sa8775p-ride dts should specify 'managed = "in-band-status"'. Thanks, Andrew
