[PATCH] i3c: master: dw-i3c-master: fix OD timing for first broadcast
NG, TZE YEE
tze.yee.ng at altera.com
Wed Jul 8 23:42:24 PDT 2026
On 1/7/2026 5:06 pm, Guntupalli, Manikanta wrote:
> Public
>
> Hi,
>
> Adding the below contact:
> + at Datta, Shubhrajyoti
>
> Thanks,
> Manikanta
>
Hi Manikanta, Datta,
Gentle follow-up on the AMD AMDI0015 quirk question below. It has been
about a week since Datta was brought into the thread, and I have not
received guidance yet on how ->set_speed() and send_ccc_cmd() should
interact with AMD_I3C_OD_PP_TIMING.
If AMD is not able to respond shortly, would it be acceptable to proceed
this patch without AMD-specific changes for now? The patch targets
generic DesignWare I3C, and I would rather avoid changing AMD behavior
without confirmation. AMD support can be handled in a follow-up patch
once your team confirms the expected flow.
Thanks,
Tze Yee
>> -----Original Message-----
>> From: NG, TZE YEE <tze.yee.ng at altera.com>
>> Sent: Wednesday, July 1, 2026 11:16 AM
>> To: Guntupalli, Manikanta <manikanta.guntupalli at amd.com>
>> Cc: linux-i3c at lists.infradead.org; Frank.Li at kernel.org; Frank Li
>> <Frank.li at oss.nxp.com>
>> Subject: Re: RE: [PATCH] i3c: master: dw-i3c-master: fix OD timing for first
>> broadcast
>>
>> On 24/6/2026 2:44 pm, Guntupalli, Manikanta wrote:
>>> Public
>>>
>>> Hi Tze Yee Ng,
>>>
>>> We will check this on our end and get back to you shortly with an update.
>>>
>>> Thanks,
>>> Manikanta
>>>
>>>
>>
>> Hi Manikanta,
>>
>> Gentle follow-up on the AMD quirk question below. It has been about a week since
>> your 24 June reply, and I have not seen an update yet.
>>
>> For AMDI0015 with AMD_I3C_OD_PP_TIMING, I still need your guidance on:
>> - what OD timing ->set_speed(NORMAL) should restore,
>> - how ->set_speed(SLOW) should be programmed, and
>> - whether send_ccc_cmd() should keep forcing AMD_I3C_OD_TIMING before every
>> CCC.
>>
>> Once I have your input, I can send an AMD-specific follow-up patch.
>>
>> Thanks,
>> Tze Yee Ng
>>
>>>> -----Original Message-----
>>>> From: NG, TZE YEE <tze.yee.ng at altera.com>
>>>> Sent: Friday, June 19, 2026 1:49 PM
>>>> To: Guntupalli, Manikanta <manikanta.guntupalli at amd.com>
>>>> Cc: linux-i3c at lists.infradead.org; Frank.Li at kernel.org; Frank Li
>>>> <Frank.li at oss.nxp.com>
>>>> Subject: Re: [PATCH] i3c: master: dw-i3c-master: fix OD timing for
>>>> first broadcast
>>>>
>>>> On 12/6/2026 3:58 pm, Ng, Tze Yee wrote:
>>>>> On 12/6/2026 12:21 am, Frank Li wrote:
>>>>>> [You don't often get email from frank.li at oss.nxp.com. Learn why
>>>>>> this is important at https://aka.ms/LearnAboutSenderIdentification
>>>>>> ]
>>>>>>
>>>>>> On Wed, Jun 10, 2026 at 08:36:55AM +0000, sashiko-bot at kernel.org wrote:
>>>>>>> Thank you for your contribution! Sashiko AI review found 5
>>>>>>> potential
>>>>>>> issue(s) to consider:
>>>>>>>
>>>>>>> New issues:
>>>>>>> - [High] The newly introduced `dw_i3c_master_set_speed()`
>>>>>>> permanently overwrites hardware-specific AMD timings with generic timings.
>>>>>>> - [High] The `od_hcnt` calculation for `I3C_OPEN_DRAIN_SLOW_SPEED`
>>>>>>> violates the `tHIGH_INIT` minimum requirement.
>>>>>>>
>>>>>>> Pre-existing issues:
>>>>>>> - [High] The IRQ handler accesses hardware registers without
>>>>>>> Runtime PM protection.
>>>>>>> - [High] Use-After-Free and invalid `queue_work()` due to improper
>>>>>>> teardown ordering in `dw_i3c_common_remove()`.
>>>>>>> - [High] `dw_i3c_master_set_dev_nack_retry` modifies hardware
>>>>>>> registers without resuming the device via Runtime PM.
>>>>>>> --
>>>>>>>
>>>>>>> commit bd5322e3d6d6cd3a6a8155f24cc89a12fe61be00
>>>>>>> Author: Tze Yee Ng <tze.yee.ng at altera.com>
>>>>>>>
>>>>>>> i3c: master: dw-i3c-master: fix OD timing for first broadcast
>>>>>>>
>>>>>>> This commit implements the set_speed() callback to switch
>>>>>>> open-drain timing for the first broadcast address to slow speed.
>>>>>>> It aims to fix I2C devices with spike filters not being detected
>>>>>>> on mixed buses by dynamically switching between slow and normal open
>> drain speeds.
>>>>>>>
>>>>>>>> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/
>>>>>>>> master/dw-i3c-master.c index a7593d6efac5a..6cf7071d3d06b 100644
>>>>>>>> --- a/drivers/i3c/master/dw-i3c-master.c
>>>>>>>> +++ b/drivers/i3c/master/dw-i3c-master.c
>>>>>>> [ ... ]
>>>>>>>> @@ -1484,6 +1485,57 @@ static irqreturn_t
>>>>>>>> dw_i3c_master_irq_handler(int irq, void *dev_id)
>>>>>>>> return IRQ_HANDLED;
>>>>>>>> }
>>>>>>>
>>>>>>> [Severity: High]
>>>>>>> This is a pre-existing issue, but does the IRQ handler access
>>>>>>> hardware registers without Runtime PM protection?
>>>>>>>
>>>>>>> The interrupt handler directly reads hardware registers before
>>>>>>> checking the device's PM active state:
>>>>>>>
>>>>>>> drivers/i3c/master/dw-i3c-master.c:dw_i3c_master_irq_handler() {
>>>>>>> ...
>>>>>>> status = readl(master->regs + INTR_STATUS);
>>>>>>> ...
>>>>>>> }
>>>>>>>
>>>>>>> Since the driver does not synchronize IRQs in its runtime_suspend
>>>>>>> callback, if an interrupt arrives right as the device enters
>>>>>>> runtime suspend (or if a spurious interrupt occurs), will this raw
>>>>>>> register read access powered-down hardware and cause a synchronous
>>>>>>> external abort?
>>>>>>>
>>>>>>>> +static int dw_i3c_master_set_speed(struct i3c_master_controller
>>>>>>>> +*m,
>>>>>>>> + enum i3c_open_drain_speed speed) {
>>>>>>> [ ... ]
>>>>>>>> + switch (speed) {
>>>>>>>> + case I3C_OPEN_DRAIN_SLOW_SPEED:
>>>>>>> [ ... ]
>>>>>>>> + core_period = DIV_ROUND_UP(1000000000, core_rate);
>>>>>>>> + lcnt =
>>>>>>>> +SCL_I3C_TIMING_LCNT(master->i3c_od_timing_normal);
>>>>>>>> + od_hcnt = max_t(u8, SCL_I3C_TIMING_CNT_MIN,
>>>>>>>> +
>>>>>>>> +DIV_ROUND_UP(I3C_BUS_THIGH_INIT_OD_MIN_NS,
>>>>>>>> + core_period) - 1);
>>>>>>>
>>>>>>> [Severity: High]
>>>>>>> Does this precision loss guarantee the resulting pulse is shorter
>>>>>>> than the specification minimum for certain clock frequencies?
>>>>>>>
>>>>>>> If core_period is calculated by rounding up 1000000000 /
>>>>>>> core_rate, using this overestimated integer in the division
>>>>>>> DIV_ROUND_UP(I3C_BUS_THIGH_INIT_OD_MIN_NS, core_period) - 1
>>>>>>> underestimates the necessary cycle count.
>>>>>>>
>>>>>>> For example, at a 133.3 MHz core rate, the exact period is 7.5 ns
>>>>>>> but core_period evaluates to 8 ns. The calculated cycle count
>>>>>>> evaluates to 25 cycles, which produces a 187.5 ns pulse.
>>>>>>>
>>>>>>> Does this violate the strict 200 ns minimum for tHIGH_INIT
>>>>>>> required by the I3C specification?
>>>>>>
>>>>>> Is it true?
>>>>>>
>>>>>>>
>>>>>>>> + 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);
>>>>>>>
>>>>>>> [Severity: High]
>>>>>>> Does unconditionally restoring master->i3c_od_timing_normal here
>>>>>>> permanently overwrite hardware-specific AMD timings with generic
>>>>>>> timings?
>>>>>>>
>>>>>>> If amd_configure_od_pp_quirk() is not updated to modify
>>>>>>> master->i3c_od_timing_normal, restoring it after a broadcast
>>>>>>> master->destroys
>>>>>>> the
>>>>>>> required AMD-specific quirk timings.
>>>>>>>
>>>>>>> Additionally, does dw_i3c_master_send_ccc_cmd() unconditionally
>>>>>>> overwrite the OD timing with AMD normal timing right before
>>>>>>> sending a broadcast CCC, bypassing the
>> I3C_OPEN_DRAIN_SLOW_SPEED
>>>>>>> state expected by the I3C core?
>>>>>>>
>>>>>>>> + master->i3c_od_timing = master->i3c_od_timing_normal;
>>>>>>>> + break;
>>>>>>
>>>>>> Please check this also?
>>>>>>
>>>>>> Frank
>>>>>>
>>>>>>> [ ... ]
>>>>>>>> +static int dw_i3c_master_set_dev_nack_retry(struct
>>>>>>>> i3c_master_controller *m,
>>>>>>>> + unsigned long
>>>>>>>> dev_nack_retry_cnt)
>>>>>>>> +{
>>>>>>> [ ... ]
>>>>>>>> + for (i = 0; i < master->maxdevs; i++) {
>>>>>>>> + /* Skip free/empty slots */
>>>>>>>> + if (master->free_pos & BIT(i))
>>>>>>>> + continue;
>>>>>>>> +
>>>>>>>> + reg = readl(master->regs +
>>>>>>>> +
>>>>>>>> +DEV_ADDR_TABLE_LOC(master->datstartaddr,
>>>>>>>> i));
>>>>>>>
>>>>>>> [Severity: High]
>>>>>>> This is a pre-existing issue, but does this function modify
>>>>>>> hardware registers without resuming the device via Runtime PM?
>>>>>>>
>>>>>>> The sysfs attribute dev_nack_retry_count directly invokes
>>>>>>> master->ops->set_dev_nack_retry(). Since sysfs callbacks can be
>>>>>>> triggered
>>>>>>> from userspace at any time, executing this while the device is
>>>>>>> runtime suspended will access powered-down hardware.
>>>>>>>
>>>>>>> On ARM SoCs, can this trigger a kernel panic (external abort) and
>>>>>>> allow userspace to trivially crash the system?
>>>>>>>
>>>>>>> [ ... ]
>>>>>>>
>>>>>>> [Severity: High]
>>>>>>> This is a pre-existing issue, but is there a use-after-free or
>>>>>>> invalid
>>>>>>> queue_work() due to improper teardown ordering in
>>>>>>> dw_i3c_common_remove()?
>>>>>>>
>>>>>>> dw_i3c_common_remove() calls cancel_work_sync(&master->hj_work)
>>>>>>> before the
>>>>>>> hardware interrupt is disabled and freed by devres:
>>>>>>>
>>>>>>> drivers/i3c/master/dw-i3c-master.c:dw_i3c_common_remove() {
>>>>>>> ...
>>>>>>> cancel_work_sync(&master->hj_work);
>>>>>>> i3c_master_unregister(&master->base);
>>>>>>> ...
>>>>>>> }
>>>>>>>
>>>>>>> Because the IRQ remains active throughout the function and during
>>>>>>> early devres teardown, can an incoming Hot Join interrupt trigger
>>>>>>> the IRQ handler, which will call queue_work() to re-queue hj_work?
>>>>>>>
>>>>>>> Does this either cause queue_work() to execute on a destroyed
>>>>>>> workqueue or result in a use-after-free when the workqueue
>>>>>>> subsequently processes the work using the master object that has
>>>>>>> already been freed by devres?
>>>>>>>
>>>>>>> --
>>>>>>> Sashiko AI review · https://nam10.safelinks.protection.outlook.com/?
>>>>>>>
>>>>
>> url=https%3A%2F%2Fsashiko.dev%2F%23%2Fpatchset%2Febddb8b62eae92de0a
>>>> 7
>>>>>>> eeda93cb18213c677ae96.1781077653.git.tze.yee.ng%40altera.com%3Fpar
>>>>>>> t%
>>>>>>>
>>>>
>> 3D1&data=05%7C02%7Ctze.yee.ng%40altera.com%7C8e2758995cb645fd357608
>>>> d
>>>>>>>
>>>>
>> ec7d58fc1%7Cfbd72e03d4a54110adce614d51f2077a%7C0%7C0%7C6391679172
>>>> 502
>>>>>>>
>>>>
>> 16401%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLj
>>>> AuMD
>>>>>>>
>>>>
>> AwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%
>>>> 7C&
>>>>>>>
>>>>
>> sdata=by5%2FiOLcg8kdN%2FkGUwg3B988Zkk3%2FjQJA%2FOqMllYOY0%3D&r
>>>> eserve
>>>>>>> d=0
>>>>>>>
>>>>>>> --
>>>>>>> linux-i3c mailing list
>>>>>>> linux-i3c at lists.infradead.org
>>>>>>> https://nam10.safelinks.protection.outlook.com/?
>>>>>>> url=http%3A%2F%2Flists.infradead.org%2Fmailman%2Flistinfo%2Flinux-
>>>>>>>
>>>>
>> i3c&data=05%7C02%7Ctze.yee.ng%40altera.com%7C8e2758995cb645fd357608d
>>>>>>>
>>>>
>> ec7d58fc1%7Cfbd72e03d4a54110adce614d51f2077a%7C0%7C0%7C6391679172
>>>> 502
>>>>>>>
>>>>
>> 61515%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLj
>>>> AuMD
>>>>>>>
>>>>
>> AwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%
>>>> 7C&
>>>>>>>
>> sdata=yRfffqH10vlfqPSccrkt3XJkPqEDr7Ecoe08uXGamcQ%3D&reserved=0
>>>>>
>>>>> Hi Frank,
>>>>>
>>>>> Sashiko is correct that the initial core_period-based calculation
>>>>> could undershoot the 200 ns tHIGH_INIT minimum at some core clock
>>>>> rates. I checked the Zephyr DesignWare I3C driver, which uses a
>>>>> core_rate-based calculation aligned with the DWC timing model. v2
>>>>> will adopt the same
>>>>> approach:
>>>>>
>>>>> od_hcnt = DIV_ROUND_UP((u64)I3C_BUS_THIGH_INIT_OD_MIN_NS *
>>>> core_rate,
>>>>> 1000000000ULL) - 1;
>>>>>
>>>>> still using the existing - 1 and SCL_I3C_TIMING_CNT_MIN convention
>>>>> from dw_i3c_clk_cfg().
>>>>>
>>>>> Reference: https://github.com/zephyrproject-rtos/zephyr/
>>>>> commit/4d6673bc693920bdacf96d7cd3cfeb71d421e0ae
>>>>> (drivers/i3c/i3c_dw.c,
>>>>> dw_i3c_init_scl_timing())
>>>>>
>>>>> For AMD specific timings, I left existing AMD behavior unchanged in v1.
>>>>> @Manikanta Guntupalli, for AMDI0015, should ->set_speed() use
>>>>> AMD_I3C_OD_TIMING or generic timing as the normal OD baseline at
>>>>> slow/ normal speed? I can add AMD-specific handling in v2 once that is
>> clarified.
>>>>>
>>>>> Thanks,
>>>>> Tze Yee
>>>>
>>>> Hi Manikanta,
>>>>
>>>> Just checking whether you had a chance to look at the AMD
>>>> dw-i3c-master question below.
>>>>
>>>> For AMDI0015, I need to know whether ->set_speed() should use
>>>> AMD_I3C_OD_TIMING or generic OD timing as the normal baseline, and
>>>> whether
>>>> send_ccc_cmd() should stop overwriting OD timing before RSTDAA.
>>>>
>>>> Once I have your guidance, I can send an AMD-specific follow-up patch.
>>>>
>>>> Thanks,
>>>> Tze Yee Ng
>>>
>
More information about the linux-i3c
mailing list