[PATCH v3 0/7] Add support for remoteproc modem-pil on SDM845 SoCs

[PATCH v3 0/7] Add support for remoteproc modem-pil on SDM845 SoCs

[Sibi S]

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

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

Sibi S (7):
  dt-bindings: reset: Add AOSS reset bindings for SDM845 SoCs
  reset: qcom: AOSS (always on subsystem) reset controller
  dt-bindings: mailbox: Add APCS global binding for SDM845 SoCs
  mailbox: Add support for Qualcomm SDM845 SoCs
  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/mailbox/qcom,apcs-kpss-global.txt     |   1 +
 .../devicetree/bindings/remoteproc/qcom,q6v5.txt   |   1 +
 .../devicetree/bindings/reset/qcom,aoss-reset.txt  |  54 +++++++
 drivers/mailbox/qcom-apcs-ipc-mailbox.c            |   1 +
 drivers/remoteproc/qcom_q6v5_pil.c                 | 163 ++++++++++++++++++++-
 drivers/reset/Kconfig                              |  10 ++
 drivers/reset/Makefile                             |   1 +
 drivers/reset/reset-qcom-aoss.c                    | 156 ++++++++++++++++++++
 include/dt-bindings/reset/qcom,sdm845-aoss.h       |  17 +++
 9 files changed, 398 insertions(+), 6 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 v3 1/7] dt-bindings: reset: Add AOSS reset bindings for SDM845 SoCs

[Sibi S]

Add SDM845 AOSS (always on subsystem) reset controller binding

Signed-off-by: Sibi S <sibis@codeaurora.org>
---
 .../devicetree/bindings/reset/qcom,aoss-reset.txt  | 54 ++++++++++++++++++++++
 include/dt-bindings/reset/qcom,sdm845-aoss.h       | 17 +++++++
 2 files changed, 71 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 0000000..04dca76
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/qcom,aoss-reset.txt
@@ -0,0 +1,54 @@
+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 0x20004>;
+	#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 0000000..e9b38fc
--- /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 v3 1/7] dt-bindings: reset: Add AOSS reset bindings for SDM845 SoCs

[Rob Herring]

On Wed, Mar 14, 2018 at 02:51:17PM +0530, Sibi S wrote:
> Add SDM845 AOSS (always on subsystem) reset controller binding
> 
> Signed-off-by: Sibi S <sibis@codeaurora.org>

Still need a full name.

Otherwise, looks fine.

> ---
>  .../devicetree/bindings/reset/qcom,aoss-reset.txt  | 54 ++++++++++++++++++++++
>  include/dt-bindings/reset/qcom,sdm845-aoss.h       | 17 +++++++
>  2 files changed, 71 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/qcom,aoss-reset.txt
>  create mode 100644 include/dt-bindings/reset/qcom,sdm845-aoss.h

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

[Bjorn Andersson]

On Wed 14 Mar 02:21 PDT 2018, Sibi S wrote:
> +- reg:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: must specify the base address and size of the register
> +	            space.
> +
> +

Double empty lines.

> +- #reset-cells:
> +	Usage: required
> +	Value type: <uint>
> +	Definition: must be 1; cell entry represents the reset index.
> +
> +example:

Please capitalize the initial char.

> +
> +aoss_reset: qcom,reset-controller@b2e0100 {
> +	compatible = "qcom,sdm845-aoss-reset";
> +	reg = <0xc2b0000 0x20004>;

0x20004 does seem very even, please verify this size.

> +	#reset-cells = <1>;
> +};
> +
> +
> +Specifying reset lines connected to IP modules

"AOSS reset clients"

Although you could probably get a way with just referring to reset.txt
and the header file.

> +==============================================
> +
> +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 0000000..e9b38fc
> --- /dev/null
> +++ b/include/dt-bindings/reset/qcom,sdm845-aoss.h
> @@ -0,0 +1,17 @@
> +// SPDX-License-Identifier: GPL-2.0

For tooling reasons header files should have their SPDX License tag
wrapped in /* */

> +/*
> + * 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

Apart from these nits this looks reasonable.

Regards,
Bjorn

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

[Sibi S]

Hi Bjorn,
Thanks for the review.

On 03/19/2018 04:14 AM, Bjorn Andersson wrote:
> On Wed 14 Mar 02:21 PDT 2018, Sibi S wrote:
>> +- reg:
>> +	Usage: required
>> +	Value type: <prop-encoded-array>
>> +	Definition: must specify the base address and size of the register
>> +	            space.
>> +
>> +
> 
> Double empty lines.
> 

will remove them

>> +- #reset-cells:
>> +	Usage: required
>> +	Value type: <uint>
>> +	Definition: must be 1; cell entry represents the reset index.
>> +
>> +example:
> 
> Please capitalize the initial char.
>

sure

>> +
>> +aoss_reset: qcom,reset-controller@b2e0100 {
>> +	compatible = "qcom,sdm845-aoss-reset";
>> +	reg = <0xc2b0000 0x20004>;
> 
> 0x20004 does seem very even, please verify this size.
> 

even though the reg space after that range is unused, the AOSS reset
driver is supposed to control only those listed reset lines

>> +	#reset-cells = <1>;
>> +};
>> +
>> +
>> +Specifying reset lines connected to IP modules
> 
> "AOSS reset clients"
> 

yep this heading makes much more sense

> Although you could probably get a way with just referring to reset.txt
> and the header file.
> 
>> +==============================================
>> +
>> +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 0000000..e9b38fc
>> --- /dev/null
>> +++ b/include/dt-bindings/reset/qcom,sdm845-aoss.h
>> @@ -0,0 +1,17 @@
>> +// SPDX-License-Identifier: GPL-2.0
> 
> For tooling reasons header files should have their SPDX License tag
> wrapped in /* */
> 

Sure will replace it

>> +/*
>> + * 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
> 
> Apart from these nits this looks reasonable.
> 
> Regards,
> Bjorn
> 

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

[PATCH v3 2/7] reset: qcom: AOSS (always on subsystem) reset controller

[Sibi S]

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 S <sibis@codeaurora.org>
---
 drivers/reset/Kconfig           |  10 +++
 drivers/reset/Makefile          |   1 +
 drivers/reset/reset-qcom-aoss.c | 156 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 167 insertions(+)
 create mode 100644 drivers/reset/reset-qcom-aoss.c

diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index 7fc7769..d06bd1d 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -81,6 +81,16 @@ 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
+	select MFD_SYSCON
+	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
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index 132c24f..c30044a 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_SUNXI) += reset-sunxi.o
 obj-$(CONFIG_RESET_TI_SCI) += reset-ti-sci.o
diff --git a/drivers/reset/reset-qcom-aoss.c b/drivers/reset/reset-qcom-aoss.c
new file mode 100644
index 0000000..e70fb37
--- /dev/null
+++ b/drivers/reset/reset-qcom-aoss.c
@@ -0,0 +1,156 @@
+// 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-aoss.h>
+
+struct qcom_aoss_reset_map {
+	unsigned int reg;
+	u8 bit;
+};
+
+struct qcom_aoss_desc {
+	const struct regmap_config *config;
+	const struct qcom_aoss_reset_map *resets;
+	int delay;
+	size_t num_resets;
+};
+
+struct qcom_aoss_reset_data {
+	struct reset_controller_dev rcdev;
+	struct regmap *regmap;
+	const struct qcom_aoss_desc *desc;
+};
+
+static const struct regmap_config sdm845_aoss_regmap_config = {
+	.name		= "aoss-reset",
+	.reg_bits	= 32,
+	.reg_stride	= 4,
+	.val_bits	= 32,
+	.max_register	= 0x20000,
+	.fast_io	= true,
+};
+
+static const struct qcom_aoss_reset_map sdm845_aoss_resets[] = {
+	[AOSS_CC_MSS_RESTART] = { 0x0, 0 },
+	[AOSS_CC_CAMSS_RESTART] = { 0x1000, 0 },
+	[AOSS_CC_VENUS_RESTART] = { 0x2000, 0 },
+	[AOSS_CC_GPU_RESTART] = { 0x3000, 0 },
+	[AOSS_CC_DISPSS_RESTART] = { 0x4000, 0 },
+	[AOSS_CC_WCSS_RESTART] = { 0x10000, 0 },
+	[AOSS_CC_LPASS_RESTART] = { 0x20000, 0 },
+};
+
+static const struct qcom_aoss_desc sdm845_aoss_desc = {
+	.config = &sdm845_aoss_regmap_config,
+	.resets = sdm845_aoss_resets,
+	/* Wait 6 32kHz sleep cycles for reset */
+	.delay = 200,
+	.num_resets = ARRAY_SIZE(sdm845_aoss_resets),
+};
+
+static 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];
+
+	return regmap_update_bits(data->regmap, map->reg,
+					BIT(map->bit), BIT(map->bit));
+}
+
+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];
+
+	return regmap_update_bits(data->regmap, map->reg, BIT(map->bit), 0);
+}
+
+static int qcom_aoss_control_reset(struct reset_controller_dev *rcdev,
+					unsigned long idx)
+{
+	struct qcom_aoss_reset_data *data = to_qcom_aoss_reset_data(rcdev);
+	int ret;
+
+	ret = qcom_aoss_control_assert(rcdev, idx);
+	if (ret)
+		return ret;
+
+	udelay(data->desc->delay);
+
+	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;
+	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 aoss-reset regmap");
+		return PTR_ERR(data->regmap);
+	}
+
+	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 v3 2/7] reset: qcom: AOSS (always on subsystem) reset controller

[Vivek Gautam]

Hi Sibi,


On 3/14/2018 2:51 PM, Sibi S 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 S <sibis@codeaurora.org>
> ---
>   drivers/reset/Kconfig           |  10 +++
>   drivers/reset/Makefile          |   1 +
>   drivers/reset/reset-qcom-aoss.c | 156 ++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 167 insertions(+)
>   create mode 100644 drivers/reset/reset-qcom-aoss.c
>
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 7fc7769..d06bd1d 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -81,6 +81,16 @@ 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
> +	select MFD_SYSCON

I think we don't need this after we moved away from syscon?

> +	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
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 132c24f..c30044a 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_SUNXI) += reset-sunxi.o
>   obj-$(CONFIG_RESET_TI_SCI) += reset-ti-sci.o
> diff --git a/drivers/reset/reset-qcom-aoss.c b/drivers/reset/reset-qcom-aoss.c
> new file mode 100644
> index 0000000..e70fb37
> --- /dev/null
> +++ b/drivers/reset/reset-qcom-aoss.c
> @@ -0,0 +1,156 @@
> +// 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-aoss.h>
> +
> +struct qcom_aoss_reset_map {
> +	unsigned int reg;
> +	u8 bit;
> +};
> +
> +struct qcom_aoss_desc {
> +	const struct regmap_config *config;
> +	const struct qcom_aoss_reset_map *resets;
> +	int delay;
> +	size_t num_resets;
> +};
> +
> +struct qcom_aoss_reset_data {
> +	struct reset_controller_dev rcdev;
> +	struct regmap *regmap;
> +	const struct qcom_aoss_desc *desc;
> +};
> +
> +static const struct regmap_config sdm845_aoss_regmap_config = {
> +	.name		= "aoss-reset",
> +	.reg_bits	= 32,
> +	.reg_stride	= 4,
> +	.val_bits	= 32,
> +	.max_register	= 0x20000,
> +	.fast_io	= true,
> +};
> +
> +static const struct qcom_aoss_reset_map sdm845_aoss_resets[] = {
> +	[AOSS_CC_MSS_RESTART] = { 0x0, 0 },
> +	[AOSS_CC_CAMSS_RESTART] = { 0x1000, 0 },
> +	[AOSS_CC_VENUS_RESTART] = { 0x2000, 0 },
> +	[AOSS_CC_GPU_RESTART] = { 0x3000, 0 },
> +	[AOSS_CC_DISPSS_RESTART] = { 0x4000, 0 },
> +	[AOSS_CC_WCSS_RESTART] = { 0x10000, 0 },
> +	[AOSS_CC_LPASS_RESTART] = { 0x20000, 0 },
> +};
> +
> +static const struct qcom_aoss_desc sdm845_aoss_desc = {
> +	.config = &sdm845_aoss_regmap_config,
> +	.resets = sdm845_aoss_resets,
> +	/* Wait 6 32kHz sleep cycles for reset */
> +	.delay = 200,
> +	.num_resets = ARRAY_SIZE(sdm845_aoss_resets),
> +};
> +
> +static 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);
> +}

Either a macro for such definition or 'inline'?

> +
> +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];
> +
> +	return regmap_update_bits(data->regmap, map->reg,
> +					BIT(map->bit), BIT(map->bit));
> +}
> +
> +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];
> +
> +	return regmap_update_bits(data->regmap, map->reg, BIT(map->bit), 0);
> +}
> +
> +static int qcom_aoss_control_reset(struct reset_controller_dev *rcdev,
> +					unsigned long idx)
> +{
> +	struct qcom_aoss_reset_data *data = to_qcom_aoss_reset_data(rcdev);
> +	int ret;
> +
> +	ret = qcom_aoss_control_assert(rcdev, idx);
> +	if (ret)
> +		return ret;
> +
> +	udelay(data->desc->delay);
> +
> +	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;
> +	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 aoss-reset regmap");
> +		return PTR_ERR(data->regmap);
> +	}
> +
> +	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");

Rest looks good to me. After taking above changes -
Reviewed-by: Vivek Gautam <vivek.gautam@codeaurora.org>

regards
Vivek

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

[Bjorn Andersson]

On Wed 14 Mar 02:21 PDT 2018, Sibi S wrote:
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 7fc7769..d06bd1d 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -81,6 +81,16 @@ 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
> +	select MFD_SYSCON

Drop syscon

> +	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.
> +
[..]
> diff --git a/drivers/reset/reset-qcom-aoss.c b/drivers/reset/reset-qcom-aoss.c
[..]
> +static const struct regmap_config sdm845_aoss_regmap_config = {
> +	.name		= "aoss-reset",
> +	.reg_bits	= 32,
> +	.reg_stride	= 4,
> +	.val_bits	= 32,
> +	.max_register	= 0x20000,
> +	.fast_io	= true,
> +};

Is there a particular reason why you're setting up a regmap and not just
operate on the ioremap region directly with readl/writel? It would save
you a few lines of code and some runtime memory.

> +
> +static const struct qcom_aoss_reset_map sdm845_aoss_resets[] = {
> +	[AOSS_CC_MSS_RESTART] = { 0x0, 0 },
> +	[AOSS_CC_CAMSS_RESTART] = { 0x1000, 0 },
> +	[AOSS_CC_VENUS_RESTART] = { 0x2000, 0 },
> +	[AOSS_CC_GPU_RESTART] = { 0x3000, 0 },
> +	[AOSS_CC_DISPSS_RESTART] = { 0x4000, 0 },
> +	[AOSS_CC_WCSS_RESTART] = { 0x10000, 0 },
> +	[AOSS_CC_LPASS_RESTART] = { 0x20000, 0 },

Do you have a case where bit != 0? If not please drop the bit until it's
necessary.

> +};
> +
> +static const struct qcom_aoss_desc sdm845_aoss_desc = {
> +	.config = &sdm845_aoss_regmap_config,
> +	.resets = sdm845_aoss_resets,
> +	/* Wait 6 32kHz sleep cycles for reset */
> +	.delay = 200,

Please move this constant into qcom_aoss_control_reset(), until there's
a need for it to be configurable.

> +	.num_resets = ARRAY_SIZE(sdm845_aoss_resets),
> +};
> +
[..]
> +static int qcom_aoss_control_reset(struct reset_controller_dev *rcdev,
> +					unsigned long idx)
> +{
> +	struct qcom_aoss_reset_data *data = to_qcom_aoss_reset_data(rcdev);
> +	int ret;
> +
> +	ret = qcom_aoss_control_assert(rcdev, idx);
> +	if (ret)
> +		return ret;
> +
> +	udelay(data->desc->delay);

Per Documentation/timers/timers-howto.txt please use usleep_range() when
delays are between 10us and 20ms.

> +
> +	return qcom_aoss_control_deassert(rcdev, idx);
> +}

Regards,
Bjorn

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

[Sibi S]

Hi Bjorn,
Thanks for the review.

On 03/19/2018 04:15 AM, Bjorn Andersson wrote:
> On Wed 14 Mar 02:21 PDT 2018, Sibi S wrote:
>> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
>> index 7fc7769..d06bd1d 100644
>> --- a/drivers/reset/Kconfig
>> +++ b/drivers/reset/Kconfig
>> @@ -81,6 +81,16 @@ 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
>> +	select MFD_SYSCON
> 
> Drop syscon
> 

will drop it

>> +	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.
>> +
> [..]
>> diff --git a/drivers/reset/reset-qcom-aoss.c b/drivers/reset/reset-qcom-aoss.c
> [..]
>> +static const struct regmap_config sdm845_aoss_regmap_config = {
>> +	.name		= "aoss-reset",
>> +	.reg_bits	= 32,
>> +	.reg_stride	= 4,
>> +	.val_bits	= 32,
>> +	.max_register	= 0x20000,
>> +	.fast_io	= true,
>> +};
> 
> Is there a particular reason why you're setting up a regmap and not just
> operate on the ioremap region directly with readl/writel? It would save
> you a few lines of code and some runtime memory.
> 

The idea here is to reuse the driver in modified configuration as
pdc_sync restart controller which is to be used for adsp_pil. PDC sync
reset register is a single register with multiple reset lines hence
would warrant setting up a regmap. I can remove these changes and add
them when I add pdc_sync reset controller though.

>> +
>> +static const struct qcom_aoss_reset_map sdm845_aoss_resets[] = {
>> +	[AOSS_CC_MSS_RESTART] = { 0x0, 0 },
>> +	[AOSS_CC_CAMSS_RESTART] = { 0x1000, 0 },
>> +	[AOSS_CC_VENUS_RESTART] = { 0x2000, 0 },
>> +	[AOSS_CC_GPU_RESTART] = { 0x3000, 0 },
>> +	[AOSS_CC_DISPSS_RESTART] = { 0x4000, 0 },
>> +	[AOSS_CC_WCSS_RESTART] = { 0x10000, 0 },
>> +	[AOSS_CC_LPASS_RESTART] = { 0x20000, 0 },
> 
> Do you have a case where bit != 0? If not please drop the bit until it's
> necessary.
> 

had the bit variable for the above state reason.

>> +};
>> +
>> +static const struct qcom_aoss_desc sdm845_aoss_desc = {
>> +	.config = &sdm845_aoss_regmap_config,
>> +	.resets = sdm845_aoss_resets,
>> +	/* Wait 6 32kHz sleep cycles for reset */
>> +	.delay = 200,
> 
> Please move this constant into qcom_aoss_control_reset(), until there's
> a need for it to be configurable.
> 
>> +	.num_resets = ARRAY_SIZE(sdm845_aoss_resets),
>> +};
>> +
> [..]
>> +static int qcom_aoss_control_reset(struct reset_controller_dev *rcdev,
>> +					unsigned long idx)
>> +{
>> +	struct qcom_aoss_reset_data *data = to_qcom_aoss_reset_data(rcdev);
>> +	int ret;
>> +
>> +	ret = qcom_aoss_control_assert(rcdev, idx);
>> +	if (ret)
>> +		return ret;
>> +
>> +	udelay(data->desc->delay);
> 
> Per Documentation/timers/timers-howto.txt please use usleep_range() when
> delays are between 10us and 20ms.
> 

will replace them

>> +
>> +	return qcom_aoss_control_deassert(rcdev, idx);
>> +}
> 
> Regards,
> Bjorn
> 

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

[PATCH v3 3/7] dt-bindings: mailbox: Add APCS global binding for SDM845 SoCs

[Sibi S]

Include SDM845 APCS binding to the list of possible bindings

Signed-off-by: Sibi S <sibis@codeaurora.org>
---
 Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt
index 16964f0..6a7e5d8 100644
--- a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt
+++ b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt
@@ -10,6 +10,7 @@ platforms.
 	Definition: must be one of:
 		    "qcom,msm8916-apcs-kpss-global",
 		    "qcom,msm8996-apcs-hmss-global"
+		    "qcom,sdm845-apcs-hmss-global"
 
 - reg:
 	Usage: required
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 3/7] dt-bindings: mailbox: Add APCS global binding for SDM845 SoCs

[Bjorn Andersson]

On Wed 14 Mar 02:21 PDT 2018, Sibi S wrote:

> Include SDM845 APCS binding to the list of possible bindings
> 

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

Regards,
Bjorn

> Signed-off-by: Sibi S <sibis@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt
> index 16964f0..6a7e5d8 100644
> --- a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt
> +++ b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt
> @@ -10,6 +10,7 @@ platforms.
>  	Definition: must be one of:
>  		    "qcom,msm8916-apcs-kpss-global",
>  		    "qcom,msm8996-apcs-hmss-global"
> +		    "qcom,sdm845-apcs-hmss-global"
>  
>  - reg:
>  	Usage: required
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

[PATCH v3 4/7] mailbox: Add support for Qualcomm SDM845 SoCs

[Sibi S]

Add the corresponding APCS offset for SDM845 SoC

Signed-off-by: Sibi S <sibis@codeaurora.org>
---
 drivers/mailbox/qcom-apcs-ipc-mailbox.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mailbox/qcom-apcs-ipc-mailbox.c b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
index 57bde0d..62d704d 100644
--- a/drivers/mailbox/qcom-apcs-ipc-mailbox.c
+++ b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
@@ -125,6 +125,7 @@ static int qcom_apcs_ipc_remove(struct platform_device *pdev)
 static const struct of_device_id qcom_apcs_ipc_of_match[] = {
 	{ .compatible = "qcom,msm8916-apcs-kpss-global", .data = (void *)8 },
 	{ .compatible = "qcom,msm8996-apcs-hmss-global", .data = (void *)16 },
+	{ .compatible = "qcom,sdm845-apcs-hmss-global", .data = (void *)12 },
 	{}
 };
 MODULE_DEVICE_TABLE(of, qcom_apcs_ipc_of_match);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 4/7] mailbox: Add support for Qualcomm SDM845 SoCs

[Bjorn Andersson]

On Wed 14 Mar 02:21 PDT 2018, Sibi S wrote:

> Add the corresponding APCS offset for SDM845 SoC
> 

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

Regards,
Bjorn

> Signed-off-by: Sibi S <sibis@codeaurora.org>
> ---
>  drivers/mailbox/qcom-apcs-ipc-mailbox.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/mailbox/qcom-apcs-ipc-mailbox.c b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
> index 57bde0d..62d704d 100644
> --- a/drivers/mailbox/qcom-apcs-ipc-mailbox.c
> +++ b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
> @@ -125,6 +125,7 @@ static int qcom_apcs_ipc_remove(struct platform_device *pdev)
>  static const struct of_device_id qcom_apcs_ipc_of_match[] = {
>  	{ .compatible = "qcom,msm8916-apcs-kpss-global", .data = (void *)8 },
>  	{ .compatible = "qcom,msm8996-apcs-hmss-global", .data = (void *)16 },
> +	{ .compatible = "qcom,sdm845-apcs-hmss-global", .data = (void *)12 },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, qcom_apcs_ipc_of_match);
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH v3 4/7] mailbox: Add support for Qualcomm SDM845 SoCs

[Bjorn Andersson]

On Wed 14 Mar 02:21 PDT 2018, Sibi S wrote:

> Add the corresponding APCS offset for SDM845 SoC
> 
> Signed-off-by: Sibi S <sibis@codeaurora.org>

Full name please.

> ---
>  drivers/mailbox/qcom-apcs-ipc-mailbox.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/mailbox/qcom-apcs-ipc-mailbox.c b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
> index 57bde0d..62d704d 100644
> --- a/drivers/mailbox/qcom-apcs-ipc-mailbox.c
> +++ b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
> @@ -125,6 +125,7 @@ static int qcom_apcs_ipc_remove(struct platform_device *pdev)
>  static const struct of_device_id qcom_apcs_ipc_of_match[] = {
>  	{ .compatible = "qcom,msm8916-apcs-kpss-global", .data = (void *)8 },
>  	{ .compatible = "qcom,msm8996-apcs-hmss-global", .data = (void *)16 },
> +	{ .compatible = "qcom,sdm845-apcs-hmss-global", .data = (void *)12 },

The block seems to be called "apss shared", so make the compatible
"qcom,sdm845-apss-shared".

Please resubmit this patch separate from the others in the series, as
they can be merged independently of the rest of the series.


PS. For updates like this you can generally submit the dt and code
together.

Regards,
Bjorn

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

[Sibi S]

Add new compatible string for Qualcomm SDM845 SoCs

Signed-off-by: Sibi S <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 00d3d58..d901824 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 v3 5/7] dt-bindings: remoteproc: Add Q6v5 Modem PIL binding for SDM845

[Bjorn Andersson]

On Wed 14 Mar 02:21 PDT 2018, Sibi S wrote:

> Add new compatible string for Qualcomm SDM845 SoCs
> 

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

Regards,
Bjorn

> Signed-off-by: Sibi S <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 00d3d58..d901824 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
> 

[PATCH v3 6/7] remoteproc: qcom: Add support for mss remoteproc on SDM845

[Sibi S]

>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 S <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 b4e5e72..92bf125 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;
@@ -166,6 +175,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;
 	bool need_mem_protection;
@@ -178,6 +188,7 @@ enum {
 	MSS_MSM8916,
 	MSS_MSM8974,
 	MSS_MSM8996,
+	MSS_SDM845,
 };
 
 static int q6v5_regulator_init(struct device *dev, struct reg_info *regs,
@@ -384,8 +395,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);
@@ -493,6 +531,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) {
@@ -1213,6 +1252,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");
 
@@ -1234,6 +1274,7 @@ static int q6v5_remove(struct platform_device *pdev)
 
 	rproc_del(qproc->rproc);
 
+	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);
@@ -1241,6 +1282,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*[]){
@@ -1334,6 +1396,7 @@ static int q6v5_remove(struct platform_device *pdev)
 	{ .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

[PATCH v3 7/7] remoteproc: qcom: Always assert and deassert reset signals in SDM845

[Sibi S]

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 S <sibis@codeaurora.org>
---
 drivers/remoteproc/qcom_q6v5_pil.c | 100 ++++++++++++++++++++++++++++++++++---
 1 file changed, 94 insertions(+), 6 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index 92bf125..bd8c397 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -130,11 +130,14 @@ 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;
 };
 
+struct q6v5_reset_ops;
+
 struct q6v5 {
 	struct device *dev;
 	struct rproc *rproc;
@@ -153,8 +156,10 @@ struct q6v5 {
 	unsigned stop_bit;
 
 	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];
@@ -175,6 +180,7 @@ struct q6v5 {
 	void *mpss_region;
 	size_t mpss_size;
 
+	const struct q6v5_reset_ops *ops;
 	struct qcom_rproc_glink glink_subdev;
 	struct qcom_rproc_subdev smd_subdev;
 	struct qcom_rproc_ssr ssr_subdev;
@@ -184,6 +190,11 @@ struct q6v5 {
 	int version;
 };
 
+struct q6v5_reset_ops {
+	int (*reset_start)(struct q6v5 *qproc);
+	int (*reset_stop)(struct q6v5 *qproc);
+};
+
 enum {
 	MSS_MSM8916,
 	MSS_MSM8974,
@@ -343,6 +354,52 @@ static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
 	return 0;
 }
 
+static void alt_reset_restart(struct q6v5 *qproc, u32 restart)
+{
+	writel(restart, qproc->rmb_base + RMB_MBA_ALT_RESET);
+}
+
+static int q6v5_msm_reset_stop(struct q6v5 *qproc)
+{
+	return reset_control_assert(qproc->mss_restart);
+}
+
+static int q6v5_msm_reset_start(struct q6v5 *qproc)
+{
+	return reset_control_deassert(qproc->mss_restart);
+}
+
+static int q6v5_sdm_reset_stop(struct q6v5 *qproc)
+{
+	return reset_control_reset(qproc->mss_restart);
+}
+
+static int q6v5_sdm_reset_start(struct q6v5 *qproc)
+{
+	int ret;
+
+	alt_reset_restart(qproc, 1);
+	/* Ensure alt reset is written before restart reg */
+	udelay(100);
+
+	ret = reset_control_reset(qproc->mss_restart);
+
+	udelay(100);
+	alt_reset_restart(qproc, 0);
+
+	return ret;
+}
+
+static const struct q6v5_reset_ops q6v5_msm_ops = {
+	.reset_stop = q6v5_msm_reset_stop,
+	.reset_start = q6v5_msm_reset_start,
+};
+
+static const struct q6v5_reset_ops q6v5_sdm_ops = {
+	.reset_stop = q6v5_sdm_reset_stop,
+	.reset_start = q6v5_sdm_reset_start,
+};
+
 static int q6v5_rmb_pbl_wait(struct q6v5 *qproc, int ms)
 {
 	unsigned long timeout;
@@ -418,6 +475,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->ops->reset_start(qproc);
 			return ret;
 		}
 
@@ -785,10 +844,18 @@ 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 enable reset clocks\n");
+		goto disable_vdd;
+	}
+
+	ret = qproc->ops->reset_start(qproc);
 	if (ret) {
 		dev_err(qproc->dev, "failed to deassert mss restart\n");
-		goto disable_vdd;
+		goto disable_reset_clks;
 	}
 
 	ret = q6v5_clk_enable(qproc->dev, qproc->active_clks,
@@ -880,7 +947,10 @@ static int q6v5_start(struct rproc *rproc)
 			 qproc->active_clk_count);
 
 assert_reset:
-	reset_control_assert(qproc->mss_restart);
+	qproc->ops->reset_stop(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);
@@ -930,9 +1000,11 @@ static int q6v5_stop(struct rproc *rproc)
 				      qproc->mpss_phys, qproc->mpss_size);
 	WARN_ON(ret);
 
-	reset_control_assert(qproc->mss_restart);
+	qproc->ops->reset_stop(qproc);
 	q6v5_clk_disable(qproc->dev, qproc->active_clks,
 			 qproc->active_clk_count);
+	q6v5_clk_disable(qproc->dev, qproc->reset_clks,
+			 qproc->reset_clk_count);
 	q6v5_regulator_disable(qproc, qproc->active_regs,
 			       qproc->active_reg_count);
 
@@ -1179,6 +1251,12 @@ static int q6v5_probe(struct platform_device *pdev)
 	qproc->dev = &pdev->dev;
 	qproc->rproc = rproc;
 	platform_set_drvdata(pdev, qproc);
+	qproc->version = desc->version;
+
+	if (qproc->version == MSS_SDM845)
+		qproc->ops = &q6v5_sdm_ops;
+	else
+		qproc->ops = &q6v5_msm_ops;
 
 	init_completion(&qproc->start_done);
 	init_completion(&qproc->stop_done);
@@ -1199,6 +1277,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) {
@@ -1227,7 +1313,6 @@ static int q6v5_probe(struct platform_device *pdev)
 	if (ret)
 		goto free_rproc;
 
-	qproc->version = desc->version;
 	qproc->need_mem_protection = desc->need_mem_protection;
 	ret = q6v5_request_irq(qproc, pdev, "wdog", q6v5_wdog_interrupt);
 	if (ret < 0)
@@ -1290,8 +1375,11 @@ static int q6v5_remove(struct platform_device *pdev)
 			"prng",
 			NULL
 	},
-	.active_clk_names = (char*[]){
+	.reset_clk_names = (char*[]){
 			"iface",
+			NULL
+	},
+	.active_clk_names = (char*[]){
 			"bus",
 			"mem",
 			"gpll0_mss",
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Bjorn Andersson]

On Wed 14 Mar 02:21 PDT 2018, Sibi S wrote:
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
[..]
> +struct q6v5_reset_ops {
> +	int (*reset_start)(struct q6v5 *qproc);
> +	int (*reset_stop)(struct q6v5 *qproc);
> +};

Please add these two ops directly in q6v5 instead and please keep the
naming "reset_assert" and "reset_deassert".

> +
>  enum {
>  	MSS_MSM8916,
>  	MSS_MSM8974,
> @@ -343,6 +354,52 @@ static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
>  	return 0;
>  }
>  
> +static void alt_reset_restart(struct q6v5 *qproc, u32 restart)
> +{
> +	writel(restart, qproc->rmb_base + RMB_MBA_ALT_RESET);

Just move this write into q6v5_sdm_reset_start()

> +}
> +
> +static int q6v5_msm_reset_stop(struct q6v5 *qproc)
> +{
> +	return reset_control_assert(qproc->mss_restart);
> +}
> +
> +static int q6v5_msm_reset_start(struct q6v5 *qproc)
> +{
> +	return reset_control_deassert(qproc->mss_restart);
> +}
> +
> +static int q6v5_sdm_reset_stop(struct q6v5 *qproc)
> +{
> +	return reset_control_reset(qproc->mss_restart);
> +}
> +
> +static int q6v5_sdm_reset_start(struct q6v5 *qproc)
> +{
> +	int ret;
> +
> +	alt_reset_restart(qproc, 1);
> +	/* Ensure alt reset is written before restart reg */

That's not what udelay does ;)

If you want to make sure that the register is written and then give it
100us delay until you reset the reset you have to readl() the same
register back after the writel()

I think the delay deserves a comment on what we're waiting for.

> +	udelay(100);

Use usleep_range() for delays longer than 10us.

> +
> +	ret = reset_control_reset(qproc->mss_restart);
> +
> +	udelay(100);

Same as the delay above, what are we waiting for?

> +	alt_reset_restart(qproc, 0);
> +
> +	return ret;
> +}
> +
[..]
> @@ -1179,6 +1251,12 @@ static int q6v5_probe(struct platform_device *pdev)
>  	qproc->dev = &pdev->dev;
>  	qproc->rproc = rproc;
>  	platform_set_drvdata(pdev, qproc);
> +	qproc->version = desc->version;
> +
> +	if (qproc->version == MSS_SDM845)
> +		qproc->ops = &q6v5_sdm_ops;
> +	else
> +		qproc->ops = &q6v5_msm_ops;

Can we carry a boolean for "has_alt_reset" or something that picks the
new reset ops, rather than picking by version?

Regards,
Bjorn

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

[Sibi S]

Hi Bjorn,
Thanks for the review

On 03/19/2018 04:16 AM, Bjorn Andersson wrote:
> On Wed 14 Mar 02:21 PDT 2018, Sibi S wrote:
>> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
> [..]
>> +struct q6v5_reset_ops {
>> +	int (*reset_start)(struct q6v5 *qproc);
>> +	int (*reset_stop)(struct q6v5 *qproc);
>> +};
> 
> Please add these two ops directly in q6v5 instead and please keep the
> naming "reset_assert" and "reset_deassert".
> 

sure will do

>> +
>>   enum {
>>   	MSS_MSM8916,
>>   	MSS_MSM8974,
>> @@ -343,6 +354,52 @@ static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
>>   	return 0;
>>   }
>>   
>> +static void alt_reset_restart(struct q6v5 *qproc, u32 restart)
>> +{
>> +	writel(restart, qproc->rmb_base + RMB_MBA_ALT_RESET);
> 
> Just move this write into q6v5_sdm_reset_start()
> 

sure will do

>> +}
>> +
>> +static int q6v5_msm_reset_stop(struct q6v5 *qproc)
>> +{
>> +	return reset_control_assert(qproc->mss_restart);
>> +}
>> +
>> +static int q6v5_msm_reset_start(struct q6v5 *qproc)
>> +{
>> +	return reset_control_deassert(qproc->mss_restart);
>> +}
>> +
>> +static int q6v5_sdm_reset_stop(struct q6v5 *qproc)
>> +{
>> +	return reset_control_reset(qproc->mss_restart);
>> +}
>> +
>> +static int q6v5_sdm_reset_start(struct q6v5 *qproc)
>> +{
>> +	int ret;
>> +
>> +	alt_reset_restart(qproc, 1);
>> +	/* Ensure alt reset is written before restart reg */
> 
> That's not what udelay does ;)
> 
> If you want to make sure that the register is written and then give it
> 100us delay until you reset the reset you have to readl() the same
> register back after the writel()
> 
> I think the delay deserves a comment on what we're waiting for.
>

the delay can be removed for the reasons stated below

>> +	udelay(100);
> 
> Use usleep_range() for delays longer than 10us.
>
>> +
>> +	ret = reset_control_reset(qproc->mss_restart);
>> +
>> +	udelay(100);
> 
> Same as the delay above, what are we waiting for?
>

The point is to wait for 6 32khz sleep cycles both after assert and
after de-assert of the aoss mss reset line. So moving the udelay to
the aoss reset controller should have been right thing to do. alt_reset
shouldn't need delays in between will remove them.

>> +	alt_reset_restart(qproc, 0);
>> +
>> +	return ret;
>> +}
>> +
> [..]
>> @@ -1179,6 +1251,12 @@ static int q6v5_probe(struct platform_device *pdev)
>>   	qproc->dev = &pdev->dev;
>>   	qproc->rproc = rproc;
>>   	platform_set_drvdata(pdev, qproc);
>> +	qproc->version = desc->version;
>> +
>> +	if (qproc->version == MSS_SDM845)
>> +		qproc->ops = &q6v5_sdm_ops;
>> +	else
>> +		qproc->ops = &q6v5_msm_ops;
> 
> Can we carry a boolean for "has_alt_reset" or something that picks the
> new reset ops, rather than picking by version?
>

Though alt_reset is specific to SDM845 SoCs it also includes another
reset controller (pdc_sync) in the modem bringup sequence, it isn't
included it in the patch series since it doesn't seem to affect the
modem start/stop functionality. Knowing that it may be added wouldn't
version be a better choice here?

> Regards,
> Bjorn
> 

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