Add non PAS wcss Q6 support for QCS404

PATCH Superseded Govind Singh 4 participants 16 messages Oct 12, 2018 – Dec 15, 2018

Subsystems (5)

Device tree bindings Arm Qualcomm mailing list Common clk Qualcomm clock drivers +1 more subsystems

lore

Share & tools

lore

Govind Singh

author

2018-10-12 09:40 UTC #0 lore raw

v2 cover Add non PAS wcss Q6 support for QCS404

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

Signature · 2 lines

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

Govind Singh

author

2018-10-12 09:40 UTC #1 lore raw

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

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

Signed-off-by Govind Singh <[email protected]>

---
 include/dt-bindings/clock/qcom,gcc-qcs404.h | 3 +++
 1 file changed, 3 insertions(+)

include/dt-bindings/clock/qcom,gcc-qcs404.h +3 -0

index e2def29..ba6bc55 100644
@@ -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

Signature · 2 lines

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

Rob Herring

maintainer Reviewed-by on v2 1/6

2018-10-17 20:02 UTC #2 lore raw

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

Quoted 8 lines · Govind Singh

Add device tree bindings for WiFi QDSP gcc clock controls found in QCS404 soc.
Signed-off-by Govind Singh <[email protected]>
---
 include/dt-bindings/clock/qcom,gcc-qcs404.h | 3 +++
 1 file changed, 3 insertions(+)

Reviewed-by Rob Herring <[email protected]>

Govind Singh

author

2018-10-12 09:40 UTC #3 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/clk/qcom/gcc-qcs404.c | 37 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

drivers/clk/qcom/gcc-qcs404.c +36 -1

index 6d1387e..1b38b4e 100644
@@ -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);
 }

Signature · 2 lines

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

Stephen Boyd

maintainer

2018-10-16 01:01 UTC #4 lore raw

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

Quoting Govind Singh (2018-10-12 02:40:40)

Quoted 11 lines

static const struct regmap_config gcc_qcs404_regmap_config = {

drivers/clk/qcom/gcc-qcs404.c +1 -1

@@ -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),

drivers/clk/qcom/gcc-qcs404.c static int gcc_qcs404_probe(struct platform_device *pdev) · line 2735

Quoted 4 lines

drivers/clk/qcom/gcc-qcs404.c +1 -0

@@ -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?

Quoted 7 lines

drivers/clk/qcom/gcc-qcs404.c +4 -0

+               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);
 }

Govind Singh

author

2018-12-15 17:43 UTC #5 lore raw

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

Thanks Stephen for the review.

Quoted 16 lines · Stephen Boyd

Quoting Govind Singh (2018-10-12 02:40:40)

Quoted 15 lines

static const struct regmap_config gcc_qcs404_regmap_config = {

drivers/clk/qcom/gcc-qcs404.c +1 -1

@@ -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);

Quoted 6 lines

Review static int gcc_qcs404_probe(struct · line 2737
Quoted 2 lines
(quoted diff) +1 -0
+       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.

Quoted 9 lines

drivers/clk/qcom/gcc-qcs404.c static int gcc_qcs404_probe(struct platform_device *pdev) · line 2741
Quoted 9 lines
(quoted diff) +2 -0
+               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;
drivers/clk/qcom/gcc-qcs404.c +2 -0
+       }
+
        return qcom_cc_really_probe(pdev, &gcc_qcs404_desc, regmap);
 }

BR,
Govind

Govind Singh

author

2018-10-12 09:40 UTC #6 lore raw

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

In reply to Govind Singh

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

Signed-off-by Govind Singh <[email protected]>

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

Documentation/devicetree/bindings/clock/qcom,wcsscc.txt new file +26 -0

new file mode 100644
index 0000000..2b19ef0
@@ -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>;
+	};

include/dt-bindings/clock/qcom,wcss-qcs404.h new file +24 -0

new file mode 100644
index 0000000..45dd659
@@ -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

Signature · 2 lines

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

Rob Herring

maintainer

2018-10-17 20:05 UTC #7 lore raw

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

Quoted 28 lines · Govind Singh

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

Signed-off-by Govind Singh <[email protected]>

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

Documentation/devicetree/bindings/clock/qcom,wcsscc.txt new file +11 -0

new file mode 100644
index 0000000..2b19ef0
@@ -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

Documentation/devicetree/bindings/clock/qcom,wcsscc.txt line 21

Quoted 10 lines

Documentation/devicetree/bindings/clock/qcom,wcsscc.txt new file +10 -0

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

Quoted 24 lines

Documentation/devicetree/bindings/clock/qcom,wcsscc.txt new file +5 -0

+		compatible = "qcom,qcs404-wcsscc";
+		reg = <0x07500000 0x4e000>, <0x07550000 0x8012>, <0x07400000 0x104>;
+		reg-names = "wcss_q6sstop", "wcnss_tcsr", "wcss_qdsp6ss";
+		#clock-cells = <1>;
+	};

include/dt-bindings/clock/qcom,wcss-qcs404.h new file +13 -0

new file mode 100644
index 0000000..45dd659
@@ -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

include/dt-bindings/clock/qcom,wcss-qcs404.h line 23

Quoted 10 lines

include/dt-bindings/clock/qcom,wcss-qcs404.h new file +10 -0

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

Quoted 5 lines

include/dt-bindings/clock/qcom,wcss-qcs404.h new file +1 -0
+#endif
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Govind Singh

author

2018-10-12 09:40 UTC #8 lore raw

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

In reply to 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 <[email protected]>

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

drivers/clk/qcom/Kconfig +9 -0

index 5cdcb37..9ffc003 100644
@@ -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

drivers/clk/qcom/Makefile +1 -0

index 0a4790e..d258d4f 100644
@@ -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

drivers/clk/qcom/wcsscc-qcs404.c new file +290 -0

290 lines not on this page · raw message

Signature · 2 lines

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

Stephen Boyd

maintainer

2018-10-16 01:00 UTC #9 lore raw

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

Quoting Govind Singh (2018-10-12 02:40:42)

Quoted 7 lines

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 <[email protected]>
---

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

Quoted 123 lines

drivers/clk/qcom/wcsscc-qcs404.c new file +117 -0

new file mode 100644
index 0000000..9a0c864
@@ -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",

drivers/clk/qcom/wcsscc-qcs404.c line 127

Quoted 10 lines

drivers/clk/qcom/wcsscc-qcs404.c new file +10 -0

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

Quoted 4 lines

drivers/clk/qcom/wcsscc-qcs404.c new file +4 -0

+       .clkr = {
+               .enable_reg = 0x38,
+               .enable_mask = BIT(0),
+               .hw.init = &(struct clk_init_data){

drivers/clk/qcom/wcsscc-qcs404.c line 141

Quoted 10 lines

drivers/clk/qcom/wcsscc-qcs404.c new file +10 -0

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

Quoted 7 lines

drivers/clk/qcom/wcsscc-qcs404.c new file +7 -0

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

drivers/clk/qcom/wcsscc-qcs404.c line 158

Quoted 10 lines

drivers/clk/qcom/wcsscc-qcs404.c new file +10 -0

+               },
+       },
+};
+
+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?

drivers/clk/qcom/wcsscc-qcs404.c line 162

Quoted 4 lines

drivers/clk/qcom/wcsscc-qcs404.c new file +4 -0

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

drivers/clk/qcom/wcsscc-qcs404.c line 171

Quoted 9 lines

drivers/clk/qcom/wcsscc-qcs404.c new file +9 -0

+               },
+       },
+};
+
+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?

Quoted 63 lines

drivers/clk/qcom/wcsscc-qcs404.c new file +63 -0

+};
+
+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))

drivers/clk/qcom/wcsscc-qcs404.c line 244

Quoted 10 lines

drivers/clk/qcom/wcsscc-qcs404.c new file +10 -0

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

Quoted 20 lines

drivers/clk/qcom/wcsscc-qcs404.c new file +20 -0

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

drivers/clk/qcom/wcsscc-qcs404.c line 274

Quoted 10 lines

drivers/clk/qcom/wcsscc-qcs404.c new file +10 -0

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

Govind Singh

author

2018-12-15 17:50 UTC #10 lore raw

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

Thanks Stephen for the review.

Quoted 11 lines · Stephen Boyd

Quoting Govind Singh (2018-10-12 02:40:42)
Quoted 7 lines
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 <[email protected]>
---
This needs to be sent to the clk mailing list and maintainers.

sure.

Quoted 127 lines

drivers/clk/qcom/wcsscc-qcs404.c new file +121 -0

new file mode 100644
index 0000000..9a0c864
@@ -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,
+               },
+       },

Quoted 9 lines

drivers/clk/qcom/wcsscc-qcs404.c line 127
Quoted 6 lines
drivers/clk/qcom/wcsscc-qcs404.c new file +6 -0
+};
+
+/* 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.

Quoted 9 lines

drivers/clk/qcom/wcsscc-qcs404.c line 158
Quoted 6 lines
drivers/clk/qcom/wcsscc-qcs404.c new file +6 -0
+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.

Quoted 7 lines

drivers/clk/qcom/wcsscc-qcs404.c line 162
Quoted 4 lines
drivers/clk/qcom/wcsscc-qcs404.c new file +4 -0
+               .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.

Quoted 12 lines

drivers/clk/qcom/wcsscc-qcs404.c line 171
Quoted 9 lines
drivers/clk/qcom/wcsscc-qcs404.c new file +9 -0
+               },
+       },
+};
+
+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.

Quoted 69 lines

drivers/clk/qcom/wcsscc-qcs404.c new file +54 -0

+};
+
+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,

drivers/clk/qcom/wcsscc-qcs404.c new file +12 -0

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

Quoted 9 lines

drivers/clk/qcom/wcsscc-qcs404.c line 244
Quoted 6 lines
drivers/clk/qcom/wcsscc-qcs404.c new file +6 -0
+               return PTR_ERR(regmap);
+
+       return qcom_cc_really_probe(pdev, desc, regmap);
+}
+
+/* WCSS CC clock controller */
Please drop the obvious comment!

Addressed in v3.

Quoted 25 lines

drivers/clk/qcom/wcsscc-qcs404.c new file +25 -0

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

Quoted 8 lines

drivers/clk/qcom/wcsscc-qcs404.c line 274
Quoted 5 lines
drivers/clk/qcom/wcsscc-qcs404.c new file +5 -0
+       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

Govind Singh

author

2018-10-12 09:40 UTC #11 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/remoteproc/qcom_q6v5_wcss.c | 36 ++++++++++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)

drivers/remoteproc/qcom_q6v5_wcss.c +32 -4

index f93e1e4..24e276d 100644
@@ -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);

Signature · 2 lines

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

Govind Singh

author

2018-10-12 09:40 UTC #12 lore raw

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

In reply to 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 <[email protected]>

---
 .../devicetree/bindings/remoteproc/qcom,q6v5.txt   |   1 +
 drivers/remoteproc/qcom_q6v5_wcss.c                | 638 +++++++++++++++++++--
 2 files changed, 601 insertions(+), 38 deletions(-)

Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt +1 -0

index 601dd9f..cc83832 100644
@@ -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

drivers/remoteproc/qcom_q6v5_wcss.c +600 ~~-38~~

809 lines not on this page · raw message

Signature · 2 lines

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

Rob Herring

maintainer Acked-by on v2 6/6

2018-10-17 20:08 UTC #13 lore raw

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

Quoted 7 lines · 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 <[email protected]>
---
 .../devicetree/bindings/remoteproc/qcom,q6v5.txt   |   1 +

Acked-by Rob Herring <[email protected]>

Quoted 2 lines

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

Bjorn Andersson

2018-12-06 16:48 UTC #14 lore raw

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

In reply to Govind Singh

Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt on the Qualcomm Hexagon core. · line 16

Quoted 9 lines · Govind Singh

Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt +1 -0

index 601dd9f..cc83832 100644
@@ -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.

Quoted 4 lines

Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt +0 -0
 - reg:
 	Usage: required
drivers/remoteproc/qcom_q6v5_wcss.c +0 -0

[..]

drivers/remoteproc/qcom_q6v5_wcss.c line 27

Quoted 3 lines

/* Q6SS Register Offsets */
drivers/remoteproc/qcom_q6v5_wcss.c +1 -1
-#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.

Quoted 45 lines

#define Q6SS_GFMUX_CTL_REG 0x020
#define Q6SS_PWR_CTL_REG 0x030
#define Q6SS_MEM_PWR_CTL 0x0B0

drivers/remoteproc/qcom_q6v5_wcss.c +13 -4

+#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,

drivers/remoteproc/qcom_q6v5_wcss.c line 95

Quoted 10 lines

drivers/remoteproc/qcom_q6v5_wcss.c +4 -1

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

Quoted 12 lines

drivers/remoteproc/qcom_q6v5_wcss.c +6 -0

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

drivers/remoteproc/qcom_q6v5_wcss.c struct q6v5_wcss { · line 117

Quoted 10 lines

drivers/remoteproc/qcom_q6v5_wcss.c +3 -0

 	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.

drivers/remoteproc/qcom_q6v5_wcss.c struct q6v5_wcss { · line 129

Quoted 12 lines

drivers/remoteproc/qcom_q6v5_wcss.c +12 -0

+	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

drivers/remoteproc/qcom_q6v5_wcss.c struct q6v5_wcss { · line 131

Quoted 2 lines

drivers/remoteproc/qcom_q6v5_wcss.c +2 -0
+	struct regulator *cx_supply;
+	struct regulator *px_supply;

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

Quoted 22 lines

drivers/remoteproc/qcom_q6v5_wcss.c +8 -1

+
+	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;
 }

[..]

Quoted 5 lines

drivers/remoteproc/qcom_q6v5_wcss.c +5 -0

+static int q6v5_wcss_qcs404_reset(struct q6v5_wcss *wcss)
+{
+	unsigned long val;
+
+	writel(0x80800000, wcss->reg_base + Q6SS_STRAP_ACC);

drivers/remoteproc/qcom_q6v5_wcss.c static int q6v5_wcss_start(struct rproc *rproc) · line 445

Quoted 10 lines

drivers/remoteproc/qcom_q6v5_wcss.c +10 -0

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

Quoted 23 lines

drivers/remoteproc/qcom_q6v5_wcss.c +23 -0

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

drivers/remoteproc/qcom_q6v5_wcss.c static int q6v5_wcss_start(struct rproc *rproc) · line 478

Quoted 10 lines

drivers/remoteproc/qcom_q6v5_wcss.c +10 -0

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

drivers/remoteproc/qcom_q6v5_wcss.c

Quoted 2 lines

drivers/remoteproc/qcom_q6v5_wcss.c +2 -0

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

The function can't fail...

Quoted 64 lines

drivers/remoteproc/qcom_q6v5_wcss.c +57 -0

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

drivers/remoteproc/qcom_q6v5_wcss.c static void q6v5_wcss_halt_axi_port(struct q6v5_wcss *wcss, · line 581

Quoted 10 lines

drivers/remoteproc/qcom_q6v5_wcss.c +10 -0

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

Quoted 8 lines

drivers/remoteproc/qcom_q6v5_wcss.c +8 -0

+	usleep_range(200, 300);
+
+	clk_disable_unprepare(wcss->gcc_axim_cbcr);
+
+	return 0;
+}
+
+static int q6v5_qcs404_wcss_stop(struct rproc *rproc)

drivers/remoteproc/qcom_q6v5_wcss.c static void q6v5_wcss_halt_axi_port(struct q6v5_wcss *wcss, · line 599

Quoted 10 lines

drivers/remoteproc/qcom_q6v5_wcss.c +10 -0

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

Quoted 12 lines

drivers/remoteproc/qcom_q6v5_wcss.c +8 -0

+	if (ret)
+		return ret;
+
+	qcom_q6v5_unprepare(&wcss->q6v5);
+
+	return 0;
+}
+
 static int q6v5_wcss_powerdown(struct q6v5_wcss *wcss)
 {
 	int ret;

drivers/remoteproc/qcom_q6v5_wcss.c static int q6v5_wcss_powerdown(struct q6v5_wcss *wcss) · line 641

Quoted 5 lines

drivers/remoteproc/qcom_q6v5_wcss.c +1 -1

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

drivers/remoteproc/qcom_q6v5_wcss.c static int q6v5_q6_powerdown(struct q6v5_wcss *wcss) · line 722

Quoted 9 lines

drivers/remoteproc/qcom_q6v5_wcss.c +2 -0

+		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()?

Quoted 14 lines

drivers/remoteproc/qcom_q6v5_wcss.c +10 -0

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

drivers/remoteproc/qcom_q6v5_wcss.c static int q6v5_wcss_load(struct rproc *rproc, const struct firmware *fw) · line 780

Quoted 5 lines

drivers/remoteproc/qcom_q6v5_wcss.c +1 -1

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

Quoted 14 lines

drivers/remoteproc/qcom_q6v5_wcss.c +7 -0

 	.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,
+};

drivers/remoteproc/qcom_q6v5_wcss.c static int q6v5_wcss_load(struct rproc *rproc, const struct firmware *fw) · line 800

Quoted 10 lines

drivers/remoteproc/qcom_q6v5_wcss.c +2 -4

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

drivers/remoteproc/qcom_q6v5_wcss.c static int q6v5_wcss_load(struct rproc *rproc, const struct firmware *fw) · line 808

Quoted 9 lines

drivers/remoteproc/qcom_q6v5_wcss.c +6 -1

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

drivers/remoteproc/qcom_q6v5_wcss.c static int q6v5_wcss_load(struct rproc *rproc, const struct firmware *fw) · line 814

Quoted 7 lines

drivers/remoteproc/qcom_q6v5_wcss.c +1 -1

 	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

Quoted 6 lines

drivers/remoteproc/qcom_q6v5_wcss.c +0 -0

 	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)

drivers/remoteproc/qcom_q6v5_wcss.c static int q6v5_wcss_init_reset(struct q6v5_wcss *wcss) · line 833

Quoted 10 lines

drivers/remoteproc/qcom_q6v5_wcss.c +3 -1

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

Quoted 31 lines

drivers/remoteproc/qcom_q6v5_wcss.c +18 ~~-10~~

 	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;

drivers/remoteproc/qcom_q6v5_wcss.c static int q6v5_wcss_init_reset(struct q6v5_wcss *wcss) · line 864

Quoted 10 lines

drivers/remoteproc/qcom_q6v5_wcss.c +10 -0

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

Quoted 58 lines

drivers/remoteproc/qcom_q6v5_wcss.c +40 -9

+		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");

drivers/remoteproc/qcom_q6v5_wcss.c static int q6v5_alloc_memory_region(struct q6v5_wcss *wcss) · line 948

Quoted 10 lines

drivers/remoteproc/qcom_q6v5_wcss.c +10 -0

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

[..]

drivers/remoteproc/qcom_q6v5_wcss.c static int q6v5_wcss_probe(struct platform_device *pdev) · line 1087

Quoted 8 lines

drivers/remoteproc/qcom_q6v5_wcss.c +3 -1

@@ -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?

Quoted 7 lines

drivers/remoteproc/qcom_q6v5_wcss.c +2 -0

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

drivers/remoteproc/qcom_q6v5_wcss.c static int q6v5_wcss_probe(struct platform_device *pdev) · line 1106

Quoted 10 lines

drivers/remoteproc/qcom_q6v5_wcss.c +8 -0

 		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?

Quoted 6 lines

drivers/remoteproc/qcom_q6v5_wcss.c +2 -0

+	}
+
 	ret = q6v5_wcss_init_reset(wcss);
 	if (ret)
 		goto free_rproc;

drivers/remoteproc/qcom_q6v5_wcss.c static int q6v5_wcss_probe(struct platform_device *pdev) · line 1123

Quoted 9 lines

drivers/remoteproc/qcom_q6v5_wcss.c +6 -0

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

Quoted 8 lines

drivers/remoteproc/qcom_q6v5_wcss.c +2 -0

+	}
+
 	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",

drivers/remoteproc/qcom_q6v5_wcss.c static int q6v5_wcss_remove(struct platform_device *pdev) · line 1161

Quoted 10 lines

drivers/remoteproc/qcom_q6v5_wcss.c +8 -0

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

drivers/remoteproc/qcom_q6v5_wcss.c static int q6v5_wcss_remove(struct platform_device *pdev) · line 1173

Quoted 13 lines

drivers/remoteproc/qcom_q6v5_wcss.c +9 -1

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

drivers/remoteproc/qcom_q6v5_wcss.c static int q6v5_wcss_remove(struct platform_device *pdev) · line 1177

Quoted 4 lines

drivers/remoteproc/qcom_q6v5_wcss.c +1 -0

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

Regards,
Bjorn

Govind Singh

author

2018-12-15 18:00 UTC #15 lore raw

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

Thanks Bjorn for the review.

Quoted 14 lines · Bjorn Andersson

Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt on the Qualcomm Hexagon core. · line 16
Quoted 9 lines · Govind Singh
Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt +1 -0
index 601dd9f..cc83832 100644
@@ -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.

Quoted 5 lines

Quoted 4 lines
Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt +0 -0
 - reg:
 	Usage: required
drivers/remoteproc/qcom_q6v5_wcss.c +0 -0
[..]

Quoted 8 lines

drivers/remoteproc/qcom_q6v5_wcss.c line 27
Quoted 3 lines
/* Q6SS Register Offsets */
drivers/remoteproc/qcom_q6v5_wcss.c +1 -1
-#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.

Quoted 43 lines

#define Q6SS_GFMUX_CTL_REG 0x020
#define Q6SS_PWR_CTL_REG 0x030
#define Q6SS_MEM_PWR_CTL 0x0B0

drivers/remoteproc/qcom_q6v5_wcss.c +11 -4

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

Quoted 15 lines

drivers/remoteproc/qcom_q6v5_wcss.c line 95
Quoted 12 lines
drivers/remoteproc/qcom_q6v5_wcss.c +6 -1
+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.

Quoted 20 lines

drivers/remoteproc/qcom_q6v5_wcss.c +7 -0

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

Quoted 5 lines

drivers/remoteproc/qcom_q6v5_wcss.c struct q6v5_wcss { · line 117
Quoted 2 lines
drivers/remoteproc/qcom_q6v5_wcss.c +2 -0
+	struct clk *xo;
+	struct clk *aggre2_clk;
You don't use aggre2_clk, so you can drop this.

Addressed in v3.

Quoted 7 lines

drivers/remoteproc/qcom_q6v5_wcss.c +7 -0

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

Quoted 9 lines

drivers/remoteproc/qcom_q6v5_wcss.c struct q6v5_wcss { · line 129
Quoted 5 lines
drivers/remoteproc/qcom_q6v5_wcss.c +5 -0
+	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.

Quoted 5 lines

drivers/remoteproc/qcom_q6v5_wcss.c struct q6v5_wcss { · line 131
Quoted 2 lines
drivers/remoteproc/qcom_q6v5_wcss.c +2 -0
+	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.

Quoted 33 lines

Quoted 22 lines

drivers/remoteproc/qcom_q6v5_wcss.c +8 -1

+
+	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;
 }

[..]

Quoted 10 lines

drivers/remoteproc/qcom_q6v5_wcss.c +10 -0

+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);

Quoted 9 lines

drivers/remoteproc/qcom_q6v5_wcss.c static int q6v5_wcss_start(struct rproc *rproc) · line 445
Quoted 5 lines
drivers/remoteproc/qcom_q6v5_wcss.c +5 -0
+
+	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.

Quoted 27 lines

drivers/remoteproc/qcom_q6v5_wcss.c +27 -0

+{
+	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");

Quoted 9 lines

drivers/remoteproc/qcom_q6v5_wcss.c static int q6v5_wcss_start(struct rproc *rproc) · line 478
Quoted 6 lines
drivers/remoteproc/qcom_q6v5_wcss.c +6 -0
+		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.

Quoted 5 lines

drivers/remoteproc/qcom_q6v5_wcss.c
Quoted 2 lines
drivers/remoteproc/qcom_q6v5_wcss.c +2 -0
+	if (ret)
+		dev_err(wcss->dev, "De-assert QDSP6 out of reset failed\n");
The function can't fail...

Addressed in v3.

Quoted 70 lines

drivers/remoteproc/qcom_q6v5_wcss.c +62 -0

+
+	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);
+

Quoted 9 lines

drivers/remoteproc/qcom_q6v5_wcss.c static void q6v5_wcss_halt_axi_port(struct q6v5_wcss *wcss, · line 581
Quoted 5 lines
drivers/remoteproc/qcom_q6v5_wcss.c +5 -0
+	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.

Quoted 13 lines

drivers/remoteproc/qcom_q6v5_wcss.c +13 -0

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

Quoted 9 lines

drivers/remoteproc/qcom_q6v5_wcss.c static void q6v5_wcss_halt_axi_port(struct q6v5_wcss *wcss, · line 599
Quoted 5 lines
drivers/remoteproc/qcom_q6v5_wcss.c +5 -0
+	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.

Quoted 53 lines

Quoted 12 lines
drivers/remoteproc/qcom_q6v5_wcss.c +8 -0
+	if (ret)
+		return ret;
+
+	qcom_q6v5_unprepare(&wcss->q6v5);
+
+	return 0;
+}
+
 static int q6v5_wcss_powerdown(struct q6v5_wcss *wcss)
 {
drivers/remoteproc/qcom_q6v5_wcss.c +0 -0
 	int ret;
drivers/remoteproc/qcom_q6v5_wcss.c static int q6v5_wcss_powerdown(struct q6v5_wcss · line 642
Quoted 6 lines
drivers/remoteproc/qcom_q6v5_wcss.c +1 -1
@@ -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.
drivers/remoteproc/qcom_q6v5_wcss.c static int q6v5_q6_powerdown(struct q6v5_wcss · line 723
Quoted 10 lines
drivers/remoteproc/qcom_q6v5_wcss.c +1 -0
+		reset_control_assert(wcss->wcss_aon_reset);
 
 	/* 7 - Disable WCSSAON_CONFIG 13 */
drivers/remoteproc/qcom_q6v5_wcss.c +1 -0
 	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()?
Quoted 18 lines
drivers/remoteproc/qcom_q6v5_wcss.c +10 -0
+{
+	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)
 {
drivers/remoteproc/qcom_q6v5_wcss.c +0 -0
 	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);
 }
 

Quoted 5 lines

drivers/remoteproc/qcom_q6v5_wcss.c static int q6v5_wcss_load(struct rproc *rproc, const struct firmware *fw) · line 780
Quoted 2 lines
drivers/remoteproc/qcom_q6v5_wcss.c +1 -1
-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.

Quoted 20 lines

drivers/remoteproc/qcom_q6v5_wcss.c +0 -0

 	.start = q6v5_wcss_start,
 	.stop = q6v5_wcss_stop,

drivers/remoteproc/qcom_q6v5_wcss.c +8 -0

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

Quoted 10 lines

drivers/remoteproc/qcom_q6v5_wcss.c static int q6v5_wcss_load(struct rproc *rproc, const struct firmware *fw) · line 800
Quoted 6 lines
drivers/remoteproc/qcom_q6v5_wcss.c +1 -4
-	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.

Quoted 8 lines

drivers/remoteproc/qcom_q6v5_wcss.c +5 -0

+		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);
+		}
 	}

Quoted 6 lines

drivers/remoteproc/qcom_q6v5_wcss.c static int q6v5_wcss_load(struct rproc *rproc, · line 812
Quoted 3 lines
drivers/remoteproc/qcom_q6v5_wcss.c +1 -1
-	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.

Quoted 66 lines

Quoted 8 lines

drivers/remoteproc/qcom_q6v5_wcss.c +1 -1

 	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

Quoted 7 lines

drivers/remoteproc/qcom_q6v5_wcss.c +0 -0

 	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)

drivers/remoteproc/qcom_q6v5_wcss.c static int q6v5_wcss_init_reset(struct q6v5_wcss · line 834

Quoted 10 lines

drivers/remoteproc/qcom_q6v5_wcss.c +3 -1

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

Quoted 35 lines

drivers/remoteproc/qcom_q6v5_wcss.c +22 ~~-10~~

 	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);

Quoted 9 lines

drivers/remoteproc/qcom_q6v5_wcss.c static int q6v5_wcss_init_reset(struct q6v5_wcss *wcss) · line 864
Quoted 6 lines
drivers/remoteproc/qcom_q6v5_wcss.c +6 -0
+		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.

Quoted 67 lines

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

Quoted 64 lines

drivers/remoteproc/qcom_q6v5_wcss.c +45 -9

+		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");

Quoted 9 lines

drivers/remoteproc/qcom_q6v5_wcss.c static int q6v5_alloc_memory_region(struct q6v5_wcss *wcss) · line 948
Quoted 5 lines
drivers/remoteproc/qcom_q6v5_wcss.c +5 -0
+			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.

Quoted 44 lines

[..]
drivers/remoteproc/qcom_q6v5_wcss.c static int q6v5_wcss_probe(struct · line 1088
Quoted 9 lines
drivers/remoteproc/qcom_q6v5_wcss.c +3 -1
@@ -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?
Quoted 8 lines
drivers/remoteproc/qcom_q6v5_wcss.c +2 -0
+
+	wcss->version = desc->version;
 
 	ret = q6v5_wcss_init_mmio(wcss, pdev);
drivers/remoteproc/qcom_q6v5_wcss.c +0 -0
 	if (ret)
@@ -569,6 +1096,16 @@ static int q6v5_wcss_probe(struct 
 platform_device *pdev)
 	if (ret)
drivers/remoteproc/qcom_q6v5_wcss.c static int q6v5_wcss_probe(struct · line 1107
Quoted 10 lines
drivers/remoteproc/qcom_q6v5_wcss.c +8 -0
 		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?
Quoted 10 lines
drivers/remoteproc/qcom_q6v5_wcss.c +2 -0
+	}
+
 	ret = q6v5_wcss_init_reset(wcss);
 	if (ret)
drivers/remoteproc/qcom_q6v5_wcss.c +0 -0
 		goto free_rproc;
@@ -578,6 +1115,14 @@ static int q6v5_wcss_probe(struct 
 platform_device *pdev)
 	if (ret)
 		goto free_rproc;
 

Quoted 10 lines

drivers/remoteproc/qcom_q6v5_wcss.c static int q6v5_wcss_probe(struct platform_device *pdev) · line 1123
Quoted 6 lines
drivers/remoteproc/qcom_q6v5_wcss.c +6 -0
+	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.

Quoted 13 lines

drivers/remoteproc/qcom_q6v5_wcss.c +2 -0

+	}
+
 	ret = rproc_add(rproc);
 	if (ret)

drivers/remoteproc/qcom_q6v5_wcss.c +2 -0

 		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,

Quoted 9 lines

drivers/remoteproc/qcom_q6v5_wcss.c static int q6v5_wcss_remove(struct platform_device *pdev) · line 1161
Quoted 6 lines
drivers/remoteproc/qcom_q6v5_wcss.c +6 -0
+};
+
+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.

Quoted 10 lines

drivers/remoteproc/qcom_q6v5_wcss.c +8 -0

+	.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,
 };

Quoted 8 lines

drivers/remoteproc/qcom_q6v5_wcss.c static int q6v5_wcss_remove(struct · line 1176
Quoted 5 lines
drivers/remoteproc/qcom_q6v5_wcss.c +1 -1
 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.

Quoted 8 lines

Quoted 5 lines
drivers/remoteproc/qcom_q6v5_wcss.c +1 -0
+	{ .compatible = "qcom,qcs404-wcss-non-pas", .data = 
&wcss_qcs404_res_init },
{ },
};
Regards,
Bjorn

BR,
Govind