diff mbox series

[net-next,8/8] net: stmmac: qcom-ethqos: add a DMA-reset quirk for sa8775p-ride-r3

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

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/apply fail Patch does not apply to net-next-0

Commit Message

Bartosz Golaszewski June 19, 2024, 6:45 p.m. UTC
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.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 .../stmicro/stmmac/dwmac-qcom-ethqos.c        | 33 +++++++++++++++++++
 1 file changed, 33 insertions(+)

Comments

Andrew Lunn June 19, 2024, 7:33 p.m. UTC | #1
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
Bartosz Golaszewski June 20, 2024, 8:20 a.m. UTC | #2
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
Russell King (Oracle) June 20, 2024, 10:57 a.m. UTC | #3
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.
Russell King (Oracle) June 20, 2024, 11:16 a.m. UTC | #4
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.
Bartosz Golaszewski June 20, 2024, 12:31 p.m. UTC | #5
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
Andrew Lunn June 20, 2024, 7:30 p.m. UTC | #6
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 mbox series

Patch

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;