[PATCH v3 1/2] i3c: master: svc: Add amlogic a9 support

[Junyi Zhao <junyi.zhao@amlogic.com>]

Hi sashiko,
Thks the detail.

On 06/11/2026 15:32, sashiko-bot@kernel.org wrote:
> [你通常不会收到来自 sashiko-bot@kernel.org 的电子邮件。请访问 https://aka.ms/LearnAboutSenderIdentification，以了解这一点为什么很重要]
> 
> [ EXTERNAL EMAIL ]
> 
> Thank you for your contribution! Sashiko AI review found 4 potential
> issues to consider:
> - [Medium] i3c: master: svc: unbalanced fclk disable in
>    aml_ao_i3c_tail_clocks()
> - [Medium] i3c: master: svc: NULL dereference of clks[0].id in
>    aml_i3c_init()
> - [Medium] i3c: master: svc: unclocked MMIO read in
>    aml_ao_i3c_tail_clocks()
> - [Low] i3c: master: svc: duplicate fast_clk lookup in aml_i3c_init()
> --
> 
> commit 13e8b1ef64cdb3b2e6850ddcbaa3e5a7ffee2a56
> Author: Junyi Zhao <junyi.zhao@amlogic.com>
> 
> i3c: master: svc: Add amlogic a9 support
> 
> This commit adds Amlogic A9 and A9 AO I3C controller support to the
> Silvaco I3C master driver, including a vendor_init callback that registers
> the AO variant's internal clock divider chain and configures the fast_clk
> rate.
> 
>> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
>> index e700392f0801f..aef56286ecfa8 100644
>> --- a/drivers/i3c/master/svc-i3c-master.c
>> +++ b/drivers/i3c/master/svc-i3c-master.c
> [ ... ]
>> @@ -2019,27 +2049,43 @@ static int svc_i3c_master_probe(struct platform_device *pdev)
>>        if (master->num_clks < 0)
>>                return dev_err_probe(dev, -EINVAL, "can't get I3C clocks\n");
>>
>> +     ret = clk_bulk_prepare_enable(master->num_clks, master->clks);
>> +     if (ret)
>> +             return dev_err_probe(dev, ret, "can't enable I3C clocks\n");
>> +
>> +     /*
>> +      * Vendor specific setup may register additional clocks (e.g. the
>> +      * internal divider chain of the AO variant) and program clock rates,
>> +      * so it must run with the clocks enabled.
>> +      */
>> +     if (master->drvdata->vendor_init) {
>> +             ret = master->drvdata->vendor_init(master);
>> +             if (ret)
>> +                     goto err_disable_clks;
>> +     }
>> +
>>        for (i = 0; i < master->num_clks; i++) {
>>                if (!strcmp(master->clks[i].id, "fast_clk"))
>>                        break;
>>        }
>>
>> -     if (i == master->num_clks)
>> -             return dev_err_probe(dev, -EINVAL,
>> -                                  "can't get I3C peripheral clock\n");
>> +     if (i == master->num_clks) {
>> +             ret = dev_err_probe(dev, -EINVAL,
>> +                                 "can't get I3C peripheral clock\n");
>> +             goto err_disable_clks;
>> +     }
>>
>>        master->fclk = master->clks[i].clk;
>> -     if (IS_ERR(master->fclk))
>> -             return PTR_ERR(master->fclk);
>> +     if (IS_ERR(master->fclk)) {
>> +             ret = PTR_ERR(master->fclk);
>> +             goto err_disable_clks;
>> +     }
> [ ... ]
>> @@ -2163,9 +2209,213 @@ static const struct svc_i3c_drvdata npcm845_drvdata = {
> [ ... ]
>> +static int aml_ao_i3c_tail_clocks(struct svc_i3c_master *master, struct clk *parent)
>> +{
>> +     struct device *dev = master->dev;
>> +     struct clk_bulk_data *new_clks;
>> +     struct clk_init_data init = {};
>> +     struct aml_i3c_ao_clk *ao;
>> +     struct clk *fclk;
>> +     char name[64];
>> +     int ret;
>> +
>> +     ao = devm_kzalloc(dev, sizeof(*ao), GFP_KERNEL);
>> +     if (!ao)
>> +             return -ENOMEM;
>> +
>> +     spin_lock_init(&ao->lock);
>> +
>> +     snprintf(name, sizeof(name), "%s#div_en", dev_name(dev));
>> +     init.name = name;
>> +     init.ops = &clk_gate_ops;
>> +     init.flags = CLK_IGNORE_UNUSED;
>> +     init.parent_data = &(struct clk_parent_data) {
>> +             .index = -1,
>> +             .hw = __clk_get_hw(parent),
>> +     };
>> +     init.num_parents = 1;
>> +
>> +     ao->div_en.reg = master->regs + AML_AO_I3C_FCLK_DIV_CTRL;
>> +     ao->div_en.bit_idx = AML_AO_I3C_FCLK_DIV_EN_SHIFT;
>> +     ao->div_en.hw.init = &init;
>> +     ao->div_en.lock = &ao->lock;
>> +
>> +     ret = devm_clk_hw_register(dev, &ao->div_en.hw);
>> +     if (ret)
>> +             return dev_err_probe(dev, ret, "failed to register %s\n", name);
>> +
>> +     snprintf(name, sizeof(name), "%s#div", dev_name(dev));
>> +     init.name = name;
>> +     init.ops = &clk_divider_ops;
>> +     init.flags = 0;
>> +     init.parent_data = &(struct clk_parent_data) {
>> +             .index = -1,
>> +             .hw = &ao->div_en.hw,
>> +     };
>> +     init.num_parents = 1;
>> +
>> +     ao->div.reg = master->regs + AML_AO_I3C_FCLK_DIV_CTRL;
>> +     ao->div.shift = AML_AO_I3C_FCLK_DIV_SHIFT;
>> +     ao->div.width = AML_AO_I3C_FCLK_DIV_WIDTH;
>> +     ao->div.hw.init = &init;
>> +     ao->div.lock = &ao->lock;
>> +
>> +     ret = devm_clk_hw_register(dev, &ao->div.hw);
>> +     if (ret)
>> +             return dev_err_probe(dev, ret, "failed to register %s\n", name);
>> +
>> +     snprintf(name, sizeof(name), "%s#gate", dev_name(dev));
>> +     init.name = name;
>> +     init.ops = &clk_gate_ops;
>> +     init.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED;
>> +     init.parent_data = &(struct clk_parent_data) {
>> +             .index = -1,
>> +             .hw = &ao->div.hw,
>> +     };
>> +     init.num_parents = 1;
>> +
>> +     ao->gate.reg = master->regs + AML_AO_I3C_FCLK_DIV_CTRL;
>> +     ao->gate.bit_idx = AML_AO_I3C_FCLK_EN_SHIFT;
>> +     ao->gate.hw.init = &init;
>> +     ao->gate.lock = &ao->lock;
>> +
>> +     ret = devm_clk_hw_register(dev, &ao->gate.hw);
>> +     if (ret)
>> +             return dev_err_probe(dev, ret, "failed to register %s\n", name);
> 
> [Severity: Medium]
> Can the clock framework read AML_AO_I3C_FCLK_DIV_CTRL while pclk is
> disabled during runtime suspend?
> 
> svc_i3c_master_probe() invokes vendor_init() before calling
> pm_runtime_enable(), so clk_pm_runtime_init() sees
> pm_runtime_enabled(dev) == false and leaves core->rpm_enabled unset for
> div_en, div, and gate, and none of the three clocks sets
> CLK_OPS_PARENT_ENABLE.  Once svc_i3c_runtime_suspend() disables pclk,
> any clock query (such as reading /sys/kernel/debug/clk/clk_summary)
> calls clk_gate_ops.is_enabled or clk_divider_ops.recalc_rate and performs
> an unclocked MMIO read of AML_AO_I3C_FCLK_DIV_CTRL.
> 
clk_bulk_prepare_enable is before vendor init.
So, pclk enabled before registering.

>> +     fclk = devm_clk_hw_get_clk(dev, &ao->gate.hw, NULL);
>> +     if (IS_ERR(fclk))
>> +             return dev_err_probe(dev, PTR_ERR(fclk),
>> +                                  "tail i3c fast clock failed\n");
>> +
>> +     new_clks = devm_kcalloc(dev, master->num_clks + 1, sizeof(*new_clks),
>> +                             GFP_KERNEL);
>> +     if (!new_clks)
>> +             return -ENOMEM;
>> +
>> +     memcpy(new_clks, master->clks,
>> +            master->num_clks * sizeof(*master->clks));
>> +     new_clks[master->num_clks].id = "fast_clk";
>> +     new_clks[master->num_clks].clk = fclk;
>> +     master->clks = new_clks;
>> +     master->num_clks++;
>> +
>> +     /*
>> +      * Keep the clk bulk state balanced for the following
>> +      * clk_bulk_disable_unprepare() calls (error paths and runtime PM).
>> +      */
>> +     ret = clk_prepare_enable(fclk);
>> +     if (ret)
>> +             return dev_err_probe(dev, ret, "can't enable fast_clk\n");
>> +
>> +     return 0;
>> +}
> 
> [Severity: Medium]
> If clk_prepare_enable(fclk) fails here, does the err_disable_clks path in
> svc_i3c_master_probe() call clk_bulk_disable_unprepare() on an unprepared
> and disabled fclk?  master->clks and master->num_clks are updated to
> include fclk before clk_prepare_enable(fclk) succeeds, which will trigger
> WARN() assertions in clk_core_disable() and clk_core_unprepare().
> 

Thks the detail.

Could move clk_prepare_enable(fclk) before creating new_clks.


>> +static int aml_i3c_init(struct svc_i3c_master *master)
>> +{
>> +     struct device *dev = master->dev;
>> +     struct clk *fclk;
>> +     long fin_freq;
>> +     int ret, i;
>> +
>> +     if (svc_has_quirk(master, AML_I3C_QUIRK_AO)) {
>> +             /*
>> +              * clk_bulk_get_all() preserves the DT order, and the
>> +              * binding restricts the AO variant to exactly one clock,
>> +              * which is always "pclk".  So clks[0] is the input of the
>> +              * internal divider chain.
>> +              */
>> +             if (strcmp(master->clks[0].id, "pclk"))
>> +                     return dev_err_probe(dev, -EINVAL,
>> +                                          "invalid AO input clock\n");
> 
> [Severity: Medium]
> Can this dereference a NULL pointer if the DT node omits clocks or
> clock-names?
> 
> devm_clk_bulk_get_all() returns 0 with master->clks == NULL when no
> clocks are present in DT (passing the master->num_clks < 0 check in
> svc_i3c_master_probe()), and leaves master->clks[0].id as NULL if
> clock-names is missing.


The DT binding requires the AO variant clock to be "pclk", so this case 
cannot occur.

> 
>> +
>> +             ret = aml_ao_i3c_tail_clocks(master, master->clks[0].clk);
>> +             if (ret)
>> +                     return ret;
>> +     }
>> +
>> +     for (i = 0; i < master->num_clks; i++) {
>> +             if (!strcmp(master->clks[i].id, "fast_clk"))
>> +                     break;
>> +     }
>> +
>> +     if (i == master->num_clks)
>> +             return dev_err_probe(dev, -EINVAL,
>> +                                  "can't get I3C fast clock\n");
>> +
>> +     fclk = master->clks[i].clk;
>> +     if (IS_ERR(fclk))
>> +             return PTR_ERR(fclk);
> 
> [Severity: Low]
> This isn't a bug, but this fast_clk lookup and IS_ERR() check duplicate
> the identical loop that runs in svc_i3c_master_probe() right after
> vendor_init() returns.  Could master->fclk be resolved once in a helper
> or before setting the clock rate?
>> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20261106-a9_i3c-v3-0-804b69b9095b@amlogic.com?part=1


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