Re: [PATCH v5 3/4] i2c: tegra: Add logic to support different register offsets
From: Kartik Rajput
Date: Thu Jan 08 2026 - 05:19:26 EST
On 07/01/26 20:41, Jon Hunter wrote:
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@xxxxxxxxxx>
---
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.
For consistency, as we are consolidating all the regs in `struct tegra_i2c_regs` and moving away from static macro defines.
+ .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.
You're right, I will fix this in the next patch set.
+ .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.
Actually, the mst_fifo_* are used if `has_mst_fifo = true` for that SoC.
These can be removed from DVC and VI regs.
+ .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));
Because dvc_writel() only had a writel statement. I tried to avoid any extra reads/writes
after writing to dvc specific registers to keep the logic same, but this changed the logic
for other registers.
So, looks like I need to revert the change to remove dvc_writel.
Thanks,
Kartik