Re: [PATCH v2 06/24] clk: zte: Add ZX279128S CRM clock and reset driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] clk: zte: boot failure when external oscillator is an orphan
- [Medium] clk: zte: incomplete disable and is_enabled implementation for CRM2 clocks
- [Low] clk: zte: unmet dependency risk from unconditional select of RESET_SIMPLE
--

commit 46157610926015842fd1aee202e8fff3cbf39169
Author: Navid Ghahremani <ghahramani.navid@gmail.com>

clk: zte: Add ZX279128S CRM clock and reset driver

This commit adds support for the top CRM and low-speed peripheral clock
controllers for the ZTE ZX279128S SoC. It implements custom reset controller
operations and provides CRM2 clock gating along with reset controller support
for PCIe.

> diff --git a/drivers/clk/zte/Kconfig b/drivers/clk/zte/Kconfig
> new file mode 100644
> index 0000000000000..92fd27ea42b2f
> --- /dev/null
> +++ b/drivers/clk/zte/Kconfig
> @@ -0,0 +1,14 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config CLK_ZX279128S
> +	bool "ZTE zx279128s clock driver"
> +	depends on SOC_ZX279128S || COMPILE_TEST
> +	depends on OF
> +	default SOC_ZX279128S
> +	select RESET_CONTROLLER
> +	select RESET_SIMPLE

[Severity: Low]
Does unconditionally selecting RESET_SIMPLE cause an unmet dependency
warning on platforms without HAS_IOMEM (like UML)?

Since the driver implements its own custom reset operations (zx_reset_ops,
zx_crm2_reset_ops) and doesn't use the reset-simple framework, is it possible
to drop the select RESET_SIMPLE entirely?

[ ... ]

> diff --git a/drivers/clk/zte/clk-zx279128s.c b/drivers/clk/zte/clk-zx279128s.c
> new file mode 100644
> index 0000000000000..892e0c22e5f26
> --- /dev/null
> +++ b/drivers/clk/zte/clk-zx279128s.c

[ ... ]

> +static void __init zx279128s_topcrm_init(struct device_node *np)
> +{
> +	const struct clk_hw *cpu_parents[4], *matrix_parents[4];
> +	struct clk_hw *osc, *lsp, *lsp_500m, *lsp_250m, *lsp_125m, *lsp_100m;
> +	struct clk_hw *audio, *audio_32k, *matrix, *hclk, *pclk, *m200, *m20;
> +	struct clk_hw_onecell_data *data;
> +	struct clk_hw **hws;
> +	void __iomem *base, *en;
> +	int i;
> +
> +	base = of_iomap(np, 0);
> +	if (!base) {
> +		pr_err("%pOF: no registers\n", np);
> +		return;
> +	}
> +
> +	data = kzalloc(struct_size(data, hws, TOP_NUM_CLKS), GFP_KERNEL);
> +	if (!data)
> +		return;
> +	data->num = TOP_NUM_CLKS;
> +	hws = data->hws;
> +
> +	hws[ZX279128S_TOP_PLL_A9] = zx_top_pll(np, "pll_a9", base + TOP_PLL_A9);
> +	if (IS_ERR(hws[ZX279128S_TOP_PLL_A9]))
> +		goto err;
> +	osc = clk_hw_get_parent(hws[ZX279128S_TOP_PLL_A9]);
> +	if (!osc)
> +		goto err;

[Severity: High]
Does treating a missing parent as a fatal error here break the boot process
if the external "osc" clock is not yet registered?

If the external "osc" clock is an orphan due to probe sequence or driver
loading order, clk_hw_get_parent() will return NULL. Treating this as an error
silently aborts the initialization of all CRM clocks.

Could we instead use fw_name = "osc" (or other suitable clk_parent_data
constructs) for downstream clocks, allowing the Common Clock Framework
to handle orphan resolution naturally?

[ ... ]

> +static void zx_crm2_clk_disable(struct clk_hw *hw)
> +{
> +	struct zx_crm2_gate *gate = to_zx_crm2_gate(hw);
> +	unsigned long flags;
> +	u32 val;
> +
> +	spin_lock_irqsave(gate->lock, flags);
> +	val = readl(gate->reg);
> +	val &= ~gate->clk_on;
> +	writel(val, gate->reg);

[Severity: Medium]
In zx_crm2_clk_enable(), the gate->clk_off bits are cleared to enable the
clock. Should zx_crm2_clk_disable() restore those clk_off bits when disabling?

By only clearing the clk_on bits, this might leave the hardware partially
active or fail to properly isolate the clock domain.

> +	spin_unlock_irqrestore(gate->lock, flags);
> +}
> +
> +static int zx_crm2_clk_is_enabled(struct clk_hw *hw)
> +{
> +	struct zx_crm2_gate *gate = to_zx_crm2_gate(hw);
> +	u32 val = readl(gate->reg);
> +
> +	return (val & gate->clk_on) == gate->clk_on;

[Severity: Medium]
Does this status check need to account for the gate->clk_off bits as well?

If the clock is currently in a disabled state (e.g., clk_off bits set by the
bootloader), this function will report it as enabled if the clk_on bits happen
to be set. This would prevent the Common Clock Framework from ever calling
enable() to clear those isolation bits.

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014072109.1759502-1-ghahramani.navid@gmail.com?part=6