Re: [[PATCH v2] 1/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver
From: Taniya Das
Date: Fri Apr 13 2018 - 22:03:48 EST
Hello Bjorn,
Thank you for the review comments.
On 4/10/2018 11:09 PM, Bjorn Andersson wrote:
On Sun 08 Apr 03:32 PDT 2018, Taniya Das wrote:
From: Amit Nischal <anischal@xxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxx>
Signed-off-by: Amit Nischal <anischal@xxxxxxxxxxxxxx>
Signed-off-by: Taniya Das <tdas@xxxxxxxxxxxxxx>
---
drivers/clk/qcom/Kconfig | 9 +
drivers/clk/qcom/Makefile | 1 +
drivers/clk/qcom/clk-rpmh.c | 394 ++++++++++++++++++++++++++++++++++
include/dt-bindings/clock/qcom,rpmh.h | 25 +++
4 files changed, 429 insertions(+)
create mode 100644 drivers/clk/qcom/clk-rpmh.c
create mode 100644 include/dt-bindings/clock/qcom,rpmh.h
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
QCOM_CLK_RPMH
Would update it to use "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 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..763401f
--- /dev/null
+++ b/drivers/clk/qcom/clk-rpmh.c
@@ -0,0 +1,394 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
Unused
+#include <soc/qcom/cmd-db.h>
+#include <soc/qcom/rpmh.h>
+
+#include <dt-bindings/clock/qcom,rpmh.h>
+
+#include "common.h"
+#include "clk-regmap.h"
Unused
I would remove the unused header includes.
+
+#define CLK_RPMH_ARC_EN_OFFSET 0
+#define CLK_RPMH_VRM_EN_OFFSET 4
+#define CLK_RPMH_VRM_OFF_VAL 0
+#define CLK_RPMH_VRM_ON_VAL 1
Use a bool (true/false) rather than lugging around these two defines.
I would remove these in the next patch.
+#define CLK_RPMH_APPS_RSC_AO_STATE_MASK (BIT(RPMH_WAKE_ONLY_STATE) | \
+ BIT(RPMH_ACTIVE_ONLY_STATE))
+#define CLK_RPMH_APPS_RSC_STATE_MASK (BIT(RPMH_WAKE_ONLY_STATE) | \
+ BIT(RPMH_ACTIVE_ONLY_STATE) | \
+ BIT(RPMH_SLEEP_STATE))
+struct clk_rpmh {
+ struct clk_hw hw;
+ const char *res_name;
+ u32 res_addr;
+ u32 res_en_offset;
+ u32 res_on_val;
+ u32 res_off_val;
res_off_val is 0 for all clocks.
They could change if required, so is the reason for keeping it.
+ u32 state;
+ u32 aggr_state;
+ u32 last_sent_aggr_state;
Through the use of some local variables you shouldn't have to lug around
aggr_state and last_sent_aggr_state.
We need to check if the last state and the current state requested,
has_state_changed().
+ u32 valid_state_mask;
+ struct rpmh_client *rpmh_client;
+ unsigned long rate;
+ struct clk_rpmh *peer;
+};
+
+struct rpmh_cc {
+ struct clk_onecell_data data;
+ struct clk *clks[];
+};
+
+struct clk_rpmh_desc {
+ struct clk_hw **clks;
+ size_t num_clks;
+};
+
+static DEFINE_MUTEX(rpmh_clk_lock);
+
+#define __DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name, \
+ _res_en_offset, _res_on, _res_off, _rate, \
+ _state_mask, _state_on_mask) \
+ static struct clk_rpmh _platform##_##_name_active; \
+ static struct clk_rpmh _platform##_##_name = { \
+ .res_name = _res_name, \
+ .res_en_offset = _res_en_offset, \
+ .res_on_val = _res_on, \
+ .res_off_val = _res_off, \
+ .rate = _rate, \
+ .peer = &_platform##_##_name_active, \
+ .valid_state_mask = _state_mask, \
This is always CLK_RPMH_APPS_RSC_STATE_MASK
+ .hw.init = &(struct clk_init_data){ \
+ .ops = &clk_rpmh_ops, \
+ .name = #_name, \
+ }, \
+ }; \
+ static struct clk_rpmh _platform##_##_name_active = { \
+ .res_name = _res_name, \
+ .res_en_offset = _res_en_offset, \
+ .res_on_val = _res_on, \
+ .res_off_val = _res_off, \
+ .rate = _rate, \
+ .peer = &_platform##_##_name, \
+ .valid_state_mask = _state_on_mask, \
and this is always CLK_RPMH_APPS_RSC_AO_STATE_MASK. If you just hard
code them here (until there's a need for them to be anything else) the
clock list below becomes tidier.
As of now the state_mask is CLK_RPMH_APPS_RSC_STATE_MASK and
state_on_mask is CLK_RPMH_APPS_RSC_AO_STATE_MASK. I would update the
logic if the state masks changes.
+ .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 & BIT(RPMH_SLEEP_STATE))
+ ? c->res_on_val : c->res_off_val;
As these values are reused several times in this function you could by
using some local variables get this down to the much more readable:
cmd.data = state & BIT(RPMH_SLEEP_STATE) ? on_val : off_val;
And as off_val is 0 for all your clocks you can even drop the latter.
+ ret = rpmh_write_async(c->rpmh_client, RPMH_SLEEP_STATE,
+ &cmd, 1);
+ if (ret) {
+ pr_err("%s: rpmh_write_async(%s, state=%d) failed (%d)\n",
+ __func__, c->res_name, RPMH_SLEEP_STATE, ret);
Please drop the __func__, the error string is unique in this driver; and
rather than passing a constant to a %d actually write your error message
in a human readable form, like: "set sleep state of %s failed: %d\n".
And please do carry the struct device, so that you can use dev_err()
instead.
Thanks, I would take care of the above comments and also loop thorugh
the enum to avoid duplication in the next patch.
+ return ret;
+ }
+ }
+
+ if (has_state_changed(c, RPMH_WAKE_ONLY_STATE)) {
+ cmd.data = (c->aggr_state & BIT(RPMH_WAKE_ONLY_STATE))
+ ? c->res_on_val : c->res_off_val;
+ ret = rpmh_write_async(c->rpmh_client,
+ RPMH_WAKE_ONLY_STATE, &cmd, 1);
+ if (ret) {
+ pr_err("%s: rpmh_write_async(%s, state=%d) failed (%d)\n"
You're missing a , for this to compile.
Thanks, would fix in the next patch.
+ __func__, c->res_name, RPMH_WAKE_ONLY_STATE,
+ ret);
+ return ret;
+ }
+ }
+
+ if (has_state_changed(c, RPMH_ACTIVE_ONLY_STATE)) {
+ cmd.data = (c->aggr_state & BIT(RPMH_ACTIVE_ONLY_STATE))
+ ? c->res_on_val : c->res_off_val;
+ ret = rpmh_write(c->rpmh_client, RPMH_ACTIVE_ONLY_STATE,
+ &cmd, 1);
+ if (ret) {
+ pr_err("%s: rpmh_write(%s, state=%d) failed (%d)\n",
+ __func__, c->res_name, RPMH_ACTIVE_ONLY_STATE,
+ ret);
+ return ret;
+ }
+ }
But, RPMH_SLEEP_STATE, RPMH_WAKE_ONLY_STATE and RPMH_ACTIVE_ONLY_STATE
are values in an enum, so you could roll these up in a for loop and
reduce the duplication.
+
+ c->last_sent_aggr_state = c->aggr_state;
+ c->peer->last_sent_aggr_state = c->last_sent_aggr_state;
+
+ return 0;
+}
+
+static int clk_rpmh_aggregate_state_send_command(struct clk_rpmh *c,
+ bool enable)
+{
+ /* Update state and aggregate state values based on enable value. */
+ c->state = enable ? c->valid_state_mask : 0;
+ c->aggr_state = c->state | c->peer->state;
+ c->peer->aggr_state = c->aggr_state;
+
+ ret = clk_rpmh_send_aggregate_command(c);
"ret" doesn't exist.
Thanks, I would add the missing variable.
+ if (ret && enable)
+ c->state = 0;
+ else if (ret) {
If you have any part of your conditional wrapped in braces do wrap all
the others too.
Thanks, would take care in the next patch.
+ c->state = c->valid_state_mask;
+ WARN(1, "clk: %s failed to disable\n", c->res_name);
+ }
+
+ return ret;
+}
+
+static int clk_rpmh_prepare(struct clk_hw *hw)
+{
+ struct clk_rpmh *c = to_clk_rpmh(hw);
+ int ret = 0;
+
+ mutex_lock(&rpmh_clk_lock);
+
+ if (c->state)
+ goto out;
+
+ ret = clk_rpmh_aggregate_state_send_command(c, true);
+out:
+ mutex_unlock(&rpmh_clk_lock);
+ return ret;
+};
+
+static void clk_rpmh_unprepare(struct clk_hw *hw)
+{
+ struct clk_rpmh *c = to_clk_rpmh(hw);
+
+ mutex_lock(&rpmh_clk_lock);
+
+ if (!c->state)
+ goto out;
+
+ clk_rpmh_aggregate_state_send_command(c, false);
+out:
+ mutex_unlock(&rpmh_clk_lock);
+ return;
+};
+
+static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ struct clk_rpmh *r = to_clk_rpmh(hw);
+
+ /*
+ * RPMh clocks have a fixed rate. Return static rate set
+ * at init time.
+ */
+ return r->rate;
+}
+
+static const struct clk_ops clk_rpmh_ops = {
+ .prepare = clk_rpmh_prepare,
+ .unprepare = clk_rpmh_unprepare,
+ .recalc_rate = clk_rpmh_recalc_rate,
+};
+
+/* Resource name must match resource id present in cmd-db. */
+DEFINE_CLK_RPMH_ARC(sdm845, bi_tcxo, bi_tcxo_ao, "xo.lvl", 0x3, 0x0,
+ 19200000, CLK_RPMH_APPS_RSC_STATE_MASK,
+ CLK_RPMH_APPS_RSC_AO_STATE_MASK);
+DEFINE_CLK_RPMH_VRM(sdm845, ln_bb_clk2, ln_bb_clk2_ao, "lnbclka2",
+ 19200000, CLK_RPMH_APPS_RSC_STATE_MASK,
+ CLK_RPMH_APPS_RSC_AO_STATE_MASK);
+DEFINE_CLK_RPMH_VRM(sdm845, ln_bb_clk3, ln_bb_clk3_ao, "lnbclka3",
+ 19200000, CLK_RPMH_APPS_RSC_STATE_MASK,
+ CLK_RPMH_APPS_RSC_AO_STATE_MASK);
+DEFINE_CLK_RPMH_VRM(sdm845, rf_clk1, rf_clk1_ao, "rfclka1",
+ 38400000, CLK_RPMH_APPS_RSC_STATE_MASK,
+ CLK_RPMH_APPS_RSC_AO_STATE_MASK);
+DEFINE_CLK_RPMH_VRM(sdm845, rf_clk2, rf_clk2_ao, "rfclka2",
+ 38400000, CLK_RPMH_APPS_RSC_STATE_MASK,
+ CLK_RPMH_APPS_RSC_AO_STATE_MASK);
+DEFINE_CLK_RPMH_VRM(sdm845, rf_clk3, rf_clk3_ao, "rfclka3",
+ 38400000, CLK_RPMH_APPS_RSC_STATE_MASK,
+ CLK_RPMH_APPS_RSC_AO_STATE_MASK);
+
+static struct clk_hw *sdm845_rpmh_clocks[] = {
+ [RPMH_CXO_CLK] = &sdm845_bi_tcxo.hw,
+ [RPMH_CXO_CLK_A] = &sdm845_bi_tcxo_ao.hw,
Registering these fails with EEXIST because the gcc driver also register
them.
It was added till the time RPMh driver was not ready. We would submit
removing the change from GCC driver.
+ [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);
Please move the match_table just prior to the definition of your
platform_driver.
Would update this in the next patch.
+
+static int clk_rpmh_probe(struct platform_device *pdev)
+{
+ struct clk **clks;
+ struct clk *clk;
+ struct rpmh_cc *rcc;
+ struct clk_onecell_data *data;
+ int ret;
+ size_t num_clks, i;
+ struct clk_hw **hw_clks;
+ struct clk_rpmh *rpmh_clk;
+ const struct clk_rpmh_desc *desc;
+ struct property *prop;
prop is unused.
Thanks, would remove the unused variable.
+ struct rpmh_client *rpmh_client = NULL;
Don't goto err until you have acquired rpmh_client and you don't need to
initialize this variable.
Would remove the variable initialization in the next patch.
+
+ desc = of_device_get_match_data(&pdev->dev);
+ if (!desc) {
+ ret = -EINVAL;
+ goto err;
+ }
This won't happen, no need to check for it.
Would remove this check too in the next patch.
+
+ ret = cmd_db_ready();
+ if (ret) {
+ if (ret != -EPROBE_DEFER) {
+ dev_err(&pdev->dev, "Command DB not available (%d)\n",
+ ret);
+ goto err;
goto err? There's nothing to clean up at this point.
I would fix this in the next patch.
+ }
+ return ret;
+ }
+
+ rpmh_client = rpmh_get_client(pdev);
+ if (IS_ERR(rpmh_client)) {
+ ret = PTR_ERR(rpmh_client);
+ if (ret != -EPROBE_DEFER)
You're getting a handle to your parent, it's not going to return
-EPROBE_DEFER.
I would fix the error path in the next patch.
+ 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)) {
Please add:
dev_err(&pdev->dev, "failed to register %s\n", hw_clks[i]->init->name);
+ ret = PTR_ERR(clk);
+ goto err;
+ }
+
+ clks[i] = clk;
+ }
+
+ ret = of_clk_add_provider(pdev->dev.of_node, of_clk_src_onecell_get,
+ data);
+ if (ret)
Please add:
dev_err(&pdev->dev, "failed to add clock provider\n");
+ goto err;
+
+ dev_info(&pdev->dev, "Registered RPMh clocks\n");
Please don't spam the kernel log.
+ return ret;
ret can't be anything but 0 here, so let's make it easer to read by
writing 0 here.
+
+err:
+ if (rpmh_client)
+ rpmh_release(rpmh_client);
This is annoying (but not your fault).
+
+ dev_err(&pdev->dev, "Error registering RPMh Clock driver (%d)\n", ret);
Testing the driver I got this error message, it didn't help! Had to add
the two dev_err above, just drop this one.
I would take care of the above comments in the next patch.
+ return ret;
+}
+
+static struct platform_driver clk_rpmh_driver = {
+ .probe = clk_rpmh_probe,
Your driver is tristate and works just fine as a kernel module, so you
need a remove function to call rpmh_release(rpmh_client) - or we need to
get rid of the need to call that.
Yes, I would add the remove and do the rpmh_client and of_del_provider
clean ups.
+ .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");
We use "Qualcomm" or "QCOM" in all existing driver, can you please use
that here too?
Would update it to use "QCOM".
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:clk-rpmh");
Nothing is going to attempt to autoload a driver based on the alias
platform:clk-rpmh, so just drop this.
Would drop this in the next patch.
Regards,
Bjorn
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.
--