Re: [PATCH net-next v4 RESEND] stmmac: tegra: Add MGBE support

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 12621 bytes --]

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@nvidia.com>
> > 
> > Add support for the Multi-Gigabit Ethernet (MGBE/XPCS) IP found on
> > NVIDIA Tegra234 SoCs.
> > 
> > Signed-off-by: Bhadram Varka <vbhadram@nvidia.com>
> > Signed-off-by: Thierry Reding <treding@nvidia.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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]