Re: [PATCH 4/7] drivers: clk: add support for RPMI clocks

[Yao Zi <me@ziyao.cc>]

On Fri, Jun 26, 2026 at 01:15:45PM -0700, Charles Perry wrote:
> The RISC-V Platform Management Interface (RPMI) defines a service group
> for control and monitoring of clocks [1]. This can be exposed as a
> UCLASS_CLK driver.
> 
> [1]: https://github.com/riscv-non-isa/riscv-rpmi (chapter 4.8)
> 
> Signed-off-by: Charles Perry <charles.perry@microchip.com>
> ---
>  MAINTAINERS            |   1 +
>  drivers/clk/Kconfig    |   7 +
>  drivers/clk/Makefile   |   1 +
>  drivers/clk/clk_rpmi.c | 346 +++++++++++++++++++++++++++++++++++++++++
>  include/rpmi_proto.h   |  39 +++++
>  5 files changed, 394 insertions(+)
>  create mode 100644 drivers/clk/clk_rpmi.c

...

> diff --git a/drivers/clk/clk_rpmi.c b/drivers/clk/clk_rpmi.c
> new file mode 100644
> index 000000000000..d2efb057600b
> --- /dev/null
> +++ b/drivers/clk/clk_rpmi.c
> @@ -0,0 +1,346 @@

...

> +struct rpmi_clk_def {
> +	u32 num_rates;
> +	u32 transition_latency;

rpmi_clk_def.transition_latency only gets assigned in
rpmi_clk_get_attrs() and never used. Since U-Boot's clock infrastructure
doesn't care about it at all, maybe we could drop the field?

> +	enum rpmi_clock_type type;
> +	char name[RPMI_CLK_NAME_LEN + 1];
> +};

...

> +static int _rpmi_clk_enable_disable(struct rpmi_clk_priv *priv, u32 clkid,
> +				    bool ena)
> +{
> +	struct rpmi_set_config_tx tx = {
> +		.clkid = cpu_to_le32(clkid),
> +		.config = cpu_to_le32(ena ? RPMI_CLK_STATE_ENABLED :
> +					    RPMI_CLK_STATE_DISABLED),
> +	};
> +	__le32 rx;
> +	int ret, status;
> +
> +	ret = rpmi_send_with_resp(&priv->chan, RPMI_CLK_SRV_SET_CONFIG, &tx,
> +				  sizeof(tx), &rx, sizeof(rx), NULL);
> +	if (ret)
> +		return ret;
> +
> +	status = le32_to_cpu(rx);
> +	if (status)
> +		return rpmi_to_linux_error(status);
> +
> +	return 0;

Please return rpmi_to_linux_error(status) and drop the if above.

> +}
> +
> +static ulong _rpmi_clk_get_rate(struct rpmi_clk_priv *priv, u32 clkid)
> +{
> +	__le32 tx = cpu_to_le32(clkid);
> +	struct rpmi_get_rate_rx rx;
> +	int ret, status;
> +
> +	ret = rpmi_send_with_resp(&priv->chan, RPMI_CLK_SRV_GET_RATE, &tx,
> +				  sizeof(tx), &rx, sizeof(rx), NULL);
> +	if (ret)
> +		return ret;
> +
> +	status = le32_to_cpu(rx.status);
> +	if (status)
> +		return rpmi_to_linux_error(status);
> +
> +	return (((u64)(le32_to_cpu(rx.hi)) << 32) | (u32)(le32_to_cpu(rx.lo)));
> +}

...

> +static int _rpmi_clk_set_rate(struct rpmi_clk_priv *priv, u32 clkid,
> +			      unsigned long rate)

This function is declared to return int, but...

> +{
> +	struct rpmi_set_rate_tx tx = {
> +		.clkid = cpu_to_le32(clkid),
> +		.flags = 0,
> +		.lo = cpu_to_le32(lower_32_bits(rate)),
> +		.hi = cpu_to_le32(upper_32_bits(rate)),
> +	};
> +	__le32 rx;
> +	int ret, status;
> +
> +	ret = rpmi_send_with_resp(&priv->chan, RPMI_CLK_SRV_SET_RATE, &tx,
> +				  sizeof(tx), &rx, sizeof(rx), NULL);
> +	if (ret)
> +		return ret;
> +
> +	status = le32_to_cpu(rx);
> +	if (status)
> +		return rpmi_to_linux_error(status);
> +
> +	return _rpmi_clk_get_rate(priv, clkid);

_rpmi_clk_get_rate() returns ulong. This unnecessarily truncates the
rate on 64bit platform when it's above approximately 2.1GHz.

> +}

Regards,
Yao Zi