01.09.2022 17:40, Thierry Reding пишет: > 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. The case of missing/disabled node needs to be addressed in the DMA API. Add new dma_request_chan_optional(). > 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. You should be right here, Kconfig doesn't support conditional dependencies. > 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. Replicate what is done for APBDMA. if (i2c_dev->hw->has_apb_dma) if (!IS_ENABLED(CONFIG_TEGRA20_APB_DMA)) dev_dbg(i2c_dev->dev, "DMA support not enabled\n"); return 0; } else { if (!IS_ENABLED(CONFIG_TEGRA186_GPC_DMA)) { dev_dbg(i2c_dev->dev, "DMA support not enabled\n"); return 0; } } This will enable GPCDMA. Everything else can be done later on.
