Re: [PATCH 2/2] i2c: qcom-geni: Add support for I2C High-Speed mode

[Jyothi Kumar Seerapu <jyothi.seerapu@oss.qualcomm.com>]

On 8/28/2026 1:12 AM, Mukesh Savaliya wrote:
> 
> 
Hi Mukesh, Thanks for the review comments.> On 8/24/2026 4:55 PM, Jyothi 
Kumar Seerapu wrote:
> [...]>   struct geni_i2c_err_log {
>> @@ -162,9 +180,14 @@ static const struct geni_i2c_err_log gi2c_log[] = {
>>   struct geni_i2c_clk_fld {
>>       u32    clk_freq_out;
>>       u8    clk_div;
>> -    u8    t_high_cnt;
>> -    u8    t_low_cnt;
>> -    u8    t_cycle_cnt;
>> +    /*
>> +     * In normal mode, these counter values fit within 8 bits.
>> +     * In High-Speed mode, HS_TLOW_COUNT and HS_TCYCLE_COUNT are
>> +     * 10-bit fields, so u16 is required.
>> +     */
> Now, normal mode doesn't matter once switched to 16 bit.
> May be comment also doesn't matter once change is merged, you can add 
> some information in commit log though. Will be taken as default size.> 
Sure, will remove these comments in V2.+
> u16    t_high_cnt;
>> +    u16    t_low_cnt;
>> +    u16    t_cycle_cnt;
>>   };
>>   /*
>> @@ -194,10 +217,30 @@ static const struct geni_i2c_clk_fld 
>> geni_i2c_clk_map_32mhz[] = {
>>       {}
>>   };
>> +/* source_clock = 100 MHz */
>> +static const struct geni_i2c_clk_fld geni_i2c_clk_map_100mhz[] = {
>> +    { I2C_MAX_STANDARD_MODE_FREQ, 1, 449, 548, 998 },
>> +    { I2C_MAX_FAST_MODE_FREQ, 1, 76, 167, 248 },
>> +    { I2C_MAX_FAST_MODE_PLUS_FREQ, 1, 23, 59, 98 },
>> +    {}
>> +};
>> +
>>   static int geni_i2c_clk_map_idx(struct geni_i2c_dev *gi2c)
>>   {
>>       const struct geni_i2c_clk_fld *itr;
>> +    /* Check if HS mode is requested */
>> +    if (gi2c->clk_freq_out == I2C_HS_MODE_FREQ) {
>> +        gi2c->is_hs_mode = true;
>> +        /* For HS mode, source clock should be 100 MHz */
>> +        itr = geni_i2c_clk_map_100mhz;
>> +        /* For HS mode, start with 1MHz for master code */
>> +        gi2c->clk_fld = &itr[2];
>> +        return 0;
>> +    }
>> +
> else part ?
No, else is not required here, as the if condition for I2C HS mode case 
returns from the function. For non-HS mode case the below  will execute.

+    gi2c->is_hs_mode = false;
>> +
>>       if (clk_get_rate(gi2c->se.clk) == 32 * HZ_PER_MHZ)
>>           itr = geni_i2c_clk_map_32mhz;
>>       else
>> @@ -219,7 +262,12 @@ static int qcom_geni_i2c_conf(struct geni_se *se, 
>> unsigned long freq)
>>       const struct geni_i2c_clk_fld *itr = gi2c->clk_fld;
>>       u32 val;
>> -    writel_relaxed(0, gi2c->se.base + SE_GENI_CLK_SEL);
>> +    if (gi2c->is_hs_mode) {
>> +        writel_relaxed(I2C_HS_TCYCLE_CNT, gi2c->se.base + 
>> SE_I2C_HS_TCYCLE_REG);
>> +        writel_relaxed(I2C_HS_TLOW_CNT, gi2c->se.base + 
>> SE_I2C_HS_TLOW_REG);
>> +    }
>> +
>> +    writel_relaxed(gi2c->dfs_index, gi2c->se.base + SE_GENI_CLK_SEL);
>>       val = (itr->clk_div << CLK_DIV_SHFT) | SER_CLK_EN;
>>       writel_relaxed(val, gi2c->se.base + GENI_SER_M_CLK_CFG);
>> @@ -500,7 +548,11 @@ static int geni_i2c_rx_one_msg(struct 
>> geni_i2c_dev *gi2c, struct i2c_msg *msg,
>>           geni_se_select_mode(se, GENI_SE_FIFO);
>>       writel_relaxed(len, se->base + SE_I2C_RX_TRANS_LEN);
>> -    geni_se_setup_m_cmd(se, I2C_READ, m_param);
>> +
>> +    if (gi2c->is_hs_mode) 
> gi2c->is_hs_mode ? i2c->op = I2C_HS_READ : I2C_READ;
> geni_se_setup_m_cmd(se, i2c->op, m_param);> +        
> geni_se_setup_m_cmd(se, I2C_HS_READ, m_param);
>> +    else
>> +        geni_se_setup_m_cmd(se, I2C_READ, m_param);
>>       if (dma_buf && geni_se_rx_dma_prep(se, dma_buf, len, &rx_dma)) {
>>           geni_se_select_mode(se, GENI_SE_FIFO);
>> @@ -539,7 +591,11 @@ static int geni_i2c_tx_one_msg(struct 
>> geni_i2c_dev *gi2c, struct i2c_msg *msg,
>>           geni_se_select_mode(se, GENI_SE_FIFO);
>>       writel_relaxed(len, se->base + SE_I2C_TX_TRANS_LEN);
>> -    geni_se_setup_m_cmd(se, I2C_WRITE, m_param);
>> +
>> +    if (gi2c->is_hs_mode)
>> +        geni_se_setup_m_cmd(se, I2C_HS_WRITE, m_param);
>> +    else
>> +        geni_se_setup_m_cmd(se, I2C_WRITE, m_param);
> same as above>
Yes this can be set like below:
geni_se_setup_m_cmd(se, gi2c->is_hs_mode ? I2C_HS_WRITE : I2C_WRITE, 
m_param);

>>       if (dma_buf && geni_se_tx_dma_prep(se, dma_buf, len, &tx_dma)) {
>>           geni_se_select_mode(se, GENI_SE_FIFO);
>> @@ -700,7 +756,7 @@ static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, 
>> struct i2c_msg msgs[],
>>           goto out;
>>       }
> 
> [...]
> 
>> @@ -1046,6 +1122,39 @@ static int geni_i2c_init(struct geni_i2c_dev 
>> *gi2c)
>>           return ret;
>>       }
>> +    /*
>> +     * For I2C High-Speed mode, first verify QUP HW version supports it
>> +     * (requires QUPv3 core >= 4.3 per HPG), then configure 100 MHz 
>> source clock.
>> +     */
> /* HS mode requires QUPv3 version >= 4.3 and source clock=100 MHz */
Sure, will update in V2.>> +    if (gi2c->is_hs_mode) {
>> +        u32 hw_ver = geni_se_get_qup_hw_version(&gi2c->se);
>> +        u32 major = GENI_SE_VERSION_MAJOR(hw_ver);
>> +        u32 minor = GENI_SE_VERSION_MINOR(hw_ver);
>> +
>> +        if (major < QUP_I2C_HS_MIN_MAJOR ||
>> +            (major == QUP_I2C_HS_MIN_MAJOR && minor < 
>> QUP_I2C_HS_MIN_MINOR)) {
>> +            dev_err(gi2c->se.dev,
>> +                "QUP HW v%u.%u does not support I2C HS mode (requires 
>> >= %u.%u)\n",
>> +                major, minor,
>> +                QUP_I2C_HS_MIN_MAJOR, QUP_I2C_HS_MIN_MINOR);
> then why it entered into is_hs_mode condition ?
Need to check QUP version only for I2C HS mode requests, for other I2C 
modes no need of QUP version check. > +            ret = -
> EOPNOTSUPP;
>> +            goto err;
>> +        }
>> +
>> +        ret = geni_se_clk_freq_match(&gi2c->se, I2C_HS_SRC_CLK_FREQ,
>> +                         &gi2c->dfs_index, &freq_out, false);
>> +        if (ret) {
>> +            dev_err(gi2c->se.dev, "Failed to get DFS index for HS 
>> mode: %d\n", ret);
>> +            goto err;
>> +        }
>> +
>> +        ret = clk_set_rate(gi2c->se.clk, freq_out);
>> +        if (ret) {
>> +            dev_err(gi2c->se.dev, "Failed to set HS mode clock rate: 
>> %d\n", ret);
>> +            goto err;
>> +        }
>> +    }
>> +
>>       proto = geni_se_read_proto(&gi2c->se);
>>       if (proto == GENI_SE_INVALID_PROTO) {
>>           ret = geni_load_se_firmware(&gi2c->se, GENI_SE_I2C);
>>
>