[v6 0/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver

[v6 0/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver

[Taniya Das]

 [v6]
   * Addressed comments from Rob
   * Addressed comments from Stephen

 [v5]
   * Addressed comments from Stephen
   * Introduced a new DT property to take clock divs

 [v4]
  * Addressed comments from Stephen

 [v3]
  * Addressed documentation & code review comments from Bjorn
  * Addressed bindings comments from Rob
  * Updated the patch series order for bindings

 [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

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

 .../devicetree/bindings/clock/qcom,rpmh-clk.txt    |  53 +++
 drivers/clk/qcom/Kconfig                           |   9 +
 drivers/clk/qcom/Makefile                          |   1 +
 drivers/clk/qcom/clk-rpmh.c                        | 368 +++++++++++++++++++++
 include/dt-bindings/clock/qcom,rpmh.h              |  22 ++
 5 files changed, 453 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.

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

[Taniya Das]

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: Taniya Das <tdas@codeaurora.org>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/clock/qcom,rpmh-clk.txt    | 46 ++++++++++++++++++++++
 include/dt-bindings/clock/qcom,rpmh.h              | 22 +++++++++++
 2 files changed, 68 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt
 create mode 100644 include/dt-bindings/clock/qcom,rpmh.h

diff --git a/Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt b/Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt
new file mode 100644
index 0000000..2632cae
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt
@@ -0,0 +1,46 @@
+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,sdm845-rpmh-clk"
+
+- #clock-cells : must contain 1
+
+Optional properties :
+
+These below properities would be required in the cases where the parent of the
+clocks would be "xo_board_div". The default parent for the clock is "xo_board".
+
+- assigned-clocks: from the common clock binding;
+		should be: clock specifier for each output clock of this
+		provider node
+
+- assigned-clock-parents: from the common clock binding;
+		should be: phandle of input clock listed in clocks
+		property
+Example :
+
+#include <dt-bindings/clock/qcom,rpmh.h>
+
+	&apps_rsc {
+		rpmhcc: clock-controller {
+			compatible = "qcom,sdm845-rpmh-clk";
+			#clock-cells = <1>;
+			assigned-clocks = <&clock_rpmh RPMH_CXO_CLK>,
+					<&clock_rpmh RPMH_CXO_CLK_A>,
+					<&clock_rpmh RPMH_LN_BB_CLK2>,
+					<&clock_rpmh RPMH_LN_BB_CLK2_A>,
+					<&clock_rpmh RPMH_LN_BB_CLK3>,
+					<&clock_rpmh RPMH_LN_BB_CLK3_A>;
+			assigned-clock-parents = <&xo_board_div>,
+						<&xo_board_div>,
+						<&xo_board_div>,
+						<&xo_board_div>,
+						<&xo_board_div>,
+						<&xo_board_div>,
+		};
+	};
diff --git a/include/dt-bindings/clock/qcom,rpmh.h b/include/dt-bindings/clock/qcom,rpmh.h
new file mode 100644
index 0000000..f48fbd6
--- /dev/null
+++ b/include/dt-bindings/clock/qcom,rpmh.h
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (c) 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
+
+#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] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver

[Taniya Das]

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

Signed-off-by: Taniya Das <tdas@codeaurora.org>
---
 drivers/clk/qcom/Kconfig    |   9 ++
 drivers/clk/qcom/Makefile   |   1 +
 drivers/clk/qcom/clk-rpmh.c | 366 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 376 insertions(+)
 create mode 100644 drivers/clk/qcom/clk-rpmh.c

diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index e42e1af..627e6e1 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -59,6 +59,15 @@ config QCOM_CLK_SMD_RPM
 	  Say Y if you want to support the clocks exposed by the RPM on
 	  platforms such as apq8016, apq8084, msm8974 etc.

+config QCOM_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 APQ_GCC_8084
 	tristate "APQ8084 Global Clock Controller"
 	select QCOM_GDSC
diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
index 7c09ab1..246a721 100644
--- a/drivers/clk/qcom/Makefile
+++ b/drivers/clk/qcom/Makefile
@@ -37,5 +37,6 @@ obj-$(CONFIG_MSM_MMCC_8996) += mmcc-msm8996.o
 obj-$(CONFIG_QCOM_A53PLL) += a53-pll.o
 obj-$(CONFIG_QCOM_CLK_APCS_MSM8916) += apcs-msm8916.o
 obj-$(CONFIG_QCOM_CLK_RPM) += clk-rpm.o
+obj-$(CONFIG_QCOM_CLK_RPMH) += clk-rpmh.o
 obj-$(CONFIG_QCOM_CLK_SMD_RPM) += clk-smd-rpm.o
 obj-$(CONFIG_SPMI_PMIC_CLKDIV) += clk-spmi-pmic-div.o
diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
new file mode 100644
index 0000000..21f9b9b
--- /dev/null
+++ b/drivers/clk/qcom/clk-rpmh.c
@@ -0,0 +1,366 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+#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 <soc/qcom/cmd-db.h>
+#include <soc/qcom/rpmh.h>
+
+#include <dt-bindings/clock/qcom,rpmh.h>
+
+#define CLK_RPMH_ARC_EN_OFFSET		0
+#define CLK_RPMH_VRM_EN_OFFSET		4
+
+/**
+ * struct clk_rpmh - individual rpmh clock data structure
+ * @hw:			handle between common and hardware-specific interfaces
+ * @res_name:		resource name for the rpmh clock
+ * @div:		clock divider to compute the clock rate
+ * @res_addr:		base address of the rpmh resource within the RPMh
+ * @res_on_val:		rpmh clock enable value
+ * @state:		rpmh clock requested state
+ * @aggr_state:		rpmh clock aggregated state
+ * @last_sent_aggr_state: rpmh clock last aggr state sent to RPMh
+ * @valid_state_mask:	mask to determine the state of the rpmh clock
+ * @rpmh_client:	handle used for rpmh communications
+ * @dev:		device to which it is attached
+ * @peer:		pointer to the clock rpmh sibling
+ */
+struct clk_rpmh {
+	struct clk_hw hw;
+	const char *res_name;
+	u8  div;
+	u32 res_addr;
+	u32 res_on_val;
+	u32 state;
+	u32 aggr_state;
+	u32 last_sent_aggr_state;
+	u32 valid_state_mask;
+	struct rpmh_client *rpmh_client;
+	struct device *dev;
+	struct clk_rpmh *peer;
+};
+
+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)			\
+	static struct clk_rpmh _platform##_##_name_active;		\
+	static struct clk_rpmh _platform##_##_name = {			\
+		.res_name = _res_name,					\
+		.res_addr = _res_en_offset,				\
+		.res_on_val = _res_on,					\
+		.peer = &_platform##_##_name_active,			\
+		.valid_state_mask = (BIT(RPMH_WAKE_ONLY_STATE) |	\
+				      BIT(RPMH_ACTIVE_ONLY_STATE) |	\
+				      BIT(RPMH_SLEEP_STATE)),		\
+		.hw.init = &(struct clk_init_data){			\
+			.ops = &clk_rpmh_ops,				\
+			.name = #_name,					\
+			.parent_names = (const char *[]){ "xo_board",	\
+						"xo_board_div" },	\
+			.num_parents = 2,				\
+		},							\
+	};								\
+	static struct clk_rpmh _platform##_##_name_active = {		\
+		.res_name = _res_name,					\
+		.res_addr = _res_en_offset,				\
+		.res_on_val = _res_on,					\
+		.peer = &_platform##_##_name,				\
+		.valid_state_mask = (BIT(RPMH_WAKE_ONLY_STATE) |	\
+					BIT(RPMH_ACTIVE_ONLY_STATE)),	\
+		.hw.init = &(struct clk_init_data){			\
+			.ops = &clk_rpmh_ops,				\
+			.name = #_name_active,				\
+			.parent_names = (const char *[]){ "xo_board",	\
+						"xo_board_div" },	\
+			.num_parents = 2,				\
+		},							\
+	}
+
+#define DEFINE_CLK_RPMH_ARC(_platform, _name, _name_active, _res_name,	\
+			    _res_on)					\
+	__DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name,	\
+			  CLK_RPMH_ARC_EN_OFFSET, _res_on)
+
+#define DEFINE_CLK_RPMH_VRM(_platform, _name, _name_active, _res_name)	\
+	__DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name,	\
+			  CLK_RPMH_VRM_EN_OFFSET, 1)
+
+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 };
+	u32 cmd_state, on_val;
+	enum rpmh_state state = RPMH_SLEEP_STATE;
+	int ret;
+
+	cmd.addr = c->res_addr;
+	cmd_state = c->aggr_state;
+	on_val = c->res_on_val;
+
+	for (; state <= RPMH_ACTIVE_ONLY_STATE; state++) {
+		if (has_state_changed(c, state)) {
+			if (cmd_state & BIT(state))
+				cmd.data = on_val;
+
+			ret = rpmh_write_async(c->rpmh_client, state,
+						&cmd, 1);
+			if (ret) {
+				dev_err(c->dev, "set %s state of %s failed: (%d)\n",
+					!state ? "sleep" :
+					state == RPMH_WAKE_ONLY_STATE	?
+					"wake" : "active", c->res_name, ret);
+				return ret;
+			}
+		}
+	}
+
+	c->last_sent_aggr_state = c->aggr_state;
+	c->peer->last_sent_aggr_state =  c->last_sent_aggr_state;
+
+	return 0;
+}
+
+/*
+ * Update state and aggregate state values based on enable value.
+ */
+static int clk_rpmh_aggregate_state_send_command(struct clk_rpmh *c,
+						bool enable)
+{
+	int ret;
+
+	/* Nothing required to be done if already off or on */
+	if (enable == c->state)
+		return 0;
+
+	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);
+	if (!ret)
+		return 0;
+	if (ret && enable)
+		c->state = 0;
+	else if (ret)
+		c->state = c->valid_state_mask;
+
+	WARN(1, "clk: %s failed to %s\n", c->res_name,
+				enable ? "enable" : "disable");
+	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);
+	ret = clk_rpmh_aggregate_state_send_command(c, true);
+	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);
+	clk_rpmh_aggregate_state_send_command(c, false);
+	mutex_unlock(&rpmh_clk_lock);
+};
+
+static int clk_rpmh_set_parent(struct clk_hw *hw, u8 index)
+{
+	return 0;
+}
+
+static u8 clk_rpmh_get_parent(struct clk_hw *hw)
+{
+	/* Default parent at index 0 */
+	return 0;
+}
+
+static const struct clk_ops clk_rpmh_ops = {
+	.prepare	= clk_rpmh_prepare,
+	.unprepare	= clk_rpmh_unprepare,
+	.set_parent	= clk_rpmh_set_parent,
+	.get_parent	= clk_rpmh_get_parent,
+};
+
+/* Resource name must match resource id present in cmd-db. */
+DEFINE_CLK_RPMH_ARC(sdm845, bi_tcxo, bi_tcxo_ao, "xo.lvl", 0x3);
+DEFINE_CLK_RPMH_VRM(sdm845, ln_bb_clk2, ln_bb_clk2_ao, "lnbclka2");
+DEFINE_CLK_RPMH_VRM(sdm845, ln_bb_clk3, ln_bb_clk3_ao, "lnbclka3");
+DEFINE_CLK_RPMH_VRM(sdm845, rf_clk1, rf_clk1_ao, "rfclka1");
+DEFINE_CLK_RPMH_VRM(sdm845, rf_clk2, rf_clk2_ao, "rfclka2");
+DEFINE_CLK_RPMH_VRM(sdm845, rf_clk3, rf_clk3_ao, "rfclka3");
+
+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 struct clk_hw *of_clk_rpmh_hw_get(struct of_phandle_args *clkspec,
+					     void *data)
+{
+	struct clk_rpmh_desc *rpmh = data;
+	unsigned int idx = clkspec->args[0];
+
+	if (idx >= rpmh->num_clks) {
+		pr_err("%s: invalid index %u\n", __func__, idx);
+		return ERR_PTR(-EINVAL);
+	}
+
+	return rpmh->clks[idx];
+}
+
+static int clk_rpmh_probe(struct platform_device *pdev)
+{
+	struct clk_hw **hw_clks;
+	struct clk_rpmh *rpmh_clk;
+	const struct clk_rpmh_desc *desc;
+	struct rpmh_client *rpmh_client;
+	int ret, i;
+
+	ret = cmd_db_ready();
+	if (ret) {
+		if (ret != -EPROBE_DEFER)
+			dev_err(&pdev->dev, "Command DB not available (%d)\n",
+									ret);
+		return ret;
+	}
+
+	rpmh_client = rpmh_get_client(pdev);
+	if (IS_ERR(rpmh_client)) {
+		ret = PTR_ERR(rpmh_client);
+		dev_err(&pdev->dev, "Failed to request RPMh client, ret=%d\n",
+									ret);
+		return ret;
+	}
+
+	desc = of_device_get_match_data(&pdev->dev);
+	hw_clks = desc->clks;
+
+	for (i = 0; i < desc->num_clks; i++) {
+		u32 res_addr;
+
+		rpmh_clk = to_clk_rpmh(hw_clks[i]);
+		res_addr = cmd_db_read_addr(rpmh_clk->res_name);
+		if (!res_addr) {
+			dev_err(&pdev->dev, "missing RPMh resource address for %s\n",
+					rpmh_clk->res_name);
+			ret = -ENODEV;
+			goto err;
+		}
+		rpmh_clk->res_addr += res_addr;
+
+		rpmh_clk->rpmh_client = rpmh_client;
+
+		ret = devm_clk_hw_register(&pdev->dev, hw_clks[i]);
+		if (ret) {
+			dev_err(&pdev->dev, "failed to register %s\n",
+					hw_clks[i]->init->name);
+			goto err;
+		}
+
+		rpmh_clk->dev = &pdev->dev;
+	}
+
+	ret = devm_of_clk_add_hw_provider(&pdev->dev, of_clk_rpmh_hw_get,
+				      (void *) desc);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to add clock provider\n");
+		goto err;
+	}
+
+	dev_dbg(&pdev->dev, "Registered RPMh clocks\n");
+
+	return 0;
+err:
+	if (rpmh_client)
+		rpmh_release(rpmh_client);
+
+	return ret;
+}
+
+static int clk_rpmh_remove(struct platform_device *pdev)
+{
+	const struct clk_rpmh_desc *desc =
+			of_device_get_match_data(&pdev->dev);
+	struct clk_hw **hw_clks = desc->clks;
+	struct clk_rpmh *rpmh_clk = to_clk_rpmh(hw_clks[0]);
+
+	rpmh_release(rpmh_clk->rpmh_client);
+
+	return 0;
+}
+
+static const struct of_device_id clk_rpmh_match_table[] = {
+	{ .compatible = "qcom,sdm845-rpmh-clk", .data = &clk_rpmh_sdm845},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, clk_rpmh_match_table);
+
+static struct platform_driver clk_rpmh_driver = {
+	.probe		= clk_rpmh_probe,
+	.remove		= clk_rpmh_remove,
+	.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("QCOM RPMh Clock Driver");
+MODULE_LICENSE("GPL v2");
--
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] dt-bindings: clock: Introduce QCOM RPMh clock bindings

[Stephen Boyd]

Quoting Taniya Das (2018-05-02 03:51:16)
> 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: Taniya Das <tdas@codeaurora.org>
> Reviewed-by: Rob Herring <robh@kernel.org>

When you change things you shouldn't keep around reviewed-by tags if it
significantly changes..

> +Example :
> +
> +#include <dt-bindings/clock/qcom,rpmh.h>
> +
> +       &apps_rsc {
> +               rpmhcc: clock-controller {
> +                       compatible = "qcom,sdm845-rpmh-clk";
> +                       #clock-cells = <1>;
> +                       assigned-clocks = <&clock_rpmh RPMH_CXO_CLK>,
> +                                       <&clock_rpmh RPMH_CXO_CLK_A>,
> +                                       <&clock_rpmh RPMH_LN_BB_CLK2>,
> +                                       <&clock_rpmh RPMH_LN_BB_CLK2_A>,
> +                                       <&clock_rpmh RPMH_LN_BB_CLK3>,
> +                                       <&clock_rpmh RPMH_LN_BB_CLK3_A>;
> +                       assigned-clock-parents = <&xo_board_div>,
> +                                               <&xo_board_div>,
> +                                               <&xo_board_div>,
> +                                               <&xo_board_div>,
> +                                               <&xo_board_div>,
> +                                               <&xo_board_div>,

There's only one parent, so assigned-clock-parents doesn't really make
sense. If anything, we would have 'clocks = ' here and specify the xo
board clk and the div 2 clk that live in DT. But I already merged this
binding, two patchsets ago. Please look in clk-next.

> +               };
> +#define RPMH_RF_CLK2                           8
> +#define RPMH_RF_CLK2_A                         9
> +#define RPMH_RF_CLK3                           10
> +#define RPMH_RF_CLK3_A                         11
> +

Ok. I dropped CLK4* from the previous patch you sent and dropped Amit's
signed-off-by.

Re: [v6 0/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver

[Stephen Boyd]

Quoting Taniya Das (2018-05-02 03:51:15)
>  [v6]
>    * Addressed comments from Rob
>    * Addressed comments from Stephen

Please actually describe what you addressed. Nobody wants to look back
at the old emails to figure out what comments you addressed.

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

[Stephen Boyd]

Quoting Taniya Das (2018-05-02 03:51:17)
> +
> +static DEFINE_MUTEX(rpmh_clk_lock);
> +
> +#define __DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name,   \
> +                         _res_en_offset, _res_on)                      \
> +       static struct clk_rpmh _platform##_##_name_active;              \
> +       static struct clk_rpmh _platform##_##_name = {                  \
> +               .res_name = _res_name,                                  \
> +               .res_addr = _res_en_offset,                             \
> +               .res_on_val = _res_on,                                  \
> +               .peer = &_platform##_##_name_active,                    \
> +               .valid_state_mask = (BIT(RPMH_WAKE_ONLY_STATE) |        \
> +                                     BIT(RPMH_ACTIVE_ONLY_STATE) |     \
> +                                     BIT(RPMH_SLEEP_STATE)),           \
> +               .hw.init = &(struct clk_init_data){                     \
> +                       .ops = &clk_rpmh_ops,                           \
> +                       .name = #_name,                                 \
> +                       .parent_names = (const char *[]){ "xo_board",   \
> +                                               "xo_board_div" },       \

The rpmh clks that do a div 2 can have a recalc_rate function that
returns the xo_board clk rate divided by 2 all the time. That may be
easier to do. Keep xo_board as parent, but then specify if it does div-2
or div-1 in the SoC specific listing. Then DT can only show 38.4 MHz.
The div 2 part is inside the SoC anyway so this seems more accurate too.

> +                       .num_parents = 2,                               \
> +               },                                                      \
> +       };                                                              \

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

[Stephen Boyd]

Quoting Taniya Das (2018-05-02 03:51:17)
> +               ret = devm_clk_hw_register(&pdev->dev, hw_clks[i]);
> +               if (ret) {
> +                       dev_err(&pdev->dev, "failed to register %s\n",
> +                                       hw_clks[i]->init->name);
> +                       goto err;
> +               }
> +
> +               rpmh_clk->dev = &pdev->dev;
> +       }
> +
> +       ret = devm_of_clk_add_hw_provider(&pdev->dev, of_clk_rpmh_hw_get,
> +                                     (void *) desc);

Drop the void cast.

> +       if (ret) {
> +               dev_err(&pdev->dev, "Failed to add clock provider\n");
> +               goto err;
> +       }
> +
> +       dev_dbg(&pdev->dev, "Registered RPMh clocks\n");
> +
> +       return 0;
> +err:
> +       if (rpmh_client)
> +               rpmh_release(rpmh_client);
> +
> +       return ret;
> +}
> +
> +static int clk_rpmh_remove(struct platform_device *pdev)
> +{
> +       const struct clk_rpmh_desc *desc =
> +                       of_device_get_match_data(&pdev->dev);
> +       struct clk_hw **hw_clks = desc->clks;
> +       struct clk_rpmh *rpmh_clk = to_clk_rpmh(hw_clks[0]);
> +
> +       rpmh_release(rpmh_clk->rpmh_client);
> +
> +       return 0;
> +}

I'll review Lina's series again. Still hoping to drop the client thing.

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

[Taniya Das]

Hello Stephen,

Thanks for review.

On 5/2/2018 9:00 PM, Stephen Boyd wrote:
> Quoting Taniya Das (2018-05-02 03:51:17)
>> +               ret = devm_clk_hw_register(&pdev->dev, hw_clks[i]);
>> +               if (ret) {
>> +                       dev_err(&pdev->dev, "failed to register %s\n",
>> +                                       hw_clks[i]->init->name);
>> +                       goto err;
>> +               }
>> +
>> +               rpmh_clk->dev = &pdev->dev;
>> +       }
>> +
>> +       ret = devm_of_clk_add_hw_provider(&pdev->dev, of_clk_rpmh_hw_get,
>> +                                     (void *) desc);
> 
> Drop the void cast.
> 

I see a compiler warning, that is the reason to keep the void cast.
/include/linux/clk-provider.h:881:5: note: expected ‘void *’ but 
argument is of type ‘const struct clk_rpmh_desc *’

>> +       if (ret) {
>> +               dev_err(&pdev->dev, "Failed to add clock provider\n");
>> +               goto err;
>> +       }
>> +
>> +       dev_dbg(&pdev->dev, "Registered RPMh clocks\n");
>> +
>> +       return 0;
>> +err:
>> +       if (rpmh_client)
>> +               rpmh_release(rpmh_client);
>> +
>> +       return ret;
>> +}
>> +
>> +static int clk_rpmh_remove(struct platform_device *pdev)
>> +{
>> +       const struct clk_rpmh_desc *desc =
>> +                       of_device_get_match_data(&pdev->dev);
>> +       struct clk_hw **hw_clks = desc->clks;
>> +       struct clk_rpmh *rpmh_clk = to_clk_rpmh(hw_clks[0]);
>> +
>> +       rpmh_release(rpmh_clk->rpmh_client);
>> +
>> +       return 0;
>> +}
> 
> I'll review Lina's series again. Still hoping to drop the client thing.
> 

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

--

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

[Stephen Boyd]

Quoting Taniya Das (2018-05-03 03:33:10)
> Hello Stephen,
> 
> Thanks for review.
> 
> On 5/2/2018 9:00 PM, Stephen Boyd wrote:
> > Quoting Taniya Das (2018-05-02 03:51:17)
> >> +               ret = devm_clk_hw_register(&pdev->dev, hw_clks[i]);
> >> +               if (ret) {
> >> +                       dev_err(&pdev->dev, "failed to register %s\n",
> >> +                                       hw_clks[i]->init->name);
> >> +                       goto err;
> >> +               }
> >> +
> >> +               rpmh_clk->dev = &pdev->dev;
> >> +       }
> >> +
> >> +       ret = devm_of_clk_add_hw_provider(&pdev->dev, of_clk_rpmh_hw_get,
> >> +                                     (void *) desc);
> > 
> > Drop the void cast.
> > 
> 
> I see a compiler warning, that is the reason to keep the void cast.
> /include/linux/clk-provider.h:881:5: note: expected ‘void *’ but 
> argument is of type ‘const struct clk_rpmh_desc *’

Ok. Casting away const is not good though. Can you pass a pointer to the
hw_clks array instead?