Re: [PATCH 3/3] i3c: master: svc: Add Amlogic A9 glue layer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] i3c: master: svc: broken probe deferral in amlogic_i3c_probe()
- [Medium] i3c: master: svc: clock enable leak in amlogic_i3c_remove()
- [Medium] i3c: master: svc: i3c-amlogic: missing I3C_AO_FCLK_DIV_CTRL restore on resume
- [Medium] i3c: master: svc: incorrect fclk rate and divider math in amlogic_i3c_probe()
--

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

i3c: master: svc: Add Amlogic A9 glue layer

This commit adds an Amlogic A9 glue driver for the Silvaco SVC I3C
controller, providing Amlogic-specific clock and runtime PM handling while
reusing the common SVC I3C core driver.

> diff --git a/drivers/i3c/master/svc/i3c-amlogic.c b/drivers/i3c/master/svc/i3c-amlogic.c
> new file mode 100644
> index 0000000000000..82300d6ae5a31
> --- /dev/null
> +++ b/drivers/i3c/master/svc/i3c-amlogic.c
> @@ -0,0 +1,250 @@

[ ... ]

> +static int default_i3c_fclk_rate_init(struct svc_i3c_master *master,
> +				      struct clk *fclk, unsigned long rate)
> +{
> +	int err;
> +
> +	err = clk_set_rate(fclk, rate);
> +	if (err)
> +		return dev_err_probe(master->dev, err,
> +				     "setting fast clock rate failed\n");
> +
> +	return 0;
> +}
> +
> +static int ao_i3c_fclk_rate_init(struct svc_i3c_master *master,
> +				 struct clk *fclk, unsigned long rate)
> +{
> +	unsigned int div_cnt, reg_val;
> +	unsigned long fin_freq = clk_get_rate(fclk);
> +
> +	if (!fin_freq)
> +		return dev_err_probe(master->dev, -EINVAL,
> +				     "can't get I3C fast clock rate\n");
> +
> +	div_cnt = div64_u64(fin_freq, rate);
> +	if (!div_cnt || div_cnt > I3C_AO_FCLK_DIV_MASK)
> +		return dev_err_probe(master->dev, -EINVAL,
> +				     "requested fast clock frequency too low\n");
> +
> +	reg_val = I3C_AO_FCLK_DIV_EN | I3C_AO_FCLK_EN | (div_cnt - 1);
> +	writel(reg_val, master->regs + I3C_AO_FCLK_DIV_CTRL);
> +
> +	return 0;
> +}
> +
> +static int amlogic_i3c_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	const struct amlogic_i3c_drvdata *data;
> +	struct svc_i3c_master *master;
> +	struct amlogic_i3c_priv *priv;
> +	struct clk *fclk;
> +	int ret, i;

[ ... ]

> +	priv->num_clks = devm_clk_bulk_get_all(dev, &priv->clks);
> +	if (priv->num_clks < 0)
> +		return dev_err_probe(dev, -EINVAL, "can't get I3C clocks\n");

[Severity: High]
Should amlogic_i3c_probe() pass priv->num_clks instead of -EINVAL to
dev_err_probe() here?

When devm_clk_bulk_get_all() returns -EPROBE_DEFER because the clock
controller has not probed yet, passing a hardcoded -EINVAL causes
dev_err_probe() to log an error and abort probing permanently instead of
deferring.

> +
> +	for (i = 0; i < priv->num_clks; i++) {
> +		if (!strcmp(priv->clks[i].id, "fast_clk"))
> +			break;
> +	}
> +
> +	if (i == priv->num_clks)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "can't get I3C peripheral clock\n");
> +
> +	if (IS_ERR(priv->clks[i].clk))
> +		return PTR_ERR(priv->clks[i].clk);
> +	fclk = priv->clks[i].clk;
> +
> +	ret = clk_bulk_prepare_enable(priv->num_clks, priv->clks);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "can't enable I3C clocks\n");
> +
> +	master->dev = dev;
> +	master->bsp_priv = priv;
> +	ret = data->fclk_rate_init(master, fclk, I3C_DEFAULT_FCLK_RATE);
> +	if (ret)
> +		goto err_disable_clks;
> +	master->fclk_rate = I3C_DEFAULT_FCLK_RATE;

[Severity: Medium]
Can the divider math in ao_i3c_fclk_rate_init() and the hardcoded
master->fclk_rate assignment in amlogic_i3c_probe() fail probe on a 24 MHz
input clock or produce an inaccurate fclk_rate?

In ao_i3c_fclk_rate_init(), div_cnt = div64_u64(fin_freq, rate) truncates via
integer division:

1. For the Always-On I3C controller, fast_clk is a9_ao_sys_i3c, which is
   gated from a9_ao_sys (muxed between the 24 MHz ao_xtal and sys). When
   fin_freq is 24 MHz, div64_u64(24000000, 25000000) evaluates to 0, causing
   ao_i3c_fclk_rate_init() to fail probe with -EINVAL.

2. When fin_freq is not an exact multiple of 25 MHz (for example, a
   166.67 MHz sys clock), div64_u64(fin_freq, rate) truncates down to 6,
   configuring I3C_AO_FCLK_DIV_CTRL for 27.78 MHz while master->fclk_rate is
   hardcoded to I3C_DEFAULT_FCLK_RATE (25 MHz). Similarly, in
   default_i3c_fclk_rate_init(), clk_set_rate() rounds to the closest rate
   supported by the clock tree rather than guaranteeing 25 MHz. When the
   actual clock rate exceeds 25 MHz, svc_i3c_master_bus_init() and
   svc_i3c_master_set_speed() compute ppbaud, odbaud, and i2cbaud assuming
   25 MHz, which can drive push-pull SCL above the 12.5 MHz MIPI I3C limit.

3. Because (div_cnt - 1) is written into the 8-bit I3C_AO_FCLK_DIV_MASK field
   (GENMASK(7, 0)), the maximum supported divider is I3C_AO_FCLK_DIV_MASK + 1
   (256), so checking div_cnt > I3C_AO_FCLK_DIV_MASK is off by one.

Should ao_i3c_fclk_rate_init() round the divider up (for example with
DIV_ROUND_UP_ULL) and update master->fclk_rate to the actual divided or
queried clock rate?

> +
> +	pm_runtime_set_autosuspend_delay(dev, SVC_I3C_PM_TIMEOUT_MS);
> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_get_noresume(dev);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);

[ ... ]

> +static void amlogic_i3c_remove(struct platform_device *pdev)
> +{
> +	struct svc_i3c_master *master = platform_get_drvdata(pdev);
> +
> +	svc_i3c_master_remove(master);
> +
> +	pm_runtime_dont_use_autosuspend(&pdev->dev);
> +	pm_runtime_disable(&pdev->dev);
> +}

[Severity: Medium]
Does amlogic_i3c_remove() leak clock prepare and enable references on
priv->clks when the device is still active at removal time?

In amlogic_i3c_probe(), clocks are acquired via devm_clk_bulk_get_all() and
manually enabled with clk_bulk_prepare_enable(priv->num_clks, priv->clks).

During removal, svc_i3c_master_remove() calls
i3c_master_unregister()->svc_i3c_master_bus_cleanup(), which resumes the
device via pm_runtime_resume_and_get() and calls
pm_runtime_put_autosuspend().

If CONFIG_PM is disabled (where runtime PM helpers are no-ops), or if runtime
suspend is prevented at removal time (for example via sysfs power/control or
because svc_i3c_master_enable_hotjoin() incremented master->dev's runtime PM
usage count without a matching put in svc_i3c_master_remove()),
pm_runtime_dont_use_autosuspend() will not invoke
amlogic_i3c_runtime_suspend().

Should amlogic_i3c_remove() check !pm_runtime_status_suspended(&pdev->dev)
after pm_runtime_disable(&pdev->dev) and call clk_bulk_disable_unprepare()
and pm_runtime_set_suspended(&pdev->dev)?

> +
> +static int __maybe_unused amlogic_i3c_runtime_suspend(struct device *dev)
> +{
> +	struct svc_i3c_master *master = dev_get_drvdata(dev);
> +	struct amlogic_i3c_priv *priv = master->bsp_priv;
> +
> +	svc_i3c_save_regs(master);
> +	clk_bulk_disable_unprepare(priv->num_clks, priv->clks);
> +	pinctrl_pm_select_sleep_state(dev);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused amlogic_i3c_runtime_resume(struct device *dev)
> +{
> +	struct svc_i3c_master *master = dev_get_drvdata(dev);
> +	struct amlogic_i3c_priv *priv = master->bsp_priv;
> +	int ret;
> +
> +	pinctrl_pm_select_default_state(dev);
> +	ret = clk_bulk_prepare_enable(priv->num_clks, priv->clks);
> +	if (ret)
> +		return ret;
> +
> +	svc_i3c_restore_regs(master);
> +
> +	return 0;
> +}

[Severity: Medium]
On "amlogic,i3c-ao-a9" devices, can I3C_AO_FCLK_DIV_CTRL lose its
configuration across suspend and resume?

During probe, ao_i3c_fclk_rate_init() writes I3C_AO_FCLK_DIV_EN,
I3C_AO_FCLK_EN, and the divider into master->regs + I3C_AO_FCLK_DIV_CTRL.

When suspending and resuming via amlogic_i3c_runtime_suspend() and
amlogic_i3c_runtime_resume() (which are also used for system sleep via
pm_runtime_force_suspend() and pm_runtime_force_resume()),
svc_i3c_save_regs() and svc_i3c_restore_regs() only save and restore
SVC_I3C_MCONFIG and SVC_I3C_MDYNADDR.

If the controller loses register state during suspend or hibernation,
wouldn't I3C_AO_FCLK_DIV_CTRL remain unconfigured on resume unless it is
saved and restored or re-initialized?

[ ... ]

> -- 
> 2.47.3

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019-a9_i3c-v1-0-329d819d621f@amlogic.com?part=3