When it comes to clocks with parents the SCMI clk driver assumes that
parents are always initialized before their children which might not
always be the case.

During initialization of the parent_data array we have:

	sclk->parent_data[i].hw = hws[sclk->info->parents[i]];

hws[sclk->info->parents[i]] will not yet be initialized when children
are encountered before their possible parents. Solve this by allocating
all struct scmi_clk as an array first and populating all hws[] upfront.

Fixes: 65a8a3dd3b95f ("clk: scmi: Add support for clock {set,get}_parent")
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/clk/clk-scmi.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
index 15510c2ff21c0335f5cb30677343bd4ef59c0738..f258ad7dda73e3c50c3ce567a8e22b3d2ec9836b 100644
--- a/drivers/clk/clk-scmi.c
+++ b/drivers/clk/clk-scmi.c
@@ -404,6 +404,7 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
 	const struct scmi_handle *handle = sdev->handle;
 	struct scmi_protocol_handle *ph;
 	const struct clk_ops *scmi_clk_ops_db[SCMI_MAX_CLK_OPS] = {};
+	struct scmi_clk *sclks;
 
 	if (!handle)
 		return -ENODEV;
@@ -430,18 +431,24 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
 	transport_is_atomic = handle->is_transport_atomic(handle,
 							  &atomic_threshold_us);
 
+	sclks = devm_kcalloc(dev, count, sizeof(*sclks), GFP_KERNEL);
+	if (!sclks)
+		return -ENOMEM;
+
 	for (idx = 0; idx < count; idx++) {
-		struct scmi_clk *sclk;
-		const struct clk_ops *scmi_ops;
+		struct scmi_clk *sclk = &sclks[idx];
 
-		sclk = devm_kzalloc(dev, sizeof(*sclk), GFP_KERNEL);
-		if (!sclk)
-			return -ENOMEM;
+		hws[idx] = &sclk->hw;
+	}
+
+	for (idx = 0; idx < count; idx++) {
+		struct scmi_clk *sclk = &sclks[idx];
+		const struct clk_ops *scmi_ops;
 
 		sclk->info = scmi_proto_clk_ops->info_get(ph, idx);
 		if (!sclk->info) {
 			dev_dbg(dev, "invalid clock info for idx %d\n", idx);
-			devm_kfree(dev, sclk);
+			hws[idx] = NULL;
 			continue;
 		}
 
@@ -479,13 +486,11 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
 		if (err) {
 			dev_err(dev, "failed to register clock %d\n", idx);
 			devm_kfree(dev, sclk->parent_data);
-			devm_kfree(dev, sclk);
 			hws[idx] = NULL;
 		} else {
 			dev_dbg(dev, "Registered clock:%s%s\n",
 				sclk->info->name,
 				scmi_ops->enable ? " (atomic ops)" : "");
-			hws[idx] = &sclk->hw;
 		}
 	}
 

---
base-commit: 5abc7438f1e9d62e91ad775cc83c9594c48d2282
change-id: 20250604-clk-scmi-children-parent-fix-45a8912b8ba0

Best regards,
-- 
Sascha Hauer <s.hauer@pengutronix.de>

On Wed, Jun 04, 2025 at 01:00:30PM +0200, Sascha Hauer wrote:
>When it comes to clocks with parents the SCMI clk driver assumes that
>parents are always initialized before their children which might not
>always be the case.
>
>During initialization of the parent_data array we have:
>
>	sclk->parent_data[i].hw = hws[sclk->info->parents[i]];
>
>hws[sclk->info->parents[i]] will not yet be initialized when children
>are encountered before their possible parents. Solve this by allocating
>all struct scmi_clk as an array first and populating all hws[] upfront.
>
>Fixes: 65a8a3dd3b95f ("clk: scmi: Add support for clock {set,get}_parent")
>Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Thanks for the fix. To i.MX, the index of child clk is always after
parent clk per current SM design. Not sure you met real issue, anyway
this patch itself is correct.

Reviewed-by: <peng.fan@nxp.com>

On Thu, Jun 12, 2025 at 11:47:23AM +0800, Peng Fan wrote:
> On Wed, Jun 04, 2025 at 01:00:30PM +0200, Sascha Hauer wrote:
> >When it comes to clocks with parents the SCMI clk driver assumes that
> >parents are always initialized before their children which might not
> >always be the case.
> >
> >During initialization of the parent_data array we have:
> >
> >	sclk->parent_data[i].hw = hws[sclk->info->parents[i]];
> >
> >hws[sclk->info->parents[i]] will not yet be initialized when children
> >are encountered before their possible parents. Solve this by allocating
> >all struct scmi_clk as an array first and populating all hws[] upfront.
> >
> >Fixes: 65a8a3dd3b95f ("clk: scmi: Add support for clock {set,get}_parent")
> >Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> 
> Thanks for the fix. To i.MX, the index of child clk is always after
> parent clk per current SM design. Not sure you met real issue, anyway
> this patch itself is correct.

I had issues on TI AM62L (not yet upstream).

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

On Wed, Jun 04, 2025 at 01:00:30PM +0200, Sascha Hauer wrote:
> When it comes to clocks with parents the SCMI clk driver assumes that
> parents are always initialized before their children which might not
> always be the case.
> 

Hi,

> During initialization of the parent_data array we have:
> 
> 	sclk->parent_data[i].hw = hws[sclk->info->parents[i]];
> 
> hws[sclk->info->parents[i]] will not yet be initialized when children
> are encountered before their possible parents. Solve this by allocating
> all struct scmi_clk as an array first and populating all hws[] upfront.
> 

Yes indeed, good catch.
Thanks for this.

Just one minor nitpick down below.

> Fixes: 65a8a3dd3b95f ("clk: scmi: Add support for clock {set,get}_parent")
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/clk/clk-scmi.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
> index 15510c2ff21c0335f5cb30677343bd4ef59c0738..f258ad7dda73e3c50c3ce567a8e22b3d2ec9836b 100644
> --- a/drivers/clk/clk-scmi.c
> +++ b/drivers/clk/clk-scmi.c
> @@ -404,6 +404,7 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
>  	const struct scmi_handle *handle = sdev->handle;
>  	struct scmi_protocol_handle *ph;
>  	const struct clk_ops *scmi_clk_ops_db[SCMI_MAX_CLK_OPS] = {};
> +	struct scmi_clk *sclks;
>  
>  	if (!handle)
>  		return -ENODEV;
> @@ -430,18 +431,24 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
>  	transport_is_atomic = handle->is_transport_atomic(handle,
>  							  &atomic_threshold_us);
>  
> +	sclks = devm_kcalloc(dev, count, sizeof(*sclks), GFP_KERNEL);
> +	if (!sclks)
> +		return -ENOMEM;
> +
>  	for (idx = 0; idx < count; idx++) {
> -		struct scmi_clk *sclk;
> -		const struct clk_ops *scmi_ops;
> +		struct scmi_clk *sclk = &sclks[idx];

...do we really need this intermediate local variable in this initializarion loop ?
...doesnt feel like giving more readability (even though the compiler will probably
kill it anyway...)
  
> -		sclk = devm_kzalloc(dev, sizeof(*sclk), GFP_KERNEL);
> -		if (!sclk)
> -			return -ENOMEM;
> +		hws[idx] = &sclk->hw;

....cant we just

  	for (idx = 0; idx < count; idx++)
		hws[idx] = &sclks[idx].hw;


Other than this, LGTM.
Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>

Thanks,
Cristian