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

[Akhil R]

> On 8/23/22 11:39, Dmitry Osipenko wrote:
> > On 8/23/22 08:17, Akhil R wrote:
> >>> 22.08.2022 23:33, Dmitry Osipenko пишет:
> >>>> 22.08.2022 13:29, Akhil R пишет:
> >>>>>> On 8/22/22 09:56, Akhil R wrote:
> >>>>>>>> 19.08.2022 18:15, Dmitry Osipenko пишет:
> >>>>>>>>> 19.08.2022 15:23, Akhil R пишет:
> >>>>>>>>>>      if (of_device_is_compatible(np, "nvidia,tegra210-i2c-vi"))
> >>>>>>>>>>              i2c_dev->is_vi = true;
> >>>>>>>>>> +    else
> >>>>>>>>>> +            i2c_dev->dma_support = !!(of_find_property(np, "dmas",
> >>>>>>>>>> + NULL));
> >>>>>>>>>
> >>>>>>>>> 1. You leak the np returned by of_find_property().
> >>>>>>>>>
> >>>>>>>>> 2. There is device_property_read_bool() for this kind of
> >>>>>>>>> property-exists checks.
> >>>>>>> Okay. I went by the implementation in
> of_dma_request_slave_channel() to
> >>>>>>> check 'dmas'.
> >>>>>>>
> >>>>>>>>>
> >>>>>>>>> 3. If "dmas" is missing in DT, then dma_request_chan() should return
> >>>>>>>>> NULL and everything will work fine. I suppose you haven't tried to
> >>>>>>>>> test this code.
> >>>>>>>>
> >>>>>>>> Although, no. It should return ERR_PTR(-ENODEV) and then you
> should
> >>> check
> >>>>>>>> the return code.
> >>>>>>> Yes. Agree that it is more agnostic to check for ERR_PTR(-ENODEV).
> But
> >>> since I
> >>>>>>> call tegra_init_dma() for every large transfer until DMA is initialized,
> >>> wouldn't
> >>>>>>> it be better to have a flag inside the driver so that we do not have to go
> >>>>>> through
> >>>>>>> so many functions for every attempted DMA transaction to find out
> that
> >>> the
> >>>>>> DT
> >>>>>>> properties don't exist?
> >>>>>>>
> >>>>>>> Shall I just put i2c_dev->dma_support = true here since DMA is
> supported
> >>> by
> >>>>>>> hardware? It would turn false if dma_request_chan() returns something
> >>> other
> >>>>>>> than -EPROBE_DEFER.
> >>>>>>>
> >>>>>>>       if (of_device_is_compatible(np, "nvidia,tegra210-i2c-vi"))
> >>>>>>>               i2c_dev->is_vi = true;
> >>>>>>>  +    else
> >>>>>>>  +            i2c_dev->dma_support = true;
> >>>>>>
> >>>>>> The code already has dma_mode for that. I don't see why another
> variable
> >>>>>> is needed.
> >>>>>>
> >>>>>> Either add new generic dma_request_chan_optional() that will return
> NULL
> >>>>>> if channel is not available and make Tegra I2C driver to use it, or
> >>>>>> handle the error code returned by dma_request_chan().
> >>>>>
> >>>>> Let me elaborate my thoughts.
> >>>>>
> >>>>> The function tegra_i2c_init_dma() is also called inside
> tegra_i2c_xfer_msg() if
> >>>>> DMA is not initialized before, i.e. if (!i2c_dev->dma_buf).
> >>>>
> >>>> This is not true
> >>>>
> >>>> i2c_dev->dma_mode=false if !i2c_dev->dma_buf and that's it
> >>>>
> >>>> https://elixir.bootlin.com/linux/v6.0-rc2/source/drivers/i2c/busses/i2c-
> >>> tegra.c#L1253
> >>>>
> >>>> tegra_i2c_init_dma() is invoked only during probe
> >>>>
> >>>>> So, if suppose there is no DT entry for dmas, the driver would have to go
> take
> >>> the
> >>>>> path tegra_i2c_init_dma() -> dma_request_chan() -> of_*() apis -> ... and
> >>> then figure
> >>>>> out that DMA is not supported. This would happen for each transfer of
> size
> >>> larger than
> >>>>> I2C_PIO_MODE_PREFERRED_LEN.
> >>>>>
> >>>>> To avoid this, I am looking for a variable/flag which can indicate if the
> driver
> >>> should attempt
> >>>>> to configure DMA or not. I didn't quite get the idea if dma_mode can be
> >>> extended to support
> >>>>> this, because it is updated based on xfer_size on each transfer. My idea of
> >>> i2c_dev->dma_support
> >>>>> is that it will be constant after the probe().
> >>>
> >>> I see now that it's you added tegra_i2c_init_dma() to
> >>> tegra_i2c_xfer_msg(). And tegra_i2c_init_dma() already falls back to PIO
> >>> if DMA is unavailable.
> >>>
> >>> I don't remember why !IS_ENABLED(CONFIG_TEGRA20_APB_DMA) was
> added
> >>> to
> >>> tegra_i2c_init_dma(), but if dma_request_chan() returns -EPROBE_DEFER
> >>> when there is no DMA channel available at all, then you should fix it.
> >>>
> >>> Trying to initialize DMA during transfer if it failed to initialize
> >>> during probe is a wrong approach. DMA must be initialized only once
> >>> during probe. Please make the probe to work properly.
> >>
> >> 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.

> >
> >> 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.

Regards,
Akhil