[PATCH v2 0/8] remoteproc: qcom: post mortem debug support

[PATCH v2 0/8] remoteproc: qcom: post mortem debug support

[Bjorn Andersson]

The following series introduces two components that aids in post mortem
debugging of Qualcomm systems. The first part is used to store information
about loaded images in IMEM, for post mortem tools to know where the kernel
loaded the remoteproc firmware. The second part invokes a stop operation on the
remoteprocs during a kernel panic, in order to trigger them to flush caches
etc.

Bjorn Andersson (8):
  dt-bindings: remoteproc: Add Qualcomm PIL info binding
  remoteproc: qcom: Introduce driver to store pil info in IMEM
  remoteproc: qcom: Update IMEM PIL info on load
  arm64: dts: qcom: qcs404: Add IMEM and PIL info region
  arm64: dts: qcom: sdm845: Add IMEM and PIL info region
  remoteproc: Introduce "panic" callback in ops
  remoteproc: qcom: q6v5: Add common panic handler
  remoteproc: qcom: Introduce panic handler for PAS and ADSP

 .../bindings/remoteproc/qcom,pil-info.yaml    |  35 ++++
 arch/arm64/boot/dts/qcom/qcs404.dtsi          |  10 ++
 arch/arm64/boot/dts/qcom/sdm845.dtsi          |  10 ++
 drivers/remoteproc/Kconfig                    |   6 +
 drivers/remoteproc/Makefile                   |   1 +
 drivers/remoteproc/qcom_pil_info.c            | 150 ++++++++++++++++++
 drivers/remoteproc/qcom_pil_info.h            |   8 +
 drivers/remoteproc/qcom_q6v5.c                |  19 +++
 drivers/remoteproc/qcom_q6v5.h                |   1 +
 drivers/remoteproc/qcom_q6v5_adsp.c           |  27 +++-
 drivers/remoteproc/qcom_q6v5_mss.c            |   6 +
 drivers/remoteproc/qcom_q6v5_pas.c            |  26 ++-
 drivers/remoteproc/qcom_wcnss.c               |  17 +-
 drivers/remoteproc/remoteproc_core.c          |  17 ++
 include/linux/remoteproc.h                    |   4 +
 15 files changed, 328 insertions(+), 9 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,pil-info.yaml
 create mode 100644 drivers/remoteproc/qcom_pil_info.c
 create mode 100644 drivers/remoteproc/qcom_pil_info.h

-- 
2.24.0

[PATCH v2 1/8] dt-bindings: remoteproc: Add Qualcomm PIL info binding

[Bjorn Andersson]

Add a devicetree binding for the Qualcomm periperal image loader
relocation info region found in the IMEM.

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

Changes since v1:
- New patch

 .../bindings/remoteproc/qcom,pil-info.yaml    | 35 +++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,pil-info.yaml

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,pil-info.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,pil-info.yaml
new file mode 100644
index 000000000000..715945c683ed
--- /dev/null
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,pil-info.yaml
@@ -0,0 +1,35 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/remoteproc/qcom,pil-info.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Qualcomm peripheral image loader relocation info binding
+
+description:
+  This document defines the binding for describing the Qualcomm peripheral
+  image loader relocation memory region, in IMEM, which is used for post mortem
+  debugging of remoteprocs.
+
+maintainers:
+  - Bjorn Andersson <bjorn.andersson@linaro.org>
+
+properties:
+  compatible:
+    const: qcom,pil-reloc-info
+
+  offset:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: Offset in the register map for the memory region
+
+examples:
+  - |
+    imem@146bf000 {
+      compatible = "syscon", "simple-mfd";
+      reg = <0 0x146bf000 0 0x1000>;
+
+      pil-reloc {
+        compatible ="qcom,pil-reloc-info";
+        offset = <0x94c>;
+      };
+    };
-- 
2.24.0

[PATCH v2 2/8] remoteproc: qcom: Introduce driver to store pil info in IMEM

[Bjorn Andersson]

A region in IMEM is used to communicate load addresses of remoteproc to
post mortem debug tools. Implement a driver that can be used to store
this information in order to enable these tools to process collected
ramdumps.

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

Changes since v1:
- Added helper to probe defer clients
- Fixed logical bug in slot scan
- Added SPDX header in header file

 drivers/remoteproc/Kconfig         |   3 +
 drivers/remoteproc/Makefile        |   1 +
 drivers/remoteproc/qcom_pil_info.c | 150 +++++++++++++++++++++++++++++
 drivers/remoteproc/qcom_pil_info.h |   8 ++
 4 files changed, 162 insertions(+)
 create mode 100644 drivers/remoteproc/qcom_pil_info.c
 create mode 100644 drivers/remoteproc/qcom_pil_info.h

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 94afdde4bc9f..0798602e355a 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -85,6 +85,9 @@ config KEYSTONE_REMOTEPROC
 	  It's safe to say N here if you're not interested in the Keystone
 	  DSPs or just want to use a bare minimum kernel.
 
+config QCOM_PIL_INFO
+	tristate
+
 config QCOM_RPROC_COMMON
 	tristate
 
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index 00f09e658cb3..c1b46e9033cb 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
 obj-$(CONFIG_WKUP_M3_RPROC)		+= wkup_m3_rproc.o
 obj-$(CONFIG_DA8XX_REMOTEPROC)		+= da8xx_remoteproc.o
 obj-$(CONFIG_KEYSTONE_REMOTEPROC)	+= keystone_remoteproc.o
+obj-$(CONFIG_QCOM_PIL_INFO)		+= qcom_pil_info.o
 obj-$(CONFIG_QCOM_RPROC_COMMON)		+= qcom_common.o
 obj-$(CONFIG_QCOM_Q6V5_COMMON)		+= qcom_q6v5.o
 obj-$(CONFIG_QCOM_Q6V5_ADSP)		+= qcom_q6v5_adsp.o
diff --git a/drivers/remoteproc/qcom_pil_info.c b/drivers/remoteproc/qcom_pil_info.c
new file mode 100644
index 000000000000..b0897ae9eae5
--- /dev/null
+++ b/drivers/remoteproc/qcom_pil_info.c
@@ -0,0 +1,150 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2019 Linaro Ltd.
+ */
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+#include <linux/mfd/syscon.h>
+#include <linux/slab.h>
+
+struct pil_reloc_entry {
+	char name[8];
+	__le64 base;
+	__le32 size;
+} __packed;
+
+#define PIL_INFO_SIZE	200
+#define PIL_INFO_ENTRIES (PIL_INFO_SIZE / sizeof(struct pil_reloc_entry))
+
+struct pil_reloc {
+	struct device *dev;
+	struct regmap *map;
+	u32 offset;
+	int val_bytes;
+
+	struct pil_reloc_entry entries[PIL_INFO_ENTRIES];
+};
+
+static struct pil_reloc *_reloc;
+static DEFINE_MUTEX(reloc_mutex);
+
+/**
+ * qcom_pil_info_store() - store PIL information of image in IMEM
+ * @image:	name of the image
+ * @base:	base address of the loaded image
+ * @size:	size of the loaded image
+ */
+void qcom_pil_info_store(const char *image, phys_addr_t base, size_t size)
+{
+	struct pil_reloc_entry *entry;
+	int idx = -1;
+	int i;
+
+	mutex_lock(&reloc_mutex);
+	if (!_reloc)
+		goto unlock;
+
+	for (i = 0; i < PIL_INFO_ENTRIES; i++) {
+		if (!_reloc->entries[i].name[0]) {
+			if (idx == -1)
+				idx = i;
+			continue;
+		}
+
+		if (!strncmp(_reloc->entries[i].name, image, 8)) {
+			idx = i;
+			goto found;
+		}
+	}
+
+	if (idx == -1) {
+		dev_warn(_reloc->dev, "insufficient PIL info slots\n");
+		goto unlock;
+	}
+
+found:
+	entry = &_reloc->entries[idx];
+	stracpy(entry->name, image);
+	entry->base = base;
+	entry->size = size;
+
+	regmap_bulk_write(_reloc->map, _reloc->offset + idx * sizeof(*entry),
+			  entry, sizeof(*entry) / _reloc->val_bytes);
+
+unlock:
+	mutex_unlock(&reloc_mutex);
+}
+EXPORT_SYMBOL_GPL(qcom_pil_info_store);
+
+/**
+ * qcom_pil_info_available() - query if the pil info is probed
+ *
+ * Return: boolean indicating if the pil info device is probed
+ */
+bool qcom_pil_info_available(void)
+{
+	return !!_reloc;
+}
+EXPORT_SYMBOL_GPL(qcom_pil_info_available);
+
+static int pil_reloc_probe(struct platform_device *pdev)
+{
+	struct pil_reloc *reloc;
+
+	reloc = devm_kzalloc(&pdev->dev, sizeof(*reloc), GFP_KERNEL);
+	if (!reloc)
+		return -ENOMEM;
+
+	reloc->dev = &pdev->dev;
+	reloc->map = syscon_node_to_regmap(pdev->dev.parent->of_node);
+	if (IS_ERR(reloc->map))
+		return PTR_ERR(reloc->map);
+
+	if (of_property_read_u32(pdev->dev.of_node, "offset", &reloc->offset))
+		return -EINVAL;
+
+	reloc->val_bytes = regmap_get_val_bytes(reloc->map);
+	if (reloc->val_bytes < 0)
+		return -EINVAL;
+
+	regmap_bulk_write(reloc->map, reloc->offset, reloc->entries,
+			  sizeof(reloc->entries) / reloc->val_bytes);
+
+	mutex_lock(&reloc_mutex);
+	_reloc = reloc;
+	mutex_unlock(&reloc_mutex);
+
+	return 0;
+}
+
+static int pil_reloc_remove(struct platform_device *pdev)
+{
+	mutex_lock(&reloc_mutex);
+	_reloc = NULL;
+	mutex_unlock(&reloc_mutex);
+
+	return 0;
+}
+
+static const struct of_device_id pil_reloc_of_match[] = {
+	{ .compatible = "qcom,pil-reloc-info" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, pil_reloc_of_match);
+
+static struct platform_driver pil_reloc_driver = {
+	.probe = pil_reloc_probe,
+	.remove = pil_reloc_remove,
+	.driver = {
+		.name = "qcom-pil-reloc-info",
+		.of_match_table = pil_reloc_of_match,
+	},
+};
+module_platform_driver(pil_reloc_driver);
+
+MODULE_DESCRIPTION("Qualcomm PIL relocation info");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/remoteproc/qcom_pil_info.h b/drivers/remoteproc/qcom_pil_info.h
new file mode 100644
index 000000000000..0372602fae1d
--- /dev/null
+++ b/drivers/remoteproc/qcom_pil_info.h
@@ -0,0 +1,8 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __QCOM_PIL_INFO_H__
+#define __QCOM_PIL_INFO_H__
+
+void qcom_pil_info_store(const char *image, phys_addr_t base, size_t size);
+bool qcom_pil_info_available(void);
+
+#endif
-- 
2.24.0

[PATCH v2 3/8] remoteproc: qcom: Update IMEM PIL info on load

[Bjorn Andersson]

Update the PIL info region structure in IMEM with information about
where the firmware for various remoteprocs are loaded.

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

Changes since v1:
- Squashed patches for the individual drivers into one
- Probe defer on qcom_pil_info_available()

 drivers/remoteproc/Kconfig          |  3 +++
 drivers/remoteproc/qcom_q6v5_adsp.c | 19 ++++++++++++++++---
 drivers/remoteproc/qcom_q6v5_mss.c  |  6 ++++++
 drivers/remoteproc/qcom_q6v5_pas.c  | 18 +++++++++++++++---
 drivers/remoteproc/qcom_wcnss.c     | 17 ++++++++++++++---
 5 files changed, 54 insertions(+), 9 deletions(-)

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 0798602e355a..84922bb922e0 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -135,6 +135,7 @@ config QCOM_Q6V5_PAS
 	depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n
 	depends on QCOM_SYSMON || QCOM_SYSMON=n
 	select MFD_SYSCON
+	select QCOM_PIL_INFO
 	select QCOM_MDT_LOADER
 	select QCOM_Q6V5_COMMON
 	select QCOM_RPROC_COMMON
@@ -152,6 +153,7 @@ config QCOM_Q6V5_WCSS
 	depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n
 	depends on QCOM_SYSMON || QCOM_SYSMON=n
 	select MFD_SYSCON
+	select QCOM_PIL_INFO
 	select QCOM_MDT_LOADER
 	select QCOM_Q6V5_COMMON
 	select QCOM_RPROC_COMMON
@@ -183,6 +185,7 @@ config QCOM_WCNSS_PIL
 	depends on QCOM_SMEM
 	depends on QCOM_SYSMON || QCOM_SYSMON=n
 	select QCOM_MDT_LOADER
+	select QCOM_PIL_INFO
 	select QCOM_RPROC_COMMON
 	select QCOM_SCM
 	help
diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c b/drivers/remoteproc/qcom_q6v5_adsp.c
index e953886b2eb7..1a942c92d974 100644
--- a/drivers/remoteproc/qcom_q6v5_adsp.c
+++ b/drivers/remoteproc/qcom_q6v5_adsp.c
@@ -26,6 +26,7 @@
 #include <linux/soc/qcom/smem_state.h>
 
 #include "qcom_common.h"
+#include "qcom_pil_info.h"
 #include "qcom_q6v5.h"
 #include "remoteproc_internal.h"
 
@@ -82,6 +83,7 @@ struct qcom_adsp {
 	unsigned int halt_lpass;
 
 	int crash_reason_smem;
+	const char *info_name;
 
 	struct completion start_done;
 	struct completion stop_done;
@@ -164,10 +166,17 @@ static int qcom_adsp_shutdown(struct qcom_adsp *adsp)
 static int adsp_load(struct rproc *rproc, const struct firmware *fw)
 {
 	struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
+	int ret;
+
+	ret = qcom_mdt_load_no_init(adsp->dev, fw, rproc->firmware, 0,
+				    adsp->mem_region, adsp->mem_phys, adsp->mem_size,
+				    &adsp->mem_reloc);
+	if (ret)
+		return ret;
 
-	return qcom_mdt_load_no_init(adsp->dev, fw, rproc->firmware, 0,
-			     adsp->mem_region, adsp->mem_phys, adsp->mem_size,
-			     &adsp->mem_reloc);
+	qcom_pil_info_store(adsp->info_name, adsp->mem_reloc, adsp->mem_size);
+
+	return 0;
 }
 
 static int adsp_start(struct rproc *rproc)
@@ -413,6 +422,9 @@ static int adsp_probe(struct platform_device *pdev)
 	struct rproc *rproc;
 	int ret;
 
+	if (!qcom_pil_info_available())
+		return -EPROBE_DEFER;
+
 	desc = of_device_get_match_data(&pdev->dev);
 	if (!desc)
 		return -EINVAL;
@@ -427,6 +439,7 @@ static int adsp_probe(struct platform_device *pdev)
 	adsp = (struct qcom_adsp *)rproc->priv;
 	adsp->dev = &pdev->dev;
 	adsp->rproc = rproc;
+	adsp->info_name = desc->sysmon_name;
 	platform_set_drvdata(pdev, adsp);
 
 	ret = adsp_alloc_memory_region(adsp);
diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
index 471128a2e723..6360e69b54e4 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -28,6 +28,7 @@
 
 #include "remoteproc_internal.h"
 #include "qcom_common.h"
+#include "qcom_pil_info.h"
 #include "qcom_q6v5.h"
 
 #include <linux/qcom_scm.h>
@@ -1052,6 +1053,8 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
 	else if (ret < 0)
 		dev_err(qproc->dev, "MPSS authentication failed: %d\n", ret);
 
+	qcom_pil_info_store("modem", mpss_reloc, qproc->mpss_size);
+
 release_firmware:
 	release_firmware(fw);
 out:
@@ -1400,6 +1403,9 @@ static int q6v5_probe(struct platform_device *pdev)
 	if (desc->need_mem_protection && !qcom_scm_is_available())
 		return -EPROBE_DEFER;
 
+	if (!qcom_pil_info_available())
+		return -EPROBE_DEFER;
+
 	mba_image = desc->hexagon_mba_image;
 	ret = of_property_read_string_index(pdev->dev.of_node, "firmware-name",
 					    0, &mba_image);
diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
index b890e6e305f3..4dcdf1301e50 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -25,6 +25,7 @@
 #include <linux/soc/qcom/smem_state.h>
 
 #include "qcom_common.h"
+#include "qcom_pil_info.h"
 #include "qcom_q6v5.h"
 #include "remoteproc_internal.h"
 
@@ -64,6 +65,7 @@ struct qcom_adsp {
 	int pas_id;
 	int crash_reason_smem;
 	bool has_aggre2_clk;
+	const char *info_name;
 
 	struct completion start_done;
 	struct completion stop_done;
@@ -117,11 +119,17 @@ static void adsp_pds_disable(struct qcom_adsp *adsp, struct device **pds,
 static int adsp_load(struct rproc *rproc, const struct firmware *fw)
 {
 	struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
+	int ret;
+
+	ret = qcom_mdt_load(adsp->dev, fw, rproc->firmware, adsp->pas_id,
+			    adsp->mem_region, adsp->mem_phys, adsp->mem_size,
+			    &adsp->mem_reloc);
+	if (ret)
+		return ret;
 
-	return qcom_mdt_load(adsp->dev, fw, rproc->firmware, adsp->pas_id,
-			     adsp->mem_region, adsp->mem_phys, adsp->mem_size,
-			     &adsp->mem_reloc);
+	qcom_pil_info_store(adsp->info_name, adsp->mem_reloc, adsp->mem_size);
 
+	return 0;
 }
 
 static int adsp_start(struct rproc *rproc)
@@ -376,6 +384,9 @@ static int adsp_probe(struct platform_device *pdev)
 	if (!qcom_scm_is_available())
 		return -EPROBE_DEFER;
 
+	if (!qcom_pil_info_available())
+		return -EPROBE_DEFER;
+
 	fw_name = desc->firmware_name;
 	ret = of_property_read_string(pdev->dev.of_node, "firmware-name",
 				      &fw_name);
@@ -396,6 +407,7 @@ static int adsp_probe(struct platform_device *pdev)
 	adsp->rproc = rproc;
 	adsp->pas_id = desc->pas_id;
 	adsp->has_aggre2_clk = desc->has_aggre2_clk;
+	adsp->info_name = desc->sysmon_name;
 	platform_set_drvdata(pdev, adsp);
 
 	ret = adsp_alloc_memory_region(adsp);
diff --git a/drivers/remoteproc/qcom_wcnss.c b/drivers/remoteproc/qcom_wcnss.c
index dc135754bb9c..2c1cefeacf97 100644
--- a/drivers/remoteproc/qcom_wcnss.c
+++ b/drivers/remoteproc/qcom_wcnss.c
@@ -27,6 +27,7 @@
 
 #include "qcom_common.h"
 #include "remoteproc_internal.h"
+#include "qcom_pil_info.h"
 #include "qcom_wcnss.h"
 
 #define WCNSS_CRASH_REASON_SMEM		422
@@ -145,10 +146,17 @@ void qcom_wcnss_assign_iris(struct qcom_wcnss *wcnss,
 static int wcnss_load(struct rproc *rproc, const struct firmware *fw)
 {
 	struct qcom_wcnss *wcnss = (struct qcom_wcnss *)rproc->priv;
+	int ret;
+
+	ret = qcom_mdt_load(wcnss->dev, fw, rproc->firmware, WCNSS_PAS_ID,
+			    wcnss->mem_region, wcnss->mem_phys,
+			    wcnss->mem_size, &wcnss->mem_reloc);
+	if (ret)
+		return ret;
 
-	return qcom_mdt_load(wcnss->dev, fw, rproc->firmware, WCNSS_PAS_ID,
-			     wcnss->mem_region, wcnss->mem_phys,
-			     wcnss->mem_size, &wcnss->mem_reloc);
+	qcom_pil_info_store("wcnss", wcnss->mem_reloc, wcnss->mem_size);
+
+	return 0;
 }
 
 static void wcnss_indicate_nv_download(struct qcom_wcnss *wcnss)
@@ -469,6 +477,9 @@ static int wcnss_probe(struct platform_device *pdev)
 	if (!qcom_scm_is_available())
 		return -EPROBE_DEFER;
 
+	if (!qcom_pil_info_available())
+		return -EPROBE_DEFER;
+
 	if (!qcom_scm_pas_supported(WCNSS_PAS_ID)) {
 		dev_err(&pdev->dev, "PAS is not available for WCNSS\n");
 		return -ENXIO;
-- 
2.24.0

[PATCH v2 4/8] arm64: dts: qcom: qcs404: Add IMEM and PIL info region

[Bjorn Andersson]

Add a simple-mfd representing IMEM on QCS404 and define the PIL
relocation info region, so that post mortem tools will be able to locate
the loaded remoteprocs.

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

Changes since v1:
- None

 arch/arm64/boot/dts/qcom/qcs404.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi
index f5f0c4c9cb16..9d2a2b1c64dd 100644
--- a/arch/arm64/boot/dts/qcom/qcs404.dtsi
+++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi
@@ -890,6 +890,16 @@ blsp2_spi0: spi@7af5000 {
 			status = "disabled";
 		};
 
+		imem@8600000 {
+			compatible = "syscon", "simple-mfd";
+			reg = <0x08600000 0x1000>;
+
+			pil-reloc {
+				compatible ="qcom,pil-reloc-info";
+				offset = <0x94c>;
+			};
+		};
+
 		intc: interrupt-controller@b000000 {
 			compatible = "qcom,msm-qgic2";
 			interrupt-controller;
-- 
2.24.0

[PATCH v2 5/8] arm64: dts: qcom: sdm845: Add IMEM and PIL info region

[Bjorn Andersson]

Add a simple-mfd representing IMEM on SDM845 and define the PIL
relocation info region, so that post mortem tools will be able to locate
the loaded remoteprocs.

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

Changes since v1:
- None

 arch/arm64/boot/dts/qcom/sdm845.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 407d26e92fcc..e1e13d5bfeb3 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -3020,6 +3020,16 @@ spmi_bus: spmi@c440000 {
 			cell-index = <0>;
 		};
 
+		imem@146bf000 {
+			compatible = "syscon", "simple-mfd";
+			reg = <0 0x146bf000 0 0x1000>;
+
+			pil-reloc {
+				compatible ="qcom,pil-reloc-info";
+				offset = <0x94c>;
+			};
+		};
+
 		apps_smmu: iommu@15000000 {
 			compatible = "qcom,sdm845-smmu-500", "arm,mmu-500";
 			reg = <0 0x15000000 0 0x80000>;
-- 
2.24.0

[PATCH v2 6/8] remoteproc: Introduce "panic" callback in ops

[Bjorn Andersson]

Introduce a "panic" function in the remoteproc ops table, to allow
remoteproc instances to perform operations needed in order to aid in
post mortem system debugging, such as flushing caches etc, when the
kernel panics.

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

Changes since v1:
- None

 drivers/remoteproc/remoteproc_core.c | 17 +++++++++++++++++
 include/linux/remoteproc.h           |  4 ++++
 2 files changed, 21 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 307df98347ba..779f19d6d8e7 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1832,6 +1832,17 @@ void rproc_shutdown(struct rproc *rproc)
 }
 EXPORT_SYMBOL(rproc_shutdown);
 
+static int rproc_panic_handler(struct notifier_block *nb, unsigned long event,
+			       void *ptr)
+{
+	struct rproc *rproc = container_of(nb, struct rproc, panic_nb);
+
+	if (rproc->state == RPROC_RUNNING)
+		rproc->ops->panic(rproc);
+
+	return NOTIFY_DONE;
+}
+
 /**
  * rproc_get_by_phandle() - find a remote processor by phandle
  * @phandle: phandle to the rproc
@@ -2057,6 +2068,12 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
 		rproc->ops->get_boot_addr = rproc_elf_get_boot_addr;
 	}
 
+	/* Register panic notifier for remoteprocs with "panic" callback */
+	if (rproc->ops->panic) {
+		rproc->panic_nb.notifier_call = rproc_panic_handler;
+		atomic_notifier_chain_register(&panic_notifier_list, &rproc->panic_nb);
+	}
+
 	mutex_init(&rproc->lock);
 
 	idr_init(&rproc->notifyids);
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 16ad66683ad0..7836c528d309 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -369,6 +369,7 @@ enum rsc_handling_status {
  *			expects to find it
  * @sanity_check:	sanity check the fw image
  * @get_boot_addr:	get boot address to entry point specified in firmware
+ * @panic:	optional callback to react to system panic
  */
 struct rproc_ops {
 	int (*start)(struct rproc *rproc);
@@ -383,6 +384,7 @@ struct rproc_ops {
 	int (*load)(struct rproc *rproc, const struct firmware *fw);
 	int (*sanity_check)(struct rproc *rproc, const struct firmware *fw);
 	u32 (*get_boot_addr)(struct rproc *rproc, const struct firmware *fw);
+	void (*panic)(struct rproc *rproc);
 };
 
 /**
@@ -481,6 +483,7 @@ struct rproc_dump_segment {
  * @auto_boot: flag to indicate if remote processor should be auto-started
  * @dump_segments: list of segments in the firmware
  * @nb_vdev: number of vdev currently handled by rproc
+ * @panic_nb: notifier_block for remoteproc's panic handler
  */
 struct rproc {
 	struct list_head node;
@@ -514,6 +517,7 @@ struct rproc {
 	bool auto_boot;
 	struct list_head dump_segments;
 	int nb_vdev;
+	struct notifier_block panic_nb;
 };
 
 /**
-- 
2.24.0

[PATCH v2 7/8] remoteproc: qcom: q6v5: Add common panic handler

[Bjorn Andersson]

Add a common panic handler that invokes a stop request and sleep enough
to let the remoteproc flush it's caches etc in order to aid post mortem
debugging.

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

Changes since v1:
- None

 drivers/remoteproc/qcom_q6v5.c | 19 +++++++++++++++++++
 drivers/remoteproc/qcom_q6v5.h |  1 +
 2 files changed, 20 insertions(+)

diff --git a/drivers/remoteproc/qcom_q6v5.c b/drivers/remoteproc/qcom_q6v5.c
index cb0f4a0be032..17167c980e02 100644
--- a/drivers/remoteproc/qcom_q6v5.c
+++ b/drivers/remoteproc/qcom_q6v5.c
@@ -6,6 +6,7 @@
  * Copyright (C) 2014 Sony Mobile Communications AB
  * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
  */
+#include <linux/delay.h>
 #include <linux/kernel.h>
 #include <linux/platform_device.h>
 #include <linux/interrupt.h>
@@ -15,6 +16,8 @@
 #include <linux/remoteproc.h>
 #include "qcom_q6v5.h"
 
+#define Q6V5_PANIC_DELAY_MS	200
+
 /**
  * qcom_q6v5_prepare() - reinitialize the qcom_q6v5 context before start
  * @q6v5:	reference to qcom_q6v5 context to be reinitialized
@@ -162,6 +165,22 @@ int qcom_q6v5_request_stop(struct qcom_q6v5 *q6v5)
 }
 EXPORT_SYMBOL_GPL(qcom_q6v5_request_stop);
 
+/**
+ * qcom_q6v5_panic() - panic handler to invoke a stop on the remote
+ * @q6v5:	reference to qcom_q6v5 context
+ *
+ * Set the stop bit and sleep in order to allow the remote processor to flush
+ * its caches etc for post mortem debugging.
+ */
+void qcom_q6v5_panic(struct qcom_q6v5 *q6v5)
+{
+	qcom_smem_state_update_bits(q6v5->state,
+				    BIT(q6v5->stop_bit), BIT(q6v5->stop_bit));
+
+	mdelay(Q6V5_PANIC_DELAY_MS);
+}
+EXPORT_SYMBOL_GPL(qcom_q6v5_panic);
+
 /**
  * qcom_q6v5_init() - initializer of the q6v5 common struct
  * @q6v5:	handle to be initialized
diff --git a/drivers/remoteproc/qcom_q6v5.h b/drivers/remoteproc/qcom_q6v5.h
index 7ac92c1e0f49..c37e6fd063e4 100644
--- a/drivers/remoteproc/qcom_q6v5.h
+++ b/drivers/remoteproc/qcom_q6v5.h
@@ -42,5 +42,6 @@ int qcom_q6v5_prepare(struct qcom_q6v5 *q6v5);
 int qcom_q6v5_unprepare(struct qcom_q6v5 *q6v5);
 int qcom_q6v5_request_stop(struct qcom_q6v5 *q6v5);
 int qcom_q6v5_wait_for_start(struct qcom_q6v5 *q6v5, int timeout);
+void qcom_q6v5_panic(struct qcom_q6v5 *q6v5);
 
 #endif
-- 
2.24.0

[PATCH v2 8/8] remoteproc: qcom: Introduce panic handler for PAS and ADSP

[Bjorn Andersson]

Make the PAS and ADSP/CDSP remoteproc drivers implement the panic
handler that will invoke a stop to prepare the remoteprocs for post
mortem debugging.

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

Changes since v1:
- None

 drivers/remoteproc/qcom_q6v5_adsp.c | 8 ++++++++
 drivers/remoteproc/qcom_q6v5_pas.c  | 8 ++++++++
 2 files changed, 16 insertions(+)

diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c b/drivers/remoteproc/qcom_q6v5_adsp.c
index 1a942c92d974..4189d22531f4 100644
--- a/drivers/remoteproc/qcom_q6v5_adsp.c
+++ b/drivers/remoteproc/qcom_q6v5_adsp.c
@@ -291,12 +291,20 @@ static void *adsp_da_to_va(struct rproc *rproc, u64 da, int len)
 	return adsp->mem_region + offset;
 }
 
+static void adsp_panic(struct rproc *rproc)
+{
+	struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
+
+	qcom_q6v5_panic(&adsp->q6v5);
+}
+
 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,
+	.panic = adsp_panic,
 };
 
 static int adsp_init_clock(struct qcom_adsp *adsp, const char **clk_ids)
diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
index 4dcdf1301e50..547d012cf2db 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -242,12 +242,20 @@ static void *adsp_da_to_va(struct rproc *rproc, u64 da, int len)
 	return adsp->mem_region + offset;
 }
 
+static void adsp_panic(struct rproc *rproc)
+{
+	struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
+
+	qcom_q6v5_panic(&adsp->q6v5);
+}
+
 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,
+	.panic = adsp_panic,
 };
 
 static int adsp_init_clock(struct qcom_adsp *adsp)
-- 
2.24.0

Re: [PATCH v2 1/8] dt-bindings: remoteproc: Add Qualcomm PIL info binding

[Rob Herring]

On Thu, Dec 26, 2019 at 09:32:08PM -0800, Bjorn Andersson wrote:
> Add a devicetree binding for the Qualcomm periperal image loader
> relocation info region found in the IMEM.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> 
> Changes since v1:
> - New patch
> 
>  .../bindings/remoteproc/qcom,pil-info.yaml    | 35 +++++++++++++++++++
>  1 file changed, 35 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,pil-info.yaml
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,pil-info.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,pil-info.yaml
> new file mode 100644
> index 000000000000..715945c683ed
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,pil-info.yaml
> @@ -0,0 +1,35 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/remoteproc/qcom,pil-info.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Qualcomm peripheral image loader relocation info binding
> +
> +description:
> +  This document defines the binding for describing the Qualcomm peripheral
> +  image loader relocation memory region, in IMEM, which is used for post mortem
> +  debugging of remoteprocs.
> +
> +maintainers:
> +  - Bjorn Andersson <bjorn.andersson@linaro.org>
> +
> +properties:
> +  compatible:
> +    const: qcom,pil-reloc-info
> +
> +  offset:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: Offset in the register map for the memory region

Why not use 'reg' instead?

> +
> +examples:
> +  - |
> +    imem@146bf000 {
> +      compatible = "syscon", "simple-mfd";
> +      reg = <0 0x146bf000 0 0x1000>;
> +
> +      pil-reloc {
> +        compatible ="qcom,pil-reloc-info";
> +        offset = <0x94c>;
> +      };
> +    };
> -- 
> 2.24.0
> 

Re: [PATCH v2 1/8] dt-bindings: remoteproc: Add Qualcomm PIL info binding

[Bjorn Andersson]

On Sat 04 Jan 13:38 PST 2020, Rob Herring wrote:

> On Thu, Dec 26, 2019 at 09:32:08PM -0800, Bjorn Andersson wrote:
> > Add a devicetree binding for the Qualcomm periperal image loader
> > relocation info region found in the IMEM.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> > 
> > Changes since v1:
> > - New patch
> > 
> >  .../bindings/remoteproc/qcom,pil-info.yaml    | 35 +++++++++++++++++++
> >  1 file changed, 35 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,pil-info.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,pil-info.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,pil-info.yaml
> > new file mode 100644
> > index 000000000000..715945c683ed
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/remoteproc/qcom,pil-info.yaml
> > @@ -0,0 +1,35 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: "http://devicetree.org/schemas/remoteproc/qcom,pil-info.yaml#"
> > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > +
> > +title: Qualcomm peripheral image loader relocation info binding
> > +
> > +description:
> > +  This document defines the binding for describing the Qualcomm peripheral
> > +  image loader relocation memory region, in IMEM, which is used for post mortem
> > +  debugging of remoteprocs.
> > +
> > +maintainers:
> > +  - Bjorn Andersson <bjorn.andersson@linaro.org>
> > +
> > +properties:
> > +  compatible:
> > +    const: qcom,pil-reloc-info
> > +
> > +  offset:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: Offset in the register map for the memory region
> 
> Why not use 'reg' instead?
> 

Because we have one prior example of subdevice of "imem", which is
compatible "syscon-reboot-mode" and that binding uses "offset".

That said, if you insist, reg (and ranges) would allow us to express the
size of the region...

Regards,
Bjorn

> > +
> > +examples:
> > +  - |
> > +    imem@146bf000 {
> > +      compatible = "syscon", "simple-mfd";
> > +      reg = <0 0x146bf000 0 0x1000>;
> > +
> > +      pil-reloc {
> > +        compatible ="qcom,pil-reloc-info";
> > +        offset = <0x94c>;
> > +      };
> > +    };
> > -- 
> > 2.24.0
> > 

Re: [PATCH v2 2/8] remoteproc: qcom: Introduce driver to store pil info in IMEM

[Mathieu Poirier]

Hi Bjorn,

On Thu, Dec 26, 2019 at 09:32:09PM -0800, Bjorn Andersson wrote:
> A region in IMEM is used to communicate load addresses of remoteproc to
> post mortem debug tools. Implement a driver that can be used to store
> this information in order to enable these tools to process collected
> ramdumps.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> 
> Changes since v1:
> - Added helper to probe defer clients
> - Fixed logical bug in slot scan
> - Added SPDX header in header file
> 
>  drivers/remoteproc/Kconfig         |   3 +
>  drivers/remoteproc/Makefile        |   1 +
>  drivers/remoteproc/qcom_pil_info.c | 150 +++++++++++++++++++++++++++++
>  drivers/remoteproc/qcom_pil_info.h |   8 ++
>  4 files changed, 162 insertions(+)
>  create mode 100644 drivers/remoteproc/qcom_pil_info.c
>  create mode 100644 drivers/remoteproc/qcom_pil_info.h
> 
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index 94afdde4bc9f..0798602e355a 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -85,6 +85,9 @@ config KEYSTONE_REMOTEPROC
>  	  It's safe to say N here if you're not interested in the Keystone
>  	  DSPs or just want to use a bare minimum kernel.
>  
> +config QCOM_PIL_INFO
> +	tristate
> +
>  config QCOM_RPROC_COMMON
>  	tristate
>  
> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> index 00f09e658cb3..c1b46e9033cb 100644
> --- a/drivers/remoteproc/Makefile
> +++ b/drivers/remoteproc/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
>  obj-$(CONFIG_WKUP_M3_RPROC)		+= wkup_m3_rproc.o
>  obj-$(CONFIG_DA8XX_REMOTEPROC)		+= da8xx_remoteproc.o
>  obj-$(CONFIG_KEYSTONE_REMOTEPROC)	+= keystone_remoteproc.o
> +obj-$(CONFIG_QCOM_PIL_INFO)		+= qcom_pil_info.o
>  obj-$(CONFIG_QCOM_RPROC_COMMON)		+= qcom_common.o
>  obj-$(CONFIG_QCOM_Q6V5_COMMON)		+= qcom_q6v5.o
>  obj-$(CONFIG_QCOM_Q6V5_ADSP)		+= qcom_q6v5_adsp.o
> diff --git a/drivers/remoteproc/qcom_pil_info.c b/drivers/remoteproc/qcom_pil_info.c
> new file mode 100644
> index 000000000000..b0897ae9eae5
> --- /dev/null
> +++ b/drivers/remoteproc/qcom_pil_info.c
> @@ -0,0 +1,150 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2019 Linaro Ltd.
> + */
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/slab.h>

These should be in alphabetical order if there is no depencencies
between them, something checkpatch complains about.

> +
> +struct pil_reloc_entry {
> +	char name[8];

Please add a #define for the name length and reuse it in qcom_pil_info_store()

> +	__le64 base;
> +	__le32 size;
> +} __packed;
> +
> +#define PIL_INFO_SIZE	200
> +#define PIL_INFO_ENTRIES (PIL_INFO_SIZE / sizeof(struct pil_reloc_entry))
> +
> +struct pil_reloc {
> +	struct device *dev;
> +	struct regmap *map;
> +	u32 offset;
> +	int val_bytes;
> +
> +	struct pil_reloc_entry entries[PIL_INFO_ENTRIES];
> +};
> +
> +static struct pil_reloc *_reloc;
> +static DEFINE_MUTEX(reloc_mutex);
> +
> +/**
> + * qcom_pil_info_store() - store PIL information of image in IMEM
> + * @image:	name of the image
> + * @base:	base address of the loaded image
> + * @size:	size of the loaded image
> + */
> +void qcom_pil_info_store(const char *image, phys_addr_t base, size_t size)
> +{
> +	struct pil_reloc_entry *entry;
> +	int idx = -1;
> +	int i;
> +
> +	mutex_lock(&reloc_mutex);
> +	if (!_reloc)

Since it is available, I would use function qcom_pil_info_available().  Also
checkpatch complains about indentation problems related to the 'if' condition
but I can't see what makes it angry.

> +		goto unlock;
> +
> +	for (i = 0; i < PIL_INFO_ENTRIES; i++) {
> +		if (!_reloc->entries[i].name[0]) {
> +			if (idx == -1)
> +				idx = i;
> +			continue;
> +		}
> +
> +		if (!strncmp(_reloc->entries[i].name, image, 8)) {
> +			idx = i;
> +			goto found;
> +		}
> +	}
> +
> +	if (idx == -1) {
> +		dev_warn(_reloc->dev, "insufficient PIL info slots\n");
> +		goto unlock;

Given how this function is used in the next patch I think an error should be
reported to the caller.

> +	}
> +
> +found:
> +	entry = &_reloc->entries[idx];
> +	stracpy(entry->name, image);

Function stracpy() isn't around in mainline.

> +	entry->base = base;
> +	entry->size = size;
> +
> +	regmap_bulk_write(_reloc->map, _reloc->offset + idx * sizeof(*entry),
> +			  entry, sizeof(*entry) / _reloc->val_bytes);

Same here - the error code should be handled and reported to the caller.  

> +
> +unlock:
> +	mutex_unlock(&reloc_mutex);
> +}
> +EXPORT_SYMBOL_GPL(qcom_pil_info_store);
> +
> +/**
> + * qcom_pil_info_available() - query if the pil info is probed
> + *
> + * Return: boolean indicating if the pil info device is probed
> + */
> +bool qcom_pil_info_available(void)
> +{
> +	return !!_reloc;
> +}
> +EXPORT_SYMBOL_GPL(qcom_pil_info_available);
> +
> +static int pil_reloc_probe(struct platform_device *pdev)
> +{
> +	struct pil_reloc *reloc;
> +
> +	reloc = devm_kzalloc(&pdev->dev, sizeof(*reloc), GFP_KERNEL);
> +	if (!reloc)
> +		return -ENOMEM;
> +
> +	reloc->dev = &pdev->dev;
> +	reloc->map = syscon_node_to_regmap(pdev->dev.parent->of_node);
> +	if (IS_ERR(reloc->map))
> +		return PTR_ERR(reloc->map);
> +
> +	if (of_property_read_u32(pdev->dev.of_node, "offset", &reloc->offset))
> +		return -EINVAL;
> +
> +	reloc->val_bytes = regmap_get_val_bytes(reloc->map);
> +	if (reloc->val_bytes < 0)
> +		return -EINVAL;
> +
> +	regmap_bulk_write(reloc->map, reloc->offset, reloc->entries,
> +			  sizeof(reloc->entries) / reloc->val_bytes);

Error code handling.

Thanks,
Mathieu

> +
> +	mutex_lock(&reloc_mutex);
> +	_reloc = reloc;
> +	mutex_unlock(&reloc_mutex);
> +
> +	return 0;
> +}
> +
> +static int pil_reloc_remove(struct platform_device *pdev)
> +{
> +	mutex_lock(&reloc_mutex);
> +	_reloc = NULL;
> +	mutex_unlock(&reloc_mutex);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id pil_reloc_of_match[] = {
> +	{ .compatible = "qcom,pil-reloc-info" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, pil_reloc_of_match);
> +
> +static struct platform_driver pil_reloc_driver = {
> +	.probe = pil_reloc_probe,
> +	.remove = pil_reloc_remove,
> +	.driver = {
> +		.name = "qcom-pil-reloc-info",
> +		.of_match_table = pil_reloc_of_match,
> +	},
> +};
> +module_platform_driver(pil_reloc_driver);
> +
> +MODULE_DESCRIPTION("Qualcomm PIL relocation info");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/remoteproc/qcom_pil_info.h b/drivers/remoteproc/qcom_pil_info.h
> new file mode 100644
> index 000000000000..0372602fae1d
> --- /dev/null
> +++ b/drivers/remoteproc/qcom_pil_info.h
> @@ -0,0 +1,8 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __QCOM_PIL_INFO_H__
> +#define __QCOM_PIL_INFO_H__
> +
> +void qcom_pil_info_store(const char *image, phys_addr_t base, size_t size);
> +bool qcom_pil_info_available(void);
> +
> +#endif
> -- 
> 2.24.0
> 

Re: [PATCH v2 3/8] remoteproc: qcom: Update IMEM PIL info on load

[Mathieu Poirier]

On Thu, Dec 26, 2019 at 09:32:10PM -0800, Bjorn Andersson wrote:
> Update the PIL info region structure in IMEM with information about
> where the firmware for various remoteprocs are loaded.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> 
> Changes since v1:
> - Squashed patches for the individual drivers into one
> - Probe defer on qcom_pil_info_available()
> 
>  drivers/remoteproc/Kconfig          |  3 +++
>  drivers/remoteproc/qcom_q6v5_adsp.c | 19 ++++++++++++++++---
>  drivers/remoteproc/qcom_q6v5_mss.c  |  6 ++++++
>  drivers/remoteproc/qcom_q6v5_pas.c  | 18 +++++++++++++++---
>  drivers/remoteproc/qcom_wcnss.c     | 17 ++++++++++++++---
>  5 files changed, 54 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index 0798602e355a..84922bb922e0 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -135,6 +135,7 @@ config QCOM_Q6V5_PAS
>  	depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n
>  	depends on QCOM_SYSMON || QCOM_SYSMON=n
>  	select MFD_SYSCON
> +	select QCOM_PIL_INFO
>  	select QCOM_MDT_LOADER
>  	select QCOM_Q6V5_COMMON
>  	select QCOM_RPROC_COMMON
> @@ -152,6 +153,7 @@ config QCOM_Q6V5_WCSS
>  	depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n
>  	depends on QCOM_SYSMON || QCOM_SYSMON=n
>  	select MFD_SYSCON
> +	select QCOM_PIL_INFO
>  	select QCOM_MDT_LOADER
>  	select QCOM_Q6V5_COMMON
>  	select QCOM_RPROC_COMMON
> @@ -183,6 +185,7 @@ config QCOM_WCNSS_PIL
>  	depends on QCOM_SMEM
>  	depends on QCOM_SYSMON || QCOM_SYSMON=n
>  	select QCOM_MDT_LOADER
> +	select QCOM_PIL_INFO
>  	select QCOM_RPROC_COMMON
>  	select QCOM_SCM
>  	help
> diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c b/drivers/remoteproc/qcom_q6v5_adsp.c
> index e953886b2eb7..1a942c92d974 100644
> --- a/drivers/remoteproc/qcom_q6v5_adsp.c
> +++ b/drivers/remoteproc/qcom_q6v5_adsp.c
> @@ -26,6 +26,7 @@
>  #include <linux/soc/qcom/smem_state.h>
>  
>  #include "qcom_common.h"
> +#include "qcom_pil_info.h"
>  #include "qcom_q6v5.h"
>  #include "remoteproc_internal.h"
>  
> @@ -82,6 +83,7 @@ struct qcom_adsp {
>  	unsigned int halt_lpass;
>  
>  	int crash_reason_smem;
> +	const char *info_name;
>  
>  	struct completion start_done;
>  	struct completion stop_done;
> @@ -164,10 +166,17 @@ static int qcom_adsp_shutdown(struct qcom_adsp *adsp)
>  static int adsp_load(struct rproc *rproc, const struct firmware *fw)
>  {
>  	struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
> +	int ret;
> +
> +	ret = qcom_mdt_load_no_init(adsp->dev, fw, rproc->firmware, 0,
> +				    adsp->mem_region, adsp->mem_phys, adsp->mem_size,

Line over 80 characters.

> +				    &adsp->mem_reloc);
> +	if (ret)
> +		return ret;
>  
> -	return qcom_mdt_load_no_init(adsp->dev, fw, rproc->firmware, 0,
> -			     adsp->mem_region, adsp->mem_phys, adsp->mem_size,
> -			     &adsp->mem_reloc);
> +	qcom_pil_info_store(adsp->info_name, adsp->mem_reloc, adsp->mem_size);
> +
> +	return 0;
>  }
>  
>  static int adsp_start(struct rproc *rproc)
> @@ -413,6 +422,9 @@ static int adsp_probe(struct platform_device *pdev)
>  	struct rproc *rproc;
>  	int ret;
>  
> +	if (!qcom_pil_info_available())
> +		return -EPROBE_DEFER;
> +
>  	desc = of_device_get_match_data(&pdev->dev);
>  	if (!desc)
>  		return -EINVAL;
> @@ -427,6 +439,7 @@ static int adsp_probe(struct platform_device *pdev)
>  	adsp = (struct qcom_adsp *)rproc->priv;
>  	adsp->dev = &pdev->dev;
>  	adsp->rproc = rproc;
> +	adsp->info_name = desc->sysmon_name;
>  	platform_set_drvdata(pdev, adsp);
>  
>  	ret = adsp_alloc_memory_region(adsp);
> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> index 471128a2e723..6360e69b54e4 100644
> --- a/drivers/remoteproc/qcom_q6v5_mss.c
> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> @@ -28,6 +28,7 @@
>  
>  #include "remoteproc_internal.h"
>  #include "qcom_common.h"
> +#include "qcom_pil_info.h"
>  #include "qcom_q6v5.h"
>  
>  #include <linux/qcom_scm.h>
> @@ -1052,6 +1053,8 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
>  	else if (ret < 0)
>  		dev_err(qproc->dev, "MPSS authentication failed: %d\n", ret);
>  
> +	qcom_pil_info_store("modem", mpss_reloc, qproc->mpss_size);
> +
>  release_firmware:
>  	release_firmware(fw);
>  out:
> @@ -1400,6 +1403,9 @@ static int q6v5_probe(struct platform_device *pdev)
>  	if (desc->need_mem_protection && !qcom_scm_is_available())
>  		return -EPROBE_DEFER;
>  
> +	if (!qcom_pil_info_available())
> +		return -EPROBE_DEFER;
> +
>  	mba_image = desc->hexagon_mba_image;
>  	ret = of_property_read_string_index(pdev->dev.of_node, "firmware-name",
>  					    0, &mba_image);
> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> index b890e6e305f3..4dcdf1301e50 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> @@ -25,6 +25,7 @@
>  #include <linux/soc/qcom/smem_state.h>
>  
>  #include "qcom_common.h"
> +#include "qcom_pil_info.h"
>  #include "qcom_q6v5.h"
>  #include "remoteproc_internal.h"
>  
> @@ -64,6 +65,7 @@ struct qcom_adsp {
>  	int pas_id;
>  	int crash_reason_smem;
>  	bool has_aggre2_clk;
> +	const char *info_name;
>  
>  	struct completion start_done;
>  	struct completion stop_done;
> @@ -117,11 +119,17 @@ static void adsp_pds_disable(struct qcom_adsp *adsp, struct device **pds,
>  static int adsp_load(struct rproc *rproc, const struct firmware *fw)
>  {
>  	struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
> +	int ret;
> +
> +	ret = qcom_mdt_load(adsp->dev, fw, rproc->firmware, adsp->pas_id,
> +			    adsp->mem_region, adsp->mem_phys, adsp->mem_size,
> +			    &adsp->mem_reloc);
> +	if (ret)
> +		return ret;
>  
> -	return qcom_mdt_load(adsp->dev, fw, rproc->firmware, adsp->pas_id,
> -			     adsp->mem_region, adsp->mem_phys, adsp->mem_size,
> -			     &adsp->mem_reloc);
> +	qcom_pil_info_store(adsp->info_name, adsp->mem_reloc, adsp->mem_size);
>  
> +	return 0;
>  }
>  
>  static int adsp_start(struct rproc *rproc)
> @@ -376,6 +384,9 @@ static int adsp_probe(struct platform_device *pdev)
>  	if (!qcom_scm_is_available())
>  		return -EPROBE_DEFER;
>  
> +	if (!qcom_pil_info_available())
> +		return -EPROBE_DEFER;
> +
>  	fw_name = desc->firmware_name;
>  	ret = of_property_read_string(pdev->dev.of_node, "firmware-name",
>  				      &fw_name);
> @@ -396,6 +407,7 @@ static int adsp_probe(struct platform_device *pdev)
>  	adsp->rproc = rproc;
>  	adsp->pas_id = desc->pas_id;
>  	adsp->has_aggre2_clk = desc->has_aggre2_clk;
> +	adsp->info_name = desc->sysmon_name;
>  	platform_set_drvdata(pdev, adsp);
>  
>  	ret = adsp_alloc_memory_region(adsp);
> diff --git a/drivers/remoteproc/qcom_wcnss.c b/drivers/remoteproc/qcom_wcnss.c
> index dc135754bb9c..2c1cefeacf97 100644
> --- a/drivers/remoteproc/qcom_wcnss.c
> +++ b/drivers/remoteproc/qcom_wcnss.c
> @@ -27,6 +27,7 @@
>  
>  #include "qcom_common.h"
>  #include "remoteproc_internal.h"
> +#include "qcom_pil_info.h"
>  #include "qcom_wcnss.h"
>  
>  #define WCNSS_CRASH_REASON_SMEM		422
> @@ -145,10 +146,17 @@ void qcom_wcnss_assign_iris(struct qcom_wcnss *wcnss,
>  static int wcnss_load(struct rproc *rproc, const struct firmware *fw)
>  {
>  	struct qcom_wcnss *wcnss = (struct qcom_wcnss *)rproc->priv;
> +	int ret;
> +
> +	ret = qcom_mdt_load(wcnss->dev, fw, rproc->firmware, WCNSS_PAS_ID,
> +			    wcnss->mem_region, wcnss->mem_phys,
> +			    wcnss->mem_size, &wcnss->mem_reloc);
> +	if (ret)
> +		return ret;
>  
> -	return qcom_mdt_load(wcnss->dev, fw, rproc->firmware, WCNSS_PAS_ID,
> -			     wcnss->mem_region, wcnss->mem_phys,
> -			     wcnss->mem_size, &wcnss->mem_reloc);
> +	qcom_pil_info_store("wcnss", wcnss->mem_reloc, wcnss->mem_size);
> +
> +	return 0;
>  }
>  
>  static void wcnss_indicate_nv_download(struct qcom_wcnss *wcnss)
> @@ -469,6 +477,9 @@ static int wcnss_probe(struct platform_device *pdev)
>  	if (!qcom_scm_is_available())
>  		return -EPROBE_DEFER;
>  
> +	if (!qcom_pil_info_available())
> +		return -EPROBE_DEFER;
> +
>  	if (!qcom_scm_pas_supported(WCNSS_PAS_ID)) {
>  		dev_err(&pdev->dev, "PAS is not available for WCNSS\n");
>  		return -ENXIO;
> -- 
> 2.24.0
> 

Re: [PATCH v2 6/8] remoteproc: Introduce "panic" callback in ops

[Mathieu Poirier]

On Thu, Dec 26, 2019 at 09:32:13PM -0800, Bjorn Andersson wrote:
> Introduce a "panic" function in the remoteproc ops table, to allow
> remoteproc instances to perform operations needed in order to aid in
> post mortem system debugging, such as flushing caches etc, when the
> kernel panics.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> 
> Changes since v1:
> - None
> 
>  drivers/remoteproc/remoteproc_core.c | 17 +++++++++++++++++
>  include/linux/remoteproc.h           |  4 ++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 307df98347ba..779f19d6d8e7 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1832,6 +1832,17 @@ void rproc_shutdown(struct rproc *rproc)
>  }
>  EXPORT_SYMBOL(rproc_shutdown);
>  
> +static int rproc_panic_handler(struct notifier_block *nb, unsigned long event,
> +			       void *ptr)
> +{
> +	struct rproc *rproc = container_of(nb, struct rproc, panic_nb);
> +
> +	if (rproc->state == RPROC_RUNNING)
> +		rproc->ops->panic(rproc);
> +
> +	return NOTIFY_DONE;
> +}
> +
>  /**
>   * rproc_get_by_phandle() - find a remote processor by phandle
>   * @phandle: phandle to the rproc
> @@ -2057,6 +2068,12 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
>  		rproc->ops->get_boot_addr = rproc_elf_get_boot_addr;
>  	}
>  
> +	/* Register panic notifier for remoteprocs with "panic" callback */
> +	if (rproc->ops->panic) {
> +		rproc->panic_nb.notifier_call = rproc_panic_handler;
> +		atomic_notifier_chain_register(&panic_notifier_list, &rproc->panic_nb);

Line over 80 characters.

> +	}
> +
>  	mutex_init(&rproc->lock);
>  
>  	idr_init(&rproc->notifyids);
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 16ad66683ad0..7836c528d309 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -369,6 +369,7 @@ enum rsc_handling_status {
>   *			expects to find it
>   * @sanity_check:	sanity check the fw image
>   * @get_boot_addr:	get boot address to entry point specified in firmware
> + * @panic:	optional callback to react to system panic
>   */
>  struct rproc_ops {
>  	int (*start)(struct rproc *rproc);
> @@ -383,6 +384,7 @@ struct rproc_ops {
>  	int (*load)(struct rproc *rproc, const struct firmware *fw);
>  	int (*sanity_check)(struct rproc *rproc, const struct firmware *fw);
>  	u32 (*get_boot_addr)(struct rproc *rproc, const struct firmware *fw);
> +	void (*panic)(struct rproc *rproc);
>  };
>  
>  /**
> @@ -481,6 +483,7 @@ struct rproc_dump_segment {
>   * @auto_boot: flag to indicate if remote processor should be auto-started
>   * @dump_segments: list of segments in the firmware
>   * @nb_vdev: number of vdev currently handled by rproc
> + * @panic_nb: notifier_block for remoteproc's panic handler
>   */
>  struct rproc {
>  	struct list_head node;
> @@ -514,6 +517,7 @@ struct rproc {
>  	bool auto_boot;
>  	struct list_head dump_segments;
>  	int nb_vdev;
> +	struct notifier_block panic_nb;
>  };
>  
>  /**
> -- 
> 2.24.0
> 

Re: [PATCH v2 7/8] remoteproc: qcom: q6v5: Add common panic handler

[Mathieu Poirier]

On Thu, Dec 26, 2019 at 09:32:14PM -0800, Bjorn Andersson wrote:
> Add a common panic handler that invokes a stop request and sleep enough
> to let the remoteproc flush it's caches etc in order to aid post mortem
> debugging.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> 
> Changes since v1:
> - None
> 
>  drivers/remoteproc/qcom_q6v5.c | 19 +++++++++++++++++++
>  drivers/remoteproc/qcom_q6v5.h |  1 +
>  2 files changed, 20 insertions(+)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5.c b/drivers/remoteproc/qcom_q6v5.c
> index cb0f4a0be032..17167c980e02 100644
> --- a/drivers/remoteproc/qcom_q6v5.c
> +++ b/drivers/remoteproc/qcom_q6v5.c
> @@ -6,6 +6,7 @@
>   * Copyright (C) 2014 Sony Mobile Communications AB
>   * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
>   */
> +#include <linux/delay.h>
>  #include <linux/kernel.h>
>  #include <linux/platform_device.h>
>  #include <linux/interrupt.h>
> @@ -15,6 +16,8 @@
>  #include <linux/remoteproc.h>
>  #include "qcom_q6v5.h"
>  
> +#define Q6V5_PANIC_DELAY_MS	200
> +
>  /**
>   * qcom_q6v5_prepare() - reinitialize the qcom_q6v5 context before start
>   * @q6v5:	reference to qcom_q6v5 context to be reinitialized
> @@ -162,6 +165,22 @@ int qcom_q6v5_request_stop(struct qcom_q6v5 *q6v5)
>  }
>  EXPORT_SYMBOL_GPL(qcom_q6v5_request_stop);
>  
> +/**
> + * qcom_q6v5_panic() - panic handler to invoke a stop on the remote
> + * @q6v5:	reference to qcom_q6v5 context
> + *
> + * Set the stop bit and sleep in order to allow the remote processor to flush
> + * its caches etc for post mortem debugging.
> + */
> +void qcom_q6v5_panic(struct qcom_q6v5 *q6v5)
> +{
> +	qcom_smem_state_update_bits(q6v5->state,
> +				    BIT(q6v5->stop_bit), BIT(q6v5->stop_bit));
> +
> +	mdelay(Q6V5_PANIC_DELAY_MS);

I really wonder if the delay should be part of the remoteproc core and
configurable via device tree.  Wanting the remote processor to flush its caches
is likely something other vendors will want when dealing with a kernel panic.
It would be nice to see if other people have an opinion on this topic.  If not
then we can keep the delay here and move it to the core if need be.

Thanks,
Mathieu

> +}
> +EXPORT_SYMBOL_GPL(qcom_q6v5_panic);
> +
>  /**
>   * qcom_q6v5_init() - initializer of the q6v5 common struct
>   * @q6v5:	handle to be initialized
> diff --git a/drivers/remoteproc/qcom_q6v5.h b/drivers/remoteproc/qcom_q6v5.h
> index 7ac92c1e0f49..c37e6fd063e4 100644
> --- a/drivers/remoteproc/qcom_q6v5.h
> +++ b/drivers/remoteproc/qcom_q6v5.h
> @@ -42,5 +42,6 @@ int qcom_q6v5_prepare(struct qcom_q6v5 *q6v5);
>  int qcom_q6v5_unprepare(struct qcom_q6v5 *q6v5);
>  int qcom_q6v5_request_stop(struct qcom_q6v5 *q6v5);
>  int qcom_q6v5_wait_for_start(struct qcom_q6v5 *q6v5, int timeout);
> +void qcom_q6v5_panic(struct qcom_q6v5 *q6v5);
>  
>  #endif
> -- 
> 2.24.0
> 

Re: [PATCH v2 2/8] remoteproc: qcom: Introduce driver to store pil info in IMEM

[Bjorn Andersson]

On Fri 10 Jan 13:18 PST 2020, Mathieu Poirier wrote:
> On Thu, Dec 26, 2019 at 09:32:09PM -0800, Bjorn Andersson wrote:
[..]
> > diff --git a/drivers/remoteproc/qcom_pil_info.c b/drivers/remoteproc/qcom_pil_info.c
> > new file mode 100644
> > index 000000000000..b0897ae9eae5
> > --- /dev/null
> > +++ b/drivers/remoteproc/qcom_pil_info.c
> > @@ -0,0 +1,150 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2019 Linaro Ltd.
> > + */
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/mutex.h>
> > +#include <linux/regmap.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/slab.h>
> 
> These should be in alphabetical order if there is no depencencies
> between them, something checkpatch complains about.
> 

Of course.

> > +
> > +struct pil_reloc_entry {
> > +	char name[8];
> 
> Please add a #define for the name length and reuse it in qcom_pil_info_store()
> 

Ok

[..]
> > +void qcom_pil_info_store(const char *image, phys_addr_t base, size_t size)
> > +{
> > +	struct pil_reloc_entry *entry;
> > +	int idx = -1;
> > +	int i;
> > +
> > +	mutex_lock(&reloc_mutex);
> > +	if (!_reloc)
> 
> Since it is available, I would use function qcom_pil_info_available().  Also
> checkpatch complains about indentation problems related to the 'if' condition
> but I can't see what makes it angry.
> 

Sure thing, and I'll double check the indentation.

> > +		goto unlock;
> > +
> > +	for (i = 0; i < PIL_INFO_ENTRIES; i++) {
> > +		if (!_reloc->entries[i].name[0]) {
> > +			if (idx == -1)
> > +				idx = i;
> > +			continue;
> > +		}
> > +
> > +		if (!strncmp(_reloc->entries[i].name, image, 8)) {
> > +			idx = i;
> > +			goto found;
> > +		}
> > +	}
> > +
> > +	if (idx == -1) {
> > +		dev_warn(_reloc->dev, "insufficient PIL info slots\n");
> > +		goto unlock;
> 
> Given how this function is used in the next patch I think an error should be
> reported to the caller.
> 

Just to clarify, certain global errors will cause the entire device to
be reset and allow memory contents to be extracted for analysis in post
mortem tools. This patch ensures that this information contains
(structured) information about where each remote processor is loaded.
Afaict the purpose of propagating errors from this function would be for
the caller to abort the launching of a remote processor.

I think it's better to take the risk of having insufficient data for the
post mortem tools than to fail booting a remote processor for a reason
that won't affect normal operation.

> > +	}
> > +
> > +found:
> > +	entry = &_reloc->entries[idx];
> > +	stracpy(entry->name, image);
> 
> Function stracpy() isn't around in mainline.
> 

Good catch, I'll spin this with a strscpy() to avoid build errors until
stracpy lands.

> > +	entry->base = base;
> > +	entry->size = size;
> > +
> > +	regmap_bulk_write(_reloc->map, _reloc->offset + idx * sizeof(*entry),
> > +			  entry, sizeof(*entry) / _reloc->val_bytes);
> 
> Same here - the error code should be handled and reported to the caller.  
> 

Will undo the "allocation" of _reloc->entries[idx] on failure, let me
know what you think about my reasoning above regarding propagating this
error (or in particular acting upon the propagated value).

> > +
> > +unlock:
> > +	mutex_unlock(&reloc_mutex);
> > +}
> > +EXPORT_SYMBOL_GPL(qcom_pil_info_store);
[..]
> > +static int pil_reloc_probe(struct platform_device *pdev)
> > +{
> > +	struct pil_reloc *reloc;
> > +
> > +	reloc = devm_kzalloc(&pdev->dev, sizeof(*reloc), GFP_KERNEL);
> > +	if (!reloc)
> > +		return -ENOMEM;
> > +
> > +	reloc->dev = &pdev->dev;
> > +	reloc->map = syscon_node_to_regmap(pdev->dev.parent->of_node);
> > +	if (IS_ERR(reloc->map))
> > +		return PTR_ERR(reloc->map);
> > +
> > +	if (of_property_read_u32(pdev->dev.of_node, "offset", &reloc->offset))
> > +		return -EINVAL;
> > +
> > +	reloc->val_bytes = regmap_get_val_bytes(reloc->map);
> > +	if (reloc->val_bytes < 0)
> > +		return -EINVAL;
> > +
> > +	regmap_bulk_write(reloc->map, reloc->offset, reloc->entries,
> > +			  sizeof(reloc->entries) / reloc->val_bytes);
> 
> Error code handling.
> 

Yes, that makes sense.

Thanks for the review Mathieu!

Regards,
Bjorn

> Thanks,
> Mathieu
> 
> > +
> > +	mutex_lock(&reloc_mutex);
> > +	_reloc = reloc;
> > +	mutex_unlock(&reloc_mutex);
> > +
> > +	return 0;
> > +}
> > +
> > +static int pil_reloc_remove(struct platform_device *pdev)
> > +{
> > +	mutex_lock(&reloc_mutex);
> > +	_reloc = NULL;
> > +	mutex_unlock(&reloc_mutex);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id pil_reloc_of_match[] = {
> > +	{ .compatible = "qcom,pil-reloc-info" },
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(of, pil_reloc_of_match);
> > +
> > +static struct platform_driver pil_reloc_driver = {
> > +	.probe = pil_reloc_probe,
> > +	.remove = pil_reloc_remove,
> > +	.driver = {
> > +		.name = "qcom-pil-reloc-info",
> > +		.of_match_table = pil_reloc_of_match,
> > +	},
> > +};
> > +module_platform_driver(pil_reloc_driver);
> > +
> > +MODULE_DESCRIPTION("Qualcomm PIL relocation info");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/remoteproc/qcom_pil_info.h b/drivers/remoteproc/qcom_pil_info.h
> > new file mode 100644
> > index 000000000000..0372602fae1d
> > --- /dev/null
> > +++ b/drivers/remoteproc/qcom_pil_info.h
> > @@ -0,0 +1,8 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __QCOM_PIL_INFO_H__
> > +#define __QCOM_PIL_INFO_H__
> > +
> > +void qcom_pil_info_store(const char *image, phys_addr_t base, size_t size);
> > +bool qcom_pil_info_available(void);
> > +
> > +#endif
> > -- 
> > 2.24.0
> > 

Re: [PATCH v2 2/8] remoteproc: qcom: Introduce driver to store pil info in IMEM

[Mathieu Poirier]

On Tue, 21 Jan 2020 at 19:02, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Fri 10 Jan 13:18 PST 2020, Mathieu Poirier wrote:
> > On Thu, Dec 26, 2019 at 09:32:09PM -0800, Bjorn Andersson wrote:
> [..]
> > > diff --git a/drivers/remoteproc/qcom_pil_info.c b/drivers/remoteproc/qcom_pil_info.c
> > > new file mode 100644
> > > index 000000000000..b0897ae9eae5
> > > --- /dev/null
> > > +++ b/drivers/remoteproc/qcom_pil_info.c
> > > @@ -0,0 +1,150 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Copyright (c) 2019 Linaro Ltd.
> > > + */
> > > +#include <linux/module.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/of.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/mfd/syscon.h>
> > > +#include <linux/slab.h>
> >
> > These should be in alphabetical order if there is no depencencies
> > between them, something checkpatch complains about.
> >
>
> Of course.
>
> > > +
> > > +struct pil_reloc_entry {
> > > +   char name[8];
> >
> > Please add a #define for the name length and reuse it in qcom_pil_info_store()
> >
>
> Ok
>
> [..]
> > > +void qcom_pil_info_store(const char *image, phys_addr_t base, size_t size)
> > > +{
> > > +   struct pil_reloc_entry *entry;
> > > +   int idx = -1;
> > > +   int i;
> > > +
> > > +   mutex_lock(&reloc_mutex);
> > > +   if (!_reloc)
> >
> > Since it is available, I would use function qcom_pil_info_available().  Also
> > checkpatch complains about indentation problems related to the 'if' condition
> > but I can't see what makes it angry.
> >
>
> Sure thing, and I'll double check the indentation.
>
> > > +           goto unlock;
> > > +
> > > +   for (i = 0; i < PIL_INFO_ENTRIES; i++) {
> > > +           if (!_reloc->entries[i].name[0]) {
> > > +                   if (idx == -1)
> > > +                           idx = i;
> > > +                   continue;
> > > +           }
> > > +
> > > +           if (!strncmp(_reloc->entries[i].name, image, 8)) {
> > > +                   idx = i;
> > > +                   goto found;
> > > +           }
> > > +   }
> > > +
> > > +   if (idx == -1) {
> > > +           dev_warn(_reloc->dev, "insufficient PIL info slots\n");
> > > +           goto unlock;
> >
> > Given how this function is used in the next patch I think an error should be
> > reported to the caller.
> >
>
> Just to clarify, certain global errors will cause the entire device to
> be reset and allow memory contents to be extracted for analysis in post
> mortem tools. This patch ensures that this information contains
> (structured) information about where each remote processor is loaded.
> Afaict the purpose of propagating errors from this function would be for
> the caller to abort the launching of a remote processor.
>
> I think it's better to take the risk of having insufficient data for the
> post mortem tools than to fail booting a remote processor for a reason
> that won't affect normal operation.

I understand the reasoning.  In that case it is probably best to let
the caller decide what to do with the returned error than deal with it
locally, especially since this is an exported function.  When using
qcom_pil_info_store(), I would write a comment that justifies the
reason for ignoring the return value (what you have above is quite
good).  Otherwise it is just a matter of time before automated tools
pickup on the anomaly and send patches to fix it.

Thanks,
Mathieu

>
> > > +   }
> > > +
> > > +found:
> > > +   entry = &_reloc->entries[idx];
> > > +   stracpy(entry->name, image);
> >
> > Function stracpy() isn't around in mainline.
> >
>
> Good catch, I'll spin this with a strscpy() to avoid build errors until
> stracpy lands.
>
> > > +   entry->base = base;
> > > +   entry->size = size;
> > > +
> > > +   regmap_bulk_write(_reloc->map, _reloc->offset + idx * sizeof(*entry),
> > > +                     entry, sizeof(*entry) / _reloc->val_bytes);
> >
> > Same here - the error code should be handled and reported to the caller.
> >
>
> Will undo the "allocation" of _reloc->entries[idx] on failure, let me
> know what you think about my reasoning above regarding propagating this
> error (or in particular acting upon the propagated value).
>
> > > +
> > > +unlock:
> > > +   mutex_unlock(&reloc_mutex);
> > > +}
> > > +EXPORT_SYMBOL_GPL(qcom_pil_info_store);
> [..]
> > > +static int pil_reloc_probe(struct platform_device *pdev)
> > > +{
> > > +   struct pil_reloc *reloc;
> > > +
> > > +   reloc = devm_kzalloc(&pdev->dev, sizeof(*reloc), GFP_KERNEL);
> > > +   if (!reloc)
> > > +           return -ENOMEM;
> > > +
> > > +   reloc->dev = &pdev->dev;
> > > +   reloc->map = syscon_node_to_regmap(pdev->dev.parent->of_node);
> > > +   if (IS_ERR(reloc->map))
> > > +           return PTR_ERR(reloc->map);
> > > +
> > > +   if (of_property_read_u32(pdev->dev.of_node, "offset", &reloc->offset))
> > > +           return -EINVAL;
> > > +
> > > +   reloc->val_bytes = regmap_get_val_bytes(reloc->map);
> > > +   if (reloc->val_bytes < 0)
> > > +           return -EINVAL;
> > > +
> > > +   regmap_bulk_write(reloc->map, reloc->offset, reloc->entries,
> > > +                     sizeof(reloc->entries) / reloc->val_bytes);
> >
> > Error code handling.
> >
>
> Yes, that makes sense.
>
> Thanks for the review Mathieu!
>
> Regards,
> Bjorn
>
> > Thanks,
> > Mathieu
> >
> > > +
> > > +   mutex_lock(&reloc_mutex);
> > > +   _reloc = reloc;
> > > +   mutex_unlock(&reloc_mutex);
> > > +
> > > +   return 0;
> > > +}
> > > +
> > > +static int pil_reloc_remove(struct platform_device *pdev)
> > > +{
> > > +   mutex_lock(&reloc_mutex);
> > > +   _reloc = NULL;
> > > +   mutex_unlock(&reloc_mutex);
> > > +
> > > +   return 0;
> > > +}
> > > +
> > > +static const struct of_device_id pil_reloc_of_match[] = {
> > > +   { .compatible = "qcom,pil-reloc-info" },
> > > +   {}
> > > +};
> > > +MODULE_DEVICE_TABLE(of, pil_reloc_of_match);
> > > +
> > > +static struct platform_driver pil_reloc_driver = {
> > > +   .probe = pil_reloc_probe,
> > > +   .remove = pil_reloc_remove,
> > > +   .driver = {
> > > +           .name = "qcom-pil-reloc-info",
> > > +           .of_match_table = pil_reloc_of_match,
> > > +   },
> > > +};
> > > +module_platform_driver(pil_reloc_driver);
> > > +
> > > +MODULE_DESCRIPTION("Qualcomm PIL relocation info");
> > > +MODULE_LICENSE("GPL v2");
> > > diff --git a/drivers/remoteproc/qcom_pil_info.h b/drivers/remoteproc/qcom_pil_info.h
> > > new file mode 100644
> > > index 000000000000..0372602fae1d
> > > --- /dev/null
> > > +++ b/drivers/remoteproc/qcom_pil_info.h
> > > @@ -0,0 +1,8 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +#ifndef __QCOM_PIL_INFO_H__
> > > +#define __QCOM_PIL_INFO_H__
> > > +
> > > +void qcom_pil_info_store(const char *image, phys_addr_t base, size_t size);
> > > +bool qcom_pil_info_available(void);
> > > +
> > > +#endif
> > > --
> > > 2.24.0
> > >

Re: [PATCH v2 2/8] remoteproc: qcom: Introduce driver to store pil info in IMEM

[Bjorn Andersson]

On Wed 22 Jan 11:04 PST 2020, Mathieu Poirier wrote:

> On Tue, 21 Jan 2020 at 19:02, Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> >
> > On Fri 10 Jan 13:18 PST 2020, Mathieu Poirier wrote:
> > > On Thu, Dec 26, 2019 at 09:32:09PM -0800, Bjorn Andersson wrote:
> > [..]
> > > > diff --git a/drivers/remoteproc/qcom_pil_info.c b/drivers/remoteproc/qcom_pil_info.c
> > > > new file mode 100644
> > > > index 000000000000..b0897ae9eae5
> > > > --- /dev/null
> > > > +++ b/drivers/remoteproc/qcom_pil_info.c
> > > > @@ -0,0 +1,150 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > +/*
> > > > + * Copyright (c) 2019 Linaro Ltd.
> > > > + */
> > > > +#include <linux/module.h>
> > > > +#include <linux/kernel.h>
> > > > +#include <linux/of.h>
> > > > +#include <linux/platform_device.h>
> > > > +#include <linux/mutex.h>
> > > > +#include <linux/regmap.h>
> > > > +#include <linux/mfd/syscon.h>
> > > > +#include <linux/slab.h>
> > >
> > > These should be in alphabetical order if there is no depencencies
> > > between them, something checkpatch complains about.
> > >
> >
> > Of course.
> >
> > > > +
> > > > +struct pil_reloc_entry {
> > > > +   char name[8];
> > >
> > > Please add a #define for the name length and reuse it in qcom_pil_info_store()
> > >
> >
> > Ok
> >
> > [..]
> > > > +void qcom_pil_info_store(const char *image, phys_addr_t base, size_t size)
> > > > +{
> > > > +   struct pil_reloc_entry *entry;
> > > > +   int idx = -1;
> > > > +   int i;
> > > > +
> > > > +   mutex_lock(&reloc_mutex);
> > > > +   if (!_reloc)
> > >
> > > Since it is available, I would use function qcom_pil_info_available().  Also
> > > checkpatch complains about indentation problems related to the 'if' condition
> > > but I can't see what makes it angry.
> > >
> >
> > Sure thing, and I'll double check the indentation.
> >
> > > > +           goto unlock;
> > > > +
> > > > +   for (i = 0; i < PIL_INFO_ENTRIES; i++) {
> > > > +           if (!_reloc->entries[i].name[0]) {
> > > > +                   if (idx == -1)
> > > > +                           idx = i;
> > > > +                   continue;
> > > > +           }
> > > > +
> > > > +           if (!strncmp(_reloc->entries[i].name, image, 8)) {
> > > > +                   idx = i;
> > > > +                   goto found;
> > > > +           }
> > > > +   }
> > > > +
> > > > +   if (idx == -1) {
> > > > +           dev_warn(_reloc->dev, "insufficient PIL info slots\n");
> > > > +           goto unlock;
> > >
> > > Given how this function is used in the next patch I think an error should be
> > > reported to the caller.
> > >
> >
> > Just to clarify, certain global errors will cause the entire device to
> > be reset and allow memory contents to be extracted for analysis in post
> > mortem tools. This patch ensures that this information contains
> > (structured) information about where each remote processor is loaded.
> > Afaict the purpose of propagating errors from this function would be for
> > the caller to abort the launching of a remote processor.
> >
> > I think it's better to take the risk of having insufficient data for the
> > post mortem tools than to fail booting a remote processor for a reason
> > that won't affect normal operation.
> 
> I understand the reasoning.  In that case it is probably best to let
> the caller decide what to do with the returned error than deal with it
> locally, especially since this is an exported function.  When using
> qcom_pil_info_store(), I would write a comment that justifies the
> reason for ignoring the return value (what you have above is quite
> good).  Otherwise it is just a matter of time before automated tools
> pickup on the anomaly and send patches to fix it.
> 

You're right, moving the decision to the remoteproc drivers will result
in the decision being implemented in the right place. I will respin it
accordingly.

Thanks!
Bjorn

Re: [PATCH v2 7/8] remoteproc: qcom: q6v5: Add common panic handler

[Bjorn Andersson]

On Fri 10 Jan 13:28 PST 2020, Mathieu Poirier wrote:

> On Thu, Dec 26, 2019 at 09:32:14PM -0800, Bjorn Andersson wrote:
> > Add a common panic handler that invokes a stop request and sleep enough
> > to let the remoteproc flush it's caches etc in order to aid post mortem
> > debugging.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> > 
> > Changes since v1:
> > - None
> > 
> >  drivers/remoteproc/qcom_q6v5.c | 19 +++++++++++++++++++
> >  drivers/remoteproc/qcom_q6v5.h |  1 +
> >  2 files changed, 20 insertions(+)
> > 
> > diff --git a/drivers/remoteproc/qcom_q6v5.c b/drivers/remoteproc/qcom_q6v5.c
> > index cb0f4a0be032..17167c980e02 100644
> > --- a/drivers/remoteproc/qcom_q6v5.c
> > +++ b/drivers/remoteproc/qcom_q6v5.c
> > @@ -6,6 +6,7 @@
> >   * Copyright (C) 2014 Sony Mobile Communications AB
> >   * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
> >   */
> > +#include <linux/delay.h>
> >  #include <linux/kernel.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/interrupt.h>
> > @@ -15,6 +16,8 @@
> >  #include <linux/remoteproc.h>
> >  #include "qcom_q6v5.h"
> >  
> > +#define Q6V5_PANIC_DELAY_MS	200
> > +
> >  /**
> >   * qcom_q6v5_prepare() - reinitialize the qcom_q6v5 context before start
> >   * @q6v5:	reference to qcom_q6v5 context to be reinitialized
> > @@ -162,6 +165,22 @@ int qcom_q6v5_request_stop(struct qcom_q6v5 *q6v5)
> >  }
> >  EXPORT_SYMBOL_GPL(qcom_q6v5_request_stop);
> >  
> > +/**
> > + * qcom_q6v5_panic() - panic handler to invoke a stop on the remote
> > + * @q6v5:	reference to qcom_q6v5 context
> > + *
> > + * Set the stop bit and sleep in order to allow the remote processor to flush
> > + * its caches etc for post mortem debugging.
> > + */
> > +void qcom_q6v5_panic(struct qcom_q6v5 *q6v5)
> > +{
> > +	qcom_smem_state_update_bits(q6v5->state,
> > +				    BIT(q6v5->stop_bit), BIT(q6v5->stop_bit));
> > +
> > +	mdelay(Q6V5_PANIC_DELAY_MS);
> 
> I really wonder if the delay should be part of the remoteproc core and
> configurable via device tree.  Wanting the remote processor to flush its caches
> is likely something other vendors will want when dealing with a kernel panic.
> It would be nice to see if other people have an opinion on this topic.  If not
> then we can keep the delay here and move it to the core if need be.
> 

I gave this some more thought and what we're trying to achieve is to
signal the remote processors about the panic and then give them time to
react, but per the proposal (and Qualcomm downstream iirc) we will do
this for each remote processor, one by one.

So in the typical case of a Qualcomm platform with 4-5 remoteprocs we'll
end up giving the first one a whole second to react and the last one
"only" 200ms.

Moving the delay to the core by iterating over rproc_list calling
panic() and then delaying would be cleaner imo.

It might be nice to make this configurable in DT, but I agree that it
would be nice to hear from others if this would be useful.

Regards,
Bjorn

> Thanks,
> Mathieu
> 
> > +}
> > +EXPORT_SYMBOL_GPL(qcom_q6v5_panic);
> > +
> >  /**
> >   * qcom_q6v5_init() - initializer of the q6v5 common struct
> >   * @q6v5:	handle to be initialized
> > diff --git a/drivers/remoteproc/qcom_q6v5.h b/drivers/remoteproc/qcom_q6v5.h
> > index 7ac92c1e0f49..c37e6fd063e4 100644
> > --- a/drivers/remoteproc/qcom_q6v5.h
> > +++ b/drivers/remoteproc/qcom_q6v5.h
> > @@ -42,5 +42,6 @@ int qcom_q6v5_prepare(struct qcom_q6v5 *q6v5);
> >  int qcom_q6v5_unprepare(struct qcom_q6v5 *q6v5);
> >  int qcom_q6v5_request_stop(struct qcom_q6v5 *q6v5);
> >  int qcom_q6v5_wait_for_start(struct qcom_q6v5 *q6v5, int timeout);
> > +void qcom_q6v5_panic(struct qcom_q6v5 *q6v5);
> >  
> >  #endif
> > -- 
> > 2.24.0
> > 

Re: [PATCH v2 2/8] remoteproc: qcom: Introduce driver to store pil info in IMEM

[rishabhb]

On 2019-12-26 21:32, Bjorn Andersson wrote:
> A region in IMEM is used to communicate load addresses of remoteproc to
> post mortem debug tools. Implement a driver that can be used to store
> this information in order to enable these tools to process collected
> ramdumps.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> 
> Changes since v1:
> - Added helper to probe defer clients
> - Fixed logical bug in slot scan
> - Added SPDX header in header file
> 
>  drivers/remoteproc/Kconfig         |   3 +
>  drivers/remoteproc/Makefile        |   1 +
>  drivers/remoteproc/qcom_pil_info.c | 150 +++++++++++++++++++++++++++++
>  drivers/remoteproc/qcom_pil_info.h |   8 ++
>  4 files changed, 162 insertions(+)
>  create mode 100644 drivers/remoteproc/qcom_pil_info.c
>  create mode 100644 drivers/remoteproc/qcom_pil_info.h
> 
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index 94afdde4bc9f..0798602e355a 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -85,6 +85,9 @@ config KEYSTONE_REMOTEPROC
>  	  It's safe to say N here if you're not interested in the Keystone
>  	  DSPs or just want to use a bare minimum kernel.
> 
> +config QCOM_PIL_INFO
> +	tristate
> +
>  config QCOM_RPROC_COMMON
>  	tristate
> 
> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> index 00f09e658cb3..c1b46e9033cb 100644
> --- a/drivers/remoteproc/Makefile
> +++ b/drivers/remoteproc/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
>  obj-$(CONFIG_WKUP_M3_RPROC)		+= wkup_m3_rproc.o
>  obj-$(CONFIG_DA8XX_REMOTEPROC)		+= da8xx_remoteproc.o
>  obj-$(CONFIG_KEYSTONE_REMOTEPROC)	+= keystone_remoteproc.o
> +obj-$(CONFIG_QCOM_PIL_INFO)		+= qcom_pil_info.o
>  obj-$(CONFIG_QCOM_RPROC_COMMON)		+= qcom_common.o
>  obj-$(CONFIG_QCOM_Q6V5_COMMON)		+= qcom_q6v5.o
>  obj-$(CONFIG_QCOM_Q6V5_ADSP)		+= qcom_q6v5_adsp.o
> diff --git a/drivers/remoteproc/qcom_pil_info.c
> b/drivers/remoteproc/qcom_pil_info.c
> new file mode 100644
> index 000000000000..b0897ae9eae5
> --- /dev/null
> +++ b/drivers/remoteproc/qcom_pil_info.c
> @@ -0,0 +1,150 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2019 Linaro Ltd.
> + */
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/slab.h>
> +
> +struct pil_reloc_entry {
> +	char name[8];
> +	__le64 base;
> +	__le32 size;
> +} __packed;
> +
> +#define PIL_INFO_SIZE	200
> +#define PIL_INFO_ENTRIES (PIL_INFO_SIZE / sizeof(struct 
> pil_reloc_entry))
> +
> +struct pil_reloc {
> +	struct device *dev;
> +	struct regmap *map;
> +	u32 offset;
> +	int val_bytes;
> +
> +	struct pil_reloc_entry entries[PIL_INFO_ENTRIES];
> +};
> +
> +static struct pil_reloc *_reloc;
> +static DEFINE_MUTEX(reloc_mutex);
> +
> +/**
> + * qcom_pil_info_store() - store PIL information of image in IMEM
> + * @image:	name of the image
> + * @base:	base address of the loaded image
> + * @size:	size of the loaded image
> + */
> +void qcom_pil_info_store(const char *image, phys_addr_t base, size_t 
> size)
> +{
> +	struct pil_reloc_entry *entry;
> +	int idx = -1;
> +	int i;
> +
> +	mutex_lock(&reloc_mutex);
> +	if (!_reloc)
> +		goto unlock;
> +
> +	for (i = 0; i < PIL_INFO_ENTRIES; i++) {
> +		if (!_reloc->entries[i].name[0]) {
> +			if (idx == -1)
> +				idx = i;
> +			continue;
> +		}
> +
> +		if (!strncmp(_reloc->entries[i].name, image, 8)) {
> +			idx = i;
> +			goto found;
> +		}
> +	}
> +
> +	if (idx == -1) {
> +		dev_warn(_reloc->dev, "insufficient PIL info slots\n");
> +		goto unlock;
> +	}
> +
> +found:
> +	entry = &_reloc->entries[idx];
> +	stracpy(entry->name, image);
> +	entry->base = base;
> +	entry->size = size;
> +
> +	regmap_bulk_write(_reloc->map, _reloc->offset + idx * sizeof(*entry),
> +			  entry, sizeof(*entry) / _reloc->val_bytes);
> +
> +unlock:
> +	mutex_unlock(&reloc_mutex);
> +}
> +EXPORT_SYMBOL_GPL(qcom_pil_info_store);
> +
> +/**
> + * qcom_pil_info_available() - query if the pil info is probed
> + *
> + * Return: boolean indicating if the pil info device is probed
> + */
> +bool qcom_pil_info_available(void)
> +{
> +	return !!_reloc;
> +}
> +EXPORT_SYMBOL_GPL(qcom_pil_info_available);
> +
> +static int pil_reloc_probe(struct platform_device *pdev)
> +{
> +	struct pil_reloc *reloc;
> +
> +	reloc = devm_kzalloc(&pdev->dev, sizeof(*reloc), GFP_KERNEL);
> +	if (!reloc)
> +		return -ENOMEM;
> +
> +	reloc->dev = &pdev->dev;
> +	reloc->map = syscon_node_to_regmap(pdev->dev.parent->of_node);
If there are multiple entries like "pil-reloc" in the imem node
mapping the entire imem multiple times may not work. Is there a way
we can somehow just iomap the required region for pil?
> +	if (IS_ERR(reloc->map))
> +		return PTR_ERR(reloc->map);
> +
> +	if (of_property_read_u32(pdev->dev.of_node, "offset", 
> &reloc->offset))
> +		return -EINVAL;
> +
> +	reloc->val_bytes = regmap_get_val_bytes(reloc->map);
> +	if (reloc->val_bytes < 0)
> +		return -EINVAL;
> +
> +	regmap_bulk_write(reloc->map, reloc->offset, reloc->entries,
> +			  sizeof(reloc->entries) / reloc->val_bytes);
> +
> +	mutex_lock(&reloc_mutex);
> +	_reloc = reloc;
> +	mutex_unlock(&reloc_mutex);
> +
> +	return 0;
> +}
> +
> +static int pil_reloc_remove(struct platform_device *pdev)
> +{
> +	mutex_lock(&reloc_mutex);
> +	_reloc = NULL;
> +	mutex_unlock(&reloc_mutex);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id pil_reloc_of_match[] = {
> +	{ .compatible = "qcom,pil-reloc-info" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, pil_reloc_of_match);
> +
> +static struct platform_driver pil_reloc_driver = {
> +	.probe = pil_reloc_probe,
> +	.remove = pil_reloc_remove,
> +	.driver = {
> +		.name = "qcom-pil-reloc-info",
> +		.of_match_table = pil_reloc_of_match,
> +	},
> +};
> +module_platform_driver(pil_reloc_driver);
> +
> +MODULE_DESCRIPTION("Qualcomm PIL relocation info");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/remoteproc/qcom_pil_info.h
> b/drivers/remoteproc/qcom_pil_info.h
> new file mode 100644
> index 000000000000..0372602fae1d
> --- /dev/null
> +++ b/drivers/remoteproc/qcom_pil_info.h
> @@ -0,0 +1,8 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __QCOM_PIL_INFO_H__
> +#define __QCOM_PIL_INFO_H__
> +
> +void qcom_pil_info_store(const char *image, phys_addr_t base, size_t 
> size);
> +bool qcom_pil_info_available(void);
> +
> +#endif

Re: [PATCH v2 2/8] remoteproc: qcom: Introduce driver to store pil info in IMEM

[Bjorn Andersson]

On Wed 22 Jan 14:56 PST 2020, rishabhb@codeaurora.org wrote:
> On 2019-12-26 21:32, Bjorn Andersson wrote:
> > diff --git a/drivers/remoteproc/qcom_pil_info.c
[..]
> > +static int pil_reloc_probe(struct platform_device *pdev)
> > +{
> > +	struct pil_reloc *reloc;
> > +
> > +	reloc = devm_kzalloc(&pdev->dev, sizeof(*reloc), GFP_KERNEL);
> > +	if (!reloc)
> > +		return -ENOMEM;
> > +
> > +	reloc->dev = &pdev->dev;
> > +	reloc->map = syscon_node_to_regmap(pdev->dev.parent->of_node);
> If there are multiple entries like "pil-reloc" in the imem node
> mapping the entire imem multiple times may not work. Is there a way
> we can somehow just iomap the required region for pil?

With the entire imem being represented as a syscon this will be
ioremapped once and all callers of syscon_node_to_regmap() (or one of
the other syscon getters) will get a regmap back that reference this one
mapping.

So doing it this way allow us to "map" sections of imem that is smaller
than PAGE_SIZE.


That said, it means that all imem users/clients should access imem
through this syscon regmap.

Regards,
Bjorn

Re: [PATCH v2 2/8] remoteproc: qcom: Introduce driver to store pil info in IMEM

[rishabhb]

On 2020-01-22 15:08, Bjorn Andersson wrote:
> On Wed 22 Jan 14:56 PST 2020, rishabhb@codeaurora.org wrote:
>> On 2019-12-26 21:32, Bjorn Andersson wrote:
>> > diff --git a/drivers/remoteproc/qcom_pil_info.c
> [..]
>> > +static int pil_reloc_probe(struct platform_device *pdev)
>> > +{
>> > +	struct pil_reloc *reloc;
>> > +
>> > +	reloc = devm_kzalloc(&pdev->dev, sizeof(*reloc), GFP_KERNEL);
>> > +	if (!reloc)
>> > +		return -ENOMEM;
>> > +
>> > +	reloc->dev = &pdev->dev;
>> > +	reloc->map = syscon_node_to_regmap(pdev->dev.parent->of_node);
>> If there are multiple entries like "pil-reloc" in the imem node
>> mapping the entire imem multiple times may not work. Is there a way
>> we can somehow just iomap the required region for pil?
> 
> With the entire imem being represented as a syscon this will be
> ioremapped once and all callers of syscon_node_to_regmap() (or one of
> the other syscon getters) will get a regmap back that reference this 
> one
> mapping.
> 
> So doing it this way allow us to "map" sections of imem that is smaller
> than PAGE_SIZE.
> 
> 
> That said, it means that all imem users/clients should access imem
> through this syscon regmap.
> 
> Regards,
> Bjorn
Yes, the clients are spread around in different drivers currently.
So accessing same regmap is not possible.

Re: [PATCH v2 2/8] remoteproc: qcom: Introduce driver to store pil info in IMEM

[Bjorn Andersson]

On Wed 22 Jan 15:58 PST 2020, rishabhb@codeaurora.org wrote:

> On 2020-01-22 15:08, Bjorn Andersson wrote:
> > On Wed 22 Jan 14:56 PST 2020, rishabhb@codeaurora.org wrote:
> > > On 2019-12-26 21:32, Bjorn Andersson wrote:
> > > > diff --git a/drivers/remoteproc/qcom_pil_info.c
> > [..]
> > > > +static int pil_reloc_probe(struct platform_device *pdev)
> > > > +{
> > > > +	struct pil_reloc *reloc;
> > > > +
> > > > +	reloc = devm_kzalloc(&pdev->dev, sizeof(*reloc), GFP_KERNEL);
> > > > +	if (!reloc)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	reloc->dev = &pdev->dev;
> > > > +	reloc->map = syscon_node_to_regmap(pdev->dev.parent->of_node);
> > > If there are multiple entries like "pil-reloc" in the imem node
> > > mapping the entire imem multiple times may not work. Is there a way
> > > we can somehow just iomap the required region for pil?
> > 
> > With the entire imem being represented as a syscon this will be
> > ioremapped once and all callers of syscon_node_to_regmap() (or one of
> > the other syscon getters) will get a regmap back that reference this one
> > mapping.
> > 
> > So doing it this way allow us to "map" sections of imem that is smaller
> > than PAGE_SIZE.
> > 
> > 
> > That said, it means that all imem users/clients should access imem
> > through this syscon regmap.
> > 
> > Regards,
> > Bjorn
> Yes, the clients are spread around in different drivers currently.
> So accessing same regmap is not possible.

The few examples upstream are children of the imem simple-mfd/syscon and
will thereby naturally request the regmap of the parent syscon.

For driver that doesn't fit this model (I don't find one right now), or
if you have downstream drivers that are designed differently you could
use syscon_regmap_lookup_by_phandle() to acquire the imem regmap from
any device in the system.

Regards,
Bjorn.

Re: [PATCH v2 7/8] remoteproc: qcom: q6v5: Add common panic handler

[Mathieu Poirier]

On Wed, 22 Jan 2020 at 12:40, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Fri 10 Jan 13:28 PST 2020, Mathieu Poirier wrote:
>
> > On Thu, Dec 26, 2019 at 09:32:14PM -0800, Bjorn Andersson wrote:
> > > Add a common panic handler that invokes a stop request and sleep enough
> > > to let the remoteproc flush it's caches etc in order to aid post mortem
> > > debugging.
> > >
> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > ---
> > >
> > > Changes since v1:
> > > - None
> > >
> > >  drivers/remoteproc/qcom_q6v5.c | 19 +++++++++++++++++++
> > >  drivers/remoteproc/qcom_q6v5.h |  1 +
> > >  2 files changed, 20 insertions(+)
> > >
> > > diff --git a/drivers/remoteproc/qcom_q6v5.c b/drivers/remoteproc/qcom_q6v5.c
> > > index cb0f4a0be032..17167c980e02 100644
> > > --- a/drivers/remoteproc/qcom_q6v5.c
> > > +++ b/drivers/remoteproc/qcom_q6v5.c
> > > @@ -6,6 +6,7 @@
> > >   * Copyright (C) 2014 Sony Mobile Communications AB
> > >   * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
> > >   */
> > > +#include <linux/delay.h>
> > >  #include <linux/kernel.h>
> > >  #include <linux/platform_device.h>
> > >  #include <linux/interrupt.h>
> > > @@ -15,6 +16,8 @@
> > >  #include <linux/remoteproc.h>
> > >  #include "qcom_q6v5.h"
> > >
> > > +#define Q6V5_PANIC_DELAY_MS        200
> > > +
> > >  /**
> > >   * qcom_q6v5_prepare() - reinitialize the qcom_q6v5 context before start
> > >   * @q6v5:  reference to qcom_q6v5 context to be reinitialized
> > > @@ -162,6 +165,22 @@ int qcom_q6v5_request_stop(struct qcom_q6v5 *q6v5)
> > >  }
> > >  EXPORT_SYMBOL_GPL(qcom_q6v5_request_stop);
> > >
> > > +/**
> > > + * qcom_q6v5_panic() - panic handler to invoke a stop on the remote
> > > + * @q6v5:  reference to qcom_q6v5 context
> > > + *
> > > + * Set the stop bit and sleep in order to allow the remote processor to flush
> > > + * its caches etc for post mortem debugging.
> > > + */
> > > +void qcom_q6v5_panic(struct qcom_q6v5 *q6v5)
> > > +{
> > > +   qcom_smem_state_update_bits(q6v5->state,
> > > +                               BIT(q6v5->stop_bit), BIT(q6v5->stop_bit));
> > > +
> > > +   mdelay(Q6V5_PANIC_DELAY_MS);
> >
> > I really wonder if the delay should be part of the remoteproc core and
> > configurable via device tree.  Wanting the remote processor to flush its caches
> > is likely something other vendors will want when dealing with a kernel panic.
> > It would be nice to see if other people have an opinion on this topic.  If not
> > then we can keep the delay here and move it to the core if need be.
> >
>
> I gave this some more thought and what we're trying to achieve is to
> signal the remote processors about the panic and then give them time to
> react, but per the proposal (and Qualcomm downstream iirc) we will do
> this for each remote processor, one by one.
>
> So in the typical case of a Qualcomm platform with 4-5 remoteprocs we'll
> end up giving the first one a whole second to react and the last one
> "only" 200ms.
>
> Moving the delay to the core by iterating over rproc_list calling
> panic() and then delaying would be cleaner imo.

I agree.

>
> It might be nice to make this configurable in DT, but I agree that it
> would be nice to hear from others if this would be useful.

I think the delay has to be configurable via DT if we move this to the
core.  The binding can be optional and default to 200ms if not
present.

>
> Regards,
> Bjorn
>
> > Thanks,
> > Mathieu
> >
> > > +}
> > > +EXPORT_SYMBOL_GPL(qcom_q6v5_panic);
> > > +
> > >  /**
> > >   * qcom_q6v5_init() - initializer of the q6v5 common struct
> > >   * @q6v5:  handle to be initialized
> > > diff --git a/drivers/remoteproc/qcom_q6v5.h b/drivers/remoteproc/qcom_q6v5.h
> > > index 7ac92c1e0f49..c37e6fd063e4 100644
> > > --- a/drivers/remoteproc/qcom_q6v5.h
> > > +++ b/drivers/remoteproc/qcom_q6v5.h
> > > @@ -42,5 +42,6 @@ int qcom_q6v5_prepare(struct qcom_q6v5 *q6v5);
> > >  int qcom_q6v5_unprepare(struct qcom_q6v5 *q6v5);
> > >  int qcom_q6v5_request_stop(struct qcom_q6v5 *q6v5);
> > >  int qcom_q6v5_wait_for_start(struct qcom_q6v5 *q6v5, int timeout);
> > > +void qcom_q6v5_panic(struct qcom_q6v5 *q6v5);
> > >
> > >  #endif
> > > --
> > > 2.24.0
> > >

Re: [PATCH v2 7/8] remoteproc: qcom: q6v5: Add common panic handler

[Bjorn Andersson]

On Thu 23 Jan 09:01 PST 2020, Mathieu Poirier wrote:

> On Wed, 22 Jan 2020 at 12:40, Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> >
> > On Fri 10 Jan 13:28 PST 2020, Mathieu Poirier wrote:
> >
> > > On Thu, Dec 26, 2019 at 09:32:14PM -0800, Bjorn Andersson wrote:
> > > > Add a common panic handler that invokes a stop request and sleep enough
> > > > to let the remoteproc flush it's caches etc in order to aid post mortem
> > > > debugging.
> > > >
> > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > > ---
> > > >
> > > > Changes since v1:
> > > > - None
> > > >
> > > >  drivers/remoteproc/qcom_q6v5.c | 19 +++++++++++++++++++
> > > >  drivers/remoteproc/qcom_q6v5.h |  1 +
> > > >  2 files changed, 20 insertions(+)
> > > >
> > > > diff --git a/drivers/remoteproc/qcom_q6v5.c b/drivers/remoteproc/qcom_q6v5.c
> > > > index cb0f4a0be032..17167c980e02 100644
> > > > --- a/drivers/remoteproc/qcom_q6v5.c
> > > > +++ b/drivers/remoteproc/qcom_q6v5.c
> > > > @@ -6,6 +6,7 @@
> > > >   * Copyright (C) 2014 Sony Mobile Communications AB
> > > >   * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
> > > >   */
> > > > +#include <linux/delay.h>
> > > >  #include <linux/kernel.h>
> > > >  #include <linux/platform_device.h>
> > > >  #include <linux/interrupt.h>
> > > > @@ -15,6 +16,8 @@
> > > >  #include <linux/remoteproc.h>
> > > >  #include "qcom_q6v5.h"
> > > >
> > > > +#define Q6V5_PANIC_DELAY_MS        200
> > > > +
> > > >  /**
> > > >   * qcom_q6v5_prepare() - reinitialize the qcom_q6v5 context before start
> > > >   * @q6v5:  reference to qcom_q6v5 context to be reinitialized
> > > > @@ -162,6 +165,22 @@ int qcom_q6v5_request_stop(struct qcom_q6v5 *q6v5)
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(qcom_q6v5_request_stop);
> > > >
> > > > +/**
> > > > + * qcom_q6v5_panic() - panic handler to invoke a stop on the remote
> > > > + * @q6v5:  reference to qcom_q6v5 context
> > > > + *
> > > > + * Set the stop bit and sleep in order to allow the remote processor to flush
> > > > + * its caches etc for post mortem debugging.
> > > > + */
> > > > +void qcom_q6v5_panic(struct qcom_q6v5 *q6v5)
> > > > +{
> > > > +   qcom_smem_state_update_bits(q6v5->state,
> > > > +                               BIT(q6v5->stop_bit), BIT(q6v5->stop_bit));
> > > > +
> > > > +   mdelay(Q6V5_PANIC_DELAY_MS);
> > >
> > > I really wonder if the delay should be part of the remoteproc core and
> > > configurable via device tree.  Wanting the remote processor to flush its caches
> > > is likely something other vendors will want when dealing with a kernel panic.
> > > It would be nice to see if other people have an opinion on this topic.  If not
> > > then we can keep the delay here and move it to the core if need be.
> > >
> >
> > I gave this some more thought and what we're trying to achieve is to
> > signal the remote processors about the panic and then give them time to
> > react, but per the proposal (and Qualcomm downstream iirc) we will do
> > this for each remote processor, one by one.
> >
> > So in the typical case of a Qualcomm platform with 4-5 remoteprocs we'll
> > end up giving the first one a whole second to react and the last one
> > "only" 200ms.
> >
> > Moving the delay to the core by iterating over rproc_list calling
> > panic() and then delaying would be cleaner imo.
> 
> I agree.
> 
> >
> > It might be nice to make this configurable in DT, but I agree that it
> > would be nice to hear from others if this would be useful.
> 
> I think the delay has to be configurable via DT if we move this to the
> core.  The binding can be optional and default to 200ms if not
> present.
> 

How about I make the panic() return the required delay and then we let
the core sleep for MAX() of the returned durations? That way the default
is still a property of the remoteproc drivers - and 200ms seems rather
arbitrary to put in the core, even as a default.

Regards,
Bjorn

Re: [PATCH v2 7/8] remoteproc: qcom: q6v5: Add common panic handler

[Arnaud POULIQUEN]

Hi Bjorn, Mathieu

On 1/23/20 6:15 PM, Bjorn Andersson wrote:
> On Thu 23 Jan 09:01 PST 2020, Mathieu Poirier wrote:
> 
>> On Wed, 22 Jan 2020 at 12:40, Bjorn Andersson
>> <bjorn.andersson@linaro.org> wrote:
>>>
>>> On Fri 10 Jan 13:28 PST 2020, Mathieu Poirier wrote:
>>>
>>>> On Thu, Dec 26, 2019 at 09:32:14PM -0800, Bjorn Andersson wrote:
>>>>> Add a common panic handler that invokes a stop request and sleep enough
>>>>> to let the remoteproc flush it's caches etc in order to aid post mortem
>>>>> debugging.
>>>>>
>>>>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>>>>> ---
>>>>>
>>>>> Changes since v1:
>>>>> - None
>>>>>
>>>>>  drivers/remoteproc/qcom_q6v5.c | 19 +++++++++++++++++++
>>>>>  drivers/remoteproc/qcom_q6v5.h |  1 +
>>>>>  2 files changed, 20 insertions(+)
>>>>>
>>>>> diff --git a/drivers/remoteproc/qcom_q6v5.c b/drivers/remoteproc/qcom_q6v5.c
>>>>> index cb0f4a0be032..17167c980e02 100644
>>>>> --- a/drivers/remoteproc/qcom_q6v5.c
>>>>> +++ b/drivers/remoteproc/qcom_q6v5.c
>>>>> @@ -6,6 +6,7 @@
>>>>>   * Copyright (C) 2014 Sony Mobile Communications AB
>>>>>   * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
>>>>>   */
>>>>> +#include <linux/delay.h>
>>>>>  #include <linux/kernel.h>
>>>>>  #include <linux/platform_device.h>
>>>>>  #include <linux/interrupt.h>
>>>>> @@ -15,6 +16,8 @@
>>>>>  #include <linux/remoteproc.h>
>>>>>  #include "qcom_q6v5.h"
>>>>>
>>>>> +#define Q6V5_PANIC_DELAY_MS        200
>>>>> +
>>>>>  /**
>>>>>   * qcom_q6v5_prepare() - reinitialize the qcom_q6v5 context before start
>>>>>   * @q6v5:  reference to qcom_q6v5 context to be reinitialized
>>>>> @@ -162,6 +165,22 @@ int qcom_q6v5_request_stop(struct qcom_q6v5 *q6v5)
>>>>>  }
>>>>>  EXPORT_SYMBOL_GPL(qcom_q6v5_request_stop);
>>>>>
>>>>> +/**
>>>>> + * qcom_q6v5_panic() - panic handler to invoke a stop on the remote
>>>>> + * @q6v5:  reference to qcom_q6v5 context
>>>>> + *
>>>>> + * Set the stop bit and sleep in order to allow the remote processor to flush
>>>>> + * its caches etc for post mortem debugging.
>>>>> + */
>>>>> +void qcom_q6v5_panic(struct qcom_q6v5 *q6v5)
>>>>> +{
>>>>> +   qcom_smem_state_update_bits(q6v5->state,
>>>>> +                               BIT(q6v5->stop_bit), BIT(q6v5->stop_bit));
>>>>> +
>>>>> +   mdelay(Q6V5_PANIC_DELAY_MS);
>>>>
>>>> I really wonder if the delay should be part of the remoteproc core and
>>>> configurable via device tree.  Wanting the remote processor to flush its caches
>>>> is likely something other vendors will want when dealing with a kernel panic.
>>>> It would be nice to see if other people have an opinion on this topic.  If not
>>>> then we can keep the delay here and move it to the core if need be.
>>>>
>>>
>>> I gave this some more thought and what we're trying to achieve is to
>>> signal the remote processors about the panic and then give them time to
>>> react, but per the proposal (and Qualcomm downstream iirc) we will do
>>> this for each remote processor, one by one.
>>>
>>> So in the typical case of a Qualcomm platform with 4-5 remoteprocs we'll
>>> end up giving the first one a whole second to react and the last one
>>> "only" 200ms.
>>>
>>> Moving the delay to the core by iterating over rproc_list calling
>>> panic() and then delaying would be cleaner imo.
>>
>> I agree.
>>
>>>
>>> It might be nice to make this configurable in DT, but I agree that it
>>> would be nice to hear from others if this would be useful.
>>
>> I think the delay has to be configurable via DT if we move this to the
>> core.  The binding can be optional and default to 200ms if not
>> present.
>>
> 
> How about I make the panic() return the required delay and then we let
> the core sleep for MAX() of the returned durations? That way the default
> is still a property of the remoteproc drivers - and 200ms seems rather
> arbitrary to put in the core, even as a default.

I agree with Bjorn, the delay should be provided by the platform.
But in this case i wonder if it is simpler to just let the platform take care it?
For instance for stm32mp1 the stop corresponds to the reset on the remote processor core. To inform the coprocessor about an imminent shutdown we use a signal relying on a mailbox (cf. stm32_rproc_stop).  
In this case we would need a delay between the signal and the reset, but not after (no cache management).

Regards,
Arnaud
> 
> Regards,
> Bjorn
> 

Re: [PATCH v2 1/8] dt-bindings: remoteproc: Add Qualcomm PIL info binding

[Rob Herring]

On Sat, Jan 4, 2020 at 3:17 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Sat 04 Jan 13:38 PST 2020, Rob Herring wrote:
>
> > On Thu, Dec 26, 2019 at 09:32:08PM -0800, Bjorn Andersson wrote:
> > > Add a devicetree binding for the Qualcomm periperal image loader
> > > relocation info region found in the IMEM.
> > >
> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > ---
> > >
> > > Changes since v1:
> > > - New patch
> > >
> > >  .../bindings/remoteproc/qcom,pil-info.yaml    | 35 +++++++++++++++++++
> > >  1 file changed, 35 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,pil-info.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,pil-info.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,pil-info.yaml
> > > new file mode 100644
> > > index 000000000000..715945c683ed
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/remoteproc/qcom,pil-info.yaml
> > > @@ -0,0 +1,35 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: "http://devicetree.org/schemas/remoteproc/qcom,pil-info.yaml#"
> > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > > +
> > > +title: Qualcomm peripheral image loader relocation info binding
> > > +
> > > +description:
> > > +  This document defines the binding for describing the Qualcomm peripheral
> > > +  image loader relocation memory region, in IMEM, which is used for post mortem
> > > +  debugging of remoteprocs.
> > > +
> > > +maintainers:
> > > +  - Bjorn Andersson <bjorn.andersson@linaro.org>
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: qcom,pil-reloc-info
> > > +
> > > +  offset:
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +    description: Offset in the register map for the memory region
> >
> > Why not use 'reg' instead?
> >
>
> Because we have one prior example of subdevice of "imem", which is
> compatible "syscon-reboot-mode" and that binding uses "offset".

Not that I'm proposing this, but nothing should prevent both from coexisting.

Rob

Re: [PATCH v2 7/8] remoteproc: qcom: q6v5: Add common panic handler

[Mathieu Poirier]

On Thu, 23 Jan 2020 at 10:49, Arnaud POULIQUEN <arnaud.pouliquen@st.com> wrote:
>
> Hi Bjorn, Mathieu
>
> On 1/23/20 6:15 PM, Bjorn Andersson wrote:
> > On Thu 23 Jan 09:01 PST 2020, Mathieu Poirier wrote:
> >
> >> On Wed, 22 Jan 2020 at 12:40, Bjorn Andersson
> >> <bjorn.andersson@linaro.org> wrote:
> >>>
> >>> On Fri 10 Jan 13:28 PST 2020, Mathieu Poirier wrote:
> >>>
> >>>> On Thu, Dec 26, 2019 at 09:32:14PM -0800, Bjorn Andersson wrote:
> >>>>> Add a common panic handler that invokes a stop request and sleep enough
> >>>>> to let the remoteproc flush it's caches etc in order to aid post mortem
> >>>>> debugging.
> >>>>>
> >>>>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> >>>>> ---
> >>>>>
> >>>>> Changes since v1:
> >>>>> - None
> >>>>>
> >>>>>  drivers/remoteproc/qcom_q6v5.c | 19 +++++++++++++++++++
> >>>>>  drivers/remoteproc/qcom_q6v5.h |  1 +
> >>>>>  2 files changed, 20 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/remoteproc/qcom_q6v5.c b/drivers/remoteproc/qcom_q6v5.c
> >>>>> index cb0f4a0be032..17167c980e02 100644
> >>>>> --- a/drivers/remoteproc/qcom_q6v5.c
> >>>>> +++ b/drivers/remoteproc/qcom_q6v5.c
> >>>>> @@ -6,6 +6,7 @@
> >>>>>   * Copyright (C) 2014 Sony Mobile Communications AB
> >>>>>   * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
> >>>>>   */
> >>>>> +#include <linux/delay.h>
> >>>>>  #include <linux/kernel.h>
> >>>>>  #include <linux/platform_device.h>
> >>>>>  #include <linux/interrupt.h>
> >>>>> @@ -15,6 +16,8 @@
> >>>>>  #include <linux/remoteproc.h>
> >>>>>  #include "qcom_q6v5.h"
> >>>>>
> >>>>> +#define Q6V5_PANIC_DELAY_MS        200
> >>>>> +
> >>>>>  /**
> >>>>>   * qcom_q6v5_prepare() - reinitialize the qcom_q6v5 context before start
> >>>>>   * @q6v5:  reference to qcom_q6v5 context to be reinitialized
> >>>>> @@ -162,6 +165,22 @@ int qcom_q6v5_request_stop(struct qcom_q6v5 *q6v5)
> >>>>>  }
> >>>>>  EXPORT_SYMBOL_GPL(qcom_q6v5_request_stop);
> >>>>>
> >>>>> +/**
> >>>>> + * qcom_q6v5_panic() - panic handler to invoke a stop on the remote
> >>>>> + * @q6v5:  reference to qcom_q6v5 context
> >>>>> + *
> >>>>> + * Set the stop bit and sleep in order to allow the remote processor to flush
> >>>>> + * its caches etc for post mortem debugging.
> >>>>> + */
> >>>>> +void qcom_q6v5_panic(struct qcom_q6v5 *q6v5)
> >>>>> +{
> >>>>> +   qcom_smem_state_update_bits(q6v5->state,
> >>>>> +                               BIT(q6v5->stop_bit), BIT(q6v5->stop_bit));
> >>>>> +
> >>>>> +   mdelay(Q6V5_PANIC_DELAY_MS);
> >>>>
> >>>> I really wonder if the delay should be part of the remoteproc core and
> >>>> configurable via device tree.  Wanting the remote processor to flush its caches
> >>>> is likely something other vendors will want when dealing with a kernel panic.
> >>>> It would be nice to see if other people have an opinion on this topic.  If not
> >>>> then we can keep the delay here and move it to the core if need be.
> >>>>
> >>>
> >>> I gave this some more thought and what we're trying to achieve is to
> >>> signal the remote processors about the panic and then give them time to
> >>> react, but per the proposal (and Qualcomm downstream iirc) we will do
> >>> this for each remote processor, one by one.
> >>>
> >>> So in the typical case of a Qualcomm platform with 4-5 remoteprocs we'll
> >>> end up giving the first one a whole second to react and the last one
> >>> "only" 200ms.
> >>>
> >>> Moving the delay to the core by iterating over rproc_list calling
> >>> panic() and then delaying would be cleaner imo.
> >>
> >> I agree.
> >>
> >>>
> >>> It might be nice to make this configurable in DT, but I agree that it
> >>> would be nice to hear from others if this would be useful.
> >>
> >> I think the delay has to be configurable via DT if we move this to the
> >> core.  The binding can be optional and default to 200ms if not
> >> present.
> >>
> >
> > How about I make the panic() return the required delay and then we let
> > the core sleep for MAX() of the returned durations?

I like it.

> That way the default
> > is still a property of the remoteproc drivers - and 200ms seems rather
> > arbitrary to put in the core, even as a default.
>
> I agree with Bjorn, the delay should be provided by the platform.
> But in this case i wonder if it is simpler to just let the platform take care it?

If I understand you correctly, that is what Bjorn's original
implementation was doing and it had drawbacks.

> For instance for stm32mp1 the stop corresponds to the reset on the remote processor core. To inform the coprocessor about an imminent shutdown we use a signal relying on a mailbox (cf. stm32_rproc_stop).
> In this case we would need a delay between the signal and the reset, but not after (no cache management).

Here I believe you are referring to the upper limit of 500ms that is
needed for the mbox_send_message() in stm32_rproc_stop() to complete.
Since that is a blocking call I think it would fit with Bjorn's
proposal above if a value of '0' is returned by rproc->ops->panic().
That would mean no further delays are needed (because the blocking
mbox_send_message() would have done the job already).  Let me know if
I'm in the weeds.

>
> Regards,
> Arnaud
> >
> > Regards,
> > Bjorn
> >

Re: [PATCH v2 7/8] remoteproc: qcom: q6v5: Add common panic handler

[Arnaud POULIQUEN]

On 1/24/20 7:44 PM, Mathieu Poirier wrote:
> On Thu, 23 Jan 2020 at 10:49, Arnaud POULIQUEN <arnaud.pouliquen@st.com> wrote:
>>
>> Hi Bjorn, Mathieu
>>
>> On 1/23/20 6:15 PM, Bjorn Andersson wrote:
>>> On Thu 23 Jan 09:01 PST 2020, Mathieu Poirier wrote:
>>>
>>>> On Wed, 22 Jan 2020 at 12:40, Bjorn Andersson
>>>> <bjorn.andersson@linaro.org> wrote:
>>>>>
>>>>> On Fri 10 Jan 13:28 PST 2020, Mathieu Poirier wrote:
>>>>>
>>>>>> On Thu, Dec 26, 2019 at 09:32:14PM -0800, Bjorn Andersson wrote:
>>>>>>> Add a common panic handler that invokes a stop request and sleep enough
>>>>>>> to let the remoteproc flush it's caches etc in order to aid post mortem
>>>>>>> debugging.
>>>>>>>
>>>>>>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>>>>>>> ---
>>>>>>>
>>>>>>> Changes since v1:
>>>>>>> - None
>>>>>>>
>>>>>>>  drivers/remoteproc/qcom_q6v5.c | 19 +++++++++++++++++++
>>>>>>>  drivers/remoteproc/qcom_q6v5.h |  1 +
>>>>>>>  2 files changed, 20 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/remoteproc/qcom_q6v5.c b/drivers/remoteproc/qcom_q6v5.c
>>>>>>> index cb0f4a0be032..17167c980e02 100644
>>>>>>> --- a/drivers/remoteproc/qcom_q6v5.c
>>>>>>> +++ b/drivers/remoteproc/qcom_q6v5.c
>>>>>>> @@ -6,6 +6,7 @@
>>>>>>>   * Copyright (C) 2014 Sony Mobile Communications AB
>>>>>>>   * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
>>>>>>>   */
>>>>>>> +#include <linux/delay.h>
>>>>>>>  #include <linux/kernel.h>
>>>>>>>  #include <linux/platform_device.h>
>>>>>>>  #include <linux/interrupt.h>
>>>>>>> @@ -15,6 +16,8 @@
>>>>>>>  #include <linux/remoteproc.h>
>>>>>>>  #include "qcom_q6v5.h"
>>>>>>>
>>>>>>> +#define Q6V5_PANIC_DELAY_MS        200
>>>>>>> +
>>>>>>>  /**
>>>>>>>   * qcom_q6v5_prepare() - reinitialize the qcom_q6v5 context before start
>>>>>>>   * @q6v5:  reference to qcom_q6v5 context to be reinitialized
>>>>>>> @@ -162,6 +165,22 @@ int qcom_q6v5_request_stop(struct qcom_q6v5 *q6v5)
>>>>>>>  }
>>>>>>>  EXPORT_SYMBOL_GPL(qcom_q6v5_request_stop);
>>>>>>>
>>>>>>> +/**
>>>>>>> + * qcom_q6v5_panic() - panic handler to invoke a stop on the remote
>>>>>>> + * @q6v5:  reference to qcom_q6v5 context
>>>>>>> + *
>>>>>>> + * Set the stop bit and sleep in order to allow the remote processor to flush
>>>>>>> + * its caches etc for post mortem debugging.
>>>>>>> + */
>>>>>>> +void qcom_q6v5_panic(struct qcom_q6v5 *q6v5)
>>>>>>> +{
>>>>>>> +   qcom_smem_state_update_bits(q6v5->state,
>>>>>>> +                               BIT(q6v5->stop_bit), BIT(q6v5->stop_bit));
>>>>>>> +
>>>>>>> +   mdelay(Q6V5_PANIC_DELAY_MS);
>>>>>>
>>>>>> I really wonder if the delay should be part of the remoteproc core and
>>>>>> configurable via device tree.  Wanting the remote processor to flush its caches
>>>>>> is likely something other vendors will want when dealing with a kernel panic.
>>>>>> It would be nice to see if other people have an opinion on this topic.  If not
>>>>>> then we can keep the delay here and move it to the core if need be.
>>>>>>
>>>>>
>>>>> I gave this some more thought and what we're trying to achieve is to
>>>>> signal the remote processors about the panic and then give them time to
>>>>> react, but per the proposal (and Qualcomm downstream iirc) we will do
>>>>> this for each remote processor, one by one.
>>>>>
>>>>> So in the typical case of a Qualcomm platform with 4-5 remoteprocs we'll
>>>>> end up giving the first one a whole second to react and the last one
>>>>> "only" 200ms.
>>>>>
>>>>> Moving the delay to the core by iterating over rproc_list calling
>>>>> panic() and then delaying would be cleaner imo.
>>>>
>>>> I agree.
>>>>
>>>>>
>>>>> It might be nice to make this configurable in DT, but I agree that it
>>>>> would be nice to hear from others if this would be useful.
>>>>
>>>> I think the delay has to be configurable via DT if we move this to the
>>>> core.  The binding can be optional and default to 200ms if not
>>>> present.
>>>>
>>>
>>> How about I make the panic() return the required delay and then we let
>>> the core sleep for MAX() of the returned durations?
> 
> I like it.
> 
>> That way the default
>>> is still a property of the remoteproc drivers - and 200ms seems rather
>>> arbitrary to put in the core, even as a default.
>>
>> I agree with Bjorn, the delay should be provided by the platform.
>> But in this case i wonder if it is simpler to just let the platform take care it?
> 
> If I understand you correctly, that is what Bjorn's original
> implementation was doing and it had drawbacks.
Yes, 
Please tell me if i missed something, the only drawback seems mentioned is the accumulative delay.
Could you elaborate how to implement the delay in remote proc core for multi rproc instance.
Here is my view:
To optimize the delay it would probably be necessary to compute:
- the delay based on an initial date,
- the delay requested by each rproc instance,
- the delay elapsed in each rproc panic ops.
Feasible but not straight forward... 
So I suppose that you are thinking about a solution based on the store of the max delay that would be applied after last panic() return?
anyway, how do you determine the last rproc instance? seems that a prerequisite would be that the panic ops is mandatory... 

I'm not familiar with panic mechanism, but how panic ops are scheduled in SMP? Does panics ops would be treated in parallel (using msleep instead of mdelay)?
In this case delays could not be cumulative...

> 
>> For instance for stm32mp1 the stop corresponds to the reset on the remote processor core. To inform the coprocessor about an imminent shutdown we use a signal relying on a mailbox (cf. stm32_rproc_stop).
>> In this case we would need a delay between the signal and the reset, but not after (no cache management).
> 
> Here I believe you are referring to the upper limit of 500ms that is
> needed for the mbox_send_message() in stm32_rproc_stop() to complete.
> Since that is a blocking call I think it would fit with Bjorn's
> proposal above if a value of '0' is returned by rproc->ops->panic().
> That would mean no further delays are needed (because the blocking
> mbox_send_message() would have done the job already).  Let me know if
> I'm in the weeds.
Yes you are :), this is what i thought, if delay implemented in core.

Regards,
Arnaud

> 
>>
>> Regards,
>> Arnaud
>>>
>>> Regards,
>>> Bjorn
>>>

Re: [PATCH v2 7/8] remoteproc: qcom: q6v5: Add common panic handler

[Mathieu Poirier]

On Mon, Jan 27, 2020 at 10:46:05AM +0100, Arnaud POULIQUEN wrote:
> 
> 
> On 1/24/20 7:44 PM, Mathieu Poirier wrote:
> > On Thu, 23 Jan 2020 at 10:49, Arnaud POULIQUEN <arnaud.pouliquen@st.com> wrote:
> >>
> >> Hi Bjorn, Mathieu
> >>
> >> On 1/23/20 6:15 PM, Bjorn Andersson wrote:
> >>> On Thu 23 Jan 09:01 PST 2020, Mathieu Poirier wrote:
> >>>
> >>>> On Wed, 22 Jan 2020 at 12:40, Bjorn Andersson
> >>>> <bjorn.andersson@linaro.org> wrote:
> >>>>>
> >>>>> On Fri 10 Jan 13:28 PST 2020, Mathieu Poirier wrote:
> >>>>>
> >>>>>> On Thu, Dec 26, 2019 at 09:32:14PM -0800, Bjorn Andersson wrote:
> >>>>>>> Add a common panic handler that invokes a stop request and sleep enough
> >>>>>>> to let the remoteproc flush it's caches etc in order to aid post mortem
> >>>>>>> debugging.
> >>>>>>>
> >>>>>>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> >>>>>>> ---
> >>>>>>>
> >>>>>>> Changes since v1:
> >>>>>>> - None
> >>>>>>>
> >>>>>>>  drivers/remoteproc/qcom_q6v5.c | 19 +++++++++++++++++++
> >>>>>>>  drivers/remoteproc/qcom_q6v5.h |  1 +
> >>>>>>>  2 files changed, 20 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/drivers/remoteproc/qcom_q6v5.c b/drivers/remoteproc/qcom_q6v5.c
> >>>>>>> index cb0f4a0be032..17167c980e02 100644
> >>>>>>> --- a/drivers/remoteproc/qcom_q6v5.c
> >>>>>>> +++ b/drivers/remoteproc/qcom_q6v5.c
> >>>>>>> @@ -6,6 +6,7 @@
> >>>>>>>   * Copyright (C) 2014 Sony Mobile Communications AB
> >>>>>>>   * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
> >>>>>>>   */
> >>>>>>> +#include <linux/delay.h>
> >>>>>>>  #include <linux/kernel.h>
> >>>>>>>  #include <linux/platform_device.h>
> >>>>>>>  #include <linux/interrupt.h>
> >>>>>>> @@ -15,6 +16,8 @@
> >>>>>>>  #include <linux/remoteproc.h>
> >>>>>>>  #include "qcom_q6v5.h"
> >>>>>>>
> >>>>>>> +#define Q6V5_PANIC_DELAY_MS        200
> >>>>>>> +
> >>>>>>>  /**
> >>>>>>>   * qcom_q6v5_prepare() - reinitialize the qcom_q6v5 context before start
> >>>>>>>   * @q6v5:  reference to qcom_q6v5 context to be reinitialized
> >>>>>>> @@ -162,6 +165,22 @@ int qcom_q6v5_request_stop(struct qcom_q6v5 *q6v5)
> >>>>>>>  }
> >>>>>>>  EXPORT_SYMBOL_GPL(qcom_q6v5_request_stop);
> >>>>>>>
> >>>>>>> +/**
> >>>>>>> + * qcom_q6v5_panic() - panic handler to invoke a stop on the remote
> >>>>>>> + * @q6v5:  reference to qcom_q6v5 context
> >>>>>>> + *
> >>>>>>> + * Set the stop bit and sleep in order to allow the remote processor to flush
> >>>>>>> + * its caches etc for post mortem debugging.
> >>>>>>> + */
> >>>>>>> +void qcom_q6v5_panic(struct qcom_q6v5 *q6v5)
> >>>>>>> +{
> >>>>>>> +   qcom_smem_state_update_bits(q6v5->state,
> >>>>>>> +                               BIT(q6v5->stop_bit), BIT(q6v5->stop_bit));
> >>>>>>> +
> >>>>>>> +   mdelay(Q6V5_PANIC_DELAY_MS);
> >>>>>>
> >>>>>> I really wonder if the delay should be part of the remoteproc core and
> >>>>>> configurable via device tree.  Wanting the remote processor to flush its caches
> >>>>>> is likely something other vendors will want when dealing with a kernel panic.
> >>>>>> It would be nice to see if other people have an opinion on this topic.  If not
> >>>>>> then we can keep the delay here and move it to the core if need be.
> >>>>>>
> >>>>>
> >>>>> I gave this some more thought and what we're trying to achieve is to
> >>>>> signal the remote processors about the panic and then give them time to
> >>>>> react, but per the proposal (and Qualcomm downstream iirc) we will do
> >>>>> this for each remote processor, one by one.
> >>>>>
> >>>>> So in the typical case of a Qualcomm platform with 4-5 remoteprocs we'll
> >>>>> end up giving the first one a whole second to react and the last one
> >>>>> "only" 200ms.
> >>>>>
> >>>>> Moving the delay to the core by iterating over rproc_list calling
> >>>>> panic() and then delaying would be cleaner imo.
> >>>>
> >>>> I agree.
> >>>>
> >>>>>
> >>>>> It might be nice to make this configurable in DT, but I agree that it
> >>>>> would be nice to hear from others if this would be useful.
> >>>>
> >>>> I think the delay has to be configurable via DT if we move this to the
> >>>> core.  The binding can be optional and default to 200ms if not
> >>>> present.
> >>>>
> >>>
> >>> How about I make the panic() return the required delay and then we let
> >>> the core sleep for MAX() of the returned durations?
> > 
> > I like it.
> > 
> >> That way the default
> >>> is still a property of the remoteproc drivers - and 200ms seems rather
> >>> arbitrary to put in the core, even as a default.
> >>
> >> I agree with Bjorn, the delay should be provided by the platform.
> >> But in this case i wonder if it is simpler to just let the platform take care it?
> > 
> > If I understand you correctly, that is what Bjorn's original
> > implementation was doing and it had drawbacks.
> Yes, 
> Please tell me if i missed something, the only drawback seems mentioned is the accumulative delay.

Yes, that is correct.

> Could you elaborate how to implement the delay in remote proc core for multi rproc instance.
> Here is my view:
> To optimize the delay it would probably be necessary to compute:
> - the delay based on an initial date,
> - the delay requested by each rproc instance,
> - the delay elapsed in each rproc panic ops.
> Feasible but not straight forward... 
> So I suppose that you are thinking about a solution based on the store of the max delay that would be applied after last panic() return?

Yes

> anyway, how do you determine the last rproc instance? seems that a prerequisite would be that the panic ops is mandatory... 

Each ->panic() should return the amount of time to way or 0 if no delay is
required.  If an rpoc doesn't implement ->panic() then it is treated as 0.
From there wait for the maximum time that was collected.

It would be possible to do something more complicated like taking timestamps
everytime a ->panic() returns and optimize the time to wait for but that may be
for a future set.  The first implementation could go with an simple heuristic as
detailed above.

> 
> I'm not familiar with panic mechanism, but how panic ops are scheduled in SMP? Does panics ops would be treated in parallel (using msleep instead of mdelay)?
> In this case delays could not be cumulative...

The processor that triggered the panic sequentially runs the notifier registered
with the panic_notifier_list.  Other processors are instructed to take
themselves offline.  As such there won't be multiple ->panic() running
concurrently. 

> 
> > 
> >> For instance for stm32mp1 the stop corresponds to the reset on the remote processor core. To inform the coprocessor about an imminent shutdown we use a signal relying on a mailbox (cf. stm32_rproc_stop).
> >> In this case we would need a delay between the signal and the reset, but not after (no cache management).
> > 
> > Here I believe you are referring to the upper limit of 500ms that is
> > needed for the mbox_send_message() in stm32_rproc_stop() to complete.
> > Since that is a blocking call I think it would fit with Bjorn's
> > proposal above if a value of '0' is returned by rproc->ops->panic().
> > That would mean no further delays are needed (because the blocking
> > mbox_send_message() would have done the job already).  Let me know if
> > I'm in the weeds.
> Yes you are :), this is what i thought, if delay implemented in core.

Not sure I understand your last reply but I _think_ we are saying the same
thing.

> 
> Regards,
> Arnaud
> 
> > 
> >>
> >> Regards,
> >> Arnaud
> >>>
> >>> Regards,
> >>> Bjorn
> >>>

Re: [PATCH v2 7/8] remoteproc: qcom: q6v5: Add common panic handler

[Arnaud POULIQUEN]

On 1/29/20 9:15 PM, Mathieu Poirier wrote:
> On Mon, Jan 27, 2020 at 10:46:05AM +0100, Arnaud POULIQUEN wrote:
>>
>>
>> On 1/24/20 7:44 PM, Mathieu Poirier wrote:
>>> On Thu, 23 Jan 2020 at 10:49, Arnaud POULIQUEN <arnaud.pouliquen@st.com> wrote:
>>>>
>>>> Hi Bjorn, Mathieu
>>>>
>>>> On 1/23/20 6:15 PM, Bjorn Andersson wrote:
>>>>> On Thu 23 Jan 09:01 PST 2020, Mathieu Poirier wrote:
>>>>>
>>>>>> On Wed, 22 Jan 2020 at 12:40, Bjorn Andersson
>>>>>> <bjorn.andersson@linaro.org> wrote:
>>>>>>>
>>>>>>> On Fri 10 Jan 13:28 PST 2020, Mathieu Poirier wrote:
>>>>>>>
>>>>>>>> On Thu, Dec 26, 2019 at 09:32:14PM -0800, Bjorn Andersson wrote:
>>>>>>>>> Add a common panic handler that invokes a stop request and sleep enough
>>>>>>>>> to let the remoteproc flush it's caches etc in order to aid post mortem
>>>>>>>>> debugging.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> Changes since v1:
>>>>>>>>> - None
>>>>>>>>>
>>>>>>>>>  drivers/remoteproc/qcom_q6v5.c | 19 +++++++++++++++++++
>>>>>>>>>  drivers/remoteproc/qcom_q6v5.h |  1 +
>>>>>>>>>  2 files changed, 20 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/remoteproc/qcom_q6v5.c b/drivers/remoteproc/qcom_q6v5.c
>>>>>>>>> index cb0f4a0be032..17167c980e02 100644
>>>>>>>>> --- a/drivers/remoteproc/qcom_q6v5.c
>>>>>>>>> +++ b/drivers/remoteproc/qcom_q6v5.c
>>>>>>>>> @@ -6,6 +6,7 @@
>>>>>>>>>   * Copyright (C) 2014 Sony Mobile Communications AB
>>>>>>>>>   * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
>>>>>>>>>   */
>>>>>>>>> +#include <linux/delay.h>
>>>>>>>>>  #include <linux/kernel.h>
>>>>>>>>>  #include <linux/platform_device.h>
>>>>>>>>>  #include <linux/interrupt.h>
>>>>>>>>> @@ -15,6 +16,8 @@
>>>>>>>>>  #include <linux/remoteproc.h>
>>>>>>>>>  #include "qcom_q6v5.h"
>>>>>>>>>
>>>>>>>>> +#define Q6V5_PANIC_DELAY_MS        200
>>>>>>>>> +
>>>>>>>>>  /**
>>>>>>>>>   * qcom_q6v5_prepare() - reinitialize the qcom_q6v5 context before start
>>>>>>>>>   * @q6v5:  reference to qcom_q6v5 context to be reinitialized
>>>>>>>>> @@ -162,6 +165,22 @@ int qcom_q6v5_request_stop(struct qcom_q6v5 *q6v5)
>>>>>>>>>  }
>>>>>>>>>  EXPORT_SYMBOL_GPL(qcom_q6v5_request_stop);
>>>>>>>>>
>>>>>>>>> +/**
>>>>>>>>> + * qcom_q6v5_panic() - panic handler to invoke a stop on the remote
>>>>>>>>> + * @q6v5:  reference to qcom_q6v5 context
>>>>>>>>> + *
>>>>>>>>> + * Set the stop bit and sleep in order to allow the remote processor to flush
>>>>>>>>> + * its caches etc for post mortem debugging.
>>>>>>>>> + */
>>>>>>>>> +void qcom_q6v5_panic(struct qcom_q6v5 *q6v5)
>>>>>>>>> +{
>>>>>>>>> +   qcom_smem_state_update_bits(q6v5->state,
>>>>>>>>> +                               BIT(q6v5->stop_bit), BIT(q6v5->stop_bit));
>>>>>>>>> +
>>>>>>>>> +   mdelay(Q6V5_PANIC_DELAY_MS);
>>>>>>>>
>>>>>>>> I really wonder if the delay should be part of the remoteproc core and
>>>>>>>> configurable via device tree.  Wanting the remote processor to flush its caches
>>>>>>>> is likely something other vendors will want when dealing with a kernel panic.
>>>>>>>> It would be nice to see if other people have an opinion on this topic.  If not
>>>>>>>> then we can keep the delay here and move it to the core if need be.
>>>>>>>>
>>>>>>>
>>>>>>> I gave this some more thought and what we're trying to achieve is to
>>>>>>> signal the remote processors about the panic and then give them time to
>>>>>>> react, but per the proposal (and Qualcomm downstream iirc) we will do
>>>>>>> this for each remote processor, one by one.
>>>>>>>
>>>>>>> So in the typical case of a Qualcomm platform with 4-5 remoteprocs we'll
>>>>>>> end up giving the first one a whole second to react and the last one
>>>>>>> "only" 200ms.
>>>>>>>
>>>>>>> Moving the delay to the core by iterating over rproc_list calling
>>>>>>> panic() and then delaying would be cleaner imo.
>>>>>>
>>>>>> I agree.
>>>>>>
>>>>>>>
>>>>>>> It might be nice to make this configurable in DT, but I agree that it
>>>>>>> would be nice to hear from others if this would be useful.
>>>>>>
>>>>>> I think the delay has to be configurable via DT if we move this to the
>>>>>> core.  The binding can be optional and default to 200ms if not
>>>>>> present.
>>>>>>
>>>>>
>>>>> How about I make the panic() return the required delay and then we let
>>>>> the core sleep for MAX() of the returned durations?
>>>
>>> I like it.
>>>
>>>> That way the default
>>>>> is still a property of the remoteproc drivers - and 200ms seems rather
>>>>> arbitrary to put in the core, even as a default.
>>>>
>>>> I agree with Bjorn, the delay should be provided by the platform.
>>>> But in this case i wonder if it is simpler to just let the platform take care it?
>>>
>>> If I understand you correctly, that is what Bjorn's original
>>> implementation was doing and it had drawbacks.
>> Yes, 
>> Please tell me if i missed something, the only drawback seems mentioned is the accumulative delay.
> 
> Yes, that is correct.
> 
>> Could you elaborate how to implement the delay in remote proc core for multi rproc instance.
>> Here is my view:
>> To optimize the delay it would probably be necessary to compute:
>> - the delay based on an initial date,
>> - the delay requested by each rproc instance,
>> - the delay elapsed in each rproc panic ops.
>> Feasible but not straight forward... 
>> So I suppose that you are thinking about a solution based on the store of the max delay that would be applied after last panic() return?
> 
> Yes
> 
>> anyway, how do you determine the last rproc instance? seems that a prerequisite would be that the panic ops is mandatory... 
> 
> Each ->panic() should return the amount of time to way or 0 if no delay is
> required.  If an rpoc doesn't implement ->panic() then it is treated as 0.
> From there wait for the maximum time that was collected.
> 
> It would be possible to do something more complicated like taking timestamps
> everytime a ->panic() returns and optimize the time to wait for but that may be
> for a future set.  The first implementation could go with an simple heuristic as
> detailed above.
Seems reasonable. 
A last point. i don't know if the case is realistic so i prefer to mention it:
we can imagine that a rproc platform driver already manages a panic (for instance a video decoder driver that uses a coprocessor).
In this case there is a risk that the rproc is remove during the panic. 
Depending on the panic sequence ordering this could generate a side effect (no delay as last rproc panic ops could be never called).
But seems not too tricky to take it into account in remoteproc core.
> 
>>
>> I'm not familiar with panic mechanism, but how panic ops are scheduled in SMP? Does panics ops would be treated in parallel (using msleep instead of mdelay)?
>> In this case delays could not be cumulative...
> 
> The processor that triggered the panic sequentially runs the notifier registered
> with the panic_notifier_list.  Other processors are instructed to take
> themselves offline.  As such there won't be multiple ->panic() running
> concurrently. 
Thank you for that explanation!

Regards,
Arnaud
> 
>>
>>>
>>>> For instance for stm32mp1 the stop corresponds to the reset on the remote processor core. To inform the coprocessor about an imminent shutdown we use a signal relying on a mailbox (cf. stm32_rproc_stop).
>>>> In this case we would need a delay between the signal and the reset, but not after (no cache management).
>>>
>>> Here I believe you are referring to the upper limit of 500ms that is
>>> needed for the mbox_send_message() in stm32_rproc_stop() to complete.
>>> Since that is a blocking call I think it would fit with Bjorn's
>>> proposal above if a value of '0' is returned by rproc->ops->panic().
>>> That would mean no further delays are needed (because the blocking
>>> mbox_send_message() would have done the job already).  Let me know if
>>> I'm in the weeds.
>> Yes you are :), this is what i thought, if delay implemented in core.
> 
> Not sure I understand your last reply but I _think_ we are saying the same
> thing.
> 
>>
>> Regards,
>> Arnaud
>>
>>>
>>>>
>>>> Regards,
>>>> Arnaud
>>>>>
>>>>> Regards,
>>>>> Bjorn
>>>>>

Re: [PATCH v2 6/8] remoteproc: Introduce "panic" callback in ops

[Arnaud POULIQUEN]

Hi Bjorn,

On 1/10/20 10:23 PM, Mathieu Poirier wrote:
> On Thu, Dec 26, 2019 at 09:32:13PM -0800, Bjorn Andersson wrote:
>> Introduce a "panic" function in the remoteproc ops table, to allow
>> remoteproc instances to perform operations needed in order to aid in
>> post mortem system debugging, such as flushing caches etc, when the
>> kernel panics.
>>
>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>> ---
>>
>> Changes since v1:
>> - None
>>
>>  drivers/remoteproc/remoteproc_core.c | 17 +++++++++++++++++
>>  include/linux/remoteproc.h           |  4 ++++
>>  2 files changed, 21 insertions(+)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index 307df98347ba..779f19d6d8e7 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -1832,6 +1832,17 @@ void rproc_shutdown(struct rproc *rproc)
>>  }
>>  EXPORT_SYMBOL(rproc_shutdown);
>>  
>> +static int rproc_panic_handler(struct notifier_block *nb, unsigned long event,
>> +			       void *ptr)
>> +{
>> +	struct rproc *rproc = container_of(nb, struct rproc, panic_nb);
>> +
>> +	if (rproc->state == RPROC_RUNNING)
>> +		rproc->ops->panic(rproc);
>> +
>> +	return NOTIFY_DONE;
>> +}
>> +
>>  /**
>>   * rproc_get_by_phandle() - find a remote processor by phandle
>>   * @phandle: phandle to the rproc
>> @@ -2057,6 +2068,12 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
>>  		rproc->ops->get_boot_addr = rproc_elf_get_boot_addr;
>>  	}
>>  
>> +	/* Register panic notifier for remoteprocs with "panic" callback */
>> +	if (rproc->ops->panic) {
>> +		rproc->panic_nb.notifier_call = rproc_panic_handler;
>> +		atomic_notifier_chain_register(&panic_notifier_list, &rproc->panic_nb);
> 
> Line over 80 characters.
atomic_notifier_chain_unregister should be added in rproc_del.

Regards,
Arnaud
> 
>> +	}
>> +
>>  	mutex_init(&rproc->lock);
>>  
>>  	idr_init(&rproc->notifyids);
>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> index 16ad66683ad0..7836c528d309 100644
>> --- a/include/linux/remoteproc.h
>> +++ b/include/linux/remoteproc.h
>> @@ -369,6 +369,7 @@ enum rsc_handling_status {
>>   *			expects to find it
>>   * @sanity_check:	sanity check the fw image
>>   * @get_boot_addr:	get boot address to entry point specified in firmware
>> + * @panic:	optional callback to react to system panic
>>   */
>>  struct rproc_ops {
>>  	int (*start)(struct rproc *rproc);
>> @@ -383,6 +384,7 @@ struct rproc_ops {
>>  	int (*load)(struct rproc *rproc, const struct firmware *fw);
>>  	int (*sanity_check)(struct rproc *rproc, const struct firmware *fw);
>>  	u32 (*get_boot_addr)(struct rproc *rproc, const struct firmware *fw);
>> +	void (*panic)(struct rproc *rproc);
>>  };
>>  
>>  /**
>> @@ -481,6 +483,7 @@ struct rproc_dump_segment {
>>   * @auto_boot: flag to indicate if remote processor should be auto-started
>>   * @dump_segments: list of segments in the firmware
>>   * @nb_vdev: number of vdev currently handled by rproc
>> + * @panic_nb: notifier_block for remoteproc's panic handler
>>   */
>>  struct rproc {
>>  	struct list_head node;
>> @@ -514,6 +517,7 @@ struct rproc {
>>  	bool auto_boot;
>>  	struct list_head dump_segments;
>>  	int nb_vdev;
>> +	struct notifier_block panic_nb;
>>  };
>>  
>>  /**
>> -- 
>> 2.24.0
>>