Re: [PATCH RESEND 1/2] i2c: tegra: Add GPCDMA support

[Dmitry Osipenko]

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.