* [v5 0/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver
@ 2018-05-01 8:41 Taniya Das
2018-05-01 8:41 ` [v5 1/2] dt-bindings: clock: Introduce QCOM RPMh clock bindings Taniya Das
2018-05-01 8:41 ` [v5 2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver Taniya Das
0 siblings, 2 replies; 12+ messages in thread
From: Taniya Das @ 2018-05-01 8:41 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,
devicetree, Taniya Das
[v5]
* Addressed comments from Stephen
* Introduced a new DT property to take clock divs
[v4]
* Addressed comments from Stephen
[v3]
* Addressed documentation & code review comments from Bjorn
* Addressed bindings comments from Rob
* Updated the patch series order for bindings
[v2]
* Addressed comments from Stephen
* Addressed comments from Evan
This patch series adds a driver and device tree documentation binding
for the clock control via Resource Power Manager-hardened (RPMh) on some
Qualcomm Technologies, Inc, SoCs such as SDM845. The clock RPMh driver
would send requests for the RPMh managed clock resources.
The RPMh clock driver depends upon the RPMh driver [1] and command DB
driver [2] which are both still undergoing review.
Thanks,
Taniya
[1]: https://lkml.org/lkml/2018/3/9/979
[2]: https://lkml.org/lkml/2018/3/14/787
Taniya Das (2):
dt-bindings: clock: Introduce QCOM RPMh clock bindings
clk: qcom: clk-rpmh: Add QCOM RPMh clock driver
.../devicetree/bindings/clock/qcom,rpmh-clk.txt | 32 ++
drivers/clk/qcom/Kconfig | 9 +
drivers/clk/qcom/Makefile | 1 +
drivers/clk/qcom/clk-rpmh.c | 387 +++++++++++++++++++++
include/dt-bindings/clock/qcom,rpmh.h | 22 ++
5 files changed, 451 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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [v5 1/2] dt-bindings: clock: Introduce QCOM RPMh clock bindings
2018-05-01 8:41 [v5 0/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver Taniya Das
@ 2018-05-01 8:41 ` Taniya Das
2018-05-01 21:10 ` Stephen Boyd
2018-05-01 8:41 ` [v5 2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver Taniya Das
1 sibling, 1 reply; 12+ messages in thread
From: Taniya Das @ 2018-05-01 8:41 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,
devicetree, Taniya Das
Add RPMh clock device bindings for Qualcomm Technology Inc's SoCs. These
devices would be used for communicating resource state requests to control
the clocks managed by RPMh.
Signed-off-by: Amit Nischal <anischal@codeaurora.org>
Signed-off-by: Taniya Das <tdas@codeaurora.org>
Reviewed-by: Rob Herring <robh@kernel.org>
---
.../devicetree/bindings/clock/qcom,rpmh-clk.txt | 32 ++++++++++++++++++++++
include/dt-bindings/clock/qcom,rpmh.h | 24 ++++++++++++++++
2 files changed, 56 insertions(+)
create mode 100644 Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt
create mode 100644 include/dt-bindings/clock/qcom,rpmh.h
diff --git a/Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt b/Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt
new file mode 100644
index 0000000..ecc1dbe
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt
@@ -0,0 +1,32 @@
+Qualcomm Technologies, Inc. RPMh Clocks
+-------------------------------------------------------
+
+Resource Power Manager Hardened (RPMh) manages shared resources on
+some Qualcomm Technologies Inc. SoCs. It accepts clock requests from
+other hardware subsystems via RSC to control clocks.
+
+Required properties :
+- compatible : shall contain "qcom,sdm845-rpmh-clk"
+
+- #clock-cells : must contain 1
+
+Optional properties :
+- assigned-clk-divs : property should contain a list of divs for each clock in
+ the clk-output-names property. In case divs are not
+ provided the clock rate would be same as parent rate.
+- clk-output-names : a list of strings of clock output signal for the divs to
+ be applied.
+
+Example :
+
+#include <dt-bindings/clock/qcom,rpmh.h>
+
+ &apps_rsc {
+ rpmhcc: clock-controller {
+ compatible = "qcom,sdm845-rpmh-clk";
+ #clock-cells = <1>;
+ assigned-clk-divs = <2 2 2>;
+ clk-output-names = "bi_tcxo", "lnbb_clk2",
+ "lnbb_clk3";
+ };
+ };
diff --git a/include/dt-bindings/clock/qcom,rpmh.h b/include/dt-bindings/clock/qcom,rpmh.h
new file mode 100644
index 0000000..36caab2
--- /dev/null
+++ b/include/dt-bindings/clock/qcom,rpmh.h
@@ -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.
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [v5 2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver
2018-05-01 8:41 [v5 0/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver Taniya Das
2018-05-01 8:41 ` [v5 1/2] dt-bindings: clock: Introduce QCOM RPMh clock bindings Taniya Das
@ 2018-05-01 8:41 ` Taniya Das
2018-05-01 12:43 ` Rob Herring
2018-05-01 21:27 ` Stephen Boyd
1 sibling, 2 replies; 12+ messages in thread
From: Taniya Das @ 2018-05-01 8:41 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,
devicetree, 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: 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 | 387 ++++++++++++++++++++++++++++++++++
include/dt-bindings/clock/qcom,rpmh.h | 2 -
4 files changed, 397 insertions(+), 2 deletions(-)
create mode 100644 drivers/clk/qcom/clk-rpmh.c
diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index e42e1af..627e6e1 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -59,6 +59,15 @@ config QCOM_CLK_SMD_RPM
Say Y if you want to support the clocks exposed by the RPM on
platforms such as apq8016, apq8084, msm8974 etc.
+config QCOM_CLK_RPMH
+ tristate "RPMh Clock Driver"
+ depends on COMMON_CLK_QCOM && QCOM_RPMH
+ help
+ RPMh manages shared resources on some Qualcomm Technologies, Inc.
+ SoCs. It accepts requests from other hardware subsystems via RSC.
+ Say Y if you want to support the clocks exposed by RPMh on
+ platforms such as SDM845.
+
config APQ_GCC_8084
tristate "APQ8084 Global Clock Controller"
select QCOM_GDSC
diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
index 7c09ab1..246a721 100644
--- a/drivers/clk/qcom/Makefile
+++ b/drivers/clk/qcom/Makefile
@@ -37,5 +37,6 @@ obj-$(CONFIG_MSM_MMCC_8996) += mmcc-msm8996.o
obj-$(CONFIG_QCOM_A53PLL) += a53-pll.o
obj-$(CONFIG_QCOM_CLK_APCS_MSM8916) += apcs-msm8916.o
obj-$(CONFIG_QCOM_CLK_RPM) += clk-rpm.o
+obj-$(CONFIG_QCOM_CLK_RPMH) += clk-rpmh.o
obj-$(CONFIG_QCOM_CLK_SMD_RPM) += clk-smd-rpm.o
obj-$(CONFIG_SPMI_PMIC_CLKDIV) += clk-spmi-pmic-div.o
diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
new file mode 100644
index 0000000..9cb7aa4
--- /dev/null
+++ b/drivers/clk/qcom/clk-rpmh.c
@@ -0,0 +1,387 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <soc/qcom/cmd-db.h>
+#include <soc/qcom/rpmh.h>
+
+#include <dt-bindings/clock/qcom,rpmh.h>
+
+#define CLK_RPMH_ARC_EN_OFFSET 0
+#define CLK_RPMH_VRM_EN_OFFSET 4
+
+/**
+ * struct clk_rpmh - individual rpmh clock data structure
+ * @hw: handle between common and hardware-specific interfaces
+ * @res_name: resource name for the rpmh clock
+ * @div: clock divider to compute the clock rate
+ * @res_addr: base address of the rpmh resource within the RPMh
+ * @res_on_val: rpmh clock enable value
+ * @res_off_val: rpmh clock disable value
+ * @state: rpmh clock requested state
+ * @aggr_state: rpmh clock aggregated state
+ * @last_sent_aggr_state: rpmh clock last aggr state sent to RPMh
+ * @valid_state_mask: mask to determine the state of the rpmh clock
+ * @rpmh_client: handle used for rpmh communications
+ * @dev: device to which it is attached
+ * @peer: pointer to the clock rpmh sibling
+ */
+struct clk_rpmh {
+ struct clk_hw hw;
+ const char *res_name;
+ u8 div;
+ u32 res_addr;
+ u32 res_on_val;
+ u32 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;
+};
+
+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) \
+ static struct clk_rpmh _platform##_##_name_active; \
+ static struct clk_rpmh _platform##_##_name = { \
+ .res_name = _res_name, \
+ .res_addr = _res_en_offset, \
+ .res_on_val = _res_on, \
+ .res_off_val = _res_off, \
+ .peer = &_platform##_##_name_active, \
+ .valid_state_mask = (BIT(RPMH_WAKE_ONLY_STATE) | \
+ BIT(RPMH_ACTIVE_ONLY_STATE) | \
+ BIT(RPMH_SLEEP_STATE)), \
+ .hw.init = &(struct clk_init_data){ \
+ .ops = &clk_rpmh_ops, \
+ .name = #_name, \
+ .parent_names = (const char *[]){ "xo_board" }, \
+ .num_parents = 1, \
+ }, \
+ }; \
+ static struct clk_rpmh _platform##_##_name_active = { \
+ .res_name = _res_name, \
+ .res_addr = _res_en_offset, \
+ .res_on_val = _res_on, \
+ .res_off_val = _res_off, \
+ .peer = &_platform##_##_name, \
+ .valid_state_mask = (BIT(RPMH_WAKE_ONLY_STATE) | \
+ BIT(RPMH_ACTIVE_ONLY_STATE)), \
+ .hw.init = &(struct clk_init_data){ \
+ .ops = &clk_rpmh_ops, \
+ .name = #_name_active, \
+ .parent_names = (const char *[]){ "xo_board" }, \
+ .num_parents = 1, \
+ }, \
+ }
+
+#define DEFINE_CLK_RPMH_ARC(_platform, _name, _name_active, _res_name, \
+ _res_on, _res_off) \
+ __DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name, \
+ CLK_RPMH_ARC_EN_OFFSET, _res_on, _res_off)
+
+#define DEFINE_CLK_RPMH_VRM(_platform, _name, _name_active, _res_name) \
+ __DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name, \
+ CLK_RPMH_VRM_EN_OFFSET, true, false)
+
+static inline struct clk_rpmh *to_clk_rpmh(struct clk_hw *_hw)
+{
+ return container_of(_hw, struct clk_rpmh, hw);
+}
+
+static inline bool has_state_changed(struct clk_rpmh *c, u32 state)
+{
+ return (c->last_sent_aggr_state & BIT(state))
+ != (c->aggr_state & BIT(state));
+}
+
+static int clk_rpmh_send_aggregate_command(struct clk_rpmh *c)
+{
+ struct tcs_cmd cmd = { 0 };
+ u32 cmd_state, on_val;
+ enum rpmh_state state = RPMH_SLEEP_STATE;
+ int ret;
+
+ cmd.addr = c->res_addr;
+ cmd_state = c->aggr_state;
+ on_val = c->res_on_val;
+
+ for (; state <= RPMH_ACTIVE_ONLY_STATE; state++) {
+ if (has_state_changed(c, state)) {
+ 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;
+}
+
+/*
+ * Update state and aggregate state values based on enable value.
+ */
+static int clk_rpmh_aggregate_state_send_command(struct clk_rpmh *c,
+ bool enable)
+{
+ int ret;
+
+ 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)
+{
+ struct clk_rpmh *c = to_clk_rpmh(hw);
+ int ret = 0;
+
+ mutex_lock(&rpmh_clk_lock);
+
+ if (!c->state)
+ ret = clk_rpmh_aggregate_state_send_command(c, true);
+
+ mutex_unlock(&rpmh_clk_lock);
+ return ret;
+};
+
+static void clk_rpmh_unprepare(struct clk_hw *hw)
+{
+ struct clk_rpmh *c = to_clk_rpmh(hw);
+
+ mutex_lock(&rpmh_clk_lock);
+
+ if (c->state)
+ clk_rpmh_aggregate_state_send_command(c, false);
+
+ mutex_unlock(&rpmh_clk_lock);
+};
+
+static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw,
+ unsigned long prate)
+{
+ struct clk_rpmh *r = to_clk_rpmh(hw);
+ unsigned long rate;
+
+ /*
+ * RPMh clocks have a fixed rate.
+ */
+ rate = prate;
+ do_div(rate, r->div);
+
+ return rate;
+}
+
+static const struct clk_ops clk_rpmh_ops = {
+ .prepare = clk_rpmh_prepare,
+ .unprepare = clk_rpmh_unprepare,
+ .recalc_rate = clk_rpmh_recalc_rate,
+};
+
+/* Resource name must match resource id present in cmd-db. */
+DEFINE_CLK_RPMH_ARC(sdm845, bi_tcxo, bi_tcxo_ao, "xo.lvl", 0x3, 0x0);
+DEFINE_CLK_RPMH_VRM(sdm845, ln_bb_clk2, ln_bb_clk2_ao, "lnbclka2");
+DEFINE_CLK_RPMH_VRM(sdm845, ln_bb_clk3, ln_bb_clk3_ao, "lnbclka3");
+DEFINE_CLK_RPMH_VRM(sdm845, rf_clk1, rf_clk1_ao, "rfclka1");
+DEFINE_CLK_RPMH_VRM(sdm845, rf_clk2, rf_clk2_ao, "rfclka2");
+DEFINE_CLK_RPMH_VRM(sdm845, rf_clk3, rf_clk3_ao, "rfclka3");
+
+static struct clk_hw *sdm845_rpmh_clocks[] = {
+ [RPMH_CXO_CLK] = &sdm845_bi_tcxo.hw,
+ [RPMH_CXO_CLK_A] = &sdm845_bi_tcxo_ao.hw,
+ [RPMH_LN_BB_CLK2] = &sdm845_ln_bb_clk2.hw,
+ [RPMH_LN_BB_CLK2_A] = &sdm845_ln_bb_clk2_ao.hw,
+ [RPMH_LN_BB_CLK3] = &sdm845_ln_bb_clk3.hw,
+ [RPMH_LN_BB_CLK3_A] = &sdm845_ln_bb_clk3_ao.hw,
+ [RPMH_RF_CLK1] = &sdm845_rf_clk1.hw,
+ [RPMH_RF_CLK1_A] = &sdm845_rf_clk1_ao.hw,
+ [RPMH_RF_CLK2] = &sdm845_rf_clk2.hw,
+ [RPMH_RF_CLK2_A] = &sdm845_rf_clk2_ao.hw,
+ [RPMH_RF_CLK3] = &sdm845_rf_clk3.hw,
+ [RPMH_RF_CLK3_A] = &sdm845_rf_clk3_ao.hw,
+};
+
+static const struct clk_rpmh_desc clk_rpmh_sdm845 = {
+ .clks = sdm845_rpmh_clocks,
+ .num_clks = ARRAY_SIZE(sdm845_rpmh_clocks),
+};
+
+static int clk_rpmh_probe(struct platform_device *pdev)
+{
+ struct clk *clk;
+ struct clk_hw_onecell_data *hw_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);
+ 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;
+
+ hw_data = devm_kzalloc(dev,
+ sizeof(*hw_data) + num_clks * sizeof(struct clk_hw *),
+ GFP_KERNEL);
+ if (!hw_data)
+ return -ENOMEM;
+
+ hw_data->num = num_clks;
+
+ for (i = 0; i < num_clks; i++) {
+ const char *clk_name;
+ struct property *prop;
+ const __be32 *p;
+ u32 res_addr;
+ int div, j = 0;
+
+ rpmh_clk = to_clk_rpmh(hw_clks[i]);
+ res_addr = cmd_db_read_addr(rpmh_clk->res_name);
+ if (!res_addr) {
+ dev_err(dev, "missing RPMh resource address for %s\n",
+ rpmh_clk->res_name);
+ ret = -ENODEV;
+ goto err;
+ }
+ rpmh_clk->res_addr += res_addr;
+
+ rpmh_clk->rpmh_client = rpmh_client;
+
+ /* default div for all clocks */
+ if (!rpmh_clk->div)
+ rpmh_clk->div = 1;
+
+ of_property_for_each_u32(pdev->dev.of_node, "assigned-clk-divs",
+ prop, p, div) {
+ of_property_read_string_index(pdev->dev.of_node,
+ "clk-output-names", j, &clk_name);
+ if (!strcmp(clk_name, hw_clks[i]->init->name)) {
+ rpmh_clk->div = div;
+ rpmh_clk->peer->div = div;
+ }
+ j++;
+ }
+
+ 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;
+ }
+
+ rpmh_clk->dev = &pdev->dev;
+ }
+
+ ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
+ hw_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);
+
+ return ret;
+}
+
+static int clk_rpmh_remove(struct platform_device *pdev)
+{
+ const struct clk_rpmh_desc *desc =
+ of_device_get_match_data(&pdev->dev);
+ struct clk_hw **hw_clks = desc->clks;
+ struct clk_rpmh *rpmh_clk = to_clk_rpmh(hw_clks[0]);
+
+ rpmh_release(rpmh_clk->rpmh_client);
+
+ return 0;
+}
+
+static const struct of_device_id clk_rpmh_match_table[] = {
+ { .compatible = "qcom,sdm845-rpmh-clk", .data = &clk_rpmh_sdm845},
+ { }
+};
+MODULE_DEVICE_TABLE(of, clk_rpmh_match_table);
+
+static struct platform_driver clk_rpmh_driver = {
+ .probe = clk_rpmh_probe,
+ .remove = clk_rpmh_remove,
+ .driver = {
+ .name = "clk-rpmh",
+ .of_match_table = clk_rpmh_match_table,
+ },
+};
+
+static int __init clk_rpmh_init(void)
+{
+ return platform_driver_register(&clk_rpmh_driver);
+}
+subsys_initcall(clk_rpmh_init);
+
+static void __exit clk_rpmh_exit(void)
+{
+ platform_driver_unregister(&clk_rpmh_driver);
+}
+module_exit(clk_rpmh_exit);
+
+MODULE_DESCRIPTION("QCOM RPMh Clock Driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/dt-bindings/clock/qcom,rpmh.h b/include/dt-bindings/clock/qcom,rpmh.h
index 36caab2..f48fbd6 100644
--- a/include/dt-bindings/clock/qcom,rpmh.h
+++ b/include/dt-bindings/clock/qcom,rpmh.h
@@ -18,7 +18,5 @@
#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] 12+ messages in thread
* Re: [v5 2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver
2018-05-01 8:41 ` [v5 2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver Taniya Das
@ 2018-05-01 12:43 ` Rob Herring
2018-05-01 15:33 ` Taniya Das
2018-05-01 21:27 ` Stephen Boyd
1 sibling, 1 reply; 12+ messages in thread
From: Rob Herring @ 2018-05-01 12:43 UTC (permalink / raw)
To: Taniya Das
Cc: Stephen Boyd, Michael Turquette, Andy Gross, David Brown,
Rajendra Nayak, Odelu Kukatla, Amit Nischal, linux-arm-msm,
linux-soc, linux-clk, linux-kernel, devicetree
On Tue, May 01, 2018 at 02:11:33PM +0530, Taniya Das wrote:
> Add the RPMh clock driver to control the RPMh managed clock resources on
> some of the Qualcomm Technologies, Inc. SoCs.
>
> 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 | 387 ++++++++++++++++++++++++++++++++++
> include/dt-bindings/clock/qcom,rpmh.h | 2 -
> 4 files changed, 397 insertions(+), 2 deletions(-)
> create mode 100644 drivers/clk/qcom/clk-rpmh.c
> diff --git a/include/dt-bindings/clock/qcom,rpmh.h b/include/dt-bindings/clock/qcom,rpmh.h
> index 36caab2..f48fbd6 100644
> --- a/include/dt-bindings/clock/qcom,rpmh.h
> +++ b/include/dt-bindings/clock/qcom,rpmh.h
> @@ -18,7 +18,5 @@
> #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
Did you mean to have this or squash into previous patch?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [v5 2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver
2018-05-01 12:43 ` Rob Herring
@ 2018-05-01 15:33 ` Taniya Das
0 siblings, 0 replies; 12+ messages in thread
From: Taniya Das @ 2018-05-01 15:33 UTC (permalink / raw)
To: Rob Herring
Cc: Stephen Boyd, Michael Turquette, Andy Gross, David Brown,
Rajendra Nayak, Odelu Kukatla, Amit Nischal, linux-arm-msm,
linux-soc, linux-clk, linux-kernel, devicetree
On 5/1/2018 6:13 PM, Rob Herring wrote:
> On Tue, May 01, 2018 at 02:11:33PM +0530, Taniya Das wrote:
>> Add the RPMh clock driver to control the RPMh managed clock resources on
>> some of the Qualcomm Technologies, Inc. SoCs.
>>
>> 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 | 387 ++++++++++++++++++++++++++++++++++
>> include/dt-bindings/clock/qcom,rpmh.h | 2 -
>> 4 files changed, 397 insertions(+), 2 deletions(-)
>> create mode 100644 drivers/clk/qcom/clk-rpmh.c
>
>> diff --git a/include/dt-bindings/clock/qcom,rpmh.h b/include/dt-bindings/clock/qcom,rpmh.h
>> index 36caab2..f48fbd6 100644
>> --- a/include/dt-bindings/clock/qcom,rpmh.h
>> +++ b/include/dt-bindings/clock/qcom,rpmh.h
>> @@ -18,7 +18,5 @@
>> #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
>
> Did you mean to have this or squash into previous patch?
>
Thanks, my mistake. It was meant to be squashed in the previous patch. I
would take care of it in the next series.
--
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] 12+ messages in thread
* Re: [v5 1/2] dt-bindings: clock: Introduce QCOM RPMh clock bindings
2018-05-01 8:41 ` [v5 1/2] dt-bindings: clock: Introduce QCOM RPMh clock bindings Taniya Das
2018-05-01 21:10 ` Stephen Boyd
@ 2018-05-01 21:10 ` Stephen Boyd
0 siblings, 0 replies; 12+ messages in thread
From: Stephen Boyd @ 2018-05-01 21:10 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,
devicetree, Taniya Das
Quoting Taniya Das (2018-05-01 01:41:32)
> diff --git a/Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt b/Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt
> new file mode 100644
> index 0000000..ecc1dbe
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt
> @@ -0,0 +1,32 @@
> +Qualcomm Technologies, Inc. RPMh Clocks
> +-------------------------------------------------------
> +
> +Resource Power Manager Hardened (RPMh) manages shared resources on
> +some Qualcomm Technologies Inc. SoCs. It accepts clock requests from
> +other hardware subsystems via RSC to control clocks.
> +
> +Required properties :
> +- compatible : shall contain "qcom,sdm845-rpmh-clk"
> +
> +- #clock-cells : must contain 1
> +
> +Optional properties :
> +- assigned-clk-divs : property should contain a list of divs for each clock in
> + the clk-output-names property. In case divs are not
> + provided the clock rate would be same as parent rate.
> +- clk-output-names : a list of strings of clock output signal for the divs to
> + be applied.
> +
> +Example :
> +
> +#include <dt-bindings/clock/qcom,rpmh.h>
> +
> + &apps_rsc {
> + rpmhcc: clock-controller {
> + compatible = "qcom,sdm845-rpmh-clk";
> + #clock-cells = <1>;
> + assigned-clk-divs = <2 2 2>;
This property shouldn't need to exist. Instead, add a fixed div-2 clock
to the DTS file (I guess in the SoC file) to divide the crystal
frequency into the rate you want (19.2 MHz in this case).
> + clk-output-names = "bi_tcxo", "lnbb_clk2",
> + "lnbb_clk3";
We don't need this either.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [v5 1/2] dt-bindings: clock: Introduce QCOM RPMh clock bindings
@ 2018-05-01 21:10 ` Stephen Boyd
0 siblings, 0 replies; 12+ messages in thread
From: Stephen Boyd @ 2018-05-01 21:10 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,
devicetree, Taniya Das
Quoting Taniya Das (2018-05-01 01:41:32)
> diff --git a/Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt b/Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt
> new file mode 100644
> index 0000000..ecc1dbe
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt
> @@ -0,0 +1,32 @@
> +Qualcomm Technologies, Inc. RPMh Clocks
> +-------------------------------------------------------
> +
> +Resource Power Manager Hardened (RPMh) manages shared resources on
> +some Qualcomm Technologies Inc. SoCs. It accepts clock requests from
> +other hardware subsystems via RSC to control clocks.
> +
> +Required properties :
> +- compatible : shall contain "qcom,sdm845-rpmh-clk"
> +
> +- #clock-cells : must contain 1
> +
> +Optional properties :
> +- assigned-clk-divs : property should contain a list of divs for each clock in
> + the clk-output-names property. In case divs are not
> + provided the clock rate would be same as parent rate.
> +- clk-output-names : a list of strings of clock output signal for the divs to
> + be applied.
> +
> +Example :
> +
> +#include <dt-bindings/clock/qcom,rpmh.h>
> +
> + &apps_rsc {
> + rpmhcc: clock-controller {
> + compatible = "qcom,sdm845-rpmh-clk";
> + #clock-cells = <1>;
> + assigned-clk-divs = <2 2 2>;
This property shouldn't need to exist. Instead, add a fixed div-2 clock
to the DTS file (I guess in the SoC file) to divide the crystal
frequency into the rate you want (19.2 MHz in this case).
> + clk-output-names = "bi_tcxo", "lnbb_clk2",
> + "lnbb_clk3";
We don't need this either.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [v5 1/2] dt-bindings: clock: Introduce QCOM RPMh clock bindings
@ 2018-05-01 21:10 ` Stephen Boyd
0 siblings, 0 replies; 12+ messages in thread
From: Stephen Boyd @ 2018-05-01 21:10 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,
devicetree, Taniya Das
Quoting Taniya Das (2018-05-01 01:41:32)
> diff --git a/Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt b/=
Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt
> new file mode 100644
> index 0000000..ecc1dbe
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt
> @@ -0,0 +1,32 @@
> +Qualcomm Technologies, Inc. RPMh Clocks
> +-------------------------------------------------------
> +
> +Resource Power Manager Hardened (RPMh) manages shared resources on
> +some Qualcomm Technologies Inc. SoCs. It accepts clock requests from
> +other hardware subsystems via RSC to control clocks.
> +
> +Required properties :
> +- compatible : shall contain "qcom,sdm845-rpmh-clk"
> +
> +- #clock-cells : must contain 1
> +
> +Optional properties :
> +- assigned-clk-divs : property should contain a list of divs for each cl=
ock in
> + the clk-output-names property. In case divs are n=
ot
> + provided the clock rate would be same as parent r=
ate.
> +- clk-output-names : a list of strings of clock output signal for the d=
ivs to
> + be applied.
> +
> +Example :
> +
> +#include <dt-bindings/clock/qcom,rpmh.h>
> +
> + &apps_rsc {
> + rpmhcc: clock-controller {
> + compatible =3D "qcom,sdm845-rpmh-clk";
> + #clock-cells =3D <1>;
> + assigned-clk-divs =3D <2 2 2>;
This property shouldn't need to exist. Instead, add a fixed div-2 clock
to the DTS file (I guess in the SoC file) to divide the crystal
frequency into the rate you want (19.2 MHz in this case).
> + clk-output-names =3D "bi_tcxo", "lnbb_clk2",
> + "lnbb_clk3";
We don't need this either.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [v5 2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver
2018-05-01 8:41 ` [v5 2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver Taniya Das
2018-05-01 12:43 ` Rob Herring
@ 2018-05-01 21:27 ` Stephen Boyd
1 sibling, 0 replies; 12+ messages in thread
From: Stephen Boyd @ 2018-05-01 21:27 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,
devicetree, Taniya Das
Quoting Taniya Das (2018-05-01 01:41:33)
> Add the RPMh clock driver to control the RPMh managed clock resources on
> some of the Qualcomm Technologies, Inc. SoCs.
>
> Signed-off-by: Amit Nischal <anischal@codeaurora.org>
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
Drop Amit's signoff, or make it "Co-authored-by" please. First signoff
should be the author and there should be a "From:" line either from the
email header of the sender or explicitly here before the commit text
body to indicate the authorship.
Overall this is looking close. Maybe one more round.
> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
> new file mode 100644
> index 0000000..9cb7aa4
> --- /dev/null
> +++ b/drivers/clk/qcom/clk-rpmh.c
> @@ -0,0 +1,387 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <soc/qcom/cmd-db.h>
> +#include <soc/qcom/rpmh.h>
> +
> +#include <dt-bindings/clock/qcom,rpmh.h>
> +
> +#define CLK_RPMH_ARC_EN_OFFSET 0
> +#define CLK_RPMH_VRM_EN_OFFSET 4
> +
> +/**
> + * struct clk_rpmh - individual rpmh clock data structure
> + * @hw: handle between common and hardware-specific interfaces
> + * @res_name: resource name for the rpmh clock
> + * @div: clock divider to compute the clock rate
> + * @res_addr: base address of the rpmh resource within the RPMh
> + * @res_on_val: rpmh clock enable value
> + * @res_off_val: rpmh clock disable value
It's still unused.
> + * @state: rpmh clock requested state
> + * @aggr_state: rpmh clock aggregated state
> + * @last_sent_aggr_state: rpmh clock last aggr state sent to RPMh
> + * @valid_state_mask: mask to determine the state of the rpmh clock
> + * @rpmh_client: handle used for rpmh communications
> + * @dev: device to which it is attached
> + * @peer: pointer to the clock rpmh sibling
> + */
> +struct clk_rpmh {
> + struct clk_hw hw;
> + const char *res_name;
> + u8 div;
Weird space here.
> + u32 res_addr;
> + 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;
> +};
> +
> +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) \
> + static struct clk_rpmh _platform##_##_name_active; \
> + static struct clk_rpmh _platform##_##_name = { \
> + .res_name = _res_name, \
> + .res_addr = _res_en_offset, \
> + .res_on_val = _res_on, \
> + .res_off_val = _res_off, \
> + .peer = &_platform##_##_name_active, \
> + .valid_state_mask = (BIT(RPMH_WAKE_ONLY_STATE) | \
> + BIT(RPMH_ACTIVE_ONLY_STATE) | \
> + BIT(RPMH_SLEEP_STATE)), \
> + .hw.init = &(struct clk_init_data){ \
> + .ops = &clk_rpmh_ops, \
> + .name = #_name, \
> + .parent_names = (const char *[]){ "xo_board" }, \
This would be different for the xo_board and the xo_board / 2 clk names.
> + .num_parents = 1, \
> + }, \
> + }; \
> + static struct clk_rpmh _platform##_##_name_active = { \
> + .res_name = _res_name, \
> + .res_addr = _res_en_offset, \
> + .res_on_val = _res_on, \
> + .res_off_val = _res_off, \
> + .peer = &_platform##_##_name, \
> + .valid_state_mask = (BIT(RPMH_WAKE_ONLY_STATE) | \
> + BIT(RPMH_ACTIVE_ONLY_STATE)), \
> + .hw.init = &(struct clk_init_data){ \
> + .ops = &clk_rpmh_ops, \
> + .name = #_name_active, \
> + .parent_names = (const char *[]){ "xo_board" }, \
> + .num_parents = 1, \
> + }, \
> + }
> +
> +#define DEFINE_CLK_RPMH_ARC(_platform, _name, _name_active, _res_name, \
> + _res_on, _res_off) \
> + __DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name, \
> + CLK_RPMH_ARC_EN_OFFSET, _res_on, _res_off)
> +
> +#define DEFINE_CLK_RPMH_VRM(_platform, _name, _name_active, _res_name) \
> + __DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name, \
> + CLK_RPMH_VRM_EN_OFFSET, true, false)
s/true, false/1, 0/ ?
> +
> +static inline struct clk_rpmh *to_clk_rpmh(struct clk_hw *_hw)
> +{
> + return container_of(_hw, struct clk_rpmh, hw);
> +}
> +
> +static inline bool has_state_changed(struct clk_rpmh *c, u32 state)
> +{
> + return (c->last_sent_aggr_state & BIT(state))
> + != (c->aggr_state & BIT(state));
> +}
> +
> +static int clk_rpmh_send_aggregate_command(struct clk_rpmh *c)
> +{
> + struct tcs_cmd cmd = { 0 };
> + u32 cmd_state, on_val;
> + enum rpmh_state state = RPMH_SLEEP_STATE;
> + int ret;
> +
> + cmd.addr = c->res_addr;
> + cmd_state = c->aggr_state;
> + on_val = c->res_on_val;
> +
> + for (; state <= RPMH_ACTIVE_ONLY_STATE; state++) {
> + if (has_state_changed(c, state)) {
> + cmd.data = (cmd_state & BIT(state)) ? on_val : 0;
Make this into an "if" please. Default is already 0 so just set bit if
cmd_state & BIT(state).
> + ret = rpmh_write_async(c->rpmh_client, state,
> + &cmd, 1);
> + if (ret) {
> + dev_err(c->dev, "set %s state of %s failed: (%d)\n",
> + !state ? "sleep" :
> + state == RPMH_WAKE_ONLY_STATE ?
> + "wake" : "active", c->res_name, ret);
> + return ret;
> + }
> + }
> + }
> +
> + c->last_sent_aggr_state = c->aggr_state;
> + c->peer->last_sent_aggr_state = c->last_sent_aggr_state;
> +
> + return 0;
> +}
> +
> +/*
> + * Update state and aggregate state values based on enable value.
> + */
> +static int clk_rpmh_aggregate_state_send_command(struct clk_rpmh *c,
> + bool enable)
> +{
> + int ret;
> +
> + 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;
Why don't we print a WARN if it fails to enable?
> + } 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)
> +{
> + struct clk_rpmh *c = to_clk_rpmh(hw);
> + int ret = 0;
> +
> + mutex_lock(&rpmh_clk_lock);
> +
> + if (!c->state)
Can we push this into clk_rpmh_aggregate_state_send_command()? Something
like:
/* Nothing to do if already off or on */
if (enable == c->state)
return 0;
> + ret = clk_rpmh_aggregate_state_send_command(c, true);
> +
> + mutex_unlock(&rpmh_clk_lock);
> + return ret;
> +};
> +
> +static void clk_rpmh_unprepare(struct clk_hw *hw)
> +{
> + struct clk_rpmh *c = to_clk_rpmh(hw);
> +
> + mutex_lock(&rpmh_clk_lock);
> +
> + if (c->state)
> + clk_rpmh_aggregate_state_send_command(c, false);
> +
> + mutex_unlock(&rpmh_clk_lock);
> +};
> +
> +static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw,
> + unsigned long prate)
> +{
> + struct clk_rpmh *r = to_clk_rpmh(hw);
> + unsigned long rate;
> +
> + /*
> + * RPMh clocks have a fixed rate.
> + */
> + rate = prate;
> + do_div(rate, r->div);
> +
> + return rate;
> +}
You shouldn't need any recalc rate in this driver.
> +
> +static int clk_rpmh_probe(struct platform_device *pdev)
> +{
> + struct clk *clk;
> + struct clk_hw_onecell_data *hw_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;
Just do this when dev is defined please.
> +
> + ret = cmd_db_ready();
> + if (ret) {
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev, "Command DB not available (%d)\n", ret);
> + return ret;
> + }
> +
> + rpmh_client = rpmh_get_client(pdev);
> + if (IS_ERR(rpmh_client)) {
> + ret = PTR_ERR(rpmh_client);
> + dev_err(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;
> +
> + hw_data = devm_kzalloc(dev,
> + sizeof(*hw_data) + num_clks * sizeof(struct clk_hw *),
> + GFP_KERNEL);
> + if (!hw_data)
> + return -ENOMEM;
> +
> + hw_data->num = num_clks;
> +
> + for (i = 0; i < num_clks; i++) {
> + const char *clk_name;
> + struct property *prop;
> + const __be32 *p;
> + u32 res_addr;
> + int div, j = 0;
> +
> + rpmh_clk = to_clk_rpmh(hw_clks[i]);
> + res_addr = cmd_db_read_addr(rpmh_clk->res_name);
> + if (!res_addr) {
> + dev_err(dev, "missing RPMh resource address for %s\n",
> + rpmh_clk->res_name);
> + ret = -ENODEV;
> + goto err;
> + }
> + rpmh_clk->res_addr += res_addr;
Thanks!
> +
> + rpmh_clk->rpmh_client = rpmh_client;
> +
> + /* default div for all clocks */
> + if (!rpmh_clk->div)
> + rpmh_clk->div = 1;
> +
> + of_property_for_each_u32(pdev->dev.of_node, "assigned-clk-divs",
> + prop, p, div) {
> + of_property_read_string_index(pdev->dev.of_node,
> + "clk-output-names", j, &clk_name);
> + if (!strcmp(clk_name, hw_clks[i]->init->name)) {
> + rpmh_clk->div = div;
> + rpmh_clk->peer->div = div;
> + }
> + j++;
> + }
> +
> + 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;
> + }
> +
> + rpmh_clk->dev = &pdev->dev;
> + }
> +
> + ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
Why not roll your own "getter" function and then just index directly
into your array of rpmh clks for sdm845? Then we don't have to copy
things from one place to another.
> + hw_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);
> +
> + return ret;
> +}
> +
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [v5 2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver
@ 2018-05-01 21:27 ` Stephen Boyd
0 siblings, 0 replies; 12+ messages in thread
From: Stephen Boyd @ 2018-05-01 21:27 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,
devicetree, Taniya Das
Quoting Taniya Das (2018-05-01 01:41:33)
> Add the RPMh clock driver to control the RPMh managed clock resources on
> some of the Qualcomm Technologies, Inc. SoCs.
>
> Signed-off-by: Amit Nischal <anischal@codeaurora.org>
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
Drop Amit's signoff, or make it "Co-authored-by" please. First signoff
should be the author and there should be a "From:" line either from the
email header of the sender or explicitly here before the commit text
body to indicate the authorship.
Overall this is looking close. Maybe one more round.
> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
> new file mode 100644
> index 0000000..9cb7aa4
> --- /dev/null
> +++ b/drivers/clk/qcom/clk-rpmh.c
> @@ -0,0 +1,387 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <soc/qcom/cmd-db.h>
> +#include <soc/qcom/rpmh.h>
> +
> +#include <dt-bindings/clock/qcom,rpmh.h>
> +
> +#define CLK_RPMH_ARC_EN_OFFSET 0
> +#define CLK_RPMH_VRM_EN_OFFSET 4
> +
> +/**
> + * struct clk_rpmh - individual rpmh clock data structure
> + * @hw: handle between common and hardware-specific interfaces
> + * @res_name: resource name for the rpmh clock
> + * @div: clock divider to compute the clock rate
> + * @res_addr: base address of the rpmh resource within the RPMh
> + * @res_on_val: rpmh clock enable value
> + * @res_off_val: rpmh clock disable value
It's still unused.
> + * @state: rpmh clock requested state
> + * @aggr_state: rpmh clock aggregated state
> + * @last_sent_aggr_state: rpmh clock last aggr state sent to RPMh
> + * @valid_state_mask: mask to determine the state of the rpmh clock
> + * @rpmh_client: handle used for rpmh communications
> + * @dev: device to which it is attached
> + * @peer: pointer to the clock rpmh sibling
> + */
> +struct clk_rpmh {
> + struct clk_hw hw;
> + const char *res_name;
> + u8 div;
Weird space here.
> + u32 res_addr;
> + 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;
> +};
> +
> +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) \
> + static struct clk_rpmh _platform##_##_name_active; \
> + static struct clk_rpmh _platform##_##_name = { \
> + .res_name = _res_name, \
> + .res_addr = _res_en_offset, \
> + .res_on_val = _res_on, \
> + .res_off_val = _res_off, \
> + .peer = &_platform##_##_name_active, \
> + .valid_state_mask = (BIT(RPMH_WAKE_ONLY_STATE) | \
> + BIT(RPMH_ACTIVE_ONLY_STATE) | \
> + BIT(RPMH_SLEEP_STATE)), \
> + .hw.init = &(struct clk_init_data){ \
> + .ops = &clk_rpmh_ops, \
> + .name = #_name, \
> + .parent_names = (const char *[]){ "xo_board" }, \
This would be different for the xo_board and the xo_board / 2 clk names.
> + .num_parents = 1, \
> + }, \
> + }; \
> + static struct clk_rpmh _platform##_##_name_active = { \
> + .res_name = _res_name, \
> + .res_addr = _res_en_offset, \
> + .res_on_val = _res_on, \
> + .res_off_val = _res_off, \
> + .peer = &_platform##_##_name, \
> + .valid_state_mask = (BIT(RPMH_WAKE_ONLY_STATE) | \
> + BIT(RPMH_ACTIVE_ONLY_STATE)), \
> + .hw.init = &(struct clk_init_data){ \
> + .ops = &clk_rpmh_ops, \
> + .name = #_name_active, \
> + .parent_names = (const char *[]){ "xo_board" }, \
> + .num_parents = 1, \
> + }, \
> + }
> +
> +#define DEFINE_CLK_RPMH_ARC(_platform, _name, _name_active, _res_name, \
> + _res_on, _res_off) \
> + __DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name, \
> + CLK_RPMH_ARC_EN_OFFSET, _res_on, _res_off)
> +
> +#define DEFINE_CLK_RPMH_VRM(_platform, _name, _name_active, _res_name) \
> + __DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name, \
> + CLK_RPMH_VRM_EN_OFFSET, true, false)
s/true, false/1, 0/ ?
> +
> +static inline struct clk_rpmh *to_clk_rpmh(struct clk_hw *_hw)
> +{
> + return container_of(_hw, struct clk_rpmh, hw);
> +}
> +
> +static inline bool has_state_changed(struct clk_rpmh *c, u32 state)
> +{
> + return (c->last_sent_aggr_state & BIT(state))
> + != (c->aggr_state & BIT(state));
> +}
> +
> +static int clk_rpmh_send_aggregate_command(struct clk_rpmh *c)
> +{
> + struct tcs_cmd cmd = { 0 };
> + u32 cmd_state, on_val;
> + enum rpmh_state state = RPMH_SLEEP_STATE;
> + int ret;
> +
> + cmd.addr = c->res_addr;
> + cmd_state = c->aggr_state;
> + on_val = c->res_on_val;
> +
> + for (; state <= RPMH_ACTIVE_ONLY_STATE; state++) {
> + if (has_state_changed(c, state)) {
> + cmd.data = (cmd_state & BIT(state)) ? on_val : 0;
Make this into an "if" please. Default is already 0 so just set bit if
cmd_state & BIT(state).
> + ret = rpmh_write_async(c->rpmh_client, state,
> + &cmd, 1);
> + if (ret) {
> + dev_err(c->dev, "set %s state of %s failed: (%d)\n",
> + !state ? "sleep" :
> + state == RPMH_WAKE_ONLY_STATE ?
> + "wake" : "active", c->res_name, ret);
> + return ret;
> + }
> + }
> + }
> +
> + c->last_sent_aggr_state = c->aggr_state;
> + c->peer->last_sent_aggr_state = c->last_sent_aggr_state;
> +
> + return 0;
> +}
> +
> +/*
> + * Update state and aggregate state values based on enable value.
> + */
> +static int clk_rpmh_aggregate_state_send_command(struct clk_rpmh *c,
> + bool enable)
> +{
> + int ret;
> +
> + 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;
Why don't we print a WARN if it fails to enable?
> + } 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)
> +{
> + struct clk_rpmh *c = to_clk_rpmh(hw);
> + int ret = 0;
> +
> + mutex_lock(&rpmh_clk_lock);
> +
> + if (!c->state)
Can we push this into clk_rpmh_aggregate_state_send_command()? Something
like:
/* Nothing to do if already off or on */
if (enable == c->state)
return 0;
> + ret = clk_rpmh_aggregate_state_send_command(c, true);
> +
> + mutex_unlock(&rpmh_clk_lock);
> + return ret;
> +};
> +
> +static void clk_rpmh_unprepare(struct clk_hw *hw)
> +{
> + struct clk_rpmh *c = to_clk_rpmh(hw);
> +
> + mutex_lock(&rpmh_clk_lock);
> +
> + if (c->state)
> + clk_rpmh_aggregate_state_send_command(c, false);
> +
> + mutex_unlock(&rpmh_clk_lock);
> +};
> +
> +static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw,
> + unsigned long prate)
> +{
> + struct clk_rpmh *r = to_clk_rpmh(hw);
> + unsigned long rate;
> +
> + /*
> + * RPMh clocks have a fixed rate.
> + */
> + rate = prate;
> + do_div(rate, r->div);
> +
> + return rate;
> +}
You shouldn't need any recalc rate in this driver.
> +
> +static int clk_rpmh_probe(struct platform_device *pdev)
> +{
> + struct clk *clk;
> + struct clk_hw_onecell_data *hw_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;
Just do this when dev is defined please.
> +
> + ret = cmd_db_ready();
> + if (ret) {
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev, "Command DB not available (%d)\n", ret);
> + return ret;
> + }
> +
> + rpmh_client = rpmh_get_client(pdev);
> + if (IS_ERR(rpmh_client)) {
> + ret = PTR_ERR(rpmh_client);
> + dev_err(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;
> +
> + hw_data = devm_kzalloc(dev,
> + sizeof(*hw_data) + num_clks * sizeof(struct clk_hw *),
> + GFP_KERNEL);
> + if (!hw_data)
> + return -ENOMEM;
> +
> + hw_data->num = num_clks;
> +
> + for (i = 0; i < num_clks; i++) {
> + const char *clk_name;
> + struct property *prop;
> + const __be32 *p;
> + u32 res_addr;
> + int div, j = 0;
> +
> + rpmh_clk = to_clk_rpmh(hw_clks[i]);
> + res_addr = cmd_db_read_addr(rpmh_clk->res_name);
> + if (!res_addr) {
> + dev_err(dev, "missing RPMh resource address for %s\n",
> + rpmh_clk->res_name);
> + ret = -ENODEV;
> + goto err;
> + }
> + rpmh_clk->res_addr += res_addr;
Thanks!
> +
> + rpmh_clk->rpmh_client = rpmh_client;
> +
> + /* default div for all clocks */
> + if (!rpmh_clk->div)
> + rpmh_clk->div = 1;
> +
> + of_property_for_each_u32(pdev->dev.of_node, "assigned-clk-divs",
> + prop, p, div) {
> + of_property_read_string_index(pdev->dev.of_node,
> + "clk-output-names", j, &clk_name);
> + if (!strcmp(clk_name, hw_clks[i]->init->name)) {
> + rpmh_clk->div = div;
> + rpmh_clk->peer->div = div;
> + }
> + j++;
> + }
> +
> + 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;
> + }
> +
> + rpmh_clk->dev = &pdev->dev;
> + }
> +
> + ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
Why not roll your own "getter" function and then just index directly
into your array of rpmh clks for sdm845? Then we don't have to copy
things from one place to another.
> + hw_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);
> +
> + return ret;
> +}
> +
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [v5 2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver
@ 2018-05-01 21:27 ` Stephen Boyd
0 siblings, 0 replies; 12+ messages in thread
From: Stephen Boyd @ 2018-05-01 21:27 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,
devicetree, Taniya Das
Quoting Taniya Das (2018-05-01 01:41:33)
> Add the RPMh clock driver to control the RPMh managed clock resources on
> some of the Qualcomm Technologies, Inc. SoCs.
> =
> Signed-off-by: Amit Nischal <anischal@codeaurora.org>
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
Drop Amit's signoff, or make it "Co-authored-by" please. First signoff
should be the author and there should be a "From:" line either from the
email header of the sender or explicitly here before the commit text
body to indicate the authorship.
Overall this is looking close. Maybe one more round.
> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
> new file mode 100644
> index 0000000..9cb7aa4
> --- /dev/null
> +++ b/drivers/clk/qcom/clk-rpmh.c
> @@ -0,0 +1,387 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <soc/qcom/cmd-db.h>
> +#include <soc/qcom/rpmh.h>
> +
> +#include <dt-bindings/clock/qcom,rpmh.h>
> +
> +#define CLK_RPMH_ARC_EN_OFFSET 0
> +#define CLK_RPMH_VRM_EN_OFFSET 4
> +
> +/**
> + * struct clk_rpmh - individual rpmh clock data structure
> + * @hw: handle between common and hardware-specif=
ic interfaces
> + * @res_name: resource name for the rpmh clock
> + * @div: clock divider to compute the clock rate
> + * @res_addr: base address of the rpmh resource within the RPMh
> + * @res_on_val: rpmh clock enable value
> + * @res_off_val: rpmh clock disable value
It's still unused.
> + * @state: rpmh clock requested state
> + * @aggr_state: rpmh clock aggregated state
> + * @last_sent_aggr_state: rpmh clock last aggr state sent to RPMh
> + * @valid_state_mask: mask to determine the state of the rpmh clock
> + * @rpmh_client: handle used for rpmh communications
> + * @dev: device to which it is attached
> + * @peer: pointer to the clock rpmh sibling
> + */
> +struct clk_rpmh {
> + struct clk_hw hw;
> + const char *res_name;
> + u8 div;
Weird space here.
> + u32 res_addr;
> + 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;
> +};
> +
> +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) \
> + static struct clk_rpmh _platform##_##_name_active; \
> + static struct clk_rpmh _platform##_##_name =3D { =
\
> + .res_name =3D _res_name, =
\
> + .res_addr =3D _res_en_offset, =
\
> + .res_on_val =3D _res_on, =
\
> + .res_off_val =3D _res_off, =
\
> + .peer =3D &_platform##_##_name_active, =
\
> + .valid_state_mask =3D (BIT(RPMH_WAKE_ONLY_STATE) | =
\
> + BIT(RPMH_ACTIVE_ONLY_STATE) | \
> + BIT(RPMH_SLEEP_STATE)), \
> + .hw.init =3D &(struct clk_init_data){ =
\
> + .ops =3D &clk_rpmh_ops, =
\
> + .name =3D #_name, =
\
> + .parent_names =3D (const char *[]){ "xo_board" },=
\
This would be different for the xo_board and the xo_board / 2 clk names.
> + .num_parents =3D 1, =
\
> + }, \
> + }; \
> + static struct clk_rpmh _platform##_##_name_active =3D { =
\
> + .res_name =3D _res_name, =
\
> + .res_addr =3D _res_en_offset, =
\
> + .res_on_val =3D _res_on, =
\
> + .res_off_val =3D _res_off, =
\
> + .peer =3D &_platform##_##_name, =
\
> + .valid_state_mask =3D (BIT(RPMH_WAKE_ONLY_STATE) | =
\
> + BIT(RPMH_ACTIVE_ONLY_STATE)), \
> + .hw.init =3D &(struct clk_init_data){ =
\
> + .ops =3D &clk_rpmh_ops, =
\
> + .name =3D #_name_active, =
\
> + .parent_names =3D (const char *[]){ "xo_board" },=
\
> + .num_parents =3D 1, =
\
> + }, \
> + }
> +
> +#define DEFINE_CLK_RPMH_ARC(_platform, _name, _name_active, _res_name, \
> + _res_on, _res_off) \
> + __DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name, \
> + CLK_RPMH_ARC_EN_OFFSET, _res_on, _res_off)
> +
> +#define DEFINE_CLK_RPMH_VRM(_platform, _name, _name_active, _res_name) \
> + __DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name, \
> + CLK_RPMH_VRM_EN_OFFSET, true, false)
s/true, false/1, 0/ ?
> +
> +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))
> + !=3D (c->aggr_state & BIT(state));
> +}
> +
> +static int clk_rpmh_send_aggregate_command(struct clk_rpmh *c)
> +{
> + struct tcs_cmd cmd =3D { 0 };
> + u32 cmd_state, on_val;
> + enum rpmh_state state =3D RPMH_SLEEP_STATE;
> + int ret;
> +
> + cmd.addr =3D c->res_addr;
> + cmd_state =3D c->aggr_state;
> + on_val =3D c->res_on_val;
> +
> + for (; state <=3D RPMH_ACTIVE_ONLY_STATE; state++) {
> + if (has_state_changed(c, state)) {
> + cmd.data =3D (cmd_state & BIT(state)) ? on_val : =
0;
Make this into an "if" please. Default is already 0 so just set bit if
cmd_state & BIT(state).
> + ret =3D rpmh_write_async(c->rpmh_client, state,
> + &cmd, 1);
> + if (ret) {
> + dev_err(c->dev, "set %s state of %s faile=
d: (%d)\n",
> + !state ? "sleep" :
> + state =3D=3D RPMH_WAKE_ONLY_STATE=
?
> + "wake" : "active", c->res_name, r=
et);
> + return ret;
> + }
> + }
> + }
> +
> + c->last_sent_aggr_state =3D c->aggr_state;
> + c->peer->last_sent_aggr_state =3D c->last_sent_aggr_state;
> +
> + return 0;
> +}
> +
> +/*
> + * Update state and aggregate state values based on enable value.
> + */
> +static int clk_rpmh_aggregate_state_send_command(struct clk_rpmh *c,
> + bool enable)
> +{
> + int ret;
> +
> + c->state =3D enable ? c->valid_state_mask : 0;
> + c->aggr_state =3D c->state | c->peer->state;
> + c->peer->aggr_state =3D c->aggr_state;
> +
> + ret =3D clk_rpmh_send_aggregate_command(c);
> + if (ret && enable) {
> + c->state =3D 0;
Why don't we print a WARN if it fails to enable?
> + } else if (ret) {
> + c->state =3D c->valid_state_mask;
> + WARN(1, "clk: %s failed to disable\n", c->res_name);
> + }
> +
> + return ret;
> +}
> +
> +static int clk_rpmh_prepare(struct clk_hw *hw)
> +{
> + struct clk_rpmh *c =3D to_clk_rpmh(hw);
> + int ret =3D 0;
> +
> + mutex_lock(&rpmh_clk_lock);
> +
> + if (!c->state)
Can we push this into clk_rpmh_aggregate_state_send_command()? Something
like:
/* Nothing to do if already off or on */
if (enable =3D=3D c->state)
return 0;
> + ret =3D clk_rpmh_aggregate_state_send_command(c, true);
> +
> + mutex_unlock(&rpmh_clk_lock);
> + return ret;
> +};
> +
> +static void clk_rpmh_unprepare(struct clk_hw *hw)
> +{
> + struct clk_rpmh *c =3D to_clk_rpmh(hw);
> +
> + mutex_lock(&rpmh_clk_lock);
> +
> + if (c->state)
> + clk_rpmh_aggregate_state_send_command(c, false);
> +
> + mutex_unlock(&rpmh_clk_lock);
> +};
> +
> +static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw,
> + unsigned long prate)
> +{
> + struct clk_rpmh *r =3D to_clk_rpmh(hw);
> + unsigned long rate;
> +
> + /*
> + * RPMh clocks have a fixed rate.
> + */
> + rate =3D prate;
> + do_div(rate, r->div);
> +
> + return rate;
> +}
You shouldn't need any recalc rate in this driver.
> +
> +static int clk_rpmh_probe(struct platform_device *pdev)
> +{
> + struct clk *clk;
> + struct clk_hw_onecell_data *hw_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 =3D &pdev->dev;
Just do this when dev is defined please.
> +
> + ret =3D cmd_db_ready();
> + if (ret) {
> + if (ret !=3D -EPROBE_DEFER)
> + dev_err(dev, "Command DB not available (%d)\n", r=
et);
> + return ret;
> + }
> +
> + rpmh_client =3D rpmh_get_client(pdev);
> + if (IS_ERR(rpmh_client)) {
> + ret =3D PTR_ERR(rpmh_client);
> + dev_err(dev, "Failed to request RPMh client, ret=3D%d\n",=
ret);
> + return ret;
> + }
> +
> + desc =3D of_device_get_match_data(&pdev->dev);
> + hw_clks =3D desc->clks;
> + num_clks =3D desc->num_clks;
> +
> + hw_data =3D devm_kzalloc(dev,
> + sizeof(*hw_data) + num_clks * sizeof(struct clk_h=
w *),
> + GFP_KERNEL);
> + if (!hw_data)
> + return -ENOMEM;
> +
> + hw_data->num =3D num_clks;
> +
> + for (i =3D 0; i < num_clks; i++) {
> + const char *clk_name;
> + struct property *prop;
> + const __be32 *p;
> + u32 res_addr;
> + int div, j =3D 0;
> +
> + rpmh_clk =3D to_clk_rpmh(hw_clks[i]);
> + res_addr =3D cmd_db_read_addr(rpmh_clk->res_name);
> + if (!res_addr) {
> + dev_err(dev, "missing RPMh resource address for %=
s\n",
> + rpmh_clk->res_name);
> + ret =3D -ENODEV;
> + goto err;
> + }
> + rpmh_clk->res_addr +=3D res_addr;
Thanks!
> +
> + rpmh_clk->rpmh_client =3D rpmh_client;
> +
> + /* default div for all clocks */
> + if (!rpmh_clk->div)
> + rpmh_clk->div =3D 1;
> +
> + of_property_for_each_u32(pdev->dev.of_node, "assigned-clk=
-divs",
> + prop, p, div) {
> + of_property_read_string_index(pdev->dev.of_node,
> + "clk-output-names", j, &clk_name);
> + if (!strcmp(clk_name, hw_clks[i]->init->name)) {
> + rpmh_clk->div =3D div;
> + rpmh_clk->peer->div =3D div;
> + }
> + j++;
> + }
> +
> + clk =3D devm_clk_register(&pdev->dev, hw_clks[i]);
> + if (IS_ERR(clk)) {
> + ret =3D PTR_ERR(clk);
> + dev_err(dev, "failed to register %s\n",
> + hw_clks[i]->init->name);
> + goto err;
> + }
> +
> + rpmh_clk->dev =3D &pdev->dev;
> + }
> +
> + ret =3D devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
Why not roll your own "getter" function and then just index directly
into your array of rpmh clks for sdm845? Then we don't have to copy
things from one place to another.
> + hw_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);
> +
> + return ret;
> +}
> +
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [v5 2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver
2018-05-01 21:27 ` Stephen Boyd
(?)
(?)
@ 2018-05-02 10:22 ` Taniya Das
-1 siblings, 0 replies; 12+ messages in thread
From: Taniya Das @ 2018-05-02 10:22 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,
devicetree
Thanks Stephen for the comments.
On 5/2/2018 2:57 AM, Stephen Boyd wrote:
> Quoting Taniya Das (2018-05-01 01:41:33)
>> Add the RPMh clock driver to control the RPMh managed clock resources on
>> some of the Qualcomm Technologies, Inc. SoCs.
>>
>> Signed-off-by: Amit Nischal <anischal@codeaurora.org>
>> Signed-off-by: Taniya Das <tdas@codeaurora.org>
>
> Drop Amit's signoff, or make it "Co-authored-by" please. First signoff
> should be the author and there should be a "From:" line either from the
> email header of the sender or explicitly here before the commit text
> body to indicate the authorship.
>
> Overall this is looking close. Maybe one more round.
>
>> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
>> new file mode 100644
>> index 0000000..9cb7aa4
>> --- /dev/null
>> +++ b/drivers/clk/qcom/clk-rpmh.c
>> @@ -0,0 +1,387 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include <linux/clk-provider.h>
>> +#include <linux/err.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <soc/qcom/cmd-db.h>
>> +#include <soc/qcom/rpmh.h>
>> +
>> +#include <dt-bindings/clock/qcom,rpmh.h>
>> +
>> +#define CLK_RPMH_ARC_EN_OFFSET 0
>> +#define CLK_RPMH_VRM_EN_OFFSET 4
>> +
>> +/**
>> + * struct clk_rpmh - individual rpmh clock data structure
>> + * @hw: handle between common and hardware-specific interfaces
>> + * @res_name: resource name for the rpmh clock
>> + * @div: clock divider to compute the clock rate
>> + * @res_addr: base address of the rpmh resource within the RPMh
>> + * @res_on_val: rpmh clock enable value
>> + * @res_off_val: rpmh clock disable value
>
> It's still unused.
>
It is unused as the currently the resource off value is '0'. I would
remove, will add it back if any different value is required.
>> + * @state: rpmh clock requested state
>> + * @aggr_state: rpmh clock aggregated state
>> + * @last_sent_aggr_state: rpmh clock last aggr state sent to RPMh
>> + * @valid_state_mask: mask to determine the state of the rpmh clock
>> + * @rpmh_client: handle used for rpmh communications
>> + * @dev: device to which it is attached
>> + * @peer: pointer to the clock rpmh sibling
>> + */
>> +struct clk_rpmh {
>> + struct clk_hw hw;
>> + const char *res_name;
>> + u8 div;
>
> Weird space here.
>
This is no longer required, removed.
>> + u32 res_addr;
>> + 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;
>> +};
>> +
>> +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) \
>> + static struct clk_rpmh _platform##_##_name_active; \
>> + static struct clk_rpmh _platform##_##_name = { \
>> + .res_name = _res_name, \
>> + .res_addr = _res_en_offset, \
>> + .res_on_val = _res_on, \
>> + .res_off_val = _res_off, \
>> + .peer = &_platform##_##_name_active, \
>> + .valid_state_mask = (BIT(RPMH_WAKE_ONLY_STATE) | \
>> + BIT(RPMH_ACTIVE_ONLY_STATE) | \
>> + BIT(RPMH_SLEEP_STATE)), \
>> + .hw.init = &(struct clk_init_data){ \
>> + .ops = &clk_rpmh_ops, \
>> + .name = #_name, \
>> + .parent_names = (const char *[]){ "xo_board" }, \
>
> This would be different for the xo_board and the xo_board / 2 clk names.
>
Would add parents "xo_board" & "xo_board_div".
>> + .num_parents = 1, \
>> + }, \
>> + }; \
>> + static struct clk_rpmh _platform##_##_name_active = { \
>> + .res_name = _res_name, \
>> + .res_addr = _res_en_offset, \
>> + .res_on_val = _res_on, \
>> + .res_off_val = _res_off, \
>> + .peer = &_platform##_##_name, \
>> + .valid_state_mask = (BIT(RPMH_WAKE_ONLY_STATE) | \
>> + BIT(RPMH_ACTIVE_ONLY_STATE)), \
>> + .hw.init = &(struct clk_init_data){ \
>> + .ops = &clk_rpmh_ops, \
>> + .name = #_name_active, \
>> + .parent_names = (const char *[]){ "xo_board" }, \
>> + .num_parents = 1, \
>> + }, \
>> + }
>> +
>> +#define DEFINE_CLK_RPMH_ARC(_platform, _name, _name_active, _res_name, \
>> + _res_on, _res_off) \
>> + __DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name, \
>> + CLK_RPMH_ARC_EN_OFFSET, _res_on, _res_off)
>> +
>> +#define DEFINE_CLK_RPMH_VRM(_platform, _name, _name_active, _res_name) \
>> + __DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name, \
>> + CLK_RPMH_VRM_EN_OFFSET, true, false)
>
> s/true, false/1, 0/ ?
>
Would take care of this too.
>> +
>> +static inline struct clk_rpmh *to_clk_rpmh(struct clk_hw *_hw)
>> +{
>> + return container_of(_hw, struct clk_rpmh, hw);
>> +}
>> +
>> +static inline bool has_state_changed(struct clk_rpmh *c, u32 state)
>> +{
>> + return (c->last_sent_aggr_state & BIT(state))
>> + != (c->aggr_state & BIT(state));
>> +}
>> +
>> +static int clk_rpmh_send_aggregate_command(struct clk_rpmh *c)
>> +{
>> + struct tcs_cmd cmd = { 0 };
>> + u32 cmd_state, on_val;
>> + enum rpmh_state state = RPMH_SLEEP_STATE;
>> + int ret;
>> +
>> + cmd.addr = c->res_addr;
>> + cmd_state = c->aggr_state;
>> + on_val = c->res_on_val;
>> +
>> + for (; state <= RPMH_ACTIVE_ONLY_STATE; state++) {
>> + if (has_state_changed(c, state)) {
>> + cmd.data = (cmd_state & BIT(state)) ? on_val : 0;
>
> Make this into an "if" please. Default is already 0 so just set bit if
> cmd_state & BIT(state).
>
Would take care of it.
>> + ret = rpmh_write_async(c->rpmh_client, state,
>> + &cmd, 1);
>> + if (ret) {
>> + dev_err(c->dev, "set %s state of %s failed: (%d)\n",
>> + !state ? "sleep" :
>> + state == RPMH_WAKE_ONLY_STATE ?
>> + "wake" : "active", c->res_name, ret);
>> + return ret;
>> + }
>> + }
>> + }
>> +
>> + c->last_sent_aggr_state = c->aggr_state;
>> + c->peer->last_sent_aggr_state = c->last_sent_aggr_state;
>> +
>> + return 0;
>> +}
>> +
>> +/*
>> + * Update state and aggregate state values based on enable value.
>> + */
>> +static int clk_rpmh_aggregate_state_send_command(struct clk_rpmh *c,
>> + bool enable)
>> +{
>> + int ret;
>> +
>> + 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;
>
> Why don't we print a WARN if it fails to enable?
>
I would add a WARN for enable failure too.
>> + } 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)
>> +{
>> + struct clk_rpmh *c = to_clk_rpmh(hw);
>> + int ret = 0;
>> +
>> + mutex_lock(&rpmh_clk_lock);
>> +
>> + if (!c->state)
>
> Can we push this into clk_rpmh_aggregate_state_send_command()? Something
> like:
>
> /* Nothing to do if already off or on */
> if (enable == c->state)
> return 0;
>
Thanks, would add these changes.
>> + ret = clk_rpmh_aggregate_state_send_command(c, true);
>> +
>> + mutex_unlock(&rpmh_clk_lock);
>> + return ret;
>> +};
>> +
>> +static void clk_rpmh_unprepare(struct clk_hw *hw)
>> +{
>> + struct clk_rpmh *c = to_clk_rpmh(hw);
>> +
>> + mutex_lock(&rpmh_clk_lock);
>> +
>> + if (c->state)
>> + clk_rpmh_aggregate_state_send_command(c, false);
>> +
>> + mutex_unlock(&rpmh_clk_lock);
>> +};
>> +
>> +static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw,
>> + unsigned long prate)
>> +{
>> + struct clk_rpmh *r = to_clk_rpmh(hw);
>> + unsigned long rate;
>> +
>> + /*
>> + * RPMh clocks have a fixed rate.
>> + */
>> + rate = prate;
>> + do_div(rate, r->div);
>> +
>> + return rate;
>> +}
>
> You shouldn't need any recalc rate in this driver.
>
Yes, these would be removed.
>> +
>> +static int clk_rpmh_probe(struct platform_device *pdev)
>> +{
>> + struct clk *clk;
>> + struct clk_hw_onecell_data *hw_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;
>
> Just do this when dev is defined please.
>
>> +
>> + ret = cmd_db_ready();
>> + if (ret) {
>> + if (ret != -EPROBE_DEFER)
>> + dev_err(dev, "Command DB not available (%d)\n", ret);
>> + return ret;
>> + }
>> +
>> + rpmh_client = rpmh_get_client(pdev);
>> + if (IS_ERR(rpmh_client)) {
>> + ret = PTR_ERR(rpmh_client);
>> + dev_err(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;
>> +
>> + hw_data = devm_kzalloc(dev,
>> + sizeof(*hw_data) + num_clks * sizeof(struct clk_hw *),
>> + GFP_KERNEL);
>> + if (!hw_data)
>> + return -ENOMEM;
>> +
>> + hw_data->num = num_clks;
>> +
>> + for (i = 0; i < num_clks; i++) {
>> + const char *clk_name;
>> + struct property *prop;
>> + const __be32 *p;
>> + u32 res_addr;
>> + int div, j = 0;
>> +
>> + rpmh_clk = to_clk_rpmh(hw_clks[i]);
>> + res_addr = cmd_db_read_addr(rpmh_clk->res_name);
>> + if (!res_addr) {
>> + dev_err(dev, "missing RPMh resource address for %s\n",
>> + rpmh_clk->res_name);
>> + ret = -ENODEV;
>> + goto err;
>> + }
>> + rpmh_clk->res_addr += res_addr;
>
> Thanks!
>
>> +
>> + rpmh_clk->rpmh_client = rpmh_client;
>> +
>> + /* default div for all clocks */
>> + if (!rpmh_clk->div)
>> + rpmh_clk->div = 1;
>> +
>> + of_property_for_each_u32(pdev->dev.of_node, "assigned-clk-divs",
>> + prop, p, div) {
>> + of_property_read_string_index(pdev->dev.of_node,
>> + "clk-output-names", j, &clk_name);
>> + if (!strcmp(clk_name, hw_clks[i]->init->name)) {
>> + rpmh_clk->div = div;
>> + rpmh_clk->peer->div = div;
>> + }
>> + j++;
>> + }
>> +
>> + 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;
>> + }
>> +
>> + rpmh_clk->dev = &pdev->dev;
>> + }
>> +
>> + ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
>
> Why not roll your own "getter" function and then just index directly
> into your array of rpmh clks for sdm845? Then we don't have to copy
> things from one place to another.
>
Added a new function.
>> + hw_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);
>> +
>> + return ret;
>> +}
>> +
> --
> 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] 12+ messages in thread
end of thread, other threads:[~2018-05-02 10:22 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-01 8:41 [v5 0/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver Taniya Das
2018-05-01 8:41 ` [v5 1/2] dt-bindings: clock: Introduce QCOM RPMh clock bindings Taniya Das
2018-05-01 21:10 ` Stephen Boyd
2018-05-01 21:10 ` Stephen Boyd
2018-05-01 21:10 ` Stephen Boyd
2018-05-01 8:41 ` [v5 2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver Taniya Das
2018-05-01 12:43 ` Rob Herring
2018-05-01 15:33 ` Taniya Das
2018-05-01 21:27 ` Stephen Boyd
2018-05-01 21:27 ` Stephen Boyd
2018-05-01 21:27 ` Stephen Boyd
2018-05-02 10:22 ` 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.