[PATCH 0/2] Add QCOM RPMh Clock driver

[PATCH 0/2] Add QCOM RPMh Clock driver

[Taniya Das]

Hello,

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 clock RPMh driver
  dt-bindings: clock: Introduce QCOM RPMh clock bindings

 .../devicetree/bindings/clock/qcom,rpmh.txt    |  22 ++
 drivers/clk/qcom/Kconfig                       |   9 +
 drivers/clk/qcom/Makefile                      |   1 +
 drivers/clk/qcom/clk-rpmh.c                    | 397 +++++++++++++++++++++
 include/dt-bindings/clock/qcom,rpmh.h          |  25 ++
 5 files changed, 454 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/qcom,rpmh.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 Code Aurora Forum, hosted by The Linux Foundation

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

[Taniya Das]

From: Amit Nischal <anischal@codeaurora.org>

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 <collinsd@codeaurora.org>
Signed-off-by: Amit Nischal <anischal@codeaurora.org>
Signed-off-by: Taniya Das <tdas@codeaurora.org>
---
 drivers/clk/qcom/Kconfig              |   9 +
 drivers/clk/qcom/Makefile             |   1 +
 drivers/clk/qcom/clk-rpmh.c           | 397 ++++++++++++++++++++++++++++++++++
 include/dt-bindings/clock/qcom,rpmh.h |  25 +++
 4 files changed, 432 insertions(+)
 create mode 100644 drivers/clk/qcom/clk-rpmh.c
 create mode 100644 include/dt-bindings/clock/qcom,rpmh.h

diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index fbf4532..3697a6a 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -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
diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
index 230332c..b7da05b 100644
--- a/drivers/clk/qcom/Makefile
+++ b/drivers/clk/qcom/Makefile
@@ -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
diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
new file mode 100644
index 0000000..536d102
--- /dev/null
+++ b/drivers/clk/qcom/clk-rpmh.c
@@ -0,0 +1,397 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
+ */
+
+#define pr_fmt(fmt) "%s: " fmt, __func__
+
+#include <linux/kernel.h>
+#include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/regmap.h>
+#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"
+
+#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
+#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 {
+	const char *res_name;
+	u32 res_addr;
+	u32 res_en_offset;
+	u32 res_on_val;
+	u32 res_off_val;
+	u32 state;
+	u32 aggr_state;
+	u32 last_sent_aggr_state;
+	u32 valid_state_mask;
+	struct rpmh_client *rpmh_client;
+	unsigned long rate;
+	struct clk_rpmh *peer;
+	struct clk_hw hw;
+};
+
+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, \
+			  _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,			      \
+		.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,			      \
+		.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)));
+}
+
+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 >> RPMH_SLEEP_STATE) & 1
+			? c->res_on_val : c->res_off_val;
+		ret = rpmh_write_async(c->rpmh_client, RPMH_SLEEP_STATE,
+				       &cmd, 1);
+		if (ret) {
+			pr_err("rpmh_write_async(%s, state=%d) failed (%d)\n",
+			       c->res_name, RPMH_SLEEP_STATE, ret);
+			return ret;
+		}
+	}
+
+	if (has_state_changed(c, RPMH_WAKE_ONLY_STATE)) {
+		cmd.data = (c->aggr_state >> RPMH_WAKE_ONLY_STATE) & 1
+			? 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("rpmh_write_async(%s, state=%d) failed (%d)\n",
+			       c->res_name, RPMH_WAKE_ONLY_STATE, ret);
+			return ret;
+		}
+	}
+
+	if (has_state_changed(c, RPMH_ACTIVE_ONLY_STATE)) {
+		cmd.data = (c->aggr_state >> RPMH_ACTIVE_ONLY_STATE) & 1
+			? c->res_on_val : c->res_off_val;
+		ret = rpmh_write(c->rpmh_client, RPMH_ACTIVE_ONLY_STATE,
+				 &cmd, 1);
+		if (ret) {
+			pr_err("rpmh_write(%s, state=%d) failed (%d)\n",
+			       c->res_name, RPMH_ACTIVE_ONLY_STATE, ret);
+			return ret;
+		}
+	}
+
+	c->last_sent_aggr_state = c->aggr_state;
+	c->peer->last_sent_aggr_state =  c->last_sent_aggr_state;
+
+	return 0;
+}
+
+static void clk_rpmh_aggregate_state(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;
+}
+
+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;
+
+	clk_rpmh_aggregate_state(c, true);
+
+	ret = clk_rpmh_send_aggregate_command(c);
+
+	if (ret)
+		c->state = 0;
+
+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);
+	int ret = 0;
+
+	mutex_lock(&rpmh_clk_lock);
+
+	if (!c->state)
+		goto out;
+
+	clk_rpmh_aggregate_state(c, false);
+
+	ret = clk_rpmh_send_aggregate_command(c);
+
+	if (ret) {
+		c->state = c->valid_state_mask;
+		WARN(1, "clk: %s failed to disable\n", c->res_name);
+	}
+
+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,
+		    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,
+	[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),
+};
+
+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);
+
+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;
+	struct rpmh_client *rpmh_client = NULL;
+
+	desc = of_device_get_match_data(&pdev->dev);
+	if (!desc) {
+		ret = -EINVAL;
+		goto err;
+	}
+
+	ret = cmd_db_ready();
+	if (ret) {
+		if (ret != -EPROBE_DEFER) {
+			dev_err(&pdev->dev, "Command DB not available (%d)\n",
+				ret);
+			goto err;
+		}
+		return ret;
+	}
+
+	rpmh_client = rpmh_get_client(pdev);
+	if (IS_ERR(rpmh_client)) {
+		ret = PTR_ERR(rpmh_client);
+		if (ret != -EPROBE_DEFER)
+			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) {
+			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)) {
+			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)
+		goto err;
+
+	dev_info(&pdev->dev, "Registered RPMh clocks\n");
+	return ret;
+
+err:
+	if (rpmh_client)
+		rpmh_release(rpmh_client);
+
+	dev_err(&pdev->dev, "Error registering RPMh Clock driver (%d)\n", ret);
+	return ret;
+}
+
+static struct platform_driver clk_rpmh_driver = {
+	.probe		= clk_rpmh_probe,
+	.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)
+{
+	platform_driver_unregister(&clk_rpmh_driver);
+}
+module_exit(clk_rpmh_exit);
+
+MODULE_DESCRIPTION("QTI RPMh Clock Driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:clk-rpmh");
diff --git a/include/dt-bindings/clock/qcom,rpmh.h b/include/dt-bindings/clock/qcom,rpmh.h
new file mode 100644
index 0000000..34fbf3c
--- /dev/null
+++ b/include/dt-bindings/clock/qcom,rpmh.h
@@ -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
--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the  Linux Foundation.

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

[Taniya Das]

From: Amit Nischal <anischal@codeaurora.org>

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 <anischal@codeaurora.org>
Signed-off-by: Taniya Das <tdas@codeaurora.org>
---
 .../devicetree/bindings/clock/qcom,rpmh.txt        | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/qcom,rpmh.txt

diff --git a/Documentation/devicetree/bindings/clock/qcom,rpmh.txt b/Documentation/devicetree/bindings/clock/qcom,rpmh.txt
new file mode 100644
index 0000000..8222c88
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/qcom,rpmh.txt
@@ -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 {
+		clock_rpmh: qcom,rpmhclk {
+		compatible = "qcom,rpmh-clk-sdm845";
+		#clock-cells = <1>;
+		};
+	};
--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the  Linux Foundation.

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

[Evan Green]

Hi Taniya,

On Wed, Mar 28, 2018 at 11:19 PM Taniya Das <tdas@codeaurora.org> wrote:

> From: Amit Nischal <anischal@codeaurora.org>

> 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 <collinsd@codeaurora.org>
> Signed-off-by: Amit Nischal <anischal@codeaurora.org>
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> ---
>   drivers/clk/qcom/Kconfig              |   9 +
>   drivers/clk/qcom/Makefile             |   1 +
>   drivers/clk/qcom/clk-rpmh.c           | 397
++++++++++++++++++++++++++++++++++
>   include/dt-bindings/clock/qcom,rpmh.h |  25 +++
>   4 files changed, 432 insertions(+)
>   create mode 100644 drivers/clk/qcom/clk-rpmh.c
>   create mode 100644 include/dt-bindings/clock/qcom,rpmh.h

> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
...
> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
> new file mode 100644
> index 0000000..536d102
> --- /dev/null
> +++ b/drivers/clk/qcom/clk-rpmh.c
> @@ -0,0 +1,397 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#define pr_fmt(fmt) "%s: " fmt, __func__

Stanimir recently commented on a different patch asking for using dev_*
instead of this. Seems like an applicable comment here, too.

> +
> +#include <linux/kernel.h>
> +#include <linux/err.h>
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/regmap.h>
> +#include <soc/qcom/cmd-db.h>
> +#include <soc/qcom/rpmh.h>
> +#include <dt-bindings/clock/qcom,rpmh.h>

Alphabetize includes?

> +
> +#include "common.h"
> +#include "clk-regmap.h"
> +
> +#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
> +#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 {
> +       const char *res_name;
> +       u32 res_addr;
> +       u32 res_en_offset;
> +       u32 res_on_val;
> +       u32 res_off_val;
> +       u32 state;
> +       u32 aggr_state;
> +       u32 last_sent_aggr_state;
> +       u32 valid_state_mask;
> +       struct rpmh_client *rpmh_client;
> +       unsigned long rate;
> +       struct clk_rpmh *peer;
> +       struct clk_hw hw;

I believe this member should go at the beginning of the structure. At least
that's what everyone else seems to do, and that's what the clk.txt
documentation seems to ask for.

> +};
> +
> +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, \
> +                         _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,
      \
> +               .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,
     \
> +               .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)));
> +}
> +
> +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 >> RPMH_SLEEP_STATE) & 1
> +                       ? c->res_on_val : c->res_off_val;

I found it quite confusing that you're shifting in the opposite direction
than in has_state_changed. How about this:

cmd.data = (c->aggr_state & BIT(RPMH_SLEEP_STATE)) ? c->res_on_val :
c->res_off_val;

> +               ret = rpmh_write_async(c->rpmh_client, RPMH_SLEEP_STATE,
> +                                      &cmd, 1);
> +               if (ret) {
> +                       pr_err("rpmh_write_async(%s, state=%d) failed
(%d)\n",
> +                              c->res_name, RPMH_SLEEP_STATE, ret);
> +                       return ret;
> +               }
> +       }
> +
> +       if (has_state_changed(c, RPMH_WAKE_ONLY_STATE)) {
> +               cmd.data = (c->aggr_state >> RPMH_WAKE_ONLY_STATE) & 1
> +                       ? 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("rpmh_write_async(%s, state=%d) failed
(%d)\n",
> +                              c->res_name, RPMH_WAKE_ONLY_STATE, ret);
> +                       return ret;
> +               }
> +       }
> +
> +       if (has_state_changed(c, RPMH_ACTIVE_ONLY_STATE)) {
> +               cmd.data = (c->aggr_state >> RPMH_ACTIVE_ONLY_STATE) & 1
> +                       ? c->res_on_val : c->res_off_val;
> +               ret = rpmh_write(c->rpmh_client, RPMH_ACTIVE_ONLY_STATE,
> +                                &cmd, 1);
> +               if (ret) {
> +                       pr_err("rpmh_write(%s, state=%d) failed (%d)\n",
> +                              c->res_name, RPMH_ACTIVE_ONLY_STATE, ret);
> +                       return ret;
> +               }
> +       }
> +
> +       c->last_sent_aggr_state = c->aggr_state;
> +       c->peer->last_sent_aggr_state =  c->last_sent_aggr_state;
> +
> +       return 0;
> +}
> +
> +static void clk_rpmh_aggregate_state(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;
> +}
> +
> +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;
> +
> +       clk_rpmh_aggregate_state(c, true);
> +
> +       ret = clk_rpmh_send_aggregate_command(c);
> +
> +       if (ret)
> +               c->state = 0;
> +
> +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);
> +       int ret = 0;
> +
> +       mutex_lock(&rpmh_clk_lock);
> +
> +       if (!c->state)
> +               goto out;
> +
> +       clk_rpmh_aggregate_state(c, false);
> +
> +       ret = clk_rpmh_send_aggregate_command(c);
> +
> +       if (ret) {
> +               c->state = c->valid_state_mask;
> +               WARN(1, "clk: %s failed to disable\n", c->res_name);
> +       }
> +
> +out:
> +       mutex_unlock(&rpmh_clk_lock);
> +       return;
> +};

The guts of these two functions (clk_rpmh_{un,}prepare) look like they
would collapse pretty well into a single helper function that takes an
extra enable parameter. The if would change to !!c->state == enable,
clk_rpmh_aggregate_state would take the enable parameter, and the failure
case could restore a previously saved value. And you could just ditch the
WARN entirely (probably should do that anyway). Not critical though, up to
you.

...

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

Remove this unused variable. It generates a warning (or an error for some
of us).
...
> diff --git a/include/dt-bindings/clock/qcom,rpmh.h
b/include/dt-bindings/clock/qcom,rpmh.h
> new file mode 100644
> index 0000000..34fbf3c
> --- /dev/null
> +++ b/include/dt-bindings/clock/qcom,rpmh.h
> @@ -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

Maybe _DT_BINDINGS_CLK_QCOM_RPMH_H instead?

-Evan

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

[Taniya Das]

Hello Evan,

Thanks for the review comments.

On 3/30/2018 3:19 AM, Evan Green wrote:
> Hi Taniya,
> 
> On Wed, Mar 28, 2018 at 11:19 PM Taniya Das <tdas@codeaurora.org> wrote:
> 
>> From: Amit Nischal <anischal@codeaurora.org>
> 
>> 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 <collinsd@codeaurora.org>
>> Signed-off-by: Amit Nischal <anischal@codeaurora.org>
>> Signed-off-by: Taniya Das <tdas@codeaurora.org>
>> ---
>>    drivers/clk/qcom/Kconfig              |   9 +
>>    drivers/clk/qcom/Makefile             |   1 +
>>    drivers/clk/qcom/clk-rpmh.c           | 397
> ++++++++++++++++++++++++++++++++++
>>    include/dt-bindings/clock/qcom,rpmh.h |  25 +++
>>    4 files changed, 432 insertions(+)
>>    create mode 100644 drivers/clk/qcom/clk-rpmh.c
>>    create mode 100644 include/dt-bindings/clock/qcom,rpmh.h
> 
>> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> ...
>> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
>> new file mode 100644
>> index 0000000..536d102
>> --- /dev/null
>> +++ b/drivers/clk/qcom/clk-rpmh.c
>> @@ -0,0 +1,397 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#define pr_fmt(fmt) "%s: " fmt, __func__
> 
> Stanimir recently commented on a different patch asking for using dev_*
> instead of this. Seems like an applicable comment here, too.
> 

I will drop this to see how I could accomodate dev_xxx.

>> +
>> +#include <linux/kernel.h>
>> +#include <linux/err.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/regmap.h>
>> +#include <soc/qcom/cmd-db.h>
>> +#include <soc/qcom/rpmh.h>
>> +#include <dt-bindings/clock/qcom,rpmh.h>
> 
> Alphabetize includes?

I will take care of them in the next patch series.

> 
>> +
>> +#include "common.h"
>> +#include "clk-regmap.h"
>> +
>> +#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
>> +#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 {
>> +       const char *res_name;
>> +       u32 res_addr;
>> +       u32 res_en_offset;
>> +       u32 res_on_val;
>> +       u32 res_off_val;
>> +       u32 state;
>> +       u32 aggr_state;
>> +       u32 last_sent_aggr_state;
>> +       u32 valid_state_mask;
>> +       struct rpmh_client *rpmh_client;
>> +       unsigned long rate;
>> +       struct clk_rpmh *peer;
>> +       struct clk_hw hw;
> 
> I believe this member should go at the beginning of the structure. At least
> that's what everyone else seems to do, and that's what the clk.txt
> documentation seems to ask for.
>

Yes I missed that, would update it in the next patch series.

>> +};
>> +
>> +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, \
>> +                         _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,
>        \
>> +               .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,
>       \
>> +               .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)));
>> +}
>> +
>> +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 >> RPMH_SLEEP_STATE) & 1
>> +                       ? c->res_on_val : c->res_off_val;
> 
> I found it quite confusing that you're shifting in the opposite direction
> than in has_state_changed. How about this:
> 
> cmd.data = (c->aggr_state & BIT(RPMH_SLEEP_STATE)) ? c->res_on_val :
> c->res_off_val;
> 

It is kept keeping in mind the consistency of the command data for all 
the different states.

Would it be better if I define a new macro ?

#define RPMH_CMD_DATA(aggr_state, rpmh_state) \
         ((aggr_state >> rpmh_state) & 1)

If you agree, I could use the new macro in the next series.

>> +               ret = rpmh_write_async(c->rpmh_client, RPMH_SLEEP_STATE,
>> +                                      &cmd, 1);
>> +               if (ret) {
>> +                       pr_err("rpmh_write_async(%s, state=%d) failed
> (%d)\n",
>> +                              c->res_name, RPMH_SLEEP_STATE, ret);
>> +                       return ret;
>> +               }
>> +       }
>> +
>> +       if (has_state_changed(c, RPMH_WAKE_ONLY_STATE)) {
>> +               cmd.data = (c->aggr_state >> RPMH_WAKE_ONLY_STATE) & 1
>> +                       ? 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("rpmh_write_async(%s, state=%d) failed
> (%d)\n",
>> +                              c->res_name, RPMH_WAKE_ONLY_STATE, ret);
>> +                       return ret;
>> +               }
>> +       }
>> +
>> +       if (has_state_changed(c, RPMH_ACTIVE_ONLY_STATE)) {
>> +               cmd.data = (c->aggr_state >> RPMH_ACTIVE_ONLY_STATE) & 1
>> +                       ? c->res_on_val : c->res_off_val;
>> +               ret = rpmh_write(c->rpmh_client, RPMH_ACTIVE_ONLY_STATE,
>> +                                &cmd, 1);
>> +               if (ret) {
>> +                       pr_err("rpmh_write(%s, state=%d) failed (%d)\n",
>> +                              c->res_name, RPMH_ACTIVE_ONLY_STATE, ret);
>> +                       return ret;
>> +               }
>> +       }
>> +
>> +       c->last_sent_aggr_state = c->aggr_state;
>> +       c->peer->last_sent_aggr_state =  c->last_sent_aggr_state;
>> +
>> +       return 0;
>> +}
>> +
>> +static void clk_rpmh_aggregate_state(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;
>> +}
>> +
>> +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;
>> +
>> +       clk_rpmh_aggregate_state(c, true);
>> +
>> +       ret = clk_rpmh_send_aggregate_command(c);
>> +
>> +       if (ret)
>> +               c->state = 0;
>> +
>> +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);
>> +       int ret = 0;
>> +
>> +       mutex_lock(&rpmh_clk_lock);
>> +
>> +       if (!c->state)
>> +               goto out;
>> +
>> +       clk_rpmh_aggregate_state(c, false);
>> +
>> +       ret = clk_rpmh_send_aggregate_command(c);
>> +
>> +       if (ret) {
>> +               c->state = c->valid_state_mask;
>> +               WARN(1, "clk: %s failed to disable\n", c->res_name);
>> +       }
>> +
>> +out:
>> +       mutex_unlock(&rpmh_clk_lock);
>> +       return;
>> +};
> 
> The guts of these two functions (clk_rpmh_{un,}prepare) look like they
> would collapse pretty well into a single helper function that takes an
> extra enable parameter. The if would change to !!c->state == enable,
> clk_rpmh_aggregate_state would take the enable parameter, and the failure
> case could restore a previously saved value. And you could just ditch the
> WARN entirely (probably should do that anyway). Not critical though, up to
> you.
> 
> ...
> 

I would have a new helper function, would update in the next series.

>> +
>> +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;
> 
> Remove this unused variable. It generates a warning (or an error for some
> of us).
> ...

Would clean the unused variables.

>> diff --git a/include/dt-bindings/clock/qcom,rpmh.h
> b/include/dt-bindings/clock/qcom,rpmh.h
>> new file mode 100644
>> index 0000000..34fbf3c
>> --- /dev/null
>> +++ b/include/dt-bindings/clock/qcom,rpmh.h
>> @@ -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
> 
> Maybe _DT_BINDINGS_CLK_QCOM_RPMH_H instead?
> 

Kept inline with the other target header files.

> -Evan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

--

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

[Evan Green]

Hi Taniya,

On Mon, Apr 2, 2018 at 3:33 AM Taniya Das <tdas@codeaurora.org> wrote:

> Hello Evan,

> Thanks for the review comments.

> On 3/30/2018 3:19 AM, Evan Green wrote:
> > Hi Taniya,
> >
> > On Wed, Mar 28, 2018 at 11:19 PM Taniya Das <tdas@codeaurora.org> wrote:
> >
> >> From: Amit Nischal <anischal@codeaurora.org>

> >> +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 >> RPMH_SLEEP_STATE) & 1
> >> +                       ? c->res_on_val : c->res_off_val;
> >
> > I found it quite confusing that you're shifting in the opposite
direction
> > than in has_state_changed. How about this:
> >
> > cmd.data = (c->aggr_state & BIT(RPMH_SLEEP_STATE)) ? c->res_on_val :
> > c->res_off_val;
> >

> It is kept keeping in mind the consistency of the command data for all
> the different states.

> Would it be better if I define a new macro ?

> #define RPMH_CMD_DATA(aggr_state, rpmh_state) \
>           ((aggr_state >> rpmh_state) & 1)

> If you agree, I could use the new macro in the next series.


I'm afraid I don't really understand the "consistency of command data for
all the different states". It seems like every other time you refer to a
state mask (eg state, valid_state_mask, and all the *_STATE_MASK defines,
you use BIT(state). The version above seemed like an unnecessarily
confusing variant. In this case, aggr_state is only being used as a
conditional to determine whether to put the res_on or off value, so it's
not like that form is needed because of hardware bitfields or something.
All of the state-like members seem to be purely software masks.

Anyway, I'll leave it to you after this. If you choose to keep the shifts
as written and hide them behind a macro, the name as you've suggested isn't
great, as that macro only gives you the conditional for which data to use,
not the data itself as the macro name might have you believe.

-Evan

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

[Stephen Boyd]

Quoting Taniya Das (2018-04-02 03:33:26)
> 
> > 
> >> +
> >> +#include "common.h"
> >> +#include "clk-regmap.h"
> >> +
> >> +#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
> >> +#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 {
> >> +       const char *res_name;
> >> +       u32 res_addr;
> >> +       u32 res_en_offset;
> >> +       u32 res_on_val;
> >> +       u32 res_off_val;
> >> +       u32 state;
> >> +       u32 aggr_state;
> >> +       u32 last_sent_aggr_state;
> >> +       u32 valid_state_mask;
> >> +       struct rpmh_client *rpmh_client;
> >> +       unsigned long rate;
> >> +       struct clk_rpmh *peer;
> >> +       struct clk_hw hw;
> > 
> > I believe this member should go at the beginning of the structure. At least
> > that's what everyone else seems to do, and that's what the clk.txt
> > documentation seems to ask for.
> >
> 
> Yes I missed that, would update it in the next patch series.

Where is the documentation asking for that? There isn't any place that
clk_hw should be in a structure. container_of() can figure it out all
the time, and that macro is preferred instead of plain casts from one
type to another because it provides slightly more type safety.

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

[Stephen Boyd]

Quoting Taniya Das (2018-04-02 03:33:26)
> =

> > =

> >> +
> >> +#include "common.h"
> >> +#include "clk-regmap.h"
> >> +
> >> +#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
> >> +#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 {
> >> +       const char *res_name;
> >> +       u32 res_addr;
> >> +       u32 res_en_offset;
> >> +       u32 res_on_val;
> >> +       u32 res_off_val;
> >> +       u32 state;
> >> +       u32 aggr_state;
> >> +       u32 last_sent_aggr_state;
> >> +       u32 valid_state_mask;
> >> +       struct rpmh_client *rpmh_client;
> >> +       unsigned long rate;
> >> +       struct clk_rpmh *peer;
> >> +       struct clk_hw hw;
> > =

> > I believe this member should go at the beginning of the structure. At l=
east
> > that's what everyone else seems to do, and that's what the clk.txt
> > documentation seems to ask for.
> >
> =

> Yes I missed that, would update it in the next patch series.

Where is the documentation asking for that? There isn't any place that
clk_hw should be in a structure. container_of() can figure it out all
the time, and that macro is preferred instead of plain casts from one
type to another because it provides slightly more type safety.

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

[Stephen Boyd]

Quoting Taniya Das (2018-03-28 23:17:53)
> diff --git a/Documentation/devicetree/bindings/clock/qcom,rpmh.txt b/Documentation/devicetree/bindings/clock/qcom,rpmh.txt
> new file mode 100644
> index 0000000..8222c88
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/qcom,rpmh.txt

Can the file name be qcom,rpmh-clk?

> @@ -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 {

> +               clock_rpmh: qcom,rpmhclk {

Should say clock-controller for node name.

> +               compatible = "qcom,rpmh-clk-sdm845";
> +               #clock-cells = <1>;

Is this tabbed out correctly?

> +               };
> +       };

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

[Stephen Boyd]

Quoting Taniya Das (2018-03-28 23:17:53)
> diff --git a/Documentation/devicetree/bindings/clock/qcom,rpmh.txt b/Documentation/devicetree/bindings/clock/qcom,rpmh.txt
> new file mode 100644
> index 0000000..8222c88
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/qcom,rpmh.txt

Can the file name be qcom,rpmh-clk?

> @@ -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 {

> +               clock_rpmh: qcom,rpmhclk {

Should say clock-controller for node name.

> +               compatible = "qcom,rpmh-clk-sdm845";
> +               #clock-cells = <1>;

Is this tabbed out correctly?

> +               };
> +       };

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

[Stephen Boyd]

Quoting Taniya Das (2018-03-28 23:17:53)
> diff --git a/Documentation/devicetree/bindings/clock/qcom,rpmh.txt b/Docu=
mentation/devicetree/bindings/clock/qcom,rpmh.txt
> new file mode 100644
> index 0000000..8222c88
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/qcom,rpmh.txt

Can the file name be qcom,rpmh-clk?

> @@ -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 {

> +               clock_rpmh: qcom,rpmhclk {

Should say clock-controller for node name.

> +               compatible =3D "qcom,rpmh-clk-sdm845";
> +               #clock-cells =3D <1>;

Is this tabbed out correctly?

> +               };
> +       };

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

[Taniya Das]

Thanks Stephen for the review.

On 4/6/2018 4:50 AM, Stephen Boyd wrote:
> Quoting Taniya Das (2018-03-28 23:17:53)
>> diff --git a/Documentation/devicetree/bindings/clock/qcom,rpmh.txt b/Documentation/devicetree/bindings/clock/qcom,rpmh.txt
>> new file mode 100644
>> index 0000000..8222c88
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/qcom,rpmh.txt
> 
> Can the file name be qcom,rpmh-clk?
> 

Sure will update the file name.

>> @@ -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 {
> 
>> +               clock_rpmh: qcom,rpmhclk {
> 
> Should say clock-controller for node name.
>

Would fix it in the next patch.

>> +               compatible = "qcom,rpmh-clk-sdm845";
>> +               #clock-cells = <1>;
> 
> Is this tabbed out correctly?
> 

Will fix the tabs in the next patch.

>> +               };
>> +       };

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

--

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

[Evan Green]

On Thu, Apr 5, 2018 at 4:17 PM Stephen Boyd <sboyd@kernel.org> wrote:

> Quoting Taniya Das (2018-04-02 03:33:26)
> >
> > >
> > >> +
> > >> +#include "common.h"
> > >> +#include "clk-regmap.h"
> > >> +
> > >> +#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
> > >> +#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 {
> > >> +       const char *res_name;
> > >> +       u32 res_addr;
> > >> +       u32 res_en_offset;
> > >> +       u32 res_on_val;
> > >> +       u32 res_off_val;
> > >> +       u32 state;
> > >> +       u32 aggr_state;
> > >> +       u32 last_sent_aggr_state;
> > >> +       u32 valid_state_mask;
> > >> +       struct rpmh_client *rpmh_client;
> > >> +       unsigned long rate;
> > >> +       struct clk_rpmh *peer;
> > >> +       struct clk_hw hw;
> > >
> > > I believe this member should go at the beginning of the structure. At
least
> > > that's what everyone else seems to do, and that's what the clk.txt
> > > documentation seems to ask for.
> > >
> >
> > Yes I missed that, would update it in the next patch series.

> Where is the documentation asking for that? There isn't any place that
> clk_hw should be in a structure. container_of() can figure it out all
> the time, and that macro is preferred instead of plain casts from one
> type to another because it provides slightly more type safety.

Perhaps I'm reading too strictly into the documentation, but this was the
passage that gave me that idea:

To construct a clk hardware structure for your platform you must define
the following::

struct clk_foo {
struct clk_hw hw;
... hardware specific data goes here ...
};

-Evan