Skip to contentSkip to conversation

Add QCOM RPMh Clock driver

PATCH Superseded Taniya Das 3 participants 9 messages Apr 8, 2018 – Apr 14, 2018

Subsystems (4)
Device tree bindings Arm Qualcomm mailing list Common clk Qualcomm clock drivers
lore
Share & tools
lore

Taniya Das

author
#0 lore raw

v2 cover Add QCOM RPMh Clock driver

[v2]
* Addressed comments from Stephen
* Addressed comments from Evan

This patch series adds a driver and device tree documentation binding for the clock control via Resource Power Manager-hardened (RPMh) on some Qualcomm Technologies, Inc, SoCs such as SDM845. The clock RPMh driver would send requests for the RPMh managed clock resources.

The RPMh clock driver depends upon the RPMh driver [1] and command DB driver [2] which are both still undergoing review.

Thanks,
Taniya

[1]: https://lkml.org/lkml/2018/3/9/979
[2]: https://lkml.org/lkml/2018/3/14/787

Amit Nischal (2):
clk: qcom: clk-rpmh: Add QCOM RPMh clock driver
dt-bindings: clock: Introduce QCOM RPMh clock bindings

 .../devicetree/bindings/clock/qcom,rpmh-clk.txt |  22 ++
 drivers/clk/qcom/Kconfig                        |   9 +
 drivers/clk/qcom/Makefile                       |   1 +
 drivers/clk/qcom/clk-rpmh.c                     | 394 +++++++++++++++++++++
 include/dt-bindings/clock/qcom,rpmh.h           |  25 ++
 5 files changed, 451 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt
 create mode 100644 drivers/clk/qcom/clk-rpmh.c
 create mode 100644 include/dt-bindings/clock/qcom,rpmh.h

--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member of the Code Aurora Forum, hosted by the Linux Foundation.

author
#1 lore raw

PATCH v2 1/1 1/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver

From: Amit Nischal <[email protected]>

Add the RPMh clock driver to control the RPMh managed clock resources on some of the Qualcomm Technologies, Inc. SoCs.

Signed-off-by David Collins <[email protected]>
Signed-off-by Amit Nischal <[email protected]>
Signed-off-by Taniya Das <[email protected]>
---
 drivers/clk/qcom/Kconfig              |   9 +
 drivers/clk/qcom/Makefile             |   1 +
 drivers/clk/qcom/clk-rpmh.c           | 394 ++++++++++++++++++++++++++++++++++
 include/dt-bindings/clock/qcom,rpmh.h |  25 +++
 4 files changed, 429 insertions(+)
 create mode 100644 drivers/clk/qcom/clk-rpmh.c
 create mode 100644 include/dt-bindings/clock/qcom,rpmh.h
drivers/clk/qcom/Kconfig +9 -0
index fbf4532..3697a6a 100644
@@ -112,6 +112,15 @@ config IPQ_GCC_8074
 	  i2c, USB, SD/eMMC, etc. Select this for the root clock
 	  of ipq8074.
 
+config MSM_CLK_RPMH
+	tristate "RPMh Clock Driver"
+	depends on COMMON_CLK_QCOM && QCOM_RPMH
+	help
+	 RPMh manages shared resources on some Qualcomm Technologies, Inc.
+	 SoCs. It accepts requests from other hardware subsystems via RSC.
+	 Say Y if you want to support the clocks exposed by RPMh on
+	 platforms such as sdm845.
+
 config MSM_GCC_8660
 	tristate "MSM8660 Global Clock Controller"
 	depends on COMMON_CLK_QCOM
drivers/clk/qcom/Makefile +1 -0
index 230332c..b7da05b 100644
@@ -23,6 +23,7 @@ obj-$(CONFIG_IPQ_GCC_8074) += gcc-ipq8074.o
 obj-$(CONFIG_IPQ_LCC_806X) += lcc-ipq806x.o
 obj-$(CONFIG_MDM_GCC_9615) += gcc-mdm9615.o
 obj-$(CONFIG_MDM_LCC_9615) += lcc-mdm9615.o
+obj-$(CONFIG_MSM_CLK_RPMH) += clk-rpmh.o
 obj-$(CONFIG_MSM_GCC_8660) += gcc-msm8660.o
 obj-$(CONFIG_MSM_GCC_8916) += gcc-msm8916.o
 obj-$(CONFIG_MSM_GCC_8960) += gcc-msm8960.o
drivers/clk/qcom/clk-rpmh.c new file +394 -0

394 lines not on this page · raw message

include/dt-bindings/clock/qcom,rpmh.h new file +25 -0
new file mode 100644
index 0000000..34fbf3c
@@ -0,0 +1,25 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
+ */
+
+#ifndef _DT_BINDINGS_CLK_MSM_RPMH_H
+#define _DT_BINDINGS_CLK_MSM_RPMH_H
+
+/* RPMh controlled clocks */
+#define RPMH_CXO_CLK				0
+#define RPMH_CXO_CLK_A				1
+#define RPMH_LN_BB_CLK2				2
+#define RPMH_LN_BB_CLK2_A			3
+#define RPMH_LN_BB_CLK3				4
+#define RPMH_LN_BB_CLK3_A			5
+#define RPMH_RF_CLK1				6
+#define RPMH_RF_CLK1_A				7
+#define RPMH_RF_CLK2				8
+#define RPMH_RF_CLK2_A				9
+#define RPMH_RF_CLK3				10
+#define RPMH_RF_CLK3_A				11
+#define RPMH_RF_CLK4				12
+#define RPMH_RF_CLK4_A				13
+
+#endif
Signature · 2 lines
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the  Linux Foundation.
#2 lore raw

Re: [[PATCH v2] 1/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver

Quoted 22 lines · Taniya Das

From: Amit Nischal <[email protected]>

Add the RPMh clock driver to control the RPMh managed clock resources on some of the Qualcomm Technologies, Inc. SoCs.

Signed-off-by David Collins <[email protected]>
Signed-off-by Amit Nischal <[email protected]>
Signed-off-by Taniya Das <[email protected]>
---
 drivers/clk/qcom/Kconfig              |   9 +
 drivers/clk/qcom/Makefile             |   1 +
 drivers/clk/qcom/clk-rpmh.c           | 394 ++++++++++++++++++++++++++++++++++
 include/dt-bindings/clock/qcom,rpmh.h |  25 +++
 4 files changed, 429 insertions(+)
 create mode 100644 drivers/clk/qcom/clk-rpmh.c
 create mode 100644 include/dt-bindings/clock/qcom,rpmh.h
drivers/clk/qcom/Kconfig +0 -0
index fbf4532..3697a6a 100644

drivers/clk/qcom/Kconfig config IPQ_GCC_8074 · line 115

Quoted 4 lines
drivers/clk/qcom/Kconfig +1 -0
@@ -112,6 +112,15 @@ config IPQ_GCC_8074
 	  i2c, USB, SD/eMMC, etc. Select this for the root clock
 	  of ipq8074.
 
+config MSM_CLK_RPMH

QCOM_CLK_RPMH

Quoted 33 lines
drivers/clk/qcom/Kconfig +8 -0
+	tristate "RPMh Clock Driver"
+	depends on COMMON_CLK_QCOM && QCOM_RPMH
+	help
+	 RPMh manages shared resources on some Qualcomm Technologies, Inc.
+	 SoCs. It accepts requests from other hardware subsystems via RSC.
+	 Say Y if you want to support the clocks exposed by RPMh on
+	 platforms such as sdm845.
+
 config MSM_GCC_8660
 	tristate "MSM8660 Global Clock Controller"
 	depends on COMMON_CLK_QCOM
drivers/clk/qcom/Makefile +1 -0
index 230332c..b7da05b 100644
@@ -23,6 +23,7 @@ obj-$(CONFIG_IPQ_GCC_8074) += gcc-ipq8074.o
 obj-$(CONFIG_IPQ_LCC_806X) += lcc-ipq806x.o
 obj-$(CONFIG_MDM_GCC_9615) += gcc-mdm9615.o
 obj-$(CONFIG_MDM_LCC_9615) += lcc-mdm9615.o
+obj-$(CONFIG_MSM_CLK_RPMH) += clk-rpmh.o
 obj-$(CONFIG_MSM_GCC_8660) += gcc-msm8660.o
 obj-$(CONFIG_MSM_GCC_8916) += gcc-msm8916.o
 obj-$(CONFIG_MSM_GCC_8960) += gcc-msm8960.o
drivers/clk/qcom/clk-rpmh.c new file +4 -0
new file mode 100644
index 0000000..763401f
@@ -0,0 +1,394 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
+ */

drivers/clk/qcom/clk-rpmh.c line 14

Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c new file +10 -0
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>

Unused

drivers/clk/qcom/clk-rpmh.c line 21

Quoted 7 lines
drivers/clk/qcom/clk-rpmh.c new file +7 -0
+#include <soc/qcom/cmd-db.h>
+#include <soc/qcom/rpmh.h>
+
+#include <dt-bindings/clock/qcom,rpmh.h>
+
+#include "common.h"
+#include "clk-regmap.h"

Unused

drivers/clk/qcom/clk-rpmh.c line 26

Quoted 5 lines
drivers/clk/qcom/clk-rpmh.c new file +5 -0
+
+#define CLK_RPMH_ARC_EN_OFFSET 0
+#define CLK_RPMH_VRM_EN_OFFSET 4
+#define CLK_RPMH_VRM_OFF_VAL 0
+#define CLK_RPMH_VRM_ON_VAL 1

Use a bool (true/false) rather than lugging around these two defines.

drivers/clk/qcom/clk-rpmh.c line 38

Quoted 12 lines
drivers/clk/qcom/clk-rpmh.c new file +12 -0
+#define CLK_RPMH_APPS_RSC_AO_STATE_MASK (BIT(RPMH_WAKE_ONLY_STATE) | \
+					 BIT(RPMH_ACTIVE_ONLY_STATE))
+#define CLK_RPMH_APPS_RSC_STATE_MASK (BIT(RPMH_WAKE_ONLY_STATE) | \
+				      BIT(RPMH_ACTIVE_ONLY_STATE) | \
+				      BIT(RPMH_SLEEP_STATE))
+struct clk_rpmh {
+	struct clk_hw hw;
+	const char *res_name;
+	u32 res_addr;
+	u32 res_en_offset;
+	u32 res_on_val;
+	u32 res_off_val;

res_off_val is 0 for all clocks.

drivers/clk/qcom/clk-rpmh.c line 41

Quoted 3 lines
drivers/clk/qcom/clk-rpmh.c new file +3 -0
+	u32 state;
+	u32 aggr_state;
+	u32 last_sent_aggr_state;

Through the use of some local variables you shouldn't have to lug around aggr_state and last_sent_aggr_state.

Quoted 20 lines
drivers/clk/qcom/clk-rpmh.c new file +20 -0
+	u32 valid_state_mask;
+	struct rpmh_client *rpmh_client;
+	unsigned long rate;
+	struct clk_rpmh *peer;
+};
+
+struct rpmh_cc {
+	struct clk_onecell_data data;
+	struct clk *clks[];
+};
+
+struct clk_rpmh_desc {
+	struct clk_hw **clks;
+	size_t num_clks;
+};
+
+static DEFINE_MUTEX(rpmh_clk_lock);
+
+#define __DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name, \
+			  _res_en_offset, _res_on, _res_off, _rate, \

drivers/clk/qcom/clk-rpmh.c line 71

Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c new file +10 -0
+			  _state_mask, _state_on_mask)			      \
+	static struct clk_rpmh _platform##_##_name_active;		      \
+	static struct clk_rpmh _platform##_##_name = {			      \
+		.res_name = _res_name,					      \
+		.res_en_offset = _res_en_offset,			      \
+		.res_on_val = _res_on,					      \
+		.res_off_val = _res_off,				      \
+		.rate = _rate,						      \
+		.peer = &_platform##_##_name_active,			      \
+		.valid_state_mask = _state_mask,			      \

This is always CLK_RPMH_APPS_RSC_STATE_MASK

drivers/clk/qcom/clk-rpmh.c line 84

Quoted 13 lines
drivers/clk/qcom/clk-rpmh.c new file +13 -0
+		.hw.init = &(struct clk_init_data){			      \
+			.ops = &clk_rpmh_ops,			              \
+			.name = #_name,					      \
+		},							      \
+	};								      \
+	static struct clk_rpmh _platform##_##_name_active = {		      \
+		.res_name = _res_name,					      \
+		.res_en_offset = _res_en_offset,			      \
+		.res_on_val = _res_on,					      \
+		.res_off_val = _res_off,				      \
+		.rate = _rate,						      \
+		.peer = &_platform##_##_name,				      \
+		.valid_state_mask = _state_on_mask,			      \

and this is always CLK_RPMH_APPS_RSC_AO_STATE_MASK. If you just hard code them here (until there's a need for them to be anything else) the clock list below becomes tidier.

Quoted 31 lines
drivers/clk/qcom/clk-rpmh.c new file +31 -0
+		.hw.init = &(struct clk_init_data){			      \
+			.ops = &clk_rpmh_ops,				      \
+			.name = #_name_active,				      \
+		},							      \
+	}
+
+#define DEFINE_CLK_RPMH_ARC(_platform, _name, _name_active, _res_name, \
+			    _res_on, _res_off, _rate, _state_mask, \
+			    _state_on_mask)				\
+	__DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name,	\
+			  CLK_RPMH_ARC_EN_OFFSET, _res_on, _res_off, \
+			  _rate, _state_mask, _state_on_mask)
+
+#define DEFINE_CLK_RPMH_VRM(_platform, _name, _name_active, _res_name,	\
+			    _rate, _state_mask, _state_on_mask) \
+	__DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name,	\
+			  CLK_RPMH_VRM_EN_OFFSET, CLK_RPMH_VRM_ON_VAL,	\
+			  CLK_RPMH_VRM_OFF_VAL, _rate, _state_mask, \
+			  _state_on_mask)
+
+static inline struct clk_rpmh *to_clk_rpmh(struct clk_hw *_hw)
+{
+	return container_of(_hw, struct clk_rpmh, hw);
+}
+
+static inline bool has_state_changed(struct clk_rpmh *c, u32 state)
+{
+	return ((c->last_sent_aggr_state & BIT(state))
+		!= (c->aggr_state & BIT(state)));
+}
+

drivers/clk/qcom/clk-rpmh.c line 125

Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c new file +10 -0
+static int clk_rpmh_send_aggregate_command(struct clk_rpmh *c)
+{
+	struct tcs_cmd cmd = { 0 };
+	int ret = 0;
+
+	cmd.addr = c->res_addr + c->res_en_offset;
+
+	if (has_state_changed(c, RPMH_SLEEP_STATE)) {
+		cmd.data = (c->aggr_state & BIT(RPMH_SLEEP_STATE))
+				? c->res_on_val : c->res_off_val;

As these values are reused several times in this function you could by using some local variables get this down to the much more readable:

		cmd.data = state & BIT(RPMH_SLEEP_STATE) ? on_val : off_val;

And as off_val is 0 for all your clocks you can even drop the latter.

drivers/clk/qcom/clk-rpmh.c line 130

Quoted 5 lines
drivers/clk/qcom/clk-rpmh.c new file +5 -0
+		ret = rpmh_write_async(c->rpmh_client, RPMH_SLEEP_STATE,
+				       &cmd, 1);
+		if (ret) {
+			pr_err("%s: rpmh_write_async(%s, state=%d) failed (%d)\n",
+			       __func__, c->res_name, RPMH_SLEEP_STATE, ret);

Please drop the __func__, the error string is unique in this driver; and rather than passing a constant to a %d actually write your error message in a human readable form, like: "set sleep state of %s failed: %d\n".

And please do carry the struct device, so that you can use dev_err() instead.

drivers/clk/qcom/clk-rpmh.c line 141

Quoted 11 lines
drivers/clk/qcom/clk-rpmh.c new file +11 -0
+			return ret;
+		}
+	}
+
+	if (has_state_changed(c, RPMH_WAKE_ONLY_STATE)) {
+		cmd.data = (c->aggr_state & BIT(RPMH_WAKE_ONLY_STATE))
+				? c->res_on_val : c->res_off_val;
+		ret = rpmh_write_async(c->rpmh_client,
+				       RPMH_WAKE_ONLY_STATE, &cmd, 1);
+		if (ret) {
+			pr_err("%s: rpmh_write_async(%s, state=%d) failed (%d)\n"

You're missing a , for this to compile.

Quoted 8 lines
drivers/clk/qcom/clk-rpmh.c new file +8 -0
+			       __func__, c->res_name, RPMH_WAKE_ONLY_STATE,
+				 ret);
+			return ret;
+		}
+	}
+
+	if (has_state_changed(c, RPMH_ACTIVE_ONLY_STATE)) {
+		cmd.data = (c->aggr_state & BIT(RPMH_ACTIVE_ONLY_STATE))

drivers/clk/qcom/clk-rpmh.c line 159

Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c new file +10 -0
+				? c->res_on_val : c->res_off_val;
+		ret = rpmh_write(c->rpmh_client, RPMH_ACTIVE_ONLY_STATE,
+				 &cmd, 1);
+		if (ret) {
+			pr_err("%s: rpmh_write(%s, state=%d) failed (%d)\n",
+			       __func__, c->res_name, RPMH_ACTIVE_ONLY_STATE,
+				 ret);
+			return ret;
+		}
+	}

But, RPMH_SLEEP_STATE, RPMH_WAKE_ONLY_STATE and RPMH_ACTIVE_ONLY_STATE are values in an enum, so you could roll these up in a for loop and reduce the duplication.

Quoted 6 lines
drivers/clk/qcom/clk-rpmh.c new file +6 -0
+
+	c->last_sent_aggr_state = c->aggr_state;
+	c->peer->last_sent_aggr_state =  c->last_sent_aggr_state;
+
+	return 0;
+}

drivers/clk/qcom/clk-rpmh.c line 175

Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c new file +10 -0
+
+static int clk_rpmh_aggregate_state_send_command(struct clk_rpmh *c,
+						bool enable)
+{
+	/* Update state and aggregate state values based on enable value. */
+	c->state = enable ? c->valid_state_mask : 0;
+	c->aggr_state = c->state | c->peer->state;
+	c->peer->aggr_state = c->aggr_state;
+
+	ret = clk_rpmh_send_aggregate_command(c);

"ret" doesn't exist.

drivers/clk/qcom/clk-rpmh.c line 178

Quoted 3 lines
drivers/clk/qcom/clk-rpmh.c new file +3 -0
+	if (ret && enable)
+		c->state = 0;
+	else if (ret) {

If you have any part of your conditional wrapped in braces do wrap all the others too.

Quoted 69 lines
drivers/clk/qcom/clk-rpmh.c new file +69 -0
+		c->state = c->valid_state_mask;
+		WARN(1, "clk: %s failed to disable\n", c->res_name);
+	}
+
+	return ret;
+}
+
+static int clk_rpmh_prepare(struct clk_hw *hw)
+{
+	struct clk_rpmh *c = to_clk_rpmh(hw);
+	int ret = 0;
+
+	mutex_lock(&rpmh_clk_lock);
+
+	if (c->state)
+		goto out;
+
+	ret = clk_rpmh_aggregate_state_send_command(c, true);
+out:
+	mutex_unlock(&rpmh_clk_lock);
+	return ret;
+};
+
+static void clk_rpmh_unprepare(struct clk_hw *hw)
+{
+	struct clk_rpmh *c = to_clk_rpmh(hw);
+
+	mutex_lock(&rpmh_clk_lock);
+
+	if (!c->state)
+		goto out;
+
+	clk_rpmh_aggregate_state_send_command(c, false);
+out:
+	mutex_unlock(&rpmh_clk_lock);
+	return;
+};
+
+static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw,
+					  unsigned long parent_rate)
+{
+	struct clk_rpmh *r = to_clk_rpmh(hw);
+
+	/*
+	 * RPMh clocks have a fixed rate. Return static rate set
+	 * at init time.
+	 */
+	return r->rate;
+}
+
+static const struct clk_ops clk_rpmh_ops = {
+	.prepare	= clk_rpmh_prepare,
+	.unprepare	= clk_rpmh_unprepare,
+	.recalc_rate	= clk_rpmh_recalc_rate,
+};
+
+/* Resource name must match resource id present in cmd-db. */
+DEFINE_CLK_RPMH_ARC(sdm845, bi_tcxo, bi_tcxo_ao, "xo.lvl", 0x3, 0x0,
+		    19200000, CLK_RPMH_APPS_RSC_STATE_MASK,
+		    CLK_RPMH_APPS_RSC_AO_STATE_MASK);
+DEFINE_CLK_RPMH_VRM(sdm845, ln_bb_clk2, ln_bb_clk2_ao, "lnbclka2",
+		    19200000, CLK_RPMH_APPS_RSC_STATE_MASK,
+		    CLK_RPMH_APPS_RSC_AO_STATE_MASK);
+DEFINE_CLK_RPMH_VRM(sdm845, ln_bb_clk3, ln_bb_clk3_ao, "lnbclka3",
+		    19200000, CLK_RPMH_APPS_RSC_STATE_MASK,
+		    CLK_RPMH_APPS_RSC_AO_STATE_MASK);
+DEFINE_CLK_RPMH_VRM(sdm845, rf_clk1, rf_clk1_ao, "rfclka1",
+		    38400000, CLK_RPMH_APPS_RSC_STATE_MASK,
+		    CLK_RPMH_APPS_RSC_AO_STATE_MASK);

drivers/clk/qcom/clk-rpmh.c line 257

Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c new file +10 -0
+DEFINE_CLK_RPMH_VRM(sdm845, rf_clk2, rf_clk2_ao, "rfclka2",
+		    38400000, CLK_RPMH_APPS_RSC_STATE_MASK,
+		    CLK_RPMH_APPS_RSC_AO_STATE_MASK);
+DEFINE_CLK_RPMH_VRM(sdm845, rf_clk3, rf_clk3_ao, "rfclka3",
+		    38400000, CLK_RPMH_APPS_RSC_STATE_MASK,
+		    CLK_RPMH_APPS_RSC_AO_STATE_MASK);
+
+static struct clk_hw *sdm845_rpmh_clocks[] = {
+	[RPMH_CXO_CLK]		= &sdm845_bi_tcxo.hw,
+	[RPMH_CXO_CLK_A]	= &sdm845_bi_tcxo_ao.hw,

Registering these fails with EEXIST because the gcc driver also register them.

Quoted 12 lines
drivers/clk/qcom/clk-rpmh.c new file +12 -0
+	[RPMH_LN_BB_CLK2]	= &sdm845_ln_bb_clk2.hw,
+	[RPMH_LN_BB_CLK2_A]	= &sdm845_ln_bb_clk2_ao.hw,
+	[RPMH_LN_BB_CLK3]	= &sdm845_ln_bb_clk3.hw,
+	[RPMH_LN_BB_CLK3_A]	= &sdm845_ln_bb_clk3_ao.hw,
+	[RPMH_RF_CLK1]		= &sdm845_rf_clk1.hw,
+	[RPMH_RF_CLK1_A]	= &sdm845_rf_clk1_ao.hw,
+	[RPMH_RF_CLK2]		= &sdm845_rf_clk2.hw,
+	[RPMH_RF_CLK2_A]	= &sdm845_rf_clk2_ao.hw,
+	[RPMH_RF_CLK3]		= &sdm845_rf_clk3.hw,
+	[RPMH_RF_CLK3_A]	= &sdm845_rf_clk3_ao.hw,
+};
+

drivers/clk/qcom/clk-rpmh.c line 279

Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c new file +10 -0
+static const struct clk_rpmh_desc clk_rpmh_sdm845 = {
+	.clks = sdm845_rpmh_clocks,
+	.num_clks = ARRAY_SIZE(sdm845_rpmh_clocks),
+};
+
+static const struct of_device_id clk_rpmh_match_table[] = {
+	{ .compatible = "qcom,rpmh-clk-sdm845", .data = &clk_rpmh_sdm845},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, clk_rpmh_match_table);

Please move the match_table just prior to the definition of your platform_driver.

drivers/clk/qcom/clk-rpmh.c line 292

Quoted 13 lines
drivers/clk/qcom/clk-rpmh.c new file +13 -0
+
+static int clk_rpmh_probe(struct platform_device *pdev)
+{
+	struct clk **clks;
+	struct clk *clk;
+	struct rpmh_cc *rcc;
+	struct clk_onecell_data *data;
+	int ret;
+	size_t num_clks, i;
+	struct clk_hw **hw_clks;
+	struct clk_rpmh *rpmh_clk;
+	const struct clk_rpmh_desc *desc;
+	struct property *prop;

prop is unused.

drivers/clk/qcom/clk-rpmh.c line 293

Quoted 1 line
drivers/clk/qcom/clk-rpmh.c new file +1 -0
+	struct rpmh_client *rpmh_client = NULL;

Don't goto err until you have acquired rpmh_client and you don't need to initialize this variable.

drivers/clk/qcom/clk-rpmh.c line 299

Quoted 6 lines
drivers/clk/qcom/clk-rpmh.c new file +6 -0
+
+	desc = of_device_get_match_data(&pdev->dev);
+	if (!desc) {
+		ret = -EINVAL;
+		goto err;
+	}

This won't happen, no need to check for it.

drivers/clk/qcom/clk-rpmh.c line 306

Quoted 7 lines
drivers/clk/qcom/clk-rpmh.c new file +7 -0
+
+	ret = cmd_db_ready();
+	if (ret) {
+		if (ret != -EPROBE_DEFER) {
+			dev_err(&pdev->dev, "Command DB not available (%d)\n",
+				ret);
+			goto err;

goto err? There's nothing to clean up at this point.

drivers/clk/qcom/clk-rpmh.c line 314

Quoted 8 lines
drivers/clk/qcom/clk-rpmh.c new file +8 -0
+		}
+		return ret;
+	}
+
+	rpmh_client = rpmh_get_client(pdev);
+	if (IS_ERR(rpmh_client)) {
+		ret = PTR_ERR(rpmh_client);
+		if (ret != -EPROBE_DEFER)

You're getting a handle to your parent, it's not going to return -EPROBE_DEFER.

Quoted 24 lines
drivers/clk/qcom/clk-rpmh.c new file +24 -0
+			dev_err(&pdev->dev, "failed to request RPMh client, ret=%d\n",
+				ret);
+		return ret;
+	}
+
+	hw_clks = desc->clks;
+	num_clks = desc->num_clks;
+
+	rcc = devm_kzalloc(&pdev->dev, sizeof(*rcc) + sizeof(*clks) * num_clks,
+			   GFP_KERNEL);
+	if (!rcc) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	clks = rcc->clks;
+	data = &rcc->data;
+	data->clks = clks;
+	data->clk_num = num_clks;
+
+	for (i = 0; i < num_clks; i++) {
+		rpmh_clk = to_clk_rpmh(hw_clks[i]);
+		rpmh_clk->res_addr = cmd_db_read_addr(rpmh_clk->res_name);
+		if (!rpmh_clk->res_addr) {

drivers/clk/qcom/clk-rpmh.c line 348

Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c new file +10 -0
+			dev_err(&pdev->dev, "missing RPMh resource address for %s\n",
+				rpmh_clk->res_name);
+			ret = -ENODEV;
+			goto err;
+		}
+
+		rpmh_clk->rpmh_client = rpmh_client;
+
+		clk = devm_clk_register(&pdev->dev, hw_clks[i]);
+		if (IS_ERR(clk)) {

Please add:

dev_err(&pdev->dev, "failed to register %s\n", hw_clks[i]->init->name);

drivers/clk/qcom/clk-rpmh.c line 358

Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c new file +10 -0
+			ret = PTR_ERR(clk);
+			goto err;
+		}
+
+		clks[i] = clk;
+	}
+
+	ret = of_clk_add_provider(pdev->dev.of_node, of_clk_src_onecell_get,
+				  data);
+	if (ret)

Please add:

dev_err(&pdev->dev, "failed to add clock provider\n");

drivers/clk/qcom/clk-rpmh.c line 361

Quoted 3 lines
drivers/clk/qcom/clk-rpmh.c new file +3 -0
+		goto err;
+
+	dev_info(&pdev->dev, "Registered RPMh clocks\n");

Please don't spam the kernel log.

drivers/clk/qcom/clk-rpmh.c line 362

Quoted 1 line
drivers/clk/qcom/clk-rpmh.c new file +1 -0
+	return ret;

ret can't be anything but 0 here, so let's make it easer to read by writing 0 here.

drivers/clk/qcom/clk-rpmh.c line 366

Quoted 4 lines
drivers/clk/qcom/clk-rpmh.c new file +4 -0
+
+err:
+	if (rpmh_client)
+		rpmh_release(rpmh_client);

This is annoying (but not your fault).

drivers/clk/qcom/clk-rpmh.c line 368

Quoted 2 lines
drivers/clk/qcom/clk-rpmh.c new file +2 -0
+
+	dev_err(&pdev->dev, "Error registering RPMh Clock driver (%d)\n", ret);

Testing the driver I got this error message, it didn't help! Had to add the two dev_err above, just drop this one.

drivers/clk/qcom/clk-rpmh.c line 373

Quoted 5 lines
drivers/clk/qcom/clk-rpmh.c new file +5 -0
+	return ret;
+}
+
+static struct platform_driver clk_rpmh_driver = {
+	.probe		= clk_rpmh_probe,

Your driver is tristate and works just fine as a kernel module, so you need a remove function to call rpmh_release(rpmh_client) - or we need to get rid of the need to call that.

Quoted 9 lines
drivers/clk/qcom/clk-rpmh.c new file +9 -0
+	.driver		= {
+		.name	= "clk-rpmh",
+		.of_match_table = clk_rpmh_match_table,
+	},
+};
+
+static int __init clk_rpmh_init(void)
+{
+	return platform_driver_register(&clk_rpmh_driver);

drivers/clk/qcom/clk-rpmh.c line 392

Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c new file +10 -0
+}
+subsys_initcall(clk_rpmh_init);
+
+static void __exit clk_rpmh_exit(void)
+{
+	platform_driver_unregister(&clk_rpmh_driver);
+}
+module_exit(clk_rpmh_exit);
+
+MODULE_DESCRIPTION("QTI RPMh Clock Driver");

We use "Qualcomm" or "QCOM" in all existing driver, can you please use that here too?

drivers/clk/qcom/clk-rpmh.c line 394

Quoted 2 lines
drivers/clk/qcom/clk-rpmh.c new file +2 -0
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:clk-rpmh");

Nothing is going to attempt to autoload a driver based on the alias platform:clk-rpmh, so just drop this.

Regards,
Bjorn

author
#3 lore raw

Re: [[PATCH v2] 1/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver

Hello Bjorn,

Thank you for the review comments.

Quoted 27 lines · Bjorn Andersson
Quoted 25 lines · Taniya Das

From: Amit Nischal <[email protected]>

Add the RPMh clock driver to control the RPMh managed clock resources on some of the Qualcomm Technologies, Inc. SoCs.

Signed-off-by David Collins <[email protected]>
Signed-off-by Amit Nischal <[email protected]>
Signed-off-by Taniya Das <[email protected]>
---
  drivers/clk/qcom/Kconfig              |   9 +
  drivers/clk/qcom/Makefile             |   1 +
  drivers/clk/qcom/clk-rpmh.c           | 394 ++++++++++++++++++++++++++++++++++
  include/dt-bindings/clock/qcom,rpmh.h |  25 +++
  4 files changed, 429 insertions(+)

create mode 100644 drivers/clk/qcom/clk-rpmh.c create mode 100644 include/dt-bindings/clock/qcom,rpmh.h

drivers/clk/qcom/Kconfig +0 -0
index fbf4532..3697a6a 100644
@@ -112,6 +112,15 @@ config IPQ_GCC_8074
  	  i2c, USB, SD/eMMC, etc. Select this for the root clock
  	  of ipq8074.
Quoted 4 lines

drivers/clk/qcom/Kconfig config IPQ_GCC_8074 · line 115

Quoted 1 line
drivers/clk/qcom/Kconfig +1 -0
+config MSM_CLK_RPMH

QCOM_CLK_RPMH

Would update it to use "QCOM_CLK_RPMH".

Quoted 46 lines
Quoted 33 lines
drivers/clk/qcom/Kconfig +8 -0
+	tristate "RPMh Clock Driver"
+	depends on COMMON_CLK_QCOM && QCOM_RPMH
+	help
+	 RPMh manages shared resources on some Qualcomm Technologies, Inc.
+	 SoCs. It accepts requests from other hardware subsystems via RSC.
+	 Say Y if you want to support the clocks exposed by RPMh on
+	 platforms such as sdm845.
+
  config MSM_GCC_8660
  	tristate "MSM8660 Global Clock Controller"
  	depends on COMMON_CLK_QCOM
drivers/clk/qcom/Makefile +1 -0
index 230332c..b7da05b 100644
@@ -23,6 +23,7 @@ obj-$(CONFIG_IPQ_GCC_8074) += gcc-ipq8074.o
  obj-$(CONFIG_IPQ_LCC_806X) += lcc-ipq806x.o
  obj-$(CONFIG_MDM_GCC_9615) += gcc-mdm9615.o
  obj-$(CONFIG_MDM_LCC_9615) += lcc-mdm9615.o
+obj-$(CONFIG_MSM_CLK_RPMH) += clk-rpmh.o
  obj-$(CONFIG_MSM_GCC_8660) += gcc-msm8660.o
  obj-$(CONFIG_MSM_GCC_8916) += gcc-msm8916.o
  obj-$(CONFIG_MSM_GCC_8960) += gcc-msm8960.o
drivers/clk/qcom/clk-rpmh.c new file +4 -0
new file mode 100644
index 0000000..763401f
@@ -0,0 +1,394 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
+ */

drivers/clk/qcom/clk-rpmh.c line 14

Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c new file +10 -0
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>

Unused

Quoted 10 lines

drivers/clk/qcom/clk-rpmh.c line 21

Quoted 7 lines
drivers/clk/qcom/clk-rpmh.c new file +7 -0
+#include <soc/qcom/cmd-db.h>
+#include <soc/qcom/rpmh.h>
+
+#include <dt-bindings/clock/qcom,rpmh.h>
+
+#include "common.h"
+#include "clk-regmap.h"

Unused

I would remove the unused header includes.

Quoted 7 lines

drivers/clk/qcom/clk-rpmh.c line 26

Quoted 5 lines
drivers/clk/qcom/clk-rpmh.c new file +5 -0
+
+#define CLK_RPMH_ARC_EN_OFFSET 0
+#define CLK_RPMH_VRM_EN_OFFSET 4
+#define CLK_RPMH_VRM_OFF_VAL 0
+#define CLK_RPMH_VRM_ON_VAL 1

Use a bool (true/false) rather than lugging around these two defines.

I would remove these in the next patch.

Quoted 16 lines

drivers/clk/qcom/clk-rpmh.c line 38

Quoted 13 lines
drivers/clk/qcom/clk-rpmh.c new file +12 -0
+#define CLK_RPMH_APPS_RSC_AO_STATE_MASK (BIT(RPMH_WAKE_ONLY_STATE) | \
+					 BIT(RPMH_ACTIVE_ONLY_STATE))
+#define CLK_RPMH_APPS_RSC_STATE_MASK (BIT(RPMH_WAKE_ONLY_STATE) | \
+				      BIT(RPMH_ACTIVE_ONLY_STATE) | \
+				      BIT(RPMH_SLEEP_STATE))
+struct clk_rpmh {
+	struct clk_hw hw;
+	const char *res_name;
+	u32 res_addr;
+	u32 res_en_offset;
+	u32 res_on_val;
+	u32 res_off_val;

res_off_val is 0 for all clocks.

They could change if required, so is the reason for keeping it.

Quoted 7 lines

drivers/clk/qcom/clk-rpmh.c line 41

Quoted 3 lines
drivers/clk/qcom/clk-rpmh.c new file +3 -0
+	u32 state;
+	u32 aggr_state;
+	u32 last_sent_aggr_state;

Through the use of some local variables you shouldn't have to lug around aggr_state and last_sent_aggr_state.

We need to check if the last state and the current state requested, has_state_changed().

Quoted 42 lines
Quoted 20 lines
drivers/clk/qcom/clk-rpmh.c new file +20 -0
+	u32 valid_state_mask;
+	struct rpmh_client *rpmh_client;
+	unsigned long rate;
+	struct clk_rpmh *peer;
+};
+
+struct rpmh_cc {
+	struct clk_onecell_data data;
+	struct clk *clks[];
+};
+
+struct clk_rpmh_desc {
+	struct clk_hw **clks;
+	size_t num_clks;
+};
+
+static DEFINE_MUTEX(rpmh_clk_lock);
+
+#define __DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name, \
+			  _res_en_offset, _res_on, _res_off, _rate, \

drivers/clk/qcom/clk-rpmh.c line 71

Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c new file +10 -0
+			  _state_mask, _state_on_mask)			      \
+	static struct clk_rpmh _platform##_##_name_active;		      \
+	static struct clk_rpmh _platform##_##_name = {			      \
+		.res_name = _res_name,					      \
+		.res_en_offset = _res_en_offset,			      \
+		.res_on_val = _res_on,					      \
+		.res_off_val = _res_off,				      \
+		.rate = _rate,						      \
+		.peer = &_platform##_##_name_active,			      \
+		.valid_state_mask = _state_mask,			      \

This is always CLK_RPMH_APPS_RSC_STATE_MASK

Quoted 9 lines
drivers/clk/qcom/clk-rpmh.c new file +9 -0
+		.hw.init = &(struct clk_init_data){			      \
+			.ops = &clk_rpmh_ops,			              \
+			.name = #_name,					      \
+		},							      \
+	};								      \
+	static struct clk_rpmh _platform##_##_name_active = {		      \
+		.res_name = _res_name,					      \
+		.res_en_offset = _res_en_offset,			      \
+		.res_on_val = _res_on,					      \
Quoted 9 lines

drivers/clk/qcom/clk-rpmh.c line 84

Quoted 4 lines
drivers/clk/qcom/clk-rpmh.c new file +4 -0
+		.res_off_val = _res_off,				      \
+		.rate = _rate,						      \
+		.peer = &_platform##_##_name,				      \
+		.valid_state_mask = _state_on_mask,			      \

and this is always CLK_RPMH_APPS_RSC_AO_STATE_MASK. If you just hard code them here (until there's a need for them to be anything else) the clock list below becomes tidier.

As of now the state_mask is CLK_RPMH_APPS_RSC_STATE_MASK and state_on_mask is CLK_RPMH_APPS_RSC_AO_STATE_MASK. I would update the logic if the state masks changes.

Quoted 59 lines
Quoted 31 lines
drivers/clk/qcom/clk-rpmh.c new file +31 -0
+		.hw.init = &(struct clk_init_data){			      \
+			.ops = &clk_rpmh_ops,				      \
+			.name = #_name_active,				      \
+		},							      \
+	}
+
+#define DEFINE_CLK_RPMH_ARC(_platform, _name, _name_active, _res_name, \
+			    _res_on, _res_off, _rate, _state_mask, \
+			    _state_on_mask)				\
+	__DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name,	\
+			  CLK_RPMH_ARC_EN_OFFSET, _res_on, _res_off, \
+			  _rate, _state_mask, _state_on_mask)
+
+#define DEFINE_CLK_RPMH_VRM(_platform, _name, _name_active, _res_name,	\
+			    _rate, _state_mask, _state_on_mask) \
+	__DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name,	\
+			  CLK_RPMH_VRM_EN_OFFSET, CLK_RPMH_VRM_ON_VAL,	\
+			  CLK_RPMH_VRM_OFF_VAL, _rate, _state_mask, \
+			  _state_on_mask)
+
+static inline struct clk_rpmh *to_clk_rpmh(struct clk_hw *_hw)
+{
+	return container_of(_hw, struct clk_rpmh, hw);
+}
+
+static inline bool has_state_changed(struct clk_rpmh *c, u32 state)
+{
+	return ((c->last_sent_aggr_state & BIT(state))
+		!= (c->aggr_state & BIT(state)));
+}
+

drivers/clk/qcom/clk-rpmh.c line 125

Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c new file +10 -0
+static int clk_rpmh_send_aggregate_command(struct clk_rpmh *c)
+{
+	struct tcs_cmd cmd = { 0 };
+	int ret = 0;
+
+	cmd.addr = c->res_addr + c->res_en_offset;
+
+	if (has_state_changed(c, RPMH_SLEEP_STATE)) {
+		cmd.data = (c->aggr_state & BIT(RPMH_SLEEP_STATE))
+				? c->res_on_val : c->res_off_val;

As these values are reused several times in this function you could by using some local variables get this down to the much more readable:

		cmd.data = state & BIT(RPMH_SLEEP_STATE) ? on_val : off_val;

And as off_val is 0 for all your clocks you can even drop the latter.

drivers/clk/qcom/clk-rpmh.c line 130

Quoted 5 lines
drivers/clk/qcom/clk-rpmh.c new file +5 -0
+		ret = rpmh_write_async(c->rpmh_client, RPMH_SLEEP_STATE,
+				       &cmd, 1);
+		if (ret) {
+			pr_err("%s: rpmh_write_async(%s, state=%d) failed (%d)\n",
+			       __func__, c->res_name, RPMH_SLEEP_STATE, ret);

Please drop the __func__, the error string is unique in this driver; and rather than passing a constant to a %d actually write your error message in a human readable form, like: "set sleep state of %s failed: %d\n".

Quoted 3 lines

And please do carry the struct device, so that you can use dev_err() instead.

Thanks, I would take care of the above comments and also loop thorugh the enum to avoid duplication in the next patch.

Quoted 5 lines
Quoted 5 lines
drivers/clk/qcom/clk-rpmh.c new file +5 -0
+			return ret;
+		}
+	}
+
+	if (has_state_changed(c, RPMH_WAKE_ONLY_STATE)) {
Quoted 9 lines

drivers/clk/qcom/clk-rpmh.c line 141

Quoted 6 lines
drivers/clk/qcom/clk-rpmh.c new file +6 -0
+		cmd.data = (c->aggr_state & BIT(RPMH_WAKE_ONLY_STATE))
+				? c->res_on_val : c->res_off_val;
+		ret = rpmh_write_async(c->rpmh_client,
+				       RPMH_WAKE_ONLY_STATE, &cmd, 1);
+		if (ret) {
+			pr_err("%s: rpmh_write_async(%s, state=%d) failed (%d)\n"

You're missing a , for this to compile.

Thanks, would fix in the next patch.

Quoted 33 lines
Quoted 8 lines
drivers/clk/qcom/clk-rpmh.c new file +8 -0
+			       __func__, c->res_name, RPMH_WAKE_ONLY_STATE,
+				 ret);
+			return ret;
+		}
+	}
+
+	if (has_state_changed(c, RPMH_ACTIVE_ONLY_STATE)) {
+		cmd.data = (c->aggr_state & BIT(RPMH_ACTIVE_ONLY_STATE))

drivers/clk/qcom/clk-rpmh.c line 159

Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c new file +10 -0
+				? c->res_on_val : c->res_off_val;
+		ret = rpmh_write(c->rpmh_client, RPMH_ACTIVE_ONLY_STATE,
+				 &cmd, 1);
+		if (ret) {
+			pr_err("%s: rpmh_write(%s, state=%d) failed (%d)\n",
+			       __func__, c->res_name, RPMH_ACTIVE_ONLY_STATE,
+				 ret);
+			return ret;
+		}
+	}

But, RPMH_SLEEP_STATE, RPMH_WAKE_ONLY_STATE and RPMH_ACTIVE_ONLY_STATE are values in an enum, so you could roll these up in a for loop and reduce the duplication.

Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c new file +10 -0
+
+	c->last_sent_aggr_state = c->aggr_state;
+	c->peer->last_sent_aggr_state =  c->last_sent_aggr_state;
+
+	return 0;
+}
+
+static int clk_rpmh_aggregate_state_send_command(struct clk_rpmh *c,
+						bool enable)
+{
Quoted 9 lines

drivers/clk/qcom/clk-rpmh.c line 175

Quoted 6 lines
drivers/clk/qcom/clk-rpmh.c new file +6 -0
+	/* Update state and aggregate state values based on enable value. */
+	c->state = enable ? c->valid_state_mask : 0;
+	c->aggr_state = c->state | c->peer->state;
+	c->peer->aggr_state = c->aggr_state;
+
+	ret = clk_rpmh_send_aggregate_command(c);

"ret" doesn't exist.

Thanks, I would add the missing variable.

Quoted 7 lines

drivers/clk/qcom/clk-rpmh.c line 178

Quoted 3 lines
drivers/clk/qcom/clk-rpmh.c new file +3 -0
+	if (ret && enable)
+		c->state = 0;
+	else if (ret) {

If you have any part of your conditional wrapped in braces do wrap all the others too.

Thanks, would take care in the next patch.

Quoted 74 lines
Quoted 74 lines
drivers/clk/qcom/clk-rpmh.c new file +74 -0
+		c->state = c->valid_state_mask;
+		WARN(1, "clk: %s failed to disable\n", c->res_name);
+	}
+
+	return ret;
+}
+
+static int clk_rpmh_prepare(struct clk_hw *hw)
+{
+	struct clk_rpmh *c = to_clk_rpmh(hw);
+	int ret = 0;
+
+	mutex_lock(&rpmh_clk_lock);
+
+	if (c->state)
+		goto out;
+
+	ret = clk_rpmh_aggregate_state_send_command(c, true);
+out:
+	mutex_unlock(&rpmh_clk_lock);
+	return ret;
+};
+
+static void clk_rpmh_unprepare(struct clk_hw *hw)
+{
+	struct clk_rpmh *c = to_clk_rpmh(hw);
+
+	mutex_lock(&rpmh_clk_lock);
+
+	if (!c->state)
+		goto out;
+
+	clk_rpmh_aggregate_state_send_command(c, false);
+out:
+	mutex_unlock(&rpmh_clk_lock);
+	return;
+};
+
+static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw,
+					  unsigned long parent_rate)
+{
+	struct clk_rpmh *r = to_clk_rpmh(hw);
+
+	/*
+	 * RPMh clocks have a fixed rate. Return static rate set
+	 * at init time.
+	 */
+	return r->rate;
+}
+
+static const struct clk_ops clk_rpmh_ops = {
+	.prepare	= clk_rpmh_prepare,
+	.unprepare	= clk_rpmh_unprepare,
+	.recalc_rate	= clk_rpmh_recalc_rate,
+};
+
+/* Resource name must match resource id present in cmd-db. */
+DEFINE_CLK_RPMH_ARC(sdm845, bi_tcxo, bi_tcxo_ao, "xo.lvl", 0x3, 0x0,
+		    19200000, CLK_RPMH_APPS_RSC_STATE_MASK,
+		    CLK_RPMH_APPS_RSC_AO_STATE_MASK);
+DEFINE_CLK_RPMH_VRM(sdm845, ln_bb_clk2, ln_bb_clk2_ao, "lnbclka2",
+		    19200000, CLK_RPMH_APPS_RSC_STATE_MASK,
+		    CLK_RPMH_APPS_RSC_AO_STATE_MASK);
+DEFINE_CLK_RPMH_VRM(sdm845, ln_bb_clk3, ln_bb_clk3_ao, "lnbclka3",
+		    19200000, CLK_RPMH_APPS_RSC_STATE_MASK,
+		    CLK_RPMH_APPS_RSC_AO_STATE_MASK);
+DEFINE_CLK_RPMH_VRM(sdm845, rf_clk1, rf_clk1_ao, "rfclka1",
+		    38400000, CLK_RPMH_APPS_RSC_STATE_MASK,
+		    CLK_RPMH_APPS_RSC_AO_STATE_MASK);
+DEFINE_CLK_RPMH_VRM(sdm845, rf_clk2, rf_clk2_ao, "rfclka2",
+		    38400000, CLK_RPMH_APPS_RSC_STATE_MASK,
+		    CLK_RPMH_APPS_RSC_AO_STATE_MASK);
+DEFINE_CLK_RPMH_VRM(sdm845, rf_clk3, rf_clk3_ao, "rfclka3",
+		    38400000, CLK_RPMH_APPS_RSC_STATE_MASK,
Quoted 9 lines

drivers/clk/qcom/clk-rpmh.c line 257

Quoted 5 lines
drivers/clk/qcom/clk-rpmh.c new file +5 -0
+		    CLK_RPMH_APPS_RSC_AO_STATE_MASK);
+
+static struct clk_hw *sdm845_rpmh_clocks[] = {
+	[RPMH_CXO_CLK]		= &sdm845_bi_tcxo.hw,
+	[RPMH_CXO_CLK_A]	= &sdm845_bi_tcxo_ao.hw,

Registering these fails with EEXIST because the gcc driver also register them.

It was added till the time RPMh driver was not ready. We would submit removing the change from GCC driver.

Quoted 17 lines
Quoted 17 lines
drivers/clk/qcom/clk-rpmh.c new file +17 -0
+	[RPMH_LN_BB_CLK2]	= &sdm845_ln_bb_clk2.hw,
+	[RPMH_LN_BB_CLK2_A]	= &sdm845_ln_bb_clk2_ao.hw,
+	[RPMH_LN_BB_CLK3]	= &sdm845_ln_bb_clk3.hw,
+	[RPMH_LN_BB_CLK3_A]	= &sdm845_ln_bb_clk3_ao.hw,
+	[RPMH_RF_CLK1]		= &sdm845_rf_clk1.hw,
+	[RPMH_RF_CLK1_A]	= &sdm845_rf_clk1_ao.hw,
+	[RPMH_RF_CLK2]		= &sdm845_rf_clk2.hw,
+	[RPMH_RF_CLK2_A]	= &sdm845_rf_clk2_ao.hw,
+	[RPMH_RF_CLK3]		= &sdm845_rf_clk3.hw,
+	[RPMH_RF_CLK3_A]	= &sdm845_rf_clk3_ao.hw,
+};
+
+static const struct clk_rpmh_desc clk_rpmh_sdm845 = {
+	.clks = sdm845_rpmh_clocks,
+	.num_clks = ARRAY_SIZE(sdm845_rpmh_clocks),
+};
+
Quoted 9 lines

drivers/clk/qcom/clk-rpmh.c line 279

Quoted 5 lines
drivers/clk/qcom/clk-rpmh.c new file +5 -0
+static const struct of_device_id clk_rpmh_match_table[] = {
+	{ .compatible = "qcom,rpmh-clk-sdm845", .data = &clk_rpmh_sdm845},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, clk_rpmh_match_table);

Please move the match_table just prior to the definition of your platform_driver.

Would update this in the next patch.

Quoted 7 lines
Quoted 7 lines
drivers/clk/qcom/clk-rpmh.c new file +7 -0
+
+static int clk_rpmh_probe(struct platform_device *pdev)
+{
+	struct clk **clks;
+	struct clk *clk;
+	struct rpmh_cc *rcc;
+	struct clk_onecell_data *data;
Quoted 9 lines

drivers/clk/qcom/clk-rpmh.c line 292

Quoted 6 lines
drivers/clk/qcom/clk-rpmh.c new file +6 -0
+	int ret;
+	size_t num_clks, i;
+	struct clk_hw **hw_clks;
+	struct clk_rpmh *rpmh_clk;
+	const struct clk_rpmh_desc *desc;
+	struct property *prop;

prop is unused.

Thanks, would remove the unused variable.

Quoted 5 lines

drivers/clk/qcom/clk-rpmh.c line 293

Quoted 1 line
drivers/clk/qcom/clk-rpmh.c new file +1 -0
+	struct rpmh_client *rpmh_client = NULL;

Don't goto err until you have acquired rpmh_client and you don't need to initialize this variable.

Would remove the variable initialization in the next patch.

Quoted 9 lines

drivers/clk/qcom/clk-rpmh.c line 299

Quoted 6 lines
drivers/clk/qcom/clk-rpmh.c new file +6 -0
+
+	desc = of_device_get_match_data(&pdev->dev);
+	if (!desc) {
+		ret = -EINVAL;
+		goto err;
+	}

This won't happen, no need to check for it.

Would remove this check too in the next patch.

Quoted 10 lines

drivers/clk/qcom/clk-rpmh.c line 306

Quoted 7 lines
drivers/clk/qcom/clk-rpmh.c new file +7 -0
+
+	ret = cmd_db_ready();
+	if (ret) {
+		if (ret != -EPROBE_DEFER) {
+			dev_err(&pdev->dev, "Command DB not available (%d)\n",
+				ret);
+			goto err;

goto err? There's nothing to clean up at this point.

I would fix this in the next patch.

Quoted 12 lines

drivers/clk/qcom/clk-rpmh.c line 314

Quoted 8 lines
drivers/clk/qcom/clk-rpmh.c new file +8 -0
+		}
+		return ret;
+	}
+
+	rpmh_client = rpmh_get_client(pdev);
+	if (IS_ERR(rpmh_client)) {
+		ret = PTR_ERR(rpmh_client);
+		if (ret != -EPROBE_DEFER)

You're getting a handle to your parent, it's not going to return -EPROBE_DEFER.

I would fix the error path in the next patch.

Quoted 72 lines
Quoted 24 lines
drivers/clk/qcom/clk-rpmh.c new file +24 -0
+			dev_err(&pdev->dev, "failed to request RPMh client, ret=%d\n",
+				ret);
+		return ret;
+	}
+
+	hw_clks = desc->clks;
+	num_clks = desc->num_clks;
+
+	rcc = devm_kzalloc(&pdev->dev, sizeof(*rcc) + sizeof(*clks) * num_clks,
+			   GFP_KERNEL);
+	if (!rcc) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	clks = rcc->clks;
+	data = &rcc->data;
+	data->clks = clks;
+	data->clk_num = num_clks;
+
+	for (i = 0; i < num_clks; i++) {
+		rpmh_clk = to_clk_rpmh(hw_clks[i]);
+		rpmh_clk->res_addr = cmd_db_read_addr(rpmh_clk->res_name);
+		if (!rpmh_clk->res_addr) {

drivers/clk/qcom/clk-rpmh.c line 348

Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c new file +10 -0
+			dev_err(&pdev->dev, "missing RPMh resource address for %s\n",
+				rpmh_clk->res_name);
+			ret = -ENODEV;
+			goto err;
+		}
+
+		rpmh_clk->rpmh_client = rpmh_client;
+
+		clk = devm_clk_register(&pdev->dev, hw_clks[i]);
+		if (IS_ERR(clk)) {

Please add:

dev_err(&pdev->dev, "failed to register %s\n", hw_clks[i]->init->name);

drivers/clk/qcom/clk-rpmh.c line 358

Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c new file +10 -0
+			ret = PTR_ERR(clk);
+			goto err;
+		}
+
+		clks[i] = clk;
+	}
+
+	ret = of_clk_add_provider(pdev->dev.of_node, of_clk_src_onecell_get,
+				  data);
+	if (ret)

Please add:

dev_err(&pdev->dev, "failed to add clock provider\n");

drivers/clk/qcom/clk-rpmh.c line 361

Quoted 3 lines
drivers/clk/qcom/clk-rpmh.c new file +3 -0
+		goto err;
+
+	dev_info(&pdev->dev, "Registered RPMh clocks\n");

Please don't spam the kernel log.

drivers/clk/qcom/clk-rpmh.c line 362

Quoted 1 line
drivers/clk/qcom/clk-rpmh.c new file +1 -0
+	return ret;

ret can't be anything but 0 here, so let's make it easer to read by writing 0 here.

drivers/clk/qcom/clk-rpmh.c line 366

Quoted 4 lines
drivers/clk/qcom/clk-rpmh.c new file +4 -0
+
+err:
+	if (rpmh_client)
+		rpmh_release(rpmh_client);

This is annoying (but not your fault).

Quoted 6 lines

drivers/clk/qcom/clk-rpmh.c line 368

Quoted 2 lines
drivers/clk/qcom/clk-rpmh.c new file +2 -0
+
+	dev_err(&pdev->dev, "Error registering RPMh Clock driver (%d)\n", ret);

Testing the driver I got this error message, it didn't help! Had to add the two dev_err above, just drop this one.

I would take care of the above comments in the next patch.

Quoted 10 lines

drivers/clk/qcom/clk-rpmh.c line 373

Quoted 5 lines
drivers/clk/qcom/clk-rpmh.c new file +5 -0
+	return ret;
+}
+
+static struct platform_driver clk_rpmh_driver = {
+	.probe		= clk_rpmh_probe,

Your driver is tristate and works just fine as a kernel module, so you need a remove function to call rpmh_release(rpmh_client) - or we need to get rid of the need to call that.

Yes, I would add the remove and do the rpmh_client and of_del_provider clean ups.

Quoted 14 lines
Quoted 14 lines
drivers/clk/qcom/clk-rpmh.c new file +14 -0
+	.driver		= {
+		.name	= "clk-rpmh",
+		.of_match_table = clk_rpmh_match_table,
+	},
+};
+
+static int __init clk_rpmh_init(void)
+{
+	return platform_driver_register(&clk_rpmh_driver);
+}
+subsys_initcall(clk_rpmh_init);
+
+static void __exit clk_rpmh_exit(void)
+{
Quoted 9 lines

drivers/clk/qcom/clk-rpmh.c line 392

Quoted 5 lines
drivers/clk/qcom/clk-rpmh.c new file +5 -0
+	platform_driver_unregister(&clk_rpmh_driver);
+}
+module_exit(clk_rpmh_exit);
+
+MODULE_DESCRIPTION("QTI RPMh Clock Driver");

We use "Qualcomm" or "QCOM" in all existing driver, can you please use that here too?

Would update it to use "QCOM".

Quoted 6 lines

drivers/clk/qcom/clk-rpmh.c line 394

Quoted 2 lines
drivers/clk/qcom/clk-rpmh.c new file +2 -0
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:clk-rpmh");

Nothing is going to attempt to autoload a driver based on the alias platform:clk-rpmh, so just drop this.

Would drop this in the next patch.

Quoted 3 lines

Regards,
Bjorn

Signature · 4 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.

--
maintainer
#4 lore raw

Re: [[PATCH v2] 1/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver

In reply to Taniya Das
Quoted 8 lines · Taniya Das

From: Amit Nischal <[email protected]>

Add the RPMh clock driver to control the RPMh managed clock resources on some of the Qualcomm Technologies, Inc. SoCs.

Signed-off-by David Collins <[email protected]>
Signed-off-by Amit Nischal <[email protected]>
Signed-off-by Taniya Das <[email protected]>
Quoted 6 lines
---
 drivers/clk/qcom/Kconfig              |   9 +
 drivers/clk/qcom/Makefile             |   1 +
 drivers/clk/qcom/clk-rpmh.c           | 394 ++++++++++++++++++++++++++++++++++
 include/dt-bindings/clock/qcom,rpmh.h |  25 +++

This goes with the binding patch which should come first in the series.

Quoted 3 lines
 4 files changed, 429 insertions(+)
 create mode 100644 drivers/clk/qcom/clk-rpmh.c
 create mode 100644 include/dt-bindings/clock/qcom,rpmh.h
author
#5 lore raw

Re: [[PATCH v2] 1/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver

Hello Rob,

Thank you for the review comments.

Quoted 14 lines · Rob Herring
Quoted 12 lines · Taniya Das

From: Amit Nischal <[email protected]>

Add the RPMh clock driver to control the RPMh managed clock resources on some of the Qualcomm Technologies, Inc. SoCs.

Signed-off-by David Collins <[email protected]>
Signed-off-by Amit Nischal <[email protected]>
Signed-off-by Taniya Das <[email protected]>
---
  drivers/clk/qcom/Kconfig              |   9 +
  drivers/clk/qcom/Makefile             |   1 +
  drivers/clk/qcom/clk-rpmh.c           | 394 ++++++++++++++++++++++++++++++++++
Quoted 4 lines
Quoted 1 line
  include/dt-bindings/clock/qcom,rpmh.h |  25 +++

This goes with the binding patch which should come first in the series.

I would make the change in the next patch series.

Quoted 3 lines
Quoted 3 lines
  4 files changed, 429 insertions(+)

create mode 100644 drivers/clk/qcom/clk-rpmh.c create mode 100644 include/dt-bindings/clock/qcom,rpmh.h

Signature · 4 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.

--
author
#6 lore raw

PATCH v2 1/1 2/2] dt-bindings: clock: Introduce QCOM RPMh clock bindings

In reply to Taniya Das

From: Amit Nischal <[email protected]>

Add RPMh clock device bindings for Qualcomm Technology Inc's SoCs. These devices would be used for communicating resource state requests to control the clocks managed by RPMh.

Signed-off-by Amit Nischal <[email protected]>
Signed-off-by Taniya Das <[email protected]>
---
 .../devicetree/bindings/clock/qcom,rpmh-clk.txt    | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt
Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt new file +22 -0
new file mode 100644
index 0000000..4ade82b
@@ -0,0 +1,22 @@
+Qualcomm Technologies, Inc. RPMh Clocks
+-------------------------------------------------------
+
+Resource Power Manager Hardened (RPMh) manages shared resources on
+some Qualcomm Technologies Inc. SoCs. It accepts clock requests from
+other hardware subsystems via RSC to control clocks.
+
+Required properties :
+- compatible : shall contain "qcom,rpmh-clk-sdm845"
+
+- #clock-cells : must contain 1
+
+Example :
+
+#include <dt-bindings/clock/qcom,rpmh.h>
+
+	&apps_rsc {
+		rpmh: clock-controller {
+			compatible = "qcom,rpmh-clk-sdm845";
+			#clock-cells = <1>;
+		};
+	};
Signature · 2 lines
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the  Linux Foundation.
#7 lore raw

Re: [[PATCH v2] 2/2] dt-bindings: clock: Introduce QCOM RPMh clock bindings

Quoted 19 lines · Taniya Das

From: Amit Nischal <[email protected]>

Add RPMh clock device bindings for Qualcomm Technology Inc's SoCs. These devices would be used for communicating resource state requests to control the clocks managed by RPMh.

Signed-off-by Amit Nischal <[email protected]>
Signed-off-by Taniya Das <[email protected]>
---
 .../devicetree/bindings/clock/qcom,rpmh-clk.txt    | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt
Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt new file +0 -0
new file mode 100644
index 0000000..4ade82b

Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt line 9

Quoted 9 lines
Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt new file +9 -0
@@ -0,0 +1,22 @@
+Qualcomm Technologies, Inc. RPMh Clocks
+-------------------------------------------------------
+
+Resource Power Manager Hardened (RPMh) manages shared resources on
+some Qualcomm Technologies Inc. SoCs. It accepts clock requests from
+other hardware subsystems via RSC to control clocks.
+
+Required properties :
+- compatible : shall contain "qcom,rpmh-clk-sdm845"

We have a chance to fix this to Rob's liking, so please make this "qcom,sdm845-rpmh-clock" (or maybe "qcom,sdm845-rpmhcc"?)

Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt line 18

Quoted 9 lines
Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt new file +9 -0
+
+- #clock-cells : must contain 1
+
+Example :
+
+#include <dt-bindings/clock/qcom,rpmh.h>
+
+	&apps_rsc {
+		rpmh: clock-controller {

I believe the clock-controller, the power-domain and the bus-scaler are equal contenders of the label "rpmh", so please make it more specific. How about "rpmhcc" to mimic previous platforms?

Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt line 22

Quoted 4 lines
Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt new file +4 -0
+			compatible = "qcom,rpmh-clk-sdm845";
+			#clock-cells = <1>;
+		};
+	};

Regards,
Bjorn

author
#8 lore raw

Re: [[PATCH v2] 2/2] dt-bindings: clock: Introduce QCOM RPMh clock bindings

Hello Bjorn,

Thanks for your review comments.

Quoted 25 lines · Bjorn Andersson
Quoted 23 lines · Taniya Das

From: Amit Nischal <[email protected]>

Add RPMh clock device bindings for Qualcomm Technology Inc's SoCs. These devices would be used for communicating resource state requests to control the clocks managed by RPMh.

Signed-off-by Amit Nischal <[email protected]>
Signed-off-by Taniya Das <[email protected]>
---
  .../devicetree/bindings/clock/qcom,rpmh-clk.txt    | 22 ++++++++++++++++++++++
  1 file changed, 22 insertions(+)

create mode 100644 Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt

Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt new file +4 -0
new file mode 100644
index 0000000..4ade82b
@@ -0,0 +1,22 @@
+Qualcomm Technologies, Inc. RPMh Clocks
+-------------------------------------------------------
+
+Resource Power Manager Hardened (RPMh) manages shared resources on
Quoted 8 lines

Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt line 9

Quoted 5 lines
Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt new file +5 -0
+some Qualcomm Technologies Inc. SoCs. It accepts clock requests from
+other hardware subsystems via RSC to control clocks.
+
+Required properties :
+- compatible : shall contain "qcom,rpmh-clk-sdm845"

We have a chance to fix this to Rob's liking, so please make this "qcom,sdm845-rpmh-clock" (or maybe "qcom,sdm845-rpmhcc"?)

I would fix it to "qcom,sdm845-rpmh-clk".

Quoted 15 lines

Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt line 18

Quoted 10 lines
Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt new file +9 -0
+
+- #clock-cells : must contain 1
+
+Example :
+
+#include <dt-bindings/clock/qcom,rpmh.h>
+
+	&apps_rsc {
+		rpmh: clock-controller {

I believe the clock-controller, the power-domain and the bus-scaler are equal contenders of the label "rpmh", so please make it more specific. How about "rpmhcc" to mimic previous platforms?

Would update it "rpmhcc".

Quoted 8 lines

Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt line 22

Quoted 4 lines
Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt new file +4 -0
+			compatible = "qcom,rpmh-clk-sdm845";
+			#clock-cells = <1>;
+		};
+	};

Regards,
Bjorn

Signature · 4 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.

--