Re: [PATCH v2 2/2] clk: samsung: acpm: add Exynos850 support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] clk: samsung: acpm: broken rate tracking and missing CLK_SET_RATE_PARENT
- [High] clk: samsung: acpm: missing of_node causes orphaned clocks and missing DT provider
--

commit c667cadd8b070f358391c7469c777edfe81f6617
Author: Alexey Klimov <alexey.klimov@linaro.org>

clk: samsung: acpm: add Exynos850 support

This commit adds Exynos850 support to the Samsung ACPM clock driver. It
introduces a SoC-specific get_rate() callback because the Exynos850 ACPM
firmware doesn't implement a dynamic rate read call. When get_rate() is
set, it populates pdata.fw_name to link the parent clock.

> diff --git a/drivers/clk/samsung/clk-acpm.c b/drivers/clk/samsung/clk-acpm.c
> index 4b68b44fe6158..9f46fbdd999d4 100644
> --- a/drivers/clk/samsung/clk-acpm.c
> +++ b/drivers/clk/samsung/clk-acpm.c
[ ... ]
> @@ -58,6 +73,19 @@ static const struct acpm_clk_variant gs101_acpm_clks[] = {
>  	ACPM_CLK("bo"),
>  };
>  
> +static unsigned long bypass_acpm_exynos850_get_rate(struct clk_hw *hw,
> +						    unsigned long parent_rate)
> +{
> +	return parent_rate;
> +}

[Severity: High]
If acpm_clk_set_rate() alters the hardware frequency via IPC, doesn't blindly
returning the parent rate here break the clock framework's rate tracking?

Since the clock lacks the CLK_SET_RATE_PARENT flag, the parent's cached rate
isn't updated when the rate is set. It seems clk_get_rate() will return the
stale parent rate even after a successful clk_set_rate().

> +
> +static const struct acpm_clk_driver_data acpm_clk_exynos850 = {
[ ... ]
> @@ -89,14 +120,23 @@ static const struct clk_ops acpm_clk_ops = {
>  };
>  
>  static int acpm_clk_register(struct device *dev, struct acpm_clk *aclk,
> -			     const char *name)
> +			     const char *name, const struct acpm_clk_driver_data *drv_data)
>  {
>  	struct clk_init_data init = {};
> +	struct clk_parent_data pdata = {};
>  
>  	init.name = name;
>  	init.ops = &acpm_clk_ops;
>  	aclk->hw.init = &init;
>  
> +	/* If get_rate is set, the SoC relies on parent topology */
> +	if (drv_data->get_rate) {
> +		pdata.fw_name = name;
> +		init.parent_data = &pdata;
> +		init.num_parents = 1;
> +		init.flags = CLK_GET_RATE_NOCACHE;
> +	}

[Severity: High]
Since the acpm-clocks platform device is manually created by the parent driver
and lacks an of_node, will setting pdata.fw_name fail to resolve the parent?

When core->of_node inherits NULL from dev->of_node, it appears clk_core_get()
skips the of_parse_clkspec() lookup and falls back to legacy clkdev, which
fails. Could this result in permanently orphaned clocks where the parent rate
evaluates to 0?

Also, is CLK_SET_RATE_PARENT needed here to propagate rate changes upstream?

> +
>  	return devm_clk_hw_register(dev, &aclk->hw);
>  }
[ ... ]
> @@ -145,13 +185,14 @@ static int acpm_clk_probe(struct platform_device *pdev)
[ ... ]
> -		err = acpm_clk_register(dev, aclk, drv_data->clks[i].name);
> +		err = acpm_clk_register(dev, aclk, drv_data->clks[i].name, drv_data);
>  		if (err)
>  			return dev_err_probe(dev, err,
>  					     "Failed to register clock\n");
>  	}
>  
>  	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
>  					   clk_data);
>  }

[Severity: High]
Does this fail to expose the clock provider to Device Tree consumers?

If dev->of_node is NULL, devm_of_clk_add_hw_provider() will silently return 0
without actually registering the hardware provider.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260915-exynos850-acpm-clk-v2-0-ffad4a6b3fb1@linaro.org?part=2