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 <[email protected]>
>> Sent: Friday, June 19, 2026 1:49 PM
>> To: Guntupalli, Manikanta <[email protected]>
>> Cc: [email protected]; [email protected]; Frank Li
>> <[email protected]>
>> 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 [email protected]. Learn why this
>>>> is important at https://aka.ms/LearnAboutSenderIdentification ]
>>>>
>>>> On Wed, Jun 10, 2026 at 08:36:55AM +0000, [email protected] 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 <[email protected]>
>>>>>
>>>>> 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%3Fpart%
>>>>>
>> 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
>>>>> [email protected]
>>>>> 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
>

-- 
linux-i3c mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-i3c