[PATCH v2 0/6] Add support for PDC Global on SDM845 SoCs

[PATCH v2 0/6] Add support for PDC Global on SDM845 SoCs

[Sibi Sankar]

This patch series add support for PDC Global (Power Domain Controller)
on SDM845 SoCs and adds pdc reset lines assert/deassert to remoteproc
Q6v5 modem-pil. The first two patches adds PDC Global reset driver to
control reset signals of Modem, Compute, Display, GPU, Debug, AOP,
Sensors, Audio, SP and APPS. The last four patches (cleans up)/adds pdc
reset lines to q6v5 bindings and asserts/deasserts in modem start/stop
path.

v2:
  Incorporated Philipp/Bjorn/Rob suggestions
  Renamed reset binding to pdc-global.txt
  replaced offset with #define of register name
  replaced with devm_reset_control_get_exclusive()
  Separted dt binding from the drivers 

The last 4 remoteproc patches can be picked up after the pdc global
reset driver lands on linux-next.

Sibi Sankar (6):
  dt-bindings: reset: Add PDC Global binding for SDM845 SoCs
  reset: qcom: PDC Global (Power Domain Controller) reset controller
  dt-bindings: remoteproc: Remove additional definition tag
  dt-bindings: remoteproc: Add PDC reset binding for Q6V5 PIL
  remoteproc: qcom: q6v5-pil: Explicitly get mss_restart line
  remoteproc: qcom: q6v5-pil: Add PDC reset for modem on SDM845 SoCs

 .../bindings/remoteproc/qcom,q6v5.txt         |   6 +-
 .../bindings/reset/qcom,pdc-global.txt        |  52 +++++++
 drivers/remoteproc/qcom_q6v5_pil.c            |  31 +++-
 drivers/reset/Kconfig                         |   9 ++
 drivers/reset/Makefile                        |   1 +
 drivers/reset/reset-qcom-pdc.c                | 142 ++++++++++++++++++
 include/dt-bindings/reset/qcom,sdm845-pdc.h   |  20 +++
 7 files changed, 254 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/reset/qcom,pdc-global.txt
 create mode 100644 drivers/reset/reset-qcom-pdc.c
 create mode 100644 include/dt-bindings/reset/qcom,sdm845-pdc.h

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 1/6] dt-bindings: reset: Add PDC Global binding for SDM845 SoCs

[Sibi Sankar]

Add PDC Global(Power Domain Controller) binding for SDM845 SoCs.

Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
---
 .../bindings/reset/qcom,pdc-global.txt        | 52 +++++++++++++++++++
 include/dt-bindings/reset/qcom,sdm845-pdc.h   | 20 +++++++
 2 files changed, 72 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/qcom,pdc-global.txt
 create mode 100644 include/dt-bindings/reset/qcom,sdm845-pdc.h

diff --git a/Documentation/devicetree/bindings/reset/qcom,pdc-global.txt b/Documentation/devicetree/bindings/reset/qcom,pdc-global.txt
new file mode 100644
index 000000000000..69f9edca9503
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/qcom,pdc-global.txt
@@ -0,0 +1,52 @@
+PDC Global
+======================================
+
+This binding describes a reset-controller found on PDC-Global(Power Domain
+Controller) block for Qualcomm Technologies Inc SDM845 SoCs.
+
+Required properties:
+- compatible:
+	Usage: required
+	Value type: <string>
+	Definition: must be:
+		    "qcom,sdm845-pdc-global"
+
+- reg:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: must specify the base address and size of the register
+	            space.
+
+- #reset-cells:
+	Usage: required
+	Value type: <uint>
+	Definition: must be 1; cell entry represents the reset index.
+
+Example:
+
+pdc_reset: reset-controller@b2e0000 {
+	compatible = "qcom,sdm845-pdc-global";
+	reg = <0xb2e0000 0x20000>;
+	#reset-cells = <1>;
+};
+
+PDC reset clients
+======================================
+
+Device nodes that need access to reset lines should
+specify them as a reset phandle in their corresponding node as
+specified in reset.txt.
+
+For list of all valid reset indicies see
+<dt-bindings/reset/qcom,sdm845-pdc.h>
+
+Example:
+
+modem-pil@4080000 {
+	...
+
+	resets = <&pdc_reset PDC_MODEM_SYNC_RESET>;
+	reset-names = "pdc_reset";
+
+	...
+};
diff --git a/include/dt-bindings/reset/qcom,sdm845-pdc.h b/include/dt-bindings/reset/qcom,sdm845-pdc.h
new file mode 100644
index 000000000000..53c37f9c319a
--- /dev/null
+++ b/include/dt-bindings/reset/qcom,sdm845-pdc.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2018 The Linux Foundation. All rights reserved.
+ */
+
+#ifndef _DT_BINDINGS_RESET_PDC_SDM_845_H
+#define _DT_BINDINGS_RESET_PDC_SDM_845_H
+
+#define PDC_APPS_SYNC_RESET	0
+#define PDC_SP_SYNC_RESET	1
+#define PDC_AUDIO_SYNC_RESET	2
+#define PDC_SENSORS_SYNC_RESET	3
+#define PDC_AOP_SYNC_RESET	4
+#define PDC_DEBUG_SYNC_RESET	5
+#define PDC_GPU_SYNC_RESET	6
+#define PDC_DISPLAY_SYNC_RESET	7
+#define PDC_COMPUTE_SYNC_RESET	8
+#define PDC_MODEM_SYNC_RESET	9
+
+#endif
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 2/6] reset: qcom: PDC Global (Power Domain Controller) reset controller

[Sibi Sankar]

Add reset controller for SDM845 SoCs to control reset signals provided
by PDC Global for Modem, Compute, Display, GPU, Debug, AOP, Sensors,
Audio, SP and APPS

Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
---
 drivers/reset/Kconfig          |   9 +++
 drivers/reset/Makefile         |   1 +
 drivers/reset/reset-qcom-pdc.c | 142 +++++++++++++++++++++++++++++++++
 3 files changed, 152 insertions(+)
 create mode 100644 drivers/reset/reset-qcom-pdc.c

diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index 13d28fdbdbb5..c21da9fe51ec 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -98,6 +98,15 @@ config RESET_QCOM_AOSS
 	  reset signals provided by AOSS for Modem, Venus, ADSP,
 	  GPU, Camera, Wireless, Display subsystem. Otherwise, say N.
 
+config RESET_QCOM_PDC
+	tristate "Qualcomm PDC Reset Driver"
+	depends on ARCH_QCOM || COMPILE_TEST
+	help
+	  This enables the PDC (Power Domain Controller) reset driver
+	  for Qualcomm Technologies Inc SDM845 SoCs. Say Y if you want
+	  to control reset signals provided by PDC for Modem, Compute,
+	  Display, GPU, Debug, AOP, Sensors, Audio, SP and APPS.
+
 config RESET_SIMPLE
 	bool "Simple Reset Controller Driver" if COMPILE_TEST
 	default ARCH_SOCFPGA || ARCH_STM32 || ARCH_STRATIX10 || ARCH_SUNXI || ARCH_ZX || ARCH_ASPEED
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index 4243c38228e2..d08e8b90046a 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_RESET_MESON_AUDIO_ARB) += reset-meson-audio-arb.o
 obj-$(CONFIG_RESET_OXNAS) += reset-oxnas.o
 obj-$(CONFIG_RESET_PISTACHIO) += reset-pistachio.o
 obj-$(CONFIG_RESET_QCOM_AOSS) += reset-qcom-aoss.o
+obj-$(CONFIG_RESET_QCOM_PDC) += reset-qcom-pdc.o
 obj-$(CONFIG_RESET_SIMPLE) += reset-simple.o
 obj-$(CONFIG_RESET_STM32MP157) += reset-stm32mp1.o
 obj-$(CONFIG_RESET_SUNXI) += reset-sunxi.o
diff --git a/drivers/reset/reset-qcom-pdc.c b/drivers/reset/reset-qcom-pdc.c
new file mode 100644
index 000000000000..bb6a5e5ee0f8
--- /dev/null
+++ b/drivers/reset/reset-qcom-pdc.c
@@ -0,0 +1,142 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/reset-controller.h>
+#include <linux/regmap.h>
+#include <linux/of_device.h>
+#include <dt-bindings/reset/qcom,sdm845-pdc.h>
+
+#define RPMH_PDC_SYNC_RESET	0x100
+
+struct qcom_pdc_reset_map {
+	u8 bit;
+};
+
+struct qcom_pdc_desc {
+	const struct regmap_config *config;
+	const struct qcom_pdc_reset_map *resets;
+	size_t num_resets;
+};
+
+struct qcom_pdc_reset_data {
+	struct reset_controller_dev rcdev;
+	struct regmap *regmap;
+	const struct qcom_pdc_desc *desc;
+};
+
+static const struct regmap_config sdm845_pdc_regmap_config = {
+	.name		= "pdc-reset",
+	.reg_bits	= 32,
+	.reg_stride	= 4,
+	.val_bits	= 32,
+	.max_register	= 0x20000,
+	.fast_io	= true,
+};
+
+static const struct qcom_pdc_reset_map sdm845_pdc_resets[] = {
+	[PDC_APPS_SYNC_RESET] = {0},
+	[PDC_SP_SYNC_RESET] = {1},
+	[PDC_AUDIO_SYNC_RESET] = {2},
+	[PDC_SENSORS_SYNC_RESET] = {3},
+	[PDC_AOP_SYNC_RESET] = {4},
+	[PDC_DEBUG_SYNC_RESET] = {5},
+	[PDC_GPU_SYNC_RESET] = {6},
+	[PDC_DISPLAY_SYNC_RESET] = {7},
+	[PDC_COMPUTE_SYNC_RESET] = {8},
+	[PDC_MODEM_SYNC_RESET] = {9},
+};
+
+static const struct qcom_pdc_desc sdm845_pdc_desc = {
+	.config = &sdm845_pdc_regmap_config,
+	.resets = sdm845_pdc_resets,
+	.num_resets = ARRAY_SIZE(sdm845_pdc_resets),
+};
+
+static inline struct qcom_pdc_reset_data *to_qcom_pdc_reset_data(
+				struct reset_controller_dev *rcdev)
+{
+	return container_of(rcdev, struct qcom_pdc_reset_data, rcdev);
+}
+
+static int qcom_pdc_control_assert(struct reset_controller_dev *rcdev,
+					unsigned long idx)
+{
+	struct qcom_pdc_reset_data *data = to_qcom_pdc_reset_data(rcdev);
+	const struct qcom_pdc_reset_map *map = &data->desc->resets[idx];
+
+	return regmap_update_bits(data->regmap, RPMH_PDC_SYNC_RESET,
+					BIT(map->bit), BIT(map->bit));
+}
+
+static int qcom_pdc_control_deassert(struct reset_controller_dev *rcdev,
+					unsigned long idx)
+{
+	struct qcom_pdc_reset_data *data = to_qcom_pdc_reset_data(rcdev);
+	const struct qcom_pdc_reset_map *map = &data->desc->resets[idx];
+
+	return regmap_update_bits(data->regmap, RPMH_PDC_SYNC_RESET,
+					BIT(map->bit), 0);
+}
+
+static const struct reset_control_ops qcom_pdc_reset_ops = {
+	.assert = qcom_pdc_control_assert,
+	.deassert = qcom_pdc_control_deassert,
+};
+
+static int qcom_pdc_reset_probe(struct platform_device *pdev)
+{
+	struct qcom_pdc_reset_data *data;
+	struct device *dev = &pdev->dev;
+	const struct qcom_pdc_desc *desc;
+	void __iomem *base;
+	struct resource *res;
+
+	desc = of_device_get_match_data(dev);
+	if (!desc)
+		return -EINVAL;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->desc = desc;
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	data->regmap = devm_regmap_init_mmio(dev, base, desc->config);
+	if (IS_ERR(data->regmap)) {
+		dev_err(dev, "Unable to get pdc-global regmap");
+		return PTR_ERR(data->regmap);
+	}
+
+	data->rcdev.owner = THIS_MODULE;
+	data->rcdev.ops = &qcom_pdc_reset_ops;
+	data->rcdev.nr_resets = desc->num_resets;
+	data->rcdev.of_node = dev->of_node;
+
+	return devm_reset_controller_register(dev, &data->rcdev);
+}
+
+static const struct of_device_id qcom_pdc_reset_of_match[] = {
+	{ .compatible = "qcom,sdm845-pdc-global", .data = &sdm845_pdc_desc },
+	{}
+};
+MODULE_DEVICE_TABLE(of, qcom_pdc_reset_of_match);
+
+static struct platform_driver qcom_pdc_reset_driver = {
+	.probe = qcom_pdc_reset_probe,
+	.driver = {
+		.name = "qcom_pdc_reset",
+		.of_match_table = qcom_pdc_reset_of_match,
+	},
+};
+module_platform_driver(qcom_pdc_reset_driver);
+
+MODULE_DESCRIPTION("Qualcomm PDC Reset Driver");
+MODULE_LICENSE("GPL v2");
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 3/6] dt-bindings: remoteproc: Remove additional definition tag

[Sibi Sankar]

Remove the additional definition tag declared for WCSS sub-system
under reset-names.

Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
---
 Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
index 601dd9f389aa..c45e4c131fa2 100644
--- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
@@ -58,7 +58,7 @@ on the Qualcomm Hexagon core.
 	Usage: required
 	Value type: <stringlist>
 	Definition: must be "mss_restart" for the modem sub-system
-	Definition: must be "wcss_aon_reset", "wcss_reset", "wcss_q6_reset"
+		    must be "wcss_aon_reset", "wcss_reset", "wcss_q6_reset"
 		    for the wcss syb-system
 
 - cx-supply:
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 4/6] dt-bindings: remoteproc: Add PDC reset binding for Q6V5 PIL

[Sibi Sankar]

Add additional pdc_reset binding required for Q6V5 Modem PIL on
SDM845 SoCs.

Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
---
 Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
index c45e4c131fa2..4c5473aeb697 100644
--- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
@@ -53,6 +53,8 @@ on the Qualcomm Hexagon core.
 	Definition: reference to the reset-controller for the modem sub-system
 		    reference to the list of 3 reset-controllers for the
 		    wcss sub-system
+		    reference to the list of 2 reset-controllers for the modem
+		    sub-system on SDM845 SoCs
 
 - reset-names:
 	Usage: required
@@ -60,6 +62,8 @@ on the Qualcomm Hexagon core.
 	Definition: must be "mss_restart" for the modem sub-system
 		    must be "wcss_aon_reset", "wcss_reset", "wcss_q6_reset"
 		    for the wcss syb-system
+		    must be "mss_restart", "pdc_reset" for the modem
+		    sub-system on SDM845 SoCs
 
 - cx-supply:
 - mss-supply:
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 5/6] remoteproc: qcom: q6v5-pil: Explicitly get mss_restart line

[Sibi Sankar]

Explicitly get mss_restart to facilitate adding PDC reset line
for modem on SDM845 SoCs

Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
---
 drivers/remoteproc/qcom_q6v5_pil.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index b1296d614b8b..968413edf0c8 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -1177,7 +1177,7 @@ static int q6v5_init_clocks(struct device *dev, struct clk **clks,
 static int q6v5_init_reset(struct q6v5 *qproc)
 {
 	qproc->mss_restart = devm_reset_control_get_exclusive(qproc->dev,
-							      NULL);
+							      "mss_restart");
 	if (IS_ERR(qproc->mss_restart)) {
 		dev_err(qproc->dev, "failed to acquire mss restart\n");
 		return PTR_ERR(qproc->mss_restart);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 6/6] remoteproc: qcom: q6v5-pil: Add PDC reset for modem on SDM845 SoCs

[Sibi Sankar]

In the presence of a PDC block working with subsystem RSC, assert/deassert
PDC reset in modem start/stop path.

Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
---
 drivers/remoteproc/qcom_q6v5_pil.c | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index 968413edf0c8..835fa005fe3e 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -149,6 +149,7 @@ struct q6v5 {
 	u32 halt_nc;
 
 	struct reset_control *mss_restart;
+	struct reset_control *pdc_reset;
 
 	struct qcom_q6v5 q6v5;
 
@@ -349,10 +350,17 @@ static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
 
 static int q6v5_reset_assert(struct q6v5 *qproc)
 {
-	if (qproc->has_alt_reset)
-		return reset_control_reset(qproc->mss_restart);
-	else
-		return reset_control_assert(qproc->mss_restart);
+	int ret;
+
+	if (qproc->has_alt_reset) {
+		reset_control_assert(qproc->pdc_reset);
+		ret = reset_control_reset(qproc->mss_restart);
+		reset_control_deassert(qproc->pdc_reset);
+	} else {
+		ret = reset_control_assert(qproc->mss_restart);
+	}
+
+	return ret;
 }
 
 static int q6v5_reset_deassert(struct q6v5 *qproc)
@@ -360,9 +368,11 @@ static int q6v5_reset_deassert(struct q6v5 *qproc)
 	int ret;
 
 	if (qproc->has_alt_reset) {
+		reset_control_assert(qproc->pdc_reset);
 		writel(1, qproc->rmb_base + RMB_MBA_ALT_RESET);
 		ret = reset_control_reset(qproc->mss_restart);
 		writel(0, qproc->rmb_base + RMB_MBA_ALT_RESET);
+		reset_control_deassert(qproc->pdc_reset);
 	} else {
 		ret = reset_control_deassert(qproc->mss_restart);
 	}
@@ -1183,6 +1193,15 @@ static int q6v5_init_reset(struct q6v5 *qproc)
 		return PTR_ERR(qproc->mss_restart);
 	}
 
+	if (qproc->has_alt_reset) {
+		qproc->pdc_reset = devm_reset_control_get_exclusive(qproc->dev,
+								"pdc_reset");
+		if (IS_ERR(qproc->pdc_reset)) {
+			dev_err(qproc->dev, "failed to acquire pdc reset\n");
+			return PTR_ERR(qproc->pdc_reset);
+		}
+	}
+
 	return 0;
 }
 
@@ -1303,12 +1322,12 @@ static int q6v5_probe(struct platform_device *pdev)
 	}
 	qproc->active_reg_count = ret;
 
+	qproc->has_alt_reset = desc->has_alt_reset;
 	ret = q6v5_init_reset(qproc);
 	if (ret)
 		goto free_rproc;
 
 	qproc->version = desc->version;
-	qproc->has_alt_reset = desc->has_alt_reset;
 	qproc->need_mem_protection = desc->need_mem_protection;
 
 	ret = qcom_q6v5_init(&qproc->q6v5, pdev, rproc, MPSS_CRASH_REASON_SMEM,
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 2/6] reset: qcom: PDC Global (Power Domain Controller) reset controller

[Matthias Kaehlcke]

Hi Sibi,

On Fri, Aug 24, 2018 at 06:48:56PM +0530, Sibi Sankar wrote:
> Add reset controller for SDM845 SoCs to control reset signals provided
> by PDC Global for Modem, Compute, Display, GPU, Debug, AOP, Sensors,
> Audio, SP and APPS
> 
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  drivers/reset/Kconfig          |   9 +++
>  drivers/reset/Makefile         |   1 +
>  drivers/reset/reset-qcom-pdc.c | 142 +++++++++++++++++++++++++++++++++
>  3 files changed, 152 insertions(+)
>  create mode 100644 drivers/reset/reset-qcom-pdc.c
> 
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 13d28fdbdbb5..c21da9fe51ec 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -98,6 +98,15 @@ config RESET_QCOM_AOSS
>  	  reset signals provided by AOSS for Modem, Venus, ADSP,
>  	  GPU, Camera, Wireless, Display subsystem. Otherwise, say N.
>  
> +config RESET_QCOM_PDC
> +	tristate "Qualcomm PDC Reset Driver"
> +	depends on ARCH_QCOM || COMPILE_TEST
> +	help
> +	  This enables the PDC (Power Domain Controller) reset driver
> +	  for Qualcomm Technologies Inc SDM845 SoCs. Say Y if you want
> +	  to control reset signals provided by PDC for Modem, Compute,
> +	  Display, GPU, Debug, AOP, Sensors, Audio, SP and APPS.

What exactly does APPS mean? The AP cores, the entire SoC, something
else?

> +
>  config RESET_SIMPLE
>  	bool "Simple Reset Controller Driver" if COMPILE_TEST
>  	default ARCH_SOCFPGA || ARCH_STM32 || ARCH_STRATIX10 || ARCH_SUNXI || ARCH_ZX || ARCH_ASPEED
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 4243c38228e2..d08e8b90046a 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -16,6 +16,7 @@ obj-$(CONFIG_RESET_MESON_AUDIO_ARB) += reset-meson-audio-arb.o
>  obj-$(CONFIG_RESET_OXNAS) += reset-oxnas.o
>  obj-$(CONFIG_RESET_PISTACHIO) += reset-pistachio.o
>  obj-$(CONFIG_RESET_QCOM_AOSS) += reset-qcom-aoss.o
> +obj-$(CONFIG_RESET_QCOM_PDC) += reset-qcom-pdc.o
>  obj-$(CONFIG_RESET_SIMPLE) += reset-simple.o
>  obj-$(CONFIG_RESET_STM32MP157) += reset-stm32mp1.o
>  obj-$(CONFIG_RESET_SUNXI) += reset-sunxi.o
> diff --git a/drivers/reset/reset-qcom-pdc.c b/drivers/reset/reset-qcom-pdc.c
> new file mode 100644
> index 000000000000..bb6a5e5ee0f8
> --- /dev/null
> +++ b/drivers/reset/reset-qcom-pdc.c
> @@ -0,0 +1,142 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2018 The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
> +#include <linux/regmap.h>
> +#include <linux/of_device.h>
> +#include <dt-bindings/reset/qcom,sdm845-pdc.h>

Headers should be sorted in alphabetical order.

> +
> +#define RPMH_PDC_SYNC_RESET	0x100
> +
> +struct qcom_pdc_reset_map {
> +	u8 bit;
> +};
> +
> +struct qcom_pdc_desc {
> +	const struct regmap_config *config;
> +	const struct qcom_pdc_reset_map *resets;
> +	size_t num_resets;
> +};

Not sure if this structure adds much value or just a layer of
indirection:

- .config is only accessed in _probe(), sdm845_pdc_regmap_config could
  be used directly
- .resets is used in _(de)assert(), sdm845_pdc_resets could be used
  directly
- .num_resets is only accessed in _probe(),
  ARRAY_SIZE(sdm845_pdc_resets) could be used instead

It probably makes sense if it is planned to support reset controllers
of other SoCs with this driver.

> +struct qcom_pdc_reset_data {
> +	struct reset_controller_dev rcdev;
> +	struct regmap *regmap;
> +	const struct qcom_pdc_desc *desc;
> +};
> +
> +static const struct regmap_config sdm845_pdc_regmap_config = {
> +	.name		= "pdc-reset",
> +	.reg_bits	= 32,
> +	.reg_stride	= 4,
> +	.val_bits	= 32,
> +	.max_register	= 0x20000,
> +	.fast_io	= true,
> +};
> +
> +static const struct qcom_pdc_reset_map sdm845_pdc_resets[] = {
> +	[PDC_APPS_SYNC_RESET] = {0},
> +	[PDC_SP_SYNC_RESET] = {1},
> +	[PDC_AUDIO_SYNC_RESET] = {2},
> +	[PDC_SENSORS_SYNC_RESET] = {3},
> +	[PDC_AOP_SYNC_RESET] = {4},
> +	[PDC_DEBUG_SYNC_RESET] = {5},
> +	[PDC_GPU_SYNC_RESET] = {6},
> +	[PDC_DISPLAY_SYNC_RESET] = {7},
> +	[PDC_COMPUTE_SYNC_RESET] = {8},
> +	[PDC_MODEM_SYNC_RESET] = {9},
> +};
> +
> +static const struct qcom_pdc_desc sdm845_pdc_desc = {
> +	.config = &sdm845_pdc_regmap_config,
> +	.resets = sdm845_pdc_resets,
> +	.num_resets = ARRAY_SIZE(sdm845_pdc_resets),
> +};
> +
> +static inline struct qcom_pdc_reset_data *to_qcom_pdc_reset_data(
> +				struct reset_controller_dev *rcdev)
> +{
> +	return container_of(rcdev, struct qcom_pdc_reset_data, rcdev);
> +}
> +
> +static int qcom_pdc_control_assert(struct reset_controller_dev *rcdev,
> +					unsigned long idx)
> +{
> +	struct qcom_pdc_reset_data *data = to_qcom_pdc_reset_data(rcdev);
> +	const struct qcom_pdc_reset_map *map = &data->desc->resets[idx];
> +
> +	return regmap_update_bits(data->regmap, RPMH_PDC_SYNC_RESET,
> +					BIT(map->bit), BIT(map->bit));
> +}
> +
> +static int qcom_pdc_control_deassert(struct reset_controller_dev *rcdev,
> +					unsigned long idx)
> +{
> +	struct qcom_pdc_reset_data *data = to_qcom_pdc_reset_data(rcdev);
> +	const struct qcom_pdc_reset_map *map = &data->desc->resets[idx];
> +
> +	return regmap_update_bits(data->regmap, RPMH_PDC_SYNC_RESET,
> +					BIT(map->bit), 0);
> +}
> +
> +static const struct reset_control_ops qcom_pdc_reset_ops = {
> +	.assert = qcom_pdc_control_assert,
> +	.deassert = qcom_pdc_control_deassert,
> +};
> +
> +static int qcom_pdc_reset_probe(struct platform_device *pdev)
> +{
> +	struct qcom_pdc_reset_data *data;
> +	struct device *dev = &pdev->dev;
> +	const struct qcom_pdc_desc *desc;
> +	void __iomem *base;
> +	struct resource *res;
> +
> +	desc = of_device_get_match_data(dev);
> +	if (!desc)
> +		return -EINVAL;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->desc = desc;
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	data->regmap = devm_regmap_init_mmio(dev, base, desc->config);
> +	if (IS_ERR(data->regmap)) {
> +		dev_err(dev, "Unable to get pdc-global regmap");

Add missing '\n'

Say 'pdc-reset' instead of 'pdc-global'? (see also my other comment
below).

> +		return PTR_ERR(data->regmap);
> +	}
> +
> +	data->rcdev.owner = THIS_MODULE;
> +	data->rcdev.ops = &qcom_pdc_reset_ops;
> +	data->rcdev.nr_resets = desc->num_resets;
> +	data->rcdev.of_node = dev->of_node;
> +
> +	return devm_reset_controller_register(dev, &data->rcdev);
> +}
> +
> +static const struct of_device_id qcom_pdc_reset_of_match[] = {
> +	{ .compatible = "qcom,sdm845-pdc-global", .data = &sdm845_pdc_desc },

Should this be 'qcom,sdm845-pdc-reset' which is more specific than
'global' and in line with the name and purpose of the driver?
Obviously this would require to adjust the bindings doc too.

Cheers

Matthias

Re: [PATCH v2 1/6] dt-bindings: reset: Add PDC Global binding for SDM845 SoCs

[Bjorn Andersson]

On Fri 24 Aug 06:18 PDT 2018, Sibi Sankar wrote:

> Add PDC Global(Power Domain Controller) binding for SDM845 SoCs.
> 
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
>  .../bindings/reset/qcom,pdc-global.txt        | 52 +++++++++++++++++++
>  include/dt-bindings/reset/qcom,sdm845-pdc.h   | 20 +++++++
>  2 files changed, 72 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/qcom,pdc-global.txt
>  create mode 100644 include/dt-bindings/reset/qcom,sdm845-pdc.h
> 
> diff --git a/Documentation/devicetree/bindings/reset/qcom,pdc-global.txt b/Documentation/devicetree/bindings/reset/qcom,pdc-global.txt
> new file mode 100644
> index 000000000000..69f9edca9503
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/qcom,pdc-global.txt
> @@ -0,0 +1,52 @@
> +PDC Global
> +======================================
> +
> +This binding describes a reset-controller found on PDC-Global(Power Domain
> +Controller) block for Qualcomm Technologies Inc SDM845 SoCs.
> +
> +Required properties:
> +- compatible:
> +	Usage: required
> +	Value type: <string>
> +	Definition: must be:
> +		    "qcom,sdm845-pdc-global"
> +
> +- reg:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: must specify the base address and size of the register
> +	            space.
> +
> +- #reset-cells:
> +	Usage: required
> +	Value type: <uint>
> +	Definition: must be 1; cell entry represents the reset index.
> +
> +Example:
> +
> +pdc_reset: reset-controller@b2e0000 {
> +	compatible = "qcom,sdm845-pdc-global";
> +	reg = <0xb2e0000 0x20000>;
> +	#reset-cells = <1>;
> +};
> +
> +PDC reset clients
> +======================================
> +
> +Device nodes that need access to reset lines should
> +specify them as a reset phandle in their corresponding node as
> +specified in reset.txt.
> +
> +For list of all valid reset indicies see
> +<dt-bindings/reset/qcom,sdm845-pdc.h>
> +
> +Example:
> +
> +modem-pil@4080000 {
> +	...
> +
> +	resets = <&pdc_reset PDC_MODEM_SYNC_RESET>;
> +	reset-names = "pdc_reset";
> +
> +	...
> +};
> diff --git a/include/dt-bindings/reset/qcom,sdm845-pdc.h b/include/dt-bindings/reset/qcom,sdm845-pdc.h
> new file mode 100644
> index 000000000000..53c37f9c319a
> --- /dev/null
> +++ b/include/dt-bindings/reset/qcom,sdm845-pdc.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2018 The Linux Foundation. All rights reserved.
> + */
> +
> +#ifndef _DT_BINDINGS_RESET_PDC_SDM_845_H
> +#define _DT_BINDINGS_RESET_PDC_SDM_845_H
> +
> +#define PDC_APPS_SYNC_RESET	0
> +#define PDC_SP_SYNC_RESET	1
> +#define PDC_AUDIO_SYNC_RESET	2
> +#define PDC_SENSORS_SYNC_RESET	3
> +#define PDC_AOP_SYNC_RESET	4
> +#define PDC_DEBUG_SYNC_RESET	5
> +#define PDC_GPU_SYNC_RESET	6
> +#define PDC_DISPLAY_SYNC_RESET	7
> +#define PDC_COMPUTE_SYNC_RESET	8
> +#define PDC_MODEM_SYNC_RESET	9
> +
> +#endif
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH v2 1/6] dt-bindings: reset: Add PDC Global binding for SDM845 SoCs

[Matthias Kaehlcke]

Hi Sibi,

On Fri, Aug 24, 2018 at 06:48:55PM +0530, Sibi Sankar wrote:
> Add PDC Global(Power Domain Controller) binding for SDM845 SoCs.

nit: missing blank before the opening parenthesis.

> 
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  .../bindings/reset/qcom,pdc-global.txt        | 52 +++++++++++++++++++
>  include/dt-bindings/reset/qcom,sdm845-pdc.h   | 20 +++++++
>  2 files changed, 72 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/qcom,pdc-global.txt
>  create mode 100644 include/dt-bindings/reset/qcom,sdm845-pdc.h
> 
> diff --git a/Documentation/devicetree/bindings/reset/qcom,pdc-global.txt b/Documentation/devicetree/bindings/reset/qcom,pdc-global.txt
> new file mode 100644
> index 000000000000..69f9edca9503
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/qcom,pdc-global.txt
> @@ -0,0 +1,52 @@
> +PDC Global
> +======================================
> +
> +This binding describes a reset-controller found on PDC-Global(Power Domain
> +Controller) block for Qualcomm Technologies Inc SDM845 SoCs.

Are there other PDC reset controllers that aren't 'global'? Otherwise
I'd suggest to use 'pdc-reset' instead of 'pdc-global', which is more
specific and in line with the name of the driver added by this series.
Or something like 'pdc-reset-global/main' if there are other
controllers?

> +Required properties:
> +- compatible:
> +	Usage: required
> +	Value type: <string>
> +	Definition: must be:
> +		    "qcom,sdm845-pdc-global"
> +
> +- reg:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: must specify the base address and size of the register
> +	            space.
> +
> +- #reset-cells:
> +	Usage: required
> +	Value type: <uint>
> +	Definition: must be 1; cell entry represents the reset index.
> +
> +Example:
> +
> +pdc_reset: reset-controller@b2e0000 {
> +	compatible = "qcom,sdm845-pdc-global";
> +	reg = <0xb2e0000 0x20000>;
> +	#reset-cells = <1>;
> +};
> +
> +PDC reset clients
> +======================================
> +
> +Device nodes that need access to reset lines should
> +specify them as a reset phandle in their corresponding node as
> +specified in reset.txt.
> +
> +For list of all valid reset indicies see

s/indicies/indices/ (or s/indicies/lines/ ?)

Cheers

Matthias

Re: [PATCH v2 3/6] dt-bindings: remoteproc: Remove additional definition tag

[Matthias Kaehlcke]

Hi Sibi,

Subject: dt-bindings: remoteproc: Remove additional definition tag

nit: the subject is a bit generic, you probably should at least add
'qcom: ' to the 'path'.

On Fri, Aug 24, 2018 at 06:48:57PM +0530, Sibi Sankar wrote:
> Remove the additional definition tag declared for WCSS sub-system
> under reset-names.
> 
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> index 601dd9f389aa..c45e4c131fa2 100644
> --- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> @@ -58,7 +58,7 @@ on the Qualcomm Hexagon core.
>  	Usage: required
>  	Value type: <stringlist>
>  	Definition: must be "mss_restart" for the modem sub-system
> -	Definition: must be "wcss_aon_reset", "wcss_reset", "wcss_q6_reset"
> +		    must be "wcss_aon_reset", "wcss_reset", "wcss_q6_reset"
>  		    for the wcss syb-system

Since you are already at it you might also want to fix the typo in
'wcss syb-system'.

Other than the nits:

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

Re: [PATCH v2 2/6] reset: qcom: PDC Global (Power Domain Controller) reset controller

[Bjorn Andersson]

On Mon 27 Aug 17:22 PDT 2018, Matthias Kaehlcke wrote:
> On Fri, Aug 24, 2018 at 06:48:56PM +0530, Sibi Sankar wrote:
> > diff --git a/drivers/reset/reset-qcom-pdc.c b/drivers/reset/reset-qcom-pdc.c
[..]
> > +struct qcom_pdc_desc {
> > +	const struct regmap_config *config;
> > +	const struct qcom_pdc_reset_map *resets;
> > +	size_t num_resets;
> > +};
> 
> Not sure if this structure adds much value or just a layer of
> indirection:
> 
> - .config is only accessed in _probe(), sdm845_pdc_regmap_config could
>   be used directly
> - .resets is used in _(de)assert(), sdm845_pdc_resets could be used
>   directly
> - .num_resets is only accessed in _probe(),
>   ARRAY_SIZE(sdm845_pdc_resets) could be used instead
> 
> It probably makes sense if it is planned to support reset controllers
> of other SoCs with this driver.
> 

I like this suggestion, once we need the configurability we can add the
type for it. It also shows that only .resets would need to be referenced
by qcom_pdc_reset_data.

> > +struct qcom_pdc_reset_data {
> > +	struct reset_controller_dev rcdev;
> > +	struct regmap *regmap;
> > +	const struct qcom_pdc_desc *desc;
> > +};
[..]
> > +static int qcom_pdc_reset_probe(struct platform_device *pdev)
> > +{
[..]
> > +	data->regmap = devm_regmap_init_mmio(dev, base, desc->config);
> > +	if (IS_ERR(data->regmap)) {
> > +		dev_err(dev, "Unable to get pdc-global regmap");
> 
> Add missing '\n'
> 
> Say 'pdc-reset' instead of 'pdc-global'? (see also my other comment
> below).
> 

This regmap is created out of the single anonymous "reg", so I think the
error should be reduced to "Unable to initialize regmap\n".

[..]
> > +static const struct of_device_id qcom_pdc_reset_of_match[] = {
> > +	{ .compatible = "qcom,sdm845-pdc-global", .data = &sdm845_pdc_desc },
> 
> Should this be 'qcom,sdm845-pdc-reset' which is more specific than
> 'global' and in line with the name and purpose of the driver?
> Obviously this would require to adjust the bindings doc too.
> 

No, the binding describes the hardware block named "PDC Global",
currently implemented as a reset controller. The reason for doing this
is so that we one day can expose additional interfaces in a backwards
compatible fashion.

Regards,
Bjorn

Re: [PATCH v2 1/6] dt-bindings: reset: Add PDC Global binding for SDM845 SoCs

[sibis]

Hi Matthias,
Thanks for the review

On 2018-08-28 06:08, Matthias Kaehlcke wrote:
> Hi Sibi,
> 
> On Fri, Aug 24, 2018 at 06:48:55PM +0530, Sibi Sankar wrote:
>> Add PDC Global(Power Domain Controller) binding for SDM845 SoCs.
> 
> nit: missing blank before the opening parenthesis.
> 

Will fix it

>> 
>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>> ---
>>  .../bindings/reset/qcom,pdc-global.txt        | 52 
>> +++++++++++++++++++
>>  include/dt-bindings/reset/qcom,sdm845-pdc.h   | 20 +++++++
>>  2 files changed, 72 insertions(+)
>>  create mode 100644 
>> Documentation/devicetree/bindings/reset/qcom,pdc-global.txt
>>  create mode 100644 include/dt-bindings/reset/qcom,sdm845-pdc.h
>> 
>> diff --git 
>> a/Documentation/devicetree/bindings/reset/qcom,pdc-global.txt 
>> b/Documentation/devicetree/bindings/reset/qcom,pdc-global.txt
>> new file mode 100644
>> index 000000000000..69f9edca9503
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/reset/qcom,pdc-global.txt
>> @@ -0,0 +1,52 @@
>> +PDC Global
>> +======================================
>> +
>> +This binding describes a reset-controller found on PDC-Global(Power 
>> Domain
>> +Controller) block for Qualcomm Technologies Inc SDM845 SoCs.
> 
> Are there other PDC reset controllers that aren't 'global'? Otherwise
> I'd suggest to use 'pdc-reset' instead of 'pdc-global', which is more
> specific and in line with the name of the driver added by this series.
> Or something like 'pdc-reset-global/main' if there are other
> controllers?
> 

These are the only reset lines found in the pdc-global register space. 
But as
explained by Bjorn, wouldn't it be better to leave it as such since 
pdc-global
best describes the hardware without being limited by the current 
functionality
it is being used for?

>> +Required properties:
>> +- compatible:
>> +	Usage: required
>> +	Value type: <string>
>> +	Definition: must be:
>> +		    "qcom,sdm845-pdc-global"
>> +
>> +- reg:
>> +	Usage: required
>> +	Value type: <prop-encoded-array>
>> +	Definition: must specify the base address and size of the register
>> +	            space.
>> +
>> +- #reset-cells:
>> +	Usage: required
>> +	Value type: <uint>
>> +	Definition: must be 1; cell entry represents the reset index.
>> +
>> +Example:
>> +
>> +pdc_reset: reset-controller@b2e0000 {
>> +	compatible = "qcom,sdm845-pdc-global";
>> +	reg = <0xb2e0000 0x20000>;
>> +	#reset-cells = <1>;
>> +};
>> +
>> +PDC reset clients
>> +======================================
>> +
>> +Device nodes that need access to reset lines should
>> +specify them as a reset phandle in their corresponding node as
>> +specified in reset.txt.
>> +
>> +For list of all valid reset indicies see
> 
> s/indicies/indices/ (or s/indicies/lines/ ?)
> 
> Cheers
> 
> Matthias

Re: [PATCH v2 2/6] reset: qcom: PDC Global (Power Domain Controller) reset controller

[Sibi Sankar]

Hi Bjorn/Matthias,
Thanks for the review, will fix them in the next-respin.

On 2018-08-28 08:32, Bjorn Andersson wrote:
> On Mon 27 Aug 17:22 PDT 2018, Matthias Kaehlcke wrote:
>> On Fri, Aug 24, 2018 at 06:48:56PM +0530, Sibi Sankar wrote:
>> > diff --git a/drivers/reset/reset-qcom-pdc.c b/drivers/reset/reset-qcom-pdc.c
> [..]
>> > +struct qcom_pdc_desc {
>> > +	const struct regmap_config *config;
>> > +	const struct qcom_pdc_reset_map *resets;
>> > +	size_t num_resets;
>> > +};
>> 
>> Not sure if this structure adds much value or just a layer of
>> indirection:
>> 
>> - .config is only accessed in _probe(), sdm845_pdc_regmap_config could
>>   be used directly
>> - .resets is used in _(de)assert(), sdm845_pdc_resets could be used
>>   directly
>> - .num_resets is only accessed in _probe(),
>>   ARRAY_SIZE(sdm845_pdc_resets) could be used instead
>> 
>> It probably makes sense if it is planned to support reset controllers
>> of other SoCs with this driver.
>> 
> 
> I like this suggestion, once we need the configurability we can add the
> type for it. It also shows that only .resets would need to be 
> referenced
> by qcom_pdc_reset_data.
> 

Will change it accordingly

>> > +struct qcom_pdc_reset_data {
>> > +	struct reset_controller_dev rcdev;
>> > +	struct regmap *regmap;
>> > +	const struct qcom_pdc_desc *desc;
>> > +};
> [..]
>> > +static int qcom_pdc_reset_probe(struct platform_device *pdev)
>> > +{
> [..]
>> > +	data->regmap = devm_regmap_init_mmio(dev, base, desc->config);
>> > +	if (IS_ERR(data->regmap)) {
>> > +		dev_err(dev, "Unable to get pdc-global regmap");
>> 
>> Add missing '\n'
>> 
>> Say 'pdc-reset' instead of 'pdc-global'? (see also my other comment
>> below).
>> 
> 
> This regmap is created out of the single anonymous "reg", so I think 
> the
> error should be reduced to "Unable to initialize regmap\n".
> 

Sure will add it but aren't we trying to regmap the entire pdc-global
register space though?

> [..]
>> > +static const struct of_device_id qcom_pdc_reset_of_match[] = {
>> > +	{ .compatible = "qcom,sdm845-pdc-global", .data = &sdm845_pdc_desc },
>> 
>> Should this be 'qcom,sdm845-pdc-reset' which is more specific than
>> 'global' and in line with the name and purpose of the driver?
>> Obviously this would require to adjust the bindings doc too.
>> 
> 
> No, the binding describes the hardware block named "PDC Global",
> currently implemented as a reset controller. The reason for doing this
> is so that we one day can expose additional interfaces in a backwards
> compatible fashion.
> 
> Regards,
> Bjorn

-- 
-- Sibi Sankar --
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH v2 2/6] reset: qcom: PDC Global (Power Domain Controller) reset controller

[Matthias Kaehlcke]

On Mon, Aug 27, 2018 at 08:02:53PM -0700, Bjorn Andersson wrote:
> On Mon 27 Aug 17:22 PDT 2018, Matthias Kaehlcke wrote:
> > On Fri, Aug 24, 2018 at 06:48:56PM +0530, Sibi Sankar wrote:
> > > diff --git a/drivers/reset/reset-qcom-pdc.c b/drivers/reset/reset-qcom-pdc.c
> [..]
> > > +struct qcom_pdc_desc {
> > > +	const struct regmap_config *config;
> > > +	const struct qcom_pdc_reset_map *resets;
> > > +	size_t num_resets;
> > > +};
> > 
> > Not sure if this structure adds much value or just a layer of
> > indirection:
> > 
> > - .config is only accessed in _probe(), sdm845_pdc_regmap_config could
> >   be used directly
> > - .resets is used in _(de)assert(), sdm845_pdc_resets could be used
> >   directly
> > - .num_resets is only accessed in _probe(),
> >   ARRAY_SIZE(sdm845_pdc_resets) could be used instead
> > 
> > It probably makes sense if it is planned to support reset controllers
> > of other SoCs with this driver.
> > 
> 
> I like this suggestion, once we need the configurability we can add the
> type for it. It also shows that only .resets would need to be referenced
> by qcom_pdc_reset_data.
> 
> > > +struct qcom_pdc_reset_data {
> > > +	struct reset_controller_dev rcdev;
> > > +	struct regmap *regmap;
> > > +	const struct qcom_pdc_desc *desc;
> > > +};
> [..]
> > > +static int qcom_pdc_reset_probe(struct platform_device *pdev)
> > > +{
> [..]
> > > +	data->regmap = devm_regmap_init_mmio(dev, base, desc->config);
> > > +	if (IS_ERR(data->regmap)) {
> > > +		dev_err(dev, "Unable to get pdc-global regmap");
> > 
> > Add missing '\n'
> > 
> > Say 'pdc-reset' instead of 'pdc-global'? (see also my other comment
> > below).
> > 
> 
> This regmap is created out of the single anonymous "reg", so I think the
> error should be reduced to "Unable to initialize regmap\n".
> 
> [..]
> > > +static const struct of_device_id qcom_pdc_reset_of_match[] = {
> > > +	{ .compatible = "qcom,sdm845-pdc-global", .data = &sdm845_pdc_desc },
> > 
> > Should this be 'qcom,sdm845-pdc-reset' which is more specific than
> > 'global' and in line with the name and purpose of the driver?
> > Obviously this would require to adjust the bindings doc too.
> > 
> 
> No, the binding describes the hardware block named "PDC Global",
> currently implemented as a reset controller. The reason for doing this
> is so that we one day can expose additional interfaces in a backwards
> compatible fashion.

Sounds reasonable, thanks for the clarification.

Re: [PATCH v2 1/6] dt-bindings: reset: Add PDC Global binding for SDM845 SoCs

[Matthias Kaehlcke]

On Tue, Aug 28, 2018 at 11:38:26AM +0530, sibis@codeaurora.org wrote:
> Hi Matthias,
> Thanks for the review
> 
> On 2018-08-28 06:08, Matthias Kaehlcke wrote:
> > Hi Sibi,
> > 
> > On Fri, Aug 24, 2018 at 06:48:55PM +0530, Sibi Sankar wrote:
> > > Add PDC Global(Power Domain Controller) binding for SDM845 SoCs.
> > 
> > nit: missing blank before the opening parenthesis.
> > 
> 
> Will fix it
> 
> > > 
> > > Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> > > ---
> > >  .../bindings/reset/qcom,pdc-global.txt        | 52
> > > +++++++++++++++++++
> > >  include/dt-bindings/reset/qcom,sdm845-pdc.h   | 20 +++++++
> > >  2 files changed, 72 insertions(+)
> > >  create mode 100644
> > > Documentation/devicetree/bindings/reset/qcom,pdc-global.txt
> > >  create mode 100644 include/dt-bindings/reset/qcom,sdm845-pdc.h
> > > 
> > > diff --git
> > > a/Documentation/devicetree/bindings/reset/qcom,pdc-global.txt
> > > b/Documentation/devicetree/bindings/reset/qcom,pdc-global.txt
> > > new file mode 100644
> > > index 000000000000..69f9edca9503
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/reset/qcom,pdc-global.txt
> > > @@ -0,0 +1,52 @@
> > > +PDC Global
> > > +======================================
> > > +
> > > +This binding describes a reset-controller found on PDC-Global(Power
> > > Domain
> > > +Controller) block for Qualcomm Technologies Inc SDM845 SoCs.
> > 
> > Are there other PDC reset controllers that aren't 'global'? Otherwise
> > I'd suggest to use 'pdc-reset' instead of 'pdc-global', which is more
> > specific and in line with the name of the driver added by this series.
> > Or something like 'pdc-reset-global/main' if there are other
> > controllers?
> > 
> 
> These are the only reset lines found in the pdc-global register space. But
> as
> explained by Bjorn, wouldn't it be better to leave it as such since
> pdc-global
> best describes the hardware without being limited by the current
> functionality
> it is being used for?

If I understand Bjorn's reply on
https://patchwork.kernel.org/patch/10575335/ correctly it is planned
to use the 'pdc-global' compatible string in the future in some sort
of MFD driver for the 'PDC Global' IP block, which then instantiates
other drivers like the reset controller. In this case I agree that
'pdc-global' seems a reasonable choice.

Re: [PATCH v2 1/6] dt-bindings: reset: Add PDC Global binding for SDM845 SoCs

[Rob Herring]

On Fri, 24 Aug 2018 18:48:55 +0530, Sibi Sankar wrote:
> Add PDC Global(Power Domain Controller) binding for SDM845 SoCs.
> 
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  .../bindings/reset/qcom,pdc-global.txt        | 52 +++++++++++++++++++
>  include/dt-bindings/reset/qcom,sdm845-pdc.h   | 20 +++++++
>  2 files changed, 72 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/qcom,pdc-global.txt
>  create mode 100644 include/dt-bindings/reset/qcom,sdm845-pdc.h
> 

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

Re: [PATCH v2 4/6] dt-bindings: remoteproc: Add PDC reset binding for Q6V5 PIL

[Rob Herring]

On Fri, 24 Aug 2018 18:48:58 +0530, Sibi Sankar wrote:
> Add additional pdc_reset binding required for Q6V5 Modem PIL on
> SDM845 SoCs.
> 
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 

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

Re: [PATCH v2 3/6] dt-bindings: remoteproc: Remove additional definition tag

[Sibi Sankar]

Hi Matthias,
Thanks for the review!

On 2018-08-28 06:14, Matthias Kaehlcke wrote:
> Hi Sibi,
> 
> Subject: dt-bindings: remoteproc: Remove additional definition tag
> 
> nit: the subject is a bit generic, you probably should at least add
> 'qcom: ' to the 'path'.
> 

Will add

> On Fri, Aug 24, 2018 at 06:48:57PM +0530, Sibi Sankar wrote:
>> Remove the additional definition tag declared for WCSS sub-system
>> under reset-names.
>> 
>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>> ---
>>  Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git 
>> a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt 
>> b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
>> index 601dd9f389aa..c45e4c131fa2 100644
>> --- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
>> @@ -58,7 +58,7 @@ on the Qualcomm Hexagon core.
>>  	Usage: required
>>  	Value type: <stringlist>
>>  	Definition: must be "mss_restart" for the modem sub-system
>> -	Definition: must be "wcss_aon_reset", "wcss_reset", "wcss_q6_reset"
>> +		    must be "wcss_aon_reset", "wcss_reset", "wcss_q6_reset"
>>  		    for the wcss syb-system
> 
> Since you are already at it you might also want to fix the typo in
> 'wcss syb-system'.
> 

nice catch, will correct it.

I missed your query in the other thread, APPS refers to the pdc_sync 
reset line for the Application processor.

> Other than the nits:
> 
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

-- 
-- Sibi Sankar --
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.