From: Akhil R <akhilrajeev@nvidia.com>
Add support for HS (High Speed) mode transfers, which is supported by
Tegra194 onwards.
Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
Signed-off-by: Kartik Rajput <kkartik@nvidia.com>
---
v3 -> v5:
* Set has_hs_mode_support to false for unsupported SoCs.
v2 -> v3:
* Document tlow_hs_mode and thigh_hs_mode.
v1 -> v2:
* Document has_hs_mode_support.
* Add a check to set the frequency to fastmode+ if the device
does not support HS mode but the requested frequency is more
than fastmode+.
---
drivers/i2c/busses/i2c-tegra.c | 33 +++++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index d908e5e3f0af..6f816de8b3af 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -91,6 +91,7 @@
#define I2C_HEADER_IE_ENABLE BIT(17)
#define I2C_HEADER_REPEAT_START BIT(16)
#define I2C_HEADER_CONTINUE_XFER BIT(15)
+#define I2C_HEADER_HS_MODE BIT(22)
#define I2C_HEADER_SLAVE_ADDR_SHIFT 1
#define I2C_BUS_CLEAR_CNFG 0x084
@@ -198,6 +199,8 @@ enum msg_end_type {
* @thigh_std_mode: High period of the clock in standard mode.
* @tlow_fast_fastplus_mode: Low period of the clock in fast/fast-plus modes.
* @thigh_fast_fastplus_mode: High period of the clock in fast/fast-plus modes.
+ * @tlow_hs_mode: Low period of the clock in HS mode.
+ * @thigh_hs_mode: High period of the clock in HS mode.
* @setup_hold_time_std_mode: Setup and hold time for start and stop conditions
* in standard mode.
* @setup_hold_time_fast_fast_plus_mode: Setup and hold time for start and stop
@@ -206,6 +209,7 @@ enum msg_end_type {
* in HS mode.
* @has_interface_timing_reg: Has interface timing register to program the tuned
* timing settings.
+ * @has_hs_mode_support: Has support for high speed (HS) mode transfers.
*/
struct tegra_i2c_hw_feature {
bool has_continue_xfer_support;
@@ -226,10 +230,13 @@ struct tegra_i2c_hw_feature {
u32 thigh_std_mode;
u32 tlow_fast_fastplus_mode;
u32 thigh_fast_fastplus_mode;
+ u32 tlow_hs_mode;
+ u32 thigh_hs_mode;
u32 setup_hold_time_std_mode;
u32 setup_hold_time_fast_fast_plus_mode;
u32 setup_hold_time_hs_mode;
bool has_interface_timing_reg;
+ bool has_hs_mode_support;
};
/**
@@ -717,6 +724,20 @@ 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);
+ } else if (t->bus_freq_hz > I2C_MAX_FAST_MODE_PLUS_FREQ) {
+ t->bus_freq_hz = I2C_MAX_FAST_MODE_PLUS_FREQ;
+ }
+
clk_multiplier = (tlow + thigh + 2) * (non_hs_mode + 1);
err = clk_set_rate(i2c_dev->div_clk,
@@ -1214,6 +1235,9 @@ static void tegra_i2c_push_packet_header(struct tegra_i2c_dev *i2c_dev,
if (msg->flags & I2C_M_RD)
packet_header |= I2C_HEADER_READ;
+ if (i2c_dev->timings.bus_freq_hz > I2C_MAX_FAST_MODE_PLUS_FREQ)
+ packet_header |= I2C_HEADER_HS_MODE;
+
if (i2c_dev->dma_mode && !i2c_dev->msg_read)
*dma_buf++ = packet_header;
else
@@ -1502,6 +1526,7 @@ static const struct tegra_i2c_hw_feature tegra20_i2c_hw = {
.setup_hold_time_fast_fast_plus_mode = 0x0,
.setup_hold_time_hs_mode = 0x0,
.has_interface_timing_reg = false,
+ .has_hs_mode_support = false,
};
static const struct tegra_i2c_hw_feature tegra30_i2c_hw = {
@@ -1527,6 +1552,7 @@ static const struct tegra_i2c_hw_feature tegra30_i2c_hw = {
.setup_hold_time_fast_fast_plus_mode = 0x0,
.setup_hold_time_hs_mode = 0x0,
.has_interface_timing_reg = false,
+ .has_hs_mode_support = false,
};
static const struct tegra_i2c_hw_feature tegra114_i2c_hw = {
@@ -1552,6 +1578,7 @@ static const struct tegra_i2c_hw_feature tegra114_i2c_hw = {
.setup_hold_time_fast_fast_plus_mode = 0x0,
.setup_hold_time_hs_mode = 0x0,
.has_interface_timing_reg = false,
+ .has_hs_mode_support = false,
};
static const struct tegra_i2c_hw_feature tegra124_i2c_hw = {
@@ -1577,6 +1604,7 @@ static const struct tegra_i2c_hw_feature tegra124_i2c_hw = {
.setup_hold_time_fast_fast_plus_mode = 0x0,
.setup_hold_time_hs_mode = 0x0,
.has_interface_timing_reg = true,
+ .has_hs_mode_support = false,
};
static const struct tegra_i2c_hw_feature tegra210_i2c_hw = {
@@ -1602,6 +1630,7 @@ static const struct tegra_i2c_hw_feature tegra210_i2c_hw = {
.setup_hold_time_fast_fast_plus_mode = 0,
.setup_hold_time_hs_mode = 0,
.has_interface_timing_reg = true,
+ .has_hs_mode_support = false,
};
static const struct tegra_i2c_hw_feature tegra186_i2c_hw = {
@@ -1627,6 +1656,7 @@ static const struct tegra_i2c_hw_feature tegra186_i2c_hw = {
.setup_hold_time_fast_fast_plus_mode = 0,
.setup_hold_time_hs_mode = 0,
.has_interface_timing_reg = true,
+ .has_hs_mode_support = false,
};
static const struct tegra_i2c_hw_feature tegra194_i2c_hw = {
@@ -1648,10 +1678,13 @@ static const struct tegra_i2c_hw_feature tegra194_i2c_hw = {
.thigh_std_mode = 0x7,
.tlow_fast_fastplus_mode = 0x2,
.thigh_fast_fastplus_mode = 0x2,
+ .tlow_hs_mode = 0x8,
+ .thigh_hs_mode = 0x3,
.setup_hold_time_std_mode = 0x08080808,
.setup_hold_time_fast_fast_plus_mode = 0x02020202,
.setup_hold_time_hs_mode = 0x090909,
.has_interface_timing_reg = true,
+ .has_hs_mode_support = true,
};
static const struct tegra_i2c_hw_feature tegra256_i2c_hw = {
--
2.43.0
On 17/09/2025 09:56, Kartik Rajput wrote:
> From: Akhil R <akhilrajeev@nvidia.com>
>
> Add support for HS (High Speed) mode transfers, which is supported by
> Tegra194 onwards.
>
> Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
> Signed-off-by: Kartik Rajput <kkartik@nvidia.com>
> ---
> v3 -> v5:
> * Set has_hs_mode_support to false for unsupported SoCs.
> v2 -> v3:
> * Document tlow_hs_mode and thigh_hs_mode.
> v1 -> v2:
> * Document has_hs_mode_support.
> * Add a check to set the frequency to fastmode+ if the device
> does not support HS mode but the requested frequency is more
> than fastmode+.
> ---
> drivers/i2c/busses/i2c-tegra.c | 33 +++++++++++++++++++++++++++++++++
> 1 file changed, 33 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index d908e5e3f0af..6f816de8b3af 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -91,6 +91,7 @@
> #define I2C_HEADER_IE_ENABLE BIT(17)
> #define I2C_HEADER_REPEAT_START BIT(16)
> #define I2C_HEADER_CONTINUE_XFER BIT(15)
> +#define I2C_HEADER_HS_MODE BIT(22)
> #define I2C_HEADER_SLAVE_ADDR_SHIFT 1
>
> #define I2C_BUS_CLEAR_CNFG 0x084
> @@ -198,6 +199,8 @@ enum msg_end_type {
> * @thigh_std_mode: High period of the clock in standard mode.
> * @tlow_fast_fastplus_mode: Low period of the clock in fast/fast-plus modes.
> * @thigh_fast_fastplus_mode: High period of the clock in fast/fast-plus modes.
> + * @tlow_hs_mode: Low period of the clock in HS mode.
> + * @thigh_hs_mode: High period of the clock in HS mode.
> * @setup_hold_time_std_mode: Setup and hold time for start and stop conditions
> * in standard mode.
> * @setup_hold_time_fast_fast_plus_mode: Setup and hold time for start and stop
> @@ -206,6 +209,7 @@ enum msg_end_type {
> * in HS mode.
> * @has_interface_timing_reg: Has interface timing register to program the tuned
> * timing settings.
> + * @has_hs_mode_support: Has support for high speed (HS) mode transfers.
> */
> struct tegra_i2c_hw_feature {
> bool has_continue_xfer_support;
> @@ -226,10 +230,13 @@ struct tegra_i2c_hw_feature {
> u32 thigh_std_mode;
> u32 tlow_fast_fastplus_mode;
> u32 thigh_fast_fastplus_mode;
> + u32 tlow_hs_mode;
> + u32 thigh_hs_mode;
> u32 setup_hold_time_std_mode;
> u32 setup_hold_time_fast_fast_plus_mode;
> u32 setup_hold_time_hs_mode;
> bool has_interface_timing_reg;
> + bool has_hs_mode_support;
> };
>
> /**
> @@ -717,6 +724,20 @@ 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);
> + } else if (t->bus_freq_hz > I2C_MAX_FAST_MODE_PLUS_FREQ) {
> + t->bus_freq_hz = I2C_MAX_FAST_MODE_PLUS_FREQ;
No mention in the changelog about this part. Looks like this is a fallback.
Should all of this be handled in the case statement for t->bus_freq_hz?
> + }
> +
> clk_multiplier = (tlow + thigh + 2) * (non_hs_mode + 1);
>
> err = clk_set_rate(i2c_dev->div_clk,
> @@ -1214,6 +1235,9 @@ static void tegra_i2c_push_packet_header(struct tegra_i2c_dev *i2c_dev,
> if (msg->flags & I2C_M_RD)
> packet_header |= I2C_HEADER_READ;
>
> + if (i2c_dev->timings.bus_freq_hz > I2C_MAX_FAST_MODE_PLUS_FREQ)
> + packet_header |= I2C_HEADER_HS_MODE;
> +
> if (i2c_dev->dma_mode && !i2c_dev->msg_read)
> *dma_buf++ = packet_header;
> else
> @@ -1502,6 +1526,7 @@ static const struct tegra_i2c_hw_feature tegra20_i2c_hw = {
> .setup_hold_time_fast_fast_plus_mode = 0x0,
> .setup_hold_time_hs_mode = 0x0,
> .has_interface_timing_reg = false,
> + .has_hs_mode_support = false,
> };
>
> static const struct tegra_i2c_hw_feature tegra30_i2c_hw = {
> @@ -1527,6 +1552,7 @@ static const struct tegra_i2c_hw_feature tegra30_i2c_hw = {
> .setup_hold_time_fast_fast_plus_mode = 0x0,
> .setup_hold_time_hs_mode = 0x0,
> .has_interface_timing_reg = false,
> + .has_hs_mode_support = false,
> };
>
> static const struct tegra_i2c_hw_feature tegra114_i2c_hw = {
> @@ -1552,6 +1578,7 @@ static const struct tegra_i2c_hw_feature tegra114_i2c_hw = {
> .setup_hold_time_fast_fast_plus_mode = 0x0,
> .setup_hold_time_hs_mode = 0x0,
> .has_interface_timing_reg = false,
> + .has_hs_mode_support = false,
> };
>
> static const struct tegra_i2c_hw_feature tegra124_i2c_hw = {
> @@ -1577,6 +1604,7 @@ static const struct tegra_i2c_hw_feature tegra124_i2c_hw = {
> .setup_hold_time_fast_fast_plus_mode = 0x0,
> .setup_hold_time_hs_mode = 0x0,
> .has_interface_timing_reg = true,
> + .has_hs_mode_support = false,
> };
>
> static const struct tegra_i2c_hw_feature tegra210_i2c_hw = {
> @@ -1602,6 +1630,7 @@ static const struct tegra_i2c_hw_feature tegra210_i2c_hw = {
> .setup_hold_time_fast_fast_plus_mode = 0,
> .setup_hold_time_hs_mode = 0,
> .has_interface_timing_reg = true,
> + .has_hs_mode_support = false,
> };
>
> static const struct tegra_i2c_hw_feature tegra186_i2c_hw = {
> @@ -1627,6 +1656,7 @@ static const struct tegra_i2c_hw_feature tegra186_i2c_hw = {
> .setup_hold_time_fast_fast_plus_mode = 0,
> .setup_hold_time_hs_mode = 0,
> .has_interface_timing_reg = true,
> + .has_hs_mode_support = false,
> };
>
> static const struct tegra_i2c_hw_feature tegra194_i2c_hw = {
> @@ -1648,10 +1678,13 @@ static const struct tegra_i2c_hw_feature tegra194_i2c_hw = {
> .thigh_std_mode = 0x7,
> .tlow_fast_fastplus_mode = 0x2,
> .thigh_fast_fastplus_mode = 0x2,
> + .tlow_hs_mode = 0x8,
> + .thigh_hs_mode = 0x3,
> .setup_hold_time_std_mode = 0x08080808,
> .setup_hold_time_fast_fast_plus_mode = 0x02020202,
> .setup_hold_time_hs_mode = 0x090909,
> .has_interface_timing_reg = true,
> + .has_hs_mode_support = true,
> };
>
> static const struct tegra_i2c_hw_feature tegra256_i2c_hw = {
--
nvpublic
On Wed, 17 Sep 2025 14:59:54 +0100, Jon Hunter wrote:
> On 17/09/2025 09:56, Kartik Rajput wrote:
>> From: Akhil R <akhilrajeev@nvidia.com>
>>
>> Add support for HS (High Speed) mode transfers, which is supported by
>> Tegra194 onwards.
>>
>> Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
>> Signed-off-by: Kartik Rajput <kkartik@nvidia.com>
>> ---
>> v3 -> v5:
>> * Set has_hs_mode_support to false for unsupported SoCs.
>> v2 -> v3:
>> * Document tlow_hs_mode and thigh_hs_mode.
>> v1 -> v2:
>> * Document has_hs_mode_support.
>> * Add a check to set the frequency to fastmode+ if the device
>> does not support HS mode but the requested frequency is more
>> than fastmode+.
>> ---
>> drivers/i2c/busses/i2c-tegra.c | 33 +++++++++++++++++++++++++++++++++
>> 1 file changed, 33 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
>> index d908e5e3f0af..6f816de8b3af 100644
>> --- a/drivers/i2c/busses/i2c-tegra.c
>> +++ b/drivers/i2c/busses/i2c-tegra.c
>> @@ -91,6 +91,7 @@
>> #define I2C_HEADER_IE_ENABLE BIT(17)
>> #define I2C_HEADER_REPEAT_START BIT(16)
>> #define I2C_HEADER_CONTINUE_XFER BIT(15)
>> +#define I2C_HEADER_HS_MODE BIT(22)
>> #define I2C_HEADER_SLAVE_ADDR_SHIFT 1
>>
>> #define I2C_BUS_CLEAR_CNFG 0x084
>> @@ -198,6 +199,8 @@ enum msg_end_type {
>> * @thigh_std_mode: High period of the clock in standard mode.
>> * @tlow_fast_fastplus_mode: Low period of the clock in fast/fast-plus modes.
>> * @thigh_fast_fastplus_mode: High period of the clock in fast/fast-plus modes.
>> + * @tlow_hs_mode: Low period of the clock in HS mode.
>> + * @thigh_hs_mode: High period of the clock in HS mode.
>> * @setup_hold_time_std_mode: Setup and hold time for start and stop conditions
>> * in standard mode.
>> * @setup_hold_time_fast_fast_plus_mode: Setup and hold time for start and stop
>> @@ -206,6 +209,7 @@ enum msg_end_type {
>> * in HS mode.
>> * @has_interface_timing_reg: Has interface timing register to program the tuned
>> * timing settings.
>> + * @has_hs_mode_support: Has support for high speed (HS) mode transfers.
>> */
>> struct tegra_i2c_hw_feature {
>> bool has_continue_xfer_support;
>> @@ -226,10 +230,13 @@ struct tegra_i2c_hw_feature {
>> u32 thigh_std_mode;
>> u32 tlow_fast_fastplus_mode;
>> u32 thigh_fast_fastplus_mode;
>> + u32 tlow_hs_mode;
>> + u32 thigh_hs_mode;
>> u32 setup_hold_time_std_mode;
>> u32 setup_hold_time_fast_fast_plus_mode;
>> u32 setup_hold_time_hs_mode;
>> bool has_interface_timing_reg;
>> + bool has_hs_mode_support;
>> };
>>
>> /**
>> @@ -717,6 +724,20 @@ 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);
>> + } else if (t->bus_freq_hz > I2C_MAX_FAST_MODE_PLUS_FREQ) {
>> + t->bus_freq_hz = I2C_MAX_FAST_MODE_PLUS_FREQ;
>
> No mention in the changelog about this part. Looks like this is a fallback.
>
> Should all of this be handled in the case statement for t->bus_freq_hz?
>
HS mode timing parameters are programmed in registers different from the other
speed modes. These registers does not affect the timing in other speed modes.
HS mode registers being used or not is determined by the packet header.
We may also want to program the regular timing registers, because it will be
used for the master code byte to transition to HS mode.
So, I guess, even if we move this to the switch statement, we might end up
doing something similar outside it.
Regards,
Akhil
On 18/09/2025 11:04, Akhil R wrote:
>
> On Wed, 17 Sep 2025 14:59:54 +0100, Jon Hunter wrote:
>> On 17/09/2025 09:56, Kartik Rajput wrote:
>>> From: Akhil R <akhilrajeev@nvidia.com>
>>>
>>> Add support for HS (High Speed) mode transfers, which is supported by
>>> Tegra194 onwards.
>>>
>>> Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
>>> Signed-off-by: Kartik Rajput <kkartik@nvidia.com>
>>> ---
>>> v3 -> v5:
>>> * Set has_hs_mode_support to false for unsupported SoCs.
>>> v2 -> v3:
>>> * Document tlow_hs_mode and thigh_hs_mode.
>>> v1 -> v2:
>>> * Document has_hs_mode_support.
>>> * Add a check to set the frequency to fastmode+ if the device
>>> does not support HS mode but the requested frequency is more
>>> than fastmode+.
>>> ---
>>> drivers/i2c/busses/i2c-tegra.c | 33 +++++++++++++++++++++++++++++++++
>>> 1 file changed, 33 insertions(+)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
>>> index d908e5e3f0af..6f816de8b3af 100644
>>> --- a/drivers/i2c/busses/i2c-tegra.c
>>> +++ b/drivers/i2c/busses/i2c-tegra.c
>>> @@ -91,6 +91,7 @@
>>> #define I2C_HEADER_IE_ENABLE BIT(17)
>>> #define I2C_HEADER_REPEAT_START BIT(16)
>>> #define I2C_HEADER_CONTINUE_XFER BIT(15)
>>> +#define I2C_HEADER_HS_MODE BIT(22)
>>> #define I2C_HEADER_SLAVE_ADDR_SHIFT 1
>>>
>>> #define I2C_BUS_CLEAR_CNFG 0x084
>>> @@ -198,6 +199,8 @@ enum msg_end_type {
>>> * @thigh_std_mode: High period of the clock in standard mode.
>>> * @tlow_fast_fastplus_mode: Low period of the clock in fast/fast-plus modes.
>>> * @thigh_fast_fastplus_mode: High period of the clock in fast/fast-plus modes.
>>> + * @tlow_hs_mode: Low period of the clock in HS mode.
>>> + * @thigh_hs_mode: High period of the clock in HS mode.
>>> * @setup_hold_time_std_mode: Setup and hold time for start and stop conditions
>>> * in standard mode.
>>> * @setup_hold_time_fast_fast_plus_mode: Setup and hold time for start and stop
>>> @@ -206,6 +209,7 @@ enum msg_end_type {
>>> * in HS mode.
>>> * @has_interface_timing_reg: Has interface timing register to program the tuned
>>> * timing settings.
>>> + * @has_hs_mode_support: Has support for high speed (HS) mode transfers.
>>> */
>>> struct tegra_i2c_hw_feature {
>>> bool has_continue_xfer_support;
>>> @@ -226,10 +230,13 @@ struct tegra_i2c_hw_feature {
>>> u32 thigh_std_mode;
>>> u32 tlow_fast_fastplus_mode;
>>> u32 thigh_fast_fastplus_mode;
>>> + u32 tlow_hs_mode;
>>> + u32 thigh_hs_mode;
>>> u32 setup_hold_time_std_mode;
>>> u32 setup_hold_time_fast_fast_plus_mode;
>>> u32 setup_hold_time_hs_mode;
>>> bool has_interface_timing_reg;
>>> + bool has_hs_mode_support;
>>> };
>>>
>>> /**
>>> @@ -717,6 +724,20 @@ 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);
>>> + } else if (t->bus_freq_hz > I2C_MAX_FAST_MODE_PLUS_FREQ) {
>>> + t->bus_freq_hz = I2C_MAX_FAST_MODE_PLUS_FREQ;
>>
>> No mention in the changelog about this part. Looks like this is a fallback.
>>
>> Should all of this be handled in the case statement for t->bus_freq_hz?
>>
>
> HS mode timing parameters are programmed in registers different from the other
> speed modes. These registers does not affect the timing in other speed modes.
> HS mode registers being used or not is determined by the packet header.
>
> We may also want to program the regular timing registers, because it will be
> used for the master code byte to transition to HS mode.
>
> So, I guess, even if we move this to the switch statement, we might end up
> doing something similar outside it.
The 'tlow', 'thigh' and 'tsu_thd' are configured under the case
statement and so seems logical to also configure these for HS mode under
this too. I see that there are different timing registers for HS mode,
but right now looks like we are programming both the normal ones and HS
ones. Do both need to be programmed for HS mode?
Jon
--
nvpublic
On Thu, 18 Sep 2025 11:21:14 +0100, Jon Hunter wrote: > On 18/09/2025 11:04, Akhil R wrote: >> On Wed, 17 Sep 2025 14:59:54 +0100, Jon Hunter wrote: >>> On 17/09/2025 09:56, Kartik Rajput wrote: ... ... >>> No mention in the changelog about this part. Looks like this is a fallback. >>> >>> Should all of this be handled in the case statement for t->bus_freq_hz? >>> >> >> HS mode timing parameters are programmed in registers different from the other >> speed modes. These registers does not affect the timing in other speed modes. >> HS mode registers being used or not is determined by the packet header. >> >> We may also want to program the regular timing registers, because it will be >> used for the master code byte to transition to HS mode. >> >> So, I guess, even if we move this to the switch statement, we might end up >> doing something similar outside it. > > > The 'tlow', 'thigh' and 'tsu_thd' are configured under the case > statement and so seems logical to also configure these for HS mode under > this too. I see that there are different timing registers for HS mode, We are just reusing the variables since the fields are similar. If required, we can define separate variables with _hs suffix. Do you suggest it that way? > but right now looks like we are programming both the normal ones and HS > ones. Do both need to be programmed for HS mode? Yes. As mentioned in my previous comment, the normal timing registers will be used for the 'master code' byte sent to transition to HS mode. We need to program both for HS mode. So, I am not sure if moving this section to the switch block will add any benefit. We might end up making it more complicated that it is now. Regards, Akhil
On 18/09/2025 12:16, Akhil R wrote:
> On Thu, 18 Sep 2025 11:21:14 +0100, Jon Hunter wrote:
>> On 18/09/2025 11:04, Akhil R wrote:
>>> On Wed, 17 Sep 2025 14:59:54 +0100, Jon Hunter wrote:
>>>> On 17/09/2025 09:56, Kartik Rajput wrote:
> ...
> ...
>
>>>> No mention in the changelog about this part. Looks like this is a fallback.
>>>>
>>>> Should all of this be handled in the case statement for t->bus_freq_hz?
>>>>
>>>
>>> HS mode timing parameters are programmed in registers different from the other
>>> speed modes. These registers does not affect the timing in other speed modes.
>>> HS mode registers being used or not is determined by the packet header.
>>>
>>> We may also want to program the regular timing registers, because it will be
>>> used for the master code byte to transition to HS mode.
>>>
>>> So, I guess, even if we move this to the switch statement, we might end up
>>> doing something similar outside it.
>>
>>
>> The 'tlow', 'thigh' and 'tsu_thd' are configured under the case
>> statement and so seems logical to also configure these for HS mode under
>> this too. I see that there are different timing registers for HS mode,
>
> We are just reusing the variables since the fields are similar. If required,
> we can define separate variables with _hs suffix. Do you suggest it that way?
>
>> but right now looks like we are programming both the normal ones and HS
>> ones. Do both need to be programmed for HS mode?
>
> Yes. As mentioned in my previous comment, the normal timing registers will
> be used for the 'master code' byte sent to transition to HS mode. We need
> to program both for HS mode.
OK, I see now. So we need to program the normal timings first and then
we are re-using the variables to then program the HS timings. And
because of that we cannot setup the HS timing values in the existing
case statement?
> So, I am not sure if moving this section to the switch block will add
> any benefit. We might end up making it more complicated that it is now.
Yes that's true. It was really this else part that caught my eye ...
} else if (t->bus_freq_hz > I2C_MAX_FAST_MODE_PLUS_FREQ) {
t->bus_freq_hz = I2C_MAX_FAST_MODE_PLUS_FREQ;
}
It feels like at least this part should be handled as part of the case
statement.
Jon
--
nvpublic
On Thu, 18 Sep 2025 13:55:48 +0100 Jon Hunter wrote:
> On 18/09/2025 12:16, Akhil R wrote:
>> On Thu, 18 Sep 2025 11:21:14 +0100, Jon Hunter wrote:
>>> On 18/09/2025 11:04, Akhil R wrote:
>>>> On Wed, 17 Sep 2025 14:59:54 +0100, Jon Hunter wrote:
>>>>> On 17/09/2025 09:56, Kartik Rajput wrote:
>> ...
>> ...
>>
>>>>> No mention in the changelog about this part. Looks like this is a fallback.
>>>>>
>>>>> Should all of this be handled in the case statement for t->bus_freq_hz?
>>>>>
>>>>
>>>> HS mode timing parameters are programmed in registers different from the other
>>>> speed modes. These registers does not affect the timing in other speed modes.
>>>> HS mode registers being used or not is determined by the packet header.
>>>>
>>>> We may also want to program the regular timing registers, because it will be
>>>> used for the master code byte to transition to HS mode.
>>>>
>>>> So, I guess, even if we move this to the switch statement, we might end up
>>>> doing something similar outside it.
>>>
>>>
>>> The 'tlow', 'thigh' and 'tsu_thd' are configured under the case
>>> statement and so seems logical to also configure these for HS mode under
>>> this too. I see that there are different timing registers for HS mode,
>>
>> We are just reusing the variables since the fields are similar. If required,
>> we can define separate variables with _hs suffix. Do you suggest it that way?
>>
>>> but right now looks like we are programming both the normal ones and HS
>>> ones. Do both need to be programmed for HS mode?
>>
>> Yes. As mentioned in my previous comment, the normal timing registers will
>> be used for the 'master code' byte sent to transition to HS mode. We need
>> to program both for HS mode.
>
> OK, I see now. So we need to program the normal timings first and then
> we are re-using the variables to then program the HS timings. And
> because of that we cannot setup the HS timing values in the existing
> case statement?
>
>> So, I am not sure if moving this section to the switch block will add
>> any benefit. We might end up making it more complicated that it is now.
>
> Yes that's true. It was really this else part that caught my eye ...
>
> } else if (t->bus_freq_hz > I2C_MAX_FAST_MODE_PLUS_FREQ) {
> t->bus_freq_hz = I2C_MAX_FAST_MODE_PLUS_FREQ;
> }
>
> It feels like at least this part should be handled as part of the case
> statement.
Yes. That makes sense. If you agree, we can remove the else part because
we weren't doing this before when HS mode support was not there. It is not
directly related to the HS mode support as well. We can add this at a later
point in a separate patch if found required.
Regards,
Akhil
On 18/09/2025 18:12, Akhil R wrote:
...
>> OK, I see now. So we need to program the normal timings first and then
>> we are re-using the variables to then program the HS timings. And
>> because of that we cannot setup the HS timing values in the existing
>> case statement?
>>
>>> So, I am not sure if moving this section to the switch block will add
>>> any benefit. We might end up making it more complicated that it is now.
>>
>> Yes that's true. It was really this else part that caught my eye ...
>>
>> } else if (t->bus_freq_hz > I2C_MAX_FAST_MODE_PLUS_FREQ) {
>> t->bus_freq_hz = I2C_MAX_FAST_MODE_PLUS_FREQ;
>> }
>>
>> It feels like at least this part should be handled as part of the case
>> statement.
>
> Yes. That makes sense. If you agree, we can remove the else part because
> we weren't doing this before when HS mode support was not there. It is not
> directly related to the HS mode support as well. We can add this at a later
> point in a separate patch if found required.
Hmmm ... I am not sure because then we could potentially program the
packet header incorrectly later on. May be that will never happen?
However, I think it would be better to not make any assumptions here and
make the code as robust as possible.
Jon
--
nvpublic
© 2016 - 2025 Red Hat, Inc.