[PATCH v2 0/6] Add non PAS wcss Q6 support for QCS404

[PATCH v2 0/6] Add non PAS wcss Q6 support for QCS404

[Govind Singh]

Add non PAS(Peripheral Authentication System) WCSS remoteproc
driver support and wcss clock/reset controls for QCS404 SOC.
Add WCSS q6 bootup and shutdown sequence handled from
Application Processor SubSystem(APSS).

Govind Singh (6):
  dt-bindings: clock: qcom: Add QCOM WCSS GCC clock bindings
  clk: qcom: Add WCSS gcc clock control for QCS404
  dt-bindings: clock: qcom: Introduce QCOM WCSS Q6DSP clock bindings
  clk: qcom: Add WCSS Q6DSP clock controller for QCS404
  remoteproc: qcom: wcss: populate hardcoded param using driver data
  remoteproc: qcom: wcss: Add non pas wcss Q6 support for QCS404

Changes since v1:
  Corrected clock names as per comments in v1 patch.

 .../devicetree/bindings/clock/qcom,wcsscc.txt      |  26 +
 .../devicetree/bindings/remoteproc/qcom,q6v5.txt   |   1 +
 drivers/clk/qcom/Kconfig                           |   9 +
 drivers/clk/qcom/Makefile                          |   1 +
 drivers/clk/qcom/gcc-qcs404.c                      |  37 +-
 drivers/clk/qcom/wcsscc-qcs404.c                   | 290 +++++++++
 drivers/remoteproc/qcom_q6v5_wcss.c                | 666 +++++++++++++++++++--
 include/dt-bindings/clock/qcom,gcc-qcs404.h        |   3 +
 include/dt-bindings/clock/qcom,wcss-qcs404.h       |  24 +
 9 files changed, 1018 insertions(+), 39 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/qcom,wcsscc.txt
 create mode 100644 drivers/clk/qcom/wcsscc-qcs404.c
 create mode 100644 include/dt-bindings/clock/qcom,wcss-qcs404.h

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

[PATCH v2 1/6] dt-bindings: clock: qcom: Add QCOM WCSS GCC clock bindings

[Govind Singh]

Add device tree bindings for WiFi QDSP gcc clock controls found in
QCS404 soc.

Signed-off-by: Govind Singh <govinds@codeaurora.org>
---
 include/dt-bindings/clock/qcom,gcc-qcs404.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/dt-bindings/clock/qcom,gcc-qcs404.h b/include/dt-bindings/clock/qcom,gcc-qcs404.h
index e2def29..ba6bc55 100644
--- a/include/dt-bindings/clock/qcom,gcc-qcs404.h
+++ b/include/dt-bindings/clock/qcom,gcc-qcs404.h
@@ -147,6 +147,8 @@
 #define GCC_MDP_TBU_CLK					138
 #define GCC_QDSS_DAP_CLK				139
 #define GCC_DCC_XO_CLK					140
+#define GCC_WCSS_Q6_AHB_CBCR_CLK			141
+#define GCC_WCSS_Q6_AXIM_CBCR_CLK			142
 
 #define GCC_GENI_IR_BCR					0
 #define GCC_USB_HS_BCR					1
@@ -162,5 +164,6 @@
 #define GCC_PCIE_0_LINK_DOWN_BCR			11
 #define GCC_PCIEPHY_0_PHY_BCR				12
 #define GCC_EMAC_BCR					13
+#define GCC_WDSP_RESTART				14
 
 #endif
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 2/6] clk: qcom: Add WCSS gcc clock control for QCS404

[Govind Singh]

Add support for the WCSS QDSP gcc clock control used on qcs404 based devices.
This would allow wcss remoteproc driver to control the required gcc clocks to
bring the subsystem out of reset.

Signed-off-by: Govind Singh <govinds@codeaurora.org>
---
 drivers/clk/qcom/gcc-qcs404.c | 37 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/gcc-qcs404.c b/drivers/clk/qcom/gcc-qcs404.c
index 6d1387e..1b38b4e 100644
--- a/drivers/clk/qcom/gcc-qcs404.c
+++ b/drivers/clk/qcom/gcc-qcs404.c
@@ -2505,6 +2505,32 @@ enum {
 	},
 };
 
+static struct clk_branch gcc_wdsp_q6ss_ahbs_clk = {
+	.halt_reg = 0x1e004,
+	.halt_check = BRANCH_HALT,
+	.clkr = {
+		.enable_reg = 0x1e004,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "gcc_wdsp_q6ss_ahbs_clk",
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
+static struct clk_branch gcc_wdsp_q6ss_axim_clk = {
+	.halt_reg = 0x1e008,
+	.halt_check = BRANCH_HALT,
+	.clkr = {
+		.enable_reg = 0x1e008,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "gcc_wdsp_q6ss_axim_clk",
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
 static struct clk_regmap *gcc_qcs404_clocks[] = {
 	[APSS_AHB_CLK_SRC] = &apss_ahb_clk_src.clkr,
 	[BLSP1_QUP0_I2C_APPS_CLK_SRC] = &blsp1_qup0_i2c_apps_clk_src.clkr,
@@ -2642,6 +2668,9 @@ enum {
 	[GCC_QDSS_DAP_CLK] = &gcc_qdss_dap_clk.clkr,
 	[GCC_DCC_CLK] = &gcc_dcc_clk.clkr,
 	[GCC_DCC_XO_CLK] = &gcc_dcc_xo_clk.clkr,
+	[GCC_WCSS_Q6_AHB_CBCR_CLK] = NULL,
+	[GCC_WCSS_Q6_AXIM_CBCR_CLK] =  NULL,
+
 };
 
 static const struct qcom_reset_map gcc_qcs404_resets[] = {
@@ -2659,6 +2688,7 @@ enum {
 	[GCC_PCIE_0_LINK_DOWN_BCR] = {0x3E038},
 	[GCC_PCIEPHY_0_PHY_BCR] = {0x3E03C},
 	[GCC_EMAC_BCR] = {0x4E000},
+	[GCC_WDSP_RESTART] = {0x19000},
 };
 
 static const struct regmap_config gcc_qcs404_regmap_config = {
@@ -2669,7 +2699,7 @@ enum {
 	.fast_io	= true,
 };
 
-static const struct qcom_cc_desc gcc_qcs404_desc = {
+static struct qcom_cc_desc gcc_qcs404_desc = {
 	.config = &gcc_qcs404_regmap_config,
 	.clks = gcc_qcs404_clocks,
 	.num_clks = ARRAY_SIZE(gcc_qcs404_clocks),
@@ -2702,6 +2732,11 @@ static int gcc_qcs404_probe(struct platform_device *pdev)
 	clk_prepare_enable(apss_ahb_clk_src.clkr.hw.clk);
 	clk_prepare_enable(gpll0_ao_out_main.clkr.hw.clk);
 
+	if (of_property_read_bool(pdev->dev.of_node, "qcom,wcss-protected")) {
+		gcc_qcs404_clocks[GCC_WCSS_Q6_AHB_CBCR_CLK] = &gcc_wdsp_q6ss_ahbs_clk.clkr;
+		gcc_qcs404_clocks[GCC_WCSS_Q6_AXIM_CBCR_CLK] = &gcc_wdsp_q6ss_axim_clk.clkr;
+	}
+
 	return qcom_cc_really_probe(pdev, &gcc_qcs404_desc, regmap);
 }
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 3/6] dt-bindings: clock: qcom: Introduce QCOM WCSS Q6DSP clock bindings

[Govind Singh]

Add device tree bindings for WiFi QDSP subsystem clock controls
found in OCS405 soc.

Signed-off-by: Govind Singh <govinds@codeaurora.org>
---
 .../devicetree/bindings/clock/qcom,wcsscc.txt      | 26 ++++++++++++++++++++++
 include/dt-bindings/clock/qcom,wcss-qcs404.h       | 24 ++++++++++++++++++++
 2 files changed, 50 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/qcom,wcsscc.txt
 create mode 100644 include/dt-bindings/clock/qcom,wcss-qcs404.h

diff --git a/Documentation/devicetree/bindings/clock/qcom,wcsscc.txt b/Documentation/devicetree/bindings/clock/qcom,wcsscc.txt
new file mode 100644
index 0000000..2b19ef0
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/qcom,wcsscc.txt
@@ -0,0 +1,26 @@
+Qualcomm WCSS Clock Controller Binding
+-----------------------------------------------
+
+Required properties :
+- compatible		: shall contain "qcom,qcs404-wcsscc"
+- #clock-cells		: from common clock binding, shall contain 1.
+- reg			: shall contain base register address and size,
+			  in the order
+			Index-0 maps to WCSS_Q6SSTOP clocks register region
+			Index-1 maps to WCSS_TCSR register region
+			Index-2 maps to WCSS_QDSP6SS register region
+
+Optional properties :
+- reg-names	: register names of WCSS domain
+		 "wcss_q6sstop", "wcnss_tcsr", "wcss_qdsp6ss".
+
+Example:
+The below node has to be defined in the cases where the WCSS peripheral loader
+would bring the subsystem out of reset.
+
+	clock_wcsscc: qcom,wcsscc@7000000 {
+		compatible = "qcom,qcs404-wcsscc";
+		reg = <0x07500000 0x4e000>, <0x07550000 0x8012>, <0x07400000 0x104>;
+		reg-names = "wcss_q6sstop", "wcnss_tcsr", "wcss_qdsp6ss";
+		#clock-cells = <1>;
+	};
diff --git a/include/dt-bindings/clock/qcom,wcss-qcs404.h b/include/dt-bindings/clock/qcom,wcss-qcs404.h
new file mode 100644
index 0000000..45dd659
--- /dev/null
+++ b/include/dt-bindings/clock/qcom,wcss-qcs404.h
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+#ifndef _DT_BINDINGS_CLK_WCSS_QCS404_H
+#define _DT_BINDINGS_CLK_WCSS_QCS404_H
+
+#define WCSS_AHBFABRIC_CBCR_CLK			0
+#define WCSS_AHBS_CBCR_CLK			1
+#define WCSS_TCM_CBCR_CLK			2
+#define WCSS_AHBM_CBCR_CLK			3
+#define WCSS_AXIM_CBCR_CLK			4
+#define WCSS_BCR_CBCR_CLK			5
+#define WCSS_LCC_CBCR_CLK			6
+#define WCSS_QDSP6SS_XO_CBCR_CLK		7
+#define WCSS_QDSP6SS_SLEEP_CBCR_CLK		8
+#define WCSS_QDSP6SS_GFMMUX_CLK			9
+
+#define Q6SSTOP_QDSP6SS_RESET			0
+#define Q6SSTOP_QDSP6SS_CORE_RESET		1
+#define Q6SSTOP_QDSP6SS_BUS_RESET		2
+#define Q6SSTOP_BCR_RESET			3
+#endif
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 4/6] clk: qcom: Add WCSS Q6DSP clock controller for QCS404

[Govind Singh]

Add support for the WCSS QDSP clock control used on qcs404
based devices. This would allow wcss remoteproc driver to
control the required WCSS QDSP clock/reset controls to
bring the subsystem out of reset and shutdown the WCSS QDSP.

Signed-off-by: Govind Singh <govinds@codeaurora.org>
---
 drivers/clk/qcom/Kconfig         |   9 ++
 drivers/clk/qcom/Makefile        |   1 +
 drivers/clk/qcom/wcsscc-qcs404.c | 290 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 300 insertions(+)
 create mode 100644 drivers/clk/qcom/wcsscc-qcs404.c

diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index 5cdcb37..9ffc003 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -253,6 +253,15 @@ config QCS_GCC_404
 	 Say Y if you want to use peripheral devices such as UART, SPI, I2C,
 	 USB, SD/eMMC, PCIe, etc.
 
+config QCS_WCSS_404
+	tristate "QCS404 WCSS Clock Controller"
+	depends on COMMON_CLK_QCOM
+	select QCS_GCC_404
+	help
+	  Support for the WCSS clock controller on QCS404 devices.
+	  Say Y if you want to use the WCSS branch clocks of the WCSS clock
+	  controller to reset the WCSS subsystem.
+
 config SDM_GCC_845
 	tristate "SDM845 Global Clock Controller"
 	select QCOM_GDSC
diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
index 0a4790e..d258d4f 100644
--- a/drivers/clk/qcom/Makefile
+++ b/drivers/clk/qcom/Makefile
@@ -44,6 +44,7 @@ obj-$(CONFIG_QCOM_CLK_RPMH) += clk-rpmh.o
 obj-$(CONFIG_QCOM_CLK_SMD_RPM) += clk-smd-rpm.o
 obj-$(CONFIG_SDM_DISPCC_845) += dispcc-sdm845.o
 obj-$(CONFIG_QCS_GCC_404) += gcc-qcs404.o
+obj-$(CONFIG_QCS_WCSS_404) += wcsscc-qcs404.o
 obj-$(CONFIG_SDM_GCC_845) += gcc-sdm845.o
 obj-$(CONFIG_SDM_VIDEOCC_845) += videocc-sdm845.o
 obj-$(CONFIG_SPMI_PMIC_CLKDIV) += clk-spmi-pmic-div.o
diff --git a/drivers/clk/qcom/wcsscc-qcs404.c b/drivers/clk/qcom/wcsscc-qcs404.c
new file mode 100644
index 0000000..9a0c864
--- /dev/null
+++ b/drivers/clk/qcom/wcsscc-qcs404.c
@@ -0,0 +1,290 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/bitops.h>
+#include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+
+#include <dt-bindings/clock/qcom,wcss-qcs404.h>
+#include <linux/reset-controller.h>
+
+#include "clk-regmap.h"
+#include "clk-branch.h"
+#include "common.h"
+#include "reset.h"
+
+/* Q6SSTOP clocks */
+static struct clk_branch lcc_ahbfabric_cbc_clk = {
+	.halt_reg = 0x1b004,
+	.halt_check = BRANCH_VOTED,
+	.clkr = {
+		.enable_reg = 0x1b004,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "lcc_ahbfabric_cbc_clk",
+			.ops = &clk_branch2_ops,
+			.flags = CLK_IGNORE_UNUSED,
+		},
+	},
+};
+
+static struct clk_branch lcc_q6ss_ahbs_cbc_clk = {
+	.halt_reg = 0x22000,
+	.halt_check = BRANCH_VOTED,
+	.clkr = {
+		.enable_reg = 0x22000,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "lcc_q6ss_ahbs_cbc_clk",
+			.ops = &clk_branch2_ops,
+			.flags = CLK_IGNORE_UNUSED,
+		},
+	},
+};
+
+static struct clk_branch lcc_q6ss_tcm_slave_cbc_clk = {
+	.halt_reg = 0x1c000,
+	.halt_check = BRANCH_VOTED,
+	.clkr = {
+		.enable_reg = 0x1c000,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "lcc_q6ss_tcm_slave_cbc_clk",
+			.ops = &clk_branch2_ops,
+			.flags = CLK_IGNORE_UNUSED,
+		},
+	},
+};
+
+static struct clk_branch lcc_q6ss_ahbm_cbc_clk = {
+	.halt_reg = 0x22004,
+	.halt_check = BRANCH_VOTED,
+	.clkr = {
+		.enable_reg = 0x22004,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "lcc_q6ss_ahbm_cbc_clk",
+			.ops = &clk_branch2_ops,
+			.flags = CLK_IGNORE_UNUSED,
+		},
+	},
+};
+
+static struct clk_branch lcc_q6ss_axim_cbc_clk = {
+	.halt_reg = 0x1c004,
+	.halt_check = BRANCH_VOTED,
+	.clkr = {
+		.enable_reg = 0x1c004,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "lcc_q6ss_axim_cbc_clk",
+			.ops = &clk_branch2_ops,
+			.flags = CLK_IGNORE_UNUSED,
+		},
+	},
+};
+
+static struct clk_branch lcc_q6ss_bcr_sleep_clk = {
+	.halt_reg = 0x6004,
+	.halt_check = BRANCH_VOTED,
+	.clkr = {
+		.enable_reg = 0x6004,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "lcc_q6ss_bcr_sleep_clk",
+			.ops = &clk_branch2_ops,
+			.flags = CLK_IGNORE_UNUSED,
+		},
+	},
+};
+
+/* TCSR clock */
+static struct clk_branch wcss_lcc_csr_cbcr_clk = {
+	.halt_reg = 0x8008,
+	.halt_check = BRANCH_VOTED,
+	.clkr = {
+		.enable_reg = 0x8008,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "wcss_lcc_csr_cbcr_clk",
+			.ops = &clk_branch2_ops,
+			.flags = CLK_IGNORE_UNUSED,
+		},
+	},
+};
+
+/* Q6SSTOP_QDSP6SS clock */
+static struct clk_branch q6ss_xo_clk = {
+	.halt_reg = 0x38,
+	.halt_check = BRANCH_HALT_SKIP,
+	.clkr = {
+		.enable_reg = 0x38,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "q6ss_xo_clk",
+			.ops = &clk_branch2_ops,
+			.flags = CLK_IGNORE_UNUSED,
+		},
+	},
+};
+
+static struct clk_branch q6ss_slp_clk = {
+	.halt_reg = 0x3c,
+	.halt_check = BRANCH_HALT_SKIP,
+	.clkr = {
+		.enable_reg = 0x3c,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "q6ss_slp_clk",
+			.ops = &clk_branch2_ops,
+			.flags = CLK_IGNORE_UNUSED,
+		},
+	},
+};
+
+static struct clk_branch q6sstop_q6ss_gfmux_clk_src = {
+	.halt_reg = 0x20,
+	.halt_check = BRANCH_HALT_SKIP,
+	.clkr = {
+		.enable_reg = 0x20,
+		.enable_mask = (BIT(1) | BIT(3) | BIT(8)),
+		.hw.init = &(struct clk_init_data){
+			.name = "q6sstop_q6ss_gfmux_clk_src",
+			.ops = &clk_branch2_ops,
+			.flags = CLK_IGNORE_UNUSED,
+		},
+	},
+};
+
+static struct regmap_config wcss_regmap_config = {
+	.reg_bits	= 32,
+	.reg_stride	= 4,
+	.val_bits	= 32,
+	.fast_io	= true,
+};
+
+static struct clk_regmap *wcss_q6sstop_qcs405_clocks[] = {
+	[WCSS_AHBFABRIC_CBCR_CLK] = &lcc_ahbfabric_cbc_clk.clkr,
+	[WCSS_AHBS_CBCR_CLK] = &lcc_q6ss_ahbs_cbc_clk.clkr,
+	[WCSS_TCM_CBCR_CLK] = &lcc_q6ss_tcm_slave_cbc_clk.clkr,
+	[WCSS_AHBM_CBCR_CLK] = &lcc_q6ss_ahbm_cbc_clk.clkr,
+	[WCSS_AXIM_CBCR_CLK] = &lcc_q6ss_axim_cbc_clk.clkr,
+	[WCSS_BCR_CBCR_CLK] = &lcc_q6ss_bcr_sleep_clk.clkr,
+};
+
+static const struct qcom_reset_map qdsp6ss_qcs405_resets[] = {
+	[Q6SSTOP_QDSP6SS_RESET] = {0x14, 0},
+	[Q6SSTOP_QDSP6SS_CORE_RESET] = {0x14, 1},
+	[Q6SSTOP_QDSP6SS_BUS_RESET] = {0x14, 2},
+};
+
+static const struct qcom_reset_map q6sstop_qcs405_resets[] = {
+	[Q6SSTOP_BCR_RESET] = {0x6000},
+};
+
+static const struct qcom_cc_desc wcss_q6sstop_qcs405_desc = {
+	.config = &wcss_regmap_config,
+	.clks = wcss_q6sstop_qcs405_clocks,
+	.num_clks = ARRAY_SIZE(wcss_q6sstop_qcs405_clocks),
+	.resets = q6sstop_qcs405_resets,
+	.num_resets = ARRAY_SIZE(q6sstop_qcs405_resets),
+};
+
+static struct clk_regmap *wcnss_tcsr_qcs405_clocks[] = {
+	[WCSS_LCC_CBCR_CLK] = &wcss_lcc_csr_cbcr_clk.clkr,
+};
+
+static const struct qcom_cc_desc wcnss_tcsr_qcs405_desc = {
+	.config = &wcss_regmap_config,
+	.clks = wcnss_tcsr_qcs405_clocks,
+	.num_clks = ARRAY_SIZE(wcnss_tcsr_qcs405_clocks),
+};
+
+static struct clk_regmap *wcnss_qdsp6ss_qcs405_clocks[] = {
+	[WCSS_QDSP6SS_XO_CBCR_CLK] = &q6ss_xo_clk.clkr,
+	[WCSS_QDSP6SS_SLEEP_CBCR_CLK] = &q6ss_slp_clk.clkr,
+	[WCSS_QDSP6SS_GFMMUX_CLK] = &q6sstop_q6ss_gfmux_clk_src.clkr,
+};
+
+static const struct qcom_cc_desc wcnss_qdsp6ss_qcs405_desc = {
+	.config = &wcss_regmap_config,
+	.clks = wcnss_qdsp6ss_qcs405_clocks,
+	.num_clks = ARRAY_SIZE(wcnss_qdsp6ss_qcs405_clocks),
+	.resets = qdsp6ss_qcs405_resets,
+	.num_resets = ARRAY_SIZE(qdsp6ss_qcs405_resets),
+};
+
+static int wcss_clocks_qcs405_probe(struct platform_device *pdev, int index,
+				    const struct qcom_cc_desc *desc)
+{
+	struct regmap *regmap;
+	struct resource *res;
+	void __iomem *base;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, index);
+	base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(base))
+		return -ENOMEM;
+
+	regmap = devm_regmap_init_mmio(&pdev->dev, base, desc->config);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	return qcom_cc_really_probe(pdev, desc, regmap);
+}
+
+/* WCSS CC clock controller */
+static const struct of_device_id wcss_cc_qcs405_match_table[] = {
+	{ .compatible = "qcom,qcs405-wcsscc" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, wcss_cc_qcs405_match_table);
+
+static int wcss_cc_qcs405_probe(struct platform_device *pdev)
+{
+	const struct qcom_cc_desc *desc;
+	int ret;
+
+	wcss_regmap_config.name = "wcss_q6sstop";
+	desc = &wcss_q6sstop_qcs405_desc;
+
+	ret = wcss_clocks_qcs405_probe(pdev, 0, desc);
+	if (ret)
+		return ret;
+
+	wcss_regmap_config.name = "wcnss_tcsr";
+	desc = &wcnss_tcsr_qcs405_desc;
+
+	ret = wcss_clocks_qcs405_probe(pdev, 1, desc);
+	if (ret)
+		return ret;
+
+	wcss_regmap_config.name = "wcss_qdsp6ss";
+	desc = &wcnss_qdsp6ss_qcs405_desc;
+
+	return wcss_clocks_qcs405_probe(pdev, 2, desc);
+}
+
+static struct platform_driver wcss_cc_qcs405_driver = {
+	.probe		= wcss_cc_qcs405_probe,
+	.driver		= {
+		.name	= "qcs405-wcsscc",
+		.of_match_table = wcss_cc_qcs405_match_table,
+	},
+};
+
+static int __init wcss_cc_qcs405_init(void)
+{
+	return platform_driver_register(&wcss_cc_qcs405_driver);
+}
+subsys_initcall(wcss_cc_qcs405_init);
+
+MODULE_LICENSE("GPL v2");
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 5/6] remoteproc: qcom: wcss: populate hardcoded param using driver data

[Govind Singh]

Q6 based WiFi fw loading is supported across
different targets, ex: IPQ8074/QCS404. In order to
support different fw names/pas id etc, populate
hardcoded param using driver data.

Signed-off-by: Govind Singh <govinds@codeaurora.org>
---
 drivers/remoteproc/qcom_q6v5_wcss.c | 36 ++++++++++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_wcss.c b/drivers/remoteproc/qcom_q6v5_wcss.c
index f93e1e4..24e276d 100644
--- a/drivers/remoteproc/qcom_q6v5_wcss.c
+++ b/drivers/remoteproc/qcom_q6v5_wcss.c
@@ -8,6 +8,7 @@
 #include <linux/kernel.h>
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
+#include <linux/of_device.h>
 #include <linux/of_reserved_mem.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
@@ -16,7 +17,6 @@
 #include "qcom_common.h"
 #include "qcom_q6v5.h"
 
-#define WCSS_CRASH_REASON		421
 
 /* Q6SS Register Offsets */
 #define Q6SS_RESET_REG		0x014
@@ -70,6 +70,14 @@
 #define TCSR_WCSS_CLK_MASK	0x1F
 #define TCSR_WCSS_CLK_ENABLE	0x14
 
+struct wcss_data {
+	void (*pas_handover)(struct qcom_q6v5 *q6v5);
+	const char *firmware_name;
+	int crash_reason_smem;
+	int version;
+	int pas_id;
+};
+
 struct q6v5_wcss {
 	struct device *dev;
 
@@ -91,6 +99,10 @@ struct q6v5_wcss {
 	phys_addr_t mem_reloc;
 	void *mem_region;
 	size_t mem_size;
+
+	int crash_reason_smem;
+	int pas_id;
+	int version;
 };
 
 static int q6v5_wcss_reset(struct q6v5_wcss *wcss)
@@ -527,12 +539,17 @@ static int q6v5_alloc_memory_region(struct q6v5_wcss *wcss)
 
 static int q6v5_wcss_probe(struct platform_device *pdev)
 {
+	const struct wcss_data *desc;
 	struct q6v5_wcss *wcss;
 	struct rproc *rproc;
 	int ret;
 
+	desc = of_device_get_match_data(&pdev->dev);
+	if (!desc)
+		return -EINVAL;
+
 	rproc = rproc_alloc(&pdev->dev, pdev->name, &q6v5_wcss_ops,
-			    "IPQ8074/q6_fw.mdt", sizeof(*wcss));
+			    desc->firmware_name, sizeof(*wcss));
 	if (!rproc) {
 		dev_err(&pdev->dev, "failed to allocate rproc\n");
 		return -ENOMEM;
@@ -540,6 +557,9 @@ static int q6v5_wcss_probe(struct platform_device *pdev)
 
 	wcss = rproc->priv;
 	wcss->dev = &pdev->dev;
+	wcss->pas_id = desc->pas_id;
+	wcss->version = desc->version;
+	wcss->crash_reason_smem = desc->crash_reason_smem;
 
 	ret = q6v5_wcss_init_mmio(wcss, pdev);
 	if (ret)
@@ -553,7 +573,8 @@ static int q6v5_wcss_probe(struct platform_device *pdev)
 	if (ret)
 		goto free_rproc;
 
-	ret = qcom_q6v5_init(&wcss->q6v5, pdev, rproc, WCSS_CRASH_REASON, NULL);
+	ret = qcom_q6v5_init(&wcss->q6v5, pdev, rproc, desc->crash_reason_smem,
+			     desc->pas_handover);
 	if (ret)
 		goto free_rproc;
 
@@ -581,8 +602,15 @@ static int q6v5_wcss_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct wcss_data wcss_ipq8074_res_init = {
+	.firmware_name = "IPQ8074/q6_fw.mdt",
+	.crash_reason_smem = 421,
+	.pas_handover = NULL,
+};
+
 static const struct of_device_id q6v5_wcss_of_match[] = {
-	{ .compatible = "qcom,ipq8074-wcss-pil" },
+	{ .compatible = "qcom,ipq8074-wcss-pil", .data = &wcss_ipq8074_res_init },
+
 	{ },
 };
 MODULE_DEVICE_TABLE(of, q6v5_wcss_of_match);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 6/6] remoteproc: qcom: wcss: Add non pas wcss Q6 support for QCS404

[Govind Singh]

Add non PAS WCSS remoteproc driver support for QCS404 SOC.
Add WCSS q6 bootup and shutdown sequence handled from
Application Processor SubSystem(APSS).

Signed-off-by: Govind Singh <govinds@codeaurora.org>
---
 .../devicetree/bindings/remoteproc/qcom,q6v5.txt   |   1 +
 drivers/remoteproc/qcom_q6v5_wcss.c                | 638 +++++++++++++++++++--
 2 files changed, 601 insertions(+), 38 deletions(-)

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
index 601dd9f..cc83832 100644
--- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
@@ -13,6 +13,7 @@ on the Qualcomm Hexagon core.
 		    "qcom,msm8974-mss-pil"
 		    "qcom,msm8996-mss-pil"
 		    "qcom,sdm845-mss-pil"
+		    "qcom,qcs404-wcss-non-pas"
 
 - reg:
 	Usage: required
diff --git a/drivers/remoteproc/qcom_q6v5_wcss.c b/drivers/remoteproc/qcom_q6v5_wcss.c
index 24e276d..5760554 100644
--- a/drivers/remoteproc/qcom_q6v5_wcss.c
+++ b/drivers/remoteproc/qcom_q6v5_wcss.c
@@ -4,14 +4,19 @@
  * Copyright (C) 2014 Sony Mobile Communications AB
  * Copyright (c) 2012-2018, The Linux Foundation. All rights reserved.
  */
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/io.h>
 #include <linux/iopoll.h>
 #include <linux/kernel.h>
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
+#include <linux/of_address.h>
 #include <linux/of_device.h>
 #include <linux/of_reserved_mem.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
 #include <linux/reset.h>
 #include <linux/soc/qcom/mdt_loader.h>
 #include "qcom_common.h"
@@ -19,10 +24,13 @@
 
 
 /* Q6SS Register Offsets */
-#define Q6SS_RESET_REG		0x014
+#define Q6SS_RESET_REG			0x014
 #define Q6SS_GFMUX_CTL_REG		0x020
 #define Q6SS_PWR_CTL_REG		0x030
 #define Q6SS_MEM_PWR_CTL		0x0B0
+#define Q6SS_STRAP_ACC			0x110
+#define Q6SS_CGC_OVERRIDE		0x034
+#define Q6SS_BCR_REG			0x6000
 
 /* AXI Halt Register Offsets */
 #define AXI_HALTREQ_REG			0x0
@@ -36,6 +44,9 @@
 #define Q6SS_CORE_ARES			BIT(1)
 #define Q6SS_BUS_ARES_ENABLE		BIT(2)
 
+/* Q6SS_BRC_RESET */
+#define Q6SS_BRC_BLK_ARES		BIT(0)
+
 /* Q6SS_GFMUX_CTL */
 #define Q6SS_CLK_ENABLE			BIT(1)
 
@@ -44,14 +55,15 @@
 #define Q6SS_SLP_RET_N			BIT(19)
 #define Q6SS_CLAMP_IO			BIT(20)
 #define QDSS_BHS_ON			BIT(21)
+#define QDSS_Q6_MEMORIES		GENMASK(15, 0)
 
 /* Q6SS parameters */
-#define Q6SS_LDO_BYP		BIT(25)
-#define Q6SS_BHS_ON		BIT(24)
-#define Q6SS_CLAMP_WL		BIT(21)
+#define Q6SS_LDO_BYP			BIT(25)
+#define Q6SS_BHS_ON			BIT(24)
+#define Q6SS_CLAMP_WL			BIT(21)
 #define Q6SS_CLAMP_QMC_MEM		BIT(22)
 #define HALT_CHECK_MAX_LOOPS		200
-#define Q6SS_XO_CBCR		GENMASK(5, 3)
+#define Q6SS_XO_CBCR			GENMASK(5, 3)
 
 /* Q6SS config/status registers */
 #define TCSR_GLOBAL_CFG0	0x0
@@ -70,12 +82,23 @@
 #define TCSR_WCSS_CLK_MASK	0x1F
 #define TCSR_WCSS_CLK_ENABLE	0x14
 
+enum {
+	WCSS_IPQ8074,
+	WCSS_QCS404,
+};
+
 struct wcss_data {
-	void (*pas_handover)(struct qcom_q6v5 *q6v5);
 	const char *firmware_name;
 	int crash_reason_smem;
 	int version;
 	int pas_id;
+	bool has_aggre2_clk;
+	bool aon_reset_required;
+	const char *ssr_name;
+	const char *sysmon_name;
+	int ssctl_id;
+	const struct rproc_ops *ops;
+	void (*pas_handover)(struct qcom_q6v5 *q6v5);
 };
 
 struct q6v5_wcss {
@@ -83,12 +106,37 @@ struct q6v5_wcss {
 
 	void __iomem *reg_base;
 	void __iomem *rmb_base;
+	void __iomem *q6stop_base;
 
 	struct regmap *halt_map;
 	u32 halt_q6;
 	u32 halt_wcss;
 	u32 halt_nc;
 
+	struct clk *xo;
+	struct clk *aggre2_clk;
+	struct clk *ahbfabric_cbcr_clk;
+	struct clk *gcc_abhs_cbcr;
+	struct clk *gcc_axim_cbcr;
+	struct clk *lcc_csr_cbcr;
+	struct clk *ahbs_cbcr;
+	struct clk *tcm_slave_cbcr;
+	struct clk *qdsp6ss_abhm_cbcr;
+	struct clk *qdsp6ss_sleep_cbcr;
+	struct clk *qdsp6ss_axim_cbcr;
+	struct clk *qdsp6ss_xo_cbcr;
+	struct clk *qdsp6ss_core_gfmux;
+	struct clk *wcss_bcr_cbcr;
+	struct regulator *cx_supply;
+	struct regulator *px_supply;
+
+	bool has_aggre2_clk;
+	bool aon_reset_required;
+
+	struct qcom_rproc_glink glink_subdev;
+	struct qcom_rproc_ssr ssr_subdev;
+	struct qcom_sysmon *sysmon;
+
 	struct reset_control *wcss_aon_reset;
 	struct reset_control *wcss_reset;
 	struct reset_control *wcss_q6_reset;
@@ -99,7 +147,6 @@ struct q6v5_wcss {
 	phys_addr_t mem_reloc;
 	void *mem_region;
 	size_t mem_size;
-
 	int crash_reason_smem;
 	int pas_id;
 	int version;
@@ -245,6 +292,214 @@ static int q6v5_wcss_start(struct rproc *rproc)
 	return ret;
 }
 
+static int q6v5_wcss_qcs404_power_on(struct q6v5_wcss *wcss)
+{
+	unsigned long val;
+	int ret, idx;
+
+	/* Toggle the restart */
+	reset_control_assert(wcss->wcss_reset);
+	usleep_range(200, 300);
+	reset_control_deassert(wcss->wcss_reset);
+	usleep_range(200, 300);
+
+	/* Enable GCC_WDSP_Q6SS_AHBS_CBCR clock */
+	ret = clk_prepare_enable(wcss->gcc_abhs_cbcr);
+	if (ret)
+		return ret;
+
+	/* Remove reset to the WCNSS QDSP6SS */
+	val = readl(wcss->q6stop_base + Q6SS_BCR_REG);
+	val &= ~Q6SS_BRC_BLK_ARES;
+	writel(val, wcss->q6stop_base + Q6SS_BCR_REG);
+
+	/* Enable Q6SSTOP_AHBFABRIC_CBCR clock */
+	ret = clk_prepare_enable(wcss->ahbfabric_cbcr_clk);
+	if (ret)
+		goto disable_gcc_abhs_cbcr_clk;
+
+	/* Enable the LCCCSR CBC clock, Q6SSTOP_Q6SSTOP_LCC_CSR_CBCR clock */
+	ret = clk_prepare_enable(wcss->lcc_csr_cbcr);
+	if (ret)
+		goto disable_ahbfabric_cbcr_clk;
+
+	/* Enable the Q6AHBS CBC, Q6SSTOP_Q6SS_AHBS_CBCR clock */
+	ret = clk_prepare_enable(wcss->ahbs_cbcr);
+	if (ret)
+		goto disable_csr_cbcr_clk;
+
+	/* Enable the TCM slave CBC, Q6SSTOP_Q6SS_TCM_SLAVE_CBCR clock */
+	ret = clk_prepare_enable(wcss->tcm_slave_cbcr);
+	if (ret)
+		goto disable_ahbs_cbcr_clk;
+
+	/* Enable the Q6SS AHB master CBC, Q6SSTOP_Q6SS_AHBM_CBCR clock */
+	ret = clk_prepare_enable(wcss->qdsp6ss_abhm_cbcr);
+	if (ret)
+		goto disable_tcm_slave_cbcr_clk;
+
+	/* Enable the Q6SS AXI master CBC, Q6SSTOP_Q6SS_AXIM_CBCR clock */
+	ret = clk_prepare_enable(wcss->qdsp6ss_axim_cbcr);
+	if (ret)
+		goto disable_abhm_cbcr_clk;
+
+	/* Enable the Q6SS XO CBC */
+	ret = clk_prepare_enable(wcss->qdsp6ss_xo_cbcr);
+	if (ret)
+		goto disable_axim_cbcr_clk;
+
+	writel(0, wcss->reg_base + Q6SS_CGC_OVERRIDE);
+
+	/* Enable QDSP6 sleep clock clock */
+	ret = clk_prepare_enable(wcss->qdsp6ss_sleep_cbcr);
+	if (ret)
+		goto disable_xo_cbcr_clk;
+
+	/* Enable the Enable the Q6 AXI clock, GCC_WDSP_Q6SS_AXIM_CBCR*/
+	ret = clk_prepare_enable(wcss->gcc_axim_cbcr);
+	if (ret)
+		goto disable_sleep_cbcr_clk;
+
+	/* Assert resets, stop core */
+	val = readl(wcss->reg_base + Q6SS_RESET_REG);
+	val |= Q6SS_CORE_ARES | Q6SS_BUS_ARES_ENABLE | Q6SS_STOP_CORE;
+	writel(val, wcss->reg_base + Q6SS_RESET_REG);
+
+	/* Program the QDSP6SS PWR_CTL register */
+	writel(0x01700000, wcss->reg_base + Q6SS_PWR_CTL_REG);
+
+	writel(0x03700000, wcss->reg_base + Q6SS_PWR_CTL_REG);
+
+	writel(0x03300000, wcss->reg_base + Q6SS_PWR_CTL_REG);
+
+	writel(0x033C0000, wcss->reg_base + Q6SS_PWR_CTL_REG);
+
+	/*
+	 * Enable memories by turning on the QDSP6 memory foot/head switch, one
+	 * bank at a time to avoid in-rush current
+	 */
+	for (idx = 28; idx >= 0; idx--) {
+		writel((readl(wcss->reg_base + Q6SS_MEM_PWR_CTL) |
+			(1 << idx)), wcss->reg_base + Q6SS_MEM_PWR_CTL);
+	}
+
+	writel(0x031C0000, wcss->reg_base + Q6SS_PWR_CTL_REG);
+	writel(0x030C0000, wcss->reg_base + Q6SS_PWR_CTL_REG);
+
+	val = readl(wcss->reg_base + Q6SS_RESET_REG);
+	val &= ~Q6SS_CORE_ARES;
+	writel(val, wcss->reg_base + Q6SS_RESET_REG);
+
+	/* Enable the Q6 core clock at the GFM, Q6SSTOP_QDSP6SS_GFMUX_CTL */
+	ret = clk_prepare_enable(wcss->qdsp6ss_core_gfmux);
+	if (ret)
+		goto disable_gcc_axim_cbcr_clk;
+
+	/* Enable sleep clock branch needed for BCR circuit */
+	ret = clk_prepare_enable(wcss->wcss_bcr_cbcr);
+	if (ret)
+		goto disable_core_gfmux_clk;
+
+	return 0;
+
+disable_core_gfmux_clk:
+	clk_disable_unprepare(wcss->qdsp6ss_core_gfmux);
+disable_gcc_axim_cbcr_clk:
+	clk_disable_unprepare(wcss->gcc_axim_cbcr);
+disable_sleep_cbcr_clk:
+	clk_disable_unprepare(wcss->qdsp6ss_sleep_cbcr);
+disable_xo_cbcr_clk:
+	clk_disable_unprepare(wcss->qdsp6ss_xo_cbcr);
+disable_axim_cbcr_clk:
+	clk_disable_unprepare(wcss->qdsp6ss_axim_cbcr);
+disable_abhm_cbcr_clk:
+	clk_disable_unprepare(wcss->qdsp6ss_abhm_cbcr);
+disable_tcm_slave_cbcr_clk:
+	clk_disable_unprepare(wcss->tcm_slave_cbcr);
+disable_ahbs_cbcr_clk:
+	clk_disable_unprepare(wcss->ahbs_cbcr);
+disable_csr_cbcr_clk:
+	clk_disable_unprepare(wcss->lcc_csr_cbcr);
+disable_ahbfabric_cbcr_clk:
+	clk_disable_unprepare(wcss->ahbfabric_cbcr_clk);
+disable_gcc_abhs_cbcr_clk:
+	clk_disable_unprepare(wcss->gcc_abhs_cbcr);
+
+	return ret;
+}
+
+static int q6v5_wcss_qcs404_reset(struct q6v5_wcss *wcss)
+{
+	unsigned long val;
+
+	writel(0x80800000, wcss->reg_base + Q6SS_STRAP_ACC);
+
+	/* Start core execution */
+	val = readl(wcss->reg_base + Q6SS_RESET_REG);
+	val &= ~Q6SS_STOP_CORE;
+	writel(val, wcss->reg_base + Q6SS_RESET_REG);
+
+	return 0;
+}
+
+static int q6v5_qcs404_wcss_start(struct rproc *rproc)
+{
+	struct q6v5_wcss *wcss = rproc->priv;
+	int ret;
+
+	ret = clk_prepare_enable(wcss->xo);
+	if (ret)
+		return ret;
+
+	if (wcss->has_aggre2_clk) {
+		ret = clk_prepare_enable(wcss->aggre2_clk);
+		if (ret)
+			goto disable_xo_clk;
+	}
+
+	ret = regulator_enable(wcss->cx_supply);
+	if (ret)
+		goto disable_aggre2_clk;
+
+	ret = regulator_enable(wcss->px_supply);
+	if (ret)
+		goto disable_cx_supply;
+
+	qcom_q6v5_prepare(&wcss->q6v5);
+
+	ret = q6v5_wcss_qcs404_power_on(wcss);
+	if (ret) {
+		dev_err(wcss->dev, "wcss clk_enable failed\n");
+		goto disable_px_supply;
+	}
+
+	writel(rproc->bootaddr >> 4, wcss->reg_base + Q6SS_RST_EVB);
+
+	ret = q6v5_wcss_qcs404_reset(wcss);
+	if (ret)
+		dev_err(wcss->dev, "De-assert QDSP6 out of reset failed\n");
+
+	ret = qcom_q6v5_wait_for_start(&wcss->q6v5, 5 * HZ);
+	if (ret == -ETIMEDOUT) {
+		dev_err(wcss->dev, "start timed out\n");
+		goto disable_px_supply;
+	}
+
+	return 0;
+
+disable_px_supply:
+	regulator_disable(wcss->px_supply);
+disable_cx_supply:
+	regulator_disable(wcss->cx_supply);
+disable_aggre2_clk:
+	if (wcss->has_aggre2_clk)
+		clk_disable_unprepare(wcss->aggre2_clk);
+disable_xo_clk:
+	clk_disable_unprepare(wcss->xo);
+
+	return ret;
+}
+
 static void q6v5_wcss_halt_axi_port(struct q6v5_wcss *wcss,
 				    struct regmap *halt_map,
 				    u32 offset)
@@ -279,6 +534,77 @@ static void q6v5_wcss_halt_axi_port(struct q6v5_wcss *wcss,
 	regmap_write(halt_map, offset + AXI_HALTREQ_REG, 0);
 }
 
+static int q6v5_qcs404_wcss_shutdown(struct q6v5_wcss *wcss)
+{
+	unsigned long val;
+	int ret;
+
+	q6v5_wcss_halt_axi_port(wcss, wcss->halt_map, wcss->halt_wcss);
+
+	/* assert clamps to avoid MX current inrush */
+	val = readl(wcss->reg_base + Q6SS_PWR_CTL_REG);
+	val |= (Q6SS_CLAMP_IO | Q6SS_CLAMP_WL | Q6SS_CLAMP_QMC_MEM);
+	writel(val, wcss->reg_base + Q6SS_PWR_CTL_REG);
+
+	/* Disable memories by turning off memory foot/headswitch */
+	writel((readl(wcss->reg_base + Q6SS_MEM_PWR_CTL) &
+		~QDSS_Q6_MEMORIES),
+		wcss->reg_base + Q6SS_MEM_PWR_CTL);
+
+	/* Clear the BHS_ON bit */
+	val = readl(wcss->reg_base + Q6SS_PWR_CTL_REG);
+	val &= ~Q6SS_BHS_ON;
+	writel(val, wcss->reg_base + Q6SS_PWR_CTL_REG);
+
+	clk_disable_unprepare(wcss->ahbfabric_cbcr_clk);
+	clk_disable_unprepare(wcss->lcc_csr_cbcr);
+	clk_disable_unprepare(wcss->tcm_slave_cbcr);
+	clk_disable_unprepare(wcss->qdsp6ss_abhm_cbcr);
+	clk_disable_unprepare(wcss->qdsp6ss_axim_cbcr);
+	clk_disable_unprepare(wcss->qdsp6ss_xo_cbcr);
+	clk_disable_unprepare(wcss->ahbs_cbcr);
+	clk_disable_unprepare(wcss->wcss_bcr_cbcr);
+	clk_disable_unprepare(wcss->qdsp6ss_core_gfmux);
+	clk_disable_unprepare(wcss->gcc_abhs_cbcr);
+
+	ret = reset_control_assert(wcss->wcss_reset);
+	if (ret) {
+		dev_err(wcss->dev, "wcss_reset failed\n");
+		return ret;
+	}
+	usleep_range(200, 300);
+
+	ret = reset_control_deassert(wcss->wcss_reset);
+	if (ret) {
+		dev_err(wcss->dev, "wcss_reset failed\n");
+		return ret;
+	}
+	usleep_range(200, 300);
+
+	clk_disable_unprepare(wcss->gcc_axim_cbcr);
+
+	return 0;
+}
+
+static int q6v5_qcs404_wcss_stop(struct rproc *rproc)
+{
+	struct q6v5_wcss *wcss = rproc->priv;
+	int ret;
+
+	/* WCSS powerdown */
+	ret = qcom_q6v5_request_stop(&wcss->q6v5);
+	if (ret == -ETIMEDOUT)
+		dev_err(wcss->dev, "timed out on wait\n");
+
+	ret = q6v5_qcs404_wcss_shutdown(wcss);
+	if (ret)
+		return ret;
+
+	qcom_q6v5_unprepare(&wcss->q6v5);
+
+	return 0;
+}
+
 static int q6v5_wcss_powerdown(struct q6v5_wcss *wcss)
 {
 	int ret;
@@ -312,7 +638,8 @@ static int q6v5_wcss_powerdown(struct q6v5_wcss *wcss)
 	}
 
 	/* 6 - De-assert WCSS_AON reset */
-	reset_control_assert(wcss->wcss_aon_reset);
+	if (wcss->aon_reset_required)
+		reset_control_assert(wcss->wcss_aon_reset);
 
 	/* 7 - Disable WCSSAON_CONFIG 13 */
 	val = readl(wcss->rmb_base + SSCAON_CONFIG);
@@ -392,6 +719,17 @@ static int q6v5_q6_powerdown(struct q6v5_wcss *wcss)
 	return 0;
 }
 
+static void q6v5_wcss_pas_handover(struct qcom_q6v5 *q6v5)
+{
+	struct q6v5_wcss *wcss = container_of(q6v5, struct q6v5_wcss, q6v5);
+
+	regulator_disable(wcss->px_supply);
+	regulator_disable(wcss->cx_supply);
+	if (wcss->has_aggre2_clk)
+		clk_disable_unprepare(wcss->aggre2_clk);
+	clk_disable_unprepare(wcss->xo);
+}
+
 static int q6v5_wcss_stop(struct rproc *rproc)
 {
 	struct q6v5_wcss *wcss = rproc->priv;
@@ -439,7 +777,7 @@ static int q6v5_wcss_load(struct rproc *rproc, const struct firmware *fw)
 				     wcss->mem_size, &wcss->mem_reloc);
 }
 
-static const struct rproc_ops q6v5_wcss_ops = {
+static const struct rproc_ops q6v5_wcss_ipq8074_ops = {
 	.start = q6v5_wcss_start,
 	.stop = q6v5_wcss_stop,
 	.da_to_va = q6v5_wcss_da_to_va,
@@ -447,23 +785,33 @@ static int q6v5_wcss_load(struct rproc *rproc, const struct firmware *fw)
 	.get_boot_addr = rproc_elf_get_boot_addr,
 };
 
+static const struct rproc_ops q6v5_wcss_qcs404_ops = {
+	.start = q6v5_qcs404_wcss_start,
+	.stop = q6v5_qcs404_wcss_stop,
+	.da_to_va = q6v5_wcss_da_to_va,
+	.load = q6v5_wcss_load,
+	.get_boot_addr = rproc_elf_get_boot_addr,
+};
+
 static int q6v5_wcss_init_reset(struct q6v5_wcss *wcss)
 {
 	struct device *dev = wcss->dev;
 
-	wcss->wcss_aon_reset = devm_reset_control_get(dev, "wcss_aon_reset");
-	if (IS_ERR(wcss->wcss_aon_reset)) {
-		dev_err(wcss->dev, "unable to acquire wcss_aon_reset\n");
-		return PTR_ERR(wcss->wcss_aon_reset);
+	if (wcss->aon_reset_required) {
+		wcss->wcss_aon_reset = devm_reset_control_get_exclusive(dev, "wcss_aon_reset");
+		if (IS_ERR(wcss->wcss_aon_reset)) {
+			dev_err(wcss->dev, "fail to acquire wcss_aon_reset\n");
+			return PTR_ERR(wcss->wcss_aon_reset);
+		}
 	}
 
-	wcss->wcss_reset = devm_reset_control_get(dev, "wcss_reset");
+	wcss->wcss_reset = devm_reset_control_get_exclusive(dev, "wcss_reset");
 	if (IS_ERR(wcss->wcss_reset)) {
 		dev_err(wcss->dev, "unable to acquire wcss_reset\n");
 		return PTR_ERR(wcss->wcss_reset);
 	}
 
-	wcss->wcss_q6_reset = devm_reset_control_get(dev, "wcss_q6_reset");
+	wcss->wcss_q6_reset = devm_reset_control_get_exclusive(dev, "wcss_q6_reset");
 	if (IS_ERR(wcss->wcss_q6_reset)) {
 		dev_err(wcss->dev, "unable to acquire wcss_q6_reset\n");
 		return PTR_ERR(wcss->wcss_q6_reset);
@@ -475,36 +823,73 @@ static int q6v5_wcss_init_reset(struct q6v5_wcss *wcss)
 static int q6v5_wcss_init_mmio(struct q6v5_wcss *wcss,
 			       struct platform_device *pdev)
 {
+	struct device_node *syscon;
 	struct of_phandle_args args;
 	struct resource *res;
 	int ret;
 
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "qdsp6");
-	wcss->reg_base = devm_ioremap_resource(&pdev->dev, res);
+	wcss->reg_base = devm_ioremap(&pdev->dev, res->start,
+				      resource_size(res));
 	if (IS_ERR(wcss->reg_base))
 		return PTR_ERR(wcss->reg_base);
 
-	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "rmb");
-	wcss->rmb_base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(wcss->rmb_base))
-		return PTR_ERR(wcss->rmb_base);
-
-	ret = of_parse_phandle_with_fixed_args(pdev->dev.of_node,
-					       "qcom,halt-regs", 3, 0, &args);
-	if (ret < 0) {
-		dev_err(&pdev->dev, "failed to parse qcom,halt-regs\n");
-		return -EINVAL;
+	if (wcss->version == WCSS_QCS404) {
+		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+						   "q6stop");
+		if (!res) {
+			dev_err(&pdev->dev, "invalid q6stop_base resource\n");
+			return -EINVAL;
+		}
+
+		wcss->q6stop_base = devm_ioremap(&pdev->dev, res->start,
+						 resource_size(res));
+		if (IS_ERR(wcss->q6stop_base))
+			return PTR_ERR(wcss->q6stop_base);
+
+		syscon = of_parse_phandle(pdev->dev.of_node,
+					  "qcom,halt-regs", 0);
+		if (!syscon) {
+			dev_err(&pdev->dev, "failed to parse qcom,halt-regs\n");
+			return -EINVAL;
+		}
+
+		wcss->halt_map = syscon_node_to_regmap(syscon);
+		of_node_put(syscon);
+		if (IS_ERR(wcss->halt_map))
+			return PTR_ERR(wcss->halt_map);
+
+		ret = of_property_read_u32_index(pdev->dev.of_node,
+						 "qcom,halt-regs",
+						 1, &wcss->halt_wcss);
+		if (ret < 0) {
+			dev_err(&pdev->dev, "no offset in syscon\n");
+			return ret;
+		}
+	} else {
+		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "rmb");
+		wcss->rmb_base = devm_ioremap_resource(&pdev->dev, res);
+		if (IS_ERR(wcss->rmb_base))
+			return PTR_ERR(wcss->rmb_base);
+
+		ret = of_parse_phandle_with_fixed_args(pdev->dev.of_node,
+						       "qcom,halt-regs", 3,
+							0, &args);
+		if (ret < 0) {
+			dev_err(&pdev->dev, "failed to parse qcom,halt-regs\n");
+			return -EINVAL;
+		}
+
+		wcss->halt_map = syscon_node_to_regmap(args.np);
+		of_node_put(args.np);
+		if (IS_ERR(wcss->halt_map))
+			return PTR_ERR(wcss->halt_map);
+
+		wcss->halt_q6 = args.args[0];
+		wcss->halt_wcss = args.args[1];
+		wcss->halt_nc = args.args[2];
 	}
 
-	wcss->halt_map = syscon_node_to_regmap(args.np);
-	of_node_put(args.np);
-	if (IS_ERR(wcss->halt_map))
-		return PTR_ERR(wcss->halt_map);
-
-	wcss->halt_q6 = args.args[0];
-	wcss->halt_wcss = args.args[1];
-	wcss->halt_nc = args.args[2];
-
 	return 0;
 }
 
@@ -537,6 +922,144 @@ static int q6v5_alloc_memory_region(struct q6v5_wcss *wcss)
 	return 0;
 }
 
+static int q6v5_wcss_init_clock(struct q6v5_wcss *wcss)
+{
+	int ret;
+
+	wcss->xo = devm_clk_get(wcss->dev, "xo");
+	if (IS_ERR(wcss->xo)) {
+		ret = PTR_ERR(wcss->xo);
+		if (ret != -EPROBE_DEFER)
+			dev_err(wcss->dev, "failed to get xo clock");
+		return ret;
+	}
+
+	if (wcss->has_aggre2_clk) {
+		wcss->aggre2_clk = devm_clk_get(wcss->dev, "aggre2");
+		if (IS_ERR(wcss->aggre2_clk)) {
+			ret = PTR_ERR(wcss->aggre2_clk);
+			if (ret != -EPROBE_DEFER)
+				dev_err(wcss->dev,
+					"failed to get aggre2 clock");
+			return ret;
+		}
+	}
+
+	wcss->gcc_abhs_cbcr = devm_clk_get(wcss->dev, "gcc_abhs_cbcr");
+	if (IS_ERR(wcss->gcc_abhs_cbcr)) {
+		ret = PTR_ERR(wcss->gcc_abhs_cbcr);
+		if (ret != -EPROBE_DEFER)
+			dev_err(wcss->dev, "failed to get gcc_sway clock\n");
+		return PTR_ERR(wcss->gcc_abhs_cbcr);
+	}
+
+	wcss->gcc_axim_cbcr = devm_clk_get(wcss->dev, "gcc_axim_cbcr");
+	if (IS_ERR(wcss->gcc_axim_cbcr)) {
+		ret = PTR_ERR(wcss->gcc_axim_cbcr);
+		if (ret != -EPROBE_DEFER)
+			dev_err(wcss->dev, "failed to get csr cbcr clk\n");
+		return PTR_ERR(wcss->gcc_axim_cbcr);
+	}
+
+	wcss->ahbfabric_cbcr_clk = devm_clk_get(wcss->dev,
+						"wcss_ahbfabric_cbcr");
+	if (IS_ERR(wcss->ahbfabric_cbcr_clk)) {
+		ret = PTR_ERR(wcss->ahbfabric_cbcr_clk);
+		if (ret != -EPROBE_DEFER)
+			dev_err(wcss->dev, "failed to get gcc_sway clock\n");
+		return PTR_ERR(wcss->ahbfabric_cbcr_clk);
+	}
+
+	wcss->lcc_csr_cbcr = devm_clk_get(wcss->dev, "wcnss_csr_cbcr");
+	if (IS_ERR(wcss->lcc_csr_cbcr)) {
+		ret = PTR_ERR(wcss->lcc_csr_cbcr);
+		if (ret != -EPROBE_DEFER)
+			dev_err(wcss->dev, "failed to get csr cbcr clk\n");
+		return PTR_ERR(wcss->lcc_csr_cbcr);
+	}
+
+	wcss->ahbs_cbcr = devm_clk_get(wcss->dev,
+				       "wcnss_ahbs_cbcr");
+	if (IS_ERR(wcss->ahbs_cbcr)) {
+		ret = PTR_ERR(wcss->ahbs_cbcr);
+		if (ret != -EPROBE_DEFER)
+			dev_err(wcss->dev, "failed to get ahbs_cbcr clk\n");
+		return PTR_ERR(wcss->ahbs_cbcr);
+	}
+
+	wcss->tcm_slave_cbcr = devm_clk_get(wcss->dev,
+					    "wcnss_tcm_slave_cbcr");
+	if (IS_ERR(wcss->tcm_slave_cbcr)) {
+		ret = PTR_ERR(wcss->tcm_slave_cbcr);
+		if (ret != -EPROBE_DEFER)
+			dev_err(wcss->dev, "failed to get tcm cbcr clk\n");
+		return PTR_ERR(wcss->tcm_slave_cbcr);
+	}
+
+	wcss->qdsp6ss_abhm_cbcr = devm_clk_get(wcss->dev, "wcnss_abhm_cbcr");
+	if (IS_ERR(wcss->qdsp6ss_abhm_cbcr)) {
+		ret = PTR_ERR(wcss->qdsp6ss_abhm_cbcr);
+		if (ret != -EPROBE_DEFER)
+			dev_err(wcss->dev, "failed to get abhm cbcr clk\n");
+		return PTR_ERR(wcss->qdsp6ss_abhm_cbcr);
+	}
+
+	wcss->qdsp6ss_axim_cbcr = devm_clk_get(wcss->dev, "wcnss_axim_cbcr");
+	if (IS_ERR(wcss->qdsp6ss_axim_cbcr)) {
+		ret = PTR_ERR(wcss->qdsp6ss_axim_cbcr);
+		if (ret != -EPROBE_DEFER)
+			dev_err(wcss->dev, "failed to get abhm cbcr clk\n");
+		return PTR_ERR(wcss->qdsp6ss_abhm_cbcr);
+	}
+
+	wcss->qdsp6ss_sleep_cbcr = devm_clk_get(wcss->dev, "wcnss_sleep_cbcr");
+	if (IS_ERR(wcss->qdsp6ss_sleep_cbcr)) {
+		ret = PTR_ERR(wcss->qdsp6ss_sleep_cbcr);
+		if (ret != -EPROBE_DEFER)
+			dev_err(wcss->dev, "failed to get qdsp6ss_sleep clk\n");
+		return PTR_ERR(wcss->qdsp6ss_sleep_cbcr);
+	}
+
+	wcss->qdsp6ss_xo_cbcr = devm_clk_get(wcss->dev,
+					     "wcnss_qdsp6ss_xo_cbcr");
+	if (IS_ERR(wcss->qdsp6ss_xo_cbcr)) {
+		ret = PTR_ERR(wcss->qdsp6ss_xo_cbcr);
+		if (ret != -EPROBE_DEFER)
+			dev_err(wcss->dev, "failed to get qdsp6ss_xo_cbcr clk\n");
+		return PTR_ERR(wcss->qdsp6ss_xo_cbcr);
+	}
+
+	wcss->qdsp6ss_core_gfmux = devm_clk_get(wcss->dev, "wcnss_core_gfm");
+	if (IS_ERR(wcss->qdsp6ss_core_gfmux)) {
+		ret = PTR_ERR(wcss->qdsp6ss_core_gfmux);
+		if (ret != -EPROBE_DEFER)
+			dev_err(wcss->dev, "failed to get core gfm clk\n");
+		return PTR_ERR(wcss->qdsp6ss_core_gfmux);
+	}
+
+	wcss->wcss_bcr_cbcr = devm_clk_get(wcss->dev, "wcss_bcr_cbcr");
+	if (IS_ERR(wcss->wcss_bcr_cbcr)) {
+		ret = PTR_ERR(wcss->wcss_bcr_cbcr);
+		if (ret != -EPROBE_DEFER)
+			dev_err(wcss->dev, "failed to get bcr cbcr clk\n");
+		return PTR_ERR(wcss->wcss_bcr_cbcr);
+	}
+
+	return 0;
+}
+
+static int q6v5_wcss_init_regulator(struct q6v5_wcss *wcss)
+{
+	wcss->cx_supply = devm_regulator_get(wcss->dev, "cx");
+	if (IS_ERR(wcss->cx_supply))
+		return PTR_ERR(wcss->cx_supply);
+
+	regulator_set_load(wcss->cx_supply, 100000);
+
+	wcss->px_supply = devm_regulator_get(wcss->dev, "px");
+	return PTR_ERR_OR_ZERO(wcss->px_supply);
+}
+
 static int q6v5_wcss_probe(struct platform_device *pdev)
 {
 	const struct wcss_data *desc;
@@ -548,7 +1071,7 @@ static int q6v5_wcss_probe(struct platform_device *pdev)
 	if (!desc)
 		return -EINVAL;
 
-	rproc = rproc_alloc(&pdev->dev, pdev->name, &q6v5_wcss_ops,
+	rproc = rproc_alloc(&pdev->dev, pdev->name, desc->ops,
 			    desc->firmware_name, sizeof(*wcss));
 	if (!rproc) {
 		dev_err(&pdev->dev, "failed to allocate rproc\n");
@@ -559,7 +1082,11 @@ static int q6v5_wcss_probe(struct platform_device *pdev)
 	wcss->dev = &pdev->dev;
 	wcss->pas_id = desc->pas_id;
 	wcss->version = desc->version;
-	wcss->crash_reason_smem = desc->crash_reason_smem;
+	wcss->has_aggre2_clk = desc->has_aggre2_clk;
+	wcss->aon_reset_required = desc->aon_reset_required;
+	platform_set_drvdata(pdev, wcss);
+
+	wcss->version = desc->version;
 
 	ret = q6v5_wcss_init_mmio(wcss, pdev);
 	if (ret)
@@ -569,6 +1096,16 @@ static int q6v5_wcss_probe(struct platform_device *pdev)
 	if (ret)
 		goto free_rproc;
 
+	if (wcss->version == WCSS_QCS404) {
+		ret = q6v5_wcss_init_clock(wcss);
+		if (ret)
+			goto free_rproc;
+
+		ret = q6v5_wcss_init_regulator(wcss);
+		if (ret)
+			goto free_rproc;
+	}
+
 	ret = q6v5_wcss_init_reset(wcss);
 	if (ret)
 		goto free_rproc;
@@ -578,6 +1115,14 @@ static int q6v5_wcss_probe(struct platform_device *pdev)
 	if (ret)
 		goto free_rproc;
 
+	if (wcss->version == WCSS_QCS404) {
+		qcom_add_glink_subdev(rproc, &wcss->glink_subdev);
+		qcom_add_ssr_subdev(rproc, &wcss->ssr_subdev, desc->ssr_name);
+		wcss->sysmon = qcom_add_sysmon_subdev(rproc,
+						      desc->sysmon_name,
+						      desc->ssctl_id);
+	}
+
 	ret = rproc_add(rproc);
 	if (ret)
 		goto free_rproc;
@@ -605,11 +1150,28 @@ static int q6v5_wcss_remove(struct platform_device *pdev)
 static const struct wcss_data wcss_ipq8074_res_init = {
 	.firmware_name = "IPQ8074/q6_fw.mdt",
 	.crash_reason_smem = 421,
+	.aon_reset_required = true,
 	.pas_handover = NULL,
+	.ops = &q6v5_wcss_ipq8074_ops,
+};
+
+static const struct wcss_data wcss_qcs404_res_init = {
+	.crash_reason_smem = 421,
+	.firmware_name = "wcnss.mdt",
+	.pas_id = 6,
+	.version = WCSS_QCS404,
+	.has_aggre2_clk = false,
+	.aon_reset_required = false,
+	.ssr_name = "mpss",
+	.sysmon_name = "wcnss",
+	.ssctl_id = 0x12,
+	.ops = &q6v5_wcss_qcs404_ops,
+	.pas_handover = q6v5_wcss_pas_handover,
 };
 
 static const struct of_device_id q6v5_wcss_of_match[] = {
-	{ .compatible = "qcom,ipq8074-wcss-pil", .data = &wcss_ipq8074_res_init },
+	{ .compatible = "ipq8074-wcss-pil", .data = &wcss_ipq8074_res_init },
+	{ .compatible = "qcom,qcs404-wcss-non-pas", .data = &wcss_qcs404_res_init },
 
 	{ },
 };
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 4/6] clk: qcom: Add WCSS Q6DSP clock controller for QCS404

[Stephen Boyd]

Quoting Govind Singh (2018-10-12 02:40:42)
> Add support for the WCSS QDSP clock control used on qcs404
> based devices. This would allow wcss remoteproc driver to
> control the required WCSS QDSP clock/reset controls to
> bring the subsystem out of reset and shutdown the WCSS QDSP.
> 
> Signed-off-by: Govind Singh <govinds@codeaurora.org>
> ---

This needs to be sent to the clk mailing list and maintainers.

> diff --git a/drivers/clk/qcom/wcsscc-qcs404.c b/drivers/clk/qcom/wcsscc-qcs404.c
> new file mode 100644
> index 0000000..9a0c864
> --- /dev/null
> +++ b/drivers/clk/qcom/wcsscc-qcs404.c
> @@ -0,0 +1,290 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/err.h>
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <dt-bindings/clock/qcom,wcss-qcs404.h>
> +#include <linux/reset-controller.h>
> +
> +#include "clk-regmap.h"
> +#include "clk-branch.h"
> +#include "common.h"
> +#include "reset.h"
> +
> +/* Q6SSTOP clocks */
> +static struct clk_branch lcc_ahbfabric_cbc_clk = {
> +       .halt_reg = 0x1b004,
> +       .halt_check = BRANCH_VOTED,
> +       .clkr = {
> +               .enable_reg = 0x1b004,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "lcc_ahbfabric_cbc_clk",
> +                       .ops = &clk_branch2_ops,
> +                       .flags = CLK_IGNORE_UNUSED,
> +               },
> +       },
> +};
> +
> +static struct clk_branch lcc_q6ss_ahbs_cbc_clk = {
> +       .halt_reg = 0x22000,
> +       .halt_check = BRANCH_VOTED,
> +       .clkr = {
> +               .enable_reg = 0x22000,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "lcc_q6ss_ahbs_cbc_clk",
> +                       .ops = &clk_branch2_ops,
> +                       .flags = CLK_IGNORE_UNUSED,
> +               },
> +       },
> +};
> +
> +static struct clk_branch lcc_q6ss_tcm_slave_cbc_clk = {
> +       .halt_reg = 0x1c000,
> +       .halt_check = BRANCH_VOTED,
> +       .clkr = {
> +               .enable_reg = 0x1c000,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "lcc_q6ss_tcm_slave_cbc_clk",
> +                       .ops = &clk_branch2_ops,
> +                       .flags = CLK_IGNORE_UNUSED,
> +               },
> +       },
> +};
> +
> +static struct clk_branch lcc_q6ss_ahbm_cbc_clk = {
> +       .halt_reg = 0x22004,
> +       .halt_check = BRANCH_VOTED,
> +       .clkr = {
> +               .enable_reg = 0x22004,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "lcc_q6ss_ahbm_cbc_clk",
> +                       .ops = &clk_branch2_ops,
> +                       .flags = CLK_IGNORE_UNUSED,
> +               },
> +       },
> +};
> +
> +static struct clk_branch lcc_q6ss_axim_cbc_clk = {
> +       .halt_reg = 0x1c004,
> +       .halt_check = BRANCH_VOTED,
> +       .clkr = {
> +               .enable_reg = 0x1c004,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "lcc_q6ss_axim_cbc_clk",
> +                       .ops = &clk_branch2_ops,
> +                       .flags = CLK_IGNORE_UNUSED,
> +               },
> +       },
> +};
> +
> +static struct clk_branch lcc_q6ss_bcr_sleep_clk = {
> +       .halt_reg = 0x6004,
> +       .halt_check = BRANCH_VOTED,
> +       .clkr = {
> +               .enable_reg = 0x6004,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "lcc_q6ss_bcr_sleep_clk",
> +                       .ops = &clk_branch2_ops,
> +                       .flags = CLK_IGNORE_UNUSED,
> +               },
> +       },
> +};
> +
> +/* TCSR clock */
> +static struct clk_branch wcss_lcc_csr_cbcr_clk = {
> +       .halt_reg = 0x8008,
> +       .halt_check = BRANCH_VOTED,
> +       .clkr = {
> +               .enable_reg = 0x8008,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "wcss_lcc_csr_cbcr_clk",
> +                       .ops = &clk_branch2_ops,
> +                       .flags = CLK_IGNORE_UNUSED,
> +               },
> +       },
> +};
> +
> +/* Q6SSTOP_QDSP6SS clock */
> +static struct clk_branch q6ss_xo_clk = {
> +       .halt_reg = 0x38,
> +       .halt_check = BRANCH_HALT_SKIP,

Why?

> +       .clkr = {
> +               .enable_reg = 0x38,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "q6ss_xo_clk",
> +                       .ops = &clk_branch2_ops,
> +                       .flags = CLK_IGNORE_UNUSED,
> +               },
> +       },
> +};
> +
> +static struct clk_branch q6ss_slp_clk = {
> +       .halt_reg = 0x3c,
> +       .halt_check = BRANCH_HALT_SKIP,

Ditto. Please add a comment.

> +       .clkr = {
> +               .enable_reg = 0x3c,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "q6ss_slp_clk",
> +                       .ops = &clk_branch2_ops,
> +                       .flags = CLK_IGNORE_UNUSED,
> +               },
> +       },
> +};
> +
> +static struct clk_branch q6sstop_q6ss_gfmux_clk_src = {
> +       .halt_reg = 0x20,
> +       .halt_check = BRANCH_HALT_SKIP,
> +       .clkr = {
> +               .enable_reg = 0x20,
> +               .enable_mask = (BIT(1) | BIT(3) | BIT(8)),

Drop useless parenthesis please. And this is actually three clks in one?

> +               .hw.init = &(struct clk_init_data){
> +                       .name = "q6sstop_q6ss_gfmux_clk_src",
> +                       .ops = &clk_branch2_ops,
> +                       .flags = CLK_IGNORE_UNUSED,

Why? Please explain with comment, etc.

> +               },
> +       },
> +};
> +
> +static struct regmap_config wcss_regmap_config = {
> +       .reg_bits       = 32,
> +       .reg_stride     = 4,
> +       .val_bits       = 32,
> +       .fast_io        = true,

max register? Or that gets set later on somewhere?

> +};
> +
> +static struct clk_regmap *wcss_q6sstop_qcs405_clocks[] = {
> +       [WCSS_AHBFABRIC_CBCR_CLK] = &lcc_ahbfabric_cbc_clk.clkr,
> +       [WCSS_AHBS_CBCR_CLK] = &lcc_q6ss_ahbs_cbc_clk.clkr,
> +       [WCSS_TCM_CBCR_CLK] = &lcc_q6ss_tcm_slave_cbc_clk.clkr,
> +       [WCSS_AHBM_CBCR_CLK] = &lcc_q6ss_ahbm_cbc_clk.clkr,
> +       [WCSS_AXIM_CBCR_CLK] = &lcc_q6ss_axim_cbc_clk.clkr,
> +       [WCSS_BCR_CBCR_CLK] = &lcc_q6ss_bcr_sleep_clk.clkr,
> +};
> +
> +static const struct qcom_reset_map qdsp6ss_qcs405_resets[] = {
> +       [Q6SSTOP_QDSP6SS_RESET] = {0x14, 0},
> +       [Q6SSTOP_QDSP6SS_CORE_RESET] = {0x14, 1},
> +       [Q6SSTOP_QDSP6SS_BUS_RESET] = {0x14, 2},
> +};
> +
> +static const struct qcom_reset_map q6sstop_qcs405_resets[] = {
> +       [Q6SSTOP_BCR_RESET] = {0x6000},
> +};
> +
> +static const struct qcom_cc_desc wcss_q6sstop_qcs405_desc = {
> +       .config = &wcss_regmap_config,
> +       .clks = wcss_q6sstop_qcs405_clocks,
> +       .num_clks = ARRAY_SIZE(wcss_q6sstop_qcs405_clocks),
> +       .resets = q6sstop_qcs405_resets,
> +       .num_resets = ARRAY_SIZE(q6sstop_qcs405_resets),
> +};
> +
> +static struct clk_regmap *wcnss_tcsr_qcs405_clocks[] = {
> +       [WCSS_LCC_CBCR_CLK] = &wcss_lcc_csr_cbcr_clk.clkr,
> +};
> +
> +static const struct qcom_cc_desc wcnss_tcsr_qcs405_desc = {
> +       .config = &wcss_regmap_config,
> +       .clks = wcnss_tcsr_qcs405_clocks,
> +       .num_clks = ARRAY_SIZE(wcnss_tcsr_qcs405_clocks),
> +};
> +
> +static struct clk_regmap *wcnss_qdsp6ss_qcs405_clocks[] = {
> +       [WCSS_QDSP6SS_XO_CBCR_CLK] = &q6ss_xo_clk.clkr,
> +       [WCSS_QDSP6SS_SLEEP_CBCR_CLK] = &q6ss_slp_clk.clkr,
> +       [WCSS_QDSP6SS_GFMMUX_CLK] = &q6sstop_q6ss_gfmux_clk_src.clkr,
> +};
> +
> +static const struct qcom_cc_desc wcnss_qdsp6ss_qcs405_desc = {
> +       .config = &wcss_regmap_config,
> +       .clks = wcnss_qdsp6ss_qcs405_clocks,
> +       .num_clks = ARRAY_SIZE(wcnss_qdsp6ss_qcs405_clocks),
> +       .resets = qdsp6ss_qcs405_resets,
> +       .num_resets = ARRAY_SIZE(qdsp6ss_qcs405_resets),
> +};
> +
> +static int wcss_clocks_qcs405_probe(struct platform_device *pdev, int index,
> +                                   const struct qcom_cc_desc *desc)
> +{
> +       struct regmap *regmap;
> +       struct resource *res;
> +       void __iomem *base;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, index);
> +       base = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(base))
> +               return -ENOMEM;
> +
> +       regmap = devm_regmap_init_mmio(&pdev->dev, base, desc->config);
> +       if (IS_ERR(regmap))
> +               return PTR_ERR(regmap);
> +
> +       return qcom_cc_really_probe(pdev, desc, regmap);
> +}
> +
> +/* WCSS CC clock controller */

Please drop the obvious comment!

> +static const struct of_device_id wcss_cc_qcs405_match_table[] = {
> +       { .compatible = "qcom,qcs405-wcsscc" },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(of, wcss_cc_qcs405_match_table);
> +
> +static int wcss_cc_qcs405_probe(struct platform_device *pdev)
> +{
> +       const struct qcom_cc_desc *desc;
> +       int ret;
> +
> +       wcss_regmap_config.name = "wcss_q6sstop";
> +       desc = &wcss_q6sstop_qcs405_desc;
> +
> +       ret = wcss_clocks_qcs405_probe(pdev, 0, desc);
> +       if (ret)
> +               return ret;
> +
> +       wcss_regmap_config.name = "wcnss_tcsr";
> +       desc = &wcnss_tcsr_qcs405_desc;
> +
> +       ret = wcss_clocks_qcs405_probe(pdev, 1, desc);
> +       if (ret)
> +               return ret;
> +
> +       wcss_regmap_config.name = "wcss_qdsp6ss";
> +       desc = &wcnss_qdsp6ss_qcs405_desc;
> +
> +       return wcss_clocks_qcs405_probe(pdev, 2, desc);
> +}

And this is the second clk driver in qcom land to do this, so we need to
add this to style of clk driver to clk/qcom/common.c

Re: [PATCH v2 2/6] clk: qcom: Add WCSS gcc clock control for QCS404

[Stephen Boyd]

Quoting Govind Singh (2018-10-12 02:40:40)
>  static const struct regmap_config gcc_qcs404_regmap_config = {
> @@ -2669,7 +2699,7 @@ enum {
>         .fast_io        = true,
>  };
>  
> -static const struct qcom_cc_desc gcc_qcs404_desc = {
> +static struct qcom_cc_desc gcc_qcs404_desc = {
>         .config = &gcc_qcs404_regmap_config,
>         .clks = gcc_qcs404_clocks,
>         .num_clks = ARRAY_SIZE(gcc_qcs404_clocks),
> @@ -2702,6 +2732,11 @@ static int gcc_qcs404_probe(struct platform_device *pdev)
>         clk_prepare_enable(apss_ahb_clk_src.clkr.hw.clk);
>         clk_prepare_enable(gpll0_ao_out_main.clkr.hw.clk);
>  
> +       if (of_property_read_bool(pdev->dev.of_node, "qcom,wcss-protected")) {

Is this documented? And shouldn't it be inverted? If it isn't protected
then we do this?

> +               gcc_qcs404_clocks[GCC_WCSS_Q6_AHB_CBCR_CLK] = &gcc_wdsp_q6ss_ahbs_clk.clkr;
> +               gcc_qcs404_clocks[GCC_WCSS_Q6_AXIM_CBCR_CLK] = &gcc_wdsp_q6ss_axim_clk.clkr;
> +       }
> +
>         return qcom_cc_really_probe(pdev, &gcc_qcs404_desc, regmap);
>  }
>  

Re: [PATCH v2 1/6] dt-bindings: clock: qcom: Add QCOM WCSS GCC clock bindings

[Rob Herring]

On Fri, 12 Oct 2018 15:10:39 +0530, Govind Singh wrote:
> Add device tree bindings for WiFi QDSP gcc clock controls found in
> QCS404 soc.
> 
> Signed-off-by: Govind Singh <govinds@codeaurora.org>
> ---
>  include/dt-bindings/clock/qcom,gcc-qcs404.h | 3 +++
>  1 file changed, 3 insertions(+)
> 

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

Re: [PATCH v2 3/6] dt-bindings: clock: qcom: Introduce QCOM WCSS Q6DSP clock bindings

[Rob Herring]

On Fri, Oct 12, 2018 at 03:10:41PM +0530, Govind Singh wrote:
> Add device tree bindings for WiFi QDSP subsystem clock controls
> found in OCS405 soc.
> 
> Signed-off-by: Govind Singh <govinds@codeaurora.org>
> ---
>  .../devicetree/bindings/clock/qcom,wcsscc.txt      | 26 ++++++++++++++++++++++
>  include/dt-bindings/clock/qcom,wcss-qcs404.h       | 24 ++++++++++++++++++++
>  2 files changed, 50 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/qcom,wcsscc.txt
>  create mode 100644 include/dt-bindings/clock/qcom,wcss-qcs404.h
> 
> diff --git a/Documentation/devicetree/bindings/clock/qcom,wcsscc.txt b/Documentation/devicetree/bindings/clock/qcom,wcsscc.txt
> new file mode 100644
> index 0000000..2b19ef0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/qcom,wcsscc.txt
> @@ -0,0 +1,26 @@
> +Qualcomm WCSS Clock Controller Binding
> +-----------------------------------------------
> +
> +Required properties :
> +- compatible		: shall contain "qcom,qcs404-wcsscc"
> +- #clock-cells		: from common clock binding, shall contain 1.
> +- reg			: shall contain base register address and size,
> +			  in the order
> +			Index-0 maps to WCSS_Q6SSTOP clocks register region
> +			Index-1 maps to WCSS_TCSR register region
> +			Index-2 maps to WCSS_QDSP6SS register region
> +
> +Optional properties :
> +- reg-names	: register names of WCSS domain
> +		 "wcss_q6sstop", "wcnss_tcsr", "wcss_qdsp6ss".
> +
> +Example:
> +The below node has to be defined in the cases where the WCSS peripheral loader
> +would bring the subsystem out of reset.
> +
> +	clock_wcsscc: qcom,wcsscc@7000000 {

clock-controller@7500000

> +		compatible = "qcom,qcs404-wcsscc";
> +		reg = <0x07500000 0x4e000>, <0x07550000 0x8012>, <0x07400000 0x104>;
> +		reg-names = "wcss_q6sstop", "wcnss_tcsr", "wcss_qdsp6ss";
> +		#clock-cells = <1>;
> +	};
> diff --git a/include/dt-bindings/clock/qcom,wcss-qcs404.h b/include/dt-bindings/clock/qcom,wcss-qcs404.h
> new file mode 100644
> index 0000000..45dd659
> --- /dev/null
> +++ b/include/dt-bindings/clock/qcom,wcss-qcs404.h
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#ifndef _DT_BINDINGS_CLK_WCSS_QCS404_H
> +#define _DT_BINDINGS_CLK_WCSS_QCS404_H
> +
> +#define WCSS_AHBFABRIC_CBCR_CLK			0
> +#define WCSS_AHBS_CBCR_CLK			1
> +#define WCSS_TCM_CBCR_CLK			2
> +#define WCSS_AHBM_CBCR_CLK			3
> +#define WCSS_AXIM_CBCR_CLK			4
> +#define WCSS_BCR_CBCR_CLK			5
> +#define WCSS_LCC_CBCR_CLK			6
> +#define WCSS_QDSP6SS_XO_CBCR_CLK		7
> +#define WCSS_QDSP6SS_SLEEP_CBCR_CLK		8
> +#define WCSS_QDSP6SS_GFMMUX_CLK			9
> +
> +#define Q6SSTOP_QDSP6SS_RESET			0
> +#define Q6SSTOP_QDSP6SS_CORE_RESET		1
> +#define Q6SSTOP_QDSP6SS_BUS_RESET		2
> +#define Q6SSTOP_BCR_RESET			3

Resets too? Then you need to define #reset-cells.

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

Re: [PATCH v2 6/6] remoteproc: qcom: wcss: Add non pas wcss Q6 support for QCS404

[Rob Herring]

On Fri, Oct 12, 2018 at 03:10:44PM +0530, Govind Singh wrote:
> Add non PAS WCSS remoteproc driver support for QCS404 SOC.
> Add WCSS q6 bootup and shutdown sequence handled from
> Application Processor SubSystem(APSS).
> 
> Signed-off-by: Govind Singh <govinds@codeaurora.org>
> ---
>  .../devicetree/bindings/remoteproc/qcom,q6v5.txt   |   1 +

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

>  drivers/remoteproc/qcom_q6v5_wcss.c                | 638 +++++++++++++++++++--
>  2 files changed, 601 insertions(+), 38 deletions(-)

Re: [PATCH v2 6/6] remoteproc: qcom: wcss: Add non pas wcss Q6 support for QCS404

[Bjorn Andersson]

On Fri 12 Oct 02:40 PDT 2018, Govind Singh wrote:
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> index 601dd9f..cc83832 100644
> --- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> @@ -13,6 +13,7 @@ on the Qualcomm Hexagon core.
>  		    "qcom,msm8974-mss-pil"
>  		    "qcom,msm8996-mss-pil"
>  		    "qcom,sdm845-mss-pil"
> +		    "qcom,qcs404-wcss-non-pas"

Let's use the form qcom,qcs404-wcnss-pas for the PAS case and
qcom,qcs404-wcnss-pil for the non-PAS.

>  
>  - reg:
>  	Usage: required
> diff --git a/drivers/remoteproc/qcom_q6v5_wcss.c b/drivers/remoteproc/qcom_q6v5_wcss.c
[..]
>  /* Q6SS Register Offsets */
> -#define Q6SS_RESET_REG		0x014
> +#define Q6SS_RESET_REG			0x014

Most of these changes to defines are just reformatting, making it hard
to track what actually changed. Please do send a separate patch fixing
the indentation instead.

>  #define Q6SS_GFMUX_CTL_REG		0x020
>  #define Q6SS_PWR_CTL_REG		0x030
>  #define Q6SS_MEM_PWR_CTL		0x0B0
> +#define Q6SS_STRAP_ACC			0x110
> +#define Q6SS_CGC_OVERRIDE		0x034
> +#define Q6SS_BCR_REG			0x6000
>  
>  /* AXI Halt Register Offsets */
>  #define AXI_HALTREQ_REG			0x0
> @@ -36,6 +44,9 @@
>  #define Q6SS_CORE_ARES			BIT(1)
>  #define Q6SS_BUS_ARES_ENABLE		BIT(2)
>  
> +/* Q6SS_BRC_RESET */
> +#define Q6SS_BRC_BLK_ARES		BIT(0)
> +
>  /* Q6SS_GFMUX_CTL */
>  #define Q6SS_CLK_ENABLE			BIT(1)
>  
> @@ -44,14 +55,15 @@
>  #define Q6SS_SLP_RET_N			BIT(19)
>  #define Q6SS_CLAMP_IO			BIT(20)
>  #define QDSS_BHS_ON			BIT(21)
> +#define QDSS_Q6_MEMORIES		GENMASK(15, 0)
>  
>  /* Q6SS parameters */
> -#define Q6SS_LDO_BYP		BIT(25)
> -#define Q6SS_BHS_ON		BIT(24)
> -#define Q6SS_CLAMP_WL		BIT(21)
> +#define Q6SS_LDO_BYP			BIT(25)
> +#define Q6SS_BHS_ON			BIT(24)
> +#define Q6SS_CLAMP_WL			BIT(21)
>  #define Q6SS_CLAMP_QMC_MEM		BIT(22)
>  #define HALT_CHECK_MAX_LOOPS		200
> -#define Q6SS_XO_CBCR		GENMASK(5, 3)
> +#define Q6SS_XO_CBCR			GENMASK(5, 3)
>  
>  /* Q6SS config/status registers */
>  #define TCSR_GLOBAL_CFG0	0x0
> @@ -70,12 +82,23 @@
>  #define TCSR_WCSS_CLK_MASK	0x1F
>  #define TCSR_WCSS_CLK_ENABLE	0x14
>  
> +enum {
> +	WCSS_IPQ8074,
> +	WCSS_QCS404,
> +};
> +
>  struct wcss_data {
> -	void (*pas_handover)(struct qcom_q6v5 *q6v5);
>  	const char *firmware_name;
>  	int crash_reason_smem;
>  	int version;
>  	int pas_id;
> +	bool has_aggre2_clk;

Please don't introduce properties and code that you don't use.

> +	bool aon_reset_required;
> +	const char *ssr_name;
> +	const char *sysmon_name;
> +	int ssctl_id;
> +	const struct rproc_ops *ops;
> +	void (*pas_handover)(struct qcom_q6v5 *q6v5);
>  };
>  
>  struct q6v5_wcss {
> @@ -83,12 +106,37 @@ struct q6v5_wcss {
>  
>  	void __iomem *reg_base;
>  	void __iomem *rmb_base;
> +	void __iomem *q6stop_base;
>  
>  	struct regmap *halt_map;
>  	u32 halt_q6;
>  	u32 halt_wcss;
>  	u32 halt_nc;
>  
> +	struct clk *xo;
> +	struct clk *aggre2_clk;

You don't use aggre2_clk, so you can drop this.

> +	struct clk *ahbfabric_cbcr_clk;
> +	struct clk *gcc_abhs_cbcr;
> +	struct clk *gcc_axim_cbcr;
> +	struct clk *lcc_csr_cbcr;
> +	struct clk *ahbs_cbcr;
> +	struct clk *tcm_slave_cbcr;
> +	struct clk *qdsp6ss_abhm_cbcr;
> +	struct clk *qdsp6ss_sleep_cbcr;
> +	struct clk *qdsp6ss_axim_cbcr;
> +	struct clk *qdsp6ss_xo_cbcr;
> +	struct clk *qdsp6ss_core_gfmux;
> +	struct clk *wcss_bcr_cbcr;

It looks like you're able to group most of these up in one or more
clk_bulk_data

> +	struct regulator *cx_supply;
> +	struct regulator *px_supply;

px_supply isn't documented in the DT binding, do you actually use it?

> +
> +	bool has_aggre2_clk;
> +	bool aon_reset_required;
> +
> +	struct qcom_rproc_glink glink_subdev;
> +	struct qcom_rproc_ssr ssr_subdev;
> +	struct qcom_sysmon *sysmon;
> +
>  	struct reset_control *wcss_aon_reset;
>  	struct reset_control *wcss_reset;
>  	struct reset_control *wcss_q6_reset;
> @@ -99,7 +147,6 @@ struct q6v5_wcss {
>  	phys_addr_t mem_reloc;
>  	void *mem_region;
>  	size_t mem_size;
> -
>  	int crash_reason_smem;
>  	int pas_id;
>  	int version;
> @@ -245,6 +292,214 @@ static int q6v5_wcss_start(struct rproc *rproc)
>  	return ret;
>  }
[..]
> +static int q6v5_wcss_qcs404_reset(struct q6v5_wcss *wcss)
> +{
> +	unsigned long val;
> +
> +	writel(0x80800000, wcss->reg_base + Q6SS_STRAP_ACC);
> +
> +	/* Start core execution */
> +	val = readl(wcss->reg_base + Q6SS_RESET_REG);
> +	val &= ~Q6SS_STOP_CORE;
> +	writel(val, wcss->reg_base + Q6SS_RESET_REG);
> +
> +	return 0;
> +}
> +
> +static int q6v5_qcs404_wcss_start(struct rproc *rproc)

There are commonalities between this and the existing start function,
can we align them instead of duplicating?

> +{
> +	struct q6v5_wcss *wcss = rproc->priv;
> +	int ret;
> +
> +	ret = clk_prepare_enable(wcss->xo);
> +	if (ret)
> +		return ret;
> +
> +	if (wcss->has_aggre2_clk) {
> +		ret = clk_prepare_enable(wcss->aggre2_clk);
> +		if (ret)
> +			goto disable_xo_clk;
> +	}
> +
> +	ret = regulator_enable(wcss->cx_supply);
> +	if (ret)
> +		goto disable_aggre2_clk;
> +
> +	ret = regulator_enable(wcss->px_supply);
> +	if (ret)
> +		goto disable_cx_supply;
> +
> +	qcom_q6v5_prepare(&wcss->q6v5);
> +
> +	ret = q6v5_wcss_qcs404_power_on(wcss);
> +	if (ret) {
> +		dev_err(wcss->dev, "wcss clk_enable failed\n");
> +		goto disable_px_supply;
> +	}
> +
> +	writel(rproc->bootaddr >> 4, wcss->reg_base + Q6SS_RST_EVB);
> +
> +	ret = q6v5_wcss_qcs404_reset(wcss);

Just inline the 5 lines here.

> +	if (ret)
> +		dev_err(wcss->dev, "De-assert QDSP6 out of reset failed\n");

The function can't fail...

> +
> +	ret = qcom_q6v5_wait_for_start(&wcss->q6v5, 5 * HZ);
> +	if (ret == -ETIMEDOUT) {
> +		dev_err(wcss->dev, "start timed out\n");
> +		goto disable_px_supply;
> +	}
> +
> +	return 0;
> +
> +disable_px_supply:
> +	regulator_disable(wcss->px_supply);
> +disable_cx_supply:
> +	regulator_disable(wcss->cx_supply);
> +disable_aggre2_clk:
> +	if (wcss->has_aggre2_clk)
> +		clk_disable_unprepare(wcss->aggre2_clk);
> +disable_xo_clk:
> +	clk_disable_unprepare(wcss->xo);
> +
> +	return ret;
> +}
> +
>  static void q6v5_wcss_halt_axi_port(struct q6v5_wcss *wcss,
>  				    struct regmap *halt_map,
>  				    u32 offset)
> @@ -279,6 +534,77 @@ static void q6v5_wcss_halt_axi_port(struct q6v5_wcss *wcss,
>  	regmap_write(halt_map, offset + AXI_HALTREQ_REG, 0);
>  }
>  
> +static int q6v5_qcs404_wcss_shutdown(struct q6v5_wcss *wcss)
> +{
> +	unsigned long val;
> +	int ret;
> +
> +	q6v5_wcss_halt_axi_port(wcss, wcss->halt_map, wcss->halt_wcss);
> +
> +	/* assert clamps to avoid MX current inrush */
> +	val = readl(wcss->reg_base + Q6SS_PWR_CTL_REG);
> +	val |= (Q6SS_CLAMP_IO | Q6SS_CLAMP_WL | Q6SS_CLAMP_QMC_MEM);
> +	writel(val, wcss->reg_base + Q6SS_PWR_CTL_REG);
> +
> +	/* Disable memories by turning off memory foot/headswitch */
> +	writel((readl(wcss->reg_base + Q6SS_MEM_PWR_CTL) &
> +		~QDSS_Q6_MEMORIES),
> +		wcss->reg_base + Q6SS_MEM_PWR_CTL);
> +
> +	/* Clear the BHS_ON bit */
> +	val = readl(wcss->reg_base + Q6SS_PWR_CTL_REG);
> +	val &= ~Q6SS_BHS_ON;
> +	writel(val, wcss->reg_base + Q6SS_PWR_CTL_REG);
> +
> +	clk_disable_unprepare(wcss->ahbfabric_cbcr_clk);
> +	clk_disable_unprepare(wcss->lcc_csr_cbcr);
> +	clk_disable_unprepare(wcss->tcm_slave_cbcr);
> +	clk_disable_unprepare(wcss->qdsp6ss_abhm_cbcr);
> +	clk_disable_unprepare(wcss->qdsp6ss_axim_cbcr);
> +	clk_disable_unprepare(wcss->qdsp6ss_xo_cbcr);
> +	clk_disable_unprepare(wcss->ahbs_cbcr);
> +	clk_disable_unprepare(wcss->wcss_bcr_cbcr);
> +	clk_disable_unprepare(wcss->qdsp6ss_core_gfmux);
> +	clk_disable_unprepare(wcss->gcc_abhs_cbcr);
> +
> +	ret = reset_control_assert(wcss->wcss_reset);
> +	if (ret) {
> +		dev_err(wcss->dev, "wcss_reset failed\n");
> +		return ret;
> +	}
> +	usleep_range(200, 300);
> +
> +	ret = reset_control_deassert(wcss->wcss_reset);
> +	if (ret) {
> +		dev_err(wcss->dev, "wcss_reset failed\n");
> +		return ret;
> +	}

Can't we deassert this in start(), as that would make it better follow
the IPQ implementation.

> +	usleep_range(200, 300);
> +
> +	clk_disable_unprepare(wcss->gcc_axim_cbcr);
> +
> +	return 0;
> +}
> +
> +static int q6v5_qcs404_wcss_stop(struct rproc *rproc)
> +{
> +	struct q6v5_wcss *wcss = rproc->priv;
> +	int ret;
> +
> +	/* WCSS powerdown */
> +	ret = qcom_q6v5_request_stop(&wcss->q6v5);
> +	if (ret == -ETIMEDOUT)
> +		dev_err(wcss->dev, "timed out on wait\n");
> +
> +	ret = q6v5_qcs404_wcss_shutdown(wcss);

This call is the only part that differs from the existing stop(), so
please switch here instead of duplicating the function.

> +	if (ret)
> +		return ret;
> +
> +	qcom_q6v5_unprepare(&wcss->q6v5);
> +
> +	return 0;
> +}
> +
>  static int q6v5_wcss_powerdown(struct q6v5_wcss *wcss)
>  {
>  	int ret;
> @@ -312,7 +638,8 @@ static int q6v5_wcss_powerdown(struct q6v5_wcss *wcss)
>  	}
>  
>  	/* 6 - De-assert WCSS_AON reset */
> -	reset_control_assert(wcss->wcss_aon_reset);
> +	if (wcss->aon_reset_required)

aon_reset_required is set for IPQ, which is the only case which will
call this function, so no need to check for this.

> +		reset_control_assert(wcss->wcss_aon_reset);
>  
>  	/* 7 - Disable WCSSAON_CONFIG 13 */
>  	val = readl(wcss->rmb_base + SSCAON_CONFIG);
> @@ -392,6 +719,17 @@ static int q6v5_q6_powerdown(struct q6v5_wcss *wcss)
>  	return 0;
>  }
>  
> +static void q6v5_wcss_pas_handover(struct qcom_q6v5 *q6v5)

Why does the non-PAS driver have a method named pas_handover()?

> +{
> +	struct q6v5_wcss *wcss = container_of(q6v5, struct q6v5_wcss, q6v5);
> +
> +	regulator_disable(wcss->px_supply);
> +	regulator_disable(wcss->cx_supply);
> +	if (wcss->has_aggre2_clk)
> +		clk_disable_unprepare(wcss->aggre2_clk);
> +	clk_disable_unprepare(wcss->xo);
> +}
> +
>  static int q6v5_wcss_stop(struct rproc *rproc)
>  {
>  	struct q6v5_wcss *wcss = rproc->priv;
> @@ -439,7 +777,7 @@ static int q6v5_wcss_load(struct rproc *rproc, const struct firmware *fw)
>  				     wcss->mem_size, &wcss->mem_reloc);
>  }
>  
> -static const struct rproc_ops q6v5_wcss_ops = {
> +static const struct rproc_ops q6v5_wcss_ipq8074_ops = {

Rename this when you introduce it in the previous patch.

>  	.start = q6v5_wcss_start,
>  	.stop = q6v5_wcss_stop,
>  	.da_to_va = q6v5_wcss_da_to_va,
> @@ -447,23 +785,33 @@ static int q6v5_wcss_load(struct rproc *rproc, const struct firmware *fw)
>  	.get_boot_addr = rproc_elf_get_boot_addr,
>  };
>  
> +static const struct rproc_ops q6v5_wcss_qcs404_ops = {
> +	.start = q6v5_qcs404_wcss_start,
> +	.stop = q6v5_qcs404_wcss_stop,
> +	.da_to_va = q6v5_wcss_da_to_va,
> +	.load = q6v5_wcss_load,
> +	.get_boot_addr = rproc_elf_get_boot_addr,
> +};
> +
>  static int q6v5_wcss_init_reset(struct q6v5_wcss *wcss)
>  {
>  	struct device *dev = wcss->dev;
>  
> -	wcss->wcss_aon_reset = devm_reset_control_get(dev, "wcss_aon_reset");
> -	if (IS_ERR(wcss->wcss_aon_reset)) {
> -		dev_err(wcss->dev, "unable to acquire wcss_aon_reset\n");
> -		return PTR_ERR(wcss->wcss_aon_reset);
> +	if (wcss->aon_reset_required) {

Pass desc, or an argument to the function, rather than stashing this
value in wcss. And just leave wcss_aon_reset NULL when not used.

> +		wcss->wcss_aon_reset = devm_reset_control_get_exclusive(dev, "wcss_aon_reset");
> +		if (IS_ERR(wcss->wcss_aon_reset)) {
> +			dev_err(wcss->dev, "fail to acquire wcss_aon_reset\n");
> +			return PTR_ERR(wcss->wcss_aon_reset);
> +		}
>  	}
>  
> -	wcss->wcss_reset = devm_reset_control_get(dev, "wcss_reset");
> +	wcss->wcss_reset = devm_reset_control_get_exclusive(dev, "wcss_reset");

Please submit this in a separate patch.

>  	if (IS_ERR(wcss->wcss_reset)) {
>  		dev_err(wcss->dev, "unable to acquire wcss_reset\n");
>  		return PTR_ERR(wcss->wcss_reset);
>  	}
>  
> -	wcss->wcss_q6_reset = devm_reset_control_get(dev, "wcss_q6_reset");
> +	wcss->wcss_q6_reset = devm_reset_control_get_exclusive(dev, "wcss_q6_reset");

Ditto

>  	if (IS_ERR(wcss->wcss_q6_reset)) {
>  		dev_err(wcss->dev, "unable to acquire wcss_q6_reset\n");
>  		return PTR_ERR(wcss->wcss_q6_reset);
> @@ -475,36 +823,73 @@ static int q6v5_wcss_init_reset(struct q6v5_wcss *wcss)
>  static int q6v5_wcss_init_mmio(struct q6v5_wcss *wcss,
>  			       struct platform_device *pdev)
>  {
> +	struct device_node *syscon;
>  	struct of_phandle_args args;
>  	struct resource *res;
>  	int ret;
>  
>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "qdsp6");
> -	wcss->reg_base = devm_ioremap_resource(&pdev->dev, res);
> +	wcss->reg_base = devm_ioremap(&pdev->dev, res->start,
> +				      resource_size(res));

Isn't this the same thing?

>  	if (IS_ERR(wcss->reg_base))
>  		return PTR_ERR(wcss->reg_base);
>  
> -	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "rmb");
> -	wcss->rmb_base = devm_ioremap_resource(&pdev->dev, res);
> -	if (IS_ERR(wcss->rmb_base))
> -		return PTR_ERR(wcss->rmb_base);
> -
> -	ret = of_parse_phandle_with_fixed_args(pdev->dev.of_node,
> -					       "qcom,halt-regs", 3, 0, &args);
> -	if (ret < 0) {
> -		dev_err(&pdev->dev, "failed to parse qcom,halt-regs\n");
> -		return -EINVAL;
> +	if (wcss->version == WCSS_QCS404) {
> +		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +						   "q6stop");
> +		if (!res) {
> +			dev_err(&pdev->dev, "invalid q6stop_base resource\n");
> +			return -EINVAL;
> +		}
> +
> +		wcss->q6stop_base = devm_ioremap(&pdev->dev, res->start,
> +						 resource_size(res));
> +		if (IS_ERR(wcss->q6stop_base))
> +			return PTR_ERR(wcss->q6stop_base);
> +
> +		syscon = of_parse_phandle(pdev->dev.of_node,
> +					  "qcom,halt-regs", 0);
> +		if (!syscon) {
> +			dev_err(&pdev->dev, "failed to parse qcom,halt-regs\n");
> +			return -EINVAL;
> +		}
> +
> +		wcss->halt_map = syscon_node_to_regmap(syscon);
> +		of_node_put(syscon);
> +		if (IS_ERR(wcss->halt_map))
> +			return PTR_ERR(wcss->halt_map);
> +
> +		ret = of_property_read_u32_index(pdev->dev.of_node,
> +						 "qcom,halt-regs",
> +						 1, &wcss->halt_wcss);

Okay, so the QCS404 doesn't need to halt q6 or nc?

I think you should change the fixed_args to
of_property_read_variable_u32_array() and try not to duplicate this.

> +		if (ret < 0) {
> +			dev_err(&pdev->dev, "no offset in syscon\n");
> +			return ret;
> +		}
> +	} else {
> +		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "rmb");
> +		wcss->rmb_base = devm_ioremap_resource(&pdev->dev, res);
> +		if (IS_ERR(wcss->rmb_base))
> +			return PTR_ERR(wcss->rmb_base);
> +
> +		ret = of_parse_phandle_with_fixed_args(pdev->dev.of_node,
> +						       "qcom,halt-regs", 3,
> +							0, &args);
> +		if (ret < 0) {
> +			dev_err(&pdev->dev, "failed to parse qcom,halt-regs\n");
> +			return -EINVAL;
> +		}
> +
> +		wcss->halt_map = syscon_node_to_regmap(args.np);
> +		of_node_put(args.np);
> +		if (IS_ERR(wcss->halt_map))
> +			return PTR_ERR(wcss->halt_map);
> +
> +		wcss->halt_q6 = args.args[0];
> +		wcss->halt_wcss = args.args[1];
> +		wcss->halt_nc = args.args[2];
>  	}
>  
> -	wcss->halt_map = syscon_node_to_regmap(args.np);
> -	of_node_put(args.np);
> -	if (IS_ERR(wcss->halt_map))
> -		return PTR_ERR(wcss->halt_map);
> -
> -	wcss->halt_q6 = args.args[0];
> -	wcss->halt_wcss = args.args[1];
> -	wcss->halt_nc = args.args[2];
> -
>  	return 0;
>  }
>  
> @@ -537,6 +922,144 @@ static int q6v5_alloc_memory_region(struct q6v5_wcss *wcss)
>  	return 0;
>  }
>  
> +static int q6v5_wcss_init_clock(struct q6v5_wcss *wcss)
> +{
> +	int ret;
> +
> +	wcss->xo = devm_clk_get(wcss->dev, "xo");
> +	if (IS_ERR(wcss->xo)) {
> +		ret = PTR_ERR(wcss->xo);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(wcss->dev, "failed to get xo clock");
> +		return ret;
> +	}
> +
> +	if (wcss->has_aggre2_clk) {
> +		wcss->aggre2_clk = devm_clk_get(wcss->dev, "aggre2");
> +		if (IS_ERR(wcss->aggre2_clk)) {
> +			ret = PTR_ERR(wcss->aggre2_clk);
> +			if (ret != -EPROBE_DEFER)
> +				dev_err(wcss->dev,
> +					"failed to get aggre2 clock");
> +			return ret;
> +		}
> +	}
> +
> +	wcss->gcc_abhs_cbcr = devm_clk_get(wcss->dev, "gcc_abhs_cbcr");

Please also update the binding to describe these clocks for this
compatible.

[..]
> @@ -559,7 +1082,11 @@ static int q6v5_wcss_probe(struct platform_device *pdev)
>  	wcss->dev = &pdev->dev;
>  	wcss->pas_id = desc->pas_id;
>  	wcss->version = desc->version;
> -	wcss->crash_reason_smem = desc->crash_reason_smem;
> +	wcss->has_aggre2_clk = desc->has_aggre2_clk;
> +	wcss->aon_reset_required = desc->aon_reset_required;
> +	platform_set_drvdata(pdev, wcss);

I don't see a platform_get_drvdata(), so do you really need this?

> +
> +	wcss->version = desc->version;
>  
>  	ret = q6v5_wcss_init_mmio(wcss, pdev);
>  	if (ret)
> @@ -569,6 +1096,16 @@ static int q6v5_wcss_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto free_rproc;
>  
> +	if (wcss->version == WCSS_QCS404) {
> +		ret = q6v5_wcss_init_clock(wcss);
> +		if (ret)
> +			goto free_rproc;
> +
> +		ret = q6v5_wcss_init_regulator(wcss);
> +		if (ret)
> +			goto free_rproc;

Does the IPQ really not have any clocks or regulators?

> +	}
> +
>  	ret = q6v5_wcss_init_reset(wcss);
>  	if (ret)
>  		goto free_rproc;
> @@ -578,6 +1115,14 @@ static int q6v5_wcss_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto free_rproc;
>  
> +	if (wcss->version == WCSS_QCS404) {
> +		qcom_add_glink_subdev(rproc, &wcss->glink_subdev);
> +		qcom_add_ssr_subdev(rproc, &wcss->ssr_subdev, desc->ssr_name);
> +		wcss->sysmon = qcom_add_sysmon_subdev(rproc,
> +						      desc->sysmon_name,
> +						      desc->ssctl_id);

Unless there's good reason I think you should just add these regardless
of version.

> +	}
> +
>  	ret = rproc_add(rproc);
>  	if (ret)
>  		goto free_rproc;
> @@ -605,11 +1150,28 @@ static int q6v5_wcss_remove(struct platform_device *pdev)
>  static const struct wcss_data wcss_ipq8074_res_init = {
>  	.firmware_name = "IPQ8074/q6_fw.mdt",
>  	.crash_reason_smem = 421,
> +	.aon_reset_required = true,
>  	.pas_handover = NULL,
> +	.ops = &q6v5_wcss_ipq8074_ops,
> +};
> +
> +static const struct wcss_data wcss_qcs404_res_init = {
> +	.crash_reason_smem = 421,
> +	.firmware_name = "wcnss.mdt",
> +	.pas_id = 6,

Why does your non-PAS driver have a pas-id?

> +	.version = WCSS_QCS404,
> +	.has_aggre2_clk = false,
> +	.aon_reset_required = false,
> +	.ssr_name = "mpss",
> +	.sysmon_name = "wcnss",
> +	.ssctl_id = 0x12,
> +	.ops = &q6v5_wcss_qcs404_ops,
> +	.pas_handover = q6v5_wcss_pas_handover,
>  };
>  
>  static const struct of_device_id q6v5_wcss_of_match[] = {
> -	{ .compatible = "qcom,ipq8074-wcss-pil", .data = &wcss_ipq8074_res_init },
> +	{ .compatible = "ipq8074-wcss-pil", .data = &wcss_ipq8074_res_init },

You mistakenly dropped qcom, from the compatible here.

> +	{ .compatible = "qcom,qcs404-wcss-non-pas", .data = &wcss_qcs404_res_init },
>  
>  	{ },
>  };

Regards,
Bjorn

Re: [PATCH v2 2/6] clk: qcom: Add WCSS gcc clock control for QCS404

[Govind Singh]

Thanks Stephen for the review.

On 2018-10-16 06:31, Stephen Boyd wrote:
> Quoting Govind Singh (2018-10-12 02:40:40)
>>  static const struct regmap_config gcc_qcs404_regmap_config = {
>> @@ -2669,7 +2699,7 @@ enum {
>>         .fast_io        = true,
>>  };
>> 
>> -static const struct qcom_cc_desc gcc_qcs404_desc = {
>> +static struct qcom_cc_desc gcc_qcs404_desc = {
>>         .config = &gcc_qcs404_regmap_config,
>>         .clks = gcc_qcs404_clocks,
>>         .num_clks = ARRAY_SIZE(gcc_qcs404_clocks),
>> @@ -2702,6 +2732,11 @@ static int gcc_qcs404_probe(struct 
>> platform_device *pdev)
>>         clk_prepare_enable(apss_ahb_clk_src.clkr.hw.clk);
>>         clk_prepare_enable(gpll0_ao_out_main.clkr.hw.clk);
>> 
>> +       if (of_property_read_bool(pdev->dev.of_node, 
>> "qcom,wcss-protected")) {
> 
> Is this documented? And shouldn't it be inverted? If it isn't protected
> then we do this?
> 

I have moved this to #ifdef CONFIG_QCS_WCSSCC_404 similar to lpass clk 
driver for sdm845 in v3.

>> +               gcc_qcs404_clocks[GCC_WCSS_Q6_AHB_CBCR_CLK] = 
>> &gcc_wdsp_q6ss_ahbs_clk.clkr;
>> +               gcc_qcs404_clocks[GCC_WCSS_Q6_AXIM_CBCR_CLK] = 
>> &gcc_wdsp_q6ss_axim_clk.clkr;
>> +       }
>> +
>>         return qcom_cc_really_probe(pdev, &gcc_qcs404_desc, regmap);
>>  }
>> 

BR,
Govind

Re: [PATCH v2 4/6] clk: qcom: Add WCSS Q6DSP clock controller for QCS404

[Govind Singh]

Thanks Stephen for the review.

On 2018-10-16 06:30, Stephen Boyd wrote:
> Quoting Govind Singh (2018-10-12 02:40:42)
>> Add support for the WCSS QDSP clock control used on qcs404
>> based devices. This would allow wcss remoteproc driver to
>> control the required WCSS QDSP clock/reset controls to
>> bring the subsystem out of reset and shutdown the WCSS QDSP.
>> 
>> Signed-off-by: Govind Singh <govinds@codeaurora.org>
>> ---
> 
> This needs to be sent to the clk mailing list and maintainers.
> 

sure.

>> diff --git a/drivers/clk/qcom/wcsscc-qcs404.c 
>> b/drivers/clk/qcom/wcsscc-qcs404.c
>> new file mode 100644
>> index 0000000..9a0c864
>> --- /dev/null
>> +++ b/drivers/clk/qcom/wcsscc-qcs404.c
>> @@ -0,0 +1,290 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include <linux/bitops.h>
>> +#include <linux/err.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/module.h>
>> +#include <linux/of_address.h>
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/regmap.h>
>> +#include <linux/regulator/consumer.h>
>> +
>> +#include <dt-bindings/clock/qcom,wcss-qcs404.h>
>> +#include <linux/reset-controller.h>
>> +
>> +#include "clk-regmap.h"
>> +#include "clk-branch.h"
>> +#include "common.h"
>> +#include "reset.h"
>> +
>> +/* Q6SSTOP clocks */
>> +static struct clk_branch lcc_ahbfabric_cbc_clk = {
>> +       .halt_reg = 0x1b004,
>> +       .halt_check = BRANCH_VOTED,
>> +       .clkr = {
>> +               .enable_reg = 0x1b004,
>> +               .enable_mask = BIT(0),
>> +               .hw.init = &(struct clk_init_data){
>> +                       .name = "lcc_ahbfabric_cbc_clk",
>> +                       .ops = &clk_branch2_ops,
>> +                       .flags = CLK_IGNORE_UNUSED,
>> +               },
>> +       },
>> +};
>> +
>> +static struct clk_branch lcc_q6ss_ahbs_cbc_clk = {
>> +       .halt_reg = 0x22000,
>> +       .halt_check = BRANCH_VOTED,
>> +       .clkr = {
>> +               .enable_reg = 0x22000,
>> +               .enable_mask = BIT(0),
>> +               .hw.init = &(struct clk_init_data){
>> +                       .name = "lcc_q6ss_ahbs_cbc_clk",
>> +                       .ops = &clk_branch2_ops,
>> +                       .flags = CLK_IGNORE_UNUSED,
>> +               },
>> +       },
>> +};
>> +
>> +static struct clk_branch lcc_q6ss_tcm_slave_cbc_clk = {
>> +       .halt_reg = 0x1c000,
>> +       .halt_check = BRANCH_VOTED,
>> +       .clkr = {
>> +               .enable_reg = 0x1c000,
>> +               .enable_mask = BIT(0),
>> +               .hw.init = &(struct clk_init_data){
>> +                       .name = "lcc_q6ss_tcm_slave_cbc_clk",
>> +                       .ops = &clk_branch2_ops,
>> +                       .flags = CLK_IGNORE_UNUSED,
>> +               },
>> +       },
>> +};
>> +
>> +static struct clk_branch lcc_q6ss_ahbm_cbc_clk = {
>> +       .halt_reg = 0x22004,
>> +       .halt_check = BRANCH_VOTED,
>> +       .clkr = {
>> +               .enable_reg = 0x22004,
>> +               .enable_mask = BIT(0),
>> +               .hw.init = &(struct clk_init_data){
>> +                       .name = "lcc_q6ss_ahbm_cbc_clk",
>> +                       .ops = &clk_branch2_ops,
>> +                       .flags = CLK_IGNORE_UNUSED,
>> +               },
>> +       },
>> +};
>> +
>> +static struct clk_branch lcc_q6ss_axim_cbc_clk = {
>> +       .halt_reg = 0x1c004,
>> +       .halt_check = BRANCH_VOTED,
>> +       .clkr = {
>> +               .enable_reg = 0x1c004,
>> +               .enable_mask = BIT(0),
>> +               .hw.init = &(struct clk_init_data){
>> +                       .name = "lcc_q6ss_axim_cbc_clk",
>> +                       .ops = &clk_branch2_ops,
>> +                       .flags = CLK_IGNORE_UNUSED,
>> +               },
>> +       },
>> +};
>> +
>> +static struct clk_branch lcc_q6ss_bcr_sleep_clk = {
>> +       .halt_reg = 0x6004,
>> +       .halt_check = BRANCH_VOTED,
>> +       .clkr = {
>> +               .enable_reg = 0x6004,
>> +               .enable_mask = BIT(0),
>> +               .hw.init = &(struct clk_init_data){
>> +                       .name = "lcc_q6ss_bcr_sleep_clk",
>> +                       .ops = &clk_branch2_ops,
>> +                       .flags = CLK_IGNORE_UNUSED,
>> +               },
>> +       },
>> +};
>> +
>> +/* TCSR clock */
>> +static struct clk_branch wcss_lcc_csr_cbcr_clk = {
>> +       .halt_reg = 0x8008,
>> +       .halt_check = BRANCH_VOTED,
>> +       .clkr = {
>> +               .enable_reg = 0x8008,
>> +               .enable_mask = BIT(0),
>> +               .hw.init = &(struct clk_init_data){
>> +                       .name = "wcss_lcc_csr_cbcr_clk",
>> +                       .ops = &clk_branch2_ops,
>> +                       .flags = CLK_IGNORE_UNUSED,
>> +               },
>> +       },
>> +};
>> +
>> +/* Q6SSTOP_QDSP6SS clock */
>> +static struct clk_branch q6ss_xo_clk = {
>> +       .halt_reg = 0x38,
>> +       .halt_check = BRANCH_HALT_SKIP,
> 
> Why?
> 
CLK_OFF would not toggle until WCSS is out of reset.


>> +static struct clk_branch q6sstop_q6ss_gfmux_clk_src = {
>> +       .halt_reg = 0x20,
>> +       .halt_check = BRANCH_HALT_SKIP,
>> +       .clkr = {
>> +               .enable_reg = 0x20,
>> +               .enable_mask = (BIT(1) | BIT(3) | BIT(8)),
> 
> Drop useless parenthesis please. And this is actually three clks in 
> one?
> 

Addressed in v3.
>> +               .hw.init = &(struct clk_init_data){
>> +                       .name = "q6sstop_q6ss_gfmux_clk_src",
>> +                       .ops = &clk_branch2_ops,
>> +                       .flags = CLK_IGNORE_UNUSED,
> 
> Why? Please explain with comment, etc.
> 

Added comment in the v3 version for CLK_IGNORE_UNUSED flag.

>> +               },
>> +       },
>> +};
>> +
>> +static struct regmap_config wcss_regmap_config = {
>> +       .reg_bits       = 32,
>> +       .reg_stride     = 4,
>> +       .val_bits       = 32,
>> +       .fast_io        = true,
> 
> max register? Or that gets set later on somewhere?
> 

Addressed in v3.

>> +};
>> +
>> +static struct clk_regmap *wcss_q6sstop_qcs405_clocks[] = {
>> +       [WCSS_AHBFABRIC_CBCR_CLK] = &lcc_ahbfabric_cbc_clk.clkr,
>> +       [WCSS_AHBS_CBCR_CLK] = &lcc_q6ss_ahbs_cbc_clk.clkr,
>> +       [WCSS_TCM_CBCR_CLK] = &lcc_q6ss_tcm_slave_cbc_clk.clkr,
>> +       [WCSS_AHBM_CBCR_CLK] = &lcc_q6ss_ahbm_cbc_clk.clkr,
>> +       [WCSS_AXIM_CBCR_CLK] = &lcc_q6ss_axim_cbc_clk.clkr,
>> +       [WCSS_BCR_CBCR_CLK] = &lcc_q6ss_bcr_sleep_clk.clkr,
>> +};
>> +
>> +static const struct qcom_reset_map qdsp6ss_qcs405_resets[] = {
>> +       [Q6SSTOP_QDSP6SS_RESET] = {0x14, 0},
>> +       [Q6SSTOP_QDSP6SS_CORE_RESET] = {0x14, 1},
>> +       [Q6SSTOP_QDSP6SS_BUS_RESET] = {0x14, 2},
>> +};
>> +
>> +static const struct qcom_reset_map q6sstop_qcs405_resets[] = {
>> +       [Q6SSTOP_BCR_RESET] = {0x6000},
>> +};
>> +
>> +static const struct qcom_cc_desc wcss_q6sstop_qcs405_desc = {
>> +       .config = &wcss_regmap_config,
>> +       .clks = wcss_q6sstop_qcs405_clocks,
>> +       .num_clks = ARRAY_SIZE(wcss_q6sstop_qcs405_clocks),
>> +       .resets = q6sstop_qcs405_resets,
>> +       .num_resets = ARRAY_SIZE(q6sstop_qcs405_resets),
>> +};
>> +
>> +static struct clk_regmap *wcnss_tcsr_qcs405_clocks[] = {
>> +       [WCSS_LCC_CBCR_CLK] = &wcss_lcc_csr_cbcr_clk.clkr,
>> +};
>> +
>> +static const struct qcom_cc_desc wcnss_tcsr_qcs405_desc = {
>> +       .config = &wcss_regmap_config,
>> +       .clks = wcnss_tcsr_qcs405_clocks,
>> +       .num_clks = ARRAY_SIZE(wcnss_tcsr_qcs405_clocks),
>> +};
>> +
>> +static struct clk_regmap *wcnss_qdsp6ss_qcs405_clocks[] = {
>> +       [WCSS_QDSP6SS_XO_CBCR_CLK] = &q6ss_xo_clk.clkr,
>> +       [WCSS_QDSP6SS_SLEEP_CBCR_CLK] = &q6ss_slp_clk.clkr,
>> +       [WCSS_QDSP6SS_GFMMUX_CLK] = &q6sstop_q6ss_gfmux_clk_src.clkr,
>> +};
>> +
>> +static const struct qcom_cc_desc wcnss_qdsp6ss_qcs405_desc = {
>> +       .config = &wcss_regmap_config,
>> +       .clks = wcnss_qdsp6ss_qcs405_clocks,
>> +       .num_clks = ARRAY_SIZE(wcnss_qdsp6ss_qcs405_clocks),
>> +       .resets = qdsp6ss_qcs405_resets,
>> +       .num_resets = ARRAY_SIZE(qdsp6ss_qcs405_resets),
>> +};
>> +
>> +static int wcss_clocks_qcs405_probe(struct platform_device *pdev, int 
>> index,
>> +                                   const struct qcom_cc_desc *desc)
>> +{
>> +       struct regmap *regmap;
>> +       struct resource *res;
>> +       void __iomem *base;
>> +
>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, index);
>> +       base = devm_ioremap_resource(&pdev->dev, res);
>> +       if (IS_ERR(base))
>> +               return -ENOMEM;
>> +
>> +       regmap = devm_regmap_init_mmio(&pdev->dev, base, 
>> desc->config);
>> +       if (IS_ERR(regmap))
>> +               return PTR_ERR(regmap);
>> +
>> +       return qcom_cc_really_probe(pdev, desc, regmap);
>> +}
>> +
>> +/* WCSS CC clock controller */
> 
> Please drop the obvious comment!
> 
Addressed in v3.

>> +static const struct of_device_id wcss_cc_qcs405_match_table[] = {
>> +       { .compatible = "qcom,qcs405-wcsscc" },
>> +       { }
>> +};
>> +MODULE_DEVICE_TABLE(of, wcss_cc_qcs405_match_table);
>> +
>> +static int wcss_cc_qcs405_probe(struct platform_device *pdev)
>> +{
>> +       const struct qcom_cc_desc *desc;
>> +       int ret;
>> +
>> +       wcss_regmap_config.name = "wcss_q6sstop";
>> +       desc = &wcss_q6sstop_qcs405_desc;
>> +
>> +       ret = wcss_clocks_qcs405_probe(pdev, 0, desc);
>> +       if (ret)
>> +               return ret;
>> +
>> +       wcss_regmap_config.name = "wcnss_tcsr";
>> +       desc = &wcnss_tcsr_qcs405_desc;
>> +
>> +       ret = wcss_clocks_qcs405_probe(pdev, 1, desc);
>> +       if (ret)
>> +               return ret;
>> +
>> +       wcss_regmap_config.name = "wcss_qdsp6ss";
>> +       desc = &wcnss_qdsp6ss_qcs405_desc;
>> +
>> +       return wcss_clocks_qcs405_probe(pdev, 2, desc);
>> +}
> 
> And this is the second clk driver in qcom land to do this, so we need 
> to
> add this to style of clk driver to clk/qcom/common.c

Seems LPASS clk driver is still under review, can i address this comment 
as separate patch.

BR,
Govind

Re: [PATCH v2 6/6] remoteproc: qcom: wcss: Add non pas wcss Q6 support for QCS404

[Govind Singh]

Thanks Bjorn for the review.

On 2018-12-06 22:18, Bjorn Andersson wrote:
> On Fri 12 Oct 02:40 PDT 2018, Govind Singh wrote:
>> diff --git 
>> a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt 
>> b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
>> index 601dd9f..cc83832 100644
>> --- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
>> @@ -13,6 +13,7 @@ on the Qualcomm Hexagon core.
>>  		    "qcom,msm8974-mss-pil"
>>  		    "qcom,msm8996-mss-pil"
>>  		    "qcom,sdm845-mss-pil"
>> +		    "qcom,qcs404-wcss-non-pas"
> 
> Let's use the form qcom,qcs404-wcnss-pas for the PAS case and
> qcom,qcs404-wcnss-pil for the non-PAS.
> 

Addressed in v3.
>> 
>>  - reg:
>>  	Usage: required
>> diff --git a/drivers/remoteproc/qcom_q6v5_wcss.c 
>> b/drivers/remoteproc/qcom_q6v5_wcss.c
> [..]
>>  /* Q6SS Register Offsets */
>> -#define Q6SS_RESET_REG		0x014
>> +#define Q6SS_RESET_REG			0x014
> 
> Most of these changes to defines are just reformatting, making it hard
> to track what actually changed. Please do send a separate patch fixing
> the indentation instead.
> 

Addressed in v3.

>>  #define Q6SS_GFMUX_CTL_REG		0x020
>>  #define Q6SS_PWR_CTL_REG		0x030
>>  #define Q6SS_MEM_PWR_CTL		0x0B0
>> +#define Q6SS_STRAP_ACC			0x110
>> +#define Q6SS_CGC_OVERRIDE		0x034
>> +#define Q6SS_BCR_REG			0x6000
>> 
>>  /* AXI Halt Register Offsets */
>>  #define AXI_HALTREQ_REG			0x0
>> @@ -36,6 +44,9 @@
>>  #define Q6SS_CORE_ARES			BIT(1)
>>  #define Q6SS_BUS_ARES_ENABLE		BIT(2)
>> 
>> +/* Q6SS_BRC_RESET */
>> +#define Q6SS_BRC_BLK_ARES		BIT(0)
>> +
>>  /* Q6SS_GFMUX_CTL */
>>  #define Q6SS_CLK_ENABLE			BIT(1)
>> 
>> @@ -44,14 +55,15 @@
>>  #define Q6SS_SLP_RET_N			BIT(19)
>>  #define Q6SS_CLAMP_IO			BIT(20)
>>  #define QDSS_BHS_ON			BIT(21)
>> +#define QDSS_Q6_MEMORIES		GENMASK(15, 0)
>> 
>>  /* Q6SS parameters */
>> -#define Q6SS_LDO_BYP		BIT(25)
>> -#define Q6SS_BHS_ON		BIT(24)
>> -#define Q6SS_CLAMP_WL		BIT(21)
>> +#define Q6SS_LDO_BYP			BIT(25)
>> +#define Q6SS_BHS_ON			BIT(24)
>> +#define Q6SS_CLAMP_WL			BIT(21)
>>  #define Q6SS_CLAMP_QMC_MEM		BIT(22)
>>  #define HALT_CHECK_MAX_LOOPS		200
>> -#define Q6SS_XO_CBCR		GENMASK(5, 3)
>> +#define Q6SS_XO_CBCR			GENMASK(5, 3)
>> 
>>  /* Q6SS config/status registers */
>>  #define TCSR_GLOBAL_CFG0	0x0
>> @@ -70,12 +82,23 @@
>>  #define TCSR_WCSS_CLK_MASK	0x1F
>>  #define TCSR_WCSS_CLK_ENABLE	0x14
>> 
>> +enum {
>> +	WCSS_IPQ8074,
>> +	WCSS_QCS404,
>> +};
>> +
>>  struct wcss_data {
>> -	void (*pas_handover)(struct qcom_q6v5 *q6v5);
>>  	const char *firmware_name;
>>  	int crash_reason_smem;
>>  	int version;
>>  	int pas_id;
>> +	bool has_aggre2_clk;
> 
> Please don't introduce properties and code that you don't use.
> 

Addressed in v3.

>> +	bool aon_reset_required;
>> +	const char *ssr_name;
>> +	const char *sysmon_name;
>> +	int ssctl_id;
>> +	const struct rproc_ops *ops;
>> +	void (*pas_handover)(struct qcom_q6v5 *q6v5);
>>  };
>> 
>>  struct q6v5_wcss {
>> @@ -83,12 +106,37 @@ struct q6v5_wcss {
>> 
>>  	void __iomem *reg_base;
>>  	void __iomem *rmb_base;
>> +	void __iomem *q6stop_base;
>> 
>>  	struct regmap *halt_map;
>>  	u32 halt_q6;
>>  	u32 halt_wcss;
>>  	u32 halt_nc;
>> 
>> +	struct clk *xo;
>> +	struct clk *aggre2_clk;
> 
> You don't use aggre2_clk, so you can drop this.
> 
Addressed in v3.


>> +	struct clk *ahbfabric_cbcr_clk;
>> +	struct clk *gcc_abhs_cbcr;
>> +	struct clk *gcc_axim_cbcr;
>> +	struct clk *lcc_csr_cbcr;
>> +	struct clk *ahbs_cbcr;
>> +	struct clk *tcm_slave_cbcr;
>> +	struct clk *qdsp6ss_abhm_cbcr;
>> +	struct clk *qdsp6ss_sleep_cbcr;
>> +	struct clk *qdsp6ss_axim_cbcr;
>> +	struct clk *qdsp6ss_xo_cbcr;
>> +	struct clk *qdsp6ss_core_gfmux;
>> +	struct clk *wcss_bcr_cbcr;
> 
> It looks like you're able to group most of these up in one or more
> clk_bulk_data
> 

These clocks needs specific order, hence clk_bulk_data is not working.

>> +	struct regulator *cx_supply;
>> +	struct regulator *px_supply;
> 
> px_supply isn't documented in the DT binding, do you actually use it?
> 

Removed in v3.

>> +
>> +	bool has_aggre2_clk;
>> +	bool aon_reset_required;
>> +
>> +	struct qcom_rproc_glink glink_subdev;
>> +	struct qcom_rproc_ssr ssr_subdev;
>> +	struct qcom_sysmon *sysmon;
>> +
>>  	struct reset_control *wcss_aon_reset;
>>  	struct reset_control *wcss_reset;
>>  	struct reset_control *wcss_q6_reset;
>> @@ -99,7 +147,6 @@ struct q6v5_wcss {
>>  	phys_addr_t mem_reloc;
>>  	void *mem_region;
>>  	size_t mem_size;
>> -
>>  	int crash_reason_smem;
>>  	int pas_id;
>>  	int version;
>> @@ -245,6 +292,214 @@ static int q6v5_wcss_start(struct rproc *rproc)
>>  	return ret;
>>  }
> [..]
>> +static int q6v5_wcss_qcs404_reset(struct q6v5_wcss *wcss)
>> +{
>> +	unsigned long val;
>> +
>> +	writel(0x80800000, wcss->reg_base + Q6SS_STRAP_ACC);
>> +
>> +	/* Start core execution */
>> +	val = readl(wcss->reg_base + Q6SS_RESET_REG);
>> +	val &= ~Q6SS_STOP_CORE;
>> +	writel(val, wcss->reg_base + Q6SS_RESET_REG);
>> +
>> +	return 0;
>> +}
>> +
>> +static int q6v5_qcs404_wcss_start(struct rproc *rproc)
> 
> There are commonalities between this and the existing start function,
> can we align them instead of duplicating?
> 

Addressed in v3.

>> +{
>> +	struct q6v5_wcss *wcss = rproc->priv;
>> +	int ret;
>> +
>> +	ret = clk_prepare_enable(wcss->xo);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (wcss->has_aggre2_clk) {
>> +		ret = clk_prepare_enable(wcss->aggre2_clk);
>> +		if (ret)
>> +			goto disable_xo_clk;
>> +	}
>> +
>> +	ret = regulator_enable(wcss->cx_supply);
>> +	if (ret)
>> +		goto disable_aggre2_clk;
>> +
>> +	ret = regulator_enable(wcss->px_supply);
>> +	if (ret)
>> +		goto disable_cx_supply;
>> +
>> +	qcom_q6v5_prepare(&wcss->q6v5);
>> +
>> +	ret = q6v5_wcss_qcs404_power_on(wcss);
>> +	if (ret) {
>> +		dev_err(wcss->dev, "wcss clk_enable failed\n");
>> +		goto disable_px_supply;
>> +	}
>> +
>> +	writel(rproc->bootaddr >> 4, wcss->reg_base + Q6SS_RST_EVB);
>> +
>> +	ret = q6v5_wcss_qcs404_reset(wcss);
> 
> Just inline the 5 lines here.
> 
Addressed in v3.

>> +	if (ret)
>> +		dev_err(wcss->dev, "De-assert QDSP6 out of reset failed\n");
> 
> The function can't fail...
> 
Addressed in v3.

>> +
>> +	ret = qcom_q6v5_wait_for_start(&wcss->q6v5, 5 * HZ);
>> +	if (ret == -ETIMEDOUT) {
>> +		dev_err(wcss->dev, "start timed out\n");
>> +		goto disable_px_supply;
>> +	}
>> +
>> +	return 0;
>> +
>> +disable_px_supply:
>> +	regulator_disable(wcss->px_supply);
>> +disable_cx_supply:
>> +	regulator_disable(wcss->cx_supply);
>> +disable_aggre2_clk:
>> +	if (wcss->has_aggre2_clk)
>> +		clk_disable_unprepare(wcss->aggre2_clk);
>> +disable_xo_clk:
>> +	clk_disable_unprepare(wcss->xo);
>> +
>> +	return ret;
>> +}
>> +
>>  static void q6v5_wcss_halt_axi_port(struct q6v5_wcss *wcss,
>>  				    struct regmap *halt_map,
>>  				    u32 offset)
>> @@ -279,6 +534,77 @@ static void q6v5_wcss_halt_axi_port(struct 
>> q6v5_wcss *wcss,
>>  	regmap_write(halt_map, offset + AXI_HALTREQ_REG, 0);
>>  }
>> 
>> +static int q6v5_qcs404_wcss_shutdown(struct q6v5_wcss *wcss)
>> +{
>> +	unsigned long val;
>> +	int ret;
>> +
>> +	q6v5_wcss_halt_axi_port(wcss, wcss->halt_map, wcss->halt_wcss);
>> +
>> +	/* assert clamps to avoid MX current inrush */
>> +	val = readl(wcss->reg_base + Q6SS_PWR_CTL_REG);
>> +	val |= (Q6SS_CLAMP_IO | Q6SS_CLAMP_WL | Q6SS_CLAMP_QMC_MEM);
>> +	writel(val, wcss->reg_base + Q6SS_PWR_CTL_REG);
>> +
>> +	/* Disable memories by turning off memory foot/headswitch */
>> +	writel((readl(wcss->reg_base + Q6SS_MEM_PWR_CTL) &
>> +		~QDSS_Q6_MEMORIES),
>> +		wcss->reg_base + Q6SS_MEM_PWR_CTL);
>> +
>> +	/* Clear the BHS_ON bit */
>> +	val = readl(wcss->reg_base + Q6SS_PWR_CTL_REG);
>> +	val &= ~Q6SS_BHS_ON;
>> +	writel(val, wcss->reg_base + Q6SS_PWR_CTL_REG);
>> +
>> +	clk_disable_unprepare(wcss->ahbfabric_cbcr_clk);
>> +	clk_disable_unprepare(wcss->lcc_csr_cbcr);
>> +	clk_disable_unprepare(wcss->tcm_slave_cbcr);
>> +	clk_disable_unprepare(wcss->qdsp6ss_abhm_cbcr);
>> +	clk_disable_unprepare(wcss->qdsp6ss_axim_cbcr);
>> +	clk_disable_unprepare(wcss->qdsp6ss_xo_cbcr);
>> +	clk_disable_unprepare(wcss->ahbs_cbcr);
>> +	clk_disable_unprepare(wcss->wcss_bcr_cbcr);
>> +	clk_disable_unprepare(wcss->qdsp6ss_core_gfmux);
>> +	clk_disable_unprepare(wcss->gcc_abhs_cbcr);
>> +
>> +	ret = reset_control_assert(wcss->wcss_reset);
>> +	if (ret) {
>> +		dev_err(wcss->dev, "wcss_reset failed\n");
>> +		return ret;
>> +	}
>> +	usleep_range(200, 300);
>> +
>> +	ret = reset_control_deassert(wcss->wcss_reset);
>> +	if (ret) {
>> +		dev_err(wcss->dev, "wcss_reset failed\n");
>> +		return ret;
>> +	}
> 
> Can't we deassert this in start(), as that would make it better follow
> the IPQ implementation.
> 

No, deassert in start is not working.

>> +	usleep_range(200, 300);
>> +
>> +	clk_disable_unprepare(wcss->gcc_axim_cbcr);
>> +
>> +	return 0;
>> +}
>> +
>> +static int q6v5_qcs404_wcss_stop(struct rproc *rproc)
>> +{
>> +	struct q6v5_wcss *wcss = rproc->priv;
>> +	int ret;
>> +
>> +	/* WCSS powerdown */
>> +	ret = qcom_q6v5_request_stop(&wcss->q6v5);
>> +	if (ret == -ETIMEDOUT)
>> +		dev_err(wcss->dev, "timed out on wait\n");
>> +
>> +	ret = q6v5_qcs404_wcss_shutdown(wcss);
> 
> This call is the only part that differs from the existing stop(), so
> please switch here instead of duplicating the function.
> 

Addressed in v3.

>> +	if (ret)
>> +		return ret;
>> +
>> +	qcom_q6v5_unprepare(&wcss->q6v5);
>> +
>> +	return 0;
>> +}
>> +
>>  static int q6v5_wcss_powerdown(struct q6v5_wcss *wcss)
>>  {
>>  	int ret;
>> @@ -312,7 +638,8 @@ static int q6v5_wcss_powerdown(struct q6v5_wcss 
>> *wcss)
>>  	}
>> 
>>  	/* 6 - De-assert WCSS_AON reset */
>> -	reset_control_assert(wcss->wcss_aon_reset);
>> +	if (wcss->aon_reset_required)
> 
> aon_reset_required is set for IPQ, which is the only case which will
> call this function, so no need to check for this.
> 
>> +		reset_control_assert(wcss->wcss_aon_reset);
>> 
>>  	/* 7 - Disable WCSSAON_CONFIG 13 */
>>  	val = readl(wcss->rmb_base + SSCAON_CONFIG);
>> @@ -392,6 +719,17 @@ static int q6v5_q6_powerdown(struct q6v5_wcss 
>> *wcss)
>>  	return 0;
>>  }
>> 
>> +static void q6v5_wcss_pas_handover(struct qcom_q6v5 *q6v5)
> 
> Why does the non-PAS driver have a method named pas_handover()?
> 
>> +{
>> +	struct q6v5_wcss *wcss = container_of(q6v5, struct q6v5_wcss, q6v5);
>> +
>> +	regulator_disable(wcss->px_supply);
>> +	regulator_disable(wcss->cx_supply);
>> +	if (wcss->has_aggre2_clk)
>> +		clk_disable_unprepare(wcss->aggre2_clk);
>> +	clk_disable_unprepare(wcss->xo);
>> +}
>> +
>>  static int q6v5_wcss_stop(struct rproc *rproc)
>>  {
>>  	struct q6v5_wcss *wcss = rproc->priv;
>> @@ -439,7 +777,7 @@ static int q6v5_wcss_load(struct rproc *rproc, 
>> const struct firmware *fw)
>>  				     wcss->mem_size, &wcss->mem_reloc);
>>  }
>> 
>> -static const struct rproc_ops q6v5_wcss_ops = {
>> +static const struct rproc_ops q6v5_wcss_ipq8074_ops = {
> 
> Rename this when you introduce it in the previous patch.
> 
Addressed in v3.

>>  	.start = q6v5_wcss_start,
>>  	.stop = q6v5_wcss_stop,
>>  	.da_to_va = q6v5_wcss_da_to_va,
>> @@ -447,23 +785,33 @@ static int q6v5_wcss_load(struct rproc *rproc, 
>> const struct firmware *fw)
>>  	.get_boot_addr = rproc_elf_get_boot_addr,
>>  };
>> 
>> +static const struct rproc_ops q6v5_wcss_qcs404_ops = {
>> +	.start = q6v5_qcs404_wcss_start,
>> +	.stop = q6v5_qcs404_wcss_stop,
>> +	.da_to_va = q6v5_wcss_da_to_va,
>> +	.load = q6v5_wcss_load,
>> +	.get_boot_addr = rproc_elf_get_boot_addr,
>> +};
>> +
>>  static int q6v5_wcss_init_reset(struct q6v5_wcss *wcss)
>>  {
>>  	struct device *dev = wcss->dev;
>> 
>> -	wcss->wcss_aon_reset = devm_reset_control_get(dev, 
>> "wcss_aon_reset");
>> -	if (IS_ERR(wcss->wcss_aon_reset)) {
>> -		dev_err(wcss->dev, "unable to acquire wcss_aon_reset\n");
>> -		return PTR_ERR(wcss->wcss_aon_reset);
>> +	if (wcss->aon_reset_required) {
> 
> Pass desc, or an argument to the function, rather than stashing this
> value in wcss. And just leave wcss_aon_reset NULL when not used.
> 

Addressed in v3.

>> +		wcss->wcss_aon_reset = devm_reset_control_get_exclusive(dev, 
>> "wcss_aon_reset");
>> +		if (IS_ERR(wcss->wcss_aon_reset)) {
>> +			dev_err(wcss->dev, "fail to acquire wcss_aon_reset\n");
>> +			return PTR_ERR(wcss->wcss_aon_reset);
>> +		}
>>  	}
>> 
>> -	wcss->wcss_reset = devm_reset_control_get(dev, "wcss_reset");
>> +	wcss->wcss_reset = devm_reset_control_get_exclusive(dev, 
>> "wcss_reset");
> 
> Please submit this in a separate patch.
> 

Addressed in v3.

>>  	if (IS_ERR(wcss->wcss_reset)) {
>>  		dev_err(wcss->dev, "unable to acquire wcss_reset\n");
>>  		return PTR_ERR(wcss->wcss_reset);
>>  	}
>> 
>> -	wcss->wcss_q6_reset = devm_reset_control_get(dev, "wcss_q6_reset");
>> +	wcss->wcss_q6_reset = devm_reset_control_get_exclusive(dev, 
>> "wcss_q6_reset");
> 
> Ditto
> 
>>  	if (IS_ERR(wcss->wcss_q6_reset)) {
>>  		dev_err(wcss->dev, "unable to acquire wcss_q6_reset\n");
>>  		return PTR_ERR(wcss->wcss_q6_reset);
>> @@ -475,36 +823,73 @@ static int q6v5_wcss_init_reset(struct q6v5_wcss 
>> *wcss)
>>  static int q6v5_wcss_init_mmio(struct q6v5_wcss *wcss,
>>  			       struct platform_device *pdev)
>>  {
>> +	struct device_node *syscon;
>>  	struct of_phandle_args args;
>>  	struct resource *res;
>>  	int ret;
>> 
>>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "qdsp6");
>> -	wcss->reg_base = devm_ioremap_resource(&pdev->dev, res);
>> +	wcss->reg_base = devm_ioremap(&pdev->dev, res->start,
>> +				      resource_size(res));
> 
> Isn't this the same thing?
> 
>>  	if (IS_ERR(wcss->reg_base))
>>  		return PTR_ERR(wcss->reg_base);
>> 
>> -	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "rmb");
>> -	wcss->rmb_base = devm_ioremap_resource(&pdev->dev, res);
>> -	if (IS_ERR(wcss->rmb_base))
>> -		return PTR_ERR(wcss->rmb_base);
>> -
>> -	ret = of_parse_phandle_with_fixed_args(pdev->dev.of_node,
>> -					       "qcom,halt-regs", 3, 0, &args);
>> -	if (ret < 0) {
>> -		dev_err(&pdev->dev, "failed to parse qcom,halt-regs\n");
>> -		return -EINVAL;
>> +	if (wcss->version == WCSS_QCS404) {
>> +		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>> +						   "q6stop");
>> +		if (!res) {
>> +			dev_err(&pdev->dev, "invalid q6stop_base resource\n");
>> +			return -EINVAL;
>> +		}
>> +
>> +		wcss->q6stop_base = devm_ioremap(&pdev->dev, res->start,
>> +						 resource_size(res));
>> +		if (IS_ERR(wcss->q6stop_base))
>> +			return PTR_ERR(wcss->q6stop_base);
>> +
>> +		syscon = of_parse_phandle(pdev->dev.of_node,
>> +					  "qcom,halt-regs", 0);
>> +		if (!syscon) {
>> +			dev_err(&pdev->dev, "failed to parse qcom,halt-regs\n");
>> +			return -EINVAL;
>> +		}
>> +
>> +		wcss->halt_map = syscon_node_to_regmap(syscon);
>> +		of_node_put(syscon);
>> +		if (IS_ERR(wcss->halt_map))
>> +			return PTR_ERR(wcss->halt_map);
>> +
>> +		ret = of_property_read_u32_index(pdev->dev.of_node,
>> +						 "qcom,halt-regs",
>> +						 1, &wcss->halt_wcss);
> 
> Okay, so the QCS404 doesn't need to halt q6 or nc?
> 

As per HPG doc, sequence does not have the same.

> I think you should change the fixed_args to
> of_property_read_variable_u32_array() and try not to duplicate this.
> 
>> +		if (ret < 0) {
>> +			dev_err(&pdev->dev, "no offset in syscon\n");
>> +			return ret;
>> +		}
>> +	} else {
>> +		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "rmb");
>> +		wcss->rmb_base = devm_ioremap_resource(&pdev->dev, res);
>> +		if (IS_ERR(wcss->rmb_base))
>> +			return PTR_ERR(wcss->rmb_base);
>> +
>> +		ret = of_parse_phandle_with_fixed_args(pdev->dev.of_node,
>> +						       "qcom,halt-regs", 3,
>> +							0, &args);
>> +		if (ret < 0) {
>> +			dev_err(&pdev->dev, "failed to parse qcom,halt-regs\n");
>> +			return -EINVAL;
>> +		}
>> +
>> +		wcss->halt_map = syscon_node_to_regmap(args.np);
>> +		of_node_put(args.np);
>> +		if (IS_ERR(wcss->halt_map))
>> +			return PTR_ERR(wcss->halt_map);
>> +
>> +		wcss->halt_q6 = args.args[0];
>> +		wcss->halt_wcss = args.args[1];
>> +		wcss->halt_nc = args.args[2];
>>  	}
>> 
>> -	wcss->halt_map = syscon_node_to_regmap(args.np);
>> -	of_node_put(args.np);
>> -	if (IS_ERR(wcss->halt_map))
>> -		return PTR_ERR(wcss->halt_map);
>> -
>> -	wcss->halt_q6 = args.args[0];
>> -	wcss->halt_wcss = args.args[1];
>> -	wcss->halt_nc = args.args[2];
>> -
>>  	return 0;
>>  }
>> 
>> @@ -537,6 +922,144 @@ static int q6v5_alloc_memory_region(struct 
>> q6v5_wcss *wcss)
>>  	return 0;
>>  }
>> 
>> +static int q6v5_wcss_init_clock(struct q6v5_wcss *wcss)
>> +{
>> +	int ret;
>> +
>> +	wcss->xo = devm_clk_get(wcss->dev, "xo");
>> +	if (IS_ERR(wcss->xo)) {
>> +		ret = PTR_ERR(wcss->xo);
>> +		if (ret != -EPROBE_DEFER)
>> +			dev_err(wcss->dev, "failed to get xo clock");
>> +		return ret;
>> +	}
>> +
>> +	if (wcss->has_aggre2_clk) {
>> +		wcss->aggre2_clk = devm_clk_get(wcss->dev, "aggre2");
>> +		if (IS_ERR(wcss->aggre2_clk)) {
>> +			ret = PTR_ERR(wcss->aggre2_clk);
>> +			if (ret != -EPROBE_DEFER)
>> +				dev_err(wcss->dev,
>> +					"failed to get aggre2 clock");
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	wcss->gcc_abhs_cbcr = devm_clk_get(wcss->dev, "gcc_abhs_cbcr");
> 
> Please also update the binding to describe these clocks for this
> compatible.
> 
Addressed in v3.

> [..]
>> @@ -559,7 +1082,11 @@ static int q6v5_wcss_probe(struct 
>> platform_device *pdev)
>>  	wcss->dev = &pdev->dev;
>>  	wcss->pas_id = desc->pas_id;
>>  	wcss->version = desc->version;
>> -	wcss->crash_reason_smem = desc->crash_reason_smem;
>> +	wcss->has_aggre2_clk = desc->has_aggre2_clk;
>> +	wcss->aon_reset_required = desc->aon_reset_required;
>> +	platform_set_drvdata(pdev, wcss);
> 
> I don't see a platform_get_drvdata(), so do you really need this?
> 
>> +
>> +	wcss->version = desc->version;
>> 
>>  	ret = q6v5_wcss_init_mmio(wcss, pdev);
>>  	if (ret)
>> @@ -569,6 +1096,16 @@ static int q6v5_wcss_probe(struct 
>> platform_device *pdev)
>>  	if (ret)
>>  		goto free_rproc;
>> 
>> +	if (wcss->version == WCSS_QCS404) {
>> +		ret = q6v5_wcss_init_clock(wcss);
>> +		if (ret)
>> +			goto free_rproc;
>> +
>> +		ret = q6v5_wcss_init_regulator(wcss);
>> +		if (ret)
>> +			goto free_rproc;
> 
> Does the IPQ really not have any clocks or regulators?
> 
>> +	}
>> +
>>  	ret = q6v5_wcss_init_reset(wcss);
>>  	if (ret)
>>  		goto free_rproc;
>> @@ -578,6 +1115,14 @@ static int q6v5_wcss_probe(struct 
>> platform_device *pdev)
>>  	if (ret)
>>  		goto free_rproc;
>> 
>> +	if (wcss->version == WCSS_QCS404) {
>> +		qcom_add_glink_subdev(rproc, &wcss->glink_subdev);
>> +		qcom_add_ssr_subdev(rproc, &wcss->ssr_subdev, desc->ssr_name);
>> +		wcss->sysmon = qcom_add_sysmon_subdev(rproc,
>> +						      desc->sysmon_name,
>> +						      desc->ssctl_id);
> 
> Unless there's good reason I think you should just add these regardless
> of version.
> 
Addressed in v3.

>> +	}
>> +
>>  	ret = rproc_add(rproc);
>>  	if (ret)
>>  		goto free_rproc;
>> @@ -605,11 +1150,28 @@ static int q6v5_wcss_remove(struct 
>> platform_device *pdev)
>>  static const struct wcss_data wcss_ipq8074_res_init = {
>>  	.firmware_name = "IPQ8074/q6_fw.mdt",
>>  	.crash_reason_smem = 421,
>> +	.aon_reset_required = true,
>>  	.pas_handover = NULL,
>> +	.ops = &q6v5_wcss_ipq8074_ops,
>> +};
>> +
>> +static const struct wcss_data wcss_qcs404_res_init = {
>> +	.crash_reason_smem = 421,
>> +	.firmware_name = "wcnss.mdt",
>> +	.pas_id = 6,
> 
> Why does your non-PAS driver have a pas-id?
> 

Addressed in v3.

>> +	.version = WCSS_QCS404,
>> +	.has_aggre2_clk = false,
>> +	.aon_reset_required = false,
>> +	.ssr_name = "mpss",
>> +	.sysmon_name = "wcnss",
>> +	.ssctl_id = 0x12,
>> +	.ops = &q6v5_wcss_qcs404_ops,
>> +	.pas_handover = q6v5_wcss_pas_handover,
>>  };
>> 
>>  static const struct of_device_id q6v5_wcss_of_match[] = {
>> -	{ .compatible = "qcom,ipq8074-wcss-pil", .data = 
>> &wcss_ipq8074_res_init },
>> +	{ .compatible = "ipq8074-wcss-pil", .data = &wcss_ipq8074_res_init 
>> },
> 
> You mistakenly dropped qcom, from the compatible here.
> 
Addressed in v3.


>> +	{ .compatible = "qcom,qcs404-wcss-non-pas", .data = 
>> &wcss_qcs404_res_init },
>> 
>>  	{ },
>>  };
> 
> Regards,
> Bjorn

BR,
Govind