| Message ID | 20240619184550.34524-9-brgl@bgdev.pl (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: support 2.5G ethernet in dwmac-qcom-ethqos | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next-0 |
On Wed, Jun 19, 2024 at 08:45:49PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.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 provided callbacks to do it for > this board. How does this differ to ethqos_clks_config()? Andrew
On Wed, Jun 19, 2024 at 9:33 PM Andrew Lunn <andrew@lunn.ch> wrote: > > On Wed, Jun 19, 2024 at 08:45:49PM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.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 provided callbacks to do it for > > this board. > > How does this differ to ethqos_clks_config()? > I'm not sure I understand the question. This function is called at probe/remove and suspend/resume. It's not linked to the issue solved here. Bart
On Wed, Jun 19, 2024 at 08:45:49PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.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 provided callbacks to do it for > this board. 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. > +static void qcom_ethqos_set_serdes_loopback(struct qcom_ethqos *ethqos, > + bool enable) > +{ > + rgmii_updatel(ethqos, > + SGMII_PHY_CNTRL1_SGMII_TX_TO_RX_LOOPBACK_EN, > + enable ? SGMII_PHY_CNTRL1_SGMII_TX_TO_RX_LOOPBACK_EN : 0, > + EMAC_WRAPPER_SGMII_PHY_CNTRL1); > +} > + > +static void qcom_ethqos_open(struct net_device *pdev, void *priv) > +{ > + struct qcom_ethqos *ethqos = priv; > + > + qcom_ethqos_set_serdes_loopback(ethqos, true); > +} > + > +static void qcom_ethqos_link_up(struct net_device *ndev, void *priv) > +{ > + struct qcom_ethqos *ethqos = priv; > + > + qcom_ethqos_set_serdes_loopback(ethqos, false); > +} > + 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. I don't have time to go through stmmac and make any suggestions (sorry) so I can only to say NAK to this change.
On Thu, Jun 20, 2024 at 11:57:20AM +0100, Russell King (Oracle) wrote: > I don't have time to go through stmmac and make any suggestions (sorry) > so I can only to say NAK to this change. Also... where is the cover message? I don't have it, so I don't have the context behind your patch series - and I haven't received all the patches either.
On Thu, Jun 20, 2024 at 1:16 PM Russell King (Oracle) <linux@armlinux.org.uk> wrote: > > On Thu, Jun 20, 2024 at 11:57:20AM +0100, Russell King (Oracle) wrote: > > I don't have time to go through stmmac and make any suggestions (sorry) > > so I can only to say NAK to this change. > > Also... where is the cover message? I don't have it, so I don't have the > context behind your patch series - and I haven't received all the > patches either. > It's in lore alright: https://lore.kernel.org/netdev/20240619184550.34524-1-brgl@bgdev.pl/ You were in TO for all the patches (as evident from lore) so maybe spam folder? Bart
On Thu, Jun 20, 2024 at 10:20:08AM +0200, Bartosz Golaszewski wrote: > On Wed, Jun 19, 2024 at 9:33 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > On Wed, Jun 19, 2024 at 08:45:49PM +0200, Bartosz Golaszewski wrote: > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.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 provided callbacks to do it for > > > this board. > > > > How does this differ to ethqos_clks_config()? > > > > I'm not sure I understand the question. This function is called at > probe/remove and suspend/resume. It's not linked to the issue solved > here. /* Enable functional clock to prevent DMA reset to timeout due * to lacking PHY clock after the hardware block has been power * cycled. The actual configuration will be adjusted once * ethqos_fix_mac_speed() is invoked. It sounds similar, "DMA reset", "lacking PHY clock". There is also commit 58329b03a5957904fa2b33b3824ed19e7b42c9e9 Author: Romain Gantois <romain.gantois@bootlin.com> Date: Tue Mar 26 14:32:11 2024 +0100 net: stmmac: Signal to PHY/PCS drivers to keep RX clock on There is a reocurring issue with stmmac controllers where the MAC fails to initialize its hardware if an RX clock signal isn't provided on the MAC/PHY link. This causes issues when PHY or PCS devices either go into suspend while cutting the RX clock or do not bring the clock signal up early enough for the MAC to initialize successfully. Set the mac_requires_rxc flag in the stmmac phylink config so that PHY/PCS drivers know to keep the RX clock up at all times. It would be good to explain the big pictures, why these two changes are not sufficient. Andrew
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c index dac91bc72070..ec43449d0252 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c @@ -21,6 +21,7 @@ #define RGMII_IO_MACRO_CONFIG2 0x1C #define RGMII_IO_MACRO_DEBUG1 0x20 #define EMAC_SYSTEM_LOW_POWER_DEBUG 0x28 +#define EMAC_WRAPPER_SGMII_PHY_CNTRL1 0xf4 /* RGMII_IO_MACRO_CONFIG fields */ #define RGMII_CONFIG_FUNC_CLK_EN BIT(30) @@ -79,6 +80,9 @@ #define ETHQOS_MAC_CTRL_SPEED_MODE BIT(14) #define ETHQOS_MAC_CTRL_PORT_SEL BIT(15) +/* EMAC_WRAPPER_SGMII_PHY_CNTRL1 bits */ +#define SGMII_PHY_CNTRL1_SGMII_TX_TO_RX_LOOPBACK_EN BIT(3) + #define SGMII_10M_RX_CLK_DVDR 0x31 struct ethqos_emac_por { @@ -678,6 +682,29 @@ static int ethqos_configure(struct qcom_ethqos *ethqos) return ethqos->configure_func(ethqos); } +static void qcom_ethqos_set_serdes_loopback(struct qcom_ethqos *ethqos, + bool enable) +{ + rgmii_updatel(ethqos, + SGMII_PHY_CNTRL1_SGMII_TX_TO_RX_LOOPBACK_EN, + enable ? SGMII_PHY_CNTRL1_SGMII_TX_TO_RX_LOOPBACK_EN : 0, + EMAC_WRAPPER_SGMII_PHY_CNTRL1); +} + +static void qcom_ethqos_open(struct net_device *pdev, void *priv) +{ + struct qcom_ethqos *ethqos = priv; + + qcom_ethqos_set_serdes_loopback(ethqos, true); +} + +static void qcom_ethqos_link_up(struct net_device *ndev, void *priv) +{ + struct qcom_ethqos *ethqos = priv; + + qcom_ethqos_set_serdes_loopback(ethqos, false); +} + static void ethqos_fix_mac_speed(void *priv, unsigned int speed, unsigned int mode) { struct qcom_ethqos *ethqos = priv; @@ -861,6 +888,12 @@ static int qcom_ethqos_probe(struct platform_device *pdev) if (data->dma_addr_width) plat_dat->host_dma_width = data->dma_addr_width; + if (of_device_is_compatible(np, "qcom,sa8775p-ethqos") && + ethqos->phy_mode == PHY_INTERFACE_MODE_OCSGMII) { + plat_dat->open = qcom_ethqos_open; + plat_dat->link_up = qcom_ethqos_link_up; + } + if (ethqos->serdes_phy) { plat_dat->serdes_powerup = qcom_ethqos_serdes_powerup; plat_dat->serdes_powerdown = qcom_ethqos_serdes_powerdown;