[PATCH RESEND 0/2] Add GPCDMA support to Tegra234 I2C

[PATCH RESEND 0/2] Add GPCDMA support to Tegra234 I2C

[Akhil R]

Updates in Tegra I2C driver and device tree to support GPCDMA in
Tegra234

Akhil R (2):
  i2c: tegra: Add GPCDMA support
  arm64: tegra: Add GPCDMA support for Tegra234 I2C

 arch/arm64/boot/dts/nvidia/tegra234.dtsi | 32 +++++++++++++++++++
 drivers/i2c/busses/i2c-tegra.c           | 39 +++++++++++-------------
 2 files changed, 50 insertions(+), 21 deletions(-)

-- 
2.17.1

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

[Akhil R]

Update the DMA initialization and checks to support GPCDMA.
Also add a mechanism to fall back to PIO mode, if DMA is
not available or if initialization returns error.

Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
---
 drivers/i2c/busses/i2c-tegra.c | 39 ++++++++++++++++------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 031c78ac42e6..8c4610c78b54 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -188,7 +188,6 @@ enum msg_end_type {
  *		allowing 0 length transfers.
  * @supports_bus_clear: Bus Clear support to recover from bus hang during
  *		SDA stuck low from device for some unknown reasons.
- * @has_apb_dma: Support of APBDMA on corresponding Tegra chip.
  * @tlow_std_mode: Low period of the clock in standard mode.
  * @thigh_std_mode: High period of the clock in standard mode.
  * @tlow_fast_fastplus_mode: Low period of the clock in fast/fast-plus modes.
@@ -215,7 +214,6 @@ struct tegra_i2c_hw_feature {
 	bool has_mst_fifo;
 	const struct i2c_adapter_quirks *quirks;
 	bool supports_bus_clear;
-	bool has_apb_dma;
 	u32 tlow_std_mode;
 	u32 thigh_std_mode;
 	u32 tlow_fast_fastplus_mode;
@@ -253,6 +251,7 @@ struct tegra_i2c_hw_feature {
  * @dma_phys: handle to DMA resources
  * @dma_buf: pointer to allocated DMA buffer
  * @dma_buf_size: DMA buffer size
+ * @dma_support: indicates if DMA can be enabled
  * @dma_mode: indicates active DMA transfer
  * @dma_complete: DMA completion notifier
  * @atomic_mode: indicates active atomic transfer
@@ -289,6 +288,7 @@ struct tegra_i2c_dev {
 
 	bool multimaster_mode;
 	bool atomic_mode;
+	bool dma_support;
 	bool dma_mode;
 	bool msg_read;
 	bool is_dvc;
@@ -443,13 +443,8 @@ static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev)
 	u32 *dma_buf;
 	int err;
 
-	if (!i2c_dev->hw->has_apb_dma || i2c_dev->is_vi)
-		return 0;
-
-	if (!IS_ENABLED(CONFIG_TEGRA20_APB_DMA)) {
-		dev_dbg(i2c_dev->dev, "DMA support not enabled\n");
-		return 0;
-	}
+	if (!i2c_dev->dma_support)
+		return -EOPNOTSUPP;
 
 	chan = dma_request_chan(i2c_dev->dev, "rx");
 	if (IS_ERR(chan)) {
@@ -486,6 +481,7 @@ static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev)
 err_out:
 	tegra_i2c_release_dma(i2c_dev);
 	if (err != -EPROBE_DEFER) {
+		i2c_dev->dma_support = false;
 		dev_err(i2c_dev->dev, "cannot use DMA: %d\n", err);
 		dev_err(i2c_dev->dev, "falling back to PIO\n");
 		return 0;
@@ -1251,7 +1247,16 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 	xfer_size = ALIGN(xfer_size, BYTES_PER_FIFO_WORD);
 
 	i2c_dev->dma_mode = xfer_size > I2C_PIO_MODE_PREFERRED_LEN &&
-			    i2c_dev->dma_buf && !i2c_dev->atomic_mode;
+			   i2c_dev->dma_support && !i2c_dev->atomic_mode;
+
+	/* If DMA is not initialized, initialize it now.
+	 * Fall back to PIO mode, if it fails.
+	 */
+	if (i2c_dev->dma_mode && !i2c_dev->dma_buf) {
+		err = tegra_i2c_init_dma(i2c_dev);
+		if (err)
+			i2c_dev->dma_mode = false;
+	}
 
 	tegra_i2c_config_fifo_trig(i2c_dev, xfer_size);
 
@@ -1473,7 +1478,6 @@ static const struct tegra_i2c_hw_feature tegra20_i2c_hw = {
 	.has_mst_fifo = false,
 	.quirks = &tegra_i2c_quirks,
 	.supports_bus_clear = false,
-	.has_apb_dma = true,
 	.tlow_std_mode = 0x4,
 	.thigh_std_mode = 0x2,
 	.tlow_fast_fastplus_mode = 0x4,
@@ -1497,7 +1501,6 @@ static const struct tegra_i2c_hw_feature tegra30_i2c_hw = {
 	.has_mst_fifo = false,
 	.quirks = &tegra_i2c_quirks,
 	.supports_bus_clear = false,
-	.has_apb_dma = true,
 	.tlow_std_mode = 0x4,
 	.thigh_std_mode = 0x2,
 	.tlow_fast_fastplus_mode = 0x4,
@@ -1521,7 +1524,6 @@ static const struct tegra_i2c_hw_feature tegra114_i2c_hw = {
 	.has_mst_fifo = false,
 	.quirks = &tegra_i2c_quirks,
 	.supports_bus_clear = true,
-	.has_apb_dma = true,
 	.tlow_std_mode = 0x4,
 	.thigh_std_mode = 0x2,
 	.tlow_fast_fastplus_mode = 0x4,
@@ -1545,7 +1547,6 @@ static const struct tegra_i2c_hw_feature tegra124_i2c_hw = {
 	.has_mst_fifo = false,
 	.quirks = &tegra_i2c_quirks,
 	.supports_bus_clear = true,
-	.has_apb_dma = true,
 	.tlow_std_mode = 0x4,
 	.thigh_std_mode = 0x2,
 	.tlow_fast_fastplus_mode = 0x4,
@@ -1569,7 +1570,6 @@ static const struct tegra_i2c_hw_feature tegra210_i2c_hw = {
 	.has_mst_fifo = false,
 	.quirks = &tegra_i2c_quirks,
 	.supports_bus_clear = true,
-	.has_apb_dma = true,
 	.tlow_std_mode = 0x4,
 	.thigh_std_mode = 0x2,
 	.tlow_fast_fastplus_mode = 0x4,
@@ -1593,7 +1593,6 @@ static const struct tegra_i2c_hw_feature tegra186_i2c_hw = {
 	.has_mst_fifo = false,
 	.quirks = &tegra_i2c_quirks,
 	.supports_bus_clear = true,
-	.has_apb_dma = false,
 	.tlow_std_mode = 0x4,
 	.thigh_std_mode = 0x3,
 	.tlow_fast_fastplus_mode = 0x4,
@@ -1617,7 +1616,6 @@ static const struct tegra_i2c_hw_feature tegra194_i2c_hw = {
 	.has_mst_fifo = true,
 	.quirks = &tegra194_i2c_quirks,
 	.supports_bus_clear = true,
-	.has_apb_dma = false,
 	.tlow_std_mode = 0x8,
 	.thigh_std_mode = 0x7,
 	.tlow_fast_fastplus_mode = 0x2,
@@ -1657,6 +1655,8 @@ static void tegra_i2c_parse_dt(struct tegra_i2c_dev *i2c_dev)
 
 	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));
 }
 
 static int tegra_i2c_init_reset(struct tegra_i2c_dev *i2c_dev)
@@ -1789,9 +1789,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 	if (err)
 		return err;
 
-	err = tegra_i2c_init_dma(i2c_dev);
-	if (err)
-		goto release_clocks;
+	tegra_i2c_init_dma(i2c_dev);
 
 	/*
 	 * VI I2C is in VE power domain which is not always ON and not
@@ -1838,7 +1836,6 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 	pm_runtime_disable(i2c_dev->dev);
 
 	tegra_i2c_release_dma(i2c_dev);
-release_clocks:
 	tegra_i2c_release_clocks(i2c_dev);
 
 	return err;
-- 
2.17.1

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

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

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.

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

[Dmitry Osipenko]

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

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

[Akhil R]

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

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

[Dmitry Osipenko]

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

-- 
Best regards,
Dmitry

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

[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).
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().

Regards,
Akhil

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

[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().

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

[Dmitry Osipenko]

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.

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

[Akhil R]

> 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

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

[Dmitry Osipenko]

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

-- 
Best regards,
Dmitry

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

[Dmitry Osipenko]

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.

-- 
Best regards,
Dmitry

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

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

[Dmitry Osipenko]

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.

-- 
Best regards,
Dmitry

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

[Akhil R]

> 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

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5681 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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.

[PATCH RESEND 2/2] arm64: tegra: Add GPCDMA support for Tegra234 I2C

[Akhil R]

Add dma properties to support GPCDMA in Tegra234 I2C

Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
---
 arch/arm64/boot/dts/nvidia/tegra234.dtsi | 32 ++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/arch/arm64/boot/dts/nvidia/tegra234.dtsi b/arch/arm64/boot/dts/nvidia/tegra234.dtsi
index 81a0f599685f..34026ca80b36 100644
--- a/arch/arm64/boot/dts/nvidia/tegra234.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra234.dtsi
@@ -737,6 +737,10 @@
 			clock-names = "div-clk", "parent";
 			resets = <&bpmp TEGRA234_RESET_I2C1>;
 			reset-names = "i2c";
+			dmas = <&gpcdma 21>, <&gpcdma 21>;
+			dma-names = "rx", "tx";
+			iommus = <&smmu_niso0 TEGRA234_SID_GPCDMA>;
+			dma-coherent;
 		};
 
 		cam_i2c: i2c@3180000 {
@@ -752,6 +756,10 @@
 			clock-names = "div-clk", "parent";
 			resets = <&bpmp TEGRA234_RESET_I2C3>;
 			reset-names = "i2c";
+			dmas = <&gpcdma 23>, <&gpcdma 23>;
+			dma-names = "rx", "tx";
+			iommus = <&smmu_niso0 TEGRA234_SID_GPCDMA>;
+			dma-coherent;
 		};
 
 		dp_aux_ch1_i2c: i2c@3190000 {
@@ -767,6 +775,10 @@
 			clock-names = "div-clk", "parent";
 			resets = <&bpmp TEGRA234_RESET_I2C4>;
 			reset-names = "i2c";
+			dmas = <&gpcdma 26>, <&gpcdma 26>;
+			dma-names = "rx", "tx";
+			iommus = <&smmu_niso0 TEGRA234_SID_GPCDMA>;
+			dma-coherent;
 		};
 
 		dp_aux_ch0_i2c: i2c@31b0000 {
@@ -782,6 +794,10 @@
 			clock-names = "div-clk", "parent";
 			resets = <&bpmp TEGRA234_RESET_I2C6>;
 			reset-names = "i2c";
+			dmas = <&gpcdma 30>, <&gpcdma 30>;
+			dma-names = "rx", "tx";
+			iommus = <&smmu_niso0 TEGRA234_SID_GPCDMA>;
+			dma-coherent;
 		};
 
 		dp_aux_ch2_i2c: i2c@31c0000 {
@@ -797,6 +813,10 @@
 			clock-names = "div-clk", "parent";
 			resets = <&bpmp TEGRA234_RESET_I2C7>;
 			reset-names = "i2c";
+			dmas = <&gpcdma 27>, <&gpcdma 27>;
+			dma-names = "rx", "tx";
+			iommus = <&smmu_niso0 TEGRA234_SID_GPCDMA>;
+			dma-coherent;
 		};
 
 		dp_aux_ch3_i2c: i2c@31e0000 {
@@ -812,6 +832,10 @@
 			clock-names = "div-clk", "parent";
 			resets = <&bpmp TEGRA234_RESET_I2C9>;
 			reset-names = "i2c";
+			dmas = <&gpcdma 31>, <&gpcdma 31>;
+			dma-names = "rx", "tx";
+			iommus = <&smmu_niso0 TEGRA234_SID_GPCDMA>;
+			dma-coherent;
 		};
 
 		spi@3270000 {
@@ -1109,6 +1133,10 @@
 			assigned-clock-parents = <&bpmp TEGRA234_CLK_PLLP_OUT0>;
 			resets = <&bpmp TEGRA234_RESET_I2C2>;
 			reset-names = "i2c";
+			dmas = <&gpcdma 22>, <&gpcdma 22>;
+			dma-names = "rx", "tx";
+			iommus = <&smmu_niso0 TEGRA234_SID_GPCDMA>;
+			dma-coherent;
 		};
 
 		gen8_i2c: i2c@c250000 {
@@ -1125,6 +1153,10 @@
 			assigned-clock-parents = <&bpmp TEGRA234_CLK_PLLP_OUT0>;
 			resets = <&bpmp TEGRA234_RESET_I2C8>;
 			reset-names = "i2c";
+			dmas = <&gpcdma 0>, <&gpcdma 0>;
+			dma-names = "rx", "tx";
+			iommus = <&smmu_niso0 TEGRA234_SID_GPCDMA>;
+			dma-coherent;
 		};
 
 		rtc@c2a0000 {
-- 
2.17.1