* [PATCH v3 0/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver
@ 2018-04-14 2:36 Taniya Das
2018-04-14 2:36 ` [PATCH v3 1/2] dt-bindings: clock: Introduce QCOM RPMh clock bindings Taniya Das
2018-04-14 2:36 ` [PATCH v3 2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver Taniya Das
0 siblings, 2 replies; 15+ messages in thread
From: Taniya Das @ 2018-04-14 2:36 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
[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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 1/2] dt-bindings: clock: Introduce QCOM RPMh clock bindings
2018-04-14 2:36 [PATCH v3 0/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver Taniya Das
@ 2018-04-14 2:36 ` Taniya Das
2018-04-16 17:39 ` Stephen Boyd
2018-04-16 20:45 ` Rob Herring
2018-04-14 2:36 ` [PATCH v3 2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver Taniya Das
1 sibling, 2 replies; 15+ messages in thread
From: Taniya Das @ 2018-04-14 2:36 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>
---
.../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
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..3c00765
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/qcom,rpmh-clk.txt
@@ -0,0 +1,22 @@
+Qualcomm Technologies, Inc. RPMh Clocks
+-------------------------------------------------------
+
+Resource Power Manager Hardened (RPMh) manages shared resources on
+some Qualcomm Technologies Inc. SoCs. It accepts clock requests from
+other hardware subsystems via RSC to control clocks.
+
+Required properties :
+- compatible : shall contain "qcom,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>;
+ };
+ };
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] 15+ messages in thread
* [PATCH v3 2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver
2018-04-14 2:36 [PATCH v3 0/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver Taniya Das
2018-04-14 2:36 ` [PATCH v3 1/2] dt-bindings: clock: Introduce QCOM RPMh clock bindings Taniya Das
@ 2018-04-14 2:36 ` Taniya Das
2018-04-16 17:38 ` Stephen Boyd
2018-04-17 23:57 ` [v3,2/2] " Matthias Kaehlcke
1 sibling, 2 replies; 15+ messages in thread
From: Taniya Das @ 2018-04-14 2:36 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, David Collins
Add the RPMh clock driver to control the RPMh managed clock resources on
some of the Qualcomm Technologies, Inc. SoCs.
Signed-off-by: David Collins <collinsd@codeaurora.org>
Signed-off-by: Amit Nischal <anischal@codeaurora.org>
---
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
diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index fbf4532..63c3372 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -112,6 +112,15 @@ config IPQ_GCC_8074
i2c, USB, SD/eMMC, etc. Select this for the root clock
of ipq8074.
+config QCOM_CLK_RPMH
+ tristate "RPMh Clock Driver"
+ depends on COMMON_CLK_QCOM && QCOM_RPMH
+ help
+ RPMh manages shared resources on some Qualcomm Technologies, Inc.
+ SoCs. It accepts requests from other hardware subsystems via RSC.
+ Say Y if you want to support the clocks exposed by RPMh on
+ platforms such as sdm845.
+
config MSM_GCC_8660
tristate "MSM8660 Global Clock Controller"
depends on COMMON_CLK_QCOM
diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
index 230332c..b2b5babf 100644
--- a/drivers/clk/qcom/Makefile
+++ b/drivers/clk/qcom/Makefile
@@ -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
diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
new file mode 100644
index 0000000..fa61284
--- /dev/null
+++ b/drivers/clk/qcom/clk-rpmh.c
@@ -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;
+};
+
+struct rpmh_cc {
+ struct clk_onecell_data data;
+ struct clk *clks[];
+};
+
+struct clk_rpmh_desc {
+ struct clk_hw **clks;
+ size_t num_clks;
+};
+
+static DEFINE_MUTEX(rpmh_clk_lock);
+
+#define __DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name, \
+ _res_en_offset, _res_on, _res_off, _rate) \
+ 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)
+{
+ 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;
+
+ 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;
+}
+
+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)
+{
+ struct clk_rpmh *c = to_clk_rpmh(hw);
+ int ret = 0;
+
+ mutex_lock(&rpmh_clk_lock);
+
+ if (c->state)
+ goto out;
+
+ ret = clk_rpmh_aggregate_state_send_command(c, true);
+out:
+ mutex_unlock(&rpmh_clk_lock);
+ return ret;
+};
+
+static void clk_rpmh_unprepare(struct clk_hw *hw)
+{
+ struct clk_rpmh *c = to_clk_rpmh(hw);
+
+ mutex_lock(&rpmh_clk_lock);
+
+ if (!c->state)
+ goto out;
+
+ clk_rpmh_aggregate_state_send_command(c, false);
+out:
+ mutex_unlock(&rpmh_clk_lock);
+ return;
+};
+
+static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ struct clk_rpmh *r = to_clk_rpmh(hw);
+
+ /*
+ * RPMh clocks have a fixed rate. Return static rate set
+ * at init time.
+ */
+ return r->rate;
+}
+
+static const struct clk_ops clk_rpmh_ops = {
+ .prepare = clk_rpmh_prepare,
+ .unprepare = clk_rpmh_unprepare,
+ .recalc_rate = clk_rpmh_recalc_rate,
+};
+
+/* Resource name must match resource id present in cmd-db. */
+DEFINE_CLK_RPMH_ARC(sdm845, bi_tcxo, bi_tcxo_ao, "xo.lvl", 0x3, 0x0, 19200000);
+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");
--
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] 15+ messages in thread
* Re: [PATCH v3 2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver
2018-04-14 2:36 ` [PATCH v3 2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver Taniya Das
2018-04-16 17:38 ` Stephen Boyd
@ 2018-04-16 17:38 ` Stephen Boyd
1 sibling, 0 replies; 15+ messages in thread
From: Stephen Boyd @ 2018-04-16 17:38 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, David Collins
Quoting Taniya Das (2018-04-13 19:36:41)
> Add the RPMh clock driver to control the RPMh managed clock resources on
> some of the Qualcomm Technologies, Inc. SoCs.
>
> Signed-off-by: David Collins <collinsd@codeaurora.org>
> Signed-off-by: Amit Nischal <anischal@codeaurora.org>
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.
> ---
> 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
>
> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index fbf4532..63c3372 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -112,6 +112,15 @@ config IPQ_GCC_8074
> i2c, USB, SD/eMMC, etc. Select this for the root clock
> of ipq8074.
>
> +config 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?
> +
Can this Kconfig be put into alphabetical order in this file?
> config MSM_GCC_8660
> tristate "MSM8660 Global Clock Controller"
> depends on COMMON_CLK_QCOM
> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
> index 230332c..b2b5babf 100644
> --- a/drivers/clk/qcom/Makefile
> +++ b/drivers/clk/qcom/Makefile
> @@ -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
> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
> new file mode 100644
> index 0000000..fa61284
> --- /dev/null
> +++ b/drivers/clk/qcom/clk-rpmh.c
> @@ -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?
> +#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))
Is there a reason these aren't inlined into the one place they're used?
> +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.
> + u32 res_on_val;
> + u32 res_off_val;
Is this used?
> + 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?
> +
> +struct rpmh_cc {
> + struct clk_onecell_data data;
> + struct clk *clks[];
> +};
Hopefully this structure isn't needed.
> +
> +struct clk_rpmh_desc {
> + struct clk_hw **clks;
> + size_t num_clks;
> +};
> +
[...]
> +
> +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.
> +}
> +
> +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;
> +}
> +
> +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.
> + 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)
> + 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.
> +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:
Same comment.
> + 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;
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?).
> +}
> +
> +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;
Please just return error here instead of goto.
> + }
> + 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.
> +
> + 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);
This isn't a useful error message either.
> + 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.
> + 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);
Please remove this error message.
> + 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;
> +}
Hopefully this whole remove function goes away.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver
@ 2018-04-16 17:38 ` Stephen Boyd
0 siblings, 0 replies; 15+ messages in thread
From: Stephen Boyd @ 2018-04-16 17:38 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, David Collins
Quoting Taniya Das (2018-04-13 19:36:41)
> Add the RPMh clock driver to control the RPMh managed clock resources on
> some of the Qualcomm Technologies, Inc. SoCs.
>
> Signed-off-by: David Collins <collinsd@codeaurora.org>
> Signed-off-by: Amit Nischal <anischal@codeaurora.org>
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.
> ---
> 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
>
> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index fbf4532..63c3372 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -112,6 +112,15 @@ config IPQ_GCC_8074
> i2c, USB, SD/eMMC, etc. Select this for the root clock
> of ipq8074.
>
> +config 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?
> +
Can this Kconfig be put into alphabetical order in this file?
> config MSM_GCC_8660
> tristate "MSM8660 Global Clock Controller"
> depends on COMMON_CLK_QCOM
> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
> index 230332c..b2b5babf 100644
> --- a/drivers/clk/qcom/Makefile
> +++ b/drivers/clk/qcom/Makefile
> @@ -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
> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
> new file mode 100644
> index 0000000..fa61284
> --- /dev/null
> +++ b/drivers/clk/qcom/clk-rpmh.c
> @@ -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?
> +#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))
Is there a reason these aren't inlined into the one place they're used?
> +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.
> + u32 res_on_val;
> + u32 res_off_val;
Is this used?
> + 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?
> +
> +struct rpmh_cc {
> + struct clk_onecell_data data;
> + struct clk *clks[];
> +};
Hopefully this structure isn't needed.
> +
> +struct clk_rpmh_desc {
> + struct clk_hw **clks;
> + size_t num_clks;
> +};
> +
[...]
> +
> +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.
> +}
> +
> +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;
> +}
> +
> +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.
> + 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)
> + 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.
> +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:
Same comment.
> + 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;
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?).
> +}
> +
> +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;
Please just return error here instead of goto.
> + }
> + 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.
> +
> + 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);
This isn't a useful error message either.
> + 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.
> + 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);
Please remove this error message.
> + 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;
> +}
Hopefully this whole remove function goes away.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver
@ 2018-04-16 17:38 ` Stephen Boyd
0 siblings, 0 replies; 15+ messages in thread
From: Stephen Boyd @ 2018-04-16 17:38 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, David Collins
Quoting Taniya Das (2018-04-13 19:36:41)
> Add the RPMh clock driver to control the RPMh managed clock resources on
> some of the Qualcomm Technologies, Inc. SoCs.
> =
> Signed-off-by: David Collins <collinsd@codeaurora.org>
> Signed-off-by: Amit Nischal <anischal@codeaurora.org>
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.
> ---
> 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
> =
> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index fbf4532..63c3372 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -112,6 +112,15 @@ config IPQ_GCC_8074
> i2c, USB, SD/eMMC, etc. Select this for the root clock
> of ipq8074.
> =
> +config 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?
> +
Can this Kconfig be put into alphabetical order in this file?
> config MSM_GCC_8660
> tristate "MSM8660 Global Clock Controller"
> depends on COMMON_CLK_QCOM
> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
> index 230332c..b2b5babf 100644
> --- a/drivers/clk/qcom/Makefile
> +++ b/drivers/clk/qcom/Makefile
> @@ -36,5 +36,6 @@ obj-$(CONFIG_MSM_MMCC_8996) +=3D mmcc-msm8996.o
> obj-$(CONFIG_QCOM_A53PLL) +=3D a53-pll.o
> obj-$(CONFIG_QCOM_CLK_APCS_MSM8916) +=3D apcs-msm8916.o
> obj-$(CONFIG_QCOM_CLK_RPM) +=3D clk-rpm.o
> +obj-$(CONFIG_QCOM_CLK_RPMH) +=3D clk-rpmh.o
> obj-$(CONFIG_QCOM_CLK_SMD_RPM) +=3D clk-smd-rpm.o
> obj-$(CONFIG_SPMI_PMIC_CLKDIV) +=3D 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..fa61284
> --- /dev/null
> +++ b/drivers/clk/qcom/clk-rpmh.c
> @@ -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?
> +#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))
Is there a reason these aren't inlined into the one place they're used?
> +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.
> + u32 res_on_val;
> + u32 res_off_val;
Is this used?
> + 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?
> +
> +struct rpmh_cc {
> + struct clk_onecell_data data;
> + struct clk *clks[];
> +};
Hopefully this structure isn't needed.
> +
> +struct clk_rpmh_desc {
> + struct clk_hw **clks;
> + size_t num_clks;
> +};
> +
[...]
> +
> +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)));
Too many parenthesis here.
> +}
> +
> +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 =3D 0;
> +
> + cmd.addr =3D c->res_addr + c->res_en_offset;
> + 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;
> + 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_STAT=
E) ?
> + "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;
> +}
> +
> +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.
> + 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;
> + } 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)
> + goto out;
> +
> + ret =3D clk_rpmh_aggregate_state_send_command(c, true);
Rewrite this as:
if (!c->state)
ret =3D clk_rpmh_aggregate_state_send_command(c, true);
=
and drop the goto.
> +out:
> + 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)
> + goto out;
> +
> + clk_rpmh_aggregate_state_send_command(c, false);
> +out:
Same comment.
> + 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 =3D 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?).
> +}
> +
> +static const struct clk_ops clk_rpmh_ops =3D {
> + .prepare =3D clk_rpmh_prepare,
> + .unprepare =3D clk_rpmh_unprepare,
> + .recalc_rate =3D 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, 192=
00000);
> +DEFINE_CLK_RPMH_VRM(sdm845, ln_bb_clk2, ln_bb_clk2_ao, "lnbclka2", 19200=
000);
> +DEFINE_CLK_RPMH_VRM(sdm845, ln_bb_clk3, ln_bb_clk3_ao, "lnbclka3", 19200=
000);
> +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[] =3D {
> + [RPMH_CXO_CLK] =3D &sdm845_bi_tcxo.hw,
> + [RPMH_CXO_CLK_A] =3D &sdm845_bi_tcxo_ao.hw,
> + [RPMH_LN_BB_CLK2] =3D &sdm845_ln_bb_clk2.hw,
> + [RPMH_LN_BB_CLK2_A] =3D &sdm845_ln_bb_clk2_ao.hw,
> + [RPMH_LN_BB_CLK3] =3D &sdm845_ln_bb_clk3.hw,
> + [RPMH_LN_BB_CLK3_A] =3D &sdm845_ln_bb_clk3_ao.hw,
> + [RPMH_RF_CLK1] =3D &sdm845_rf_clk1.hw,
> + [RPMH_RF_CLK1_A] =3D &sdm845_rf_clk1_ao.hw,
> + [RPMH_RF_CLK2] =3D &sdm845_rf_clk2.hw,
> + [RPMH_RF_CLK2_A] =3D &sdm845_rf_clk2_ao.hw,
> + [RPMH_RF_CLK3] =3D &sdm845_rf_clk3.hw,
> + [RPMH_RF_CLK3_A] =3D &sdm845_rf_clk3_ao.hw,
> +};
> +
> +static const struct clk_rpmh_desc clk_rpmh_sdm845 =3D {
> + .clks =3D sdm845_rpmh_clocks,
> + .num_clks =3D 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 =3D &pdev->dev;
> +
> + ret =3D cmd_db_ready();
> + if (ret) {
> + if (ret !=3D -EPROBE_DEFER) {
> + dev_err(dev, "Command DB not available (%d)\n", r=
et);
> + goto fail;
Please just return error here instead of goto.
> + }
> + 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;
> + }
I hope we get rid of rpmh_get_client() still.
> +
> + desc =3D of_device_get_match_data(&pdev->dev);
> + hw_clks =3D desc->clks;
> + num_clks =3D desc->num_clks;
> +
> + rcc =3D devm_kzalloc(&pdev->dev, sizeof(*rcc) + sizeof(*clks) * n=
um_clks,
> + GFP_KERNEL);
> + if (!rcc) {
> + ret =3D -ENOMEM;
> + goto err;
> + }
> +
> + clks =3D rcc->clks;
> + data =3D &rcc->data;
> + data->clks =3D clks;
> + data->clk_num =3D num_clks;
> +
> + for (i =3D 0; i < num_clks; i++) {
> + rpmh_clk =3D to_clk_rpmh(hw_clks[i]);
> + rpmh_clk->res_addr =3D cmd_db_read_addr(rpmh_clk->res_nam=
e);
> + if (!rpmh_clk->res_addr) {
> + dev_err(dev, "missing RPMh resource address for %=
s\n",
> + rpmh_clk->res_name);
> + ret =3D -ENODEV;
> + goto err;
> + }
> +
> + rpmh_clk->rpmh_client =3D rpmh_client;
> +
> + 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);
This isn't a useful error message either.
> + goto err;
> + }
> +
> + clks[i] =3D clk;
> + rpmh_clk->dev =3D &pdev->dev;
> + }
> +
> + ret =3D 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.
> + 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);
Please remove this error message.
> + return ret;
> +}
> +
> +static int clk_rpmh_remove(struct platform_device *pdev)
> +{
> + const struct clk_rpmh_desc *desc =3D
> + of_device_get_match_data(&pdev->dev);
> + struct clk_hw **hw_clks =3D desc->clks;
> + struct clk_rpmh *rpmh_clk =3D 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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: clock: Introduce QCOM RPMh clock bindings
2018-04-14 2:36 ` [PATCH v3 1/2] dt-bindings: clock: Introduce QCOM RPMh clock bindings Taniya Das
2018-04-16 17:39 ` Stephen Boyd
@ 2018-04-16 17:39 ` Stephen Boyd
1 sibling, 0 replies; 15+ messages in thread
From: Stephen Boyd @ 2018-04-16 17:39 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-04-13 19:36:40)
> 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>
Is Amit the author? Needs a "From: Amit .." then.
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: clock: Introduce QCOM RPMh clock bindings
@ 2018-04-16 17:39 ` Stephen Boyd
0 siblings, 0 replies; 15+ messages in thread
From: Stephen Boyd @ 2018-04-16 17:39 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-04-13 19:36:40)
> 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>
Is Amit the author? Needs a "From: Amit .." then.
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: clock: Introduce QCOM RPMh clock bindings
@ 2018-04-16 17:39 ` Stephen Boyd
0 siblings, 0 replies; 15+ messages in thread
From: Stephen Boyd @ 2018-04-16 17:39 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-04-13 19:36:40)
> 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>
Is Amit the author? Needs a "From: Amit .." then.
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: clock: Introduce QCOM RPMh clock bindings
2018-04-14 2:36 ` [PATCH v3 1/2] dt-bindings: clock: Introduce QCOM RPMh clock bindings Taniya Das
2018-04-16 17:39 ` Stephen Boyd
@ 2018-04-16 20:45 ` Rob Herring
1 sibling, 0 replies; 15+ messages in thread
From: Rob Herring @ 2018-04-16 20:45 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 Sat, Apr 14, 2018 at 08:06:40AM +0530, Taniya Das wrote:
> Add RPMh clock device bindings for Qualcomm Technology Inc's SoCs. These
> devices would be used for communicating resource state requests to control
> the clocks managed by RPMh.
>
> Signed-off-by: Amit Nischal <anischal@codeaurora.org>
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> ---
> .../devicetree/bindings/clock/qcom,rpmh-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
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver
2018-04-16 17:38 ` Stephen Boyd
(?)
(?)
@ 2018-04-16 21:47 ` David Collins
-1 siblings, 0 replies; 15+ messages in thread
From: David Collins @ 2018-04-16 21:47 UTC (permalink / raw)
To: Stephen Boyd, 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
Hello Taniya,
On 04/16/2018 10:38 AM, Stephen Boyd wrote:
> Quoting Taniya Das (2018-04-13 19:36:41)
>> Add the RPMh clock driver to control the RPMh managed clock resources on
>> some of the Qualcomm Technologies, Inc. SoCs.
>>
>> Signed-off-by: David Collins <collinsd@codeaurora.org>
>> Signed-off-by: Amit Nischal <anischal@codeaurora.org>
>
> 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
[1]:
https://source.codeaurora.org/quic/la/kernel/msm-4.9/commit/?id=11d3623c37afb945ce36755501812a44efeb6cd9
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [v3,2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver
2018-04-14 2:36 ` [PATCH v3 2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver Taniya Das
2018-04-16 17:38 ` Stephen Boyd
@ 2018-04-17 23:57 ` Matthias Kaehlcke
1 sibling, 0 replies; 15+ messages in thread
From: Matthias Kaehlcke @ 2018-04-17 23:57 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, David Collins
On Sat, Apr 14, 2018 at 08:06:41AM +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: David Collins <collinsd@codeaurora.org>
> Signed-off-by: Amit Nischal <anischal@codeaurora.org>
> ---
> 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
>
> --
> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index fbf4532..63c3372 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -112,6 +112,15 @@ config IPQ_GCC_8074
> i2c, USB, SD/eMMC, etc. Select this for the root clock
> of ipq8074.
>
> +config QCOM_CLK_RPMH
> + tristate "RPMh Clock Driver"
> + depends on COMMON_CLK_QCOM && QCOM_RPMH
> + help
> + RPMh manages shared resources on some Qualcomm Technologies, Inc.
> + SoCs. It accepts requests from other hardware subsystems via RSC.
> + Say Y if you want to support the clocks exposed by RPMh on
> + platforms such as sdm845.
> +
> config MSM_GCC_8660
> tristate "MSM8660 Global Clock Controller"
> depends on COMMON_CLK_QCOM
> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
> index 230332c..b2b5babf 100644
> --- a/drivers/clk/qcom/Makefile
> +++ b/drivers/clk/qcom/Makefile
> @@ -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
> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
> new file mode 100644
> index 0000000..fa61284
> --- /dev/null
> +++ b/drivers/clk/qcom/clk-rpmh.c
> @@ -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;
> +};
> +
> +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?
> +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)
> +{
> + 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.
> +
> + 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.
> + if (has_state_changed(c, state)) {
> + cmd.data = (cmd_state & BIT(state)) ? on_val : 0;
~
Shouldn't this be c->res_off_val?
> + 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)
> +{
> + 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?
> +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");
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver
2018-04-16 17:38 ` Stephen Boyd
` (2 preceding siblings ...)
(?)
@ 2018-04-23 16:50 ` Taniya Das
2018-04-24 15:40 ` Stephen Boyd
-1 siblings, 1 reply; 15+ messages in thread
From: Taniya Das @ 2018-04-23 16:50 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, David Collins
Thanks Stephen for the review comments.
On 4/16/2018 11:08 PM, Stephen Boyd wrote:
> Quoting Taniya Das (2018-04-13 19:36:41)
>> Add the RPMh clock driver to control the RPMh managed clock resources on
>> some of the Qualcomm Technologies, Inc. SoCs.
>>
>> Signed-off-by: David Collins <collinsd@codeaurora.org>
>> Signed-off-by: Amit Nischal <anischal@codeaurora.org>
>
> 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.
>
>> ---
>> 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
>>
>> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
>> index fbf4532..63c3372 100644
>> --- a/drivers/clk/qcom/Kconfig
>> +++ b/drivers/clk/qcom/Kconfig
>> @@ -112,6 +112,15 @@ config IPQ_GCC_8074
>> i2c, USB, SD/eMMC, etc. Select this for the root clock
>> of ipq8074.
>>
>> +config 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?
>
>> +
>
Would use SDM845.
> Can this Kconfig be put into alphabetical order in this file?
>
Would place it with the QCOM_* configs in this file.
>> config MSM_GCC_8660
>> tristate "MSM8660 Global Clock Controller"
>> depends on COMMON_CLK_QCOM
>> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
>> index 230332c..b2b5babf 100644
>> --- a/drivers/clk/qcom/Makefile
>> +++ b/drivers/clk/qcom/Makefile
>> @@ -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
>> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
>> new file mode 100644
>> index 0000000..fa61284
>> --- /dev/null
>> +++ b/drivers/clk/qcom/clk-rpmh.c
>> @@ -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?
>
Would remove this include.
>> +#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))
>
> Is there a reason these aren't inlined into the one place they're used?
>
Would clean the above.
>> +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.
>
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.
>> + u32 res_on_val;
>> + u32 res_off_val;
>
> Is this used?
Yes the above are used.
>
>> + 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?
>
Sure will add the same.
>> +
>> +struct rpmh_cc {
>> + struct clk_onecell_data data;
>> + struct clk *clks[];
>> +};
>
> Hopefully this structure isn't needed.
>
Yes, would remove the above.
>> +
>> +struct clk_rpmh_desc {
>> + struct clk_hw **clks;
>> + size_t num_clks;
>> +};
>> +
> [...]
>> +
>> +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.
>> +}
>> +
>> +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;
>> +}
>> +
>> +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.
>> + 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)
>> + 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.
>
Would cleanup to remove goto.
>> +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:
>
> Same comment.
>
>> + 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;
>
> 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?).
>
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.
>> +}
>> +
>> +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;
>
> Please just return error here instead of goto.
>
Sure would clean this.
>> + }
>> + 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.
> >> +
>> + 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);
>
> This isn't a useful error message either.
>
>> + 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.
>
Sure would move to use devm & would register clk_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);
>> +fail:
>> + dev_err(dev, "Error registering RPMh Clock driver (%d)\n", ret);
>
> Please remove this error message.
>
Thanks would remove the above.
>> + 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;
>> +}
>
> Hopefully this whole remove function goes away.
> We would still require the rpmh_release code.
--
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] 15+ messages in thread
* Re: [PATCH v3 2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver
2018-04-23 16:50 ` Taniya Das
@ 2018-04-24 15:40 ` Stephen Boyd
0 siblings, 0 replies; 15+ messages in thread
From: Stephen Boyd @ 2018-04-24 15:40 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, David Collins
Quoting Taniya Das (2018-04-23 09:50:22)
> On 4/16/2018 11:08 PM, Stephen Boyd wrote:
> > Quoting Taniya Das (2018-04-13 19:36:41)
>
> >> +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.
> >
>
> 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.
Yes. But why can't we store the combination of the two?
>
> >> + 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.
> >
> >> + 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?
> >
> Sure will add the same.
Great! Hopefully that clarifies things.
> >> + /*
> >> + * 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?).
> >
>
> 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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver
@ 2018-04-24 15:40 ` Stephen Boyd
0 siblings, 0 replies; 15+ messages in thread
From: Stephen Boyd @ 2018-04-24 15:40 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, David Collins
Quoting Taniya Das (2018-04-23 09:50:22)
> On 4/16/2018 11:08 PM, Stephen Boyd wrote:
> > Quoting Taniya Das (2018-04-13 19:36:41)
> =
> >> +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.
> > =
> =
> 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.
Yes. But why can't we store the combination of the two?
> =
> >> + 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.
> > =
> >> + 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?
> > =
> Sure will add the same.
Great! Hopefully that clarifies things.
> >> + /*
> >> + * 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?).
> > =
> =
> 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.
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2018-04-24 15:40 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-14 2:36 [PATCH v3 0/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver Taniya Das
2018-04-14 2:36 ` [PATCH v3 1/2] dt-bindings: clock: Introduce QCOM RPMh clock bindings Taniya Das
2018-04-16 17:39 ` Stephen Boyd
2018-04-16 17:39 ` Stephen Boyd
2018-04-16 17:39 ` Stephen Boyd
2018-04-16 20:45 ` Rob Herring
2018-04-14 2:36 ` [PATCH v3 2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver Taniya Das
2018-04-16 17:38 ` Stephen Boyd
2018-04-16 17:38 ` Stephen Boyd
2018-04-16 17:38 ` Stephen Boyd
2018-04-16 21:47 ` David Collins
2018-04-23 16:50 ` Taniya Das
2018-04-24 15:40 ` Stephen Boyd
2018-04-24 15:40 ` Stephen Boyd
2018-04-17 23:57 ` [v3,2/2] " Matthias Kaehlcke
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.