clk: qcom: clk-rpmh: Add QCOM RPMh clock driver
v3 cover clk: qcom: clk-rpmh: Add QCOM RPMh clock driver
[v3]
* Addressed documentation & code review comments from Bjorn
* Addressed bindings comments from Rob
* Updated the patch series order for bindings
[v2]
* Addressed comments from Stephen
* Addressed comments from Evan
This patch series adds a driver and device tree documentation binding for the clock control via Resource Power Manager-hardened (RPMh) on some Qualcomm Technologies, Inc, SoCs such as SDM845. The clock RPMh driver would send requests for the RPMh managed clock resources.
The RPMh clock driver depends upon the RPMh driver [1] and command DB driver [2] which are both still undergoing review.
Thanks,
Taniya
[1]: https://lkml.org/lkml/2018/3/9/979
[2]: https://lkml.org/lkml/2018/3/14/787
Taniya Das (2):
dt-bindings: clock: Introduce QCOM RPMh clock bindings
clk: qcom: clk-rpmh: Add QCOM RPMh clock driver
.../devicetree/bindings/clock/qcom,rpmh-clk.txt | 22 ++ drivers/clk/qcom/Kconfig | 9 + drivers/clk/qcom/Makefile | 1 + drivers/clk/qcom/clk-rpmh.c | 367 +++++++++++++++++++++ include/dt-bindings/clock/qcom,rpmh.h | 24 ++ 5 files changed, 423 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt create mode 100644 drivers/clk/qcom/clk-rpmh.c create mode 100644 include/dt-bindings/clock/qcom,rpmh.h
--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member of the Code Aurora Forum, hosted by the Linux Foundation.
PATCH v3 1/2 dt-bindings: clock: Introduce QCOM RPMh clock bindings
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.
--- .../devicetree/bindings/clock/qcom,rpmh-clk.txt | 22 ++++++++++++++++++++ include/dt-bindings/clock/qcom,rpmh.h | 24 ++++++++++++++++++++++ 2 files changed, 46 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt create mode 100644 include/dt-bindings/clock/qcom,rpmh.h
Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt new file +22 -0
@@ -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,sdm845-rpmh-clk" + +- #clock-cells : must contain 1 + +Example : + +#include <dt-bindings/clock/qcom,rpmh.h> + + &apps_rsc { + rpmhcc: clock-controller { + compatible = "qcom,sdm845-rpmh-clk"; + #clock-cells = <1>; + }; + };
include/dt-bindings/clock/qcom,rpmh.h new file +24 -0
@@ -0,0 +1,24 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* Copyright (c) 2018, The Linux Foundation. All rights reserved. */ + + +#ifndef _DT_BINDINGS_CLK_MSM_RPMH_H +#define _DT_BINDINGS_CLK_MSM_RPMH_H + +/* RPMh controlled clocks */ +#define RPMH_CXO_CLK 0 +#define RPMH_CXO_CLK_A 1 +#define RPMH_LN_BB_CLK2 2 +#define RPMH_LN_BB_CLK2_A 3 +#define RPMH_LN_BB_CLK3 4 +#define RPMH_LN_BB_CLK3_A 5 +#define RPMH_RF_CLK1 6 +#define RPMH_RF_CLK1_A 7 +#define RPMH_RF_CLK2 8 +#define RPMH_RF_CLK2_A 9 +#define RPMH_RF_CLK3 10 +#define RPMH_RF_CLK3_A 11 +#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.
Stephen Boyd
maintainerRe: [PATCH v3 1/2] dt-bindings: clock: Introduce QCOM RPMh clock bindings
Quoting Taniya Das (2018-04-13 19:36:40)
Quoted 5 lines
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 <[email protected]>
Is Amit the author? Needs a "From: Amit .." then.
Quoted 1 line
Signed-off-by Taniya Das <[email protected]>
Re: [PATCH v3 1/2] dt-bindings: clock: Introduce QCOM RPMh clock bindings
In reply to Taniya DasQuoted 4 lines · Taniya Das
Add RPMh clock device bindings for Qualcomm Technology Inc's SoCs. These devices would be used for communicating resource state requests to control the clocks managed by RPMh.
Quoted 8 lines
Signed-off-by Amit Nischal <[email protected]>Signed-off-by Taniya Das <[email protected]>--- .../devicetree/bindings/clock/qcom,rpmh-clk.txt | 22 ++++++++++++++++++++ include/dt-bindings/clock/qcom,rpmh.h | 24 ++++++++++++++++++++++ 2 files changed, 46 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt create mode 100644 include/dt-bindings/clock/qcom,rpmh.h
PATCH v3 2/2 clk: qcom: clk-rpmh: Add QCOM RPMh clock driver
In reply to Taniya DasAdd the RPMh clock driver to control the RPMh managed clock resources on some of the Qualcomm Technologies, Inc. SoCs.
--- drivers/clk/qcom/Kconfig | 9 ++ drivers/clk/qcom/Makefile | 1 + drivers/clk/qcom/clk-rpmh.c | 367 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 377 insertions(+) create mode 100644 drivers/clk/qcom/clk-rpmh.c
drivers/clk/qcom/Kconfig +9 -0
@@ -112,6 +112,15 @@ config IPQ_GCC_8074 i2c, USB, SD/eMMC, etc. Select this for the root clock of ipq8074. +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 MSM_GCC_8660 tristate "MSM8660 Global Clock Controller" depends on COMMON_CLK_QCOM
drivers/clk/qcom/Makefile +1 -0
@@ -36,5 +36,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
drivers/clk/qcom/clk-rpmh.c new file +367 -0
367 lines not on this page · raw message
--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member of the Code Aurora Forum, hosted by the Linux Foundation.
Stephen Boyd
maintainerQuoting Taniya Das (2018-04-13 19:36:41)
Quoted 5 lines
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 <[email protected]>Signed-off-by Amit Nischal <[email protected]>
Your signoff chain is very confused. The first signoff should match the "From:" header but that doesn't seem to be the case here. And the sender should be the last in the chain.
Quoted 13 lines
--- drivers/clk/qcom/Kconfig | 9 ++ drivers/clk/qcom/Makefile | 1 + drivers/clk/qcom/clk-rpmh.c | 367 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 377 insertions(+) create mode 100644 drivers/clk/qcom/clk-rpmh.cdrivers/clk/qcom/Kconfig +0
-0@@ -112,6 +112,15 @@ config IPQ_GCC_8074 i2c, USB, SD/eMMC, etc. Select this for the root clock
drivers/clk/qcom/Kconfig config IPQ_GCC_8074 · line 122
Quoted 10 lines
drivers/clk/qcom/Kconfig +8
-0of ipq8074. +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.
Capitalize sdm?
drivers/clk/qcom/Kconfig config IPQ_GCC_8074 · line 123
Quoted 1 line
drivers/clk/qcom/Kconfig +1
-0+
Can this Kconfig be put into alphabetical order in this file?
Quoted 20 lines
drivers/clk/qcom/Kconfig +0
-0config MSM_GCC_8660 tristate "MSM8660 Global Clock Controller" depends on COMMON_CLK_QCOMdrivers/clk/qcom/Makefile +1
-0@@ -36,5 +36,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.odrivers/clk/qcom/clk-rpmh.c new file +0
-0
drivers/clk/qcom/clk-rpmh.c line 6
Quoted 6 lines
drivers/clk/qcom/clk-rpmh.c new file +6
-0@@ -0,0 +1,367 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Copyright (c) 2018, The Linux Foundation. All rights reserved. +*/ + +#include <linux/clk.h>
Is this include used?
Quoted 9 lines
drivers/clk/qcom/clk-rpmh.c new file +9
-0+#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>
drivers/clk/qcom/clk-rpmh.c line 25
Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c new file +10
-0+ +#include <dt-bindings/clock/qcom,rpmh.h> + +#define CLK_RPMH_ARC_EN_OFFSET 0 +#define CLK_RPMH_VRM_EN_OFFSET 4 +#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))
Is there a reason these aren't inlined into the one place they're used?
drivers/clk/qcom/clk-rpmh.c line 30
Quoted 5 lines
drivers/clk/qcom/clk-rpmh.c new file +5
-0+struct clk_rpmh { +struct clk_hw hw; +const char *res_name; +u32 res_addr; +u32 res_en_offset;
Why do we store both res_addr and res_en_offset? Can't we just store res_en_offset and then use that all the time? I don't see a user of res_addr anywhere.
drivers/clk/qcom/clk-rpmh.c line 32
Quoted 2 lines
drivers/clk/qcom/clk-rpmh.c new file +2
-0+u32 res_on_val; +u32 res_off_val;
Is this used?
drivers/clk/qcom/clk-rpmh.c line 41
Quoted 9 lines
drivers/clk/qcom/clk-rpmh.c new file +9
-0+u32 state; +u32 aggr_state; +u32 last_sent_aggr_state; +u32 valid_state_mask; +struct rpmh_client *rpmh_client; +struct device *dev; +struct clk_rpmh *peer; +unsigned long rate; +};
Can you add some kernel-doc on these structure members?
drivers/clk/qcom/clk-rpmh.c line 46
Quoted 5 lines
drivers/clk/qcom/clk-rpmh.c new file +5
-0+ +struct rpmh_cc { +struct clk_onecell_data data; +struct clk *clks[]; +};
Hopefully this structure isn't needed.
Quoted 6 lines
drivers/clk/qcom/clk-rpmh.c new file +6
-0+ +struct clk_rpmh_desc { +struct clk_hw **clks; +size_t num_clks; +}; +
[...]
drivers/clk/qcom/clk-rpmh.c line 103
Quoted 5 lines
drivers/clk/qcom/clk-rpmh.c new file +5
-0+ +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)));
Too many parenthesis here.
Quoted 30 lines
drivers/clk/qcom/clk-rpmh.c new file +30
-0+} + +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 =0 ; + + cmd.addr = c->res_addr + c->res_en_offset; + cmd_state = c->aggr_state; + on_val = c->res_on_val; + +for (; state <=RPMH_ACTIVE_ONLY_STATE ; state++) { +if (has_state_changed (c, state)) { + cmd.data = (cmd_state &BIT (state)) ? on_val :0 ; + ret =rpmh_write_async (c->rpmh_client, state, + &cmd,1 ); +if (ret) { +dev_err (c->dev,"set %s state of %s failed: (%d)\n" , + (!state) ?"sleep" : + (state ==RPMH_WAKE_ONLY_STATE ) ? +"wake" :"active" , c->res_name, ret); +return ret; + } + } + } + + c->last_sent_aggr_state = c->aggr_state; + c->peer->last_sent_aggr_state = c->last_sent_aggr_state;
drivers/clk/qcom/clk-rpmh.c line 143
Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c new file +10
-0+ +return 0 ; +} + +static int clk_rpmh_aggregate_state_send_command (struct clk_rpmh *c, +bool enable) +{ +int ret; + +/* Update state and aggregate state values based on enable value. */
Maybe this comment can go above the function itself.
Quoted 16 lines
drivers/clk/qcom/clk-rpmh.c new file +16
-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 && enable) { + c->state =0 ; + }else if (ret) { + 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)
drivers/clk/qcom/clk-rpmh.c line 169
Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c new file +10
-0+{ +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);
Rewrite this as:
if (!c->state) ret = clk_rpmh_aggregate_state_send_command(c, true);
and drop the goto.
Quoted 6 lines
drivers/clk/qcom/clk-rpmh.c new file +6
-0+out: +mutex_unlock (&rpmh_clk_lock); +return ret; +}; + +static void clk_rpmh_unprepare (struct clk_hw *hw)
drivers/clk/qcom/clk-rpmh.c line 185
Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c new file +10
-0+{ +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:
Same comment.
Quoted 4 lines
drivers/clk/qcom/clk-rpmh.c new file +4
-0+mutex_unlock (&rpmh_clk_lock); +return ; +}; +
drivers/clk/qcom/clk-rpmh.c line 199
Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c new file +10
-0+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;
The rate should come from the parent. In the case of tcxo it would be board_xo clk rate (or maybe some fixed div-2 on the board XO that's also defined in DT because the board_xo seems to be two times 19.2 MHz?).
Quoted 47 lines
drivers/clk/qcom/clk-rpmh.c new file +47
-0+} + +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 ); +DEFINE_CLK_RPMH_VRM (sdm845, ln_bb_clk2, ln_bb_clk2_ao,"lnbclka2" ,19200000 ); +DEFINE_CLK_RPMH_VRM (sdm845, ln_bb_clk3, ln_bb_clk3_ao,"lnbclka3" ,19200000 ); +DEFINE_CLK_RPMH_VRM (sdm845, rf_clk1, rf_clk1_ao,"rfclka1" ,38400000 ); +DEFINE_CLK_RPMH_VRM (sdm845, rf_clk2, rf_clk2_ao,"rfclka2" ,38400000 ); +DEFINE_CLK_RPMH_VRM (sdm845, rf_clk3, rf_clk3_ao,"rfclka3" ,38400000 ); + +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 int clk_rpmh_probe (struct platform_device *pdev) +{ +struct clk **clks; +struct clk *clk; +struct rpmh_cc *rcc; +struct clk_onecell_data *data; +struct clk_hw **hw_clks; +struct clk_rpmh *rpmh_clk; +const struct clk_rpmh_desc *desc; +struct rpmh_client *rpmh_client; +struct device *dev;
drivers/clk/qcom/clk-rpmh.c line 256
Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c new file +10
-0+size_t num_clks, i; +int ret; + + dev = &pdev->dev; + + ret =cmd_db_ready (); +if (ret) { +if (ret != -EPROBE_DEFER ) { +dev_err (dev,"Command DB not available (%d)\n" , ret); + goto fail;
Please just return error here instead of goto.
drivers/clk/qcom/clk-rpmh.c line 266
Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c new file +10
-0+ } +return ret; + } + + rpmh_client =rpmh_get_client (pdev); +if (IS_ERR (rpmh_client)) { + ret =PTR_ERR (rpmh_client); +dev_err (dev,"Failed to request RPMh client, ret=%d\n" , ret); +return ret; + }
I hope we get rid of rpmh_get_client() still.
Quoted 24 lines
drivers/clk/qcom/clk-rpmh.c new file +24
-0+ + desc =of_device_get_match_data (&pdev->dev); + 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 (dev,"missing RPMh resource address for %s\n" , + rpmh_clk->res_name); + ret = -ENODEV ;
drivers/clk/qcom/clk-rpmh.c line 300
Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c new file +10
-0+ 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); +dev_err (dev,"failed to register %s\n" , + hw_clks[i]->init->name);
This isn't a useful error message either.
drivers/clk/qcom/clk-rpmh.c line 308
Quoted 8 lines
drivers/clk/qcom/clk-rpmh.c new file +8
-0+ goto err; + } + + clks[i] = clk; + rpmh_clk->dev = &pdev->dev; + } + + ret = of_clk_add_provider(pdev->dev.of_node, of_clk_src_onecell_get,
Use devm please and register clk_hw pointers instead of clk pointers.
Quoted 4 lines
drivers/clk/qcom/clk-rpmh.c new file +4
-0+ data); +if (ret) { +dev_err (dev,"Failed to add clock provider\n" ); + goto err;
drivers/clk/qcom/clk-rpmh.c line 322
Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c new file +10
-0+ } + +dev_dbg (dev,"Registered RPMh clocks\n" ); + +return 0 ; +err: +if (rpmh_client) +rpmh_release (rpmh_client); +fail: +dev_err (dev,"Error registering RPMh Clock driver (%d)\n" , ret);
Please remove this error message.
Quoted 5 lines
drivers/clk/qcom/clk-rpmh.c new file +5
-0+return ret; +} + +static int clk_rpmh_remove (struct platform_device *pdev) +{
drivers/clk/qcom/clk-rpmh.c line 337
Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c new file +10
-0+const struct clk_rpmh_desc *desc = +of_device_get_match_data (&pdev->dev); +struct clk_hw **hw_clks = desc->clks; +struct clk_rpmh *rpmh_clk =to_clk_rpmh (hw_clks[0 ]); + +rpmh_release (rpmh_clk->rpmh_client); +of_clk_del_provider (pdev->dev.of_node); + +return 0 ; +}
Hopefully this whole remove function goes away.
Hello Taniya,
Quoted 10 lines · Stephen Boyd
Quoting Taniya Das (2018-04-13 19:36:41)
Quoted 5 lines
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 <[email protected]>Signed-off-by Amit Nischal <[email protected]>Your signoff chain is very confused. The first signoff should match the "From:" header but that doesn't seem to be the case here. And the sender should be the last in the chain.
It looks like the provenance of this driver go muddled during downstream propagation between branches. The original downstream version of the clk-rpmh driver was written by Osvaldo Banuelos [1]. I think that the best course of action here would be to remove my Signed-off-by, add Osvaldo's, and add yours.
Thanks,
David
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Thanks Stephen for the review comments.
Quoted 26 lines · Stephen Boyd
Quoting Taniya Das (2018-04-13 19:36:41)
Quoted 5 lines
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 <[email protected]>Signed-off-by Amit Nischal <[email protected]>Your signoff chain is very confused. The first signoff should match the "From:" header but that doesn't seem to be the case here. And the sender should be the last in the chain.
Quoted 15 lines
--- drivers/clk/qcom/Kconfig | 9 ++ drivers/clk/qcom/Makefile | 1 + drivers/clk/qcom/clk-rpmh.c | 367 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 377 insertions(+)create mode 100644
drivers/clk/qcom/clk-rpmh.cdrivers/clk/qcom/Kconfig +0
-0@@ -112,6 +112,15 @@ config IPQ_GCC_8074 i2c, USB, SD/eMMC, etc. Select this for the root clock of ipq8074.
Quoted 13 lines
drivers/clk/qcom/Kconfig config IPQ_GCC_8074 · line 122
Quoted 8 lines
drivers/clk/qcom/Kconfig +8
-0+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.Capitalize sdm?
Quoted 1 line
drivers/clk/qcom/Kconfig +1
-0+
Would use SDM845.
Quoted 2 lines
Can this
Kconfigbe put into alphabetical order in this file?
Would place it with the QCOM_* configs in this file.
Quoted 20 lines
Quoted 20 lines
drivers/clk/qcom/Kconfig +0
-0config MSM_GCC_8660 tristate "MSM8660 Global Clock Controller" depends on COMMON_CLK_QCOMdrivers/clk/qcom/Makefile +1
-0@@ -36,5 +36,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.odrivers/clk/qcom/clk-rpmh.c new file +0
-0@@ -0,0 +1,367 @@
Quoted 9 lines
drivers/clk/qcom/clk-rpmh.c line 6
Quoted 6 lines
drivers/clk/qcom/clk-rpmh.c new file +6
-0+// SPDX-License-Identifier: GPL-2.0 +/* +* Copyright (c) 2018, The Linux Foundation. All rights reserved. +*/ + +#include <linux/clk.h> Is this include used?
Would remove this include.
Quoted 13 lines
Quoted 13 lines
drivers/clk/qcom/clk-rpmh.c new file +13
-0+#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
Quoted 9 lines
drivers/clk/qcom/clk-rpmh.c line 25
Quoted 6 lines
drivers/clk/qcom/clk-rpmh.c new file +6
-0+#define CLK_RPMH_VRM_EN_OFFSET 4 +#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))Is there a reason these aren't inlined into the one place they're used?
Would clean the above.
Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c line 30
Quoted 5 lines
drivers/clk/qcom/clk-rpmh.c new file +5
-0+struct clk_rpmh { +struct clk_hw hw; +const char *res_name; +u32 res_addr; +u32 res_en_offset;Why do we store both
res_addrandres_en_offset? Can't we just storeres_en_offsetand then use that all the time? I don't see a user ofres_addranywhere.
The res_addr would be the address for the resource returned by the cmd_db_read_addr. And the res_en_offset would be the offsets of ARC_EN or VRM_EN.
Quoted 4 lines
drivers/clk/qcom/clk-rpmh.c line 32
Quoted 2 lines
drivers/clk/qcom/clk-rpmh.c new file +2
-0+u32 res_on_val; +u32 res_off_val;Is this used?
Yes the above are used.
Quoted 11 lines
drivers/clk/qcom/clk-rpmh.c line 41
Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c new file +9
-0+u32 state; +u32 aggr_state; +u32 last_sent_aggr_state; +u32 valid_state_mask; +struct rpmh_client *rpmh_client; +struct device *dev; +struct clk_rpmh *peer; +unsigned long rate; +};
Quoted 2 lines
Can you add some kernel-doc on these structure members?
Sure will add the same.
Quoted 8 lines
drivers/clk/qcom/clk-rpmh.c line 46
Quoted 5 lines
drivers/clk/qcom/clk-rpmh.c new file +5
-0+ +struct rpmh_cc { +struct clk_onecell_data data; +struct clk *clks[]; +};Hopefully this structure isn't needed.
Yes, would remove the above.
Quoted 7 lines
Quoted 6 lines
drivers/clk/qcom/clk-rpmh.c new file +6
-0+ +struct clk_rpmh_desc { +struct clk_hw **clks; +size_t num_clks; +}; +[...]
Quoted 8 lines
drivers/clk/qcom/clk-rpmh.c line 103
Quoted 5 lines
drivers/clk/qcom/clk-rpmh.c new file +5
-0+ +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)));Too many parenthesis here.
Would clean it up.
Quoted 34 lines
Quoted 34 lines
drivers/clk/qcom/clk-rpmh.c new file +34
-0+} + +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 =0 ; + + cmd.addr = c->res_addr + c->res_en_offset; + cmd_state = c->aggr_state; + on_val = c->res_on_val; + +for (; state <=RPMH_ACTIVE_ONLY_STATE ; state++) { +if (has_state_changed (c, state)) { + cmd.data = (cmd_state &BIT (state)) ? on_val :0 ; + ret =rpmh_write_async (c->rpmh_client, state, + &cmd,1 ); +if (ret) { +dev_err (c->dev,"set %s state of %s failed: (%d)\n" , + (!state) ?"sleep" : + (state ==RPMH_WAKE_ONLY_STATE ) ? +"wake" :"active" , c->res_name, ret); +return ret; + } + } + } + + c->last_sent_aggr_state = c->aggr_state; + c->peer->last_sent_aggr_state = c->last_sent_aggr_state; + +return 0 ; +} +
Quoted 9 lines
drivers/clk/qcom/clk-rpmh.c line 143
Quoted 6 lines
drivers/clk/qcom/clk-rpmh.c new file +6
-0+static int clk_rpmh_aggregate_state_send_command (struct clk_rpmh *c, +bool enable) +{ +int ret; + +/* Update state and aggregate state values based on enable value. */ Maybe this comment can go above the function itself.
Sure would move it above the function.
Quoted 29 lines
Quoted 16 lines
drivers/clk/qcom/clk-rpmh.c new file +16
-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 && enable) { + c->state =0 ; + }else if (ret) { + 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)drivers/clk/qcom/clk-rpmh.c line 169
Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c new file +10
-0+{ +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);Rewrite this as:
Quoted 5 lines
if (!c->state) ret = clk_rpmh_aggregate_state_send_command(c, true);and drop the goto.
Would cleanup to remove goto.
Quoted 29 lines
Quoted 6 lines
drivers/clk/qcom/clk-rpmh.c new file +6
-0+out: +mutex_unlock (&rpmh_clk_lock); +return ret; +}; + +static void clk_rpmh_unprepare (struct clk_hw *hw)drivers/clk/qcom/clk-rpmh.c line 185
Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c new file +10
-0+{ +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:Same comment.
Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c new file +10
-0+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); + +/*
Quoted 9 lines
drivers/clk/qcom/clk-rpmh.c line 199
Quoted 4 lines
drivers/clk/qcom/clk-rpmh.c new file +4
-0+* RPMh clocks have a fixed rate. Return static rate set +* at init time. +*/ +return r->rate;The rate should come from the parent. In the case of tcxo it would be
board_xoclk rate (or maybe some fixed div-2 on the board XO that's also defined in DT because theboard_xoseems to be two times 19.2 MHz?).
There would not be any parent for the RPMH clock, they would be the parent for other clocks.
The TCXO is 19.2MHz and once we have the RPMH clocks, we would remove the DT reference for board_xo.
Quoted 51 lines
Quoted 51 lines
drivers/clk/qcom/clk-rpmh.c new file +51
-0+} + +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 ); +DEFINE_CLK_RPMH_VRM (sdm845, ln_bb_clk2, ln_bb_clk2_ao,"lnbclka2" ,19200000 ); +DEFINE_CLK_RPMH_VRM (sdm845, ln_bb_clk3, ln_bb_clk3_ao,"lnbclka3" ,19200000 ); +DEFINE_CLK_RPMH_VRM (sdm845, rf_clk1, rf_clk1_ao,"rfclka1" ,38400000 ); +DEFINE_CLK_RPMH_VRM (sdm845, rf_clk2, rf_clk2_ao,"rfclka2" ,38400000 ); +DEFINE_CLK_RPMH_VRM (sdm845, rf_clk3, rf_clk3_ao,"rfclka3" ,38400000 ); + +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 int clk_rpmh_probe (struct platform_device *pdev) +{ +struct clk **clks; +struct clk *clk; +struct rpmh_cc *rcc; +struct clk_onecell_data *data; +struct clk_hw **hw_clks; +struct clk_rpmh *rpmh_clk; +const struct clk_rpmh_desc *desc; +struct rpmh_client *rpmh_client; +struct device *dev; +size_t num_clks, i; +int ret; + + dev = &pdev->dev;
Quoted 9 lines
drivers/clk/qcom/clk-rpmh.c line 256
Quoted 6 lines
drivers/clk/qcom/clk-rpmh.c new file +6
-0+ + ret =cmd_db_ready (); +if (ret) { +if (ret != -EPROBE_DEFER ) { +dev_err (dev,"Command DB not available (%d)\n" , ret); + goto fail;Please just return error here instead of goto.
Sure would clean this.
Quoted 49 lines
drivers/clk/qcom/clk-rpmh.c line 266
Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c new file +10
-0+ } +return ret; + } + + rpmh_client =rpmh_get_client (pdev); +if (IS_ERR (rpmh_client)) { + ret =PTR_ERR (rpmh_client); +dev_err (dev,"Failed to request RPMh client, ret=%d\n" , ret); +return ret; + }I hope we get rid of
rpmh_get_client()still.Quoted 24 lines
Quoted 1 line
+
drivers/clk/qcom/clk-rpmh.c new file +23
-0+ desc =of_device_get_match_data (&pdev->dev); + 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 (dev,"missing RPMh resource address for %s\n" , + rpmh_clk->res_name); + ret = -ENODEV ;drivers/clk/qcom/clk-rpmh.c line 300
Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c new file +10
-0+ 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); +dev_err (dev,"failed to register %s\n" , + hw_clks[i]->init->name);This isn't a useful error message either.
Quoted 11 lines
drivers/clk/qcom/clk-rpmh.c line 308
Quoted 8 lines
drivers/clk/qcom/clk-rpmh.c new file +8
-0+ goto err; + } + + clks[i] = clk; + rpmh_clk->dev = &pdev->dev; + } + + ret = of_clk_add_provider(pdev->dev.of_node, of_clk_src_onecell_get,Use devm please and register
clk_hwpointers instead of clk pointers.
Sure would move to use devm & would register clk_hw.
Quoted 8 lines
Quoted 8 lines
drivers/clk/qcom/clk-rpmh.c new file +8
-0+ data); +if (ret) { +dev_err (dev,"Failed to add clock provider\n" ); + goto err; + } + +dev_dbg (dev,"Registered RPMh clocks\n" ); +
Quoted 9 lines
drivers/clk/qcom/clk-rpmh.c line 322
Quoted 6 lines
drivers/clk/qcom/clk-rpmh.c new file +6
-0+return 0 ; +err: +if (rpmh_client) +rpmh_release (rpmh_client); +fail: +dev_err (dev,"Error registering RPMh Clock driver (%d)\n" , ret);Please remove this error message.
Thanks would remove the above.
Quoted 18 lines
Quoted 5 lines
drivers/clk/qcom/clk-rpmh.c new file +5
-0+return ret; +} + +static int clk_rpmh_remove (struct platform_device *pdev) +{drivers/clk/qcom/clk-rpmh.c line 337
Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c new file +10
-0+const struct clk_rpmh_desc *desc = +of_device_get_match_data (&pdev->dev); +struct clk_hw **hw_clks = desc->clks; +struct clk_rpmh *rpmh_clk =to_clk_rpmh (hw_clks[0 ]); + +rpmh_release (rpmh_clk->rpmh_client); +of_clk_del_provider (pdev->dev.of_node); + +return 0 ; +}Hopefully this whole remove function goes away.
We would still require therpmh_releasecode.
Signature · 4 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation. --
Stephen Boyd
maintainerQuoting Taniya Das (2018-04-23 09:50:22)
Quoted 9 lines · Stephen Boyd
Quoted 8 lines
Quoting Taniya Das (2018-04-13 19:36:41)
Quoted 5 lines
drivers/clk/qcom/clk-rpmh.c new file +5
-0+struct clk_rpmh { +struct clk_hw hw; +const char *res_name; +u32 res_addr; +u32 res_en_offset;
Quoted 8 lines
Quoted 4 lines
Why do we store both
res_addrandres_en_offset? Can't we just storeres_en_offsetand then use that all the time? I don't see a user ofres_addranywhere.The
res_addrwould be the address for the resource returned by thecmd_db_read_addr. And theres_en_offsetwould be the offsets ofARC_ENorVRM_EN.
Yes. But why can't we store the combination of the two?
Quoted 7 lines
Quoted 5 lines
drivers/clk/qcom/clk-rpmh.c line 32
Quoted 3 lines
drivers/clk/qcom/clk-rpmh.c new file +2
-0+u32 res_on_val; +u32 res_off_val;Is this used?
Yes the above are used.
I just meant res_off_val. Which looks unused.
Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c new file +9
-0+u32 state; +u32 aggr_state; +u32 last_sent_aggr_state; +u32 valid_state_mask; +struct rpmh_client *rpmh_client; +struct device *dev; +struct clk_rpmh *peer; +unsigned long rate; +};
Quoted 3 lines
Quoted 2 lines
Can you add some kernel-doc on these structure members?
Sure will add the same.
Great! Hopefully that clarifies things.
Quoted 11 lines
Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c line 199
Quoted 5 lines
drivers/clk/qcom/clk-rpmh.c new file +5
-0+/* +* RPMh clocks have a fixed rate. Return static rate set +* at init time. +*/ +return r->rate;The rate should come from the parent. In the case of tcxo it would be
board_xoclk rate (or maybe some fixed div-2 on the board XO that's also defined in DT because theboard_xoseems to be two times 19.2 MHz?).
Quoted 5 lines
There would not be any parent for the RPMH clock, they would be the parent for other clocks.
The TCXO is 19.2MHz and once we have the RPMH clocks, we would remove the DT reference for
board_xo.
No that's wrong. There is a parent of the RPMh clks, and that's the board XO clk in the DT file. We will never remove the board clks.
Quoted 91 lines · 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 David Collins <[email protected]>Signed-off-by Amit Nischal <[email protected]>--- drivers/clk/qcom/Kconfig | 9 ++ drivers/clk/qcom/Makefile | 1 + drivers/clk/qcom/clk-rpmh.c | 367 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 377 insertions(+) create mode 100644 drivers/clk/qcom/clk-rpmh.c--
drivers/clk/qcom/Kconfig +9
-0@@ -112,6 +112,15 @@ config IPQ_GCC_8074 i2c, USB, SD/eMMC, etc. Select this for the root clock of ipq8074. +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 MSM_GCC_8660 tristate "MSM8660 Global Clock Controller" depends on COMMON_CLK_QCOMdrivers/clk/qcom/Makefile +1
-0@@ -36,5 +36,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.odrivers/clk/qcom/clk-rpmh.c new file +41
-0@@ -0,0 +1,367 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Copyright (c) 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 <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 +#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; +u32 state; +u32 aggr_state; +u32 last_sent_aggr_state; +u32 valid_state_mask; +struct rpmh_client *rpmh_client; +struct device *dev; +struct clk_rpmh *peer; +unsigned long rate; +};
drivers/clk/qcom/clk-rpmh.c line 51
Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c new file +10
-0+ +struct rpmh_cc { +struct clk_onecell_data data; +struct clk *clks[]; +}; + +struct clk_rpmh_desc { +struct clk_hw **clks; +size_t num_clks; +};
Could you briefly document the struct members, especially struct clk_rpmh?
Quoted 49 lines
drivers/clk/qcom/clk-rpmh.c new file +49
-0+static DEFINE_MUTEX (rpmh_clk_lock); + +#define __DEFINE_CLK_RPMH (_platform, _name, _name_active, _res_name, \ + _res_en_offset, _res_on, _res_off, _rate) \ + 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 = 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 = CLK_RPMH_APPS_RSC_AO_STATE_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) \ + __DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name, \ + CLK_RPMH_ARC_EN_OFFSET, _res_on, _res_off, _rate) + +#define DEFINE_CLK_RPMH_VRM (_platform, _name, _name_active, _res_name, \ + _rate) \ + __DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name, \ + CLK_RPMH_VRM_EN_OFFSET, true, false, _rate) + +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) +{
drivers/clk/qcom/clk-rpmh.c line 111
Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c new file +10
-0+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 =0 ;
nit: initialization of 'ret' is not needed.
drivers/clk/qcom/clk-rpmh.c line 117
Quoted 6 lines
drivers/clk/qcom/clk-rpmh.c new file +6
-0+ + cmd.addr = c->res_addr + c->res_en_offset; + cmd_state = c->aggr_state; + on_val = c->res_on_val; + +for (; state <=RPMH_ACTIVE_ONLY_STATE ; state++) {
It would be clearer to initialize 'state' here.
drivers/clk/qcom/clk-rpmh.c line 119
Quoted 2 lines
drivers/clk/qcom/clk-rpmh.c new file +2
-0+if (has_state_changed (c, state)) { + cmd.data = (cmd_state &BIT (state)) ? on_val :0 ;
~
Shouldn't this be c->res_off_val?
Quoted 40 lines
drivers/clk/qcom/clk-rpmh.c new file +40
-0+ ret =rpmh_write_async (c->rpmh_client, state, + &cmd,1 ); +if (ret) { +dev_err (c->dev,"set %s state of %s failed: (%d)\n" , + (!state) ?"sleep" : + (state ==RPMH_WAKE_ONLY_STATE ) ? +"wake" :"active" , c->res_name, ret); +return ret; + } + } + } + + c->last_sent_aggr_state = c->aggr_state; + c->peer->last_sent_aggr_state = c->last_sent_aggr_state; + +return 0 ; +} + +static int clk_rpmh_aggregate_state_send_command (struct clk_rpmh *c, +bool enable) +{ +int ret; + +/* 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); +if (ret && enable) { + c->state =0 ; + }else if (ret) { + 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)
drivers/clk/qcom/clk-rpmh.c line 169
Quoted 10 lines
drivers/clk/qcom/clk-rpmh.c new file +10
-0+{ +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);
This ends up calling rpmh_write_async(), hence the operation might still be in flight when the function returns. Is this intended?
Quoted 198 lines
drivers/clk/qcom/clk-rpmh.c new file +198
-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); + +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 ); +DEFINE_CLK_RPMH_VRM (sdm845, ln_bb_clk2, ln_bb_clk2_ao,"lnbclka2" ,19200000 ); +DEFINE_CLK_RPMH_VRM (sdm845, ln_bb_clk3, ln_bb_clk3_ao,"lnbclka3" ,19200000 ); +DEFINE_CLK_RPMH_VRM (sdm845, rf_clk1, rf_clk1_ao,"rfclka1" ,38400000 ); +DEFINE_CLK_RPMH_VRM (sdm845, rf_clk2, rf_clk2_ao,"rfclka2" ,38400000 ); +DEFINE_CLK_RPMH_VRM (sdm845, rf_clk3, rf_clk3_ao,"rfclka3" ,38400000 ); + +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 int clk_rpmh_probe (struct platform_device *pdev) +{ +struct clk **clks; +struct clk *clk; +struct rpmh_cc *rcc; +struct clk_onecell_data *data; +struct clk_hw **hw_clks; +struct clk_rpmh *rpmh_clk; +const struct clk_rpmh_desc *desc; +struct rpmh_client *rpmh_client; +struct device *dev; +size_t num_clks, i; +int ret; + + dev = &pdev->dev; + + ret =cmd_db_ready (); +if (ret) { +if (ret != -EPROBE_DEFER ) { +dev_err (dev,"Command DB not available (%d)\n" , ret); + goto fail; + } +return ret; + } + + rpmh_client =rpmh_get_client (pdev); +if (IS_ERR (rpmh_client)) { + ret =PTR_ERR (rpmh_client); +dev_err (dev,"Failed to request RPMh client, ret=%d\n" , ret); +return ret; + } + + desc =of_device_get_match_data (&pdev->dev); + 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 (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); +dev_err (dev,"failed to register %s\n" , + hw_clks[i]->init->name); + goto err; + } + + clks[i] = clk; + rpmh_clk->dev = &pdev->dev; + } + + ret =of_clk_add_provider (pdev->dev.of_node, of_clk_src_onecell_get, + data); +if (ret) { +dev_err (dev,"Failed to add clock provider\n" ); + goto err; + } + +dev_dbg (dev,"Registered RPMh clocks\n" ); + +return 0 ; +err: +if (rpmh_client) +rpmh_release (rpmh_client); +fail: +dev_err (dev,"Error registering RPMh Clock driver (%d)\n" , ret); +return ret; +} + +static int clk_rpmh_remove (struct platform_device *pdev) +{ +const struct clk_rpmh_desc *desc = +of_device_get_match_data (&pdev->dev); +struct clk_hw **hw_clks = desc->clks; +struct clk_rpmh *rpmh_clk =to_clk_rpmh (hw_clks[0 ]); + +rpmh_release (rpmh_clk->rpmh_client); +of_clk_del_provider (pdev->dev.of_node); + +return 0 ; +} + +static const struct of_device_id clk_rpmh_match_table[] = { + { .compatible ="qcom,sdm845-rpmh-clk" , .data = &clk_rpmh_sdm845}, + { } +}; +MODULE_DEVICE_TABLE (of, clk_rpmh_match_table); + +static struct platform_driver clk_rpmh_driver = { + .probe = clk_rpmh_probe, + .remove = clk_rpmh_remove, + .driver = { + .name ="clk-rpmh" , + .of_match_table = clk_rpmh_match_table, + }, +}; + +static int __init clk_rpmh_init (void ) +{ +return platform_driver_register (&clk_rpmh_driver); +} +subsys_initcall (clk_rpmh_init); + +static void __exit clk_rpmh_exit(void) +{ +platform_driver_unregister (&clk_rpmh_driver); +} +module_exit (clk_rpmh_exit); + +MODULE_DESCRIPTION ("QCOM RPMh Clock Driver" ); +MODULE_LICENSE ("GPL v2" );