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.

Co-developed-by: Anup Patel <apatel@ventanamicro.com>
Signed-off-by: Anup Patel <apatel@ventanamicro.com>
Signed-off-by: Rahul Pathak <rpathak@ventanamicro.com>
---
 drivers/clk/Kconfig                        |   8 +
 drivers/clk/Makefile                       |   1 +
 drivers/clk/clk-rpmi.c                     | 590 +++++++++++++++++++++
 include/linux/mailbox/riscv-rpmi-message.h |  16 +
 include/linux/wordpart.h                   |   8 +
 5 files changed, 623 insertions(+)
 create mode 100644 drivers/clk/clk-rpmi.c

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 19c1ed280fd7..2e385b146f8b 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -493,6 +493,14 @@ config COMMON_CLK_SP7021
 	  Not all features of the PLL are currently supported
 	  by the driver.
 
+config COMMON_CLK_RPMI
+	tristate "Clock driver based on RISC-V RPMI"
+	depends on MAILBOX
+	default RISCV
+	help
+	  Support for clocks based on the clock service group defined by
+	  the RISC-V platform management interface (RPMI) specification.
+
 source "drivers/clk/actions/Kconfig"
 source "drivers/clk/analogbits/Kconfig"
 source "drivers/clk/baikal-t1/Kconfig"
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 42867cd37c33..48866e429a40 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -84,6 +84,7 @@ obj-$(CONFIG_CLK_LS1028A_PLLDIG)	+= clk-plldig.o
 obj-$(CONFIG_COMMON_CLK_PWM)		+= clk-pwm.o
 obj-$(CONFIG_CLK_QORIQ)			+= clk-qoriq.o
 obj-$(CONFIG_COMMON_CLK_RK808)		+= clk-rk808.o
+obj-$(CONFIG_COMMON_CLK_RPMI)		+= clk-rpmi.o
 obj-$(CONFIG_COMMON_CLK_HI655X)		+= clk-hi655x.o
 obj-$(CONFIG_COMMON_CLK_S2MPS11)	+= clk-s2mps11.o
 obj-$(CONFIG_COMMON_CLK_SCMI)           += clk-scmi.o
diff --git a/drivers/clk/clk-rpmi.c b/drivers/clk/clk-rpmi.c
new file mode 100644
index 000000000000..61c27ef2840f
--- /dev/null
+++ b/drivers/clk/clk-rpmi.c
@@ -0,0 +1,590 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * RISC-V MPXY Based Clock Driver
+ *
+ * Copyright (C) 2025 Ventana Micro Systems Ltd.
+ */
+
+#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>
+
+#define RPMI_CLK_DISCRETE_MAX_NUM_RATES		16
+#define RPMI_CLK_NAME_LEN			16
+
+#define to_rpmi_clk(clk)	container_of(clk, struct rpmi_clk, hw)
+
+#define rpmi_clkrate_u64(hi, lo)	make_u64_from_two_u32(hi, lo)
+
+enum rpmi_clk_config {
+	RPMI_CLK_DISABLE = 0,
+	RPMI_CLK_ENABLE = 1
+};
+
+enum rpmi_clk_type {
+	RPMI_CLK_DISCRETE = 0,
+	RPMI_CLK_LINEAR = 1,
+	RPMI_CLK_TYPE_MAX_IDX
+};
+
+struct rpmi_clk_context {
+	struct device *dev;
+	struct mbox_chan *chan;
+	struct mbox_client client;
+	u32 max_msg_data_size;
+};
+
+union rpmi_clk_rates {
+	u64 discrete[RPMI_CLK_DISCRETE_MAX_NUM_RATES];
+	struct {
+		u64 min;
+		u64 max;
+		u64 step;
+	} linear;
+};
+
+struct rpmi_clk {
+	struct rpmi_clk_context *context;
+	u32 id;
+	u32 num_rates;
+	u32 transition_latency;
+	enum rpmi_clk_type type;
+	union rpmi_clk_rates *rates;
+	char name[RPMI_CLK_NAME_LEN];
+	struct clk_hw hw;
+};
+
+struct rpmi_clk_rate_discrete {
+	__le32 lo;
+	__le32 hi;
+};
+
+struct rpmi_clk_rate_linear {
+	__le32 min_lo;
+	__le32 min_hi;
+	__le32 max_lo;
+	__le32 max_hi;
+	__le32 step_lo;
+	__le32 step_hi;
+};
+
+struct rpmi_get_num_clocks_rx {
+	__le32 status;
+	__le32 num_clocks;
+};
+
+struct rpmi_get_attrs_tx {
+	__le32 clkid;
+};
+
+struct rpmi_get_attrs_rx {
+	__le32 status;
+	__le32 flags;
+	__le32 num_rates;
+	__le32 transition_latency;
+	char name[RPMI_CLK_NAME_LEN];
+};
+
+struct rpmi_get_supp_rates_tx {
+	__le32 clkid;
+	__le32 clk_rate_idx;
+};
+
+struct rpmi_get_supp_rates_rx {
+	__le32 status;
+	__le32 flags;
+	__le32 remaining;
+	__le32 returned;
+	__le32 rates[];
+};
+
+struct rpmi_get_rate_tx {
+	__le32 clkid;
+};
+
+struct rpmi_get_rate_rx {
+	__le32 status;
+	__le32 lo;
+	__le32 hi;
+};
+
+struct rpmi_set_rate_tx {
+	__le32 clkid;
+	__le32 flags;
+	__le32 lo;
+	__le32 hi;
+};
+
+struct rpmi_set_rate_rx {
+	__le32 status;
+};
+
+struct rpmi_set_config_tx {
+	__le32 clkid;
+	__le32 config;
+};
+
+struct rpmi_set_config_rx {
+	__le32 status;
+};
+
+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));
+	ret = rpmi_mbox_send_message(context->chan, &msg);
+
+	if (ret || rx.status)
+		return 0;
+
+	return le32_to_cpu(rx.num_clocks);
+}
+
+static int rpmi_clk_get_attrs(u32 clkid, struct rpmi_clk *rpmi_clk)
+{
+	struct rpmi_clk_context *context = rpmi_clk->context;
+	struct rpmi_mbox_message msg;
+	struct rpmi_get_attrs_tx tx;
+	struct rpmi_get_attrs_rx rx;
+	u8 format;
+	int ret;
+
+	tx.clkid = cpu_to_le32(clkid);
+	rpmi_mbox_init_send_with_response(&msg, RPMI_CLK_SRV_GET_ATTRIBUTES,
+					  &tx, sizeof(tx), &rx, sizeof(rx));
+	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));
+
+	rpmi_clk->id = clkid;
+	rpmi_clk->num_rates = le32_to_cpu(rx.num_rates);
+	rpmi_clk->transition_latency = le32_to_cpu(rx.transition_latency);
+	strscpy(rpmi_clk->name, rx.name, RPMI_CLK_NAME_LEN);
+
+	format = le32_to_cpu(rx.flags) & 3U;
+	if (format >= RPMI_CLK_TYPE_MAX_IDX)
+		return -EINVAL;
+
+	rpmi_clk->type = format;
+
+	return 0;
+}
+
+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;
+	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 unsigned long rpmi_clk_recalc_rate(struct clk_hw *hw,
+					  unsigned long parent_rate)
+{
+	struct rpmi_clk *rpmi_clk = to_rpmi_clk(hw);
+	struct rpmi_clk_context *context = rpmi_clk->context;
+	struct rpmi_mbox_message msg;
+	struct rpmi_get_rate_tx tx;
+	struct rpmi_get_rate_rx rx;
+	int ret;
+
+	tx.clkid = cpu_to_le32(rpmi_clk->id);
+
+	rpmi_mbox_init_send_with_response(&msg, RPMI_CLK_SRV_GET_RATE,
+					  &tx, sizeof(tx), &rx, sizeof(rx));
+	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));
+
+	return rpmi_clkrate_u64(le32_to_cpu(rx.hi), le32_to_cpu(rx.lo));
+}
+
+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) {
+		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 int rpmi_clk_set_rate(struct clk_hw *hw, unsigned long rate,
+			     unsigned long parent_rate)
+{
+	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_rate_tx tx;
+	struct rpmi_set_rate_rx rx;
+	int ret;
+
+	tx.clkid = cpu_to_le32(rpmi_clk->id);
+	tx.lo = cpu_to_le32(lower_32_bits(rate));
+	tx.hi = cpu_to_le32(upper_32_bits(rate));
+
+	rpmi_mbox_init_send_with_response(&msg, RPMI_CLK_SRV_SET_RATE,
+					  &tx, sizeof(tx), &rx, sizeof(rx));
+	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));
+
+	return 0;
+}
+
+static int rpmi_clk_enable(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_ENABLE);
+	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)
+		return ret;
+	if (rx.status)
+		return rpmi_to_linux_error(le32_to_cpu(rx.status));
+
+	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);
+}
+
+static const struct clk_ops rpmi_clk_ops = {
+	.recalc_rate = rpmi_clk_recalc_rate,
+	.determine_rate = rpmi_clk_determine_rate,
+	.set_rate = rpmi_clk_set_rate,
+	.prepare = rpmi_clk_enable,
+	.unprepare = rpmi_clk_disable,
+};
+
+static struct clk_hw *rpmi_clk_enumerate(struct rpmi_clk_context *context, u32 clkid)
+{
+	struct device *dev = context->dev;
+	unsigned long min_rate, max_rate;
+	union rpmi_clk_rates *rates;
+	struct rpmi_clk *rpmi_clk;
+	struct clk_init_data init = {};
+	struct clk_hw *clk_hw;
+	int ret;
+
+	rates = devm_kzalloc(dev, sizeof(*rates), GFP_KERNEL);
+	if (!rates)
+		return ERR_PTR(-ENOMEM);
+
+	rpmi_clk = devm_kzalloc(dev, sizeof(*rpmi_clk), GFP_KERNEL);
+	if (!rpmi_clk)
+		return ERR_PTR(-ENOMEM);
+
+	rpmi_clk->context = context;
+	rpmi_clk->rates = rates;
+
+	ret = rpmi_clk_get_attrs(clkid, rpmi_clk);
+	if (ret)
+		return dev_err_ptr_probe(dev, ret,
+					 "Failed to get clk-%u attributes\n",
+					 clkid);
+
+	ret = rpmi_clk_get_supported_rates(clkid, rpmi_clk);
+	if (ret)
+		return dev_err_ptr_probe(dev, ret,
+					 "Get supported rates failed for clk-%u\n",
+					 clkid);
+
+	init.flags = CLK_GET_RATE_NOCACHE;
+	init.num_parents = 0;
+	init.ops = &rpmi_clk_ops;
+	init.name = rpmi_clk->name;
+	clk_hw = &rpmi_clk->hw;
+	clk_hw->init = &init;
+
+	ret = devm_clk_hw_register(dev, clk_hw);
+	if (ret)
+		return dev_err_ptr_probe(dev, ret,
+					 "Unable to register clk-%u\n",
+					 clkid);
+
+	if (rpmi_clk->type == RPMI_CLK_DISCRETE) {
+		min_rate = rpmi_clk->rates->discrete[0];
+		max_rate = rpmi_clk->rates->discrete[rpmi_clk->num_rates -  1];
+	} else {
+		min_rate = rpmi_clk->rates->linear.min;
+		max_rate = rpmi_clk->rates->linear.max;
+	}
+
+	clk_hw_set_rate_range(clk_hw, min_rate, max_rate);
+
+	return clk_hw;
+}
+
+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);
+
+	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);
+	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);
+}
+
+static const struct of_device_id rpmi_clk_of_match[] = {
+	{ .compatible = "riscv,rpmi-clock" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, rpmi_clk_of_match);
+
+static struct platform_driver rpmi_clk_driver = {
+	.driver = {
+		.name = "riscv-rpmi-clock",
+		.of_match_table = rpmi_clk_of_match,
+	},
+	.probe = rpmi_clk_probe,
+	.remove = rpmi_clk_remove,
+};
+module_platform_driver(rpmi_clk_driver);
+
+MODULE_AUTHOR("Rahul Pathak <rpathak@ventanamicro.com>");
+MODULE_DESCRIPTION("Clock Driver based on RPMI message protocol");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mailbox/riscv-rpmi-message.h b/include/linux/mailbox/riscv-rpmi-message.h
index 4f7209e0a9bc..78f0aafbff68 100644
--- a/include/linux/mailbox/riscv-rpmi-message.h
+++ b/include/linux/mailbox/riscv-rpmi-message.h
@@ -90,6 +90,22 @@ static inline int rpmi_to_linux_error(int rpmi_error)
 	}
 }
 
+/* RPMI service group IDs */
+#define RPMI_SRVGRP_CLOCK		0x00008
+
+/* RPMI clock service IDs */
+enum rpmi_clock_service_id {
+	RPMI_CLK_SRV_ENABLE_NOTIFICATION = 0x01,
+	RPMI_CLK_SRV_GET_NUM_CLOCKS = 0x02,
+	RPMI_CLK_SRV_GET_ATTRIBUTES = 0x03,
+	RPMI_CLK_SRV_GET_SUPPORTED_RATES = 0x04,
+	RPMI_CLK_SRV_SET_CONFIG = 0x05,
+	RPMI_CLK_SRV_GET_CONFIG = 0x06,
+	RPMI_CLK_SRV_SET_RATE = 0x07,
+	RPMI_CLK_SRV_GET_RATE = 0x08,
+	RPMI_CLK_SRV_ID_MAX_COUNT
+};
+
 /* RPMI Linux mailbox attribute IDs */
 enum rpmi_mbox_attribute_id {
 	RPMI_MBOX_ATTR_SPEC_VERSION,
diff --git a/include/linux/wordpart.h b/include/linux/wordpart.h
index ed8717730037..5cad9c244bf2 100644
--- a/include/linux/wordpart.h
+++ b/include/linux/wordpart.h
@@ -39,6 +39,14 @@
  */
 #define make_u32_from_two_u16(hi, lo)	(((u32)(hi) << 16) | (u32)(lo))
 
+/**
+ * make_u64_from_two_u32 - return u64 number by combining
+ * two u32 numbers.
+ * @hi: upper 32 bit number
+ * @lo: lower 32 bit number
+ */
+#define make_u64_from_two_u32(hi, lo)	(((u64)(hi) << 32) | (u32)(lo))
+
 /**
  * REPEAT_BYTE - repeat the value @x multiple times as an unsigned long value
  * @x: value to repeat
-- 
2.43.0

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

On Mon, Jun 23, 2025 at 2:36 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> 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.
>
> ...
>
> ...
>
> > +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.
>

Ok, will update

> > +};
>
> ...
>
> > +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?
>

I did the first time only when you commented. And i dont see any
benefit in that.
linear_range has slightly different way to access any value using `sel`.
Here this union represents how RPMI protocol represents the rates and
reusing linear_range will only introduce conversion to and fro.

> > +};
>
> ...
>
> > +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 ^^^

Sure, I will update.

>
> > +     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.

Yes, the rpmi_mbox_init_send_with_response is a helper function which links
the rx to msg. It's a very simple function which only performs assignments.

Using msg instead of rx directly will require additional typecasting
which will only clutter
I can add a comment if that helps wherever the rpmi_mbox_init_send_with_response
is used.

>
> 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.
>
I dont understand what is the problem with this assignment. If any
code added in the future reuse it then it has to make sure that
clk_rate_idx has the correct initial value before any further references.

> > +     int ret, rateidx, j;
> > +

...

> > +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.

Sure, I think it's better to keep and split it into two as you suggest
instead of dropping.

>
> > +}
>
> > +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,

Understood. So we can use devm_add_action_or_reset to link a release function
with the context->chan. Is this what you are suggesting? This will also make
the .remove callback redundant which can be removed.

...

> > +
> > +     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.

Once the change mentioned in above comment will be done this will take
care of the rest of the exit scenarios.

>
> > +     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.

Agree

>
> --
> With Best Regards,
> Andy Shevchenko
>
>


-- 

Thanks
Rahul Pathak

On Thu, Jun 26, 2025 at 12:32:14PM +0530, Rahul Pathak wrote:
> On Mon, Jun 23, 2025 at 2:36 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Wed, Jun 18, 2025 at 05:43:44PM +0530, Anup Patel wrote:

...

> > > +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?
> 
> I did the first time only when you commented. And i dont see any
> benefit in that.
> linear_range has slightly different way to access any value using `sel`.
> Here this union represents how RPMI protocol represents the rates and
> reusing linear_range will only introduce conversion to and fro.

Summarize this in the comment on top of the struct definition so one will not
attempt to convert the driver in the future.

> > > +};

...

> > > +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 ^^^
> 
> Sure, I will update.
> 
> >
> > > +     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.
> 
> Yes, the rpmi_mbox_init_send_with_response is a helper function which links
> the rx to msg. It's a very simple function which only performs assignments.
> 
> Using msg instead of rx directly will require additional typecasting
> which will only clutter
> I can add a comment if that helps wherever the rpmi_mbox_init_send_with_response
> is used.

This is besides harder-to-read code is kinda of layering violation.
If you afraid of a casting, add a helper to check for the status error.
Comment won't help much as making code better to begin with.

> > Seems the same question to the all similar checks in the code.
> >
> > > +     return le32_to_cpu(rx.num_clocks);
> > > +}

...

> > > +     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;

This should be assigned where it's used. NULL assignment is not encouraged.

> > > +     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.
> >
> I dont understand what is the problem with this assignment. If any
> code added in the future reuse it then it has to make sure that
> clk_rate_idx has the correct initial value before any further references.

Yes, it will add an additional churn and require more brain activity to
maintain such a code. It's a general recommendation to assign when it's
actually needed (there are, of course, exceptions for some small functions,
but this one is not).

> > > +     int ret, rateidx, j;

...

> > > +     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 it will fix the bug,
> 
> Understood. So we can use devm_add_action_or_reset to link a release function
> with the context->chan. Is this what you are suggesting? This will also make
> the .remove callback redundant which can be removed.

Yes.

-- 
With Best Regards,
Andy Shevchenko

> > > > +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?
> >
> > I did the first time only when you commented. And i dont see any
> > benefit in that.
> > linear_range has slightly different way to access any value using `sel`.
> > Here this union represents how RPMI protocol represents the rates and
> > reusing linear_range will only introduce conversion to and fro.
>
> Summarize this in the comment on top of the struct definition so one will not
> attempt to convert the driver in the future.
>

Sure, I will update.

> >
> > >
> > > > +     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.
> >
> > Yes, the rpmi_mbox_init_send_with_response is a helper function which links
> > the rx to msg. It's a very simple function which only performs assignments.
> >
> > Using msg instead of rx directly will require additional typecasting
> > which will only clutter
> > I can add a comment if that helps wherever the rpmi_mbox_init_send_with_response
> > is used.
>
> This is besides harder-to-read code is kinda of layering violation.
> If you afraid of a casting, add a helper to check for the status error.
> Comment won't help much as making code better to begin with.

Why using rx is the issue in the first place when it's the same layer
which links the rx with msg using the helper function and then
uses it directly?  Infact using rx directly avoids unnecessary code
which is only increasing redundant code which ultimately results in
same thing. Even if I add a helper function that will require additional
pointer passing with NULL checking which all is currently avoided.
And, we are not just talking about rx.status but a lot of other fields.

>
> > > > +     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;
>
> This should be assigned where it's used. NULL assignment is not encouraged.

Sure, I will update.

>
> > > > +     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.
> > >
> > I dont understand what is the problem with this assignment. If any
> > code added in the future reuse it then it has to make sure that
> > clk_rate_idx has the correct initial value before any further references.
>
> Yes, it will add an additional churn and require more brain activity to
> maintain such a code. It's a general recommendation to assign when it's
> actually needed (there are, of course, exceptions for some small functions,
> but this one is not).

Sure, I will update.



-- 

Thanks
Rahul Pathak

On Fri, Jun 27, 2025 at 08:36:41PM +0530, Rahul Pathak wrote:

...

> > > > > +     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.
> > >
> > > Yes, the rpmi_mbox_init_send_with_response is a helper function which links
> > > the rx to msg. It's a very simple function which only performs assignments.
> > >
> > > Using msg instead of rx directly will require additional typecasting
> > > which will only clutter
> > > I can add a comment if that helps wherever the rpmi_mbox_init_send_with_response
> > > is used.
> >
> > This is besides harder-to-read code is kinda of layering violation.
> > If you afraid of a casting, add a helper to check for the status error.
> > Comment won't help much as making code better to begin with.
> 
> Why using rx is the issue in the first place when it's the same layer
> which links the rx with msg using the helper function and then
> uses it directly?  Infact using rx directly avoids unnecessary code
> which is only increasing redundant code which ultimately results in
> same thing. Even if I add a helper function that will require additional
> pointer passing with NULL checking which all is currently avoided.
> And, we are not just talking about rx.status but a lot of other fields.

Because it's simply bad code, look at the simplified model:

	int foo, bar;
	int ret;

	func_1(..., &foo, &bar);
	ret = func_2(&foo);
	if (ret || bar)
		...do something...

When one reads this code the immediate reaction will be like mine.
This is also (without deeper understanding) tempting to someone
who even thinks that the code can be simplified (w/o knowing that it
may not) to change it as

	func_1(..., &foo, &bar);
	if (bar)
		...do something...

	ret = func_2(&foo);
	if (ret)
		...do something...

Using msg is the right thing to do. In that way there is no questions asked
and everything is clear. Also why layering violation? Because the conditional
requires to know the guts of rx in the code which doesn't use rx that way
(or rather using it as semi-opaque object).

-- 
With Best Regards,
Andy Shevchenko

Quoting Anup Patel (2025-06-18 05:13:44)
> 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.
> 
> Co-developed-by: Anup Patel <apatel@ventanamicro.com>
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> Signed-off-by: Rahul Pathak <rpathak@ventanamicro.com>
> ---

Reviewed-by: Stephen Boyd <sboyd@kernel.org>