Re: [PATCH v6 2/3] i3c: master: Add Qualcomm I3C controller driver
From: Mukesh Savaliya
Date: Mon Sep 21 2026 - 16:36:15 EST
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:Sure, let me clarify it and update the commit log.
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.
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.
[..]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.
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?
Also, why "keep as dummy"?Removed it as we are not using it.
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.+};[..]
+
+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?
Yes, commit message says rightly.
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)
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.Yes, right now these functions can be removed as it doesn't do anything.+}[..]
+
+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?
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