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

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

Hi frank，
     Thanks for the review.

On 12/11/2026 03:36, Frank Li wrote:
> [ EXTERNAL EMAIL ]
> 
> On Tue, Nov 10, 2026 at 01:23:23PM +0800, Junyi Zhao wrote:
>>
>> Hi frank，
>>        thks for your review.
>> On 07/11/2026 01:15, Frank Li wrote:
>>> [ EXTERNAL EMAIL ]
>>>
>>> On Fri, Nov 06, 2026 at 03:13:08PM +0800, Junyi Zhao wrote:
>>>> Amlogic a9 SoCs integrate the Silvaco I3C IP, and their peripheral
>>>> clocks vary from the vanilla implementation:
>>>>
>>>>     - The EE-domain controller (amlogic,a9-i3c) takes two clocks,
>>>>       "pclk" and "fast_clk", and the fast clock rate must be programmed
>>>>       before the controller can be used.
>>>>
>>>>     - The AO-domain controller (amlogic,a9-ao-i3c) exposes only "pclk":
>>>>       its bus clock is generated by an internal divider located in the
>>>>       controller's own register space.
>>>>
>>>> Both variants need setup that the generic driver does not perform:
>>>> the fast clock rate must be rounded and set at probe time, and on the
>>>> AO variant the internal divider must be registered with the clock
>>>> framework so that "fast_clk" even exists. The divider registers sit
>>>> in the controller's own register space, so they can only be accessed
>>>> while pclk is running.
>>>>
>>>> Add a vendor_init callback, invoked from the probe with the clocks
>>>> enabled, to handle this. On the AO variant it registers the internal
>>>> divider chain (div_en -> div -> gate) with the common clock framework
>>>> and appends the resulting clock to the clk bulk array as "fast_clk";
>>>> for both variants it then rounds and sets the fast clock rate
>>>> (target 25 MHz). As vendor_init now runs after
>>>> clk_bulk_prepare_enable(), the probe error paths that follow disable
>>>> the clocks on failure.
>>>>
>>>> Extend the quirks bitmask with an Amlogic-specific bit
>>>> (AML_I3C_QUIRK_AO) marking the AO variant's internal-divider
>>>> behaviour. Since the driver now registers clocks through the common
>>>> clock framework, SVC_I3C_MASTER also depends on COMMON_CLK.
>>>>
>>>> Signed-off-by: Junyi Zhao <junyi.zhao@amlogic.com>
>>>> ---
>>>>    drivers/i3c/master/Kconfig          |   1 +
>>>>    drivers/i3c/master/svc-i3c-master.c | 274 ++++++++++++++++++++++++++++++++++--
>>>>    2 files changed, 263 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/drivers/i3c/master/Kconfig b/drivers/i3c/master/Kconfig
>>>> index da96d2aaa..ac7d246c2 100644
>>>> --- a/drivers/i3c/master/Kconfig
>>>> +++ b/drivers/i3c/master/Kconfig
>>>> @@ -47,6 +47,7 @@ config AST2600_I3C_MASTER
>>>>
>>>>    config SVC_I3C_MASTER
>>>>         tristate "Silvaco I3C Dual-Role Master driver"
>>>> +     depends on COMMON_CLK
>>>>         depends on HAS_IOMEM
>>>>         depends on !(ALPHA || PARISC)
>>>>         help
>>>> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
>>>> index e700392f0..aef56286e 100644
>>>> --- a/drivers/i3c/master/svc-i3c-master.c
>>>> +++ b/drivers/i3c/master/svc-i3c-master.c
>>>> @@ -9,6 +9,7 @@
>>>>
>>>>    #include <linux/bitfield.h>
>>>>    #include <linux/clk.h>
>>>> +#include <linux/clk-provider.h>
>>>>    #include <linux/completion.h>
>>>>    #include <linux/errno.h>
>>>>    #include <linux/i3c/master.h>
>>>> @@ -166,6 +167,31 @@
>>>>     */
>>>>    #define SVC_I3C_QUIRK_DAA_CORRUPT    BIT(2)
>>>>
>>>> +/*
>>>> + * AML_I3C_QUIRK_AO:
>>>> + * AO is the Always-On domain on Amlogic SoCs (remains powered in suspend).
>>>> + * Its clock is generated by an internal divider located in the I3C
>>>> + * controller's own register space.
>>>> + */
>>>> +#define AML_I3C_QUIRK_AO     BIT(3)
>>>> +
>>>> +/*
>>>> + * Quirks shared by all the Amlogic A9 I3C instances.
>>>> + */
>>>> +#define AML_A9_I3C_QUIRKS    (SVC_I3C_QUIRK_FIFO_EMPTY | \
>>>> +                              SVC_I3C_QUIRK_FALSE_SLVSTART | \
>>>> +                              SVC_I3C_QUIRK_DAA_CORRUPT)
>>>> +
>>>> +/* Amlogic specific registers */
>>>> +#define AML_AO_I3C_FCLK_DIV_CTRL             0x304
>>>
>>> Do you have other registers after 0x304? Is it possible create indivial
>>> clock driver for it, reg space is such as 0x300..0x400.
>>>
>>> I3C use 0..0x300
>>>
>>> So needn't distinguish w/o AO.
>>>
>>
>> e.g. I3C_AO_DEBUG_MODIFY at 0x308.
> 
> Does it use in I3C driver?
> 

Not by the current driver code. I3C_AO_DEBUG_MODIFY (0x308) is a debug 
switch that bypasses the Amlogic-specific customizations integrated into 
this controller instance. It can revert the block to the vanilla Silvaco 
behaviour. That places it unambiguously in the I3C controller's own 
register file, not in a clock block， So the 0x300-0x400 tail is better 
described as the Amlogic integration registers of the I3C AO controller 
(divider +debug/bypass switch), not a dedicated clock region.

The debug/bypass switch in AO instance is exactly the kind of register a 
future may need to touch. I would rather not carve the controller's own 
integration registers away from the controller driver.

>>
>> 0x300-0x400 is not a clean clock block.
>>
>> Its registers are only accessible while the controller's pclk is
>> running,
> 
> You can set pclk as parent clock, so access's parent clock should be
> already enabled.
> 

Agreed - CLK_OPS_PARENT_ENABLE , I will set that flag on the internally 
registered clocks in v4 regardless (it also fixes the unclocked-MMIO 
issue the bot reported).

> Frank
> 
>> so its state is tightly coupled to the controller's own
>> power/clock state.
>>
>>
>>> Frank
>>>
>>>> +#define AML_AO_I3C_FCLK_DIV_EN_SHIFT         8
>>>> +#define AML_AO_I3C_FCLK_DIV_SHIFT            0
>>>> +#define AML_AO_I3C_FCLK_DIV_WIDTH            8
>>>> +#define AML_AO_I3C_FCLK_EN_SHIFT             9
>>>> +
>>>> +/* Target fast clock rate of the Amlogic A9 I3C controllers, in Hz */
>>>> +#define AML_I3C_FAST_CLK_RATE                25000000
>>>> +
>>>>    struct svc_i3c_cmd {
>>>>         u8 addr;
>>>>         union {
>>>> @@ -195,8 +221,11 @@ struct svc_i3c_regs_save {
>>>>         u32 mdynaddr;
>>>>    };
>>>>
>>>> +struct svc_i3c_master;
>>>> +
>>>>    struct svc_i3c_drvdata {
>>>>         u32 quirks;
>>>> +     int (*vendor_init)(struct svc_i3c_master *master);
>>>>    };
>>>>
>>>>    /**
>>>> @@ -2011,6 +2040,7 @@ static int svc_i3c_master_probe(struct platform_device *pdev)
>>>>         if (!master->drvdata)
>>>>                 return -EINVAL;
>>>>
>>>> +     master->dev = dev;
>>>>         master->regs = devm_platform_ioremap_resource(pdev, 0);
>>>>         if (IS_ERR(master->regs))
>>>>                 return PTR_ERR(master->regs);
>>>> @@ -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;
>>>> +     }
>>>>
>>>>         master->irq = platform_get_irq(pdev, 0);
>>>> -     if (master->irq < 0)
>>>> -             return master->irq;
>>>> -
>>>> -     master->dev = dev;
>>>> -     ret = clk_bulk_prepare_enable(master->num_clks, master->clks);
>>>> -     if (ret)
>>>> -             return dev_err_probe(dev, ret, "can't enable I3C clocks\n");
>>>> +     if (master->irq < 0) {
>>>> +             ret = master->irq;
>>>> +             goto err_disable_clks;
>>>> +     }
>>>>
>>>>         mutex_init(&master->lock);
>>>>
>>>> @@ -2163,9 +2209,213 @@ static const struct svc_i3c_drvdata npcm845_drvdata = {
>>>>
>>>>    static const struct svc_i3c_drvdata svc_default_drvdata = {};
>>>>
>>>> +/**
>>>> + * struct aml_i3c_ao_clk - container for the AO variant's internal fast clock
>>>> + * @div_en: enables the internal divider
>>>> + * @div: bus clock divider
>>>> + * @gate: output gate of the generated fast clock
>>>> + * @lock: spinlock shared by the gate and divider register operations
>>>> + *
>>>> + * The clock HW structures are registered with the clock framework and must
>>>> + * stay alive for the whole driver lifetime, so they are kept along with the
>>>> + * shared lock used by their gate/divider operations.
>>>> + */
>>>> +struct aml_i3c_ao_clk {
>>>> +     struct clk_gate div_en;
>>>> +     struct clk_divider div;
>>>> +     struct clk_gate gate;
>>>> +     spinlock_t lock;
>>>> +};
>>>> +
>>>> +/**
>>>> + * aml_ao_i3c_tail_clocks - register the AO variant's internal fast clock
>>>> + * @master: I3C master controller
>>>> + * @parent: parent clock of the internal divider chain; must be "pclk", the
>>>> + *          only clock the AO variant exposes (see silvaco,i3c-master.yaml)
>>>> + *
>>>> + * Register the internal divider chain that produces the AO variant's
>>>> + * fast_clk and append it to the clk bulk array as "fast_clk". The caller
>>>> + * must run with the clocks enabled since the divider registers are
>>>> + * accessed during registration. The appended clock is prepared and
>>>> + * enabled here as well, because clk_bulk_prepare_enable() already ran
>>>> + * in the probe with the original clock list.
>>>> + *
>>>> + * Return: 0 on success, or a negative error code.
>>>> + */
>>>> +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);
>>>> +
>>>> +     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;
>>>> +}
>>>> +
>>>> +/**
>>>> + * aml_i3c_init - Amlogic A9 specific init, called with the clocks enabled
>>>> + * @master: I3C master controller
>>>> + *
>>>> + * Register the AO variant's internal divider chain and round/set the fast
>>>> + * clock rate for both variants.
>>>> + *
>>>> + * Return: 0 on success, or a negative error code.
>>>> + */
>>>> +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");
>>>> +
>>>> +             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);
>>>> +
>>>> +     fin_freq = clk_round_rate(fclk, AML_I3C_FAST_CLK_RATE);
>>>> +     if (fin_freq <= 0)
>>>> +             return dev_err_probe(dev, fin_freq ? fin_freq : -EINVAL,
>>>> +                                  "invalid fast_clk\n");
>>>> +
>>>> +     ret = clk_set_rate(fclk, fin_freq);
>>>> +     if (ret)
>>>> +             return dev_err_probe(dev, ret, "setting fast_clk rate failed\n");
>>>> +
>>>> +     return 0;
>>>> +}
>>>> +
>>>> +static const struct svc_i3c_drvdata aml_a9_drvdata = {
>>>> +     .quirks = AML_A9_I3C_QUIRKS,
>>>> +     .vendor_init = aml_i3c_init,
>>>> +};
>>>> +
>>>> +static const struct svc_i3c_drvdata aml_a9_ao_drvdata = {
>>>> +     .quirks = AML_A9_I3C_QUIRKS | AML_I3C_QUIRK_AO,
>>>> +     .vendor_init = aml_i3c_init,
>>>> +};
>>>> +
>>>>    static const struct of_device_id svc_i3c_master_of_match_tbl[] = {
>>>>         { .compatible = "nuvoton,npcm845-i3c", .data = &npcm845_drvdata },
>>>>         { .compatible = "silvaco,i3c-master-v1", .data = &svc_default_drvdata },
>>>> +     { .compatible = "amlogic,a9-i3c", .data = &aml_a9_drvdata },
>>>> +     { .compatible = "amlogic,a9-ao-i3c", .data = &aml_a9_ao_drvdata },
>>>>         { /* sentinel */ },
>>>>    };
>>>>    MODULE_DEVICE_TABLE(of, svc_i3c_master_of_match_tbl);
>>>>
>>>> --
>>>> 2.42.0
>>>>
>>