Re: [PATCH v1] soc/tegra: fuse: Drop Kconfig dependency on TEGRA20_APB_DMA

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

[-- Attachment #1: Type: text/plain, Size: 3491 bytes --]

On Mon, Nov 16, 2020 at 04:48:39PM +0300, Dmitry Osipenko wrote:
> 16.11.2020 16:20, Thierry Reding пишет:
> > On Wed, Sep 23, 2020 at 03:34:21AM +0300, Dmitry Osipenko wrote:
> >> The DMA subsystem could be entirely disabled in Kconfig and then the
> >> TEGRA20_APB_DMA option isn't available too. Hence kernel configuration
> >> fails if DMADEVICES Kconfig option is disabled due to the unsatisfiable
> >> dependency.
> >>
> >> The FUSE driver isn't a critical driver and currently it only provides
> >> NVMEM interface to userspace which isn't known to be widely used, and
> >> thus, it's fine if FUSE driver fails to load.
> > 
> > This isn't entirely accurate. The FUSE driver also provides SKU specific
> > information via tegra_sku_info and exposes some of the FUSE registers to
> > other drivers, such as the calibration data for XUSB.
> 
> The SKU data is read out only once during early boot and the DMA is not
> used for this. The FUSE platform driver exists separately from the early
> FUSE code.

True, but the commit message isn't entirely accurate as-is, because on
later Tegra SoCs the driver does a bit more than that. So if you don't
mind I'll reword this to be a little more accurate if I end up deciding
to apply it.

> > The APB DMA is really only needed to work around an issue on Tegra20, so
> > perhaps this really is okay. On the other hand, could we not just change
> > the dependency to something like
> > 
> > 	select DMADEVICES if ARCH_TEGRA_2x_SOC
> > 	select TEGRA20_APB_DMA if ARCH_TEGRA_2x_SOC
> > 
> > to fix the Kconfig issue but keep the explicit documentation of this
> > dependency?
> 
> On Tegra20, the APB DMA is used only for by NVMEM which is exposed via
> sysfs. If DMA is disabled, then NVMEM isn't registered because
> tegra20_fuse_probe() returns -EPROBE_DEFER.

Again, true. But -EPROBE_DEFER is a silent error, so if somebody indeed
has DMADEVICES disabled and TEGRA20_APB_DMA is not available, then they
will not get FUSE support and they are going to struggle to find out why
that's not working.

> Hence there is no real need to enforce the extra dependencies. It's
> always better to allow minimizing the build dependencies whenever
> possible, IMO, and it's possible to do it in this case.

I don't entirely agree with this. Dependencies (and especially selects)
are used to pull in driver and/or features that are generally considered
useful. In this particular case, TEGRA20_APB_DMA is needed for the FUSE
driver to work correctly on Tegra20. Whether FUSE support on Tegra20 is
useful or not isn't quite relevant at this point.

There's also a balance as to what makes sense and what doesn't. APB DMA
is a useful feature in itself and disabling it is very much discouraged
because there are plenty of other drivers that can make use of it. That
is also the reason why we enable it in the default configuration. So I
don't consider a select on a symbol that's enabled by default an extra
dependency. Instead it's more of an explicit statement that you really
do want this enabled if you want that driver to work.

And like I said, if we don't have this and the driver will fail to probe
because of -EPROBE_DEFER, the user is going to have a very difficult
time of finding out why exactly that's happening. We're not even
emitting an error for this, so there's no way of knowing, even if they
enable driver debugging, /why/ the FUSE driver isn't working.

Thierry

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