| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <85d5583c-a679-454e-959d-438e7726ffa4@nvidia.com>
Date: Wed, 7 Jan 2026 15:11:58 +0000
From: Jon Hunter <jonathanh@...dia.com>
To: Kartik Rajput <kkartik@...dia.com>, ldewangan@...dia.com,
digetx@...il.com, andi.shyti@...nel.org, thierry.reding@...il.com,
akhilrajeev@...dia.com, smangipudi@...dia.com, linux-i2c@...r.kernel.org,
linux-tegra@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 3/4] i2c: tegra: Add logic to support different
register offsets
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@...dia.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
Powered by blists - more mailing lists