Message-ID: <20201001075811.GA3919720@ulmo>
Date:   Thu, 1 Oct 2020 09:58:11 +0200
From:   Thierry Reding <thierry.reding@...il.com>
To:     Dmitry Osipenko <digetx@...il.com>
Cc:     Nicolin Chen <nicoleotsuka@...il.com>, joro@...tes.org,
        krzk@...nel.org, vdumpa@...dia.com, jonathanh@...dia.com,
        linux-tegra@...r.kernel.org, iommu@...ts.linux-foundation.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and
 .attach_dev

On Thu, Oct 01, 2020 at 05:11:30AM +0300, Dmitry Osipenko wrote:
> 30.09.2020 19:47, Thierry Reding пишет:
> > On Wed, Sep 30, 2020 at 07:25:41PM +0300, Dmitry Osipenko wrote:
> >> 30.09.2020 19:06, Thierry Reding пишет:
> >>> On Wed, Sep 30, 2020 at 06:36:52PM +0300, Dmitry Osipenko wrote:
> >>>>  I'...
> >>>>>> +	struct tegra_mc *mc = devm_tegra_get_memory_controller(dev);
> >>>>>> +	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> >>>>>
> >>>>> It looks to me like the only reason why you need this new global API is
> >>>>> because PCI devices may not have a device tree node with a phandle to
> >>>>> the IOMMU. However, SMMU support for PCI will only be enabled if the
> >>>>> root complex has an iommus property, right? In that case, can't we
> >>>>> simply do something like this:
> >>>>>
> >>>>> 	if (dev_is_pci(dev))
> >>>>> 		np = find_host_bridge(dev)->of_node;
> >>>>> 	else
> >>>>> 		np = dev->of_node;
> >>>>>
> >>>>> ? I'm not sure exactly what find_host_bridge() is called, but I'm pretty
> >>>>> sure that exists.
> >>>>>
> >>>>> Once we have that we can still iterate over the iommus property and do
> >>>>> not need to rely on this global variable.
> >>>>
> >>>> This sounds more complicated than the current variant.
> >>>>
> >>>> Secondly, I'm already about to use the new tegra_get_memory_controller()
> >>>> API for all the T20/30/124/210 EMC and devfreq drivers.
> >>>
> >>> Why do we need it there? They seem to work fine without it right now.
> >>
> >> All the Tegra30/124/210 EMC drivers are already duplicating that MC
> >> lookup code and only the recent T210 driver does it properly.
> >>
> >>> If it is required for new functionality, we can always make the dependent
> >>> on a DT reference via phandle without breaking any existing code.
> >>
> >> That's correct, it will be also needed for the new functionality as
> >> well, hence even more drivers will need to perform the MC lookup.
> > 
> > I don't have any issues with adding a helper if we need it from several
> > different locations. But the helper should be working off of a given
> > device and look up the device via the device tree node referenced by
> > phandle. We already have those phandles in place for the EMC devices,
> > and any other device that needs to interoperate with the MC should also
> > get such a reference.
> > 
> >> I don't quite understand why you're asking for the phandle reference,
> >> it's absolutely not needed for the MC lookup and won't work for the
> > 
> > We need that phandle in order to establish a link between the devices.
> > Yes, you can probably do it without the phandle and just match by
> > compatible string. But we don't do that for other types of devices
> > either, right? For a display driver we reference the attached panel via
> > phandle, but we could also just look it up via name or absolute path or
> > some other heuristic. But a phandle is just a much more explicit way of
> > linking the devices, so why not use it?
> 
> There are dozens variants of the panels and system could easily have
> more than one panel, hence a direct lookup by phandle is a natural
> choice for the panels.

Not really, there's typically only just one panel. But that's just one
example. EMC would be another. There's only a single EMC on Tegra and
yet for something like interconnects we still reference it by phandle.
PMC is another case and so is CAR, GPIO (at least on early Tegra SoCs)
and pinmux, etc.

The example of GPIO shows very well how this is important. If we had
made the assumption from the beginning that there was only ever going to
be a single GPIO controller, then we would've had a big problem when the
first SoC shipped that had multiple GPIO controllers.

> While all Tegra SoCs have a single fixed MC in the system, and thus,
> there is no real need to use phandle because we can't mix up MC with
> anything else.

The same is true for the SMMU, and yet the iommus property references
the SMMU by phandle. There are a *lot* of cases where you could imply
dependencies because you have intimate knowledge about the hardware
within drivers. But the point is to avoid this wherever possible so
that the DTB is as self-describing as possible.

> >> older DTs if DT change will be needed. Please give a detailed explanation.
> > 
> > New functionality doesn't have to work with older DTs.
> 
> This is fine in general, but I'm afraid that in this particular case we
> will need to have a fall back anyways because otherwise it should break
> the old functionality.

It looks like tegra20-devfreq is the only one that currently does this
lookup via compatible string. And looking at the driver, what it does is
pretty horrible, to be honest. It gets a reference to the memory
controller and then simply accesses registers within the memory
controller without any type of protection against concurrent accesses or
reference counting to make sure the registers it accesses are still
valid. At the very least this should've been a regmap. And not
coincidentally, regmaps are usually passed around by referencing their
provider via phandle.

That's exactly the kind of hack that I want to prevent from happening.
If you can just grab a pointer to the memory controller with a global
function pointer it makes people think that it's okay to use this kind
of shortcut. But it isn't.

Given the above, the lookup-by-compatible fallback should stay limited
to tegra20-devfreq. Everything else should move to something saner. So
this new helper should look up by phandle and not have a fallback, but
instead the tegra20-devfreq should fall back if the new helper doesn't
return anything useful (probably something like -ENOENT, meaning that
there's no phandle and that we're using an old device tree). Bonus
points for updating the DT bindings for tegra20-devfreq to also allow
the memory controller to be specified by phandle and use a regmap for
the shared registers.

> So I don't think that using phandle for the MC device finding is really
> warrant.
> 
> Phandle is kinda more applicable for the cases where only the DT node
> lookup is needed (not the lookup of the MC device driver), but even then
> it is also not mandatory.
> 
> I hope you agree.

I strongly disagree. We look up a bunch of devices via phandle, and in
only very rare cases do we care only about the DT node. When we look up
clocks, resets or GPIOs, we always get some sort of resource handle. It
makes sense to do so because we need to operate on these resources and
having only a DT node doesn't allow us to do that.

Ultimately this is a matter of principle. Yes, it's true that there is
only a single MC on Tegra and we can "cheat" by taking advantage of that
knowledge. But we don't have to, and it's not even very difficult to not
rely on that knowledge. Treating the MC differently from everything else
also makes it more difficult to understand the associated code and on
top of that it sets a bad example because other people seeing this code
will think it's a good idea.

The big advantage of uniformity is that it drastically simplifies things
and causes less suprises.

Thierry

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

Powered by blists - more mailing lists