| Message ID | f2a14edb5761d372ec939ccbea4fb8dfd1fdab91.1731685185.git.pnewman@connecttech.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | net: stmmac: dwmac-tegra: Read iommu stream id from device tree | expand |
On 11/15/24 17:31, Parker Newman wrote: > From: Parker Newman <pnewman@connecttech.com> > > Read the iommu stream id from device tree rather than hard coding to mgbe0. > Fixes kernel panics when using mgbe controllers other than mgbe0. It's better to include the full Oops backtrace, possibly decoded. > Tested with Orin AGX 64GB module on Connect Tech Forge carrier board. Since this looks like a fix, you should include a suitable 'Fixes' tag here, and specify the 'net' target tree in the subj prefix. > @@ -241,6 +243,12 @@ static int tegra_mgbe_probe(struct platform_device *pdev) > if (IS_ERR(mgbe->xpcs)) > return PTR_ERR(mgbe->xpcs); > > + /* get controller's stream id from iommu property in device tree */ > + if (!tegra_dev_iommu_get_stream_id(mgbe->dev, &mgbe->iommu_sid)) { > + dev_err(mgbe->dev, "failed to get iommu stream id\n"); > + return -EINVAL; > + } I *think* it would be better to fallback (possibly with a warning or notice) to the previous default value when the device tree property is not available, to avoid regressions. Thanks, Paolo
On Fri, 15 Nov 2024 18:17:07 +0100 Paolo Abeni <pabeni@redhat.com> wrote: > On 11/15/24 17:31, Parker Newman wrote: > > From: Parker Newman <pnewman@connecttech.com> > > > > Read the iommu stream id from device tree rather than hard coding to mgbe0. > > Fixes kernel panics when using mgbe controllers other than mgbe0. > > It's better to include the full Oops backtrace, possibly decoded. > Will do, there are many different ones but I can add the most common. > > Tested with Orin AGX 64GB module on Connect Tech Forge carrier board. > > Since this looks like a fix, you should include a suitable 'Fixes' tag > here, and specify the 'net' target tree in the subj prefix. > Sorry I missed the "net" tag. The bug has existed since dwmac-tegra.c was added. I can add a Fixes tag but in the past I was told they aren't needed in that situation? > > @@ -241,6 +243,12 @@ static int tegra_mgbe_probe(struct platform_device *pdev) > > if (IS_ERR(mgbe->xpcs)) > > return PTR_ERR(mgbe->xpcs); > > > > + /* get controller's stream id from iommu property in device tree */ > > + if (!tegra_dev_iommu_get_stream_id(mgbe->dev, &mgbe->iommu_sid)) { > > + dev_err(mgbe->dev, "failed to get iommu stream id\n"); > > + return -EINVAL; > > + } > > I *think* it would be better to fallback (possibly with a warning or > notice) to the previous default value when the device tree property is > not available, to avoid regressions. > I debated this as well... In theory the iommu must be setup for the mgbe controller to work anyways. Doing it this way means the worst case is probe() fails and you lose an ethernet port. Having it fall back to mgbe0's SID adds the risk of the entire system crashing. I can see arguments for both methods. I can add the fallback to mgbe0's SID and change the message to a warning when I send V2 if you like. Thanks! Parker > Thanks, > > Paolo >
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c index 3827997d2132..dc903b846b1b 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c @@ -1,4 +1,5 @@ // SPDX-License-Identifier: GPL-2.0-only +#include <linux/iommu.h> #include <linux/platform_device.h> #include <linux/of.h> #include <linux/module.h> @@ -19,6 +20,8 @@ struct tegra_mgbe { struct reset_control *rst_mac; struct reset_control *rst_pcs; + u32 iommu_sid; + void __iomem *hv; void __iomem *regs; void __iomem *xpcs; @@ -50,7 +53,6 @@ struct tegra_mgbe { #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 int __maybe_unused tegra_mgbe_suspend(struct device *dev) { @@ -84,7 +86,7 @@ static int __maybe_unused tegra_mgbe_resume(struct device *dev) writel(MAC_SBD_INTR, mgbe->regs + MGBE_WRAP_COMMON_INTR_ENABLE); /* Program SID */ - writel(MGBE_SID, mgbe->hv + MGBE_WRAP_AXI_ASID0_CTRL); + writel(mgbe->iommu_sid, mgbe->hv + MGBE_WRAP_AXI_ASID0_CTRL); value = readl(mgbe->xpcs + XPCS_WRAP_UPHY_STATUS); if ((value & XPCS_WRAP_UPHY_STATUS_TX_P_UP) == 0) { @@ -241,6 +243,12 @@ static int tegra_mgbe_probe(struct platform_device *pdev) if (IS_ERR(mgbe->xpcs)) return PTR_ERR(mgbe->xpcs); + /* get controller's stream id from iommu property in device tree */ + if (!tegra_dev_iommu_get_stream_id(mgbe->dev, &mgbe->iommu_sid)) { + dev_err(mgbe->dev, "failed to get iommu stream id\n"); + return -EINVAL; + } + res.addr = mgbe->regs; res.irq = irq; @@ -346,7 +354,7 @@ static int tegra_mgbe_probe(struct platform_device *pdev) writel(MAC_SBD_INTR, mgbe->regs + MGBE_WRAP_COMMON_INTR_ENABLE); /* Program SID */ - writel(MGBE_SID, mgbe->hv + MGBE_WRAP_AXI_ASID0_CTRL); + writel(mgbe->iommu_sid, mgbe->hv + MGBE_WRAP_AXI_ASID0_CTRL); plat->flags |= STMMAC_FLAG_SERDES_UP_AFTER_PHY_LINKUP;