| 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: <69d4316a-f841-46a3-89de-6fa4412db25f@nvidia.com>
Date: Thu, 8 Jan 2026 14:03:06 +0530
From: Kartik Rajput <kkartik@...dia.com>
To: Jon Hunter <jonathanh@...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/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@...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.
>
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
Powered by blists - more mailing lists