* [PATCH 0/2] Add QCOM RPMh Clock driver @ 2018-03-29 6:17 Taniya Das 2018-03-29 6:17 ` [PATCH 1/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver Taniya Das 2018-03-29 6:17 ` [PATCH 2/2] dt-bindings: clock: Introduce QCOM RPMh clock bindings Taniya Das 0 siblings, 2 replies; 13+ messages in thread From: Taniya Das @ 2018-03-29 6:17 UTC (permalink / raw) To: Stephen Boyd, Michael Turquette Cc: Andy Gross, David Brown, Rajendra Nayak, Odelu Kukatla, Amit Nischal, linux-arm-msm, linux-soc, linux-clk, linux-kernel, 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver 2018-03-29 6:17 [PATCH 0/2] Add QCOM RPMh Clock driver Taniya Das @ 2018-03-29 6:17 ` Taniya Das 2018-03-29 21:49 ` Evan Green 2018-03-29 6:17 ` [PATCH 2/2] dt-bindings: clock: Introduce QCOM RPMh clock bindings Taniya Das 1 sibling, 1 reply; 13+ messages in thread From: Taniya Das @ 2018-03-29 6:17 UTC (permalink / raw) To: Stephen Boyd, Michael Turquette Cc: Andy Gross, David Brown, Rajendra Nayak, Odelu Kukatla, Amit Nischal, linux-arm-msm, linux-soc, linux-clk, linux-kernel, David Collins, 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. ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver 2018-03-29 6:17 ` [PATCH 1/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver Taniya Das @ 2018-03-29 21:49 ` Evan Green 2018-04-02 10:33 ` Taniya Das 0 siblings, 1 reply; 13+ messages in thread From: Evan Green @ 2018-03-29 21:49 UTC (permalink / raw) To: tdas Cc: sboyd, mturquette, Andy Gross, David Brown, Rajendra Nayak, okukatla, anischal, linux-arm-msm, linux-soc, linux-clk, linux-kernel, collinsd 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver 2018-03-29 21:49 ` Evan Green @ 2018-04-02 10:33 ` Taniya Das 2018-04-03 17:55 ` Evan Green 2018-04-05 23:17 ` Stephen Boyd 0 siblings, 2 replies; 13+ messages in thread From: Taniya Das @ 2018-04-02 10:33 UTC (permalink / raw) To: Evan Green Cc: sboyd, mturquette, Andy Gross, David Brown, Rajendra Nayak, okukatla, anischal, linux-arm-msm, linux-soc, linux-clk, linux-kernel, collinsd 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. -- ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver 2018-04-02 10:33 ` Taniya Das @ 2018-04-03 17:55 ` Evan Green 2018-04-05 23:17 ` Stephen Boyd 1 sibling, 0 replies; 13+ messages in thread From: Evan Green @ 2018-04-03 17:55 UTC (permalink / raw) To: tdas Cc: sboyd, Michael Turquette, Andy Gross, David Brown, Rajendra Nayak, okukatla, anischal, linux-arm-msm, linux-soc, linux-clk, linux-kernel, collinsd 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver 2018-04-02 10:33 ` Taniya Das @ 2018-04-05 23:17 ` Stephen Boyd 2018-04-05 23:17 ` Stephen Boyd 1 sibling, 0 replies; 13+ messages in thread From: Stephen Boyd @ 2018-04-05 23:17 UTC (permalink / raw) To: Evan Green, Taniya Das Cc: sboyd, mturquette, Andy Gross, David Brown, Rajendra Nayak, okukatla, anischal, linux-arm-msm, linux-soc, linux-clk, linux-kernel, collinsd 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver @ 2018-04-05 23:17 ` Stephen Boyd 0 siblings, 0 replies; 13+ messages in thread From: Stephen Boyd @ 2018-04-05 23:17 UTC (permalink / raw) To: Evan Green, Taniya Das Cc: sboyd, mturquette, Andy Gross, David Brown, Rajendra Nayak, okukatla, anischal, linux-arm-msm, linux-soc, linux-clk, linux-kernel, collinsd 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver 2018-04-05 23:17 ` Stephen Boyd (?) @ 2018-04-10 17:05 ` Evan Green -1 siblings, 0 replies; 13+ messages in thread From: Evan Green @ 2018-04-10 17:05 UTC (permalink / raw) To: sboyd Cc: tdas, sboyd, Michael Turquette, Andy Gross, David Brown, Rajendra Nayak, okukatla, anischal, linux-arm-msm, linux-soc, linux-clk, linux-kernel, collinsd 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/2] dt-bindings: clock: Introduce QCOM RPMh clock bindings 2018-03-29 6:17 [PATCH 0/2] Add QCOM RPMh Clock driver Taniya Das 2018-03-29 6:17 ` [PATCH 1/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver Taniya Das @ 2018-03-29 6:17 ` Taniya Das 2018-04-05 23:20 ` Stephen Boyd 1 sibling, 1 reply; 13+ messages in thread From: Taniya Das @ 2018-03-29 6:17 UTC (permalink / raw) To: Stephen Boyd, Michael Turquette Cc: Andy Gross, David Brown, Rajendra Nayak, Odelu Kukatla, Amit Nischal, linux-arm-msm, linux-soc, linux-clk, linux-kernel, 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. ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] dt-bindings: clock: Introduce QCOM RPMh clock bindings 2018-03-29 6:17 ` [PATCH 2/2] dt-bindings: clock: Introduce QCOM RPMh clock bindings Taniya Das 2018-04-05 23:20 ` Stephen Boyd @ 2018-04-05 23:20 ` Stephen Boyd 0 siblings, 0 replies; 13+ messages in thread From: Stephen Boyd @ 2018-04-05 23:20 UTC (permalink / raw) To: Michael Turquette, Stephen Boyd Cc: Andy Gross, David Brown, Rajendra Nayak, Odelu Kukatla, Amit Nischal, linux-arm-msm, linux-soc, linux-clk, linux-kernel, Taniya Das 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? > + }; > + }; ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] dt-bindings: clock: Introduce QCOM RPMh clock bindings @ 2018-04-05 23:20 ` Stephen Boyd 0 siblings, 0 replies; 13+ messages in thread From: Stephen Boyd @ 2018-04-05 23:20 UTC (permalink / raw) To: Michael Turquette, Stephen Boyd, Taniya Das Cc: Andy Gross, David Brown, Rajendra Nayak, Odelu Kukatla, Amit Nischal, linux-arm-msm, linux-soc, linux-clk, linux-kernel, Taniya Das 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? > + }; > + }; ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] dt-bindings: clock: Introduce QCOM RPMh clock bindings @ 2018-04-05 23:20 ` Stephen Boyd 0 siblings, 0 replies; 13+ messages in thread From: Stephen Boyd @ 2018-04-05 23:20 UTC (permalink / raw) To: Michael Turquette, Stephen Boyd, Taniya Das Cc: Andy Gross, David Brown, Rajendra Nayak, Odelu Kukatla, Amit Nischal, linux-arm-msm, linux-soc, linux-clk, linux-kernel, Taniya Das 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? > + }; > + }; ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] dt-bindings: clock: Introduce QCOM RPMh clock bindings 2018-04-05 23:20 ` Stephen Boyd (?) (?) @ 2018-04-08 10:11 ` Taniya Das -1 siblings, 0 replies; 13+ messages in thread From: Taniya Das @ 2018-04-08 10:11 UTC (permalink / raw) To: Stephen Boyd, Michael Turquette, Stephen Boyd Cc: Andy Gross, David Brown, Rajendra Nayak, Odelu Kukatla, Amit Nischal, linux-arm-msm, linux-soc, linux-clk, linux-kernel 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. -- ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-04-10 17:06 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-03-29 6:17 [PATCH 0/2] Add QCOM RPMh Clock driver Taniya Das 2018-03-29 6:17 ` [PATCH 1/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver Taniya Das 2018-03-29 21:49 ` Evan Green 2018-04-02 10:33 ` Taniya Das 2018-04-03 17:55 ` Evan Green 2018-04-05 23:17 ` Stephen Boyd 2018-04-05 23:17 ` Stephen Boyd 2018-04-10 17:05 ` Evan Green 2018-03-29 6:17 ` [PATCH 2/2] dt-bindings: clock: Introduce QCOM RPMh clock bindings Taniya Das 2018-04-05 23:20 ` Stephen Boyd 2018-04-05 23:20 ` Stephen Boyd 2018-04-05 23:20 ` Stephen Boyd 2018-04-08 10:11 ` Taniya Das
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.