[PATCH v3 0/2] Add Amlogic A9 I3C support

[PATCH v3 0/2] Add Amlogic A9 I3C support

[Junyi Zhao via B4 Relay]

Amlogic A9 SoCs integrate the Silvaco I3C IP, but their peripheral
clock setup differs from the original implementation: the EE-domain
controller (amlogic,a9-i3c) takes two clocks, "pclk" and "fast_clk",
while the AO-domain controller (amlogic,a9-ao-i3c) exposes only
"pclk" and derives its bus clock from an internal divider inside the
controller's own register region.

This series consists of:

  - Add a vendor_init callback for the clock setup and an
    Amlogic-specific bit (AML_I3C_QUIRK_AO) for AO-specific behaviour
    to the Silvaco I3C master driver, and enable the existing
    Silvaco workaround quirks for both Amlogic variants.

  - Add the Amlogic compatibles and per-variant clock
    constraints to the silvaco,i3c-master binding schema.

Verified on Amlogic A9 (BY401) with both EE and AO controller
instances. dt_binding_check and dtbs_check pass with this series.

Signed-off-by: Junyi Zhao <junyi.zhao@amlogic.com>
---
Changes in v3:
  - Merge vendor quirks into the legacy quirks bitmask and add
    AML_I3C_QUIRK_AO for the AO variant.
  - Check the devm_kcalloc() return value when allocating the new
    clk bulk array.
  - Enable the clocks before aml_ao_i3c_tail_clocks(), because the
    divider registers are accessed during registration.
  - Rework the probe error paths so the clocks are disabled when
    vendor_init or later steps fail.
  - Reorder the local variables (longest first).
  - Convert comments to kernel-doc format.
  - Depend on COMMON_CLK in Kconfig.
  - Drop the silvaco,i3c-master-v1 fallback and add a blank line
    before the new if/then block in the binding.
  - Explain the I3C peripheral integration details in the binding
    commit message.

- Link to v2: https://patch.msgid.link/20261102-a9_i3c-v2-0-0c474bfccffa@amlogic.com

Changes in v2:

  - No need rework the Silvaco I3C driver. 
  - Based on vanilla driver, adapt A9 SoCs through callback 
    and quirks to handle the variant.

- Link to v1: https://patch.msgid.link/20261019-a9_i3c-v1-0-329d819d621f@amlogic.com

---
Junyi Zhao (2):
      i3c: master: svc: Add amlogic a9 support
      dt-bindings: i3c: silvaco,i3c-master: Add Amlogic A9 support

 .../bindings/i3c/silvaco,i3c-master.yaml           |  26 +-
 drivers/i3c/master/Kconfig                         |   1 +
 drivers/i3c/master/svc-i3c-master.c                | 274 ++++++++++++++++++++-
 3 files changed, 287 insertions(+), 14 deletions(-)
---
base-commit: aac26bee2287c88af5be5a5ff96d783b19a28790
change-id: 20261010-a9_i3c-9d5245d094c8

Best regards,
--  
Junyi Zhao <junyi.zhao@amlogic.com>



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

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

[Junyi Zhao via B4 Relay]

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.

Both variants need setup that the generic driver does not perform:
the fast clock rate must be rounded and set at probe time, and on the
AO variant the internal divider must be registered with the clock
framework so that "fast_clk" even exists. The divider registers sit
in the controller's own register space, so they can only be accessed
while pclk is running.

Add a vendor_init callback, invoked from the probe with the clocks
enabled, to handle this. On the AO variant it registers the internal
divider chain (div_en -> div -> gate) with the common clock framework
and appends the resulting clock to the clk bulk array as "fast_clk";
for both variants it then rounds and sets the fast clock rate
(target 25 MHz). As vendor_init now runs after
clk_bulk_prepare_enable(), the probe error paths that follow disable
the clocks on failure.

Extend the quirks bitmask with an Amlogic-specific bit
(AML_I3C_QUIRK_AO) marking the AO variant's internal-divider
behaviour. Since the driver now registers clocks through the common
clock framework, SVC_I3C_MASTER also depends on COMMON_CLK.

Signed-off-by: Junyi Zhao <junyi.zhao@amlogic.com>
---
 drivers/i3c/master/Kconfig          |   1 +
 drivers/i3c/master/svc-i3c-master.c | 274 ++++++++++++++++++++++++++++++++++--
 2 files changed, 263 insertions(+), 12 deletions(-)

diff --git a/drivers/i3c/master/Kconfig b/drivers/i3c/master/Kconfig
index da96d2aaa..ac7d246c2 100644
--- a/drivers/i3c/master/Kconfig
+++ b/drivers/i3c/master/Kconfig
@@ -47,6 +47,7 @@ config AST2600_I3C_MASTER
 
 config SVC_I3C_MASTER
 	tristate "Silvaco I3C Dual-Role Master driver"
+	depends on COMMON_CLK
 	depends on HAS_IOMEM
 	depends on !(ALPHA || PARISC)
 	help
diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index e700392f0..aef56286e 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -9,6 +9,7 @@
 
 #include <linux/bitfield.h>
 #include <linux/clk.h>
+#include <linux/clk-provider.h>
 #include <linux/completion.h>
 #include <linux/errno.h>
 #include <linux/i3c/master.h>
@@ -166,6 +167,31 @@
  */
 #define SVC_I3C_QUIRK_DAA_CORRUPT	BIT(2)
 
+/*
+ * AML_I3C_QUIRK_AO:
+ * AO is the Always-On domain on Amlogic SoCs (remains powered in suspend).
+ * Its clock is generated by an internal divider located in the I3C
+ * controller's own register space.
+ */
+#define AML_I3C_QUIRK_AO	BIT(3)
+
+/*
+ * Quirks shared by all the Amlogic A9 I3C instances.
+ */
+#define AML_A9_I3C_QUIRKS	(SVC_I3C_QUIRK_FIFO_EMPTY | \
+				 SVC_I3C_QUIRK_FALSE_SLVSTART | \
+				 SVC_I3C_QUIRK_DAA_CORRUPT)
+
+/* Amlogic specific registers */
+#define AML_AO_I3C_FCLK_DIV_CTRL		0x304
+#define AML_AO_I3C_FCLK_DIV_EN_SHIFT		8
+#define AML_AO_I3C_FCLK_DIV_SHIFT		0
+#define AML_AO_I3C_FCLK_DIV_WIDTH		8
+#define AML_AO_I3C_FCLK_EN_SHIFT		9
+
+/* Target fast clock rate of the Amlogic A9 I3C controllers, in Hz */
+#define AML_I3C_FAST_CLK_RATE		25000000
+
 struct svc_i3c_cmd {
 	u8 addr;
 	union {
@@ -195,8 +221,11 @@ struct svc_i3c_regs_save {
 	u32 mdynaddr;
 };
 
+struct svc_i3c_master;
+
 struct svc_i3c_drvdata {
 	u32 quirks;
+	int (*vendor_init)(struct svc_i3c_master *master);
 };
 
 /**
@@ -2011,6 +2040,7 @@ static int svc_i3c_master_probe(struct platform_device *pdev)
 	if (!master->drvdata)
 		return -EINVAL;
 
+	master->dev = dev;
 	master->regs = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(master->regs))
 		return PTR_ERR(master->regs);
@@ -2019,27 +2049,43 @@ static int svc_i3c_master_probe(struct platform_device *pdev)
 	if (master->num_clks < 0)
 		return dev_err_probe(dev, -EINVAL, "can't get I3C clocks\n");
 
+	ret = clk_bulk_prepare_enable(master->num_clks, master->clks);
+	if (ret)
+		return dev_err_probe(dev, ret, "can't enable I3C clocks\n");
+
+	/*
+	 * Vendor specific setup may register additional clocks (e.g. the
+	 * internal divider chain of the AO variant) and program clock rates,
+	 * so it must run with the clocks enabled.
+	 */
+	if (master->drvdata->vendor_init) {
+		ret = master->drvdata->vendor_init(master);
+		if (ret)
+			goto err_disable_clks;
+	}
+
 	for (i = 0; i < master->num_clks; i++) {
 		if (!strcmp(master->clks[i].id, "fast_clk"))
 			break;
 	}
 
-	if (i == master->num_clks)
-		return dev_err_probe(dev, -EINVAL,
-				     "can't get I3C peripheral clock\n");
+	if (i == master->num_clks) {
+		ret = dev_err_probe(dev, -EINVAL,
+				    "can't get I3C peripheral clock\n");
+		goto err_disable_clks;
+	}
 
 	master->fclk = master->clks[i].clk;
-	if (IS_ERR(master->fclk))
-		return PTR_ERR(master->fclk);
+	if (IS_ERR(master->fclk)) {
+		ret = PTR_ERR(master->fclk);
+		goto err_disable_clks;
+	}
 
 	master->irq = platform_get_irq(pdev, 0);
-	if (master->irq < 0)
-		return master->irq;
-
-	master->dev = dev;
-	ret = clk_bulk_prepare_enable(master->num_clks, master->clks);
-	if (ret)
-		return dev_err_probe(dev, ret, "can't enable I3C clocks\n");
+	if (master->irq < 0) {
+		ret = master->irq;
+		goto err_disable_clks;
+	}
 
 	mutex_init(&master->lock);
 
@@ -2163,9 +2209,213 @@ static const struct svc_i3c_drvdata npcm845_drvdata = {
 
 static const struct svc_i3c_drvdata svc_default_drvdata = {};
 
+/**
+ * struct aml_i3c_ao_clk - container for the AO variant's internal fast clock
+ * @div_en: enables the internal divider
+ * @div: bus clock divider
+ * @gate: output gate of the generated fast clock
+ * @lock: spinlock shared by the gate and divider register operations
+ *
+ * The clock HW structures are registered with the clock framework and must
+ * stay alive for the whole driver lifetime, so they are kept along with the
+ * shared lock used by their gate/divider operations.
+ */
+struct aml_i3c_ao_clk {
+	struct clk_gate div_en;
+	struct clk_divider div;
+	struct clk_gate gate;
+	spinlock_t lock;
+};
+
+/**
+ * aml_ao_i3c_tail_clocks - register the AO variant's internal fast clock
+ * @master: I3C master controller
+ * @parent: parent clock of the internal divider chain; must be "pclk", the
+ *          only clock the AO variant exposes (see silvaco,i3c-master.yaml)
+ *
+ * Register the internal divider chain that produces the AO variant's
+ * fast_clk and append it to the clk bulk array as "fast_clk". The caller
+ * must run with the clocks enabled since the divider registers are
+ * accessed during registration. The appended clock is prepared and
+ * enabled here as well, because clk_bulk_prepare_enable() already ran
+ * in the probe with the original clock list.
+ *
+ * Return: 0 on success, or a negative error code.
+ */
+static int aml_ao_i3c_tail_clocks(struct svc_i3c_master *master, struct clk *parent)
+{
+	struct device *dev = master->dev;
+	struct clk_bulk_data *new_clks;
+	struct clk_init_data init = {};
+	struct aml_i3c_ao_clk *ao;
+	struct clk *fclk;
+	char name[64];
+	int ret;
+
+	ao = devm_kzalloc(dev, sizeof(*ao), GFP_KERNEL);
+	if (!ao)
+		return -ENOMEM;
+
+	spin_lock_init(&ao->lock);
+
+	snprintf(name, sizeof(name), "%s#div_en", dev_name(dev));
+	init.name = name;
+	init.ops = &clk_gate_ops;
+	init.flags = CLK_IGNORE_UNUSED;
+	init.parent_data = &(struct clk_parent_data) {
+		.index = -1,
+		.hw = __clk_get_hw(parent),
+	};
+	init.num_parents = 1;
+
+	ao->div_en.reg = master->regs + AML_AO_I3C_FCLK_DIV_CTRL;
+	ao->div_en.bit_idx = AML_AO_I3C_FCLK_DIV_EN_SHIFT;
+	ao->div_en.hw.init = &init;
+	ao->div_en.lock = &ao->lock;
+
+	ret = devm_clk_hw_register(dev, &ao->div_en.hw);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to register %s\n", name);
+
+	snprintf(name, sizeof(name), "%s#div", dev_name(dev));
+	init.name = name;
+	init.ops = &clk_divider_ops;
+	init.flags = 0;
+	init.parent_data = &(struct clk_parent_data) {
+		.index = -1,
+		.hw = &ao->div_en.hw,
+	};
+	init.num_parents = 1;
+
+	ao->div.reg = master->regs + AML_AO_I3C_FCLK_DIV_CTRL;
+	ao->div.shift = AML_AO_I3C_FCLK_DIV_SHIFT;
+	ao->div.width = AML_AO_I3C_FCLK_DIV_WIDTH;
+	ao->div.hw.init = &init;
+	ao->div.lock = &ao->lock;
+
+	ret = devm_clk_hw_register(dev, &ao->div.hw);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to register %s\n", name);
+
+	snprintf(name, sizeof(name), "%s#gate", dev_name(dev));
+	init.name = name;
+	init.ops = &clk_gate_ops;
+	init.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED;
+	init.parent_data = &(struct clk_parent_data) {
+		.index = -1,
+		.hw = &ao->div.hw,
+	};
+	init.num_parents = 1;
+
+	ao->gate.reg = master->regs + AML_AO_I3C_FCLK_DIV_CTRL;
+	ao->gate.bit_idx = AML_AO_I3C_FCLK_EN_SHIFT;
+	ao->gate.hw.init = &init;
+	ao->gate.lock = &ao->lock;
+
+	ret = devm_clk_hw_register(dev, &ao->gate.hw);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to register %s\n", name);
+
+	fclk = devm_clk_hw_get_clk(dev, &ao->gate.hw, NULL);
+	if (IS_ERR(fclk))
+		return dev_err_probe(dev, PTR_ERR(fclk),
+				     "tail i3c fast clock failed\n");
+
+	new_clks = devm_kcalloc(dev, master->num_clks + 1, sizeof(*new_clks),
+				GFP_KERNEL);
+	if (!new_clks)
+		return -ENOMEM;
+
+	memcpy(new_clks, master->clks,
+	       master->num_clks * sizeof(*master->clks));
+	new_clks[master->num_clks].id = "fast_clk";
+	new_clks[master->num_clks].clk = fclk;
+	master->clks = new_clks;
+	master->num_clks++;
+
+	/*
+	 * Keep the clk bulk state balanced for the following
+	 * clk_bulk_disable_unprepare() calls (error paths and runtime PM).
+	 */
+	ret = clk_prepare_enable(fclk);
+	if (ret)
+		return dev_err_probe(dev, ret, "can't enable fast_clk\n");
+
+	return 0;
+}
+
+/**
+ * aml_i3c_init - Amlogic A9 specific init, called with the clocks enabled
+ * @master: I3C master controller
+ *
+ * Register the AO variant's internal divider chain and round/set the fast
+ * clock rate for both variants.
+ *
+ * Return: 0 on success, or a negative error code.
+ */
+static int aml_i3c_init(struct svc_i3c_master *master)
+{
+	struct device *dev = master->dev;
+	struct clk *fclk;
+	long fin_freq;
+	int ret, i;
+
+	if (svc_has_quirk(master, AML_I3C_QUIRK_AO)) {
+		/*
+		 * clk_bulk_get_all() preserves the DT order, and the
+		 * binding restricts the AO variant to exactly one clock,
+		 * which is always "pclk".  So clks[0] is the input of the
+		 * internal divider chain.
+		 */
+		if (strcmp(master->clks[0].id, "pclk"))
+			return dev_err_probe(dev, -EINVAL,
+					     "invalid AO input clock\n");
+
+		ret = aml_ao_i3c_tail_clocks(master, master->clks[0].clk);
+		if (ret)
+			return ret;
+	}
+
+	for (i = 0; i < master->num_clks; i++) {
+		if (!strcmp(master->clks[i].id, "fast_clk"))
+			break;
+	}
+
+	if (i == master->num_clks)
+		return dev_err_probe(dev, -EINVAL,
+				     "can't get I3C fast clock\n");
+
+	fclk = master->clks[i].clk;
+	if (IS_ERR(fclk))
+		return PTR_ERR(fclk);
+
+	fin_freq = clk_round_rate(fclk, AML_I3C_FAST_CLK_RATE);
+	if (fin_freq <= 0)
+		return dev_err_probe(dev, fin_freq ? fin_freq : -EINVAL,
+				     "invalid fast_clk\n");
+
+	ret = clk_set_rate(fclk, fin_freq);
+	if (ret)
+		return dev_err_probe(dev, ret, "setting fast_clk rate failed\n");
+
+	return 0;
+}
+
+static const struct svc_i3c_drvdata aml_a9_drvdata = {
+	.quirks = AML_A9_I3C_QUIRKS,
+	.vendor_init = aml_i3c_init,
+};
+
+static const struct svc_i3c_drvdata aml_a9_ao_drvdata = {
+	.quirks = AML_A9_I3C_QUIRKS | AML_I3C_QUIRK_AO,
+	.vendor_init = aml_i3c_init,
+};
+
 static const struct of_device_id svc_i3c_master_of_match_tbl[] = {
 	{ .compatible = "nuvoton,npcm845-i3c", .data = &npcm845_drvdata },
 	{ .compatible = "silvaco,i3c-master-v1", .data = &svc_default_drvdata },
+	{ .compatible = "amlogic,a9-i3c", .data = &aml_a9_drvdata },
+	{ .compatible = "amlogic,a9-ao-i3c", .data = &aml_a9_ao_drvdata },
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, svc_i3c_master_of_match_tbl);

-- 
2.42.0



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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [Medium] i3c: master: svc: unbalanced fclk disable in
  aml_ao_i3c_tail_clocks()
- [Medium] i3c: master: svc: NULL dereference of clks[0].id in
  aml_i3c_init()
- [Medium] i3c: master: svc: unclocked MMIO read in
  aml_ao_i3c_tail_clocks()
- [Low] i3c: master: svc: duplicate fast_clk lookup in aml_i3c_init()
--

commit 13e8b1ef64cdb3b2e6850ddcbaa3e5a7ffee2a56
Author: Junyi Zhao <junyi.zhao@amlogic.com>

i3c: master: svc: Add amlogic a9 support

This commit adds Amlogic A9 and A9 AO I3C controller support to the
Silvaco I3C master driver, including a vendor_init callback that registers
the AO variant's internal clock divider chain and configures the fast_clk
rate.

> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index e700392f0801f..aef56286ecfa8 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c
[ ... ]
> @@ -2019,27 +2049,43 @@ static int svc_i3c_master_probe(struct platform_device *pdev)
>  	if (master->num_clks < 0)
>  		return dev_err_probe(dev, -EINVAL, "can't get I3C clocks\n");
>  
> +	ret = clk_bulk_prepare_enable(master->num_clks, master->clks);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "can't enable I3C clocks\n");
> +
> +	/*
> +	 * Vendor specific setup may register additional clocks (e.g. the
> +	 * internal divider chain of the AO variant) and program clock rates,
> +	 * so it must run with the clocks enabled.
> +	 */
> +	if (master->drvdata->vendor_init) {
> +		ret = master->drvdata->vendor_init(master);
> +		if (ret)
> +			goto err_disable_clks;
> +	}
> +
>  	for (i = 0; i < master->num_clks; i++) {
>  		if (!strcmp(master->clks[i].id, "fast_clk"))
>  			break;
>  	}
>  
> -	if (i == master->num_clks)
> -		return dev_err_probe(dev, -EINVAL,
> -				     "can't get I3C peripheral clock\n");
> +	if (i == master->num_clks) {
> +		ret = dev_err_probe(dev, -EINVAL,
> +				    "can't get I3C peripheral clock\n");
> +		goto err_disable_clks;
> +	}
>  
>  	master->fclk = master->clks[i].clk;
> -	if (IS_ERR(master->fclk))
> -		return PTR_ERR(master->fclk);
> +	if (IS_ERR(master->fclk)) {
> +		ret = PTR_ERR(master->fclk);
> +		goto err_disable_clks;
> +	}
[ ... ]
> @@ -2163,9 +2209,213 @@ static const struct svc_i3c_drvdata npcm845_drvdata = {
[ ... ]
> +static int aml_ao_i3c_tail_clocks(struct svc_i3c_master *master, struct clk *parent)
> +{
> +	struct device *dev = master->dev;
> +	struct clk_bulk_data *new_clks;
> +	struct clk_init_data init = {};
> +	struct aml_i3c_ao_clk *ao;
> +	struct clk *fclk;
> +	char name[64];
> +	int ret;
> +
> +	ao = devm_kzalloc(dev, sizeof(*ao), GFP_KERNEL);
> +	if (!ao)
> +		return -ENOMEM;
> +
> +	spin_lock_init(&ao->lock);
> +
> +	snprintf(name, sizeof(name), "%s#div_en", dev_name(dev));
> +	init.name = name;
> +	init.ops = &clk_gate_ops;
> +	init.flags = CLK_IGNORE_UNUSED;
> +	init.parent_data = &(struct clk_parent_data) {
> +		.index = -1,
> +		.hw = __clk_get_hw(parent),
> +	};
> +	init.num_parents = 1;
> +
> +	ao->div_en.reg = master->regs + AML_AO_I3C_FCLK_DIV_CTRL;
> +	ao->div_en.bit_idx = AML_AO_I3C_FCLK_DIV_EN_SHIFT;
> +	ao->div_en.hw.init = &init;
> +	ao->div_en.lock = &ao->lock;
> +
> +	ret = devm_clk_hw_register(dev, &ao->div_en.hw);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to register %s\n", name);
> +
> +	snprintf(name, sizeof(name), "%s#div", dev_name(dev));
> +	init.name = name;
> +	init.ops = &clk_divider_ops;
> +	init.flags = 0;
> +	init.parent_data = &(struct clk_parent_data) {
> +		.index = -1,
> +		.hw = &ao->div_en.hw,
> +	};
> +	init.num_parents = 1;
> +
> +	ao->div.reg = master->regs + AML_AO_I3C_FCLK_DIV_CTRL;
> +	ao->div.shift = AML_AO_I3C_FCLK_DIV_SHIFT;
> +	ao->div.width = AML_AO_I3C_FCLK_DIV_WIDTH;
> +	ao->div.hw.init = &init;
> +	ao->div.lock = &ao->lock;
> +
> +	ret = devm_clk_hw_register(dev, &ao->div.hw);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to register %s\n", name);
> +
> +	snprintf(name, sizeof(name), "%s#gate", dev_name(dev));
> +	init.name = name;
> +	init.ops = &clk_gate_ops;
> +	init.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED;
> +	init.parent_data = &(struct clk_parent_data) {
> +		.index = -1,
> +		.hw = &ao->div.hw,
> +	};
> +	init.num_parents = 1;
> +
> +	ao->gate.reg = master->regs + AML_AO_I3C_FCLK_DIV_CTRL;
> +	ao->gate.bit_idx = AML_AO_I3C_FCLK_EN_SHIFT;
> +	ao->gate.hw.init = &init;
> +	ao->gate.lock = &ao->lock;
> +
> +	ret = devm_clk_hw_register(dev, &ao->gate.hw);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to register %s\n", name);

[Severity: Medium]
Can the clock framework read AML_AO_I3C_FCLK_DIV_CTRL while pclk is
disabled during runtime suspend?

svc_i3c_master_probe() invokes vendor_init() before calling
pm_runtime_enable(), so clk_pm_runtime_init() sees
pm_runtime_enabled(dev) == false and leaves core->rpm_enabled unset for
div_en, div, and gate, and none of the three clocks sets
CLK_OPS_PARENT_ENABLE.  Once svc_i3c_runtime_suspend() disables pclk,
any clock query (such as reading /sys/kernel/debug/clk/clk_summary)
calls clk_gate_ops.is_enabled or clk_divider_ops.recalc_rate and performs
an unclocked MMIO read of AML_AO_I3C_FCLK_DIV_CTRL.

> +	fclk = devm_clk_hw_get_clk(dev, &ao->gate.hw, NULL);
> +	if (IS_ERR(fclk))
> +		return dev_err_probe(dev, PTR_ERR(fclk),
> +				     "tail i3c fast clock failed\n");
> +
> +	new_clks = devm_kcalloc(dev, master->num_clks + 1, sizeof(*new_clks),
> +				GFP_KERNEL);
> +	if (!new_clks)
> +		return -ENOMEM;
> +
> +	memcpy(new_clks, master->clks,
> +	       master->num_clks * sizeof(*master->clks));
> +	new_clks[master->num_clks].id = "fast_clk";
> +	new_clks[master->num_clks].clk = fclk;
> +	master->clks = new_clks;
> +	master->num_clks++;
> +
> +	/*
> +	 * Keep the clk bulk state balanced for the following
> +	 * clk_bulk_disable_unprepare() calls (error paths and runtime PM).
> +	 */
> +	ret = clk_prepare_enable(fclk);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "can't enable fast_clk\n");
> +
> +	return 0;
> +}

[Severity: Medium]
If clk_prepare_enable(fclk) fails here, does the err_disable_clks path in
svc_i3c_master_probe() call clk_bulk_disable_unprepare() on an unprepared
and disabled fclk?  master->clks and master->num_clks are updated to
include fclk before clk_prepare_enable(fclk) succeeds, which will trigger
WARN() assertions in clk_core_disable() and clk_core_unprepare().

> +static int aml_i3c_init(struct svc_i3c_master *master)
> +{
> +	struct device *dev = master->dev;
> +	struct clk *fclk;
> +	long fin_freq;
> +	int ret, i;
> +
> +	if (svc_has_quirk(master, AML_I3C_QUIRK_AO)) {
> +		/*
> +		 * clk_bulk_get_all() preserves the DT order, and the
> +		 * binding restricts the AO variant to exactly one clock,
> +		 * which is always "pclk".  So clks[0] is the input of the
> +		 * internal divider chain.
> +		 */
> +		if (strcmp(master->clks[0].id, "pclk"))
> +			return dev_err_probe(dev, -EINVAL,
> +					     "invalid AO input clock\n");

[Severity: Medium]
Can this dereference a NULL pointer if the DT node omits clocks or
clock-names?

devm_clk_bulk_get_all() returns 0 with master->clks == NULL when no
clocks are present in DT (passing the master->num_clks < 0 check in
svc_i3c_master_probe()), and leaves master->clks[0].id as NULL if
clock-names is missing.

> +
> +		ret = aml_ao_i3c_tail_clocks(master, master->clks[0].clk);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	for (i = 0; i < master->num_clks; i++) {
> +		if (!strcmp(master->clks[i].id, "fast_clk"))
> +			break;
> +	}
> +
> +	if (i == master->num_clks)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "can't get I3C fast clock\n");
> +
> +	fclk = master->clks[i].clk;
> +	if (IS_ERR(fclk))
> +		return PTR_ERR(fclk);

[Severity: Low]
This isn't a bug, but this fast_clk lookup and IS_ERR() check duplicate
the identical loop that runs in svc_i3c_master_probe() right after
vendor_init() returns.  Could master->fclk be resolved once in a helper
or before setting the clock rate?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-a9_i3c-v3-0-804b69b9095b@amlogic.com?part=1

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

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

[Frank Li]

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

Do you have other registers after 0x304? Is it possible create indivial
clock driver for it, reg space is such as 0x300..0x400.

I3C use 0..0x300

So needn't distinguish w/o AO.

Frank

> +#define AML_AO_I3C_FCLK_DIV_EN_SHIFT		8
> +#define AML_AO_I3C_FCLK_DIV_SHIFT		0
> +#define AML_AO_I3C_FCLK_DIV_WIDTH		8
> +#define AML_AO_I3C_FCLK_EN_SHIFT		9
> +
> +/* Target fast clock rate of the Amlogic A9 I3C controllers, in Hz */
> +#define AML_I3C_FAST_CLK_RATE		25000000
> +
>  struct svc_i3c_cmd {
>  	u8 addr;
>  	union {
> @@ -195,8 +221,11 @@ struct svc_i3c_regs_save {
>  	u32 mdynaddr;
>  };
>
> +struct svc_i3c_master;
> +
>  struct svc_i3c_drvdata {
>  	u32 quirks;
> +	int (*vendor_init)(struct svc_i3c_master *master);
>  };
>
>  /**
> @@ -2011,6 +2040,7 @@ static int svc_i3c_master_probe(struct platform_device *pdev)
>  	if (!master->drvdata)
>  		return -EINVAL;
>
> +	master->dev = dev;
>  	master->regs = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(master->regs))
>  		return PTR_ERR(master->regs);
> @@ -2019,27 +2049,43 @@ static int svc_i3c_master_probe(struct platform_device *pdev)
>  	if (master->num_clks < 0)
>  		return dev_err_probe(dev, -EINVAL, "can't get I3C clocks\n");
>
> +	ret = clk_bulk_prepare_enable(master->num_clks, master->clks);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "can't enable I3C clocks\n");
> +
> +	/*
> +	 * Vendor specific setup may register additional clocks (e.g. the
> +	 * internal divider chain of the AO variant) and program clock rates,
> +	 * so it must run with the clocks enabled.
> +	 */
> +	if (master->drvdata->vendor_init) {
> +		ret = master->drvdata->vendor_init(master);
> +		if (ret)
> +			goto err_disable_clks;
> +	}
> +
>  	for (i = 0; i < master->num_clks; i++) {
>  		if (!strcmp(master->clks[i].id, "fast_clk"))
>  			break;
>  	}
>
> -	if (i == master->num_clks)
> -		return dev_err_probe(dev, -EINVAL,
> -				     "can't get I3C peripheral clock\n");
> +	if (i == master->num_clks) {
> +		ret = dev_err_probe(dev, -EINVAL,
> +				    "can't get I3C peripheral clock\n");
> +		goto err_disable_clks;
> +	}
>
>  	master->fclk = master->clks[i].clk;
> -	if (IS_ERR(master->fclk))
> -		return PTR_ERR(master->fclk);
> +	if (IS_ERR(master->fclk)) {
> +		ret = PTR_ERR(master->fclk);
> +		goto err_disable_clks;
> +	}
>
>  	master->irq = platform_get_irq(pdev, 0);
> -	if (master->irq < 0)
> -		return master->irq;
> -
> -	master->dev = dev;
> -	ret = clk_bulk_prepare_enable(master->num_clks, master->clks);
> -	if (ret)
> -		return dev_err_probe(dev, ret, "can't enable I3C clocks\n");
> +	if (master->irq < 0) {
> +		ret = master->irq;
> +		goto err_disable_clks;
> +	}
>
>  	mutex_init(&master->lock);
>
> @@ -2163,9 +2209,213 @@ static const struct svc_i3c_drvdata npcm845_drvdata = {
>
>  static const struct svc_i3c_drvdata svc_default_drvdata = {};
>
> +/**
> + * struct aml_i3c_ao_clk - container for the AO variant's internal fast clock
> + * @div_en: enables the internal divider
> + * @div: bus clock divider
> + * @gate: output gate of the generated fast clock
> + * @lock: spinlock shared by the gate and divider register operations
> + *
> + * The clock HW structures are registered with the clock framework and must
> + * stay alive for the whole driver lifetime, so they are kept along with the
> + * shared lock used by their gate/divider operations.
> + */
> +struct aml_i3c_ao_clk {
> +	struct clk_gate div_en;
> +	struct clk_divider div;
> +	struct clk_gate gate;
> +	spinlock_t lock;
> +};
> +
> +/**
> + * aml_ao_i3c_tail_clocks - register the AO variant's internal fast clock
> + * @master: I3C master controller
> + * @parent: parent clock of the internal divider chain; must be "pclk", the
> + *          only clock the AO variant exposes (see silvaco,i3c-master.yaml)
> + *
> + * Register the internal divider chain that produces the AO variant's
> + * fast_clk and append it to the clk bulk array as "fast_clk". The caller
> + * must run with the clocks enabled since the divider registers are
> + * accessed during registration. The appended clock is prepared and
> + * enabled here as well, because clk_bulk_prepare_enable() already ran
> + * in the probe with the original clock list.
> + *
> + * Return: 0 on success, or a negative error code.
> + */
> +static int aml_ao_i3c_tail_clocks(struct svc_i3c_master *master, struct clk *parent)
> +{
> +	struct device *dev = master->dev;
> +	struct clk_bulk_data *new_clks;
> +	struct clk_init_data init = {};
> +	struct aml_i3c_ao_clk *ao;
> +	struct clk *fclk;
> +	char name[64];
> +	int ret;
> +
> +	ao = devm_kzalloc(dev, sizeof(*ao), GFP_KERNEL);
> +	if (!ao)
> +		return -ENOMEM;
> +
> +	spin_lock_init(&ao->lock);
> +
> +	snprintf(name, sizeof(name), "%s#div_en", dev_name(dev));
> +	init.name = name;
> +	init.ops = &clk_gate_ops;
> +	init.flags = CLK_IGNORE_UNUSED;
> +	init.parent_data = &(struct clk_parent_data) {
> +		.index = -1,
> +		.hw = __clk_get_hw(parent),
> +	};
> +	init.num_parents = 1;
> +
> +	ao->div_en.reg = master->regs + AML_AO_I3C_FCLK_DIV_CTRL;
> +	ao->div_en.bit_idx = AML_AO_I3C_FCLK_DIV_EN_SHIFT;
> +	ao->div_en.hw.init = &init;
> +	ao->div_en.lock = &ao->lock;
> +
> +	ret = devm_clk_hw_register(dev, &ao->div_en.hw);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to register %s\n", name);
> +
> +	snprintf(name, sizeof(name), "%s#div", dev_name(dev));
> +	init.name = name;
> +	init.ops = &clk_divider_ops;
> +	init.flags = 0;
> +	init.parent_data = &(struct clk_parent_data) {
> +		.index = -1,
> +		.hw = &ao->div_en.hw,
> +	};
> +	init.num_parents = 1;
> +
> +	ao->div.reg = master->regs + AML_AO_I3C_FCLK_DIV_CTRL;
> +	ao->div.shift = AML_AO_I3C_FCLK_DIV_SHIFT;
> +	ao->div.width = AML_AO_I3C_FCLK_DIV_WIDTH;
> +	ao->div.hw.init = &init;
> +	ao->div.lock = &ao->lock;
> +
> +	ret = devm_clk_hw_register(dev, &ao->div.hw);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to register %s\n", name);
> +
> +	snprintf(name, sizeof(name), "%s#gate", dev_name(dev));
> +	init.name = name;
> +	init.ops = &clk_gate_ops;
> +	init.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED;
> +	init.parent_data = &(struct clk_parent_data) {
> +		.index = -1,
> +		.hw = &ao->div.hw,
> +	};
> +	init.num_parents = 1;
> +
> +	ao->gate.reg = master->regs + AML_AO_I3C_FCLK_DIV_CTRL;
> +	ao->gate.bit_idx = AML_AO_I3C_FCLK_EN_SHIFT;
> +	ao->gate.hw.init = &init;
> +	ao->gate.lock = &ao->lock;
> +
> +	ret = devm_clk_hw_register(dev, &ao->gate.hw);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to register %s\n", name);
> +
> +	fclk = devm_clk_hw_get_clk(dev, &ao->gate.hw, NULL);
> +	if (IS_ERR(fclk))
> +		return dev_err_probe(dev, PTR_ERR(fclk),
> +				     "tail i3c fast clock failed\n");
> +
> +	new_clks = devm_kcalloc(dev, master->num_clks + 1, sizeof(*new_clks),
> +				GFP_KERNEL);
> +	if (!new_clks)
> +		return -ENOMEM;
> +
> +	memcpy(new_clks, master->clks,
> +	       master->num_clks * sizeof(*master->clks));
> +	new_clks[master->num_clks].id = "fast_clk";
> +	new_clks[master->num_clks].clk = fclk;
> +	master->clks = new_clks;
> +	master->num_clks++;
> +
> +	/*
> +	 * Keep the clk bulk state balanced for the following
> +	 * clk_bulk_disable_unprepare() calls (error paths and runtime PM).
> +	 */
> +	ret = clk_prepare_enable(fclk);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "can't enable fast_clk\n");
> +
> +	return 0;
> +}
> +
> +/**
> + * aml_i3c_init - Amlogic A9 specific init, called with the clocks enabled
> + * @master: I3C master controller
> + *
> + * Register the AO variant's internal divider chain and round/set the fast
> + * clock rate for both variants.
> + *
> + * Return: 0 on success, or a negative error code.
> + */
> +static int aml_i3c_init(struct svc_i3c_master *master)
> +{
> +	struct device *dev = master->dev;
> +	struct clk *fclk;
> +	long fin_freq;
> +	int ret, i;
> +
> +	if (svc_has_quirk(master, AML_I3C_QUIRK_AO)) {
> +		/*
> +		 * clk_bulk_get_all() preserves the DT order, and the
> +		 * binding restricts the AO variant to exactly one clock,
> +		 * which is always "pclk".  So clks[0] is the input of the
> +		 * internal divider chain.
> +		 */
> +		if (strcmp(master->clks[0].id, "pclk"))
> +			return dev_err_probe(dev, -EINVAL,
> +					     "invalid AO input clock\n");
> +
> +		ret = aml_ao_i3c_tail_clocks(master, master->clks[0].clk);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	for (i = 0; i < master->num_clks; i++) {
> +		if (!strcmp(master->clks[i].id, "fast_clk"))
> +			break;
> +	}
> +
> +	if (i == master->num_clks)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "can't get I3C fast clock\n");
> +
> +	fclk = master->clks[i].clk;
> +	if (IS_ERR(fclk))
> +		return PTR_ERR(fclk);
> +
> +	fin_freq = clk_round_rate(fclk, AML_I3C_FAST_CLK_RATE);
> +	if (fin_freq <= 0)
> +		return dev_err_probe(dev, fin_freq ? fin_freq : -EINVAL,
> +				     "invalid fast_clk\n");
> +
> +	ret = clk_set_rate(fclk, fin_freq);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "setting fast_clk rate failed\n");
> +
> +	return 0;
> +}
> +
> +static const struct svc_i3c_drvdata aml_a9_drvdata = {
> +	.quirks = AML_A9_I3C_QUIRKS,
> +	.vendor_init = aml_i3c_init,
> +};
> +
> +static const struct svc_i3c_drvdata aml_a9_ao_drvdata = {
> +	.quirks = AML_A9_I3C_QUIRKS | AML_I3C_QUIRK_AO,
> +	.vendor_init = aml_i3c_init,
> +};
> +
>  static const struct of_device_id svc_i3c_master_of_match_tbl[] = {
>  	{ .compatible = "nuvoton,npcm845-i3c", .data = &npcm845_drvdata },
>  	{ .compatible = "silvaco,i3c-master-v1", .data = &svc_default_drvdata },
> +	{ .compatible = "amlogic,a9-i3c", .data = &aml_a9_drvdata },
> +	{ .compatible = "amlogic,a9-ao-i3c", .data = &aml_a9_ao_drvdata },
>  	{ /* sentinel */ },
>  };
>  MODULE_DEVICE_TABLE(of, svc_i3c_master_of_match_tbl);
>
> --
> 2.42.0
>

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

[PATCH v3 2/2] dt-bindings: i3c: silvaco,i3c-master: Add Amlogic A9 support

[Junyi Zhao via B4 Relay]

From: Junyi Zhao <junyi.zhao@amlogic.com>

Amlogic A9 SoCs integrate the Silvaco I3C master IP in two domains:
the EE-domain instance (amlogic,a9-i3c) takes "pclk" and "fast_clk"
like the NXP integration, while the AO-domain instance
(amlogic,a9-ao-i3c) lives in the Always-On domain and generates its
bus clock from a divider inside the controller's own register space,
so there is no external fast clock and only "pclk" is exposed.

The two instances differ from the vanilla Silvaco/NXP integrations in
clock topology and required init, so the driver matches them by
dedicated compatibles instead of the "silvaco,i3c-master-v1"
fallback: a fallback would bind the generic driver data, which
neither programs the fast clock rate nor handles the AO internal
divider, and the variant would then fail to work.

Add both compatibles to the schema without the fallback, following
nuvoton,npcm845-i3c, and use per-compatible if/then conditionals to
enforce the clock counts: two clocks for the EE-domain instance and
one clock for the AO-domain instance.

Signed-off-by: Junyi Zhao <junyi.zhao@amlogic.com>
---
 .../bindings/i3c/silvaco,i3c-master.yaml           | 26 ++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/i3c/silvaco,i3c-master.yaml b/Documentation/devicetree/bindings/i3c/silvaco,i3c-master.yaml
index 853092f75..d892d42be 100644
--- a/Documentation/devicetree/bindings/i3c/silvaco,i3c-master.yaml
+++ b/Documentation/devicetree/bindings/i3c/silvaco,i3c-master.yaml
@@ -13,6 +13,8 @@ properties:
   compatible:
     oneOf:
       - enum:
+          - amlogic,a9-i3c
+          - amlogic,a9-ao-i3c
           - nuvoton,npcm845-i3c
           - silvaco,i3c-master-v1
       - items:
@@ -28,14 +30,14 @@ properties:
     maxItems: 1
 
   clocks:
-    minItems: 2
+    minItems: 1
     items:
       - description: system clock
       - description: bus clock
       - description: other (slower) events clock
 
   clock-names:
-    minItems: 2
+    minItems: 1
     items:
       - const: pclk
       - const: fast_clk
@@ -70,15 +72,35 @@ allOf:
         compatible:
           contains:
             enum:
+              - amlogic,a9-i3c
               - nxp,imx94-i3c
               - nxp,imx95-i3c
     then:
       properties:
         clocks:
+          minItems: 2
           maxItems: 2
         clock-names:
+          minItems: 2
           maxItems: 2
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - amlogic,a9-ao-i3c
+    then:
+      properties:
+        clocks:
+          maxItems: 1
+          description:
+            The "ao" variant is the controller instance in the Always-On (AO)
+            domain, which has no separate bus clock and derives the
+            bus clock from an internal divider.
+        clock-names:
+          maxItems: 1
+
 unevaluatedProperties: false
 
 examples:

-- 
2.42.0



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

Re: [PATCH v3 2/2] dt-bindings: i3c: silvaco,i3c-master: Add Amlogic A9 support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Low] dt-bindings: i3c: silvaco: unwieldy per-compatible clock
  conditionals
- [Low] dt-bindings: i3c: silvaco: single-item enum and description in
  if/then
--

commit dbac4436ecc3f857ef0fa7dbe10d0fd5116cdb2d
Author: Junyi Zhao <junyi.zhao@amlogic.com>

dt-bindings: i3c: silvaco,i3c-master: Add Amlogic A9 support

This commit adds the amlogic,a9-i3c and amlogic,a9-ao-i3c compatible
strings and their per-variant clock constraints to the Silvaco I3C master
Device Tree binding schema.

> diff --git a/Documentation/devicetree/bindings/i3c/silvaco,i3c-master.yaml b/Documentation/devicetree/bindings/i3c/silvaco,i3c-master.yaml
> index 853092f7522d3..d892d42bee65c 100644
> --- a/Documentation/devicetree/bindings/i3c/silvaco,i3c-master.yaml
> +++ b/Documentation/devicetree/bindings/i3c/silvaco,i3c-master.yaml
> @@ -13,6 +13,8 @@ properties:
>    compatible:
>      oneOf:
>        - enum:
> +          - amlogic,a9-i3c
> +          - amlogic,a9-ao-i3c
>            - nuvoton,npcm845-i3c
>            - silvaco,i3c-master-v1
>        - items:
> @@ -28,14 +30,14 @@ properties:
>      maxItems: 1
>  
>    clocks:
> -    minItems: 2
> +    minItems: 1
>      items:
>        - description: system clock
>        - description: bus clock
>        - description: other (slower) events clock
>  
>    clock-names:
> -    minItems: 2
> +    minItems: 1
>      items:
>        - const: pclk
>        - const: fast_clk
> @@ -70,15 +72,35 @@ allOf:
>          compatible:
>            contains:
>              enum:
> +              - amlogic,a9-i3c
>                - nxp,imx94-i3c
>                - nxp,imx95-i3c
>      then:
>        properties:
>          clocks:
> +          minItems: 2
>            maxItems: 2
>          clock-names:
> +          minItems: 2
>            maxItems: 2

[Severity: Low]
Should the Amlogic A9 variants be split into a separate binding schema
instead of adding more per-compatible conditionals here?

Neither amlogic,a9-i3c nor amlogic,a9-ao-i3c uses the silvaco,i3c-master-v1
fallback, and accommodating the 1-clock AO variant weakens the top-level
clocks and clock-names minItems to 1 so that 3-clock, 2-clock, and 1-clock
variants all have to be constrained via separate if/then blocks in allOf.

> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - amlogic,a9-ao-i3c
> +    then:
> +      properties:
> +        clocks:
> +          maxItems: 1
> +          description:
> +            The "ao" variant is the controller instance in the Always-On (AO)
> +            domain, which has no separate bus clock and derives the
> +            bus clock from an internal divider.
> +        clock-names:
> +          maxItems: 1

[Severity: Low]
Does this new if/then block pass dt_binding_check?

The dtschema meta-schema requires enum arrays to have at least two items,
so matching the single amlogic,a9-ao-i3c string under contains needs const
rather than a single-entry enum.  Also, property descriptions belong in the
top-level properties block rather than inside an if/then conditional.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-a9_i3c-v3-0-804b69b9095b@amlogic.com?part=2

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