Date:   Tue, 27 Sep 2022 17:33:16 +0200
From:   Thierry Reding <thierry.reding@...il.com>
To:     Paolo Abeni <pabeni@...hat.com>,
        Bhadram Varka <vbhadram@...dia.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Jon Hunter <jonathanh@...dia.com>, linux-tegra@...r.kernel.org,
        netdev@...r.kernel.org
Subject: Re: [PATCH net-next v4 RESEND] stmmac: tegra: Add MGBE support

On Tue, Sep 27, 2022 at 11:29:32AM +0200, Paolo Abeni wrote:
> On Fri, 2022-09-23 at 13:49 +0200, Thierry Reding wrote:
> > From: Bhadram Varka <vbhadram@...dia.com>
> > 
> > Add support for the Multi-Gigabit Ethernet (MGBE/XPCS) IP found on
> > NVIDIA Tegra234 SoCs.
> > 
> > Signed-off-by: Bhadram Varka <vbhadram@...dia.com>
> > Signed-off-by: Thierry Reding <treding@...dia.com>
> > ---
> >  drivers/net/ethernet/stmicro/stmmac/Kconfig   |   6 +
> >  drivers/net/ethernet/stmicro/stmmac/Makefile  |   1 +
> >  .../net/ethernet/stmicro/stmmac/dwmac-tegra.c | 290 ++++++++++++++++++
> >  3 files changed, 297 insertions(+)
> >  create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> > index 31ff35174034..e9f61bdaf7c4 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
> > +++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> > @@ -235,6 +235,12 @@ config DWMAC_INTEL_PLAT
> >  	  the stmmac device driver. This driver is used for the Intel Keem Bay
> >  	  SoC.
> >  
> > +config DWMAC_TEGRA
> > +	tristate "NVIDIA Tegra MGBE support"
> > +	depends on ARCH_TEGRA || COMPILE_TEST
> > +	help
> > +	  Support for the MGBE controller found on Tegra SoCs.
> > +
> >  config DWMAC_VISCONTI
> >  	tristate "Toshiba Visconti DWMAC support"
> >  	default ARCH_VISCONTI
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile
> > index d4e12e9ace4f..057e4bab5c08 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/Makefile
> > +++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
> > @@ -31,6 +31,7 @@ obj-$(CONFIG_DWMAC_DWC_QOS_ETH)	+= dwmac-dwc-qos-eth.o
> >  obj-$(CONFIG_DWMAC_INTEL_PLAT)	+= dwmac-intel-plat.o
> >  obj-$(CONFIG_DWMAC_GENERIC)	+= dwmac-generic.o
> >  obj-$(CONFIG_DWMAC_IMX8)	+= dwmac-imx.o
> > +obj-$(CONFIG_DWMAC_TEGRA)	+= dwmac-tegra.o
> >  obj-$(CONFIG_DWMAC_VISCONTI)	+= dwmac-visconti.o
> >  stmmac-platform-objs:= stmmac_platform.o
> >  dwmac-altr-socfpga-objs := altr_tse_pcs.o dwmac-socfpga.o
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c
> > new file mode 100644
> > index 000000000000..bb4b540820fa
> > --- /dev/null
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c
> > @@ -0,0 +1,290 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +#include <linux/platform_device.h>
> > +#include <linux/of_device.h>
> > +#include <linux/module.h>
> > +#include <linux/stmmac.h>
> > +#include <linux/clk.h>
> > +
> > +#include "stmmac_platform.h"
> > +
> > +static const char *const mgbe_clks[] = {
> > +	"rx-pcs", "tx", "tx-pcs", "mac-divider", "mac", "mgbe", "ptp-ref", "mac"
> > +};
> > +
> > +struct tegra_mgbe {
> > +	struct device *dev;
> > +
> > +	struct clk_bulk_data *clks;
> > +
> > +	struct reset_control *rst_mac;
> > +	struct reset_control *rst_pcs;
> > +
> > +	void __iomem *hv;
> > +	void __iomem *regs;
> > +	void __iomem *xpcs;
> > +
> > +	struct mii_bus *mii;
> > +};
> > +
> > +#define XPCS_WRAP_UPHY_RX_CONTROL 0x801c
> > +#define XPCS_WRAP_UPHY_RX_CONTROL_RX_SW_OVRD BIT(31)
> > +#define XPCS_WRAP_UPHY_RX_CONTROL_RX_PCS_PHY_RDY BIT(10)
> > +#define XPCS_WRAP_UPHY_RX_CONTROL_RX_CDR_RESET BIT(9)
> > +#define XPCS_WRAP_UPHY_RX_CONTROL_RX_CAL_EN BIT(8)
> > +#define XPCS_WRAP_UPHY_RX_CONTROL_RX_SLEEP (BIT(7) | BIT(6))
> > +#define XPCS_WRAP_UPHY_RX_CONTROL_AUX_RX_IDDQ BIT(5)
> > +#define XPCS_WRAP_UPHY_RX_CONTROL_RX_IDDQ BIT(4)
> > +#define XPCS_WRAP_UPHY_RX_CONTROL_RX_DATA_EN BIT(0)
> > +#define XPCS_WRAP_UPHY_HW_INIT_CTRL 0x8020
> > +#define XPCS_WRAP_UPHY_HW_INIT_CTRL_TX_EN BIT(0)
> > +#define XPCS_WRAP_UPHY_HW_INIT_CTRL_RX_EN BIT(2)
> > +#define XPCS_WRAP_UPHY_STATUS 0x8044
> > +#define XPCS_WRAP_UPHY_STATUS_TX_P_UP BIT(0)
> > +#define XPCS_WRAP_IRQ_STATUS 0x8050
> > +#define XPCS_WRAP_IRQ_STATUS_PCS_LINK_STS BIT(6)
> > +
> > +#define XPCS_REG_ADDR_SHIFT 10
> > +#define XPCS_REG_ADDR_MASK 0x1fff
> > +#define XPCS_ADDR 0x3fc
> > +
> > +#define MGBE_WRAP_COMMON_INTR_ENABLE	0x8704
> > +#define MAC_SBD_INTR			BIT(2)
> > +#define MGBE_WRAP_AXI_ASID0_CTRL	0x8400
> > +#define MGBE_SID			0x6
> > +
> > +static void mgbe_uphy_lane_bringup(struct tegra_mgbe *mgbe)
> > +{
> > +	unsigned int retry = 300;
> > +	u32 value;
> > +	int err;
> > +
> > +	value = readl(mgbe->xpcs + XPCS_WRAP_UPHY_STATUS);
> > +	if ((value & XPCS_WRAP_UPHY_STATUS_TX_P_UP) == 0) {
> > +		value = readl(mgbe->xpcs + XPCS_WRAP_UPHY_HW_INIT_CTRL);
> > +		value |= XPCS_WRAP_UPHY_HW_INIT_CTRL_TX_EN;
> > +		writel(value, mgbe->xpcs + XPCS_WRAP_UPHY_HW_INIT_CTRL);
> > +	}
> > +
> > +	err = readl_poll_timeout(mgbe->xpcs + XPCS_WRAP_UPHY_HW_INIT_CTRL, value,
> > +				 (value & XPCS_WRAP_UPHY_HW_INIT_CTRL_TX_EN) == 0,
> > +				 500, 500 * 2000);
> > +	if (err < 0)
> > +		dev_err(mgbe->dev, "timeout waiting for TX lane to become enabled\n");
> 
> Why you don't need to propagate this error to the caller?
> 
> Same question for more error cases below.

I suspect that we can simply propagate the error in these cases. We
never run into these issues in practice, so it went unnoticed.

> 
> > +
> > +	usleep_range(10000, 20000);
> > +
> > +	value = readl(mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
> > +	value |= XPCS_WRAP_UPHY_RX_CONTROL_RX_SW_OVRD;
> > +	writel(value, mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
> > +
> > +	value = readl(mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
> > +	value &= ~XPCS_WRAP_UPHY_RX_CONTROL_RX_IDDQ;
> > +	writel(value, mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
> > +
> > +	value = readl(mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
> > +	value &= ~XPCS_WRAP_UPHY_RX_CONTROL_AUX_RX_IDDQ;
> > +	writel(value, mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
> > +
> > +	value = readl(mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
> > +	value &= ~XPCS_WRAP_UPHY_RX_CONTROL_RX_SLEEP;
> > +	writel(value, mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
> > +
> > +	value = readl(mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
> > +	value |= XPCS_WRAP_UPHY_RX_CONTROL_RX_CAL_EN;
> > +	writel(value, mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
> > +
> > +	err = readl_poll_timeout(mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL, value,
> > +				 (value & XPCS_WRAP_UPHY_RX_CONTROL_RX_CAL_EN) == 0,
> > +				 1000, 1000 * 2000);
> > +	if (err < 0)
> > +		dev_err(mgbe->dev, "timeout waiting for RX calibration to become enabled\n");
> > +
> > +	value = readl(mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
> > +	value |= XPCS_WRAP_UPHY_RX_CONTROL_RX_DATA_EN;
> > +	writel(value, mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
> > +
> > +	value = readl(mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
> > +	value |= XPCS_WRAP_UPHY_RX_CONTROL_RX_CDR_RESET;
> > +	writel(value, mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
> > +
> > +	value = readl(mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
> > +	value &= ~XPCS_WRAP_UPHY_RX_CONTROL_RX_CDR_RESET;
> > +	writel(value, mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
> > +
> > +	value = readl(mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
> > +	value |= XPCS_WRAP_UPHY_RX_CONTROL_RX_PCS_PHY_RDY;
> > +	writel(value, mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
> > +
> > +	while (--retry) {
> > +		err = readl_poll_timeout(mgbe->xpcs + XPCS_WRAP_IRQ_STATUS, value,
> > +					 value & XPCS_WRAP_IRQ_STATUS_PCS_LINK_STS,
> > +					 500, 500 * 2000);
> > +		if (err < 0) {
> > +			dev_err(mgbe->dev, "timeout waiting for link to become ready\n");
> > +			usleep_range(10000, 20000);
> > +			continue;
> > +		}
> > +		break;
> > +	}
> 
> It looks like the above loop can take up to 150 seconds (300
> iterations, 500000usec each), can it be shortned?

This is likely left-over from debugging. It might be possible to get rid
of the loop altogether and just use the built-in retry mechanism from
readl_poll_timeout().

Bhadram, do you have any concerns with removing the outer while loop
here? In your experience, if the link doesn't become ready within the 1
second timeout of the readl_poll_timeout() call, is it at all likely to
succeed subsequently or can we safely assume that something has gone
wrong?

> 
> > +
> > +	/* clear status */
> > +	writel(value, mgbe->xpcs + XPCS_WRAP_IRQ_STATUS);
> > +}
> > +
> > +static int tegra_mgbe_probe(struct platform_device *pdev)
> > +{
> > +	struct plat_stmmacenet_data *plat;
> > +	struct stmmac_resources res;
> > +	struct tegra_mgbe *mgbe;
> > +	int irq, err, i;
> > +
> > +	mgbe = devm_kzalloc(&pdev->dev, sizeof(*mgbe), GFP_KERNEL);
> > +	if (!mgbe)
> > +		return -ENOMEM;
> > +
> > +	mgbe->dev = &pdev->dev;
> > +
> > +	memset(&res, 0, sizeof(res));
> > +
> > +	irq = platform_get_irq(pdev, 0);
> > +	if (irq < 0)
> > +		return irq;
> > +
> > +	mgbe->hv = devm_platform_ioremap_resource_byname(pdev, "hypervisor");
> > +	if (IS_ERR(mgbe->hv))
> > +		return PTR_ERR(mgbe->hv);
> > +
> > +	mgbe->regs = devm_platform_ioremap_resource_byname(pdev, "mac");
> > +	if (IS_ERR(mgbe->regs))
> > +		return PTR_ERR(mgbe->regs);
> > +
> > +	mgbe->xpcs = devm_platform_ioremap_resource_byname(pdev, "xpcs");
> > +	if (IS_ERR(mgbe->xpcs))
> > +		return PTR_ERR(mgbe->xpcs);
> > +
> > +	res.addr = mgbe->regs;
> > +	res.irq = irq;
> > +
> > +	mgbe->clks = devm_kzalloc(&pdev->dev, sizeof(*mgbe->clks), GFP_KERNEL);
> > +	if (!mgbe->clks)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i <  ARRAY_SIZE(mgbe_clks); i++)
> > +		mgbe->clks[i].id = mgbe_clks[i];
> > +
> > +	err = devm_clk_bulk_get(mgbe->dev, ARRAY_SIZE(mgbe_clks), mgbe->clks);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	err = clk_bulk_prepare_enable(ARRAY_SIZE(mgbe_clks), mgbe->clks);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	/* Perform MAC reset */
> > +	mgbe->rst_mac = devm_reset_control_get(&pdev->dev, "mac");
> > +	if (IS_ERR(mgbe->rst_mac))
> > +		return PTR_ERR(mgbe->rst_mac);
> > +
> > +	err = reset_control_assert(mgbe->rst_mac);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	usleep_range(2000, 4000);
> > +
> > +	err = reset_control_deassert(mgbe->rst_mac);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	/* Perform PCS reset */
> > +	mgbe->rst_pcs = devm_reset_control_get(&pdev->dev, "pcs");
> > +	if (IS_ERR(mgbe->rst_pcs))
> > +		return PTR_ERR(mgbe->rst_pcs);
> > +
> > +	err = reset_control_assert(mgbe->rst_pcs);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	usleep_range(2000, 4000);
> > +
> > +	err = reset_control_deassert(mgbe->rst_pcs);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	plat = stmmac_probe_config_dt(pdev, res.mac);
> > +	if (IS_ERR(plat))
> > +		return PTR_ERR(plat);
> > +
> > +	plat->has_xgmac = 1;
> > +	plat->tso_en = 1;
> > +	plat->pmt = 1;
> > +	plat->bsp_priv = mgbe;
> > +
> > +	if (!plat->mdio_node)
> > +		plat->mdio_node = of_get_child_by_name(pdev->dev.of_node, "mdio");
> > +
> > +	if (!plat->mdio_bus_data) {
> > +		plat->mdio_bus_data = devm_kzalloc(&pdev->dev, sizeof(*plat->mdio_bus_data),
> > +						   GFP_KERNEL);
> > +		if (!plat->mdio_bus_data) {
> > +			err = -ENOMEM;
> > +			goto remove;
> > +		}
> > +	}
> > +
> > +	plat->mdio_bus_data->needs_reset = true;
> > +
> > +	mgbe_uphy_lane_bringup(mgbe);
> > +
> > +	/* Tx FIFO Size - 128KB */
> > +	plat->tx_fifo_size = 131072;
> > +	/* Rx FIFO Size - 192KB */
> > +	plat->rx_fifo_size = 196608;
> > +
> > +	/* Enable common interrupt at wrapper level */
> > +	writel(MAC_SBD_INTR, mgbe->regs + MGBE_WRAP_COMMON_INTR_ENABLE);
> > +
> > +	/* Program SID */
> > +	writel(MGBE_SID, mgbe->hv + MGBE_WRAP_AXI_ASID0_CTRL);
> > +
> > +	err = stmmac_dvr_probe(&pdev->dev, plat, &res);
> > +	if (err < 0)
> > +		goto remove;
> > +
> > +	return 0;
> > +
> > +remove:
> > +	stmmac_remove_config_dt(pdev, plat);
> > +	return err;
> > +}
> > +
> > +static int tegra_mgbe_remove(struct platform_device *pdev)
> > +{
> > +	struct tegra_mgbe *mgbe = get_stmmac_bsp_priv(&pdev->dev);
> > +
> > +	clk_bulk_disable_unprepare(ARRAY_SIZE(mgbe_clks), mgbe->clks);
> > +
> > +	stmmac_pltfr_remove(pdev);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id tegra_mgbe_match[] = {
> > +	{ .compatible = "nvidia,tegra234-mgbe", },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, tegra_mgbe_match);
> 
> The DT bindings will land in 6.1, right?

Yes, the DT bindings and the device tree changes for Jetson AGX Orin
are currently targetted for 6.1.

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists