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

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

[Sibi Sankar]

Add SDM845 PDC (Power Domain Controller) reset controller binding

Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
---
 .../bindings/reset/qcom,pdc-reset.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-reset.txt
 create mode 100644 include/dt-bindings/reset/qcom,sdm845-pdc.h

diff --git a/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
new file mode 100644
index 000000000000..85e159962e08
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
@@ -0,0 +1,52 @@
+PDC Reset Controller
+======================================
+
+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_restart";
+
+	...
+};
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 2/4] reset: qcom: PDC (Power Domain Controller) reset controller

[Sibi Sankar]

Add reset controller for SDM845 SoC to control reset signals
provided by PDC 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 | 139 +++++++++++++++++++++++++++++++++
 3 files changed, 149 insertions(+)
 create mode 100644 drivers/reset/reset-qcom-pdc.c

diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index 13d28fdbdbb5..5344e202a630 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
+	bool "Qcom 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..64a0041e3452
--- /dev/null
+++ b/drivers/reset/reset-qcom-pdc.c
@@ -0,0 +1,139 @@
+// 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>
+
+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, 0x100,
+					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, 0x100, 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 },
+	{}
+};
+
+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,
+	},
+};
+
+builtin_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 3/4] remoteproc: qcom: q6v5-pil: Explicitly get mss_restart line

[Sibi Sankar]

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

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

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index b1296d614b8b..d57fdb34e3dd 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -1176,8 +1176,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);
+	qproc->mss_restart = devm_reset_control_get(qproc->dev, "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 4/4] remoteproc: qcom: q6v5-pil: Add PDC restart for modem on SDM845 SoCs

[Sibi Sankar]

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

Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
---
 .../bindings/remoteproc/qcom,q6v5.txt         |  4 +++
 drivers/remoteproc/qcom_q6v5_pil.c            | 27 ++++++++++++++++---
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
index 601dd9f389aa..124fb1dc6fb8 100644
--- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
@@ -51,6 +51,8 @@ on the Qualcomm Hexagon core.
 	Usage: required
 	Value type: <phandle>
 	Definition: reference to the reset-controller for the modem sub-system
+		    reference to the list of 2 reset-controllers for the modem
+		    sub-system on SDM845 SoCs
 		    reference to the list of 3 reset-controllers for the
 		    wcss sub-system
 
@@ -58,6 +60,8 @@ on the Qualcomm Hexagon core.
 	Usage: required
 	Value type: <stringlist>
 	Definition: must be "mss_restart" for the modem sub-system
+	Definition: must be "mss_restart", "pdc_restart" for the modem
+		    sub-system on SDM845 SoCs
 	Definition: must be "wcss_aon_reset", "wcss_reset", "wcss_q6_reset"
 		    for the wcss syb-system
 
diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index d57fdb34e3dd..5be794639fc3 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_restart;
 
 	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_restart);
+		ret = reset_control_reset(qproc->mss_restart);
+		reset_control_deassert(qproc->pdc_restart);
+	} 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_restart);
 		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_restart);
 	} else {
 		ret = reset_control_deassert(qproc->mss_restart);
 	}
@@ -1182,6 +1192,15 @@ static int q6v5_init_reset(struct q6v5 *qproc)
 		return PTR_ERR(qproc->mss_restart);
 	}
 
+	if (qproc->has_alt_reset) {
+		qproc->pdc_restart = devm_reset_control_get(qproc->dev,
+							    "pdc_restart");
+		if (IS_ERR(qproc->pdc_restart)) {
+			dev_err(qproc->dev, "failed to acquire pdc restart\n");
+			return PTR_ERR(qproc->pdc_restart);
+		}
+	}
+
 	return 0;
 }
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Philipp Zabel]

Hi Sibi,

On Fri, 2018-07-27 at 20:58 +0530, Sibi Sankar wrote:
> Add SDM845 PDC (Power Domain Controller) reset controller binding
> 
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  .../bindings/reset/qcom,pdc-reset.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-reset.txt
>  create mode 100644 include/dt-bindings/reset/qcom,sdm845-pdc.h
> 
> diff --git a/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
> new file mode 100644
> index 000000000000..85e159962e08
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
> @@ -0,0 +1,52 @@
> +PDC Reset Controller
> +======================================
> +
> +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 {

Is this really just a reset controller?

The name makes it sound like a driver binding to this should also
provide pm_genpd and the binding should probably call this a power-
controller: Documentation/devicetree/bindings/power/power_domain.txt.

> +	compatible = "qcom,sdm845-pdc-global";
> +	reg = <0xb2e0000 0x20000>;

This looks like this is the register space of the complete PDC, not just
the reset register?

> +	#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_restart";
> +
> +	...
> +};
> 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

regards
Philipp

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

[Philipp Zabel]

On Fri, 2018-07-27 at 20:58 +0530, Sibi Sankar wrote:
> Add reset controller for SDM845 SoC to control reset signals
> provided by PDC 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 | 139 +++++++++++++++++++++++++++++++++
>  3 files changed, 149 insertions(+)
>  create mode 100644 drivers/reset/reset-qcom-pdc.c
> 
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 13d28fdbdbb5..5344e202a630 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
> +	bool "Qcom 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..64a0041e3452
> --- /dev/null
> +++ b/drivers/reset/reset-qcom-pdc.c
> @@ -0,0 +1,139 @@
> +// 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>
> +
> +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, 0x100,

Does this register have a name? If so, a #define would be preferable.
Otherwise this driver looks fine to me.

> +					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, 0x100, 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 },
> +	{}
> +};
> +
> +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,
> +	},
> +};
> +
> +builtin_platform_driver(qcom_pdc_reset_driver);
> +
> +MODULE_DESCRIPTION("Qualcomm PDC Reset Driver");
> +MODULE_LICENSE("GPL v2");

regards
Philipp

Re: [PATCH 4/4] remoteproc: qcom: q6v5-pil: Add PDC restart for modem on SDM845 SoCs

[Philipp Zabel]

On Fri, 2018-07-27 at 20:58 +0530, Sibi Sankar wrote:
> In the presence of a PDC block working with subsystem RSC,
> assert/deassert PDC restart in modem start/stop path.
> 
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  .../bindings/remoteproc/qcom,q6v5.txt         |  4 +++
>  drivers/remoteproc/qcom_q6v5_pil.c            | 27 ++++++++++++++++---
>  2 files changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> index 601dd9f389aa..124fb1dc6fb8 100644
> --- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
[...]
> @@ -1182,6 +1192,15 @@ static int q6v5_init_reset(struct q6v5 *qproc)
>  		return PTR_ERR(qproc->mss_restart);
>  	}
>  
> +	if (qproc->has_alt_reset) {
> +		qproc->pdc_restart = devm_reset_control_get(qproc->dev,
> +							    "pdc_restart");

Please use devm_reset_control_get_exclusive() instead.

> +		if (IS_ERR(qproc->pdc_restart)) {
> +			dev_err(qproc->dev, "failed to acquire pdc restart\n");
> +			return PTR_ERR(qproc->pdc_restart);
> +		}
> +	}
> +
>  	return 0;
>  }

regards
Philipp

Re: [PATCH 3/4] remoteproc: qcom: q6v5-pil: Explicitly get mss_restart line

[Philipp Zabel]

On Fri, 2018-07-27 at 20:58 +0530, Sibi Sankar wrote:
> Explicitly get mss_restart to facilitate adding PDC
> restart line for modem on SDM845 SoCs
> 
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_q6v5_pil.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
> index b1296d614b8b..d57fdb34e3dd 100644
> --- a/drivers/remoteproc/qcom_q6v5_pil.c
> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> @@ -1176,8 +1176,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);
> +	qproc->mss_restart = devm_reset_control_get(qproc->dev, "mss_restart");

Please keep using devm_reset_control_get_exclusive.

regards
Philipp

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

[Sibi S]

Hi Philipp,
Thanks for the review!

On 07/31/2018 02:12 PM, Philipp Zabel wrote:
> Hi Sibi,
> 
> On Fri, 2018-07-27 at 20:58 +0530, Sibi Sankar wrote:
>> Add SDM845 PDC (Power Domain Controller) reset controller binding
>>
>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>> ---
>>   .../bindings/reset/qcom,pdc-reset.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-reset.txt
>>   create mode 100644 include/dt-bindings/reset/qcom,sdm845-pdc.h
>>
>> diff --git a/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
>> new file mode 100644
>> index 000000000000..85e159962e08
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
>> @@ -0,0 +1,52 @@
>> +PDC Reset Controller
>> +======================================
>> +
>> +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 {
> 
> Is this really just a reset controller?
> 
> The name makes it sound like a driver binding to this should also
> provide pm_genpd and the binding should probably call this a power-
> controller: Documentation/devicetree/bindings/power/power_domain.txt.
> 

The PDC-global reg space which is a part of PDC-wrapper reg space seems
to be only used for the reset lines.

Couple of other drivers use other parts of the PDC-wrapper reg space:
https://patchwork.kernel.org/patch/10223701/ (PDC-Interrupt controller)
https://patchwork.kernel.org/patch/10255767/ (GMU-PDC incorrectly tries
to occupy the entire pdc-wrapper reg space)

since it couldn't be logically mapped into pdc-interrupt driver, it had
to be included as a separate reset driver.

>> +	compatible = "qcom,sdm845-pdc-global";
>> +	reg = <0xb2e0000 0x20000>;
> 
> This looks like this is the register space of the complete PDC, not just
> the reset register?
> 

The entire register space was chosen because it is only used for its
reset lines (had a good look at the downstream kernel and had a 
conversation with Lina) and to ensure break backward compatibility for
the for the dt entry if the reg-space was used for other purposes in
the future.

>> +	#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_restart";
>> +
>> +	...
>> +};
>> 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
> 
> regards
> Philipp
> 

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

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

[Sibi S]

Hi Philipp,
Thanks for the review!

On 07/31/2018 02:21 PM, Philipp Zabel wrote:
> On Fri, 2018-07-27 at 20:58 +0530, Sibi Sankar wrote:
>> Add reset controller for SDM845 SoC to control reset signals
>> provided by PDC 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 | 139 +++++++++++++++++++++++++++++++++
>>   3 files changed, 149 insertions(+)
>>   create mode 100644 drivers/reset/reset-qcom-pdc.c
>>
>> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
>> index 13d28fdbdbb5..5344e202a630 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
>> +	bool "Qcom 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..64a0041e3452
>> --- /dev/null
>> +++ b/drivers/reset/reset-qcom-pdc.c
>> @@ -0,0 +1,139 @@
>> +// 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>
>> +
>> +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, 0x100,
> 
> Does this register have a name? If so, a #define would be preferable.
> Otherwise this driver looks fine to me.
> 

Yes will had a separate #define for it.

>> +					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, 0x100, 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 },
>> +	{}
>> +};
>> +
>> +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,
>> +	},
>> +};
>> +
>> +builtin_platform_driver(qcom_pdc_reset_driver);
>> +
>> +MODULE_DESCRIPTION("Qualcomm PDC Reset Driver");
>> +MODULE_LICENSE("GPL v2");
> 
> regards
> Philipp
> 

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

Re: [PATCH 3/4] remoteproc: qcom: q6v5-pil: Explicitly get mss_restart line

[Sibi S]

Hi Philipp,
Thanks for the review!

On 07/31/2018 02:27 PM, Philipp Zabel wrote:
> On Fri, 2018-07-27 at 20:58 +0530, Sibi Sankar wrote:
>> Explicitly get mss_restart to facilitate adding PDC
>> restart line for modem on SDM845 SoCs
>>
>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>> ---
>>   drivers/remoteproc/qcom_q6v5_pil.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
>> index b1296d614b8b..d57fdb34e3dd 100644
>> --- a/drivers/remoteproc/qcom_q6v5_pil.c
>> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
>> @@ -1176,8 +1176,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);
>> +	qproc->mss_restart = devm_reset_control_get(qproc->dev, "mss_restart");
> 
> Please keep using devm_reset_control_get_exclusive.
> 

got misled when I saw it being used in a recent driver :), will revert
it back.

> regards
> Philipp
> 

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

Re: [PATCH 4/4] remoteproc: qcom: q6v5-pil: Add PDC restart for modem on SDM845 SoCs

[Sibi S]

Hi Philipp,
Thanks for the review!

On 07/31/2018 02:24 PM, Philipp Zabel wrote:
> On Fri, 2018-07-27 at 20:58 +0530, Sibi Sankar wrote:
>> In the presence of a PDC block working with subsystem RSC,
>> assert/deassert PDC restart in modem start/stop path.
>>
>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>> ---
>>   .../bindings/remoteproc/qcom,q6v5.txt         |  4 +++
>>   drivers/remoteproc/qcom_q6v5_pil.c            | 27 ++++++++++++++++---
>>   2 files changed, 27 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
>> index 601dd9f389aa..124fb1dc6fb8 100644
>> --- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> [...]
>> @@ -1182,6 +1192,15 @@ static int q6v5_init_reset(struct q6v5 *qproc)
>>   		return PTR_ERR(qproc->mss_restart);
>>   	}
>>   
>> +	if (qproc->has_alt_reset) {
>> +		qproc->pdc_restart = devm_reset_control_get(qproc->dev,
>> +							    "pdc_restart");
> 
> Please use devm_reset_control_get_exclusive() instead.
> 

will replace it in the next re-spin.

>> +		if (IS_ERR(qproc->pdc_restart)) {
>> +			dev_err(qproc->dev, "failed to acquire pdc restart\n");
>> +			return PTR_ERR(qproc->pdc_restart);
>> +		}
>> +	}
>> +
>>   	return 0;
>>   }
> 
> regards
> Philipp
> 

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

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

[Rob Herring]

On Tue, Jul 31, 2018 at 06:27:24PM +0530, Sibi S wrote:
> Hi Philipp,
> Thanks for the review!
> 
> On 07/31/2018 02:12 PM, Philipp Zabel wrote:
> > Hi Sibi,
> > 
> > On Fri, 2018-07-27 at 20:58 +0530, Sibi Sankar wrote:
> > > Add SDM845 PDC (Power Domain Controller) reset controller binding
> > > 
> > > Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> > > ---
> > >   .../bindings/reset/qcom,pdc-reset.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-reset.txt
> > >   create mode 100644 include/dt-bindings/reset/qcom,sdm845-pdc.h
> > > 
> > > diff --git a/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
> > > new file mode 100644
> > > index 000000000000..85e159962e08
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
> > > @@ -0,0 +1,52 @@
> > > +PDC Reset Controller
> > > +======================================
> > > +
> > > +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 {
> > 
> > Is this really just a reset controller?
> > 
> > The name makes it sound like a driver binding to this should also
> > provide pm_genpd and the binding should probably call this a power-
> > controller: Documentation/devicetree/bindings/power/power_domain.txt.
> > 
> 
> The PDC-global reg space which is a part of PDC-wrapper reg space seems
> to be only used for the reset lines.
> 
> Couple of other drivers use other parts of the PDC-wrapper reg space:
> https://patchwork.kernel.org/patch/10223701/ (PDC-Interrupt controller)
> https://patchwork.kernel.org/patch/10255767/ (GMU-PDC incorrectly tries
> to occupy the entire pdc-wrapper reg space)
> 
> since it couldn't be logically mapped into pdc-interrupt driver, it had
> to be included as a separate reset driver.

You can't have overlapping regions in DT (well, you can because we have 
to work-around existing DTs that do, but you shouldn't).

A single node can be multiple providers such as interrupt controller and 
reset controller. It's an OS problem to split that into multiple 
drivers.
 
> > > +	compatible = "qcom,sdm845-pdc-global";
> > > +	reg = <0xb2e0000 0x20000>;
> > 
> > This looks like this is the register space of the complete PDC, not just
> > the reset register?
> > 
> 
> The entire register space was chosen because it is only used for its
> reset lines (had a good look at the downstream kernel and had a conversation
> with Lina) and to ensure break backward compatibility for
> the for the dt entry if the reg-space was used for other purposes in
> the future.

Why do you want to ensure breaking backwards compatibility?

Rob

Re: [PATCH 4/4] remoteproc: qcom: q6v5-pil: Add PDC restart for modem on SDM845 SoCs

[Rob Herring]

On Fri, Jul 27, 2018 at 08:58:11PM +0530, Sibi Sankar wrote:
> In the presence of a PDC block working with subsystem RSC,
> assert/deassert PDC restart in modem start/stop path.
> 
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  .../bindings/remoteproc/qcom,q6v5.txt         |  4 +++

Please split bindings to separate patch.

>  drivers/remoteproc/qcom_q6v5_pil.c            | 27 ++++++++++++++++---
>  2 files changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> index 601dd9f389aa..124fb1dc6fb8 100644
> --- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> @@ -51,6 +51,8 @@ on the Qualcomm Hexagon core.
>  	Usage: required
>  	Value type: <phandle>
>  	Definition: reference to the reset-controller for the modem sub-system
> +		    reference to the list of 2 reset-controllers for the modem
> +		    sub-system on SDM845 SoCs
>  		    reference to the list of 3 reset-controllers for the
>  		    wcss sub-system
>  
> @@ -58,6 +60,8 @@ on the Qualcomm Hexagon core.
>  	Usage: required
>  	Value type: <stringlist>
>  	Definition: must be "mss_restart" for the modem sub-system
> +	Definition: must be "mss_restart", "pdc_restart" for the modem
> +		    sub-system on SDM845 SoCs
>  	Definition: must be "wcss_aon_reset", "wcss_reset", "wcss_q6_reset"
>  		    for the wcss syb-system
>  

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

[Sibi Sankar]

Hi Rob,
Thanks for the review

On 08/07/2018 11:46 PM, Rob Herring wrote:
> On Tue, Jul 31, 2018 at 06:27:24PM +0530, Sibi S wrote:
>> Hi Philipp,
>> Thanks for the review!
>>
>> On 07/31/2018 02:12 PM, Philipp Zabel wrote:
>>> Hi Sibi,
>>>
>>> On Fri, 2018-07-27 at 20:58 +0530, Sibi Sankar wrote:
>>>> Add SDM845 PDC (Power Domain Controller) reset controller binding
>>>>
>>>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>>>> ---
>>>>    .../bindings/reset/qcom,pdc-reset.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-reset.txt
>>>>    create mode 100644 include/dt-bindings/reset/qcom,sdm845-pdc.h
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
>>>> new file mode 100644
>>>> index 000000000000..85e159962e08
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
>>>> @@ -0,0 +1,52 @@
>>>> +PDC Reset Controller
>>>> +======================================
>>>> +
>>>> +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 {
>>>
>>> Is this really just a reset controller?
>>>
>>> The name makes it sound like a driver binding to this should also
>>> provide pm_genpd and the binding should probably call this a power-
>>> controller: Documentation/devicetree/bindings/power/power_domain.txt.
>>>
>>
>> The PDC-global reg space which is a part of PDC-wrapper reg space seems
>> to be only used for the reset lines.
>>
>> Couple of other drivers use other parts of the PDC-wrapper reg space:
>> https://patchwork.kernel.org/patch/10223701/ (PDC-Interrupt controller)
>> https://patchwork.kernel.org/patch/10255767/ (GMU-PDC incorrectly tries
>> to occupy the entire pdc-wrapper reg space)
>>
>> since it couldn't be logically mapped into pdc-interrupt driver, it had
>> to be included as a separate reset driver.
> 
> You can't have overlapping regions in DT (well, you can because we have
> to work-around existing DTs that do, but you shouldn't).
> 
> A single node can be multiple providers such as interrupt controller and
> reset controller. It's an OS problem to split that into multiple
> drivers.
>   

There will be no overlaps. Jordan will be changing the dt binding of
gmu_pdc so that there is no overlap I guess. What I meant to say is that
pdc-global is a separate reg-space and currently has no other
functionality other than exposing the reset lines.

>>>> +	compatible = "qcom,sdm845-pdc-global";
>>>> +	reg = <0xb2e0000 0x20000>;
>>>
>>> This looks like this is the register space of the complete PDC, not just
>>> the reset register?
>>>
>>
>> The entire register space was chosen because it is only used for its
>> reset lines (had a good look at the downstream kernel and had a conversation
>> with Lina) and to ensure break backward compatibility for
>> the for the dt entry if the reg-space was used for other purposes in
>> the future.
> 
> Why do you want to ensure breaking backwards compatibility?
>

Similar to the AOSS reset driver which had a unused clock part, this
driver also exposes a reg space of which only reset lines are used.

> Rob
> 

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

Re: [PATCH 4/4] remoteproc: qcom: q6v5-pil: Add PDC restart for modem on SDM845 SoCs

[Sibi Sankar]

On 08/07/2018 11:48 PM, Rob Herring wrote:
> On Fri, Jul 27, 2018 at 08:58:11PM +0530, Sibi Sankar wrote:
>> In the presence of a PDC block working with subsystem RSC,
>> assert/deassert PDC restart in modem start/stop path.
>>
>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>> ---
>>   .../bindings/remoteproc/qcom,q6v5.txt         |  4 +++
> 
> Please split bindings to separate patch.
>

Okay will split them.

>>   drivers/remoteproc/qcom_q6v5_pil.c            | 27 ++++++++++++++++---
>>   2 files changed, 27 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
>> index 601dd9f389aa..124fb1dc6fb8 100644
>> --- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
>> @@ -51,6 +51,8 @@ on the Qualcomm Hexagon core.
>>   	Usage: required
>>   	Value type: <phandle>
>>   	Definition: reference to the reset-controller for the modem sub-system
>> +		    reference to the list of 2 reset-controllers for the modem
>> +		    sub-system on SDM845 SoCs
>>   		    reference to the list of 3 reset-controllers for the
>>   		    wcss sub-system
>>   
>> @@ -58,6 +60,8 @@ on the Qualcomm Hexagon core.
>>   	Usage: required
>>   	Value type: <stringlist>
>>   	Definition: must be "mss_restart" for the modem sub-system
>> +	Definition: must be "mss_restart", "pdc_restart" for the modem
>> +		    sub-system on SDM845 SoCs
>>   	Definition: must be "wcss_aon_reset", "wcss_reset", "wcss_q6_reset"
>>   		    for the wcss syb-system
>>   

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

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

[Jordan Crouse]

On Wed, Aug 08, 2018 at 09:14:05PM +0530, Sibi Sankar wrote:
> Hi Rob,
> Thanks for the review
> 
> On 08/07/2018 11:46 PM, Rob Herring wrote:
> >On Tue, Jul 31, 2018 at 06:27:24PM +0530, Sibi S wrote:
> >>Hi Philipp,
> >>Thanks for the review!
> >>
> >>On 07/31/2018 02:12 PM, Philipp Zabel wrote:
> >>>Hi Sibi,
> >>>
> >>>On Fri, 2018-07-27 at 20:58 +0530, Sibi Sankar wrote:
> >>>>Add SDM845 PDC (Power Domain Controller) reset controller binding
> >>>>
> >>>>Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> >>>>---
> >>>>   .../bindings/reset/qcom,pdc-reset.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-reset.txt
> >>>>   create mode 100644 include/dt-bindings/reset/qcom,sdm845-pdc.h
> >>>>
> >>>>diff --git a/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
> >>>>new file mode 100644
> >>>>index 000000000000..85e159962e08
> >>>>--- /dev/null
> >>>>+++ b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
> >>>>@@ -0,0 +1,52 @@
> >>>>+PDC Reset Controller
> >>>>+======================================
> >>>>+
> >>>>+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 {
> >>>
> >>>Is this really just a reset controller?
> >>>
> >>>The name makes it sound like a driver binding to this should also
> >>>provide pm_genpd and the binding should probably call this a power-
> >>>controller: Documentation/devicetree/bindings/power/power_domain.txt.
> >>>
> >>
> >>The PDC-global reg space which is a part of PDC-wrapper reg space seems
> >>to be only used for the reset lines.
> >>
> >>Couple of other drivers use other parts of the PDC-wrapper reg space:
> >>https://patchwork.kernel.org/patch/10223701/ (PDC-Interrupt controller)
> >>https://patchwork.kernel.org/patch/10255767/ (GMU-PDC incorrectly tries
> >>to occupy the entire pdc-wrapper reg space)
> >>
> >>since it couldn't be logically mapped into pdc-interrupt driver, it had
> >>to be included as a separate reset driver.
> >
> >You can't have overlapping regions in DT (well, you can because we have
> >to work-around existing DTs that do, but you shouldn't).
> >
> >A single node can be multiple providers such as interrupt controller and
> >reset controller. It's an OS problem to split that into multiple
> >drivers.
> 
> There will be no overlaps. Jordan will be changing the dt binding of
> gmu_pdc so that there is no overlap I guess. What I meant to say is that
> pdc-global is a separate reg-space and currently has no other
> functionality other than exposing the reset lines.

Correct - the updated GPU DT will be:

  reg = <0x506a000 0x30000>,
     <0xb280000 0x10000>,
     <0xb480000, 0x10000>;
  reg-names = "gmu", "gmu_pdc", "gmu_pdc_seq";

Jordan

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

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

[Jordan Crouse]

On Wed, Aug 08, 2018 at 03:37:24PM -0600, Jordan Crouse wrote:
> On Wed, Aug 08, 2018 at 09:14:05PM +0530, Sibi Sankar wrote:
> > Hi Rob,
> > Thanks for the review
> > 
> > On 08/07/2018 11:46 PM, Rob Herring wrote:
> > >On Tue, Jul 31, 2018 at 06:27:24PM +0530, Sibi S wrote:
> > >>Hi Philipp,
> > >>Thanks for the review!
> > >>
> > >>On 07/31/2018 02:12 PM, Philipp Zabel wrote:
> > >>>Hi Sibi,
> > >>>
> > >>>On Fri, 2018-07-27 at 20:58 +0530, Sibi Sankar wrote:
> > >>>>Add SDM845 PDC (Power Domain Controller) reset controller binding
> > >>>>
> > >>>>Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> > >>>>---
> > >>>>   .../bindings/reset/qcom,pdc-reset.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-reset.txt
> > >>>>   create mode 100644 include/dt-bindings/reset/qcom,sdm845-pdc.h
> > >>>>
> > >>>>diff --git a/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
> > >>>>new file mode 100644
> > >>>>index 000000000000..85e159962e08
> > >>>>--- /dev/null
> > >>>>+++ b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
> > >>>>@@ -0,0 +1,52 @@
> > >>>>+PDC Reset Controller
> > >>>>+======================================
> > >>>>+
> > >>>>+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 {
> > >>>
> > >>>Is this really just a reset controller?
> > >>>
> > >>>The name makes it sound like a driver binding to this should also
> > >>>provide pm_genpd and the binding should probably call this a power-
> > >>>controller: Documentation/devicetree/bindings/power/power_domain.txt.
> > >>>
> > >>
> > >>The PDC-global reg space which is a part of PDC-wrapper reg space seems
> > >>to be only used for the reset lines.
> > >>
> > >>Couple of other drivers use other parts of the PDC-wrapper reg space:
> > >>https://patchwork.kernel.org/patch/10223701/ (PDC-Interrupt controller)
> > >>https://patchwork.kernel.org/patch/10255767/ (GMU-PDC incorrectly tries
> > >>to occupy the entire pdc-wrapper reg space)
> > >>
> > >>since it couldn't be logically mapped into pdc-interrupt driver, it had
> > >>to be included as a separate reset driver.
> > >
> > >You can't have overlapping regions in DT (well, you can because we have
> > >to work-around existing DTs that do, but you shouldn't).
> > >
> > >A single node can be multiple providers such as interrupt controller and
> > >reset controller. It's an OS problem to split that into multiple
> > >drivers.
> > 
> > There will be no overlaps. Jordan will be changing the dt binding of
> > gmu_pdc so that there is no overlap I guess. What I meant to say is that
> > pdc-global is a separate reg-space and currently has no other
> > functionality other than exposing the reset lines.
> 
> Correct - the updated GPU DT will be:
> 
>   reg = <0x506a000 0x30000>,
>      <0xb280000 0x10000>,
>      <0xb480000, 0x10000>;
>   reg-names = "gmu", "gmu_pdc", "gmu_pdc_seq";
> 
> Jordan

Code change:  https://patchwork.freedesktop.org/patch/243513/
DT change: https://patchwork.freedesktop.org/patch/243539/

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

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

[Bjorn Andersson]

On Fri 27 Jul 08:28 PDT 2018, Sibi Sankar wrote:

> Add SDM845 PDC (Power Domain Controller) reset controller binding
> 

Even though this is currently describing only a reset controller I think
this binding better be talking about the "PDC Global" hardware.

> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  .../bindings/reset/qcom,pdc-reset.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-reset.txt
>  create mode 100644 include/dt-bindings/reset/qcom,sdm845-pdc.h
> 
> diff --git a/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt

Rename this qcom,pdc-global to match the compatible, and hardware name.

> new file mode 100644
> index 000000000000..85e159962e08
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
> @@ -0,0 +1,52 @@
> +PDC Reset Controller

PDC Global

> +======================================
> +
> +This binding describes a reset-controller found on PDC-Global(Power Domain
> +Controller) block for Qualcomm Technologies Inc SDM845 SoCs.

This looks good.

> +
> +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 {

This is perfectly fine, in its current form it is a reset-controller.

> +	compatible = "qcom,sdm845-pdc-global";
> +	reg = <0xb2e0000 0x20000>;
> +	#reset-cells = <1>;
> +};
> +

Apart from this, the binding looks good!

Regards,
Bjorn

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

[Bjorn Andersson]

On Fri 27 Jul 08:28 PDT 2018, Sibi Sankar wrote:

> Add reset controller for SDM845 SoC to control reset signals
> provided by PDC 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 | 139 +++++++++++++++++++++++++++++++++
>  3 files changed, 149 insertions(+)
>  create mode 100644 drivers/reset/reset-qcom-pdc.c
> 
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 13d28fdbdbb5..5344e202a630 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
> +	bool "Qcom PDC Reset Driver"

"Qualcomm"

And I do not see a strong reason for this not being tristate, the
consumers we've talked about so far can all be modules.

> +	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.
> +

Implementation looks fine.

Regards,
Bjorn

Re: [PATCH 4/4] remoteproc: qcom: q6v5-pil: Add PDC restart for modem on SDM845 SoCs

[Bjorn Andersson]

On Fri 27 Jul 08:28 PDT 2018, Sibi Sankar wrote:

> In the presence of a PDC block working with subsystem RSC,
> assert/deassert PDC restart in modem start/stop path.
> 
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  .../bindings/remoteproc/qcom,q6v5.txt         |  4 +++
>  drivers/remoteproc/qcom_q6v5_pil.c            | 27 ++++++++++++++++---
>  2 files changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> index 601dd9f389aa..124fb1dc6fb8 100644
> --- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> @@ -51,6 +51,8 @@ on the Qualcomm Hexagon core.
>  	Usage: required
>  	Value type: <phandle>
>  	Definition: reference to the reset-controller for the modem sub-system
> +		    reference to the list of 2 reset-controllers for the modem
> +		    sub-system on SDM845 SoCs
>  		    reference to the list of 3 reset-controllers for the
>  		    wcss sub-system
>  
> @@ -58,6 +60,8 @@ on the Qualcomm Hexagon core.
>  	Usage: required
>  	Value type: <stringlist>
>  	Definition: must be "mss_restart" for the modem sub-system
> +	Definition: must be "mss_restart", "pdc_restart" for the modem
> +		    sub-system on SDM845 SoCs
>  	Definition: must be "wcss_aon_reset", "wcss_reset", "wcss_q6_reset"
>  		    for the wcss syb-system

Seems like we got an additional "Definition:" added here as we added the
wcss. Please don't add another one.

The rest looks good.

Regards,
Bjorn