Re: [PATCH v6 09/23] clk: Add clock driver for the RISC-V RPMI clock service group

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, Jun 18, 2025 at 05:43:44PM +0530, Anup Patel wrote:
> From: Rahul Pathak <rpathak@ventanamicro.com>
> 
> The RPMI specification defines a clock service group which can be
> accessed via SBI MPXY extension or dedicated S-mode RPMI transport.
> 
> Add mailbox client based clock driver for the RISC-V RPMI clock
> service group.

...

> +#include <linux/clk-provider.h>
> +#include <linux/err.h>
> +#include <linux/mailbox_client.h>
> +#include <linux/mailbox/riscv-rpmi-message.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/types.h>
> +#include <linux/slab.h>
> +#include <linux/wordpart.h>

...

> +enum rpmi_clk_config {
> +	RPMI_CLK_DISABLE = 0,
> +	RPMI_CLK_ENABLE = 1

It's still unclear if this enum can be expanded in the future (and no, you may
not answer this either). Hence, to reduce potential churn in the future, leave
the trailing comma here.

> +};

...

> +union rpmi_clk_rates {
> +	u64 discrete[RPMI_CLK_DISCRETE_MAX_NUM_RATES];
> +	struct {
> +		u64 min;
> +		u64 max;
> +		u64 step;
> +	} linear;

Have you looked at the linear_range.h? Why can it not be (re-)used here?

> +};

...

> +static u32 rpmi_clk_get_num_clocks(struct rpmi_clk_context *context)
> +{
> +	struct rpmi_get_num_clocks_rx rx;
> +	struct rpmi_mbox_message msg;
> +	int ret;
> +
> +	rpmi_mbox_init_send_with_response(&msg, RPMI_CLK_SRV_GET_NUM_CLOCKS,
> +					  NULL, 0, &rx, sizeof(rx));

...here

> +	ret = rpmi_mbox_send_message(context->chan, &msg);
> +

This blank line should be rather ^^^

> +	if (ret || rx.status)
> +		return 0;

Why rx.status can't be checked before calling to a sending message?
Sounds like the rpmi_mbox_init_send_with_response() links rx to msg somehow.
If this is the case, use msg here, otherwise move the check to be in the
correct place.

Seems the same question to the all similar checks in the code.

> +	return le32_to_cpu(rx.num_clocks);
> +}

...

> +static int rpmi_clk_get_supported_rates(u32 clkid, struct rpmi_clk *rpmi_clk)
> +{
> +	struct rpmi_clk_context *context = rpmi_clk->context;
> +	struct rpmi_clk_rate_discrete *rate_discrete;
> +	struct rpmi_clk_rate_linear *rate_linear;
> +	struct rpmi_get_supp_rates_rx *rx __free(kfree) = NULL;
> +	struct rpmi_get_supp_rates_tx tx;
> +	struct rpmi_mbox_message msg;

> +	size_t clk_rate_idx = 0;

This kind of assignments is hard to maintain and it's mistake prone in case
some additional code is injected in the future that might reuse it.

> +	int ret, rateidx, j;
> +
> +	tx.clkid = cpu_to_le32(clkid);
> +	tx.clk_rate_idx = 0;
> +
> +	/*
> +	 * Make sure we allocate rx buffer sufficient to be accommodate all
> +	 * the rates sent in one RPMI message.
> +	 */
> +	rx = kzalloc(context->max_msg_data_size, GFP_KERNEL);
> +	if (!rx)
> +		return -ENOMEM;
> +
> +	rpmi_mbox_init_send_with_response(&msg, RPMI_CLK_SRV_GET_SUPPORTED_RATES,
> +					  &tx, sizeof(tx), rx, context->max_msg_data_size);
> +	ret = rpmi_mbox_send_message(context->chan, &msg);
> +	if (ret)
> +		return ret;
> +	if (rx->status)
> +		return rpmi_to_linux_error(le32_to_cpu(rx->status));
> +	if (!le32_to_cpu(rx->returned))
> +		return -EINVAL;
> +
> +	if (rpmi_clk->type == RPMI_CLK_DISCRETE) {
> +		rate_discrete = (struct rpmi_clk_rate_discrete *)rx->rates;
> +
> +		for (rateidx = 0; rateidx < le32_to_cpu(rx->returned); rateidx++) {
> +			rpmi_clk->rates->discrete[rateidx] =
> +				rpmi_clkrate_u64(le32_to_cpu(rate_discrete[rateidx].hi),
> +						 le32_to_cpu(rate_discrete[rateidx].lo));
> +		}
> +
> +		/*
> +		 * Keep sending the request message until all
> +		 * the rates are received.
> +		 */
> +		while (le32_to_cpu(rx->remaining)) {
> +			clk_rate_idx += le32_to_cpu(rx->returned);
> +			tx.clk_rate_idx = cpu_to_le32(clk_rate_idx);
> +
> +			rpmi_mbox_init_send_with_response(&msg,
> +							  RPMI_CLK_SRV_GET_SUPPORTED_RATES,
> +							  &tx, sizeof(tx),
> +							  rx, context->max_msg_data_size);
> +			ret = rpmi_mbox_send_message(context->chan, &msg);
> +			if (ret)
> +				return ret;
> +			if (rx->status)
> +				return rpmi_to_linux_error(le32_to_cpu(rx->status));
> +			if (!le32_to_cpu(rx->returned))
> +				return -EINVAL;
> +
> +			for (j = 0; j < le32_to_cpu(rx->returned); j++) {
> +				if (rateidx >= clk_rate_idx + le32_to_cpu(rx->returned))
> +					break;
> +				rpmi_clk->rates->discrete[rateidx++] =
> +					rpmi_clkrate_u64(le32_to_cpu(rate_discrete[j].hi),
> +							 le32_to_cpu(rate_discrete[j].lo));
> +			}
> +		}
> +	} else if (rpmi_clk->type == RPMI_CLK_LINEAR) {
> +		rate_linear = (struct rpmi_clk_rate_linear *)rx->rates;
> +
> +		rpmi_clk->rates->linear.min = rpmi_clkrate_u64(le32_to_cpu(rate_linear->min_hi),
> +							       le32_to_cpu(rate_linear->min_lo));
> +		rpmi_clk->rates->linear.max = rpmi_clkrate_u64(le32_to_cpu(rate_linear->max_hi),
> +							       le32_to_cpu(rate_linear->max_lo));
> +		rpmi_clk->rates->linear.step = rpmi_clkrate_u64(le32_to_cpu(rate_linear->step_hi),
> +								le32_to_cpu(rate_linear->step_lo));
> +	}
> +
> +	return 0;
> +}

...

> +static int rpmi_clk_determine_rate(struct clk_hw *hw,
> +				   struct clk_rate_request *req)
> +{
> +	struct rpmi_clk *rpmi_clk = to_rpmi_clk(hw);
> +	u64 fmin, fmax, ftmp;
> +
> +	/*
> +	 * Keep the requested rate if the clock format
> +	 * is of discrete type. Let the platform which
> +	 * is actually controlling the clock handle that.
> +	 */
> +	if (rpmi_clk->type == RPMI_CLK_DISCRETE)
> +		return 0;
> +
> +	fmin = rpmi_clk->rates->linear.min;
> +	fmax = rpmi_clk->rates->linear.max;
> +
> +	if (req->rate <= fmin) {
> +		req->rate = fmin;
> +		return 0;

> +	} else if (req->rate >= fmax) {

Redundant 'else', but I see the wish to tight the conditional together.

> +		req->rate = fmax;
> +		return 0;
> +	}
> +
> +	ftmp = req->rate - fmin;
> +	ftmp += rpmi_clk->rates->linear.step - 1;
> +	do_div(ftmp, rpmi_clk->rates->linear.step);
> +
> +	req->rate = ftmp * rpmi_clk->rates->linear.step + fmin;
> +
> +	return 0;
> +}

...

> +static void rpmi_clk_disable(struct clk_hw *hw)
> +{
> +	struct rpmi_clk *rpmi_clk = to_rpmi_clk(hw);
> +	struct rpmi_clk_context *context = rpmi_clk->context;
> +	struct rpmi_mbox_message msg;
> +	struct rpmi_set_config_tx tx;
> +	struct rpmi_set_config_rx rx;
> +	int ret;
> +
> +	tx.config = cpu_to_le32(RPMI_CLK_DISABLE);
> +	tx.clkid = cpu_to_le32(rpmi_clk->id);
> +
> +	rpmi_mbox_init_send_with_response(&msg, RPMI_CLK_SRV_SET_CONFIG,
> +					  &tx, sizeof(tx), &rx, sizeof(rx));
> +	ret = rpmi_mbox_send_message(context->chan, &msg);
> +	if (ret || rx.status)
> +		pr_err("Failed to disable clk-%u\n", rpmi_clk->id);

Close to useless message. You may improve it by splitting to two and printing
rx.status in one and ret in the other with different text. Or drop it.

> +}

> +static int rpmi_clk_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	unsigned int num_clocks, i;
> +	struct clk_hw_onecell_data *clk_data;
> +	struct rpmi_clk_context *context;
> +	struct rpmi_mbox_message msg;
> +	struct clk_hw *hw_ptr;
> +	struct device *dev = &pdev->dev;
> +
> +	context = devm_kzalloc(dev, sizeof(*context), GFP_KERNEL);
> +	if (!context)
> +		return -ENOMEM;
> +	context->dev = dev;
> +	platform_set_drvdata(pdev, context);
> +
> +	context->client.dev		= context->dev;
> +	context->client.rx_callback	= NULL;
> +	context->client.tx_block	= false;
> +	context->client.knows_txdone	= true;
> +	context->client.tx_tout		= 0;
> +
> +	context->chan = mbox_request_channel(&context->client, 0);
> +	if (IS_ERR(context->chan))
> +		return PTR_ERR(context->chan);

Here is an incorrect order of the freeing resources. Besides that, wrapping the
mbox_free_channel() into managed resources reduces this code by more
than 10 LoCs! At bare minimum if will fix the bug,

> +	rpmi_mbox_init_get_attribute(&msg, RPMI_MBOX_ATTR_SPEC_VERSION);
> +	ret = rpmi_mbox_send_message(context->chan, &msg);
> +	if (ret) {
> +		mbox_free_channel(context->chan);
> +		return dev_err_probe(dev, ret, "Failed to get spec version\n");
> +	}
> +	if (msg.attr.value < RPMI_MKVER(1, 0)) {
> +		mbox_free_channel(context->chan);
> +		return dev_err_probe(dev, -EINVAL,
> +				     "msg protocol version mismatch, expected 0x%x, found 0x%x\n",
> +				     RPMI_MKVER(1, 0), msg.attr.value);
> +	}
> +
> +	rpmi_mbox_init_get_attribute(&msg, RPMI_MBOX_ATTR_SERVICEGROUP_ID);
> +	ret = rpmi_mbox_send_message(context->chan, &msg);
> +	if (ret) {
> +		mbox_free_channel(context->chan);
> +		return dev_err_probe(dev, ret, "Failed to get service group ID\n");
> +	}
> +	if (msg.attr.value != RPMI_SRVGRP_CLOCK) {
> +		mbox_free_channel(context->chan);
> +		return dev_err_probe(dev, -EINVAL,
> +				     "service group match failed, expected 0x%x, found 0x%x\n",
> +				     RPMI_SRVGRP_CLOCK, msg.attr.value);
> +	}
> +
> +	rpmi_mbox_init_get_attribute(&msg, RPMI_MBOX_ATTR_SERVICEGROUP_VERSION);
> +	ret = rpmi_mbox_send_message(context->chan, &msg);
> +	if (ret) {
> +		mbox_free_channel(context->chan);
> +		return dev_err_probe(dev, ret, "Failed to get service group version\n");
> +	}
> +	if (msg.attr.value < RPMI_MKVER(1, 0)) {
> +		mbox_free_channel(context->chan);
> +		return dev_err_probe(dev, -EINVAL,
> +				     "service group version failed, expected 0x%x, found 0x%x\n",
> +				     RPMI_MKVER(1, 0), msg.attr.value);
> +	}
> +
> +	rpmi_mbox_init_get_attribute(&msg, RPMI_MBOX_ATTR_MAX_MSG_DATA_SIZE);
> +	ret = rpmi_mbox_send_message(context->chan, &msg);
> +	if (ret) {
> +		mbox_free_channel(context->chan);
> +		return dev_err_probe(dev, ret, "Failed to get max message data size\n");
> +	}
> +
> +	context->max_msg_data_size = msg.attr.value;
> +	num_clocks = rpmi_clk_get_num_clocks(context);
> +	if (!num_clocks) {
> +		mbox_free_channel(context->chan);
> +		return dev_err_probe(dev, -ENODEV, "No clocks found\n");
> +	}
> +
> +	clk_data = devm_kzalloc(dev, struct_size(clk_data, hws, num_clocks),
> +				GFP_KERNEL);

(The above mention problem comes here after the successful allocation of
 clk_data but failing of any further code.

> +	if (!clk_data) {
> +		mbox_free_channel(context->chan);
> +		return dev_err_probe(dev, -ENOMEM, "No memory for clock data\n");
> +	}
> +	clk_data->num = num_clocks;
> +
> +	for (i = 0; i < clk_data->num; i++) {
> +		hw_ptr = rpmi_clk_enumerate(context, i);
> +		if (IS_ERR(hw_ptr)) {
> +			mbox_free_channel(context->chan);
> +			return dev_err_probe(dev, PTR_ERR(hw_ptr),
> +					     "failed to register clk-%d\n", i);
> +		}
> +		clk_data->hws[i] = hw_ptr;
> +	}
> +
> +	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_data);
> +	if (ret) {
> +		mbox_free_channel(context->chan);
> +		return dev_err_probe(dev, ret, "failed to register clock HW provider\n");
> +	}
> +
> +	return 0;
> +}

...

> +static void rpmi_clk_remove(struct platform_device *pdev)
> +{
> +	struct rpmi_clk_context *context = platform_get_drvdata(pdev);
> +
> +	mbox_free_channel(context->chan);
> +}

This function will be gone. See above.

-- 
With Best Regards,
Andy Shevchenko