[PATCH 0/5] Add I2C support for Tegra264

[PATCH 0/5] Add I2C support for Tegra264

[Kartik Rajput]

Following series of patches add support for Tegra264 and High Speed (HS)
Mode in i2c-tegra.c driver.

Akhil R (3):
  i2c: tegra: Add HS mode support
  i2c: tegra: Add Tegra264 support
  i2c: tegra: Add support for SW Mutex register

Kartik Rajput (2):
  dt-bindings: i2c: nvidia,tegra20-i2c: Document Tegra264 I2C
  i2c: tegra: Do not configure DMA if not supported

 .../bindings/i2c/nvidia,tegra20-i2c.yaml      |   6 +
 drivers/i2c/busses/i2c-tegra.c                | 179 ++++++++++++++++--
 2 files changed, 170 insertions(+), 15 deletions(-)

-- 
2.43.0

[PATCH 1/5] i2c: tegra: Add HS mode support

[Kartik Rajput]

From: Akhil R <akhilrajeev@nvidia.com>

Add support for HS (High Speed) mode tranfers, which is supported by
Tegra194 onwards.

Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
Signed-off-by: Kartik Rajput <kkartik@nvidia.com>
---
 drivers/i2c/busses/i2c-tegra.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 87976e99e6d0..7b97c6d347ee 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -91,6 +91,7 @@
 #define I2C_HEADER_IE_ENABLE			BIT(17)
 #define I2C_HEADER_REPEAT_START			BIT(16)
 #define I2C_HEADER_CONTINUE_XFER		BIT(15)
+#define I2C_HEADER_HS_MODE			BIT(22)
 #define I2C_HEADER_SLAVE_ADDR_SHIFT		1
 
 #define I2C_BUS_CLEAR_CNFG			0x084
@@ -220,10 +221,13 @@ struct tegra_i2c_hw_feature {
 	u32 thigh_std_mode;
 	u32 tlow_fast_fastplus_mode;
 	u32 thigh_fast_fastplus_mode;
+	u32 tlow_hs_mode;
+	u32 thigh_hs_mode;
 	u32 setup_hold_time_std_mode;
 	u32 setup_hold_time_fast_fast_plus_mode;
 	u32 setup_hold_time_hs_mode;
 	bool has_interface_timing_reg;
+	bool has_hs_mode_support;
 };
 
 /**
@@ -681,6 +685,18 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
 	if (i2c_dev->hw->has_interface_timing_reg && tsu_thd)
 		i2c_writel(i2c_dev, tsu_thd, I2C_INTERFACE_TIMING_1);
 
+	/* Write HS mode registers. These will get used only for HS mode*/
+	if (i2c_dev->hw->has_hs_mode_support) {
+		tlow = i2c_dev->hw->tlow_hs_mode;
+		thigh = i2c_dev->hw->thigh_hs_mode;
+		tsu_thd = i2c_dev->hw->setup_hold_time_hs_mode;
+
+		val = FIELD_PREP(I2C_HS_INTERFACE_TIMING_THIGH, thigh) |
+			FIELD_PREP(I2C_HS_INTERFACE_TIMING_TLOW, tlow);
+		i2c_writel(i2c_dev, val, I2C_HS_INTERFACE_TIMING_0);
+		i2c_writel(i2c_dev, tsu_thd, I2C_HS_INTERFACE_TIMING_1);
+	}
+
 	clk_multiplier = (tlow + thigh + 2) * (non_hs_mode + 1);
 
 	err = clk_set_rate(i2c_dev->div_clk,
@@ -1178,6 +1194,9 @@ static void tegra_i2c_push_packet_header(struct tegra_i2c_dev *i2c_dev,
 	if (msg->flags & I2C_M_RD)
 		packet_header |= I2C_HEADER_READ;
 
+	if (i2c_dev->timings.bus_freq_hz > I2C_MAX_FAST_MODE_PLUS_FREQ)
+		packet_header |= I2C_HEADER_HS_MODE;
+
 	if (i2c_dev->dma_mode && !i2c_dev->msg_read)
 		*dma_buf++ = packet_header;
 	else
@@ -1618,10 +1637,13 @@ static const struct tegra_i2c_hw_feature tegra194_i2c_hw = {
 	.thigh_std_mode = 0x7,
 	.tlow_fast_fastplus_mode = 0x2,
 	.thigh_fast_fastplus_mode = 0x2,
+	.tlow_hs_mode = 0x8,
+	.thigh_hs_mode = 0x3,
 	.setup_hold_time_std_mode = 0x08080808,
 	.setup_hold_time_fast_fast_plus_mode = 0x02020202,
 	.setup_hold_time_hs_mode = 0x090909,
 	.has_interface_timing_reg = true,
+	.has_hs_mode_support = true,
 };
 
 static const struct of_device_id tegra_i2c_of_match[] = {
-- 
2.43.0

Re: [PATCH 1/5] i2c: tegra: Add HS mode support

[Thierry Reding]

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

On Wed, Jan 08, 2025 at 04:36:16PM +0530, Kartik Rajput wrote:
> From: Akhil R <akhilrajeev@nvidia.com>
> 
> Add support for HS (High Speed) mode tranfers, which is supported by
> Tegra194 onwards.
> 
> Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
> Signed-off-by: Kartik Rajput <kkartik@nvidia.com>
> ---
>  drivers/i2c/busses/i2c-tegra.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index 87976e99e6d0..7b97c6d347ee 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -91,6 +91,7 @@
>  #define I2C_HEADER_IE_ENABLE			BIT(17)
>  #define I2C_HEADER_REPEAT_START			BIT(16)
>  #define I2C_HEADER_CONTINUE_XFER		BIT(15)
> +#define I2C_HEADER_HS_MODE			BIT(22)
>  #define I2C_HEADER_SLAVE_ADDR_SHIFT		1
>  
>  #define I2C_BUS_CLEAR_CNFG			0x084
> @@ -220,10 +221,13 @@ struct tegra_i2c_hw_feature {
>  	u32 thigh_std_mode;
>  	u32 tlow_fast_fastplus_mode;
>  	u32 thigh_fast_fastplus_mode;
> +	u32 tlow_hs_mode;
> +	u32 thigh_hs_mode;
>  	u32 setup_hold_time_std_mode;
>  	u32 setup_hold_time_fast_fast_plus_mode;
>  	u32 setup_hold_time_hs_mode;
>  	bool has_interface_timing_reg;
> +	bool has_hs_mode_support;
>  };
>  
>  /**
> @@ -681,6 +685,18 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
>  	if (i2c_dev->hw->has_interface_timing_reg && tsu_thd)
>  		i2c_writel(i2c_dev, tsu_thd, I2C_INTERFACE_TIMING_1);
>  
> +	/* Write HS mode registers. These will get used only for HS mode*/
> +	if (i2c_dev->hw->has_hs_mode_support) {
> +		tlow = i2c_dev->hw->tlow_hs_mode;
> +		thigh = i2c_dev->hw->thigh_hs_mode;
> +		tsu_thd = i2c_dev->hw->setup_hold_time_hs_mode;
> +
> +		val = FIELD_PREP(I2C_HS_INTERFACE_TIMING_THIGH, thigh) |
> +			FIELD_PREP(I2C_HS_INTERFACE_TIMING_TLOW, tlow);
> +		i2c_writel(i2c_dev, val, I2C_HS_INTERFACE_TIMING_0);
> +		i2c_writel(i2c_dev, tsu_thd, I2C_HS_INTERFACE_TIMING_1);
> +	}
> +
>  	clk_multiplier = (tlow + thigh + 2) * (non_hs_mode + 1);
>  
>  	err = clk_set_rate(i2c_dev->div_clk,
> @@ -1178,6 +1194,9 @@ static void tegra_i2c_push_packet_header(struct tegra_i2c_dev *i2c_dev,
>  	if (msg->flags & I2C_M_RD)
>  		packet_header |= I2C_HEADER_READ;
>  
> +	if (i2c_dev->timings.bus_freq_hz > I2C_MAX_FAST_MODE_PLUS_FREQ)
> +		packet_header |= I2C_HEADER_HS_MODE;
> +

Do we need any kind of checking here? What happens if the requested
frequency is higher than fastmode+ but the device doesn't support the
new high-speed mode? I guess maybe we don't have to care because such
cases won't ever show up because, well, they won't work.

Still, it might be a good idea to be explicit about it and have an extra
check in place (or perhaps a little higher up in the call chain) and
return an error if we're trying to use a frequency that isn't supported
on the chip.

Thierry

>  	if (i2c_dev->dma_mode && !i2c_dev->msg_read)
>  		*dma_buf++ = packet_header;
>  	else
> @@ -1618,10 +1637,13 @@ static const struct tegra_i2c_hw_feature tegra194_i2c_hw = {
>  	.thigh_std_mode = 0x7,
>  	.tlow_fast_fastplus_mode = 0x2,
>  	.thigh_fast_fastplus_mode = 0x2,
> +	.tlow_hs_mode = 0x8,
> +	.thigh_hs_mode = 0x3,
>  	.setup_hold_time_std_mode = 0x08080808,
>  	.setup_hold_time_fast_fast_plus_mode = 0x02020202,
>  	.setup_hold_time_hs_mode = 0x090909,
>  	.has_interface_timing_reg = true,
> +	.has_hs_mode_support = true,
>  };
>  
>  static const struct of_device_id tegra_i2c_of_match[] = {
> -- 
> 2.43.0
> 

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

Re: [PATCH 1/5] i2c: tegra: Add HS mode support

[Kartik Rajput]

On Wed, 2025-01-08 at 17:45 +0100, Thierry Reding wrote:
> On Wed, Jan 08, 2025 at 04:36:16PM +0530, Kartik Rajput wrote:
> > From: Akhil R <akhilrajeev@nvidia.com>
> > 
> > Add support for HS (High Speed) mode tranfers, which is supported
> > by
> > Tegra194 onwards.
> > 
> > Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
> > Signed-off-by: Kartik Rajput <kkartik@nvidia.com>
> > ---
> >  drivers/i2c/busses/i2c-tegra.c | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> > 
> > diff --git a/drivers/i2c/busses/i2c-tegra.c
> > b/drivers/i2c/busses/i2c-tegra.c
> > index 87976e99e6d0..7b97c6d347ee 100644
> > --- a/drivers/i2c/busses/i2c-tegra.c
> > +++ b/drivers/i2c/busses/i2c-tegra.c
> > @@ -91,6 +91,7 @@
> >  #define I2C_HEADER_IE_ENABLE                   BIT(17)
> >  #define I2C_HEADER_REPEAT_START                        BIT(16)
> >  #define I2C_HEADER_CONTINUE_XFER               BIT(15)
> > +#define I2C_HEADER_HS_MODE                     BIT(22)
> >  #define I2C_HEADER_SLAVE_ADDR_SHIFT            1
> >  
> >  #define I2C_BUS_CLEAR_CNFG                     0x084
> > @@ -220,10 +221,13 @@ struct tegra_i2c_hw_feature {
> >         u32 thigh_std_mode;
> >         u32 tlow_fast_fastplus_mode;
> >         u32 thigh_fast_fastplus_mode;
> > +       u32 tlow_hs_mode;
> > +       u32 thigh_hs_mode;
> >         u32 setup_hold_time_std_mode;
> >         u32 setup_hold_time_fast_fast_plus_mode;
> >         u32 setup_hold_time_hs_mode;
> >         bool has_interface_timing_reg;
> > +       bool has_hs_mode_support;
> >  };
> >  
> >  /**
> > @@ -681,6 +685,18 @@ static int tegra_i2c_init(struct tegra_i2c_dev
> > *i2c_dev)
> >         if (i2c_dev->hw->has_interface_timing_reg && tsu_thd)
> >                 i2c_writel(i2c_dev, tsu_thd,
> > I2C_INTERFACE_TIMING_1);
> >  
> > +       /* Write HS mode registers. These will get used only for HS
> > mode*/
> > +       if (i2c_dev->hw->has_hs_mode_support) {
> > +               tlow = i2c_dev->hw->tlow_hs_mode;
> > +               thigh = i2c_dev->hw->thigh_hs_mode;
> > +               tsu_thd = i2c_dev->hw->setup_hold_time_hs_mode;
> > +
> > +               val = FIELD_PREP(I2C_HS_INTERFACE_TIMING_THIGH,
> > thigh) |
> > +                       FIELD_PREP(I2C_HS_INTERFACE_TIMING_TLOW,
> > tlow);
> > +               i2c_writel(i2c_dev, val,
> > I2C_HS_INTERFACE_TIMING_0);
> > +               i2c_writel(i2c_dev, tsu_thd,
> > I2C_HS_INTERFACE_TIMING_1);
> > +       }
> > +
> >         clk_multiplier = (tlow + thigh + 2) * (non_hs_mode + 1);
> >  
> >         err = clk_set_rate(i2c_dev->div_clk,
> > @@ -1178,6 +1194,9 @@ static void
> > tegra_i2c_push_packet_header(struct tegra_i2c_dev *i2c_dev,
> >         if (msg->flags & I2C_M_RD)
> >                 packet_header |= I2C_HEADER_READ;
> >  
> > +       if (i2c_dev->timings.bus_freq_hz >
> > I2C_MAX_FAST_MODE_PLUS_FREQ)
> > +               packet_header |= I2C_HEADER_HS_MODE;
> > +
> 
> Do we need any kind of checking here? What happens if the requested
> frequency is higher than fastmode+ but the device doesn't support the
> new high-speed mode? I guess maybe we don't have to care because such
> cases won't ever show up because, well, they won't work.
> 
> Still, it might be a good idea to be explicit about it and have an
> extra
> check in place (or perhaps a little higher up in the call chain) and
> return an error if we're trying to use a frequency that isn't
> supported
> on the chip.
> 
> Thierry

ACK. I will add a check for this.

> 
> >         if (i2c_dev->dma_mode && !i2c_dev->msg_read)
> >                 *dma_buf++ = packet_header;
> >         else
> > @@ -1618,10 +1637,13 @@ static const struct tegra_i2c_hw_feature
> > tegra194_i2c_hw = {
> >         .thigh_std_mode = 0x7,
> >         .tlow_fast_fastplus_mode = 0x2,
> >         .thigh_fast_fastplus_mode = 0x2,
> > +       .tlow_hs_mode = 0x8,
> > +       .thigh_hs_mode = 0x3,
> >         .setup_hold_time_std_mode = 0x08080808,
> >         .setup_hold_time_fast_fast_plus_mode = 0x02020202,
> >         .setup_hold_time_hs_mode = 0x090909,
> >         .has_interface_timing_reg = true,
> > +       .has_hs_mode_support = true,
> >  };
> >  
> >  static const struct of_device_id tegra_i2c_of_match[] = {
> > -- 
> > 2.43.0
> > 

Re: [PATCH 1/5] i2c: tegra: Add HS mode support

[kernel test robot]

Hi Kartik,

kernel test robot noticed the following build warnings:

[auto build test WARNING on tegra/for-next]
[also build test WARNING on andi-shyti/i2c/i2c-host robh/for-next linus/master v6.13-rc6 next-20250109]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Kartik-Rajput/i2c-tegra-Add-HS-mode-support/20250108-190816
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tegra/linux.git for-next
patch link:    https://lore.kernel.org/r/20250108110620.86900-2-kkartik%40nvidia.com
patch subject: [PATCH 1/5] i2c: tegra: Add HS mode support
config: arm-randconfig-001-20250109 (https://download.01.org/0day-ci/archive/20250109/202501091951.yESb0LaA-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250109/202501091951.yESb0LaA-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202501091951.yESb0LaA-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/i2c/busses/i2c-tegra.c:231: warning: Function parameter or struct member 'tlow_hs_mode' not described in 'tegra_i2c_hw_feature'
>> drivers/i2c/busses/i2c-tegra.c:231: warning: Function parameter or struct member 'thigh_hs_mode' not described in 'tegra_i2c_hw_feature'
>> drivers/i2c/busses/i2c-tegra.c:231: warning: Function parameter or struct member 'has_hs_mode_support' not described in 'tegra_i2c_hw_feature'
   drivers/i2c/busses/i2c-tegra.c:301: warning: Function parameter or struct member 'dma_dev' not described in 'tegra_i2c_dev'


vim +231 drivers/i2c/busses/i2c-tegra.c

db811ca0f48578 Colin Cross         2011-02-20  163  
6ad068ed63100f Laxman Dewangan     2012-08-19  164  /**
94a5573f0719cf Dmitry Osipenko     2020-09-30  165   * struct tegra_i2c_hw_feature : per hardware generation features
94a5573f0719cf Dmitry Osipenko     2020-09-30  166   * @has_continue_xfer_support: continue-transfer supported
2a2897bab2d3d5 Laxman Dewangan     2013-01-05  167   * @has_per_pkt_xfer_complete_irq: Has enable/disable capability for transfer
94a5573f0719cf Dmitry Osipenko     2020-09-30  168   *		completion interrupt on per packet basis.
6f4664b2e2c2cf Laxman Dewangan     2015-06-30  169   * @has_config_load_reg: Has the config load register to load the new
6f4664b2e2c2cf Laxman Dewangan     2015-06-30  170   *		configuration.
2a2897bab2d3d5 Laxman Dewangan     2013-01-05  171   * @clk_divisor_hs_mode: Clock divisor in HS mode.
0940d24912e925 Sowjanya Komatineni 2019-02-12  172   * @clk_divisor_std_mode: Clock divisor in standard mode. It is
0940d24912e925 Sowjanya Komatineni 2019-02-12  173   *		applicable if there is no fast clock source i.e. single clock
0940d24912e925 Sowjanya Komatineni 2019-02-12  174   *		source.
0940d24912e925 Sowjanya Komatineni 2019-02-12  175   * @clk_divisor_fast_mode: Clock divisor in fast mode. It is
2a2897bab2d3d5 Laxman Dewangan     2013-01-05  176   *		applicable if there is no fast clock source i.e. single clock
2a2897bab2d3d5 Laxman Dewangan     2013-01-05  177   *		source.
0604ee4aefa20f Thierry Reding      2018-12-17  178   * @clk_divisor_fast_plus_mode: Clock divisor in fast mode plus. It is
0604ee4aefa20f Thierry Reding      2018-12-17  179   *		applicable if there is no fast clock source (i.e. single
0604ee4aefa20f Thierry Reding      2018-12-17  180   *		clock source).
0604ee4aefa20f Thierry Reding      2018-12-17  181   * @has_multi_master_mode: The I2C controller supports running in single-master
0604ee4aefa20f Thierry Reding      2018-12-17  182   *		or multi-master mode.
0604ee4aefa20f Thierry Reding      2018-12-17  183   * @has_slcg_override_reg: The I2C controller supports a register that
0604ee4aefa20f Thierry Reding      2018-12-17  184   *		overrides the second level clock gating.
0604ee4aefa20f Thierry Reding      2018-12-17  185   * @has_mst_fifo: The I2C controller contains the new MST FIFO interface that
0604ee4aefa20f Thierry Reding      2018-12-17  186   *		provides additional features and allows for longer messages to
0604ee4aefa20f Thierry Reding      2018-12-17  187   *		be transferred in one go.
94a5573f0719cf Dmitry Osipenko     2020-09-30  188   * @quirks: I2C adapter quirks for limiting write/read transfer size and not
b67d4530cdade7 Sowjanya Komatineni 2019-01-08  189   *		allowing 0 length transfers.
ce9562424501de Sowjanya Komatineni 2019-02-12  190   * @supports_bus_clear: Bus Clear support to recover from bus hang during
ce9562424501de Sowjanya Komatineni 2019-02-12  191   *		SDA stuck low from device for some unknown reasons.
86c92b9965ff17 Sowjanya Komatineni 2019-02-12  192   * @has_apb_dma: Support of APBDMA on corresponding Tegra chip.
0940d24912e925 Sowjanya Komatineni 2019-02-12  193   * @tlow_std_mode: Low period of the clock in standard mode.
0940d24912e925 Sowjanya Komatineni 2019-02-12  194   * @thigh_std_mode: High period of the clock in standard mode.
0940d24912e925 Sowjanya Komatineni 2019-02-12  195   * @tlow_fast_fastplus_mode: Low period of the clock in fast/fast-plus modes.
0940d24912e925 Sowjanya Komatineni 2019-02-12  196   * @thigh_fast_fastplus_mode: High period of the clock in fast/fast-plus modes.
0940d24912e925 Sowjanya Komatineni 2019-02-12  197   * @setup_hold_time_std_mode: Setup and hold time for start and stop conditions
0940d24912e925 Sowjanya Komatineni 2019-02-12  198   *		in standard mode.
0940d24912e925 Sowjanya Komatineni 2019-02-12  199   * @setup_hold_time_fast_fast_plus_mode: Setup and hold time for start and stop
0940d24912e925 Sowjanya Komatineni 2019-02-12  200   *		conditions in fast/fast-plus modes.
0940d24912e925 Sowjanya Komatineni 2019-02-12  201   * @setup_hold_time_hs_mode: Setup and hold time for start and stop conditions
0940d24912e925 Sowjanya Komatineni 2019-02-12  202   *		in HS mode.
0940d24912e925 Sowjanya Komatineni 2019-02-12  203   * @has_interface_timing_reg: Has interface timing register to program the tuned
0940d24912e925 Sowjanya Komatineni 2019-02-12  204   *		timing settings.
6ad068ed63100f Laxman Dewangan     2012-08-19  205   */
6ad068ed63100f Laxman Dewangan     2012-08-19  206  struct tegra_i2c_hw_feature {
6ad068ed63100f Laxman Dewangan     2012-08-19  207  	bool has_continue_xfer_support;
2a2897bab2d3d5 Laxman Dewangan     2013-01-05  208  	bool has_per_pkt_xfer_complete_irq;
6f4664b2e2c2cf Laxman Dewangan     2015-06-30  209  	bool has_config_load_reg;
f1c2ff98065dce Dmitry Osipenko     2020-09-30  210  	u32 clk_divisor_hs_mode;
f1c2ff98065dce Dmitry Osipenko     2020-09-30  211  	u32 clk_divisor_std_mode;
f1c2ff98065dce Dmitry Osipenko     2020-09-30  212  	u32 clk_divisor_fast_mode;
f1c2ff98065dce Dmitry Osipenko     2020-09-30  213  	u32 clk_divisor_fast_plus_mode;
497fbe24987bd2 Shardar Shariff Md  2016-03-14  214  	bool has_multi_master_mode;
497fbe24987bd2 Shardar Shariff Md  2016-03-14  215  	bool has_slcg_override_reg;
c5907c6b96f187 Thierry Reding      2018-06-19  216  	bool has_mst_fifo;
b67d4530cdade7 Sowjanya Komatineni 2019-01-08  217  	const struct i2c_adapter_quirks *quirks;
ce9562424501de Sowjanya Komatineni 2019-02-12  218  	bool supports_bus_clear;
86c92b9965ff17 Sowjanya Komatineni 2019-02-12  219  	bool has_apb_dma;
f1c2ff98065dce Dmitry Osipenko     2020-09-30  220  	u32 tlow_std_mode;
f1c2ff98065dce Dmitry Osipenko     2020-09-30  221  	u32 thigh_std_mode;
f1c2ff98065dce Dmitry Osipenko     2020-09-30  222  	u32 tlow_fast_fastplus_mode;
f1c2ff98065dce Dmitry Osipenko     2020-09-30  223  	u32 thigh_fast_fastplus_mode;
19a765ab587022 Akhil R             2025-01-08  224  	u32 tlow_hs_mode;
19a765ab587022 Akhil R             2025-01-08  225  	u32 thigh_hs_mode;
0940d24912e925 Sowjanya Komatineni 2019-02-12  226  	u32 setup_hold_time_std_mode;
0940d24912e925 Sowjanya Komatineni 2019-02-12  227  	u32 setup_hold_time_fast_fast_plus_mode;
0940d24912e925 Sowjanya Komatineni 2019-02-12  228  	u32 setup_hold_time_hs_mode;
0940d24912e925 Sowjanya Komatineni 2019-02-12  229  	bool has_interface_timing_reg;
19a765ab587022 Akhil R             2025-01-08  230  	bool has_hs_mode_support;
6ad068ed63100f Laxman Dewangan     2012-08-19 @231  };
6ad068ed63100f Laxman Dewangan     2012-08-19  232  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 2/5] dt-bindings: i2c: nvidia,tegra20-i2c: Document Tegra264 I2C

[Kartik Rajput]

Tegra264 has 17 generic I2C controllers, two of which are in always-on
partition of the SoC. In addition to the features supported by Tegra194
it also supports a MUTEX register to allow sharing the same I2C instance
across multiple firmware.

Document compatible string "nvidia,tegra264-i2c" for Tegra264 I2C.

Signed-off-by: Kartik Rajput <kkartik@nvidia.com>
---
 .../devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml         | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml b/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml
index b57ae6963e62..2a016359328e 100644
--- a/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml
+++ b/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml
@@ -80,6 +80,12 @@ properties:
           support for 64 KiB transactions whereas earlier chips supported no
           more than 4 KiB per transactions.
         const: nvidia,tegra194-i2c
+      - description: |
+          Tegra264 has 17 generic I2C controllers, two of which are in the AON
+          (always-on) partition of the SoC. In addition to the features from
+          T194, a MUTEX register is added to support use of the same I2C
+          instance across multiple firmware.
+        const: nvidia,tegra264-i2c
 
   reg:
     maxItems: 1
-- 
2.43.0

Re: [PATCH 2/5] dt-bindings: i2c: nvidia,tegra20-i2c: Document Tegra264 I2C

[Thierry Reding]

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

On Wed, Jan 08, 2025 at 04:36:17PM +0530, Kartik Rajput wrote:
> Tegra264 has 17 generic I2C controllers, two of which are in always-on
> partition of the SoC. In addition to the features supported by Tegra194
> it also supports a MUTEX register to allow sharing the same I2C instance
> across multiple firmware.
> 
> Document compatible string "nvidia,tegra264-i2c" for Tegra264 I2C.
> 
> Signed-off-by: Kartik Rajput <kkartik@nvidia.com>
> ---
>  .../devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml         | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml b/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml
> index b57ae6963e62..2a016359328e 100644
> --- a/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml
> +++ b/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml
> @@ -80,6 +80,12 @@ properties:
>            support for 64 KiB transactions whereas earlier chips supported no
>            more than 4 KiB per transactions.
>          const: nvidia,tegra194-i2c
> +      - description: |
> +          Tegra264 has 17 generic I2C controllers, two of which are in the AON
> +          (always-on) partition of the SoC. In addition to the features from
> +          T194, a MUTEX register is added to support use of the same I2C

Maybe spell out Tegra194 above for consistency?

> +          instance across multiple firmware.

I don't know if this last sentence makes much sense in a DT bindings
document, but doesn't hurt either. Maybe s/firmware/firmwares/.

Thierry

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

Re: [PATCH 2/5] dt-bindings: i2c: nvidia,tegra20-i2c: Document Tegra264 I2C

[Kartik Rajput]

On Wed, 2025-01-08 at 17:47 +0100, Thierry Reding wrote:
> On Wed, Jan 08, 2025 at 04:36:17PM +0530, Kartik Rajput wrote:
> > Tegra264 has 17 generic I2C controllers, two of which are in
> > always-on
> > partition of the SoC. In addition to the features supported by
> > Tegra194
> > it also supports a MUTEX register to allow sharing the same I2C
> > instance
> > across multiple firmware.
> > 
> > Document compatible string "nvidia,tegra264-i2c" for Tegra264 I2C.
> > 
> > Signed-off-by: Kartik Rajput <kkartik@nvidia.com>
> > ---
> >  .../devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml         | 6
> > ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/i2c/nvidia,tegra20-
> > i2c.yaml b/Documentation/devicetree/bindings/i2c/nvidia,tegra20-
> > i2c.yaml
> > index b57ae6963e62..2a016359328e 100644
> > --- a/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml
> > +++ b/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml
> > @@ -80,6 +80,12 @@ properties:
> >            support for 64 KiB transactions whereas earlier chips
> > supported no
> >            more than 4 KiB per transactions.
> >          const: nvidia,tegra194-i2c
> > +      - description: |
> > +          Tegra264 has 17 generic I2C controllers, two of which
> > are in the AON
> > +          (always-on) partition of the SoC. In addition to the
> > features from
> > +          T194, a MUTEX register is added to support use of the
> > same I2C
> 
> Maybe spell out Tegra194 above for consistency?
> 
> > +          instance across multiple firmware.
> 
> I don't know if this last sentence makes much sense in a DT bindings
> document, but doesn't hurt either. Maybe s/firmware/firmwares/.
> 
> Thierry

ACK. I will update this in the next patch set.

Re: [PATCH 2/5] dt-bindings: i2c: nvidia,tegra20-i2c: Document Tegra264 I2C

[Rob Herring]

On Wed, Jan 08, 2025 at 04:36:17PM +0530, Kartik Rajput wrote:
> Tegra264 has 17 generic I2C controllers, two of which are in always-on
> partition of the SoC. In addition to the features supported by Tegra194
> it also supports a MUTEX register to allow sharing the same I2C instance
> across multiple firmware.
> 
> Document compatible string "nvidia,tegra264-i2c" for Tegra264 I2C.
> 
> Signed-off-by: Kartik Rajput <kkartik@nvidia.com>
> ---
>  .../devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml         | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml b/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml
> index b57ae6963e62..2a016359328e 100644
> --- a/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml
> +++ b/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml
> @@ -80,6 +80,12 @@ properties:
>            support for 64 KiB transactions whereas earlier chips supported no
>            more than 4 KiB per transactions.
>          const: nvidia,tegra194-i2c
> +      - description: |

Don't need '|' if no formatting.

> +          Tegra264 has 17 generic I2C controllers, two of which are in the AON
> +          (always-on) partition of the SoC. In addition to the features from
> +          T194, a MUTEX register is added to support use of the same I2C
> +          instance across multiple firmware.
> +        const: nvidia,tegra264-i2c
>  
>    reg:
>      maxItems: 1
> -- 
> 2.43.0
> 

Re: [PATCH 2/5] dt-bindings: i2c: nvidia,tegra20-i2c: Document Tegra264 I2C

[Kartik Rajput]

Thanks Rob!

On Fri, 2025-01-10 at 09:55 -0600, Rob Herring wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Wed, Jan 08, 2025 at 04:36:17PM +0530, Kartik Rajput wrote:
> > Tegra264 has 17 generic I2C controllers, two of which are in
> > always-on
> > partition of the SoC. In addition to the features supported by
> > Tegra194
> > it also supports a MUTEX register to allow sharing the same I2C
> > instance
> > across multiple firmware.
> > 
> > Document compatible string "nvidia,tegra264-i2c" for Tegra264 I2C.
> > 
> > Signed-off-by: Kartik Rajput <kkartik@nvidia.com>
> > ---
> >  .../devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml         | 6
> > ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/i2c/nvidia,tegra20-
> > i2c.yaml b/Documentation/devicetree/bindings/i2c/nvidia,tegra20-
> > i2c.yaml
> > index b57ae6963e62..2a016359328e 100644
> > --- a/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml
> > +++ b/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml
> > @@ -80,6 +80,12 @@ properties:
> >            support for 64 KiB transactions whereas earlier chips
> > supported no
> >            more than 4 KiB per transactions.
> >          const: nvidia,tegra194-i2c
> > +      - description: |
> 
> Don't need '|' if no formatting.
> 

ACK. Will fix this in v2.

Thanks & Regards,
Kartik

[PATCH 3/5] i2c: tegra: Add Tegra264 support

[Kartik Rajput]

From: Akhil R <akhilrajeev@nvidia.com>

Add support for Tegra264 SoC which supports 17 generic I2C controllers,
two of which are in the AON (always-on) partition of the SoC. Tegra264
I2C supports all the features supported by Tegra194 I2C controllers.

Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
Signed-off-by: Kartik Rajput <kkartik@nvidia.com>
---
 drivers/i2c/busses/i2c-tegra.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 7b97c6d347ee..cf05937cb826 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -1646,7 +1646,35 @@ static const struct tegra_i2c_hw_feature tegra194_i2c_hw = {
 	.has_hs_mode_support = true,
 };
 
+static const struct tegra_i2c_hw_feature tegra264_i2c_hw = {
+	.has_continue_xfer_support = true,
+	.has_per_pkt_xfer_complete_irq = true,
+	.clk_divisor_hs_mode = 1,
+	.clk_divisor_std_mode = 0x1d,
+	.clk_divisor_fast_mode = 0x15,
+	.clk_divisor_fast_plus_mode = 0x8,
+	.has_config_load_reg = true,
+	.has_multi_master_mode = true,
+	.has_slcg_override_reg = true,
+	.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,
+	.thigh_fast_fastplus_mode = 0x2,
+	.tlow_hs_mode = 0x4,
+	.thigh_hs_mode = 0x2,
+	.setup_hold_time_std_mode = 0x08080808,
+	.setup_hold_time_fast_fast_plus_mode = 0x02020202,
+	.setup_hold_time_hs_mode = 0x090909,
+	.has_interface_timing_reg = true,
+	.has_hs_mode_support = true,
+};
+
 static const struct of_device_id tegra_i2c_of_match[] = {
+	{ .compatible = "nvidia,tegra264-i2c", .data = &tegra264_i2c_hw, },
 	{ .compatible = "nvidia,tegra194-i2c", .data = &tegra194_i2c_hw, },
 	{ .compatible = "nvidia,tegra186-i2c", .data = &tegra186_i2c_hw, },
 #if IS_ENABLED(CONFIG_ARCH_TEGRA_210_SOC)
-- 
2.43.0

Re: [PATCH 3/5] i2c: tegra: Add Tegra264 support

[Mukesh Kumar Savaliya]

Hi Kartik,

On 1/8/2025 4:36 PM, Kartik Rajput wrote:
> From: Akhil R <akhilrajeev@nvidia.com>
> 
> Add support for Tegra264 SoC which supports 17 generic I2C controllers,
> two of which are in the AON (always-on) partition of the SoC. Tegra264
> I2C supports all the features supported by Tegra194 I2C controllers.
> 
> Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
> Signed-off-by: Kartik Rajput <kkartik@nvidia.com>
> ---
>   drivers/i2c/busses/i2c-tegra.c | 28 ++++++++++++++++++++++++++++
>   1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index 7b97c6d347ee..cf05937cb826 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -1646,7 +1646,35 @@ static const struct tegra_i2c_hw_feature tegra194_i2c_hw = {
>   	.has_hs_mode_support = true,
>   };
>   
> +static const struct tegra_i2c_hw_feature tegra264_i2c_hw = {
I could see 7 controllers have been already added. And this may keep 
growing.
Can we make either default set which is common for most of and change 
only sepcific fields ?

Second option - read these fields from DT and overwrite default if it's 
mentioned in DTSI.

Please review and see if this makes sense. what others say ?
> +	.has_continue_xfer_support = true,
> +	.has_per_pkt_xfer_complete_irq = true,
> +	.clk_divisor_hs_mode = 1,
> +	.clk_divisor_std_mode = 0x1d,
> +	.clk_divisor_fast_mode = 0x15,
> +	.clk_divisor_fast_plus_mode = 0x8,
> +	.has_config_load_reg = true,
> +	.has_multi_master_mode = true,
> +	.has_slcg_override_reg = true,
> +	.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,
> +	.thigh_fast_fastplus_mode = 0x2,
> +	.tlow_hs_mode = 0x4,
> +	.thigh_hs_mode = 0x2,
> +	.setup_hold_time_std_mode = 0x08080808,
> +	.setup_hold_time_fast_fast_plus_mode = 0x02020202,
> +	.setup_hold_time_hs_mode = 0x090909,
> +	.has_interface_timing_reg = true,
> +	.has_hs_mode_support = true,
> +};
> +
>   static const struct of_device_id tegra_i2c_of_match[] = {
> +	{ .compatible = "nvidia,tegra264-i2c", .data = &tegra264_i2c_hw, },
>   	{ .compatible = "nvidia,tegra194-i2c", .data = &tegra194_i2c_hw, },
>   	{ .compatible = "nvidia,tegra186-i2c", .data = &tegra186_i2c_hw, },
>   #if IS_ENABLED(CONFIG_ARCH_TEGRA_210_SOC)

Re: [PATCH 3/5] i2c: tegra: Add Tegra264 support

[Thierry Reding]

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

On Thu, Jan 09, 2025 at 10:13:48AM +0530, Mukesh Kumar Savaliya wrote:
> Hi Kartik,
> 
> On 1/8/2025 4:36 PM, Kartik Rajput wrote:
> > From: Akhil R <akhilrajeev@nvidia.com>
> > 
> > Add support for Tegra264 SoC which supports 17 generic I2C controllers,
> > two of which are in the AON (always-on) partition of the SoC. Tegra264
> > I2C supports all the features supported by Tegra194 I2C controllers.
> > 
> > Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
> > Signed-off-by: Kartik Rajput <kkartik@nvidia.com>
> > ---
> >   drivers/i2c/busses/i2c-tegra.c | 28 ++++++++++++++++++++++++++++
> >   1 file changed, 28 insertions(+)
> > 
> > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> > index 7b97c6d347ee..cf05937cb826 100644
> > --- a/drivers/i2c/busses/i2c-tegra.c
> > +++ b/drivers/i2c/busses/i2c-tegra.c
> > @@ -1646,7 +1646,35 @@ static const struct tegra_i2c_hw_feature tegra194_i2c_hw = {
> >   	.has_hs_mode_support = true,
> >   };
> > +static const struct tegra_i2c_hw_feature tegra264_i2c_hw = {
> I could see 7 controllers have been already added. And this may keep
> growing.

I'm not sure I understand the concern here. This is IP that's been in
use ever since the first Tegra chip was released about 15 years ago.
It's quite normal that the list of supported hardware will grow over
time. At the same time there will be occasional improvements of the
hardware that require certain parameterization.

> Can we make either default set which is common for most of and change only
> sepcific fields ?

It's difficult to do. These are const structures on purpose so that they
can go into .rodata, so as such there's no good way to reuse defaults. I
suppose we could do something like add preprocessor defines, but I doubt
that they would make things any better (these are quite fine-grained, so
macros would likely only cover one or two fields at a time).

> Second option - read these fields from DT and overwrite default if it's
> mentioned in DTSI.

Some information is already parsed from DT. What's in this structure can
all be derived from the compatible string, hence why it's associated
with the compatible string via the of_device_id table. Moreover, we
cannot move any of this information out into device tree (at least not
for existing chips) because it would break DT ABI.

> Please review and see if this makes sense. what others say ?

I'm always open to suggestions, but I also don't see this as very
problematic. It's data that is cleanly structured out, not difficult to
maintain and doesn't take up a huge amount of space.

Thierry

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

Re: [PATCH 3/5] i2c: tegra: Add Tegra264 support

[Mukesh Kumar Savaliya]

Thanks Thierry !

On 1/9/2025 3:54 PM, Thierry Reding wrote:
> On Thu, Jan 09, 2025 at 10:13:48AM +0530, Mukesh Kumar Savaliya wrote:
>> Hi Kartik,
>>
>> On 1/8/2025 4:36 PM, Kartik Rajput wrote:
>>> From: Akhil R <akhilrajeev@nvidia.com>
>>>
>>> Add support for Tegra264 SoC which supports 17 generic I2C controllers,
>>> two of which are in the AON (always-on) partition of the SoC. Tegra264
>>> I2C supports all the features supported by Tegra194 I2C controllers.
>>>
>>> Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
>>> Signed-off-by: Kartik Rajput <kkartik@nvidia.com>
>>> ---
>>>    drivers/i2c/busses/i2c-tegra.c | 28 ++++++++++++++++++++++++++++
>>>    1 file changed, 28 insertions(+)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
>>> index 7b97c6d347ee..cf05937cb826 100644
>>> --- a/drivers/i2c/busses/i2c-tegra.c
>>> +++ b/drivers/i2c/busses/i2c-tegra.c
>>> @@ -1646,7 +1646,35 @@ static const struct tegra_i2c_hw_feature tegra194_i2c_hw = {
>>>    	.has_hs_mode_support = true,
>>>    };
>>> +static const struct tegra_i2c_hw_feature tegra264_i2c_hw = {
>> I could see 7 controllers have been already added. And this may keep
>> growing.
> 
> I'm not sure I understand the concern here. This is IP that's been in
> use ever since the first Tegra chip was released about 15 years ago.
> It's quite normal that the list of supported hardware will grow over
> time. At the same time there will be occasional improvements of the
> hardware that require certain parameterization.
> 
yes, i understand it can grow with new controllers. Was trying to 
optimize the growing list with common fields.

Example: tegra30_i2c_hw and tegra20_i2c_hw has one field changing
from 20 fields. So was thinking after seeing this commonality.

One suggestion: can one structure be default and then delta can be 
overridden ?

No concern if no other way as you mentioned below.

>> Can we make either default set which is common for most of and change only
>> sepcific fields ?
> 
> It's difficult to do. These are const structures on purpose so that they
> can go into .rodata, so as such there's no good way to reuse defaults. I
> suppose we could do something like add preprocessor defines, but I doubt
> that they would make things any better (these are quite fine-grained, so
> macros would likely only cover one or two fields at a time).
> 
Sure. Let's wait for others opinion. I understand complexity.

>> Second option - read these fields from DT and overwrite default if it's
>> mentioned in DTSI.
> 
> Some information is already parsed from DT. What's in this structure can
> all be derived from the compatible string, hence why it's associated
> with the compatible string via the of_device_id table. Moreover, we
> cannot move any of this information out into device tree (at least not
> for existing chips) because it would break DT ABI.
> 
Got it.
>> Please review and see if this makes sense. what others say ?
> 
> I'm always open to suggestions, but I also don't see this as very
> problematic. It's data that is cleanly structured out, not difficult to
> maintain and doesn't take up a huge amount of space.
> 
I Agree.
> Thierry

[PATCH 4/5] i2c: tegra: Add support for SW Mutex register

[Kartik Rajput]

From: Akhil R <akhilrajeev@nvidia.com>

Add support for SW Mutex register introduced in Tegra264 to provide
an option to share the interface between multiple firmware and/or
Virtual Machines.

However, the hardware does not ensure any protection based on the
values. The driver/firmware should honor the peer who already holds
the mutex.

Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
Signed-off-by: Kartik Rajput <kkartik@nvidia.com>
---
 drivers/i2c/busses/i2c-tegra.c | 126 +++++++++++++++++++++++++++++----
 1 file changed, 111 insertions(+), 15 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index cf05937cb826..a5974af5b1af 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -135,6 +135,11 @@
 #define I2C_MST_FIFO_STATUS_TX			GENMASK(23, 16)
 #define I2C_MST_FIFO_STATUS_RX			GENMASK(7, 0)
 
+#define I2C_SW_MUTEX				0x0ec
+#define I2C_SW_MUTEX_REQUEST			GENMASK(3, 0)
+#define I2C_SW_MUTEX_GRANT			GENMASK(7, 4)
+#define I2C_SW_MUTEX_ID				9
+
 /* configuration load timeout in microseconds */
 #define I2C_CONFIG_LOAD_TIMEOUT			1000000
 
@@ -202,6 +207,7 @@ enum msg_end_type {
  *		in HS mode.
  * @has_interface_timing_reg: Has interface timing register to program the tuned
  *		timing settings.
+ * @has_mutex: Has Mutex register for mutual exclusion with other firmwares or VM.
  */
 struct tegra_i2c_hw_feature {
 	bool has_continue_xfer_support;
@@ -228,6 +234,7 @@ struct tegra_i2c_hw_feature {
 	u32 setup_hold_time_hs_mode;
 	bool has_interface_timing_reg;
 	bool has_hs_mode_support;
+	bool has_mutex;
 };
 
 /**
@@ -371,6 +378,99 @@ static void i2c_readsl(struct tegra_i2c_dev *i2c_dev, void *data,
 	readsl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg), data, len);
 }
 
+static int tegra_i2c_poll_register(struct tegra_i2c_dev *i2c_dev,
+				   u32 reg, u32 mask, u32 delay_us,
+				   u32 timeout_us)
+{
+	void __iomem *addr = i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg);
+	u32 val;
+
+	if (!i2c_dev->atomic_mode)
+		return readl_relaxed_poll_timeout(addr, val, !(val & mask),
+						  delay_us, timeout_us);
+
+	return readl_relaxed_poll_timeout_atomic(addr, val, !(val & mask),
+						 delay_us, timeout_us);
+}
+
+static int tegra_i2c_mutex_trylock(struct tegra_i2c_dev *i2c_dev)
+{
+	u32 val, id;
+
+	val = i2c_readl(i2c_dev, I2C_SW_MUTEX);
+	id = FIELD_GET(I2C_SW_MUTEX_GRANT, val);
+	if (id != 0)
+		return 0;
+
+	val = FIELD_PREP(I2C_SW_MUTEX_REQUEST, I2C_SW_MUTEX_ID);
+	i2c_writel(i2c_dev, val, I2C_SW_MUTEX);
+
+	val = i2c_readl(i2c_dev, I2C_SW_MUTEX);
+	id = FIELD_GET(I2C_SW_MUTEX_GRANT, val);
+
+	if (id != I2C_SW_MUTEX_ID)
+		return 0;
+
+	return 1;
+}
+
+static void tegra_i2c_mutex_lock(struct tegra_i2c_dev *i2c_dev)
+{
+	/* Poll until mutex is acquired. */
+	while (tegra_i2c_mutex_trylock(i2c_dev))
+		cpu_relax();
+}
+
+static void tegra_i2c_mutex_unlock(struct tegra_i2c_dev *i2c_dev)
+{
+	u32 val, id;
+
+	val = i2c_readl(i2c_dev, I2C_SW_MUTEX);
+	id = FIELD_GET(I2C_SW_MUTEX_GRANT, val);
+
+	if (WARN_ON(id != I2C_SW_MUTEX_ID))
+		return;
+
+	i2c_writel(i2c_dev, 0, I2C_SW_MUTEX);
+}
+
+static void tegra_i2c_bus_lock(struct i2c_adapter *adapter,
+			       unsigned int flags)
+{
+	struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(adapter);
+
+	rt_mutex_lock_nested(&adapter->bus_lock, i2c_adapter_depth(adapter));
+	tegra_i2c_mutex_lock(i2c_dev);
+}
+
+static int tegra_i2c_bus_trylock(struct i2c_adapter *adapter,
+				  unsigned int flags)
+{
+	struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(adapter);
+	int ret;
+
+	ret = rt_mutex_trylock(&adapter->bus_lock);
+	if (ret)
+		ret = tegra_i2c_mutex_trylock(i2c_dev);
+
+	return ret;
+}
+
+static void tegra_i2c_bus_unlock(struct i2c_adapter *adapter,
+				 unsigned int flags)
+{
+	struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(adapter);
+
+	rt_mutex_unlock(&adapter->bus_lock);
+	tegra_i2c_mutex_unlock(i2c_dev);
+}
+
+static const struct i2c_lock_operations tegra_i2c_lock_ops = {
+	.lock_bus = tegra_i2c_bus_lock,
+	.trylock_bus = tegra_i2c_bus_trylock,
+	.unlock_bus = tegra_i2c_bus_unlock,
+};
+
 static void tegra_i2c_mask_irq(struct tegra_i2c_dev *i2c_dev, u32 mask)
 {
 	u32 int_mask;
@@ -546,21 +646,6 @@ static void tegra_i2c_vi_init(struct tegra_i2c_dev *i2c_dev)
 	i2c_writel(i2c_dev, 0x0, I2C_TLOW_SEXT);
 }
 
-static int tegra_i2c_poll_register(struct tegra_i2c_dev *i2c_dev,
-				   u32 reg, u32 mask, u32 delay_us,
-				   u32 timeout_us)
-{
-	void __iomem *addr = i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg);
-	u32 val;
-
-	if (!i2c_dev->atomic_mode)
-		return readl_relaxed_poll_timeout(addr, val, !(val & mask),
-						  delay_us, timeout_us);
-
-	return readl_relaxed_poll_timeout_atomic(addr, val, !(val & mask),
-						 delay_us, timeout_us);
-}
-
 static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev)
 {
 	u32 mask, val, offset;
@@ -1497,6 +1582,7 @@ static const struct tegra_i2c_hw_feature tegra20_i2c_hw = {
 	.setup_hold_time_fast_fast_plus_mode = 0x0,
 	.setup_hold_time_hs_mode = 0x0,
 	.has_interface_timing_reg = false,
+	.has_mutex = false,
 };
 
 static const struct tegra_i2c_hw_feature tegra30_i2c_hw = {
@@ -1521,6 +1607,7 @@ static const struct tegra_i2c_hw_feature tegra30_i2c_hw = {
 	.setup_hold_time_fast_fast_plus_mode = 0x0,
 	.setup_hold_time_hs_mode = 0x0,
 	.has_interface_timing_reg = false,
+	.has_mutex = false,
 };
 
 static const struct tegra_i2c_hw_feature tegra114_i2c_hw = {
@@ -1545,6 +1632,7 @@ static const struct tegra_i2c_hw_feature tegra114_i2c_hw = {
 	.setup_hold_time_fast_fast_plus_mode = 0x0,
 	.setup_hold_time_hs_mode = 0x0,
 	.has_interface_timing_reg = false,
+	.has_mutex = false,
 };
 
 static const struct tegra_i2c_hw_feature tegra124_i2c_hw = {
@@ -1569,6 +1657,7 @@ static const struct tegra_i2c_hw_feature tegra124_i2c_hw = {
 	.setup_hold_time_fast_fast_plus_mode = 0x0,
 	.setup_hold_time_hs_mode = 0x0,
 	.has_interface_timing_reg = true,
+	.has_mutex = false,
 };
 
 static const struct tegra_i2c_hw_feature tegra210_i2c_hw = {
@@ -1593,6 +1682,7 @@ static const struct tegra_i2c_hw_feature tegra210_i2c_hw = {
 	.setup_hold_time_fast_fast_plus_mode = 0,
 	.setup_hold_time_hs_mode = 0,
 	.has_interface_timing_reg = true,
+	.has_mutex = false,
 };
 
 static const struct tegra_i2c_hw_feature tegra186_i2c_hw = {
@@ -1617,6 +1707,7 @@ static const struct tegra_i2c_hw_feature tegra186_i2c_hw = {
 	.setup_hold_time_fast_fast_plus_mode = 0,
 	.setup_hold_time_hs_mode = 0,
 	.has_interface_timing_reg = true,
+	.has_mutex = false,
 };
 
 static const struct tegra_i2c_hw_feature tegra194_i2c_hw = {
@@ -1644,6 +1735,7 @@ static const struct tegra_i2c_hw_feature tegra194_i2c_hw = {
 	.setup_hold_time_hs_mode = 0x090909,
 	.has_interface_timing_reg = true,
 	.has_hs_mode_support = true,
+	.has_mutex = false,
 };
 
 static const struct tegra_i2c_hw_feature tegra264_i2c_hw = {
@@ -1671,6 +1763,7 @@ static const struct tegra_i2c_hw_feature tegra264_i2c_hw = {
 	.setup_hold_time_hs_mode = 0x090909,
 	.has_interface_timing_reg = true,
 	.has_hs_mode_support = true,
+	.has_mutex = true,
 };
 
 static const struct of_device_id tegra_i2c_of_match[] = {
@@ -1875,6 +1968,9 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 	i2c_dev->adapter.nr = pdev->id;
 	ACPI_COMPANION_SET(&i2c_dev->adapter.dev, ACPI_COMPANION(&pdev->dev));
 
+	if (i2c_dev->hw->has_mutex)
+		i2c_dev->adapter.lock_ops = &tegra_i2c_lock_ops;
+
 	if (i2c_dev->hw->supports_bus_clear)
 		i2c_dev->adapter.bus_recovery_info = &tegra_i2c_recovery_info;
 
-- 
2.43.0

Re: [PATCH 4/5] i2c: tegra: Add support for SW Mutex register

[Thierry Reding]

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

On Wed, Jan 08, 2025 at 04:36:19PM +0530, Kartik Rajput wrote:
> From: Akhil R <akhilrajeev@nvidia.com>
> 
> Add support for SW Mutex register introduced in Tegra264 to provide

The spelling is a bit inconsistent. Earlier you referred to this as SW
MUTEX register, which makes sense if that's what it's called. But then
you call it "SW Mutex" register here. If you don't want to refer to it
by the documented name, it should probably be "SW mutex" instead.

> an option to share the interface between multiple firmware and/or

"firmwares"

> Virtual Machines.

"virtual machines" or "VMs"

> However, the hardware does not ensure any protection based on the
> values. The driver/firmware should honor the peer who already holds
> the mutex.
> 
> Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
> Signed-off-by: Kartik Rajput <kkartik@nvidia.com>
> ---
>  drivers/i2c/busses/i2c-tegra.c | 126 +++++++++++++++++++++++++++++----
>  1 file changed, 111 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index cf05937cb826..a5974af5b1af 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -135,6 +135,11 @@
>  #define I2C_MST_FIFO_STATUS_TX			GENMASK(23, 16)
>  #define I2C_MST_FIFO_STATUS_RX			GENMASK(7, 0)
>  
> +#define I2C_SW_MUTEX				0x0ec
> +#define I2C_SW_MUTEX_REQUEST			GENMASK(3, 0)
> +#define I2C_SW_MUTEX_GRANT			GENMASK(7, 4)
> +#define I2C_SW_MUTEX_ID				9
> +
>  /* configuration load timeout in microseconds */
>  #define I2C_CONFIG_LOAD_TIMEOUT			1000000
>  
> @@ -202,6 +207,7 @@ enum msg_end_type {
>   *		in HS mode.
>   * @has_interface_timing_reg: Has interface timing register to program the tuned
>   *		timing settings.
> + * @has_mutex: Has Mutex register for mutual exclusion with other firmwares or VM.

"mutex"

>   */
>  struct tegra_i2c_hw_feature {
>  	bool has_continue_xfer_support;
> @@ -228,6 +234,7 @@ struct tegra_i2c_hw_feature {
>  	u32 setup_hold_time_hs_mode;
>  	bool has_interface_timing_reg;
>  	bool has_hs_mode_support;
> +	bool has_mutex;
>  };
>  
>  /**
> @@ -371,6 +378,99 @@ static void i2c_readsl(struct tegra_i2c_dev *i2c_dev, void *data,
>  	readsl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg), data, len);
>  }
>  
> +static int tegra_i2c_poll_register(struct tegra_i2c_dev *i2c_dev,
> +				   u32 reg, u32 mask, u32 delay_us,
> +				   u32 timeout_us)
> +{
> +	void __iomem *addr = i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg);
> +	u32 val;
> +
> +	if (!i2c_dev->atomic_mode)
> +		return readl_relaxed_poll_timeout(addr, val, !(val & mask),
> +						  delay_us, timeout_us);
> +
> +	return readl_relaxed_poll_timeout_atomic(addr, val, !(val & mask),
> +						 delay_us, timeout_us);
> +}
> +
> +static int tegra_i2c_mutex_trylock(struct tegra_i2c_dev *i2c_dev)
> +{
> +	u32 val, id;
> +
> +	val = i2c_readl(i2c_dev, I2C_SW_MUTEX);
> +	id = FIELD_GET(I2C_SW_MUTEX_GRANT, val);
> +	if (id != 0)
> +		return 0;
> +
> +	val = FIELD_PREP(I2C_SW_MUTEX_REQUEST, I2C_SW_MUTEX_ID);
> +	i2c_writel(i2c_dev, val, I2C_SW_MUTEX);
> +
> +	val = i2c_readl(i2c_dev, I2C_SW_MUTEX);
> +	id = FIELD_GET(I2C_SW_MUTEX_GRANT, val);
> +
> +	if (id != I2C_SW_MUTEX_ID)
> +		return 0;
> +
> +	return 1;
> +}
> +
> +static void tegra_i2c_mutex_lock(struct tegra_i2c_dev *i2c_dev)
> +{
> +	/* Poll until mutex is acquired. */
> +	while (tegra_i2c_mutex_trylock(i2c_dev))
> +		cpu_relax();
> +}

Don't we want to use a timeout here? Otherwise we risk blocking the
thread that this runs on if some firmware decides not to release the
mutex.

Also, is the logic not the wrong way around? I.e. trylock returns true
if the hardware mutex was successfully locked, in which case it doesn't
make sense to keep spinning, right? Or do I misunderstand how this
works?

Thierry

> +
> +static void tegra_i2c_mutex_unlock(struct tegra_i2c_dev *i2c_dev)
> +{
> +	u32 val, id;
> +
> +	val = i2c_readl(i2c_dev, I2C_SW_MUTEX);
> +	id = FIELD_GET(I2C_SW_MUTEX_GRANT, val);
> +
> +	if (WARN_ON(id != I2C_SW_MUTEX_ID))
> +		return;
> +
> +	i2c_writel(i2c_dev, 0, I2C_SW_MUTEX);
> +}
> +
> +static void tegra_i2c_bus_lock(struct i2c_adapter *adapter,
> +			       unsigned int flags)
> +{
> +	struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(adapter);
> +
> +	rt_mutex_lock_nested(&adapter->bus_lock, i2c_adapter_depth(adapter));
> +	tegra_i2c_mutex_lock(i2c_dev);
> +}
> +
> +static int tegra_i2c_bus_trylock(struct i2c_adapter *adapter,
> +				  unsigned int flags)
> +{
> +	struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(adapter);
> +	int ret;
> +
> +	ret = rt_mutex_trylock(&adapter->bus_lock);
> +	if (ret)
> +		ret = tegra_i2c_mutex_trylock(i2c_dev);
> +
> +	return ret;
> +}
> +
> +static void tegra_i2c_bus_unlock(struct i2c_adapter *adapter,
> +				 unsigned int flags)
> +{
> +	struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(adapter);
> +
> +	rt_mutex_unlock(&adapter->bus_lock);
> +	tegra_i2c_mutex_unlock(i2c_dev);
> +}
> +
> +static const struct i2c_lock_operations tegra_i2c_lock_ops = {
> +	.lock_bus = tegra_i2c_bus_lock,
> +	.trylock_bus = tegra_i2c_bus_trylock,
> +	.unlock_bus = tegra_i2c_bus_unlock,
> +};
> +
>  static void tegra_i2c_mask_irq(struct tegra_i2c_dev *i2c_dev, u32 mask)
>  {
>  	u32 int_mask;
> @@ -546,21 +646,6 @@ static void tegra_i2c_vi_init(struct tegra_i2c_dev *i2c_dev)
>  	i2c_writel(i2c_dev, 0x0, I2C_TLOW_SEXT);
>  }
>  
> -static int tegra_i2c_poll_register(struct tegra_i2c_dev *i2c_dev,
> -				   u32 reg, u32 mask, u32 delay_us,
> -				   u32 timeout_us)
> -{
> -	void __iomem *addr = i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg);
> -	u32 val;
> -
> -	if (!i2c_dev->atomic_mode)
> -		return readl_relaxed_poll_timeout(addr, val, !(val & mask),
> -						  delay_us, timeout_us);
> -
> -	return readl_relaxed_poll_timeout_atomic(addr, val, !(val & mask),
> -						 delay_us, timeout_us);
> -}
> -
>  static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev)
>  {
>  	u32 mask, val, offset;
> @@ -1497,6 +1582,7 @@ static const struct tegra_i2c_hw_feature tegra20_i2c_hw = {
>  	.setup_hold_time_fast_fast_plus_mode = 0x0,
>  	.setup_hold_time_hs_mode = 0x0,
>  	.has_interface_timing_reg = false,
> +	.has_mutex = false,
>  };
>  
>  static const struct tegra_i2c_hw_feature tegra30_i2c_hw = {
> @@ -1521,6 +1607,7 @@ static const struct tegra_i2c_hw_feature tegra30_i2c_hw = {
>  	.setup_hold_time_fast_fast_plus_mode = 0x0,
>  	.setup_hold_time_hs_mode = 0x0,
>  	.has_interface_timing_reg = false,
> +	.has_mutex = false,
>  };
>  
>  static const struct tegra_i2c_hw_feature tegra114_i2c_hw = {
> @@ -1545,6 +1632,7 @@ static const struct tegra_i2c_hw_feature tegra114_i2c_hw = {
>  	.setup_hold_time_fast_fast_plus_mode = 0x0,
>  	.setup_hold_time_hs_mode = 0x0,
>  	.has_interface_timing_reg = false,
> +	.has_mutex = false,
>  };
>  
>  static const struct tegra_i2c_hw_feature tegra124_i2c_hw = {
> @@ -1569,6 +1657,7 @@ static const struct tegra_i2c_hw_feature tegra124_i2c_hw = {
>  	.setup_hold_time_fast_fast_plus_mode = 0x0,
>  	.setup_hold_time_hs_mode = 0x0,
>  	.has_interface_timing_reg = true,
> +	.has_mutex = false,
>  };
>  
>  static const struct tegra_i2c_hw_feature tegra210_i2c_hw = {
> @@ -1593,6 +1682,7 @@ static const struct tegra_i2c_hw_feature tegra210_i2c_hw = {
>  	.setup_hold_time_fast_fast_plus_mode = 0,
>  	.setup_hold_time_hs_mode = 0,
>  	.has_interface_timing_reg = true,
> +	.has_mutex = false,
>  };
>  
>  static const struct tegra_i2c_hw_feature tegra186_i2c_hw = {
> @@ -1617,6 +1707,7 @@ static const struct tegra_i2c_hw_feature tegra186_i2c_hw = {
>  	.setup_hold_time_fast_fast_plus_mode = 0,
>  	.setup_hold_time_hs_mode = 0,
>  	.has_interface_timing_reg = true,
> +	.has_mutex = false,
>  };
>  
>  static const struct tegra_i2c_hw_feature tegra194_i2c_hw = {
> @@ -1644,6 +1735,7 @@ static const struct tegra_i2c_hw_feature tegra194_i2c_hw = {
>  	.setup_hold_time_hs_mode = 0x090909,
>  	.has_interface_timing_reg = true,
>  	.has_hs_mode_support = true,
> +	.has_mutex = false,
>  };
>  
>  static const struct tegra_i2c_hw_feature tegra264_i2c_hw = {
> @@ -1671,6 +1763,7 @@ static const struct tegra_i2c_hw_feature tegra264_i2c_hw = {
>  	.setup_hold_time_hs_mode = 0x090909,
>  	.has_interface_timing_reg = true,
>  	.has_hs_mode_support = true,
> +	.has_mutex = true,
>  };
>  
>  static const struct of_device_id tegra_i2c_of_match[] = {
> @@ -1875,6 +1968,9 @@ static int tegra_i2c_probe(struct platform_device *pdev)
>  	i2c_dev->adapter.nr = pdev->id;
>  	ACPI_COMPANION_SET(&i2c_dev->adapter.dev, ACPI_COMPANION(&pdev->dev));
>  
> +	if (i2c_dev->hw->has_mutex)
> +		i2c_dev->adapter.lock_ops = &tegra_i2c_lock_ops;
> +
>  	if (i2c_dev->hw->supports_bus_clear)
>  		i2c_dev->adapter.bus_recovery_info = &tegra_i2c_recovery_info;
>  
> -- 
> 2.43.0
> 

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

Re: [PATCH 4/5] i2c: tegra: Add support for SW Mutex register

[Kartik Rajput]

Thanks for reviewing the patch Thierry!


On Wed, 2025-01-08 at 18:01 +0100, Thierry Reding wrote:
> On Wed, Jan 08, 2025 at 04:36:19PM +0530, Kartik Rajput wrote:
> > From: Akhil R <akhilrajeev@nvidia.com>
> > 
> > Add support for SW Mutex register introduced in Tegra264 to provide
> 
> The spelling is a bit inconsistent. Earlier you referred to this as
> SW
> MUTEX register, which makes sense if that's what it's called. But
> then
> you call it "SW Mutex" register here. If you don't want to refer to
> it
> by the documented name, it should probably be "SW mutex" instead.
> 
> > an option to share the interface between multiple firmware and/or
> 
> "firmwares"
> 
> > Virtual Machines.
> 
> "virtual machines" or "VMs"
> 

ACK. I will fix this in the next patchset.

> > However, the hardware does not ensure any protection based on the
> > values. The driver/firmware should honor the peer who already holds
> > the mutex.
> > 
> > Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
> > Signed-off-by: Kartik Rajput <kkartik@nvidia.com>
> > ---
> >  drivers/i2c/busses/i2c-tegra.c | 126
> > +++++++++++++++++++++++++++++----
> >  1 file changed, 111 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-tegra.c
> > b/drivers/i2c/busses/i2c-tegra.c
> > index cf05937cb826..a5974af5b1af 100644
> > --- a/drivers/i2c/busses/i2c-tegra.c
> > +++ b/drivers/i2c/busses/i2c-tegra.c
> > @@ -135,6 +135,11 @@
> >  #define I2C_MST_FIFO_STATUS_TX                 GENMASK(23, 16)
> >  #define I2C_MST_FIFO_STATUS_RX                 GENMASK(7, 0)
> >  
> > +#define I2C_SW_MUTEX                           0x0ec
> > +#define I2C_SW_MUTEX_REQUEST                   GENMASK(3, 0)
> > +#define I2C_SW_MUTEX_GRANT                     GENMASK(7, 4)
> > +#define I2C_SW_MUTEX_ID                                9
> > +
> >  /* configuration load timeout in microseconds */
> >  #define I2C_CONFIG_LOAD_TIMEOUT                        1000000
> >  
> > @@ -202,6 +207,7 @@ enum msg_end_type {
> >   *             in HS mode.
> >   * @has_interface_timing_reg: Has interface timing register to
> > program the tuned
> >   *             timing settings.
> > + * @has_mutex: Has Mutex register for mutual exclusion with other
> > firmwares or VM.
> 
> "mutex"
> 
> >   */
> >  struct tegra_i2c_hw_feature {
> >         bool has_continue_xfer_support;
> > @@ -228,6 +234,7 @@ struct tegra_i2c_hw_feature {
> >         u32 setup_hold_time_hs_mode;
> >         bool has_interface_timing_reg;
> >         bool has_hs_mode_support;
> > +       bool has_mutex;
> >  };
> >  
> >  /**
> > @@ -371,6 +378,99 @@ static void i2c_readsl(struct tegra_i2c_dev
> > *i2c_dev, void *data,
> >         readsl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg),
> > data, len);
> >  }
> >  
> > +static int tegra_i2c_poll_register(struct tegra_i2c_dev *i2c_dev,
> > +                                  u32 reg, u32 mask, u32 delay_us,
> > +                                  u32 timeout_us)
> > +{
> > +       void __iomem *addr = i2c_dev->base +
> > tegra_i2c_reg_addr(i2c_dev, reg);
> > +       u32 val;
> > +
> > +       if (!i2c_dev->atomic_mode)
> > +               return readl_relaxed_poll_timeout(addr, val, !(val
> > & mask),
> > +                                                 delay_us,
> > timeout_us);
> > +
> > +       return readl_relaxed_poll_timeout_atomic(addr, val, !(val &
> > mask),
> > +                                                delay_us,
> > timeout_us);
> > +}
> > +
> > +static int tegra_i2c_mutex_trylock(struct tegra_i2c_dev *i2c_dev)
> > +{
> > +       u32 val, id;
> > +
> > +       val = i2c_readl(i2c_dev, I2C_SW_MUTEX);
> > +       id = FIELD_GET(I2C_SW_MUTEX_GRANT, val);
> > +       if (id != 0)
> > +               return 0;
> > +
> > +       val = FIELD_PREP(I2C_SW_MUTEX_REQUEST, I2C_SW_MUTEX_ID);
> > +       i2c_writel(i2c_dev, val, I2C_SW_MUTEX);
> > +
> > +       val = i2c_readl(i2c_dev, I2C_SW_MUTEX);
> > +       id = FIELD_GET(I2C_SW_MUTEX_GRANT, val);
> > +
> > +       if (id != I2C_SW_MUTEX_ID)
> > +               return 0;
> > +
> > +       return 1;
> > +}
> > +
> > +static void tegra_i2c_mutex_lock(struct tegra_i2c_dev *i2c_dev)
> > +{
> > +       /* Poll until mutex is acquired. */
> > +       while (tegra_i2c_mutex_trylock(i2c_dev))
> > +               cpu_relax();
> > +}
> 
> Don't we want to use a timeout here? Otherwise we risk blocking the
> thread that this runs on if some firmware decides not to release the
> mutex.
> 

We can continue to access the controller with a warning in case
the request times out.

Something like this?

static void tegra_i2c_mutex_lock(struct tegra_i2c_dev *i2c_dev)
{
        unsigned int num_retries = 25; // Move this to a macro.

        /* Poll until mutex is acquired or timeout. */
        while ( --num_retries && !tegra_i2c_mutex_trylock(i2c_dev))
                msleep(1);

        WARN_ON(!num_retries);
}


> Also, is the logic not the wrong way around? I.e. trylock returns
> true
> if the hardware mutex was successfully locked, in which case it
> doesn't
> make sense to keep spinning, right? Or do I misunderstand how this
> works?
> 
> Thierry
> 

The logic is indeed wrong here, apologies for the oversight. I will fix
this in the next patchset.


> > +
> > +static void tegra_i2c_mutex_unlock(struct tegra_i2c_dev *i2c_dev)
> > +{
> > +       u32 val, id;
> > +
> > +       val = i2c_readl(i2c_dev, I2C_SW_MUTEX);
> > +       id = FIELD_GET(I2C_SW_MUTEX_GRANT, val);
> > +
> > +       if (WARN_ON(id != I2C_SW_MUTEX_ID))
> > +               return;
> > +
> > +       i2c_writel(i2c_dev, 0, I2C_SW_MUTEX);
> > +}
> > +
> > +static void tegra_i2c_bus_lock(struct i2c_adapter *adapter,
> > +                              unsigned int flags)
> > +{
> > +       struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(adapter);
> > +
> > +       rt_mutex_lock_nested(&adapter->bus_lock,
> > i2c_adapter_depth(adapter));
> > +       tegra_i2c_mutex_lock(i2c_dev);
> > +}
> > +
> > +static int tegra_i2c_bus_trylock(struct i2c_adapter *adapter,
> > +                                 unsigned int flags)
> > +{
> > +       struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(adapter);
> > +       int ret;
> > +
> > +       ret = rt_mutex_trylock(&adapter->bus_lock);
> > +       if (ret)
> > +               ret = tegra_i2c_mutex_trylock(i2c_dev);
> > +
> > +       return ret;
> > +}
> > +
> > +static void tegra_i2c_bus_unlock(struct i2c_adapter *adapter,
> > +                                unsigned int flags)
> > +{
> > +       struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(adapter);
> > +
> > +       rt_mutex_unlock(&adapter->bus_lock);
> > +       tegra_i2c_mutex_unlock(i2c_dev);
> > +}
> > +
> > +static const struct i2c_lock_operations tegra_i2c_lock_ops = {
> > +       .lock_bus = tegra_i2c_bus_lock,
> > +       .trylock_bus = tegra_i2c_bus_trylock,
> > +       .unlock_bus = tegra_i2c_bus_unlock,
> > +};
> > +
> >  static void tegra_i2c_mask_irq(struct tegra_i2c_dev *i2c_dev, u32
> > mask)
> >  {
> >         u32 int_mask;
> > @@ -546,21 +646,6 @@ static void tegra_i2c_vi_init(struct
> > tegra_i2c_dev *i2c_dev)
> >         i2c_writel(i2c_dev, 0x0, I2C_TLOW_SEXT);
> >  }
> >  
> > -static int tegra_i2c_poll_register(struct tegra_i2c_dev *i2c_dev,
> > -                                  u32 reg, u32 mask, u32 delay_us,
> > -                                  u32 timeout_us)
> > -{
> > -       void __iomem *addr = i2c_dev->base +
> > tegra_i2c_reg_addr(i2c_dev, reg);
> > -       u32 val;
> > -
> > -       if (!i2c_dev->atomic_mode)
> > -               return readl_relaxed_poll_timeout(addr, val, !(val
> > & mask),
> > -                                                 delay_us,
> > timeout_us);
> > -
> > -       return readl_relaxed_poll_timeout_atomic(addr, val, !(val &
> > mask),
> > -                                                delay_us,
> > timeout_us);
> > -}
> > -
> >  static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev)
> >  {
> >         u32 mask, val, offset;
> > @@ -1497,6 +1582,7 @@ static const struct tegra_i2c_hw_feature
> > tegra20_i2c_hw = {
> >         .setup_hold_time_fast_fast_plus_mode = 0x0,
> >         .setup_hold_time_hs_mode = 0x0,
> >         .has_interface_timing_reg = false,
> > +       .has_mutex = false,
> >  };
> >  
> >  static const struct tegra_i2c_hw_feature tegra30_i2c_hw = {
> > @@ -1521,6 +1607,7 @@ static const struct tegra_i2c_hw_feature
> > tegra30_i2c_hw = {
> >         .setup_hold_time_fast_fast_plus_mode = 0x0,
> >         .setup_hold_time_hs_mode = 0x0,
> >         .has_interface_timing_reg = false,
> > +       .has_mutex = false,
> >  };
> >  
> >  static const struct tegra_i2c_hw_feature tegra114_i2c_hw = {
> > @@ -1545,6 +1632,7 @@ static const struct tegra_i2c_hw_feature
> > tegra114_i2c_hw = {
> >         .setup_hold_time_fast_fast_plus_mode = 0x0,
> >         .setup_hold_time_hs_mode = 0x0,
> >         .has_interface_timing_reg = false,
> > +       .has_mutex = false,
> >  };
> >  
> >  static const struct tegra_i2c_hw_feature tegra124_i2c_hw = {
> > @@ -1569,6 +1657,7 @@ static const struct tegra_i2c_hw_feature
> > tegra124_i2c_hw = {
> >         .setup_hold_time_fast_fast_plus_mode = 0x0,
> >         .setup_hold_time_hs_mode = 0x0,
> >         .has_interface_timing_reg = true,
> > +       .has_mutex = false,
> >  };
> >  
> >  static const struct tegra_i2c_hw_feature tegra210_i2c_hw = {
> > @@ -1593,6 +1682,7 @@ static const struct tegra_i2c_hw_feature
> > tegra210_i2c_hw = {
> >         .setup_hold_time_fast_fast_plus_mode = 0,
> >         .setup_hold_time_hs_mode = 0,
> >         .has_interface_timing_reg = true,
> > +       .has_mutex = false,
> >  };
> >  
> >  static const struct tegra_i2c_hw_feature tegra186_i2c_hw = {
> > @@ -1617,6 +1707,7 @@ static const struct tegra_i2c_hw_feature
> > tegra186_i2c_hw = {
> >         .setup_hold_time_fast_fast_plus_mode = 0,
> >         .setup_hold_time_hs_mode = 0,
> >         .has_interface_timing_reg = true,
> > +       .has_mutex = false,
> >  };
> >  
> >  static const struct tegra_i2c_hw_feature tegra194_i2c_hw = {
> > @@ -1644,6 +1735,7 @@ static const struct tegra_i2c_hw_feature
> > tegra194_i2c_hw = {
> >         .setup_hold_time_hs_mode = 0x090909,
> >         .has_interface_timing_reg = true,
> >         .has_hs_mode_support = true,
> > +       .has_mutex = false,
> >  };
> >  
> >  static const struct tegra_i2c_hw_feature tegra264_i2c_hw = {
> > @@ -1671,6 +1763,7 @@ static const struct tegra_i2c_hw_feature
> > tegra264_i2c_hw = {
> >         .setup_hold_time_hs_mode = 0x090909,
> >         .has_interface_timing_reg = true,
> >         .has_hs_mode_support = true,
> > +       .has_mutex = true,
> >  };
> >  
> >  static const struct of_device_id tegra_i2c_of_match[] = {
> > @@ -1875,6 +1968,9 @@ static int tegra_i2c_probe(struct
> > platform_device *pdev)
> >         i2c_dev->adapter.nr = pdev->id;
> >         ACPI_COMPANION_SET(&i2c_dev->adapter.dev,
> > ACPI_COMPANION(&pdev->dev));
> >  
> > +       if (i2c_dev->hw->has_mutex)
> > +               i2c_dev->adapter.lock_ops = &tegra_i2c_lock_ops;
> > +
> >         if (i2c_dev->hw->supports_bus_clear)
> >                 i2c_dev->adapter.bus_recovery_info =
> > &tegra_i2c_recovery_info;
> >  
> > -- 
> > 2.43.0
> > 

[PATCH 5/5] i2c: tegra: Do not configure DMA if not supported

[Kartik Rajput]

On Tegra264, not all I2C controllers support DMA, this causes failures
when function tegra_i2c_init_dma() is called.

Ensure that "dmas" device-tree property is present before initializing
DMA in function tegra_i2c_init_dma().

Signed-off-by: Kartik Rajput <kkartik@nvidia.com>
---
 drivers/i2c/busses/i2c-tegra.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index a5974af5b1af..9957802fa4ed 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -546,6 +546,9 @@ static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev)
 	if (IS_VI(i2c_dev))
 		return 0;
 
+	if (!device_property_present(i2c_dev->dev, "dmas"))
+		return 0;
+
 	if (i2c_dev->hw->has_apb_dma) {
 		if (!IS_ENABLED(CONFIG_TEGRA20_APB_DMA)) {
 			dev_dbg(i2c_dev->dev, "APB DMA support not enabled\n");
-- 
2.43.0

Re: [PATCH 5/5] i2c: tegra: Do not configure DMA if not supported

[Thierry Reding]

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

On Wed, Jan 08, 2025 at 04:36:20PM +0530, Kartik Rajput wrote:
> On Tegra264, not all I2C controllers support DMA, this causes failures
> when function tegra_i2c_init_dma() is called.

What exactly does it mean when you say "not all I2C controllers support
DMA"? Do they not have the necessary interface to the GPC DMA or are
there not sufficient DMA channels to support all I2C controllers?

Thierry

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

Re: [PATCH 5/5] i2c: tegra: Do not configure DMA if not supported

[Kartik Rajput]

On Thu, 2025-01-09 at 11:33 +0100, Thierry Reding wrote:
> On Wed, Jan 08, 2025 at 04:36:20PM +0530, Kartik Rajput wrote:
> > On Tegra264, not all I2C controllers support DMA, this causes
> > failures
> > when function tegra_i2c_init_dma() is called.
> 
> What exactly does it mean when you say "not all I2C controllers
> support
> DMA"? Do they not have the necessary interface to the GPC DMA or are
> there not sufficient DMA channels to support all I2C controllers?
> 
> Thierry

They do not have the neccessary interface to the GPC DMA.

Re: [PATCH 0/5] Add I2C support for Tegra264

[Thierry Reding]

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

On Wed, Jan 08, 2025 at 04:36:15PM +0530, Kartik Rajput wrote:
> Following series of patches add support for Tegra264 and High Speed (HS)
> Mode in i2c-tegra.c driver.
> 
> Akhil R (3):
>   i2c: tegra: Add HS mode support
>   i2c: tegra: Add Tegra264 support
>   i2c: tegra: Add support for SW Mutex register
> 
> Kartik Rajput (2):
>   dt-bindings: i2c: nvidia,tegra20-i2c: Document Tegra264 I2C
>   i2c: tegra: Do not configure DMA if not supported

It'd probably make sense to restructure this series a little bit. It's
customary to have the DT patch first. It would then make more sense to
add preparatory patches and then follow those up by the Tegra264 support
patches, so something like this:

	dt-bindings: i2c: nvidia,tegra20-i2c: Document Tegra264 I2C
	i2c: tegra: Do not configure DMA if not supported
	i2c: tegra: Add HS mode support
	i2c: tegra: Add support for SW Mutex register
	i2c: tegra: Add Tegra264 support

Thierry

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

Re: [PATCH 0/5] Add I2C support for Tegra264

[Kartik Rajput]

On Thu, 2025-01-09 at 11:37 +0100, Thierry Reding wrote:
> On Wed, Jan 08, 2025 at 04:36:15PM +0530, Kartik Rajput wrote:
> > Following series of patches add support for Tegra264 and High Speed
> > (HS)
> > Mode in i2c-tegra.c driver.
> > 
> > Akhil R (3):
> >   i2c: tegra: Add HS mode support
> >   i2c: tegra: Add Tegra264 support
> >   i2c: tegra: Add support for SW Mutex register
> > 
> > Kartik Rajput (2):
> >   dt-bindings: i2c: nvidia,tegra20-i2c: Document Tegra264 I2C
> >   i2c: tegra: Do not configure DMA if not supported
> 
> It'd probably make sense to restructure this series a little bit.
> It's
> customary to have the DT patch first. It would then make more sense
> to
> add preparatory patches and then follow those up by the Tegra264
> support
> patches, so something like this:
> 
>         dt-bindings: i2c: nvidia,tegra20-i2c: Document Tegra264 I2C
>         i2c: tegra: Do not configure DMA if not supported
>         i2c: tegra: Add HS mode support
>         i2c: tegra: Add support for SW Mutex register
>         i2c: tegra: Add Tegra264 support
> 
> Thierry

Thanks for reviewing the patches in this series Thierry!
I will re-arrange this and post a new patch set for review.

Thanks & Regards,
Kartik