Re: [PATCH v5 3/4] i2c: tegra: Add logic to support different register offsets

[Jon Hunter <jonathanh@nvidia.com>]

On 07/01/2026 14:26, Kartik Rajput wrote:
> Tegra410 use different offsets for existing I2C registers, update
> the logic to use appropriate offsets per SoC.
> 
> As the registers offsets are now also defined for dvc and vi, following
> function are not required and they are removed:
>   - tegra_i2c_reg_addr(): No translation required.
>   - dvc_writel(): Replaced with i2c_writel() with DVC check.
>   - dvc_readl(): Replaced with i2c_readl().
> 
> Signed-off-by: Kartik Rajput <kkartik@nvidia.com>
> ---
> Changes in v2:
> 	* Replace individual is_dvc and is_vi flags with an I2C variant.
> 	* Add tegra20_dvc_i2c_hw and tegra210_vi_i2c_hw in a separate
> 	  patch.
> 	* Use calculated offsets for tegra20_dvc_i2c_regs and
> 	  tegra210_vi_i2c_regs.
> 	* Initialize registers only if they are used on the given SoC.
> ---
>   drivers/i2c/busses/i2c-tegra.c | 393 +++++++++++++++++++++------------
>   1 file changed, 251 insertions(+), 142 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index cb6455fb3ee1..821e7627e56e 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c

...

> @@ -159,6 +137,149 @@
>    */
>   #define I2C_PIO_MODE_PREFERRED_LEN		32
>   
> +struct tegra_i2c_regs {
> +	unsigned int cnfg;
> +	unsigned int status;
> +	unsigned int sl_cnfg;
> +	unsigned int sl_addr1;
> +	unsigned int sl_addr2;
> +	unsigned int tlow_sext;
> +	unsigned int tx_fifo;
> +	unsigned int rx_fifo;
> +	unsigned int packet_transfer_status;
> +	unsigned int fifo_control;
> +	unsigned int fifo_status;
> +	unsigned int int_mask;
> +	unsigned int int_status;
> +	unsigned int clk_divisor;
> +	unsigned int bus_clear_cnfg;
> +	unsigned int bus_clear_status;
> +	unsigned int config_load;
> +	unsigned int clken_override;
> +	unsigned int interface_timing_0;
> +	unsigned int interface_timing_1;
> +	unsigned int hs_interface_timing_0;
> +	unsigned int hs_interface_timing_1;
> +	unsigned int master_reset_cntrl;
> +	unsigned int mst_fifo_control;
> +	unsigned int mst_fifo_status;
> +	unsigned int sw_mutex;
> +	unsigned int dvc_ctrl_reg1;
> +	unsigned int dvc_ctrl_reg3;
> +	unsigned int dvc_status;
> +};
> +
> +static const struct tegra_i2c_regs tegra20_i2c_regs = {
> +	.cnfg = 0x000,
> +	.status = 0x01c,
> +	.sl_cnfg = 0x020,
> +	.sl_addr1 = 0x02c,
> +	.sl_addr2 = 0x030,
> +	.tx_fifo = 0x050,
> +	.rx_fifo = 0x054,
> +	.packet_transfer_status = 0x058,
> +	.fifo_control = 0x05c,
> +	.fifo_status = 0x060,
> +	.int_mask = 0x064,
> +	.int_status = 0x068,
> +	.clk_divisor = 0x06c,
> +	.bus_clear_cnfg = 0x084,
> +	.bus_clear_status = 0x088,
> +	.config_load = 0x08c,
> +	.clken_override = 0x090,
> +	.interface_timing_0 = 0x094,
> +	.interface_timing_1 = 0x098,
> +	.hs_interface_timing_0 = 0x09c,
> +	.hs_interface_timing_1 = 0x0a0,
> +	.master_reset_cntrl = 0x0a8,
> +	.mst_fifo_control = 0x0b4,
> +	.mst_fifo_status = 0x0b8,
> +};
> +
> +#if IS_ENABLED(CONFIG_ARCH_TEGRA_2x_SOC)
> +static const struct tegra_i2c_regs tegra20_dvc_i2c_regs = {
> +	.dvc_ctrl_reg1 = 0x000,
> +	.dvc_ctrl_reg3 = 0x008,
> +	.dvc_status = 0x00c,

Why do we need to add the DVC? These don't change.

> +	.cnfg = 0x040,
> +	.status = 0x05c,
> +	.tx_fifo = 0x060,
> +	.rx_fifo = 0x064,
> +	.packet_transfer_status = 0x068,
> +	.fifo_control = 0x06c,
> +	.fifo_status = 0x070,
> +	.int_mask = 0x074,
> +	.int_status = 0x078,
> +	.clk_divisor = 0x07c,
> +	.bus_clear_cnfg = 0x0c4,

Shouldn't this be 0x094? We are only adding 0x10 if greater or equal to 
TX_FIFO.

> +	.bus_clear_status = 0x0c8,

Shouldn't this be 0x09c?

> +	.config_load = 0x0cc,
> +	.clken_override = 0x0d0,
> +	.interface_timing_0 = 0x0d4,
> +	.interface_timing_1 = 0x0d8,
> +	.hs_interface_timing_0 = 0x0dc,
> +	.hs_interface_timing_1 = 0x0e0,
> +	.master_reset_cntrl = 0x0e8,
> +	.mst_fifo_control = 0x0c4,

So now we have 2 regs at 0x0c4.

> +	.mst_fifo_status = 0x0c8,
> +};

...

> -/*
> - * If necessary, i2c_writel() and i2c_readl() will offset the register
> - * in order to talk to the I2C block inside the DVC block.
> - */
> -static u32 tegra_i2c_reg_addr(struct tegra_i2c_dev *i2c_dev, unsigned int reg)
> -{
> -	if (IS_DVC(i2c_dev))
> -		reg += (reg >= I2C_TX_FIFO) ? 0x10 : 0x40;
> -	else if (IS_VI(i2c_dev))
> -		reg = 0xc00 + (reg << 2);
> -
> -	return reg;
> -}
> -
>   static void i2c_writel(struct tegra_i2c_dev *i2c_dev, u32 val, unsigned int reg)
>   {
> -	writel_relaxed(val, i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg));
> +	writel_relaxed(val, i2c_dev->base + reg);
>   
>   	/* read back register to make sure that register writes completed */
> -	if (reg != I2C_TX_FIFO)
> -		readl_relaxed(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg));
> +	if (!IS_DVC(i2c_dev) && reg != i2c_dev->hw->regs->tx_fifo)
> +		readl_relaxed(i2c_dev->base + reg);

I am not sure why this changed. Why is this now dependent on !IS_DVC?

I understand that DVC has a different I2C_TX_FIFO address and but don't 
we just want ...

  if (reg != i2c_dev->hw->regs->tx_fifo)
      readl_relaxed(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg));


>   	else if (IS_VI(i2c_dev))
> -		readl_relaxed(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, I2C_INT_STATUS));
> +		readl_relaxed(i2c_dev->base + i2c_dev->hw->regs->int_status);
>   }

Jon

-- 
nvpublic