[PATCH v6] Add support for LPASS clock controller for SDM845

[PATCH v6] Add support for LPASS clock controller for SDM845

[Taniya Das]

 [v6]
  * Update the logic to register the lpass clocks when the device tree property
   is not present.
  * Add the CLK_IGNORE_UNUSED flag for the lpass clocks to not gate the clocks
   at late_init.

 [v5]
  * Address the comments in device tree binding to update the reg-names,
    update the unit address in lpass clock node example and also
    add reg property for the gcc clock node.
  * Update the lpass driver to take care of the reg-names.

 [v4]
  * Update the description in GCC Documentation binding for
  'qcom,lpass-protected'.
  * Remove 'qcom,lpass-protected' from LPASS Documentation binding.
  * Update KConfig to use Low Power Audio Subsystem.
  * Add module_exit() and also update return value for
    devm_ioremap_resource failure.

 [v3]
  * Add a device tree property to identify lpass protected GCC clocks.
  * Update the GCC driver code to register the lpass clocks when the flag is
   defined.
  * Add comment for clocks using the BRANCH_HALT_SKIP flag.
  * Use platform APIs instead of of_address_to_resource.
  * Replace devm_ioremap with devm_ioremap_resource.
  * Use fixed index for 'lpass_cc' & 'lpass_qdsp6ss' in probe.

 [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 (1):
  clk: qcom: Add lpass clock controller driver for SDM845

 drivers/clk/qcom/Kconfig          |   9 ++
 drivers/clk/qcom/Makefile         |   1 +
 drivers/clk/qcom/gcc-sdm845.c     |  35 +++++++
 drivers/clk/qcom/lpasscc-sdm845.c | 201 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 246 insertions(+)
 create mode 100644 drivers/clk/qcom/lpasscc-sdm845.c

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

[PATCH v6] 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.
LPASS clocks present on the global clock controller would be registered
with the clock framework based on the device tree flag. Also do not gate
these clocks if they are left unused.

Signed-off-by: Taniya Das <tdas@codeaurora.org>
---
 drivers/clk/qcom/Kconfig          |   9 ++
 drivers/clk/qcom/Makefile         |   1 +
 drivers/clk/qcom/gcc-sdm845.c     |  35 +++++++
 drivers/clk/qcom/lpasscc-sdm845.c | 201 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 246 insertions(+)
 create mode 100644 drivers/clk/qcom/lpasscc-sdm845.c

diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index 5b181b1..747ffb4 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -272,6 +272,15 @@ config SDM_DISPCC_845
 	  Say Y if you want to support display devices and functionality such as
 	  splash screen.

+config SDM_LPASSCC_845
+	tristate "SDM845 Low Power Audio Subsystem (LPAAS) 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/Makefile b/drivers/clk/qcom/Makefile
index 935f142..53a5283 100644
--- a/drivers/clk/qcom/Makefile
+++ b/drivers/clk/qcom/Makefile
@@ -42,5 +42,6 @@ obj-$(CONFIG_QCOM_CLK_SMD_RPM) += clk-smd-rpm.o
 obj-$(CONFIG_SDM_CAMCC_845) += camcc-sdm845.o
 obj-$(CONFIG_SDM_DISPCC_845) += dispcc-sdm845.o
 obj-$(CONFIG_SDM_GCC_845) += gcc-sdm845.o
+obj-$(CONFIG_SDM_LPASSCC_845) += lpasscc-sdm845.o
 obj-$(CONFIG_SDM_VIDEOCC_845) += videocc-sdm845.o
 obj-$(CONFIG_SPMI_PMIC_CLKDIV) += clk-spmi-pmic-div.o
diff --git a/drivers/clk/qcom/gcc-sdm845.c b/drivers/clk/qcom/gcc-sdm845.c
index 08d593e..6379b8b 100644
--- a/drivers/clk/qcom/gcc-sdm845.c
+++ b/drivers/clk/qcom/gcc-sdm845.c
@@ -3169,6 +3169,32 @@ enum {
 	},
 };

+static struct clk_branch gcc_lpass_q6_axi_clk = {
+	.halt_reg = 0x47000,
+	.halt_check = BRANCH_HALT,
+	.clkr = {
+		.enable_reg = 0x47000,
+		.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 = 0x47008,
+	.halt_check = BRANCH_HALT,
+	.clkr = {
+		.enable_reg = 0x47008,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "gcc_lpass_sway_clk",
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
 static struct gdsc pcie_0_gdsc = {
 	.gdscr = 0x6b004,
 	.pd = {
@@ -3469,6 +3495,8 @@ enum {
 	[GCC_QSPI_CORE_CLK_SRC] = &gcc_qspi_core_clk_src.clkr,
 	[GCC_QSPI_CORE_CLK] = &gcc_qspi_core_clk.clkr,
 	[GCC_QSPI_CNOC_PERIPH_AHB_CLK] = &gcc_qspi_cnoc_periph_ahb_clk.clkr,
+	[GCC_LPASS_Q6_AXI_CLK] = NULL,
+	[GCC_LPASS_SWAY_CLK] = NULL,
 };

 static const struct qcom_reset_map gcc_sdm845_resets[] = {
@@ -3583,6 +3611,13 @@ static int gcc_sdm845_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;

+	if (!of_property_read_bool(pdev->dev.of_node, "qcom,lpass-protected")) {
+		gcc_sdm845_clocks[GCC_LPASS_Q6_AXI_CLK] =
+			&gcc_lpass_q6_axi_clk.clkr;
+		gcc_sdm845_clocks[GCC_LPASS_SWAY_CLK] =
+			&gcc_lpass_sway_clk.clkr;
+	}
+
 	return qcom_cc_really_probe(pdev, &gcc_sdm845_desc, regmap);
 }

diff --git a/drivers/clk/qcom/lpasscc-sdm845.c b/drivers/clk/qcom/lpasscc-sdm845.c
new file mode 100644
index 0000000..f7b9b0f
--- /dev/null
+++ b/drivers/clk/qcom/lpasscc-sdm845.c
@@ -0,0 +1,201 @@
+// 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 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",
+			.flags = CLK_IGNORE_UNUSED,
+			.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",
+			.flags = CLK_IGNORE_UNUSED,
+			.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",
+			.flags = CLK_IGNORE_UNUSED,
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
+/* CLK_OFF would not toggle until LPASS is not out of reset */
+static struct clk_branch lpass_qdsp6ss_core_clk = {
+	.halt_reg = 0x20,
+	.halt_check = BRANCH_HALT_SKIP,
+	.clkr = {
+		.enable_reg = 0x20,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "lpass_qdsp6ss_core_clk",
+			.flags = CLK_IGNORE_UNUSED,
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
+/* CLK_OFF would not toggle until LPASS is not out of reset */
+static struct clk_branch lpass_qdsp6ss_xo_clk = {
+	.halt_reg = 0x38,
+	.halt_check = BRANCH_HALT_SKIP,
+	.clkr = {
+		.enable_reg = 0x38,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "lpass_qdsp6ss_xo_clk",
+			.flags = CLK_IGNORE_UNUSED,
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
+/* CLK_OFF would not toggle until LPASS is not out of reset */
+static struct clk_branch lpass_qdsp6ss_sleep_clk = {
+	.halt_reg = 0x3c,
+	.halt_check = BRANCH_HALT_SKIP,
+	.clkr = {
+		.enable_reg = 0x3c,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "lpass_qdsp6ss_sleep_clk",
+			.flags = CLK_IGNORE_UNUSED,
+			.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_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 regmap *regmap;
+	struct resource *res;
+	void __iomem *base;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, index);
+	base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	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;
+	int ret;
+
+	lpass_regmap_config.name = "cc";
+	desc = &lpass_cc_sdm845_desc;
+
+	ret = lpass_clocks_sdm845_probe(pdev, 0, desc);
+	if (ret)
+		return ret;
+
+	lpass_regmap_config.name = "qdsp6ss";
+	desc = &lpass_qdsp6ss_sdm845_desc;
+
+	return lpass_clocks_sdm845_probe(pdev, 1, desc);
+}
+
+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);
+
+static void __exit lpass_cc_sdm845_exit(void)
+{
+	platform_driver_unregister(&lpass_cc_sdm845_driver);
+}
+module_exit(lpass_cc_sdm845_exit);
+
+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 v6] clk: qcom: Add lpass clock controller driver for SDM845

[Stephen Boyd]

Quoting Taniya Das (2018-10-04 05:02:26)
> 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.
> LPASS clocks present on the global clock controller would be registered
> with the clock framework based on the device tree flag. Also do not gate
> these clocks if they are left unused.

Why not gate them? This statement states what the code is doing, not why
it's doing it which is the more crucial information that should be
described in the commit text. Also, please add a comment about it to the
code next to the flag.

I am concerned that it doesn't make any sense though, so probably it
shouldn't be marked as CLK_IGNORE_UNUSED and it's papering over some
other larger bug that needs to be fixed.

> 
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> ---
> diff --git a/drivers/clk/qcom/gcc-sdm845.c b/drivers/clk/qcom/gcc-sdm845.c
> index 08d593e..6379b8b 100644
> --- a/drivers/clk/qcom/gcc-sdm845.c
> +++ b/drivers/clk/qcom/gcc-sdm845.c
> @@ -3169,6 +3169,32 @@ enum {
>         },
>  };
> 
> +static struct clk_branch gcc_lpass_q6_axi_clk = {
> +       .halt_reg = 0x47000,
> +       .halt_check = BRANCH_HALT,
> +       .clkr = {
> +               .enable_reg = 0x47000,
> +               .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 = 0x47008,
> +       .halt_check = BRANCH_HALT,
> +       .clkr = {
> +               .enable_reg = 0x47008,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "gcc_lpass_sway_clk",
> +                       .ops = &clk_branch2_ops,
> +               },
> +       },
> +};
> +
>  static struct gdsc pcie_0_gdsc = {
>         .gdscr = 0x6b004,
>         .pd = {
> @@ -3469,6 +3495,8 @@ enum {
>         [GCC_QSPI_CORE_CLK_SRC] = &gcc_qspi_core_clk_src.clkr,
>         [GCC_QSPI_CORE_CLK] = &gcc_qspi_core_clk.clkr,
>         [GCC_QSPI_CNOC_PERIPH_AHB_CLK] = &gcc_qspi_cnoc_periph_ahb_clk.clkr,
> +       [GCC_LPASS_Q6_AXI_CLK] = NULL,
> +       [GCC_LPASS_SWAY_CLK] = NULL,
>  };
> 
>  static const struct qcom_reset_map gcc_sdm845_resets[] = {
> @@ -3583,6 +3611,13 @@ static int gcc_sdm845_probe(struct platform_device *pdev)
>         if (ret)
>                 return ret;
> 
> +       if (!of_property_read_bool(pdev->dev.of_node, "qcom,lpass-protected")) {
> +               gcc_sdm845_clocks[GCC_LPASS_Q6_AXI_CLK] =
> +                       &gcc_lpass_q6_axi_clk.clkr;
> +               gcc_sdm845_clocks[GCC_LPASS_SWAY_CLK] =
> +                       &gcc_lpass_sway_clk.clkr;
> +       }
> +
>         return qcom_cc_really_probe(pdev, &gcc_sdm845_desc, regmap);
>  }
> 
> diff --git a/drivers/clk/qcom/lpasscc-sdm845.c b/drivers/clk/qcom/lpasscc-sdm845.c
> new file mode 100644
> index 0000000..f7b9b0f
> --- /dev/null
> +++ b/drivers/clk/qcom/lpasscc-sdm845.c
> @@ -0,0 +1,201 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/bitops.h>

Is this needed?

> +/* CLK_OFF would not toggle until LPASS is not out of reset */
> +static struct clk_branch lpass_qdsp6ss_xo_clk = {
> +       .halt_reg = 0x38,
> +       .halt_check = BRANCH_HALT_SKIP,
> +       .clkr = {
> +               .enable_reg = 0x38,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "lpass_qdsp6ss_xo_clk",
> +                       .flags = CLK_IGNORE_UNUSED,
> +                       .ops = &clk_branch2_ops,
> +               },
> +       },
> +};
> +
> +/* CLK_OFF would not toggle until LPASS is not out of reset */

Move this comment next to BRANCH_HALT_SKIP please so we know what it
relates to.

> +static struct clk_branch lpass_qdsp6ss_sleep_clk = {
> +       .halt_reg = 0x3c,
> +       .halt_check = BRANCH_HALT_SKIP,
> +       .clkr = {
> +               .enable_reg = 0x3c,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "lpass_qdsp6ss_sleep_clk",
> +                       .flags = CLK_IGNORE_UNUSED,
> +                       .ops = &clk_branch2_ops,
> +               },
> +       },
> +};
> +
> +static int lpass_clocks_sdm845_probe(struct platform_device *pdev, int index,
> +                                    const struct qcom_cc_desc *desc)
> +{
> +       struct regmap *regmap;
> +       struct resource *res;
> +       void __iomem *base;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, index);
> +       base = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(base))
> +               return PTR_ERR(base);
> +
> +       regmap = devm_regmap_init_mmio(&pdev->dev, base, desc->config);
> +       if (IS_ERR(regmap))
> +               return PTR_ERR(regmap);

If this happens again in the future we should move this into the
common.c file and let qcom_cc_probe_index() exist.

> +
> +       return qcom_cc_really_probe(pdev, desc, regmap);
> +}
> +
> +/* LPASS CC clock controller */

Please remove this comment. It's useless.

> +static const struct of_device_id lpass_cc_sdm845_match_table[] = {
> +       { .compatible = "qcom,sdm845-lpasscc" },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(of, lpass_cc_sdm845_match_table);
> +

Re: [PATCH v6] clk: qcom: Add lpass clock controller driver for SDM845

[Taniya Das]

Hello Stephen,

On 10/8/2018 8:14 AM, Stephen Boyd wrote:
> Quoting Taniya Das (2018-10-04 05:02:26)
>> 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.
>> LPASS clocks present on the global clock controller would be registered
>> with the clock framework based on the device tree flag. Also do not gate
>> these clocks if they are left unused.
> 
> Why not gate them? This statement states what the code is doing, not why
> it's doing it which is the more crucial information that should be
> described in the commit text. Also, please add a comment about it to the
> code next to the flag.
> 
> I am concerned that it doesn't make any sense though, so probably it
> shouldn't be marked as CLK_IGNORE_UNUSED and it's papering over some
> other larger bug that needs to be fixed.
> 

It does not have any bug, it is just that to access these lpass 
registers we would need the GCC lpass registers to be enabled. I would 
update the same in the commit text.

During clock late_init these clocks should not be accessed to check the 
clock status as they would result in unclocked access. The client would 
request these clocks in the correct order and it would not have any issue.


>>
>> Signed-off-by: Taniya Das <tdas@codeaurora.org>
>> ---
>> diff --git a/drivers/clk/qcom/gcc-sdm845.c b/drivers/clk/qcom/gcc-sdm845.c
>> index 08d593e..6379b8b 100644
>> --- a/drivers/clk/qcom/gcc-sdm845.c
>> +++ b/drivers/clk/qcom/gcc-sdm845.c
>> @@ -3169,6 +3169,32 @@ enum {
>>          },
>>   };
>>
>> +static struct clk_branch gcc_lpass_q6_axi_clk = {
>> +       .halt_reg = 0x47000,
>> +       .halt_check = BRANCH_HALT,
>> +       .clkr = {
>> +               .enable_reg = 0x47000,
>> +               .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 = 0x47008,
>> +       .halt_check = BRANCH_HALT,
>> +       .clkr = {
>> +               .enable_reg = 0x47008,
>> +               .enable_mask = BIT(0),
>> +               .hw.init = &(struct clk_init_data){
>> +                       .name = "gcc_lpass_sway_clk",
>> +                       .ops = &clk_branch2_ops,
>> +               },
>> +       },
>> +};
>> +
>>   static struct gdsc pcie_0_gdsc = {
>>          .gdscr = 0x6b004,
>>          .pd = {
>> @@ -3469,6 +3495,8 @@ enum {
>>          [GCC_QSPI_CORE_CLK_SRC] = &gcc_qspi_core_clk_src.clkr,
>>          [GCC_QSPI_CORE_CLK] = &gcc_qspi_core_clk.clkr,
>>          [GCC_QSPI_CNOC_PERIPH_AHB_CLK] = &gcc_qspi_cnoc_periph_ahb_clk.clkr,
>> +       [GCC_LPASS_Q6_AXI_CLK] = NULL,
>> +       [GCC_LPASS_SWAY_CLK] = NULL,
>>   };
>>
>>   static const struct qcom_reset_map gcc_sdm845_resets[] = {
>> @@ -3583,6 +3611,13 @@ static int gcc_sdm845_probe(struct platform_device *pdev)
>>          if (ret)
>>                  return ret;
>>
>> +       if (!of_property_read_bool(pdev->dev.of_node, "qcom,lpass-protected")) {
>> +               gcc_sdm845_clocks[GCC_LPASS_Q6_AXI_CLK] =
>> +                       &gcc_lpass_q6_axi_clk.clkr;
>> +               gcc_sdm845_clocks[GCC_LPASS_SWAY_CLK] =
>> +                       &gcc_lpass_sway_clk.clkr;
>> +       }
>> +
>>          return qcom_cc_really_probe(pdev, &gcc_sdm845_desc, regmap);
>>   }
>>
>> diff --git a/drivers/clk/qcom/lpasscc-sdm845.c b/drivers/clk/qcom/lpasscc-sdm845.c
>> new file mode 100644
>> index 0000000..f7b9b0f
>> --- /dev/null
>> +++ b/drivers/clk/qcom/lpasscc-sdm845.c
>> @@ -0,0 +1,201 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include <linux/bitops.h>
> 
> Is this needed?
> 

Would check this.

>> +/* CLK_OFF would not toggle until LPASS is not out of reset */
>> +static struct clk_branch lpass_qdsp6ss_xo_clk = {
>> +       .halt_reg = 0x38,
>> +       .halt_check = BRANCH_HALT_SKIP,
>> +       .clkr = {
>> +               .enable_reg = 0x38,
>> +               .enable_mask = BIT(0),
>> +               .hw.init = &(struct clk_init_data){
>> +                       .name = "lpass_qdsp6ss_xo_clk",
>> +                       .flags = CLK_IGNORE_UNUSED,
>> +                       .ops = &clk_branch2_ops,
>> +               },
>> +       },
>> +};
>> +
>> +/* CLK_OFF would not toggle until LPASS is not out of reset */
> 
> Move this comment next to BRANCH_HALT_SKIP please so we know what it
> relates to.
> 

Sure would take care in the next patch.

>> +static struct clk_branch lpass_qdsp6ss_sleep_clk = {
>> +       .halt_reg = 0x3c,
>> +       .halt_check = BRANCH_HALT_SKIP,
>> +       .clkr = {
>> +               .enable_reg = 0x3c,
>> +               .enable_mask = BIT(0),
>> +               .hw.init = &(struct clk_init_data){
>> +                       .name = "lpass_qdsp6ss_sleep_clk",
>> +                       .flags = CLK_IGNORE_UNUSED,
>> +                       .ops = &clk_branch2_ops,
>> +               },
>> +       },
>> +};
>> +
>> +static int lpass_clocks_sdm845_probe(struct platform_device *pdev, int index,
>> +                                    const struct qcom_cc_desc *desc)
>> +{
>> +       struct regmap *regmap;
>> +       struct resource *res;
>> +       void __iomem *base;
>> +
>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, index);
>> +       base = devm_ioremap_resource(&pdev->dev, res);
>> +       if (IS_ERR(base))
>> +               return PTR_ERR(base);
>> +
>> +       regmap = devm_regmap_init_mmio(&pdev->dev, base, desc->config);
>> +       if (IS_ERR(regmap))
>> +               return PTR_ERR(regmap);
> 
> If this happens again in the future we should move this into the
> common.c file and let qcom_cc_probe_index() exist.
> 

Yes, I agree, could submit a patch to add the new function and then 
clean it up.

>> +
>> +       return qcom_cc_really_probe(pdev, desc, regmap);
>> +}
>> +
>> +/* LPASS CC clock controller */
> 
> Please remove this comment. It's useless.
> 

Would take care in the next patch.

>> +static const struct of_device_id lpass_cc_sdm845_match_table[] = {
>> +       { .compatible = "qcom,sdm845-lpasscc" },
>> +       { }
>> +};
>> +MODULE_DEVICE_TABLE(of, lpass_cc_sdm845_match_table);
>> +

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

--

Re: [PATCH v6] clk: qcom: Add lpass clock controller driver for SDM845

[Stephen Boyd]

Quoting Taniya Das (2018-10-09 10:26:38)
> Hello Stephen,
> 
> On 10/8/2018 8:14 AM, Stephen Boyd wrote:
> > Quoting Taniya Das (2018-10-04 05:02:26)
> >> 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.
> >> LPASS clocks present on the global clock controller would be registered
> >> with the clock framework based on the device tree flag. Also do not gate
> >> these clocks if they are left unused.
> > 
> > Why not gate them? This statement states what the code is doing, not why
> > it's doing it which is the more crucial information that should be
> > described in the commit text. Also, please add a comment about it to the
> > code next to the flag.
> > 
> > I am concerned that it doesn't make any sense though, so probably it
> > shouldn't be marked as CLK_IGNORE_UNUSED and it's papering over some
> > other larger bug that needs to be fixed.
> > 
> 
> It does not have any bug, it is just that to access these lpass 
> registers we would need the GCC lpass registers to be enabled. I would 
> update the same in the commit text.
> 
> During clock late_init these clocks should not be accessed to check the 
> clock status as they would result in unclocked access. The client would 
> request these clocks in the correct order and it would not have any issue.
> 

That seems like the bug right there. If the LPASS registers can't be
accessed unless the clks in GCC are enabled then this driver needs to
turn the clks on before reading/writing registers. Marking the clks as
ignore unused is skipping around the real problem.

> 
> >>
> >> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> >> ---
> >> diff --git a/drivers/clk/qcom/gcc-sdm845.c b/drivers/clk/qcom/gcc-sdm845.c
> >> index 08d593e..6379b8b 100644
> >> --- a/drivers/clk/qcom/gcc-sdm845.c
> >> +++ b/drivers/clk/qcom/gcc-sdm845.c
> >> @@ -3583,6 +3611,13 @@ static int gcc_sdm845_probe(struct platform_device *pdev)
> >>          if (ret)
> >>                  return ret;
> >>
> >> +       if (!of_property_read_bool(pdev->dev.of_node, "qcom,lpass-protected")) {
> >> +               gcc_sdm845_clocks[GCC_LPASS_Q6_AXI_CLK] =
> >> +                       &gcc_lpass_q6_axi_clk.clkr;
> >> +               gcc_sdm845_clocks[GCC_LPASS_SWAY_CLK] =
> >> +                       &gcc_lpass_sway_clk.clkr;

For all intents and purposes could we not just mark these two as
CLK_IS_CRITICAL and then let the LPASS turn these on and off when it
cares (does it ever do so)? Or even just turn them on once in probe here
with direct register writes and then not care anymore to touch them
again? Or do we need to turn these clks on again later on when the
subsystem crashes to read/write the registers and cycle the clks back on
and off?

> >> +       }
> >> +
> >>          return qcom_cc_really_probe(pdev, &gcc_sdm845_desc, regmap);
> >>   }
> >>
> >> diff --git a/drivers/clk/qcom/lpasscc-sdm845.c b/drivers/clk/qcom/lpasscc-sdm845.c
> >> new file mode 100644
> >> index 0000000..f7b9b0f
> >> --- /dev/null
> >> +++ b/drivers/clk/qcom/lpasscc-sdm845.c
> >> +               },
> >> +       },
> >> +};
> >> +
> >> +static int lpass_clocks_sdm845_probe(struct platform_device *pdev, int index,
> >> +                                    const struct qcom_cc_desc *desc)
> >> +{
> >> +       struct regmap *regmap;
> >> +       struct resource *res;
> >> +       void __iomem *base;
> >> +
> >> +       res = platform_get_resource(pdev, IORESOURCE_MEM, index);
> >> +       base = devm_ioremap_resource(&pdev->dev, res);
> >> +       if (IS_ERR(base))
> >> +               return PTR_ERR(base);
> >> +
> >> +       regmap = devm_regmap_init_mmio(&pdev->dev, base, desc->config);
> >> +       if (IS_ERR(regmap))
> >> +               return PTR_ERR(regmap);
> > 
> > If this happens again in the future we should move this into the
> > common.c file and let qcom_cc_probe_index() exist.
> > 
> 
> Yes, I agree, could submit a patch to add the new function and then 
> clean it up.

Ok, but please don't do anything now because we don't care yet.

Re: [PATCH v6] clk: qcom: Add lpass clock controller driver for SDM845

[Taniya Das]

On 10/10/2018 2:22 AM, Stephen Boyd wrote:
> Quoting Taniya Das (2018-10-09 10:26:38)
>> Hello Stephen,
>>
>> On 10/8/2018 8:14 AM, Stephen Boyd wrote:
>>> Quoting Taniya Das (2018-10-04 05:02:26)
>>>> 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.
>>>> LPASS clocks present on the global clock controller would be registered
>>>> with the clock framework based on the device tree flag. Also do not gate
>>>> these clocks if they are left unused.
>>>
>>> Why not gate them? This statement states what the code is doing, not why
>>> it's doing it which is the more crucial information that should be
>>> described in the commit text. Also, please add a comment about it to the
>>> code next to the flag.
>>>
>>> I am concerned that it doesn't make any sense though, so probably it
>>> shouldn't be marked as CLK_IGNORE_UNUSED and it's papering over some
>>> other larger bug that needs to be fixed.
>>>
>>
>> It does not have any bug, it is just that to access these lpass
>> registers we would need the GCC lpass registers to be enabled. I would
>> update the same in the commit text.
>>
>> During clock late_init these clocks should not be accessed to check the
>> clock status as they would result in unclocked access. The client would
>> request these clocks in the correct order and it would not have any issue.
>>
> 
> That seems like the bug right there. If the LPASS registers can't be
> accessed unless the clks in GCC are enabled then this driver needs to
> turn the clks on before reading/writing registers. Marking the clks as
> ignore unused is skipping around the real problem.
> 

If the driver requests for the clocks they would maintain the order. But 
if the clock late init call is invoked before the driver requests, there 
is no way I could manage this dependency, that is the only reason to 
mark them unused.

>>
>>>>
>>>> Signed-off-by: Taniya Das <tdas@codeaurora.org>
>>>> ---
>>>> diff --git a/drivers/clk/qcom/gcc-sdm845.c b/drivers/clk/qcom/gcc-sdm845.c
>>>> index 08d593e..6379b8b 100644
>>>> --- a/drivers/clk/qcom/gcc-sdm845.c
>>>> +++ b/drivers/clk/qcom/gcc-sdm845.c
>>>> @@ -3583,6 +3611,13 @@ static int gcc_sdm845_probe(struct platform_device *pdev)
>>>>           if (ret)
>>>>                   return ret;
>>>>
>>>> +       if (!of_property_read_bool(pdev->dev.of_node, "qcom,lpass-protected")) {
>>>> +               gcc_sdm845_clocks[GCC_LPASS_Q6_AXI_CLK] =
>>>> +                       &gcc_lpass_q6_axi_clk.clkr;
>>>> +               gcc_sdm845_clocks[GCC_LPASS_SWAY_CLK] =
>>>> +                       &gcc_lpass_sway_clk.clkr;
> 
> For all intents and purposes could we not just mark these two as
> CLK_IS_CRITICAL and then let the LPASS turn these on and off when it
> cares (does it ever do so)? Or even just turn them on once in probe here
> with direct register writes and then not care anymore to touch them
> again? Or do we need to turn these clks on again later on when the
> subsystem crashes to read/write the registers and cycle the clks back on
> and off?
> 
>>>> +       }
>>>> +
>>>>           return qcom_cc_really_probe(pdev, &gcc_sdm845_desc, regmap);
>>>>    }
>>>>
>>>> diff --git a/drivers/clk/qcom/lpasscc-sdm845.c b/drivers/clk/qcom/lpasscc-sdm845.c
>>>> new file mode 100644
>>>> index 0000000..f7b9b0f
>>>> --- /dev/null
>>>> +++ b/drivers/clk/qcom/lpasscc-sdm845.c
>>>> +               },
>>>> +       },
>>>> +};
>>>> +
>>>> +static int lpass_clocks_sdm845_probe(struct platform_device *pdev, int index,
>>>> +                                    const struct qcom_cc_desc *desc)
>>>> +{
>>>> +       struct regmap *regmap;
>>>> +       struct resource *res;
>>>> +       void __iomem *base;
>>>> +
>>>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, index);
>>>> +       base = devm_ioremap_resource(&pdev->dev, res);
>>>> +       if (IS_ERR(base))
>>>> +               return PTR_ERR(base);
>>>> +
>>>> +       regmap = devm_regmap_init_mmio(&pdev->dev, base, desc->config);
>>>> +       if (IS_ERR(regmap))
>>>> +               return PTR_ERR(regmap);
>>>
>>> If this happens again in the future we should move this into the
>>> common.c file and let qcom_cc_probe_index() exist.
>>>
>>
>> Yes, I agree, could submit a patch to add the new function and then
>> clean it up.
> 
> Ok, but please don't do anything now because we don't care yet.
> 

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

--

Re: [PATCH v6] clk: qcom: Add lpass clock controller driver for SDM845

[Stephen Boyd]

Quoting Taniya Das (2018-10-09 23:12:27)
> 
> 
> On 10/10/2018 2:22 AM, Stephen Boyd wrote:
> > Quoting Taniya Das (2018-10-09 10:26:38)
> >> Hello Stephen,
> >>
> >> On 10/8/2018 8:14 AM, Stephen Boyd wrote:
> >>> Quoting Taniya Das (2018-10-04 05:02:26)
> >>>> 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.
> >>>> LPASS clocks present on the global clock controller would be registered
> >>>> with the clock framework based on the device tree flag. Also do not gate
> >>>> these clocks if they are left unused.
> >>>
> >>> Why not gate them? This statement states what the code is doing, not why
> >>> it's doing it which is the more crucial information that should be
> >>> described in the commit text. Also, please add a comment about it to the
> >>> code next to the flag.
> >>>
> >>> I am concerned that it doesn't make any sense though, so probably it
> >>> shouldn't be marked as CLK_IGNORE_UNUSED and it's papering over some
> >>> other larger bug that needs to be fixed.
> >>>
> >>
> >> It does not have any bug, it is just that to access these lpass
> >> registers we would need the GCC lpass registers to be enabled. I would
> >> update the same in the commit text.
> >>
> >> During clock late_init these clocks should not be accessed to check the
> >> clock status as they would result in unclocked access. The client would
> >> request these clocks in the correct order and it would not have any issue.
> >>
> > 
> > That seems like the bug right there. If the LPASS registers can't be
> > accessed unless the clks in GCC are enabled then this driver needs to
> > turn the clks on before reading/writing registers. Marking the clks as
> > ignore unused is skipping around the real problem.
> > 
> 
> If the driver requests for the clocks they would maintain the order. But 
> if the clock late init call is invoked before the driver requests, there 
> is no way I could manage this dependency, that is the only reason to 
> mark them unused.
> 

Which driver are we talking about here? The lpass clk driver? Presumably
the lpass clk driver would request the GCC clks and turn them on in
probe and then register any lpass clks. If the lpass clk driver probes
bfeore late init, then the gcc clks will be enabled and everything
works, and if the lpass clk driver probes after late init then the clks
that can't be touched without gcc clks enabled won't be registered, and
then they won't be touched. What goes wrong?

Re: [PATCH v6] clk: qcom: Add lpass clock controller driver for SDM845

[Taniya Das]

Hello Stephen,

On 10/12/2018 11:05 PM, Stephen Boyd wrote:
> Quoting Taniya Das (2018-10-09 23:12:27)
>>
>>
>> On 10/10/2018 2:22 AM, Stephen Boyd wrote:
>>> Quoting Taniya Das (2018-10-09 10:26:38)
>>>> Hello Stephen,
>>>>
>>>> On 10/8/2018 8:14 AM, Stephen Boyd wrote:
>>>>> Quoting Taniya Das (2018-10-04 05:02:26)
>>>>>> 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.
>>>>>> LPASS clocks present on the global clock controller would be registered
>>>>>> with the clock framework based on the device tree flag. Also do not gate
>>>>>> these clocks if they are left unused.
>>>>>
>>>>> Why not gate them? This statement states what the code is doing, not why
>>>>> it's doing it which is the more crucial information that should be
>>>>> described in the commit text. Also, please add a comment about it to the
>>>>> code next to the flag.
>>>>>
>>>>> I am concerned that it doesn't make any sense though, so probably it
>>>>> shouldn't be marked as CLK_IGNORE_UNUSED and it's papering over some
>>>>> other larger bug that needs to be fixed.
>>>>>
>>>>
>>>> It does not have any bug, it is just that to access these lpass
>>>> registers we would need the GCC lpass registers to be enabled. I would
>>>> update the same in the commit text.
>>>>
>>>> During clock late_init these clocks should not be accessed to check the
>>>> clock status as they would result in unclocked access. The client would
>>>> request these clocks in the correct order and it would not have any issue.
>>>>
>>>
>>> That seems like the bug right there. If the LPASS registers can't be
>>> accessed unless the clks in GCC are enabled then this driver needs to
>>> turn the clks on before reading/writing registers. Marking the clks as
>>> ignore unused is skipping around the real problem.
>>>
>>
>> If the driver requests for the clocks they would maintain the order. But
>> if the clock late init call is invoked before the driver requests, there
>> is no way I could manage this dependency, that is the only reason to
>> mark them unused.
>>
> 
> Which driver are we talking about here? The lpass clk driver? Presumably
> the lpass clk driver would request the GCC clks and turn them on in
> probe and then register any lpass clks. If the lpass clk driver probes
> bfeore late init, then the gcc clks will be enabled and everything
> works, and if the lpass clk driver probes after late init then the clks
> that can't be touched without gcc clks enabled won't be registered, and
> then they won't be touched. What goes wrong?
> 
> 

Okay, sure, I will take the GCC clock handles and then enable/disable 
them accordingly.


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

--

Re: [PATCH v6] clk: qcom: Add lpass clock controller driver for SDM845

[Taniya Das]

On 10/17/2018 5:07 PM, Taniya Das wrote:
> Hello Stephen,
> 
> On 10/12/2018 11:05 PM, Stephen Boyd wrote:
>> Quoting Taniya Das (2018-10-09 23:12:27)
>>>
>>>
>>> On 10/10/2018 2:22 AM, Stephen Boyd wrote:
>>>> Quoting Taniya Das (2018-10-09 10:26:38)
>>>>> Hello Stephen,
>>>>>
>>>>> On 10/8/2018 8:14 AM, Stephen Boyd wrote:
>>>>>> Quoting Taniya Das (2018-10-04 05:02:26)
>>>>>>> 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.
>>>>>>> LPASS clocks present on the global clock controller would be 
>>>>>>> registered
>>>>>>> with the clock framework based on the device tree flag. Also do 
>>>>>>> not gate
>>>>>>> these clocks if they are left unused.
>>>>>>
>>>>>> Why not gate them? This statement states what the code is doing, 
>>>>>> not why
>>>>>> it's doing it which is the more crucial information that should be
>>>>>> described in the commit text. Also, please add a comment about it 
>>>>>> to the
>>>>>> code next to the flag.
>>>>>>
>>>>>> I am concerned that it doesn't make any sense though, so probably it
>>>>>> shouldn't be marked as CLK_IGNORE_UNUSED and it's papering over some
>>>>>> other larger bug that needs to be fixed.
>>>>>>
>>>>>
>>>>> It does not have any bug, it is just that to access these lpass
>>>>> registers we would need the GCC lpass registers to be enabled. I would
>>>>> update the same in the commit text.
>>>>>
>>>>> During clock late_init these clocks should not be accessed to check 
>>>>> the
>>>>> clock status as they would result in unclocked access. The client 
>>>>> would
>>>>> request these clocks in the correct order and it would not have any 
>>>>> issue.
>>>>>
>>>>
>>>> That seems like the bug right there. If the LPASS registers can't be
>>>> accessed unless the clks in GCC are enabled then this driver needs to
>>>> turn the clks on before reading/writing registers. Marking the clks as
>>>> ignore unused is skipping around the real problem.
>>>>
>>>
>>> If the driver requests for the clocks they would maintain the order. But
>>> if the clock late init call is invoked before the driver requests, there
>>> is no way I could manage this dependency, that is the only reason to
>>> mark them unused.
>>>
>>
>> Which driver are we talking about here? The lpass clk driver? Presumably
>> the lpass clk driver would request the GCC clks and turn them on in
>> probe and then register any lpass clks. If the lpass clk driver probes
>> bfeore late init, then the gcc clks will be enabled and everything
>> works, and if the lpass clk driver probes after late init then the clks
>> that can't be touched without gcc clks enabled won't be registered, and
>> then they won't be touched. What goes wrong?
>>
>>
> 
> Okay, sure, I will take the GCC clock handles and then enable/disable 
> them accordingly.
> 
> I missed earlier, so here is what you suggest

gcc_probe --> GCC LPASS clocks registered.
lpass_probe --> clk_get on gcc_lpass_clocks/ clk_prepare_enable -->
register the lpass clocks --> clk_disable_unprepare gcc_lpass_clocks.

But the problem is not during the above. It is the below
static void clk_disable_unused_subtree(struct clk_core *core)
{
....

         if (clk_core_is_enabled(core)) {  --> This access fails.
....

}

Please let me know your comments in case I missed something.
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.

--

Re: [PATCH v6] clk: qcom: Add lpass clock controller driver for SDM845

[Stephen Boyd]

Quoting Taniya Das (2018-10-17 05:04:10)
> 
> 
> On 10/17/2018 5:07 PM, Taniya Das wrote:
> > Hello Stephen,
> > 
> > On 10/12/2018 11:05 PM, Stephen Boyd wrote:
> >> Quoting Taniya Das (2018-10-09 23:12:27)
> >>>
> >>>
> >>> On 10/10/2018 2:22 AM, Stephen Boyd wrote:
> >>>> Quoting Taniya Das (2018-10-09 10:26:38)
> >>>>> Hello Stephen,
> >>>>>
> >>>>> On 10/8/2018 8:14 AM, Stephen Boyd wrote:
> >>>>>> Quoting Taniya Das (2018-10-04 05:02:26)
> >>>>>>> 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.
> >>>>>>> LPASS clocks present on the global clock controller would be 
> >>>>>>> registered
> >>>>>>> with the clock framework based on the device tree flag. Also do 
> >>>>>>> not gate
> >>>>>>> these clocks if they are left unused.
> >>>>>>
> >>>>>> Why not gate them? This statement states what the code is doing, 
> >>>>>> not why
> >>>>>> it's doing it which is the more crucial information that should be
> >>>>>> described in the commit text. Also, please add a comment about it 
> >>>>>> to the
> >>>>>> code next to the flag.
> >>>>>>
> >>>>>> I am concerned that it doesn't make any sense though, so probably it
> >>>>>> shouldn't be marked as CLK_IGNORE_UNUSED and it's papering over some
> >>>>>> other larger bug that needs to be fixed.
> >>>>>>
> >>>>>
> >>>>> It does not have any bug, it is just that to access these lpass
> >>>>> registers we would need the GCC lpass registers to be enabled. I would
> >>>>> update the same in the commit text.
> >>>>>
> >>>>> During clock late_init these clocks should not be accessed to check 
> >>>>> the
> >>>>> clock status as they would result in unclocked access. The client 
> >>>>> would
> >>>>> request these clocks in the correct order and it would not have any 
> >>>>> issue.
> >>>>>
> >>>>
> >>>> That seems like the bug right there. If the LPASS registers can't be
> >>>> accessed unless the clks in GCC are enabled then this driver needs to
> >>>> turn the clks on before reading/writing registers. Marking the clks as
> >>>> ignore unused is skipping around the real problem.
> >>>>
> >>>
> >>> If the driver requests for the clocks they would maintain the order. But
> >>> if the clock late init call is invoked before the driver requests, there
> >>> is no way I could manage this dependency, that is the only reason to
> >>> mark them unused.
> >>>
> >>
> >> Which driver are we talking about here? The lpass clk driver? Presumably
> >> the lpass clk driver would request the GCC clks and turn them on in
> >> probe and then register any lpass clks. If the lpass clk driver probes
> >> bfeore late init, then the gcc clks will be enabled and everything
> >> works, and if the lpass clk driver probes after late init then the clks
> >> that can't be touched without gcc clks enabled won't be registered, and
> >> then they won't be touched. What goes wrong?
> >>
> >>
> > 
> > Okay, sure, I will take the GCC clock handles and then enable/disable 
> > them accordingly.
> > 
> > I missed earlier, so here is what you suggest
> 
> gcc_probe --> GCC LPASS clocks registered.
> lpass_probe --> clk_get on gcc_lpass_clocks/ clk_prepare_enable -->
> register the lpass clocks --> clk_disable_unprepare gcc_lpass_clocks.

Why did the gcc_lpass_clocks get turned off? Shouldn't they just stay
enabled all the time?

> 
> But the problem is not during the above. It is the below
> static void clk_disable_unused_subtree(struct clk_core *core)
> {
> ....
> 
>          if (clk_core_is_enabled(core)) {  --> This access fails.
> ....
> 
> }
> 

You may need to add some prepare_ops to turn on clks needed to
read/write lpass registers. Or you can look into using some sort of
genpd to enable required clks when these clks are enabled or disabled.
But I suspect it would be easier to just leave the clks in GCC for lpass
always enabled and not worry about the complicated genpd things.

Re: [PATCH v6] clk: qcom: Add lpass clock controller driver for SDM845

[Taniya Das]

On 10/17/2018 7:50 PM, Stephen Boyd wrote:
> Quoting Taniya Das (2018-10-17 05:04:10)
>>
>>
>> On 10/17/2018 5:07 PM, Taniya Das wrote:
>>> Hello Stephen,
>>>
>>> On 10/12/2018 11:05 PM, Stephen Boyd wrote:
>>>> Quoting Taniya Das (2018-10-09 23:12:27)
>>>>>
>>>>>
>>>>> On 10/10/2018 2:22 AM, Stephen Boyd wrote:
>>>>>> Quoting Taniya Das (2018-10-09 10:26:38)
>>>>>>> Hello Stephen,
>>>>>>>
>>>>>>> On 10/8/2018 8:14 AM, Stephen Boyd wrote:
>>>>>>>> Quoting Taniya Das (2018-10-04 05:02:26)
>>>>>>>>> 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.
>>>>>>>>> LPASS clocks present on the global clock controller would be
>>>>>>>>> registered
>>>>>>>>> with the clock framework based on the device tree flag. Also do
>>>>>>>>> not gate
>>>>>>>>> these clocks if they are left unused.
>>>>>>>>
>>>>>>>> Why not gate them? This statement states what the code is doing,
>>>>>>>> not why
>>>>>>>> it's doing it which is the more crucial information that should be
>>>>>>>> described in the commit text. Also, please add a comment about it
>>>>>>>> to the
>>>>>>>> code next to the flag.
>>>>>>>>
>>>>>>>> I am concerned that it doesn't make any sense though, so probably it
>>>>>>>> shouldn't be marked as CLK_IGNORE_UNUSED and it's papering over some
>>>>>>>> other larger bug that needs to be fixed.
>>>>>>>>
>>>>>>>
>>>>>>> It does not have any bug, it is just that to access these lpass
>>>>>>> registers we would need the GCC lpass registers to be enabled. I would
>>>>>>> update the same in the commit text.
>>>>>>>
>>>>>>> During clock late_init these clocks should not be accessed to check
>>>>>>> the
>>>>>>> clock status as they would result in unclocked access. The client
>>>>>>> would
>>>>>>> request these clocks in the correct order and it would not have any
>>>>>>> issue.
>>>>>>>
>>>>>>
>>>>>> That seems like the bug right there. If the LPASS registers can't be
>>>>>> accessed unless the clks in GCC are enabled then this driver needs to
>>>>>> turn the clks on before reading/writing registers. Marking the clks as
>>>>>> ignore unused is skipping around the real problem.
>>>>>>
>>>>>
>>>>> If the driver requests for the clocks they would maintain the order. But
>>>>> if the clock late init call is invoked before the driver requests, there
>>>>> is no way I could manage this dependency, that is the only reason to
>>>>> mark them unused.
>>>>>
>>>>
>>>> Which driver are we talking about here? The lpass clk driver? Presumably
>>>> the lpass clk driver would request the GCC clks and turn them on in
>>>> probe and then register any lpass clks. If the lpass clk driver probes
>>>> bfeore late init, then the gcc clks will be enabled and everything
>>>> works, and if the lpass clk driver probes after late init then the clks
>>>> that can't be touched without gcc clks enabled won't be registered, and
>>>> then they won't be touched. What goes wrong?
>>>>
>>>>
>>>
>>> Okay, sure, I will take the GCC clock handles and then enable/disable
>>> them accordingly.
>>>
>>> I missed earlier, so here is what you suggest
>>
>> gcc_probe --> GCC LPASS clocks registered.
>> lpass_probe --> clk_get on gcc_lpass_clocks/ clk_prepare_enable -->
>> register the lpass clocks --> clk_disable_unprepare gcc_lpass_clocks.
> 
> Why did the gcc_lpass_clocks get turned off? Shouldn't they just stay
> enabled all the time?
> 

I don't think they are kept enabled all the time.

>>
>> But the problem is not during the above. It is the below
>> static void clk_disable_unused_subtree(struct clk_core *core)
>> {
>> ....
>>
>>           if (clk_core_is_enabled(core)) {  --> This access fails.
>> ....
>>
>> }
>>
> 
> You may need to add some prepare_ops to turn on clks needed to
> read/write lpass registers. Or you can look into using some sort of
> genpd to enable required clks when these clks are enabled or disabled.
> But I suspect it would be easier to just leave the clks in GCC for lpass
> always enabled and not worry about the complicated genpd things.
> 

I need to check if keeping them enabled/marking them CRITICAL could have 
an impact on the reset of the subsystem.

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

--

Re: [PATCH v6] clk: qcom: Add lpass clock controller driver for SDM845

[tdas]

On 2018-10-19 16:09, Taniya Das wrote:
> On 10/17/2018 7:50 PM, Stephen Boyd wrote:
>> Quoting Taniya Das (2018-10-17 05:04:10)
>>> 
>>> 
>>> On 10/17/2018 5:07 PM, Taniya Das wrote:
>>>> Hello Stephen,
>>>> 
>>>> On 10/12/2018 11:05 PM, Stephen Boyd wrote:
>>>>> Quoting Taniya Das (2018-10-09 23:12:27)
>>>>>> 
>>>>>> 
>>>>>> On 10/10/2018 2:22 AM, Stephen Boyd wrote:
>>>>>>> Quoting Taniya Das (2018-10-09 10:26:38)
>>>>>>>> Hello Stephen,
>>>>>>>> 
>>>>>>>> On 10/8/2018 8:14 AM, Stephen Boyd wrote:
>>>>>>>>> Quoting Taniya Das (2018-10-04 05:02:26)
>>>>>>>>>> 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.
>>>>>>>>>> LPASS clocks present on the global clock controller would be
>>>>>>>>>> registered
>>>>>>>>>> with the clock framework based on the device tree flag. Also 
>>>>>>>>>> do
>>>>>>>>>> not gate
>>>>>>>>>> these clocks if they are left unused.
>>>>>>>>> 
>>>>>>>>> Why not gate them? This statement states what the code is 
>>>>>>>>> doing,
>>>>>>>>> not why
>>>>>>>>> it's doing it which is the more crucial information that should 
>>>>>>>>> be
>>>>>>>>> described in the commit text. Also, please add a comment about 
>>>>>>>>> it
>>>>>>>>> to the
>>>>>>>>> code next to the flag.
>>>>>>>>> 
>>>>>>>>> I am concerned that it doesn't make any sense though, so 
>>>>>>>>> probably it
>>>>>>>>> shouldn't be marked as CLK_IGNORE_UNUSED and it's papering over 
>>>>>>>>> some
>>>>>>>>> other larger bug that needs to be fixed.
>>>>>>>>> 
>>>>>>>> 
>>>>>>>> It does not have any bug, it is just that to access these lpass
>>>>>>>> registers we would need the GCC lpass registers to be enabled. I 
>>>>>>>> would
>>>>>>>> update the same in the commit text.
>>>>>>>> 
>>>>>>>> During clock late_init these clocks should not be accessed to 
>>>>>>>> check
>>>>>>>> the
>>>>>>>> clock status as they would result in unclocked access. The 
>>>>>>>> client
>>>>>>>> would
>>>>>>>> request these clocks in the correct order and it would not have 
>>>>>>>> any
>>>>>>>> issue.
>>>>>>>> 
>>>>>>> 
>>>>>>> That seems like the bug right there. If the LPASS registers can't 
>>>>>>> be
>>>>>>> accessed unless the clks in GCC are enabled then this driver 
>>>>>>> needs to
>>>>>>> turn the clks on before reading/writing registers. Marking the 
>>>>>>> clks as
>>>>>>> ignore unused is skipping around the real problem.
>>>>>>> 
>>>>>> 
>>>>>> If the driver requests for the clocks they would maintain the 
>>>>>> order. But
>>>>>> if the clock late init call is invoked before the driver requests, 
>>>>>> there
>>>>>> is no way I could manage this dependency, that is the only reason 
>>>>>> to
>>>>>> mark them unused.
>>>>>> 
>>>>> 
>>>>> Which driver are we talking about here? The lpass clk driver? 
>>>>> Presumably
>>>>> the lpass clk driver would request the GCC clks and turn them on in
>>>>> probe and then register any lpass clks. If the lpass clk driver 
>>>>> probes
>>>>> bfeore late init, then the gcc clks will be enabled and everything
>>>>> works, and if the lpass clk driver probes after late init then the 
>>>>> clks
>>>>> that can't be touched without gcc clks enabled won't be registered, 
>>>>> and
>>>>> then they won't be touched. What goes wrong?
>>>>> 
>>>>> 
>>>> 
>>>> Okay, sure, I will take the GCC clock handles and then 
>>>> enable/disable
>>>> them accordingly.
>>>> 
>>>> I missed earlier, so here is what you suggest
>>> 
>>> gcc_probe --> GCC LPASS clocks registered.
>>> lpass_probe --> clk_get on gcc_lpass_clocks/ clk_prepare_enable -->
>>> register the lpass clocks --> clk_disable_unprepare gcc_lpass_clocks.
>> 
>> Why did the gcc_lpass_clocks get turned off? Shouldn't they just stay
>> enabled all the time?
>> 
> 
> I don't think they are kept enabled all the time.
> 
>>> 
>>> But the problem is not during the above. It is the below
>>> static void clk_disable_unused_subtree(struct clk_core *core)
>>> {
>>> ....
>>> 
>>>           if (clk_core_is_enabled(core)) {  --> This access fails.
>>> ....
>>> 
>>> }
>>> 
>> 
>> You may need to add some prepare_ops to turn on clks needed to
>> read/write lpass registers. Or you can look into using some sort of
>> genpd to enable required clks when these clks are enabled or disabled.
>> But I suspect it would be easier to just leave the clks in GCC for 
>> lpass
>> always enabled and not worry about the complicated genpd things.
>> 
> 
> I need to check if keeping them enabled/marking them CRITICAL could
> have an impact on the reset of the subsystem.

I have checked internally with the teams and the GCC LPASS clocks could 
be left enabled.
Would submit a patch keeping them CRITICAL.

Re: [PATCH v6] clk: qcom: Add lpass clock controller driver for SDM845

[Stephen Boyd]

Quoting tdas@codeaurora.org (2018-10-25 03:51:01)
> On 2018-10-19 16:09, Taniya Das wrote:
> > On 10/17/2018 7:50 PM, Stephen Boyd wrote:
> >> Quoting Taniya Das (2018-10-17 05:04:10)
> >>> 
> >>> 
> >>> 
> >>> But the problem is not during the above. It is the below
> >>> static void clk_disable_unused_subtree(struct clk_core *core)
> >>> {
> >>> ....
> >>> 
> >>>           if (clk_core_is_enabled(core)) {  --> This access fails.
> >>> ....
> >>> 
> >>> }
> >>> 
> >> 
> >> You may need to add some prepare_ops to turn on clks needed to
> >> read/write lpass registers. Or you can look into using some sort of
> >> genpd to enable required clks when these clks are enabled or disabled.
> >> But I suspect it would be easier to just leave the clks in GCC for 
> >> lpass
> >> always enabled and not worry about the complicated genpd things.
> >> 
> > 
> > I need to check if keeping them enabled/marking them CRITICAL could
> > have an impact on the reset of the subsystem.
> 
> I have checked internally with the teams and the GCC LPASS clocks could 
> be left enabled.
> Would submit a patch keeping them CRITICAL.

Awesome! Thanks for checking.