[PATCH v4 1/3] dt-bindings: i2c: nvidia,tegra20-i2c: Specify the required properties

[PATCH v4 1/3] dt-bindings: i2c: nvidia,tegra20-i2c: Specify the required properties

[Akhil R]

Specify the properties which are essential and which are not for the
Tegra I2C driver to function correctly. This was not added correctly when
the TXT binding was converted to yaml. All the existing DT nodes have
these properties already and hence this does not break the ABI.

dmas and dma-names which were specified as a must in the TXT binding
is now made optional since the driver can work in PIO mode if dmas are
missing.

Fixes: f10a9b722f80 ("dt-bindings: i2c: tegra: Convert to json-schema”)
Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
---
v3->v4:
  * Updated commit message, and "description" section.
v2->v3:
  * Updated commit description on the details and fixed indentation
    issue.
v1->v2:
  * Added all required properties

 .../bindings/i2c/nvidia,tegra20-i2c.yaml      | 24 ++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml b/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml
index b57ae6963e62..10f30d6a1fad 100644
--- a/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml
+++ b/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml
@@ -97,7 +97,10 @@ properties:
 
   resets:
     items:
-      - description: module reset
+      - description:
+          Module reset. This property is optional for controllers in Tegra194,
+          Tegra234 etc where an internal software reset is available as an
+          alternative.
 
   reset-names:
     items:
@@ -116,6 +119,13 @@ properties:
       - const: rx
       - const: tx
 
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+
 allOf:
   - $ref: /schemas/i2c/i2c-controller.yaml
   - if:
@@ -169,6 +179,18 @@ allOf:
       properties:
         power-domains: false
 
+  - if:
+      not:
+        properties:
+          compatible:
+            contains:
+              enum:
+                - nvidia,tegra194-i2c
+    then:
+      required:
+        - resets
+        - reset-names
+
 unevaluatedProperties: false
 
 examples:
-- 
2.49.0

[PATCH v4 2/3] i2c: tegra: make reset an optional property

[Akhil R]

For controllers that has an internal software reset, make the reset
property optional. This is useful in systems that choose to restrict
reset control from Linux.

Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
---
v3->v4: No change
v2->v3: No change
v1->v2:
  * Call devm_reset_control_get_optional_exclusive() unconditionally.
  * Add more delay based on HW recommendation.

 drivers/i2c/busses/i2c-tegra.c | 33 +++++++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 87976e99e6d0..22ddbae9d847 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -134,6 +134,8 @@
 #define I2C_MST_FIFO_STATUS_TX			GENMASK(23, 16)
 #define I2C_MST_FIFO_STATUS_RX			GENMASK(7, 0)
 
+#define I2C_MASTER_RESET_CNTRL			0x0a8
+
 /* configuration load timeout in microseconds */
 #define I2C_CONFIG_LOAD_TIMEOUT			1000000
 
@@ -184,6 +186,9 @@ enum msg_end_type {
  * @has_mst_fifo: The I2C controller contains the new MST FIFO interface that
  *		provides additional features and allows for longer messages to
  *		be transferred in one go.
+ * @has_mst_reset: The I2C controller contains MASTER_RESET_CTRL register which
+ *		provides an alternative to controller reset when configured as
+ *		I2C master
  * @quirks: I2C adapter quirks for limiting write/read transfer size and not
  *		allowing 0 length transfers.
  * @supports_bus_clear: Bus Clear support to recover from bus hang during
@@ -213,6 +218,7 @@ struct tegra_i2c_hw_feature {
 	bool has_multi_master_mode;
 	bool has_slcg_override_reg;
 	bool has_mst_fifo;
+	bool has_mst_reset;
 	const struct i2c_adapter_quirks *quirks;
 	bool supports_bus_clear;
 	bool has_apb_dma;
@@ -604,6 +610,20 @@ static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev)
 	return 0;
 }
 
+static int tegra_i2c_master_reset(struct tegra_i2c_dev *i2c_dev)
+{
+	if (!i2c_dev->hw->has_mst_reset)
+		return -EOPNOTSUPP;
+
+	i2c_writel(i2c_dev, 0x1, I2C_MASTER_RESET_CNTRL);
+	udelay(2);
+
+	i2c_writel(i2c_dev, 0x0, I2C_MASTER_RESET_CNTRL);
+	udelay(2);
+
+	return 0;
+}
+
 static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
 {
 	u32 val, clk_divisor, clk_multiplier, tsu_thd, tlow, thigh, non_hs_mode;
@@ -621,8 +641,10 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
 	 */
 	if (handle)
 		err = acpi_evaluate_object(handle, "_RST", NULL, NULL);
-	else
+	else if (i2c_dev->rst)
 		err = reset_control_reset(i2c_dev->rst);
+	else
+		err = tegra_i2c_master_reset(i2c_dev);
 
 	WARN_ON_ONCE(err);
 
@@ -1467,6 +1489,7 @@ static const struct tegra_i2c_hw_feature tegra20_i2c_hw = {
 	.has_multi_master_mode = false,
 	.has_slcg_override_reg = false,
 	.has_mst_fifo = false,
+	.has_mst_reset = false,
 	.quirks = &tegra_i2c_quirks,
 	.supports_bus_clear = false,
 	.has_apb_dma = true,
@@ -1491,6 +1514,7 @@ static const struct tegra_i2c_hw_feature tegra30_i2c_hw = {
 	.has_multi_master_mode = false,
 	.has_slcg_override_reg = false,
 	.has_mst_fifo = false,
+	.has_mst_reset = false,
 	.quirks = &tegra_i2c_quirks,
 	.supports_bus_clear = false,
 	.has_apb_dma = true,
@@ -1515,6 +1539,7 @@ static const struct tegra_i2c_hw_feature tegra114_i2c_hw = {
 	.has_multi_master_mode = false,
 	.has_slcg_override_reg = false,
 	.has_mst_fifo = false,
+	.has_mst_reset = false,
 	.quirks = &tegra_i2c_quirks,
 	.supports_bus_clear = true,
 	.has_apb_dma = true,
@@ -1539,6 +1564,7 @@ static const struct tegra_i2c_hw_feature tegra124_i2c_hw = {
 	.has_multi_master_mode = false,
 	.has_slcg_override_reg = true,
 	.has_mst_fifo = false,
+	.has_mst_reset = false,
 	.quirks = &tegra_i2c_quirks,
 	.supports_bus_clear = true,
 	.has_apb_dma = true,
@@ -1563,6 +1589,7 @@ static const struct tegra_i2c_hw_feature tegra210_i2c_hw = {
 	.has_multi_master_mode = false,
 	.has_slcg_override_reg = true,
 	.has_mst_fifo = false,
+	.has_mst_reset = false,
 	.quirks = &tegra_i2c_quirks,
 	.supports_bus_clear = true,
 	.has_apb_dma = true,
@@ -1587,6 +1614,7 @@ static const struct tegra_i2c_hw_feature tegra186_i2c_hw = {
 	.has_multi_master_mode = false,
 	.has_slcg_override_reg = true,
 	.has_mst_fifo = false,
+	.has_mst_reset = false,
 	.quirks = &tegra_i2c_quirks,
 	.supports_bus_clear = true,
 	.has_apb_dma = false,
@@ -1611,6 +1639,7 @@ static const struct tegra_i2c_hw_feature tegra194_i2c_hw = {
 	.has_multi_master_mode = true,
 	.has_slcg_override_reg = true,
 	.has_mst_fifo = true,
+	.has_mst_reset = true,
 	.quirks = &tegra194_i2c_quirks,
 	.supports_bus_clear = true,
 	.has_apb_dma = false,
@@ -1666,7 +1695,7 @@ static int tegra_i2c_init_reset(struct tegra_i2c_dev *i2c_dev)
 	if (ACPI_HANDLE(i2c_dev->dev))
 		return 0;
 
-	i2c_dev->rst = devm_reset_control_get_exclusive(i2c_dev->dev, "i2c");
+	i2c_dev->rst = devm_reset_control_get_optional_exclusive(i2c_dev->dev, "i2c");
 	if (IS_ERR(i2c_dev->rst))
 		return dev_err_probe(i2c_dev->dev, PTR_ERR(i2c_dev->rst),
 				      "failed to get reset control\n");
-- 
2.49.0

[PATCH v4 3/3] i2c: tegra: Remove dma_sync_*() calls

[Akhil R]

Calling dma_sync_*() on a buffer from dma_alloc_coherent() is pointless.
The driver should not be doing its own bounce-buffering if the buffer is
allocated through dma_alloc_coherent()

Suggested-by: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
---
v3->v4: No change
v2->v3: No change
v1->v2: No change

 drivers/i2c/busses/i2c-tegra.c | 20 +-------------------
 1 file changed, 1 insertion(+), 19 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 22ddbae9d847..b10a4bc9cb34 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -1292,17 +1292,9 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 
 	if (i2c_dev->dma_mode) {
 		if (i2c_dev->msg_read) {
-			dma_sync_single_for_device(i2c_dev->dma_dev,
-						   i2c_dev->dma_phys,
-						   xfer_size, DMA_FROM_DEVICE);
-
 			err = tegra_i2c_dma_submit(i2c_dev, xfer_size);
 			if (err)
 				return err;
-		} else {
-			dma_sync_single_for_cpu(i2c_dev->dma_dev,
-						i2c_dev->dma_phys,
-						xfer_size, DMA_TO_DEVICE);
 		}
 	}
 
@@ -1312,11 +1304,6 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 		if (i2c_dev->dma_mode) {
 			memcpy(i2c_dev->dma_buf + I2C_PACKET_HEADER_SIZE,
 			       msg->buf, i2c_dev->msg_len);
-
-			dma_sync_single_for_device(i2c_dev->dma_dev,
-						   i2c_dev->dma_phys,
-						   xfer_size, DMA_TO_DEVICE);
-
 			err = tegra_i2c_dma_submit(i2c_dev, xfer_size);
 			if (err)
 				return err;
@@ -1357,13 +1344,8 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 			return -ETIMEDOUT;
 		}
 
-		if (i2c_dev->msg_read && i2c_dev->msg_err == I2C_ERR_NONE) {
-			dma_sync_single_for_cpu(i2c_dev->dma_dev,
-						i2c_dev->dma_phys,
-						xfer_size, DMA_FROM_DEVICE);
-
+		if (i2c_dev->msg_read && i2c_dev->msg_err == I2C_ERR_NONE)
 			memcpy(i2c_dev->msg_buf, i2c_dev->dma_buf, i2c_dev->msg_len);
-		}
 	}
 
 	time_left = tegra_i2c_wait_completion(i2c_dev, &i2c_dev->msg_complete,
-- 
2.49.0

Re: [PATCH v4 1/3] dt-bindings: i2c: nvidia,tegra20-i2c: Specify the required properties

[Krzysztof Kozlowski]

On 03/06/2025 17:30, Akhil R wrote:
> Specify the properties which are essential and which are not for the
> Tegra I2C driver to function correctly. This was not added correctly when
> the TXT binding was converted to yaml. All the existing DT nodes have
> these properties already and hence this does not break the ABI.
> 
> dmas and dma-names which were specified as a must in the TXT binding
> is now made optional since the driver can work in PIO mode if dmas are
> missing.
> 
> Fixes: f10a9b722f80 ("dt-bindings: i2c: tegra: Convert to json-schema”)
> Signed-off-by: Akhil R <akhilrajeev@nvidia.com>

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v4 1/3] dt-bindings: i2c: nvidia,tegra20-i2c: Specify the required properties

[Akhil R]

>> Specify the properties which are essential and which are not for the
>> Tegra I2C driver to function correctly. This was not added correctly when
>> the TXT binding was converted to yaml. All the existing DT nodes have
>> these properties already and hence this does not break the ABI.
>> 
>> dmas and dma-names which were specified as a must in the TXT binding
>> is now made optional since the driver can work in PIO mode if dmas are
>> missing.
>> 
>> Fixes: f10a9b722f80 ("dt-bindings: i2c: tegra: Convert to json-schema”)
>> Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Thanks Krzysztof for the review.

Hi Andi and Wolfram,

Could you share if you see any more concerns with these patches?

Regards,
Akhil

Re: [PATCH v4 3/3] i2c: tegra: Remove dma_sync_*() calls

[Thierry Reding]

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

On Tue, Jun 03, 2025 at 09:00:22PM +0530, Akhil R wrote:
> Calling dma_sync_*() on a buffer from dma_alloc_coherent() is pointless.
> The driver should not be doing its own bounce-buffering if the buffer is
> allocated through dma_alloc_coherent()
> 
> Suggested-by: Robin Murphy <robin.murphy@arm.com>
> Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
> ---
> v3->v4: No change
> v2->v3: No change
> v1->v2: No change
> 
>  drivers/i2c/busses/i2c-tegra.c | 20 +-------------------
>  1 file changed, 1 insertion(+), 19 deletions(-)

I've had a patch like this in my local tree for a while and never got
around to send it out. It turns out that this is actually not just
unnecessary but can also cause issues. We were seeing really strange
crashes in the QSPI driver that seem related to this. I don't know
exactly what causes it, but removing the dma_sync_*() calls (for memory
that is also dma_alloc_coherent()-allocated) fixes things.

For QSPI I'm thinking it would probably be better to move to streaming
DMA mappings because the buffers in DMA mode are sufficiently large (at
least 2 cache lines) that we might see some benefit from the caching. I
don't know if this would apply for I2C as well? I vaguely recall that
certain transfers can be quite large and I wonder if cached mappings
would be advantageous here as well. Do you have good data on what the
usage patterns are?

That said, maybe the extra amount of work isn't worth it because both
I2C and QSPI are fairly slow busses, so the impact of caches is probably
minimal in comparison.

Anyway:

Reviewed-by: Thierry Reding <treding@nvidia.com>

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

Re: [PATCH v4 1/3] dt-bindings: i2c: nvidia,tegra20-i2c: Specify the required properties

[Krzysztof Kozlowski]

On 09/06/2025 11:02, Akhil R wrote:
>>> Specify the properties which are essential and which are not for the
>>> Tegra I2C driver to function correctly. This was not added correctly when
>>> the TXT binding was converted to yaml. All the existing DT nodes have
>>> these properties already and hence this does not break the ABI.
>>>
>>> dmas and dma-names which were specified as a must in the TXT binding
>>> is now made optional since the driver can work in PIO mode if dmas are
>>> missing.
>>>
>>> Fixes: f10a9b722f80 ("dt-bindings: i2c: tegra: Convert to json-schema”)
>>> Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
>>
>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
> Thanks Krzysztof for the review.
> 
> Hi Andi and Wolfram,
> 
> Could you share if you see any more concerns with these patches?
You sent it during merge window and a week later you ping? Considering
the quiet time, you sent it basically yesterday, so you gave people only
1 day... Don't ping after one day. Two weeks.

Best regards,
Krzysztof

Re: [PATCH v4 2/3] i2c: tegra: make reset an optional property

[Thierry Reding]

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

On Tue, Jun 03, 2025 at 09:00:21PM +0530, Akhil R wrote:
> For controllers that has an internal software reset, make the reset
> property optional. This is useful in systems that choose to restrict
> reset control from Linux.
> 
> Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
> ---
> v3->v4: No change
> v2->v3: No change
> v1->v2:
>   * Call devm_reset_control_get_optional_exclusive() unconditionally.
>   * Add more delay based on HW recommendation.
> 
>  drivers/i2c/busses/i2c-tegra.c | 33 +++++++++++++++++++++++++++++++--
>  1 file changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index 87976e99e6d0..22ddbae9d847 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -134,6 +134,8 @@
>  #define I2C_MST_FIFO_STATUS_TX			GENMASK(23, 16)
>  #define I2C_MST_FIFO_STATUS_RX			GENMASK(7, 0)
>  
> +#define I2C_MASTER_RESET_CNTRL			0x0a8
> +
>  /* configuration load timeout in microseconds */
>  #define I2C_CONFIG_LOAD_TIMEOUT			1000000
>  
> @@ -184,6 +186,9 @@ enum msg_end_type {
>   * @has_mst_fifo: The I2C controller contains the new MST FIFO interface that
>   *		provides additional features and allows for longer messages to
>   *		be transferred in one go.
> + * @has_mst_reset: The I2C controller contains MASTER_RESET_CTRL register which
> + *		provides an alternative to controller reset when configured as
> + *		I2C master
>   * @quirks: I2C adapter quirks for limiting write/read transfer size and not
>   *		allowing 0 length transfers.
>   * @supports_bus_clear: Bus Clear support to recover from bus hang during
> @@ -213,6 +218,7 @@ struct tegra_i2c_hw_feature {
>  	bool has_multi_master_mode;
>  	bool has_slcg_override_reg;
>  	bool has_mst_fifo;
> +	bool has_mst_reset;
>  	const struct i2c_adapter_quirks *quirks;
>  	bool supports_bus_clear;
>  	bool has_apb_dma;
> @@ -604,6 +610,20 @@ static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev)
>  	return 0;
>  }
>  
> +static int tegra_i2c_master_reset(struct tegra_i2c_dev *i2c_dev)
> +{
> +	if (!i2c_dev->hw->has_mst_reset)
> +		return -EOPNOTSUPP;
> +
> +	i2c_writel(i2c_dev, 0x1, I2C_MASTER_RESET_CNTRL);
> +	udelay(2);
> +
> +	i2c_writel(i2c_dev, 0x0, I2C_MASTER_RESET_CNTRL);
> +	udelay(2);
> +
> +	return 0;
> +}
> +
>  static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
>  {
>  	u32 val, clk_divisor, clk_multiplier, tsu_thd, tlow, thigh, non_hs_mode;
> @@ -621,8 +641,10 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
>  	 */
>  	if (handle)
>  		err = acpi_evaluate_object(handle, "_RST", NULL, NULL);

How is the internal reset handled on ACPI? Does the _RST method do the
internal reset?

Thierry

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

Re: [PATCH v4 2/3] i2c: tegra: make reset an optional property

[Akhil R]

On Tue, 10 Jun 2025 10:32:56 +0200, Thierry Reding wrote:
>> For controllers that has an internal software reset, make the reset
>> property optional. This is useful in systems that choose to restrict
>> reset control from Linux.
>> 
>> Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
>> ---
>> v3->v4: No change
>> v2->v3: No change
>> v1->v2:
>>   * Call devm_reset_control_get_optional_exclusive() unconditionally.
>>   * Add more delay based on HW recommendation.
>> 
>>  drivers/i2c/busses/i2c-tegra.c | 33 +++++++++++++++++++++++++++++++--
>>  1 file changed, 31 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
>> index 87976e99e6d0..22ddbae9d847 100644
>> --- a/drivers/i2c/busses/i2c-tegra.c
>> +++ b/drivers/i2c/busses/i2c-tegra.c
>> @@ -134,6 +134,8 @@
>>  #define I2C_MST_FIFO_STATUS_TX			GENMASK(23, 16)
>>  #define I2C_MST_FIFO_STATUS_RX			GENMASK(7, 0)
>>  
>> +#define I2C_MASTER_RESET_CNTRL			0x0a8
>> +
>>  /* configuration load timeout in microseconds */
>>  #define I2C_CONFIG_LOAD_TIMEOUT			1000000
>>  
>> @@ -184,6 +186,9 @@ enum msg_end_type {
>>   * @has_mst_fifo: The I2C controller contains the new MST FIFO interface that
>>   *		provides additional features and allows for longer messages to
>>   *		be transferred in one go.
>> + * @has_mst_reset: The I2C controller contains MASTER_RESET_CTRL register which
>> + *		provides an alternative to controller reset when configured as
>> + *		I2C master
>>   * @quirks: I2C adapter quirks for limiting write/read transfer size and not
>>   *		allowing 0 length transfers.
>>   * @supports_bus_clear: Bus Clear support to recover from bus hang during
>> @@ -213,6 +218,7 @@ struct tegra_i2c_hw_feature {
>>  	bool has_multi_master_mode;
>>  	bool has_slcg_override_reg;
>>  	bool has_mst_fifo;
>> +	bool has_mst_reset;
>>  	const struct i2c_adapter_quirks *quirks;
>>  	bool supports_bus_clear;
>>  	bool has_apb_dma;
>> @@ -604,6 +610,20 @@ static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev)
>>  	return 0;
>>  }
>>  
>> +static int tegra_i2c_master_reset(struct tegra_i2c_dev *i2c_dev)
>> +{
>> +	if (!i2c_dev->hw->has_mst_reset)
>> +		return -EOPNOTSUPP;
>> +
>> +	i2c_writel(i2c_dev, 0x1, I2C_MASTER_RESET_CNTRL);
>> +	udelay(2);
>> +
>> +	i2c_writel(i2c_dev, 0x0, I2C_MASTER_RESET_CNTRL);
>> +	udelay(2);
>> +
>> +	return 0;
>> +}
>> +
>>  static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
>>  {
>>  	u32 val, clk_divisor, clk_multiplier, tsu_thd, tlow, thigh, non_hs_mode;
>> @@ -621,8 +641,10 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
>>  	 */
>>  	if (handle)
>>  		err = acpi_evaluate_object(handle, "_RST", NULL, NULL);
>
> How is the internal reset handled on ACPI? Does the _RST method do the
> internal reset?

Right now, devices using ACPI would have to rely on the _RST method implementation.
It is unlikely that it implements an internal reset mechanism.

Do you suggest adding a check with 'acpi_has_method(handle, "_RST")' and fallback to
internal reset when it is false?

Regards,
Akhil

Re: [PATCH v4 2/3] i2c: tegra: make reset an optional property

[Thierry Reding]

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

On Tue, Jun 10, 2025 at 03:21:57PM +0530, Akhil R wrote:
[...]
> >> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
[...]
> >> @@ -621,8 +641,10 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
> >>  	 */
> >>  	if (handle)
> >>  		err = acpi_evaluate_object(handle, "_RST", NULL, NULL);
> >
> > How is the internal reset handled on ACPI? Does the _RST method do the
> > internal reset?
> 
> Right now, devices using ACPI would have to rely on the _RST method implementation.
> It is unlikely that it implements an internal reset mechanism.
> 
> Do you suggest adding a check with 'acpi_has_method(handle, "_RST")' and fallback to
> internal reset when it is false?

I don't know if that's desirable (because _RST may be left unimplemented
on purpose on an ACPI platform, and using the internal reset may break
something), but yeah, that would be the equivalent of the DT path.

Thierry

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

Re: [PATCH v4 2/3] i2c: tegra: make reset an optional property

[Andi Shyti]

Hi Andy,

...

> @@ -184,6 +186,9 @@ enum msg_end_type {
>   * @has_mst_fifo: The I2C controller contains the new MST FIFO interface that
>   *		provides additional features and allows for longer messages to
>   *		be transferred in one go.
> + * @has_mst_reset: The I2C controller contains MASTER_RESET_CTRL register which
> + *		provides an alternative to controller reset when configured as
> + *		I2C master
>   * @quirks: I2C adapter quirks for limiting write/read transfer size and not
>   *		allowing 0 length transfers.
>   * @supports_bus_clear: Bus Clear support to recover from bus hang during
> @@ -213,6 +218,7 @@ struct tegra_i2c_hw_feature {
>  	bool has_multi_master_mode;
>  	bool has_slcg_override_reg;
>  	bool has_mst_fifo;
> +	bool has_mst_reset;
>  	const struct i2c_adapter_quirks *quirks;
>  	bool supports_bus_clear;
>  	bool has_apb_dma;
> @@ -604,6 +610,20 @@ static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev)
>  	return 0;
>  }
>  
> +static int tegra_i2c_master_reset(struct tegra_i2c_dev *i2c_dev)
> +{
> +	if (!i2c_dev->hw->has_mst_reset)
> +		return -EOPNOTSUPP;
> +
> +	i2c_writel(i2c_dev, 0x1, I2C_MASTER_RESET_CNTRL);
> +	udelay(2);
> +
> +	i2c_writel(i2c_dev, 0x0, I2C_MASTER_RESET_CNTRL);
> +	udelay(2);
> +
> +	return 0;
> +}
> +
>  static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
>  {
>  	u32 val, clk_divisor, clk_multiplier, tsu_thd, tlow, thigh, non_hs_mode;
> @@ -621,8 +641,10 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
>  	 */
>  	if (handle)
>  		err = acpi_evaluate_object(handle, "_RST", NULL, NULL);
> -	else
> +	else if (i2c_dev->rst)
>  		err = reset_control_reset(i2c_dev->rst);
> +	else
> +		err = tegra_i2c_master_reset(i2c_dev);

Can you please take a look here? Should the reset happen in ACPI?

Thanks,
Andi

Re: [PATCH v4 1/3] dt-bindings: i2c: nvidia,tegra20-i2c: Specify the required properties

[Andi Shyti]

Hi Akhil,

On Tue, Jun 03, 2025 at 09:00:20PM +0530, Akhil R wrote:
> Specify the properties which are essential and which are not for the
> Tegra I2C driver to function correctly. This was not added correctly when
> the TXT binding was converted to yaml. All the existing DT nodes have
> these properties already and hence this does not break the ABI.
> 
> dmas and dma-names which were specified as a must in the TXT binding
> is now made optional since the driver can work in PIO mode if dmas are
> missing.
> 
> Fixes: f10a9b722f80 ("dt-bindings: i2c: tegra: Convert to json-schema”)
> Signed-off-by: Akhil R <akhilrajeev@nvidia.com>

I took just this patch into i2c/i2c-host-fixes.

Thanks,
Andi

Re: [PATCH v4 2/3] i2c: tegra: make reset an optional property

[Andy Shevchenko]

On Thu, Jun 12, 2025 at 02:57:36AM +0200, Andi Shyti wrote:

...

> > +static int tegra_i2c_master_reset(struct tegra_i2c_dev *i2c_dev)
> > +{
> > +	if (!i2c_dev->hw->has_mst_reset)
> > +		return -EOPNOTSUPP;
> > +
> > +	i2c_writel(i2c_dev, 0x1, I2C_MASTER_RESET_CNTRL);
> > +	udelay(2);
> > +
> > +	i2c_writel(i2c_dev, 0x0, I2C_MASTER_RESET_CNTRL);
> > +	udelay(2);
> > +
> > +	return 0;
> > +}
> > +
> >  static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
> >  {
> >  	u32 val, clk_divisor, clk_multiplier, tsu_thd, tlow, thigh, non_hs_mode;
> > @@ -621,8 +641,10 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
> >  	 */
> >  	if (handle)
> >  		err = acpi_evaluate_object(handle, "_RST", NULL, NULL);
> > -	else
> > +	else if (i2c_dev->rst)
> >  		err = reset_control_reset(i2c_dev->rst);
> > +	else
> > +		err = tegra_i2c_master_reset(i2c_dev);
> 
> Can you please take a look here? Should the reset happen in ACPI?

This is a good question. Without seeing all the implementations of _RST method
for the platforms based on this SoC it's hard to say. Ideally the _RST (which
is called above) must handle it properly, but firmwares have bugs...

TL;DR: I think the approach is correct, and if any bug in ACPI will be found,
the workaround (quirk) needs to be added here later on.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 2/3] i2c: tegra: make reset an optional property

[Akhil R]

On Thu, 12 Jun 2025 15:55:21 +0300, Andy Shevchenko wrote:

>
>> >  static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
>> >  {
>> >     u32 val, clk_divisor, clk_multiplier, tsu_thd, tlow, thigh, non_hs_mode;
>> > @@ -621,8 +641,10 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
>> >      */
>> >     if (handle)
>> >             err = acpi_evaluate_object(handle, "_RST", NULL, NULL);
>> > -   else
>> > +   else if (i2c_dev->rst)
>> >             err = reset_control_reset(i2c_dev->rst);
>> > +   else
>> > +           err = tegra_i2c_master_reset(i2c_dev);
>>
>> Can you please take a look here? Should the reset happen in ACPI?
>
> This is a good question. Without seeing all the implementations of _RST method
> for the platforms based on this SoC it's hard to say. Ideally the _RST (which
> is called above) must handle it properly, but firmwares have bugs...
>
> TL;DR: I think the approach is correct, and if any bug in ACPI will be found,
> the workaround (quirk) needs to be added here later on.

As in Thierry's comment, I was in thought of updating the code as below.
Does it make sense or would it be better keep what it is there now?

if (handle && acpi_has_method(handle, "_RST"))
	err = acpi_evaluate_object(handle, "_RST", NULL, NULL);
else if (i2c_dev->rst)
	err = reset_control_reset(i2c_dev->rst);
else
	err = tegra_i2c_master_reset(i2c_dev);

Regards,
Akhil

Re: [PATCH v4 2/3] i2c: tegra: make reset an optional property

[Andy Shevchenko]

Thu, Jun 12, 2025 at 09:03:38PM +0530, Akhil R kirjoitti:
> On Thu, 12 Jun 2025 15:55:21 +0300, Andy Shevchenko wrote:

...

> >> >     if (handle)
> >> >             err = acpi_evaluate_object(handle, "_RST", NULL, NULL);
> >> > -   else
> >> > +   else if (i2c_dev->rst)
> >> >             err = reset_control_reset(i2c_dev->rst);
> >> > +   else
> >> > +           err = tegra_i2c_master_reset(i2c_dev);
> >>
> >> Can you please take a look here? Should the reset happen in ACPI?
> >
> > This is a good question. Without seeing all the implementations of _RST method
> > for the platforms based on this SoC it's hard to say. Ideally the _RST (which
> > is called above) must handle it properly, but firmwares have bugs...
> >
> > TL;DR: I think the approach is correct, and if any bug in ACPI will be found,
> > the workaround (quirk) needs to be added here later on.
> 
> As in Thierry's comment, I was in thought of updating the code as below.
> Does it make sense or would it be better keep what it is there now?
> 
> if (handle && acpi_has_method(handle, "_RST"))
> 	err = acpi_evaluate_object(handle, "_RST", NULL, NULL);
> else if (i2c_dev->rst)
> 	err = reset_control_reset(i2c_dev->rst);
> else
> 	err = tegra_i2c_master_reset(i2c_dev);

This will change current behaviour for the ACPI based platforms that do not
have an _RST method. At bare minumum this has to be elaborated in the commit
message with an explanation why it's not a probnlem.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 2/3] i2c: tegra: make reset an optional property

[Akhil R]

On Thu, 12 Jun 2025 21:43:56 +0300, Andy Shevchenko wrote:

>> >> >     if (handle)
>> >> >             err = acpi_evaluate_object(handle, "_RST", NULL, NULL);
>> >> > -   else
>> >> > +   else if (i2c_dev->rst)
>> >> >             err = reset_control_reset(i2c_dev->rst);
>> >> > +   else
>> >> > +           err = tegra_i2c_master_reset(i2c_dev);
>> >>
>> >> Can you please take a look here? Should the reset happen in ACPI?
>> >
>> > This is a good question. Without seeing all the implementations of _RST method
>> > for the platforms based on this SoC it's hard to say. Ideally the _RST (which
>> > is called above) must handle it properly, but firmwares have bugs...
>> >
>> > TL;DR: I think the approach is correct, and if any bug in ACPI will be found,
>> > the workaround (quirk) needs to be added here later on.
>> 
>> As in Thierry's comment, I was in thought of updating the code as below.
>> Does it make sense or would it be better keep what it is there now?
>> 
>> if (handle && acpi_has_method(handle, "_RST"))
>> 	err = acpi_evaluate_object(handle, "_RST", NULL, NULL);
>> else if (i2c_dev->rst)
>> 	err = reset_control_reset(i2c_dev->rst);
>> else
>> 	err = tegra_i2c_master_reset(i2c_dev);
>
> This will change current behaviour for the ACPI based platforms that do not
> have an _RST method. At bare minumum this has to be elaborated in the commit
> message with an explanation why it's not a probnlem.
>

This sequence is hit only at boot and on any error. It should be good to reset
the controller internally at least for those cases. We are reconfiguring the I2C
anyway after this and hence should not cause any problem.
Will add these in the commit message as well.

Thanks & Regards,
Akhil

Re: [PATCH v4 2/3] i2c: tegra: make reset an optional property

[Andy Shevchenko]

On Fri, Jun 13, 2025 at 11:30:32AM +0530, Akhil R wrote:
> On Thu, 12 Jun 2025 21:43:56 +0300, Andy Shevchenko wrote:

> >> >> >     if (handle)
> >> >> >             err = acpi_evaluate_object(handle, "_RST", NULL, NULL);
> >> >> > -   else
> >> >> > +   else if (i2c_dev->rst)
> >> >> >             err = reset_control_reset(i2c_dev->rst);
> >> >> > +   else
> >> >> > +           err = tegra_i2c_master_reset(i2c_dev);
> >> >>
> >> >> Can you please take a look here? Should the reset happen in ACPI?
> >> >
> >> > This is a good question. Without seeing all the implementations of _RST method
> >> > for the platforms based on this SoC it's hard to say. Ideally the _RST (which
> >> > is called above) must handle it properly, but firmwares have bugs...
> >> >
> >> > TL;DR: I think the approach is correct, and if any bug in ACPI will be found,
> >> > the workaround (quirk) needs to be added here later on.
> >> 
> >> As in Thierry's comment, I was in thought of updating the code as below.
> >> Does it make sense or would it be better keep what it is there now?
> >> 
> >> if (handle && acpi_has_method(handle, "_RST"))
> >> 	err = acpi_evaluate_object(handle, "_RST", NULL, NULL);
> >> else if (i2c_dev->rst)
> >> 	err = reset_control_reset(i2c_dev->rst);
> >> else
> >> 	err = tegra_i2c_master_reset(i2c_dev);
> >
> > This will change current behaviour for the ACPI based platforms that do not
> > have an _RST method. At bare minumum this has to be elaborated in the commit
> > message with an explanation why it's not a probnlem.
> 
> This sequence is hit only at boot and on any error. It should be good to reset
> the controller internally at least for those cases. We are reconfiguring the I2C
> anyway after this and hence should not cause any problem.
> Will add these in the commit message as well.

This is not enough. You should explain the ACPI case. The above is just generic
wording as I read it. It does not explain 1) if there are ACPI firmwares that
have no _RST method for this device; 2) why it's not a problem for them to do
like this and why it was not supported before (with the current code this
platform will return an error on the method evaluation. Moreover the current
code is buggy. The acpi_evaluate_object() returns an ACPI error code and not
Linux one. so, for the such platforms (which I think do not exist, but still)
the err will have positive code which may be interpreted incorrectly.

So, fix the bug first, then rebase your code based on that change and
extend the commit message to really elaborate on all of the aspects.
W/o this done it's no go change.



-- 
With Best Regards,
Andy Shevchenko