[PATCH v2 0/5] Add APCS support for SDX55

[PATCH v2 0/5] Add APCS support for SDX55

[Manivannan Sadhasivam]

Hello,

This series adds APCS mailbox and clock support for SDX55. The APCS IP
in SDX55 provides IPC and clock functionalities. Hence, mailbox support
is added to the "qcom-apcs-ipc-mailbox" driver and a dedicated clock
driver "apcs-sdx55" is added.

Also, the clock to the APCS block is coming from 3 different sources:

1. Board XO
2. Fixed rate GPLL0
3. A7 PLL

First source is from crystal osc, second is from GCC and third one is a
separate clock source. Hence, a dedicated clk driver is added for the A7
PLL as well.

Apart from the mailbox support, another intention of this series is to add
the CPUFreq support to SDX55 platform. Since there is no dedicated hardware
IP in SDX55 to do CPUFreq duties, this platform makes use of the clock and
regulators directly via cpufreq-dt driver.

The trick here is attaching the power domain to cpudev. Usually the power
domains for the target device is attached in the bus driver or in the
dedicated device drivers. But in this case, there is no dedicated CPUFreq
driver nor a bus driver. After discussing with Viresh, I concluded that
A7 PLL driver might be the best place to do this!

But this decision is subject to discussion, hence added Ulf and Viresh to
this series.

Thanks,
Mani

Changes in v2:

* Modified the max_register value as per the SDX55 IPC offset in mailbox
  driver.

Manivannan Sadhasivam (5):
  dt-bindings: mailbox: Add binding for SDX55 APCS
  mailbox: qcom: Add support for SDX55 APCS IPC
  dt-bindings: clock: Add Qualcomm A7 PLL binding
  clk: qcom: Add A7 PLL support
  clk: qcom: Add SDX55 APCS clock controller support

 .../devicetree/bindings/clock/qcom,a7pll.yaml |  51 ++++++
 .../mailbox/qcom,apcs-kpss-global.yaml        |  59 +++++--
 drivers/clk/qcom/Kconfig                      |  17 ++
 drivers/clk/qcom/Makefile                     |   2 +
 drivers/clk/qcom/a7-pll.c                     | 100 ++++++++++++
 drivers/clk/qcom/apcs-sdx55.c                 | 149 ++++++++++++++++++
 drivers/mailbox/qcom-apcs-ipc-mailbox.c       |   7 +-
 7 files changed, 375 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/qcom,a7pll.yaml
 create mode 100644 drivers/clk/qcom/a7-pll.c
 create mode 100644 drivers/clk/qcom/apcs-sdx55.c

-- 
2.25.1

[PATCH v2 1/5] dt-bindings: mailbox: Add binding for SDX55 APCS

[Manivannan Sadhasivam]

Add devicetree YAML binding for SDX55 APCS GCC block. The APCS block
acts as the mailbox controller and also provides a clock output and
takes 3 clock sources (pll, aux, ref) as input.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 .../mailbox/qcom,apcs-kpss-global.yaml        | 59 ++++++++++++++++---
 1 file changed, 50 insertions(+), 9 deletions(-)

diff --git a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml
index ffd09b664ff5..3c75ea0b6040 100644
--- a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml
+++ b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml
@@ -27,26 +27,24 @@ properties:
       - qcom,sdm660-apcs-hmss-global
       - qcom,sdm845-apss-shared
       - qcom,sm8150-apss-shared
+      - qcom,sdx55-apcs-gcc
 
   reg:
     maxItems: 1
 
-  clocks:
-    description: phandles to the parent clocks of the clock driver
-    items:
-      - description: primary pll parent of the clock driver
-      - description: auxiliary parent
-
   '#mbox-cells':
     const: 1
 
   '#clock-cells':
     const: 0
 
+  clocks:
+    minItems: 2
+    maxItems: 3
+
   clock-names:
-    items:
-      - const: pll
-      - const: aux
+    minItems: 2
+    maxItems: 3
 
 required:
   - compatible
@@ -55,6 +53,49 @@ required:
 
 additionalProperties: false
 
+allOf:
+  - if:
+      properties:
+        compatible:
+          enum:
+            - qcom,ipq6018-apcs-apps-global
+            - qcom,ipq8074-apcs-apps-global
+            - qcom,msm8916-apcs-kpss-global
+            - qcom,msm8994-apcs-kpss-global
+            - qcom,msm8996-apcs-hmss-global
+            - qcom,msm8998-apcs-hmss-global
+            - qcom,qcs404-apcs-apps-global
+            - qcom,sc7180-apss-shared
+            - qcom,sdm660-apcs-hmss-global
+            - qcom,sdm845-apss-shared
+            - qcom,sm8150-apss-shared
+    then:
+      properties:
+        clocks:
+          items:
+            - description: Primary PLL parent of the clock driver
+            - description: Auxiliary parent
+        clock-names:
+          items:
+            - const: pll
+            - const: aux
+  - if:
+      properties:
+        compatible:
+          enum:
+            - qcom,sdx55-apcs-gcc
+    then:
+      properties:
+        clocks:
+          items:
+            - description: Primary PLL parent of the clock driver
+            - description: Auxiliary parent
+            - description: Reference clock
+        clock-names:
+          items:
+            - const: pll
+            - const: aux
+            - const: ref
 examples:
 
   # Example apcs with msm8996
-- 
2.25.1

[PATCH v2 2/5] mailbox: qcom: Add support for SDX55 APCS IPC

[Manivannan Sadhasivam]

In SDX55, the IPC bits are located in the APCS GCC block. Also, this block
can provide clock functionality. Hence, add support for IPC with correct
offset and name of the clock provider.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/mailbox/qcom-apcs-ipc-mailbox.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/mailbox/qcom-apcs-ipc-mailbox.c b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
index 077e5c6a9ef7..1c205832a1cc 100644
--- a/drivers/mailbox/qcom-apcs-ipc-mailbox.c
+++ b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
@@ -61,11 +61,15 @@ static const struct qcom_apcs_ipc_data apps_shared_apcs_data = {
 	.offset = 12, .clk_name = NULL
 };
 
+static const struct qcom_apcs_ipc_data sdx55_apcs_data = {
+	.offset = 0x1008, .clk_name = "qcom-sdx55-acps-clk"
+};
+
 static const struct regmap_config apcs_regmap_config = {
 	.reg_bits = 32,
 	.reg_stride = 4,
 	.val_bits = 32,
-	.max_register = 0xFFC,
+	.max_register = 0x1008,
 	.fast_io = true,
 };
 
@@ -162,6 +166,7 @@ static const struct of_device_id qcom_apcs_ipc_of_match[] = {
 	{ .compatible = "qcom,sdm660-apcs-hmss-global", .data = &sdm660_apcs_data },
 	{ .compatible = "qcom,sdm845-apss-shared", .data = &apps_shared_apcs_data },
 	{ .compatible = "qcom,sm8150-apss-shared", .data = &apps_shared_apcs_data },
+	{ .compatible = "qcom,sdx55-apcs-gcc", .data = &sdx55_apcs_data },
 	{}
 };
 MODULE_DEVICE_TABLE(of, qcom_apcs_ipc_of_match);
-- 
2.25.1

[PATCH v2 3/5] dt-bindings: clock: Add Qualcomm A7 PLL binding

[Manivannan Sadhasivam]

Add devicetree YAML binding for Cortex A7 PLL clock in Qualcomm
platforms like SDX55.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 .../devicetree/bindings/clock/qcom,a7pll.yaml | 51 +++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/qcom,a7pll.yaml

diff --git a/Documentation/devicetree/bindings/clock/qcom,a7pll.yaml b/Documentation/devicetree/bindings/clock/qcom,a7pll.yaml
new file mode 100644
index 000000000000..8666e995725f
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/qcom,a7pll.yaml
@@ -0,0 +1,51 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/qcom,a7pll.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm A7 PLL Binding
+
+maintainers:
+  - Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
+
+description:
+  The A7 PLL on the Qualcomm platforms like SDX55 is used to provide high
+  frequency clock to the CPU.
+
+properties:
+  compatible:
+    enum:
+      - qcom,sdx55-a7pll
+
+  reg:
+    maxItems: 1
+
+  '#clock-cells':
+    const: 0
+
+  clocks:
+    items:
+      - description: board XO clock
+
+  clock-names:
+    items:
+      - const: bi_tcxo
+
+required:
+  - compatible
+  - reg
+  - '#clock-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/qcom,rpmh.h>
+    a7pll: clock@17808000 {
+        compatible = "qcom,sdx55-a7pll";
+        reg = <0x17808000 0x1000>;
+        clocks = <&rpmhcc RPMH_CXO_CLK>;
+        clock-names = "bi_tcxo";
+        #clock-cells = <0>;
+    };
-- 
2.25.1

[PATCH v2 4/5] clk: qcom: Add A7 PLL support

[Manivannan Sadhasivam]

Add support for PLL found in Qualcomm SDX55 platforms which is used to
provide clock to the Cortex A7 CPU via a mux. This PLL can provide high
frequency clock to the CPU above 1GHz as compared to the other sources
like GPLL0.

In this driver, the power domain is attached to the cpudev. This is
required for CPUFreq functionality and there seems to be no better place
to do other than this driver (no dedicated CPUFreq driver).

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/clk/qcom/Kconfig  |   8 +++
 drivers/clk/qcom/Makefile |   1 +
 drivers/clk/qcom/a7-pll.c | 100 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 109 insertions(+)
 create mode 100644 drivers/clk/qcom/a7-pll.c

diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index d32bb12cd8d0..d6f4aee4427a 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -28,6 +28,14 @@ config QCOM_A53PLL
 	  Say Y if you want to support higher CPU frequencies on MSM8916
 	  devices.
 
+config QCOM_A7PLL
+	tristate "SDX55 A7 PLL"
+	help
+	  Support for the A7 PLL on SDX55 devices. It provides the CPU with
+	  frequencies above 1GHz.
+	  Say Y if you want to support higher CPU frequencies on SDX55
+	  devices.
+
 config QCOM_CLK_APCS_MSM8916
 	tristate "MSM8916 APCS Clock Controller"
 	depends on QCOM_APCS_IPC || COMPILE_TEST
diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
index 9e5e0e3cb7b4..e7e0ac382176 100644
--- a/drivers/clk/qcom/Makefile
+++ b/drivers/clk/qcom/Makefile
@@ -44,6 +44,7 @@ obj-$(CONFIG_MSM_MMCC_8974) += mmcc-msm8974.o
 obj-$(CONFIG_MSM_MMCC_8996) += mmcc-msm8996.o
 obj-$(CONFIG_MSM_MMCC_8998) += mmcc-msm8998.o
 obj-$(CONFIG_QCOM_A53PLL) += a53-pll.o
+obj-$(CONFIG_QCOM_A7PLL) += a7-pll.o
 obj-$(CONFIG_QCOM_CLK_APCS_MSM8916) += apcs-msm8916.o
 obj-$(CONFIG_QCOM_CLK_APCC_MSM8996) += clk-cpu-8996.o
 obj-$(CONFIG_QCOM_CLK_RPM) += clk-rpm.o
diff --git a/drivers/clk/qcom/a7-pll.c b/drivers/clk/qcom/a7-pll.c
new file mode 100644
index 000000000000..e171d3caf2cf
--- /dev/null
+++ b/drivers/clk/qcom/a7-pll.c
@@ -0,0 +1,100 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Qualcomm A7 PLL driver
+ *
+ * Copyright (c) 2020, Linaro Limited
+ * Author: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#include "clk-alpha-pll.h"
+
+#define LUCID_PLL_OFF_L_VAL 0x04
+
+static const struct pll_vco lucid_vco[] = {
+	{ 249600000, 2000000000, 0 },
+};
+
+static struct clk_alpha_pll a7pll = {
+	.offset = 0x100,
+	.vco_table = lucid_vco,
+	.num_vco = ARRAY_SIZE(lucid_vco),
+	.regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_LUCID],
+	.clkr = {
+		.hw.init = &(struct clk_init_data){
+			.name = "a7pll",
+			.parent_data =  &(const struct clk_parent_data){
+				.fw_name = "bi_tcxo",
+			},
+			.num_parents = 1,
+			.ops = &clk_alpha_pll_lucid_ops,
+		},
+	},
+};
+
+static const struct alpha_pll_config a7pll_config = {
+	.l = 0x39,
+	.config_ctl_val = 0x20485699,
+	.config_ctl_hi_val = 0x2261,
+	.config_ctl_hi1_val = 0x029A699C,
+	.user_ctl_val = 0x1,
+	.user_ctl_hi_val = 0x805,
+};
+
+static const struct regmap_config a7pll_regmap_config = {
+	.reg_bits		= 32,
+	.reg_stride		= 4,
+	.val_bits		= 32,
+	.max_register		= 0x1000,
+	.fast_io		= true,
+};
+
+static int qcom_a7pll_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct regmap *regmap;
+	void __iomem *base;
+	u32 l_val;
+	int ret;
+
+	base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	regmap = devm_regmap_init_mmio(dev, base, &a7pll_regmap_config);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	/* Configure PLL only if the l_val is zero */
+	regmap_read(regmap, a7pll.offset + LUCID_PLL_OFF_L_VAL, &l_val);
+	if (!l_val)
+		clk_lucid_pll_configure(&a7pll, regmap, &a7pll_config);
+
+	ret = devm_clk_register_regmap(dev, &a7pll.clkr);
+	if (ret)
+		return ret;
+
+	return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get,
+					   &a7pll.clkr.hw);
+}
+
+static const struct of_device_id qcom_a7pll_match_table[] = {
+	{ .compatible = "qcom,sdx55-a7pll" },
+	{ }
+};
+
+static struct platform_driver qcom_a7pll_driver = {
+	.probe = qcom_a7pll_probe,
+	.driver = {
+		.name = "qcom-a7pll",
+		.of_match_table = qcom_a7pll_match_table,
+	},
+};
+module_platform_driver(qcom_a7pll_driver);
+
+MODULE_DESCRIPTION("Qualcomm A7 PLL Driver");
+MODULE_LICENSE("GPL v2");
-- 
2.25.1

[PATCH v2 5/5] clk: qcom: Add SDX55 APCS clock controller support

[Manivannan Sadhasivam]

Add a driver for the SDX55 APCS clock controller. It is part of the APCS
hardware block, which among other things implements also a combined mux
and half integer divider functionality. The APCS clock controller has 3
parent clocks:

1. Board XO
2. Fixed rate GPLL0
3. A7 PLL

The source and the divider can be set both at the same time.

This is required for enabling CPU frequency scaling on SDX55-based
platforms.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/clk/qcom/Kconfig      |   9 ++
 drivers/clk/qcom/Makefile     |   1 +
 drivers/clk/qcom/apcs-sdx55.c | 149 ++++++++++++++++++++++++++++++++++
 3 files changed, 159 insertions(+)
 create mode 100644 drivers/clk/qcom/apcs-sdx55.c

diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index d6f4aee4427a..2c67fdfae913 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -45,6 +45,15 @@ config QCOM_CLK_APCS_MSM8916
 	  Say Y if you want to support CPU frequency scaling on devices
 	  such as msm8916.
 
+config QCOM_CLK_APCS_SDX55
+	tristate "SDX55 APCS Clock Controller"
+	depends on QCOM_APCS_IPC || COMPILE_TEST
+	help
+	  Support for the APCS Clock Controller on SDX55 platform. The
+	  APCS is managing the mux and divider which feeds the CPUs.
+	  Say Y if you want to support CPU frequency scaling on devices
+	  such as SDX55.
+
 config QCOM_CLK_APCC_MSM8996
 	tristate "MSM8996 CPU Clock Controller"
 	select QCOM_KRYO_L2_ACCESSORS
diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
index e7e0ac382176..a9271f40916c 100644
--- a/drivers/clk/qcom/Makefile
+++ b/drivers/clk/qcom/Makefile
@@ -46,6 +46,7 @@ obj-$(CONFIG_MSM_MMCC_8998) += mmcc-msm8998.o
 obj-$(CONFIG_QCOM_A53PLL) += a53-pll.o
 obj-$(CONFIG_QCOM_A7PLL) += a7-pll.o
 obj-$(CONFIG_QCOM_CLK_APCS_MSM8916) += apcs-msm8916.o
+obj-$(CONFIG_QCOM_CLK_APCS_SDX55) += apcs-sdx55.o
 obj-$(CONFIG_QCOM_CLK_APCC_MSM8996) += clk-cpu-8996.o
 obj-$(CONFIG_QCOM_CLK_RPM) += clk-rpm.o
 obj-$(CONFIG_QCOM_CLK_RPMH) += clk-rpmh.o
diff --git a/drivers/clk/qcom/apcs-sdx55.c b/drivers/clk/qcom/apcs-sdx55.c
new file mode 100644
index 000000000000..14413c957d83
--- /dev/null
+++ b/drivers/clk/qcom/apcs-sdx55.c
@@ -0,0 +1,149 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Qualcomm SDX55 APCS clock controller driver
+ *
+ * Copyright (c) 2020, Linaro Limited
+ * Author: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/cpu.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#include "clk-regmap.h"
+#include "clk-regmap-mux-div.h"
+#include "common.h"
+
+static const u32 apcs_mux_clk_parent_map[] = { 0, 1, 5 };
+
+static const struct clk_parent_data pdata[] = {
+	{ .fw_name = "ref", .name = "bi_tcxo", },
+	{ .fw_name = "aux", .name = "gpll0", },
+	{ .fw_name = "pll", .name = "a7pll", },
+};
+
+/*
+ * We use the notifier function for switching to a temporary safe configuration
+ * (mux and divider), while the A7 PLL is reconfigured.
+ */
+static int a7cc_notifier_cb(struct notifier_block *nb, unsigned long event,
+			    void *data)
+{
+	int ret = 0;
+	struct clk_regmap_mux_div *md = container_of(nb,
+						     struct clk_regmap_mux_div,
+						     clk_nb);
+	if (event == PRE_RATE_CHANGE)
+		/* set the mux and divider to safe frequency (400mhz) */
+		ret = mux_div_set_src_div(md, 1, 2);
+
+	return notifier_from_errno(ret);
+}
+
+static int qcom_apcs_sdx55_clk_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device *parent = dev->parent;
+	struct device *cpu_dev;
+	struct clk_regmap_mux_div *a7cc;
+	struct regmap *regmap;
+	struct clk_init_data init = { };
+	int ret = -ENODEV;
+
+	regmap = dev_get_regmap(parent, NULL);
+	if (!regmap) {
+		dev_err(dev, "Failed to get parent regmap: %d\n", ret);
+		return ret;
+	}
+
+	a7cc = devm_kzalloc(dev, sizeof(*a7cc), GFP_KERNEL);
+	if (!a7cc)
+		return -ENOMEM;
+
+	init.name = "a7mux";
+	init.parent_data = pdata;
+	init.num_parents = ARRAY_SIZE(pdata);
+	init.ops = &clk_regmap_mux_div_ops;
+
+	a7cc->clkr.hw.init = &init;
+	a7cc->clkr.regmap = regmap;
+	a7cc->reg_offset = 0x8;
+	a7cc->hid_width = 5;
+	a7cc->hid_shift = 0;
+	a7cc->src_width = 3;
+	a7cc->src_shift = 8;
+	a7cc->parent_map = apcs_mux_clk_parent_map;
+
+	a7cc->pclk = devm_clk_get(parent, "pll");
+	if (IS_ERR(a7cc->pclk)) {
+		ret = PTR_ERR(a7cc->pclk);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "Failed to get PLL clk: %d\n", ret);
+		return ret;
+	}
+
+	a7cc->clk_nb.notifier_call = a7cc_notifier_cb;
+	ret = clk_notifier_register(a7cc->pclk, &a7cc->clk_nb);
+	if (ret) {
+		dev_err(dev, "Failed to register clock notifier: %d\n", ret);
+		return ret;
+	}
+
+	ret = devm_clk_register_regmap(dev, &a7cc->clkr);
+	if (ret) {
+		dev_err(dev, "Failed to register regmap clock: %d\n", ret);
+		goto err;
+	}
+
+	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get,
+					  &a7cc->clkr.hw);
+	if (ret) {
+		dev_err(dev, "Failed to add clock provider: %d\n", ret);
+		goto err;
+	}
+
+	platform_set_drvdata(pdev, a7cc);
+
+	/*
+	 * Attach the power domain to cpudev. There seems to be no better place
+	 * to do this, so do it here.
+	 */
+	cpu_dev = get_cpu_device(0);
+	dev_pm_domain_attach(cpu_dev, true);
+
+	return 0;
+
+err:
+	clk_notifier_unregister(a7cc->pclk, &a7cc->clk_nb);
+	return ret;
+}
+
+static int qcom_apcs_sdx55_clk_remove(struct platform_device *pdev)
+{
+	struct device *cpu_dev = get_cpu_device(0);
+	struct clk_regmap_mux_div *a7cc = platform_get_drvdata(pdev);
+
+	clk_notifier_unregister(a7cc->pclk, &a7cc->clk_nb);
+	dev_pm_domain_detach(cpu_dev, true);
+
+	return 0;
+}
+
+static struct platform_driver qcom_apcs_sdx55_clk_driver = {
+	.probe = qcom_apcs_sdx55_clk_probe,
+	.remove = qcom_apcs_sdx55_clk_remove,
+	.driver = {
+		.name = "qcom-sdx55-acps-clk",
+	},
+};
+module_platform_driver(qcom_apcs_sdx55_clk_driver);
+
+MODULE_AUTHOR("Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Qualcomm SDX55 APCS clock driver");
-- 
2.25.1

Re: [PATCH v2 1/5] dt-bindings: mailbox: Add binding for SDX55 APCS

[Rob Herring]

On Fri, Jan 08, 2021 at 05:02:29PM +0530, Manivannan Sadhasivam wrote:
> Add devicetree YAML binding for SDX55 APCS GCC block. The APCS block
> acts as the mailbox controller and also provides a clock output and
> takes 3 clock sources (pll, aux, ref) as input.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  .../mailbox/qcom,apcs-kpss-global.yaml        | 59 ++++++++++++++++---
>  1 file changed, 50 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml
> index ffd09b664ff5..3c75ea0b6040 100644
> --- a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml
> +++ b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml
> @@ -27,26 +27,24 @@ properties:
>        - qcom,sdm660-apcs-hmss-global
>        - qcom,sdm845-apss-shared
>        - qcom,sm8150-apss-shared
> +      - qcom,sdx55-apcs-gcc
>  
>    reg:
>      maxItems: 1
>  
> -  clocks:
> -    description: phandles to the parent clocks of the clock driver
> -    items:
> -      - description: primary pll parent of the clock driver
> -      - description: auxiliary parent

Keep this here and add the 3rd item and:

minItems: 2

Then the if/then can just restrict things to 2 or 3 items.

> -
>    '#mbox-cells':
>      const: 1
>  
>    '#clock-cells':
>      const: 0
>  
> +  clocks:
> +    minItems: 2
> +    maxItems: 3
> +
>    clock-names:
> -    items:
> -      - const: pll
> -      - const: aux
> +    minItems: 2
> +    maxItems: 3
>  
>  required:
>    - compatible
> @@ -55,6 +53,49 @@ required:
>  
>  additionalProperties: false
>  
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          enum:
> +            - qcom,ipq6018-apcs-apps-global
> +            - qcom,ipq8074-apcs-apps-global
> +            - qcom,msm8916-apcs-kpss-global
> +            - qcom,msm8994-apcs-kpss-global
> +            - qcom,msm8996-apcs-hmss-global
> +            - qcom,msm8998-apcs-hmss-global
> +            - qcom,qcs404-apcs-apps-global
> +            - qcom,sc7180-apss-shared
> +            - qcom,sdm660-apcs-hmss-global
> +            - qcom,sdm845-apss-shared
> +            - qcom,sm8150-apss-shared
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: Primary PLL parent of the clock driver
> +            - description: Auxiliary parent
> +        clock-names:
> +          items:
> +            - const: pll
> +            - const: aux
> +  - if:
> +      properties:
> +        compatible:
> +          enum:
> +            - qcom,sdx55-apcs-gcc
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: Primary PLL parent of the clock driver
> +            - description: Auxiliary parent
> +            - description: Reference clock
> +        clock-names:
> +          items:
> +            - const: pll
> +            - const: aux
> +            - const: ref
>  examples:
>  
>    # Example apcs with msm8996
> -- 
> 2.25.1
> 

Re: [PATCH v2 3/5] dt-bindings: clock: Add Qualcomm A7 PLL binding

[Rob Herring]

On Fri, 08 Jan 2021 17:02:31 +0530, Manivannan Sadhasivam wrote:
> Add devicetree YAML binding for Cortex A7 PLL clock in Qualcomm
> platforms like SDX55.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  .../devicetree/bindings/clock/qcom,a7pll.yaml | 51 +++++++++++++++++++
>  1 file changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/qcom,a7pll.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 5/5] clk: qcom: Add SDX55 APCS clock controller support

[Stephen Boyd]

Quoting Manivannan Sadhasivam (2021-01-08 03:32:33)
> Add a driver for the SDX55 APCS clock controller. It is part of the APCS
> hardware block, which among other things implements also a combined mux
> and half integer divider functionality. The APCS clock controller has 3
> parent clocks:
> 
> 1. Board XO
> 2. Fixed rate GPLL0
> 3. A7 PLL
> 
> The source and the divider can be set both at the same time.

I don't understand what that means. Presumably it's a mux/divider
combined?

> 
> This is required for enabling CPU frequency scaling on SDX55-based
> platforms.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/clk/qcom/Kconfig      |   9 ++
>  drivers/clk/qcom/Makefile     |   1 +
>  drivers/clk/qcom/apcs-sdx55.c | 149 ++++++++++++++++++++++++++++++++++
>  3 files changed, 159 insertions(+)
>  create mode 100644 drivers/clk/qcom/apcs-sdx55.c
> 
> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index d6f4aee4427a..2c67fdfae913 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -45,6 +45,15 @@ config QCOM_CLK_APCS_MSM8916
>           Say Y if you want to support CPU frequency scaling on devices
>           such as msm8916.
>  
> +config QCOM_CLK_APCS_SDX55

APCC comes before APCS

> +       tristate "SDX55 APCS Clock Controller"
> +       depends on QCOM_APCS_IPC || COMPILE_TEST
> +       help
> +         Support for the APCS Clock Controller on SDX55 platform. The
> +         APCS is managing the mux and divider which feeds the CPUs.
> +         Say Y if you want to support CPU frequency scaling on devices
> +         such as SDX55.
> +
>  config QCOM_CLK_APCC_MSM8996
>         tristate "MSM8996 CPU Clock Controller"
>         select QCOM_KRYO_L2_ACCESSORS
> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
> index e7e0ac382176..a9271f40916c 100644
> --- a/drivers/clk/qcom/Makefile
> +++ b/drivers/clk/qcom/Makefile
> @@ -46,6 +46,7 @@ obj-$(CONFIG_MSM_MMCC_8998) += mmcc-msm8998.o
>  obj-$(CONFIG_QCOM_A53PLL) += a53-pll.o
>  obj-$(CONFIG_QCOM_A7PLL) += a7-pll.o
>  obj-$(CONFIG_QCOM_CLK_APCS_MSM8916) += apcs-msm8916.o
> +obj-$(CONFIG_QCOM_CLK_APCS_SDX55) += apcs-sdx55.o
>  obj-$(CONFIG_QCOM_CLK_APCC_MSM8996) += clk-cpu-8996.o
>  obj-$(CONFIG_QCOM_CLK_RPM) += clk-rpm.o
>  obj-$(CONFIG_QCOM_CLK_RPMH) += clk-rpmh.o
> diff --git a/drivers/clk/qcom/apcs-sdx55.c b/drivers/clk/qcom/apcs-sdx55.c
> new file mode 100644
> index 000000000000..14413c957d83
> --- /dev/null
> +++ b/drivers/clk/qcom/apcs-sdx55.c
> @@ -0,0 +1,149 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Qualcomm SDX55 APCS clock controller driver
> + *
> + * Copyright (c) 2020, Linaro Limited
> + * Author: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/cpu.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +#include "clk-regmap.h"
> +#include "clk-regmap-mux-div.h"
> +#include "common.h"

Curious what common is needed for?

> +
> +static const u32 apcs_mux_clk_parent_map[] = { 0, 1, 5 };
> +
> +static const struct clk_parent_data pdata[] = {
> +       { .fw_name = "ref", .name = "bi_tcxo", },
> +       { .fw_name = "aux", .name = "gpll0", },
> +       { .fw_name = "pll", .name = "a7pll", },

Please remove name from here. It shouldn't be necessary if the DT
describes things properly. Or there isn't DT for this device?

> +};
> +
> +/*
> + * We use the notifier function for switching to a temporary safe configuration
> + * (mux and divider), while the A7 PLL is reconfigured.
> + */
> +static int a7cc_notifier_cb(struct notifier_block *nb, unsigned long event,
> +                           void *data)
> +{
> +       int ret = 0;
> +       struct clk_regmap_mux_div *md = container_of(nb,
> +                                                    struct clk_regmap_mux_div,
> +                                                    clk_nb);
> +       if (event == PRE_RATE_CHANGE)
> +               /* set the mux and divider to safe frequency (400mhz) */
> +               ret = mux_div_set_src_div(md, 1, 2);
> +
> +       return notifier_from_errno(ret);
> +}
> +
> +static int qcom_apcs_sdx55_clk_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct device *parent = dev->parent;
> +       struct device *cpu_dev;
> +       struct clk_regmap_mux_div *a7cc;
> +       struct regmap *regmap;
> +       struct clk_init_data init = { };
> +       int ret = -ENODEV;

Drop assignement..

> +
> +       regmap = dev_get_regmap(parent, NULL);
> +       if (!regmap) {
> +               dev_err(dev, "Failed to get parent regmap: %d\n", ret);
> +               return ret;

.. and Just return -ENODEV?

> +       }
> +
> +       a7cc = devm_kzalloc(dev, sizeof(*a7cc), GFP_KERNEL);
> +       if (!a7cc)
> +               return -ENOMEM;
> +
> +       init.name = "a7mux";
> +       init.parent_data = pdata;
> +       init.num_parents = ARRAY_SIZE(pdata);
> +       init.ops = &clk_regmap_mux_div_ops;
> +
> +       a7cc->clkr.hw.init = &init;
> +       a7cc->clkr.regmap = regmap;
> +       a7cc->reg_offset = 0x8;
> +       a7cc->hid_width = 5;
> +       a7cc->hid_shift = 0;
> +       a7cc->src_width = 3;
> +       a7cc->src_shift = 8;
> +       a7cc->parent_map = apcs_mux_clk_parent_map;
> +
> +       a7cc->pclk = devm_clk_get(parent, "pll");
> +       if (IS_ERR(a7cc->pclk)) {
> +               ret = PTR_ERR(a7cc->pclk);
> +               if (ret != -EPROBE_DEFER)
> +                       dev_err(dev, "Failed to get PLL clk: %d\n", ret);

Use dev_err_probe() please.

> +               return ret;
> +       }
> +
> +       a7cc->clk_nb.notifier_call = a7cc_notifier_cb;
> +       ret = clk_notifier_register(a7cc->pclk, &a7cc->clk_nb);
> +       if (ret) {
> +               dev_err(dev, "Failed to register clock notifier: %d\n", ret);
> +               return ret;
> +       }
> +
> +       ret = devm_clk_register_regmap(dev, &a7cc->clkr);
> +       if (ret) {
> +               dev_err(dev, "Failed to register regmap clock: %d\n", ret);
> +               goto err;
> +       }
> +
> +       ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get,
> +                                         &a7cc->clkr.hw);
> +       if (ret) {
> +               dev_err(dev, "Failed to add clock provider: %d\n", ret);
> +               goto err;
> +       }
> +
> +       platform_set_drvdata(pdev, a7cc);
> +
> +       /*
> +        * Attach the power domain to cpudev. There seems to be no better place
> +        * to do this, so do it here.
> +        */
> +       cpu_dev = get_cpu_device(0);
> +       dev_pm_domain_attach(cpu_dev, true);

I guess this works given that we don't have CPU drivers. The comment
says what the code is doing but doesn't say why it's doing it. Adding
why may help understand in the future and would be a better comment.
Why can't cpufreq-dt attach a power domain from DT for a cpu device? Is
that a bad idea?

> +
> +       return 0;
> +
> +err:
> +       clk_notifier_unregister(a7cc->pclk, &a7cc->clk_nb);
> +       return ret;
> +}

Re: [PATCH v2 4/5] clk: qcom: Add A7 PLL support

[Stephen Boyd]

Quoting Manivannan Sadhasivam (2021-01-08 03:32:32)
> Add support for PLL found in Qualcomm SDX55 platforms which is used to
> provide clock to the Cortex A7 CPU via a mux. This PLL can provide high
> frequency clock to the CPU above 1GHz as compared to the other sources
> like GPLL0.
> 
> In this driver, the power domain is attached to the cpudev. This is
> required for CPUFreq functionality and there seems to be no better place
> to do other than this driver (no dedicated CPUFreq driver).
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---

Looks good to me.

Re: [PATCH v2 5/5] clk: qcom: Add SDX55 APCS clock controller support

[Manivannan Sadhasivam]

On Tue, Jan 12, 2021 at 11:37:04PM -0800, Stephen Boyd wrote:
> Quoting Manivannan Sadhasivam (2021-01-08 03:32:33)
> > Add a driver for the SDX55 APCS clock controller. It is part of the APCS
> > hardware block, which among other things implements also a combined mux
> > and half integer divider functionality. The APCS clock controller has 3
> > parent clocks:
> > 
> > 1. Board XO
> > 2. Fixed rate GPLL0
> > 3. A7 PLL
> > 
> > The source and the divider can be set both at the same time.
> 
> I don't understand what that means. Presumably it's a mux/divider
> combined?
> 

Yeah, will make it clear.

> > 
> > This is required for enabling CPU frequency scaling on SDX55-based
> > platforms.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/clk/qcom/Kconfig      |   9 ++
> >  drivers/clk/qcom/Makefile     |   1 +
> >  drivers/clk/qcom/apcs-sdx55.c | 149 ++++++++++++++++++++++++++++++++++
> >  3 files changed, 159 insertions(+)
> >  create mode 100644 drivers/clk/qcom/apcs-sdx55.c
> > 
> > diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> > index d6f4aee4427a..2c67fdfae913 100644
> > --- a/drivers/clk/qcom/Kconfig
> > +++ b/drivers/clk/qcom/Kconfig
> > @@ -45,6 +45,15 @@ config QCOM_CLK_APCS_MSM8916
> >           Say Y if you want to support CPU frequency scaling on devices
> >           such as msm8916.
> >  
> > +config QCOM_CLK_APCS_SDX55
> 
> APCC comes before APCS
> 

Okay

> > +       tristate "SDX55 APCS Clock Controller"
> > +       depends on QCOM_APCS_IPC || COMPILE_TEST
> > +       help
> > +         Support for the APCS Clock Controller on SDX55 platform. The
> > +         APCS is managing the mux and divider which feeds the CPUs.
> > +         Say Y if you want to support CPU frequency scaling on devices
> > +         such as SDX55.
> > +
> >  config QCOM_CLK_APCC_MSM8996
> >         tristate "MSM8996 CPU Clock Controller"
> >         select QCOM_KRYO_L2_ACCESSORS
> > diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
> > index e7e0ac382176..a9271f40916c 100644
> > --- a/drivers/clk/qcom/Makefile
> > +++ b/drivers/clk/qcom/Makefile
> > @@ -46,6 +46,7 @@ obj-$(CONFIG_MSM_MMCC_8998) += mmcc-msm8998.o
> >  obj-$(CONFIG_QCOM_A53PLL) += a53-pll.o
> >  obj-$(CONFIG_QCOM_A7PLL) += a7-pll.o
> >  obj-$(CONFIG_QCOM_CLK_APCS_MSM8916) += apcs-msm8916.o
> > +obj-$(CONFIG_QCOM_CLK_APCS_SDX55) += apcs-sdx55.o
> >  obj-$(CONFIG_QCOM_CLK_APCC_MSM8996) += clk-cpu-8996.o
> >  obj-$(CONFIG_QCOM_CLK_RPM) += clk-rpm.o
> >  obj-$(CONFIG_QCOM_CLK_RPMH) += clk-rpmh.o
> > diff --git a/drivers/clk/qcom/apcs-sdx55.c b/drivers/clk/qcom/apcs-sdx55.c
> > new file mode 100644
> > index 000000000000..14413c957d83
> > --- /dev/null
> > +++ b/drivers/clk/qcom/apcs-sdx55.c
> > @@ -0,0 +1,149 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Qualcomm SDX55 APCS clock controller driver
> > + *
> > + * Copyright (c) 2020, Linaro Limited
> > + * Author: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/cpu.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_domain.h>
> > +#include <linux/regmap.h>
> > +#include <linux/slab.h>
> > +
> > +#include "clk-regmap.h"
> > +#include "clk-regmap-mux-div.h"
> > +#include "common.h"
> 
> Curious what common is needed for?
> 

Not needed, will remove.

> > +
> > +static const u32 apcs_mux_clk_parent_map[] = { 0, 1, 5 };
> > +
> > +static const struct clk_parent_data pdata[] = {
> > +       { .fw_name = "ref", .name = "bi_tcxo", },
> > +       { .fw_name = "aux", .name = "gpll0", },
> > +       { .fw_name = "pll", .name = "a7pll", },
> 
> Please remove name from here. It shouldn't be necessary if the DT
> describes things properly. Or there isn't DT for this device?
> 

Will remove.

> > +};
> > +
> > +/*
> > + * We use the notifier function for switching to a temporary safe configuration
> > + * (mux and divider), while the A7 PLL is reconfigured.
> > + */
> > +static int a7cc_notifier_cb(struct notifier_block *nb, unsigned long event,
> > +                           void *data)
> > +{
> > +       int ret = 0;
> > +       struct clk_regmap_mux_div *md = container_of(nb,
> > +                                                    struct clk_regmap_mux_div,
> > +                                                    clk_nb);
> > +       if (event == PRE_RATE_CHANGE)
> > +               /* set the mux and divider to safe frequency (400mhz) */
> > +               ret = mux_div_set_src_div(md, 1, 2);
> > +
> > +       return notifier_from_errno(ret);
> > +}
> > +
> > +static int qcom_apcs_sdx55_clk_probe(struct platform_device *pdev)
> > +{
> > +       struct device *dev = &pdev->dev;
> > +       struct device *parent = dev->parent;
> > +       struct device *cpu_dev;
> > +       struct clk_regmap_mux_div *a7cc;
> > +       struct regmap *regmap;
> > +       struct clk_init_data init = { };
> > +       int ret = -ENODEV;
> 
> Drop assignement..
> 
> > +
> > +       regmap = dev_get_regmap(parent, NULL);
> > +       if (!regmap) {
> > +               dev_err(dev, "Failed to get parent regmap: %d\n", ret);
> > +               return ret;
> 
> .. and Just return -ENODEV?
> 
> > +       }
> > +
> > +       a7cc = devm_kzalloc(dev, sizeof(*a7cc), GFP_KERNEL);
> > +       if (!a7cc)
> > +               return -ENOMEM;
> > +
> > +       init.name = "a7mux";
> > +       init.parent_data = pdata;
> > +       init.num_parents = ARRAY_SIZE(pdata);
> > +       init.ops = &clk_regmap_mux_div_ops;
> > +
> > +       a7cc->clkr.hw.init = &init;
> > +       a7cc->clkr.regmap = regmap;
> > +       a7cc->reg_offset = 0x8;
> > +       a7cc->hid_width = 5;
> > +       a7cc->hid_shift = 0;
> > +       a7cc->src_width = 3;
> > +       a7cc->src_shift = 8;
> > +       a7cc->parent_map = apcs_mux_clk_parent_map;
> > +
> > +       a7cc->pclk = devm_clk_get(parent, "pll");
> > +       if (IS_ERR(a7cc->pclk)) {
> > +               ret = PTR_ERR(a7cc->pclk);
> > +               if (ret != -EPROBE_DEFER)
> > +                       dev_err(dev, "Failed to get PLL clk: %d\n", ret);
> 
> Use dev_err_probe() please.
> 
> > +               return ret;
> > +       }
> > +
> > +       a7cc->clk_nb.notifier_call = a7cc_notifier_cb;
> > +       ret = clk_notifier_register(a7cc->pclk, &a7cc->clk_nb);
> > +       if (ret) {
> > +               dev_err(dev, "Failed to register clock notifier: %d\n", ret);
> > +               return ret;
> > +       }
> > +
> > +       ret = devm_clk_register_regmap(dev, &a7cc->clkr);
> > +       if (ret) {
> > +               dev_err(dev, "Failed to register regmap clock: %d\n", ret);
> > +               goto err;
> > +       }
> > +
> > +       ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get,
> > +                                         &a7cc->clkr.hw);
> > +       if (ret) {
> > +               dev_err(dev, "Failed to add clock provider: %d\n", ret);
> > +               goto err;
> > +       }
> > +
> > +       platform_set_drvdata(pdev, a7cc);
> > +
> > +       /*
> > +        * Attach the power domain to cpudev. There seems to be no better place
> > +        * to do this, so do it here.
> > +        */
> > +       cpu_dev = get_cpu_device(0);
> > +       dev_pm_domain_attach(cpu_dev, true);
> 
> I guess this works given that we don't have CPU drivers. The comment
> says what the code is doing but doesn't say why it's doing it. Adding
> why may help understand in the future and would be a better comment.
> Why can't cpufreq-dt attach a power domain from DT for a cpu device? Is
> that a bad idea?
> 

Yeah, I talked with Viresh about using cpufreq-dt for attaching the power
domain but he said it isn't the appropriate place. Hence, I decided to use
this driver.

Will make the comment more elaborate.

Thanks,
Mani

> > +
> > +       return 0;
> > +
> > +err:
> > +       clk_notifier_unregister(a7cc->pclk, &a7cc->clk_nb);
> > +       return ret;
> > +}