Message-ID: <uad6id6omswjm7e4eqwd75c52sy5pddtxru3bcuxlukhecvj4u@klzgrws24r2q>
Date: Wed, 4 Dec 2024 19:06:30 +0100
From: Thierry Reding <thierry.reding@...il.com>
To: Parker Newman <parker@...est.io>
Cc: Andrew Lunn <andrew@...n.ch>, Paolo Abeni <pabeni@...hat.com>, 
	Alexandre Torgue <alexandre.torgue@...s.st.com>, Jose Abreu <joabreu@...opsys.com>, 
	Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, 
	Maxime Coquelin <mcoquelin.stm32@...il.com>, Jonathan Hunter <jonathanh@...dia.com>, netdev@...r.kernel.org, 
	linux-stm32@...md-mailman.stormreply.com, linux-arm-kernel@...ts.infradead.org, 
	linux-tegra@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Parker Newman <pnewman@...necttech.com>
Subject: Re: [PATCH v1 1/1] net: stmmac: dwmac-tegra: Read iommu stream id
 from device tree

On Wed, Dec 04, 2024 at 11:53:17AM -0500, Parker Newman wrote:
> On Wed, 4 Dec 2024 17:23:53 +0100
> Thierry Reding <thierry.reding@...il.com> wrote:
> 
> > On Tue, Nov 19, 2024 at 02:47:29PM -0500, Parker Newman wrote:
> > > On Tue, 19 Nov 2024 20:18:00 +0100
> > > Andrew Lunn <andrew@...n.ch> wrote:
> > >
> > > > > I think there is some confusion here. I will try to summarize:
> > > > > - Ihe iommu is supported by the Tegra SOC.
> > > > > - The way the mgbe driver is written the iommu DT property is REQUIRED.
> > > >
> > > > If it is required, please also include a patch to
> > > > nvidia,tegra234-mgbe.yaml and make iommus required.
> > > >
> > >
> > > I will add this when I submit a v2 of the patch.
> > >
> > > > > - "iommus" is a SOC DT property and is defined in tegra234.dtsi.
> > > > > - The mgbe device tree nodes in tegra234.dtsi DO have the iommus property.
> > > > > - There are no device tree changes required to to make this patch work.
> > > > > - This patch works fine with existing device trees.
> > > > >
> > > > > I will add the fallback however in case there is changes made to the iommu
> > > > > subsystem in the future.
> > > >
> > > > I would suggest you make iommus a required property and run the tests
> > > > over the existing .dts files.
> > > >
> > > > I looked at the history of tegra234.dtsi. The ethernet nodes were
> > > > added in:
> > > >
> > > > 610cdf3186bc604961bf04851e300deefd318038
> > > > Author: Thierry Reding <treding@...dia.com>
> > > > Date:   Thu Jul 7 09:48:15 2022 +0200
> > > >
> > > >     arm64: tegra: Add MGBE nodes on Tegra234
> > > >
> > > > and the iommus property is present. So the requires is safe.
> > > >
> > > > Please expand the commit message. It is clear from all the questions
> > > > and backwards and forwards, it does not provide enough details.
> > > >
> > >
> > > I will add more details when I submit V2.
> > >
> > > > I just have one open issue. The code has been like this for over 2
> > > > years. Why has it only now started crashing?
> > > >
> > >
> > > It is rare for Nvidia Jetson users to use the mainline kernel. Nvidia
> > > provides a custom kernel package with many out of tree drivers including a
> > > driver for the mgbe controllers.
> > >
> > > Also, while the Orin AGX SOC (tegra234) has 4 instances of the mgbe controller,
> > > the Nvidia Orin AGX devkit only uses mgbe0. Connect Tech has carrier boards
> > > that use 2 or more of the mgbe controllers which is why we found the bug.
> >
> > Correct. Also, this was a really stupid thing that I overlooked. I don't
> > recall the exact circumstances, but I vaguely recall there had been
> > discussions about adding the tegra_dev_iommu_get_stream_id() helper
> > (that this patch uses) around the time that this driver was created. In
> > the midst of all of this I likely forgot to update the driver after the
> > discussions had settled.
> >
> > Anyway, I agree with the conclusion that we don't need a compatibility
> > fallback for this, both because it would be actively wrong to do it and
> > we've had the required IOMMU properties in device tree since the start,
> > so there can't be any regressions caused by this.
> >
> > I don't think it's necessary to make the iommus property required,
> > though, because there's technically no requirement for these devices to
> > be attached to an IOMMU. They usually are, and it's better if they are,
> > but they should be able to work correctly without an IOMMU.
> >
> Thanks for confirming from the Nvidia side! I wasn't sure if they would
> work without the iommu. That said, if you did NOT want to use the iommu
> and removed the iommu DT property then the probe will fail after my patch.
> Would we not need a guard around the writes to MGBE_WRAP_AXI_ASID0_CTRL as well?

Well... frankly, I don't know. There's an override in the memory
controller which we set when a device is attached to an IOMMU. That's
usually how the stream ID is programmed. If we don't do that it will
typically default to a special passthrough stream ID (technically the
firmware can lock down those register and force them to a specific
stream ID all the time). I'm not sure what exactly the impact is of
these ASID registers (there's a few other instances where those are
needed). They are required if you want to use the IOMMU, but I don't
know what their meaning is if the IOMMU is not enabled.

There's also different cases: the IOMMU can be disabled altogether, in
which case no page tables and such exist for any device and no
translation should happen whatsoever. But there's also the case where an
IOMMU is enabled, but certain devices shouldn't attach to them. I should
make it very clear that both of these are not recommended setups and I
don't know if they'll work. And they are mostly untested. I've been
meaning, but haven't found the time, to test some of these cases.

Thierry

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

Powered by blists - more mailing lists