[PATCH v3 0/2] Add ADSP PIL driver for SDM845

[PATCH v3 0/2] Add ADSP PIL driver for SDM845

[Rohit kumar]

This provides initial patchset for ADSP PIL driver for SDM845.

Changes since v2:
	Separated dt-bindings documentation into separate patch.
	Used clk_bulk APIs for adsp clocks.
	Used power domain API and use reference to rpmhd resource.
	Renamed qcom_nonpas_adsp_pil.c to qcom_adsp_pil.c
	Removed smd as sdm845 does not support smd.
	Removed px_supply and aggre2_clk for sdm845 as they are not required.
	Removed reg-names as there is only one register.

This patch is dependent on the rpmh powerdomain driver https://lkml.org/lkml/2018/6/27/7
and renaming of Hexagon v5 PAS driver https://lkml.org/lkml/2018/8/28/129 .

Rohit kumar (2):
  dt-binding: remoteproc: Add QTI ADSP PIL bindings
  remoteproc: qcom: Introduce Non-PAS ADSP PIL driver

 .../bindings/remoteproc/qcom,adsp-pil.txt          | 123 +++++
 drivers/remoteproc/Kconfig                         |  14 +
 drivers/remoteproc/Makefile                        |   1 +
 drivers/remoteproc/qcom_adsp_pil.c                 | 500 +++++++++++++++++++++
 4 files changed, 638 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,adsp-pil.txt
 create mode 100644 drivers/remoteproc/qcom_adsp_pil.c

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v3 1/2] dt-binding: remoteproc: Add QTI ADSP PIL bindings

[Rohit kumar]

Add devicetree bindings documentation file for Qualcomm
Technolgies Inc ADSP Peripheral Image Loader.

Signed-off-by: Rohit kumar <rohitkr@codeaurora.org>
---
 .../bindings/remoteproc/qcom,adsp-pil.txt          | 123 +++++++++++++++++++++
 1 file changed, 123 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,adsp-pil.txt

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,adsp-pil.txt b/Documentation/devicetree/bindings/remoteproc/qcom,adsp-pil.txt
new file mode 100644
index 0000000..f1c215a
--- /dev/null
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,adsp-pil.txt
@@ -0,0 +1,123 @@
+Qualcomm Technology Inc. ADSP Peripheral Image Loader
+
+This document defines the binding for a component that loads and boots firmware
+on the Qualcomm Technology Inc. ADSP Hexagon core.
+
+- compatible:
+	Usage: required
+	Value type: <string>
+	Definition: must be one of:
+		    "qcom,sdm845-adsp-pil"
+
+- reg:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: must specify the base address and size of the qdsp6ss register
+
+- interrupts-extended:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: must list the watchdog, fatal IRQs ready, handover and
+		    stop-ack IRQs
+
+- interrupt-names:
+	Usage: required
+	Value type: <stringlist>
+	Definition: must be "wdog", "fatal", "ready", "handover", "stop-ack"
+
+- clocks:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition:  List of phandle and clock specifier pairs
+
+- clock-names:
+	Usage: required
+	Value type: <stringlist>
+	Definition: List of clock input name strings sorted in the same
+				order as the clocks property.
+
+- power-domains:
+	Usage: required
+	Value type: <phandle>
+	Definition: reference to cx power domain node.
+
+- resets:
+	Usage: required
+	Value type: <phandle>
+	Definition: reference to the reset-controller for the lpass
+
+- reset-names:
+        Usage: required
+        Value type: <stringlist>
+        Definition: must be "pdc_sync" and "cc_lpass"
+
+- qcom,halt-regs:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: a phandle reference to a syscon representing TCSR followed
+			by the offset within syscon for lpass halt register.
+
+- memory-region:
+	Usage: required
+	Value type: <phandle>
+	Definition: reference to the reserved-memory for the ADSP
+
+- qcom,smem-states:
+	Usage: required
+	Value type: <phandle>
+	Definition: reference to the smem state for requesting the ADSP to
+		    shut down
+
+- qcom,smem-state-names:
+	Usage: required
+	Value type: <stringlist>
+	Definition: must be "stop"
+
+
+= SUBNODES
+The adsp node may have an subnode named "glink-edge" that describes the
+communication edge, channels and devices related to the ADSP.
+See ../soc/qcom/qcom,glink.txt for details on how to describe these.
+
+= EXAMPLE
+The following example describes the resources needed to boot control the
+ADSP, as it is found on SDM845 boards.
+	adsp-pil {
+		compatible = "qcom,sdm845-adsp-pil";
+
+		reg = <0x17300000 0x40c>;
+
+		interrupts-extended = <&intc 0 162 IRQ_TYPE_EDGE_RISING>,
+			<&adsp_smp2p_in 0 IRQ_TYPE_EDGE_RISING>,
+			<&adsp_smp2p_in 1 IRQ_TYPE_EDGE_RISING>,
+			<&adsp_smp2p_in 2 IRQ_TYPE_EDGE_RISING>,
+			<&adsp_smp2p_in 3 IRQ_TYPE_EDGE_RISING>;
+		interrupt-names = "wdog", "fatal", "ready",
+			"handover", "stop-ack";
+
+		clocks = <&rpmhcc RPMH_CXO_CLK>,
+			<&gcc GCC_LPASS_SWAY_CLK>,
+			<&lpasscc LPASS_AUDIO_WRAPPER_AON_CLK>,
+			<&lpasscc LPASS_Q6SS_AHBS_AON_CLK>,
+			<&lpasscc LPASS_Q6SS_AHBM_AON_CLK>,
+			<&lpasscc LPASS_QDSP6SS_XO_CLK>,
+			<&lpasscc LPASS_QDSP6SS_SLEEP_CLK>,
+			<&lpasscc LPASS_QDSP6SS_CORE_CLK>;
+		clock-names = "xo", "sway_cbcr", "lpass_aon",
+			"lpass_ahbs_aon_cbcr",
+			"lpass_ahbm_aon_cbcr", "qdsp6ss_xo",
+			"qdsp6ss_sleep", "qdsp6ss_core";
+
+		power-domains = <&rpmhpd SDM845_CX>;
+
+		resets = <&pdc_reset PDC_AUDIO_SYNC_RESET>,
+			 <&aoss_reset AOSS_CC_LPASS_RESTART>;
+		reset-names = "pdc_sync", "cc_lpass";
+
+		qcom,halt-regs = <&tcsr_mutex_regs 0x22000>;
+
+		memory-region = <&pil_adsp_mem>;
+
+		qcom,smem-states = <&adsp_smp2p_out 0>;
+		qcom,smem-state-names = "stop";
+	};
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v3 2/2] remoteproc: qcom: Introduce Non-PAS ADSP PIL driver

[Rohit kumar]

This adds Non PAS ADSP PIL driver for Qualcomm
Technologies Inc SoCs.
Added initial support for SDM845 with ADSP bootup and
shutdown operation handled from Application Processor
SubSystem(APSS).

Signed-off-by: Rohit kumar <rohitkr@codeaurora.org>
---
 drivers/remoteproc/Kconfig         |  14 ++
 drivers/remoteproc/Makefile        |   1 +
 drivers/remoteproc/qcom_adsp_pil.c | 500 +++++++++++++++++++++++++++++++++++++
 3 files changed, 515 insertions(+)
 create mode 100644 drivers/remoteproc/qcom_adsp_pil.c

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index c98c0b2..445de2d 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -139,6 +139,20 @@ config QCOM_Q6V5_WCSS
 	  Say y here to support the Qualcomm Peripheral Image Loader for the
 	  Hexagon V5 based WCSS remote processors.
 
+config QCOM_ADSP_PIL
+	tristate "Qualcomm Technology Inc ADSP Peripheral Image Loader"
+	depends on OF && ARCH_QCOM
+	depends on QCOM_SMEM
+	depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n
+	depends on QCOM_SYSMON || QCOM_SYSMON=n
+	select MFD_SYSCON
+	select QCOM_MDT_LOADER
+	select QCOM_Q6V5_COMMON
+	select QCOM_RPROC_COMMON
+	help
+	  Say y here to support the Peripherial Image Loader
+	  for the Qualcomm Technology Inc. ADSP remote processors.
+
 config QCOM_SYSMON
 	tristate "Qualcomm sysmon driver"
 	depends on RPMSG
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index eb86c8b..1258519 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_QCOM_Q6V5_COMMON)		+= qcom_q6v5.o
 obj-$(CONFIG_QCOM_Q6V5_PAS)		+= qcom_q6v5_pas.o
 obj-$(CONFIG_QCOM_Q6V5_PIL)		+= qcom_q6v5_pil.o
 obj-$(CONFIG_QCOM_Q6V5_WCSS)		+= qcom_q6v5_wcss.o
+obj-$(CONFIG_QCOM_ADSP_PIL)		+= qcom_adsp_pil.o
 obj-$(CONFIG_QCOM_SYSMON)		+= qcom_sysmon.o
 obj-$(CONFIG_QCOM_WCNSS_PIL)		+= qcom_wcnss_pil.o
 qcom_wcnss_pil-y			+= qcom_wcnss.o
diff --git a/drivers/remoteproc/qcom_adsp_pil.c b/drivers/remoteproc/qcom_adsp_pil.c
new file mode 100644
index 0000000..977e804
--- /dev/null
+++ b/drivers/remoteproc/qcom_adsp_pil.c
@@ -0,0 +1,500 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Qualcomm Technology Inc. ADSP Peripheral Image Loader for SDM845.
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/clk.h>
+#include <linux/firmware.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/reset.h>
+#include <linux/iopoll.h>
+#include <linux/pm_domain.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/remoteproc.h>
+#include <linux/soc/qcom/mdt_loader.h>
+#include <linux/soc/qcom/smem.h>
+#include <linux/soc/qcom/smem_state.h>
+
+#include "qcom_common.h"
+#include "qcom_q6v5.h"
+#include "remoteproc_internal.h"
+
+/* time out value */
+#define ACK_TIMEOUT			1000
+#define BOOT_FSM_TIMEOUT		10000
+/* mask values */
+#define EVB_MASK			GENMASK(27, 4)
+/*QDSP6SS register offsets*/
+#define RST_EVB_REG			0x10
+#define CORE_START_REG			0x400
+#define BOOT_CMD_REG			0x404
+#define BOOT_STATUS_REG			0x408
+#define RET_CFG_REG			0x1C
+/*TCSR register offsets*/
+#define LPASS_MASTER_IDLE_REG		0x8
+#define LPASS_HALTACK_REG		0x4
+#define LPASS_PWR_ON_REG		0x10
+#define LPASS_HALTREQ_REG		0x0
+
+/* list of clocks required by ADSP PIL */
+static const char * const adsp_clk_id[] = {
+	"sway_cbcr", "lpass_aon", "lpass_ahbs_aon_cbcr", "lpass_ahbm_aon_cbcr",
+	"qdsp6ss_xo", "qdsp6ss_sleep", "qdsp6ss_core",
+};
+
+struct adsp_pil_data {
+	int crash_reason_smem;
+	const char *firmware_name;
+
+	const char *ssr_name;
+	const char *sysmon_name;
+	int ssctl_id;
+};
+
+struct qcom_adsp {
+	struct device *dev;
+	struct rproc *rproc;
+
+	struct qcom_q6v5 q6v5;
+
+	struct clk *xo;
+
+	int num_clks;
+	struct clk_bulk_data *clks;
+
+	void __iomem *qdsp6ss_base;
+
+	struct reset_control *pdc_sync_reset;
+	struct reset_control *cc_lpass_restart;
+
+	struct regmap *halt_map;
+	unsigned int  halt_lpass;
+
+	int crash_reason_smem;
+
+	struct completion start_done;
+	struct completion stop_done;
+
+	phys_addr_t mem_phys;
+	phys_addr_t mem_reloc;
+	void *mem_region;
+	size_t mem_size;
+
+	struct qcom_rproc_glink glink_subdev;
+	struct qcom_rproc_ssr ssr_subdev;
+	struct qcom_sysmon *sysmon;
+};
+
+static int qcom_adsp_shutdown(struct qcom_adsp *adsp)
+{
+	unsigned long timeout;
+	unsigned int val;
+	int ret;
+
+	/* Reset the retention logic */
+	val = readl(adsp->qdsp6ss_base + RET_CFG_REG);
+	val |= 0x1;
+	writel(val, adsp->qdsp6ss_base + RET_CFG_REG);
+
+	clk_bulk_disable_unprepare(adsp->num_clks, adsp->clks);
+
+	/* QDSP6 master port needs to be explicitly halted */
+	ret = regmap_read(adsp->halt_map,
+			adsp->halt_lpass + LPASS_PWR_ON_REG, &val);
+	if (ret || !val)
+		goto reset;
+
+	ret = regmap_read(adsp->halt_map,
+			adsp->halt_lpass + LPASS_MASTER_IDLE_REG,
+			&val);
+	if (ret || val)
+		goto reset;
+
+	regmap_write(adsp->halt_map,
+			adsp->halt_lpass + LPASS_HALTREQ_REG, 1);
+
+	/*  Wait for halt ACK from QDSP6 */
+	timeout = jiffies + msecs_to_jiffies(ACK_TIMEOUT);
+	for (;;) {
+		ret = regmap_read(adsp->halt_map,
+			adsp->halt_lpass + LPASS_HALTACK_REG, &val);
+		if (ret || val || time_after(jiffies, timeout))
+			break;
+
+		udelay(1000);
+	}
+
+	ret = regmap_read(adsp->halt_map,
+			adsp->halt_lpass + LPASS_MASTER_IDLE_REG, &val);
+	if (ret || !val)
+		dev_err(adsp->dev, "port failed halt\n");
+
+reset:
+	/* Assert the LPASS PDC Reset */
+	reset_control_assert(adsp->pdc_sync_reset);
+	/* Place the LPASS processor into reset */
+	reset_control_assert(adsp->cc_lpass_restart);
+	/* wait after asserting subsystem restart from AOSS */
+	udelay(200);
+
+	/* Clear the halt request for the AXIM and AHBM for Q6 */
+	regmap_write(adsp->halt_map, adsp->halt_lpass + LPASS_HALTREQ_REG, 0);
+
+	/* De-assert the LPASS PDC Reset */
+	reset_control_deassert(adsp->pdc_sync_reset);
+	/* Remove the LPASS reset */
+	reset_control_deassert(adsp->cc_lpass_restart);
+	/* wait after de-asserting subsystem restart from AOSS */
+	udelay(200);
+
+	return 0;
+}
+
+static int adsp_load(struct rproc *rproc, const struct firmware *fw)
+{
+	struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
+
+	return qcom_mdt_load_no_init(adsp->dev, fw, rproc->firmware, 0,
+			     adsp->mem_region, adsp->mem_phys, adsp->mem_size,
+			     &adsp->mem_reloc);
+}
+
+static int adsp_start(struct rproc *rproc)
+{
+	struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
+	int ret;
+	unsigned int val;
+
+	qcom_q6v5_prepare(&adsp->q6v5);
+
+	ret = clk_prepare_enable(adsp->xo);
+	if (ret)
+		return ret;
+
+	dev_pm_genpd_set_performance_state(adsp->dev, INT_MAX);
+	ret = pm_runtime_get_sync(adsp->dev);
+	if (ret)
+		goto disable_xo_clk;
+
+	ret = clk_bulk_prepare_enable(adsp->num_clks, adsp->clks);
+	if (ret) {
+		dev_err(adsp->dev, "adsp clk_enable failed\n");
+		goto disable_power_domain;
+	}
+
+	/* Program boot address */
+	writel(adsp->mem_phys >> 4, adsp->qdsp6ss_base + RST_EVB_REG);
+
+	/* De-assert QDSP6 stop core. QDSP6 will execute after out of reset */
+	writel(0x1, adsp->qdsp6ss_base + CORE_START_REG);
+
+	/* Trigger boot FSM to start QDSP6 */
+	writel(0x1, adsp->qdsp6ss_base + BOOT_CMD_REG);
+
+	/* Wait for core to come out of reset */
+	ret = readl_poll_timeout(adsp->qdsp6ss_base + BOOT_STATUS_REG,
+			val, (val & BIT(0)) != 0, 10, BOOT_FSM_TIMEOUT);
+	if (ret) {
+		dev_err(adsp->dev, "failed to bootup adsp\n");
+		goto disable_adsp_clks;
+	}
+
+	ret = qcom_q6v5_wait_for_start(&adsp->q6v5, msecs_to_jiffies(5 * HZ));
+	if (ret == -ETIMEDOUT) {
+		dev_err(adsp->dev, "start timed out\n");
+		goto disable_adsp_clks;
+	}
+
+	return 0;
+
+disable_adsp_clks:
+	clk_bulk_disable_unprepare(adsp->num_clks, adsp->clks);
+disable_power_domain:
+	dev_pm_genpd_set_performance_state(adsp->dev, 0);
+	pm_runtime_put(adsp->dev);
+disable_xo_clk:
+	clk_disable_unprepare(adsp->xo);
+
+	return ret;
+}
+
+static void qcom_adsp_pil_handover(struct qcom_q6v5 *q6v5)
+{
+	struct qcom_adsp *adsp = container_of(q6v5, struct qcom_adsp, q6v5);
+
+	clk_disable_unprepare(adsp->xo);
+	dev_pm_genpd_set_performance_state(adsp->dev, 0);
+	pm_runtime_put(adsp->dev);
+}
+
+static int adsp_stop(struct rproc *rproc)
+{
+	struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
+	int handover;
+	int ret;
+
+	ret = qcom_q6v5_request_stop(&adsp->q6v5);
+	if (ret == -ETIMEDOUT)
+		dev_err(adsp->dev, "timed out on wait\n");
+
+	ret = qcom_adsp_shutdown(adsp);
+	if (ret)
+		dev_err(adsp->dev, "failed to shutdown: %d\n", ret);
+
+	handover = qcom_q6v5_unprepare(&adsp->q6v5);
+	if (handover)
+		qcom_adsp_pil_handover(&adsp->q6v5);
+
+	return ret;
+}
+
+static void *adsp_da_to_va(struct rproc *rproc, u64 da, int len)
+{
+	struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
+	int offset;
+
+	offset = da - adsp->mem_reloc;
+	if (offset < 0 || offset + len > adsp->mem_size)
+		return NULL;
+
+	return adsp->mem_region + offset;
+}
+
+static const struct rproc_ops adsp_ops = {
+	.start = adsp_start,
+	.stop = adsp_stop,
+	.da_to_va = adsp_da_to_va,
+	.parse_fw = qcom_register_dump_segments,
+	.load = adsp_load,
+};
+
+static int adsp_init_clock(struct qcom_adsp *adsp)
+{
+	int i, ret;
+
+	adsp->xo = devm_clk_get(adsp->dev, "xo");
+	if (IS_ERR(adsp->xo)) {
+		ret = PTR_ERR(adsp->xo);
+		if (ret != -EPROBE_DEFER)
+			dev_err(adsp->dev, "failed to get xo clock");
+		return ret;
+	}
+
+	adsp->num_clks = ARRAY_SIZE(adsp_clk_id);
+	adsp->clks = devm_kcalloc(adsp->dev, adsp->num_clks,
+				sizeof(*adsp->clks), GFP_KERNEL);
+	if (IS_ERR(adsp->clks)) {
+		ret = PTR_ERR(adsp->clks);
+		if (ret != -EPROBE_DEFER)
+			dev_err(adsp->dev, "failed to get adsp clock");
+		return ret;
+	}
+
+	for (i = 0; i < adsp->num_clks; i++)
+		adsp->clks[i].id = adsp_clk_id[i];
+
+	return devm_clk_bulk_get(adsp->dev, adsp->num_clks, adsp->clks);
+}
+
+static int adsp_init_reset(struct qcom_adsp *adsp)
+{
+	adsp->pdc_sync_reset = devm_reset_control_get_exclusive(adsp->dev,
+			"pdc_sync");
+	if (IS_ERR(adsp->pdc_sync_reset)) {
+		dev_err(adsp->dev, "failed to acquire pdc_sync reset\n");
+		return PTR_ERR(adsp->pdc_sync_reset);
+	}
+
+	adsp->cc_lpass_restart = devm_reset_control_get_exclusive(adsp->dev,
+			"cc_lpass");
+	if (IS_ERR(adsp->cc_lpass_restart)) {
+		dev_err(adsp->dev, "failed to acquire cc_lpass restart\n");
+		return PTR_ERR(adsp->cc_lpass_restart);
+	}
+
+	return 0;
+}
+
+static int adsp_init_mmio(struct qcom_adsp *adsp,
+				struct platform_device *pdev)
+{
+	struct device_node *syscon;
+	struct resource *res;
+	int ret;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	adsp->qdsp6ss_base = devm_ioremap(&pdev->dev, res->start,
+			resource_size(res));
+	if (IS_ERR(adsp->qdsp6ss_base)) {
+		dev_err(adsp->dev, "failed to map QDSP6SS registers\n");
+		return PTR_ERR(adsp->qdsp6ss_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;
+	}
+
+	adsp->halt_map = syscon_node_to_regmap(syscon);
+	of_node_put(syscon);
+	if (IS_ERR(adsp->halt_map))
+		return PTR_ERR(adsp->halt_map);
+
+	ret = of_property_read_u32_index(pdev->dev.of_node, "qcom,halt-regs",
+			1, &adsp->halt_lpass);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "no offset in syscon\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int adsp_alloc_memory_region(struct qcom_adsp *adsp)
+{
+	struct device_node *node;
+	struct resource r;
+	int ret;
+
+	node = of_parse_phandle(adsp->dev->of_node, "memory-region", 0);
+	if (!node) {
+		dev_err(adsp->dev, "no memory-region specified\n");
+		return -EINVAL;
+	}
+
+	ret = of_address_to_resource(node, 0, &r);
+	if (ret)
+		return ret;
+
+	adsp->mem_phys = adsp->mem_reloc = r.start;
+	adsp->mem_size = resource_size(&r);
+	adsp->mem_region = devm_ioremap_wc(adsp->dev,
+				adsp->mem_phys, adsp->mem_size);
+	if (!adsp->mem_region) {
+		dev_err(adsp->dev, "unable to map memory region: %pa+%zx\n",
+			&r.start, adsp->mem_size);
+		return -EBUSY;
+	}
+
+	return 0;
+}
+
+static int adsp_probe(struct platform_device *pdev)
+{
+	const struct adsp_pil_data *desc;
+	struct qcom_adsp *adsp;
+	struct rproc *rproc;
+	int ret;
+
+	desc = of_device_get_match_data(&pdev->dev);
+	if (!desc)
+		return -EINVAL;
+
+	rproc = rproc_alloc(&pdev->dev, pdev->name, &adsp_ops,
+			    desc->firmware_name, sizeof(*adsp));
+	if (!rproc) {
+		dev_err(&pdev->dev, "unable to allocate remoteproc\n");
+		return -ENOMEM;
+	}
+
+	adsp = (struct qcom_adsp *)rproc->priv;
+	adsp->dev = &pdev->dev;
+	adsp->rproc = rproc;
+	platform_set_drvdata(pdev, adsp);
+
+	ret = adsp_alloc_memory_region(adsp);
+	if (ret)
+		goto free_rproc;
+
+	ret = adsp_init_clock(adsp);
+	if (ret)
+		goto free_rproc;
+
+	pm_runtime_enable(adsp->dev);
+
+	ret = adsp_init_reset(adsp);
+	if (ret)
+		goto disable_pm;
+
+	ret = adsp_init_mmio(adsp, pdev);
+	if (ret)
+		goto disable_pm;
+
+	ret = qcom_q6v5_init(&adsp->q6v5, pdev, rproc, desc->crash_reason_smem,
+			     qcom_adsp_pil_handover);
+	if (ret)
+		goto disable_pm;
+
+	qcom_add_glink_subdev(rproc, &adsp->glink_subdev);
+	qcom_add_ssr_subdev(rproc, &adsp->ssr_subdev, desc->ssr_name);
+	adsp->sysmon = qcom_add_sysmon_subdev(rproc,
+					      desc->sysmon_name,
+					      desc->ssctl_id);
+
+	ret = rproc_add(rproc);
+	if (ret)
+		goto disable_pm;
+
+	return 0;
+
+disable_pm:
+	pm_runtime_disable(adsp->dev);
+free_rproc:
+	rproc_free(rproc);
+
+	return ret;
+}
+
+static int adsp_remove(struct platform_device *pdev)
+{
+	struct qcom_adsp *adsp = platform_get_drvdata(pdev);
+
+	rproc_del(adsp->rproc);
+
+	qcom_remove_glink_subdev(adsp->rproc, &adsp->glink_subdev);
+	qcom_remove_sysmon_subdev(adsp->sysmon);
+	qcom_remove_ssr_subdev(adsp->rproc, &adsp->ssr_subdev);
+	rproc_free(adsp->rproc);
+	pm_runtime_disable(adsp->dev);
+
+	return 0;
+}
+
+static const struct adsp_pil_data adsp_resource_init = {
+	.crash_reason_smem = 423,
+	.firmware_name = "adsp.mdt",
+	.ssr_name = "lpass",
+	.sysmon_name = "adsp",
+	.ssctl_id = 0x14,
+};
+
+static const struct of_device_id adsp_of_match[] = {
+	{ .compatible = "qcom,sdm845-adsp-pil",
+	  .data = &adsp_resource_init},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, adsp_of_match);
+
+static struct platform_driver adsp_pil_driver = {
+	.probe = adsp_probe,
+	.remove = adsp_remove,
+	.driver = {
+		.name = "qcom_adsp_pil",
+		.of_match_table = adsp_of_match,
+	},
+};
+
+module_platform_driver(adsp_pil_driver);
+MODULE_DESCRIPTION("QTi SDM845 ADSP Peripherial Image Loader");
+MODULE_LICENSE("GPL v2");
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v3 1/2] dt-binding: remoteproc: Add QTI ADSP PIL bindings

[Bjorn Andersson]

On Mon 03 Sep 04:52 PDT 2018, Rohit kumar wrote:

> Add devicetree bindings documentation file for Qualcomm
> Technolgies Inc ADSP Peripheral Image Loader.
> 
> Signed-off-by: Rohit kumar <rohitkr@codeaurora.org>

Rob, this revision looks good to me and I would like to move ahead and
merge it.

Regards,
Bjorn

> ---
>  .../bindings/remoteproc/qcom,adsp-pil.txt          | 123 +++++++++++++++++++++
>  1 file changed, 123 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,adsp-pil.txt
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,adsp-pil.txt b/Documentation/devicetree/bindings/remoteproc/qcom,adsp-pil.txt
> new file mode 100644
> index 0000000..f1c215a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,adsp-pil.txt
> @@ -0,0 +1,123 @@
> +Qualcomm Technology Inc. ADSP Peripheral Image Loader
> +
> +This document defines the binding for a component that loads and boots firmware
> +on the Qualcomm Technology Inc. ADSP Hexagon core.
> +
> +- compatible:
> +	Usage: required
> +	Value type: <string>
> +	Definition: must be one of:
> +		    "qcom,sdm845-adsp-pil"
> +
> +- reg:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: must specify the base address and size of the qdsp6ss register
> +
> +- interrupts-extended:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: must list the watchdog, fatal IRQs ready, handover and
> +		    stop-ack IRQs
> +
> +- interrupt-names:
> +	Usage: required
> +	Value type: <stringlist>
> +	Definition: must be "wdog", "fatal", "ready", "handover", "stop-ack"
> +
> +- clocks:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition:  List of phandle and clock specifier pairs
> +
> +- clock-names:
> +	Usage: required
> +	Value type: <stringlist>
> +	Definition: List of clock input name strings sorted in the same
> +				order as the clocks property.
> +
> +- power-domains:
> +	Usage: required
> +	Value type: <phandle>
> +	Definition: reference to cx power domain node.
> +
> +- resets:
> +	Usage: required
> +	Value type: <phandle>
> +	Definition: reference to the reset-controller for the lpass
> +
> +- reset-names:
> +        Usage: required
> +        Value type: <stringlist>
> +        Definition: must be "pdc_sync" and "cc_lpass"
> +
> +- qcom,halt-regs:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: a phandle reference to a syscon representing TCSR followed
> +			by the offset within syscon for lpass halt register.
> +
> +- memory-region:
> +	Usage: required
> +	Value type: <phandle>
> +	Definition: reference to the reserved-memory for the ADSP
> +
> +- qcom,smem-states:
> +	Usage: required
> +	Value type: <phandle>
> +	Definition: reference to the smem state for requesting the ADSP to
> +		    shut down
> +
> +- qcom,smem-state-names:
> +	Usage: required
> +	Value type: <stringlist>
> +	Definition: must be "stop"
> +
> +
> += SUBNODES
> +The adsp node may have an subnode named "glink-edge" that describes the
> +communication edge, channels and devices related to the ADSP.
> +See ../soc/qcom/qcom,glink.txt for details on how to describe these.
> +
> += EXAMPLE
> +The following example describes the resources needed to boot control the
> +ADSP, as it is found on SDM845 boards.
> +	adsp-pil {
> +		compatible = "qcom,sdm845-adsp-pil";
> +
> +		reg = <0x17300000 0x40c>;
> +
> +		interrupts-extended = <&intc 0 162 IRQ_TYPE_EDGE_RISING>,
> +			<&adsp_smp2p_in 0 IRQ_TYPE_EDGE_RISING>,
> +			<&adsp_smp2p_in 1 IRQ_TYPE_EDGE_RISING>,
> +			<&adsp_smp2p_in 2 IRQ_TYPE_EDGE_RISING>,
> +			<&adsp_smp2p_in 3 IRQ_TYPE_EDGE_RISING>;
> +		interrupt-names = "wdog", "fatal", "ready",
> +			"handover", "stop-ack";
> +
> +		clocks = <&rpmhcc RPMH_CXO_CLK>,
> +			<&gcc GCC_LPASS_SWAY_CLK>,
> +			<&lpasscc LPASS_AUDIO_WRAPPER_AON_CLK>,
> +			<&lpasscc LPASS_Q6SS_AHBS_AON_CLK>,
> +			<&lpasscc LPASS_Q6SS_AHBM_AON_CLK>,
> +			<&lpasscc LPASS_QDSP6SS_XO_CLK>,
> +			<&lpasscc LPASS_QDSP6SS_SLEEP_CLK>,
> +			<&lpasscc LPASS_QDSP6SS_CORE_CLK>;
> +		clock-names = "xo", "sway_cbcr", "lpass_aon",
> +			"lpass_ahbs_aon_cbcr",
> +			"lpass_ahbm_aon_cbcr", "qdsp6ss_xo",
> +			"qdsp6ss_sleep", "qdsp6ss_core";
> +
> +		power-domains = <&rpmhpd SDM845_CX>;
> +
> +		resets = <&pdc_reset PDC_AUDIO_SYNC_RESET>,
> +			 <&aoss_reset AOSS_CC_LPASS_RESTART>;
> +		reset-names = "pdc_sync", "cc_lpass";
> +
> +		qcom,halt-regs = <&tcsr_mutex_regs 0x22000>;
> +
> +		memory-region = <&pil_adsp_mem>;
> +
> +		qcom,smem-states = <&adsp_smp2p_out 0>;
> +		qcom,smem-state-names = "stop";
> +	};
> -- 
> Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
> is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
> 

Re: [PATCH v3 2/2] remoteproc: qcom: Introduce Non-PAS ADSP PIL driver

[Bjorn Andersson]

On Mon 03 Sep 04:52 PDT 2018, Rohit kumar wrote:

> This adds Non PAS ADSP PIL driver for Qualcomm
> Technologies Inc SoCs.
> Added initial support for SDM845 with ADSP bootup and
> shutdown operation handled from Application Processor
> SubSystem(APSS).
> 
> Signed-off-by: Rohit kumar <rohitkr@codeaurora.org>

Thanks for the changes Rohit, this looks good.

Once we hear from DT maintainers that patch 1 can be applied I will
update the name of the file and driver as I apply it to match the naming
scheme I'm aiming for - no need for you to resend because of this.

> ---
>  drivers/remoteproc/Kconfig         |  14 ++
>  drivers/remoteproc/Makefile        |   1 +
>  drivers/remoteproc/qcom_adsp_pil.c | 500 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 515 insertions(+)
>  create mode 100644 drivers/remoteproc/qcom_adsp_pil.c
> 
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index c98c0b2..445de2d 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -139,6 +139,20 @@ config QCOM_Q6V5_WCSS
>  	  Say y here to support the Qualcomm Peripheral Image Loader for the
>  	  Hexagon V5 based WCSS remote processors.
>  
> +config QCOM_ADSP_PIL

I will make this QCOM_Q6V5_ADSP

[..]
> diff --git a/drivers/remoteproc/qcom_adsp_pil.c b/drivers/remoteproc/qcom_adsp_pil.c

Make this qcom_q6v5_adsp.c

[..]
> +static struct platform_driver adsp_pil_driver = {
> +	.probe = adsp_probe,
> +	.remove = adsp_remove,
> +	.driver = {
> +		.name = "qcom_adsp_pil",

and this qcom_q6v5_adsp".

> +		.of_match_table = adsp_of_match,
> +	},
> +};

Please let me know if you have any objections to this.

Regards,
Bjorn

Re: [PATCH v3 1/2] dt-binding: remoteproc: Add QTI ADSP PIL bindings

[Rob Herring]

On Mon, Sep 03, 2018 at 05:22:39PM +0530, Rohit kumar wrote:
> Add devicetree bindings documentation file for Qualcomm
> Technolgies Inc ADSP Peripheral Image Loader.
> 
> Signed-off-by: Rohit kumar <rohitkr@codeaurora.org>
> ---
>  .../bindings/remoteproc/qcom,adsp-pil.txt          | 123 +++++++++++++++++++++
>  1 file changed, 123 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,adsp-pil.txt
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,adsp-pil.txt b/Documentation/devicetree/bindings/remoteproc/qcom,adsp-pil.txt
> new file mode 100644
> index 0000000..f1c215a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,adsp-pil.txt
> @@ -0,0 +1,123 @@
> +Qualcomm Technology Inc. ADSP Peripheral Image Loader
> +
> +This document defines the binding for a component that loads and boots firmware
> +on the Qualcomm Technology Inc. ADSP Hexagon core.
> +
> +- compatible:
> +	Usage: required
> +	Value type: <string>
> +	Definition: must be one of:
> +		    "qcom,sdm845-adsp-pil"
> +
> +- reg:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: must specify the base address and size of the qdsp6ss register
> +
> +- interrupts-extended:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: must list the watchdog, fatal IRQs ready, handover and
> +		    stop-ack IRQs
> +
> +- interrupt-names:
> +	Usage: required
> +	Value type: <stringlist>
> +	Definition: must be "wdog", "fatal", "ready", "handover", "stop-ack"
> +
> +- clocks:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition:  List of phandle and clock specifier pairs

How many clocks?

> +
> +- clock-names:
> +	Usage: required
> +	Value type: <stringlist>
> +	Definition: List of clock input name strings sorted in the same
> +				order as the clocks property.

What are the names?

> +
> +- power-domains:
> +	Usage: required
> +	Value type: <phandle>
> +	Definition: reference to cx power domain node.
> +
> +- resets:
> +	Usage: required
> +	Value type: <phandle>
> +	Definition: reference to the reset-controller for the lpass

How many?

> +
> +- reset-names:
> +        Usage: required
> +        Value type: <stringlist>
> +        Definition: must be "pdc_sync" and "cc_lpass"
> +
> +- qcom,halt-regs:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: a phandle reference to a syscon representing TCSR followed
> +			by the offset within syscon for lpass halt register.
> +
> +- memory-region:
> +	Usage: required
> +	Value type: <phandle>
> +	Definition: reference to the reserved-memory for the ADSP
> +
> +- qcom,smem-states:
> +	Usage: required
> +	Value type: <phandle>
> +	Definition: reference to the smem state for requesting the ADSP to
> +		    shut down
> +
> +- qcom,smem-state-names:
> +	Usage: required
> +	Value type: <stringlist>
> +	Definition: must be "stop"
> +
> +
> += SUBNODES
> +The adsp node may have an subnode named "glink-edge" that describes the
> +communication edge, channels and devices related to the ADSP.
> +See ../soc/qcom/qcom,glink.txt for details on how to describe these.
> +
> += EXAMPLE
> +The following example describes the resources needed to boot control the
> +ADSP, as it is found on SDM845 boards.
> +	adsp-pil {
> +		compatible = "qcom,sdm845-adsp-pil";
> +
> +		reg = <0x17300000 0x40c>;
> +
> +		interrupts-extended = <&intc 0 162 IRQ_TYPE_EDGE_RISING>,
> +			<&adsp_smp2p_in 0 IRQ_TYPE_EDGE_RISING>,
> +			<&adsp_smp2p_in 1 IRQ_TYPE_EDGE_RISING>,
> +			<&adsp_smp2p_in 2 IRQ_TYPE_EDGE_RISING>,
> +			<&adsp_smp2p_in 3 IRQ_TYPE_EDGE_RISING>;
> +		interrupt-names = "wdog", "fatal", "ready",
> +			"handover", "stop-ack";
> +
> +		clocks = <&rpmhcc RPMH_CXO_CLK>,
> +			<&gcc GCC_LPASS_SWAY_CLK>,
> +			<&lpasscc LPASS_AUDIO_WRAPPER_AON_CLK>,
> +			<&lpasscc LPASS_Q6SS_AHBS_AON_CLK>,
> +			<&lpasscc LPASS_Q6SS_AHBM_AON_CLK>,
> +			<&lpasscc LPASS_QDSP6SS_XO_CLK>,
> +			<&lpasscc LPASS_QDSP6SS_SLEEP_CLK>,
> +			<&lpasscc LPASS_QDSP6SS_CORE_CLK>;
> +		clock-names = "xo", "sway_cbcr", "lpass_aon",
> +			"lpass_ahbs_aon_cbcr",
> +			"lpass_ahbm_aon_cbcr", "qdsp6ss_xo",
> +			"qdsp6ss_sleep", "qdsp6ss_core";
> +
> +		power-domains = <&rpmhpd SDM845_CX>;
> +
> +		resets = <&pdc_reset PDC_AUDIO_SYNC_RESET>,
> +			 <&aoss_reset AOSS_CC_LPASS_RESTART>;
> +		reset-names = "pdc_sync", "cc_lpass";
> +
> +		qcom,halt-regs = <&tcsr_mutex_regs 0x22000>;
> +
> +		memory-region = <&pil_adsp_mem>;
> +
> +		qcom,smem-states = <&adsp_smp2p_out 0>;
> +		qcom,smem-state-names = "stop";
> +	};
> -- 
> Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
> is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
> 

Re: [PATCH v3 1/2] dt-binding: remoteproc: Add QTI ADSP PIL bindings

[Rohit Kumar]

Thanks Rob for reviewing.


On 9/11/2018 1:31 AM, Rob Herring wrote:
> On Mon, Sep 03, 2018 at 05:22:39PM +0530, Rohit kumar wrote:
>> Add devicetree bindings documentation file for Qualcomm
>> Technolgies Inc ADSP Peripheral Image Loader.
>>
>> Signed-off-by: Rohit kumar <rohitkr@codeaurora.org>
>> ---
>>   .../bindings/remoteproc/qcom,adsp-pil.txt          | 123 +++++++++++++++++++++
>>   1 file changed, 123 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,adsp-pil.txt
>>
>> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,adsp-pil.txt b/Documentation/devicetree/bindings/remoteproc/qcom,adsp-pil.txt
>> new file mode 100644
>> index 0000000..f1c215a
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,adsp-pil.txt
>> @@ -0,0 +1,123 @@
>> +Qualcomm Technology Inc. ADSP Peripheral Image Loader
>> +
>> +This document defines the binding for a component that loads and boots firmware
>> +on the Qualcomm Technology Inc. ADSP Hexagon core.
>> +
>> +- compatible:
>> +	Usage: required
>> +	Value type: <string>
>> +	Definition: must be one of:
>> +		    "qcom,sdm845-adsp-pil"
>> +
>> +- reg:
>> +	Usage: required
>> +	Value type: <prop-encoded-array>
>> +	Definition: must specify the base address and size of the qdsp6ss register
>> +
>> +- interrupts-extended:
>> +	Usage: required
>> +	Value type: <prop-encoded-array>
>> +	Definition: must list the watchdog, fatal IRQs ready, handover and
>> +		    stop-ack IRQs
>> +
>> +- interrupt-names:
>> +	Usage: required
>> +	Value type: <stringlist>
>> +	Definition: must be "wdog", "fatal", "ready", "handover", "stop-ack"
>> +
>> +- clocks:
>> +	Usage: required
>> +	Value type: <prop-encoded-array>
>> +	Definition:  List of phandle and clock specifier pairs
> How many clocks?
>
>> +
>> +- clock-names:
>> +	Usage: required
>> +	Value type: <stringlist>
>> +	Definition: List of clock input name strings sorted in the same
>> +				order as the clocks property.
> What are the names?

I will update these in next spin.
>> +
>> +- power-domains:
>> +	Usage: required
>> +	Value type: <phandle>
>> +	Definition: reference to cx power domain node.
>> +
>> +- resets:
>> +	Usage: required
>> +	Value type: <phandle>
>> +	Definition: reference to the reset-controller for the lpass
> How many?
>
>> +
>> +- reset-names:
>> +        Usage: required
>> +        Value type: <stringlist>
>> +        Definition: must be "pdc_sync" and "cc_lpass"
>> +
>> +- qcom,halt-regs:
>> +	Usage: required
>> +	Value type: <prop-encoded-array>
>> +	Definition: a phandle reference to a syscon representing TCSR followed
>> +			by the offset within syscon for lpass halt register.
>> +
>> +- memory-region:
>> +	Usage: required
>> +	Value type: <phandle>
>> +	Definition: reference to the reserved-memory for the ADSP
>> +
>> +- qcom,smem-states:
>> +	Usage: required
>> +	Value type: <phandle>
>> +	Definition: reference to the smem state for requesting the ADSP to
>> +		    shut down
>> +
>> +- qcom,smem-state-names:
>> +	Usage: required
>> +	Value type: <stringlist>
>> +	Definition: must be "stop"
>> +
>> +
>> += SUBNODES
>> +The adsp node may have an subnode named "glink-edge" that describes the
>> +communication edge, channels and devices related to the ADSP.
>> +See ../soc/qcom/qcom,glink.txt for details on how to describe these.
>> +
>> += EXAMPLE
>> +The following example describes the resources needed to boot control the
>> +ADSP, as it is found on SDM845 boards.
>> +	adsp-pil {
>> +		compatible = "qcom,sdm845-adsp-pil";
>> +
>> +		reg = <0x17300000 0x40c>;
>> +
>> +		interrupts-extended = <&intc 0 162 IRQ_TYPE_EDGE_RISING>,
>> +			<&adsp_smp2p_in 0 IRQ_TYPE_EDGE_RISING>,
>> +			<&adsp_smp2p_in 1 IRQ_TYPE_EDGE_RISING>,
>> +			<&adsp_smp2p_in 2 IRQ_TYPE_EDGE_RISING>,
>> +			<&adsp_smp2p_in 3 IRQ_TYPE_EDGE_RISING>;
>> +		interrupt-names = "wdog", "fatal", "ready",
>> +			"handover", "stop-ack";
>> +
>> +		clocks = <&rpmhcc RPMH_CXO_CLK>,
>> +			<&gcc GCC_LPASS_SWAY_CLK>,
>> +			<&lpasscc LPASS_AUDIO_WRAPPER_AON_CLK>,
>> +			<&lpasscc LPASS_Q6SS_AHBS_AON_CLK>,
>> +			<&lpasscc LPASS_Q6SS_AHBM_AON_CLK>,
>> +			<&lpasscc LPASS_QDSP6SS_XO_CLK>,
>> +			<&lpasscc LPASS_QDSP6SS_SLEEP_CLK>,
>> +			<&lpasscc LPASS_QDSP6SS_CORE_CLK>;
>> +		clock-names = "xo", "sway_cbcr", "lpass_aon",
>> +			"lpass_ahbs_aon_cbcr",
>> +			"lpass_ahbm_aon_cbcr", "qdsp6ss_xo",
>> +			"qdsp6ss_sleep", "qdsp6ss_core";
>> +
>> +		power-domains = <&rpmhpd SDM845_CX>;
>> +
>> +		resets = <&pdc_reset PDC_AUDIO_SYNC_RESET>,
>> +			 <&aoss_reset AOSS_CC_LPASS_RESTART>;
>> +		reset-names = "pdc_sync", "cc_lpass";
>> +
>> +		qcom,halt-regs = <&tcsr_mutex_regs 0x22000>;
>> +
>> +		memory-region = <&pil_adsp_mem>;
>> +
>> +		qcom,smem-states = <&adsp_smp2p_out 0>;
>> +		qcom,smem-state-names = "stop";
>> +	};
>> -- 
>> Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
>> is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
Thanks,
Rohit

Re: [PATCH v3 2/2] remoteproc: qcom: Introduce Non-PAS ADSP PIL driver

[Rohit Kumar]

Thanks Bjorn for reviewing.


On 9/11/2018 12:01 AM, Bjorn Andersson wrote:
> On Mon 03 Sep 04:52 PDT 2018, Rohit kumar wrote:
>
>> This adds Non PAS ADSP PIL driver for Qualcomm
>> Technologies Inc SoCs.
>> Added initial support for SDM845 with ADSP bootup and
>> shutdown operation handled from Application Processor
>> SubSystem(APSS).
>>
>> Signed-off-by: Rohit kumar <rohitkr@codeaurora.org>
> Thanks for the changes Rohit, this looks good.
>
> Once we hear from DT maintainers that patch 1 can be applied I will
> update the name of the file and driver as I apply it to match the naming
> scheme I'm aiming for - no need for you to resend because of this.
Sure, I will just update dt-bindings with addressing some comments given 
by Rob.

>> ---
>>   drivers/remoteproc/Kconfig         |  14 ++
>>   drivers/remoteproc/Makefile        |   1 +
>>   drivers/remoteproc/qcom_adsp_pil.c | 500 +++++++++++++++++++++++++++++++++++++
>>   3 files changed, 515 insertions(+)
>>   create mode 100644 drivers/remoteproc/qcom_adsp_pil.c
>>
>> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
>> index c98c0b2..445de2d 100644
>> --- a/drivers/remoteproc/Kconfig
>> +++ b/drivers/remoteproc/Kconfig
>> @@ -139,6 +139,20 @@ config QCOM_Q6V5_WCSS
>>   	  Say y here to support the Qualcomm Peripheral Image Loader for the
>>   	  Hexagon V5 based WCSS remote processors.
>>   
>> +config QCOM_ADSP_PIL
> I will make this QCOM_Q6V5_ADSP
>
> [..]
>> diff --git a/drivers/remoteproc/qcom_adsp_pil.c b/drivers/remoteproc/qcom_adsp_pil.c
> Make this qcom_q6v5_adsp.c
>
> [..]
>> +static struct platform_driver adsp_pil_driver = {
>> +	.probe = adsp_probe,
>> +	.remove = adsp_remove,
>> +	.driver = {
>> +		.name = "qcom_adsp_pil",
> and this qcom_q6v5_adsp".
>
>> +		.of_match_table = adsp_of_match,
>> +	},
>> +};
> Please let me know if you have any objections to this.
Naming looks fine.

Thanks,
Rohit
> Regards,
> Bjorn

Re: [PATCH v3 2/2] remoteproc: qcom: Introduce Non-PAS ADSP PIL driver

[Sibi Sankar]

Hi Rohit,

On 2018-09-03 17:22, Rohit kumar wrote:
> This adds Non PAS ADSP PIL driver for Qualcomm
> Technologies Inc SoCs.
> Added initial support for SDM845 with ADSP bootup and
> shutdown operation handled from Application Processor
> SubSystem(APSS).
> 
> Signed-off-by: Rohit kumar <rohitkr@codeaurora.org>
> ---
....
> +	select QCOM_MDT_LOADER
> +	select QCOM_Q6V5_COMMON
> +	select QCOM_RPROC_COMMON
> +	help
> +	  Say y here to support the Peripherial Image Loader

replace Peripherial/Peripheral.
....
> +#include <linux/clk.h>
> +#include <linux/firmware.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/reset.h>
> +#include <linux/iopoll.h>
> +#include <linux/pm_domain.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/remoteproc.h>
> +#include <linux/soc/qcom/mdt_loader.h>
> +#include <linux/soc/qcom/smem.h>
> +#include <linux/soc/qcom/smem_state.h>
> +

The headers should be in alphabetical order.

....
> +	struct reset_control *pdc_sync_reset;
> +	struct reset_control *cc_lpass_restart;
> +
> +	struct regmap *halt_map;
> +	unsigned int  halt_lpass;

remove the double spaces above.
....
> +	if (ret || val)
> +		goto reset;
> +
> +	regmap_write(adsp->halt_map,
> +			adsp->halt_lpass + LPASS_HALTREQ_REG, 1);
> +
> +	/*  Wait for halt ACK from QDSP6 */

Minor nit, please remove the double spaces in the comment above.

> +	timeout = jiffies + msecs_to_jiffies(ACK_TIMEOUT);
> +	for (;;) {
> +		ret = regmap_read(adsp->halt_map,
> +			adsp->halt_lpass + LPASS_HALTACK_REG, &val);
> +		if (ret || val || time_after(jiffies, timeout))
> +			break;
> +
> +		udelay(1000);

According to Documentation/timers/timers-howto.txt please use 
usleep_range()
when the delay range is between(10us - 20ms).

> +	}
> +
> +	ret = regmap_read(adsp->halt_map,
> +			adsp->halt_lpass + LPASS_MASTER_IDLE_REG, &val);
....
> +	/* wait after asserting subsystem restart from AOSS */
> +	udelay(200);

ditto

> +
> +	/* Clear the halt request for the AXIM and AHBM for Q6 */
> +	regmap_write(adsp->halt_map, adsp->halt_lpass + LPASS_HALTREQ_REG, 
> 0);
> +
> +	/* De-assert the LPASS PDC Reset */
> +	reset_control_deassert(adsp->pdc_sync_reset);
> +	/* Remove the LPASS reset */
> +	reset_control_deassert(adsp->cc_lpass_restart);
> +	/* wait after de-asserting subsystem restart from AOSS */
> +	udelay(200);

ditto

> +
> +	return 0;
> +}
....
> +static int adsp_start(struct rproc *rproc)
> +{
> +	struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
> +	int ret;
> +	unsigned int val;
> +
> +	qcom_q6v5_prepare(&adsp->q6v5);
> +
> +	ret = clk_prepare_enable(adsp->xo);
> +	if (ret)
> +		return ret;

please call qcom_q6v5_unprepare on clk_prepare_enable failure to 
disable_irqs

> +
> +	dev_pm_genpd_set_performance_state(adsp->dev, INT_MAX);
> +	ret = pm_runtime_get_sync(adsp->dev);
> +	if (ret)
> +		goto disable_xo_clk;
> +
....
> +static int adsp_init_reset(struct qcom_adsp *adsp)
> +{
> +	adsp->pdc_sync_reset = devm_reset_control_get_exclusive(adsp->dev,
> +			"pdc_sync");
> +	if (IS_ERR(adsp->pdc_sync_reset)) {
> +		dev_err(adsp->dev, "failed to acquire pdc_sync reset\n");
> +		return PTR_ERR(adsp->pdc_sync_reset);
> +	}

Bjorn, should we return EPROBE_DEFER here since PDC global reset 
controller can be a module?

> +
> +	adsp->cc_lpass_restart = devm_reset_control_get_exclusive(adsp->dev,
> +			"cc_lpass");
> +	if (IS_ERR(adsp->cc_lpass_restart)) {
> +		dev_err(adsp->dev, "failed to acquire cc_lpass restart\n");
> +		return PTR_ERR(adsp->cc_lpass_restart);
> +	}
> +
> +	return 0;
....
> +static int adsp_remove(struct platform_device *pdev)
> +{
> +	struct qcom_adsp *adsp = platform_get_drvdata(pdev);
> +
> +	rproc_del(adsp->rproc);
> +
> +	qcom_remove_glink_subdev(adsp->rproc, &adsp->glink_subdev);
> +	qcom_remove_sysmon_subdev(adsp->sysmon);
> +	qcom_remove_ssr_subdev(adsp->rproc, &adsp->ssr_subdev);
> +	rproc_free(adsp->rproc);
> +	pm_runtime_disable(adsp->dev);
> +

rmmod of the driver resulted in the following kernel panic:
having a pm_runtime_disable after rproc_free seems to be the cause of 
the kernel panic.
Please call pm_runtime_disable before rproc_free.

do_raw_spin_lock+0x28/0x118
__raw_spin_lock_irq+0x30/0x3c
__pm_runtime_disable+0x28/0xf4
adsp_remove+0x4c/0x5c [qcom_adsp_pil]
platform_drv_remove+0x28/0x50
device_release_driver_internal+0x124/0x1c8
driver_detach+0x44/0x80
bus_remove_driver+0x78/0x9c
driver_unregister+0x34/0x54
platform_driver_unregister+0x1c/0x28
cleanup_module+0x14/0x6bc [qcom_adsp_pil]
SyS_delete_module+0x1c4/0x214

> +	return 0;
> +}
> +
> +static const struct adsp_pil_data adsp_resource_init = {
> +	.crash_reason_smem = 423,
> +	.firmware_name = "adsp.mdt",
> +	.ssr_name = "lpass",
> +	.sysmon_name = "adsp",
> +	.ssctl_id = 0x14,
> +};
....
> +module_platform_driver(adsp_pil_driver);
> +MODULE_DESCRIPTION("QTi SDM845 ADSP Peripherial Image Loader");

replace QTi/QTI and Peripherial/Peripheral.

....
Also I see the following warns on stopping the adsp remoteproc, couldn't 
root cause it though:
  device_del+0x84/0x29c
  platform_device_del+0x2c/0x88
  platform_device_unregister+0x1c/0x30
  of_platform_device_destroy+0x98/0xa8
  device_for_each_child+0x54/0xa4
  of_platform_depopulate+0x38/0x54
  q6asm_remove+0x1c/0x2c
  apr_device_remove+0x38/0x70
  device_release_driver_internal+0x124/0x1c8
  device_release_driver+0x24/0x30
  bus_remove_device+0xcc/0xe4
  device_del+0x1f8/0x29c
  device_unregister+0x1c/0x30
  apr_remove_device+0x1c/0x2c
  device_for_each_child+0x54/0xa4
  apr_remove+0x28/0x34
  rpmsg_dev_remove+0x48/0x70
  device_release_driver_internal+0x124/0x1c8
  device_release_driver+0x24/0x30
  bus_remove_device+0xcc/0xe4
  device_del+0x1f8/0x29c
  device_unregister+0x1c/0x30
  qcom_glink_remove_device+0x1c/0x2c
  device_for_each_child+0x54/0xa4
  qcom_glink_native_remove+0x54/0x15c
  qcom_glink_smem_unregister+0x1c/0x30
  glink_subdev_stop+0x1c/0x2c [qcom_common]
  rproc_stop+0x40/0xc0
  rproc_shutdown+0x6c/0xc0
  rproc_del+0x28/0xa0
  adsp_remove+0x20/0x5c [qcom_adsp_pil]
  platform_drv_remove+0x28/0x50
  device_release_driver_internal+0x124/0x1c8
  driver_detach+0x44/0x80
  bus_remove_driver+0x78/0x9c
  driver_unregister+0x34/0x54
  platform_driver_unregister+0x1c/0x28
  cleanup_module+0x14/0x6bc [qcom_adsp_pil]
  SyS_delete_module+0x1c4/0x214

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

Re: [PATCH v3 2/2] remoteproc: qcom: Introduce Non-PAS ADSP PIL driver

[Rohit Kumar]

Thanks Sibi for reviewing.


On 9/22/2018 1:11 AM, Sibi Sankar wrote:
> Hi Rohit,
>
> On 2018-09-03 17:22, Rohit kumar wrote:
>> This adds Non PAS ADSP PIL driver for Qualcomm
>> Technologies Inc SoCs.
>> Added initial support for SDM845 with ADSP bootup and
>> shutdown operation handled from Application Processor
>> SubSystem(APSS).
>>
>> Signed-off-by: Rohit kumar <rohitkr@codeaurora.org>
>> ---
> ....
>> +    select QCOM_MDT_LOADER
>> +    select QCOM_Q6V5_COMMON
>> +    select QCOM_RPROC_COMMON
>> +    help
>> +      Say y here to support the Peripherial Image Loader
>
> replace Peripherial/Peripheral.

sure
> ....
>> +#include <linux/clk.h>
>> +#include <linux/firmware.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/delay.h>
>> +#include <linux/io.h>
>> +#include <linux/reset.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/pm_domain.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/regmap.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/remoteproc.h>
>> +#include <linux/soc/qcom/mdt_loader.h>
>> +#include <linux/soc/qcom/smem.h>
>> +#include <linux/soc/qcom/smem_state.h>
>> +
>
> The headers should be in alphabetical order.
>

ok, will do
> ....
>> +    struct reset_control *pdc_sync_reset;
>> +    struct reset_control *cc_lpass_restart;
>> +
>> +    struct regmap *halt_map;
>> +    unsigned int  halt_lpass;
>
> remove the double spaces above.

ok
> ....
>> +    if (ret || val)
>> +        goto reset;
>> +
>> +    regmap_write(adsp->halt_map,
>> +            adsp->halt_lpass + LPASS_HALTREQ_REG, 1);
>> +
>> +    /*  Wait for halt ACK from QDSP6 */
>
> Minor nit, please remove the double spaces in the comment above.

ok
>
>> +    timeout = jiffies + msecs_to_jiffies(ACK_TIMEOUT);
>> +    for (;;) {
>> +        ret = regmap_read(adsp->halt_map,
>> +            adsp->halt_lpass + LPASS_HALTACK_REG, &val);
>> +        if (ret || val || time_after(jiffies, timeout))
>> +            break;
>> +
>> +        udelay(1000);
>
> According to Documentation/timers/timers-howto.txt please use 
> usleep_range()
> when the delay range is between(10us - 20ms).

okay, will update in next spin.
>
>> +    }
>> +
>> +    ret = regmap_read(adsp->halt_map,
>> +            adsp->halt_lpass + LPASS_MASTER_IDLE_REG, &val);
> ....
>> +    /* wait after asserting subsystem restart from AOSS */
>> +    udelay(200);
>
> ditto

ok
>
>> +
>> +    /* Clear the halt request for the AXIM and AHBM for Q6 */
>> +    regmap_write(adsp->halt_map, adsp->halt_lpass + 
>> LPASS_HALTREQ_REG, 0);
>> +
>> +    /* De-assert the LPASS PDC Reset */
>> +    reset_control_deassert(adsp->pdc_sync_reset);
>> +    /* Remove the LPASS reset */
>> +    reset_control_deassert(adsp->cc_lpass_restart);
>> +    /* wait after de-asserting subsystem restart from AOSS */
>> +    udelay(200);
>
> ditto
>
>> +
>> +    return 0;
>> +}
> ....
>> +static int adsp_start(struct rproc *rproc)
>> +{
>> +    struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
>> +    int ret;
>> +    unsigned int val;
>> +
>> +    qcom_q6v5_prepare(&adsp->q6v5);
>> +
>> +    ret = clk_prepare_enable(adsp->xo);
>> +    if (ret)
>> +        return ret;
>
> please call qcom_q6v5_unprepare on clk_prepare_enable failure to 
> disable_irqs
>

sure, will do that
>> +
>> +    dev_pm_genpd_set_performance_state(adsp->dev, INT_MAX);
>> +    ret = pm_runtime_get_sync(adsp->dev);
>> +    if (ret)
>> +        goto disable_xo_clk;
>> +
> ....
>> +static int adsp_init_reset(struct qcom_adsp *adsp)
>> +{
>> +    adsp->pdc_sync_reset = devm_reset_control_get_exclusive(adsp->dev,
>> +            "pdc_sync");
>> +    if (IS_ERR(adsp->pdc_sync_reset)) {
>> +        dev_err(adsp->dev, "failed to acquire pdc_sync reset\n");
>> +        return PTR_ERR(adsp->pdc_sync_reset);
>> +    }
>
> Bjorn, should we return EPROBE_DEFER here since PDC global reset 
> controller can be a module?
>

devm_reset_control_get_exclusive itself returns EPROBE_DEFER until PDC 
reset driver is probed.
return PTR_ERR(adsp->pdc_sync_reset) handles this case.

>> +
>> +    adsp->cc_lpass_restart = 
>> devm_reset_control_get_exclusive(adsp->dev,
>> +            "cc_lpass");
>> +    if (IS_ERR(adsp->cc_lpass_restart)) {
>> +        dev_err(adsp->dev, "failed to acquire cc_lpass restart\n");
>> +        return PTR_ERR(adsp->cc_lpass_restart);
>> +    }
>> +
>> +    return 0;
> ....
>> +static int adsp_remove(struct platform_device *pdev)
>> +{
>> +    struct qcom_adsp *adsp = platform_get_drvdata(pdev);
>> +
>> +    rproc_del(adsp->rproc);
>> +
>> +    qcom_remove_glink_subdev(adsp->rproc, &adsp->glink_subdev);
>> +    qcom_remove_sysmon_subdev(adsp->sysmon);
>> +    qcom_remove_ssr_subdev(adsp->rproc, &adsp->ssr_subdev);
>> +    rproc_free(adsp->rproc);
>> +    pm_runtime_disable(adsp->dev);
>> +
>
> rmmod of the driver resulted in the following kernel panic:
> having a pm_runtime_disable after rproc_free seems to be the cause of 
> the kernel panic.
> Please call pm_runtime_disable before rproc_free.
>

Thanks for pointing out, will update.
> do_raw_spin_lock+0x28/0x118
> __raw_spin_lock_irq+0x30/0x3c
> __pm_runtime_disable+0x28/0xf4
> adsp_remove+0x4c/0x5c [qcom_adsp_pil]
> platform_drv_remove+0x28/0x50
> device_release_driver_internal+0x124/0x1c8
> driver_detach+0x44/0x80
> bus_remove_driver+0x78/0x9c
> driver_unregister+0x34/0x54
> platform_driver_unregister+0x1c/0x28
> cleanup_module+0x14/0x6bc [qcom_adsp_pil]
> SyS_delete_module+0x1c4/0x214
>
>> +    return 0;
>> +}
>> +
>> +static const struct adsp_pil_data adsp_resource_init = {
>> +    .crash_reason_smem = 423,
>> +    .firmware_name = "adsp.mdt",
>> +    .ssr_name = "lpass",
>> +    .sysmon_name = "adsp",
>> +    .ssctl_id = 0x14,
>> +};
> ....
>> +module_platform_driver(adsp_pil_driver);
>> +MODULE_DESCRIPTION("QTi SDM845 ADSP Peripherial Image Loader");
>
> replace QTi/QTI and Peripherial/Peripheral.
>
ok
> ....
> Also I see the following warns on stopping the adsp remoteproc, 
> couldn't root cause it though:

It should be issue in Q6 drivers. I will check and update q6 drivers. 
Thanks for reporting.

>  device_del+0x84/0x29c
>  platform_device_del+0x2c/0x88
>  platform_device_unregister+0x1c/0x30
>  of_platform_device_destroy+0x98/0xa8
>  device_for_each_child+0x54/0xa4
>  of_platform_depopulate+0x38/0x54
>  q6asm_remove+0x1c/0x2c
>  apr_device_remove+0x38/0x70
>  device_release_driver_internal+0x124/0x1c8
>  device_release_driver+0x24/0x30
>  bus_remove_device+0xcc/0xe4
>  device_del+0x1f8/0x29c
>  device_unregister+0x1c/0x30
>  apr_remove_device+0x1c/0x2c
>  device_for_each_child+0x54/0xa4
>  apr_remove+0x28/0x34
>  rpmsg_dev_remove+0x48/0x70
>  device_release_driver_internal+0x124/0x1c8
>  device_release_driver+0x24/0x30
>  bus_remove_device+0xcc/0xe4
>  device_del+0x1f8/0x29c
>  device_unregister+0x1c/0x30
>  qcom_glink_remove_device+0x1c/0x2c
>  device_for_each_child+0x54/0xa4
>  qcom_glink_native_remove+0x54/0x15c
>  qcom_glink_smem_unregister+0x1c/0x30
>  glink_subdev_stop+0x1c/0x2c [qcom_common]
>  rproc_stop+0x40/0xc0
>  rproc_shutdown+0x6c/0xc0
>  rproc_del+0x28/0xa0
>  adsp_remove+0x20/0x5c [qcom_adsp_pil]
>  platform_drv_remove+0x28/0x50
>  device_release_driver_internal+0x124/0x1c8
>  driver_detach+0x44/0x80
>  bus_remove_driver+0x78/0x9c
>  driver_unregister+0x34/0x54
>  platform_driver_unregister+0x1c/0x28
>  cleanup_module+0x14/0x6bc [qcom_adsp_pil]
>  SyS_delete_module+0x1c4/0x214
>

Thanks,
Rohit

Re: [PATCH v3 2/2] remoteproc: qcom: Introduce Non-PAS ADSP PIL driver

[Rohit Kumar]

On 9/24/2018 12:19 PM, Rohit Kumar wrote:
> Thanks Sibi for reviewing.
>
>
> On 9/22/2018 1:11 AM, Sibi Sankar wrote:
>> Hi Rohit,
>>
>> On 2018-09-03 17:22, Rohit kumar wrote:
>>> This adds Non PAS ADSP PIL driver for Qualcomm
>>> Technologies Inc SoCs.
>>> Added initial support for SDM845 with ADSP bootup and
>>> shutdown operation handled from Application Processor
>>> SubSystem(APSS).
>>>
>>> Signed-off-by: Rohit kumar <rohitkr@codeaurora.org>
>>> ---
>> ....
...
>> ....
>> Also I see the following warns on stopping the adsp remoteproc, 
>> couldn't root cause it though:
>
> It should be issue in Q6 drivers. I will check and update q6 drivers. 
> Thanks for reporting.
>

Checked this warning. This is a core driver issue fixed with 
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/base/core.c?h=next-20180924&id=2ec16150179888b81717d1d3ce84e634f4736af2 

>>  device_del+0x84/0x29c
>>  platform_device_del+0x2c/0x88
>>  platform_device_unregister+0x1c/0x30
>>  of_platform_device_destroy+0x98/0xa8
>>  device_for_each_child+0x54/0xa4
>>  of_platform_depopulate+0x38/0x54
>>  q6asm_remove+0x1c/0x2c
>>  apr_device_remove+0x38/0x70
>>  device_release_driver_internal+0x124/0x1c8
>>  device_release_driver+0x24/0x30
>>  bus_remove_device+0xcc/0xe4
>>  device_del+0x1f8/0x29c
>>  device_unregister+0x1c/0x30
>>  apr_remove_device+0x1c/0x2c
>>  device_for_each_child+0x54/0xa4
>>  apr_remove+0x28/0x34
>>  rpmsg_dev_remove+0x48/0x70
>>  device_release_driver_internal+0x124/0x1c8
>>  device_release_driver+0x24/0x30
>>  bus_remove_device+0xcc/0xe4
>>  device_del+0x1f8/0x29c
>>  device_unregister+0x1c/0x30
>>  qcom_glink_remove_device+0x1c/0x2c
>>  device_for_each_child+0x54/0xa4
>>  qcom_glink_native_remove+0x54/0x15c
>>  qcom_glink_smem_unregister+0x1c/0x30
>>  glink_subdev_stop+0x1c/0x2c [qcom_common]
>>  rproc_stop+0x40/0xc0
>>  rproc_shutdown+0x6c/0xc0
>>  rproc_del+0x28/0xa0
>>  adsp_remove+0x20/0x5c [qcom_adsp_pil]
>>  platform_drv_remove+0x28/0x50
>>  device_release_driver_internal+0x124/0x1c8
>>  driver_detach+0x44/0x80
>>  bus_remove_driver+0x78/0x9c
>>  driver_unregister+0x34/0x54
>>  platform_driver_unregister+0x1c/0x28
>>  cleanup_module+0x14/0x6bc [qcom_adsp_pil]
>>  SyS_delete_module+0x1c4/0x214
>>
>
> Thanks,
> Rohit
Thanks

Re: [PATCH v3 2/2] remoteproc: qcom: Introduce Non-PAS ADSP PIL driver

[Sibi Sankar]

On 2018-09-24 12:38, Rohit Kumar wrote:
> On 9/24/2018 12:19 PM, Rohit Kumar wrote:
>> Thanks Sibi for reviewing.
>> 
>> 
>> On 9/22/2018 1:11 AM, Sibi Sankar wrote:
>>> Hi Rohit,
>>> 
>>> On 2018-09-03 17:22, Rohit kumar wrote:
>>>> This adds Non PAS ADSP PIL driver for Qualcomm
>>>> Technologies Inc SoCs.
>>>> Added initial support for SDM845 with ADSP bootup and
>>>> shutdown operation handled from Application Processor
>>>> SubSystem(APSS).
>>>> 
>>>> Signed-off-by: Rohit kumar <rohitkr@codeaurora.org>
>>>> ---
>>> ....
> ...
>>> ....
>>> Also I see the following warns on stopping the adsp remoteproc, 
>>> couldn't root cause it though:
>> 
>> It should be issue in Q6 drivers. I will check and update q6 drivers. 
>> Thanks for reporting.
>> 
> 
> Checked this warning. This is a core driver issue fixed with
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/base/core.c?h=next-20180924&id=2ec16150179888b81717d1d3ce84e634f4736af2
> 

Thanks for pointing this out.

>>>  device_del+0x84/0x29c
>>>  platform_device_del+0x2c/0x88
>>>  platform_device_unregister+0x1c/0x30
>>>  of_platform_device_destroy+0x98/0xa8
>>>  device_for_each_child+0x54/0xa4
>>>  of_platform_depopulate+0x38/0x54
>>>  q6asm_remove+0x1c/0x2c
>>>  apr_device_remove+0x38/0x70
>>>  device_release_driver_internal+0x124/0x1c8
>>>  device_release_driver+0x24/0x30
>>>  bus_remove_device+0xcc/0xe4
>>>  device_del+0x1f8/0x29c
>>>  device_unregister+0x1c/0x30
>>>  apr_remove_device+0x1c/0x2c
>>>  device_for_each_child+0x54/0xa4
>>>  apr_remove+0x28/0x34
>>>  rpmsg_dev_remove+0x48/0x70
>>>  device_release_driver_internal+0x124/0x1c8
>>>  device_release_driver+0x24/0x30
>>>  bus_remove_device+0xcc/0xe4
>>>  device_del+0x1f8/0x29c
>>>  device_unregister+0x1c/0x30
>>>  qcom_glink_remove_device+0x1c/0x2c
>>>  device_for_each_child+0x54/0xa4
>>>  qcom_glink_native_remove+0x54/0x15c
>>>  qcom_glink_smem_unregister+0x1c/0x30
>>>  glink_subdev_stop+0x1c/0x2c [qcom_common]
>>>  rproc_stop+0x40/0xc0
>>>  rproc_shutdown+0x6c/0xc0
>>>  rproc_del+0x28/0xa0
>>>  adsp_remove+0x20/0x5c [qcom_adsp_pil]
>>>  platform_drv_remove+0x28/0x50
>>>  device_release_driver_internal+0x124/0x1c8
>>>  driver_detach+0x44/0x80
>>>  bus_remove_driver+0x78/0x9c
>>>  driver_unregister+0x34/0x54
>>>  platform_driver_unregister+0x1c/0x28
>>>  cleanup_module+0x14/0x6bc [qcom_adsp_pil]
>>>  SyS_delete_module+0x1c4/0x214
>>> 
>> 
>> Thanks,
>> Rohit
> Thanks

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

Re: [PATCH v3 2/2] remoteproc: qcom: Introduce Non-PAS ADSP PIL driver

[Sibi Sankar]

On 2018-09-24 12:19, Rohit Kumar wrote:
> Thanks Sibi for reviewing.
> 
> 
> On 9/22/2018 1:11 AM, Sibi Sankar wrote:
>> Hi Rohit,
>> 
>> On 2018-09-03 17:22, Rohit kumar wrote:
>>> This adds Non PAS ADSP PIL driver for Qualcomm
>>> Technologies Inc SoCs.
>>> Added initial support for SDM845 with ADSP bootup and
>>> shutdown operation handled from Application Processor
>>> SubSystem(APSS).
>>> 
>>> Signed-off-by: Rohit kumar <rohitkr@codeaurora.org>
>>> ---
>> ....
>>> +    select QCOM_MDT_LOADER
>>> +    select QCOM_Q6V5_COMMON
>>> +    select QCOM_RPROC_COMMON
>>> +    help
>>> +      Say y here to support the Peripherial Image Loader
>> 
>> replace Peripherial/Peripheral.
> 
> sure
>> ....
>>> +#include <linux/clk.h>
>>> +#include <linux/firmware.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/io.h>
>>> +#include <linux/reset.h>
>>> +#include <linux/iopoll.h>
>>> +#include <linux/pm_domain.h>
>>> +#include <linux/pm_runtime.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/mfd/syscon.h>
>>> +#include <linux/of_address.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/remoteproc.h>
>>> +#include <linux/soc/qcom/mdt_loader.h>
>>> +#include <linux/soc/qcom/smem.h>
>>> +#include <linux/soc/qcom/smem_state.h>
>>> +
>> 
>> The headers should be in alphabetical order.
>> 
> 
> ok, will do
>> ....
>>> +    struct reset_control *pdc_sync_reset;
>>> +    struct reset_control *cc_lpass_restart;
>>> +
>>> +    struct regmap *halt_map;
>>> +    unsigned int  halt_lpass;
>> 
>> remove the double spaces above.
> 
> ok
>> ....
>>> +    if (ret || val)
>>> +        goto reset;
>>> +
>>> +    regmap_write(adsp->halt_map,
>>> +            adsp->halt_lpass + LPASS_HALTREQ_REG, 1);
>>> +
>>> +    /*  Wait for halt ACK from QDSP6 */
>> 
>> Minor nit, please remove the double spaces in the comment above.
> 
> ok
>> 
>>> +    timeout = jiffies + msecs_to_jiffies(ACK_TIMEOUT);
>>> +    for (;;) {
>>> +        ret = regmap_read(adsp->halt_map,
>>> +            adsp->halt_lpass + LPASS_HALTACK_REG, &val);
>>> +        if (ret || val || time_after(jiffies, timeout))
>>> +            break;
>>> +
>>> +        udelay(1000);
>> 
>> According to Documentation/timers/timers-howto.txt please use 
>> usleep_range()
>> when the delay range is between(10us - 20ms).
> 
> okay, will update in next spin.
>> 
>>> +    }
>>> +
>>> +    ret = regmap_read(adsp->halt_map,
>>> +            adsp->halt_lpass + LPASS_MASTER_IDLE_REG, &val);
>> ....
>>> +    /* wait after asserting subsystem restart from AOSS */
>>> +    udelay(200);
>> 
>> ditto
> 
> ok
>> 
>>> +
>>> +    /* Clear the halt request for the AXIM and AHBM for Q6 */
>>> +    regmap_write(adsp->halt_map, adsp->halt_lpass + 
>>> LPASS_HALTREQ_REG, 0);
>>> +
>>> +    /* De-assert the LPASS PDC Reset */
>>> +    reset_control_deassert(adsp->pdc_sync_reset);
>>> +    /* Remove the LPASS reset */
>>> +    reset_control_deassert(adsp->cc_lpass_restart);
>>> +    /* wait after de-asserting subsystem restart from AOSS */
>>> +    udelay(200);
>> 
>> ditto
>> 
>>> +
>>> +    return 0;
>>> +}
>> ....
>>> +static int adsp_start(struct rproc *rproc)
>>> +{
>>> +    struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
>>> +    int ret;
>>> +    unsigned int val;
>>> +
>>> +    qcom_q6v5_prepare(&adsp->q6v5);
>>> +
>>> +    ret = clk_prepare_enable(adsp->xo);
>>> +    if (ret)
>>> +        return ret;
>> 
>> please call qcom_q6v5_unprepare on clk_prepare_enable failure to 
>> disable_irqs
>> 
> 
> sure, will do that
>>> +
>>> +    dev_pm_genpd_set_performance_state(adsp->dev, INT_MAX);
>>> +    ret = pm_runtime_get_sync(adsp->dev);
>>> +    if (ret)
>>> +        goto disable_xo_clk;
>>> +
>> ....
>>> +static int adsp_init_reset(struct qcom_adsp *adsp)
>>> +{
>>> +    adsp->pdc_sync_reset = 
>>> devm_reset_control_get_exclusive(adsp->dev,
>>> +            "pdc_sync");
>>> +    if (IS_ERR(adsp->pdc_sync_reset)) {
>>> +        dev_err(adsp->dev, "failed to acquire pdc_sync reset\n");
>>> +        return PTR_ERR(adsp->pdc_sync_reset);
>>> +    }
>> 
>> Bjorn, should we return EPROBE_DEFER here since PDC global reset 
>> controller can be a module?
>> 
> 
> devm_reset_control_get_exclusive itself returns EPROBE_DEFER until PDC
> reset driver is probed.
> return PTR_ERR(adsp->pdc_sync_reset) handles this case.
> 

Thanks for pointing this out, missed this.

>>> +
>>> +    adsp->cc_lpass_restart = 
>>> devm_reset_control_get_exclusive(adsp->dev,
>>> +            "cc_lpass");
>>> +    if (IS_ERR(adsp->cc_lpass_restart)) {
>>> +        dev_err(adsp->dev, "failed to acquire cc_lpass restart\n");
>>> +        return PTR_ERR(adsp->cc_lpass_restart);
>>> +    }
>>> +
>>> +    return 0;
>> ....
>>> +static int adsp_remove(struct platform_device *pdev)
>>> +{
>>> +    struct qcom_adsp *adsp = platform_get_drvdata(pdev);
>>> +
>>> +    rproc_del(adsp->rproc);
>>> +
>>> +    qcom_remove_glink_subdev(adsp->rproc, &adsp->glink_subdev);
>>> +    qcom_remove_sysmon_subdev(adsp->sysmon);
>>> +    qcom_remove_ssr_subdev(adsp->rproc, &adsp->ssr_subdev);
>>> +    rproc_free(adsp->rproc);
>>> +    pm_runtime_disable(adsp->dev);
>>> +
>> 
>> rmmod of the driver resulted in the following kernel panic:
>> having a pm_runtime_disable after rproc_free seems to be the cause of 
>> the kernel panic.
>> Please call pm_runtime_disable before rproc_free.
>> 
> 
> Thanks for pointing out, will update.
>> do_raw_spin_lock+0x28/0x118
>> __raw_spin_lock_irq+0x30/0x3c
>> __pm_runtime_disable+0x28/0xf4
>> adsp_remove+0x4c/0x5c [qcom_adsp_pil]
>> platform_drv_remove+0x28/0x50
>> device_release_driver_internal+0x124/0x1c8
>> driver_detach+0x44/0x80
>> bus_remove_driver+0x78/0x9c
>> driver_unregister+0x34/0x54
>> platform_driver_unregister+0x1c/0x28
>> cleanup_module+0x14/0x6bc [qcom_adsp_pil]
>> SyS_delete_module+0x1c4/0x214
>> 
>>> +    return 0;
>>> +}
>>> +
>>> +static const struct adsp_pil_data adsp_resource_init = {
>>> +    .crash_reason_smem = 423,
>>> +    .firmware_name = "adsp.mdt",
>>> +    .ssr_name = "lpass",
>>> +    .sysmon_name = "adsp",
>>> +    .ssctl_id = 0x14,
>>> +};
>> ....
>>> +module_platform_driver(adsp_pil_driver);
>>> +MODULE_DESCRIPTION("QTi SDM845 ADSP Peripherial Image Loader");
>> 
>> replace QTi/QTI and Peripherial/Peripheral.
>> 
> ok
>> ....
>> Also I see the following warns on stopping the adsp remoteproc, 
>> couldn't root cause it though:
> 
> It should be issue in Q6 drivers. I will check and update q6 drivers.
> Thanks for reporting.
> 
>>  device_del+0x84/0x29c
>>  platform_device_del+0x2c/0x88
>>  platform_device_unregister+0x1c/0x30
>>  of_platform_device_destroy+0x98/0xa8
>>  device_for_each_child+0x54/0xa4
>>  of_platform_depopulate+0x38/0x54
>>  q6asm_remove+0x1c/0x2c
>>  apr_device_remove+0x38/0x70
>>  device_release_driver_internal+0x124/0x1c8
>>  device_release_driver+0x24/0x30
>>  bus_remove_device+0xcc/0xe4
>>  device_del+0x1f8/0x29c
>>  device_unregister+0x1c/0x30
>>  apr_remove_device+0x1c/0x2c
>>  device_for_each_child+0x54/0xa4
>>  apr_remove+0x28/0x34
>>  rpmsg_dev_remove+0x48/0x70
>>  device_release_driver_internal+0x124/0x1c8
>>  device_release_driver+0x24/0x30
>>  bus_remove_device+0xcc/0xe4
>>  device_del+0x1f8/0x29c
>>  device_unregister+0x1c/0x30
>>  qcom_glink_remove_device+0x1c/0x2c
>>  device_for_each_child+0x54/0xa4
>>  qcom_glink_native_remove+0x54/0x15c
>>  qcom_glink_smem_unregister+0x1c/0x30
>>  glink_subdev_stop+0x1c/0x2c [qcom_common]
>>  rproc_stop+0x40/0xc0
>>  rproc_shutdown+0x6c/0xc0
>>  rproc_del+0x28/0xa0
>>  adsp_remove+0x20/0x5c [qcom_adsp_pil]
>>  platform_drv_remove+0x28/0x50
>>  device_release_driver_internal+0x124/0x1c8
>>  driver_detach+0x44/0x80
>>  bus_remove_driver+0x78/0x9c
>>  driver_unregister+0x34/0x54
>>  platform_driver_unregister+0x1c/0x28
>>  cleanup_module+0x14/0x6bc [qcom_adsp_pil]
>>  SyS_delete_module+0x1c4/0x214
>> 
> 
> Thanks,
> Rohit

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