Re: [PATCH v6 2/3] i3c: master: Add Qualcomm I3C controller driver

[Mukesh Savaliya <mukesh.savaliya@oss.qualcomm.com>]

Hi Bjorn, Please find my responses inline below. I have migrated from my 
@quicinc.com email address to @oss.qualcomm.com and could not locate the 
original email thread.

The replies below are from a draft I had saved earlier. I am sending 
them now from my OSS account after resolving the slave setup and 
validation issues that had delayed the response. I shall upload V7 
cleanly with OSS ID.

On 3/5/2026 9:12 PM, Mukesh Kumar Savaliya wrote:
> Thanks Bjorn !
> I am sorry for the late reply, i was completely off from this work. I 
> have restarted this series work and will continue now onwards.
> 
> On 7/2/2025 2:26 AM, Bjorn Andersson wrote:
>> On Tue, Jul 01, 2025 at 12:48:51PM +0530, Mukesh Kumar Savaliya wrote:
>>> Add support for the Qualcomm I3C controller driver, which implements
>>> I3C master functionality as defined in the MIPI Alliance Specification
>>> for I3C, Version 1.0.
>>>
>>> This driver supports master role in SDR mode.
>>>
>>> Unlike some other I3C master controllers, this implementation
>>> does not support In-Band Interrupts (IBI) and Hot-join requests.
>>
>> I believe the capitalization of the last feature is "Hot-Join"
>>
>> It's not entirely clear from this sentence if it's the controller or the
>> driver that doesn't support these features. Please update to make it
>> clear.
>>
> Sure, let me clarify it and update the commit log.
> It's the driver which doesn't support Hot-Join for now, we plan to add 
> it in future.
> 
> I would rectify citing controller driver doesn't support these features.
>> [..]
>>> diff --git a/drivers/i3c/master/Makefile b/drivers/i3c/master/Makefile
>>> index 3e97960160bc..0e3ad9d96424 100644
>>> --- a/drivers/i3c/master/Makefile
>>> +++ b/drivers/i3c/master/Makefile
>>> @@ -1,5 +1,6 @@
>>>   # SPDX-License-Identifier: GPL-2.0-only
>>>   obj-$(CONFIG_CDNS_I3C_MASTER)        += i3c-master-cdns.o
>>> +obj-$(CONFIG_I3C_QCOM_GENI)        += i3c-qcom-geni.o
>>>   obj-$(CONFIG_DW_I3C_MASTER)        += dw-i3c-master.o
>>>   obj-$(CONFIG_AST2600_I3C_MASTER)    += ast2600-i3c-master.o
>>>   obj-$(CONFIG_SVC_I3C_MASTER)        += svc-i3c-master.o
>>> diff --git a/drivers/i3c/master/i3c-qcom-geni.c b/drivers/i3c/master/ 
>>> i3c-qcom-geni.c
>> [..]
>>> +
>>> +struct geni_i3c_i2c_dev_data {
>>> +    u32 ibi_keeping;  /* Plan to save IBI information, keep as dummy 
>>> for now */
>>
>> Commit message says that QUP doesn't support IBI, so what is this?
>>
> No, i meant to say I3C driver doesn't support IBI along with Hot-Join. I 
> have enhanced the commit log to make it clear.
> 
>> Also, why "keep as dummy"?
>>
> Removed it as we are not using it.
> 
>>> +};
>>> +
>> [..]
>>> +static void qcom_geni_i3c_conf(struct geni_i3c_dev *gi3c, enum 
>>> i3c_bus_phase bus_phase)
>>> +{
>>> +    const struct geni_i3c_clk_settings *clk_idx = gi3c->clk_cfg;
>>> +    unsigned long freq;
>>> +    u32 val, dfs_idx;
>>> +    int ret;
>>> +
>>> +    if (bus_phase == OPEN_DRAIN_MODE)
>>> +        clk_idx = gi3c->clk_od_cfg;
>>> +
>>> +    ret = geni_se_clk_freq_match(&gi3c->se, clk_idx->clk_src_freq,
>>> +                     &dfs_idx, &freq, false);
>>> +    if (ret)
>>> +        dfs_idx = 0;
>>> +
>>> +    writel_relaxed(dfs_idx, gi3c->se.base + SE_GENI_CLK_SEL);
>>> +
>>> +    val = FIELD_PREP(CLK_DIV_VALUE_MASK, clk_idx->clk_div);
>>> +    val |= SER_CLK_EN;
>>> +    writel_relaxed(val, gi3c->se.base + GENI_SER_M_CLK_CFG);
>>> +
>>> +    val = FIELD_PREP(I2C_SCL_HIGH_COUNTER_MASK, clk_idx- 
>>> >i2c_t_high_cnt);
>>> +    val |= FIELD_PREP(I2C_SCL_LOW_COUNTER_MASK, clk_idx- 
>>> >i2c_t_low_cnt);
>>> +    val |= FIELD_PREP(I2C_SCL_CYCLE_COUNTER_MASK, clk_idx- 
>>> >i2c_t_cycle_cnt);
>>> +    writel_relaxed(val, gi3c->se.base + SE_I2C_SCL_COUNTERS);
>>> +
>>> +    writel_relaxed(clk_idx->i3c_t_cycle_cnt, gi3c->se.base + 
>>> SE_I3C_SCL_CYCLE);
>>> +    writel_relaxed(clk_idx->i3c_t_high_cnt, gi3c->se.base + 
>>> SE_I3C_SCL_HIGH);
>>> +
>>> +    writel_relaxed(M_IBI_IRQ_IGNORE, gi3c->se.base + 
>>> SE_GENI_HW_IRQ_IGNORE_ON_ACTIVE);
>>> +
>>> +    val = M_IBI_IRQ_PARAM_STOP_STALL | M_IBI_IRQ_PARAM_7E;
>>> +    writel_relaxed(val, gi3c->se.base + SE_GENI_HW_IRQ_CMD_PARAM_0);
>>> +
>>> +    writel_relaxed(M_IBI_IRQ_EN, gi3c->se.base + SE_GENI_HW_IRQ_EN);
>>
>> Don't you want a non-relaxed write here, to clarify that the ordering of
>> this write and the previous are significant?
>>
> Do you mean for IBI_IRQ_EN or the whole function ? If yes, then i can 
> change to ensure no re-ordering. But i don't see reordering for complete 
> function is required.
>>
>> As above, the commit message says the controller doesn't do IBI, so why
>> are we enabling IBI interrupts? (Just guessing based on the IRQ names)
>>
> Yes, commit message says rightly.
> We are enabling just to ensure I3C controller works with IBI controller 
> enabled. And that's mandatory per HW programming sequence as we don't 
> disable IBI controller even we don't use IBI features like IBI interrupt.
>>> +}
>>> +
>> [..]
>>> +static int geni_i3c_master_attach_i2c_dev(struct i2c_dev_desc *dev)
>>> +{
>>> +    struct geni_i3c_i2c_dev_data *data;
>>> +
>>> +    data = kzalloc(sizeof(*data), GFP_KERNEL);
>>> +    if (!data)
>>> +        return -ENOMEM;
>>> +
>>> +    i2c_dev_set_master_data(dev, data);
>>
>> As far as I can tell, the master_data is private to the controller
>> driver, and the only thing I can find you do with it to free it again on
>> detach.
>>
>> Am I missing something or can these 4 optional functions be removed?
>>
> Yes, right now these functions can be removed as it doesn't do anything.
> I had kept it there to add something really required in future.
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static void geni_i3c_master_detach_i2c_dev(struct i2c_dev_desc *dev)
>>> +{
>>> +    struct geni_i3c_i2c_dev_data *data = i2c_dev_get_master_data(dev);
>>> +
>>> +    i2c_dev_set_master_data(dev, NULL);
>>> +    kfree(data);
>>> +}
>>> +
>>> +static int geni_i3c_master_attach_i3c_dev(struct i3c_dev_desc *dev)
>>> +{
>>> +    struct geni_i3c_i2c_dev_data *data;
>>> +
>>> +    data = kzalloc(sizeof(*data), GFP_KERNEL);
>>> +    if (!data)
>>> +        return -ENOMEM;
>>> +
>>> +    i3c_dev_set_master_data(dev, data);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static void geni_i3c_master_detach_i3c_dev(struct i3c_dev_desc *dev)
>>> +{
>>> +    struct geni_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
>>> +
>>> +    i3c_dev_set_master_data(dev, NULL);
>>> +    kfree(data);
>>> +}
>>> +
>>
>> Regards,
>> Bjorn
>>
>