Re: [PATCH v9 2/4] i2c: tegra: Add HS mode support

Akhil R
2025-11-06T06:18:02Z

On Fri, 24 Oct 2025 16:28:50 +0100, Jon Hunter wrote:
> On 01/10/2025 07:47, Kartik Rajput wrote:

...

>> /**
>> @@ -678,16 +685,28 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
>> tegra_i2c_vi_init(i2c_dev);
>>
>> switch (t->bus_freq_hz) {
>> - case I2C_MAX_STANDARD_MODE_FREQ + 1 ... I2C_MAX_FAST_MODE_PLUS_FREQ:
>> default:
>> + if (!i2c_dev->hw->has_hs_mode_support)
>> + t->bus_freq_hz = I2C_MAX_FAST_MODE_PLUS_FREQ;
>> + fallthrough;
>> +
>>
> This looks odd. I guess this is carry over from the previous code, but
> now it looks very odd to someone reviewing the code after this change
> has been made. We need to make the code here more logical so that the
> reader stands a chance of understanding the new logic.

Would it look better if I update as below?

@@ -678,8 +685,26 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
tegra_i2c_vi_init(i2c_dev);

switch (t->bus_freq_hz) {
- case I2C_MAX_STANDARD_MODE_FREQ + 1 ... I2C_MAX_FAST_MODE_PLUS_FREQ:
default:
+ /*
+ * When HS mode is supported, the non-hs timing registers will be used for the
+ * master code byte for transition to HS mode. As per the spec, the 8 bit master
+ * code should be sent at max 400kHz. Therefore, limit the bus speed to fast mode.
+ * Whereas when HS mode is not supported, allow the highest speed mode capable.
+ */
+ if (i2c_dev->hw->has_hs_mode_support) {
+ tlow = i2c_dev->hw->tlow_fast_fastplus_mode;
+ thigh = i2c_dev->hw->thigh_fast_fastplus_mode;
+ tsu_thd = i2c_dev->hw->setup_hold_time_fast_fast_plus_mode;
+ non_hs_mode = i2c_dev->hw->clk_divisor_fast_mode;
+
+ break;
+ } else {
+ t->bus_freq_hz = I2C_MAX_FAST_MODE_PLUS_FREQ;
+ }
+ fallthrough;
+
+ case I2C_MAX_STANDARD_MODE_FREQ + 1 ... I2C_MAX_FAST_MODE_PLUS_FREQ:
tlow = i2c_dev->hw->tlow_fast_fastplus_mode;
thigh = i2c_dev->hw->thigh_fast_fastplus_mode;
tsu_thd = i2c_dev->hw->setup_hold_time_fast_fast_plus_mode;
@@ -688,6 +713,7 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)

...

>> @@ -717,6 +736,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);
>> + }
>> +
>
> I still think all of the above needs a bit of work.

I suppose the above section can be as is since HS mode registers are independent
of other speed modes. Any suggestions or thoughts?

Regards,
Akhil