[PATCH v2 0/2] Add support for LPASS clock controller for SDM845

[PATCH v2 0/2] Add support for LPASS clock controller for SDM845

[Taniya Das]

 [v2]
  * Make gcc_lpass_sway_clk static.
  * Remove using child nodes and use reg-names to differentiate various
    domains of LPASS CC.

Add support for the lpass clock controller found on SDM845 based devices.
This would allow lpass peripheral loader drivers to control the clocks to
bring the subsystem out of reset.

Taniya Das (2):
  dt-bindings: clock: Introduce QCOM LPASS clock bindings
  clk: qcom: Add lpass clock controller driver for SDM845

 .../devicetree/bindings/clock/qcom,lpasscc.txt     |  31 +++
 drivers/clk/qcom/Kconfig                           |   9 +
 drivers/clk/qcom/lpasscc-sdm845.c                  | 243 +++++++++++++++++++++
 include/dt-bindings/clock/qcom,lpass-sdm845.h      |  18 ++
 4 files changed, 301 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/qcom,lpasscc.txt
 create mode 100644 drivers/clk/qcom/lpasscc-sdm845.c
 create mode 100644 include/dt-bindings/clock/qcom,lpass-sdm845.h

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

[PATCH v2 1/2] dt-bindings: clock: Introduce QCOM LPASS clock bindings

[Taniya Das]

Add device tree bindings for Low Power Audio subsystem clock controller for
Qualcomm Technology Inc's SDM845 SoCs.

Signed-off-by: Taniya Das <tdas@codeaurora.org>
---
 .../devicetree/bindings/clock/qcom,lpasscc.txt     | 22 ++++++++++++++++++++++
 include/dt-bindings/clock/qcom,lpass-sdm845.h      | 18 ++++++++++++++++++
 2 files changed, 40 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/qcom,lpasscc.txt
 create mode 100644 include/dt-bindings/clock/qcom,lpass-sdm845.h

diff --git a/Documentation/devicetree/bindings/clock/qcom,lpasscc.txt b/Documentation/devicetree/bindings/clock/qcom,lpasscc.txt
new file mode 100644
index 0000000..fe7378b
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/qcom,lpasscc.txt
@@ -0,0 +1,22 @@
+Qualcomm LPASS Clock Controller Binding
+-----------------------------------------------
+
+Required properties :
+- compatible	: shall contain "qcom,sdm845-lpasscc"
+- #clock-cells	: from common clock binding, shall contain 1.
+- reg		: shall contain base register address and size.
+- reg-names	: shall contain the register names of LPASS domain
+			"lpass_gcc", "lpass_cc", "lpass_qdsp6ss".
+
+Example:
+
+The below node has to be defined in the cases where the LPASS peripheral loader
+would bring the subsystem out of reset.
+
+	lpasscc: clock-controller {
+		compatible = "qcom,sdm845-lpasscc";
+		reg = <0x00147000 0x20>, <0x17014000 0x1f004>,
+			<0x17300020 0x20>;
+		reg-names = "lpass_gcc", "lpass_cc", "lpass_qdsp6ss";
+		#clock-cells = <1>;
+	};
diff --git a/include/dt-bindings/clock/qcom,lpass-sdm845.h b/include/dt-bindings/clock/qcom,lpass-sdm845.h
new file mode 100644
index 0000000..b9d816e
--- /dev/null
+++ b/include/dt-bindings/clock/qcom,lpass-sdm845.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+#ifndef _DT_BINDINGS_CLK_SDM_LPASS_SDM845_H
+#define _DT_BINDINGS_CLK_SDM_LPASS_SDM845_H
+
+#define GCC_LPASS_Q6_AXI_CLK				0
+#define GCC_LPASS_SWAY_CLK				1
+#define LPASS_AUDIO_WRAPPER_AON_CLK			2
+#define LPASS_Q6SS_AHBM_AON_CLK				3
+#define LPASS_Q6SS_AHBS_AON_CLK				4
+#define LPASS_QDSP6SS_XO_CLK				5
+#define LPASS_QDSP6SS_SLEEP_CLK				6
+#define LPASS_QDSP6SS_CORE_CLK				7
+
+#endif
--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the  Linux Foundation.

[PATCH v2 2/2] clk: qcom: Add lpass clock controller driver for SDM845

[Taniya Das]

Add support for the lpass clock controller found on SDM845 based devices.
This would allow lpass peripheral loader drivers to control the clocks to
bring the subsystem out of reset.

Signed-off-by: Taniya Das <tdas@codeaurora.org>
---
 drivers/clk/qcom/Kconfig          |   9 ++
 drivers/clk/qcom/lpasscc-sdm845.c | 243 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 252 insertions(+)
 create mode 100644 drivers/clk/qcom/lpasscc-sdm845.c

diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index 9c3480d..06b3f2e 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -245,6 +245,15 @@ config SDM_VIDEOCC_845
 	  Say Y if you want to support video devices and functionality such as
 	  video encode and decode.

+config SDM_LPASSCC_845
+	tristate "SDM845 LPASS Clock Controller"
+	depends on COMMON_CLK_QCOM
+	select SDM_GCC_845
+	help
+	  Support for the LPASS clock controller on SDM845 devices.
+	  Say Y if you want to use the LPASS branch clocks of the LPASS clock
+	  controller to reset the LPASS subsystem.
+
 config SPMI_PMIC_CLKDIV
 	tristate "SPMI PMIC clkdiv Support"
 	depends on (COMMON_CLK_QCOM && SPMI) || COMPILE_TEST
diff --git a/drivers/clk/qcom/lpasscc-sdm845.c b/drivers/clk/qcom/lpasscc-sdm845.c
new file mode 100644
index 0000000..5285b26
--- /dev/null
+++ b/drivers/clk/qcom/lpasscc-sdm845.c
@@ -0,0 +1,243 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/bitops.h>
+#include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/regmap.h>
+
+#include <dt-bindings/clock/qcom,lpass-sdm845.h>
+
+#include "clk-regmap.h"
+#include "clk-branch.h"
+#include "common.h"
+
+static struct clk_branch gcc_lpass_q6_axi_clk = {
+	.halt_reg = 0x0,
+	.halt_check = BRANCH_HALT,
+	.clkr = {
+		.enable_reg = 0x0,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "gcc_lpass_q6_axi_clk",
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
+static struct clk_branch gcc_lpass_sway_clk = {
+	.halt_reg = 0x8,
+	.halt_check = BRANCH_HALT,
+	.clkr = {
+		.enable_reg = 0x8,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "gcc_lpass_sway_clk",
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
+static struct clk_branch lpass_audio_wrapper_aon_clk = {
+	.halt_reg = 0x098,
+	.halt_check = BRANCH_VOTED,
+	.clkr = {
+		.enable_reg = 0x098,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "lpass_audio_wrapper_aon_clk",
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
+static struct clk_branch lpass_q6ss_ahbm_aon_clk = {
+	.halt_reg = 0x12000,
+	.halt_check = BRANCH_VOTED,
+	.clkr = {
+		.enable_reg = 0x12000,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "lpass_q6ss_ahbm_aon_clk",
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
+static struct clk_branch lpass_q6ss_ahbs_aon_clk = {
+	.halt_reg = 0x1f000,
+	.halt_check = BRANCH_VOTED,
+	.clkr = {
+		.enable_reg = 0x1f000,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "lpass_q6ss_ahbs_aon_clk",
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
+static struct clk_branch lpass_qdsp6ss_xo_clk = {
+	.halt_reg = 0x18,
+	.halt_check = BRANCH_HALT_SKIP,
+	.clkr = {
+		.enable_reg = 0x18,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "lpass_qdsp6ss_xo_clk",
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
+static struct clk_branch lpass_qdsp6ss_sleep_clk = {
+	.halt_reg = 0x1c,
+	.halt_check = BRANCH_HALT_SKIP,
+	.clkr = {
+		.enable_reg = 0x1c,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "lpass_qdsp6ss_sleep_clk",
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
+static struct clk_branch lpass_qdsp6ss_core_clk = {
+	.halt_reg = 0x0,
+	.halt_check = BRANCH_HALT_SKIP,
+	.clkr = {
+		.enable_reg = 0x0,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "lpass_qdsp6ss_core_clk",
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
+static struct regmap_config lpass_regmap_config = {
+	.reg_bits	= 32,
+	.reg_stride	= 4,
+	.val_bits	= 32,
+	.fast_io	= true,
+};
+
+static struct clk_regmap *lpass_gcc_sdm845_clocks[] = {
+	[GCC_LPASS_Q6_AXI_CLK] = &gcc_lpass_q6_axi_clk.clkr,
+	[GCC_LPASS_SWAY_CLK] = &gcc_lpass_sway_clk.clkr,
+};
+
+static const struct qcom_cc_desc lpass_gcc_sdm845_desc = {
+	.config = &lpass_regmap_config,
+	.clks = lpass_gcc_sdm845_clocks,
+	.num_clks = ARRAY_SIZE(lpass_gcc_sdm845_clocks),
+};
+
+static struct clk_regmap *lpass_cc_sdm845_clocks[] = {
+	[LPASS_AUDIO_WRAPPER_AON_CLK] = &lpass_audio_wrapper_aon_clk.clkr,
+	[LPASS_Q6SS_AHBM_AON_CLK] = &lpass_q6ss_ahbm_aon_clk.clkr,
+	[LPASS_Q6SS_AHBS_AON_CLK] = &lpass_q6ss_ahbs_aon_clk.clkr,
+};
+
+static const struct qcom_cc_desc lpass_cc_sdm845_desc = {
+	.config = &lpass_regmap_config,
+	.clks = lpass_cc_sdm845_clocks,
+	.num_clks = ARRAY_SIZE(lpass_cc_sdm845_clocks),
+};
+
+static struct clk_regmap *lpass_qdsp6ss_sdm845_clocks[] = {
+	[LPASS_QDSP6SS_XO_CLK] = &lpass_qdsp6ss_xo_clk.clkr,
+	[LPASS_QDSP6SS_SLEEP_CLK] = &lpass_qdsp6ss_sleep_clk.clkr,
+	[LPASS_QDSP6SS_CORE_CLK] = &lpass_qdsp6ss_core_clk.clkr,
+};
+
+static const struct qcom_cc_desc lpass_qdsp6ss_sdm845_desc = {
+	.config = &lpass_regmap_config,
+	.clks = lpass_qdsp6ss_sdm845_clocks,
+	.num_clks = ARRAY_SIZE(lpass_qdsp6ss_sdm845_clocks),
+};
+
+static int lpass_clocks_sdm845_probe(struct platform_device *pdev, int index,
+				     const struct qcom_cc_desc *desc)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct regmap *regmap;
+	struct resource res;
+	void __iomem *base;
+
+	if (of_address_to_resource(np, index, &res))
+		return -ENOMEM;
+
+	base = devm_ioremap(&pdev->dev, res.start, resource_size(&res));
+	if (IS_ERR(base))
+		return -ENOMEM;
+
+	regmap = devm_regmap_init_mmio(&pdev->dev, base, desc->config);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	return qcom_cc_really_probe(pdev, desc, regmap);
+}
+
+/* LPASS CC clock controller */
+static const struct of_device_id lpass_cc_sdm845_match_table[] = {
+	{ .compatible = "qcom,sdm845-lpasscc" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, lpass_cc_sdm845_match_table);
+
+static int lpass_cc_sdm845_probe(struct platform_device *pdev)
+{
+	const struct qcom_cc_desc *desc;
+	struct device_node *np;
+	int ret, index;
+
+	np = pdev->dev.of_node;
+
+	lpass_regmap_config.name = "lpass_gcc";
+	desc = &lpass_gcc_sdm845_desc;
+	index = of_property_match_string(np, "reg-names", "lpass_gcc");
+
+	ret = lpass_clocks_sdm845_probe(pdev, index, desc);
+	if (ret)
+		return ret;
+
+	lpass_regmap_config.name = "lpass_cc";
+	desc = &lpass_cc_sdm845_desc;
+	index = of_property_match_string(np, "reg-names", "lpass_cc");
+
+	ret = lpass_clocks_sdm845_probe(pdev, index, desc);
+	if (ret)
+		return ret;
+
+	lpass_regmap_config.name = "lpass_qdsp6ss";
+	desc = &lpass_qdsp6ss_sdm845_desc;
+	index = of_property_match_string(np, "reg-names", "lpass_qdsp6ss");
+
+	ret = lpass_clocks_sdm845_probe(pdev, index, desc);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static struct platform_driver lpass_cc_sdm845_driver = {
+	.probe		= lpass_cc_sdm845_probe,
+	.driver		= {
+		.name	= "sdm845-lpasscc",
+		.of_match_table = lpass_cc_sdm845_match_table,
+	},
+};
+
+static int __init lpass_cc_sdm845_init(void)
+{
+	return platform_driver_register(&lpass_cc_sdm845_driver);
+}
+subsys_initcall(lpass_cc_sdm845_init);
+
+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.

Re: [PATCH v2 2/2] clk: qcom: Add lpass clock controller driver for SDM845

[Stephen Boyd]

Quoting Taniya Das (2018-07-04 23:55:21)
> diff --git a/drivers/clk/qcom/lpasscc-sdm845.c b/drivers/clk/qcom/lpasscc-sdm845.c
> new file mode 100644
> index 0000000..5285b26
> --- /dev/null
> +++ b/drivers/clk/qcom/lpasscc-sdm845.c
> @@ -0,0 +1,243 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/err.h>
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/regmap.h>
> +
> +#include <dt-bindings/clock/qcom,lpass-sdm845.h>
> +
> +#include "clk-regmap.h"
> +#include "clk-branch.h"
> +#include "common.h"
> +
> +static struct clk_branch gcc_lpass_q6_axi_clk = {
> +       .halt_reg = 0x0,
> +       .halt_check = BRANCH_HALT,
> +       .clkr = {
> +               .enable_reg = 0x0,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "gcc_lpass_q6_axi_clk",
> +                       .ops = &clk_branch2_ops,
> +               },
> +       },
> +};
> +
> +static struct clk_branch gcc_lpass_sway_clk = {
> +       .halt_reg = 0x8,
> +       .halt_check = BRANCH_HALT,
> +       .clkr = {
> +               .enable_reg = 0x8,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "gcc_lpass_sway_clk",
> +                       .ops = &clk_branch2_ops,
> +               },
> +       },
> +};
> +
> +static struct clk_branch lpass_audio_wrapper_aon_clk = {
> +       .halt_reg = 0x098,
> +       .halt_check = BRANCH_VOTED,
> +       .clkr = {
> +               .enable_reg = 0x098,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "lpass_audio_wrapper_aon_clk",
> +                       .ops = &clk_branch2_ops,
> +               },
> +       },
> +};
> +
> +static struct clk_branch lpass_q6ss_ahbm_aon_clk = {
> +       .halt_reg = 0x12000,
> +       .halt_check = BRANCH_VOTED,

I'll take your word for it.

> +       .clkr = {
> +               .enable_reg = 0x12000,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "lpass_q6ss_ahbm_aon_clk",
> +                       .ops = &clk_branch2_ops,
> +               },
> +       },
> +};
> +
> +static struct clk_branch lpass_q6ss_ahbs_aon_clk = {
> +       .halt_reg = 0x1f000,
> +       .halt_check = BRANCH_VOTED,
> +       .clkr = {
> +               .enable_reg = 0x1f000,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "lpass_q6ss_ahbs_aon_clk",
> +                       .ops = &clk_branch2_ops,
> +               },
> +       },
> +};
> +
> +static struct clk_branch lpass_qdsp6ss_xo_clk = {
> +       .halt_reg = 0x18,
> +       .halt_check = BRANCH_HALT_SKIP,

Why? Hint, add a comment.

> +       .clkr = {
> +               .enable_reg = 0x18,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "lpass_qdsp6ss_xo_clk",
> +                       .ops = &clk_branch2_ops,
> +               },
> +       },
> +};
> +
> +static struct clk_branch lpass_qdsp6ss_sleep_clk = {
> +       .halt_reg = 0x1c,
> +       .halt_check = BRANCH_HALT_SKIP,

Why? Hint, add a comment.

> +       .clkr = {
> +               .enable_reg = 0x1c,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "lpass_qdsp6ss_sleep_clk",
> +                       .ops = &clk_branch2_ops,
> +               },
> +       },
> +};
> +
> +static struct clk_branch lpass_qdsp6ss_core_clk = {
> +       .halt_reg = 0x0,
> +       .halt_check = BRANCH_HALT_SKIP,

Again.

> +       .clkr = {
> +               .enable_reg = 0x0,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "lpass_qdsp6ss_core_clk",
> +                       .ops = &clk_branch2_ops,
> +               },
> +       },
> +};
> +
> +static struct regmap_config lpass_regmap_config = {
> +       .reg_bits       = 32,
> +       .reg_stride     = 4,
> +       .val_bits       = 32,
> +       .fast_io        = true,
> +};
> +
> +static struct clk_regmap *lpass_gcc_sdm845_clocks[] = {
> +       [GCC_LPASS_Q6_AXI_CLK] = &gcc_lpass_q6_axi_clk.clkr,
> +       [GCC_LPASS_SWAY_CLK] = &gcc_lpass_sway_clk.clkr,
> +};
> +
> +static const struct qcom_cc_desc lpass_gcc_sdm845_desc = {
> +       .config = &lpass_regmap_config,
> +       .clks = lpass_gcc_sdm845_clocks,
> +       .num_clks = ARRAY_SIZE(lpass_gcc_sdm845_clocks),
> +};
> +
> +static struct clk_regmap *lpass_cc_sdm845_clocks[] = {
> +       [LPASS_AUDIO_WRAPPER_AON_CLK] = &lpass_audio_wrapper_aon_clk.clkr,
> +       [LPASS_Q6SS_AHBM_AON_CLK] = &lpass_q6ss_ahbm_aon_clk.clkr,
> +       [LPASS_Q6SS_AHBS_AON_CLK] = &lpass_q6ss_ahbs_aon_clk.clkr,

So it's kinda sad that these lists have holes in them and we just waste
space forever the more we add. How about using the same linear array,
but indexing into the array with different offsets based on the start of
the numberspace for those clks? It would mean that adding new clks
wouldn't work, so add all the clks now instead of later.

Otherwise, maybe we need to add support for qcom_cc_desc having two
cells and then indicating which first cell to map into? So the binding
would look like:

	<&lpasscc 0 GCC_LPASS_Q6_AXI_CLK>

	<&lpasscc 1 LPASS_Q6SS_AHBM_AON_CLK>

and our lookup function would need to be different for the multi-cell
thing, but otherwise works and avoids the hole and adding numbers later
problem.

> +};
> +
> +static const struct qcom_cc_desc lpass_cc_sdm845_desc = {
> +       .config = &lpass_regmap_config,
> +       .clks = lpass_cc_sdm845_clocks,
> +       .num_clks = ARRAY_SIZE(lpass_cc_sdm845_clocks),
> +};
> +
> +static struct clk_regmap *lpass_qdsp6ss_sdm845_clocks[] = {
> +       [LPASS_QDSP6SS_XO_CLK] = &lpass_qdsp6ss_xo_clk.clkr,
> +       [LPASS_QDSP6SS_SLEEP_CLK] = &lpass_qdsp6ss_sleep_clk.clkr,
> +       [LPASS_QDSP6SS_CORE_CLK] = &lpass_qdsp6ss_core_clk.clkr,
> +};
> +
> +static const struct qcom_cc_desc lpass_qdsp6ss_sdm845_desc = {
> +       .config = &lpass_regmap_config,
> +       .clks = lpass_qdsp6ss_sdm845_clocks,
> +       .num_clks = ARRAY_SIZE(lpass_qdsp6ss_sdm845_clocks),
> +};
> +
> +static int lpass_clocks_sdm845_probe(struct platform_device *pdev, int index,
> +                                    const struct qcom_cc_desc *desc)
> +{
> +       struct device_node *np = pdev->dev.of_node;
> +       struct regmap *regmap;
> +       struct resource res;
> +       void __iomem *base;
> +
> +       if (of_address_to_resource(np, index, &res))

Just use platform device APIs instead please.

> +               return -ENOMEM;
> +
> +       base = devm_ioremap(&pdev->dev, res.start, resource_size(&res));

And devm_ioremap_resource().

> +       if (IS_ERR(base))
> +               return -ENOMEM;
> +
> +       regmap = devm_regmap_init_mmio(&pdev->dev, base, desc->config);
> +       if (IS_ERR(regmap))
> +               return PTR_ERR(regmap);
> +
> +       return qcom_cc_really_probe(pdev, desc, regmap);
> +}
> +
> +/* LPASS CC clock controller */
> +static const struct of_device_id lpass_cc_sdm845_match_table[] = {
> +       { .compatible = "qcom,sdm845-lpasscc" },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(of, lpass_cc_sdm845_match_table);
> +
> +static int lpass_cc_sdm845_probe(struct platform_device *pdev)
> +{
> +       const struct qcom_cc_desc *desc;
> +       struct device_node *np;
> +       int ret, index;
> +
> +       np = pdev->dev.of_node;
> +
> +       lpass_regmap_config.name = "lpass_gcc";
> +       desc = &lpass_gcc_sdm845_desc;
> +       index = of_property_match_string(np, "reg-names", "lpass_gcc");
> +
> +       ret = lpass_clocks_sdm845_probe(pdev, index, desc);
> +       if (ret)
> +               return ret;
> +
> +       lpass_regmap_config.name = "lpass_cc";
> +       desc = &lpass_cc_sdm845_desc;
> +       index = of_property_match_string(np, "reg-names", "lpass_cc");
> +
> +       ret = lpass_clocks_sdm845_probe(pdev, index, desc);
> +       if (ret)
> +               return ret;
> +
> +       lpass_regmap_config.name = "lpass_qdsp6ss";
> +       desc = &lpass_qdsp6ss_sdm845_desc;
> +       index = of_property_match_string(np, "reg-names", "lpass_qdsp6ss");

We shouldn't need to do this. Just index them by number and specify that
the order has to be exactly that order. reg-names is supposed to be
optional.

> +
> +       ret = lpass_clocks_sdm845_probe(pdev, index, desc);
> +       if (ret)
> +               return ret;
> +
> +       return 0;

Just 'return lpass_clocks_sdm845_probe()'.

Re: [PATCH v2 1/2] dt-bindings: clock: Introduce QCOM LPASS clock bindings

[Stephen Boyd]

Quoting Taniya Das (2018-07-04 23:55:20)
> diff --git a/Documentation/devicetree/bindings/clock/qcom,lpasscc.txt b/Documentation/devicetree/bindings/clock/qcom,lpasscc.txt
> new file mode 100644
> index 0000000..fe7378b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/qcom,lpasscc.txt
> @@ -0,0 +1,22 @@
> +Qualcomm LPASS Clock Controller Binding
> +-----------------------------------------------
> +
> +Required properties :
> +- compatible   : shall contain "qcom,sdm845-lpasscc"
> +- #clock-cells : from common clock binding, shall contain 1.
> +- reg          : shall contain base register address and size.
> +- reg-names    : shall contain the register names of LPASS domain
> +                       "lpass_gcc", "lpass_cc", "lpass_qdsp6ss".
> +
> +Example:
> +
> +The below node has to be defined in the cases where the LPASS peripheral loader
> +would bring the subsystem out of reset.
> +
> +       lpasscc: clock-controller {
> +               compatible = "qcom,sdm845-lpasscc";
> +               reg = <0x00147000 0x20>, <0x17014000 0x1f004>,

This first reg is inside GCC though? Why isn't it added to the gcc
sdm845 driver? And then the next two might make sense as a different
region, but the reg property ending in 20 looks really weird.

> +                       <0x17300020 0x20>;
> +               reg-names = "lpass_gcc", "lpass_cc", "lpass_qdsp6ss";
> +               #clock-cells = <1>;
> +       };

Re: [PATCH v2 2/2] clk: qcom: Add lpass clock controller driver for SDM845

[Taniya Das]

On 7/7/2018 5:09 AM, Stephen Boyd wrote:
> Quoting Taniya Das (2018-07-04 23:55:21)
>> diff --git a/drivers/clk/qcom/lpasscc-sdm845.c b/drivers/clk/qcom/lpasscc-sdm845.c
>> new file mode 100644
>> index 0000000..5285b26
>> --- /dev/null
>> +++ b/drivers/clk/qcom/lpasscc-sdm845.c
>> @@ -0,0 +1,243 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include <linux/bitops.h>
>> +#include <linux/err.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/module.h>
>> +#include <linux/of_address.h>
>> +#include <linux/regmap.h>
>> +
>> +#include <dt-bindings/clock/qcom,lpass-sdm845.h>
>> +
>> +#include "clk-regmap.h"
>> +#include "clk-branch.h"
>> +#include "common.h"
>> +
>> +static struct clk_branch gcc_lpass_q6_axi_clk = {
>> +       .halt_reg = 0x0,
>> +       .halt_check = BRANCH_HALT,
>> +       .clkr = {
>> +               .enable_reg = 0x0,
>> +               .enable_mask = BIT(0),
>> +               .hw.init = &(struct clk_init_data){
>> +                       .name = "gcc_lpass_q6_axi_clk",
>> +                       .ops = &clk_branch2_ops,
>> +               },
>> +       },
>> +};
>> +
>> +static struct clk_branch gcc_lpass_sway_clk = {
>> +       .halt_reg = 0x8,
>> +       .halt_check = BRANCH_HALT,
>> +       .clkr = {
>> +               .enable_reg = 0x8,
>> +               .enable_mask = BIT(0),
>> +               .hw.init = &(struct clk_init_data){
>> +                       .name = "gcc_lpass_sway_clk",
>> +                       .ops = &clk_branch2_ops,
>> +               },
>> +       },
>> +};
>> +

Moved the above clocks to GCC driver.

>> +static struct clk_branch lpass_audio_wrapper_aon_clk = {
>> +       .halt_reg = 0x098,
>> +       .halt_check = BRANCH_VOTED,
>> +       .clkr = {
>> +               .enable_reg = 0x098,
>> +               .enable_mask = BIT(0),
>> +               .hw.init = &(struct clk_init_data){
>> +                       .name = "lpass_audio_wrapper_aon_clk",
>> +                       .ops = &clk_branch2_ops,
>> +               },
>> +       },
>> +};
>> +
>> +static struct clk_branch lpass_q6ss_ahbm_aon_clk = {
>> +       .halt_reg = 0x12000,
>> +       .halt_check = BRANCH_VOTED,
> 
> I'll take your word for it.
> 
>> +       .clkr = {
>> +               .enable_reg = 0x12000,
>> +               .enable_mask = BIT(0),
>> +               .hw.init = &(struct clk_init_data){
>> +                       .name = "lpass_q6ss_ahbm_aon_clk",
>> +                       .ops = &clk_branch2_ops,
>> +               },
>> +       },
>> +};
>> +
>> +static struct clk_branch lpass_q6ss_ahbs_aon_clk = {
>> +       .halt_reg = 0x1f000,
>> +       .halt_check = BRANCH_VOTED,
>> +       .clkr = {
>> +               .enable_reg = 0x1f000,
>> +               .enable_mask = BIT(0),
>> +               .hw.init = &(struct clk_init_data){
>> +                       .name = "lpass_q6ss_ahbs_aon_clk",
>> +                       .ops = &clk_branch2_ops,
>> +               },
>> +       },
>> +};
>> +
>> +static struct clk_branch lpass_qdsp6ss_xo_clk = {
>> +       .halt_reg = 0x18,
>> +       .halt_check = BRANCH_HALT_SKIP,
> 
> Why? Hint, add a comment.
> 

Added a comment in the next patch.

>> +       .clkr = {
>> +               .enable_reg = 0x18,
>> +               .enable_mask = BIT(0),
>> +               .hw.init = &(struct clk_init_data){
>> +                       .name = "lpass_qdsp6ss_xo_clk",
>> +                       .ops = &clk_branch2_ops,
>> +               },
>> +       },
>> +};
>> +
>> +static struct clk_branch lpass_qdsp6ss_sleep_clk = {
>> +       .halt_reg = 0x1c,
>> +       .halt_check = BRANCH_HALT_SKIP,
> 
> Why? Hint, add a comment.

Added a comment in the next patch.

> 
>> +       .clkr = {
>> +               .enable_reg = 0x1c,
>> +               .enable_mask = BIT(0),
>> +               .hw.init = &(struct clk_init_data){
>> +                       .name = "lpass_qdsp6ss_sleep_clk",
>> +                       .ops = &clk_branch2_ops,
>> +               },
>> +       },
>> +};
>> +
>> +static struct clk_branch lpass_qdsp6ss_core_clk = {
>> +       .halt_reg = 0x0,
>> +       .halt_check = BRANCH_HALT_SKIP,
> 
> Again.
> 

Added a comment in the next patch.

>> +       .clkr = {
>> +               .enable_reg = 0x0,
>> +               .enable_mask = BIT(0),
>> +               .hw.init = &(struct clk_init_data){
>> +                       .name = "lpass_qdsp6ss_core_clk",
>> +                       .ops = &clk_branch2_ops,
>> +               },
>> +       },
>> +};
>> +
>> +static struct regmap_config lpass_regmap_config = {
>> +       .reg_bits       = 32,
>> +       .reg_stride     = 4,
>> +       .val_bits       = 32,
>> +       .fast_io        = true,
>> +};
>> +
>> +static struct clk_regmap *lpass_gcc_sdm845_clocks[] = {
>> +       [GCC_LPASS_Q6_AXI_CLK] = &gcc_lpass_q6_axi_clk.clkr,
>> +       [GCC_LPASS_SWAY_CLK] = &gcc_lpass_sway_clk.clkr,
>> +};
>> +
>> +static const struct qcom_cc_desc lpass_gcc_sdm845_desc = {
>> +       .config = &lpass_regmap_config,
>> +       .clks = lpass_gcc_sdm845_clocks,
>> +       .num_clks = ARRAY_SIZE(lpass_gcc_sdm845_clocks),
>> +};
>> +
>> +static struct clk_regmap *lpass_cc_sdm845_clocks[] = {
>> +       [LPASS_AUDIO_WRAPPER_AON_CLK] = &lpass_audio_wrapper_aon_clk.clkr,
>> +       [LPASS_Q6SS_AHBM_AON_CLK] = &lpass_q6ss_ahbm_aon_clk.clkr,
>> +       [LPASS_Q6SS_AHBS_AON_CLK] = &lpass_q6ss_ahbs_aon_clk.clkr,
> 
> So it's kinda sad that these lists have holes in them and we just waste
> space forever the more we add. How about using the same linear array,
> but indexing into the array with different offsets based on the start of
> the numberspace for those clks? It would mean that adding new clks
> wouldn't work, so add all the clks now instead of later.
> 

There are no more clocks from these CCs which would be required to be 
controlled. These are only required for the Low Pass Audio subsystem to 
be brought out of reset.

> Otherwise, maybe we need to add support for qcom_cc_desc having two
> cells and then indicating which first cell to map into? So the binding
> would look like:
> 
> 	<&lpasscc 0 GCC_LPASS_Q6_AXI_CLK>
> 
> 	<&lpasscc 1 LPASS_Q6SS_AHBM_AON_CLK>
> 
> and our lookup function would need to be different for the multi-cell
> thing, but otherwise works and avoids the hole and adding numbers later
> problem.
> 
>> +};
>> +
>> +static const struct qcom_cc_desc lpass_cc_sdm845_desc = {
>> +       .config = &lpass_regmap_config,
>> +       .clks = lpass_cc_sdm845_clocks,
>> +       .num_clks = ARRAY_SIZE(lpass_cc_sdm845_clocks),
>> +};
>> +
>> +static struct clk_regmap *lpass_qdsp6ss_sdm845_clocks[] = {
>> +       [LPASS_QDSP6SS_XO_CLK] = &lpass_qdsp6ss_xo_clk.clkr,
>> +       [LPASS_QDSP6SS_SLEEP_CLK] = &lpass_qdsp6ss_sleep_clk.clkr,
>> +       [LPASS_QDSP6SS_CORE_CLK] = &lpass_qdsp6ss_core_clk.clkr,
>> +};
>> +
>> +static const struct qcom_cc_desc lpass_qdsp6ss_sdm845_desc = {
>> +       .config = &lpass_regmap_config,
>> +       .clks = lpass_qdsp6ss_sdm845_clocks,
>> +       .num_clks = ARRAY_SIZE(lpass_qdsp6ss_sdm845_clocks),
>> +};
>> +
>> +static int lpass_clocks_sdm845_probe(struct platform_device *pdev, int index,
>> +                                    const struct qcom_cc_desc *desc)
>> +{
>> +       struct device_node *np = pdev->dev.of_node;
>> +       struct regmap *regmap;
>> +       struct resource res;
>> +       void __iomem *base;
>> +
>> +       if (of_address_to_resource(np, index, &res))
> 
> Just use platform device APIs instead please.
> 

I have moved to use the platform device APIs.

>> +               return -ENOMEM;
>> +
>> +       base = devm_ioremap(&pdev->dev, res.start, resource_size(&res));
> 
> And devm_ioremap_resource().

Use the above API to map.

> 
>> +       if (IS_ERR(base))
>> +               return -ENOMEM;
>> +
>> +       regmap = devm_regmap_init_mmio(&pdev->dev, base, desc->config);
>> +       if (IS_ERR(regmap))
>> +               return PTR_ERR(regmap);
>> +
>> +       return qcom_cc_really_probe(pdev, desc, regmap);
>> +}
>> +
>> +/* LPASS CC clock controller */
>> +static const struct of_device_id lpass_cc_sdm845_match_table[] = {
>> +       { .compatible = "qcom,sdm845-lpasscc" },
>> +       { }
>> +};
>> +MODULE_DEVICE_TABLE(of, lpass_cc_sdm845_match_table);
>> +
>> +static int lpass_cc_sdm845_probe(struct platform_device *pdev)
>> +{
>> +       const struct qcom_cc_desc *desc;
>> +       struct device_node *np;
>> +       int ret, index;
>> +
>> +       np = pdev->dev.of_node;
>> +
>> +       lpass_regmap_config.name = "lpass_gcc";
>> +       desc = &lpass_gcc_sdm845_desc;
>> +       index = of_property_match_string(np, "reg-names", "lpass_gcc");
>> +
>> +       ret = lpass_clocks_sdm845_probe(pdev, index, desc);
>> +       if (ret)
>> +               return ret;
>> +
>> +       lpass_regmap_config.name = "lpass_cc";
>> +       desc = &lpass_cc_sdm845_desc;
>> +       index = of_property_match_string(np, "reg-names", "lpass_cc");
>> +
>> +       ret = lpass_clocks_sdm845_probe(pdev, index, desc);
>> +       if (ret)
>> +               return ret;
>> +
>> +       lpass_regmap_config.name = "lpass_qdsp6ss";
>> +       desc = &lpass_qdsp6ss_sdm845_desc;
>> +       index = of_property_match_string(np, "reg-names", "lpass_qdsp6ss");
> 
> We shouldn't need to do this. Just index them by number and specify that
> the order has to be exactly that order. reg-names is supposed to be
> optional.
> 

Have fixed the index in the next patch.

>> +
>> +       ret = lpass_clocks_sdm845_probe(pdev, index, desc);
>> +       if (ret)
>> +               return ret;
>> +
>> +       return 0;
> 
> Just 'return lpass_clocks_sdm845_probe()'.
> 

removed the check for ret.

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

--

Re: [PATCH v2 1/2] dt-bindings: clock: Introduce QCOM LPASS clock bindings

[Taniya Das]

On 7/7/2018 5:12 AM, Stephen Boyd wrote:
> Quoting Taniya Das (2018-07-04 23:55:20)
>> diff --git a/Documentation/devicetree/bindings/clock/qcom,lpasscc.txt b/Documentation/devicetree/bindings/clock/qcom,lpasscc.txt
>> new file mode 100644
>> index 0000000..fe7378b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/qcom,lpasscc.txt
>> @@ -0,0 +1,22 @@
>> +Qualcomm LPASS Clock Controller Binding
>> +-----------------------------------------------
>> +
>> +Required properties :
>> +- compatible   : shall contain "qcom,sdm845-lpasscc"
>> +- #clock-cells : from common clock binding, shall contain 1.
>> +- reg          : shall contain base register address and size.
>> +- reg-names    : shall contain the register names of LPASS domain
>> +                       "lpass_gcc", "lpass_cc", "lpass_qdsp6ss".
>> +
>> +Example:
>> +
>> +The below node has to be defined in the cases where the LPASS peripheral loader
>> +would bring the subsystem out of reset.
>> +
>> +       lpasscc: clock-controller {
>> +               compatible = "qcom,sdm845-lpasscc";
>> +               reg = <0x00147000 0x20>, <0x17014000 0x1f004>,
> 
> This first reg is inside GCC though? Why isn't it added to the gcc
> sdm845 driver? And then the next two might make sense as a different
> region, but the reg property ending in 20 looks really weird.
>
I have moved the GCC registers in the GCC driver with a device tree 
property flag. And also have mapped the lpass_qdsp6ss CC region.

>> +                       <0x17300020 0x20>;
>> +               reg-names = "lpass_gcc", "lpass_cc", "lpass_qdsp6ss";
>> +               #clock-cells = <1>;
>> +       };

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

--