Re: [GIT PULL 1/7] soc/tegra: Changes for v5.20-rc1

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

[-- Attachment #1.1: Type: text/plain, Size: 5995 bytes --]

On Wed, Jul 13, 2022 at 02:14:27PM +0200, Arnd Bergmann wrote:
> On Wed, Jul 13, 2022 at 12:58 PM Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > On Tue, Jul 12, 2022 at 03:27:16PM +0200, Arnd Bergmann wrote:
> > > On Fri, Jul 8, 2022 at 8:56 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> > >
> > > I fear I'm going to skip this for the current merge window. It looks like
> > > the CBB driver you add here would fit into the existing drivers/edac/
> > > subsystem, or at the minimum should have been reviewed by the
> > > corresponding maintainers (added to Cc)  to decide whether it goes
> > > there or not.
> > >
> > > I had not previously seen this driver, but I'll let them have a look first.
> >
> > EDAC looks like it's used primarily for memory controllers, which this
> > is not. But then I also see explicit references to non-memory-controller
> > references in the infrastructure, so perhaps this does fit in there. The
> > CBB driver is primarily a means to provide additional information about
> > runtime errors, so it's not directly a means of discovering the errors
> > (they would be detected anyway and cause a crash) and I don't think we
> > have a means of correcting any of these errors.
> 
> I think this is just a reflection of what other hardware can do:
> most machines only detect memory errors, but the EDAC subsystem
> can work with any type in principle. There are also a lot of
> conditions elsewhere that can be detected but not corrected.
> 
> > I'll ask Sumit to work with the EDAC maintainers on this.
> 
> Thanks
> 
> > > For the other patches, I found two more problems:
> > >
> > > > Bitan Biswas (1):
> > > >       soc/tegra: fuse: Expose Tegra production status
> > >
> > > Please don't just add random attributes in the soc device infrastructure.
> > > This one has a completely generic name but a SoC specific
> > > meaning, and it lacks a description in Documentation/ABI.
> > > Not sure what the right ABI is here, but this is something that needs
> > > to be discussed more broadly when you send a new version.
> >
> > I wasn't aware that the SoC device infrastructure was restricted to only
> > standardized attributes. Looks like there are a few other outliers that
> > add custom attributes: UX500, ARM Integrator and RealView, and OMAP2.
> >
> > Do we have some other place where this kind of thing can be exposed? Or
> > do we just need to come up with some better way of namespacing these?
> > Perhaps it would also be sufficient if all of these were better
> > documented so that people know what to look for on their platform of
> > interest.
> 
> It's not a 100% strict rule, I've just tried to limit it as much as possible,
> and sometimes missed drivers doing it anyway. My main goal here is
> to make things consistent between SoC families, so if one piece of
> information is provided by a number of them, I'd rather have a standard
> attribute, or a common way of encoding this in the existing attributes
> than to have too many custom attributes with similar names.

The major/minor attributes that we have on Tegra SoCs should be easy to
standardize. It seems like those could be fairly common. The other one
that we have is the "platform" one, which I suppose is not as easy to
standardize. I don't recall the exact details, but I think we're mostly
interested in whether or not the platform is simulation or silicon. The
exact simulation value is not something that userspace scripts will look
at, as far as I recall.

Jon, correct me if I'm wrong.

Perhaps this can be deprecated in favour of a more standardized property
that can more easily be implemented on other SoCs.

The production mode is something that is read from a fuse and we expose
those via the nvmem subsystem already. Currently nvmem exposes only a
binary attribute in sysfs that userspace would need to parse and ideally
we'd have something a little easier to work with, but perhaps nvmem can
be enhanced to expose individual cells as separate attributes in some
standard format. We also have some other values in the fuses that we
want to make available to userspace (IDs and that sort of thing), so
it's good that you noticed this now before we would've added even more.

> > > > YueHaibing (1):
> > > >      soc/tegra: fuse: Add missing DMADEVICES dependency
> > >
> > > This one fixes the warning the wrong way: we don't 'select' random
> > > drivers from other subsystems, and selecting the entire
> > > subsystem makes it worse. Just drop the 'select' here and
> > > enable the drivers in the defconfig.
> >
> > This doesn't actually select the DMADEVICES property. It adds a
> > dependency on DMADEVICES and if that is met it will select
> > TEGRA20_APB_DMA.
> 
> My mistake. However, I still think it's wrong to select
> TEGRA20_APB_DMA here, unless there is a build-time
> dependency that prevents it from being compiled otherwise.
> 
> The dmaengine subsystem is meant to abstract the relation
> between the drivers using DMA and those providing the feature,
> the same way we abstract all the other subsystems. The
> fuse driver may only be used on machines that use
> TEGRA20_APB_DMA, but neither the driver code nor
> Kconfig should care about that.

This dependency has existed for quite a while and my recollection is
that we wanted to make this very explicit because the lack of the
TEGRA20_APB_DMA driver makes the FUSE driver completely useless on
Tegra20 and that in turn has a very negative impact on the rest of the
system, so we deemed a default configuration change insufficient.

Perhaps a better way to solve this would be to make TEGRA20_APB_DMA
default to "y" if ARCH_TEGRA_2x_SOC. And then perhaps make the FUSE
driver depend on DMADEVICES. That still wouldn't ensure that we get
SOC_TEGRA_FUSE enabled automatically all the time, but perhaps it'd
document the dependency a bit more explicitly.

Thierry

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel