[v8] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver

[v8] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver

[Taniya Das]

 [v8]
   * cleanup code to remove of rpmh_client handle.
   * pass the hw_clks array to the devm_of_clk_add_hw_provider getter function.

 [v7]
   * Addressed comments from Stephen
	- Keep only xo_board as clock parent.
	- Associate divs from SoC specific clock listing to calculate
	   the clock rate.
	- Add back the recalc_rate clock ops to return the clock rate
	   based on the parent rate and div associated with clock.
	- void cast kept to avoid compiler warnings.

 [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 (1):
  clk: qcom: clk-rpmh: Add QCOM RPMh clock driver

 drivers/clk/qcom/Kconfig    |   9 ++
 drivers/clk/qcom/Makefile   |   1 +
 drivers/clk/qcom/clk-rpmh.c | 334 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 344 insertions(+)
 create mode 100644 drivers/clk/qcom/clk-rpmh.c

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

[v8] 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 | 334 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 344 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..944fe04
--- /dev/null
+++ b/drivers/clk/qcom/clk-rpmh.c
@@ -0,0 +1,334 @@
+// 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
+ * @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 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, _div)		\
+	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,					\
+		.div = _div,						\
+		.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" },	\
+			.num_parents = 1,				\
+		},							\
+	};								\
+	static struct clk_rpmh _platform##_##_name_active = {		\
+		.res_name = _res_name,					\
+		.res_addr = _res_en_offset,				\
+		.res_on_val = _res_on,					\
+		.div = _div,						\
+		.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" },	\
+			.num_parents = 1,				\
+		},							\
+	}
+
+#define DEFINE_CLK_RPMH_ARC(_platform, _name, _name_active, _res_name,	\
+			    _res_on, _div)				\
+	__DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name,	\
+			  CLK_RPMH_ARC_EN_OFFSET, _res_on, _div)
+
+#define DEFINE_CLK_RPMH_VRM(_platform, _name, _name_active, _res_name,	\
+				_div)					\
+	__DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name,	\
+			  CLK_RPMH_VRM_EN_OFFSET, 1, _div)
+
+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->dev, 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 unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw,
+					unsigned long prate)
+{
+	struct clk_rpmh *r = to_clk_rpmh(hw);
+	unsigned long rate = prate;
+
+	/*
+	 * RPMh clocks have a fixed rate. Return static rate.
+	 */
+	do_div(rate, r->div);
+	return 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, 2);
+DEFINE_CLK_RPMH_VRM(sdm845, ln_bb_clk2, ln_bb_clk2_ao, "lnbclka2", 2);
+DEFINE_CLK_RPMH_VRM(sdm845, ln_bb_clk3, ln_bb_clk3_ao, "lnbclka3", 2);
+DEFINE_CLK_RPMH_VRM(sdm845, rf_clk1, rf_clk1_ao, "rfclka1", 1);
+DEFINE_CLK_RPMH_VRM(sdm845, rf_clk2, rf_clk2_ao, "rfclka2", 1);
+DEFINE_CLK_RPMH_VRM(sdm845, rf_clk3, rf_clk3_ao, "rfclka3", 1);
+
+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_hw *hw_clks = data;
+	unsigned int idx = clkspec->args[0];
+
+	if (idx < 0) {
+		pr_err("%s: invalid index %u\n", __func__, idx);
+		return ERR_PTR(-EINVAL);
+	}
+
+	return &hw_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;
+	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;
+	}
+
+	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);
+			return -ENODEV;
+		}
+		rpmh_clk->res_addr += res_addr;
+		rpmh_clk->dev = &pdev->dev;
+
+		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);
+			return ret;
+		}
+	}
+
+	ret = devm_of_clk_add_hw_provider(&pdev->dev, of_clk_rpmh_hw_get,
+				      desc->clks);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to add clock provider\n");
+		return ret;
+	}
+
+	dev_dbg(&pdev->dev, "Registered RPMh clocks\n");
+
+	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,
+	.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: [v8] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver

[Stephen Boyd]

Quoting Taniya Das (2018-05-04 03:02:38)
> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
> new file mode 100644
> index 0000000..944fe04
> --- /dev/null
> +++ b/drivers/clk/qcom/clk-rpmh.c
> @@ -0,0 +1,334 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */
> +
> +};
> +
> +struct clk_rpmh_desc {
> +       struct clk_hw **clks;
> +       size_t num_clks;
> +};

This could be replaced with the clk_hw_onecell_data struct and then the
only problem becomes the const part which seems pretty impossible to fix
at this point. One "workaround" is to memdup the structure. Ugh.

> +
> +static DEFINE_MUTEX(rpmh_clk_lock);
> +
> +#define __DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name,   \
> +                         _res_en_offset, _res_on, _div)                \
> +       static struct clk_rpmh _platform##_##_name_active;              \
> +       static struct clk_rpmh _platform##_##_name = {                  \
[..]
> +
> +static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw,
> +                                       unsigned long prate)
> +{
> +       struct clk_rpmh *r = to_clk_rpmh(hw);
> +       unsigned long rate = prate;
> +
> +       /*
> +        * RPMh clocks have a fixed rate. Return static rate.
> +        */
> +       do_div(rate, r->div);

Integer division won't suffice?

> +       return rate;
> +}
[...]
> +
> +static struct clk_hw *of_clk_rpmh_hw_get(struct of_phandle_args *clkspec,
> +                                            void *data)
> +{
> +       struct clk_hw *hw_clks = data;
> +       unsigned int idx = clkspec->args[0];
> +
> +       if (idx < 0) {

It can't be less than 0 though because it's unsigned.

> +               pr_err("%s: invalid index %u\n", __func__, idx);
> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       return &hw_clks[idx];
> +}

Re: [v8] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver

[Stephen Boyd]

Quoting Taniya Das (2018-05-04 03:02:38)
> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
> new file mode 100644
> index 0000000..944fe04
> --- /dev/null
> +++ b/drivers/clk/qcom/clk-rpmh.c
> @@ -0,0 +1,334 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */
> +
> +};
> +
> +struct clk_rpmh_desc {
> +       struct clk_hw **clks;
> +       size_t num_clks;
> +};

This could be replaced with the clk_hw_onecell_data struct and then the
only problem becomes the const part which seems pretty impossible to fix
at this point. One "workaround" is to memdup the structure. Ugh.

> +
> +static DEFINE_MUTEX(rpmh_clk_lock);
> +
> +#define __DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name,   \
> +                         _res_en_offset, _res_on, _div)                \
> +       static struct clk_rpmh _platform##_##_name_active;              \
> +       static struct clk_rpmh _platform##_##_name = {                  \
[..]
> +
> +static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw,
> +                                       unsigned long prate)
> +{
> +       struct clk_rpmh *r = to_clk_rpmh(hw);
> +       unsigned long rate = prate;
> +
> +       /*
> +        * RPMh clocks have a fixed rate. Return static rate.
> +        */
> +       do_div(rate, r->div);

Integer division won't suffice?

> +       return rate;
> +}
[...]
> +
> +static struct clk_hw *of_clk_rpmh_hw_get(struct of_phandle_args *clkspec,
> +                                            void *data)
> +{
> +       struct clk_hw *hw_clks = data;
> +       unsigned int idx = clkspec->args[0];
> +
> +       if (idx < 0) {

It can't be less than 0 though because it's unsigned.

> +               pr_err("%s: invalid index %u\n", __func__, idx);
> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       return &hw_clks[idx];
> +}

Re: [v8] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver

[Stephen Boyd]

Quoting Taniya Das (2018-05-04 03:02:38)
> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
> new file mode 100644
> index 0000000..944fe04
> --- /dev/null
> +++ b/drivers/clk/qcom/clk-rpmh.c
> @@ -0,0 +1,334 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */
> +
> +};
> +
> +struct clk_rpmh_desc {
> +       struct clk_hw **clks;
> +       size_t num_clks;
> +};

This could be replaced with the clk_hw_onecell_data struct and then the
only problem becomes the const part which seems pretty impossible to fix
at this point. One "workaround" is to memdup the structure. Ugh.

> +
> +static DEFINE_MUTEX(rpmh_clk_lock);
> +
> +#define __DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name,   \
> +                         _res_en_offset, _res_on, _div)                \
> +       static struct clk_rpmh _platform##_##_name_active;              \
> +       static struct clk_rpmh _platform##_##_name =3D {                 =
 \
[..]
> +
> +static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw,
> +                                       unsigned long prate)
> +{
> +       struct clk_rpmh *r =3D to_clk_rpmh(hw);
> +       unsigned long rate =3D prate;
> +
> +       /*
> +        * RPMh clocks have a fixed rate. Return static rate.
> +        */
> +       do_div(rate, r->div);

Integer division won't suffice?

> +       return rate;
> +}
[...]
> +
> +static struct clk_hw *of_clk_rpmh_hw_get(struct of_phandle_args *clkspec,
> +                                            void *data)
> +{
> +       struct clk_hw *hw_clks =3D data;
> +       unsigned int idx =3D clkspec->args[0];
> +
> +       if (idx < 0) {

It can't be less than 0 though because it's unsigned.

> +               pr_err("%s: invalid index %u\n", __func__, idx);
> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       return &hw_clks[idx];
> +}

Re: [v8] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver

[Taniya Das]

Hello Stephen,

Could you please let me know your comments on the below.

On 5/4/2018 10:21 PM, Stephen Boyd wrote:
> Quoting Taniya Das (2018-05-04 03:02:38)
>> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
>> new file mode 100644
>> index 0000000..944fe04
>> --- /dev/null
>> +++ b/drivers/clk/qcom/clk-rpmh.c
>> @@ -0,0 +1,334 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +};
>> +
>> +struct clk_rpmh_desc {
>> +       struct clk_hw **clks;
>> +       size_t num_clks;
>> +};
> 
> This could be replaced with the clk_hw_onecell_data struct and then the
> only problem becomes the const part which seems pretty impossible to fix
> at this point. One "workaround" is to memdup the structure. Ugh.
> 

Will be okay, if I can the following?

_probe...
{
	struct clk_rpmh_desc *hw_desc_data;
	....

	hw_desc_data = kmemdup(desc, sizeof(*desc), GFP_KERNEL);

	...
	ret = devm_of_clk_add_hw_provider(&pdev->dev, 	  of_clk_rpmh_hw_get, 
hw_desc_data);
         ....

}

And also I fix the "getter" function.

>> +
>> +static DEFINE_MUTEX(rpmh_clk_lock);
>> +
>> +#define __DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name,   \
>> +                         _res_en_offset, _res_on, _div)                \
>> +       static struct clk_rpmh _platform##_##_name_active;              \
>> +       static struct clk_rpmh _platform##_##_name = {                  \
> [..]
>> +
>> +static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw,
>> +                                       unsigned long prate)
>> +{
>> +       struct clk_rpmh *r = to_clk_rpmh(hw);
>> +       unsigned long rate = prate;
>> +
>> +       /*
>> +        * RPMh clocks have a fixed rate. Return static rate.
>> +        */
>> +       do_div(rate, r->div);
> 
> Integer division won't suffice?
> 
>> +       return rate;
>> +}
> [...]
>> +
>> +static struct clk_hw *of_clk_rpmh_hw_get(struct of_phandle_args *clkspec,
>> +                                            void *data)
>> +{
>> +       struct clk_hw *hw_clks = data;
>> +       unsigned int idx = clkspec->args[0];
>> +
>> +       if (idx < 0) {
> 
> It can't be less than 0 though because it's unsigned.
> 
>> +               pr_err("%s: invalid index %u\n", __func__, idx);
>> +               return ERR_PTR(-EINVAL);
>> +       }
>> +
>> +       return &hw_clks[idx];
>> +}

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

--

Re: [v8] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver

[Stephen Boyd]

Quoting Taniya Das (2018-05-07 03:48:06)
> Hello Stephen,
> 
> Could you please let me know your comments on the below.
> 
> On 5/4/2018 10:21 PM, Stephen Boyd wrote:
> > Quoting Taniya Das (2018-05-04 03:02:38)
> >> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
> >> new file mode 100644
> >> index 0000000..944fe04
> >> --- /dev/null
> >> +++ b/drivers/clk/qcom/clk-rpmh.c
> >> @@ -0,0 +1,334 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> >> + */
> >> +
> >> +};
> >> +
> >> +struct clk_rpmh_desc {
> >> +       struct clk_hw **clks;
> >> +       size_t num_clks;
> >> +};
> > 
> > This could be replaced with the clk_hw_onecell_data struct and then the
> > only problem becomes the const part which seems pretty impossible to fix
> > at this point. One "workaround" is to memdup the structure. Ugh.
> > 
> 
> Will be okay, if I can the following?
> 
> _probe...
> {
>         struct clk_rpmh_desc *hw_desc_data;
>         ....
> 
>         hw_desc_data = kmemdup(desc, sizeof(*desc), GFP_KERNEL);
> 
>         ...
>         ret = devm_of_clk_add_hw_provider(&pdev->dev,     of_clk_rpmh_hw_get, 
> hw_desc_data);
>          ....
> 
> }
> 
> And also I fix the "getter" function.

I'd rather see the check for out of bounds number just go away, unless
that's helping something. The kmemdup() doesn't look good.

Re: [v8] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver

[Stephen Boyd]

Quoting Taniya Das (2018-05-07 03:48:06)
> Hello Stephen,
> =

> Could you please let me know your comments on the below.
> =

> On 5/4/2018 10:21 PM, Stephen Boyd wrote:
> > Quoting Taniya Das (2018-05-04 03:02:38)
> >> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
> >> new file mode 100644
> >> index 0000000..944fe04
> >> --- /dev/null
> >> +++ b/drivers/clk/qcom/clk-rpmh.c
> >> @@ -0,0 +1,334 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> >> + */
> >> +
> >> +};
> >> +
> >> +struct clk_rpmh_desc {
> >> +       struct clk_hw **clks;
> >> +       size_t num_clks;
> >> +};
> > =

> > This could be replaced with the clk_hw_onecell_data struct and then the
> > only problem becomes the const part which seems pretty impossible to fix
> > at this point. One "workaround" is to memdup the structure. Ugh.
> > =

> =

> Will be okay, if I can the following?
> =

> _probe...
> {
>         struct clk_rpmh_desc *hw_desc_data;
>         ....
> =

>         hw_desc_data =3D kmemdup(desc, sizeof(*desc), GFP_KERNEL);
> =

>         ...
>         ret =3D devm_of_clk_add_hw_provider(&pdev->dev,     of_clk_rpmh_h=
w_get, =

> hw_desc_data);
>          ....
> =

> }
> =

> And also I fix the "getter" function.

I'd rather see the check for out of bounds number just go away, unless
that's helping something. The kmemdup() doesn't look good.

Re: [v8] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver

[Taniya Das]

Hello Stephen,

Thanks for your review comments, please check my comments below,  so 
that I could submit the next patch series.

On 5/8/2018 5:58 AM, Stephen Boyd wrote:
> Quoting Taniya Das (2018-05-07 03:48:06)
>> Hello Stephen,
>>
>> Could you please let me know your comments on the below.
>>
>> On 5/4/2018 10:21 PM, Stephen Boyd wrote:
>>> Quoting Taniya Das (2018-05-04 03:02:38)
>>>> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
>>>> new file mode 100644
>>>> index 0000000..944fe04
>>>> --- /dev/null
>>>> +++ b/drivers/clk/qcom/clk-rpmh.c
>>>> @@ -0,0 +1,334 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>>>> + */
>>>> +
>>>> +};
>>>> +
>>>> +struct clk_rpmh_desc {
>>>> +       struct clk_hw **clks;
>>>> +       size_t num_clks;
>>>> +};
>>>
>>> This could be replaced with the clk_hw_onecell_data struct and then the
>>> only problem becomes the const part which seems pretty impossible to fix
>>> at this point. One "workaround" is to memdup the structure. Ugh.
>>>
>>
>> Will be okay, if I can the following?
>>
>> _probe...
>> {
>>          struct clk_rpmh_desc *hw_desc_data;
>>          ....
>>
>>          hw_desc_data = kmemdup(desc, sizeof(*desc), GFP_KERNEL);
>>
>>          ...
>>          ret = devm_of_clk_add_hw_provider(&pdev->dev,     of_clk_rpmh_hw_get,
>> hw_desc_data);
>>           ....
>>
>> }
>>
>> And also I fix the "getter" function.
> 
> I'd rather see the check for out of bounds number just go away, unless
> that's helping something. The kmemdup() doesn't look good.
> 

I think it is better to copy the desc data to "clk_hw_onecell_data", as 
I would also have to fix the "getter" function to check for out of bound 
numbers (idx >= num_clks).

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

--

Re: [v8] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver

[Stephen Boyd]

Quoting Taniya Das (2018-05-08 09:57:36)
> On 5/8/2018 5:58 AM, Stephen Boyd wrote:
> > Quoting Taniya Das (2018-05-07 03:48:06)
> >> Will be okay, if I can the following?
> >>
> >> _probe...
> >> {
> >>          struct clk_rpmh_desc *hw_desc_data;
> >>          ....
> >>
> >>          hw_desc_data = kmemdup(desc, sizeof(*desc), GFP_KERNEL);
> >>
> >>          ...
> >>          ret = devm_of_clk_add_hw_provider(&pdev->dev,     of_clk_rpmh_hw_get,
> >> hw_desc_data);
> >>           ....
> >>
> >> }
> >>
> >> And also I fix the "getter" function.
> > 
> > I'd rather see the check for out of bounds number just go away, unless
> > that's helping something. The kmemdup() doesn't look good.
> > 
> 
> I think it is better to copy the desc data to "clk_hw_onecell_data", as 
> I would also have to fix the "getter" function to check for out of bound 
> numbers (idx >= num_clks).
> 

Hmm ok. It would be nicer if we didn't have to duplicate the pointers
just because of const though. I'd rather have the cast away of const and
a comment indicating we know what we're doing. Maybe in the future we
can make the getter function take a const void pointer.

Re: [v8] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver

[Stephen Boyd]

Quoting Taniya Das (2018-05-08 09:57:36)
> On 5/8/2018 5:58 AM, Stephen Boyd wrote:
> > Quoting Taniya Das (2018-05-07 03:48:06)
> >> Will be okay, if I can the following?
> >>
> >> _probe...
> >> {
> >>          struct clk_rpmh_desc *hw_desc_data;
> >>          ....
> >>
> >>          hw_desc_data =3D kmemdup(desc, sizeof(*desc), GFP_KERNEL);
> >>
> >>          ...
> >>          ret =3D devm_of_clk_add_hw_provider(&pdev->dev,     of_clk_rp=
mh_hw_get,
> >> hw_desc_data);
> >>           ....
> >>
> >> }
> >>
> >> And also I fix the "getter" function.
> > =

> > I'd rather see the check for out of bounds number just go away, unless
> > that's helping something. The kmemdup() doesn't look good.
> > =

> =

> I think it is better to copy the desc data to "clk_hw_onecell_data", as =

> I would also have to fix the "getter" function to check for out of bound =

> numbers (idx >=3D num_clks).
> =


Hmm ok. It would be nicer if we didn't have to duplicate the pointers
just because of const though. I'd rather have the cast away of const and
a comment indicating we know what we're doing. Maybe in the future we
can make the getter function take a const void pointer.