Message-ID: <YxDEVUq3jbjnOmnI@orome>
Date:   Thu, 1 Sep 2022 16:40:21 +0200
From:   Thierry Reding <thierry.reding@...il.com>
To:     Dmitry Osipenko <dmitry.osipenko@...labora.com>
Cc:     Akhil R <akhilrajeev@...dia.com>,
        Dmitry Osipenko <digetx@...il.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Laxman Dewangan <ldewangan@...dia.com>,
        "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
        "wsa@...nel.org" <wsa@...nel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "christian.koenig@....com" <christian.koenig@....com>,
        "sumit.semwal@...aro.org" <sumit.semwal@...aro.org>,
        "Kartik ." <kkartik@...dia.com>
Subject: Re: [PATCH RESEND 1/2] i2c: tegra: Add GPCDMA support

On Tue, Aug 23, 2022 at 04:32:11PM +0300, Dmitry Osipenko wrote:
> On 8/23/22 15:55, Akhil R wrote:
> ...
> >>>> What I am trying for is to have a mechanism that doesn't halt the i2c
> >> transfers
> >>>> till DMA is available. Also, I do not want to drop DMA because it was
> >> unavailable
> >>>> during probe().
> >>>
> >>> Why is it unavailable? Sounds like you're not packaging kernel properly.
> > Unavailable until initramfs or systemd is started since the module has to be
> > loaded from either of it.
> > 
> >>>
> >>>> This situation is sure to hit if we have I2C driver as built in and DMA driver as a
> >>>> module. In such cases, I2C will never be able to use the DMA.
> >>>
> >>> For Tegra I2C built-in + DMA driver module you should add the dma.ko to
> >>> initramfs and then it will work. This is a common practice for many
> >>> kernel drivers.
> >>>
> >>> It's also similar to a problem with firmware files that must be
> >>> available to drivers during boot,
> > 
> > Isn't the initramfs loaded after the driver initcalls? Wasn't very much clear for me
> > from the code and docs. We did try adding the module in initramfs initially, but
> > couldn't find much of a difference from when it is loaded by systemd in rootfs.
> > Will explore more on this if this really helps.
> 
> It doesn't matter when initramfs is loaded. Tegra I2C should be
> re-probed once DMA driver is ready, that's the point of deferred
> probing. I'd assume that your DMA driver module isn't loading.

One problem we have with this, and it's another part of the reason why
we have the TEGRA20_APB_DMA conditional in there, is that if no DMA
driver is enabled, then the I2C driver will essentially defer probe
indefinitely.

The same would happen if for whatever reason someone was to disable the
DMA engine via status = "disabled" in device tree. And that's not
something we can easily discover, as far as I can tell. Although perhaps
code could be added to discover these kinds of situations.

Both of the above scenarios could also be considered as bugs, I suppose,
and in that case the fix would be to update the configuration and/or the
device tree.

> >>>> Another option I thought about was to request and free DMA channel for
> >> each
> >>>> transfer, which many serial drivers already do. But I am a bit anxious if that
> >> will
> >>>> increase the latency of transfer.
> >>>
> >>> Perhaps all you need to do is to add MODULE_SOFTDEP to Tegra I2C driver
> >>> like we did it for the EMC driver [1].
> >>>
> >>> [1]
> >>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-
> >> next.git/commit/?id=14b43c20c283de36131da0cb44f3170b9ffa7630
> >>>
> >>
> >> Although, probably MODULE_SOFTDEP won't work for a built-in driver. In
> >> that case, change Tegra I2C kconfig to depend on the DMA driver.
> > 
> > Since I2C can work without DMA, wouldn't it limit the flexibility of I2C driver.
> 
> There are kernel configurations that are not worthwhile to support
> because nobody use them in practice. I think this is exactly the case
> here. The TEGRA20_APB_DMA driver dependency created troubles for a long
> time.
> 
> If DMA driver is enabled in kernel config, then you should provide the
> driver module to kernel and it will work.
> 
> If DMA driver is disabled in kernel config, then Tegra I2C driver should
> take that into account. I'm now recalling that this was the reason of
> "!IS_ENABLED(CONFIG_TEGRA20_APB_DMA)" in the code.
> 
> Since all h/w gens now provide DMA support for Tegra I2C, then should be
> better and easier to make DMA a dependency for Tegra I2C and don't
> maintain kernel build configurations that nobody cares about.

This is a suboptimal solution because we have APB DMA for Tegra20
through Tegra210 and GPC DMA for Tegra186 and later. So we'd need to
depend on two drivers and that would then pull in GPC DMA basically on
all generations.

One potential workaround would be to have a fairly elaborate check in
the driver to make sure that for SoC generations that support APB DMA
that that driver is enabled, and for SoC generations that have GPC DMA
that the corresponding driver is enabled. That's quite ugly and it
doesn't solve the status = "disabled" problem, so we'd need that as
well.

Another thing that I've been thinking about is to use the deferred probe
timeout to remedy this. driver_deferred_probe_check_state() can be used
by subsystems to help figure out these kinds of situations. Basically if
we integrated that into dma_request_channel(), this would at some point
(fairly) late into boot return -ETIMEDOUT (or -ENODEV if modules are
disabled). So this would help with status = "disabled" and allow us to
avoid Kconfig dependencies/conditionals. Unfortunately it seems like
that is in the process of being removed, so not sure if that's a long-
term option.

What that doesn't help with is the potentially long delay that probe
deferral can cause, so it means that all I2C devices may not get a
chance to probe until very late into the boot process. We may need to
survey what exactly that means to better judge what to do about it. I
do agree that probe deferral is the right tool for the job, but it may
be prohibitively slow to get I2C working with that.

Another mitigation would be for the driver to keep probing for the DMA
channels in the background. Sort of like an asynchronous probe deferral
of that subset. Similar things were discussed at some point when the
whole fw_devlink and such were hashed out, though at the time I think
the preferred proposal was a notification mechanism.

Thierry

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

Powered by blists - more mailing lists