[PATCH v2] i3c: master: dw-i3c-master: fix OD timing for first broadcast

[PATCH v2] i3c: master: dw-i3c-master: fix OD timing for first broadcast

[tze.yee.ng]

From: Tze Yee Ng <tze.yee.ng@altera.com>

Implement ->set_speed() so the I3C core can switch open-drain timing for
the first broadcast address per spec: I3C_OPEN_DRAIN_SLOW_SPEED programs
tHIGH_INIT (200 ns) before RSTDAA, and I3C_OPEN_DRAIN_NORMAL_SPEED restores
normal OD timing afterward. Cache the normal OD register value during bus
init and use a separate od_hcnt for the slow path so SDR extended timing
remains derived from the normal PP hcnt.

For AMD_I3C_OD_PP_TIMING, cache AMD_I3C_OD_TIMING as the normal OD
baseline and stop rewriting OD timing in send_ccc_cmd()/runtime resume so
I3C_OPEN_DRAIN_SLOW_SPEED is preserved through RSTDAA.

Use PM_RUNTIME_ACQUIRE_AUTOSUSPEND() in set_speed().

Fixes I2C devices with spike filters not being detected on mixed buses.

Signed-off-by: Tze Yee Ng <tze.yee.ng@altera.com>
---
Changes in v2:
- Recalculate od_hcnt with Zephyr-style formula (Sashiko)
- Fix AMD_I3C_OD_PP_TIMING interaction with set_speed() (Sashiko)
- Switch set_speed() to PM_RUNTIME_ACQUIRE_AUTOSUSPEND() (Frank)
---
 drivers/i3c/master/dw-i3c-master.c | 80 +++++++++++++++++++++++-------
 drivers/i3c/master/dw-i3c-master.h |  1 +
 include/linux/i3c/master.h         |  1 +
 3 files changed, 65 insertions(+), 17 deletions(-)

diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
index 2f8c0c4683e0..2a383e2163d4 100644
--- a/drivers/i3c/master/dw-i3c-master.c
+++ b/drivers/i3c/master/dw-i3c-master.c
@@ -554,6 +554,13 @@ static void dw_i3c_master_set_intr_regs(struct dw_i3c_master *master)
 	writel(IBI_REQ_REJECT_ALL, master->regs + IBI_MR_REQ_REJECT);
 }
 
+static void amd_configure_od_pp_quirk(struct dw_i3c_master *master)
+{
+	master->i3c_od_timing = AMD_I3C_OD_TIMING;
+	master->i3c_od_timing_normal = AMD_I3C_OD_TIMING;
+	master->i3c_pp_timing = AMD_I3C_PP_TIMING;
+}
+
 static int dw_i3c_clk_cfg(struct dw_i3c_master *master)
 {
 	unsigned long core_rate, core_period;
@@ -592,6 +599,18 @@ static int dw_i3c_clk_cfg(struct dw_i3c_master *master)
 	scl_timing = SCL_I3C_TIMING_HCNT(hcnt) | SCL_I3C_TIMING_LCNT(lcnt);
 	writel(scl_timing, master->regs + SCL_I3C_OD_TIMING);
 	master->i3c_od_timing = scl_timing;
+	master->i3c_od_timing_normal = scl_timing;
+
+	/*
+	 * AMD legacy platforms need fixed OD/PP timings. Cache them as the
+	 * normal OD baseline so set_speed(NORMAL) restores AMD values, and
+	 * set_speed(SLOW) can stretch HCNT while keeping the AMD LCNT.
+	 */
+	if (master->quirks & AMD_I3C_OD_PP_TIMING) {
+		amd_configure_od_pp_quirk(master);
+		writel(master->i3c_pp_timing, master->regs + SCL_I3C_PP_TIMING);
+		writel(master->i3c_od_timing, master->regs + SCL_I3C_OD_TIMING);
+	}
 
 	lcnt = DIV_ROUND_UP(core_rate, I3C_BUS_SDR1_SCL_RATE) - hcnt;
 	scl_timing = SCL_EXT_LCNT_1(lcnt);
@@ -786,12 +805,6 @@ static int dw_i3c_ccc_get(struct dw_i3c_master *master, struct i3c_ccc_cmd *ccc)
 	return ret;
 }
 
-static void amd_configure_od_pp_quirk(struct dw_i3c_master *master)
-{
-	master->i3c_od_timing = AMD_I3C_OD_TIMING;
-	master->i3c_pp_timing = AMD_I3C_PP_TIMING;
-}
-
 static int dw_i3c_master_send_ccc_cmd(struct i3c_master_controller *m,
 				      struct i3c_ccc_cmd *ccc)
 {
@@ -801,13 +814,6 @@ static int dw_i3c_master_send_ccc_cmd(struct i3c_master_controller *m,
 	if (ccc->id == I3C_CCC_ENTDAA)
 		return -EINVAL;
 
-	/* AMD platform specific OD and PP timings */
-	if (master->quirks & AMD_I3C_OD_PP_TIMING) {
-		amd_configure_od_pp_quirk(master);
-		writel(master->i3c_pp_timing, master->regs + SCL_I3C_PP_TIMING);
-		writel(master->i3c_od_timing, master->regs + SCL_I3C_OD_TIMING);
-	}
-
 	ret = pm_runtime_resume_and_get(master->dev);
 	if (ret < 0) {
 		dev_err(master->dev,
@@ -1484,6 +1490,48 @@ static irqreturn_t dw_i3c_master_irq_handler(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static int dw_i3c_master_set_speed(struct i3c_master_controller *m,
+				   enum i3c_open_drain_speed speed)
+{
+	struct dw_i3c_master *master = to_dw_i3c_master(m);
+	unsigned long core_rate;
+	u32 scl_timing;
+	u8 od_hcnt, lcnt;
+
+	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(master->dev, pm);
+	if (PM_RUNTIME_ACQUIRE_ERR(&pm))
+		return -ENXIO;
+
+	switch (speed) {
+	case I3C_OPEN_DRAIN_SLOW_SPEED:
+		core_rate = clk_get_rate(master->core_clk);
+		if (!core_rate)
+			return -EINVAL;
+
+		lcnt = SCL_I3C_TIMING_LCNT(master->i3c_od_timing_normal);
+		od_hcnt = DIV_ROUND_UP((u64)I3C_BUS_THIGH_INIT_OD_MIN_NS * core_rate,
+				       1000000000ULL) - 1;
+		if (od_hcnt < SCL_I3C_TIMING_CNT_MIN)
+			od_hcnt = SCL_I3C_TIMING_CNT_MIN;
+		scl_timing = SCL_I3C_TIMING_HCNT(od_hcnt) |
+			     SCL_I3C_TIMING_LCNT(lcnt);
+		writel(scl_timing, master->regs + SCL_I3C_OD_TIMING);
+		master->i3c_od_timing = scl_timing;
+		break;
+
+	case I3C_OPEN_DRAIN_NORMAL_SPEED:
+		writel(master->i3c_od_timing_normal,
+		       master->regs + SCL_I3C_OD_TIMING);
+		master->i3c_od_timing = master->i3c_od_timing_normal;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int dw_i3c_master_set_dev_nack_retry(struct i3c_master_controller *m,
 					    unsigned int dev_nack_retry_cnt)
 {
@@ -1538,6 +1586,7 @@ static const struct i3c_master_controller_ops dw_mipi_i3c_ops = {
 	.recycle_ibi_slot = dw_i3c_master_recycle_ibi_slot,
 	.enable_hotjoin = dw_i3c_master_enable_hotjoin,
 	.disable_hotjoin = dw_i3c_master_disable_hotjoin,
+	.set_speed = dw_i3c_master_set_speed,
 	.set_dev_nack_retry = dw_i3c_master_set_dev_nack_retry,
 };
 
@@ -1728,10 +1777,7 @@ static void dw_i3c_master_restore_addrs(struct dw_i3c_master *master)
 
 static void dw_i3c_master_restore_timing_regs(struct dw_i3c_master *master)
 {
-	/* AMD platform specific OD and PP timings */
-	if (master->quirks & AMD_I3C_OD_PP_TIMING)
-		amd_configure_od_pp_quirk(master);
-
+	/* Preserve cached OD timing; it may be the SLOW setting from set_speed(). */
 	writel(master->i3c_pp_timing, master->regs + SCL_I3C_PP_TIMING);
 	writel(master->bus_free_timing, master->regs + BUS_FREE_TIMING);
 	writel(master->i3c_od_timing, master->regs + SCL_I3C_OD_TIMING);
diff --git a/drivers/i3c/master/dw-i3c-master.h b/drivers/i3c/master/dw-i3c-master.h
index 28e9348f2153..17ad817d1f8e 100644
--- a/drivers/i3c/master/dw-i3c-master.h
+++ b/drivers/i3c/master/dw-i3c-master.h
@@ -46,6 +46,7 @@ struct dw_i3c_master {
 	u32 dev_addr;
 	u32 i3c_pp_timing;
 	u32 i3c_od_timing;
+	u32 i3c_od_timing_normal;
 	u32 ext_lcnt_timing;
 	u32 bus_free_timing;
 	u32 i2c_fm_timing;
diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
index 4d2a68793324..eca79ea1598a 100644
--- a/include/linux/i3c/master.h
+++ b/include/linux/i3c/master.h
@@ -259,6 +259,7 @@ struct i3c_device {
 #define I3C_BUS_THIGH_MIXED_MAX_NS	41
 #define I3C_BUS_TIDLE_MIN_NS		200000
 #define I3C_BUS_TLOW_OD_MIN_NS		200
+#define I3C_BUS_THIGH_INIT_OD_MIN_NS	200
 
 /**
  * enum i3c_bus_mode - I3C bus mode
-- 
2.43.7


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH v2] i3c: master: dw-i3c-master: fix OD timing for first broadcast

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The calculation for `od_hcnt` can silently overflow its 8-bit capacity if the core clock rate exceeds ~1.28 GHz, resulting in an inappropriately fast open-drain speed.
- [Low] Using DIV_ROUND_UP() with a 64-bit numerator or divisor causes a linker error (__udivdi3 undefined) on 32-bit platforms.
--

--- Patch [1]: [PATCH v2] i3c: master: dw-i3c-master: fix OD timing for first broadcast ---
commit b82f29a59ae98bd18cbb544ded9a3bf8f8d60fc6
Author: Tze Yee Ng <tze.yee.ng@altera.com>

i3c: master: dw-i3c-master: fix OD timing for first broadcast

This commit implements ->set_speed() to switch open-drain timing for the
first broadcast address according to the I3C specification. It caches the
normal OD register value during init and uses a separate od_hcnt for the
slow path so SDR extended timing remains derived from the normal PP hcnt.

> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index 84ffcb1893389..077f2bf4b0b3f 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c

[ ... ]

> @@ -1513,6 +1519,48 @@ static irqreturn_t dw_i3c_master_irq_handler(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> +static int dw_i3c_master_set_speed(struct i3c_master_controller *m,
> +				   enum i3c_open_drain_speed speed)
> +{
> +	struct dw_i3c_master *master = to_dw_i3c_master(m);
> +	unsigned long core_rate;
> +	u32 scl_timing;
> +	u8 od_hcnt, lcnt;
> +
> +	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(master->dev, pm);
> +	if (PM_RUNTIME_ACQUIRE_ERR(&pm))
> +		return -ENXIO;
> +
> +	switch (speed) {
> +	case I3C_OPEN_DRAIN_SLOW_SPEED:
> +		core_rate = clk_get_rate(master->core_clk);
> +		if (!core_rate)
> +			return -EINVAL;
> +
> +		lcnt = SCL_I3C_TIMING_LCNT(master->i3c_od_timing_normal);
> +		od_hcnt = DIV_ROUND_UP((u64)I3C_BUS_THIGH_INIT_OD_MIN_NS * core_rate,
> +				       1000000000ULL) - 1;

[Severity: Low]
Will this cause a build failure on 32-bit platforms?

Using DIV_ROUND_UP() with a 64-bit divisor (1000000000ULL) will likely
result in a linker error for __udivdi3 on 32-bit architectures like ARM32
or i386. Should this use DIV_ROUND_UP_ULL() instead?

[Severity: Medium]
Can od_hcnt silently overflow here?

Because od_hcnt is declared as a u8 at the top of the function, if the
core clock rate exceeds ~1.28 GHz, the calculation will evaluate to a
value greater than 255.

This would truncate when assigned to od_hcnt, bypassing the minimum 200ns
requirement and potentially causing spike filters on devices to drop the
broadcast.

> +		if (od_hcnt < SCL_I3C_TIMING_CNT_MIN)
> +			od_hcnt = SCL_I3C_TIMING_CNT_MIN;
> +		scl_timing = SCL_I3C_TIMING_HCNT(od_hcnt) |
> +			     SCL_I3C_TIMING_LCNT(lcnt);
> +		writel(scl_timing, master->regs + SCL_I3C_OD_TIMING);
> +		master->i3c_od_timing = scl_timing;
> +		break;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721033159.11789-1-tze.yee.ng@altera.com?part=1

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH v2] i3c: master: dw-i3c-master: fix OD timing for first broadcast

[Frank Li]

On Tue, Jul 21, 2026 at 03:47:50AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] The calculation for `od_hcnt` can silently overflow its 8-bit capacity if the core clock rate exceeds ~1.28 GHz, resulting in an inappropriately fast open-drain speed.
> - [Low] Using DIV_ROUND_UP() with a 64-bit numerator or divisor causes a linker error (__udivdi3 undefined) on 32-bit platforms.
> --
>
> --- Patch [1]: [PATCH v2] i3c: master: dw-i3c-master: fix OD timing for first broadcast ---
> commit b82f29a59ae98bd18cbb544ded9a3bf8f8d60fc6
> Author: Tze Yee Ng <tze.yee.ng@altera.com>
>
> i3c: master: dw-i3c-master: fix OD timing for first broadcast
>
> This commit implements ->set_speed() to switch open-drain timing for the
> first broadcast address according to the I3C specification. It caches the
> normal OD register value during init and uses a separate od_hcnt for the
> slow path so SDR extended timing remains derived from the normal PP hcnt.
>
> > diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> > index 84ffcb1893389..077f2bf4b0b3f 100644
> > --- a/drivers/i3c/master/dw-i3c-master.c
> > +++ b/drivers/i3c/master/dw-i3c-master.c
>
> [ ... ]
>
> > @@ -1513,6 +1519,48 @@ static irqreturn_t dw_i3c_master_irq_handler(int irq, void *dev_id)
> >  	return IRQ_HANDLED;
> >  }
> >
> > +static int dw_i3c_master_set_speed(struct i3c_master_controller *m,
> > +				   enum i3c_open_drain_speed speed)
> > +{
> > +	struct dw_i3c_master *master = to_dw_i3c_master(m);
> > +	unsigned long core_rate;
> > +	u32 scl_timing;
> > +	u8 od_hcnt, lcnt;
> > +
> > +	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(master->dev, pm);
> > +	if (PM_RUNTIME_ACQUIRE_ERR(&pm))
> > +		return -ENXIO;
> > +
> > +	switch (speed) {
> > +	case I3C_OPEN_DRAIN_SLOW_SPEED:
> > +		core_rate = clk_get_rate(master->core_clk);
> > +		if (!core_rate)
> > +			return -EINVAL;
> > +
> > +		lcnt = SCL_I3C_TIMING_LCNT(master->i3c_od_timing_normal);
> > +		od_hcnt = DIV_ROUND_UP((u64)I3C_BUS_THIGH_INIT_OD_MIN_NS * core_rate,
> > +				       1000000000ULL) - 1;
>
> [Severity: Low]
> Will this cause a build failure on 32-bit platforms?
>
> Using DIV_ROUND_UP() with a 64-bit divisor (1000000000ULL) will likely
> result in a linker error for __udivdi3 on 32-bit architectures like ARM32
> or i386. Should this use DIV_ROUND_UP_ULL() instead?
>

make sense,

> [Severity: Medium]
> Can od_hcnt silently overflow here?
>
> Because od_hcnt is declared as a u8 at the top of the function, if the
> core clock rate exceeds ~1.28 GHz, the calculation will evaluate to a
> value greater than 255.

less possible, but suggest fix it.

Frank

>
> This would truncate when assigned to od_hcnt, bypassing the minimum 200ns
> requirement and potentially causing spike filters on devices to drop the
> broadcast.
>
> > +		if (od_hcnt < SCL_I3C_TIMING_CNT_MIN)
> > +			od_hcnt = SCL_I3C_TIMING_CNT_MIN;
> > +		scl_timing = SCL_I3C_TIMING_HCNT(od_hcnt) |
> > +			     SCL_I3C_TIMING_LCNT(lcnt);
> > +		writel(scl_timing, master->regs + SCL_I3C_OD_TIMING);
> > +		master->i3c_od_timing = scl_timing;
> > +		break;
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260721033159.11789-1-tze.yee.ng@altera.com?part=1
>
> --
> linux-i3c mailing list
> linux-i3c@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-i3c

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c