RE: [PATCH v5 3/3] clk: imx: imx8qxp: add soft dependency on SCU power domain driver

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to
> consider:
> - [High] Relying on MODULE_SOFTDEP for driver probe ordering is an
> anti-pattern that fails to guarantee resource availability and is completely
> ineffective in built-in/module configurations.
> --
> 
> --- Patch [3]: [PATCH v5 3/3] clk: imx: imx8qxp: add soft dependency on SCU
> power domain driver --- commit
> b23cf3ab8ab04f852f48d1fd72b4b4fa914a8cd0
> Author: Zhipeng Wang <[email protected]>
> 
> clk: imx: imx8qxp: add soft dependency on SCU power domain driver
> 
> This commit adds a MODULE_SOFTDEP to ensure the SCU power domain
> module loads before the clock module. This is intended to work around
> failures when attaching power domains to clock devices before the provider
> has probed.
> 
> > diff --git a/drivers/clk/imx/clk-imx8qxp.c
> > b/drivers/clk/imx/clk-imx8qxp.c index c781425a005ef..17302ee628b76
> > 100644
> > --- a/drivers/clk/imx/clk-imx8qxp.c
> > +++ b/drivers/clk/imx/clk-imx8qxp.c
> > @@ -373,3 +373,4 @@ module_exit(imx8qxp_clk_exit);
> > MODULE_AUTHOR("Aisheng Dong <[email protected]>");
> > MODULE_DESCRIPTION("NXP i.MX8QXP clock driver");
> MODULE_LICENSE("GPL
> > v2");
> > +MODULE_SOFTDEP("pre: scu_pd");
> 
> [Severity: High]
> Does this softdep approach fully resolve the issue if one driver is built-in and
> the other is a module?
> 
> Relying on module load ordering doesn't guarantee resource availability if the
> kernel is configured with CONFIG_CLK_IMX8QXP=y and
> CONFIG_IMX_SCU_PD=m, or if the power domain provider simply defers its
> probe. If the power domains are not attached, the clocks are still registered
> and exposed.
> Accessing them without an active power domain can lead to hardware hangs.
> 
> Looking at drivers/clk/imx/clk-scu.c, it appears the root of the problem is that
> imx_clk_scu_alloc_dev() ignores the error instead of propagating it:
> 
> imx_clk_scu_alloc_dev()
>     ...
>     ret = imx_clk_scu_attach_pd(&pdev->dev, rsrc_id);
>     if (ret)
>         pr_warn("%s: failed to attached the power domain %d\n", name, ret);
>     ...
> 
> Would it be safer to propagate the error (like -EPROBE_DEFER) when
> imx_clk_scu_attach_pd() fails, rather than registering the clock hardware in a
> broken state and relying on module load order?
> 
> --
> Sashiko AI
> review · https://sashiko.dev/#/patchset/20260723020524.1886806-1-Zhipen
> [email protected]?part=3

The i.MX8QXP SCU clock driver has a non-standard architecture where
~120 clock platform devices are dynamically created via
platform_device_alloc() in imx_clk_scu_alloc_dev(). These devices
have no of_node, so fw_devlink cannot establish dependencies
automatically (unlike e.g. Qualcomm GCC which declares power-domains
in DT and gets automatic probe ordering via fw_devlink).

Regarding CONFIG_CLK_IMX8QXP=y + CONFIG_IMX_SCU_PD=m: when
IMX_SCU_PD is built as a module and CLK_IMX8QXP is built-in, the
clock driver probes at device_initcall before the power domain
module is loaded. However, this mixed configuration is not a
practical deployment - in GKI both are modules, and in traditional
kernels both are built-in. The subsys_initcall() in patch 2 covers
the built-in case, and MODULE_SOFTDEP covers the module case.

Propagating -EPROBE_DEFER from imx_clk_scu_attach_pd() would require
reworking the error handling across all 120 imx_clk_scu() call sites
in the probe function, which currently ignore the return value. This
is a larger architectural change better suited for a separate series.

MODULE_SOFTDEP is consistent with the approach used by Qualcomm's
gcc-sdm845 (commit 1d9054e3a4fd) for the same class of problem.

BRs,
Zhipeng