* [PATCH v2 1/2] i3c: master: svc: Add amlogic a9 support
@ 2026-11-02 11:05 ` Junyi Zhao via B4 Relay
0 siblings, 0 replies; 48+ messages in thread
From: Junyi Zhao via B4 Relay @ 2026-11-02 11:05 UTC (permalink / raw)
To: Alexandre Belloni, Frank Li, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Neil Armstrong, Kevin Hilman, Jerome Brunet,
Martin Blumenstingl, Miquel Raynal, Kees Cook,
Gustavo A. R. Silva
Cc: linux-i3c, devicetree, linux-kernel, linux-arm-kernel,
linux-amlogic, imx, linux-hardening, xianwei.zhao, Kelvin.Zhang,
Junyi Zhao
From: Junyi Zhao <junyi.zhao@amlogic.com>
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.
Add a vendor_init callback to handle this clock setup.
Add a vendor_quirks field to describe AO-specific behavior.
Signed-off-by: Junyi Zhao <junyi.zhao@amlogic.com>
---
drivers/i3c/master/svc-i3c-master.c | 214 +++++++++++++++++++++++++++++++++++-
1 file changed, 209 insertions(+), 5 deletions(-)
diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index e700392f0..98aa6eb0f 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,21 @@
*/
#define SVC_I3C_QUIRK_DAA_CORRUPT BIT(2)
+/* Amlogic specific registers */
+#define AML_AO_I3C_FCLK_DIV_CTRL 0x304
+#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
+
+/*
+ * 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 own register space.
+ */
+#define AML_I3C_QUIRK_AO BIT(0)
+
struct svc_i3c_cmd {
u8 addr;
union {
@@ -195,10 +211,6 @@ struct svc_i3c_regs_save {
u32 mdynaddr;
};
-struct svc_i3c_drvdata {
- u32 quirks;
-};
-
/**
* struct svc_i3c_master - Silvaco I3C Master structure
* @base: I3C master controller
@@ -257,6 +269,12 @@ struct svc_i3c_master {
u32 mctrl_config;
};
+struct svc_i3c_drvdata {
+ u32 quirks;
+ u32 vendor_quirks;
+ int (*vendor_init)(struct svc_i3c_master *master);
+};
+
/**
* struct svc_i3c_i2c_dev_data - Device specific data
* @index: Index in the master tables corresponding to this device
@@ -274,6 +292,11 @@ static inline bool svc_has_quirk(struct svc_i3c_master *master, u32 quirk)
return (master->drvdata->quirks & quirk);
}
+static inline bool vendor_has_quirk(struct svc_i3c_master *master, u32 quirk)
+{
+ return (master->drvdata->vendor_quirks & quirk);
+}
+
static inline bool svc_has_daa_corrupt(struct svc_i3c_master *master)
{
return ((master->drvdata->quirks & SVC_I3C_QUIRK_DAA_CORRUPT) &&
@@ -2011,6 +2034,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,6 +2043,12 @@ 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");
+ if (master->drvdata->vendor_init) {
+ ret = master->drvdata->vendor_init(master);
+ if (ret)
+ return ret;
+ }
+
for (i = 0; i < master->num_clks; i++) {
if (!strcmp(master->clks[i].id, "fast_clk"))
break;
@@ -2036,7 +2066,6 @@ static int svc_i3c_master_probe(struct platform_device *pdev)
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");
@@ -2163,9 +2192,184 @@ static const struct svc_i3c_drvdata npcm845_drvdata = {
static const struct svc_i3c_drvdata svc_default_drvdata = {};
+/*
+ * Container for the tail clocks of the AO domain variant. 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 internal divider chain that
+ * produces the AO variant's fast_clk, and append it to the clk bulk
+ * array as "fast_clk". @parent must be "pclk", the only clock the AO
+ * variant exposes (see silvaco,i3c-master.yaml).
+ */
+static int aml_ao_i3c_tail_clocks(struct svc_i3c_master *master, struct clk *parent)
+{
+ struct device *dev = master->dev;
+ struct aml_i3c_ao_clk *ao;
+ struct clk_init_data init = {};
+ char name[64];
+ int ret;
+ struct clk_bulk_data *new_clks;
+ struct clk *fclk;
+
+ 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);
+ 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++;
+
+ return 0;
+}
+
+static int aml_i3c_init(struct svc_i3c_master *master)
+{
+ struct device *dev = master->dev;
+ long fin_freq;
+ int ret, i;
+ struct clk *fclk;
+
+ if (vendor_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.
+ */
+ 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, 25000000);
+ if (fin_freq <= 0) {
+ dev_err(dev, "invalid fast_clk\n");
+ return fin_freq ? fin_freq : -EINVAL;
+ }
+
+ 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 = SVC_I3C_QUIRK_FIFO_EMPTY |
+ SVC_I3C_QUIRK_FALSE_SLVSTART |
+ SVC_I3C_QUIRK_DAA_CORRUPT,
+ .vendor_init = aml_i3c_init,
+};
+
+static const struct svc_i3c_drvdata aml_a9_ao_drvdata = {
+ .quirks = SVC_I3C_QUIRK_FIFO_EMPTY |
+ SVC_I3C_QUIRK_FALSE_SLVSTART |
+ SVC_I3C_QUIRK_DAA_CORRUPT,
+ .vendor_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
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply related [flat|nested] 48+ messages in thread* [PATCH v2 1/2] i3c: master: svc: Add amlogic a9 support
@ 2026-11-02 11:05 ` Junyi Zhao via B4 Relay
0 siblings, 0 replies; 48+ messages in thread
From: Junyi Zhao via B4 Relay @ 2026-11-02 11:05 UTC (permalink / raw)
To: Alexandre Belloni, Frank Li, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Neil Armstrong, Kevin Hilman, Jerome Brunet,
Martin Blumenstingl, Miquel Raynal, Kees Cook,
Gustavo A. R. Silva
Cc: linux-i3c, devicetree, linux-kernel, linux-arm-kernel,
linux-amlogic, imx, linux-hardening, xianwei.zhao, Kelvin.Zhang,
Junyi Zhao
From: Junyi Zhao <junyi.zhao@amlogic.com>
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.
Add a vendor_init callback to handle this clock setup.
Add a vendor_quirks field to describe AO-specific behavior.
Signed-off-by: Junyi Zhao <junyi.zhao@amlogic.com>
---
drivers/i3c/master/svc-i3c-master.c | 214 +++++++++++++++++++++++++++++++++++-
1 file changed, 209 insertions(+), 5 deletions(-)
diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index e700392f0..98aa6eb0f 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,21 @@
*/
#define SVC_I3C_QUIRK_DAA_CORRUPT BIT(2)
+/* Amlogic specific registers */
+#define AML_AO_I3C_FCLK_DIV_CTRL 0x304
+#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
+
+/*
+ * 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 own register space.
+ */
+#define AML_I3C_QUIRK_AO BIT(0)
+
struct svc_i3c_cmd {
u8 addr;
union {
@@ -195,10 +211,6 @@ struct svc_i3c_regs_save {
u32 mdynaddr;
};
-struct svc_i3c_drvdata {
- u32 quirks;
-};
-
/**
* struct svc_i3c_master - Silvaco I3C Master structure
* @base: I3C master controller
@@ -257,6 +269,12 @@ struct svc_i3c_master {
u32 mctrl_config;
};
+struct svc_i3c_drvdata {
+ u32 quirks;
+ u32 vendor_quirks;
+ int (*vendor_init)(struct svc_i3c_master *master);
+};
+
/**
* struct svc_i3c_i2c_dev_data - Device specific data
* @index: Index in the master tables corresponding to this device
@@ -274,6 +292,11 @@ static inline bool svc_has_quirk(struct svc_i3c_master *master, u32 quirk)
return (master->drvdata->quirks & quirk);
}
+static inline bool vendor_has_quirk(struct svc_i3c_master *master, u32 quirk)
+{
+ return (master->drvdata->vendor_quirks & quirk);
+}
+
static inline bool svc_has_daa_corrupt(struct svc_i3c_master *master)
{
return ((master->drvdata->quirks & SVC_I3C_QUIRK_DAA_CORRUPT) &&
@@ -2011,6 +2034,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,6 +2043,12 @@ 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");
+ if (master->drvdata->vendor_init) {
+ ret = master->drvdata->vendor_init(master);
+ if (ret)
+ return ret;
+ }
+
for (i = 0; i < master->num_clks; i++) {
if (!strcmp(master->clks[i].id, "fast_clk"))
break;
@@ -2036,7 +2066,6 @@ static int svc_i3c_master_probe(struct platform_device *pdev)
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");
@@ -2163,9 +2192,184 @@ static const struct svc_i3c_drvdata npcm845_drvdata = {
static const struct svc_i3c_drvdata svc_default_drvdata = {};
+/*
+ * Container for the tail clocks of the AO domain variant. 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 internal divider chain that
+ * produces the AO variant's fast_clk, and append it to the clk bulk
+ * array as "fast_clk". @parent must be "pclk", the only clock the AO
+ * variant exposes (see silvaco,i3c-master.yaml).
+ */
+static int aml_ao_i3c_tail_clocks(struct svc_i3c_master *master, struct clk *parent)
+{
+ struct device *dev = master->dev;
+ struct aml_i3c_ao_clk *ao;
+ struct clk_init_data init = {};
+ char name[64];
+ int ret;
+ struct clk_bulk_data *new_clks;
+ struct clk *fclk;
+
+ 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);
+ 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++;
+
+ return 0;
+}
+
+static int aml_i3c_init(struct svc_i3c_master *master)
+{
+ struct device *dev = master->dev;
+ long fin_freq;
+ int ret, i;
+ struct clk *fclk;
+
+ if (vendor_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.
+ */
+ 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, 25000000);
+ if (fin_freq <= 0) {
+ dev_err(dev, "invalid fast_clk\n");
+ return fin_freq ? fin_freq : -EINVAL;
+ }
+
+ 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 = SVC_I3C_QUIRK_FIFO_EMPTY |
+ SVC_I3C_QUIRK_FALSE_SLVSTART |
+ SVC_I3C_QUIRK_DAA_CORRUPT,
+ .vendor_init = aml_i3c_init,
+};
+
+static const struct svc_i3c_drvdata aml_a9_ao_drvdata = {
+ .quirks = SVC_I3C_QUIRK_FIFO_EMPTY |
+ SVC_I3C_QUIRK_FALSE_SLVSTART |
+ SVC_I3C_QUIRK_DAA_CORRUPT,
+ .vendor_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
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply related [flat|nested] 48+ messages in thread* [PATCH v2 1/2] i3c: master: svc: Add amlogic a9 support
@ 2026-11-02 11:05 ` Junyi Zhao via B4 Relay
0 siblings, 0 replies; 48+ messages in thread
From: Junyi Zhao via B4 Relay @ 2026-11-02 11:05 UTC (permalink / raw)
To: Alexandre Belloni, Frank Li, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Neil Armstrong, Kevin Hilman, Jerome Brunet,
Martin Blumenstingl, Miquel Raynal, Kees Cook,
Gustavo A. R. Silva
Cc: linux-i3c, devicetree, linux-kernel, linux-arm-kernel,
linux-amlogic, imx, linux-hardening, xianwei.zhao, Kelvin.Zhang,
Junyi Zhao
From: Junyi Zhao <junyi.zhao@amlogic.com>
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.
Add a vendor_init callback to handle this clock setup.
Add a vendor_quirks field to describe AO-specific behavior.
Signed-off-by: Junyi Zhao <junyi.zhao@amlogic.com>
---
drivers/i3c/master/svc-i3c-master.c | 214 +++++++++++++++++++++++++++++++++++-
1 file changed, 209 insertions(+), 5 deletions(-)
diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index e700392f0..98aa6eb0f 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,21 @@
*/
#define SVC_I3C_QUIRK_DAA_CORRUPT BIT(2)
+/* Amlogic specific registers */
+#define AML_AO_I3C_FCLK_DIV_CTRL 0x304
+#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
+
+/*
+ * 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 own register space.
+ */
+#define AML_I3C_QUIRK_AO BIT(0)
+
struct svc_i3c_cmd {
u8 addr;
union {
@@ -195,10 +211,6 @@ struct svc_i3c_regs_save {
u32 mdynaddr;
};
-struct svc_i3c_drvdata {
- u32 quirks;
-};
-
/**
* struct svc_i3c_master - Silvaco I3C Master structure
* @base: I3C master controller
@@ -257,6 +269,12 @@ struct svc_i3c_master {
u32 mctrl_config;
};
+struct svc_i3c_drvdata {
+ u32 quirks;
+ u32 vendor_quirks;
+ int (*vendor_init)(struct svc_i3c_master *master);
+};
+
/**
* struct svc_i3c_i2c_dev_data - Device specific data
* @index: Index in the master tables corresponding to this device
@@ -274,6 +292,11 @@ static inline bool svc_has_quirk(struct svc_i3c_master *master, u32 quirk)
return (master->drvdata->quirks & quirk);
}
+static inline bool vendor_has_quirk(struct svc_i3c_master *master, u32 quirk)
+{
+ return (master->drvdata->vendor_quirks & quirk);
+}
+
static inline bool svc_has_daa_corrupt(struct svc_i3c_master *master)
{
return ((master->drvdata->quirks & SVC_I3C_QUIRK_DAA_CORRUPT) &&
@@ -2011,6 +2034,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,6 +2043,12 @@ 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");
+ if (master->drvdata->vendor_init) {
+ ret = master->drvdata->vendor_init(master);
+ if (ret)
+ return ret;
+ }
+
for (i = 0; i < master->num_clks; i++) {
if (!strcmp(master->clks[i].id, "fast_clk"))
break;
@@ -2036,7 +2066,6 @@ static int svc_i3c_master_probe(struct platform_device *pdev)
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");
@@ -2163,9 +2192,184 @@ static const struct svc_i3c_drvdata npcm845_drvdata = {
static const struct svc_i3c_drvdata svc_default_drvdata = {};
+/*
+ * Container for the tail clocks of the AO domain variant. 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 internal divider chain that
+ * produces the AO variant's fast_clk, and append it to the clk bulk
+ * array as "fast_clk". @parent must be "pclk", the only clock the AO
+ * variant exposes (see silvaco,i3c-master.yaml).
+ */
+static int aml_ao_i3c_tail_clocks(struct svc_i3c_master *master, struct clk *parent)
+{
+ struct device *dev = master->dev;
+ struct aml_i3c_ao_clk *ao;
+ struct clk_init_data init = {};
+ char name[64];
+ int ret;
+ struct clk_bulk_data *new_clks;
+ struct clk *fclk;
+
+ 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);
+ 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++;
+
+ return 0;
+}
+
+static int aml_i3c_init(struct svc_i3c_master *master)
+{
+ struct device *dev = master->dev;
+ long fin_freq;
+ int ret, i;
+ struct clk *fclk;
+
+ if (vendor_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.
+ */
+ 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, 25000000);
+ if (fin_freq <= 0) {
+ dev_err(dev, "invalid fast_clk\n");
+ return fin_freq ? fin_freq : -EINVAL;
+ }
+
+ 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 = SVC_I3C_QUIRK_FIFO_EMPTY |
+ SVC_I3C_QUIRK_FALSE_SLVSTART |
+ SVC_I3C_QUIRK_DAA_CORRUPT,
+ .vendor_init = aml_i3c_init,
+};
+
+static const struct svc_i3c_drvdata aml_a9_ao_drvdata = {
+ .quirks = SVC_I3C_QUIRK_FIFO_EMPTY |
+ SVC_I3C_QUIRK_FALSE_SLVSTART |
+ SVC_I3C_QUIRK_DAA_CORRUPT,
+ .vendor_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
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [PATCH v2 1/2] i3c: master: svc: Add amlogic a9 support
2026-11-02 11:05 ` Junyi Zhao via B4 Relay
(?)
@ 2026-11-02 11:18 ` sashiko-bot
-1 siblings, 0 replies; 48+ messages in thread
From: sashiko-bot @ 2026-11-02 11:18 UTC (permalink / raw)
To: Junyi Zhao
Cc: linux-i3c, devicetree, linux-amlogic, imx, robh, conor+dt,
Frank.Li, neil.armstrong, Alexandre Belloni
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] i3c: master: svc: NULL dereference on unchecked devm_kcalloc() in aml_ao_i3c_tail_clocks()
- [High] i3c: master: svc: unclocked MMIO access to AML_AO_I3C_FCLK_DIV_CTRL in aml_i3c_init()
- [Low] i3c: master: svc: missing COMMON_CLK dependency for aml_ao_i3c_tail_clocks()
--
commit 5d539aa0589681cba0b31f37c581a2a552c6d504
Author: Junyi Zhao <junyi.zhao@amlogic.com>
i3c: master: svc: Add amlogic a9 support
This commit adds support for the Silvaco I3C master controller on Amlogic
a9 SoCs (both EE-domain and AO-domain variants). It introduces a
vendor_init callback and vendor_quirks to register the AO-domain internal
clock divider chain and configure the fast_clk rate during probe.
> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index e700392f0801f..98aa6eb0f49ca 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>
[ ... ]
> @@ -2019,6 +2043,12 @@ 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");
>
> + if (master->drvdata->vendor_init) {
> + ret = master->drvdata->vendor_init(master);
> + if (ret)
> + return ret;
> + }
> +
> for (i = 0; i < master->num_clks; i++) {
> if (!strcmp(master->clks[i].id, "fast_clk"))
> break;
> @@ -2036,7 +2066,6 @@ static int svc_i3c_master_probe(struct platform_device *pdev)
> 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");
> @@ -2163,9 +2192,184 @@ static const struct svc_i3c_drvdata npcm845_drvdata = {
>
> static const struct svc_i3c_drvdata svc_default_drvdata = {};
>
> +/*
> + * Container for the tail clocks of the AO domain variant. 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 internal divider chain that
> + * produces the AO variant's fast_clk, and append it to the clk bulk
> + * array as "fast_clk". @parent must be "pclk", the only clock the AO
> + * variant exposes (see silvaco,i3c-master.yaml).
> + */
> +static int aml_ao_i3c_tail_clocks(struct svc_i3c_master *master, struct clk *parent)
> +{
> + struct device *dev = master->dev;
> + struct aml_i3c_ao_clk *ao;
> + struct clk_init_data init = {};
> + char name[64];
> + int ret;
> + struct clk_bulk_data *new_clks;
> + struct clk *fclk;
> +
> + 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");
[Severity: Low]
Will this fail to link when the common clock framework is disabled?
aml_ao_i3c_tail_clocks() unconditionally references clk_gate_ops,
clk_divider_ops, devm_clk_hw_register(), and devm_clk_hw_get_clk(), which do
not have stub definitions in include/linux/clk-provider.h when common clock
support is not enabled.
In drivers/i3c/master/Kconfig, the driver currently has:
config SVC_I3C_MASTER
tristate "Silvaco I3C Dual-Role Master driver"
depends on HAS_IOMEM
depends on !(ALPHA || PARISC)
Should the Kconfig entry also depend on common clock support?
> +
> + new_clks = devm_kcalloc(dev,
> + master->num_clks + 1,
> + sizeof(*new_clks),
> + GFP_KERNEL);
> + 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;
[Severity: High]
What happens if devm_kcalloc() returns null in aml_ao_i3c_tail_clocks()?
new_clks is passed directly to memcpy() and dereferenced at
new_clks[master->num_clks] without checking whether the allocation succeeded.
Should this return -ENOMEM if !new_clks?
> + master->clks = new_clks;
> + master->num_clks++;
> +
> + return 0;
> +}
> +
> +static int aml_i3c_init(struct svc_i3c_master *master)
> +{
> + struct device *dev = master->dev;
> + long fin_freq;
> + int ret, i;
> + struct clk *fclk;
> +
> + if (vendor_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.
> + */
> + 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, 25000000);
> + if (fin_freq <= 0) {
> + dev_err(dev, "invalid fast_clk\n");
> + return fin_freq ? fin_freq : -EINVAL;
> + }
> +
> + ret = clk_set_rate(fclk, fin_freq);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "setting fast_clk rate failed\n");
[Severity: High]
On the AO variant, is it safe to access master->regs + AML_AO_I3C_FCLK_DIV_CTRL
before pclk is prepared and enabled?
In svc_i3c_master_probe(), master->drvdata->vendor_init(master) is called right
after devm_clk_bulk_get_all(), before clk_bulk_prepare_enable() enables pclk
and before pm_runtime_enable() is called:
svc_i3c_master_probe()
devm_clk_bulk_get_all()
aml_i3c_init()
aml_ao_i3c_tail_clocks()
devm_clk_hw_register(&ao->div.hw)
__clk_core_init()
clk_divider_recalc_rate()
clk_set_rate(fclk, fin_freq)
clk_divider_set_rate()
...
clk_bulk_prepare_enable()
...
pm_runtime_enable()
Both devm_clk_hw_register(&ao->div.hw) and clk_set_rate(fclk, fin_freq) read
and write master->regs + AML_AO_I3C_FCLK_DIV_CTRL while pclk is still
disabled, and none of the registered AO clocks set CLK_OPS_PARENT_ENABLE.
In addition, because devm_clk_hw_register() runs before pm_runtime_enable(),
runtime PM is not bound to the registered clocks, so reading debugfs
clk_summary while the controller is runtime-suspended (and pclk is gated by
svc_i3c_runtime_suspend()) will also read master->regs +
AML_AO_I3C_FCLK_DIV_CTRL with pclk disabled.
Should pclk be enabled before registering and programming the internal divider
clocks (or should the AO clocks use CLK_OPS_PARENT_ENABLE / runtime PM)?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-a9_i3c-v2-0-0c474bfccffa@amlogic.com?part=1
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v2 1/2] i3c: master: svc: Add amlogic a9 support
@ 2026-11-02 11:18 ` sashiko-bot
0 siblings, 0 replies; 48+ messages in thread
From: sashiko-bot @ 2026-11-02 11:18 UTC (permalink / raw)
To: Junyi Zhao
Cc: linux-i3c, devicetree, linux-amlogic, imx, robh, conor+dt,
Frank.Li, neil.armstrong, Alexandre Belloni
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] i3c: master: svc: NULL dereference on unchecked devm_kcalloc() in aml_ao_i3c_tail_clocks()
- [High] i3c: master: svc: unclocked MMIO access to AML_AO_I3C_FCLK_DIV_CTRL in aml_i3c_init()
- [Low] i3c: master: svc: missing COMMON_CLK dependency for aml_ao_i3c_tail_clocks()
--
commit 5d539aa0589681cba0b31f37c581a2a552c6d504
Author: Junyi Zhao <junyi.zhao@amlogic.com>
i3c: master: svc: Add amlogic a9 support
This commit adds support for the Silvaco I3C master controller on Amlogic
a9 SoCs (both EE-domain and AO-domain variants). It introduces a
vendor_init callback and vendor_quirks to register the AO-domain internal
clock divider chain and configure the fast_clk rate during probe.
> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index e700392f0801f..98aa6eb0f49ca 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>
[ ... ]
> @@ -2019,6 +2043,12 @@ 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");
>
> + if (master->drvdata->vendor_init) {
> + ret = master->drvdata->vendor_init(master);
> + if (ret)
> + return ret;
> + }
> +
> for (i = 0; i < master->num_clks; i++) {
> if (!strcmp(master->clks[i].id, "fast_clk"))
> break;
> @@ -2036,7 +2066,6 @@ static int svc_i3c_master_probe(struct platform_device *pdev)
> 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");
> @@ -2163,9 +2192,184 @@ static const struct svc_i3c_drvdata npcm845_drvdata = {
>
> static const struct svc_i3c_drvdata svc_default_drvdata = {};
>
> +/*
> + * Container for the tail clocks of the AO domain variant. 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 internal divider chain that
> + * produces the AO variant's fast_clk, and append it to the clk bulk
> + * array as "fast_clk". @parent must be "pclk", the only clock the AO
> + * variant exposes (see silvaco,i3c-master.yaml).
> + */
> +static int aml_ao_i3c_tail_clocks(struct svc_i3c_master *master, struct clk *parent)
> +{
> + struct device *dev = master->dev;
> + struct aml_i3c_ao_clk *ao;
> + struct clk_init_data init = {};
> + char name[64];
> + int ret;
> + struct clk_bulk_data *new_clks;
> + struct clk *fclk;
> +
> + 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");
[Severity: Low]
Will this fail to link when the common clock framework is disabled?
aml_ao_i3c_tail_clocks() unconditionally references clk_gate_ops,
clk_divider_ops, devm_clk_hw_register(), and devm_clk_hw_get_clk(), which do
not have stub definitions in include/linux/clk-provider.h when common clock
support is not enabled.
In drivers/i3c/master/Kconfig, the driver currently has:
config SVC_I3C_MASTER
tristate "Silvaco I3C Dual-Role Master driver"
depends on HAS_IOMEM
depends on !(ALPHA || PARISC)
Should the Kconfig entry also depend on common clock support?
> +
> + new_clks = devm_kcalloc(dev,
> + master->num_clks + 1,
> + sizeof(*new_clks),
> + GFP_KERNEL);
> + 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;
[Severity: High]
What happens if devm_kcalloc() returns null in aml_ao_i3c_tail_clocks()?
new_clks is passed directly to memcpy() and dereferenced at
new_clks[master->num_clks] without checking whether the allocation succeeded.
Should this return -ENOMEM if !new_clks?
> + master->clks = new_clks;
> + master->num_clks++;
> +
> + return 0;
> +}
> +
> +static int aml_i3c_init(struct svc_i3c_master *master)
> +{
> + struct device *dev = master->dev;
> + long fin_freq;
> + int ret, i;
> + struct clk *fclk;
> +
> + if (vendor_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.
> + */
> + 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, 25000000);
> + if (fin_freq <= 0) {
> + dev_err(dev, "invalid fast_clk\n");
> + return fin_freq ? fin_freq : -EINVAL;
> + }
> +
> + ret = clk_set_rate(fclk, fin_freq);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "setting fast_clk rate failed\n");
[Severity: High]
On the AO variant, is it safe to access master->regs + AML_AO_I3C_FCLK_DIV_CTRL
before pclk is prepared and enabled?
In svc_i3c_master_probe(), master->drvdata->vendor_init(master) is called right
after devm_clk_bulk_get_all(), before clk_bulk_prepare_enable() enables pclk
and before pm_runtime_enable() is called:
svc_i3c_master_probe()
devm_clk_bulk_get_all()
aml_i3c_init()
aml_ao_i3c_tail_clocks()
devm_clk_hw_register(&ao->div.hw)
__clk_core_init()
clk_divider_recalc_rate()
clk_set_rate(fclk, fin_freq)
clk_divider_set_rate()
...
clk_bulk_prepare_enable()
...
pm_runtime_enable()
Both devm_clk_hw_register(&ao->div.hw) and clk_set_rate(fclk, fin_freq) read
and write master->regs + AML_AO_I3C_FCLK_DIV_CTRL while pclk is still
disabled, and none of the registered AO clocks set CLK_OPS_PARENT_ENABLE.
In addition, because devm_clk_hw_register() runs before pm_runtime_enable(),
runtime PM is not bound to the registered clocks, so reading debugfs
clk_summary while the controller is runtime-suspended (and pclk is gated by
svc_i3c_runtime_suspend()) will also read master->regs +
AML_AO_I3C_FCLK_DIV_CTRL with pclk disabled.
Should pclk be enabled before registering and programming the internal divider
clocks (or should the AO clocks use CLK_OPS_PARENT_ENABLE / runtime PM)?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-a9_i3c-v2-0-0c474bfccffa@amlogic.com?part=1
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v2 1/2] i3c: master: svc: Add amlogic a9 support
@ 2026-11-02 11:18 ` sashiko-bot
0 siblings, 0 replies; 48+ messages in thread
From: sashiko-bot @ 2026-11-02 11:18 UTC (permalink / raw)
To: Junyi Zhao
Cc: linux-i3c, devicetree, linux-amlogic, imx, robh, conor+dt,
Frank.Li, neil.armstrong, Alexandre Belloni
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] i3c: master: svc: NULL dereference on unchecked devm_kcalloc() in aml_ao_i3c_tail_clocks()
- [High] i3c: master: svc: unclocked MMIO access to AML_AO_I3C_FCLK_DIV_CTRL in aml_i3c_init()
- [Low] i3c: master: svc: missing COMMON_CLK dependency for aml_ao_i3c_tail_clocks()
--
commit 5d539aa0589681cba0b31f37c581a2a552c6d504
Author: Junyi Zhao <junyi.zhao@amlogic.com>
i3c: master: svc: Add amlogic a9 support
This commit adds support for the Silvaco I3C master controller on Amlogic
a9 SoCs (both EE-domain and AO-domain variants). It introduces a
vendor_init callback and vendor_quirks to register the AO-domain internal
clock divider chain and configure the fast_clk rate during probe.
> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index e700392f0801f..98aa6eb0f49ca 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>
[ ... ]
> @@ -2019,6 +2043,12 @@ 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");
>
> + if (master->drvdata->vendor_init) {
> + ret = master->drvdata->vendor_init(master);
> + if (ret)
> + return ret;
> + }
> +
> for (i = 0; i < master->num_clks; i++) {
> if (!strcmp(master->clks[i].id, "fast_clk"))
> break;
> @@ -2036,7 +2066,6 @@ static int svc_i3c_master_probe(struct platform_device *pdev)
> 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");
> @@ -2163,9 +2192,184 @@ static const struct svc_i3c_drvdata npcm845_drvdata = {
>
> static const struct svc_i3c_drvdata svc_default_drvdata = {};
>
> +/*
> + * Container for the tail clocks of the AO domain variant. 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 internal divider chain that
> + * produces the AO variant's fast_clk, and append it to the clk bulk
> + * array as "fast_clk". @parent must be "pclk", the only clock the AO
> + * variant exposes (see silvaco,i3c-master.yaml).
> + */
> +static int aml_ao_i3c_tail_clocks(struct svc_i3c_master *master, struct clk *parent)
> +{
> + struct device *dev = master->dev;
> + struct aml_i3c_ao_clk *ao;
> + struct clk_init_data init = {};
> + char name[64];
> + int ret;
> + struct clk_bulk_data *new_clks;
> + struct clk *fclk;
> +
> + 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");
[Severity: Low]
Will this fail to link when the common clock framework is disabled?
aml_ao_i3c_tail_clocks() unconditionally references clk_gate_ops,
clk_divider_ops, devm_clk_hw_register(), and devm_clk_hw_get_clk(), which do
not have stub definitions in include/linux/clk-provider.h when common clock
support is not enabled.
In drivers/i3c/master/Kconfig, the driver currently has:
config SVC_I3C_MASTER
tristate "Silvaco I3C Dual-Role Master driver"
depends on HAS_IOMEM
depends on !(ALPHA || PARISC)
Should the Kconfig entry also depend on common clock support?
> +
> + new_clks = devm_kcalloc(dev,
> + master->num_clks + 1,
> + sizeof(*new_clks),
> + GFP_KERNEL);
> + 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;
[Severity: High]
What happens if devm_kcalloc() returns null in aml_ao_i3c_tail_clocks()?
new_clks is passed directly to memcpy() and dereferenced at
new_clks[master->num_clks] without checking whether the allocation succeeded.
Should this return -ENOMEM if !new_clks?
> + master->clks = new_clks;
> + master->num_clks++;
> +
> + return 0;
> +}
> +
> +static int aml_i3c_init(struct svc_i3c_master *master)
> +{
> + struct device *dev = master->dev;
> + long fin_freq;
> + int ret, i;
> + struct clk *fclk;
> +
> + if (vendor_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.
> + */
> + 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, 25000000);
> + if (fin_freq <= 0) {
> + dev_err(dev, "invalid fast_clk\n");
> + return fin_freq ? fin_freq : -EINVAL;
> + }
> +
> + ret = clk_set_rate(fclk, fin_freq);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "setting fast_clk rate failed\n");
[Severity: High]
On the AO variant, is it safe to access master->regs + AML_AO_I3C_FCLK_DIV_CTRL
before pclk is prepared and enabled?
In svc_i3c_master_probe(), master->drvdata->vendor_init(master) is called right
after devm_clk_bulk_get_all(), before clk_bulk_prepare_enable() enables pclk
and before pm_runtime_enable() is called:
svc_i3c_master_probe()
devm_clk_bulk_get_all()
aml_i3c_init()
aml_ao_i3c_tail_clocks()
devm_clk_hw_register(&ao->div.hw)
__clk_core_init()
clk_divider_recalc_rate()
clk_set_rate(fclk, fin_freq)
clk_divider_set_rate()
...
clk_bulk_prepare_enable()
...
pm_runtime_enable()
Both devm_clk_hw_register(&ao->div.hw) and clk_set_rate(fclk, fin_freq) read
and write master->regs + AML_AO_I3C_FCLK_DIV_CTRL while pclk is still
disabled, and none of the registered AO clocks set CLK_OPS_PARENT_ENABLE.
In addition, because devm_clk_hw_register() runs before pm_runtime_enable(),
runtime PM is not bound to the registered clocks, so reading debugfs
clk_summary while the controller is runtime-suspended (and pclk is gated by
svc_i3c_runtime_suspend()) will also read master->regs +
AML_AO_I3C_FCLK_DIV_CTRL with pclk disabled.
Should pclk be enabled before registering and programming the internal divider
clocks (or should the AO clocks use CLK_OPS_PARENT_ENABLE / runtime PM)?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-a9_i3c-v2-0-0c474bfccffa@amlogic.com?part=1
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v2 1/2] i3c: master: svc: Add amlogic a9 support
2026-11-02 11:05 ` Junyi Zhao via B4 Relay
(?)
@ 2026-11-02 22:10 ` Frank Li
-1 siblings, 0 replies; 48+ messages in thread
From: Frank Li @ 2026-11-02 22:10 UTC (permalink / raw)
To: Junyi Zhao
Cc: Alexandre Belloni, Frank Li, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Neil Armstrong, Kevin Hilman, Jerome Brunet,
Martin Blumenstingl, Miquel Raynal, Kees Cook,
Gustavo A. R. Silva, linux-i3c, devicetree, linux-kernel,
linux-arm-kernel, linux-amlogic, imx, linux-hardening,
xianwei.zhao, Kelvin.Zhang
On Mon, Nov 02, 2026 at 07:05:39PM +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.
>
> Add a vendor_init callback to handle this clock setup.
> Add a vendor_quirks field to describe AO-specific behavior.
>
> Signed-off-by: Junyi Zhao <junyi.zhao@amlogic.com>
> ---
> drivers/i3c/master/svc-i3c-master.c | 214 +++++++++++++++++++++++++++++++++++-
> 1 file changed, 209 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index e700392f0..98aa6eb0f 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,21 @@
> */
> #define SVC_I3C_QUIRK_DAA_CORRUPT BIT(2)
>
> +/* Amlogic specific registers */
> +#define AML_AO_I3C_FCLK_DIV_CTRL 0x304
> +#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
> +
> +/*
> + * 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 own register space.
> + */
> +#define AML_I3C_QUIRK_AO BIT(0)
> +
There are not much quick now, just put after SVC_I3C_QUIRK_DAA_CORRUPT
#define AML_I3C_QUIRK_AO BIT(3)
use a unified quirk define.
> struct svc_i3c_cmd {
> u8 addr;
> union {
> @@ -195,10 +211,6 @@ struct svc_i3c_regs_save {
> u32 mdynaddr;
> };
>
> -struct svc_i3c_drvdata {
> - u32 quirks;
> -};
> -
> /**
> * struct svc_i3c_master - Silvaco I3C Master structure
> * @base: I3C master controller
> @@ -257,6 +269,12 @@ struct svc_i3c_master {
> u32 mctrl_config;
> };
>
> +struct svc_i3c_drvdata {
> + u32 quirks;
> + u32 vendor_quirks;
Needn't this, direct use quirks
> + int (*vendor_init)(struct svc_i3c_master *master);
> +};
> +
why need move this struct
> /**
> * struct svc_i3c_i2c_dev_data - Device specific data
> * @index: Index in the master tables corresponding to this device
> @@ -274,6 +292,11 @@ static inline bool svc_has_quirk(struct svc_i3c_master *master, u32 quirk)
> return (master->drvdata->quirks & quirk);
> }
>
> +static inline bool vendor_has_quirk(struct svc_i3c_master *master, u32 quirk)
> +{
> + return (master->drvdata->vendor_quirks & quirk);
> +}
> +
Needn't this one
> static inline bool svc_has_daa_corrupt(struct svc_i3c_master *master)
> {
> return ((master->drvdata->quirks & SVC_I3C_QUIRK_DAA_CORRUPT) &&
> @@ -2011,6 +2034,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,6 +2043,12 @@ 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");
>
> + if (master->drvdata->vendor_init) {
> + ret = master->drvdata->vendor_init(master);
> + if (ret)
> + return ret;
> + }
> +
> for (i = 0; i < master->num_clks; i++) {
> if (!strcmp(master->clks[i].id, "fast_clk"))
> break;
> @@ -2036,7 +2066,6 @@ static int svc_i3c_master_probe(struct platform_device *pdev)
> 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");
> @@ -2163,9 +2192,184 @@ static const struct svc_i3c_drvdata npcm845_drvdata = {
>
> static const struct svc_i3c_drvdata svc_default_drvdata = {};
>
> +/*
> + * Container for the tail clocks of the AO domain variant. 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 internal divider chain that
> + * produces the AO variant's fast_clk, and append it to the clk bulk
> + * array as "fast_clk". @parent must be "pclk", the only clock the AO
> + * variant exposes (see silvaco,i3c-master.yaml).
> + */
use kernel doc format.
> +static int aml_ao_i3c_tail_clocks(struct svc_i3c_master *master, struct clk *parent)
> +{
> + struct device *dev = master->dev;
> + struct aml_i3c_ao_clk *ao;
> + struct clk_init_data init = {};
> + char name[64];
> + int ret;
> + struct clk_bulk_data *new_clks;
> + struct clk *fclk;
keep revise Chemistry order
Frank
> +
> + 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);
> + 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++;
> +
> + return 0;
> +}
> +
> +static int aml_i3c_init(struct svc_i3c_master *master)
> +{
> + struct device *dev = master->dev;
> + long fin_freq;
> + int ret, i;
> + struct clk *fclk;
> +
> + if (vendor_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.
> + */
> + 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, 25000000);
> + if (fin_freq <= 0) {
> + dev_err(dev, "invalid fast_clk\n");
> + return fin_freq ? fin_freq : -EINVAL;
> + }
> +
> + 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 = SVC_I3C_QUIRK_FIFO_EMPTY |
> + SVC_I3C_QUIRK_FALSE_SLVSTART |
> + SVC_I3C_QUIRK_DAA_CORRUPT,
> + .vendor_init = aml_i3c_init,
> +};
> +
> +static const struct svc_i3c_drvdata aml_a9_ao_drvdata = {
> + .quirks = SVC_I3C_QUIRK_FIFO_EMPTY |
> + SVC_I3C_QUIRK_FALSE_SLVSTART |
> + SVC_I3C_QUIRK_DAA_CORRUPT,
> + .vendor_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
>
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v2 1/2] i3c: master: svc: Add amlogic a9 support
@ 2026-11-02 22:10 ` Frank Li
0 siblings, 0 replies; 48+ messages in thread
From: Frank Li @ 2026-11-02 22:10 UTC (permalink / raw)
To: Junyi Zhao
Cc: Alexandre Belloni, Frank Li, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Neil Armstrong, Kevin Hilman, Jerome Brunet,
Martin Blumenstingl, Miquel Raynal, Kees Cook,
Gustavo A. R. Silva, linux-i3c, devicetree, linux-kernel,
linux-arm-kernel, linux-amlogic, imx, linux-hardening,
xianwei.zhao, Kelvin.Zhang
On Mon, Nov 02, 2026 at 07:05:39PM +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.
>
> Add a vendor_init callback to handle this clock setup.
> Add a vendor_quirks field to describe AO-specific behavior.
>
> Signed-off-by: Junyi Zhao <junyi.zhao@amlogic.com>
> ---
> drivers/i3c/master/svc-i3c-master.c | 214 +++++++++++++++++++++++++++++++++++-
> 1 file changed, 209 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index e700392f0..98aa6eb0f 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,21 @@
> */
> #define SVC_I3C_QUIRK_DAA_CORRUPT BIT(2)
>
> +/* Amlogic specific registers */
> +#define AML_AO_I3C_FCLK_DIV_CTRL 0x304
> +#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
> +
> +/*
> + * 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 own register space.
> + */
> +#define AML_I3C_QUIRK_AO BIT(0)
> +
There are not much quick now, just put after SVC_I3C_QUIRK_DAA_CORRUPT
#define AML_I3C_QUIRK_AO BIT(3)
use a unified quirk define.
> struct svc_i3c_cmd {
> u8 addr;
> union {
> @@ -195,10 +211,6 @@ struct svc_i3c_regs_save {
> u32 mdynaddr;
> };
>
> -struct svc_i3c_drvdata {
> - u32 quirks;
> -};
> -
> /**
> * struct svc_i3c_master - Silvaco I3C Master structure
> * @base: I3C master controller
> @@ -257,6 +269,12 @@ struct svc_i3c_master {
> u32 mctrl_config;
> };
>
> +struct svc_i3c_drvdata {
> + u32 quirks;
> + u32 vendor_quirks;
Needn't this, direct use quirks
> + int (*vendor_init)(struct svc_i3c_master *master);
> +};
> +
why need move this struct
> /**
> * struct svc_i3c_i2c_dev_data - Device specific data
> * @index: Index in the master tables corresponding to this device
> @@ -274,6 +292,11 @@ static inline bool svc_has_quirk(struct svc_i3c_master *master, u32 quirk)
> return (master->drvdata->quirks & quirk);
> }
>
> +static inline bool vendor_has_quirk(struct svc_i3c_master *master, u32 quirk)
> +{
> + return (master->drvdata->vendor_quirks & quirk);
> +}
> +
Needn't this one
> static inline bool svc_has_daa_corrupt(struct svc_i3c_master *master)
> {
> return ((master->drvdata->quirks & SVC_I3C_QUIRK_DAA_CORRUPT) &&
> @@ -2011,6 +2034,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,6 +2043,12 @@ 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");
>
> + if (master->drvdata->vendor_init) {
> + ret = master->drvdata->vendor_init(master);
> + if (ret)
> + return ret;
> + }
> +
> for (i = 0; i < master->num_clks; i++) {
> if (!strcmp(master->clks[i].id, "fast_clk"))
> break;
> @@ -2036,7 +2066,6 @@ static int svc_i3c_master_probe(struct platform_device *pdev)
> 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");
> @@ -2163,9 +2192,184 @@ static const struct svc_i3c_drvdata npcm845_drvdata = {
>
> static const struct svc_i3c_drvdata svc_default_drvdata = {};
>
> +/*
> + * Container for the tail clocks of the AO domain variant. 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 internal divider chain that
> + * produces the AO variant's fast_clk, and append it to the clk bulk
> + * array as "fast_clk". @parent must be "pclk", the only clock the AO
> + * variant exposes (see silvaco,i3c-master.yaml).
> + */
use kernel doc format.
> +static int aml_ao_i3c_tail_clocks(struct svc_i3c_master *master, struct clk *parent)
> +{
> + struct device *dev = master->dev;
> + struct aml_i3c_ao_clk *ao;
> + struct clk_init_data init = {};
> + char name[64];
> + int ret;
> + struct clk_bulk_data *new_clks;
> + struct clk *fclk;
keep revise Chemistry order
Frank
> +
> + 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);
> + 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++;
> +
> + return 0;
> +}
> +
> +static int aml_i3c_init(struct svc_i3c_master *master)
> +{
> + struct device *dev = master->dev;
> + long fin_freq;
> + int ret, i;
> + struct clk *fclk;
> +
> + if (vendor_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.
> + */
> + 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, 25000000);
> + if (fin_freq <= 0) {
> + dev_err(dev, "invalid fast_clk\n");
> + return fin_freq ? fin_freq : -EINVAL;
> + }
> +
> + 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 = SVC_I3C_QUIRK_FIFO_EMPTY |
> + SVC_I3C_QUIRK_FALSE_SLVSTART |
> + SVC_I3C_QUIRK_DAA_CORRUPT,
> + .vendor_init = aml_i3c_init,
> +};
> +
> +static const struct svc_i3c_drvdata aml_a9_ao_drvdata = {
> + .quirks = SVC_I3C_QUIRK_FIFO_EMPTY |
> + SVC_I3C_QUIRK_FALSE_SLVSTART |
> + SVC_I3C_QUIRK_DAA_CORRUPT,
> + .vendor_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
>
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v2 1/2] i3c: master: svc: Add amlogic a9 support
@ 2026-11-02 22:10 ` Frank Li
0 siblings, 0 replies; 48+ messages in thread
From: Frank Li @ 2026-11-02 22:10 UTC (permalink / raw)
To: Junyi Zhao
Cc: Alexandre Belloni, Frank Li, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Neil Armstrong, Kevin Hilman, Jerome Brunet,
Martin Blumenstingl, Miquel Raynal, Kees Cook,
Gustavo A. R. Silva, linux-i3c, devicetree, linux-kernel,
linux-arm-kernel, linux-amlogic, imx, linux-hardening,
xianwei.zhao, Kelvin.Zhang
On Mon, Nov 02, 2026 at 07:05:39PM +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.
>
> Add a vendor_init callback to handle this clock setup.
> Add a vendor_quirks field to describe AO-specific behavior.
>
> Signed-off-by: Junyi Zhao <junyi.zhao@amlogic.com>
> ---
> drivers/i3c/master/svc-i3c-master.c | 214 +++++++++++++++++++++++++++++++++++-
> 1 file changed, 209 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index e700392f0..98aa6eb0f 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,21 @@
> */
> #define SVC_I3C_QUIRK_DAA_CORRUPT BIT(2)
>
> +/* Amlogic specific registers */
> +#define AML_AO_I3C_FCLK_DIV_CTRL 0x304
> +#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
> +
> +/*
> + * 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 own register space.
> + */
> +#define AML_I3C_QUIRK_AO BIT(0)
> +
There are not much quick now, just put after SVC_I3C_QUIRK_DAA_CORRUPT
#define AML_I3C_QUIRK_AO BIT(3)
use a unified quirk define.
> struct svc_i3c_cmd {
> u8 addr;
> union {
> @@ -195,10 +211,6 @@ struct svc_i3c_regs_save {
> u32 mdynaddr;
> };
>
> -struct svc_i3c_drvdata {
> - u32 quirks;
> -};
> -
> /**
> * struct svc_i3c_master - Silvaco I3C Master structure
> * @base: I3C master controller
> @@ -257,6 +269,12 @@ struct svc_i3c_master {
> u32 mctrl_config;
> };
>
> +struct svc_i3c_drvdata {
> + u32 quirks;
> + u32 vendor_quirks;
Needn't this, direct use quirks
> + int (*vendor_init)(struct svc_i3c_master *master);
> +};
> +
why need move this struct
> /**
> * struct svc_i3c_i2c_dev_data - Device specific data
> * @index: Index in the master tables corresponding to this device
> @@ -274,6 +292,11 @@ static inline bool svc_has_quirk(struct svc_i3c_master *master, u32 quirk)
> return (master->drvdata->quirks & quirk);
> }
>
> +static inline bool vendor_has_quirk(struct svc_i3c_master *master, u32 quirk)
> +{
> + return (master->drvdata->vendor_quirks & quirk);
> +}
> +
Needn't this one
> static inline bool svc_has_daa_corrupt(struct svc_i3c_master *master)
> {
> return ((master->drvdata->quirks & SVC_I3C_QUIRK_DAA_CORRUPT) &&
> @@ -2011,6 +2034,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,6 +2043,12 @@ 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");
>
> + if (master->drvdata->vendor_init) {
> + ret = master->drvdata->vendor_init(master);
> + if (ret)
> + return ret;
> + }
> +
> for (i = 0; i < master->num_clks; i++) {
> if (!strcmp(master->clks[i].id, "fast_clk"))
> break;
> @@ -2036,7 +2066,6 @@ static int svc_i3c_master_probe(struct platform_device *pdev)
> 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");
> @@ -2163,9 +2192,184 @@ static const struct svc_i3c_drvdata npcm845_drvdata = {
>
> static const struct svc_i3c_drvdata svc_default_drvdata = {};
>
> +/*
> + * Container for the tail clocks of the AO domain variant. 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 internal divider chain that
> + * produces the AO variant's fast_clk, and append it to the clk bulk
> + * array as "fast_clk". @parent must be "pclk", the only clock the AO
> + * variant exposes (see silvaco,i3c-master.yaml).
> + */
use kernel doc format.
> +static int aml_ao_i3c_tail_clocks(struct svc_i3c_master *master, struct clk *parent)
> +{
> + struct device *dev = master->dev;
> + struct aml_i3c_ao_clk *ao;
> + struct clk_init_data init = {};
> + char name[64];
> + int ret;
> + struct clk_bulk_data *new_clks;
> + struct clk *fclk;
keep revise Chemistry order
Frank
> +
> + 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);
> + 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++;
> +
> + return 0;
> +}
> +
> +static int aml_i3c_init(struct svc_i3c_master *master)
> +{
> + struct device *dev = master->dev;
> + long fin_freq;
> + int ret, i;
> + struct clk *fclk;
> +
> + if (vendor_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.
> + */
> + 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, 25000000);
> + if (fin_freq <= 0) {
> + dev_err(dev, "invalid fast_clk\n");
> + return fin_freq ? fin_freq : -EINVAL;
> + }
> +
> + 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 = SVC_I3C_QUIRK_FIFO_EMPTY |
> + SVC_I3C_QUIRK_FALSE_SLVSTART |
> + SVC_I3C_QUIRK_DAA_CORRUPT,
> + .vendor_init = aml_i3c_init,
> +};
> +
> +static const struct svc_i3c_drvdata aml_a9_ao_drvdata = {
> + .quirks = SVC_I3C_QUIRK_FIFO_EMPTY |
> + SVC_I3C_QUIRK_FALSE_SLVSTART |
> + SVC_I3C_QUIRK_DAA_CORRUPT,
> + .vendor_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
>
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v2 1/2] i3c: master: svc: Add amlogic a9 support
2026-11-02 22:10 ` Frank Li
(?)
@ 2026-11-04 2:44 ` Junyi Zhao
-1 siblings, 0 replies; 48+ messages in thread
From: Junyi Zhao @ 2026-11-04 2:44 UTC (permalink / raw)
To: Frank Li
Cc: Alexandre Belloni, Frank Li, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Neil Armstrong, Kevin Hilman, Jerome Brunet,
Martin Blumenstingl, Miquel Raynal, Kees Cook,
Gustavo A. R. Silva, linux-i3c, devicetree, linux-kernel,
linux-arm-kernel, linux-amlogic, imx, linux-hardening,
xianwei.zhao, Kelvin.Zhang
Hi Frank,
Thanks for the review.
On 03/11/2026 06:10, Frank Li wrote:
> [ EXTERNAL EMAIL ]
>
> On Mon, Nov 02, 2026 at 07:05:39PM +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.
>>
>> Add a vendor_init callback to handle this clock setup.
>> Add a vendor_quirks field to describe AO-specific behavior.
>>
>> Signed-off-by: Junyi Zhao <junyi.zhao@amlogic.com>
>> ---
>> drivers/i3c/master/svc-i3c-master.c | 214 +++++++++++++++++++++++++++++++++++-
>> 1 file changed, 209 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
>> index e700392f0..98aa6eb0f 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,21 @@
>> */
>> #define SVC_I3C_QUIRK_DAA_CORRUPT BIT(2)
>>
>> +/* Amlogic specific registers */
>> +#define AML_AO_I3C_FCLK_DIV_CTRL 0x304
>> +#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
>> +
>> +/*
>> + * 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 own register space.
>> + */
>> +#define AML_I3C_QUIRK_AO BIT(0)
>> +
>
> There are not much quick now, just put after SVC_I3C_QUIRK_DAA_CORRUPT
>
> #define AML_I3C_QUIRK_AO BIT(3)
>
> use a unified quirk define.
will do.
>
>> struct svc_i3c_cmd {
>> u8 addr;
>> union {
>> @@ -195,10 +211,6 @@ struct svc_i3c_regs_save {
>> u32 mdynaddr;
>> };
>>
>> -struct svc_i3c_drvdata {
>> - u32 quirks;
>> -};
>> -
>> /**
>> * struct svc_i3c_master - Silvaco I3C Master structure
>> * @base: I3C master controller
>> @@ -257,6 +269,12 @@ struct svc_i3c_master {
>> u32 mctrl_config;
>> };
>>
>> +struct svc_i3c_drvdata {
>> + u32 quirks;
>> + u32 vendor_quirks;
>
> Needn't this, direct use quirks
will do.
>> + int (*vendor_init)(struct svc_i3c_master *master);
>> +};
>> +
>
> why need move this struct
because callback need param struct svc_i3c_master *master， and if not，
compile error. undefined.
>
>> /**
>> * struct svc_i3c_i2c_dev_data - Device specific data
>> * @index: Index in the master tables corresponding to this device
>> @@ -274,6 +292,11 @@ static inline bool svc_has_quirk(struct svc_i3c_master *master, u32 quirk)
>> return (master->drvdata->quirks & quirk);
>> }
>>
>> +static inline bool vendor_has_quirk(struct svc_i3c_master *master, u32 quirk)
>> +{
>> + return (master->drvdata->vendor_quirks & quirk);
>> +}
>> +
>
> Needn't this one
will delete it.
>> static inline bool svc_has_daa_corrupt(struct svc_i3c_master *master)
>> {
>> return ((master->drvdata->quirks & SVC_I3C_QUIRK_DAA_CORRUPT) &&
>> @@ -2011,6 +2034,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,6 +2043,12 @@ 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");
>>
>> + if (master->drvdata->vendor_init) {
>> + ret = master->drvdata->vendor_init(master);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> for (i = 0; i < master->num_clks; i++) {
>> if (!strcmp(master->clks[i].id, "fast_clk"))
>> break;
>> @@ -2036,7 +2066,6 @@ static int svc_i3c_master_probe(struct platform_device *pdev)
>> 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");
>> @@ -2163,9 +2192,184 @@ static const struct svc_i3c_drvdata npcm845_drvdata = {
>>
>> static const struct svc_i3c_drvdata svc_default_drvdata = {};
>>
>> +/*
>> + * Container for the tail clocks of the AO domain variant. 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 internal divider chain that
>> + * produces the AO variant's fast_clk, and append it to the clk bulk
>> + * array as "fast_clk". @parent must be "pclk", the only clock the AO
>> + * variant exposes (see silvaco,i3c-master.yaml).
>> + */
>
> use kernel doc format.
will change.
>> +static int aml_ao_i3c_tail_clocks(struct svc_i3c_master *master, struct clk *parent)
>> +{
>> + struct device *dev = master->dev;
>> + struct aml_i3c_ao_clk *ao;
>> + struct clk_init_data init = {};
>> + char name[64];
>> + int ret;
>> + struct clk_bulk_data *new_clks;
>> + struct clk *fclk;
>
> keep revise Chemistry order
>
will change order：
struct aml_i3c_ao_clk *ao;
struct clk *fclk;
struct clk_bulk_data *new_clks;
struct device *dev = master->dev;
struct clk_init_data init = {};
char name[64];
int ret;
> Frank
>
>> +
>> + 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);
>> + 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++;
>> +
>> + return 0;
>> +}
>> +
>> +static int aml_i3c_init(struct svc_i3c_master *master)
>> +{
>> + struct device *dev = master->dev;
>> + long fin_freq;
>> + int ret, i;
>> + struct clk *fclk;
>> +
>> + if (vendor_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.
>> + */
>> + 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, 25000000);
>> + if (fin_freq <= 0) {
>> + dev_err(dev, "invalid fast_clk\n");
>> + return fin_freq ? fin_freq : -EINVAL;
>> + }
>> +
>> + 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 = SVC_I3C_QUIRK_FIFO_EMPTY |
>> + SVC_I3C_QUIRK_FALSE_SLVSTART |
>> + SVC_I3C_QUIRK_DAA_CORRUPT,
>> + .vendor_init = aml_i3c_init,
>> +};
>> +
>> +static const struct svc_i3c_drvdata aml_a9_ao_drvdata = {
>> + .quirks = SVC_I3C_QUIRK_FIFO_EMPTY |
>> + SVC_I3C_QUIRK_FALSE_SLVSTART |
>> + SVC_I3C_QUIRK_DAA_CORRUPT,
>> + .vendor_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
>>
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v2 1/2] i3c: master: svc: Add amlogic a9 support
@ 2026-11-04 2:44 ` Junyi Zhao
0 siblings, 0 replies; 48+ messages in thread
From: Junyi Zhao @ 2026-11-04 2:44 UTC (permalink / raw)
To: Frank Li
Cc: Alexandre Belloni, Frank Li, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Neil Armstrong, Kevin Hilman, Jerome Brunet,
Martin Blumenstingl, Miquel Raynal, Kees Cook,
Gustavo A. R. Silva, linux-i3c, devicetree, linux-kernel,
linux-arm-kernel, linux-amlogic, imx, linux-hardening,
xianwei.zhao, Kelvin.Zhang
Hi Frank,
Thanks for the review.
On 03/11/2026 06:10, Frank Li wrote:
> [ EXTERNAL EMAIL ]
>
> On Mon, Nov 02, 2026 at 07:05:39PM +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.
>>
>> Add a vendor_init callback to handle this clock setup.
>> Add a vendor_quirks field to describe AO-specific behavior.
>>
>> Signed-off-by: Junyi Zhao <junyi.zhao@amlogic.com>
>> ---
>> drivers/i3c/master/svc-i3c-master.c | 214 +++++++++++++++++++++++++++++++++++-
>> 1 file changed, 209 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
>> index e700392f0..98aa6eb0f 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,21 @@
>> */
>> #define SVC_I3C_QUIRK_DAA_CORRUPT BIT(2)
>>
>> +/* Amlogic specific registers */
>> +#define AML_AO_I3C_FCLK_DIV_CTRL 0x304
>> +#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
>> +
>> +/*
>> + * 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 own register space.
>> + */
>> +#define AML_I3C_QUIRK_AO BIT(0)
>> +
>
> There are not much quick now, just put after SVC_I3C_QUIRK_DAA_CORRUPT
>
> #define AML_I3C_QUIRK_AO BIT(3)
>
> use a unified quirk define.
will do.
>
>> struct svc_i3c_cmd {
>> u8 addr;
>> union {
>> @@ -195,10 +211,6 @@ struct svc_i3c_regs_save {
>> u32 mdynaddr;
>> };
>>
>> -struct svc_i3c_drvdata {
>> - u32 quirks;
>> -};
>> -
>> /**
>> * struct svc_i3c_master - Silvaco I3C Master structure
>> * @base: I3C master controller
>> @@ -257,6 +269,12 @@ struct svc_i3c_master {
>> u32 mctrl_config;
>> };
>>
>> +struct svc_i3c_drvdata {
>> + u32 quirks;
>> + u32 vendor_quirks;
>
> Needn't this, direct use quirks
will do.
>> + int (*vendor_init)(struct svc_i3c_master *master);
>> +};
>> +
>
> why need move this struct
because callback need param struct svc_i3c_master *master， and if not，
compile error. undefined.
>
>> /**
>> * struct svc_i3c_i2c_dev_data - Device specific data
>> * @index: Index in the master tables corresponding to this device
>> @@ -274,6 +292,11 @@ static inline bool svc_has_quirk(struct svc_i3c_master *master, u32 quirk)
>> return (master->drvdata->quirks & quirk);
>> }
>>
>> +static inline bool vendor_has_quirk(struct svc_i3c_master *master, u32 quirk)
>> +{
>> + return (master->drvdata->vendor_quirks & quirk);
>> +}
>> +
>
> Needn't this one
will delete it.
>> static inline bool svc_has_daa_corrupt(struct svc_i3c_master *master)
>> {
>> return ((master->drvdata->quirks & SVC_I3C_QUIRK_DAA_CORRUPT) &&
>> @@ -2011,6 +2034,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,6 +2043,12 @@ 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");
>>
>> + if (master->drvdata->vendor_init) {
>> + ret = master->drvdata->vendor_init(master);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> for (i = 0; i < master->num_clks; i++) {
>> if (!strcmp(master->clks[i].id, "fast_clk"))
>> break;
>> @@ -2036,7 +2066,6 @@ static int svc_i3c_master_probe(struct platform_device *pdev)
>> 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");
>> @@ -2163,9 +2192,184 @@ static const struct svc_i3c_drvdata npcm845_drvdata = {
>>
>> static const struct svc_i3c_drvdata svc_default_drvdata = {};
>>
>> +/*
>> + * Container for the tail clocks of the AO domain variant. 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 internal divider chain that
>> + * produces the AO variant's fast_clk, and append it to the clk bulk
>> + * array as "fast_clk". @parent must be "pclk", the only clock the AO
>> + * variant exposes (see silvaco,i3c-master.yaml).
>> + */
>
> use kernel doc format.
will change.
>> +static int aml_ao_i3c_tail_clocks(struct svc_i3c_master *master, struct clk *parent)
>> +{
>> + struct device *dev = master->dev;
>> + struct aml_i3c_ao_clk *ao;
>> + struct clk_init_data init = {};
>> + char name[64];
>> + int ret;
>> + struct clk_bulk_data *new_clks;
>> + struct clk *fclk;
>
> keep revise Chemistry order
>
will change order：
struct aml_i3c_ao_clk *ao;
struct clk *fclk;
struct clk_bulk_data *new_clks;
struct device *dev = master->dev;
struct clk_init_data init = {};
char name[64];
int ret;
> Frank
>
>> +
>> + 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);
>> + 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++;
>> +
>> + return 0;
>> +}
>> +
>> +static int aml_i3c_init(struct svc_i3c_master *master)
>> +{
>> + struct device *dev = master->dev;
>> + long fin_freq;
>> + int ret, i;
>> + struct clk *fclk;
>> +
>> + if (vendor_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.
>> + */
>> + 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, 25000000);
>> + if (fin_freq <= 0) {
>> + dev_err(dev, "invalid fast_clk\n");
>> + return fin_freq ? fin_freq : -EINVAL;
>> + }
>> +
>> + 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 = SVC_I3C_QUIRK_FIFO_EMPTY |
>> + SVC_I3C_QUIRK_FALSE_SLVSTART |
>> + SVC_I3C_QUIRK_DAA_CORRUPT,
>> + .vendor_init = aml_i3c_init,
>> +};
>> +
>> +static const struct svc_i3c_drvdata aml_a9_ao_drvdata = {
>> + .quirks = SVC_I3C_QUIRK_FIFO_EMPTY |
>> + SVC_I3C_QUIRK_FALSE_SLVSTART |
>> + SVC_I3C_QUIRK_DAA_CORRUPT,
>> + .vendor_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
>>
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v2 1/2] i3c: master: svc: Add amlogic a9 support
@ 2026-11-04 2:44 ` Junyi Zhao
0 siblings, 0 replies; 48+ messages in thread
From: Junyi Zhao @ 2026-11-04 2:44 UTC (permalink / raw)
To: Frank Li
Cc: Alexandre Belloni, Frank Li, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Neil Armstrong, Kevin Hilman, Jerome Brunet,
Martin Blumenstingl, Miquel Raynal, Kees Cook,
Gustavo A. R. Silva, linux-i3c, devicetree, linux-kernel,
linux-arm-kernel, linux-amlogic, imx, linux-hardening,
xianwei.zhao, Kelvin.Zhang
Hi Frank,
Thanks for the review.
On 03/11/2026 06:10, Frank Li wrote:
> [ EXTERNAL EMAIL ]
>
> On Mon, Nov 02, 2026 at 07:05:39PM +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.
>>
>> Add a vendor_init callback to handle this clock setup.
>> Add a vendor_quirks field to describe AO-specific behavior.
>>
>> Signed-off-by: Junyi Zhao <junyi.zhao@amlogic.com>
>> ---
>> drivers/i3c/master/svc-i3c-master.c | 214 +++++++++++++++++++++++++++++++++++-
>> 1 file changed, 209 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
>> index e700392f0..98aa6eb0f 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,21 @@
>> */
>> #define SVC_I3C_QUIRK_DAA_CORRUPT BIT(2)
>>
>> +/* Amlogic specific registers */
>> +#define AML_AO_I3C_FCLK_DIV_CTRL 0x304
>> +#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
>> +
>> +/*
>> + * 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 own register space.
>> + */
>> +#define AML_I3C_QUIRK_AO BIT(0)
>> +
>
> There are not much quick now, just put after SVC_I3C_QUIRK_DAA_CORRUPT
>
> #define AML_I3C_QUIRK_AO BIT(3)
>
> use a unified quirk define.
will do.
>
>> struct svc_i3c_cmd {
>> u8 addr;
>> union {
>> @@ -195,10 +211,6 @@ struct svc_i3c_regs_save {
>> u32 mdynaddr;
>> };
>>
>> -struct svc_i3c_drvdata {
>> - u32 quirks;
>> -};
>> -
>> /**
>> * struct svc_i3c_master - Silvaco I3C Master structure
>> * @base: I3C master controller
>> @@ -257,6 +269,12 @@ struct svc_i3c_master {
>> u32 mctrl_config;
>> };
>>
>> +struct svc_i3c_drvdata {
>> + u32 quirks;
>> + u32 vendor_quirks;
>
> Needn't this, direct use quirks
will do.
>> + int (*vendor_init)(struct svc_i3c_master *master);
>> +};
>> +
>
> why need move this struct
because callback need param struct svc_i3c_master *master， and if not，
compile error. undefined.
>
>> /**
>> * struct svc_i3c_i2c_dev_data - Device specific data
>> * @index: Index in the master tables corresponding to this device
>> @@ -274,6 +292,11 @@ static inline bool svc_has_quirk(struct svc_i3c_master *master, u32 quirk)
>> return (master->drvdata->quirks & quirk);
>> }
>>
>> +static inline bool vendor_has_quirk(struct svc_i3c_master *master, u32 quirk)
>> +{
>> + return (master->drvdata->vendor_quirks & quirk);
>> +}
>> +
>
> Needn't this one
will delete it.
>> static inline bool svc_has_daa_corrupt(struct svc_i3c_master *master)
>> {
>> return ((master->drvdata->quirks & SVC_I3C_QUIRK_DAA_CORRUPT) &&
>> @@ -2011,6 +2034,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,6 +2043,12 @@ 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");
>>
>> + if (master->drvdata->vendor_init) {
>> + ret = master->drvdata->vendor_init(master);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> for (i = 0; i < master->num_clks; i++) {
>> if (!strcmp(master->clks[i].id, "fast_clk"))
>> break;
>> @@ -2036,7 +2066,6 @@ static int svc_i3c_master_probe(struct platform_device *pdev)
>> 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");
>> @@ -2163,9 +2192,184 @@ static const struct svc_i3c_drvdata npcm845_drvdata = {
>>
>> static const struct svc_i3c_drvdata svc_default_drvdata = {};
>>
>> +/*
>> + * Container for the tail clocks of the AO domain variant. 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 internal divider chain that
>> + * produces the AO variant's fast_clk, and append it to the clk bulk
>> + * array as "fast_clk". @parent must be "pclk", the only clock the AO
>> + * variant exposes (see silvaco,i3c-master.yaml).
>> + */
>
> use kernel doc format.
will change.
>> +static int aml_ao_i3c_tail_clocks(struct svc_i3c_master *master, struct clk *parent)
>> +{
>> + struct device *dev = master->dev;
>> + struct aml_i3c_ao_clk *ao;
>> + struct clk_init_data init = {};
>> + char name[64];
>> + int ret;
>> + struct clk_bulk_data *new_clks;
>> + struct clk *fclk;
>
> keep revise Chemistry order
>
will change order：
struct aml_i3c_ao_clk *ao;
struct clk *fclk;
struct clk_bulk_data *new_clks;
struct device *dev = master->dev;
struct clk_init_data init = {};
char name[64];
int ret;
> Frank
>
>> +
>> + 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);
>> + 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++;
>> +
>> + return 0;
>> +}
>> +
>> +static int aml_i3c_init(struct svc_i3c_master *master)
>> +{
>> + struct device *dev = master->dev;
>> + long fin_freq;
>> + int ret, i;
>> + struct clk *fclk;
>> +
>> + if (vendor_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.
>> + */
>> + 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, 25000000);
>> + if (fin_freq <= 0) {
>> + dev_err(dev, "invalid fast_clk\n");
>> + return fin_freq ? fin_freq : -EINVAL;
>> + }
>> +
>> + 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 = SVC_I3C_QUIRK_FIFO_EMPTY |
>> + SVC_I3C_QUIRK_FALSE_SLVSTART |
>> + SVC_I3C_QUIRK_DAA_CORRUPT,
>> + .vendor_init = aml_i3c_init,
>> +};
>> +
>> +static const struct svc_i3c_drvdata aml_a9_ao_drvdata = {
>> + .quirks = SVC_I3C_QUIRK_FIFO_EMPTY |
>> + SVC_I3C_QUIRK_FALSE_SLVSTART |
>> + SVC_I3C_QUIRK_DAA_CORRUPT,
>> + .vendor_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
>>
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v2 1/2] i3c: master: svc: Add amlogic a9 support
2026-11-04 2:44 ` Junyi Zhao
(?)
@ 2026-11-04 3:19 ` Frank Li
-1 siblings, 0 replies; 48+ messages in thread
From: Frank Li @ 2026-11-04 3:19 UTC (permalink / raw)
To: Junyi Zhao
Cc: Alexandre Belloni, Frank Li, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Neil Armstrong, Kevin Hilman, Jerome Brunet,
Martin Blumenstingl, Miquel Raynal, Kees Cook,
Gustavo A. R. Silva, linux-i3c, devicetree, linux-kernel,
linux-arm-kernel, linux-amlogic, imx, linux-hardening,
xianwei.zhao, Kelvin.Zhang
On Wed, Nov 04, 2026 at 10:44:59AM +0800, Junyi Zhao wrote:
> Hi Frank,
>
> Thanks for the review.
>
> On 03/11/2026 06:10, Frank Li wrote:
> > [ EXTERNAL EMAIL ]
> >
> > On Mon, Nov 02, 2026 at 07:05:39PM +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.
> > >
> > > Add a vendor_init callback to handle this clock setup.
> > > Add a vendor_quirks field to describe AO-specific behavior.
> > >
> > > Signed-off-by: Junyi Zhao <junyi.zhao@amlogic.com>
> > > ---
...
> > > + int (*vendor_init)(struct svc_i3c_master *master);
> > > +};
> > > +
> >
> > why need move this struct
>
> because callback need param struct svc_i3c_master *master， and if not，
> compile error. undefined.
Most likely svc_i3c_master need save a point for drvdata in future.
You add forward declear before drvdata
struct svc_i3c_drvdata;
>
...
> > > +static int aml_ao_i3c_tail_clocks(struct svc_i3c_master *master, struct clk *parent)
> > > +{
> > > + struct device *dev = master->dev;
> > > + struct aml_i3c_ao_clk *ao;
> > > + struct clk_init_data init = {};
> > > + char name[64];
> > > + int ret;
> > > + struct clk_bulk_data *new_clks;
> > > + struct clk *fclk;
> >
> > keep revise Chemistry order
> >
> will change order：
> struct aml_i3c_ao_clk *ao;
> struct clk *fclk;
> struct clk_bulk_data *new_clks;
> struct device *dev = master->dev;
> struct clk_init_data init = {};
> char name[64];
> int ret;
longest is first
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;
Frank
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v2 1/2] i3c: master: svc: Add amlogic a9 support
@ 2026-11-04 3:19 ` Frank Li
0 siblings, 0 replies; 48+ messages in thread
From: Frank Li @ 2026-11-04 3:19 UTC (permalink / raw)
To: Junyi Zhao
Cc: Alexandre Belloni, Frank Li, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Neil Armstrong, Kevin Hilman, Jerome Brunet,
Martin Blumenstingl, Miquel Raynal, Kees Cook,
Gustavo A. R. Silva, linux-i3c, devicetree, linux-kernel,
linux-arm-kernel, linux-amlogic, imx, linux-hardening,
xianwei.zhao, Kelvin.Zhang
On Wed, Nov 04, 2026 at 10:44:59AM +0800, Junyi Zhao wrote:
> Hi Frank,
>
> Thanks for the review.
>
> On 03/11/2026 06:10, Frank Li wrote:
> > [ EXTERNAL EMAIL ]
> >
> > On Mon, Nov 02, 2026 at 07:05:39PM +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.
> > >
> > > Add a vendor_init callback to handle this clock setup.
> > > Add a vendor_quirks field to describe AO-specific behavior.
> > >
> > > Signed-off-by: Junyi Zhao <junyi.zhao@amlogic.com>
> > > ---
...
> > > + int (*vendor_init)(struct svc_i3c_master *master);
> > > +};
> > > +
> >
> > why need move this struct
>
> because callback need param struct svc_i3c_master *master， and if not，
> compile error. undefined.
Most likely svc_i3c_master need save a point for drvdata in future.
You add forward declear before drvdata
struct svc_i3c_drvdata;
>
...
> > > +static int aml_ao_i3c_tail_clocks(struct svc_i3c_master *master, struct clk *parent)
> > > +{
> > > + struct device *dev = master->dev;
> > > + struct aml_i3c_ao_clk *ao;
> > > + struct clk_init_data init = {};
> > > + char name[64];
> > > + int ret;
> > > + struct clk_bulk_data *new_clks;
> > > + struct clk *fclk;
> >
> > keep revise Chemistry order
> >
> will change order：
> struct aml_i3c_ao_clk *ao;
> struct clk *fclk;
> struct clk_bulk_data *new_clks;
> struct device *dev = master->dev;
> struct clk_init_data init = {};
> char name[64];
> int ret;
longest is first
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;
Frank
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v2 1/2] i3c: master: svc: Add amlogic a9 support
@ 2026-11-04 3:19 ` Frank Li
0 siblings, 0 replies; 48+ messages in thread
From: Frank Li @ 2026-11-04 3:19 UTC (permalink / raw)
To: Junyi Zhao
Cc: Alexandre Belloni, Frank Li, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Neil Armstrong, Kevin Hilman, Jerome Brunet,
Martin Blumenstingl, Miquel Raynal, Kees Cook,
Gustavo A. R. Silva, linux-i3c, devicetree, linux-kernel,
linux-arm-kernel, linux-amlogic, imx, linux-hardening,
xianwei.zhao, Kelvin.Zhang
On Wed, Nov 04, 2026 at 10:44:59AM +0800, Junyi Zhao wrote:
> Hi Frank,
>
> Thanks for the review.
>
> On 03/11/2026 06:10, Frank Li wrote:
> > [ EXTERNAL EMAIL ]
> >
> > On Mon, Nov 02, 2026 at 07:05:39PM +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.
> > >
> > > Add a vendor_init callback to handle this clock setup.
> > > Add a vendor_quirks field to describe AO-specific behavior.
> > >
> > > Signed-off-by: Junyi Zhao <junyi.zhao@amlogic.com>
> > > ---
...
> > > + int (*vendor_init)(struct svc_i3c_master *master);
> > > +};
> > > +
> >
> > why need move this struct
>
> because callback need param struct svc_i3c_master *master， and if not，
> compile error. undefined.
Most likely svc_i3c_master need save a point for drvdata in future.
You add forward declear before drvdata
struct svc_i3c_drvdata;
>
...
> > > +static int aml_ao_i3c_tail_clocks(struct svc_i3c_master *master, struct clk *parent)
> > > +{
> > > + struct device *dev = master->dev;
> > > + struct aml_i3c_ao_clk *ao;
> > > + struct clk_init_data init = {};
> > > + char name[64];
> > > + int ret;
> > > + struct clk_bulk_data *new_clks;
> > > + struct clk *fclk;
> >
> > keep revise Chemistry order
> >
> will change order：
> struct aml_i3c_ao_clk *ao;
> struct clk *fclk;
> struct clk_bulk_data *new_clks;
> struct device *dev = master->dev;
> struct clk_init_data init = {};
> char name[64];
> int ret;
longest is first
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;
Frank
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v2 1/2] i3c: master: svc: Add amlogic a9 support
2026-11-04 3:19 ` Frank Li
(?)
@ 2026-11-04 5:18 ` Junyi Zhao
-1 siblings, 0 replies; 48+ messages in thread
From: Junyi Zhao @ 2026-11-04 5:18 UTC (permalink / raw)
To: Frank Li
Cc: Alexandre Belloni, Frank Li, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Neil Armstrong, Kevin Hilman, Jerome Brunet,
Martin Blumenstingl, Miquel Raynal, Kees Cook,
Gustavo A. R. Silva, linux-i3c, devicetree, linux-kernel,
linux-arm-kernel, linux-amlogic, imx, linux-hardening,
xianwei.zhao, Kelvin.Zhang
Hi Frank,
thanks for your detail.
On 04/11/2026 11:19, Frank Li wrote:
> [ EXTERNAL EMAIL ]
>
> On Wed, Nov 04, 2026 at 10:44:59AM +0800, Junyi Zhao wrote:
>> Hi Frank,
>>
>> Thanks for the review.
>>
>> On 03/11/2026 06:10, Frank Li wrote:
>>> [ EXTERNAL EMAIL ]
>>>
>>> On Mon, Nov 02, 2026 at 07:05:39PM +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.
>>>>
>>>> Add a vendor_init callback to handle this clock setup.
>>>> Add a vendor_quirks field to describe AO-specific behavior.
>>>>
>>>> Signed-off-by: Junyi Zhao <junyi.zhao@amlogic.com>
>>>> ---
> ...
>>>> + int (*vendor_init)(struct svc_i3c_master *master);
>>>> +};
>>>> +
>>>
>>> why need move this struct
>>
>> because callback need param struct svc_i3c_master *master， and if not，
>> compile error. undefined.
>
> Most likely svc_i3c_master need save a point for drvdata in future.
>
> You add forward declear before drvdata
>
> struct svc_i3c_drvdata;
>
will do.
>>
> ...
>>>> +static int aml_ao_i3c_tail_clocks(struct svc_i3c_master *master, struct clk *parent)
>>>> +{
>>>> + struct device *dev = master->dev;
>>>> + struct aml_i3c_ao_clk *ao;
>>>> + struct clk_init_data init = {};
>>>> + char name[64];
>>>> + int ret;
>>>> + struct clk_bulk_data *new_clks;
>>>> + struct clk *fclk;
>>>
>>> keep revise Chemistry order
>>>
>> will change order：
>> struct aml_i3c_ao_clk *ao;
>> struct clk *fclk;
>> struct clk_bulk_data *new_clks;
>> struct device *dev = master->dev;
>> struct clk_init_data init = {};
>> char name[64];
>> int ret;
>
>
> longest is first
>
> 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;
>
will do. thanks the detail.
>
> Frank
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v2 1/2] i3c: master: svc: Add amlogic a9 support
@ 2026-11-04 5:18 ` Junyi Zhao
0 siblings, 0 replies; 48+ messages in thread
From: Junyi Zhao @ 2026-11-04 5:18 UTC (permalink / raw)
To: Frank Li
Cc: Alexandre Belloni, Frank Li, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Neil Armstrong, Kevin Hilman, Jerome Brunet,
Martin Blumenstingl, Miquel Raynal, Kees Cook,
Gustavo A. R. Silva, linux-i3c, devicetree, linux-kernel,
linux-arm-kernel, linux-amlogic, imx, linux-hardening,
xianwei.zhao, Kelvin.Zhang
Hi Frank,
thanks for your detail.
On 04/11/2026 11:19, Frank Li wrote:
> [ EXTERNAL EMAIL ]
>
> On Wed, Nov 04, 2026 at 10:44:59AM +0800, Junyi Zhao wrote:
>> Hi Frank,
>>
>> Thanks for the review.
>>
>> On 03/11/2026 06:10, Frank Li wrote:
>>> [ EXTERNAL EMAIL ]
>>>
>>> On Mon, Nov 02, 2026 at 07:05:39PM +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.
>>>>
>>>> Add a vendor_init callback to handle this clock setup.
>>>> Add a vendor_quirks field to describe AO-specific behavior.
>>>>
>>>> Signed-off-by: Junyi Zhao <junyi.zhao@amlogic.com>
>>>> ---
> ...
>>>> + int (*vendor_init)(struct svc_i3c_master *master);
>>>> +};
>>>> +
>>>
>>> why need move this struct
>>
>> because callback need param struct svc_i3c_master *master， and if not，
>> compile error. undefined.
>
> Most likely svc_i3c_master need save a point for drvdata in future.
>
> You add forward declear before drvdata
>
> struct svc_i3c_drvdata;
>
will do.
>>
> ...
>>>> +static int aml_ao_i3c_tail_clocks(struct svc_i3c_master *master, struct clk *parent)
>>>> +{
>>>> + struct device *dev = master->dev;
>>>> + struct aml_i3c_ao_clk *ao;
>>>> + struct clk_init_data init = {};
>>>> + char name[64];
>>>> + int ret;
>>>> + struct clk_bulk_data *new_clks;
>>>> + struct clk *fclk;
>>>
>>> keep revise Chemistry order
>>>
>> will change order：
>> struct aml_i3c_ao_clk *ao;
>> struct clk *fclk;
>> struct clk_bulk_data *new_clks;
>> struct device *dev = master->dev;
>> struct clk_init_data init = {};
>> char name[64];
>> int ret;
>
>
> longest is first
>
> 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;
>
will do. thanks the detail.
>
> Frank
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v2 1/2] i3c: master: svc: Add amlogic a9 support
@ 2026-11-04 5:18 ` Junyi Zhao
0 siblings, 0 replies; 48+ messages in thread
From: Junyi Zhao @ 2026-11-04 5:18 UTC (permalink / raw)
To: Frank Li
Cc: Alexandre Belloni, Frank Li, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Neil Armstrong, Kevin Hilman, Jerome Brunet,
Martin Blumenstingl, Miquel Raynal, Kees Cook,
Gustavo A. R. Silva, linux-i3c, devicetree, linux-kernel,
linux-arm-kernel, linux-amlogic, imx, linux-hardening,
xianwei.zhao, Kelvin.Zhang
Hi Frank,
thanks for your detail.
On 04/11/2026 11:19, Frank Li wrote:
> [ EXTERNAL EMAIL ]
>
> On Wed, Nov 04, 2026 at 10:44:59AM +0800, Junyi Zhao wrote:
>> Hi Frank,
>>
>> Thanks for the review.
>>
>> On 03/11/2026 06:10, Frank Li wrote:
>>> [ EXTERNAL EMAIL ]
>>>
>>> On Mon, Nov 02, 2026 at 07:05:39PM +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.
>>>>
>>>> Add a vendor_init callback to handle this clock setup.
>>>> Add a vendor_quirks field to describe AO-specific behavior.
>>>>
>>>> Signed-off-by: Junyi Zhao <junyi.zhao@amlogic.com>
>>>> ---
> ...
>>>> + int (*vendor_init)(struct svc_i3c_master *master);
>>>> +};
>>>> +
>>>
>>> why need move this struct
>>
>> because callback need param struct svc_i3c_master *master， and if not，
>> compile error. undefined.
>
> Most likely svc_i3c_master need save a point for drvdata in future.
>
> You add forward declear before drvdata
>
> struct svc_i3c_drvdata;
>
will do.
>>
> ...
>>>> +static int aml_ao_i3c_tail_clocks(struct svc_i3c_master *master, struct clk *parent)
>>>> +{
>>>> + struct device *dev = master->dev;
>>>> + struct aml_i3c_ao_clk *ao;
>>>> + struct clk_init_data init = {};
>>>> + char name[64];
>>>> + int ret;
>>>> + struct clk_bulk_data *new_clks;
>>>> + struct clk *fclk;
>>>
>>> keep revise Chemistry order
>>>
>> will change order：
>> struct aml_i3c_ao_clk *ao;
>> struct clk *fclk;
>> struct clk_bulk_data *new_clks;
>> struct device *dev = master->dev;
>> struct clk_init_data init = {};
>> char name[64];
>> int ret;
>
>
> longest is first
>
> 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;
>
will do. thanks the detail.
>
> Frank
^ permalink raw reply [flat|nested] 48+ messages in thread