Re: [PATCH v2 4/4] clk: qcom: lpass: Add support for LPASS clock controller for SC7180

From: Taniya Das
Date: Wed Jun 10 2020 - 13:12:07 EST


Thanks for your review.

On 5/27/2020 8:40 AM, Stephen Boyd wrote:
Quoting Taniya Das (2020-05-17 02:22:24)
diff --git a/drivers/clk/qcom/lpasscorecc-sc7180.c b/drivers/clk/qcom/lpasscorecc-sc7180.c
new file mode 100644
index 0000000..86e3599
--- /dev/null
+++ b/drivers/clk/qcom/lpasscorecc-sc7180.c
@@ -0,0 +1,479 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2020, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/pm_clock.h>
+#include <linux/pm_runtime.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+
+#include <dt-bindings/clock/qcom,lpasscorecc-sc7180.h>
+
+#include "clk-alpha-pll.h"
+#include "clk-branch.h"
+#include "clk-rcg.h"
+#include "clk-regmap.h"
+#include "common.h"
+#include "gdsc.h"
+
+enum {
+ P_BI_TCXO,
+ P_LPASS_LPAAUDIO_DIG_PLL_OUT_ODD,
+ P_SLEEP_CLK,
+};
+
+static struct pll_vco fabia_vco[] = {
+ { 249600000, 2000000000, 0 },
+};
+
+static const struct alpha_pll_config lpass_lpaaudio_dig_pll_config = {
+ .l = 0x20,
[...]
+
+static struct regmap_config lpass_core_cc_sc7180_regmap_config = {
+ .reg_bits = 32,
+ .reg_stride = 4,
+ .val_bits = 32,
+ .fast_io = true,
+};
+
+static const struct qcom_cc_desc lpass_core_hm_sc7180_desc = {
+ .config = &lpass_core_cc_sc7180_regmap_config,
+ .gdscs = lpass_core_hm_sc7180_gdscs,
+ .num_gdscs = ARRAY_SIZE(lpass_core_hm_sc7180_gdscs),
+};
+
+static const struct qcom_cc_desc lpass_core_cc_sc7180_desc = {
+ .config = &lpass_core_cc_sc7180_regmap_config,
+ .clks = lpass_core_cc_sc7180_clocks,
+ .num_clks = ARRAY_SIZE(lpass_core_cc_sc7180_clocks),
+};
+
+static const struct qcom_cc_desc lpass_audio_hm_sc7180_desc = {
+ .config = &lpass_core_cc_sc7180_regmap_config,
+ .gdscs = lpass_audio_hm_sc7180_gdscs,
+ .num_gdscs = ARRAY_SIZE(lpass_audio_hm_sc7180_gdscs),
+};
+
+

Drop double newline please.


Done.

+static int lpass_core_cc_sc7180_probe(struct platform_device *pdev)
+{
+ const struct qcom_cc_desc *desc;
+ struct regmap *regmap;
+ int ret;
+
+ lpass_core_cc_sc7180_regmap_config.name = "lpass_audio_cc";
+ desc = &lpass_audio_hm_sc7180_desc;
+ ret = qcom_cc_probe_by_index(pdev, 1, desc);
+ if (ret)
+ return ret;
+
+ lpass_core_cc_sc7180_regmap_config.name = "lpass_core_cc";
+ regmap = qcom_cc_map(pdev, &lpass_core_cc_sc7180_desc);
+ if (IS_ERR(regmap))
+ return PTR_ERR(regmap);
+
+ /*
+ * Keep the CLK always-ON

Why? Presumably to make sure we can access the lpass sysnoc path all the
time?


This is an always ON clock from HW, just making sure to keep it enabled.

+ * LPASS_AUDIO_CORE_SYSNOC_SWAY_CORE_CLK
+ */
+ regmap_update_bits(regmap, 0x24000, BIT(0), BIT(0));
+
+ /* PLL settings */
+ regmap_write(regmap, 0x1008, 0x20);
+ regmap_update_bits(regmap, 0x1014, BIT(0), BIT(0));
+
+ clk_fabia_pll_configure(&lpass_lpaaudio_dig_pll, regmap,
+ &lpass_lpaaudio_dig_pll_config);
+
+ return qcom_cc_really_probe(pdev, &lpass_core_cc_sc7180_desc, regmap);
+}
+
+static int lpass_hm_core_probe(struct platform_device *pdev)
+{
+ const struct qcom_cc_desc *desc;
+ int ret;
+
+ lpass_core_cc_sc7180_regmap_config.name = "lpass_hm_core";
+ desc = &lpass_core_hm_sc7180_desc;
+
+ return qcom_cc_probe_by_index(pdev, 0, desc);
+}
+
+static const struct of_device_id lpass_core_cc_sc7180_match_table[] = {
+ {
+ .compatible = "qcom,sc7180-lpasshm",
+ .data = lpass_hm_core_probe,
+ },
+ {
+ .compatible = "qcom,sc7180-lpasscorecc",
+ .data = lpass_core_cc_sc7180_probe,
+ },
+ { }
+};
+MODULE_DEVICE_TABLE(of, lpass_core_cc_sc7180_match_table);
+
+static int lpass_core_sc7180_probe(struct platform_device *pdev)
+{
+ int (*clk_probe)(struct platform_device *p);
+ int ret;
+
+ pm_runtime_enable(&pdev->dev);
+ ret = pm_clk_create(&pdev->dev);
+ if (ret)
+ return ret;
+
+ ret = pm_clk_add(&pdev->dev, "gcc_lpass_sway");
+ if (ret < 0) {
+ dev_err(&pdev->dev, "failed to acquire iface clock\n");

Can the clk name be 'iface' if it's actually the interface clk?
"gcc_lpass_sway" looks to be the actual clk name which we shouldn't care
about here. It should be whatever clk name we consider it to be, which
would mean iface probably.


Yes would use "iface".

+ goto disable_pm_runtime;
+ }
+
+ clk_probe = of_device_get_match_data(&pdev->dev);
+ if (!clk_probe)
+ return -EINVAL;
+
+ ret = clk_probe(pdev);
+ if (ret)
+ goto destroy_pm_clk;
+
+ return 0;

--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.

--