Re: [PATCH v4 2/8] clk: renesas: r8a78000: Add SCMI remapping support

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Geert,

Thank you for the patch.

On Fri, Oct 16, 2026 at 05:55:41PM +0200, Geert Uytterhoeven wrote:
> Extend the Clock Pulse Generator driver for R-Car X5H with SCMI support,
> to remap DT clock IDs to abstractions (clocks) offered by the SCMI
> firmware when running on a system with SCMI, and SCMI support is
> enabled.  Take into account some peculiarities of the current SCMI
> firmware implementations, and fall-back to fixed-rate clocks as
> replacements for SCMI clocks that are not available.
> Support for bus clocks used by serial, I2C, and SPI devices is included.
> 
> This lets us avoid putting SCMI IDs in DT, and allows us to use the same
> hardware description in SoC DTS for systems with and without SCMI
> firmware.
> 
> For now, it is assumed SCMI clock IDs are stable, and match SCP FW
> SDKv4.31.0 and later.
> 
> Suggested-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v4:
>   - Drop unneeded IS_ERR() check after __clk_get_hw(),
>   - Mark scmi_clk_np __free(device_node),
>   - Assume all SCMI clock IDs are stable, and drop support for handling
>     different ID spaces,
>   - Include initial mapping table for SCP FW SDKv4.31.0 and later,
>   - Try SCMI clocks first, and fall back to the default fixed-frequency
>     clocks when not available, as recent firmware versions no longer
>     restrict access to some bus clocks,
>   - Drop renesas,scmi-firmware support, just find an arm,scmi node
>     instead,
>   - Fall back to the non-SCMI defaults when SCMI clock support is
>     disabled,
> 
> v3:
>   - Rebase,
>   - Consistently use /* sentinel */ for zero sentinels,
>   - s/firmware/renesas,scmi-firmware/,
>   - Reword,
> 
> v2:
>   - Spin off from "clk: renesas: Add R-Car X5H CPG SCMI remapping
>     driver",
>   - Use reverse xmas ordering in r8a78000_cpg_probe(),
>   - Propagate error codes from scmi_get_base_info() now it can return
>     -EPROBE_DEFER,
>   - Improve error message on scmi_get_base_info() failure,
>   - Check if SCMI firmware node is available,
>   - Replace the scmi_clk_np member of struct r8a78000_cpg_priv by a
>     local variable in r8a78000_cpg_probe(), and pass it to
>     fill_clk_map(),
>   - Join split short lines,
>   - Fall back to the default support when SCMI support or firmware is
>     not available, or when the detected SCMI firmware version is not
>     supported by the driver,
>   - Drop mapping tables.
> ---
>  drivers/clk/renesas/r8a78000-cpg.c | 149 ++++++++++++++++++++++++++---
>  1 file changed, 138 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/clk/renesas/r8a78000-cpg.c b/drivers/clk/renesas/r8a78000-cpg.c
> index 5ffd7b89cb48e41c..758cfe78ae58e0fb 100644
> --- a/drivers/clk/renesas/r8a78000-cpg.c
> +++ b/drivers/clk/renesas/r8a78000-cpg.c
> @@ -5,6 +5,7 @@
>   * Copyright (C) 2026 Glider bv
>   */
>  
> +#include <linux/cleanup.h>
>  #include <linux/clk.h>
>  #include <linux/clk-provider.h>
>  #include <linux/dev_printk.h>
> @@ -12,21 +13,27 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> +#include <linux/scmi_protocol.h>
>  #include <linux/slab.h>
>  
>  #include <dt-bindings/clock/renesas,r8a78000-cpg.h>
>  
>  struct clk_map_in {
>  	int dt_id;		/* DT binding clock ID or -1 sentinel */
> -	u32 fw_id;		/* FIXED_CLK() ID */
> +	u32 fw_id;		/* SCMI firmware clock ID or FIXED_CLK() ID */
>  };
>  
>  struct clk_map {
>  	int dt_id;		/* DT binding clock ID or -1 sentinel */
> -	u32 fw_id;		/* FIXED_CLK() ID */
> +	u32 fw_id;		/* SCMI firmware clock ID or FIXED_CLK() ID */
>  	struct clk_hw *hw;
>  };
>  
> +struct cpg_data {
> +	const struct clk_map_in *default_map;
> +	const struct clk_map_in *fw_map;
> +};
> +
>  enum fixed_clk {
>  	FIXED_CLK_66M,
>  	FIXED_CLK_133M,
> @@ -39,13 +46,14 @@ static const unsigned long fixed_clk_rates[] = {
>  	[FIXED_CLK_266M] = 266660000,
>  };
>  
> -#define FIXED_CLK(rate)		FIXED_CLK_ ## rate
> +#define FIXED_CLK_OFFSET	0x80000000
> +#define FIXED_CLK(rate)		FIXED_CLK_OFFSET + FIXED_CLK_ ## rate

#define FIXED_CLK(rate)		(FIXED_CLK_OFFSET + FIXED_CLK_ ## rate)

>  
>  /**
>   * struct r8a78000_cpg_priv - Clock Pulse Generator Private Data
>   *
>   * @dev: CPG device
> - * @map: Mapping from DT clock IDs to fixed-rate clocks
> + * @map: Mapping from DT clock IDs to SCMI or fixed-rate clocks
>   */
>  struct r8a78000_cpg_priv {
>  	struct device *dev;
> @@ -85,8 +93,19 @@ static struct clk_hw *r8a78000_clk_get(struct of_phandle_args *spec,
>  		return ERR_PTR(-ENOENT);
>  	}
>  
> -	dev_dbg(dev, "Mapping DT clock %u to fixed clock %u\n", id, map->fw_id);
> +	if (map->fw_id < FIXED_CLK_OFFSET)
> +		dev_dbg(dev, "Mapping DT clock %u to SCMI clock %u\n", id,
> +			map->fw_id);
> +	else
> +		dev_dbg(dev, "Mapping DT clock %u to fixed clock %u\n", id,
> +			 map->fw_id - FIXED_CLK_OFFSET);
> +
>  	hw = map->hw;
> +	if (!hw) {
> +		/* CLOCK_ATTRIBUTES is not supported */
> +		dev_err(dev, "Clock %u is not available\n", id);
> +		return ERR_PTR(-ENOENT);
> +	}
>  
>  	dev_dbg(dev, "clock %u is %s at %lu Hz\n", id, clk_hw_get_name(hw),
>  		clk_hw_get_rate(hw));
> @@ -94,6 +113,21 @@ static struct clk_hw *r8a78000_clk_get(struct of_phandle_args *spec,
>  	return hw;
>  }
>  
> +static struct device_node *scmi_find_proto(struct device_node *scmi, u32 proto)

This is the kind of function name that looks prone to a namespace clash.
I'd name it r8a78000_clk_scmi_find_proto().

> +{
> +	for_each_available_child_of_node_scoped(scmi, child) {
> +		u32 reg;
> +
> +		if (of_property_read_u32(child, "reg", &reg))
> +			continue;
> +
> +		if (reg == proto)
> +			return_ptr(child);
> +	}
> +
> +	return NULL;
> +}
> +
>  static struct clk_hw **register_fixed_clks(struct device *dev)
>  {
>  	unsigned int n = ARRAY_SIZE(fixed_clk_rates);
> @@ -122,11 +156,16 @@ static struct clk_hw **register_fixed_clks(struct device *dev)
>  }
>  
>  static const struct clk_map *fill_clk_map(struct r8a78000_cpg_priv *priv,
> -					  const struct clk_map_in *map_in)
> +					  const struct clk_map_in *map_in,
> +					  const struct clk_map_in *fallback,
> +					  struct device_node *scmi_clk_np)
>  {
> +	struct of_phandle_args scmi_spec;
>  	struct device *dev = priv->dev;
>  	struct clk_hw **fixed_hws;
>  	struct clk_map *map;
> +	struct clk_hw *hw;
> +	struct clk *clk;
>  	unsigned int i;
>  
>  	fixed_hws = register_fixed_clks(dev);
> @@ -145,7 +184,46 @@ static const struct clk_map *fill_clk_map(struct r8a78000_cpg_priv *priv,
>  			break;
>  
>  		map[i].fw_id = map_in[i].fw_id;
> -		map[i].hw = fixed_hws[map[i].fw_id];
> +		if (map[i].fw_id >= FIXED_CLK_OFFSET) {
> +			enum fixed_clk idx = map[i].fw_id - FIXED_CLK_OFFSET;
> +
> +			map[i].hw = fixed_hws[idx];
> +			continue;
> +		}
> +
> +		scmi_spec.np = scmi_clk_np;

This seems to be safe because a NULL scmi_clk_np implies usage of the
default map, which only contains fixed clock offsets, but that makes the
code a bit too fragile for my taste.

> +		scmi_spec.args_count = 1;
> +		scmi_spec.args[0] = map[i].fw_id;
> +
> +		clk = of_clk_get_from_provider(&scmi_spec);
> +		if (IS_ERR(clk))
> +			return dev_err_cast_probe(dev, clk,
> +				"Failed to get SCMI clock %u\n", map[i].fw_id);
> +
> +		hw = __clk_get_hw(clk);
> +		if (!hw && fallback != map_in) {

And if the above is correct, fallback should always be different from
map_in when fw_id corresponds to an SCMI clock.

What issue does this catch ? Should the fw map contain a fixed clock
entry for clocks not available in SCMI ?

> +			for (unsigned int j = 0; fallback[j].dt_id >= 0; j++) {
> +				if (fallback[j].dt_id == map_in[i].dt_id) {
> +					enum fixed_clk idx = fallback[j].fw_id - FIXED_CLK_OFFSET;
> +
> +					map[i].hw = fixed_hws[idx];
> +					break;
> +				}
> +			}
> +			if (map[i].hw)
> +				continue;
> +		}
> +
> +		if (!hw) {
> +			/* CLOCK_ATTRIBUTES is not supported */
> +			dev_warn(dev, "SCMI clock %u is NULL\n", map[i].fw_id);
> +			continue;
> +		}
> +
> +		dev_dbg(priv->dev, "SCMI clock %u is %s at %lu Hz\n",
> +			map[i].fw_id, clk_hw_get_name(hw), clk_hw_get_rate(hw));
> +
> +		map[i].hw = hw;
>  	}
>  
>  	devm_kfree(dev, fixed_hws);
> @@ -154,20 +232,55 @@ static const struct clk_map *fill_clk_map(struct r8a78000_cpg_priv *priv,
>  
>  static int r8a78000_cpg_probe(struct platform_device *pdev)
>  {
> +	struct device_node *scmi_clk_np __free(device_node) = NULL;
> +	struct device_node *scmi __free(device_node) = NULL;
>  	struct device *dev = &pdev->dev;
> +	const struct cpg_data *cpg_data;
>  	struct r8a78000_cpg_priv *priv;
>  	const struct clk_map_in *map;
>  
> -	map = of_device_get_match_data(dev);
> -	if (!map)
> +	cpg_data = of_device_get_match_data(dev);
> +	if (!cpg_data)
>  		return -ENODEV;
>  
> +	map = cpg_data->default_map;
> +
>  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
>  		return -ENOMEM;
>  
>  	priv->dev = dev;
> -	priv->map = fill_clk_map(priv, map);
> +
> +	if (!IS_ENABLED(CONFIG_COMMON_CLK_SCMI)) {
> +		dev_dbg(dev, "SCMI clock support is disabled\n");
> +		goto fallback;
> +	}
> +
> +	scmi = of_find_compatible_node(NULL, NULL, "arm,scmi");
> +	if (!scmi) {
> +		dev_dbg(dev, "Cannot find SCMI firmware node\n");
> +		goto fallback;
> +	}
> +
> +	if (!of_device_is_available(scmi)) {
> +		dev_dbg(dev, "SCMI firmware node is not available\n");
> +		goto fallback;
> +	}
> +
> +	scmi_clk_np = scmi_find_proto(scmi, SCMI_PROTOCOL_CLOCK);
> +	if (!scmi_clk_np) {
> +		dev_dbg(dev, "Cannot find SCMI clock management protocol\n");
> +		goto fallback;
> +	}
> +
> +	map = cpg_data->fw_map;
> +
> +fallback:
> +	/*
> +	 * We cannot do lazy look-up in r8a78000_clk_get(), as that function is
> +	 * called with of_clk_mutex already held.
> +	 */
> +	priv->map = fill_clk_map(priv, map, cpg_data->default_map, scmi_clk_np);
>  	if (IS_ERR(priv->map))
>  		return PTR_ERR(priv->map);
>  
> @@ -181,10 +294,24 @@ static const struct clk_map_in r8a78000_cpg_default[] = {
>  	{ -1 }
>  };
>  
> +static const struct clk_map_in r8a78000_cpg_fw[] = {
> +	{ R8A78000_CPG_SGASYNCD4_PERW_BUS,	1661 },
> +	{ R8A78000_CPG_SGASYNCD8_PERW_BUS,	1662 },
> +	{ R8A78000_CPG_SGASYNCD16_PERW_BUS,	1663 },
> +	{ R8A78000_CPG_MSO_SCP_MAIN,		1459 },
> +	{ R8A78000_CPG_MSOCK_PERW_BUS,		1667 },
> +	{ -1 }
> +};
> +
> +static const struct cpg_data r8a78000_cpg_data = {
> +	.default_map = r8a78000_cpg_default,
> +	.fw_map = r8a78000_cpg_fw,
> +};
> +
>  static const struct of_device_id r8a78000_cpg_match[] = {
>  	{
>  		.compatible = "renesas,r8a78000-cpg",
> -		.data = &r8a78000_cpg_default,
> +		.data = &r8a78000_cpg_data,
>  	},
>  	{ /* sentinel */ }
>  };

-- 
Regards,

Laurent Pinchart