Re: [PATCH v2 5/6] clk: add a driver for the Atheros/Qualcomm ath79 PLL controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] clk: ath79: fw_devlink blocks ath79_clk_probe() when ref
  lacks clock-frequency
- [Medium] clk: ath79: missing UPDATING wait in
  ath79_pcie_pll_unprepare()
--

commit dd79c08de0fc1914f23e0beb52d8e93809a55692
Author: Rosen Penev <rosenp@gmail.com>

clk: add a driver for the Atheros/Qualcomm ath79 PLL controller

This commit adds a common clock driver for the Atheros/Qualcomm ath79 PLL
controller, registering the reference and CPU clocks early via
CLK_OF_DECLARE_DRIVER() and the remaining clocks from a platform driver.

> diff --git a/drivers/clk/clk-ath79.c b/drivers/clk/clk-ath79.c
> new file mode 100644
> index 0000000000000..3ddbd020afb12
> --- /dev/null
> +++ b/drivers/clk/clk-ath79.c
> @@ -0,0 +1,778 @@

[ ... ]

> +static int ath79_pcie_pll_prepare(struct clk_hw *hw)
> +{
> +	struct ath79_pcie_pll *pll = to_ath79_pcie_pll(hw);
> +	u32 t;
> +	int ret;
> +
> +	/*
> +	 * Every write freezes the high speed clocks for about 60 us, so
> +	 * leave a PLL that the bootloader already started alone.
> +	 */
> +	if (ath79_pcie_pll_is_prepared(hw))
> +		return 0;
> +
> +	t = readl(pll->reg) & ~pll->pwd;
> +	writel(t, pll->reg);
> +	ret = ath79_pcie_pll_wait(pll);
> +	if (ret)
> +		return ret;
> +
> +	writel(t & ~pll->bypass, pll->reg);
> +	return ath79_pcie_pll_wait(pll);
> +}
> +
> +static void ath79_pcie_pll_unprepare(struct clk_hw *hw)
> +{
> +	struct ath79_pcie_pll *pll = to_ath79_pcie_pll(hw);
> +	u32 t;
> +
> +	t = readl(pll->reg) | pll->bypass;
> +	writel(t, pll->reg);
> +	writel(t | pll->pwd, pll->reg);
> +}

[Severity: Medium]
Should ath79_pcie_pll_unprepare() call ath79_pcie_pll_wait(pll) after each
write to pll->reg, like ath79_pcie_pll_prepare() does?  Without waiting
for pll->updating to clear, the second write is issued while the ~60 us
update from setting pll->bypass is still in progress, and the function
returns before the PLL finishes powering down.

[ ... ]

> +static void __init ath79_clk_early_init(struct device_node *np)
> +{
> +	const struct ath79_clk_soc_data *soc;
> +	unsigned long rates[ATH79_OUT_NUM];
> +	struct ath79_clk_regs regs = {};
> +	struct clk_hw *ref, *cpu;
> +	struct clk *ref_clk;
> +	int ret, i;
> +
> +	soc = of_match_node(ath79_clk_of_match, np)->data;
> +
> +	/*
> +	 * The board's crystal. Old device trees leave its rate out, and the
> +	 * arch code then registers it from the bootstrap pins as "ref".
> +	 */
> +	ref_clk = of_clk_get_by_name(np, "ref");
> +	if (IS_ERR(ref_clk))
> +		ref_clk = clk_get_sys(NULL, "ref");

[ ... ]

> +static int ath79_clk_probe(struct platform_device *pdev)
> +{

[Severity: Medium]
Does fw_devlink permanently block ath79_clk_probe() with -EPROBE_DEFER on
device trees where the "ref" fixed-clock node omits clock-frequency?

When the "ref" node has no clock-frequency property, _of_fixed_clk_setup()
fails with -EIO before of_clk_add_hw_provider() can mark its fwnode
initialized, while of_clk_init() still sets OF_POPULATED on the node so no
platform device is created for it.  Because the PLL node has clocks =
<&ref>, fw_devlink creates a fwnode_link to the uninitialized "ref" node,
and device_links_check_suppliers() rejects probing ath79_clk_driver with
-EPROBE_DEFER, leaving the DDR, AHB, MDIO, UART1 and PCIe clocks
permanently at ERR_PTR(-EPROBE_DEFER).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110013646.1582970-1-rosenp@gmail.com?part=5