Message-ID: <4dbf5bcb-76af-53c5-31cf-880debbcebf3@gmail.com>
Date:   Thu, 1 Oct 2020 22:04:30 +0300
From:   Dmitry Osipenko <digetx@...il.com>
To:     Thierry Reding <thierry.reding@...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

...
>> 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.

Interconnect is a generic API.

> 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.

This is true, but only if all these words are applied to the generic APIs.

>> 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.

Regmap is about abstracting accesses to devices that may sit on
different types of buses, like I2C or SPI for example. Or devices that
have a non-trivial registers mapping, or have slow IO and need caching.

I think you meant regmap in regards to protecting IO accesses, but this
is not what regmap is about if IO accesses are atomic by nature.

The tegra20-devfreq functionality is very separated from the rest of the
memory controller, hence there are no conflicts in regards to hardware
accesses, so there is nothing to protect.

Also, Regmap API itself doesn't manage refcounting of the mappings.

> And not
> coincidentally, regmaps are usually passed around by referencing their
> provider via phandle.

Any real-world examples? I think you're mixing up regmap with something
else.

The devfreq driver works just like the SMMU and GART. The devfreq device
is supposed to be created only once both MC and EMC drivers are loaded
and we know that they can't go away [1].

[1]
https://patchwork.ozlabs.org/project/linux-tegra/patch/20200814000621.8415-32-digetx@gmail.com/

Hence the tegra20-devfreq driver is horrible as much as the SMMU and
GART drivers. Perhaps not much could be done about it unless MC driver
is converted to MFD. But MFD won't work for tegra20-devfreq driver
anyways because it depends on presence of both MC and EMC drivers
simultaneously :)

Besides you didn't want the MFD couple years ago [2].

[2]
https://patchwork.ozlabs.org/project/linux-tegra/patch/675f74f82378b5f7d8f61d35e929614a0e156141.1523301400.git.digetx@gmail.com/#1902020

> 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.
The tegra20-devfreq driver doesn't share registers with other drivers.
MC statistics collection is a part of the MC, but it has no connection
to the other functions of the MC, at least from SW perspective.

Apparently you're missing that it's still not a problem to change the
T20 DT because all the MC-related drivers are still inactive in the
upstream kernel and awaiting the interconnect support addition.

Secondly, you're missing that tegra30-devfreq driver will also need to
perform the MC lookup [3] and then driver will no longer work for the
older DTs if phandle becomes mandatory because these DTs do not have the
MC phandle in the ACTMON node.

[3]
https://github.com/grate-driver/linux/commit/441d19281f9b3428a4db1ecb3a02e1ec02a8ad7f

So we will need the fall back for T30/124 as well.

>> 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.

We don't have this problem because MC drivers do not support unloading.
Hence this is not something to really care about for now.

Maybe MC drivers won't ever support unloading and then why do we need to
discuss it at all? Should be better to get back to this once there will
be a real need.

> 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.

But we aren't talking about the uniformity, at least I'm not.

To be honest, it feels to me that you're talking from perspective of
yours generic memory controller API series and want to see everything
tailored for it, which is okay :)

While I'm merely wanting to improve the existing codebase with minimal
efforts and without a need to change DTs.

Alright, I'll consider to add the phandles as a part of the interconnect
series since you're so concentrated on it :) But the series is already
40+ patches.. although, the vast majority of the patches are very
trivial, so maybe okay to add couple more trivial patches. I'm very
hoping that you will manage to allocate time for reviewing of the
upcoming v6 and that it will be good for merging into to the v5.11!

Powered by blists - more mailing lists