Skip to contentSkip to conversation

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

v2 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Govind Singh

author
#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
author
#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
maintainer Reviewed-by on v2 1/6
#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]>
author
#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
maintainer
#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);
 }
author
#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

author
#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
maintainer
#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
author
#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
maintainer
#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

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

author
#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
author
#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
maintainer Acked-by on v2 6/6
#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(-)
#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

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