| Message ID | 20220819122313.40445-1-akhilrajeev@nvidia.com |
|---|---|
| Headers | show |
| Series | Add GPCDMA support to Tegra234 I2C | expand |
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. 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.
> 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;
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().
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.
> 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(). 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. 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. Regards, Akhil
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. > >> 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, > >> 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.
> 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
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. >>>> 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.
> 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. DMA module does get loaded. I2C probe eventually succeeds and can use DMA then. But my idea here is to avoid blocking of I2C transfers until DMA is available. I am looking for a way that I2C can work in PIO mode until DMA comes up and then switch to DMA once it is available. I am hoping that it would help other I2C dependent drivers as well during boot. > > 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. I am somehow still not very much inclined to add GPCDMA as a dependency for I2C. Even if we discard the fact that the driver extends to support chips that does not have GPCDMA, I would look at DMA as an additional feature only and not a dependency. Regards, Akhil
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