[PATCH v4 0/5] Add support for remoteproc modem-pil on SDM845 SoCs

[PATCH v4 0/5] Add support for remoteproc modem-pil on SDM845 SoCs

[Sibi Sankar]

This patch series add support for remoteproc Q6v5 modem-pil on Qualcomm
SDM845 SoC. The first patch adds AOSS (Always on subsystem) reset driver
to provide for mss reset line. The last couple of patches add the resets
sequence for Q6 on SDM845 and adds helper functions for arbitrary reset
assert/deassert sequences.

V4:
   Removed regmap depencencies from aoss reset driver
   Separted apss shared mailbox into separate patch
   Corrected all nits and replaced with author full name

V3:
   Removed syscon dependency for the aoss reset driver
   Split dt-bindings and the aoss reset driver into separate patches
   Corrected few typos and replaced misconfigured author name

V2:
   Addressed reset-qcom-aoss review suggestions and reworked
   re-ordering of the active clk and reset sequence in
   qcom_q6v5_pil

Depends on:
https://patchwork.kernel.org/patch/10363397/

Sibi Sankar (5):
  dt-bindings: reset: Add AOSS reset bindings for SDM845 SoCs
  reset: qcom: AOSS (always on subsystem) reset controller
  dt-bindings: remoteproc: Add Q6v5 Modem PIL binding for SDM845
  remoteproc: qcom: Add support for mss remoteproc on SDM845
  remoteproc: qcom: Always assert and deassert reset signals in SDM845

 .../bindings/remoteproc/qcom,q6v5.txt         |   1 +
 .../bindings/reset/qcom,aoss-reset.txt        |  52 +++++++
 drivers/remoteproc/qcom_q6v5_pil.c            | 144 +++++++++++++++++-
 drivers/reset/Kconfig                         |   9 ++
 drivers/reset/Makefile                        |   1 +
 drivers/reset/reset-qcom-aoss.c               | 133 ++++++++++++++++
 include/dt-bindings/reset/qcom,sdm845-aoss.h  |  17 +++
 7 files changed, 352 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/reset/qcom,aoss-reset.txt
 create mode 100644 drivers/reset/reset-qcom-aoss.c
 create mode 100644 include/dt-bindings/reset/qcom,sdm845-aoss.h

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

[PATCH v4 1/5] dt-bindings: reset: Add AOSS reset bindings for SDM845 SoCs

[Sibi Sankar]

Add SDM845 AOSS (always on subsystem) reset controller binding

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

diff --git a/Documentation/devicetree/bindings/reset/qcom,aoss-reset.txt b/Documentation/devicetree/bindings/reset/qcom,aoss-reset.txt
new file mode 100644
index 000000000000..e5201de9a314
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/qcom,aoss-reset.txt
@@ -0,0 +1,52 @@
+Qualcomm AOSS Reset Controller
+======================================
+
+This binding describes a reset-controller found on AOSS (always on subsystem)
+for Qualcomm SDM845 SoCs.
+
+Required properties:
+- compatible:
+	Usage: required
+	Value type: <string>
+	Definition: must be:
+		    "qcom,sdm845-aoss-reset"
+
+- 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:
+
+aoss_reset: qcom,reset-controller@b2e0100 {
+	compatible = "qcom,sdm845-aoss-reset";
+	reg = <0xc2b0000 0x21000>;
+	#reset-cells = <1>;
+};
+
+Specifying reset lines connected to IP modules
+==============================================
+
+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-aoss.h>
+
+Example:
+
+modem-pil@4080000 {
+	...
+
+	resets = <&aoss_reset AOSS_CC_MSS_RESTART>;
+	reset-names = "mss_restart";
+
+	...
+};
diff --git a/include/dt-bindings/reset/qcom,sdm845-aoss.h b/include/dt-bindings/reset/qcom,sdm845-aoss.h
new file mode 100644
index 000000000000..476c5fc873b6
--- /dev/null
+++ b/include/dt-bindings/reset/qcom,sdm845-aoss.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2018 The Linux Foundation. All rights reserved.
+ */
+
+#ifndef _DT_BINDINGS_RESET_AOSS_SDM_845_H
+#define _DT_BINDINGS_RESET_AOSS_SDM_845_H
+
+#define AOSS_CC_MSS_RESTART	0
+#define AOSS_CC_CAMSS_RESTART	1
+#define AOSS_CC_VENUS_RESTART	2
+#define AOSS_CC_GPU_RESTART	3
+#define AOSS_CC_DISPSS_RESTART	4
+#define AOSS_CC_WCSS_RESTART	5
+#define AOSS_CC_LPASS_RESTART	6
+
+#endif
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v4 1/5] dt-bindings: reset: Add AOSS reset bindings for SDM845 SoCs

[Philipp Zabel]

Hi Sibi,

On Wed, 2018-04-25 at 20:38 +0530, Sibi Sankar wrote:
> Add SDM845 AOSS (always on subsystem) reset controller binding
> 
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  .../bindings/reset/qcom,aoss-reset.txt        | 52 +++++++++++++++++++
>  include/dt-bindings/reset/qcom,sdm845-aoss.h  | 17 ++++++
>  2 files changed, 69 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/qcom,aoss-reset.txt
>  create mode 100644 include/dt-bindings/reset/qcom,sdm845-aoss.h
> 
> diff --git a/Documentation/devicetree/bindings/reset/qcom,aoss-reset.txt b/Documentation/devicetree/bindings/reset/qcom,aoss-reset.txt
> new file mode 100644
> index 000000000000..e5201de9a314
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/qcom,aoss-reset.txt
> @@ -0,0 +1,52 @@
> +Qualcomm AOSS Reset Controller
> +======================================
> +
> +This binding describes a reset-controller found on AOSS (always on subsystem)
> +for Qualcomm SDM845 SoCs.
> +
> +Required properties:
> +- compatible:
> +	Usage: required
> +	Value type: <string>
> +	Definition: must be:
> +		    "qcom,sdm845-aoss-reset"
> +
> +- 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:
> +
> +aoss_reset: qcom,reset-controller@b2e0100 {

The node name should be "reset-controller", not "qcom,reset-controller"

> +	compatible = "qcom,sdm845-aoss-reset";
> +	reg = <0xc2b0000 0x21000>;

The address here should match the address part of the node name above.

Apart from those two nitpicks, the binding looks good to me.

regards
Philipp

Re: [PATCH v4 1/5] dt-bindings: reset: Add AOSS reset bindings for SDM845 SoCs

[Sibi S]

Hi Philipp,

Thanks for the review

On 04/27/2018 03:54 PM, Philipp Zabel wrote:
> Hi Sibi,
> 
> On Wed, 2018-04-25 at 20:38 +0530, Sibi Sankar wrote:
>> Add SDM845 AOSS (always on subsystem) reset controller binding
>>
>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>> ---
>>   .../bindings/reset/qcom,aoss-reset.txt        | 52 +++++++++++++++++++
>>   include/dt-bindings/reset/qcom,sdm845-aoss.h  | 17 ++++++
>>   2 files changed, 69 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/reset/qcom,aoss-reset.txt
>>   create mode 100644 include/dt-bindings/reset/qcom,sdm845-aoss.h
>>
>> diff --git a/Documentation/devicetree/bindings/reset/qcom,aoss-reset.txt b/Documentation/devicetree/bindings/reset/qcom,aoss-reset.txt
>> new file mode 100644
>> index 000000000000..e5201de9a314
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/reset/qcom,aoss-reset.txt
>> @@ -0,0 +1,52 @@
>> +Qualcomm AOSS Reset Controller
>> +======================================
>> +
>> +This binding describes a reset-controller found on AOSS (always on subsystem)
>> +for Qualcomm SDM845 SoCs.
>> +
>> +Required properties:
>> +- compatible:
>> +	Usage: required
>> +	Value type: <string>
>> +	Definition: must be:
>> +		    "qcom,sdm845-aoss-reset"
>> +
>> +- 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:
>> +
>> +aoss_reset: qcom,reset-controller@b2e0100 {
> 
> The node name should be "reset-controller", not "qcom,reset-controller"
> 
>> +	compatible = "qcom,sdm845-aoss-reset";
>> +	reg = <0xc2b0000 0x21000>;
> 
> The address here should match the address part of the node name above.
> 

sure will change them

> Apart from those two nitpicks, the binding looks good to me.
>
> regards
> Philipp
> 

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

[PATCH v4 2/5] reset: qcom: AOSS (always on subsystem) reset controller

[Sibi Sankar]

Add reset controller driver for Qualcomm SDM845 SoC to
control reset signals provided by AOSS for Modem, Venus
ADSP, GPU, Camera, Wireless, Display subsystem

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

diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index c0b292be1b72..756ad2b27d0f 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -82,6 +82,15 @@ config RESET_PISTACHIO
 	help
 	  This enables the reset driver for ImgTec Pistachio SoCs.
 
+config RESET_QCOM_AOSS
+	bool "Qcom AOSS Reset Driver"
+	depends on ARCH_QCOM || COMPILE_TEST
+	help
+	  This enables the AOSS (always on subsystem) reset driver
+	  for Qualcomm SDM845 SoCs. Say Y if you want to control
+	  reset signals provided by AOSS for Modem, Venus, ADSP,
+	  GPU, Camera, Wireless, Display subsystem. Otherwise, say N.
+
 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 c1261dcfe9ad..6881e4d287f0 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_RESET_LPC18XX) += reset-lpc18xx.o
 obj-$(CONFIG_RESET_MESON) += reset-meson.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_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-aoss.c b/drivers/reset/reset-qcom-aoss.c
new file mode 100644
index 000000000000..3b0bbb387f7b
--- /dev/null
+++ b/drivers/reset/reset-qcom-aoss.c
@@ -0,0 +1,133 @@
+// 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/delay.h>
+#include <linux/io.h>
+#include <linux/of_device.h>
+#include <dt-bindings/reset/qcom,sdm845-aoss.h>
+
+struct qcom_aoss_reset_map {
+	unsigned int reg;
+};
+
+struct qcom_aoss_desc {
+	const struct qcom_aoss_reset_map *resets;
+	size_t num_resets;
+};
+
+struct qcom_aoss_reset_data {
+	struct reset_controller_dev rcdev;
+	void __iomem *base;
+	const struct qcom_aoss_desc *desc;
+};
+
+static const struct qcom_aoss_reset_map sdm845_aoss_resets[] = {
+	[AOSS_CC_MSS_RESTART] = {0x0},
+	[AOSS_CC_CAMSS_RESTART] = {0x1000},
+	[AOSS_CC_VENUS_RESTART] = {0x2000},
+	[AOSS_CC_GPU_RESTART] = {0x3000},
+	[AOSS_CC_DISPSS_RESTART] = {0x4000},
+	[AOSS_CC_WCSS_RESTART] = {0x10000},
+	[AOSS_CC_LPASS_RESTART] = {0x20000},
+};
+
+static const struct qcom_aoss_desc sdm845_aoss_desc = {
+	.resets = sdm845_aoss_resets,
+	.num_resets = ARRAY_SIZE(sdm845_aoss_resets),
+};
+
+static inline struct qcom_aoss_reset_data *to_qcom_aoss_reset_data(
+				struct reset_controller_dev *rcdev)
+{
+	return container_of(rcdev, struct qcom_aoss_reset_data, rcdev);
+}
+
+static int qcom_aoss_control_assert(struct reset_controller_dev *rcdev,
+				    unsigned long idx)
+{
+	struct qcom_aoss_reset_data *data = to_qcom_aoss_reset_data(rcdev);
+	const struct qcom_aoss_reset_map *map = &data->desc->resets[idx];
+
+	writel(1, data->base + map->reg);
+	/* Wait 6 32kHz sleep cycles for reset */
+	usleep_range(200, 210);
+	return 0;
+}
+
+static int qcom_aoss_control_deassert(struct reset_controller_dev *rcdev,
+				      unsigned long idx)
+{
+	struct qcom_aoss_reset_data *data = to_qcom_aoss_reset_data(rcdev);
+	const struct qcom_aoss_reset_map *map = &data->desc->resets[idx];
+
+	writel(0, data->base + map->reg);
+	/* Wait 6 32kHz sleep cycles for reset */
+	usleep_range(200, 210);
+	return 0;
+}
+
+static int qcom_aoss_control_reset(struct reset_controller_dev *rcdev,
+					unsigned long idx)
+{
+	qcom_aoss_control_assert(rcdev, idx);
+
+	return qcom_aoss_control_deassert(rcdev, idx);
+}
+
+static const struct reset_control_ops qcom_aoss_reset_ops = {
+	.reset = qcom_aoss_control_reset,
+	.assert = qcom_aoss_control_assert,
+	.deassert = qcom_aoss_control_deassert,
+};
+
+static int qcom_aoss_reset_probe(struct platform_device *pdev)
+{
+	struct qcom_aoss_reset_data *data;
+	struct device *dev = &pdev->dev;
+	const struct qcom_aoss_desc *desc;
+	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);
+	data->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(data->base))
+		return PTR_ERR(data->base);
+
+	data->rcdev.owner = THIS_MODULE;
+	data->rcdev.ops = &qcom_aoss_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_aoss_reset_of_match[] = {
+	{ .compatible = "qcom,sdm845-aoss-reset", .data = &sdm845_aoss_desc },
+	{}
+};
+
+static struct platform_driver qcom_aoss_reset_driver = {
+	.probe = qcom_aoss_reset_probe,
+	.driver  = {
+		.name = "qcom_aoss_reset",
+		.of_match_table = qcom_aoss_reset_of_match,
+	},
+};
+
+builtin_platform_driver(qcom_aoss_reset_driver);
+
+MODULE_DESCRIPTION("Qualcomm AOSS Reset Driver");
+MODULE_LICENSE("GPL v2");
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v4 2/5] reset: qcom: AOSS (always on subsystem) reset controller

[Philipp Zabel]

On Wed, 2018-04-25 at 20:38 +0530, Sibi Sankar wrote:
> Add reset controller driver for Qualcomm SDM845 SoC to
> control reset signals provided by AOSS for Modem, Venus
> ADSP, GPU, Camera, Wireless, Display subsystem
> 
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  drivers/reset/Kconfig           |   9 +++
>  drivers/reset/Makefile          |   1 +
>  drivers/reset/reset-qcom-aoss.c | 133 ++++++++++++++++++++++++++++++++
>  3 files changed, 143 insertions(+)
>  create mode 100644 drivers/reset/reset-qcom-aoss.c
> 
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index c0b292be1b72..756ad2b27d0f 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -82,6 +82,15 @@ config RESET_PISTACHIO
>  	help
>  	  This enables the reset driver for ImgTec Pistachio SoCs.
>  
> +config RESET_QCOM_AOSS
> +	bool "Qcom AOSS Reset Driver"
> +	depends on ARCH_QCOM || COMPILE_TEST
> +	help
> +	  This enables the AOSS (always on subsystem) reset driver
> +	  for Qualcomm SDM845 SoCs. Say Y if you want to control
> +	  reset signals provided by AOSS for Modem, Venus, ADSP,
> +	  GPU, Camera, Wireless, Display subsystem. Otherwise, say N.
> +
>  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 c1261dcfe9ad..6881e4d287f0 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_RESET_LPC18XX) += reset-lpc18xx.o
>  obj-$(CONFIG_RESET_MESON) += reset-meson.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_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-aoss.c b/drivers/reset/reset-qcom-aoss.c
> new file mode 100644
> index 000000000000..3b0bbb387f7b
> --- /dev/null
> +++ b/drivers/reset/reset-qcom-aoss.c
> @@ -0,0 +1,133 @@
> +// 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/delay.h>
> +#include <linux/io.h>
> +#include <linux/of_device.h>
> +#include <dt-bindings/reset/qcom,sdm845-aoss.h>
> +
> +struct qcom_aoss_reset_map {
> +	unsigned int reg;
> +};
> +
> +struct qcom_aoss_desc {
> +	const struct qcom_aoss_reset_map *resets;
> +	size_t num_resets;
> +};
> +
> +struct qcom_aoss_reset_data {
> +	struct reset_controller_dev rcdev;
> +	void __iomem *base;
> +	const struct qcom_aoss_desc *desc;
> +};
> +
> +static const struct qcom_aoss_reset_map sdm845_aoss_resets[] = {
> +	[AOSS_CC_MSS_RESTART] = {0x0},
> +	[AOSS_CC_CAMSS_RESTART] = {0x1000},
> +	[AOSS_CC_VENUS_RESTART] = {0x2000},
> +	[AOSS_CC_GPU_RESTART] = {0x3000},
> +	[AOSS_CC_DISPSS_RESTART] = {0x4000},
> +	[AOSS_CC_WCSS_RESTART] = {0x10000},
> +	[AOSS_CC_LPASS_RESTART] = {0x20000},
> +};
> +
> +static const struct qcom_aoss_desc sdm845_aoss_desc = {
> +	.resets = sdm845_aoss_resets,
> +	.num_resets = ARRAY_SIZE(sdm845_aoss_resets),
> +};
> +
> +static inline struct qcom_aoss_reset_data *to_qcom_aoss_reset_data(
> +				struct reset_controller_dev *rcdev)
> +{
> +	return container_of(rcdev, struct qcom_aoss_reset_data, rcdev);
> +}
> +
> +static int qcom_aoss_control_assert(struct reset_controller_dev *rcdev,
> +				    unsigned long idx)
> +{
> +	struct qcom_aoss_reset_data *data = to_qcom_aoss_reset_data(rcdev);
> +	const struct qcom_aoss_reset_map *map = &data->desc->resets[idx];
> +
> +	writel(1, data->base + map->reg);
> +	/* Wait 6 32kHz sleep cycles for reset */
> +	usleep_range(200, 210);
> +	return 0;
> +}
> +
> +static int qcom_aoss_control_deassert(struct reset_controller_dev *rcdev,
> +				      unsigned long idx)
> +{
> +	struct qcom_aoss_reset_data *data = to_qcom_aoss_reset_data(rcdev);
> +	const struct qcom_aoss_reset_map *map = &data->desc->resets[idx];
> +
> +	writel(0, data->base + map->reg);
> +	/* Wait 6 32kHz sleep cycles for reset */
> +	usleep_range(200, 210);

6 32 kHz cycles are about 188 µs (184 µs for 32.768 kHz).
Just out of curiosity, is the minimum increased to 200 µs on purpose, or
to have a nice round number? The maximum seems oddly small, unless it is
essential to wait less than 7 cycles.

The driver looks good to me now. I plan to apply patches 1 and 2 with
Rob's ack.
Is it ok to merge them independently from the remoteproc driver, or is
there a dependency?

regards
Philipp

Re: [PATCH v4 2/5] reset: qcom: AOSS (always on subsystem) reset controller

[Sibi S]

Hi Philipp,

Thanks for the review.

On 04/27/2018 04:11 PM, Philipp Zabel wrote:
> On Wed, 2018-04-25 at 20:38 +0530, Sibi Sankar wrote:
>> Add reset controller driver for Qualcomm SDM845 SoC to
>> control reset signals provided by AOSS for Modem, Venus
>> ADSP, GPU, Camera, Wireless, Display subsystem
>>
>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>> ---
>>   drivers/reset/Kconfig           |   9 +++
>>   drivers/reset/Makefile          |   1 +
>>   drivers/reset/reset-qcom-aoss.c | 133 ++++++++++++++++++++++++++++++++
>>   3 files changed, 143 insertions(+)
>>   create mode 100644 drivers/reset/reset-qcom-aoss.c
>>
>> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
>> index c0b292be1b72..756ad2b27d0f 100644
>> --- a/drivers/reset/Kconfig
>> +++ b/drivers/reset/Kconfig
>> @@ -82,6 +82,15 @@ config RESET_PISTACHIO
>>   	help
>>   	  This enables the reset driver for ImgTec Pistachio SoCs.
>>   
>> +config RESET_QCOM_AOSS
>> +	bool "Qcom AOSS Reset Driver"
>> +	depends on ARCH_QCOM || COMPILE_TEST
>> +	help
>> +	  This enables the AOSS (always on subsystem) reset driver
>> +	  for Qualcomm SDM845 SoCs. Say Y if you want to control
>> +	  reset signals provided by AOSS for Modem, Venus, ADSP,
>> +	  GPU, Camera, Wireless, Display subsystem. Otherwise, say N.
>> +
>>   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 c1261dcfe9ad..6881e4d287f0 100644
>> --- a/drivers/reset/Makefile
>> +++ b/drivers/reset/Makefile
>> @@ -14,6 +14,7 @@ obj-$(CONFIG_RESET_LPC18XX) += reset-lpc18xx.o
>>   obj-$(CONFIG_RESET_MESON) += reset-meson.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_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-aoss.c b/drivers/reset/reset-qcom-aoss.c
>> new file mode 100644
>> index 000000000000..3b0bbb387f7b
>> --- /dev/null
>> +++ b/drivers/reset/reset-qcom-aoss.c
>> @@ -0,0 +1,133 @@
>> +// 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/delay.h>
>> +#include <linux/io.h>
>> +#include <linux/of_device.h>
>> +#include <dt-bindings/reset/qcom,sdm845-aoss.h>
>> +
>> +struct qcom_aoss_reset_map {
>> +	unsigned int reg;
>> +};
>> +
>> +struct qcom_aoss_desc {
>> +	const struct qcom_aoss_reset_map *resets;
>> +	size_t num_resets;
>> +};
>> +
>> +struct qcom_aoss_reset_data {
>> +	struct reset_controller_dev rcdev;
>> +	void __iomem *base;
>> +	const struct qcom_aoss_desc *desc;
>> +};
>> +
>> +static const struct qcom_aoss_reset_map sdm845_aoss_resets[] = {
>> +	[AOSS_CC_MSS_RESTART] = {0x0},
>> +	[AOSS_CC_CAMSS_RESTART] = {0x1000},
>> +	[AOSS_CC_VENUS_RESTART] = {0x2000},
>> +	[AOSS_CC_GPU_RESTART] = {0x3000},
>> +	[AOSS_CC_DISPSS_RESTART] = {0x4000},
>> +	[AOSS_CC_WCSS_RESTART] = {0x10000},
>> +	[AOSS_CC_LPASS_RESTART] = {0x20000},
>> +};
>> +
>> +static const struct qcom_aoss_desc sdm845_aoss_desc = {
>> +	.resets = sdm845_aoss_resets,
>> +	.num_resets = ARRAY_SIZE(sdm845_aoss_resets),
>> +};
>> +
>> +static inline struct qcom_aoss_reset_data *to_qcom_aoss_reset_data(
>> +				struct reset_controller_dev *rcdev)
>> +{
>> +	return container_of(rcdev, struct qcom_aoss_reset_data, rcdev);
>> +}
>> +
>> +static int qcom_aoss_control_assert(struct reset_controller_dev *rcdev,
>> +				    unsigned long idx)
>> +{
>> +	struct qcom_aoss_reset_data *data = to_qcom_aoss_reset_data(rcdev);
>> +	const struct qcom_aoss_reset_map *map = &data->desc->resets[idx];
>> +
>> +	writel(1, data->base + map->reg);
>> +	/* Wait 6 32kHz sleep cycles for reset */
>> +	usleep_range(200, 210);
>> +	return 0;
>> +}
>> +
>> +static int qcom_aoss_control_deassert(struct reset_controller_dev *rcdev,
>> +				      unsigned long idx)
>> +{
>> +	struct qcom_aoss_reset_data *data = to_qcom_aoss_reset_data(rcdev);
>> +	const struct qcom_aoss_reset_map *map = &data->desc->resets[idx];
>> +
>> +	writel(0, data->base + map->reg);
>> +	/* Wait 6 32kHz sleep cycles for reset */
>> +	usleep_range(200, 210);
> 
> 6 32 kHz cycles are about 188 µs (184 µs for 32.768 kHz).
> Just out of curiosity, is the minimum increased to 200 µs on purpose, or
> to have a nice round number? The maximum seems oddly small, unless it is
> essential to wait less than 7 cycles.
> 

anything above 188 µs should be fine 200 µs is just a round number.

no it is not essential to wait less than 7 cycles so I can increase
the max limit to 300 µs.

> The driver looks good to me now. I plan to apply patches 1 and 2 with
> Rob's ack.
> Is it ok to merge them independently from the remoteproc driver, or is
> there a dependency?
>

Yes it should be fine to merge them independently. I can add a dt entry
to the dtsi and separate the 2 patches from the remoteproc patches if
that helps ?

> regards
> Philipp
> 

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

Re: [PATCH v4 2/5] reset: qcom: AOSS (always on subsystem) reset controller

[Philipp Zabel]

Hi Sibi,

On Fri, 2018-04-27 at 16:45 +0530, Sibi S wrote:
[...]
> > > +	/* Wait 6 32kHz sleep cycles for reset */
> > > +	usleep_range(200, 210);
> > 
> > 6 32 kHz cycles are about 188 µs (184 µs for 32.768 kHz).
> > Just out of curiosity, is the minimum increased to 200 µs on purpose, or
> > to have a nice round number? The maximum seems oddly small, unless it is
> > essential to wait less than 7 cycles.
> 
> anything above 188 µs should be fine 200 µs is just a round number.
> 
> no it is not essential to wait less than 7 cycles so I can increase
> the max limit to 300 µs.

Ok, thanks for the info.

> > The driver looks good to me now. I plan to apply patches 1 and 2 with
> > Rob's ack.
> > Is it ok to merge them independently from the remoteproc driver, or is
> > there a dependency?
> 
> Yes it should be fine to merge them independently. I can add a dt entry
> to the dtsi and separate the 2 patches from the remoteproc patches if
> that helps ?

I plan to just pick up the reset DT binding documentation patch (1/5)
and the reset driver patch (2/5) then, whether they are sent separately
or as part of the larger remoteproc series does not matter to me.

The dtsi patch would have to go through the arm/qualcomm tree.
I can put the reset patches onto a separate branch that could be pulled
into the qcom tree ahead of the dtsi change, or you could send the dtsi
change with numbers at first, and only replace them with the header
defines after patch 1 is merged through the reset tree.

regards
Philipp

[PATCH v4 3/5] dt-bindings: remoteproc: Add Q6v5 Modem PIL binding for SDM845

[Sibi Sankar]

Add new compatible string for Qualcomm SDM845 SoCs

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

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
index 00d3d58a102f..d90182425450 100644
--- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
@@ -11,6 +11,7 @@ on the Qualcomm Hexagon core.
 		    "qcom,msm8916-mss-pil",
 		    "qcom,msm8974-mss-pil"
 		    "qcom,msm8996-mss-pil"
+		    "qcom,sdm845-mss-pil"
 
 - reg:
 	Usage: required
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v4 3/5] dt-bindings: remoteproc: Add Q6v5 Modem PIL binding for SDM845

[Rob Herring]

On Wed, Apr 25, 2018 at 08:38:41PM +0530, Sibi Sankar wrote:
> Add new compatible string for Qualcomm SDM845 SoCs
> 
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt | 1 +
>  1 file changed, 1 insertion(+)

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

[PATCH v4 4/5] remoteproc: qcom: Add support for mss remoteproc on SDM845

[Sibi Sankar]

>From SDM845, the Q6SS reset sequence on software side has been
simplified with the introduction of boot FSM which assists in
bringing the Q6 out of reset

Add GLINK subdevice to allow definition of GLINK edge as a
child of modem-pil

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

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index 7e2d04d4f2f0..4d9504e8bf8e 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -57,6 +57,8 @@
 #define RMB_PMI_META_DATA_REG		0x10
 #define RMB_PMI_CODE_START_REG		0x14
 #define RMB_PMI_CODE_LENGTH_REG		0x18
+#define RMB_MBA_MSS_STATUS		0x40
+#define RMB_MBA_ALT_RESET		0x44
 
 #define RMB_CMD_META_DATA_READY		0x1
 #define RMB_CMD_LOAD_READY		0x2
@@ -104,6 +106,13 @@
 #define QDSP6SS_XO_CBCR		0x0038
 #define QDSP6SS_ACC_OVERRIDE_VAL		0x20
 
+/* QDSP6v65 parameters */
+#define QDSP6SS_SLEEP                   0x3C
+#define QDSP6SS_BOOT_CORE_START         0x400
+#define QDSP6SS_BOOT_CMD                0x404
+#define SLEEP_CHECK_MAX_LOOPS           200
+#define BOOT_FSM_TIMEOUT                10000
+
 struct reg_info {
 	struct regulator *reg;
 	int uV;
@@ -170,6 +179,7 @@ struct q6v5 {
 	void *mpss_region;
 	size_t mpss_size;
 
+	struct qcom_rproc_glink glink_subdev;
 	struct qcom_rproc_subdev smd_subdev;
 	struct qcom_rproc_ssr ssr_subdev;
 	struct qcom_sysmon *sysmon;
@@ -184,6 +194,7 @@ enum {
 	MSS_MSM8916,
 	MSS_MSM8974,
 	MSS_MSM8996,
+	MSS_SDM845,
 };
 
 static int q6v5_regulator_init(struct device *dev, struct reg_info *regs,
@@ -390,8 +401,35 @@ static int q6v5proc_reset(struct q6v5 *qproc)
 	int ret;
 	int i;
 
+	if (qproc->version == MSS_SDM845) {
 
-	if (qproc->version == MSS_MSM8996) {
+		val = readl(qproc->reg_base + QDSP6SS_SLEEP);
+		val |= 0x1;
+		writel(val, qproc->reg_base + QDSP6SS_SLEEP);
+
+		ret = readl_poll_timeout(qproc->reg_base + QDSP6SS_SLEEP,
+					 val, !(val & BIT(31)), 1,
+					 SLEEP_CHECK_MAX_LOOPS);
+		if (ret) {
+			dev_err(qproc->dev, "QDSP6SS Sleep clock timed out\n");
+			return -ETIMEDOUT;
+		}
+
+		/* De-assert QDSP6 stop core */
+		writel(1, qproc->reg_base + QDSP6SS_BOOT_CORE_START);
+		/* Trigger boot FSM */
+		writel(1, qproc->reg_base + QDSP6SS_BOOT_CMD);
+
+		ret = readl_poll_timeout(qproc->rmb_base + RMB_MBA_MSS_STATUS,
+				val, (val & BIT(0)) != 0, 10, BOOT_FSM_TIMEOUT);
+		if (ret) {
+			dev_err(qproc->dev, "Boot FSM failed to complete.\n");
+			return ret;
+		}
+
+		goto pbl_wait;
+
+	} else if (qproc->version == MSS_MSM8996) {
 		/* Override the ACC value if required */
 		writel(QDSP6SS_ACC_OVERRIDE_VAL,
 		       qproc->reg_base + QDSP6SS_STRAP_ACC);
@@ -499,6 +537,7 @@ static int q6v5proc_reset(struct q6v5 *qproc)
 	val &= ~Q6SS_STOP_CORE;
 	writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
 
+pbl_wait:
 	/* Wait for PBL status */
 	ret = q6v5_rmb_pbl_wait(qproc, 1000);
 	if (ret == -ETIMEDOUT) {
@@ -1256,6 +1295,7 @@ static int q6v5_probe(struct platform_device *pdev)
 	}
 	qproc->mpss_perm = BIT(QCOM_SCM_VMID_HLOS);
 	qproc->mba_perm = BIT(QCOM_SCM_VMID_HLOS);
+	qcom_add_glink_subdev(rproc, &qproc->glink_subdev);
 	qcom_add_smd_subdev(rproc, &qproc->smd_subdev);
 	qcom_add_ssr_subdev(rproc, &qproc->ssr_subdev, "mpss");
 	qproc->sysmon = qcom_add_sysmon_subdev(rproc, "modem", 0x12);
@@ -1279,6 +1319,7 @@ static int q6v5_remove(struct platform_device *pdev)
 	rproc_del(qproc->rproc);
 
 	qcom_remove_sysmon_subdev(qproc->sysmon);
+	qcom_remove_glink_subdev(qproc->rproc, &qproc->glink_subdev);
 	qcom_remove_smd_subdev(qproc->rproc, &qproc->smd_subdev);
 	qcom_remove_ssr_subdev(qproc->rproc, &qproc->ssr_subdev);
 	rproc_free(qproc->rproc);
@@ -1286,6 +1327,27 @@ static int q6v5_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct rproc_hexagon_res sdm845_mss = {
+	.hexagon_mba_image = "mba.mbn",
+	.proxy_clk_names = (char*[]){
+			"xo",
+			"axis2",
+			"prng",
+			NULL
+	},
+	.active_clk_names = (char*[]){
+			"iface",
+			"bus",
+			"mem",
+			"gpll0_mss",
+			"snoc_axi",
+			"mnoc_axi",
+			NULL
+	},
+	.need_mem_protection = true,
+	.version = MSS_SDM845,
+};
+
 static const struct rproc_hexagon_res msm8996_mss = {
 	.hexagon_mba_image = "mba.mbn",
 	.proxy_clk_names = (char*[]){
@@ -1379,6 +1441,7 @@ static const struct of_device_id q6v5_of_match[] = {
 	{ .compatible = "qcom,msm8916-mss-pil", .data = &msm8916_mss},
 	{ .compatible = "qcom,msm8974-mss-pil", .data = &msm8974_mss},
 	{ .compatible = "qcom,msm8996-mss-pil", .data = &msm8996_mss},
+	{ .compatible = "qcom,sdm845-mss-pil", .data = &sdm845_mss},
 	{ },
 };
 MODULE_DEVICE_TABLE(of, q6v5_of_match);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v4 4/5] remoteproc: qcom: Add support for mss remoteproc on SDM845

[Bjorn Andersson]

On Wed 25 Apr 08:08 PDT 2018, Sibi Sankar wrote:

> From SDM845, the Q6SS reset sequence on software side has been
> simplified with the introduction of boot FSM which assists in
> bringing the Q6 out of reset
> 
> Add GLINK subdevice to allow definition of GLINK edge as a
> child of modem-pil
> 

Please split this in two patches; one adding sdm845 and one adding the
glink subdev. You can squash in the addition of the compatible in the dt
binding into the sdm845 code patch, you wish as well.

Apart from that this looks good!

Regards,
Bjorn

> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_q6v5_pil.c | 65 +++++++++++++++++++++++++++++-
>  1 file changed, 64 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
> index 7e2d04d4f2f0..4d9504e8bf8e 100644
> --- a/drivers/remoteproc/qcom_q6v5_pil.c
> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> @@ -57,6 +57,8 @@
>  #define RMB_PMI_META_DATA_REG		0x10
>  #define RMB_PMI_CODE_START_REG		0x14
>  #define RMB_PMI_CODE_LENGTH_REG		0x18
> +#define RMB_MBA_MSS_STATUS		0x40
> +#define RMB_MBA_ALT_RESET		0x44
>  
>  #define RMB_CMD_META_DATA_READY		0x1
>  #define RMB_CMD_LOAD_READY		0x2
> @@ -104,6 +106,13 @@
>  #define QDSP6SS_XO_CBCR		0x0038
>  #define QDSP6SS_ACC_OVERRIDE_VAL		0x20
>  
> +/* QDSP6v65 parameters */
> +#define QDSP6SS_SLEEP                   0x3C
> +#define QDSP6SS_BOOT_CORE_START         0x400
> +#define QDSP6SS_BOOT_CMD                0x404
> +#define SLEEP_CHECK_MAX_LOOPS           200
> +#define BOOT_FSM_TIMEOUT                10000
> +
>  struct reg_info {
>  	struct regulator *reg;
>  	int uV;
> @@ -170,6 +179,7 @@ struct q6v5 {
>  	void *mpss_region;
>  	size_t mpss_size;
>  
> +	struct qcom_rproc_glink glink_subdev;
>  	struct qcom_rproc_subdev smd_subdev;
>  	struct qcom_rproc_ssr ssr_subdev;
>  	struct qcom_sysmon *sysmon;
> @@ -184,6 +194,7 @@ enum {
>  	MSS_MSM8916,
>  	MSS_MSM8974,
>  	MSS_MSM8996,
> +	MSS_SDM845,
>  };
>  
>  static int q6v5_regulator_init(struct device *dev, struct reg_info *regs,
> @@ -390,8 +401,35 @@ static int q6v5proc_reset(struct q6v5 *qproc)
>  	int ret;
>  	int i;
>  
> +	if (qproc->version == MSS_SDM845) {
>  
> -	if (qproc->version == MSS_MSM8996) {
> +		val = readl(qproc->reg_base + QDSP6SS_SLEEP);
> +		val |= 0x1;
> +		writel(val, qproc->reg_base + QDSP6SS_SLEEP);
> +
> +		ret = readl_poll_timeout(qproc->reg_base + QDSP6SS_SLEEP,
> +					 val, !(val & BIT(31)), 1,
> +					 SLEEP_CHECK_MAX_LOOPS);
> +		if (ret) {
> +			dev_err(qproc->dev, "QDSP6SS Sleep clock timed out\n");
> +			return -ETIMEDOUT;
> +		}
> +
> +		/* De-assert QDSP6 stop core */
> +		writel(1, qproc->reg_base + QDSP6SS_BOOT_CORE_START);
> +		/* Trigger boot FSM */
> +		writel(1, qproc->reg_base + QDSP6SS_BOOT_CMD);
> +
> +		ret = readl_poll_timeout(qproc->rmb_base + RMB_MBA_MSS_STATUS,
> +				val, (val & BIT(0)) != 0, 10, BOOT_FSM_TIMEOUT);
> +		if (ret) {
> +			dev_err(qproc->dev, "Boot FSM failed to complete.\n");
> +			return ret;
> +		}
> +
> +		goto pbl_wait;
> +
> +	} else if (qproc->version == MSS_MSM8996) {
>  		/* Override the ACC value if required */
>  		writel(QDSP6SS_ACC_OVERRIDE_VAL,
>  		       qproc->reg_base + QDSP6SS_STRAP_ACC);
> @@ -499,6 +537,7 @@ static int q6v5proc_reset(struct q6v5 *qproc)
>  	val &= ~Q6SS_STOP_CORE;
>  	writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
>  
> +pbl_wait:
>  	/* Wait for PBL status */
>  	ret = q6v5_rmb_pbl_wait(qproc, 1000);
>  	if (ret == -ETIMEDOUT) {
> @@ -1256,6 +1295,7 @@ static int q6v5_probe(struct platform_device *pdev)
>  	}
>  	qproc->mpss_perm = BIT(QCOM_SCM_VMID_HLOS);
>  	qproc->mba_perm = BIT(QCOM_SCM_VMID_HLOS);
> +	qcom_add_glink_subdev(rproc, &qproc->glink_subdev);
>  	qcom_add_smd_subdev(rproc, &qproc->smd_subdev);
>  	qcom_add_ssr_subdev(rproc, &qproc->ssr_subdev, "mpss");
>  	qproc->sysmon = qcom_add_sysmon_subdev(rproc, "modem", 0x12);
> @@ -1279,6 +1319,7 @@ static int q6v5_remove(struct platform_device *pdev)
>  	rproc_del(qproc->rproc);
>  
>  	qcom_remove_sysmon_subdev(qproc->sysmon);
> +	qcom_remove_glink_subdev(qproc->rproc, &qproc->glink_subdev);
>  	qcom_remove_smd_subdev(qproc->rproc, &qproc->smd_subdev);
>  	qcom_remove_ssr_subdev(qproc->rproc, &qproc->ssr_subdev);
>  	rproc_free(qproc->rproc);
> @@ -1286,6 +1327,27 @@ static int q6v5_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static const struct rproc_hexagon_res sdm845_mss = {
> +	.hexagon_mba_image = "mba.mbn",
> +	.proxy_clk_names = (char*[]){
> +			"xo",
> +			"axis2",
> +			"prng",
> +			NULL
> +	},
> +	.active_clk_names = (char*[]){
> +			"iface",
> +			"bus",
> +			"mem",
> +			"gpll0_mss",
> +			"snoc_axi",
> +			"mnoc_axi",
> +			NULL
> +	},
> +	.need_mem_protection = true,
> +	.version = MSS_SDM845,
> +};
> +
>  static const struct rproc_hexagon_res msm8996_mss = {
>  	.hexagon_mba_image = "mba.mbn",
>  	.proxy_clk_names = (char*[]){
> @@ -1379,6 +1441,7 @@ static const struct of_device_id q6v5_of_match[] = {
>  	{ .compatible = "qcom,msm8916-mss-pil", .data = &msm8916_mss},
>  	{ .compatible = "qcom,msm8974-mss-pil", .data = &msm8974_mss},
>  	{ .compatible = "qcom,msm8996-mss-pil", .data = &msm8996_mss},
> +	{ .compatible = "qcom,sdm845-mss-pil", .data = &sdm845_mss},
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(of, q6v5_of_match);
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH v4 4/5] remoteproc: qcom: Add support for mss remoteproc on SDM845

[Sibi S]

Hi Bjorn,
Thanks for the review.

On 05/19/2018 03:01 AM, Bjorn Andersson wrote:
> On Wed 25 Apr 08:08 PDT 2018, Sibi Sankar wrote:
> 
>>  From SDM845, the Q6SS reset sequence on software side has been
>> simplified with the introduction of boot FSM which assists in
>> bringing the Q6 out of reset
>>
>> Add GLINK subdevice to allow definition of GLINK edge as a
>> child of modem-pil
>>
> 
> Please split this in two patches; one adding sdm845 and one adding the
> glink subdev. You can squash in the addition of the compatible in the dt
> binding into the sdm845 code patch, you wish as well.
> 

Will split it into two commits
Will still keep the dt-binding as a separate patch

> Apart from that this looks good!
> 
> Regards,
> Bjorn
> 
>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>> ---
>>   drivers/remoteproc/qcom_q6v5_pil.c | 65 +++++++++++++++++++++++++++++-
>>   1 file changed, 64 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
>> index 7e2d04d4f2f0..4d9504e8bf8e 100644
>> --- a/drivers/remoteproc/qcom_q6v5_pil.c
>> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
>> @@ -57,6 +57,8 @@
>>   #define RMB_PMI_META_DATA_REG		0x10
>>   #define RMB_PMI_CODE_START_REG		0x14
>>   #define RMB_PMI_CODE_LENGTH_REG		0x18
>> +#define RMB_MBA_MSS_STATUS		0x40
>> +#define RMB_MBA_ALT_RESET		0x44
>>   
>>   #define RMB_CMD_META_DATA_READY		0x1
>>   #define RMB_CMD_LOAD_READY		0x2
>> @@ -104,6 +106,13 @@
>>   #define QDSP6SS_XO_CBCR		0x0038
>>   #define QDSP6SS_ACC_OVERRIDE_VAL		0x20
>>   
>> +/* QDSP6v65 parameters */
>> +#define QDSP6SS_SLEEP                   0x3C
>> +#define QDSP6SS_BOOT_CORE_START         0x400
>> +#define QDSP6SS_BOOT_CMD                0x404
>> +#define SLEEP_CHECK_MAX_LOOPS           200
>> +#define BOOT_FSM_TIMEOUT                10000
>> +
>>   struct reg_info {
>>   	struct regulator *reg;
>>   	int uV;
>> @@ -170,6 +179,7 @@ struct q6v5 {
>>   	void *mpss_region;
>>   	size_t mpss_size;
>>   
>> +	struct qcom_rproc_glink glink_subdev;
>>   	struct qcom_rproc_subdev smd_subdev;
>>   	struct qcom_rproc_ssr ssr_subdev;
>>   	struct qcom_sysmon *sysmon;
>> @@ -184,6 +194,7 @@ enum {
>>   	MSS_MSM8916,
>>   	MSS_MSM8974,
>>   	MSS_MSM8996,
>> +	MSS_SDM845,
>>   };
>>   
>>   static int q6v5_regulator_init(struct device *dev, struct reg_info *regs,
>> @@ -390,8 +401,35 @@ static int q6v5proc_reset(struct q6v5 *qproc)
>>   	int ret;
>>   	int i;
>>   
>> +	if (qproc->version == MSS_SDM845) {
>>   
>> -	if (qproc->version == MSS_MSM8996) {
>> +		val = readl(qproc->reg_base + QDSP6SS_SLEEP);
>> +		val |= 0x1;
>> +		writel(val, qproc->reg_base + QDSP6SS_SLEEP);
>> +
>> +		ret = readl_poll_timeout(qproc->reg_base + QDSP6SS_SLEEP,
>> +					 val, !(val & BIT(31)), 1,
>> +					 SLEEP_CHECK_MAX_LOOPS);
>> +		if (ret) {
>> +			dev_err(qproc->dev, "QDSP6SS Sleep clock timed out\n");
>> +			return -ETIMEDOUT;
>> +		}
>> +
>> +		/* De-assert QDSP6 stop core */
>> +		writel(1, qproc->reg_base + QDSP6SS_BOOT_CORE_START);
>> +		/* Trigger boot FSM */
>> +		writel(1, qproc->reg_base + QDSP6SS_BOOT_CMD);
>> +
>> +		ret = readl_poll_timeout(qproc->rmb_base + RMB_MBA_MSS_STATUS,
>> +				val, (val & BIT(0)) != 0, 10, BOOT_FSM_TIMEOUT);
>> +		if (ret) {
>> +			dev_err(qproc->dev, "Boot FSM failed to complete.\n");
>> +			return ret;
>> +		}
>> +
>> +		goto pbl_wait;
>> +
>> +	} else if (qproc->version == MSS_MSM8996) {
>>   		/* Override the ACC value if required */
>>   		writel(QDSP6SS_ACC_OVERRIDE_VAL,
>>   		       qproc->reg_base + QDSP6SS_STRAP_ACC);
>> @@ -499,6 +537,7 @@ static int q6v5proc_reset(struct q6v5 *qproc)
>>   	val &= ~Q6SS_STOP_CORE;
>>   	writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
>>   
>> +pbl_wait:
>>   	/* Wait for PBL status */
>>   	ret = q6v5_rmb_pbl_wait(qproc, 1000);
>>   	if (ret == -ETIMEDOUT) {
>> @@ -1256,6 +1295,7 @@ static int q6v5_probe(struct platform_device *pdev)
>>   	}
>>   	qproc->mpss_perm = BIT(QCOM_SCM_VMID_HLOS);
>>   	qproc->mba_perm = BIT(QCOM_SCM_VMID_HLOS);
>> +	qcom_add_glink_subdev(rproc, &qproc->glink_subdev);
>>   	qcom_add_smd_subdev(rproc, &qproc->smd_subdev);
>>   	qcom_add_ssr_subdev(rproc, &qproc->ssr_subdev, "mpss");
>>   	qproc->sysmon = qcom_add_sysmon_subdev(rproc, "modem", 0x12);
>> @@ -1279,6 +1319,7 @@ static int q6v5_remove(struct platform_device *pdev)
>>   	rproc_del(qproc->rproc);
>>   
>>   	qcom_remove_sysmon_subdev(qproc->sysmon);
>> +	qcom_remove_glink_subdev(qproc->rproc, &qproc->glink_subdev);
>>   	qcom_remove_smd_subdev(qproc->rproc, &qproc->smd_subdev);
>>   	qcom_remove_ssr_subdev(qproc->rproc, &qproc->ssr_subdev);
>>   	rproc_free(qproc->rproc);
>> @@ -1286,6 +1327,27 @@ static int q6v5_remove(struct platform_device *pdev)
>>   	return 0;
>>   }
>>   
>> +static const struct rproc_hexagon_res sdm845_mss = {
>> +	.hexagon_mba_image = "mba.mbn",
>> +	.proxy_clk_names = (char*[]){
>> +			"xo",
>> +			"axis2",
>> +			"prng",
>> +			NULL
>> +	},
>> +	.active_clk_names = (char*[]){
>> +			"iface",
>> +			"bus",
>> +			"mem",
>> +			"gpll0_mss",
>> +			"snoc_axi",
>> +			"mnoc_axi",
>> +			NULL
>> +	},
>> +	.need_mem_protection = true,
>> +	.version = MSS_SDM845,
>> +};
>> +
>>   static const struct rproc_hexagon_res msm8996_mss = {
>>   	.hexagon_mba_image = "mba.mbn",
>>   	.proxy_clk_names = (char*[]){
>> @@ -1379,6 +1441,7 @@ static const struct of_device_id q6v5_of_match[] = {
>>   	{ .compatible = "qcom,msm8916-mss-pil", .data = &msm8916_mss},
>>   	{ .compatible = "qcom,msm8974-mss-pil", .data = &msm8974_mss},
>>   	{ .compatible = "qcom,msm8996-mss-pil", .data = &msm8996_mss},
>> +	{ .compatible = "qcom,sdm845-mss-pil", .data = &sdm845_mss},
>>   	{ },
>>   };
>>   MODULE_DEVICE_TABLE(of, q6v5_of_match);
>> -- 
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>
> 

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

[PATCH v4 5/5] remoteproc: qcom: Always assert and deassert reset signals in SDM845

[Sibi Sankar]

SDM845 brings a new reset signal ALT_RESET which is a part of the MSS
subsystem hence requires some of the active clks to be enabled before
assert/deassert

Reset the modem if the BOOT FSM does timeout

Reset assert/deassert sequence vary across SoCs adding reset, adding
start/stop helper functions to handle SoC specific reset sequences

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

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index 4d9504e8bf8e..99ef3f51c528 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -130,9 +130,11 @@ struct rproc_hexagon_res {
 	struct qcom_mss_reg_res *proxy_supply;
 	struct qcom_mss_reg_res *active_supply;
 	char **proxy_clk_names;
+	char **reset_clk_names;
 	char **active_clk_names;
 	int version;
 	bool need_mem_protection;
+	bool has_alt_reset;
 };
 
 struct q6v5 {
@@ -157,8 +159,10 @@ struct q6v5 {
 	unsigned int fatal_interrupt;
 
 	struct clk *active_clks[8];
+	struct clk *reset_clks[4];
 	struct clk *proxy_clks[4];
 	int active_clk_count;
+	int reset_clk_count;
 	int proxy_clk_count;
 
 	struct reg_info active_regs[1];
@@ -179,6 +183,9 @@ struct q6v5 {
 	void *mpss_region;
 	size_t mpss_size;
 
+	int (*reset_deassert)(struct q6v5 *qproc);
+	int (*reset_assert)(struct q6v5 *qproc);
+
 	struct qcom_rproc_glink glink_subdev;
 	struct qcom_rproc_subdev smd_subdev;
 	struct qcom_rproc_ssr ssr_subdev;
@@ -349,6 +356,32 @@ static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
 	return 0;
 }
 
+static int q6v5_reset_assert(struct q6v5 *qproc)
+{
+	return reset_control_assert(qproc->mss_restart);
+}
+
+static int q6v5_reset_deassert(struct q6v5 *qproc)
+{
+	return reset_control_deassert(qproc->mss_restart);
+}
+
+static int q6v5_alt_reset_assert(struct q6v5 *qproc)
+{
+	return reset_control_reset(qproc->mss_restart);
+}
+
+static int q6v5_alt_reset_deassert(struct q6v5 *qproc)
+{
+	/* Ensure alt reset is written before restart reg */
+	writel(1, qproc->rmb_base + RMB_MBA_ALT_RESET);
+
+	reset_control_reset(qproc->mss_restart);
+
+	writel(0, qproc->rmb_base + RMB_MBA_ALT_RESET);
+	return 0;
+}
+
 static int q6v5_rmb_pbl_wait(struct q6v5 *qproc, int ms)
 {
 	unsigned long timeout;
@@ -424,6 +457,8 @@ static int q6v5proc_reset(struct q6v5 *qproc)
 				val, (val & BIT(0)) != 0, 10, BOOT_FSM_TIMEOUT);
 		if (ret) {
 			dev_err(qproc->dev, "Boot FSM failed to complete.\n");
+			/* Reset the modem so that boot FSM is in reset state */
+			qproc->reset_deassert(qproc);
 			return ret;
 		}
 
@@ -792,12 +827,20 @@ static int q6v5_start(struct rproc *rproc)
 		dev_err(qproc->dev, "failed to enable supplies\n");
 		goto disable_proxy_clk;
 	}
-	ret = reset_control_deassert(qproc->mss_restart);
+
+	ret = q6v5_clk_enable(qproc->dev, qproc->reset_clks,
+			      qproc->reset_clk_count);
 	if (ret) {
-		dev_err(qproc->dev, "failed to deassert mss restart\n");
+		dev_err(qproc->dev, "failed to enable reset clocks\n");
 		goto disable_vdd;
 	}
 
+	ret = qproc->reset_deassert(qproc);
+	if (ret) {
+		dev_err(qproc->dev, "failed to deassert mss restart\n");
+		goto disable_reset_clks;
+	}
+
 	ret = q6v5_clk_enable(qproc->dev, qproc->active_clks,
 			      qproc->active_clk_count);
 	if (ret) {
@@ -888,7 +931,10 @@ static int q6v5_start(struct rproc *rproc)
 			 qproc->active_clk_count);
 
 assert_reset:
-	reset_control_assert(qproc->mss_restart);
+	qproc->reset_assert(qproc);
+disable_reset_clks:
+	q6v5_clk_disable(qproc->dev, qproc->reset_clks,
+			 qproc->reset_clk_count);
 disable_vdd:
 	q6v5_regulator_disable(qproc, qproc->active_regs,
 			       qproc->active_reg_count);
@@ -938,7 +984,7 @@ static int q6v5_stop(struct rproc *rproc)
 				      qproc->mpss_phys, qproc->mpss_size);
 	WARN_ON(ret);
 
-	reset_control_assert(qproc->mss_restart);
+	qproc->reset_assert(qproc);
 	disable_irq(qproc->handover_interrupt);
 	if (!qproc->unvoted_flag) {
 		q6v5_clk_disable(qproc->dev, qproc->proxy_clks,
@@ -949,6 +995,8 @@ static int q6v5_stop(struct rproc *rproc)
 	disable_irq(qproc->wdog_interrupt);
 	disable_irq(qproc->fatal_interrupt);
 
+	q6v5_clk_disable(qproc->dev, qproc->reset_clks,
+			 qproc->reset_clk_count);
 	q6v5_clk_disable(qproc->dev, qproc->active_clks,
 			 qproc->active_clk_count);
 	q6v5_regulator_disable(qproc, qproc->active_regs,
@@ -1211,6 +1259,14 @@ static int q6v5_probe(struct platform_device *pdev)
 	qproc->rproc = rproc;
 	platform_set_drvdata(pdev, qproc);
 
+	if (desc->has_alt_reset) {
+		qproc->reset_deassert = q6v5_alt_reset_deassert;
+		qproc->reset_assert = q6v5_alt_reset_assert;
+	} else {
+		qproc->reset_deassert = q6v5_reset_deassert;
+		qproc->reset_assert = q6v5_reset_assert;
+	}
+
 	init_completion(&qproc->start_done);
 	init_completion(&qproc->stop_done);
 
@@ -1230,6 +1286,14 @@ static int q6v5_probe(struct platform_device *pdev)
 	}
 	qproc->proxy_clk_count = ret;
 
+	ret = q6v5_init_clocks(&pdev->dev, qproc->reset_clks,
+			       desc->reset_clk_names);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to get reset clocks.\n");
+		goto free_rproc;
+	}
+	qproc->reset_clk_count = ret;
+
 	ret = q6v5_init_clocks(&pdev->dev, qproc->active_clks,
 			       desc->active_clk_names);
 	if (ret < 0) {
@@ -1335,8 +1399,11 @@ static const struct rproc_hexagon_res sdm845_mss = {
 			"prng",
 			NULL
 	},
-	.active_clk_names = (char*[]){
+	.reset_clk_names = (char*[]){
 			"iface",
+			NULL
+	},
+	.active_clk_names = (char*[]){
 			"bus",
 			"mem",
 			"gpll0_mss",
@@ -1345,6 +1412,7 @@ static const struct rproc_hexagon_res sdm845_mss = {
 			NULL
 	},
 	.need_mem_protection = true,
+	.has_alt_reset = true,
 	.version = MSS_SDM845,
 };
 
@@ -1363,6 +1431,7 @@ static const struct rproc_hexagon_res msm8996_mss = {
 			NULL
 	},
 	.need_mem_protection = true,
+	.has_alt_reset = false,
 	.version = MSS_MSM8996,
 };
 
@@ -1394,6 +1463,7 @@ static const struct rproc_hexagon_res msm8916_mss = {
 		NULL
 	},
 	.need_mem_protection = false,
+	.has_alt_reset = false,
 	.version = MSS_MSM8916,
 };
 
@@ -1433,6 +1503,7 @@ static const struct rproc_hexagon_res msm8974_mss = {
 		NULL
 	},
 	.need_mem_protection = false,
+	.has_alt_reset = false,
 	.version = MSS_MSM8974,
 };
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v4 5/5] remoteproc: qcom: Always assert and deassert reset signals in SDM845

[Bjorn Andersson]

On Wed 25 Apr 08:08 PDT 2018, Sibi Sankar wrote:

> SDM845 brings a new reset signal ALT_RESET which is a part of the MSS
> subsystem hence requires some of the active clks to be enabled before
> assert/deassert
> 
> Reset the modem if the BOOT FSM does timeout
> 
> Reset assert/deassert sequence vary across SoCs adding reset, adding
> start/stop helper functions to handle SoC specific reset sequences
> 
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_q6v5_pil.c | 81 ++++++++++++++++++++++++++++--
>  1 file changed, 76 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
[..]
> @@ -349,6 +356,32 @@ static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
>  	return 0;
>  }
>  
> +static int q6v5_reset_assert(struct q6v5 *qproc)
> +{
> +	return reset_control_assert(qproc->mss_restart);
> +}
> +
> +static int q6v5_reset_deassert(struct q6v5 *qproc)
> +{
> +	return reset_control_deassert(qproc->mss_restart);
> +}
> +
> +static int q6v5_alt_reset_assert(struct q6v5 *qproc)
> +{
> +	return reset_control_reset(qproc->mss_restart);
> +}
> +
> +static int q6v5_alt_reset_deassert(struct q6v5 *qproc)
> +{
> +	/* Ensure alt reset is written before restart reg */
> +	writel(1, qproc->rmb_base + RMB_MBA_ALT_RESET);
> +
> +	reset_control_reset(qproc->mss_restart);
> +
> +	writel(0, qproc->rmb_base + RMB_MBA_ALT_RESET);
> +	return 0;
> +}
> +

Rather than having these four functions and scattering jumps to some
function pointer in the code I think it will be shorter and cleaner to
just have the q6v5_reset_{asert,deassert}() functions and in there check
if has_alt_reset and take appropriate action.

>  static int q6v5_rmb_pbl_wait(struct q6v5 *qproc, int ms)
>  {
>  	unsigned long timeout;
> @@ -424,6 +457,8 @@ static int q6v5proc_reset(struct q6v5 *qproc)
>  				val, (val & BIT(0)) != 0, 10, BOOT_FSM_TIMEOUT);
>  		if (ret) {
>  			dev_err(qproc->dev, "Boot FSM failed to complete.\n");
> +			/* Reset the modem so that boot FSM is in reset state */
> +			qproc->reset_deassert(qproc);


A thing like this typically should go into it's own patch, to keep a
clear record of why it was changed, but as this is simply amending the
previous patch it indicates that that one wasn't complete.

So if you reorder the two patches you can just put this directly into
the sdm845 patch, making it "complete".

(This also means that I want to merge the handover vs ready interrupt
patch before that one, so please include it in the next revision of the
series).

>  			return ret;
>  		}
>  
> @@ -792,12 +827,20 @@ static int q6v5_start(struct rproc *rproc)
>  		dev_err(qproc->dev, "failed to enable supplies\n");
>  		goto disable_proxy_clk;
>  	}
> -	ret = reset_control_deassert(qproc->mss_restart);
> +
> +	ret = q6v5_clk_enable(qproc->dev, qproc->reset_clks,
> +			      qproc->reset_clk_count);

Remind me, why can't you always enable the active clock before
deasserting reset? That way we wouldn't have to split out the iface
clock handling to be just slightly longer than the active clocks.

>  	if (ret) {
> -		dev_err(qproc->dev, "failed to deassert mss restart\n");
> +		dev_err(qproc->dev, "failed to enable reset clocks\n");
>  		goto disable_vdd;
>  	}
>  
> +	ret = qproc->reset_deassert(qproc);
> +	if (ret) {
> +		dev_err(qproc->dev, "failed to deassert mss restart\n");
> +		goto disable_reset_clks;
> +	}
> +
[..]
> @@ -1335,8 +1399,11 @@ static const struct rproc_hexagon_res sdm845_mss = {
>  			"prng",
>  			NULL
>  	},
> -	.active_clk_names = (char*[]){
> +	.reset_clk_names = (char*[]){
>  			"iface",
> +			NULL
> +	},
> +	.active_clk_names = (char*[]){

Again, if you reorder your patches to first add the support for
alt_reset and then introduce sdm845 you don't need to modify the
previous patch directly to make it work.

>  			"bus",
>  			"mem",
>  			"gpll0_mss",

Regards,
Bjorn

Re: [PATCH v4 5/5] remoteproc: qcom: Always assert and deassert reset signals in SDM845

[Sibi S]

Hi Bjorn,
Thanks for the review. Will make all the required changes in v5.

On 05/19/2018 03:17 AM, Bjorn Andersson wrote:
> On Wed 25 Apr 08:08 PDT 2018, Sibi Sankar wrote:
> 
>> SDM845 brings a new reset signal ALT_RESET which is a part of the MSS
>> subsystem hence requires some of the active clks to be enabled before
>> assert/deassert
>>
>> Reset the modem if the BOOT FSM does timeout
>>
>> Reset assert/deassert sequence vary across SoCs adding reset, adding
>> start/stop helper functions to handle SoC specific reset sequences
>>
>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>> ---
>>   drivers/remoteproc/qcom_q6v5_pil.c | 81 ++++++++++++++++++++++++++++--
>>   1 file changed, 76 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
> [..]
>> @@ -349,6 +356,32 @@ static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
>>   	return 0;
>>   }
>>   
>> +static int q6v5_reset_assert(struct q6v5 *qproc)
>> +{
>> +	return reset_control_assert(qproc->mss_restart);
>> +}
>> +
>> +static int q6v5_reset_deassert(struct q6v5 *qproc)
>> +{
>> +	return reset_control_deassert(qproc->mss_restart);
>> +}
>> +
>> +static int q6v5_alt_reset_assert(struct q6v5 *qproc)
>> +{
>> +	return reset_control_reset(qproc->mss_restart);
>> +}
>> +
>> +static int q6v5_alt_reset_deassert(struct q6v5 *qproc)
>> +{
>> +	/* Ensure alt reset is written before restart reg */
>> +	writel(1, qproc->rmb_base + RMB_MBA_ALT_RESET);
>> +
>> +	reset_control_reset(qproc->mss_restart);
>> +
>> +	writel(0, qproc->rmb_base + RMB_MBA_ALT_RESET);
>> +	return 0;
>> +}
>> +
> 
> Rather than having these four functions and scattering jumps to some
> function pointer in the code I think it will be shorter and cleaner to
> just have the q6v5_reset_{asert,deassert}() functions and in there check
> if has_alt_reset and take appropriate action.
> 

yes this seems simpler

>>   static int q6v5_rmb_pbl_wait(struct q6v5 *qproc, int ms)
>>   {
>>   	unsigned long timeout;
>> @@ -424,6 +457,8 @@ static int q6v5proc_reset(struct q6v5 *qproc)
>>   				val, (val & BIT(0)) != 0, 10, BOOT_FSM_TIMEOUT);
>>   		if (ret) {
>>   			dev_err(qproc->dev, "Boot FSM failed to complete.\n");
>> +			/* Reset the modem so that boot FSM is in reset state */
>> +			qproc->reset_deassert(qproc);
> 
> 
> A thing like this typically should go into it's own patch, to keep a
> clear record of why it was changed, but as this is simply amending the
> previous patch it indicates that that one wasn't complete.
> 
> So if you reorder the two patches you can just put this directly into
> the sdm845 patch, making it "complete".
> 
> (This also means that I want to merge the handover vs ready interrupt
> patch before that one, so please include it in the next revision of the
> series).
> 

Will re-order them.


>>   			return ret;
>>   		}
>>   
>> @@ -792,12 +827,20 @@ static int q6v5_start(struct rproc *rproc)
>>   		dev_err(qproc->dev, "failed to enable supplies\n");
>>   		goto disable_proxy_clk;
>>   	}
>> -	ret = reset_control_deassert(qproc->mss_restart);
>> +
>> +	ret = q6v5_clk_enable(qproc->dev, qproc->reset_clks,
>> +			      qproc->reset_clk_count);
> 
> Remind me, why can't you always enable the active clock before
> deasserting reset? That way we wouldn't have to split out the iface
> clock handling to be just slightly longer than the active clocks.
> 

Have to introduce reset clks for backward compatibility, both msm8916
and msm8996 require the mss_reset to be deasserted before enabling
the active clks.

>>   	if (ret) {
>> -		dev_err(qproc->dev, "failed to deassert mss restart\n");
>> +		dev_err(qproc->dev, "failed to enable reset clocks\n");
>>   		goto disable_vdd;
>>   	}
>>   
>> +	ret = qproc->reset_deassert(qproc);
>> +	if (ret) {
>> +		dev_err(qproc->dev, "failed to deassert mss restart\n");
>> +		goto disable_reset_clks;
>> +	}
>> +
> [..]
>> @@ -1335,8 +1399,11 @@ static const struct rproc_hexagon_res sdm845_mss = {
>>   			"prng",
>>   			NULL
>>   	},
>> -	.active_clk_names = (char*[]){
>> +	.reset_clk_names = (char*[]){
>>   			"iface",
>> +			NULL
>> +	},
>> +	.active_clk_names = (char*[]){
> 
> Again, if you reorder your patches to first add the support for
> alt_reset and then introduce sdm845 you don't need to modify the
> previous patch directly to make it work.
> 

yeah I'll re-order them

>>   			"bus",
>>   			"mem",
>>   			"gpll0_mss",
> 
> Regards,
> Bjorn
> 

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