[PATCH v2 0/6] Add basic Minidump kernel driver support

[PATCH v2 0/6] Add basic Minidump kernel driver support

[Mukesh Ojha]

Minidump is a best effort mechanism to collect useful and predefined data
for first level of debugging on end user devices running on Qualcomm SoCs.
It is built on the premise that System on Chip (SoC) or subsystem part of
SoC crashes, due to a range of hardware and software bugs. Hence, the
ability to collect accurate data is only a best-effort. The data collected
could be invalid or corrupted, data collection itself could fail, and so on.

Qualcomm devices in engineering mode provides a mechanism for generating
full system ramdumps for post mortem debugging. But in some cases it's
however not feasible to capture the entire content of RAM. The minidump
mechanism provides the means for selecting which snippets should be
included in the ramdump.

The core of minidump feature is part of Qualcomm's boot firmware code.
It initializes shared memory (SMEM), which is a part of DDR and
allocates a small section of SMEM to minidump table i.e also called
global table of content (G-ToC). Each subsystem (APSS, ADSP, ...) has
their own table of segments to be included in the minidump and all get
their reference from G-ToC. Each segment/region has some details like
name, physical address and it's size etc. and it could be anywhere
scattered in the DDR.

Existing upstream Qualcomm remoteproc driver[1] already supports minidump
feature for remoteproc instances like ADSP, MODEM, ... where predefined
selective segments of subsystem region can be dumped as part of
coredump collection which generates smaller size artifacts compared to
complete coredump of subsystem on crash.

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/remoteproc/qcom_common.c#n142

In addition to managing and querying the APSS minidump description,
the Linux driver maintains a ELF header in a segment. This segment
gets updated with section/program header whenever a new entry gets
registered.

Patch 1/6 is very trivial change.
Patch 2/6 moves the minidump specific data structure and macro to
 qcom_minidump.h so that (4/6) minidump driver can use.
Patch 3/6 documents qualcomm minidump guide for users.
Patch 4/6 implements qualcomm minidump kernel driver and exports
 symbol which other minidump kernel client can use.
Patch 5/6 enables the qualcomm minidump driver.
Patch 6/6 Use the exported symbol from minidump driver in qcom_common
 for querying minidump descriptor for a subsystem.

Testing of the patches has been done on sm8450 target with the help
of out of tree patch which helps to set the download mode and storage
type and to warm reset the device.

Download mode setting patches are floating here,
https://lore.kernel.org/lkml/1679070482-8391-1-git-send-email-quic_mojha@quicinc.com/

Default storage type is set to via USB, so minidump would be
downloaded with the help of x86_64 machine running PCAT attached
to Qualcomm device which has backed minidump boot firmware
support(more can be found patch 3/6)

Below patch [1] is to warm reset Qualcomm device which has upstream qcom
watchdog driver support.

After applying all patches, we can boot the device and can execute
following command.

echo mini > /sys/module/qcom_scm/parameters/download_mode
echo c > /proc/sysrq-trigger

This will make the device go to download mode and collect the
minidump on to the attached x86 machine running the Qualcomm
PCAT tool.

We will see a bunch of predefined registered region as binary
blobs starts with md_*. A sample client example to dump a linux
region has been given in 3/6.

[1]
--------------------------->8-------------------------------------

commit f1124ccebd47550b4c9627aa162d9cdceba2b76f
Author: Mukesh Ojha <quic_mojha@quicinc.com>
Date:   Thu Mar 16 14:08:35 2023 +0530

    do not merge: watchdog bite on panic

    Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>

diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
index 0d2209c..767e84a 100644
--- a/drivers/watchdog/qcom-wdt.c
+++ b/drivers/watchdog/qcom-wdt.c
@@ -12,6 +12,7 @@
 #include <linux/platform_device.h>
 #include <linux/watchdog.h>
 #include <linux/of_device.h>
+#include <linux/panic.h>

 enum wdt_reg {
        WDT_RST,
@@ -114,12 +115,28 @@ static int qcom_wdt_set_pretimeout(struct watchdog_device *wdd,
        return qcom_wdt_start(wdd);
 }

+static void qcom_wdt_bite_on_panic(struct qcom_wdt *wdt)
+{
+       writel(0, wdt_addr(wdt, WDT_EN));
+       writel(1, wdt_addr(wdt, WDT_BITE_TIME));
+       writel(1, wdt_addr(wdt, WDT_RST));
+       writel(QCOM_WDT_ENABLE, wdt_addr(wdt, WDT_EN));
+
+       wmb();
+
+       while(1)
+               udelay(1);
+}
+
 static int qcom_wdt_restart(struct watchdog_device *wdd, unsigned long action,
                            void *data)
 {
        struct qcom_wdt *wdt = to_qcom_wdt(wdd);
        u32 timeout;

+       if (in_panic)
+               qcom_wdt_bite_on_panic(wdt);
+
        /*
         * Trigger watchdog bite:
         *    Setup BITE_TIME to be 128ms, and enable WDT.
diff --git a/include/linux/panic.h b/include/linux/panic.h
index 979b776..f913629 100644
--- a/include/linux/panic.h
+++ b/include/linux/panic.h
@@ -22,6 +22,7 @@ extern int panic_on_oops;
 extern int panic_on_unrecovered_nmi;
 extern int panic_on_io_nmi;
 extern int panic_on_warn;
+extern bool in_panic;

 extern unsigned long panic_on_taint;
 extern bool panic_on_taint_nousertaint;
diff --git a/kernel/panic.c b/kernel/panic.c
index 487f5b0..714f7f4 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -65,6 +65,8 @@ static unsigned int warn_limit __read_mostly;

 int panic_timeout = CONFIG_PANIC_TIMEOUT;
 EXPORT_SYMBOL_GPL(panic_timeout);
+bool in_panic = false;
+EXPORT_SYMBOL_GPL(in_panic);

 #define PANIC_PRINT_TASK_INFO          0x00000001
 #define PANIC_PRINT_MEM_INFO           0x00000002
@@ -261,6 +263,7 @@ void panic(const char *fmt, ...)
        int old_cpu, this_cpu;
        bool _crash_kexec_post_notifiers = crash_kexec_post_notifiers;

+       in_panic = true;
        if (panic_on_warn) {
                /*
                 * This thread may hit another WARN() in the panic path.
--------------------------------------------------------------------------

Changes in v2:
 - Addressed review comment made by [quic_tsoni/bmasney] to add documentation.
 - Addressed comments made by [srinivas.kandagatla]
 - Dropped pstore 6/6 from the last series, till i get conclusion to get pstore
   region in minidump.
 - Fixed issue reported by kernel test robot.


Changes in v1: https://lore.kernel.org/lkml/1676978713-7394-1-git-send-email-quic_mojha@quicinc.com/

Mukesh Ojha (6):
  remoteproc: qcom: Expand MD_* as MINIDUMP_*
  remoteproc: qcom: Move minidump specific data to qcom_minidump.h
  docs: qcom: Add qualcomm minidump guide
  soc: qcom: Add Qualcomm minidump kernel driver
  arm64: defconfig: Enable Qualcomm minidump driver
  remoterproc: qcom: refactor to leverage exported minidump symbol

 Documentation/admin-guide/qcom_minidump.rst | 240 +++++++++++++
 arch/arm64/configs/defconfig                |   1 +
 drivers/remoteproc/qcom_common.c            |  75 +---
 drivers/soc/qcom/Kconfig                    |  14 +
 drivers/soc/qcom/Makefile                   |   1 +
 drivers/soc/qcom/qcom_minidump.c            | 537 ++++++++++++++++++++++++++++
 include/soc/qcom/minidump.h                 |  40 +++
 include/soc/qcom/qcom_minidump.h            |  88 +++++
 8 files changed, 927 insertions(+), 69 deletions(-)
 create mode 100644 Documentation/admin-guide/qcom_minidump.rst
 create mode 100644 drivers/soc/qcom/qcom_minidump.c
 create mode 100644 include/soc/qcom/minidump.h
 create mode 100644 include/soc/qcom/qcom_minidump.h

-- 
2.7.4

[PATCH v2 1/6] remoteproc: qcom: Expand MD_* as MINIDUMP_*

[Mukesh Ojha]

Expand MD_* as MINIDUMP_* which makes more sense than the
abbreviation.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/remoteproc/qcom_common.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
index a0d4238..805e525 100644
--- a/drivers/remoteproc/qcom_common.c
+++ b/drivers/remoteproc/qcom_common.c
@@ -29,9 +29,9 @@
 #define MAX_NUM_OF_SS           10
 #define MAX_REGION_NAME_LENGTH  16
 #define SBL_MINIDUMP_SMEM_ID	602
-#define MD_REGION_VALID		('V' << 24 | 'A' << 16 | 'L' << 8 | 'I' << 0)
-#define MD_SS_ENCR_DONE		('D' << 24 | 'O' << 16 | 'N' << 8 | 'E' << 0)
-#define MD_SS_ENABLED		('E' << 24 | 'N' << 16 | 'B' << 8 | 'L' << 0)
+#define MINIDUMP_REGION_VALID		('V' << 24 | 'A' << 16 | 'L' << 8 | 'I' << 0)
+#define MINIDUMP_SS_ENCR_DONE		('D' << 24 | 'O' << 16 | 'N' << 8 | 'E' << 0)
+#define MINIDUMP_SS_ENABLED		('E' << 24 | 'N' << 16 | 'B' << 8 | 'L' << 0)
 
 /**
  * struct minidump_region - Minidump region
@@ -125,7 +125,7 @@ static int qcom_add_minidump_segments(struct rproc *rproc, struct minidump_subsy
 
 	for (i = 0; i < seg_cnt; i++) {
 		memcpy_fromio(&region, ptr + i, sizeof(region));
-		if (le32_to_cpu(region.valid) == MD_REGION_VALID) {
+		if (le32_to_cpu(region.valid) == MINIDUMP_REGION_VALID) {
 			name = kstrndup(region.name, MAX_REGION_NAME_LENGTH - 1, GFP_KERNEL);
 			if (!name) {
 				iounmap(ptr);
@@ -168,8 +168,8 @@ void qcom_minidump(struct rproc *rproc, unsigned int minidump_id,
 	 */
 	if (subsystem->regions_baseptr == 0 ||
 	    le32_to_cpu(subsystem->status) != 1 ||
-	    le32_to_cpu(subsystem->enabled) != MD_SS_ENABLED ||
-	    le32_to_cpu(subsystem->encryption_status) != MD_SS_ENCR_DONE) {
+	    le32_to_cpu(subsystem->enabled) != MINIDUMP_SS_ENABLED ||
+	    le32_to_cpu(subsystem->encryption_status) != MINIDUMP_SS_ENCR_DONE) {
 		dev_err(&rproc->dev, "Minidump not ready, skipping\n");
 		return;
 	}
-- 
2.7.4

[PATCH v2 2/6] remoteproc: qcom: Move minidump specific data to qcom_minidump.h

[Mukesh Ojha]

Move minidump specific data types and macros to a separate internal
header(qcom_minidump.h) so that it can be shared among different
Qualcomm drivers.

There is no change in functional behavior after this.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/remoteproc/qcom_common.c | 56 +---------------------------------
 include/soc/qcom/qcom_minidump.h | 66 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+), 55 deletions(-)
 create mode 100644 include/soc/qcom/qcom_minidump.h

diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
index 805e525..88fc984 100644
--- a/drivers/remoteproc/qcom_common.c
+++ b/drivers/remoteproc/qcom_common.c
@@ -18,6 +18,7 @@
 #include <linux/slab.h>
 #include <linux/soc/qcom/mdt_loader.h>
 #include <linux/soc/qcom/smem.h>
+#include <soc/qcom/qcom_minidump.h>
 
 #include "remoteproc_internal.h"
 #include "qcom_common.h"
@@ -26,61 +27,6 @@
 #define to_smd_subdev(d) container_of(d, struct qcom_rproc_subdev, subdev)
 #define to_ssr_subdev(d) container_of(d, struct qcom_rproc_ssr, subdev)
 
-#define MAX_NUM_OF_SS           10
-#define MAX_REGION_NAME_LENGTH  16
-#define SBL_MINIDUMP_SMEM_ID	602
-#define MINIDUMP_REGION_VALID		('V' << 24 | 'A' << 16 | 'L' << 8 | 'I' << 0)
-#define MINIDUMP_SS_ENCR_DONE		('D' << 24 | 'O' << 16 | 'N' << 8 | 'E' << 0)
-#define MINIDUMP_SS_ENABLED		('E' << 24 | 'N' << 16 | 'B' << 8 | 'L' << 0)
-
-/**
- * struct minidump_region - Minidump region
- * @name		: Name of the region to be dumped
- * @seq_num:		: Use to differentiate regions with same name.
- * @valid		: This entry to be dumped (if set to 1)
- * @address		: Physical address of region to be dumped
- * @size		: Size of the region
- */
-struct minidump_region {
-	char	name[MAX_REGION_NAME_LENGTH];
-	__le32	seq_num;
-	__le32	valid;
-	__le64	address;
-	__le64	size;
-};
-
-/**
- * struct minidump_subsystem - Subsystem's SMEM Table of content
- * @status : Subsystem toc init status
- * @enabled : if set to 1, this region would be copied during coredump
- * @encryption_status: Encryption status for this subsystem
- * @encryption_required : Decides to encrypt the subsystem regions or not
- * @region_count : Number of regions added in this subsystem toc
- * @regions_baseptr : regions base pointer of the subsystem
- */
-struct minidump_subsystem {
-	__le32	status;
-	__le32	enabled;
-	__le32	encryption_status;
-	__le32	encryption_required;
-	__le32	region_count;
-	__le64	regions_baseptr;
-};
-
-/**
- * struct minidump_global_toc - Global Table of Content
- * @status : Global Minidump init status
- * @md_revision : Minidump revision
- * @enabled : Minidump enable status
- * @subsystems : Array of subsystems toc
- */
-struct minidump_global_toc {
-	__le32				status;
-	__le32				md_revision;
-	__le32				enabled;
-	struct minidump_subsystem	subsystems[MAX_NUM_OF_SS];
-};
-
 struct qcom_ssr_subsystem {
 	const char *name;
 	struct srcu_notifier_head notifier_list;
diff --git a/include/soc/qcom/qcom_minidump.h b/include/soc/qcom/qcom_minidump.h
new file mode 100644
index 0000000..84c8605
--- /dev/null
+++ b/include/soc/qcom/qcom_minidump.h
@@ -0,0 +1,66 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Qualcomm minidump shared data structures and macros
+ *
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#ifndef _QCOM_MINIDUMP_H_
+#define _QCOM_MINIDUMP_H_
+
+#define MAX_NUM_OF_SS           10
+#define MAX_REGION_NAME_LENGTH  16
+#define SBL_MINIDUMP_SMEM_ID	602
+#define MINIDUMP_REGION_VALID		('V' << 24 | 'A' << 16 | 'L' << 8 | 'I' << 0)
+#define MINIDUMP_SS_ENCR_DONE		('D' << 24 | 'O' << 16 | 'N' << 8 | 'E' << 0)
+#define MINIDUMP_SS_ENABLED		('E' << 24 | 'N' << 16 | 'B' << 8 | 'L' << 0)
+
+/**
+ * struct minidump_region - Minidump region
+ * @name		: Name of the region to be dumped
+ * @seq_num:		: Use to differentiate regions with same name.
+ * @valid		: This entry to be dumped (if set to 1)
+ * @address		: Physical address of region to be dumped
+ * @size		: Size of the region
+ */
+struct minidump_region {
+	char	name[MAX_REGION_NAME_LENGTH];
+	__le32	seq_num;
+	__le32	valid;
+	__le64	address;
+	__le64	size;
+};
+
+/**
+ * struct minidump_subsystem - Subsystem's SMEM Table of content
+ * @status : Subsystem toc init status
+ * @enabled : if set to 1, this region would be copied during coredump
+ * @encryption_status: Encryption status for this subsystem
+ * @encryption_required : Decides to encrypt the subsystem regions or not
+ * @region_count : Number of regions added in this subsystem toc
+ * @regions_baseptr : regions base pointer of the subsystem
+ */
+struct minidump_subsystem {
+	__le32	status;
+	__le32	enabled;
+	__le32	encryption_status;
+	__le32	encryption_required;
+	__le32	region_count;
+	__le64	regions_baseptr;
+};
+
+/**
+ * struct minidump_global_toc - Global Table of Content
+ * @status : Global Minidump init status
+ * @md_revision : Minidump revision
+ * @enabled : Minidump enable status
+ * @subsystems : Array of subsystems toc
+ */
+struct minidump_global_toc {
+	__le32				status;
+	__le32				md_revision;
+	__le32				enabled;
+	struct minidump_subsystem	subsystems[MAX_NUM_OF_SS];
+};
+
+#endif  /* _QCOM_MINIDUMP_H_ */
-- 
2.7.4

[PATCH v2 3/6] docs: qcom: Add qualcomm minidump guide

[Mukesh Ojha]

Add the qualcomm minidump guide for the users which
tries to cover the dependency and the way to test
and collect minidump on Qualcomm supported platforms.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 Documentation/admin-guide/qcom_minidump.rst | 240 ++++++++++++++++++++++++++++
 1 file changed, 240 insertions(+)
 create mode 100644 Documentation/admin-guide/qcom_minidump.rst

diff --git a/Documentation/admin-guide/qcom_minidump.rst b/Documentation/admin-guide/qcom_minidump.rst
new file mode 100644
index 0000000..bdff1c9
--- /dev/null
+++ b/Documentation/admin-guide/qcom_minidump.rst
@@ -0,0 +1,240 @@
+Qualcomm Minidump Feature
+=========================
+
+Introduction
+------------
+
+Minidump is a best effort mechanism to collect useful and predefined
+data for first level of debugging on end user devices running on
+Qualcomm SoCs. It is built on the premise that System on Chip (SoC)
+or subsystem part of SoC crashes, due to a range of hardware and
+software bugs. Hence, the ability to collect accurate data is only
+a best-effort. The data collected could be invalid or corrupted, data
+collection itself could fail, and so on.
+
+Qualcomm devices in engineering mode provides a mechanism for generating
+full system ramdumps for post mortem debugging. But in some cases it's
+however not feasible to capture the entire content of RAM. The minidump
+mechanism provides the means for selecting region should be included in
+the ramdump.
+
+::
+
+   +-----------------------------------------------+
+   |   DDR                       +-------------+   |
+   |                             |      SS0-ToC|   |
+   | +----------------+     +----------------+ |   |
+   | |Shared memory   |     |         SS1-ToC| |   |
+   | |(SMEM)          |     |                | |   |
+   | |                | +-->|--------+       | |   |
+   | |G-ToC           | |   | SS-ToC  \      | |   |
+   | |+-------------+ | |   | +-----------+  | |   |
+   | ||-------------| | |   | |-----------|  | |   |
+   | || SS0-ToC     | | | +-|<|SS1 region1|  | |   |
+   | ||-------------| | | | | |-----------|  | |   |
+   | || SS1-ToC     |-|>+ | | |SS1 region2|  | |   |
+   | ||-------------| |   | | |-----------|  | |   |
+   | || SS2-ToC     | |   | | |  ...      |  | |   |
+   | ||-------------| |   | | |-----------|  | |   |
+   | ||  ...        | |   |-|<|SS1 regionN|  | |   |
+   | ||-------------| |   | | |-----------|  | |   |
+   | || SSn-ToC     | |   | | +-----------+  | |   |
+   | |+-------------+ |   | |                | |   |
+   | |                |   | |----------------| |   |
+   | |                |   +>|  regionN       | |   |
+   | |                |   | |----------------| |   |
+   | +----------------+   | |                | |   |
+   |                      | |----------------| |   |
+   |                      +>|  region1       | |   |
+   |                        |----------------| |   |
+   |                        |                | |   |
+   |                        |----------------|-+   |
+   |                        |  region5       |     |
+   |                        |----------------|     |
+   |                        |                |     |
+   |  Region information    +----------------+     |
+   | +---------------+                             |
+   | |region name    |                             |
+   | |---------------|                             |
+   | |region address |                             |
+   | |---------------|                             |
+   | |region size    |                             |
+   | +---------------+                             |
+   +-----------------------------------------------+
+       G-ToC: Global table of content
+       SS-ToC: Subsystem table of content
+       SS0-SSn: Subsystem numbered from 0 to n
+
+The core of minidump feature is part of Qualcomm's boot firmware code.
+It initializes shared memory(SMEM), which is a part of DDR and
+allocates a small section of it to minidump table i.e also called
+global table of content (G-ToC). Each subsystem (APSS, ADSP, ...) has
+their own table of segments to be included in the minidump, all
+references from a descriptor in SMEM (G-ToC). Each segment/region has
+some details like name, physical address and it's size etc. and it
+could be anywhere scattered in the DDR.
+
+Minidump kernel driver concept
+------------------------------
+
+Qualcomm minidump kernel driver adds the capability to add linux region
+to be dumped as part of ram dump collection. It provides appropriate
+symbol to check its enablement and register client region.
+
+To simplify post mortem debugging, it creates and maintain an ELF
+header as first region that gets updated each time a new region gets
+registered.
+
+The solution supports extracting the ramdump/minidump produced either
+over USB or stored to an attached storage device.
+
+Dependency of minidump kernel driver
+------------------------------------
+
+It is noted that minidump kernel driver depends on Qualcomm's
+shared memory (SMEM) driver and its device tree entry (device
+presence). If above support is present and Qualcomm boot firmware
+has minidump support, so it is possible to dump linux region as
+part of minidump collection.
+
+How a kernel client driver can register region with minidump
+------------------------------------------------------------
+
+Client driver can use qcom_minidump_ready() symbol to check if the
+minidump driver is supported on the target and if it return -ENODEV
+then it means minidump is not supported on this target and the client
+should not try to register their minidump region further and
+continue with their normal execution, and if it is supported it
+can go ahead register its region with qcom_minidump_region_register().
+
+Client need to fill their region by filling qcom_minidump_region
+structure object which consist of the region name, region's
+virtual and physical address and its size.
+
+Below is one sample client driver snippet which try to allocate
+a region from kernel heap of certain size and it writes a certain
+known pattern (that can help in verification after collection
+that we got the exact pattern, what we wrote) and registers with
+minidump.
+
+ .. code-block:: c
+
+  #include <soc/qcom/minidump.h>
+  [...]
+
+
+  [... inside a function ...]
+  struct qcom_minidump_region region;
+
+  [...]
+
+  client_mem_region = kzalloc(region_size, GFP_KERNEL);
+  if (!client_mem_region)
+	return -ENOMEM;
+
+  [... Just write a pattern ...]
+  memset(client_mem_region, 0xAB, region_size);
+
+  [... Check if minidump is ready return -EPROBE_DEFER
+   if this is a probe function otherwise return accordingly ...]
+  err = qcom_minidump_ready();
+  if (err && err != -ENODEV)
+	return -EPROBE_DEFER;
+
+  strlcpy(region.name, "REGION_A", sizeof(region.name));
+  region.virt_addr = client_mem_region;
+  region.phys_addr = virt_to_phys(client_mem_region);
+  region.size = region_size;
+
+  if (!err) {
+	ret = qcom_minidump_region_register(&region);
+	if (ret < 0) {
+		pr_err("failed to add region in minidump: err: %d\n", ret);
+		return ret;
+	}
+  }
+
+  [...]
+
+
+Test
+----
+
+Existing Qualcomm devices already supports entire ddr dump (also called
+full dump) via writing appropriate value to Qualcomm's top control and
+status register(tcsr) in driver/firmware/qcom_scm.c .
+
+SCM device Tree bindings required to support download mode
+For example (sm8450) ::
+
+	/ {
+
+	[...]
+
+		firmware {
+			scm: scm {
+				compatible = "qcom,scm-sm8450", "qcom,scm";
+				[... tcsr register ... ]
+				qcom,dload-mode = <&tcsr 0x13000>;
+
+				[...]
+			};
+		};
+
+	[...]
+
+		soc: soc@0 {
+
+			[...]
+
+			tcsr: syscon@1fc0000 {
+				compatible = "qcom,sm8450-tcsr", "syscon";
+				reg = <0x0 0x1fc0000 0x0 0x30000>;
+			};
+
+			[...]
+		};
+	[...]
+
+	};
+
+User of minidump can pass qcom_scm.download_mode="mini" to kernel
+commandline to set the current download mode to minidump.
+
+Similarly, "full" is passed to set the download mode to full dump
+where entire ddr dump will be collected while setting it "both"
+will set the mode to both (mini + full).
+
+sysfs can also be used to set the mode to minidump.
+
+::
+	echo "mini" > /sys/module/qcom_scm/parameter/download_mode
+
+Once the download mode is set, any kind of crash will make the device collect
+respective dump as per set download mode.
+
+Dump collection
+---------------
+
+The solution supports extracting the minidump produced either over USB or
+stored to an attached storage device.
+
+By default, dumps are downloaded via USB to the attached x86_64 machine
+running PCAT (Qualcomm tool) software. Upon download, we will see
+a set of binary blobs starts with name md_* in PCAT configured directory
+in x86_64 machine, so for above example from the client it will be
+md_REGION_A.BIN. This binary blob depends on region content to determine
+whether it needs external parser support to get the content of the region,
+so for simple plain ASCII text we don't need any parsing and the content
+can be seen just opening the binary file.
+
+To collect the dump to attached storage type, one need to write appropriate
+value to IMEM register, in that case dumps are collected in rawdump
+partition on the target device itself.
+
+One need to read the entire rawdump partition and pull out content to
+save it onto the attached x86_64 machine over USB. Later, this rawdump
+can be pass it to another tool dexter.exe(Qualcomm tool) which converts
+this into the similar binary blobs which we have got it when download type
+was set to USB i.e a set of registered region as blobs and their name
+starts with md_*.
-- 
2.7.4

[PATCH v2 4/6] soc: qcom: Add Qualcomm minidump kernel driver

[Mukesh Ojha]

Minidump is a best effort mechanism to collect useful and predefined
data for first level of debugging on end user devices running on
Qualcomm SoCs. It is built on the premise that System on Chip (SoC)
or subsystem part of SoC crashes, due to a range of hardware and
software bugs. Hence, the ability to collect accurate data is only
a best-effort. The data collected could be invalid or corrupted,
data collection itself could fail, and so on.

Qualcomm devices in engineering mode provides a mechanism for
generating full system ramdumps for post mortem debugging. But in some
cases it's however not feasible to capture the entire content of RAM.
The minidump mechanism provides the means for selecting region should
be included in the ramdump. The solution supports extracting the
ramdump/minidump produced either over USB or stored to an attached
storage device.

The core of minidump feature is part of Qualcomm's boot firmware code.
It initializes shared memory(SMEM), which is a part of DDR and
allocates a small section of it to minidump table i.e also called
global table of content (G-ToC). Each subsystem (APSS, ADSP, ...) has
their own table of segments to be included in the minidump, all
references from a descriptor in SMEM (G-ToC). Each segment/region has
some details like name, physical address and it's size etc. and it
could be anywhere scattered in the DDR.

Minidump kernel driver adds the capability to add linux region to be
dumped as part of ram dump collection. It provides appropriate symbol
to check its enablement and register client regions.

To simplify post mortem debugging, it creates and maintain an ELF
header as first region that gets updated upon registration
of a new region.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/soc/qcom/Kconfig         |  14 +
 drivers/soc/qcom/Makefile        |   1 +
 drivers/soc/qcom/qcom_minidump.c | 537 +++++++++++++++++++++++++++++++++++++++
 include/soc/qcom/minidump.h      |  40 +++
 include/soc/qcom/qcom_minidump.h |  24 +-
 5 files changed, 615 insertions(+), 1 deletion(-)
 create mode 100644 drivers/soc/qcom/qcom_minidump.c
 create mode 100644 include/soc/qcom/minidump.h

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index a8f2830..bb0bc32 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -275,4 +275,18 @@ config QCOM_ICC_BWMON
 	  the fixed bandwidth votes from cpufreq (CPU nodes) thus achieve high
 	  memory throughput even with lower CPU frequencies.
 
+config QCOM_MINIDUMP
+	tristate "QCOM Minidump Support"
+	depends on ARCH_QCOM || COMPILE_TEST
+	select QCOM_SMEM
+	help
+	  Enablement of core minidump feature is controlled from boot firmware
+	  side, and this config allow linux to query and manages APPS minidump
+	  table.
+
+	  Client drivers can register their internal data structures and debug
+	  messages as part of the minidump region and when the SoC is crashed,
+	  these selective regions will be dumped instead of the entire DDR.
+	  This saves significant amount of time and/or storage space.
+
 endmenu
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 6e88da8..dc7f0ab 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -32,3 +32,4 @@ obj-$(CONFIG_QCOM_RPMHPD) += rpmhpd.o
 obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
 obj-$(CONFIG_QCOM_KRYO_L2_ACCESSORS) +=	kryo-l2-accessors.o
 obj-$(CONFIG_QCOM_ICC_BWMON)	+= icc-bwmon.o
+obj-$(CONFIG_QCOM_MINIDUMP) += qcom_minidump.o
diff --git a/drivers/soc/qcom/qcom_minidump.c b/drivers/soc/qcom/qcom_minidump.c
new file mode 100644
index 0000000..44770ed
--- /dev/null
+++ b/drivers/soc/qcom/qcom_minidump.c
@@ -0,0 +1,537 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/*
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#define pr_fmt(fmt) "Minidump: " fmt
+
+#include <linux/init.h>
+#include <linux/export.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/elf.h>
+#include <linux/errno.h>
+#include <linux/io.h>
+#include <linux/string.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/printk.h>
+#include <linux/soc/qcom/smem.h>
+#include <soc/qcom/qcom_minidump.h>
+#include <soc/qcom/minidump.h>
+
+/**
+ * DOC: Overview
+ *
+ *   +-----------------------------------------------+
+ *   |   DDR                       +-------------+   |
+ *   |                             |      SS0-ToC|   |
+ *   | +----------------+     +----------------+ |   |
+ *   | |Shared memory   |     |         SS1-ToC| |   |
+ *   | |(SMEM)          |     |                | |   |
+ *   | |                | +-->|--------+       | |   |
+ *   | |G-ToC           | |   | SS-ToC  \      | |   |
+ *   | |+-------------+ | |   | +-----------+  | |   |
+ *   | ||-------------| | |   | |-----------|  | |   |
+ *   | || SS0-ToC     | | | +-|<|SS1 region1|  | |   |
+ *   | ||-------------| | | | | |-----------|  | |   |
+ *   | || SS1-ToC     |-|>+ | | |SS1 region2|  | |   |
+ *   | ||-------------| |   | | |-----------|  | |   |
+ *   | || SS2-ToC     | |   | | |  ...      |  | |   |
+ *   | ||-------------| |   | | |-----------|  | |   |
+ *   | ||  ...        | |   |-|<|SS1 regionN|  | |   |
+ *   | ||-------------| |   | | |-----------|  | |   |
+ *   | || SSn-ToC     | |   | | +-----------+  | |   |
+ *   | |+-------------+ |   | |                | |   |
+ *   | |                |   | |----------------| |   |
+ *   | |                |   +>|  regionN       | |   |
+ *   | |                |   | |----------------| |   |
+ *   | +----------------+   | |                | |   |
+ *   |                      | |----------------| |   |
+ *   |                      +>|  region1       | |   |
+ *   |                        |----------------| |   |
+ *   |                        |                | |   |
+ *   |                        |----------------|-+   |
+ *   |                        |  region5       |     |
+ *   |                        |----------------|     |
+ *   |                        |                |     |
+ *   |  Region information    +----------------+     |
+ *   | +---------------+                             |
+ *   | |region name    |                             |
+ *   | |---------------|                             |
+ *   | |region address |                             |
+ *   | |---------------|                             |
+ *   | |region size    |                             |
+ *   | +---------------+                             |
+ *   +-----------------------------------------------+
+ *       G-ToC: Global table of content
+ *       SS-ToC: Subsystem table of content
+ *       SS0-SSn: Subsystem numbered from 0 to n
+ *
+ * The core of minidump feature is part of Qualcomm's boot firmware code.
+ * It initializes shared memory(SMEM), which is a part of DDR and
+ * allocates a small section of it to minidump table i.e also called
+ * global table of content (G-ToC).
+ *
+ * Each subsystem (APSS, ADSP, ...) has their own table of segments to be
+ * included in the minidump, all references from a descriptor in SMEM
+ * (G-ToC). Each segment/region has some details like name, physical
+ * address and it's size etc. and it could be anywhere scattered
+ * in the DDR.
+ *
+ * Qualcomm SoCs supports extracting the ramdump/minidump produced
+ * either over USB or stored to an attached storage device.
+ */
+
+/**
+ * struct minidump_elfhdr - Minidump table elf header
+ * @ehdr: Elf main header
+ * @shdr: Section header
+ * @phdr: Program header
+ * @elf_offset: Section offset in elf
+ * @strtable_idx: String table current index position
+ */
+struct minidump_elfhdr {
+	struct elfhdr		*ehdr;
+	struct elf_shdr		*shdr;
+	struct elf_phdr		*phdr;
+	size_t			elf_offset;
+	size_t			strtable_idx;
+};
+
+/**
+ * struct minidump - Minidump driver private data
+ * @md_gbl_toc	: Global TOC pointer
+ * @md_ss_toc	: High level OS TOC pointer
+ * @md_regions	: High level OS region base pointer
+ * @elf		: Minidump elf header
+ * @init_done	: Minidump is initialized or not
+ * @ready	: If minidump is ready for the clients.
+ */
+struct minidump {
+	struct minidump_global_toc	*md_gbl_toc;
+	struct minidump_subsystem	*md_ss_toc;
+	struct minidump_region		*md_regions;
+	struct minidump_elfhdr		elf;
+	bool				init_done;
+	bool				ready;
+};
+
+/*
+ * In some of the Old Qualcomm devices, boot firmware statically allocates 300
+ * as total number of supported region (including all co-processors) in
+ * minidump table out of which linux was using 200. In future, this limitation
+ * from boot firmware might get removed by allocating the region dynamically.
+ * So, keep it compatible with older devices, we can the current limit for Linux
+ * to 200.
+ */
+#define MAX_NUM_ENTRIES	  200
+#define MAX_STRTBL_SIZE	  (MAX_NUM_ENTRIES * MAX_REGION_NAME_LENGTH)
+
+static struct minidump minidump;
+static DEFINE_MUTEX(minidump_lock);
+
+static struct elf_shdr *elf_shdr_entry_addr(struct elfhdr *ehdr, int idx)
+{
+	struct elf_shdr *eshdr = (struct elf_shdr *)((size_t)ehdr + ehdr->e_shoff);
+
+	return &eshdr[idx];
+}
+
+static struct elf_phdr *elf_phdr_entry_addr(struct elfhdr *ehdr, int idx)
+{
+	struct elf_phdr *ephdr = (struct elf_phdr *)((size_t)ehdr + ehdr->e_phoff);
+
+	return &ephdr[idx];
+}
+
+static char *elf_str_table_start(struct elfhdr *ehdr)
+{
+	struct elf_shdr *eshdr;
+
+	if (ehdr->e_shstrndx == SHN_UNDEF)
+		return NULL;
+
+	eshdr = elf_shdr_entry_addr(ehdr, ehdr->e_shstrndx);
+	return (char *)ehdr + eshdr->sh_offset;
+}
+
+static unsigned int append_str_to_strtable(const char *name)
+{
+	char *strtab = elf_str_table_start(minidump.elf.ehdr);
+	unsigned int old_idx = minidump.elf.strtable_idx;
+	unsigned int ret;
+
+	if (!strtab || !name)
+		return 0;
+
+	ret = old_idx;
+	old_idx += strscpy((strtab + old_idx), name, MAX_REGION_NAME_LENGTH);
+	minidump.elf.strtable_idx = old_idx + 1;
+	return ret;
+}
+
+static int get_minidump_region_index(const struct qcom_minidump_region *region)
+{
+	struct minidump_region *mdr;
+	unsigned int i;
+	unsigned int count;
+
+	count = le32_to_cpu(minidump.md_ss_toc->region_count);
+	for (i = 0; i < count; i++) {
+		mdr = &minidump.md_regions[i];
+		if (!strcmp(mdr->name, region->name))
+			return i;
+	}
+	return -ENOENT;
+}
+
+/* Update ELF header */
+static void minidump_update_elf_header(const struct qcom_minidump_region *region)
+{
+	struct elfhdr *ehdr = minidump.elf.ehdr;
+	struct elf_shdr *shdr;
+	struct elf_phdr *phdr;
+
+	shdr = elf_shdr_entry_addr(ehdr, ehdr->e_shnum++);
+	phdr = elf_phdr_entry_addr(ehdr, ehdr->e_phnum++);
+
+	shdr->sh_type = SHT_PROGBITS;
+	shdr->sh_name = append_str_to_strtable(region->name);
+	shdr->sh_addr = (elf_addr_t)region->virt_addr;
+	shdr->sh_size = region->size;
+	shdr->sh_flags = SHF_WRITE;
+	shdr->sh_offset = minidump.elf.elf_offset;
+	shdr->sh_entsize = 0;
+
+	phdr->p_type = PT_LOAD;
+	phdr->p_offset = minidump.elf.elf_offset;
+	phdr->p_vaddr = (elf_addr_t)region->virt_addr;
+	phdr->p_paddr = region->phys_addr;
+	phdr->p_filesz = phdr->p_memsz = region->size;
+	phdr->p_flags = PF_R | PF_W;
+	minidump.elf.elf_offset += shdr->sh_size;
+}
+
+/* Add region in subsystem ToC */
+static void minidump_ss_add_region(const struct qcom_minidump_region *region)
+{
+	struct minidump_region *mdr;
+	unsigned int region_cnt = le32_to_cpu(minidump.md_ss_toc->region_count);
+
+	mdr = &minidump.md_regions[region_cnt];
+	strscpy(mdr->name, region->name, sizeof(mdr->name));
+	mdr->address = cpu_to_le64(region->phys_addr);
+	mdr->size = cpu_to_le64(region->size);
+	mdr->valid = cpu_to_le32(MINIDUMP_REGION_VALID);
+	region_cnt++;
+	minidump.md_ss_toc->region_count = cpu_to_le32(region_cnt);
+}
+
+static bool qcom_minidump_valid_region(const struct qcom_minidump_region *region)
+{
+	return region &&
+		strnlen(region->name, MAX_NAME_LENGTH) < MAX_NAME_LENGTH &&
+		region->virt_addr &&
+		region->size &&
+		IS_ALIGNED(region->size, 4);
+}
+
+static int minidump_add_elf_header(void)
+{
+	struct qcom_minidump_region elfregion;
+	struct elfhdr *ehdr;
+	struct elf_shdr *shdr;
+	struct elf_phdr *phdr;
+	unsigned int  elfh_size;
+	unsigned int strtbl_off;
+	unsigned int phdr_off;
+	char *banner;
+	unsigned int banner_len;
+
+	banner_len = strlen(linux_banner);
+	/*
+	 * Header buffer contains:
+	 * ELF header, (MAX_NUM_ENTRIES + 4) of Section and Program ELF headers,
+	 * where, 4 additional entries, one for empty header, one for string table
+	 * one for minidump table and one for linux banner.
+	 *
+	 * Linux banner is stored in minidump to aid post mortem tools to determine
+	 * the kernel version.
+	 */
+	elfh_size = sizeof(*ehdr);
+	elfh_size += MAX_STRTBL_SIZE;
+	elfh_size += banner_len + 1;
+	elfh_size += ((sizeof(*shdr) + sizeof(*phdr)) * (MAX_NUM_ENTRIES + 4));
+	elfh_size = ALIGN(elfh_size, 4);
+
+	minidump.elf.ehdr = kzalloc(elfh_size, GFP_KERNEL);
+	if (!minidump.elf.ehdr)
+		return -ENOMEM;
+
+	/* Register ELF header as first region */
+	strscpy(elfregion.name, "KELF_HEADER", sizeof(elfregion.name));
+	elfregion.virt_addr = minidump.elf.ehdr;
+	elfregion.phys_addr = virt_to_phys(minidump.elf.ehdr);
+	elfregion.size = elfh_size;
+	minidump_ss_add_region(&elfregion);
+
+	ehdr = minidump.elf.ehdr;
+	/* Assign Section/Program headers offset */
+	minidump.elf.shdr = shdr = (struct elf_shdr *)(ehdr + 1);
+	minidump.elf.phdr = phdr = (struct elf_phdr *)(shdr + MAX_NUM_ENTRIES);
+	phdr_off = sizeof(*ehdr) + (sizeof(*shdr) * MAX_NUM_ENTRIES);
+
+	memcpy(ehdr->e_ident, ELFMAG, SELFMAG);
+	ehdr->e_ident[EI_CLASS] = ELF_CLASS;
+	ehdr->e_ident[EI_DATA] = ELF_DATA;
+	ehdr->e_ident[EI_VERSION] = EV_CURRENT;
+	ehdr->e_ident[EI_OSABI] = ELF_OSABI;
+	ehdr->e_type = ET_CORE;
+	ehdr->e_machine  = ELF_ARCH;
+	ehdr->e_version = EV_CURRENT;
+	ehdr->e_ehsize = sizeof(*ehdr);
+	ehdr->e_phoff = phdr_off;
+	ehdr->e_phentsize = sizeof(*phdr);
+	ehdr->e_shoff = sizeof(*ehdr);
+	ehdr->e_shentsize = sizeof(*shdr);
+	ehdr->e_shstrndx = 1;
+
+	minidump.elf.elf_offset = elfh_size;
+
+	/*
+	 * The zeroth index of the section header is reserved and is rarely used.
+	 * Set the section header as null (SHN_UNDEF) and move to the next one.
+	 * 2nd Section is String table.
+	 */
+	minidump.elf.strtable_idx = 1;
+	strtbl_off = sizeof(*ehdr) + ((sizeof(*phdr) + sizeof(*shdr)) * MAX_NUM_ENTRIES);
+	shdr++;
+	shdr->sh_type = SHT_STRTAB;
+	shdr->sh_offset = (elf_addr_t)strtbl_off;
+	shdr->sh_size = MAX_STRTBL_SIZE;
+	shdr->sh_entsize = 0;
+	shdr->sh_flags = 0;
+	shdr->sh_name = append_str_to_strtable("STR_TBL");
+	shdr++;
+
+	/* 3rd Section is for Minidump_table VA, used by parsers */
+	shdr->sh_type = SHT_PROGBITS;
+	shdr->sh_entsize = 0;
+	shdr->sh_flags = 0;
+	shdr->sh_addr = (elf_addr_t)&minidump;
+	shdr->sh_name = append_str_to_strtable("minidump_table");
+	shdr++;
+
+	/* 4th Section is Linux banner */
+	banner = (char *)ehdr + strtbl_off + MAX_STRTBL_SIZE;
+	memcpy(banner, linux_banner, banner_len);
+
+	shdr->sh_type = SHT_PROGBITS;
+	shdr->sh_offset = (elf_addr_t)(strtbl_off + MAX_STRTBL_SIZE);
+	shdr->sh_size = banner_len + 1;
+	shdr->sh_addr = (elf_addr_t)linux_banner;
+	shdr->sh_entsize = 0;
+	shdr->sh_flags = SHF_WRITE;
+	shdr->sh_name = append_str_to_strtable("linux_banner");
+
+	phdr->p_type = PT_LOAD;
+	phdr->p_offset = (elf_addr_t)(strtbl_off + MAX_STRTBL_SIZE);
+	phdr->p_vaddr = (elf_addr_t)linux_banner;
+	phdr->p_paddr = virt_to_phys(linux_banner);
+	phdr->p_filesz = phdr->p_memsz = banner_len + 1;
+	phdr->p_flags = PF_R | PF_W;
+
+	/* Update the program header/section header count added above */
+	ehdr->e_phnum = 1;
+	ehdr->e_shnum = 4;
+
+	return 0;
+}
+
+static int qcom_minidump_init(void)
+{
+	struct minidump_subsystem *mdsstoc;
+
+	mdsstoc = qcom_minidump_subsystem_desc(MINIDUMP_APSS_DESC);
+	if (IS_ERR(mdsstoc))
+		return -EINVAL;
+
+	minidump.md_regions = kcalloc(MAX_NUM_ENTRIES,
+			      sizeof(struct minidump_region), GFP_KERNEL);
+	if (!minidump.md_regions)
+		return -ENOMEM;
+
+	minidump.md_ss_toc = mdsstoc;
+	/* Share memory table update */
+	mdsstoc->regions_baseptr = cpu_to_le64(virt_to_phys(minidump.md_regions));
+	/* Tell bootloader not to encrypt the regions of this subsystem */
+	mdsstoc->encryption_status = cpu_to_le32(MINIDUMP_SS_ENCR_DONE);
+	mdsstoc->encryption_required = cpu_to_le32(MINIDUMP_SS_ENCR_NOTREQ);
+
+	mdsstoc->enabled = cpu_to_le32(MINIDUMP_SS_ENABLED);
+	mdsstoc->status = cpu_to_le32(1);
+	mdsstoc->region_count = cpu_to_le32(0);
+
+	return 0;
+}
+
+/**
+ * qcom_minidump_ready - Check, if minidump is ready for client registration or not.
+ *
+ * Return: zero on success and negative on failure.
+ *
+ * Qualcomm minidump feature is dependent on Qualcomm's shared memory and it is
+ * possible for a arm64 target to not have it's device tree entry present, for
+ * such case, qcom_minidump_ready() returns -ENODEV and client should not further
+ * try to register their region with minidump driver.
+ */
+int qcom_minidump_ready(void)
+{
+	struct device_node *np;
+	void *ptr;
+	int ret = 0;
+
+	mutex_lock(&minidump_lock);
+	if (minidump.ready)
+		goto unlock;
+
+	np = of_find_compatible_node(NULL, NULL, "qcom,smem");
+	if (!np) {
+		ret = -ENODEV;
+		goto unlock;
+	}
+
+	of_node_put(np);
+	if (!of_device_is_available(np)) {
+		ret = -EPROBE_DEFER;
+		goto unlock;
+	}
+
+	ptr = qcom_smem_get(QCOM_SMEM_HOST_ANY, SBL_MINIDUMP_SMEM_ID, NULL);
+	if (IS_ERR(ptr)) {
+		ret = PTR_ERR(ptr);
+		goto unlock;
+	}
+
+	minidump.md_gbl_toc = ptr;
+	minidump.ready = true;
+
+unlock:
+	if (ret < 0)
+		minidump.ready = false;
+	mutex_unlock(&minidump_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(qcom_minidump_ready);
+
+/**
+ * qcom_minidump_subsystem_desc() - Get minidump subsystem descriptor.
+ * @minidump_index: minidump index for a subsystem in minidump table
+ *
+ * Return: minidump subsystem descriptor address on success and error
+ * on failure
+ */
+struct minidump_subsystem *qcom_minidump_subsystem_desc(unsigned int minidump_index)
+{
+	struct minidump_global_toc *mdgtoc;
+	size_t size;
+
+	if (minidump.md_gbl_toc)
+		return &minidump.md_gbl_toc->subsystems[minidump_index];
+
+	mdgtoc = qcom_smem_get(QCOM_SMEM_HOST_ANY, SBL_MINIDUMP_SMEM_ID, &size);
+	if (IS_ERR(mdgtoc)) {
+		pr_err("Unable to find minidump smem item\n");
+		return ERR_CAST(mdgtoc);
+	}
+
+	if (size < sizeof(*mdgtoc) || !mdgtoc->status) {
+		pr_err("Minidump table is not initialized\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	return &mdgtoc->subsystems[minidump_index];
+}
+EXPORT_SYMBOL_GPL(qcom_minidump_subsystem_desc);
+
+/**
+ * qcom_minidump_region_register() - Register a region in Minidump table.
+ * @region: minidump region.
+ *
+ * Client should not call this directly instead first call qcom_minidump_ready()
+ * to check if minidump is ready to do registration if yes, then call this API.
+ *
+ * Return: On success, it returns region index in minidump table and negative
+ * error value on failure.
+ */
+int qcom_minidump_region_register(const struct qcom_minidump_region *region)
+{
+	unsigned int num_region;
+	int ret;
+
+	mutex_lock(&minidump_lock);
+	if (!minidump.ready) {
+		pr_info("minidump is not ready\n");
+		ret = -EINVAL;
+		goto unlock;
+	}
+
+	/* Initialize minidump context on first call */
+	if (!minidump.init_done) {
+		ret = qcom_minidump_init();
+		if (ret)
+			goto unlock;
+
+		/* First entry would be ELF header */
+		ret = minidump_add_elf_header();
+		if (ret) {
+			kfree(minidump.md_regions);
+			goto unlock;
+		}
+
+		minidump.init_done = true;
+	}
+
+	if (!qcom_minidump_valid_region(region)) {
+		ret = -EINVAL;
+		goto unlock;
+	}
+
+	ret = get_minidump_region_index(region);
+	if (ret >= 0) {
+		pr_info("%s region is already registered\n", region->name);
+		ret = -EEXIST;
+		goto unlock;
+	}
+
+	/* Check if there is a room for a new entry */
+	ret = num_region = le32_to_cpu(minidump.md_ss_toc->region_count);
+	if (num_region >= MAX_NUM_ENTRIES) {
+		pr_err("maximum region limit %u reached\n", num_region);
+		ret = -ENOSPC;
+		goto unlock;
+	}
+
+	minidump_ss_add_region(region);
+	minidump_update_elf_header(region);
+unlock:
+	mutex_unlock(&minidump_lock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(qcom_minidump_region_register);
+
+static void __exit qcom_minidump_exit(void)
+{
+	mutex_lock(&minidump_lock);
+	kfree(minidump.elf.ehdr);
+	kfree(minidump.md_regions);
+	mutex_unlock(&minidump_lock);
+}
+module_exit(qcom_minidump_exit);
+
+MODULE_DESCRIPTION("Qualcomm minidump driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/soc/qcom/minidump.h b/include/soc/qcom/minidump.h
new file mode 100644
index 0000000..a50843f
--- /dev/null
+++ b/include/soc/qcom/minidump.h
@@ -0,0 +1,40 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#ifndef _MINIDUMP_H_
+#define _MINIDUMP_H_
+
+#include <linux/types.h>
+
+#define MAX_NAME_LENGTH		12
+
+/**
+ * struct qcom_minidump_region - Minidump region information
+ *
+ * @name:	Entry name, Minidump will dump binary with this name.
+ * @virt_addr:  Virtual address of the entry.
+ * @phys_addr:	Physical address of the entry to dump.
+ * @size:	Number of byte to dump from @address location,
+ *		and it should be 4 byte aligned.
+ */
+struct qcom_minidump_region {
+	char		name[MAX_NAME_LENGTH];
+	void		*virt_addr;
+	phys_addr_t	phys_addr;
+	size_t		size;
+};
+
+#if IS_ENABLED(CONFIG_QCOM_MINIDUMP)
+extern int qcom_minidump_ready(void);
+extern int qcom_minidump_region_register(const struct qcom_minidump_region *entry);
+#else
+static inline int qcom_minidump_ready(void) { return 0; }
+static inline int qcom_minidump_region_register(const struct qcom_minidump_region *entry)
+{
+	/* Return quietly, if minidump is not enabled */
+	return 0;
+}
+#endif
+#endif /* _MINIDUMP_H_ */
diff --git a/include/soc/qcom/qcom_minidump.h b/include/soc/qcom/qcom_minidump.h
index 84c8605..e8630ed 100644
--- a/include/soc/qcom/qcom_minidump.h
+++ b/include/soc/qcom/qcom_minidump.h
@@ -10,11 +10,25 @@
 
 #define MAX_NUM_OF_SS           10
 #define MAX_REGION_NAME_LENGTH  16
+
+#define MINIDUMP_REVISION	1
 #define SBL_MINIDUMP_SMEM_ID	602
+
+/* Application processor minidump descriptor */
+#define MINIDUMP_APSS_DESC	0
+#define SMEM_ENTRY_SIZE		40
+
 #define MINIDUMP_REGION_VALID		('V' << 24 | 'A' << 16 | 'L' << 8 | 'I' << 0)
+#define MINIDUMP_REGION_INVALID		('I' << 24 | 'N' << 16 | 'V' << 8 | 'A' << 0)
+#define MINIDUMP_REGION_INIT		('I' << 24 | 'N' << 16 | 'I' << 8 | 'T' << 0)
+#define MINIDUMP_REGION_NOINIT		0
+
+#define MINIDUMP_SS_ENCR_REQ		(0 << 24 | 'Y' << 16 | 'E' << 8 | 'S' << 0)
+#define MINIDUMP_SS_ENCR_NOTREQ		(0 << 24 | 0 << 16 | 'N' << 8 | 'R' << 0)
+#define MINIDUMP_SS_ENCR_NONE		('N' << 24 | 'O' << 16 | 'N' << 8 | 'E' << 0)
 #define MINIDUMP_SS_ENCR_DONE		('D' << 24 | 'O' << 16 | 'N' << 8 | 'E' << 0)
+#define MINIDUMP_SS_ENCR_START		('S' << 24 | 'T' << 16 | 'R' << 8 | 'T' << 0)
 #define MINIDUMP_SS_ENABLED		('E' << 24 | 'N' << 16 | 'B' << 8 | 'L' << 0)
-
 /**
  * struct minidump_region - Minidump region
  * @name		: Name of the region to be dumped
@@ -63,4 +77,12 @@ struct minidump_global_toc {
 	struct minidump_subsystem	subsystems[MAX_NUM_OF_SS];
 };
 
+#if IS_ENABLED(CONFIG_QCOM_MINIDUMP)
+extern struct minidump_subsystem *qcom_minidump_subsystem_desc(unsigned int minidump_index);
+#else
+static inline struct minidump_subsystem *qcom_minidump_subsystem_desc(unsigned int minidump_index)
+{
+	return NULL;
+}
+#endif
 #endif  /* _QCOM_MINIDUMP_H_ */
-- 
2.7.4

[PATCH v2 5/6] arm64: defconfig: Enable Qualcomm minidump driver

[Mukesh Ojha]

Previous patches add the Qualcomm minidump driver support, so
lets enable minidump config so that it can be used by kernel
clients.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 7790ee4..389198b 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -1215,6 +1215,7 @@ CONFIG_QCOM_STATS=m
 CONFIG_QCOM_WCNSS_CTRL=m
 CONFIG_QCOM_APR=m
 CONFIG_QCOM_ICC_BWMON=m
+CONFIG_QCOM_MINIDUMP=y
 CONFIG_ARCH_R8A77995=y
 CONFIG_ARCH_R8A77990=y
 CONFIG_ARCH_R8A77950=y
-- 
2.7.4

[PATCH v2 6/6] remoterproc: qcom: refactor to leverage exported minidump symbol

[Mukesh Ojha]

qcom_minidump driver provides qcom_minidump_subsystem_desc()
exported API which other driver can use it query subsystem
descriptor. Refactor qcom_minidump() to use this symbol.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/remoteproc/qcom_common.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
index 88fc984..240e9f7 100644
--- a/drivers/remoteproc/qcom_common.c
+++ b/drivers/remoteproc/qcom_common.c
@@ -94,19 +94,10 @@ void qcom_minidump(struct rproc *rproc, unsigned int minidump_id,
 {
 	int ret;
 	struct minidump_subsystem *subsystem;
-	struct minidump_global_toc *toc;
 
-	/* Get Global minidump ToC*/
-	toc = qcom_smem_get(QCOM_SMEM_HOST_ANY, SBL_MINIDUMP_SMEM_ID, NULL);
-
-	/* check if global table pointer exists and init is set */
-	if (IS_ERR(toc) || !toc->status) {
-		dev_err(&rproc->dev, "Minidump TOC not found in SMEM\n");
+	subsystem = qcom_minidump_subsystem_desc(minidump_id);
+	if (IS_ERR(subsystem))
 		return;
-	}
-
-	/* Get subsystem table of contents using the minidump id */
-	subsystem = &toc->subsystems[minidump_id];
 
 	/**
 	 * Collect minidump if SS ToC is valid and segment table
-- 
2.7.4

Re: [PATCH v2 0/6] Add basic Minidump kernel driver support

[Mukesh Ojha]

Gentle ping;

-Mukesh

On 3/22/2023 7:00 PM, Mukesh Ojha wrote:
> Minidump is a best effort mechanism to collect useful and predefined data
> for first level of debugging on end user devices running on Qualcomm SoCs.
> It is built on the premise that System on Chip (SoC) or subsystem part of
> SoC crashes, due to a range of hardware and software bugs. Hence, the
> ability to collect accurate data is only a best-effort. The data collected
> could be invalid or corrupted, data collection itself could fail, and so on.
> 
> Qualcomm devices in engineering mode provides a mechanism for generating
> full system ramdumps for post mortem debugging. But in some cases it's
> however not feasible to capture the entire content of RAM. The minidump
> mechanism provides the means for selecting which snippets should be
> included in the ramdump.
> 
> The core of minidump feature is part of Qualcomm's boot firmware code.
> It initializes shared memory (SMEM), which is a part of DDR and
> allocates a small section of SMEM to minidump table i.e also called
> global table of content (G-ToC). Each subsystem (APSS, ADSP, ...) has
> their own table of segments to be included in the minidump and all get
> their reference from G-ToC. Each segment/region has some details like
> name, physical address and it's size etc. and it could be anywhere
> scattered in the DDR.
> 
> Existing upstream Qualcomm remoteproc driver[1] already supports minidump
> feature for remoteproc instances like ADSP, MODEM, ... where predefined
> selective segments of subsystem region can be dumped as part of
> coredump collection which generates smaller size artifacts compared to
> complete coredump of subsystem on crash.
> 
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/remoteproc/qcom_common.c#n142
> 
> In addition to managing and querying the APSS minidump description,
> the Linux driver maintains a ELF header in a segment. This segment
> gets updated with section/program header whenever a new entry gets
> registered.
> 
> Patch 1/6 is very trivial change.
> Patch 2/6 moves the minidump specific data structure and macro to
>   qcom_minidump.h so that (4/6) minidump driver can use.
> Patch 3/6 documents qualcomm minidump guide for users.
> Patch 4/6 implements qualcomm minidump kernel driver and exports
>   symbol which other minidump kernel client can use.
> Patch 5/6 enables the qualcomm minidump driver.
> Patch 6/6 Use the exported symbol from minidump driver in qcom_common
>   for querying minidump descriptor for a subsystem.
> 
> Testing of the patches has been done on sm8450 target with the help
> of out of tree patch which helps to set the download mode and storage
> type and to warm reset the device.
> 
> Download mode setting patches are floating here,
> https://lore.kernel.org/lkml/1679070482-8391-1-git-send-email-quic_mojha@quicinc.com/
> 
> Default storage type is set to via USB, so minidump would be
> downloaded with the help of x86_64 machine running PCAT attached
> to Qualcomm device which has backed minidump boot firmware
> support(more can be found patch 3/6)
> 
> Below patch [1] is to warm reset Qualcomm device which has upstream qcom
> watchdog driver support.
> 
> After applying all patches, we can boot the device and can execute
> following command.
> 
> echo mini > /sys/module/qcom_scm/parameters/download_mode
> echo c > /proc/sysrq-trigger
> 
> This will make the device go to download mode and collect the
> minidump on to the attached x86 machine running the Qualcomm
> PCAT tool.
> 
> We will see a bunch of predefined registered region as binary
> blobs starts with md_*. A sample client example to dump a linux
> region has been given in 3/6.
> 
> [1]
> --------------------------->8-------------------------------------
> 
> commit f1124ccebd47550b4c9627aa162d9cdceba2b76f
> Author: Mukesh Ojha <quic_mojha@quicinc.com>
> Date:   Thu Mar 16 14:08:35 2023 +0530
> 
>      do not merge: watchdog bite on panic
> 
>      Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> 
> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> index 0d2209c..767e84a 100644
> --- a/drivers/watchdog/qcom-wdt.c
> +++ b/drivers/watchdog/qcom-wdt.c
> @@ -12,6 +12,7 @@
>   #include <linux/platform_device.h>
>   #include <linux/watchdog.h>
>   #include <linux/of_device.h>
> +#include <linux/panic.h>
> 
>   enum wdt_reg {
>          WDT_RST,
> @@ -114,12 +115,28 @@ static int qcom_wdt_set_pretimeout(struct watchdog_device *wdd,
>          return qcom_wdt_start(wdd);
>   }
> 
> +static void qcom_wdt_bite_on_panic(struct qcom_wdt *wdt)
> +{
> +       writel(0, wdt_addr(wdt, WDT_EN));
> +       writel(1, wdt_addr(wdt, WDT_BITE_TIME));
> +       writel(1, wdt_addr(wdt, WDT_RST));
> +       writel(QCOM_WDT_ENABLE, wdt_addr(wdt, WDT_EN));
> +
> +       wmb();
> +
> +       while(1)
> +               udelay(1);
> +}
> +
>   static int qcom_wdt_restart(struct watchdog_device *wdd, unsigned long action,
>                              void *data)
>   {
>          struct qcom_wdt *wdt = to_qcom_wdt(wdd);
>          u32 timeout;
> 
> +       if (in_panic)
> +               qcom_wdt_bite_on_panic(wdt);
> +
>          /*
>           * Trigger watchdog bite:
>           *    Setup BITE_TIME to be 128ms, and enable WDT.
> diff --git a/include/linux/panic.h b/include/linux/panic.h
> index 979b776..f913629 100644
> --- a/include/linux/panic.h
> +++ b/include/linux/panic.h
> @@ -22,6 +22,7 @@ extern int panic_on_oops;
>   extern int panic_on_unrecovered_nmi;
>   extern int panic_on_io_nmi;
>   extern int panic_on_warn;
> +extern bool in_panic;
> 
>   extern unsigned long panic_on_taint;
>   extern bool panic_on_taint_nousertaint;
> diff --git a/kernel/panic.c b/kernel/panic.c
> index 487f5b0..714f7f4 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -65,6 +65,8 @@ static unsigned int warn_limit __read_mostly;
> 
>   int panic_timeout = CONFIG_PANIC_TIMEOUT;
>   EXPORT_SYMBOL_GPL(panic_timeout);
> +bool in_panic = false;
> +EXPORT_SYMBOL_GPL(in_panic);
> 
>   #define PANIC_PRINT_TASK_INFO          0x00000001
>   #define PANIC_PRINT_MEM_INFO           0x00000002
> @@ -261,6 +263,7 @@ void panic(const char *fmt, ...)
>          int old_cpu, this_cpu;
>          bool _crash_kexec_post_notifiers = crash_kexec_post_notifiers;
> 
> +       in_panic = true;
>          if (panic_on_warn) {
>                  /*
>                   * This thread may hit another WARN() in the panic path.
> --------------------------------------------------------------------------
> 
> Changes in v2:
>   - Addressed review comment made by [quic_tsoni/bmasney] to add documentation.
>   - Addressed comments made by [srinivas.kandagatla]
>   - Dropped pstore 6/6 from the last series, till i get conclusion to get pstore
>     region in minidump.
>   - Fixed issue reported by kernel test robot.
> 
> 
> Changes in v1: https://lore.kernel.org/lkml/1676978713-7394-1-git-send-email-quic_mojha@quicinc.com/
> 
> Mukesh Ojha (6):
>    remoteproc: qcom: Expand MD_* as MINIDUMP_*
>    remoteproc: qcom: Move minidump specific data to qcom_minidump.h
>    docs: qcom: Add qualcomm minidump guide
>    soc: qcom: Add Qualcomm minidump kernel driver
>    arm64: defconfig: Enable Qualcomm minidump driver
>    remoterproc: qcom: refactor to leverage exported minidump symbol
> 
>   Documentation/admin-guide/qcom_minidump.rst | 240 +++++++++++++
>   arch/arm64/configs/defconfig                |   1 +
>   drivers/remoteproc/qcom_common.c            |  75 +---
>   drivers/soc/qcom/Kconfig                    |  14 +
>   drivers/soc/qcom/Makefile                   |   1 +
>   drivers/soc/qcom/qcom_minidump.c            | 537 ++++++++++++++++++++++++++++
>   include/soc/qcom/minidump.h                 |  40 +++
>   include/soc/qcom/qcom_minidump.h            |  88 +++++
>   8 files changed, 927 insertions(+), 69 deletions(-)
>   create mode 100644 Documentation/admin-guide/qcom_minidump.rst
>   create mode 100644 drivers/soc/qcom/qcom_minidump.c
>   create mode 100644 include/soc/qcom/minidump.h
>   create mode 100644 include/soc/qcom/qcom_minidump.h
> 

Re: [PATCH v2 4/6] soc: qcom: Add Qualcomm minidump kernel driver

[Srinivas Kandagatla]

On 22/03/2023 13:30, Mukesh Ojha wrote:
> Minidump is a best effort mechanism to collect useful and predefined
> data for first level of debugging on end user devices running on
> Qualcomm SoCs. It is built on the premise that System on Chip (SoC)
> or subsystem part of SoC crashes, due to a range of hardware and
> software bugs. Hence, the ability to collect accurate data is only
> a best-effort. The data collected could be invalid or corrupted,
> data collection itself could fail, and so on.
> 
> Qualcomm devices in engineering mode provides a mechanism for
> generating full system ramdumps for post mortem debugging. But in some
> cases it's however not feasible to capture the entire content of RAM.
> The minidump mechanism provides the means for selecting region should
> be included in the ramdump. The solution supports extracting the
> ramdump/minidump produced either over USB or stored to an attached
> storage device.
> 
> The core of minidump feature is part of Qualcomm's boot firmware code.
> It initializes shared memory(SMEM), which is a part of DDR and
> allocates a small section of it to minidump table i.e also called
> global table of content (G-ToC). Each subsystem (APSS, ADSP, ...) has
> their own table of segments to be included in the minidump, all
> references from a descriptor in SMEM (G-ToC). Each segment/region has
> some details like name, physical address and it's size etc. and it
> could be anywhere scattered in the DDR.
> 
> Minidump kernel driver adds the capability to add linux region to be
> dumped as part of ram dump collection. It provides appropriate symbol
> to check its enablement and register client regions.
> 
> To simplify post mortem debugging, it creates and maintain an ELF
> header as first region that gets updated upon registration
> of a new region.
> 
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
>   drivers/soc/qcom/Kconfig         |  14 +
>   drivers/soc/qcom/Makefile        |   1 +
>   drivers/soc/qcom/qcom_minidump.c | 537 +++++++++++++++++++++++++++++++++++++++
>   include/soc/qcom/minidump.h      |  40 +++
>   include/soc/qcom/qcom_minidump.h |  24 +-
>   5 files changed, 615 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/soc/qcom/qcom_minidump.c
>   create mode 100644 include/soc/qcom/minidump.h
> 
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index a8f2830..bb0bc32 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -275,4 +275,18 @@ config QCOM_ICC_BWMON
>   	  the fixed bandwidth votes from cpufreq (CPU nodes) thus achieve high
>   	  memory throughput even with lower CPU frequencies.
>   
> +config QCOM_MINIDUMP
> +	tristate "QCOM Minidump Support"
> +	depends on ARCH_QCOM || COMPILE_TEST
> +	select QCOM_SMEM
> +	help
> +	  Enablement of core minidump feature is controlled from boot firmware
> +	  side, and this config allow linux to query and manages APPS minidump
> +	  table.
> +
> +	  Client drivers can register their internal data structures and debug
> +	  messages as part of the minidump region and when the SoC is crashed,
> +	  these selective regions will be dumped instead of the entire DDR.
> +	  This saves significant amount of time and/or storage space.

           To compile this driver as a module, choose M here: the
           module will be called qcom_minidump.

> +
>   endmenu
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index 6e88da8..dc7f0ab 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -32,3 +32,4 @@ obj-$(CONFIG_QCOM_RPMHPD) += rpmhpd.o
>   obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
>   obj-$(CONFIG_QCOM_KRYO_L2_ACCESSORS) +=	kryo-l2-accessors.o
>   obj-$(CONFIG_QCOM_ICC_BWMON)	+= icc-bwmon.o
> +obj-$(CONFIG_QCOM_MINIDUMP) += qcom_minidump.o
> diff --git a/drivers/soc/qcom/qcom_minidump.c b/drivers/soc/qcom/qcom_minidump.c
> new file mode 100644
> index 0000000..44770ed
> --- /dev/null
> +++ b/drivers/soc/qcom/qcom_minidump.c
> @@ -0,0 +1,537 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +/*
> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#define pr_fmt(fmt) "Minidump: " fmt
> +
> +#include <linux/init.h>
> +#include <linux/export.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/err.h>
> +#include <linux/elf.h>
> +#include <linux/errno.h>
> +#include <linux/io.h>
> +#include <linux/string.h>
> +#include <linux/slab.h>
> +#include <linux/of.h>
> +#include <linux/printk.h>
> +#include <linux/soc/qcom/smem.h>
> +#include <soc/qcom/qcom_minidump.h>
> +#include <soc/qcom/minidump.h>
> +
 >snip<
> +/**
> + * DOC: Overview
> + *
> + *   +-----------------------------------------------+
> + *   |   DDR                       +-------------+   |
> + *   |                             |      SS0-ToC|   |
> + *   | +----------------+     +----------------+ |   |
> + *   | |Shared memory   |     |         SS1-ToC| |   |
> + *   | |(SMEM)          |     |                | |   |
> + *   | |                | +-->|--------+       | |   |
> + *   | |G-ToC           | |   | SS-ToC  \      | |   |
> + *   | |+-------------+ | |   | +-----------+  | |   |
> + *   | ||-------------| | |   | |-----------|  | |   |
> + *   | || SS0-ToC     | | | +-|<|SS1 region1|  | |   |
> + *   | ||-------------| | | | | |-----------|  | |   |
> + *   | || SS1-ToC     |-|>+ | | |SS1 region2|  | |   |
> + *   | ||-------------| |   | | |-----------|  | |   |
> + *   | || SS2-ToC     | |   | | |  ...      |  | |   |
> + *   | ||-------------| |   | | |-----------|  | |   |
> + *   | ||  ...        | |   |-|<|SS1 regionN|  | |   |
> + *   | ||-------------| |   | | |-----------|  | |   |
> + *   | || SSn-ToC     | |   | | +-----------+  | |   |
> + *   | |+-------------+ |   | |                | |   |
> + *   | |                |   | |----------------| |   |
> + *   | |                |   +>|  regionN       | |   |
> + *   | |                |   | |----------------| |   |
> + *   | +----------------+   | |                | |   |
> + *   |                      | |----------------| |   |
> + *   |                      +>|  region1       | |   |
> + *   |                        |----------------| |   |
> + *   |                        |                | |   |
> + *   |                        |----------------|-+   |
> + *   |                        |  region5       |     |
> + *   |                        |----------------|     |
> + *   |                        |                |     |
> + *   |  Region information    +----------------+     |
> + *   | +---------------+                             |
> + *   | |region name    |                             |
> + *   | |---------------|                             |
> + *   | |region address |                             |
> + *   | |---------------|                             |
> + *   | |region size    |                             |
> + *   | +---------------+                             |
> + *   +-----------------------------------------------+
> + *       G-ToC: Global table of content
> + *       SS-ToC: Subsystem table of content
> + *       SS0-SSn: Subsystem numbered from 0 to n
> + *
> + * The core of minidump feature is part of Qualcomm's boot firmware code.
> + * It initializes shared memory(SMEM), which is a part of DDR and
> + * allocates a small section of it to minidump table i.e also called
> + * global table of content (G-ToC).
> + *
> + * Each subsystem (APSS, ADSP, ...) has their own table of segments to be
> + * included in the minidump, all references from a descriptor in SMEM
> + * (G-ToC). Each segment/region has some details like name, physical
> + * address and it's size etc. and it could be anywhere scattered
> + * in the DDR.
> + *
> + * Qualcomm SoCs supports extracting the ramdump/minidump produced
> + * either over USB or stored to an attached storage device.
> + */
Isn't the above doc already available in the
  Documentation/admin-guide/qcom_minidump.rst, looks redundant.
> +
> +/**
> + * struct minidump_elfhdr - Minidump table elf header
> + * @ehdr: Elf main header
> + * @shdr: Section header
> + * @phdr: Program header
> + * @elf_offset: Section offset in elf
> + * @strtable_idx: String table current index position
> + */
> +struct minidump_elfhdr {
> +	struct elfhdr		*ehdr;
> +	struct elf_shdr		*shdr;
> +	struct elf_phdr		*phdr;
> +	size_t			elf_offset;
> +	size_t			strtable_idx;
> +};
> +
> +/**
> + * struct minidump - Minidump driver private data
> + * @md_gbl_toc	: Global TOC pointer
> + * @md_ss_toc	: High level OS TOC pointer
> + * @md_regions	: High level OS region base pointer
> + * @elf		: Minidump elf header
> + * @init_done	: Minidump is initialized or not
> + * @ready	: If minidump is ready for the clients.
> + */
> +struct minidump {
> +	struct minidump_global_toc	*md_gbl_toc;
> +	struct minidump_subsystem	*md_ss_toc;
> +	struct minidump_region		*md_regions;
> +	struct minidump_elfhdr		elf;
> +	bool				init_done;
> +	bool				ready;
> +};
> +
> +/*
> + * In some of the Old Qualcomm devices, boot firmware statically allocates 300
> + * as total number of supported region (including all co-processors) in
> + * minidump table out of which linux was using 200. In future, this limitation
> + * from boot firmware might get removed by allocating the region dynamically.
> + * So, keep it compatible with older devices, we can the current limit for Linux
> + * to 200.
> + */
> +#define MAX_NUM_ENTRIES	  200
> +#define MAX_STRTBL_SIZE	  (MAX_NUM_ENTRIES * MAX_REGION_NAME_LENGTH)
> +
> +static struct minidump minidump;
> +static DEFINE_MUTEX(minidump_lock);
> +
> +static struct elf_shdr *elf_shdr_entry_addr(struct elfhdr *ehdr, int idx)
> +{
> +	struct elf_shdr *eshdr = (struct elf_shdr *)((size_t)ehdr + ehdr->e_shoff);
> +
> +	return &eshdr[idx];
> +}
> +
> +static struct elf_phdr *elf_phdr_entry_addr(struct elfhdr *ehdr, int idx)
> +{
> +	struct elf_phdr *ephdr = (struct elf_phdr *)((size_t)ehdr + ehdr->e_phoff);
> +
> +	return &ephdr[idx];
> +}
> +
> +static char *elf_str_table_start(struct elfhdr *ehdr)
> +{
> +	struct elf_shdr *eshdr;
> +
> +	if (ehdr->e_shstrndx == SHN_UNDEF)
> +		return NULL;
> +
> +	eshdr = elf_shdr_entry_addr(ehdr, ehdr->e_shstrndx);
> +	return (char *)ehdr + eshdr->sh_offset;
> +}
> +
> +static unsigned int append_str_to_strtable(const char *name)
> +{
> +	char *strtab = elf_str_table_start(minidump.elf.ehdr);
> +	unsigned int old_idx = minidump.elf.strtable_idx;
> +	unsigned int ret;
> +
> +	if (!strtab || !name)
> +		return 0;
> +
> +	ret = old_idx;
> +	old_idx += strscpy((strtab + old_idx), name, MAX_REGION_NAME_LENGTH);
> +	minidump.elf.strtable_idx = old_idx + 1;
> +	return ret;
> +}
> +
> +static int get_minidump_region_index(const struct qcom_minidump_region *region)
> +{
> +	struct minidump_region *mdr;
> +	unsigned int i;
> +	unsigned int count;
> +
> +	count = le32_to_cpu(minidump.md_ss_toc->region_count);
> +	for (i = 0; i < count; i++) {
> +		mdr = &minidump.md_regions[i];
> +		if (!strcmp(mdr->name, region->name))
> +			return i;
> +	}
> +	return -ENOENT;
> +}
> +

> +/* Update ELF header */

TBH, these type of comments are not adding any value.

> +static void minidump_update_elf_header(const struct qcom_minidump_region *region)
> +{
> +	struct elfhdr *ehdr = minidump.elf.ehdr;
> +	struct elf_shdr *shdr;
> +	struct elf_phdr *phdr;
> +
> +	shdr = elf_shdr_entry_addr(ehdr, ehdr->e_shnum++);
> +	phdr = elf_phdr_entry_addr(ehdr, ehdr->e_phnum++);
> +
> +	shdr->sh_type = SHT_PROGBITS;
> +	shdr->sh_name = append_str_to_strtable(region->name);
> +	shdr->sh_addr = (elf_addr_t)region->virt_addr;
> +	shdr->sh_size = region->size;
> +	shdr->sh_flags = SHF_WRITE;
> +	shdr->sh_offset = minidump.elf.elf_offset;
> +	shdr->sh_entsize = 0;
> +
> +	phdr->p_type = PT_LOAD;
> +	phdr->p_offset = minidump.elf.elf_offset;
> +	phdr->p_vaddr = (elf_addr_t)region->virt_addr;
> +	phdr->p_paddr = region->phys_addr;
> +	phdr->p_filesz = phdr->p_memsz = region->size;
> +	phdr->p_flags = PF_R | PF_W;
> +	minidump.elf.elf_offset += shdr->sh_size;
> +}
> +
> +/* Add region in subsystem ToC */
> +static void minidump_ss_add_region(const struct qcom_minidump_region *region)
> +{
> +	struct minidump_region *mdr;
> +	unsigned int region_cnt = le32_to_cpu(minidump.md_ss_toc->region_count);
> +
> +	mdr = &minidump.md_regions[region_cnt];
> +	strscpy(mdr->name, region->name, sizeof(mdr->name));
> +	mdr->address = cpu_to_le64(region->phys_addr);
> +	mdr->size = cpu_to_le64(region->size);
> +	mdr->valid = cpu_to_le32(MINIDUMP_REGION_VALID);
> +	region_cnt++;
> +	minidump.md_ss_toc->region_count = cpu_to_le32(region_cnt);
> +}
> +
> +static bool qcom_minidump_valid_region(const struct qcom_minidump_region *region)
> +{
> +	return region &&
> +		strnlen(region->name, MAX_NAME_LENGTH) < MAX_NAME_LENGTH &&
> +		region->virt_addr &&
> +		region->size &&
> +		IS_ALIGNED(region->size, 4);
> +}
> +
> +static int minidump_add_elf_header(void)
> +{
> +	struct qcom_minidump_region elfregion;
> +	struct elfhdr *ehdr;
> +	struct elf_shdr *shdr;
> +	struct elf_phdr *phdr;
> +	unsigned int  elfh_size;
> +	unsigned int strtbl_off;
> +	unsigned int phdr_off;
> +	char *banner;
> +	unsigned int banner_len;
> +
> +	banner_len = strlen(linux_banner);
> +	/*
> +	 * Header buffer contains:
> +	 * ELF header, (MAX_NUM_ENTRIES + 4) of Section and Program ELF headers,
> +	 * where, 4 additional entries, one for empty header, one for string table
> +	 * one for minidump table and one for linux banner.
> +	 *
> +	 * Linux banner is stored in minidump to aid post mortem tools to determine
> +	 * the kernel version.
> +	 */
> +	elfh_size = sizeof(*ehdr);
> +	elfh_size += MAX_STRTBL_SIZE;
> +	elfh_size += banner_len + 1;
> +	elfh_size += ((sizeof(*shdr) + sizeof(*phdr)) * (MAX_NUM_ENTRIES + 4));
> +	elfh_size = ALIGN(elfh_size, 4);
> +
> +	minidump.elf.ehdr = kzalloc(elfh_size, GFP_KERNEL);
> +	if (!minidump.elf.ehdr)
> +		return -ENOMEM;
> +
> +	/* Register ELF header as first region */
> +	strscpy(elfregion.name, "KELF_HEADER", sizeof(elfregion.name));
> +	elfregion.virt_addr = minidump.elf.ehdr;
> +	elfregion.phys_addr = virt_to_phys(minidump.elf.ehdr);
> +	elfregion.size = elfh_size;
> +	minidump_ss_add_region(&elfregion);
> +
> +	ehdr = minidump.elf.ehdr;
> +	/* Assign Section/Program headers offset */
> +	minidump.elf.shdr = shdr = (struct elf_shdr *)(ehdr + 1);
> +	minidump.elf.phdr = phdr = (struct elf_phdr *)(shdr + MAX_NUM_ENTRIES);
> +	phdr_off = sizeof(*ehdr) + (sizeof(*shdr) * MAX_NUM_ENTRIES);
> +
> +	memcpy(ehdr->e_ident, ELFMAG, SELFMAG);
> +	ehdr->e_ident[EI_CLASS] = ELF_CLASS;
> +	ehdr->e_ident[EI_DATA] = ELF_DATA;
> +	ehdr->e_ident[EI_VERSION] = EV_CURRENT;
> +	ehdr->e_ident[EI_OSABI] = ELF_OSABI;
> +	ehdr->e_type = ET_CORE;
> +	ehdr->e_machine  = ELF_ARCH;
> +	ehdr->e_version = EV_CURRENT;
> +	ehdr->e_ehsize = sizeof(*ehdr);
> +	ehdr->e_phoff = phdr_off;
> +	ehdr->e_phentsize = sizeof(*phdr);
> +	ehdr->e_shoff = sizeof(*ehdr);
> +	ehdr->e_shentsize = sizeof(*shdr);
> +	ehdr->e_shstrndx = 1;
> +
> +	minidump.elf.elf_offset = elfh_size;
> +
> +	/*
> +	 * The zeroth index of the section header is reserved and is rarely used.
> +	 * Set the section header as null (SHN_UNDEF) and move to the next one.
> +	 * 2nd Section is String table.
> +	 */
> +	minidump.elf.strtable_idx = 1;
> +	strtbl_off = sizeof(*ehdr) + ((sizeof(*phdr) + sizeof(*shdr)) * MAX_NUM_ENTRIES);
> +	shdr++;
> +	shdr->sh_type = SHT_STRTAB;
> +	shdr->sh_offset = (elf_addr_t)strtbl_off;
> +	shdr->sh_size = MAX_STRTBL_SIZE;
> +	shdr->sh_entsize = 0;
> +	shdr->sh_flags = 0;
> +	shdr->sh_name = append_str_to_strtable("STR_TBL");
> +	shdr++;
> +
> +	/* 3rd Section is for Minidump_table VA, used by parsers */
> +	shdr->sh_type = SHT_PROGBITS;
> +	shdr->sh_entsize = 0;
> +	shdr->sh_flags = 0;
> +	shdr->sh_addr = (elf_addr_t)&minidump;
> +	shdr->sh_name = append_str_to_strtable("minidump_table");
> +	shdr++;
> +
> +	/* 4th Section is Linux banner */
> +	banner = (char *)ehdr + strtbl_off + MAX_STRTBL_SIZE;
> +	memcpy(banner, linux_banner, banner_len);
> +
> +	shdr->sh_type = SHT_PROGBITS;
> +	shdr->sh_offset = (elf_addr_t)(strtbl_off + MAX_STRTBL_SIZE);
> +	shdr->sh_size = banner_len + 1;
> +	shdr->sh_addr = (elf_addr_t)linux_banner;
> +	shdr->sh_entsize = 0;
> +	shdr->sh_flags = SHF_WRITE;
> +	shdr->sh_name = append_str_to_strtable("linux_banner");
> +
> +	phdr->p_type = PT_LOAD;
> +	phdr->p_offset = (elf_addr_t)(strtbl_off + MAX_STRTBL_SIZE);
> +	phdr->p_vaddr = (elf_addr_t)linux_banner;
> +	phdr->p_paddr = virt_to_phys(linux_banner);
> +	phdr->p_filesz = phdr->p_memsz = banner_len + 1;
> +	phdr->p_flags = PF_R | PF_W;
> +
> +	/* Update the program header/section header count added above */
> +	ehdr->e_phnum = 1;
> +	ehdr->e_shnum = 4;
> +
> +	return 0;
> +}
> +
> +static int qcom_minidump_init(void)
> +{
> +	struct minidump_subsystem *mdsstoc;
> +
> +	mdsstoc = qcom_minidump_subsystem_desc(MINIDUMP_APSS_DESC);
> +	if (IS_ERR(mdsstoc))
> +		return -EINVAL;
> +
> +	minidump.md_regions = kcalloc(MAX_NUM_ENTRIES,
> +			      sizeof(struct minidump_region), GFP_KERNEL);
> +	if (!minidump.md_regions)
> +		return -ENOMEM;
> +
> +	minidump.md_ss_toc = mdsstoc;
> +	/* Share memory table update */
> +	mdsstoc->regions_baseptr = cpu_to_le64(virt_to_phys(minidump.md_regions));
> +	/* Tell bootloader not to encrypt the regions of this subsystem */
> +	mdsstoc->encryption_status = cpu_to_le32(MINIDUMP_SS_ENCR_DONE);
> +	mdsstoc->encryption_required = cpu_to_le32(MINIDUMP_SS_ENCR_NOTREQ);
> +
> +	mdsstoc->enabled = cpu_to_le32(MINIDUMP_SS_ENABLED);
> +	mdsstoc->status = cpu_to_le32(1);
> +	mdsstoc->region_count = cpu_to_le32(0);
> +
> +	return 0;
> +}
> +
> +/**
> + * qcom_minidump_ready - Check, if minidump is ready for client registration or not.
> + *
> + * Return: zero on success and negative on failure.
> + *
> + * Qualcomm minidump feature is dependent on Qualcomm's shared memory and it is
> + * possible for a arm64 target to not have it's device tree entry present, for
> + * such case, qcom_minidump_ready() returns -ENODEV and client should not further
> + * try to register their region with minidump driver.
> + */
> +int qcom_minidump_ready(void)

return type can be bool instead of int.
> +{
> +	struct device_node *np;
> +	void *ptr;
> +	int ret = 0;
> +
> +	mutex_lock(&minidump_lock);
> +	if (minidump.ready)
> +		goto unlock;
> +

<--
> +	np = of_find_compatible_node(NULL, NULL, "qcom,smem");
> +	if (!np) {
> +		ret = -ENODEV;
> +		goto unlock;
> +	}
> +
> +	of_node_put(np);
> +	if (!of_device_is_available(np)) {
> +		ret = -EPROBE_DEFER;
> +		goto unlock;
> +	}
> +
-->

Above snippet looks redundant as qcom_smem_get will already check for 
the above.

> +	ptr = qcom_smem_get(QCOM_SMEM_HOST_ANY, SBL_MINIDUMP_SMEM_ID, NULL);
> +	if (IS_ERR(ptr)) {
> +		ret = PTR_ERR(ptr);
> +		goto unlock;
> +	}
> +
> +	minidump.md_gbl_toc = ptr;
> +	minidump.ready = true;
> +
> +unlock:
> +	if (ret < 0)
> +		minidump.ready = false;
> +	mutex_unlock(&minidump_lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(qcom_minidump_ready);
> +
> +/**
> + * qcom_minidump_subsystem_desc() - Get minidump subsystem descriptor.
> + * @minidump_index: minidump index for a subsystem in minidump table
> + *
> + * Return: minidump subsystem descriptor address on success and error
> + * on failure
> + */
> +struct minidump_subsystem *qcom_minidump_subsystem_desc(unsigned int minidump_index)
> +{
> +	struct minidump_global_toc *mdgtoc;
> +	size_t size;
> +
> +	if (minidump.md_gbl_toc)
> +		return &minidump.md_gbl_toc->subsystems[minidump_index];
> +
Why would we end up here, if the user is supposed to call 
qcom_minidump_ready() before calling this qcom_minidump_subsystem_desc()

A check for minidump.ready should do the job.
I also see locking is missing.

> +	mdgtoc = qcom_smem_get(QCOM_SMEM_HOST_ANY, SBL_MINIDUMP_SMEM_ID, &size);
> +	if (IS_ERR(mdgtoc)) {
> +		pr_err("Unable to find minidump smem item\n");
> +		return ERR_CAST(mdgtoc);
> +	}
> +
> +	if (size < sizeof(*mdgtoc) || !mdgtoc->status) {
> +		pr_err("Minidump table is not initialized\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	return &mdgtoc->subsystems[minidump_index];
> +}
> +EXPORT_SYMBOL_GPL(qcom_minidump_subsystem_desc);
> +
> +/**
> + * qcom_minidump_region_register() - Register a region in Minidump table.
What happens if lets say a driver calls qcom_minidump_region_register() 
on every probe and driver is loaded and unloaded multiple times, we will 
endup with stale pointers for that region phys_addr comes from 
dynamically allocated region.
Is there a reason why we do not have a  qcom_minidump_region_unregister() ?


> + * @region: minidump region.
> + *
> + * Client should not call this directly instead first call qcom_minidump_ready()
> + * to check if minidump is ready to do registration if yes, then call this API.
> + *
> + * Return: On success, it returns region index in minidump table and negative
> + * error value on failure.
> + */
> +int qcom_minidump_region_register(const struct qcom_minidump_region *region)
> +{
> +	unsigned int num_region;
> +	int ret;
> +
> +	mutex_lock(&minidump_lock);
> +	if (!minidump.ready) {
> +		pr_info("minidump is not ready\n");
> +		ret = -EINVAL;
> +		goto unlock;
> +	}
> +
> +	/* Initialize minidump context on first call */
> +	if (!minidump.init_done) {
> +		ret = qcom_minidump_init();
> +		if (ret)
> +			goto unlock;
> +
> +		/* First entry would be ELF header */
> +		ret = minidump_add_elf_header();
> +		if (ret) {
> +			kfree(minidump.md_regions);
> +			goto unlock;
> +		}
> +
> +		minidump.init_done = true;
> +	}
> +
> +	if (!qcom_minidump_valid_region(region)) {
> +		ret = -EINVAL;
> +		goto unlock;
> +	}
> +
> +	ret = get_minidump_region_index(region);
> +	if (ret >= 0) {
> +		pr_info("%s region is already registered\n", region->name);
> +		ret = -EEXIST;
> +		goto unlock;
> +	}
> +
> +	/* Check if there is a room for a new entry */
> +	ret = num_region = le32_to_cpu(minidump.md_ss_toc->region_count);
> +	if (num_region >= MAX_NUM_ENTRIES) {
> +		pr_err("maximum region limit %u reached\n", num_region);
> +		ret = -ENOSPC;
> +		goto unlock;
> +	}
> +
> +	minidump_ss_add_region(region);
> +	minidump_update_elf_header(region);
> +unlock:
> +	mutex_unlock(&minidump_lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(qcom_minidump_region_register);
> +
> +static void __exit qcom_minidump_exit(void)
> +{
> +	mutex_lock(&minidump_lock);
> +	kfree(minidump.elf.ehdr);
> +	kfree(minidump.md_regions);

AFAIU, if minidump driver is unloaded then we will endup with stale 
pointers in smem.

> +	mutex_unlock(&minidump_lock);
> +}
> +module_exit(qcom_minidump_exit);
> +
> +MODULE_DESCRIPTION("Qualcomm minidump driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/soc/qcom/minidump.h b/include/soc/qcom/minidump.h

include/soc/qcom/qcom_minidump.h should be merged with 
include/soc/qcom/minidump.h, I see no point in having two headers for 
same thing.


> new file mode 100644
> index 0000000..a50843f
> --- /dev/null
> +++ b/include/soc/qcom/minidump.h
> @@ -0,0 +1,40 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#ifndef _MINIDUMP_H_
> +#define _MINIDUMP_H_
> +
> +#include <linux/types.h>
> +
> +#define MAX_NAME_LENGTH		12
> +
> +/**
> + * struct qcom_minidump_region - Minidump region information
> + *
> + * @name:	Entry name, Minidump will dump binary with this name.
> + * @virt_addr:  Virtual address of the entry.
> + * @phys_addr:	Physical address of the entry to dump.
> + * @size:	Number of byte to dump from @address location,
> + *		and it should be 4 byte aligned.
> + */
> +struct qcom_minidump_region {
> +	char		name[MAX_NAME_LENGTH];
> +	void		*virt_addr;
> +	phys_addr_t	phys_addr;
> +	size_t		size;
> +};
> +
> +#if IS_ENABLED(CONFIG_QCOM_MINIDUMP)
> +extern int qcom_minidump_ready(void);
> +extern int qcom_minidump_region_register(const struct qcom_minidump_region *entry);
> +#else
> +static inline int qcom_minidump_ready(void) { return 0; }
> +static inline int qcom_minidump_region_register(const struct qcom_minidump_region *entry)
> +{
> +	/* Return quietly, if minidump is not enabled */
> +	return 0;
> +}
> +#endif
> +#endif /* _MINIDUMP_H_ */
> diff --git a/include/soc/qcom/qcom_minidump.h b/include/soc/qcom/qcom_minidump.h
> index 84c8605..e8630ed 100644
> --- a/include/soc/qcom/qcom_minidump.h
> +++ b/include/soc/qcom/qcom_minidump.h
> @@ -10,11 +10,25 @@
>   
>   #define MAX_NUM_OF_SS           10
>   #define MAX_REGION_NAME_LENGTH  16
> +
> +#define MINIDUMP_REVISION	1
>   #define SBL_MINIDUMP_SMEM_ID	602
> +
> +/* Application processor minidump descriptor */
> +#define MINIDUMP_APSS_DESC	0
> +#define SMEM_ENTRY_SIZE		40
> +
>   #define MINIDUMP_REGION_VALID		('V' << 24 | 'A' << 16 | 'L' << 8 | 'I' << 0)
> +#define MINIDUMP_REGION_INVALID		('I' << 24 | 'N' << 16 | 'V' << 8 | 'A' << 0)
> +#define MINIDUMP_REGION_INIT		('I' << 24 | 'N' << 16 | 'I' << 8 | 'T' << 0)
> +#define MINIDUMP_REGION_NOINIT		0
> +
> +#define MINIDUMP_SS_ENCR_REQ		(0 << 24 | 'Y' << 16 | 'E' << 8 | 'S' << 0)
> +#define MINIDUMP_SS_ENCR_NOTREQ		(0 << 24 | 0 << 16 | 'N' << 8 | 'R' << 0)
> +#define MINIDUMP_SS_ENCR_NONE		('N' << 24 | 'O' << 16 | 'N' << 8 | 'E' << 0)
>   #define MINIDUMP_SS_ENCR_DONE		('D' << 24 | 'O' << 16 | 'N' << 8 | 'E' << 0)
> +#define MINIDUMP_SS_ENCR_START		('S' << 24 | 'T' << 16 | 'R' << 8 | 'T' << 0)
>   #define MINIDUMP_SS_ENABLED		('E' << 24 | 'N' << 16 | 'B' << 8 | 'L' << 0)
> -
>   /**
>    * struct minidump_region - Minidump region
>    * @name		: Name of the region to be dumped
> @@ -63,4 +77,12 @@ struct minidump_global_toc {
>   	struct minidump_subsystem	subsystems[MAX_NUM_OF_SS];
>   };
>   
> +#if IS_ENABLED(CONFIG_QCOM_MINIDUMP)
> +extern struct minidump_subsystem *qcom_minidump_subsystem_desc(unsigned int minidump_index);
> +#else
> +static inline struct minidump_subsystem *qcom_minidump_subsystem_desc(unsigned int minidump_index)
> +{
> +	return NULL;
> +}
> +#endif
>   #endif  /* _QCOM_MINIDUMP_H_ */

Re: [PATCH v2 3/6] docs: qcom: Add qualcomm minidump guide

[Srinivas Kandagatla]

On 22/03/2023 13:30, Mukesh Ojha wrote:
> +Dump collection
> +---------------
> +
> +The solution supports extracting the minidump produced either over USB or
> +stored to an attached storage device.
> +
> +By default, dumps are downloaded via USB to the attached x86_64 machine
> +running PCAT (Qualcomm tool) software. Upon download, we will see

Are these both PCAT and dexter tools public?

--srini
> +a set of binary blobs starts with name md_* in PCAT configured directory
> +in x86_64 machine, so for above example from the client it will be
> +md_REGION_A.BIN. This binary blob depends on region content to determine
> +whether it needs external parser support to get the content of the region,
> +so for simple plain ASCII text we don't need any parsing and the content
> +can be seen just opening the binary file.
> +
> +To collect the dump to attached storage type, one need to write appropriate
> +value to IMEM register, in that case dumps are collected in rawdump
> +partition on the target device itself.
> +
> +One need to read the entire rawdump partition and pull out content to
> +save it onto the attached x86_64 machine over USB. Later, this rawdump
> +can be pass it to another tool dexter.exe(Qualcomm tool) which converts
> +this into the similar binary blobs which we have got it when download type
> +was set to USB i.e a set of registered region as blobs and their name
> +starts with md_*.
> -- 2.7.4

Re: [PATCH v2 2/6] remoteproc: qcom: Move minidump specific data to qcom_minidump.h

[Srinivas Kandagatla]

On 22/03/2023 13:30, Mukesh Ojha wrote:
> Move minidump specific data types and macros to a separate internal
> header(qcom_minidump.h) so that it can be shared among different

minidump.h should be good as we are already in include/soc/qcom/

--srini

> Qualcomm drivers.
> 
> There is no change in functional behavior after this.
> 
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
>   drivers/remoteproc/qcom_common.c | 56 +---------------------------------
>   include/soc/qcom/qcom_minidump.h | 66 ++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 67 insertions(+), 55 deletions(-)
>   create mode 100644 include/soc/qcom/qcom_minidump.h
> 
> diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
> index 805e525..88fc984 100644
> --- a/drivers/remoteproc/qcom_common.c
> +++ b/drivers/remoteproc/qcom_common.c
> @@ -18,6 +18,7 @@
>   #include <linux/slab.h>
>   #include <linux/soc/qcom/mdt_loader.h>
>   #include <linux/soc/qcom/smem.h>
> +#include <soc/qcom/qcom_minidump.h>
>   
>   #include "remoteproc_internal.h"
>   #include "qcom_common.h"
> @@ -26,61 +27,6 @@
>   #define to_smd_subdev(d) container_of(d, struct qcom_rproc_subdev, subdev)
>   #define to_ssr_subdev(d) container_of(d, struct qcom_rproc_ssr, subdev)
>   
> -#define MAX_NUM_OF_SS           10
> -#define MAX_REGION_NAME_LENGTH  16
> -#define SBL_MINIDUMP_SMEM_ID	602
> -#define MINIDUMP_REGION_VALID		('V' << 24 | 'A' << 16 | 'L' << 8 | 'I' << 0)
> -#define MINIDUMP_SS_ENCR_DONE		('D' << 24 | 'O' << 16 | 'N' << 8 | 'E' << 0)
> -#define MINIDUMP_SS_ENABLED		('E' << 24 | 'N' << 16 | 'B' << 8 | 'L' << 0)
> -
> -/**
> - * struct minidump_region - Minidump region
> - * @name		: Name of the region to be dumped
> - * @seq_num:		: Use to differentiate regions with same name.
> - * @valid		: This entry to be dumped (if set to 1)
> - * @address		: Physical address of region to be dumped
> - * @size		: Size of the region
> - */
> -struct minidump_region {
> -	char	name[MAX_REGION_NAME_LENGTH];
> -	__le32	seq_num;
> -	__le32	valid;
> -	__le64	address;
> -	__le64	size;
> -};
> -
> -/**
> - * struct minidump_subsystem - Subsystem's SMEM Table of content
> - * @status : Subsystem toc init status
> - * @enabled : if set to 1, this region would be copied during coredump
> - * @encryption_status: Encryption status for this subsystem
> - * @encryption_required : Decides to encrypt the subsystem regions or not
> - * @region_count : Number of regions added in this subsystem toc
> - * @regions_baseptr : regions base pointer of the subsystem
> - */
> -struct minidump_subsystem {
> -	__le32	status;
> -	__le32	enabled;
> -	__le32	encryption_status;
> -	__le32	encryption_required;
> -	__le32	region_count;
> -	__le64	regions_baseptr;
> -};
> -
> -/**
> - * struct minidump_global_toc - Global Table of Content
> - * @status : Global Minidump init status
> - * @md_revision : Minidump revision
> - * @enabled : Minidump enable status
> - * @subsystems : Array of subsystems toc
> - */
> -struct minidump_global_toc {
> -	__le32				status;
> -	__le32				md_revision;
> -	__le32				enabled;
> -	struct minidump_subsystem	subsystems[MAX_NUM_OF_SS];
> -};
> -
>   struct qcom_ssr_subsystem {
>   	const char *name;
>   	struct srcu_notifier_head notifier_list;
> diff --git a/include/soc/qcom/qcom_minidump.h b/include/soc/qcom/qcom_minidump.h
> new file mode 100644
> index 0000000..84c8605
> --- /dev/null
> +++ b/include/soc/qcom/qcom_minidump.h
> @@ -0,0 +1,66 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Qualcomm minidump shared data structures and macros
> + *
> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#ifndef _QCOM_MINIDUMP_H_
> +#define _QCOM_MINIDUMP_H_
> +
> +#define MAX_NUM_OF_SS           10
> +#define MAX_REGION_NAME_LENGTH  16
> +#define SBL_MINIDUMP_SMEM_ID	602
> +#define MINIDUMP_REGION_VALID		('V' << 24 | 'A' << 16 | 'L' << 8 | 'I' << 0)
> +#define MINIDUMP_SS_ENCR_DONE		('D' << 24 | 'O' << 16 | 'N' << 8 | 'E' << 0)
> +#define MINIDUMP_SS_ENABLED		('E' << 24 | 'N' << 16 | 'B' << 8 | 'L' << 0)
> +
> +/**
> + * struct minidump_region - Minidump region
> + * @name		: Name of the region to be dumped
> + * @seq_num:		: Use to differentiate regions with same name.
> + * @valid		: This entry to be dumped (if set to 1)
> + * @address		: Physical address of region to be dumped
> + * @size		: Size of the region
> + */
> +struct minidump_region {
> +	char	name[MAX_REGION_NAME_LENGTH];
> +	__le32	seq_num;
> +	__le32	valid;
> +	__le64	address;
> +	__le64	size;
> +};
> +
> +/**
> + * struct minidump_subsystem - Subsystem's SMEM Table of content
> + * @status : Subsystem toc init status
> + * @enabled : if set to 1, this region would be copied during coredump
> + * @encryption_status: Encryption status for this subsystem
> + * @encryption_required : Decides to encrypt the subsystem regions or not
> + * @region_count : Number of regions added in this subsystem toc
> + * @regions_baseptr : regions base pointer of the subsystem
> + */
> +struct minidump_subsystem {
> +	__le32	status;
> +	__le32	enabled;
> +	__le32	encryption_status;
> +	__le32	encryption_required;
> +	__le32	region_count;
> +	__le64	regions_baseptr;
> +};
> +
> +/**
> + * struct minidump_global_toc - Global Table of Content
> + * @status : Global Minidump init status
> + * @md_revision : Minidump revision
> + * @enabled : Minidump enable status
> + * @subsystems : Array of subsystems toc
> + */
> +struct minidump_global_toc {
> +	__le32				status;
> +	__le32				md_revision;
> +	__le32				enabled;
> +	struct minidump_subsystem	subsystems[MAX_NUM_OF_SS];
> +};
> +
> +#endif  /* _QCOM_MINIDUMP_H_ */

Re: [PATCH v2 2/6] remoteproc: qcom: Move minidump specific data to qcom_minidump.h

[Mukesh Ojha]

Thanks again for coming back on this.

On 4/14/2023 4:02 AM, Srinivas Kandagatla wrote:
> 
> 
> On 22/03/2023 13:30, Mukesh Ojha wrote:
>> Move minidump specific data types and macros to a separate internal
>> header(qcom_minidump.h) so that it can be shared among different
> 
> minidump.h should be good as we are already in include/soc/qcom/


Initially, i wanted to protect the content of qcom_minidump.h between 
qcom_minidump.c and qcom_common.c

Ideally, here qcom_minidump.h should be supplier/provider header and can 
be shared among above qcom_minidump.c and qcom_common.c but since they 
are not in same directory, moved it inside include/soc/qcom/ as separate 
header than consumer header minidump.h .

-Mukesh
> 
> --srini
> 
>> Qualcomm drivers.
>>
>> There is no change in functional behavior after this.
>>
>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>> ---
>>   drivers/remoteproc/qcom_common.c | 56 
>> +---------------------------------
>>   include/soc/qcom/qcom_minidump.h | 66 
>> ++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 67 insertions(+), 55 deletions(-)
>>   create mode 100644 include/soc/qcom/qcom_minidump.h
>>
>> diff --git a/drivers/remoteproc/qcom_common.c 
>> b/drivers/remoteproc/qcom_common.c
>> index 805e525..88fc984 100644
>> --- a/drivers/remoteproc/qcom_common.c
>> +++ b/drivers/remoteproc/qcom_common.c
>> @@ -18,6 +18,7 @@
>>   #include <linux/slab.h>
>>   #include <linux/soc/qcom/mdt_loader.h>
>>   #include <linux/soc/qcom/smem.h>
>> +#include <soc/qcom/qcom_minidump.h>
>>   #include "remoteproc_internal.h"
>>   #include "qcom_common.h"
>> @@ -26,61 +27,6 @@
>>   #define to_smd_subdev(d) container_of(d, struct qcom_rproc_subdev, 
>> subdev)
>>   #define to_ssr_subdev(d) container_of(d, struct qcom_rproc_ssr, subdev)
>> -#define MAX_NUM_OF_SS           10
>> -#define MAX_REGION_NAME_LENGTH  16
>> -#define SBL_MINIDUMP_SMEM_ID    602
>> -#define MINIDUMP_REGION_VALID        ('V' << 24 | 'A' << 16 | 'L' << 
>> 8 | 'I' << 0)
>> -#define MINIDUMP_SS_ENCR_DONE        ('D' << 24 | 'O' << 16 | 'N' << 
>> 8 | 'E' << 0)
>> -#define MINIDUMP_SS_ENABLED        ('E' << 24 | 'N' << 16 | 'B' << 8 
>> | 'L' << 0)
>> -
>> -/**
>> - * struct minidump_region - Minidump region
>> - * @name        : Name of the region to be dumped
>> - * @seq_num:        : Use to differentiate regions with same name.
>> - * @valid        : This entry to be dumped (if set to 1)
>> - * @address        : Physical address of region to be dumped
>> - * @size        : Size of the region
>> - */
>> -struct minidump_region {
>> -    char    name[MAX_REGION_NAME_LENGTH];
>> -    __le32    seq_num;
>> -    __le32    valid;
>> -    __le64    address;
>> -    __le64    size;
>> -};
>> -
>> -/**
>> - * struct minidump_subsystem - Subsystem's SMEM Table of content
>> - * @status : Subsystem toc init status
>> - * @enabled : if set to 1, this region would be copied during coredump
>> - * @encryption_status: Encryption status for this subsystem
>> - * @encryption_required : Decides to encrypt the subsystem regions or 
>> not
>> - * @region_count : Number of regions added in this subsystem toc
>> - * @regions_baseptr : regions base pointer of the subsystem
>> - */
>> -struct minidump_subsystem {
>> -    __le32    status;
>> -    __le32    enabled;
>> -    __le32    encryption_status;
>> -    __le32    encryption_required;
>> -    __le32    region_count;
>> -    __le64    regions_baseptr;
>> -};
>> -
>> -/**
>> - * struct minidump_global_toc - Global Table of Content
>> - * @status : Global Minidump init status
>> - * @md_revision : Minidump revision
>> - * @enabled : Minidump enable status
>> - * @subsystems : Array of subsystems toc
>> - */
>> -struct minidump_global_toc {
>> -    __le32                status;
>> -    __le32                md_revision;
>> -    __le32                enabled;
>> -    struct minidump_subsystem    subsystems[MAX_NUM_OF_SS];
>> -};
>> -
>>   struct qcom_ssr_subsystem {
>>       const char *name;
>>       struct srcu_notifier_head notifier_list;
>> diff --git a/include/soc/qcom/qcom_minidump.h 
>> b/include/soc/qcom/qcom_minidump.h
>> new file mode 100644
>> index 0000000..84c8605
>> --- /dev/null
>> +++ b/include/soc/qcom/qcom_minidump.h
>> @@ -0,0 +1,66 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Qualcomm minidump shared data structures and macros
>> + *
>> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights 
>> reserved.
>> + */
>> +
>> +#ifndef _QCOM_MINIDUMP_H_
>> +#define _QCOM_MINIDUMP_H_
>> +
>> +#define MAX_NUM_OF_SS           10
>> +#define MAX_REGION_NAME_LENGTH  16
>> +#define SBL_MINIDUMP_SMEM_ID    602
>> +#define MINIDUMP_REGION_VALID        ('V' << 24 | 'A' << 16 | 'L' << 
>> 8 | 'I' << 0)
>> +#define MINIDUMP_SS_ENCR_DONE        ('D' << 24 | 'O' << 16 | 'N' << 
>> 8 | 'E' << 0)
>> +#define MINIDUMP_SS_ENABLED        ('E' << 24 | 'N' << 16 | 'B' << 8 
>> | 'L' << 0)
>> +
>> +/**
>> + * struct minidump_region - Minidump region
>> + * @name        : Name of the region to be dumped
>> + * @seq_num:        : Use to differentiate regions with same name.
>> + * @valid        : This entry to be dumped (if set to 1)
>> + * @address        : Physical address of region to be dumped
>> + * @size        : Size of the region
>> + */
>> +struct minidump_region {
>> +    char    name[MAX_REGION_NAME_LENGTH];
>> +    __le32    seq_num;
>> +    __le32    valid;
>> +    __le64    address;
>> +    __le64    size;
>> +};
>> +
>> +/**
>> + * struct minidump_subsystem - Subsystem's SMEM Table of content
>> + * @status : Subsystem toc init status
>> + * @enabled : if set to 1, this region would be copied during coredump
>> + * @encryption_status: Encryption status for this subsystem
>> + * @encryption_required : Decides to encrypt the subsystem regions or 
>> not
>> + * @region_count : Number of regions added in this subsystem toc
>> + * @regions_baseptr : regions base pointer of the subsystem
>> + */
>> +struct minidump_subsystem {
>> +    __le32    status;
>> +    __le32    enabled;
>> +    __le32    encryption_status;
>> +    __le32    encryption_required;
>> +    __le32    region_count;
>> +    __le64    regions_baseptr;
>> +};
>> +
>> +/**
>> + * struct minidump_global_toc - Global Table of Content
>> + * @status : Global Minidump init status
>> + * @md_revision : Minidump revision
>> + * @enabled : Minidump enable status
>> + * @subsystems : Array of subsystems toc
>> + */
>> +struct minidump_global_toc {
>> +    __le32                status;
>> +    __le32                md_revision;
>> +    __le32                enabled;
>> +    struct minidump_subsystem    subsystems[MAX_NUM_OF_SS];
>> +};
>> +
>> +#endif  /* _QCOM_MINIDUMP_H_ */

Re: [PATCH v2 2/6] remoteproc: qcom: Move minidump specific data to qcom_minidump.h

[Srinivas Kandagatla]

On 14/04/2023 08:05, Mukesh Ojha wrote:
> Thanks again for coming back on this.
> 
> On 4/14/2023 4:02 AM, Srinivas Kandagatla wrote:
>>
>>
>> On 22/03/2023 13:30, Mukesh Ojha wrote:
>>> Move minidump specific data types and macros to a separate internal
>>> header(qcom_minidump.h) so that it can be shared among different
>>
>> minidump.h should be good as we are already in include/soc/qcom/
> 
> 
> Initially, i wanted to protect the content of qcom_minidump.h between 
> qcom_minidump.c and qcom_common.c
> 
> Ideally, here qcom_minidump.h should be supplier/provider header and can 

Am not sure if I understand the supplier concept correctly.
AFAIU, we have a 2 sets of apis

1. get hold of minidump descriptor based on minidump_id fro gtoc using 
qcom_minidump_subsystem_desc(). Am assuming which ever driver uses this 
api will set there segments and regions in there respective drivers.

2. setting regions/segments in APSS minidump descriptors which are done 
via qcom_minidump_region_register(). TBH this should be renamed to 
qcom_apss_minidump_region_register().

mixing of thsee apis makes it bit confusing, specially we have these two 
category of apis that deal with different things.

Does it make sense to spit and abstract them properly by doing?


1. minidump driver to deal with handling gtoc and providing descriptors 
to the consumers like remoteproc or apss, This can probably live within 
smem driver as loc for this support is very minimal and proabably rename 
the api accordingly.

2. apss_minidump driver to allow other qcom drivers to 
register/unregister regions within apss minidump descriptor.

did I miss something?

thanks,
Srini

> be shared among above qcom_minidump.c and qcom_common.c but since they 
> are not in same directory, moved it inside include/soc/qcom/ as separate 
> header than consumer header minidump.h . >
> -Mukesh
>>
>> --srini
>>
>>> Qualcomm drivers.
>>>
>>> There is no change in functional behavior after this.
>>>
>>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>>> ---
>>>   drivers/remoteproc/qcom_common.c | 56 
>>> +---------------------------------
>>>   include/soc/qcom/qcom_minidump.h | 66 
>>> ++++++++++++++++++++++++++++++++++++++++
>>>   2 files changed, 67 insertions(+), 55 deletions(-)
>>>   create mode 100644 include/soc/qcom/qcom_minidump.h
>>>
>>> diff --git a/drivers/remoteproc/qcom_common.c 
>>> b/drivers/remoteproc/qcom_common.c
>>> index 805e525..88fc984 100644
>>> --- a/drivers/remoteproc/qcom_common.c
>>> +++ b/drivers/remoteproc/qcom_common.c
>>> @@ -18,6 +18,7 @@
>>>   #include <linux/slab.h>
>>>   #include <linux/soc/qcom/mdt_loader.h>
>>>   #include <linux/soc/qcom/smem.h>
>>> +#include <soc/qcom/qcom_minidump.h>
>>>   #include "remoteproc_internal.h"
>>>   #include "qcom_common.h"
>>> @@ -26,61 +27,6 @@
>>>   #define to_smd_subdev(d) container_of(d, struct qcom_rproc_subdev, 
>>> subdev)
>>>   #define to_ssr_subdev(d) container_of(d, struct qcom_rproc_ssr, 
>>> subdev)
>>> -#define MAX_NUM_OF_SS           10
>>> -#define MAX_REGION_NAME_LENGTH  16
>>> -#define SBL_MINIDUMP_SMEM_ID    602
>>> -#define MINIDUMP_REGION_VALID        ('V' << 24 | 'A' << 16 | 'L' << 
>>> 8 | 'I' << 0)
>>> -#define MINIDUMP_SS_ENCR_DONE        ('D' << 24 | 'O' << 16 | 'N' << 
>>> 8 | 'E' << 0)
>>> -#define MINIDUMP_SS_ENABLED        ('E' << 24 | 'N' << 16 | 'B' << 8 
>>> | 'L' << 0)
>>> -
>>> -/**
>>> - * struct minidump_region - Minidump region
>>> - * @name        : Name of the region to be dumped
>>> - * @seq_num:        : Use to differentiate regions with same name.
>>> - * @valid        : This entry to be dumped (if set to 1)
>>> - * @address        : Physical address of region to be dumped
>>> - * @size        : Size of the region
>>> - */
>>> -struct minidump_region {
>>> -    char    name[MAX_REGION_NAME_LENGTH];
>>> -    __le32    seq_num;
>>> -    __le32    valid;
>>> -    __le64    address;
>>> -    __le64    size;
>>> -};
>>> -
>>> -/**
>>> - * struct minidump_subsystem - Subsystem's SMEM Table of content
>>> - * @status : Subsystem toc init status
>>> - * @enabled : if set to 1, this region would be copied during coredump
>>> - * @encryption_status: Encryption status for this subsystem
>>> - * @encryption_required : Decides to encrypt the subsystem regions 
>>> or not
>>> - * @region_count : Number of regions added in this subsystem toc
>>> - * @regions_baseptr : regions base pointer of the subsystem
>>> - */
>>> -struct minidump_subsystem {
>>> -    __le32    status;
>>> -    __le32    enabled;
>>> -    __le32    encryption_status;
>>> -    __le32    encryption_required;
>>> -    __le32    region_count;
>>> -    __le64    regions_baseptr;
>>> -};
>>> -
>>> -/**
>>> - * struct minidump_global_toc - Global Table of Content
>>> - * @status : Global Minidump init status
>>> - * @md_revision : Minidump revision
>>> - * @enabled : Minidump enable status
>>> - * @subsystems : Array of subsystems toc
>>> - */
>>> -struct minidump_global_toc {
>>> -    __le32                status;
>>> -    __le32                md_revision;
>>> -    __le32                enabled;
>>> -    struct minidump_subsystem    subsystems[MAX_NUM_OF_SS];
>>> -};
>>> -
>>>   struct qcom_ssr_subsystem {
>>>       const char *name;
>>>       struct srcu_notifier_head notifier_list;
>>> diff --git a/include/soc/qcom/qcom_minidump.h 
>>> b/include/soc/qcom/qcom_minidump.h
>>> new file mode 100644
>>> index 0000000..84c8605
>>> --- /dev/null
>>> +++ b/include/soc/qcom/qcom_minidump.h
>>> @@ -0,0 +1,66 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * Qualcomm minidump shared data structures and macros
>>> + *
>>> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights 
>>> reserved.
>>> + */
>>> +
>>> +#ifndef _QCOM_MINIDUMP_H_
>>> +#define _QCOM_MINIDUMP_H_
>>> +
>>> +#define MAX_NUM_OF_SS           10
>>> +#define MAX_REGION_NAME_LENGTH  16
>>> +#define SBL_MINIDUMP_SMEM_ID    602
>>> +#define MINIDUMP_REGION_VALID        ('V' << 24 | 'A' << 16 | 'L' << 
>>> 8 | 'I' << 0)
>>> +#define MINIDUMP_SS_ENCR_DONE        ('D' << 24 | 'O' << 16 | 'N' << 
>>> 8 | 'E' << 0)
>>> +#define MINIDUMP_SS_ENABLED        ('E' << 24 | 'N' << 16 | 'B' << 8 
>>> | 'L' << 0)
>>> +
>>> +/**
>>> + * struct minidump_region - Minidump region
>>> + * @name        : Name of the region to be dumped
>>> + * @seq_num:        : Use to differentiate regions with same name.
>>> + * @valid        : This entry to be dumped (if set to 1)
>>> + * @address        : Physical address of region to be dumped
>>> + * @size        : Size of the region
>>> + */
>>> +struct minidump_region {
>>> +    char    name[MAX_REGION_NAME_LENGTH];
>>> +    __le32    seq_num;
>>> +    __le32    valid;
>>> +    __le64    address;
>>> +    __le64    size;
>>> +};
>>> +
>>> +/**
>>> + * struct minidump_subsystem - Subsystem's SMEM Table of content
>>> + * @status : Subsystem toc init status
>>> + * @enabled : if set to 1, this region would be copied during coredump
>>> + * @encryption_status: Encryption status for this subsystem
>>> + * @encryption_required : Decides to encrypt the subsystem regions 
>>> or not
>>> + * @region_count : Number of regions added in this subsystem toc
>>> + * @regions_baseptr : regions base pointer of the subsystem
>>> + */
>>> +struct minidump_subsystem {
>>> +    __le32    status;
>>> +    __le32    enabled;
>>> +    __le32    encryption_status;
>>> +    __le32    encryption_required;
>>> +    __le32    region_count;
>>> +    __le64    regions_baseptr;
>>> +};
>>> +
>>> +/**
>>> + * struct minidump_global_toc - Global Table of Content
>>> + * @status : Global Minidump init status
>>> + * @md_revision : Minidump revision
>>> + * @enabled : Minidump enable status
>>> + * @subsystems : Array of subsystems toc
>>> + */
>>> +struct minidump_global_toc {
>>> +    __le32                status;
>>> +    __le32                md_revision;
>>> +    __le32                enabled;
>>> +    struct minidump_subsystem    subsystems[MAX_NUM_OF_SS];
>>> +};
>>> +
>>> +#endif  /* _QCOM_MINIDUMP_H_ */

Re: [PATCH v2 6/6] remoterproc: qcom: refactor to leverage exported minidump symbol

[Srinivas Kandagatla]

On 22/03/2023 13:30, Mukesh Ojha wrote:
> qcom_minidump driver provides qcom_minidump_subsystem_desc()
> exported API which other driver can use it query subsystem
> descriptor. Refactor qcom_minidump() to use this symbol.
> 
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
>   drivers/remoteproc/qcom_common.c | 13 ++-----------
>   1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
> index 88fc984..240e9f7 100644
> --- a/drivers/remoteproc/qcom_common.c
> +++ b/drivers/remoteproc/qcom_common.c
> @@ -94,19 +94,10 @@ void qcom_minidump(struct rproc *rproc, unsigned int minidump_id,
>   {
>   	int ret;
>   	struct minidump_subsystem *subsystem;
> -	struct minidump_global_toc *toc;
>   
> -	/* Get Global minidump ToC*/
> -	toc = qcom_smem_get(QCOM_SMEM_HOST_ANY, SBL_MINIDUMP_SMEM_ID, NULL);
> -
> -	/* check if global table pointer exists and init is set */
> -	if (IS_ERR(toc) || !toc->status) {
> -		dev_err(&rproc->dev, "Minidump TOC not found in SMEM\n");
> +	subsystem = qcom_minidump_subsystem_desc(minidump_id);
> +	if (IS_ERR(subsystem))
>   		return;

Sorry If I am missing something but I got lost looking at the below code 
snippet in drivers/remoteproc/qcom_common.c


-------------------->cut<-----------------------------
	subsystem = qcom_minidump_subsystem_desc(minidump_id);
	if (IS_ERR(subsystem))
		return;

	/**
	 * Collect minidump if SS ToC is valid and segment table
	 * is initialized in memory and encryption status is set.
	 */
	if (subsystem->regions_baseptr == 0 ||
	    le32_to_cpu(subsystem->status) != 1 ||
	    le32_to_cpu(subsystem->enabled) != MINIDUMP_SS_ENABLED ||
	    le32_to_cpu(subsystem->encryption_status) != MINIDUMP_SS_ENCR_DONE) {
		dev_err(&rproc->dev, "Minidump not ready, skipping\n");
		return;
	}
-------------------->cut<-----------------------------

where does "subsystem->regions_baseptr" for this ADSP minidump 
descriptor get set?


--srini

> -	}
> -
> -	/* Get subsystem table of contents using the minidump id */
> -	subsystem = &toc->subsystems[minidump_id];
>   
>   	/**
>   	 * Collect minidump if SS ToC is valid and segment table

Re: [PATCH v2 6/6] remoterproc: qcom: refactor to leverage exported minidump symbol

[Mukesh Ojha]

On 4/14/2023 4:14 PM, Srinivas Kandagatla wrote:
> 
> 
> On 22/03/2023 13:30, Mukesh Ojha wrote:
>> qcom_minidump driver provides qcom_minidump_subsystem_desc()
>> exported API which other driver can use it query subsystem
>> descriptor. Refactor qcom_minidump() to use this symbol.
>>
>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>> ---
>>   drivers/remoteproc/qcom_common.c | 13 ++-----------
>>   1 file changed, 2 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/remoteproc/qcom_common.c 
>> b/drivers/remoteproc/qcom_common.c
>> index 88fc984..240e9f7 100644
>> --- a/drivers/remoteproc/qcom_common.c
>> +++ b/drivers/remoteproc/qcom_common.c
>> @@ -94,19 +94,10 @@ void qcom_minidump(struct rproc *rproc, unsigned 
>> int minidump_id,
>>   {
>>       int ret;
>>       struct minidump_subsystem *subsystem;
>> -    struct minidump_global_toc *toc;
>> -    /* Get Global minidump ToC*/
>> -    toc = qcom_smem_get(QCOM_SMEM_HOST_ANY, SBL_MINIDUMP_SMEM_ID, NULL);
>> -
>> -    /* check if global table pointer exists and init is set */
>> -    if (IS_ERR(toc) || !toc->status) {
>> -        dev_err(&rproc->dev, "Minidump TOC not found in SMEM\n");
>> +    subsystem = qcom_minidump_subsystem_desc(minidump_id);
>> +    if (IS_ERR(subsystem))
>>           return;
> 
> Sorry If I am missing something but I got lost looking at the below code 
> snippet in drivers/remoteproc/qcom_common.c
> 
> 
> -------------------->cut<-----------------------------
>      subsystem = qcom_minidump_subsystem_desc(minidump_id);
>      if (IS_ERR(subsystem))
>          return;
> 
>      /**
>       * Collect minidump if SS ToC is valid and segment table
>       * is initialized in memory and encryption status is set.
>       */
>      if (subsystem->regions_baseptr == 0 ||
>          le32_to_cpu(subsystem->status) != 1 ||
>          le32_to_cpu(subsystem->enabled) != MINIDUMP_SS_ENABLED ||
>          le32_to_cpu(subsystem->encryption_status) != 
> MINIDUMP_SS_ENCR_DONE) {
>          dev_err(&rproc->dev, "Minidump not ready, skipping\n");
>          return;
>      }
> -------------------->cut<-----------------------------
> 
> where does "subsystem->regions_baseptr" for this ADSP minidump 
> descriptor get set?

Other co-processor such as adsp/cdsp/Mpss has their own way of 
registering their region/segment (mostly they are static known
regions) with minidump global infra and which could be happening
from firmware side .


-Mukesh

> 
> 
> --srini
> 
>> -    }
>> -
>> -    /* Get subsystem table of contents using the minidump id */
>> -    subsystem = &toc->subsystems[minidump_id];
>>       /**
>>        * Collect minidump if SS ToC is valid and segment table

Re: [PATCH v2 6/6] remoterproc: qcom: refactor to leverage exported minidump symbol

[Srinivas Kandagatla]

On 14/04/2023 12:14, Mukesh Ojha wrote:
> 
> 
> On 4/14/2023 4:14 PM, Srinivas Kandagatla wrote:
>>
>>
>> On 22/03/2023 13:30, Mukesh Ojha wrote:
>>> qcom_minidump driver provides qcom_minidump_subsystem_desc()
>>> exported API which other driver can use it query subsystem
>>> descriptor. Refactor qcom_minidump() to use this symbol.
>>>
>>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>>> ---
>>>   drivers/remoteproc/qcom_common.c | 13 ++-----------
>>>   1 file changed, 2 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/remoteproc/qcom_common.c 
>>> b/drivers/remoteproc/qcom_common.c
>>> index 88fc984..240e9f7 100644
>>> --- a/drivers/remoteproc/qcom_common.c
>>> +++ b/drivers/remoteproc/qcom_common.c
>>> @@ -94,19 +94,10 @@ void qcom_minidump(struct rproc *rproc, unsigned 
>>> int minidump_id,
>>>   {
>>>       int ret;
>>>       struct minidump_subsystem *subsystem;
>>> -    struct minidump_global_toc *toc;
>>> -    /* Get Global minidump ToC*/
>>> -    toc = qcom_smem_get(QCOM_SMEM_HOST_ANY, SBL_MINIDUMP_SMEM_ID, 
>>> NULL);
>>> -
>>> -    /* check if global table pointer exists and init is set */
>>> -    if (IS_ERR(toc) || !toc->status) {
>>> -        dev_err(&rproc->dev, "Minidump TOC not found in SMEM\n");
>>> +    subsystem = qcom_minidump_subsystem_desc(minidump_id);
>>> +    if (IS_ERR(subsystem))
>>>           return;
>>
>> Sorry If I am missing something but I got lost looking at the below 
>> code snippet in drivers/remoteproc/qcom_common.c
>>
>>
>> -------------------->cut<-----------------------------
>>      subsystem = qcom_minidump_subsystem_desc(minidump_id);
>>      if (IS_ERR(subsystem))
>>          return;
>>
>>      /**
>>       * Collect minidump if SS ToC is valid and segment table
>>       * is initialized in memory and encryption status is set.
>>       */
>>      if (subsystem->regions_baseptr == 0 ||
>>          le32_to_cpu(subsystem->status) != 1 ||
>>          le32_to_cpu(subsystem->enabled) != MINIDUMP_SS_ENABLED ||
>>          le32_to_cpu(subsystem->encryption_status) != 
>> MINIDUMP_SS_ENCR_DONE) {
>>          dev_err(&rproc->dev, "Minidump not ready, skipping\n");
>>          return;
>>      }
>> -------------------->cut<-----------------------------
>>
>> where does "subsystem->regions_baseptr" for this ADSP minidump 
>> descriptor get set?
> 
> Other co-processor such as adsp/cdsp/Mpss has their own way of 
> registering their region/segment (mostly they are static known
> regions) with minidump global infra and which could be happening
> from firmware side .
If its happening from firmware side, then that ram phys address range 
should be reserved from kernel usage I guess.

Do you have more details on where exactly is this reserved from within 
linux kernel?


--srini

> 
> 
> -Mukesh
> 
>>
>>
>> --srini
>>
>>> -    }
>>> -
>>> -    /* Get subsystem table of contents using the minidump id */
>>> -    subsystem = &toc->subsystems[minidump_id];
>>>       /**
>>>        * Collect minidump if SS ToC is valid and segment table

Re: [PATCH v2 6/6] remoterproc: qcom: refactor to leverage exported minidump symbol

[Mukesh Ojha]

On 4/14/2023 5:10 PM, Srinivas Kandagatla wrote:
> 
> 
> On 14/04/2023 12:14, Mukesh Ojha wrote:
>>
>>
>> On 4/14/2023 4:14 PM, Srinivas Kandagatla wrote:
>>>
>>>
>>> On 22/03/2023 13:30, Mukesh Ojha wrote:
>>>> qcom_minidump driver provides qcom_minidump_subsystem_desc()
>>>> exported API which other driver can use it query subsystem
>>>> descriptor. Refactor qcom_minidump() to use this symbol.
>>>>
>>>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>>>> ---
>>>>   drivers/remoteproc/qcom_common.c | 13 ++-----------
>>>>   1 file changed, 2 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/remoteproc/qcom_common.c 
>>>> b/drivers/remoteproc/qcom_common.c
>>>> index 88fc984..240e9f7 100644
>>>> --- a/drivers/remoteproc/qcom_common.c
>>>> +++ b/drivers/remoteproc/qcom_common.c
>>>> @@ -94,19 +94,10 @@ void qcom_minidump(struct rproc *rproc, unsigned 
>>>> int minidump_id,
>>>>   {
>>>>       int ret;
>>>>       struct minidump_subsystem *subsystem;
>>>> -    struct minidump_global_toc *toc;
>>>> -    /* Get Global minidump ToC*/
>>>> -    toc = qcom_smem_get(QCOM_SMEM_HOST_ANY, SBL_MINIDUMP_SMEM_ID, 
>>>> NULL);
>>>> -
>>>> -    /* check if global table pointer exists and init is set */
>>>> -    if (IS_ERR(toc) || !toc->status) {
>>>> -        dev_err(&rproc->dev, "Minidump TOC not found in SMEM\n");
>>>> +    subsystem = qcom_minidump_subsystem_desc(minidump_id);
>>>> +    if (IS_ERR(subsystem))
>>>>           return;
>>>
>>> Sorry If I am missing something but I got lost looking at the below 
>>> code snippet in drivers/remoteproc/qcom_common.c
>>>
>>>
>>> -------------------->cut<-----------------------------
>>>      subsystem = qcom_minidump_subsystem_desc(minidump_id);
>>>      if (IS_ERR(subsystem))
>>>          return;
>>>
>>>      /**
>>>       * Collect minidump if SS ToC is valid and segment table
>>>       * is initialized in memory and encryption status is set.
>>>       */
>>>      if (subsystem->regions_baseptr == 0 ||
>>>          le32_to_cpu(subsystem->status) != 1 ||
>>>          le32_to_cpu(subsystem->enabled) != MINIDUMP_SS_ENABLED ||
>>>          le32_to_cpu(subsystem->encryption_status) != 
>>> MINIDUMP_SS_ENCR_DONE) {
>>>          dev_err(&rproc->dev, "Minidump not ready, skipping\n");
>>>          return;
>>>      }
>>> -------------------->cut<-----------------------------
>>>
>>> where does "subsystem->regions_baseptr" for this ADSP minidump 
>>> descriptor get set?
>>
>> Other co-processor such as adsp/cdsp/Mpss has their own way of 
>> registering their region/segment (mostly they are static known
>> regions) with minidump global infra and which could be happening
>> from firmware side .
> If its happening from firmware side, then that ram phys address range 
> should be reserved from kernel usage I guess.
> 
> Do you have more details on where exactly is this reserved from within 
> linux kernel?

These regions are inside remoteproc memory carve-out.
like.

adsp_mem: memory@85e00000 {
	reg = <0x0 0x85e00000 0x0 0x2100000>;
	no-map;
};



remoteproc_adsp: remoteproc@30000000 {
	compatible = "qcom,sm8450-adsp-pas";
	reg = <0 0x30000000 0 0x100>;
            ...
            ...
	memory-region = <&adsp_mem>; <==

-Mukesh
	
> 
> 
> --srini
> 
>>
>>
>> -Mukesh
>>
>>>
>>>
>>> --srini
>>>
>>>> -    }
>>>> -
>>>> -    /* Get subsystem table of contents using the minidump id */
>>>> -    subsystem = &toc->subsystems[minidump_id];
>>>>       /**
>>>>        * Collect minidump if SS ToC is valid and segment table

Re: [PATCH v2 6/6] remoterproc: qcom: refactor to leverage exported minidump symbol

[Srinivas Kandagatla]

On 14/04/2023 12:49, Mukesh Ojha wrote:
> 
> 
> On 4/14/2023 5:10 PM, Srinivas Kandagatla wrote:
>>
>>
>> On 14/04/2023 12:14, Mukesh Ojha wrote:
>>>
>>>
>>> On 4/14/2023 4:14 PM, Srinivas Kandagatla wrote:
>>>>
>>>>
>>>> On 22/03/2023 13:30, Mukesh Ojha wrote:
>>>>> qcom_minidump driver provides qcom_minidump_subsystem_desc()
>>>>> exported API which other driver can use it query subsystem
>>>>> descriptor. Refactor qcom_minidump() to use this symbol.
>>>>>
>>>>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>>>>> ---
>>>>>   drivers/remoteproc/qcom_common.c | 13 ++-----------
>>>>>   1 file changed, 2 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/drivers/remoteproc/qcom_common.c 
>>>>> b/drivers/remoteproc/qcom_common.c
>>>>> index 88fc984..240e9f7 100644
>>>>> --- a/drivers/remoteproc/qcom_common.c
>>>>> +++ b/drivers/remoteproc/qcom_common.c
>>>>> @@ -94,19 +94,10 @@ void qcom_minidump(struct rproc *rproc, 
>>>>> unsigned int minidump_id,
>>>>>   {
>>>>>       int ret;
>>>>>       struct minidump_subsystem *subsystem;
>>>>> -    struct minidump_global_toc *toc;
>>>>> -    /* Get Global minidump ToC*/
>>>>> -    toc = qcom_smem_get(QCOM_SMEM_HOST_ANY, SBL_MINIDUMP_SMEM_ID, 
>>>>> NULL);
>>>>> -
>>>>> -    /* check if global table pointer exists and init is set */
>>>>> -    if (IS_ERR(toc) || !toc->status) {
>>>>> -        dev_err(&rproc->dev, "Minidump TOC not found in SMEM\n");
>>>>> +    subsystem = qcom_minidump_subsystem_desc(minidump_id);
>>>>> +    if (IS_ERR(subsystem))
>>>>>           return;
>>>>
>>>> Sorry If I am missing something but I got lost looking at the below 
>>>> code snippet in drivers/remoteproc/qcom_common.c
>>>>
>>>>
>>>> -------------------->cut<-----------------------------
>>>>      subsystem = qcom_minidump_subsystem_desc(minidump_id);
>>>>      if (IS_ERR(subsystem))
>>>>          return;
>>>>
>>>>      /**
>>>>       * Collect minidump if SS ToC is valid and segment table
>>>>       * is initialized in memory and encryption status is set.
>>>>       */
>>>>      if (subsystem->regions_baseptr == 0 ||
>>>>          le32_to_cpu(subsystem->status) != 1 ||
>>>>          le32_to_cpu(subsystem->enabled) != MINIDUMP_SS_ENABLED ||
>>>>          le32_to_cpu(subsystem->encryption_status) != 
>>>> MINIDUMP_SS_ENCR_DONE) {
>>>>          dev_err(&rproc->dev, "Minidump not ready, skipping\n");
>>>>          return;
>>>>      }
>>>> -------------------->cut<-----------------------------
>>>>
>>>> where does "subsystem->regions_baseptr" for this ADSP minidump 
>>>> descriptor get set?
>>>
>>> Other co-processor such as adsp/cdsp/Mpss has their own way of 
>>> registering their region/segment (mostly they are static known
>>> regions) with minidump global infra and which could be happening
>>> from firmware side .
>> If its happening from firmware side, then that ram phys address range 
>> should be reserved from kernel usage I guess.
>>
>> Do you have more details on where exactly is this reserved from within 
>> linux kernel?
> 
> These regions are inside remoteproc memory carve-out.
> like.
> 
> adsp_mem: memory@85e00000 {
>      reg = <0x0 0x85e00000 0x0 0x2100000>;
>      no-map;
> };

thanks for explaining.

--srini
> 
> 
> 
> remoteproc_adsp: remoteproc@30000000 {
>      compatible = "qcom,sm8450-adsp-pas";
>      reg = <0 0x30000000 0 0x100>;
>             ...
>             ...
>      memory-region = <&adsp_mem>; <==
> 
> -Mukesh
> 
>>
>>
>> --srini
>>
>>>
>>>
>>> -Mukesh
>>>
>>>>
>>>>
>>>> --srini
>>>>
>>>>> -    }
>>>>> -
>>>>> -    /* Get subsystem table of contents using the minidump id */
>>>>> -    subsystem = &toc->subsystems[minidump_id];
>>>>>       /**
>>>>>        * Collect minidump if SS ToC is valid and segment table

Re: [PATCH v2 2/6] remoteproc: qcom: Move minidump specific data to qcom_minidump.h

[Mukesh Ojha]

On 4/14/2023 4:10 PM, Srinivas Kandagatla wrote:
> 
> 
> On 14/04/2023 08:05, Mukesh Ojha wrote:
>> Thanks again for coming back on this.
>>
>> On 4/14/2023 4:02 AM, Srinivas Kandagatla wrote:
>>>
>>>
>>> On 22/03/2023 13:30, Mukesh Ojha wrote:
>>>> Move minidump specific data types and macros to a separate internal
>>>> header(qcom_minidump.h) so that it can be shared among different
>>>
>>> minidump.h should be good as we are already in include/soc/qcom/
>>
>>
>> Initially, i wanted to protect the content of qcom_minidump.h between 
>> qcom_minidump.c and qcom_common.c
>>
>> Ideally, here qcom_minidump.h should be supplier/provider header and can 
> 
> Am not sure if I understand the supplier concept correctly.
> AFAIU, we have a 2 sets of apis
> 
> 1. get hold of minidump descriptor based on minidump_id fro gtoc using 
> qcom_minidump_subsystem_desc(). Am assuming which ever driver uses this 
> api will set there segments and regions in there respective drivers.
> 
> 2. setting regions/segments in APSS minidump descriptors which are done 
> via qcom_minidump_region_register(). TBH this should be renamed to 
> qcom_apss_minidump_region_register().
> 
> mixing of thsee apis makes it bit confusing, specially we have these two 
> category of apis that deal with different things.
> 
> Does it make sense to spit and abstract them properly by doing?
> 
> 
> 1. minidump driver to deal with handling gtoc and providing descriptors 
> to the consumers like remoteproc or apss, This can probably live within 
> smem driver as loc for this support is very minimal and proabably rename 
> the api accordingly.
> 

> 2. apss_minidump driver to allow other qcom drivers to 
> register/unregister regions within apss minidump descriptor.
> 
> did I miss something?

No, you are correct with your understanding.

To your suggestion to split code and to keep 
qcom_minidump_subsystem_desc() live inside smem driver,

And how about the header qcom_minidump.h, should we keep it separate 
than the apss minidump client header minidump.h ?

Since, you kind of understand the driver now, do you think it is worth
to create platform device for minidump from smem driver, and
have a probe inside apss minidump driver to solve chicken and egg 
problem for the clients who comes before minidump and try to register 
itself without doing ready check and apss_minidump can note this client 
entry and only register this region once minidump probe success and then 
it can register all the noted clients in one go.

The reason to do this would be apss_minidump driver has no meaning 
without smem, and for this reason checking qcom_minidump_ready() would 
not be ideal for at least qcom clients and for core kernel may be.

--Mukesh
> 
> thanks,
> Srini
> 
>> be shared among above qcom_minidump.c and qcom_common.c but since they 
>> are not in same directory, moved it inside include/soc/qcom/ as 
>> separate header than consumer header minidump.h . >
>> -Mukesh
>>>
>>> --srini
>>>
>>>> Qualcomm drivers.
>>>>
>>>> There is no change in functional behavior after this.
>>>>
>>>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>>>> ---
>>>>   drivers/remoteproc/qcom_common.c | 56 
>>>> +---------------------------------
>>>>   include/soc/qcom/qcom_minidump.h | 66 
>>>> ++++++++++++++++++++++++++++++++++++++++
>>>>   2 files changed, 67 insertions(+), 55 deletions(-)
>>>>   create mode 100644 include/soc/qcom/qcom_minidump.h
>>>>
>>>> diff --git a/drivers/remoteproc/qcom_common.c 
>>>> b/drivers/remoteproc/qcom_common.c
>>>> index 805e525..88fc984 100644
>>>> --- a/drivers/remoteproc/qcom_common.c
>>>> +++ b/drivers/remoteproc/qcom_common.c
>>>> @@ -18,6 +18,7 @@
>>>>   #include <linux/slab.h>
>>>>   #include <linux/soc/qcom/mdt_loader.h>
>>>>   #include <linux/soc/qcom/smem.h>
>>>> +#include <soc/qcom/qcom_minidump.h>
>>>>   #include "remoteproc_internal.h"
>>>>   #include "qcom_common.h"
>>>> @@ -26,61 +27,6 @@
>>>>   #define to_smd_subdev(d) container_of(d, struct qcom_rproc_subdev, 
>>>> subdev)
>>>>   #define to_ssr_subdev(d) container_of(d, struct qcom_rproc_ssr, 
>>>> subdev)
>>>> -#define MAX_NUM_OF_SS           10
>>>> -#define MAX_REGION_NAME_LENGTH  16
>>>> -#define SBL_MINIDUMP_SMEM_ID    602
>>>> -#define MINIDUMP_REGION_VALID        ('V' << 24 | 'A' << 16 | 'L' 
>>>> << 8 | 'I' << 0)
>>>> -#define MINIDUMP_SS_ENCR_DONE        ('D' << 24 | 'O' << 16 | 'N' 
>>>> << 8 | 'E' << 0)
>>>> -#define MINIDUMP_SS_ENABLED        ('E' << 24 | 'N' << 16 | 'B' << 
>>>> 8 | 'L' << 0)
>>>> -
>>>> -/**
>>>> - * struct minidump_region - Minidump region
>>>> - * @name        : Name of the region to be dumped
>>>> - * @seq_num:        : Use to differentiate regions with same name.
>>>> - * @valid        : This entry to be dumped (if set to 1)
>>>> - * @address        : Physical address of region to be dumped
>>>> - * @size        : Size of the region
>>>> - */
>>>> -struct minidump_region {
>>>> -    char    name[MAX_REGION_NAME_LENGTH];
>>>> -    __le32    seq_num;
>>>> -    __le32    valid;
>>>> -    __le64    address;
>>>> -    __le64    size;
>>>> -};
>>>> -
>>>> -/**
>>>> - * struct minidump_subsystem - Subsystem's SMEM Table of content
>>>> - * @status : Subsystem toc init status
>>>> - * @enabled : if set to 1, this region would be copied during coredump
>>>> - * @encryption_status: Encryption status for this subsystem
>>>> - * @encryption_required : Decides to encrypt the subsystem regions 
>>>> or not
>>>> - * @region_count : Number of regions added in this subsystem toc
>>>> - * @regions_baseptr : regions base pointer of the subsystem
>>>> - */
>>>> -struct minidump_subsystem {
>>>> -    __le32    status;
>>>> -    __le32    enabled;
>>>> -    __le32    encryption_status;
>>>> -    __le32    encryption_required;
>>>> -    __le32    region_count;
>>>> -    __le64    regions_baseptr;
>>>> -};
>>>> -
>>>> -/**
>>>> - * struct minidump_global_toc - Global Table of Content
>>>> - * @status : Global Minidump init status
>>>> - * @md_revision : Minidump revision
>>>> - * @enabled : Minidump enable status
>>>> - * @subsystems : Array of subsystems toc
>>>> - */
>>>> -struct minidump_global_toc {
>>>> -    __le32                status;
>>>> -    __le32                md_revision;
>>>> -    __le32                enabled;
>>>> -    struct minidump_subsystem    subsystems[MAX_NUM_OF_SS];
>>>> -};
>>>> -
>>>>   struct qcom_ssr_subsystem {
>>>>       const char *name;
>>>>       struct srcu_notifier_head notifier_list;
>>>> diff --git a/include/soc/qcom/qcom_minidump.h 
>>>> b/include/soc/qcom/qcom_minidump.h
>>>> new file mode 100644
>>>> index 0000000..84c8605
>>>> --- /dev/null
>>>> +++ b/include/soc/qcom/qcom_minidump.h
>>>> @@ -0,0 +1,66 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>>> +/*
>>>> + * Qualcomm minidump shared data structures and macros
>>>> + *
>>>> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights 
>>>> reserved.
>>>> + */
>>>> +
>>>> +#ifndef _QCOM_MINIDUMP_H_
>>>> +#define _QCOM_MINIDUMP_H_
>>>> +
>>>> +#define MAX_NUM_OF_SS           10
>>>> +#define MAX_REGION_NAME_LENGTH  16
>>>> +#define SBL_MINIDUMP_SMEM_ID    602
>>>> +#define MINIDUMP_REGION_VALID        ('V' << 24 | 'A' << 16 | 'L' 
>>>> << 8 | 'I' << 0)
>>>> +#define MINIDUMP_SS_ENCR_DONE        ('D' << 24 | 'O' << 16 | 'N' 
>>>> << 8 | 'E' << 0)
>>>> +#define MINIDUMP_SS_ENABLED        ('E' << 24 | 'N' << 16 | 'B' << 
>>>> 8 | 'L' << 0)
>>>> +
>>>> +/**
>>>> + * struct minidump_region - Minidump region
>>>> + * @name        : Name of the region to be dumped
>>>> + * @seq_num:        : Use to differentiate regions with same name.
>>>> + * @valid        : This entry to be dumped (if set to 1)
>>>> + * @address        : Physical address of region to be dumped
>>>> + * @size        : Size of the region
>>>> + */
>>>> +struct minidump_region {
>>>> +    char    name[MAX_REGION_NAME_LENGTH];
>>>> +    __le32    seq_num;
>>>> +    __le32    valid;
>>>> +    __le64    address;
>>>> +    __le64    size;
>>>> +};
>>>> +
>>>> +/**
>>>> + * struct minidump_subsystem - Subsystem's SMEM Table of content
>>>> + * @status : Subsystem toc init status
>>>> + * @enabled : if set to 1, this region would be copied during coredump
>>>> + * @encryption_status: Encryption status for this subsystem
>>>> + * @encryption_required : Decides to encrypt the subsystem regions 
>>>> or not
>>>> + * @region_count : Number of regions added in this subsystem toc
>>>> + * @regions_baseptr : regions base pointer of the subsystem
>>>> + */
>>>> +struct minidump_subsystem {
>>>> +    __le32    status;
>>>> +    __le32    enabled;
>>>> +    __le32    encryption_status;
>>>> +    __le32    encryption_required;
>>>> +    __le32    region_count;
>>>> +    __le64    regions_baseptr;
>>>> +};
>>>> +
>>>> +/**
>>>> + * struct minidump_global_toc - Global Table of Content
>>>> + * @status : Global Minidump init status
>>>> + * @md_revision : Minidump revision
>>>> + * @enabled : Minidump enable status
>>>> + * @subsystems : Array of subsystems toc
>>>> + */
>>>> +struct minidump_global_toc {
>>>> +    __le32                status;
>>>> +    __le32                md_revision;
>>>> +    __le32                enabled;
>>>> +    struct minidump_subsystem    subsystems[MAX_NUM_OF_SS];
>>>> +};
>>>> +
>>>> +#endif  /* _QCOM_MINIDUMP_H_ */

Re: [PATCH v2 2/6] remoteproc: qcom: Move minidump specific data to qcom_minidump.h

[Srinivas Kandagatla]

On 17/04/2023 14:22, Mukesh Ojha wrote:
> 
> 
> On 4/14/2023 4:10 PM, Srinivas Kandagatla wrote:
>>
>>
>> On 14/04/2023 08:05, Mukesh Ojha wrote:
>>> Thanks again for coming back on this.
>>>
>>> On 4/14/2023 4:02 AM, Srinivas Kandagatla wrote:
>>>>
>>>>
>>>> On 22/03/2023 13:30, Mukesh Ojha wrote:
>>>>> Move minidump specific data types and macros to a separate internal
>>>>> header(qcom_minidump.h) so that it can be shared among different
>>>>
>>>> minidump.h should be good as we are already in include/soc/qcom/
>>>
>>>
>>> Initially, i wanted to protect the content of qcom_minidump.h between 
>>> qcom_minidump.c and qcom_common.c
>>>
>>> Ideally, here qcom_minidump.h should be supplier/provider header and can 
>>
>> Am not sure if I understand the supplier concept correctly.
>> AFAIU, we have a 2 sets of apis
>>
>> 1. get hold of minidump descriptor based on minidump_id fro gtoc using 
>> qcom_minidump_subsystem_desc(). Am assuming which ever driver uses 
>> this api will set there segments and regions in there respective drivers.
>>
>> 2. setting regions/segments in APSS minidump descriptors which are 
>> done via qcom_minidump_region_register(). TBH this should be renamed 
>> to qcom_apss_minidump_region_register().
>>
>> mixing of thsee apis makes it bit confusing, specially we have these 
>> two category of apis that deal with different things.
>>
>> Does it make sense to spit and abstract them properly by doing?
>>
>>
>> 1. minidump driver to deal with handling gtoc and providing 
>> descriptors to the consumers like remoteproc or apss, This can 
>> probably live within smem driver as loc for this support is very 
>> minimal and proabably rename the api accordingly.
>>
> 
>> 2. apss_minidump driver to allow other qcom drivers to 
>> register/unregister regions within apss minidump descriptor.
>>
>> did I miss something?
> 
> No, you are correct with your understanding.
> 
> To your suggestion to split code and to keep 
> qcom_minidump_subsystem_desc() live inside smem driver,
> 
> And how about the header qcom_minidump.h, should we keep it separate 
> than the apss minidump client header minidump.h ?

I tried to do what I suggested and it kinda looked okay w.r.t design but 
started to look like a bit overdo.

other thing that I tried like what you have in patches but creating a 
dedicated platform driver for minidump (along with apss minidump), just 
like other smem devices like socinfo..

This should also get rid of qcom_minidump_ready().

> 
> Since, you kind of understand the driver now, do you think it is worth
> to create platform device for minidump from smem driver, and
> have a probe inside apss minidump driver to solve chicken and egg 
> problem for the clients who comes before minidump and try to register 
> itself without doing ready check and apss_minidump can note this client 
> entry and only register this region once minidump probe success and then 
> it can register all the noted clients in one go.
yes, it is doable.

> 
> The reason to do this would be apss_minidump driver has no meaning 
> without smem, and for this reason checking qcom_minidump_ready() would 
> not be ideal for at least qcom clients and for core kernel may be.
> 

how about something like this:

-------------------->cut<-----------------------
diff --git a/drivers/soc/qcom/minidump.c b/drivers/soc/qcom/minidump.c
new file mode 100644
index 000000000000..4171054b268d
--- /dev/null
+++ b/drivers/soc/qcom/minidump.c
@@ -0,0 +1,100 @@
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/soc/qcom/smem.h>
+#include <linux/soc/qcom/minidump.h>
+
+...
+
+struct minidump {
+       struct minidump_global_toc      *md_gbl_toc;
+       struct minidump_subsystem       *md_apss_toc;
+       ...
+};
+
+struct minidump *__md;
+
+struct minidump_subsystem *qcom_minidump_subsystem_desc(unsigned int 
minidump_index)
+{
+       if (!__md)
+               return PTR_ERR(-EPROBE_DEFER);
+
+       return &__mdgtoc->subsystems[minidump_index];
+}
+EXPORT_SYMBOL(qcom_minidump_subsystem_desc);
+
+
+int qcom_minidump_apss_region_register(const struct 
qcom_minidump_region *region)
+{
+       if (!__mdgtoc)
+               return -EPROBE_DEFER;
+       ...
+}
+EXPORT_SYMBOL_GPL(qcom_minidump_apss_region_register);
+
+static int qcom_minidump_init_apss_subsystem(struct minidumd *md)
+{
+       struct minidump_subsystem *apsstoc;
+
+       apsstoc = qcom_minidump_subsystem_desc(MINIDUMP_APSS_DESC);
+       if (IS_ERR(apsstoc))
+               return -EINVAL;
+
+       md->md_regions = kcalloc(MAX_NUM_ENTRIES,
+                             sizeof(struct minidump_region), GFP_KERNEL);
+       if (!md->md_regions)
+               return -ENOMEM;
+
+       minidump.apss_toc = apsstoc;
+
+       ...
+
+       return 0;
+}
+static int qcom_minidump_probe(struct platform_device *pdev)
+{
+       struct minidump *md;
+       struct minidump_global_toc *mdgtoc;
+
+       md = devm_kzalloc(&pdev->dev, sizeof(*md), GFP_KERNEL);
+
+       ...
+       mdgtoc = qcom_smem_get(QCOM_SMEM_HOST_ANY, SBL_MINIDUMP_SMEM_ID, 
&size);
+       if (IS_ERR(mdgtoc)) {
+               dev_err(&pdev->dev, "Couldn't find minidump\n");
+               return ERR_PTR(mdgtoc);
+       }
+
+       if (size < sizeof(*mdgtoc) || !mdgtoc->status) {
+               dev_err(&pdev->dev, "Minidump table is not initialized\n");
+               return ERR_PTR(-EINVAL);
+       }
+       ...
+       qcom_minidump_init_apss_subsystem(md);
+
+       __md = md;
+       ...
+       return 0;
+}
+
+static int qcom_minidump_remove(struct platform_device *pdev)
+{
+       struct qcom_minidump *qs = platform_get_drvdata(pdev);
+
+       ...
+
+       return 0;
+}
+
+static struct platform_driver qcom_minidump_driver = {
+       .probe = qcom_minidump_probe,
+       .remove = qcom_minidump_remove,
+       .driver  = {
+               .name = "qcom-minidump",
+       },
+};
+
+module_platform_driver(qcom_minidump_driver);
+
+MODULE_DESCRIPTION("Qualcomm minidump driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:qcom-minidump");
diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index 4f163d62942c..d54b07095ddd 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -279,6 +279,7 @@ struct qcom_smem {

         u32 item_count;
         struct platform_device *socinfo;
+       struct platform_device *minidump;
         struct smem_ptable *ptable;
         struct smem_partition global_partition;
         struct smem_partition partitions[SMEM_HOST_COUNT];
@@ -1151,12 +1152,19 @@ static int qcom_smem_probe(struct 
platform_device *pdev)
         if (IS_ERR(smem->socinfo))
                 dev_dbg(&pdev->dev, "failed to register socinfo device\n");

+       smem->minidump = platform_device_register_data(&pdev->dev, 
"qcom-minidump",
+ 
PLATFORM_DEVID_NONE, NULL,
+                                                     0);
+       if (IS_ERR(smem->minidump))
+               dev_dbg(&pdev->dev, "failed to register minidump device\n");
+
         return 0;
  }

  static int qcom_smem_remove(struct platform_device *pdev)
  {
         platform_device_unregister(__smem->socinfo);
+       platform_device_unregister(__smem->minidump);

         hwspin_lock_free(__smem->hwlock);
         __smem = NULL;
(END)
-------------------->cut<-----------------------

> --Mukesh
>>
>> thanks,
>> Srini
>>
>>> be shared among above qcom_minidump.c and qcom_common.c but since 
>>> they are not in same directory, moved it inside include/soc/qcom/ as 
>>> separate header than consumer header minidump.h . >
>>> -Mukesh
>>>>
>>>> --srini
>>>>
>>>>> Qualcomm drivers.
>>>>>
>>>>> There is no change in functional behavior after this.
>>>>>
>>>>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>>>>> ---
>>>>>   drivers/remoteproc/qcom_common.c | 56 
>>>>> +---------------------------------
>>>>>   include/soc/qcom/qcom_minidump.h | 66 
>>>>> ++++++++++++++++++++++++++++++++++++++++
>>>>>   2 files changed, 67 insertions(+), 55 deletions(-)
>>>>>   create mode 100644 include/soc/qcom/qcom_minidump.h
>>>>>
>>>>> diff --git a/drivers/remoteproc/qcom_common.c 
>>>>> b/drivers/remoteproc/qcom_common.c
>>>>> index 805e525..88fc984 100644
>>>>> --- a/drivers/remoteproc/qcom_common.c
>>>>> +++ b/drivers/remoteproc/qcom_common.c
>>>>> @@ -18,6 +18,7 @@
>>>>>   #include <linux/slab.h>
>>>>>   #include <linux/soc/qcom/mdt_loader.h>
>>>>>   #include <linux/soc/qcom/smem.h>
>>>>> +#include <soc/qcom/qcom_minidump.h>
>>>>>   #include "remoteproc_internal.h"
>>>>>   #include "qcom_common.h"
>>>>> @@ -26,61 +27,6 @@
>>>>>   #define to_smd_subdev(d) container_of(d, struct 
>>>>> qcom_rproc_subdev, subdev)
>>>>>   #define to_ssr_subdev(d) container_of(d, struct qcom_rproc_ssr, 
>>>>> subdev)
>>>>> -#define MAX_NUM_OF_SS           10
>>>>> -#define MAX_REGION_NAME_LENGTH  16
>>>>> -#define SBL_MINIDUMP_SMEM_ID    602
>>>>> -#define MINIDUMP_REGION_VALID        ('V' << 24 | 'A' << 16 | 'L' 
>>>>> << 8 | 'I' << 0)
>>>>> -#define MINIDUMP_SS_ENCR_DONE        ('D' << 24 | 'O' << 16 | 'N' 
>>>>> << 8 | 'E' << 0)
>>>>> -#define MINIDUMP_SS_ENABLED        ('E' << 24 | 'N' << 16 | 'B' << 
>>>>> 8 | 'L' << 0)
>>>>> -
>>>>> -/**
>>>>> - * struct minidump_region - Minidump region
>>>>> - * @name        : Name of the region to be dumped
>>>>> - * @seq_num:        : Use to differentiate regions with same name.
>>>>> - * @valid        : This entry to be dumped (if set to 1)
>>>>> - * @address        : Physical address of region to be dumped
>>>>> - * @size        : Size of the region
>>>>> - */
>>>>> -struct minidump_region {
>>>>> -    char    name[MAX_REGION_NAME_LENGTH];
>>>>> -    __le32    seq_num;
>>>>> -    __le32    valid;
>>>>> -    __le64    address;
>>>>> -    __le64    size;
>>>>> -};
>>>>> -
>>>>> -/**
>>>>> - * struct minidump_subsystem - Subsystem's SMEM Table of content
>>>>> - * @status : Subsystem toc init status
>>>>> - * @enabled : if set to 1, this region would be copied during 
>>>>> coredump
>>>>> - * @encryption_status: Encryption status for this subsystem
>>>>> - * @encryption_required : Decides to encrypt the subsystem regions 
>>>>> or not
>>>>> - * @region_count : Number of regions added in this subsystem toc
>>>>> - * @regions_baseptr : regions base pointer of the subsystem
>>>>> - */
>>>>> -struct minidump_subsystem {
>>>>> -    __le32    status;
>>>>> -    __le32    enabled;
>>>>> -    __le32    encryption_status;
>>>>> -    __le32    encryption_required;
>>>>> -    __le32    region_count;
>>>>> -    __le64    regions_baseptr;
>>>>> -};
>>>>> -
>>>>> -/**
>>>>> - * struct minidump_global_toc - Global Table of Content
>>>>> - * @status : Global Minidump init status
>>>>> - * @md_revision : Minidump revision
>>>>> - * @enabled : Minidump enable status
>>>>> - * @subsystems : Array of subsystems toc
>>>>> - */
>>>>> -struct minidump_global_toc {
>>>>> -    __le32                status;
>>>>> -    __le32                md_revision;
>>>>> -    __le32                enabled;
>>>>> -    struct minidump_subsystem    subsystems[MAX_NUM_OF_SS];
>>>>> -};
>>>>> -
>>>>>   struct qcom_ssr_subsystem {
>>>>>       const char *name;
>>>>>       struct srcu_notifier_head notifier_list;
>>>>> diff --git a/include/soc/qcom/qcom_minidump.h 
>>>>> b/include/soc/qcom/qcom_minidump.h
>>>>> new file mode 100644
>>>>> index 0000000..84c8605
>>>>> --- /dev/null
>>>>> +++ b/include/soc/qcom/qcom_minidump.h
>>>>> @@ -0,0 +1,66 @@
>>>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>>>> +/*
>>>>> + * Qualcomm minidump shared data structures and macros
>>>>> + *
>>>>> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights 
>>>>> reserved.
>>>>> + */
>>>>> +
>>>>> +#ifndef _QCOM_MINIDUMP_H_
>>>>> +#define _QCOM_MINIDUMP_H_
>>>>> +
>>>>> +#define MAX_NUM_OF_SS           10
>>>>> +#define MAX_REGION_NAME_LENGTH  16
>>>>> +#define SBL_MINIDUMP_SMEM_ID    602
>>>>> +#define MINIDUMP_REGION_VALID        ('V' << 24 | 'A' << 16 | 'L' 
>>>>> << 8 | 'I' << 0)
>>>>> +#define MINIDUMP_SS_ENCR_DONE        ('D' << 24 | 'O' << 16 | 'N' 
>>>>> << 8 | 'E' << 0)
>>>>> +#define MINIDUMP_SS_ENABLED        ('E' << 24 | 'N' << 16 | 'B' << 
>>>>> 8 | 'L' << 0)
>>>>> +
>>>>> +/**
>>>>> + * struct minidump_region - Minidump region
>>>>> + * @name        : Name of the region to be dumped
>>>>> + * @seq_num:        : Use to differentiate regions with same name.
>>>>> + * @valid        : This entry to be dumped (if set to 1)
>>>>> + * @address        : Physical address of region to be dumped
>>>>> + * @size        : Size of the region
>>>>> + */
>>>>> +struct minidump_region {
>>>>> +    char    name[MAX_REGION_NAME_LENGTH];
>>>>> +    __le32    seq_num;
>>>>> +    __le32    valid;
>>>>> +    __le64    address;
>>>>> +    __le64    size;
>>>>> +};
>>>>> +
>>>>> +/**
>>>>> + * struct minidump_subsystem - Subsystem's SMEM Table of content
>>>>> + * @status : Subsystem toc init status
>>>>> + * @enabled : if set to 1, this region would be copied during 
>>>>> coredump
>>>>> + * @encryption_status: Encryption status for this subsystem
>>>>> + * @encryption_required : Decides to encrypt the subsystem regions 
>>>>> or not
>>>>> + * @region_count : Number of regions added in this subsystem toc
>>>>> + * @regions_baseptr : regions base pointer of the subsystem
>>>>> + */
>>>>> +struct minidump_subsystem {
>>>>> +    __le32    status;
>>>>> +    __le32    enabled;
>>>>> +    __le32    encryption_status;
>>>>> +    __le32    encryption_required;
>>>>> +    __le32    region_count;
>>>>> +    __le64    regions_baseptr;
>>>>> +};
>>>>> +
>>>>> +/**
>>>>> + * struct minidump_global_toc - Global Table of Content
>>>>> + * @status : Global Minidump init status
>>>>> + * @md_revision : Minidump revision
>>>>> + * @enabled : Minidump enable status
>>>>> + * @subsystems : Array of subsystems toc
>>>>> + */
>>>>> +struct minidump_global_toc {
>>>>> +    __le32                status;
>>>>> +    __le32                md_revision;
>>>>> +    __le32                enabled;
>>>>> +    struct minidump_subsystem    subsystems[MAX_NUM_OF_SS];
>>>>> +};
>>>>> +
>>>>> +#endif  /* _QCOM_MINIDUMP_H_ */

Re: [PATCH v2 2/6] remoteproc: qcom: Move minidump specific data to qcom_minidump.h

[Mukesh Ojha]

On 4/18/2023 7:32 PM, Srinivas Kandagatla wrote:
> 
> 
> On 17/04/2023 14:22, Mukesh Ojha wrote:
>>
>>
>> On 4/14/2023 4:10 PM, Srinivas Kandagatla wrote:
>>>
>>>
>>> On 14/04/2023 08:05, Mukesh Ojha wrote:
>>>> Thanks again for coming back on this.
>>>>
>>>> On 4/14/2023 4:02 AM, Srinivas Kandagatla wrote:
>>>>>
>>>>>
>>>>> On 22/03/2023 13:30, Mukesh Ojha wrote:
>>>>>> Move minidump specific data types and macros to a separate internal
>>>>>> header(qcom_minidump.h) so that it can be shared among different
>>>>>
>>>>> minidump.h should be good as we are already in include/soc/qcom/
>>>>
>>>>
>>>> Initially, i wanted to protect the content of qcom_minidump.h 
>>>> between qcom_minidump.c and qcom_common.c
>>>>
>>>> Ideally, here qcom_minidump.h should be supplier/provider header and 
>>>> can 
>>>
>>> Am not sure if I understand the supplier concept correctly.
>>> AFAIU, we have a 2 sets of apis
>>>
>>> 1. get hold of minidump descriptor based on minidump_id fro gtoc 
>>> using qcom_minidump_subsystem_desc(). Am assuming which ever driver 
>>> uses this api will set there segments and regions in there respective 
>>> drivers.
>>>
>>> 2. setting regions/segments in APSS minidump descriptors which are 
>>> done via qcom_minidump_region_register(). TBH this should be renamed 
>>> to qcom_apss_minidump_region_register().
>>>
>>> mixing of thsee apis makes it bit confusing, specially we have these 
>>> two category of apis that deal with different things.
>>>
>>> Does it make sense to spit and abstract them properly by doing?
>>>
>>>
>>> 1. minidump driver to deal with handling gtoc and providing 
>>> descriptors to the consumers like remoteproc or apss, This can 
>>> probably live within smem driver as loc for this support is very 
>>> minimal and proabably rename the api accordingly.
>>>
>>
>>> 2. apss_minidump driver to allow other qcom drivers to 
>>> register/unregister regions within apss minidump descriptor.
>>>
>>> did I miss something?
>>
>> No, you are correct with your understanding.
>>
>> To your suggestion to split code and to keep 
>> qcom_minidump_subsystem_desc() live inside smem driver,
>>
>> And how about the header qcom_minidump.h, should we keep it separate 
>> than the apss minidump client header minidump.h ?
> 
> I tried to do what I suggested and it kinda looked okay w.r.t design but 
> started to look like a bit overdo.
> 
> other thing that I tried like what you have in patches but creating a 
> dedicated platform driver for minidump (along with apss minidump), just 
> like other smem devices like socinfo..
> 
> This should also get rid of qcom_minidump_ready().

This is exactly i have said below .

> 
>>
>> Since, you kind of understand the driver now, do you think it is worth
>> to create platform device for minidump from smem driver, and
>> have a probe inside apss minidump driver to solve chicken and egg 
>> problem for the clients who comes before minidump and try to register 
>> itself without doing ready check and apss_minidump can note this 
>> client entry and only register this region once minidump probe success 
>> and then it can register all the noted clients in one go.
> yes, it is doable.
> 
>>
>> The reason to do this would be apss_minidump driver has no meaning 
>> without smem, and for this reason checking qcom_minidump_ready() would 
>> not be ideal for at least qcom clients and for core kernel may be.
>>
> 
> how about something like this:

We are totally in sync, Thanks for your time spent on the snippet.

-Mukesh
> 
> -------------------->cut<-----------------------
> diff --git a/drivers/soc/qcom/minidump.c b/drivers/soc/qcom/minidump.c
> new file mode 100644
> index 000000000000..4171054b268d
> --- /dev/null
> +++ b/drivers/soc/qcom/minidump.c
> @@ -0,0 +1,100 @@
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/soc/qcom/smem.h>
> +#include <linux/soc/qcom/minidump.h>
> +
> +...
> +
> +struct minidump {
> +       struct minidump_global_toc      *md_gbl_toc;
> +       struct minidump_subsystem       *md_apss_toc;
> +       ...
> +};
> +
> +struct minidump *__md;
> +
> +struct minidump_subsystem *qcom_minidump_subsystem_desc(unsigned int 
> minidump_index)
> +{
> +       if (!__md)
> +               return PTR_ERR(-EPROBE_DEFER);
> +
> +       return &__mdgtoc->subsystems[minidump_index];
> +}
> +EXPORT_SYMBOL(qcom_minidump_subsystem_desc);
> +
> +
> +int qcom_minidump_apss_region_register(const struct 
> qcom_minidump_region *region)
> +{
> +       if (!__mdgtoc)
> +               return -EPROBE_DEFER;
> +       ...
> +}
> +EXPORT_SYMBOL_GPL(qcom_minidump_apss_region_register);
> +
> +static int qcom_minidump_init_apss_subsystem(struct minidumd *md)
> +{
> +       struct minidump_subsystem *apsstoc;
> +
> +       apsstoc = qcom_minidump_subsystem_desc(MINIDUMP_APSS_DESC);
> +       if (IS_ERR(apsstoc))
> +               return -EINVAL;
> +
> +       md->md_regions = kcalloc(MAX_NUM_ENTRIES,
> +                             sizeof(struct minidump_region), GFP_KERNEL);
> +       if (!md->md_regions)
> +               return -ENOMEM;
> +
> +       minidump.apss_toc = apsstoc;
> +
> +       ...
> +
> +       return 0;
> +}
> +static int qcom_minidump_probe(struct platform_device *pdev)
> +{
> +       struct minidump *md;
> +       struct minidump_global_toc *mdgtoc;
> +
> +       md = devm_kzalloc(&pdev->dev, sizeof(*md), GFP_KERNEL);
> +
> +       ...
> +       mdgtoc = qcom_smem_get(QCOM_SMEM_HOST_ANY, SBL_MINIDUMP_SMEM_ID, 
> &size);
> +       if (IS_ERR(mdgtoc)) {
> +               dev_err(&pdev->dev, "Couldn't find minidump\n");
> +               return ERR_PTR(mdgtoc);
> +       }
> +
> +       if (size < sizeof(*mdgtoc) || !mdgtoc->status) {
> +               dev_err(&pdev->dev, "Minidump table is not initialized\n");
> +               return ERR_PTR(-EINVAL);
> +       }
> +       ...
> +       qcom_minidump_init_apss_subsystem(md);
> +
> +       __md = md;
> +       ...
> +       return 0;
> +}
> +
> +static int qcom_minidump_remove(struct platform_device *pdev)
> +{
> +       struct qcom_minidump *qs = platform_get_drvdata(pdev);
> +
> +       ...
> +
> +       return 0;
> +}
> +
> +static struct platform_driver qcom_minidump_driver = {
> +       .probe = qcom_minidump_probe,
> +       .remove = qcom_minidump_remove,
> +       .driver  = {
> +               .name = "qcom-minidump",
> +       },
> +};
> +
> +module_platform_driver(qcom_minidump_driver);
> +
> +MODULE_DESCRIPTION("Qualcomm minidump driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:qcom-minidump");
> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> index 4f163d62942c..d54b07095ddd 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -279,6 +279,7 @@ struct qcom_smem {
> 
>          u32 item_count;
>          struct platform_device *socinfo;
> +       struct platform_device *minidump;
>          struct smem_ptable *ptable;
>          struct smem_partition global_partition;
>          struct smem_partition partitions[SMEM_HOST_COUNT];
> @@ -1151,12 +1152,19 @@ static int qcom_smem_probe(struct 
> platform_device *pdev)
>          if (IS_ERR(smem->socinfo))
>                  dev_dbg(&pdev->dev, "failed to register socinfo 
> device\n");
> 
> +       smem->minidump = platform_device_register_data(&pdev->dev, 
> "qcom-minidump",
> + PLATFORM_DEVID_NONE, NULL,
> +                                                     0);
> +       if (IS_ERR(smem->minidump))
> +               dev_dbg(&pdev->dev, "failed to register minidump 
> device\n");
> +
>          return 0;
>   }
> 
>   static int qcom_smem_remove(struct platform_device *pdev)
>   {
>          platform_device_unregister(__smem->socinfo);
> +       platform_device_unregister(__smem->minidump);
> 
>          hwspin_lock_free(__smem->hwlock);
>          __smem = NULL;
> (END)
> -------------------->cut<-----------------------
> 
>> --Mukesh
>>>
>>> thanks,
>>> Srini
>>>
>>>> be shared among above qcom_minidump.c and qcom_common.c but since 
>>>> they are not in same directory, moved it inside include/soc/qcom/ as 
>>>> separate header than consumer header minidump.h . >
>>>> -Mukesh
>>>>>
>>>>> --srini
>>>>>
>>>>>> Qualcomm drivers.
>>>>>>
>>>>>> There is no change in functional behavior after this.
>>>>>>
>>>>>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>>>>>> ---
>>>>>>   drivers/remoteproc/qcom_common.c | 56 
>>>>>> +---------------------------------
>>>>>>   include/soc/qcom/qcom_minidump.h | 66 
>>>>>> ++++++++++++++++++++++++++++++++++++++++
>>>>>>   2 files changed, 67 insertions(+), 55 deletions(-)
>>>>>>   create mode 100644 include/soc/qcom/qcom_minidump.h
>>>>>>
>>>>>> diff --git a/drivers/remoteproc/qcom_common.c 
>>>>>> b/drivers/remoteproc/qcom_common.c
>>>>>> index 805e525..88fc984 100644
>>>>>> --- a/drivers/remoteproc/qcom_common.c
>>>>>> +++ b/drivers/remoteproc/qcom_common.c
>>>>>> @@ -18,6 +18,7 @@
>>>>>>   #include <linux/slab.h>
>>>>>>   #include <linux/soc/qcom/mdt_loader.h>
>>>>>>   #include <linux/soc/qcom/smem.h>
>>>>>> +#include <soc/qcom/qcom_minidump.h>
>>>>>>   #include "remoteproc_internal.h"
>>>>>>   #include "qcom_common.h"
>>>>>> @@ -26,61 +27,6 @@
>>>>>>   #define to_smd_subdev(d) container_of(d, struct 
>>>>>> qcom_rproc_subdev, subdev)
>>>>>>   #define to_ssr_subdev(d) container_of(d, struct qcom_rproc_ssr, 
>>>>>> subdev)
>>>>>> -#define MAX_NUM_OF_SS           10
>>>>>> -#define MAX_REGION_NAME_LENGTH  16
>>>>>> -#define SBL_MINIDUMP_SMEM_ID    602
>>>>>> -#define MINIDUMP_REGION_VALID        ('V' << 24 | 'A' << 16 | 'L' 
>>>>>> << 8 | 'I' << 0)
>>>>>> -#define MINIDUMP_SS_ENCR_DONE        ('D' << 24 | 'O' << 16 | 'N' 
>>>>>> << 8 | 'E' << 0)
>>>>>> -#define MINIDUMP_SS_ENABLED        ('E' << 24 | 'N' << 16 | 'B' 
>>>>>> << 8 | 'L' << 0)
>>>>>> -
>>>>>> -/**
>>>>>> - * struct minidump_region - Minidump region
>>>>>> - * @name        : Name of the region to be dumped
>>>>>> - * @seq_num:        : Use to differentiate regions with same name.
>>>>>> - * @valid        : This entry to be dumped (if set to 1)
>>>>>> - * @address        : Physical address of region to be dumped
>>>>>> - * @size        : Size of the region
>>>>>> - */
>>>>>> -struct minidump_region {
>>>>>> -    char    name[MAX_REGION_NAME_LENGTH];
>>>>>> -    __le32    seq_num;
>>>>>> -    __le32    valid;
>>>>>> -    __le64    address;
>>>>>> -    __le64    size;
>>>>>> -};
>>>>>> -
>>>>>> -/**
>>>>>> - * struct minidump_subsystem - Subsystem's SMEM Table of content
>>>>>> - * @status : Subsystem toc init status
>>>>>> - * @enabled : if set to 1, this region would be copied during 
>>>>>> coredump
>>>>>> - * @encryption_status: Encryption status for this subsystem
>>>>>> - * @encryption_required : Decides to encrypt the subsystem 
>>>>>> regions or not
>>>>>> - * @region_count : Number of regions added in this subsystem toc
>>>>>> - * @regions_baseptr : regions base pointer of the subsystem
>>>>>> - */
>>>>>> -struct minidump_subsystem {
>>>>>> -    __le32    status;
>>>>>> -    __le32    enabled;
>>>>>> -    __le32    encryption_status;
>>>>>> -    __le32    encryption_required;
>>>>>> -    __le32    region_count;
>>>>>> -    __le64    regions_baseptr;
>>>>>> -};
>>>>>> -
>>>>>> -/**
>>>>>> - * struct minidump_global_toc - Global Table of Content
>>>>>> - * @status : Global Minidump init status
>>>>>> - * @md_revision : Minidump revision
>>>>>> - * @enabled : Minidump enable status
>>>>>> - * @subsystems : Array of subsystems toc
>>>>>> - */
>>>>>> -struct minidump_global_toc {
>>>>>> -    __le32                status;
>>>>>> -    __le32                md_revision;
>>>>>> -    __le32                enabled;
>>>>>> -    struct minidump_subsystem    subsystems[MAX_NUM_OF_SS];
>>>>>> -};
>>>>>> -
>>>>>>   struct qcom_ssr_subsystem {
>>>>>>       const char *name;
>>>>>>       struct srcu_notifier_head notifier_list;
>>>>>> diff --git a/include/soc/qcom/qcom_minidump.h 
>>>>>> b/include/soc/qcom/qcom_minidump.h
>>>>>> new file mode 100644
>>>>>> index 0000000..84c8605
>>>>>> --- /dev/null
>>>>>> +++ b/include/soc/qcom/qcom_minidump.h
>>>>>> @@ -0,0 +1,66 @@
>>>>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>>>>> +/*
>>>>>> + * Qualcomm minidump shared data structures and macros
>>>>>> + *
>>>>>> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights 
>>>>>> reserved.
>>>>>> + */
>>>>>> +
>>>>>> +#ifndef _QCOM_MINIDUMP_H_
>>>>>> +#define _QCOM_MINIDUMP_H_
>>>>>> +
>>>>>> +#define MAX_NUM_OF_SS           10
>>>>>> +#define MAX_REGION_NAME_LENGTH  16
>>>>>> +#define SBL_MINIDUMP_SMEM_ID    602
>>>>>> +#define MINIDUMP_REGION_VALID        ('V' << 24 | 'A' << 16 | 'L' 
>>>>>> << 8 | 'I' << 0)
>>>>>> +#define MINIDUMP_SS_ENCR_DONE        ('D' << 24 | 'O' << 16 | 'N' 
>>>>>> << 8 | 'E' << 0)
>>>>>> +#define MINIDUMP_SS_ENABLED        ('E' << 24 | 'N' << 16 | 'B' 
>>>>>> << 8 | 'L' << 0)
>>>>>> +
>>>>>> +/**
>>>>>> + * struct minidump_region - Minidump region
>>>>>> + * @name        : Name of the region to be dumped
>>>>>> + * @seq_num:        : Use to differentiate regions with same name.
>>>>>> + * @valid        : This entry to be dumped (if set to 1)
>>>>>> + * @address        : Physical address of region to be dumped
>>>>>> + * @size        : Size of the region
>>>>>> + */
>>>>>> +struct minidump_region {
>>>>>> +    char    name[MAX_REGION_NAME_LENGTH];
>>>>>> +    __le32    seq_num;
>>>>>> +    __le32    valid;
>>>>>> +    __le64    address;
>>>>>> +    __le64    size;
>>>>>> +};
>>>>>> +
>>>>>> +/**
>>>>>> + * struct minidump_subsystem - Subsystem's SMEM Table of content
>>>>>> + * @status : Subsystem toc init status
>>>>>> + * @enabled : if set to 1, this region would be copied during 
>>>>>> coredump
>>>>>> + * @encryption_status: Encryption status for this subsystem
>>>>>> + * @encryption_required : Decides to encrypt the subsystem 
>>>>>> regions or not
>>>>>> + * @region_count : Number of regions added in this subsystem toc
>>>>>> + * @regions_baseptr : regions base pointer of the subsystem
>>>>>> + */
>>>>>> +struct minidump_subsystem {
>>>>>> +    __le32    status;
>>>>>> +    __le32    enabled;
>>>>>> +    __le32    encryption_status;
>>>>>> +    __le32    encryption_required;
>>>>>> +    __le32    region_count;
>>>>>> +    __le64    regions_baseptr;
>>>>>> +};
>>>>>> +
>>>>>> +/**
>>>>>> + * struct minidump_global_toc - Global Table of Content
>>>>>> + * @status : Global Minidump init status
>>>>>> + * @md_revision : Minidump revision
>>>>>> + * @enabled : Minidump enable status
>>>>>> + * @subsystems : Array of subsystems toc
>>>>>> + */
>>>>>> +struct minidump_global_toc {
>>>>>> +    __le32                status;
>>>>>> +    __le32                md_revision;
>>>>>> +    __le32                enabled;
>>>>>> +    struct minidump_subsystem    subsystems[MAX_NUM_OF_SS];
>>>>>> +};
>>>>>> +
>>>>>> +#endif  /* _QCOM_MINIDUMP_H_ */

Re: [PATCH v2 3/6] docs: qcom: Add qualcomm minidump guide

[Mukesh Ojha]

+@Brian

On 4/14/2023 4:01 AM, Srinivas Kandagatla wrote:
> 
> 
> On 22/03/2023 13:30, Mukesh Ojha wrote:
>> +Dump collection
>> +---------------
>> +
>> +The solution supports extracting the minidump produced either over 
>> USB or
>> +stored to an attached storage device.
>> +
>> +By default, dumps are downloaded via USB to the attached x86_64 machine
>> +running PCAT (Qualcomm tool) software. Upon download, we will see
> 
> Are these both PCAT and dexter tools public?

I think, PCAT comes as part of Qcom Package Kit.

Last time, I checked with @Brian, he was saying the they use PCAT 
software tool running on x86_64 machine attached to QCOM device to
get the dump(via USB) out of the device.

Dexter.exe seems private tool, that only requires if we use storage
(via ufs/emmc) to save minidump on the target device itself and later 
use adb to pull out the rawdump partition dump and pass it through
dexter to convert it to same binary blobs which we got directly through
PCAT.

I don't at least have any way to avoid dexter tool at the moment.
However, i will think if we can develop any script which does the
same.

-- Mukesh

> 
> --srini
>> +a set of binary blobs starts with name md_* in PCAT configured directory
>> +in x86_64 machine, so for above example from the client it will be
>> +md_REGION_A.BIN. This binary blob depends on region content to determine
>> +whether it needs external parser support to get the content of the 
>> region,
>> +so for simple plain ASCII text we don't need any parsing and the content
>> +can be seen just opening the binary file.
>> +
>> +To collect the dump to attached storage type, one need to write 
>> appropriate
>> +value to IMEM register, in that case dumps are collected in rawdump
>> +partition on the target device itself.
>> +
>> +One need to read the entire rawdump partition and pull out content to
>> +save it onto the attached x86_64 machine over USB. Later, this rawdump
>> +can be pass it to another tool dexter.exe(Qualcomm tool) which converts
>> +this into the similar binary blobs which we have got it when download 
>> type
>> +was set to USB i.e a set of registered region as blobs and their name
>> +starts with md_*.
>> -- 2.7.4

Re: [PATCH v2 3/6] docs: qcom: Add qualcomm minidump guide

[Srinivas Kandagatla]

On 18/04/2023 16:19, Mukesh Ojha wrote:
> +@Brian
> 
> On 4/14/2023 4:01 AM, Srinivas Kandagatla wrote:
>>
>>
>> On 22/03/2023 13:30, Mukesh Ojha wrote:
>>> +Dump collection
>>> +---------------
>>> +
>>> +The solution supports extracting the minidump produced either over 
>>> USB or
>>> +stored to an attached storage device.
>>> +
>>> +By default, dumps are downloaded via USB to the attached x86_64 machine
>>> +running PCAT (Qualcomm tool) software. Upon download, we will see
>>
>> Are these both PCAT and dexter tools public?
> 
> I think, PCAT comes as part of Qcom Package Kit.

yes, this is part of Qualcomm Package Manager.

> 
> Last time, I checked with @Brian, he was saying the they use PCAT 
> software tool running on x86_64 machine attached to QCOM device to
> get the dump(via USB) out of the device.
> 
> Dexter.exe seems private tool, that only requires if we use storage
> (via ufs/emmc) to save minidump on the target device itself and later 
> use adb to pull out the rawdump partition dump and pass it through
> dexter to convert it to same binary blobs which we got directly through
> PCAT.
> 
> I don't at least have any way to avoid dexter tool at the moment.
> However, i will think if we can develop any script which does the
> same.
That would be nice!

--srini
> 
> -- Mukesh
> 
>>
>> --srini
>>> +a set of binary blobs starts with name md_* in PCAT configured 
>>> directory
>>> +in x86_64 machine, so for above example from the client it will be
>>> +md_REGION_A.BIN. This binary blob depends on region content to 
>>> determine
>>> +whether it needs external parser support to get the content of the 
>>> region,
>>> +so for simple plain ASCII text we don't need any parsing and the 
>>> content
>>> +can be seen just opening the binary file.
>>> +
>>> +To collect the dump to attached storage type, one need to write 
>>> appropriate
>>> +value to IMEM register, in that case dumps are collected in rawdump
>>> +partition on the target device itself.
>>> +
>>> +One need to read the entire rawdump partition and pull out content to
>>> +save it onto the attached x86_64 machine over USB. Later, this rawdump
>>> +can be pass it to another tool dexter.exe(Qualcomm tool) which converts
>>> +this into the similar binary blobs which we have got it when 
>>> download type
>>> +was set to USB i.e a set of registered region as blobs and their name
>>> +starts with md_*.
>>> -- 2.7.4