[PATCH v5 00/17] Add Qualcomm Minidump kernel driver related support

[PATCH v5 00/17] Add Qualcomm Minidump kernel driver related support

[Mukesh Ojha]

Hi All,

This is to continuation from the conversation happened at v4

https://lore.kernel.org/lkml/632c5b97-4a91-c3e8-1e6c-33d6c4f6454f@quicinc.com/

https://lore.kernel.org/lkml/695133e6-105f-de2a-5559-555cea0a0462@quicinc.com/

We have put abstract on LPC on this topic as well as initiated a mail thread
with other SoC vendors but did not get much traction on it.

https://lore.kernel.org/lkml/0199db00-1b1d-0c63-58ff-03efae02cb21@quicinc.com/

We explored most of possiblity present in kernel to address this issue[1] but
solution like kdump/fadump does not seems safe/secure/performant from our
perspective.

Hence, with this series we tried to make the minidump kernel driver, simple
and tied with pstore frontends, so that it collects the present available
frontends data like dmesg, ftrace, pmsg, ftrace., Also, we will be working
towards enhancing generic pstore to capture more debug data which will be
helpful for first hand of debugging that can benefit both other pstore users
as well as us as minidump users.

One of the proposal made here,
https://lore.kernel.org/lkml/1683561060-2197-1-git-send-email-quic_mojha@quicinc.com/

Looking forward for your comments.

Thanks,
Mukesh

[1]
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 SMEM based 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 SMEM
based 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.

Changes in v5:
 - On suggestion from Pavan.k, to have single function call for minidump collection
   from remoteproc driver, separated the logic to have separate minidump file called
   qcom_rproc_minidump.c and also renamed the function from qcom_minidump() to 
   qcom_rproc_minidump(); however, dropped his suggestion about rework on lazy deletion
   during region unregister in this series, will pursue it in next series.

 - To simplify the minidump driver, removed the complication for frontend and different
   backend from Greg suggestion, will pursue this once main driver gets mainlined.

 - Move the dynamic ramoops region allocation from Device tree approach to command line
   approch with the introduction command line parsing and memblock reservation during
   early boot up; Not added documentation about it yet, will add if it gets positive
   response.

 - Exporting linux banner from kernel to make minidump build also as module, however,
   minidump is a debug module and should be kernel built to get most debug information
   from kernel.

 - Tried to address comments given on dload patch series. 

Changes in v4: https://lore.kernel.org/lkml/1687955688-20809-1-git-send-email-quic_mojha@quicinc.com/
 - Redesigned the driver and divided the driver into front end and backend (smem) so
   that any new backend can be attached easily to avoid code duplication.
 - Patch reordering as per the driver and subsystem to easier review of the code.
 - Removed minidump specific code from remoteproc to minidump smem based driver.
 - Enabled the all the driver as modules.
 - Address comments made on documentation and yaml and Device tree file [Krzysztof/Konrad]
 - Address comments made qcom_pstore_minidump driver and given its Device tree
   same set of properties as ramoops. [Luca/Kees]
 - Added patch for MAINTAINER file.
 - Include defconfig change as one patch as per [Krzysztof] suggestion.
 - Tried to remove the redundant file scope variables from the module as per [Krzysztof] suggestion.
 - Addressed comments made on dload mode patch v6 version
   https://lore.kernel.org/lkml/1680076012-10785-1-git-send-email-quic_mojha@quicinc.com/

Changes in v3: https://lore.kernel.org/lkml/1683133352-10046-1-git-send-email-quic_mojha@quicinc.com/
 - Addressed most of the comments by Srini on v2 and refactored the minidump driver.
    - Added platform device support
    - Unregister region support.
 - Added update region for clients.
 - Added pending region support.
 - Modified the documentation guide accordingly.
 - Added qcom_pstore_ramdump client driver which happen to add ramoops platform
   device and also registers ramoops region with minidump.
 - Added download mode patch series with this minidump series.
    https://lore.kernel.org/lkml/1680076012-10785-1-git-send-email-quic_mojha@quicinc.com/

Changes in v2: https://lore.kernel.org/lkml/1679491817-2498-1-git-send-email-quic_mojha@quicinc.com/
 - 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/

Testing of the patches has been done on sm8450 target after enabling config like
CONFIG_PSTORE_RAM and CONFIG_PSTORE_CONSOLE and once the device boots up.

 echo mini > /sys/module/qcom_scm/parameters/download_mode

Try crashing it via devmem2 0xf11c000(this is known to create xpu violation and
and put the device in download mode) on command prompt.

Default storage type is set to via USB, so minidump would be downloaded with the
help of x86_64 machine (running PCAT tool) attached to Qualcomm device which has
backed minidump boot firmware support.

This will make the device go to download mode and collect the minidump on to the
attached x86 machine running the Qualcomm PCAT tool(This comes as part Qualcomm
package manager kit).

After that we will see a bunch of predefined registered region as binary blobs files
starts with md_* downloaded on the x86 machine on given location in PCAT tool from
the target device, more about this can be found in qualcomm minidump guide patch.

Mukesh Ojha (17):
  docs: qcom: Add qualcomm minidump guide
  soc: qcom: Add qcom_rproc_minidump module
  remoteproc: qcom_q6v5_pas: Use qcom_rproc_minidump()
  remoteproc: qcom: Remove minidump related data from qcom_common.c
  init: export linux_banner data variable
  soc: qcom: Add Qualcomm APSS minidump kernel driver
  soc: qcom: minidump: Add pending region registration
  arm64: mm: Add dynamic ramoops region support through command line
  pstore/ram: Use dynamic ramoops reserve resource
  pstore: Add pstore_region_defined() helper and export it
  qcom_minidump: Register ramoops region with minidump
  MAINTAINERS: Add entry for minidump related files
  firmware: qcom_scm: provide a read-modify-write function
  pinctrl: qcom: Use qcom_scm_io_update_field()
  firmware: scm: Modify only the download bits in TCSR register
  firmware: qcom_scm: Refactor code to support multiple download mode
  firmware: qcom_scm: Add multiple download mode support

 Documentation/admin-guide/index.rst         |   1 +
 Documentation/admin-guide/qcom_minidump.rst | 272 +++++++++++
 MAINTAINERS                                 |  10 +
 arch/arm64/mm/init.c                        |  94 ++++
 drivers/firmware/Kconfig                    |  11 -
 drivers/firmware/qcom_scm.c                 |  90 +++-
 drivers/pinctrl/qcom/pinctrl-msm.c          |  10 +-
 drivers/remoteproc/Kconfig                  |   1 +
 drivers/remoteproc/qcom_common.c            | 150 ------
 drivers/remoteproc/qcom_q6v5_pas.c          |   3 +-
 drivers/soc/qcom/Kconfig                    |  24 +
 drivers/soc/qcom/Makefile                   |   3 +
 drivers/soc/qcom/qcom_minidump.c            | 727 ++++++++++++++++++++++++++++
 drivers/soc/qcom/qcom_minidump_internal.h   |  74 +++
 drivers/soc/qcom/qcom_ramoops_minidump.c    |  88 ++++
 drivers/soc/qcom/qcom_ramoops_minidump.h    |  10 +
 drivers/soc/qcom/qcom_rproc_minidump.c      | 111 +++++
 drivers/soc/qcom/smem.c                     |  18 +
 fs/pstore/platform.c                        |  15 +
 fs/pstore/ram.c                             |  52 +-
 include/linux/firmware/qcom/qcom_scm.h      |   2 +
 include/linux/init.h                        |   3 +
 include/linux/pstore.h                      |   6 +
 include/linux/pstore_ram.h                  |   2 +
 include/linux/soc/qcom/smem.h               |   2 +
 include/soc/qcom/qcom_minidump.h            |  56 +++
 init/version-timestamp.c                    |   3 +
 27 files changed, 1659 insertions(+), 179 deletions(-)
 create mode 100644 Documentation/admin-guide/qcom_minidump.rst
 create mode 100644 drivers/soc/qcom/qcom_minidump.c
 create mode 100644 drivers/soc/qcom/qcom_minidump_internal.h
 create mode 100644 drivers/soc/qcom/qcom_ramoops_minidump.c
 create mode 100644 drivers/soc/qcom/qcom_ramoops_minidump.h
 create mode 100644 drivers/soc/qcom/qcom_rproc_minidump.c
 create mode 100644 include/soc/qcom/qcom_minidump.h

-- 
2.7.4

[PATCH v5 01/17] docs: qcom: Add qualcomm minidump guide

[Mukesh Ojha]

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

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

diff --git a/Documentation/admin-guide/index.rst b/Documentation/admin-guide/index.rst
index 43ea35613dfc..251d070486c2 100644
--- a/Documentation/admin-guide/index.rst
+++ b/Documentation/admin-guide/index.rst
@@ -120,6 +120,7 @@ configure specific aspects of kernel behavior to your liking.
    perf-security
    pm/index
    pnp
+   qcom_minidump
    rapidio
    ras
    rtc
diff --git a/Documentation/admin-guide/qcom_minidump.rst b/Documentation/admin-guide/qcom_minidump.rst
new file mode 100644
index 000000000000..20202da8ca40
--- /dev/null
+++ b/Documentation/admin-guide/qcom_minidump.rst
@@ -0,0 +1,272 @@
+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 RAM dumps 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 selected 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 contents
+       SS-ToC: Subsystem table of contents
+       SS0-SSn: Subsystem numbered from 0 to n
+
+It depends on SoC where the underlying firmware is keeping the
+minidump global table taking care of subsystem ToC part for
+minidump like for above diagram, it is for shared memory sitting
+in DDR and it is shared among various master however it is possible
+that this could be implemented via memory mapped regions but the
+general idea should remain same. Here, various subsystem could be
+DSP's like ADSP/CDSP/MODEM etc, along with Application processor
+(APSS) where Linux runs. DSP minidump gets collected when DSP's goes
+for recovery followed by a crash. The minidump part of code for
+that resides in ``qcom_rproc_minidump.c``.
+
+
+SMEM as backend
+----------------
+
+In this document, SMEM will be used as the backend implementation
+of minidump.
+
+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 contents (G-ToC). Each subsystem (APSS, ADSP, ...) has
+its 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 its size etc. and it
+could be anywhere scattered in the DDR.
+
+Qualcomm APSS Minidump kernel driver concept
+--------------------------------------------
+::
+
+Qualcomm APSS minidump kernel driver adds the capability to add Linux
+region to be dumped as part of RAM dump collection. At the moment,
+shared memory driver creates platform device for minidump driver and
+give a means to APSS minidump to initialize itself on probe.
+
+This driver provides ``qcom_minidump_region_register`` and
+``qcom_minidump_region_unregister`` API's to register and unregister
+APSS minidump region. It also supports registration for the clients
+who came before minidump driver was initialized. It maintains pending
+list of clients who came before minidump and once minidump is initialized
+it registers them in one go.
+
+To simplify post-mortem debugging, driver creates and maintain an ELF
+header as first region that gets updated each time a new region gets
+registered.
+
+The solution supports extracting the RAM dump/minidump produced either
+over USB or stored to an attached storage device.
+
+Dependency of minidump kernel driver
+------------------------------------
+
+It is to note that whole of minidump depends on Qualcomm boot firmware
+whether it supports minidump or not. So, if the minidump SMEM ID is
+present in shared memory, it indicates that minidump is supported from
+boot firmware and it is possible to dump Linux (APSS) region as part
+of minidump collection.
+
+How a kernel client driver can register region with minidump
+------------------------------------------------------------
+
+Client driver can use ``qcom_minidump_region_register`` API's to register
+and ``qcom_minidump_region_unregister`` to unregister their region from
+minidump driver.
+
+Client needs to fill their region by filling ``qcom_minidump_region``
+structure object which consists of the region name, region's virtual
+and physical address and its size.
+
+Below, is one sample client driver snippet which tries 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 it with minidump.
+
+ .. code-block:: c
+
+  #include <soc/qcom/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);
+
+  [... Fill up the region object ...]
+  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;
+
+  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 RAM dump (also called
+full dump) by 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 RAM dump will be collected while setting it ``"full,mini"``
+will collect minidump along with fulldump.
+
+Writing to sysfs node 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
+---------------
+
+	+-----------+
+	|           |
+	|           |         +------+
+	|           |         |      |
+	|           |         +--+---+ Product(Qualcomm SoC)
+	+-----------+             |
+	|+++++++++++|<------------+
+	|+++++++++++|    usb cable
+	+-----------+
+            x86_64 PC
+
+The solution supports a product running with Qualcomm SoC (where minidump)
+is supported from the firmware) connected to x86_64 host PC running PCAT
+tool. It supports downloading the minidump produced from product to the
+host PC over USB or to save the minidump to the product attached storage
+device(UFS/eMMC/SD Card) into minidump dedicated partition.
+
+By default, dumps are downloaded via USB to the attached x86_64 PC running
+PCAT (Qualcomm tool) software. Upon download, we will see a set of binary
+blobs starting 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 needs to write appropriate
+value to IMEM register, in that case dumps are collected in rawdump
+partition on the product device itself.
+
+One needs 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 passed 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 regions as blobs
+and their name starts with ``md_*``.
+
+Replacing the ``dexter.exe`` with some open source tool can be added as future
+scope of this document.
-- 
2.7.4

[PATCH v5 02/17] soc: qcom: Add qcom_rproc_minidump module

[Mukesh Ojha]

Add qcom_rproc_minidump module in a preparation to remove
minidump specific code from driver/remoteproc/qcom_common.c
and provide needed exported API, this as well helps to
abstract minidump specific data layout from qualcomm's
remoteproc driver.

It is just a copying of qcom_minidump() functionality from
driver/remoteproc/qcom_common.c into a separate file under
qcom_rproc_minidump().

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/soc/qcom/Kconfig                  |  10 +++
 drivers/soc/qcom/Makefile                 |   1 +
 drivers/soc/qcom/qcom_minidump_internal.h |  64 +++++++++++++++++
 drivers/soc/qcom/qcom_rproc_minidump.c    | 111 ++++++++++++++++++++++++++++++
 include/soc/qcom/qcom_minidump.h          |  23 +++++++
 5 files changed, 209 insertions(+)
 create mode 100644 drivers/soc/qcom/qcom_minidump_internal.h
 create mode 100644 drivers/soc/qcom/qcom_rproc_minidump.c
 create mode 100644 include/soc/qcom/qcom_minidump.h

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index e597799e8121..ff38deac6a7d 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -290,4 +290,14 @@ config QCOM_INLINE_CRYPTO_ENGINE
 	tristate
 	select QCOM_SCM
 
+config QCOM_RPROC_MINIDUMP
+	tristate "QCOM Remoteproc Minidump Support"
+	depends on ARCH_QCOM || COMPILE_TEST
+	depends on QCOM_SMEM
+	help
+	  Enablement of core minidump feature is controlled from boot firmware
+	  side, so if it is enabled from firmware, this config allow linux to
+	  query predefined minidump segments associated with the remote processor
+	  and check its validity and end up collecting the dump on remote processor
+	  crash during its recovery.
 endmenu
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 99114c71092b..a5fd2fed0923 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -35,3 +35,4 @@ obj-$(CONFIG_QCOM_KRYO_L2_ACCESSORS) +=	kryo-l2-accessors.o
 obj-$(CONFIG_QCOM_ICC_BWMON)	+= icc-bwmon.o
 qcom_ice-objs			+= ice.o
 obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE)	+= qcom_ice.o
+obj-$(CONFIG_QCOM_RPROC_MINIDUMP)	+= qcom_rproc_minidump.o
diff --git a/drivers/soc/qcom/qcom_minidump_internal.h b/drivers/soc/qcom/qcom_minidump_internal.h
new file mode 100644
index 000000000000..71709235b196
--- /dev/null
+++ b/drivers/soc/qcom/qcom_minidump_internal.h
@@ -0,0 +1,64 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#ifndef _QCOM_MINIDUMP_INTERNAL_H_
+#define _QCOM_MINIDUMP_INTERNAL_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_INTERNAL_H_ */
diff --git a/drivers/soc/qcom/qcom_rproc_minidump.c b/drivers/soc/qcom/qcom_rproc_minidump.c
new file mode 100644
index 000000000000..9bc84cc2536f
--- /dev/null
+++ b/drivers/soc/qcom/qcom_rproc_minidump.c
@@ -0,0 +1,111 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/remoteproc.h>
+#include <linux/soc/qcom/smem.h>
+#include <linux/string.h>
+#include <soc/qcom/qcom_minidump.h>
+
+#include "qcom_minidump_internal.h"
+
+static void qcom_minidump_cleanup(struct rproc *rproc)
+{
+	struct rproc_dump_segment *entry, *tmp;
+
+	list_for_each_entry_safe(entry, tmp, &rproc->dump_segments, node) {
+		list_del(&entry->node);
+		kfree(entry->priv);
+		kfree(entry);
+	}
+}
+
+static int qcom_add_minidump_segments(struct rproc *rproc, struct minidump_subsystem *subsystem,
+			void (*rproc_dumpfn_t)(struct rproc *rproc, struct rproc_dump_segment *segment,
+				void *dest, size_t offset, size_t size))
+{
+	struct minidump_region __iomem *ptr;
+	struct minidump_region region;
+	int seg_cnt, i;
+	dma_addr_t da;
+	size_t size;
+	char *name;
+
+	if (WARN_ON(!list_empty(&rproc->dump_segments))) {
+		dev_err(&rproc->dev, "dump segment list already populated\n");
+		return -EUCLEAN;
+	}
+
+	seg_cnt = le32_to_cpu(subsystem->region_count);
+	ptr = ioremap((unsigned long)le64_to_cpu(subsystem->regions_baseptr),
+		      seg_cnt * sizeof(struct minidump_region));
+	if (!ptr)
+		return -EFAULT;
+
+	for (i = 0; i < seg_cnt; i++) {
+		memcpy_fromio(&region, ptr + i, sizeof(region));
+		if (le32_to_cpu(region.valid) == MINIDUMP_REGION_VALID) {
+			name = kstrndup(region.name, MAX_REGION_NAME_LENGTH - 1, GFP_KERNEL);
+			if (!name) {
+				iounmap(ptr);
+				return -ENOMEM;
+			}
+			da = le64_to_cpu(region.address);
+			size = le64_to_cpu(region.size);
+			rproc_coredump_add_custom_segment(rproc, da, size, rproc_dumpfn_t, name);
+		}
+	}
+
+	iounmap(ptr);
+	return 0;
+}
+
+void qcom_rproc_minidump(struct rproc *rproc, unsigned int minidump_id,
+		void (*rproc_dumpfn_t)(struct rproc *rproc,
+		struct rproc_dump_segment *segment, void *dest, size_t offset,
+		size_t size))
+{
+	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");
+		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
+	 * 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;
+	}
+
+	ret = qcom_add_minidump_segments(rproc, subsystem, rproc_dumpfn_t);
+	if (ret) {
+		dev_err(&rproc->dev, "Failed with error: %d while adding minidump entries\n", ret);
+		goto clean_minidump;
+	}
+	rproc_coredump_using_sections(rproc);
+clean_minidump:
+	qcom_minidump_cleanup(rproc);
+}
+EXPORT_SYMBOL_GPL(qcom_rproc_minidump);
+
+MODULE_DESCRIPTION("Qualcomm remoteproc minidump(smem) helper module");
+MODULE_LICENSE("GPL");
diff --git a/include/soc/qcom/qcom_minidump.h b/include/soc/qcom/qcom_minidump.h
new file mode 100644
index 000000000000..cd87caef919d
--- /dev/null
+++ b/include/soc/qcom/qcom_minidump.h
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#ifndef _QCOM_MINIDUMP_H_
+#define _QCOM_MINIDUMP_H_
+
+struct rproc;
+struct rproc_dump_segment;
+
+#if IS_ENABLED(CONFIG_QCOM_RPROC_MINIDUMP)
+void qcom_rproc_minidump(struct rproc *rproc, unsigned int minidump_id,
+		   void (*rproc_dumpfn_t)(struct rproc *rproc,
+		   struct rproc_dump_segment *segment, void *dest, size_t offset,
+		   size_t size));
+#else
+static inline void qcom_rproc_minidump(struct rproc *rproc, unsigned int minidump_id,
+		   void (*rproc_dumpfn_t)(struct rproc *rproc,
+		   struct rproc_dump_segment *segment, void *dest, size_t offset,
+		   size_t size)) { }
+#endif /* CONFIG_QCOM_RPROC_MINIDUMP */
+#endif /* _QCOM_MINIDUMP_H_ */
-- 
2.7.4

[PATCH v5 03/17] remoteproc: qcom_q6v5_pas: Use qcom_rproc_minidump()

[Mukesh Ojha]

Now, as all the minidump specific data structure is moved to
minidump specific files and implementation wise qcom_rproc_minidump()
and qcom_minidump() exactly same and the name qcom_rproc_minidump
make more sense as it happen to collect the minidump for the
remoteproc processors. So, let's use qcom_rproc_minidump() and
we will be removing qcom_minidump() and minidump related stuff
from driver/remoteproc/qcom_common.c .

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/remoteproc/Kconfig         | 1 +
 drivers/remoteproc/qcom_q6v5_pas.c | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 48845dc8fa85..cea960749e2c 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -166,6 +166,7 @@ config QCOM_PIL_INFO
 
 config QCOM_RPROC_COMMON
 	tristate
+	select QCOM_RPROC_MINIDUMP
 
 config QCOM_Q6V5_COMMON
 	tristate
diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
index 3153d82037e7..f235daae84ff 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -24,6 +24,7 @@
 #include <linux/soc/qcom/mdt_loader.h>
 #include <linux/soc/qcom/smem.h>
 #include <linux/soc/qcom/smem_state.h>
+#include <soc/qcom/qcom_minidump.h>
 
 #include "qcom_common.h"
 #include "qcom_pil_info.h"
@@ -130,7 +131,7 @@ static void adsp_minidump(struct rproc *rproc)
 	if (rproc->dump_conf == RPROC_COREDUMP_DISABLED)
 		return;
 
-	qcom_minidump(rproc, adsp->minidump_id, adsp_segment_dump);
+	qcom_rproc_minidump(rproc, adsp->minidump_id, adsp_segment_dump);
 }
 
 static int adsp_pds_enable(struct qcom_adsp *adsp, struct device **pds,
-- 
2.7.4

[PATCH v5 04/17] remoteproc: qcom: Remove minidump related data from qcom_common.c

[Mukesh Ojha]

As minidump specific data structure and functions move under
config QCOM_RPROC_MINIDUMP, so remove minidump specific data
from driver/remoteproc/qcom_common.c .

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

diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
index a0d4238492e9..24dc3144b15a 100644
--- a/drivers/remoteproc/qcom_common.c
+++ b/drivers/remoteproc/qcom_common.c
@@ -26,61 +26,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 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)
-
-/**
- * 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;
@@ -90,101 +35,6 @@ struct qcom_ssr_subsystem {
 static LIST_HEAD(qcom_ssr_subsystem_list);
 static DEFINE_MUTEX(qcom_ssr_subsys_lock);
 
-static void qcom_minidump_cleanup(struct rproc *rproc)
-{
-	struct rproc_dump_segment *entry, *tmp;
-
-	list_for_each_entry_safe(entry, tmp, &rproc->dump_segments, node) {
-		list_del(&entry->node);
-		kfree(entry->priv);
-		kfree(entry);
-	}
-}
-
-static int qcom_add_minidump_segments(struct rproc *rproc, struct minidump_subsystem *subsystem,
-			void (*rproc_dumpfn_t)(struct rproc *rproc, struct rproc_dump_segment *segment,
-				void *dest, size_t offset, size_t size))
-{
-	struct minidump_region __iomem *ptr;
-	struct minidump_region region;
-	int seg_cnt, i;
-	dma_addr_t da;
-	size_t size;
-	char *name;
-
-	if (WARN_ON(!list_empty(&rproc->dump_segments))) {
-		dev_err(&rproc->dev, "dump segment list already populated\n");
-		return -EUCLEAN;
-	}
-
-	seg_cnt = le32_to_cpu(subsystem->region_count);
-	ptr = ioremap((unsigned long)le64_to_cpu(subsystem->regions_baseptr),
-		      seg_cnt * sizeof(struct minidump_region));
-	if (!ptr)
-		return -EFAULT;
-
-	for (i = 0; i < seg_cnt; i++) {
-		memcpy_fromio(&region, ptr + i, sizeof(region));
-		if (le32_to_cpu(region.valid) == MD_REGION_VALID) {
-			name = kstrndup(region.name, MAX_REGION_NAME_LENGTH - 1, GFP_KERNEL);
-			if (!name) {
-				iounmap(ptr);
-				return -ENOMEM;
-			}
-			da = le64_to_cpu(region.address);
-			size = le64_to_cpu(region.size);
-			rproc_coredump_add_custom_segment(rproc, da, size, rproc_dumpfn_t, name);
-		}
-	}
-
-	iounmap(ptr);
-	return 0;
-}
-
-void qcom_minidump(struct rproc *rproc, unsigned int minidump_id,
-		void (*rproc_dumpfn_t)(struct rproc *rproc,
-		struct rproc_dump_segment *segment, void *dest, size_t offset,
-		size_t size))
-{
-	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");
-		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
-	 * 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) != MD_SS_ENABLED ||
-	    le32_to_cpu(subsystem->encryption_status) != MD_SS_ENCR_DONE) {
-		dev_err(&rproc->dev, "Minidump not ready, skipping\n");
-		return;
-	}
-
-	ret = qcom_add_minidump_segments(rproc, subsystem, rproc_dumpfn_t);
-	if (ret) {
-		dev_err(&rproc->dev, "Failed with error: %d while adding minidump entries\n", ret);
-		goto clean_minidump;
-	}
-	rproc_coredump_using_sections(rproc);
-clean_minidump:
-	qcom_minidump_cleanup(rproc);
-}
-EXPORT_SYMBOL_GPL(qcom_minidump);
-
 static int glink_subdev_start(struct rproc_subdev *subdev)
 {
 	struct qcom_rproc_glink *glink = to_glink_subdev(subdev);
-- 
2.7.4

[PATCH v5 05/17] init: export linux_banner data variable

[Mukesh Ojha]

Some debug loadable module like minidump is interested in knowing
the kernel version against which it is being build. Let's export
linux_banner.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 include/linux/init.h     | 3 +++
 init/version-timestamp.c | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/include/linux/init.h b/include/linux/init.h
index 266c3e1640d4..1e5d03c2de68 100644
--- a/include/linux/init.h
+++ b/include/linux/init.h
@@ -148,6 +148,9 @@ extern char *saved_command_line;
 extern unsigned int saved_command_line_len;
 extern unsigned int reset_devices;
 
+/* Defined in init/version-timestamp.c */
+extern const char linux_banner[];
+
 /* used by init/main.c */
 void setup_arch(char **);
 void prepare_namespace(void);
diff --git a/init/version-timestamp.c b/init/version-timestamp.c
index 043cbf80a766..a48f2c19e5d7 100644
--- a/init/version-timestamp.c
+++ b/init/version-timestamp.c
@@ -6,6 +6,7 @@
 #include <linux/refcount.h>
 #include <linux/uts.h>
 #include <linux/utsname.h>
+#include <linux/init.h>
 
 struct uts_namespace init_uts_ns = {
 	.ns.count = REFCOUNT_INIT(2),
@@ -28,3 +29,5 @@ struct uts_namespace init_uts_ns = {
 const char linux_banner[] =
 	"Linux version " UTS_RELEASE " (" LINUX_COMPILE_BY "@"
 	LINUX_COMPILE_HOST ") (" LINUX_COMPILER ") " UTS_VERSION "\n";
+
+EXPORT_SYMBOL_GPL(linux_banner);
-- 
2.7.4

[PATCH v5 06/17] soc: qcom: Add Qualcomm APSS 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.

qcom_minidump(core or frontend) driver adds the capability to add inux
region to be dumped as part of ram dump collection. It provides
appropriate symbol to register/unregister 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                  |  13 +
 drivers/soc/qcom/Makefile                 |   1 +
 drivers/soc/qcom/qcom_minidump.c          | 603 ++++++++++++++++++++++++++++++
 drivers/soc/qcom/qcom_minidump_internal.h |  10 +
 drivers/soc/qcom/smem.c                   |  18 +
 include/linux/soc/qcom/smem.h             |   2 +
 include/soc/qcom/qcom_minidump.h          |  33 ++
 7 files changed, 680 insertions(+)
 create mode 100644 drivers/soc/qcom/qcom_minidump.c

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index ff38deac6a7d..4b36d46807bc 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -300,4 +300,17 @@ config QCOM_RPROC_MINIDUMP
 	  query predefined minidump segments associated with the remote processor
 	  and check its validity and end up collecting the dump on remote processor
 	  crash during its recovery.
+
+config QCOM_MINIDUMP
+	tristate "QCOM APSS Minidump driver"
+	depends on ARCH_QCOM || COMPILE_TEST
+	depends on QCOM_SMEM
+	help
+	  This config enables linux core infrastructure for Application
+	  processor subsystem (APSS) minidump collection i.e, it enables
+	  Linux clients drivers to register their internal data structures
+	  and debug messages as part of the apss minidump table and when
+	  the SoC is crashed, these selective regions will be dumped
+	  instead of the entire DDR dump. This saves significant amount
+	  of time and/or storage space.
 endmenu
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index a5fd2fed0923..9c1a409679ec 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -36,3 +36,4 @@ obj-$(CONFIG_QCOM_ICC_BWMON)	+= icc-bwmon.o
 qcom_ice-objs			+= ice.o
 obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE)	+= qcom_ice.o
 obj-$(CONFIG_QCOM_RPROC_MINIDUMP)	+= qcom_rproc_minidump.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 000000000000..86f4d09a7b4e
--- /dev/null
+++ b/drivers/soc/qcom/qcom_minidump.c
@@ -0,0 +1,603 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/device.h>
+#include <linux/elf.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/export.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mutex.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/printk.h>
+#include <linux/soc/qcom/smem.h>
+#include <linux/string.h>
+#include <soc/qcom/qcom_minidump.h>
+
+#include "qcom_minidump_internal.h"
+
+/**
+ * struct minidump_ss_data - Minidump subsystem private data
+ * @md_ss_toc	: Application Subsystem TOC pointer
+ * @md_regions	: Application Subsystem region base pointer
+ */
+struct minidump_ss_data {
+	struct minidump_subsystem	*md_ss_toc;
+	struct minidump_region		*md_regions;
+};
+
+/**
+ * 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 data information
+ * @elf		  : Minidump elf header
+ * @dev		  : Minidump backend device
+ * @max_num_limit : Maximum number of region limit
+ * @apss_data	  : Backend driver's private data pointer
+ */
+struct minidump {
+	struct minidump_elfhdr		elf;
+	struct device			*dev;
+	struct minidump_ss_data		*apss_data;
+	struct mutex			md_lock;
+};
+
+/*
+ * 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 201. In future, this limitation
+ * from boot firmware might get removed by allocating the region dynamically.
+ * So, keep it compatible with older devices, we can keep the current limit for
+ * Linux to 201.
+ */
+#define MAX_NUM_ENTRIES	  201
+#define MAX_STRTBL_SIZE	  (MAX_NUM_ENTRIES * MAX_REGION_NAME_LENGTH)
+
+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 char *elf_lookup_string(struct minidump *md, struct elfhdr *ehdr, int offset)
+{
+	char *strtab = elf_str_table_start(ehdr);
+
+	if (!strtab || (md->elf.strtable_idx < offset))
+		return NULL;
+
+	return strtab + offset;
+}
+
+static unsigned int append_str_to_strtable(struct minidump *md, const char *name)
+{
+	char *strtab = elf_str_table_start(md->elf.ehdr);
+	unsigned int old_idx = md->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);
+	md->elf.strtable_idx = old_idx + 1;
+
+	return ret;
+}
+
+static int qcom_md_clear_elfheader(struct minidump *md,
+				   const struct qcom_minidump_region *region)
+{
+	struct elfhdr *ehdr = md->elf.ehdr;
+	struct elf_shdr *shdr;
+	struct elf_shdr *tmp_shdr;
+	struct elf_phdr *phdr;
+	struct elf_phdr *tmp_phdr;
+	unsigned int phidx;
+	unsigned int shidx;
+	unsigned int len;
+	unsigned int i;
+	char *shname;
+
+	for (i = 0; i < ehdr->e_phnum; i++) {
+		phdr = elf_phdr_entry_addr(ehdr, i);
+		if (phdr->p_paddr == region->phys_addr &&
+		    phdr->p_memsz == region->size)
+			break;
+	}
+
+	if (i == ehdr->e_phnum) {
+		dev_err(md->dev, "Cannot find program header entry in elf\n");
+		return -EINVAL;
+	}
+
+	phidx = i;
+	for (i = 0; i < ehdr->e_shnum; i++) {
+		shdr = elf_shdr_entry_addr(ehdr, i);
+		shname = elf_lookup_string(md, ehdr, shdr->sh_name);
+		if (shname && !strcmp(shname, region->name) &&
+		    shdr->sh_addr == (elf_addr_t)region->virt_addr &&
+		    shdr->sh_size == region->size)
+			break;
+	}
+
+	if (i == ehdr->e_shnum) {
+		dev_err(md->dev, "Cannot find section header entry in elf\n");
+		return -EINVAL;
+	}
+
+	shidx = i;
+	if (shdr->sh_offset != phdr->p_offset) {
+		dev_err(md->dev, "Invalid entry details for region: %s\n", region->name);
+		return -EINVAL;
+	}
+
+	/* Clear name in string table */
+	len = strlen(shname) + 1;
+	memmove(shname, shname + len, md->elf.strtable_idx - shdr->sh_name - len);
+	md->elf.strtable_idx -= len;
+
+	/* Clear program header */
+	tmp_phdr = elf_phdr_entry_addr(ehdr, phidx);
+	for (i = phidx; i < ehdr->e_phnum - 1; i++) {
+		tmp_phdr = elf_phdr_entry_addr(ehdr, i + 1);
+		phdr = elf_phdr_entry_addr(ehdr, i);
+		memcpy(phdr, tmp_phdr, sizeof(struct elf_phdr));
+		phdr->p_offset = phdr->p_offset - region->size;
+	}
+	memset(tmp_phdr, 0, sizeof(struct elf_phdr));
+	ehdr->e_phnum--;
+
+	/* Clear section header */
+	tmp_shdr = elf_shdr_entry_addr(ehdr, shidx);
+	for (i = shidx; i < ehdr->e_shnum - 1; i++) {
+		tmp_shdr = elf_shdr_entry_addr(ehdr, i + 1);
+		shdr = elf_shdr_entry_addr(ehdr, i);
+		memcpy(shdr, tmp_shdr, sizeof(struct elf_shdr));
+		shdr->sh_offset -= region->size;
+		shdr->sh_name -= len;
+	}
+
+	memset(tmp_shdr, 0, sizeof(struct elf_shdr));
+	ehdr->e_shnum--;
+	md->elf.elf_offset -= region->size;
+
+	return 0;
+}
+
+static void qcom_md_update_elfheader(struct minidump *md,
+				     const struct qcom_minidump_region *region)
+{
+	struct elfhdr *ehdr = md->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(md, region->name);
+	shdr->sh_addr = (elf_addr_t)region->virt_addr;
+	shdr->sh_size = region->size;
+	shdr->sh_flags = SHF_WRITE;
+	shdr->sh_offset = md->elf.elf_offset;
+	shdr->sh_entsize = 0;
+
+	phdr->p_type = PT_LOAD;
+	phdr->p_offset = md->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;
+	md->elf.elf_offset += shdr->sh_size;
+}
+
+static void qcom_md_add_region(struct minidump_ss_data *mdss_data,
+			       const struct qcom_minidump_region *region)
+{
+	struct minidump_subsystem *mdss_toc = mdss_data->md_ss_toc;
+	struct minidump_region *mdr;
+	unsigned int region_cnt;
+
+	region_cnt = le32_to_cpu(mdss_toc->region_count);
+	mdr = &mdss_data->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++;
+	mdss_toc->region_count = cpu_to_le32(region_cnt);
+}
+
+static int qcom_md_get_region_index(struct minidump_ss_data *mdss_data,
+				    const struct qcom_minidump_region *region)
+{
+	struct minidump_subsystem *mdss_toc = mdss_data->md_ss_toc;
+	struct minidump_region *mdr;
+	unsigned int i;
+	unsigned int count;
+
+	count = le32_to_cpu(mdss_toc->region_count);
+	for (i = 0; i < count; i++) {
+		mdr = &mdss_data->md_regions[i];
+		if (!strcmp(mdr->name, region->name))
+			return i;
+	}
+
+	return -ENOENT;
+}
+
+static int qcom_md_region_unregister(struct minidump *md,
+				     const struct qcom_minidump_region *region)
+{
+	struct minidump_ss_data *mdss_data = md->apss_data;
+	struct minidump_subsystem *mdss_toc = mdss_data->md_ss_toc;
+	struct minidump_region *mdr;
+	unsigned int region_cnt;
+	unsigned int idx;
+	int ret;
+
+	ret = qcom_md_get_region_index(mdss_data, region);
+	if (ret < 0) {
+		dev_err(md->dev, "%s region is not present\n", region->name);
+		return ret;
+	}
+
+	idx = ret;
+	mdr = &mdss_data->md_regions[0];
+	region_cnt = le32_to_cpu(mdss_toc->region_count);
+	/*
+	 * Left shift all the regions exist after this removed region
+	 * index by 1 to fill the gap and zero out the last region
+	 * present at the end.
+	 */
+	memmove(&mdr[idx], &mdr[idx + 1],
+		(region_cnt - idx - 1) * sizeof(struct minidump_region));
+	memset(&mdr[region_cnt - 1], 0, sizeof(struct minidump_region));
+	region_cnt--;
+	mdss_toc->region_count = cpu_to_le32(region_cnt);
+
+	return 0;
+}
+
+static int qcom_md_region_register(struct minidump *md,
+				   const struct qcom_minidump_region *region)
+{
+	struct minidump_ss_data *mdss_data = md->apss_data;
+	struct minidump_subsystem *mdss_toc = mdss_data->md_ss_toc;
+	unsigned int num_region;
+	int ret;
+
+	ret = qcom_md_get_region_index(mdss_data, region);
+	if (ret >= 0) {
+		dev_info(md->dev, "%s region is already registered\n", region->name);
+		return -EEXIST;
+	}
+
+	/* Check if there is a room for a new entry */
+	num_region = le32_to_cpu(mdss_toc->region_count);
+	if (num_region >= MAX_NUM_ENTRIES) {
+		dev_err(md->dev, "maximum region limit %u reached\n", num_region);
+		return -ENOSPC;
+	}
+
+	qcom_md_add_region(mdss_data, region);
+
+	return 0;
+}
+
+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);
+}
+
+/**
+ * qcom_minidump_region_register() - Register region in APSS Minidump table.
+ * @region: minidump region.
+ *
+ * Return: On success, it returns 0 and negative error value on failure.
+ */
+int qcom_minidump_region_register(const struct qcom_minidump_region *region)
+{
+	struct minidump *md;
+	int ret;
+
+	md = qcom_smem_minidump_ready();
+	if (!md)
+		return -EPROBE_DEFER;
+
+	if (!qcom_minidump_valid_region(region))
+		return -EINVAL;
+
+	mutex_lock(&md->md_lock);
+	ret = qcom_md_region_register(md, region);
+	if (ret)
+		goto unlock;
+
+	qcom_md_update_elfheader(md, region);
+unlock:
+	mutex_unlock(&md->md_lock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(qcom_minidump_region_register);
+
+/**
+ * qcom_minidump_region_unregister() - Unregister region from APSS Minidump table.
+ * @region: minidump region.
+ *
+ * Return: On success, it returns 0 and negative error value on failure.
+ */
+int qcom_minidump_region_unregister(const struct qcom_minidump_region *region)
+{
+	struct minidump *md;
+	int ret;
+
+	md = qcom_smem_minidump_ready();
+	if (!md)
+		return -EPROBE_DEFER;
+
+	if (!qcom_minidump_valid_region(region))
+		return -EINVAL;
+
+	mutex_lock(&md->md_lock);
+	ret = qcom_md_region_unregister(md, region);
+	if (ret)
+		goto unlock;
+
+	ret = qcom_md_clear_elfheader(md, region);
+unlock:
+	mutex_unlock(&md->md_lock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(qcom_minidump_region_unregister);
+
+static int qcom_md_add_elfheader(struct minidump *md)
+{
+	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;
+	unsigned int banner_len;
+	char *banner;
+
+	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);
+
+	md->elf.ehdr = devm_kzalloc(md->dev, elfh_size, GFP_KERNEL);
+	if (!md->elf.ehdr)
+		return -ENOMEM;
+
+	ehdr = md->elf.ehdr;
+	/* Assign Section/Program headers offset */
+	md->elf.shdr = shdr = (struct elf_shdr *)(ehdr + 1);
+	md->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;
+
+	md->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.
+	 */
+	md->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(md, "STR_TBL");
+	shdr++;
+
+	/* 3rd 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(md, "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;
+
+	/*
+	 * Above are some prdefined sections/program header used
+	 * for debug, update their count here.
+	 */
+	ehdr->e_phnum = 1;
+	ehdr->e_shnum = 3;
+
+	/* Register ELF header as first region */
+	strscpy(elfregion.name, "KELF_HEADER", sizeof(elfregion.name));
+	elfregion.virt_addr = md->elf.ehdr;
+	elfregion.phys_addr = virt_to_phys(md->elf.ehdr);
+	elfregion.size = elfh_size;
+
+	return qcom_md_region_register(md, &elfregion);
+}
+
+static int qcom_apss_md_table_init(struct minidump *md,
+				   struct minidump_subsystem *mdss_toc)
+{
+	struct minidump_ss_data *mdss_data;
+
+	mdss_data = devm_kzalloc(md->dev, sizeof(*mdss_data), GFP_KERNEL);
+	if (!mdss_data)
+		return -ENOMEM;
+
+	mdss_data->md_ss_toc = mdss_toc;
+	mdss_data->md_regions = devm_kcalloc(md->dev, MAX_NUM_ENTRIES,
+					     sizeof(struct minidump_region),
+					     GFP_KERNEL);
+	if (!mdss_data->md_regions)
+		return -ENOMEM;
+
+	mdss_toc = mdss_data->md_ss_toc;
+	mdss_toc->regions_baseptr = cpu_to_le64(virt_to_phys(mdss_data->md_regions));
+	mdss_toc->enabled = cpu_to_le32(MINIDUMP_SS_ENABLED);
+	mdss_toc->status = cpu_to_le32(1);
+	mdss_toc->region_count = cpu_to_le32(0);
+
+	/* Tell bootloader not to encrypt the regions of this subsystem */
+	mdss_toc->encryption_status = cpu_to_le32(MINIDUMP_SS_ENCR_DONE);
+	mdss_toc->encryption_required = cpu_to_le32(MINIDUMP_SS_ENCR_NOTREQ);
+
+	md->apss_data = mdss_data;
+
+	return 0;
+}
+
+static int qcom_apss_minidump_probe(struct platform_device *pdev)
+{
+	struct minidump_global_toc *mdgtoc;
+	struct minidump *md;
+	size_t size;
+	int ret;
+
+	md = devm_kzalloc(&pdev->dev, sizeof(struct minidump), GFP_KERNEL);
+	if (!md)
+		return -ENOMEM;
+
+	md->dev = &pdev->dev;
+	mdgtoc = qcom_smem_get(QCOM_SMEM_HOST_ANY, SBL_MINIDUMP_SMEM_ID, &size);
+	if (IS_ERR(mdgtoc)) {
+		ret = PTR_ERR(mdgtoc);
+		dev_err(md->dev, "Couldn't find minidump smem item: %d\n", ret);
+		return ret;
+	}
+
+	if (size < sizeof(*mdgtoc) || !mdgtoc->status) {
+		dev_err(md->dev, "minidump table is not initialized: %d\n", ret);
+		return -EINVAL;
+	}
+
+	mutex_init(&md->md_lock);
+	ret = qcom_apss_md_table_init(md, &mdgtoc->subsystems[MINIDUMP_APSS_DESC]);
+	if (ret) {
+		dev_err(md->dev, "apss minidump initialization failed: %d\n", ret);
+		return ret;
+	}
+
+	/* First entry would be ELF header */
+	ret = qcom_md_add_elfheader(md);
+	if (ret) {
+		dev_err(md->dev, "Failed to add elf header: %d\n", ret);
+		memset(md->apss_data->md_ss_toc, 0, sizeof(struct minidump_subsystem));
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, md);
+
+	return ret;
+}
+
+static int qcom_apss_minidump_remove(struct platform_device *pdev)
+{
+	struct minidump *md = platform_get_drvdata(pdev);
+	struct minidump_ss_data *mdss_data;
+
+	mdss_data = md->apss_data;
+	memset(mdss_data->md_ss_toc, cpu_to_le32(0), sizeof(struct minidump_subsystem));
+	md = NULL;
+
+	return 0;
+}
+
+static struct platform_driver qcom_minidump_driver = {
+	.probe = qcom_apss_minidump_probe,
+	.remove = qcom_apss_minidump_remove,
+	.driver  = {
+		.name = "qcom-minidump-smem",
+	},
+};
+
+module_platform_driver(qcom_minidump_driver);
+
+MODULE_DESCRIPTION("Qualcomm APSS minidump driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:qcom-minidump-smem");
diff --git a/drivers/soc/qcom/qcom_minidump_internal.h b/drivers/soc/qcom/qcom_minidump_internal.h
index 71709235b196..a2aebe5b690a 100644
--- a/drivers/soc/qcom/qcom_minidump_internal.h
+++ b/drivers/soc/qcom/qcom_minidump_internal.h
@@ -9,10 +9,20 @@
 #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_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_START	   ('S' << 24 | 'T' << 16 | 'R' << 8 | 'T' << 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)
 
+#define MINIDUMP_APSS_DESC	   0
+
 /**
  * struct minidump_region - Minidump region
  * @name		: Name of the region to be dumped
diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index b0d59e815c3b..a9c8d1c85ad0 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -270,6 +270,7 @@ struct smem_region {
  * @partitions: list of partitions of current processor/host
  * @item_count: max accepted item number
  * @socinfo:	platform device pointer
+ * @minidump:	minidump platform device pointer
  * @num_regions: number of @regions
  * @regions:	list of the memory regions defining the shared memory
  */
@@ -280,6 +281,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];
@@ -795,6 +797,13 @@ int qcom_smem_get_soc_id(u32 *id)
 }
 EXPORT_SYMBOL_GPL(qcom_smem_get_soc_id);
 
+void *qcom_smem_minidump_ready(void)
+{
+	return (__smem && __smem->minidump) ? platform_get_drvdata(__smem->minidump) :
+		NULL;
+}
+EXPORT_SYMBOL_GPL(qcom_smem_minidump_ready);
+
 static int qcom_smem_get_sbl_version(struct qcom_smem *smem)
 {
 	struct smem_header *header;
@@ -1174,11 +1183,20 @@ 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-smem",
+						      PLATFORM_DEVID_NONE, NULL,
+						      0);
+	if (IS_ERR(smem->minidump)) {
+		dev_dbg(&pdev->dev, "failed to register minidump device\n");
+		smem->minidump = NULL;
+	}
+
 	return 0;
 }
 
 static int qcom_smem_remove(struct platform_device *pdev)
 {
+	platform_device_unregister(__smem->minidump);
 	platform_device_unregister(__smem->socinfo);
 
 	hwspin_lock_free(__smem->hwlock);
diff --git a/include/linux/soc/qcom/smem.h b/include/linux/soc/qcom/smem.h
index 223db6a9c733..efcd15b8112b 100644
--- a/include/linux/soc/qcom/smem.h
+++ b/include/linux/soc/qcom/smem.h
@@ -13,4 +13,6 @@ phys_addr_t qcom_smem_virt_to_phys(void *p);
 
 int qcom_smem_get_soc_id(u32 *id);
 
+void *qcom_smem_minidump_ready(void);
+
 #endif
diff --git a/include/soc/qcom/qcom_minidump.h b/include/soc/qcom/qcom_minidump.h
index cd87caef919d..d993604ed2bf 100644
--- a/include/soc/qcom/qcom_minidump.h
+++ b/include/soc/qcom/qcom_minidump.h
@@ -6,6 +6,39 @@
 #ifndef _QCOM_MINIDUMP_H_
 #define _QCOM_MINIDUMP_H_
 
+#define MAX_NAME_LENGTH		12
+
+/**
+ * struct qcom_minidump_region - APSS 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)
+int qcom_minidump_region_register(const struct qcom_minidump_region *region);
+int qcom_minidump_region_unregister(const struct qcom_minidump_region *region);
+#else
+static inline int qcom_minidump_region_register(const struct qcom_minidump_region *region)
+{
+	/* Return quietly, if minidump config is not enabled */
+	return 0;
+}
+static inline int qcom_minidump_region_unregister(const struct qcom_minidump_region *region)
+{
+	return 0;
+}
+#endif /* CONFIG_QCOM_MINIDUMP */
+
 struct rproc;
 struct rproc_dump_segment;
 
-- 
2.7.4

[PATCH v5 07/17] soc: qcom: minidump: Add pending region registration

[Mukesh Ojha]

Pending regions are those apss minidump regions which came for
registration before minidump was initialized or ready to do
register the region.

We can add regions to pending region list and register all of
them from apss minidump driver probe in one go.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/soc/qcom/qcom_minidump.c | 140 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 130 insertions(+), 10 deletions(-)

diff --git a/drivers/soc/qcom/qcom_minidump.c b/drivers/soc/qcom/qcom_minidump.c
index 86f4d09a7b4e..4ce36f154e89 100644
--- a/drivers/soc/qcom/qcom_minidump.c
+++ b/drivers/soc/qcom/qcom_minidump.c
@@ -13,6 +13,7 @@
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
+#include <linux/list.h>
 #include <linux/mutex.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
@@ -63,6 +64,33 @@ struct minidump {
 	struct mutex			md_lock;
 };
 
+/**
+ * struct minidump_pregion - Minidump pending region
+ * @list       : Pending region list pointer
+ * @region     : APSS minidump client region
+ */
+struct minidump_pregion {
+	struct list_head	     list;
+	struct qcom_minidump_region  region;
+};
+
+/**
+ * struct minidump_plist - Minidump pending region list
+ * @plist	: List of pending region to be registered
+ * @pregion_cnt	: Count of the pending region to be registered
+ */
+struct minidump_plist {
+	struct list_head  plist;
+	int 		  pregion_cnt;
+	struct mutex	  plock;
+};
+
+static struct minidump_plist md_plist = {
+	.plist = LIST_HEAD_INIT(md_plist.plist),
+	.pregion_cnt = 0,
+	.plock = __MUTEX_INITIALIZER(md_plist.plock),
+};
+
 /*
  * In some of the Old Qualcomm devices, boot firmware statically allocates 300
  * as total number of supported region (including all co-processors) in
@@ -336,6 +364,26 @@ static bool qcom_minidump_valid_region(const struct qcom_minidump_region *region
 		IS_ALIGNED(region->size, 4);
 }
 
+static struct minidump_pregion *
+check_region_in_plist(const struct qcom_minidump_region *region)
+{
+	struct minidump_pregion *md_pregion;
+	struct minidump_pregion *tmp;
+	bool found = false;
+
+	list_for_each_entry_safe(md_pregion, tmp, &md_plist.plist, list) {
+		struct qcom_minidump_region *md_region;
+
+		md_region = &md_pregion->region;
+		if (!strcmp(md_region->name, region->name)) {
+			found = true;
+			break;
+		}
+	}
+
+	return found ? md_pregion : NULL;
+}
+
 /**
  * qcom_minidump_region_register() - Register region in APSS Minidump table.
  * @region: minidump region.
@@ -344,16 +392,44 @@ static bool qcom_minidump_valid_region(const struct qcom_minidump_region *region
  */
 int qcom_minidump_region_register(const struct qcom_minidump_region *region)
 {
+	struct minidump_pregion *md_pregion;
 	struct minidump *md;
-	int ret;
-
-	md = qcom_smem_minidump_ready();
-	if (!md)
-		return -EPROBE_DEFER;
+	int ret = 0;
 
 	if (!qcom_minidump_valid_region(region))
 		return -EINVAL;
 
+	mutex_lock(&md_plist.plock);
+	md = qcom_smem_minidump_ready();
+	if (!md) {
+		if (md_plist.pregion_cnt >= MAX_NUM_ENTRIES - 1) {
+			pr_err("maximum region limit %u reached\n", md_plist.pregion_cnt);
+			ret = -ENOSPC;
+			goto unlock_plock;
+		}
+
+		md_pregion = check_region_in_plist(region);
+		if (md_pregion) {
+			pr_info("%s region is already exist\n", region->name);
+			ret = -EEXIST;
+			goto unlock_plock;
+		}
+		/*
+		 * Maintain a list of client regions which came before
+		 * minidump driver was ready and once it is ready,
+		 * register them in one go from minidump probe function.
+		 */
+		md_pregion = kzalloc(sizeof(*md_pregion), GFP_KERNEL);
+		if (!md_pregion) {
+			ret = -ENOMEM;
+			goto unlock_plock;
+		}
+		md_pregion->region = *region;
+		list_add_tail(&md_pregion->list, &md_plist.plist);
+		md_plist.pregion_cnt++;
+		goto unlock_plock;
+	}
+
 	mutex_lock(&md->md_lock);
 	ret = qcom_md_region_register(md, region);
 	if (ret)
@@ -362,6 +438,10 @@ int qcom_minidump_region_register(const struct qcom_minidump_region *region)
 	qcom_md_update_elfheader(md, region);
 unlock:
 	mutex_unlock(&md->md_lock);
+	return 0;
+
+unlock_plock:
+	mutex_unlock(&md_plist.plock);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(qcom_minidump_region_register);
@@ -374,16 +454,28 @@ EXPORT_SYMBOL_GPL(qcom_minidump_region_register);
  */
 int qcom_minidump_region_unregister(const struct qcom_minidump_region *region)
 {
+	struct minidump_pregion *md_pregion;
 	struct minidump *md;
-	int ret;
-
-	md = qcom_smem_minidump_ready();
-	if (!md)
-		return -EPROBE_DEFER;
+	int ret = 0;
 
 	if (!qcom_minidump_valid_region(region))
 		return -EINVAL;
 
+	mutex_lock(&md_plist.plock);
+	md = qcom_smem_minidump_ready();
+	if (!md) {
+		md_pregion = check_region_in_plist(region);
+		if (!md_pregion) {
+			ret = -ENOENT;
+			goto unlock_plock;
+		}
+
+		list_del(&md_pregion->list);
+		kfree(md_pregion);
+		md_plist.pregion_cnt--;
+		goto unlock_plock;
+	}
+
 	mutex_lock(&md->md_lock);
 	ret = qcom_md_region_unregister(md, region);
 	if (ret)
@@ -393,6 +485,10 @@ int qcom_minidump_region_unregister(const struct qcom_minidump_region *region)
 unlock:
 	mutex_unlock(&md->md_lock);
 	return ret;
+
+unlock_plock:
+	mutex_unlock(&md_plist.plock);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(qcom_minidump_region_unregister);
 
@@ -532,6 +628,27 @@ static int qcom_apss_md_table_init(struct minidump *md,
 	return 0;
 }
 
+void qcom_apss_register_pending_regions(struct minidump *md)
+{
+	struct minidump_ss_data *mdss_data = md->apss_data;
+	struct minidump_pregion *md_pregion;
+	struct minidump_pregion *tmp;
+
+	list_for_each_entry_safe(md_pregion, tmp, &md_plist.plist, list) {
+		struct qcom_minidump_region *region;
+
+		region = &md_pregion->region;
+		mutex_lock(&md->md_lock);
+		qcom_md_add_region(mdss_data, region);
+		qcom_md_update_elfheader(md, region);
+		mutex_unlock(&md->md_lock);
+
+		list_del(&md_pregion->list);
+		kfree(md_pregion);
+		md_plist.pregion_cnt--;
+	}
+}
+
 static int qcom_apss_minidump_probe(struct platform_device *pdev)
 {
 	struct minidump_global_toc *mdgtoc;
@@ -571,7 +688,10 @@ static int qcom_apss_minidump_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	mutex_lock(&md_plist.plock);
 	platform_set_drvdata(pdev, md);
+	qcom_apss_register_pending_regions(md);
+	mutex_unlock(&md_plist.plock);
 
 	return ret;
 }
-- 
2.7.4

[PATCH v5 08/17] arm64: mm: Add dynamic ramoops region support through command line

[Mukesh Ojha]

The reserved memory region for ramoops is assumed to be at a fixed
and known location when read from the devicetree. This may not be
required for something like Qualcomm's minidump which is interested
in knowing addresses of ramoops region but it does not put hard
requirement of address being fixed as most of it's SoC does not
support warm reset and does not use pstorefs at all instead it has
firmware way of collecting ramoops region if it gets to know the
address and register it with apss minidump table which is sitting
in shared memory region in DDR and firmware will have access to
these table during reset and collects it on crash of SoC.

So, add the support of reserving ramoops region to be dynamically
allocated early during boot if it is request through command line
via 'dyn_ramoops_size=' and fill up reserved resource structure and
export the structure, so that it can be read by ramoops driver.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 arch/arm64/mm/init.c       | 94 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pstore_ram.h |  2 +
 2 files changed, 96 insertions(+)

diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index d31c3a9290c5..14d7086758bf 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -31,6 +31,7 @@
 #include <linux/hugetlb.h>
 #include <linux/acpi_iort.h>
 #include <linux/kmemleak.h>
+#include <linux/pstore_ram.h>
 
 #include <asm/boot.h>
 #include <asm/fixmap.h>
@@ -73,6 +74,93 @@ phys_addr_t __ro_after_init arm64_dma_phys_limit;
 
 #define DEFAULT_CRASH_KERNEL_LOW_SIZE	(128UL << 20)
 
+#define RAMOOPS_ADDR_HIGH_MAX		(PHYS_MASK + 1)
+
+/* Location of the reserved area for the dynamic ramoops */
+struct resource dyn_ramoops_res = {
+	.name  = "ramoops",
+	.start = 0,
+	.end   = 0,
+	.flags = IORESOURCE_BUSY | IORESOURCE_SYSTEM_RAM,
+	.desc  = IORES_DESC_NONE,
+};
+EXPORT_SYMBOL(dyn_ramoops_res);
+
+static int __init parse_dynamic_ramoops(char *cmdline, unsigned long long *size)
+{
+	const char *name = "dyn_ramoops_size=";
+	char *p = NULL;
+	char *q = NULL;
+	char *tmp;
+
+	if (!cmdline)
+		return -ENOENT;
+
+	/* Check for "dyn_ramoops_size" and use the later if there are more */
+	p = strstr(cmdline, name);
+	while (p) {
+		q = p;
+		p = strchr(p, ' ');
+		if (!p)
+			break;
+
+		p = strstr(p + 1, name);
+	}
+
+	if (!q) {
+		pr_err("ramoops: No entry found for %s\n", name);
+		return -ENOENT;
+	}
+
+	p = q + strlen(name);
+	*size = memparse(p, &tmp);
+	if (p == tmp) {
+		pr_err("ramoops: memory value expected\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int __init parse_dyn_ramoops_size_dummy(char *arg)
+{
+	return 0;
+}
+early_param("dyn_ramoops_size", parse_dyn_ramoops_size_dummy);
+
+/*
+ * reserve_dynamic_ramoops() - reserves memory for dynamic ramoops
+ *
+ * This enable dynamic reserve memory support for ramoops through
+ * command line.
+ */
+static void __init reserve_dynamic_ramoops(void)
+{
+	char *cmdline = boot_command_line;
+	unsigned long long ramoops_base;
+	unsigned long long ramoops_size;
+
+	if (!IS_ENABLED(CONFIG_PSTORE_RAM))
+		return;
+
+	if (parse_dynamic_ramoops(cmdline, &ramoops_size))
+		return;
+
+	ramoops_base = memblock_phys_alloc_range(ramoops_size, SMP_CACHE_BYTES,
+						 0, RAMOOPS_ADDR_HIGH_MAX);
+	if (!ramoops_base) {
+		pr_err("cannot allocate ramoops dynamic memory (size:0x%llx).\n",
+			ramoops_size);
+		return;
+	}
+
+	kmemleak_ignore_phys(ramoops_base);
+
+	dyn_ramoops_res.start = ramoops_base;
+	dyn_ramoops_res.end = ramoops_base + ramoops_size - 1;
+	insert_resource(&iomem_resource, &dyn_ramoops_res);
+}
+
 static int __init reserve_crashkernel_low(unsigned long long low_size)
 {
 	unsigned long long low_base;
@@ -456,6 +544,12 @@ void __init bootmem_init(void)
 	 */
 	reserve_crashkernel();
 
+	/*
+	 * Reserving ramoops region resource dynamically in case it is
+	 * requested from command line.
+	 */
+	reserve_dynamic_ramoops();
+
 	memblock_dump_all();
 }
 
diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h
index 9d65ff94e216..07d700b7649d 100644
--- a/include/linux/pstore_ram.h
+++ b/include/linux/pstore_ram.h
@@ -10,6 +10,8 @@
 
 #include <linux/pstore.h>
 
+extern struct resource dyn_ramoops_res;
+
 struct persistent_ram_ecc_info {
 	int block_size;
 	int ecc_size;
-- 
2.7.4

[PATCH v5 09/17] pstore/ram: Use dynamic ramoops reserve resource

[Mukesh Ojha]

As dynamic ramoops command line parsing is now added, so
lets add the support in ramoops driver to get the resource
structure and add it during platform device registration.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 fs/pstore/ram.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 2f625e1fa8d8..e73fbbc1b5b5 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -913,13 +913,17 @@ static void __init ramoops_register_dummy(void)
 
 	/*
 	 * Prepare a dummy platform data structure to carry the module
-	 * parameters. If mem_size isn't set, then there are no module
-	 * parameters, and we can skip this.
+	 * parameters. If mem_size isn't set, check for dynamic ramoops
+	 * size and extract the information if it is set.
 	 */
-	if (!mem_size)
+	if (!mem_size && !dyn_ramoops_res.end)
 		return;
 
 	pr_info("using module parameters\n");
+	if (dyn_ramoops_res.end) {
+		mem_size = resource_size(&dyn_ramoops_res);
+		mem_address = dyn_ramoops_res.start;
+	}
 
 	memset(&pdata, 0, sizeof(pdata));
 	pdata.mem_size = mem_size;
-- 
2.7.4

[PATCH v5 10/17] pstore: Add pstore_region_defined() helper and export it

[Mukesh Ojha]

There are users like Qualcomm minidump which is interested in
knowing the pstore frontend addresses and sizes from the backend
(ram) to be able to register it with firmware to finally collect
them during crash for debugging.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 fs/pstore/platform.c   | 15 +++++++++++++++
 fs/pstore/ram.c        | 42 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/pstore.h |  6 ++++++
 3 files changed, 63 insertions(+)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index cbc0b468c1ab..cb80116a05cb 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -137,6 +137,21 @@ enum pstore_type_id pstore_name_to_type(const char *name)
 }
 EXPORT_SYMBOL_GPL(pstore_name_to_type);
 
+int pstore_region_defined(struct pstore_record *record,
+			  void **virt, phys_addr_t *phys,
+			  size_t *size, unsigned int *max_dump_cnt)
+{
+	if (!psinfo)
+		return -EINVAL;
+
+	record->psi = psinfo;
+
+	return psinfo->region_info ?
+	       psinfo->region_info(record, virt, phys, size, max_dump_cnt) :
+	       -EINVAL;
+}
+EXPORT_SYMBOL_GPL(pstore_region_defined);
+
 static void pstore_timer_kick(void)
 {
 	if (pstore_update_ms < 0)
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index e73fbbc1b5b5..2ebb1f5f6350 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -436,6 +436,47 @@ static int ramoops_pstore_erase(struct pstore_record *record)
 	return 0;
 }
 
+static int ramoops_region_info(struct pstore_record *record,
+			       void **virt, phys_addr_t *phys,
+			       size_t *size, unsigned int *max_dump_cnt)
+{
+	struct ramoops_context *cxt = record->psi->data;
+	struct persistent_ram_zone *prz;
+
+	switch (record->type) {
+	case PSTORE_TYPE_DMESG:
+		if (record->id >= cxt->max_dump_cnt)
+			return -EINVAL;
+		prz = cxt->dprzs[record->id];
+		*max_dump_cnt = cxt->max_dump_cnt;
+		break;
+	case PSTORE_TYPE_CONSOLE:
+		if (!cxt->console_size)
+			return -EINVAL;
+		prz = cxt->cprz;
+		break;
+	case PSTORE_TYPE_FTRACE:
+		if (record->id >= cxt->max_ftrace_cnt)
+			return -EINVAL;
+		prz = cxt->fprzs[record->id];
+		*max_dump_cnt = cxt->max_ftrace_cnt;
+		break;
+	case PSTORE_TYPE_PMSG:
+		if (!cxt->pmsg_size)
+			return -EINVAL;
+		prz = cxt->mprz;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	*virt = prz->vaddr;
+	*phys = prz->paddr;
+	*size = prz->size;
+
+	return 0;
+}
+
 static struct ramoops_context oops_cxt = {
 	.pstore = {
 		.owner	= THIS_MODULE,
@@ -445,6 +486,7 @@ static struct ramoops_context oops_cxt = {
 		.write	= ramoops_pstore_write,
 		.write_user	= ramoops_pstore_write_user,
 		.erase	= ramoops_pstore_erase,
+		.region_info = ramoops_region_info,
 	},
 };
 
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index 638507a3c8ff..a64d866e8711 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -199,6 +199,9 @@ struct pstore_info {
 	int		(*write_user)(struct pstore_record *record,
 				      const char __user *buf);
 	int		(*erase)(struct pstore_record *record);
+	int		(*region_info)(struct pstore_record *record,
+				       void **virt, phys_addr_t *phys,
+				       size_t *size, unsigned int *max_dump_cnt);
 };
 
 /* Supported frontends */
@@ -230,6 +233,9 @@ struct pstore_ftrace_record {
 #define TS_CPU_SHIFT 8
 #define TS_CPU_MASK (BIT(TS_CPU_SHIFT) - 1)
 
+int pstore_region_defined(struct pstore_record *record,
+			  void **virt, phys_addr_t *phys,
+			  size_t *size, unsigned int *max_dump_cnt);
 /*
  * If CPU number can be stored in IP, store it there, otherwise store it in
  * the time stamp. This means more timestamp resolution is available when
-- 
2.7.4

[PATCH v5 11/17] qcom_minidump: Register ramoops region with minidump

[Mukesh Ojha]

Register all the pstore frontend with minidump, so that they can
be dumped as default Linux minidump region to be collected on
SoC where minidump is enabled.

Helper functions is written in separate file and built along with
the minidump driver, since it is client of minidump and also call
it at appropriate place from minidump probe so that they always
get registered.

While at it also rename the out minidump module object name during
build as qcom_apss_minidump which basically depicts as Qualcomm
Application processor subsystem minidump.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/soc/qcom/Kconfig                 |  1 +
 drivers/soc/qcom/Makefile                |  3 +-
 drivers/soc/qcom/qcom_minidump.c         |  4 ++
 drivers/soc/qcom/qcom_ramoops_minidump.c | 88 ++++++++++++++++++++++++++++++++
 drivers/soc/qcom/qcom_ramoops_minidump.h | 10 ++++
 5 files changed, 105 insertions(+), 1 deletion(-)
 create mode 100644 drivers/soc/qcom/qcom_ramoops_minidump.c
 create mode 100644 drivers/soc/qcom/qcom_ramoops_minidump.h

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 4b36d46807bc..b3977f1687d8 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -305,6 +305,7 @@ config QCOM_MINIDUMP
 	tristate "QCOM APSS Minidump driver"
 	depends on ARCH_QCOM || COMPILE_TEST
 	depends on QCOM_SMEM
+	depends on PSTORE
 	help
 	  This config enables linux core infrastructure for Application
 	  processor subsystem (APSS) minidump collection i.e, it enables
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 9c1a409679ec..fca80df8b5b1 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -36,4 +36,5 @@ obj-$(CONFIG_QCOM_ICC_BWMON)	+= icc-bwmon.o
 qcom_ice-objs			+= ice.o
 obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE)	+= qcom_ice.o
 obj-$(CONFIG_QCOM_RPROC_MINIDUMP)	+= qcom_rproc_minidump.o
-obj-$(CONFIG_QCOM_MINIDUMP)		+= qcom_minidump.o
+obj-$(CONFIG_QCOM_MINIDUMP)		+= qcom_apss_minidump.o
+qcom_apss_minidump-objs			+= qcom_minidump.o qcom_ramoops_minidump.o
diff --git a/drivers/soc/qcom/qcom_minidump.c b/drivers/soc/qcom/qcom_minidump.c
index 4ce36f154e89..7930a80b9100 100644
--- a/drivers/soc/qcom/qcom_minidump.c
+++ b/drivers/soc/qcom/qcom_minidump.c
@@ -23,6 +23,7 @@
 #include <soc/qcom/qcom_minidump.h>
 
 #include "qcom_minidump_internal.h"
+#include "qcom_ramoops_minidump.h"
 
 /**
  * struct minidump_ss_data - Minidump subsystem private data
@@ -688,6 +689,8 @@ static int qcom_apss_minidump_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	qcom_ramoops_minidump_register(md->dev);
+
 	mutex_lock(&md_plist.plock);
 	platform_set_drvdata(pdev, md);
 	qcom_apss_register_pending_regions(md);
@@ -701,6 +704,7 @@ static int qcom_apss_minidump_remove(struct platform_device *pdev)
 	struct minidump *md = platform_get_drvdata(pdev);
 	struct minidump_ss_data *mdss_data;
 
+	qcom_ramoops_minidump_unregister();
 	mdss_data = md->apss_data;
 	memset(mdss_data->md_ss_toc, cpu_to_le32(0), sizeof(struct minidump_subsystem));
 	md = NULL;
diff --git a/drivers/soc/qcom/qcom_ramoops_minidump.c b/drivers/soc/qcom/qcom_ramoops_minidump.c
new file mode 100644
index 000000000000..eb97310e3858
--- /dev/null
+++ b/drivers/soc/qcom/qcom_ramoops_minidump.c
@@ -0,0 +1,88 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pstore.h>
+#include <linux/slab.h>
+#include <soc/qcom/qcom_minidump.h>
+
+#include "qcom_ramoops_minidump.h"
+
+static LIST_HEAD(ramoops_region_list);
+
+struct md_region_list {
+	struct qcom_minidump_region md_region;
+	struct list_head list;
+};
+
+static int qcom_ramoops_region_register(struct device *dev, int type)
+{
+	struct qcom_minidump_region *md_region;
+	struct md_region_list *mdr_list;
+	struct pstore_record record;
+	unsigned int max_dump_cnt;
+	phys_addr_t phys;
+	const char *name;
+	void *virt;
+	size_t size;
+	int ret;
+
+	record.type = type;
+	record.id = 0;
+	max_dump_cnt = 0;
+	name = pstore_type_to_name(record.type);
+	do {
+		ret = pstore_region_defined(&record, &virt, &phys, &size, &max_dump_cnt);
+		if (ret < 0)
+			break;
+
+		mdr_list = devm_kzalloc(dev, sizeof(struct md_region_list), GFP_KERNEL);
+		if (!mdr_list)
+			return -ENOMEM;
+
+		md_region = &mdr_list->md_region;
+		scnprintf(md_region->name, sizeof(md_region->name) - 1, "K%s%llu", name, record.id);
+		md_region->virt_addr = virt;
+		md_region->phys_addr = phys;
+		md_region->size = size;
+		ret = qcom_minidump_region_register(md_region);
+		if (ret) {
+			pr_err("failed to register minidump region\n");
+			break;
+		}
+
+		list_add(&mdr_list->list, &ramoops_region_list);
+	} while (record.id < max_dump_cnt && ++record.id);
+
+	return ret;
+}
+
+void qcom_ramoops_minidump_register(struct device *dev)
+{
+	int type_arr[4] = { PSTORE_TYPE_DMESG, PSTORE_TYPE_CONSOLE,
+			    PSTORE_TYPE_FTRACE, PSTORE_TYPE_PMSG,
+			  };
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(type_arr); i++)
+		qcom_ramoops_region_register(dev, type_arr[i]);
+}
+
+void qcom_ramoops_minidump_unregister(void)
+{
+	struct md_region_list *mdr_list, *tmp;
+
+	list_for_each_entry_safe(mdr_list, tmp, &ramoops_region_list, list) {
+		struct qcom_minidump_region *region;
+
+		region = &mdr_list->md_region;
+		qcom_minidump_region_unregister(region);
+		list_del(&mdr_list->list);
+	}
+}
diff --git a/drivers/soc/qcom/qcom_ramoops_minidump.h b/drivers/soc/qcom/qcom_ramoops_minidump.h
new file mode 100644
index 000000000000..6086342cd398
--- /dev/null
+++ b/drivers/soc/qcom/qcom_ramoops_minidump.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __QCOM_RAMOOPS_MINIDUMP_H__
+#define __QCOM_RAMOOPS_MINIDUMP_H__
+
+#include <linux/types.h>
+
+void qcom_ramoops_minidump_register(struct device *dev);
+void qcom_ramoops_minidump_unregister(void);
+
+#endif
-- 
2.7.4

[PATCH v5 12/17] MAINTAINERS: Add entry for minidump related files

[Mukesh Ojha]

Add the entries into maintainer file for all the minidump related
modules.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 MAINTAINERS | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index d590ce31aa72..0595176e76fb 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17640,6 +17640,16 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/regulator/vqmmc-ipq4019-regulator.yaml
 F:	drivers/regulator/vqmmc-ipq4019-regulator.c
 
+QUALCOMM MINIDUMP DRIVER
+M:	Mukesh Ojha <quic_mojha@quicinc.com>
+L:	linux-arm-msm@vger.kernel.org
+S:	Maintained
+F:	Documentation/admin-guide/qcom_minidump.rst
+F:	drivers/soc/qcom/qcom_rproc_minidump.c
+F:	drivers/soc/qcom/qcom_minidump.c
+F:	drivers/soc/qcom/qcom_ramoops_minidump.c
+
+
 QUALCOMM NAND CONTROLLER DRIVER
 M:	Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
 L:	linux-mtd@lists.infradead.org
-- 
2.7.4

[PATCH v5 13/17] firmware: qcom_scm: provide a read-modify-write function

[Mukesh Ojha]

It was realized by Srinivas K. that there is a need of
read-modify-write scm exported function so that it can
be used by multiple clients.

Let's introduce qcom_scm_io_update_field() which masks
out the bits and write the passed value to that
bit-offset. Subsequent patch will use this function.

Suggested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/firmware/qcom_scm.c            | 20 ++++++++++++++++++++
 include/linux/firmware/qcom/qcom_scm.h |  2 ++
 2 files changed, 22 insertions(+)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index fde33acd46b7..5ea8fc4fd4e8 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -78,6 +78,7 @@ static const char * const qcom_scm_convention_names[] = {
 };
 
 static struct qcom_scm *__scm;
+static DEFINE_SPINLOCK(lock);
 
 static int qcom_scm_clk_enable(void)
 {
@@ -407,6 +408,25 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
 }
 EXPORT_SYMBOL(qcom_scm_set_remote_state);
 
+int qcom_scm_io_update_field(phys_addr_t addr, unsigned int mask, unsigned int val)
+{
+	unsigned int old, new;
+	int ret;
+
+	spin_lock(&lock);
+	ret = qcom_scm_io_readl(addr, &old);
+	if (ret)
+		goto unlock;
+
+	new = (old & ~mask) | (val & mask);
+
+	ret = qcom_scm_io_writel(addr, new);
+unlock:
+	spin_unlock(&lock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(qcom_scm_io_update_field);
+
 static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
 {
 	struct qcom_scm_desc desc = {
diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
index 250ea4efb7cb..ca41e4eb33ad 100644
--- a/include/linux/firmware/qcom/qcom_scm.h
+++ b/include/linux/firmware/qcom/qcom_scm.h
@@ -84,6 +84,8 @@ extern bool qcom_scm_pas_supported(u32 peripheral);
 
 extern int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val);
 extern int qcom_scm_io_writel(phys_addr_t addr, unsigned int val);
+extern int qcom_scm_io_update_field(phys_addr_t addr, unsigned int mask,
+				    unsigned int val);
 
 extern bool qcom_scm_restore_sec_cfg_available(void);
 extern int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare);
-- 
2.7.4

[PATCH v5 14/17] pinctrl: qcom: Use qcom_scm_io_update_field()

[Mukesh Ojha]

Use qcom_scm_io_update_field() exported function in
pinctrl-msm driver.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 115b83e2d8e6..2b9182375702 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -1078,22 +1078,20 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 	if (g->intr_target_width)
 		intr_target_mask = GENMASK(g->intr_target_width - 1, 0);
 
+	intr_target_mask <<= g->intr_target_bit;
 	if (pctrl->intr_target_use_scm) {
 		u32 addr = pctrl->phys_base[0] + g->intr_target_reg;
 		int ret;
 
-		qcom_scm_io_readl(addr, &val);
-		val &= ~(intr_target_mask << g->intr_target_bit);
-		val |= g->intr_target_kpss_val << g->intr_target_bit;
-
-		ret = qcom_scm_io_writel(addr, val);
+		val = g->intr_target_kpss_val << g->intr_target_bit;
+		ret = qcom_scm_io_update_field(addr, intr_target_mask, val);
 		if (ret)
 			dev_err(pctrl->dev,
 				"Failed routing %lu interrupt to Apps proc",
 				d->hwirq);
 	} else {
 		val = msm_readl_intr_target(pctrl, g);
-		val &= ~(intr_target_mask << g->intr_target_bit);
+		val &= ~intr_target_mask;
 		val |= g->intr_target_kpss_val << g->intr_target_bit;
 		msm_writel_intr_target(val, pctrl, g);
 	}
-- 
2.7.4

[PATCH v5 15/17] firmware: scm: Modify only the download bits in TCSR register

[Mukesh Ojha]

Crashdump collection is based on the DLOAD bit of TCSR register.
To retain other bits, we read the register and modify only the
DLOAD bit as the other bits have their own significance.

Co-developed-by: Poovendhan Selvaraj <quic_poovendh@quicinc.com>
Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/firmware/qcom_scm.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 5ea8fc4fd4e8..eda92f713019 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -5,6 +5,8 @@
 #include <linux/platform_device.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
+#include <linux/bitfield.h>
+#include <linux/bits.h>
 #include <linux/completion.h>
 #include <linux/cpumask.h>
 #include <linux/export.h>
@@ -30,6 +32,10 @@ module_param(download_mode, bool, 0);
 #define SCM_HAS_IFACE_CLK	BIT(1)
 #define SCM_HAS_BUS_CLK		BIT(2)
 
+#define QCOM_DLOAD_MASK		GENMASK(5, 4)
+#define QCOM_DLOAD_FULLDUMP	0x1
+#define QCOM_DLOAD_NODUMP	0x0
+
 struct qcom_scm {
 	struct device *dev;
 	struct clk *core_clk;
@@ -444,6 +450,7 @@ static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
 
 static void qcom_scm_set_download_mode(bool enable)
 {
+	u32 val = enable ? QCOM_DLOAD_FULLDUMP : QCOM_DLOAD_NODUMP;
 	bool avail;
 	int ret = 0;
 
@@ -453,8 +460,9 @@ static void qcom_scm_set_download_mode(bool enable)
 	if (avail) {
 		ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
 	} else if (__scm->dload_mode_addr) {
-		ret = qcom_scm_io_writel(__scm->dload_mode_addr,
-				enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0);
+		ret = qcom_scm_io_update_field(__scm->dload_mode_addr,
+					       QCOM_DLOAD_MASK,
+					       FIELD_PREP(QCOM_DLOAD_MASK, val));
 	} else {
 		dev_err(__scm->dev,
 			"No available mechanism for setting download mode\n");
-- 
2.7.4

[PATCH v5 16/17] firmware: qcom_scm: Refactor code to support multiple download mode

[Mukesh Ojha]

Currently on Qualcomm SoC, download_mode is enabled if
CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT is selected.

Refactor the code such that it supports multiple download
modes and drop CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT config
instead, give interface to set the download mode from
module parameter.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/firmware/Kconfig    | 11 ---------
 drivers/firmware/qcom_scm.c | 56 +++++++++++++++++++++++++++++++++++++++------
 2 files changed, 49 insertions(+), 18 deletions(-)

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index b59e3041fd62..ff7e9f330559 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -215,17 +215,6 @@ config MTK_ADSP_IPC
 config QCOM_SCM
 	tristate
 
-config QCOM_SCM_DOWNLOAD_MODE_DEFAULT
-	bool "Qualcomm download mode enabled by default"
-	depends on QCOM_SCM
-	help
-	  A device with "download mode" enabled will upon an unexpected
-	  warm-restart enter a special debug mode that allows the user to
-	  "download" memory content over USB for offline postmortem analysis.
-	  The feature can be enabled/disabled on the kernel command line.
-
-	  Say Y here to enable "download mode" by default.
-
 config SYSFB
 	bool
 	select BOOT_VESA_SUPPORT
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index eda92f713019..689bf882cb69 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -20,13 +20,13 @@
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/clk.h>
+#include <linux/kstrtox.h>
 #include <linux/reset-controller.h>
 #include <linux/arm-smccc.h>
 
 #include "qcom_scm.h"
 
-static bool download_mode = IS_ENABLED(CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT);
-module_param(download_mode, bool, 0);
+static u32 download_mode;
 
 #define SCM_HAS_CORE_CLK	BIT(0)
 #define SCM_HAS_IFACE_CLK	BIT(1)
@@ -83,6 +83,11 @@ static const char * const qcom_scm_convention_names[] = {
 	[SMC_CONVENTION_LEGACY] = "smc legacy",
 };
 
+static const char * const download_mode_name[] = {
+	[QCOM_DLOAD_NODUMP]	= "off",
+	[QCOM_DLOAD_FULLDUMP]	= "full",
+};
+
 static struct qcom_scm *__scm;
 static DEFINE_SPINLOCK(lock);
 
@@ -448,9 +453,10 @@ static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
 	return qcom_scm_call_atomic(__scm->dev, &desc, NULL);
 }
 
-static void qcom_scm_set_download_mode(bool enable)
+static void qcom_scm_set_download_mode(u32 download_mode)
 {
-	u32 val = enable ? QCOM_DLOAD_FULLDUMP : QCOM_DLOAD_NODUMP;
+	bool enable = !!download_mode;
+	u32 val = download_mode;
 	bool avail;
 	int ret = 0;
 
@@ -1430,6 +1436,42 @@ static irqreturn_t qcom_scm_irq_handler(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+static int get_download_mode(char *buffer, const struct kernel_param *kp)
+{
+	if (download_mode >= ARRAY_SIZE(download_mode_name))
+		return sysfs_emit(buffer, "unknown mode\n");
+
+	return sysfs_emit(buffer, "%s\n", download_mode_name[download_mode]);
+}
+
+static int set_download_mode(const char *val, const struct kernel_param *kp)
+{
+	u32 old = download_mode;
+	int ret;
+
+	ret = sysfs_match_string(download_mode_name, val);
+	if (ret < 0) {
+		download_mode = old;
+		pr_err("qcom_scm: unknown download mode: %s\n", val);
+		return -EINVAL;
+	}
+
+	download_mode = ret;
+	if (__scm)
+		qcom_scm_set_download_mode(download_mode);
+
+	return 0;
+}
+
+static const struct kernel_param_ops download_mode_param_ops = {
+	.get = get_download_mode,
+	.set = set_download_mode,
+};
+
+module_param_cb(download_mode, &download_mode_param_ops, NULL, 0644);
+MODULE_PARM_DESC(download_mode,
+		"download mode: off/full are acceptable values");
+
 static int qcom_scm_probe(struct platform_device *pdev)
 {
 	struct qcom_scm *scm;
@@ -1523,12 +1565,12 @@ static int qcom_scm_probe(struct platform_device *pdev)
 	__get_convention();
 
 	/*
-	 * If requested enable "download mode", from this point on warmboot
+	 * If "download mode" is requested, from this point on warmboot
 	 * will cause the boot stages to enter download mode, unless
 	 * disabled below by a clean shutdown/reboot.
 	 */
 	if (download_mode)
-		qcom_scm_set_download_mode(true);
+		qcom_scm_set_download_mode(download_mode);
 
 	return 0;
 }
@@ -1536,7 +1578,7 @@ static int qcom_scm_probe(struct platform_device *pdev)
 static void qcom_scm_shutdown(struct platform_device *pdev)
 {
 	/* Clean shutdown, disable download mode to allow normal restart */
-	qcom_scm_set_download_mode(false);
+	qcom_scm_set_download_mode(QCOM_DLOAD_NODUMP);
 }
 
 static const struct of_device_id qcom_scm_dt_match[] = {
-- 
2.7.4

[PATCH v5 17/17] firmware: qcom_scm: Add multiple download mode support

[Mukesh Ojha]

Currently, scm driver only supports full dump when download
mode is selected. Add support to enable minidump as well as
enable it along with fulldump.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/firmware/qcom_scm.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 689bf882cb69..9faf0431d47a 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -34,6 +34,8 @@ static u32 download_mode;
 
 #define QCOM_DLOAD_MASK		GENMASK(5, 4)
 #define QCOM_DLOAD_FULLDUMP	0x1
+#define QCOM_DLOAD_MINIDUMP	0x2
+#define QCOM_DLOAD_BOTHDUMP	(QCOM_DLOAD_FULLDUMP | QCOM_DLOAD_MINIDUMP)
 #define QCOM_DLOAD_NODUMP	0x0
 
 struct qcom_scm {
@@ -86,6 +88,8 @@ static const char * const qcom_scm_convention_names[] = {
 static const char * const download_mode_name[] = {
 	[QCOM_DLOAD_NODUMP]	= "off",
 	[QCOM_DLOAD_FULLDUMP]	= "full",
+	[QCOM_DLOAD_MINIDUMP]	= "mini",
+	[QCOM_DLOAD_BOTHDUMP]	= "full,mini",
 };
 
 static struct qcom_scm *__scm;
@@ -1470,7 +1474,7 @@ static const struct kernel_param_ops download_mode_param_ops = {
 
 module_param_cb(download_mode, &download_mode_param_ops, NULL, 0644);
 MODULE_PARM_DESC(download_mode,
-		"download mode: off/full are acceptable values");
+		"download mode: off/full/mini/full,mini are acceptable values");
 
 static int qcom_scm_probe(struct platform_device *pdev)
 {
-- 
2.7.4

Re: [PATCH v5 08/17] arm64: mm: Add dynamic ramoops region support through command line

[Pavan Kondeti]

On Sun, Sep 10, 2023 at 01:46:09AM +0530, Mukesh Ojha wrote:
> The reserved memory region for ramoops is assumed to be at a fixed
> and known location when read from the devicetree. This may not be
> required for something like Qualcomm's minidump which is interested
> in knowing addresses of ramoops region but it does not put hard
> requirement of address being fixed as most of it's SoC does not
> support warm reset and does not use pstorefs at all instead it has
> firmware way of collecting ramoops region if it gets to know the
> address and register it with apss minidump table which is sitting
> in shared memory region in DDR and firmware will have access to
> these table during reset and collects it on crash of SoC.
> 
> So, add the support of reserving ramoops region to be dynamically
> allocated early during boot if it is request through command line
> via 'dyn_ramoops_size=' and fill up reserved resource structure and
> export the structure, so that it can be read by ramoops driver.
> 

This needs to be documented at
Documentation/admin-guide/kernel-parameters.txt

> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
>  arch/arm64/mm/init.c       | 94 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pstore_ram.h |  2 +
>  2 files changed, 96 insertions(+)
> 
> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> index d31c3a9290c5..14d7086758bf 100644
> --- a/arch/arm64/mm/init.c
> +++ b/arch/arm64/mm/init.c
> @@ -31,6 +31,7 @@
>  #include <linux/hugetlb.h>
>  #include <linux/acpi_iort.h>
>  #include <linux/kmemleak.h>
> +#include <linux/pstore_ram.h>
>  
>  #include <asm/boot.h>
>  #include <asm/fixmap.h>
> @@ -73,6 +74,93 @@ phys_addr_t __ro_after_init arm64_dma_phys_limit;
>  
>  #define DEFAULT_CRASH_KERNEL_LOW_SIZE	(128UL << 20)
>  
> +#define RAMOOPS_ADDR_HIGH_MAX		(PHYS_MASK + 1)
> +
> +/* Location of the reserved area for the dynamic ramoops */
> +struct resource dyn_ramoops_res = {
> +	.name  = "ramoops",
> +	.start = 0,
> +	.end   = 0,
> +	.flags = IORESOURCE_BUSY | IORESOURCE_SYSTEM_RAM,
> +	.desc  = IORES_DESC_NONE,
> +};
> +EXPORT_SYMBOL(dyn_ramoops_res);

Use EXPORT_SYMBOL_GPL.

> +
> +static int __init parse_dynamic_ramoops(char *cmdline, unsigned long long *size)
> +{
> +	const char *name = "dyn_ramoops_size=";
> +	char *p = NULL;
> +	char *q = NULL;
> +	char *tmp;
> +
> +	if (!cmdline)
> +		return -ENOENT;
> +
> +	/* Check for "dyn_ramoops_size" and use the later if there are more */
> +	p = strstr(cmdline, name);
> +	while (p) {
> +		q = p;
> +		p = strchr(p, ' ');
> +		if (!p)
> +			break;
> +
> +		p = strstr(p + 1, name);
> +	}
> +
> +	if (!q) {
> +		pr_err("ramoops: No entry found for %s\n", name);
> +		return -ENOENT;
> +	}
> +
> +	p = q + strlen(name);
> +	*size = memparse(p, &tmp);
> +	if (p == tmp) {
> +		pr_err("ramoops: memory value expected\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int __init parse_dyn_ramoops_size_dummy(char *arg)
> +{
> +	return 0;
> +}
> +early_param("dyn_ramoops_size", parse_dyn_ramoops_size_dummy);
> +

Any reason why we can't parse and cache the size in early param handler
it self?

> +/*
> + * reserve_dynamic_ramoops() - reserves memory for dynamic ramoops
> + *
> + * This enable dynamic reserve memory support for ramoops through
> + * command line.
> + */
> +static void __init reserve_dynamic_ramoops(void)
> +{
> +	char *cmdline = boot_command_line;
> +	unsigned long long ramoops_base;
> +	unsigned long long ramoops_size;
> +
> +	if (!IS_ENABLED(CONFIG_PSTORE_RAM))
> +		return;
> +

Should not most part of this patch be under CONFIG_PSTORE_RAM?

> +	if (parse_dynamic_ramoops(cmdline, &ramoops_size))
> +		return;
> +
> +	ramoops_base = memblock_phys_alloc_range(ramoops_size, SMP_CACHE_BYTES,
> +						 0, RAMOOPS_ADDR_HIGH_MAX);

It may be appropriate to use one of MEMBLOCK_ALLOC_xxx flags for the end
marker.

> +	if (!ramoops_base) {
> +		pr_err("cannot allocate ramoops dynamic memory (size:0x%llx).\n",
> +			ramoops_size);
> +		return;
> +	}
> +
> +	kmemleak_ignore_phys(ramoops_base);

Looks like you need MEMBLOCK_ALLOC_NOLEAKTRACE

> +
> +	dyn_ramoops_res.start = ramoops_base;
> +	dyn_ramoops_res.end = ramoops_base + ramoops_size - 1;
> +	insert_resource(&iomem_resource, &dyn_ramoops_res);
> +}
> +
>  static int __init reserve_crashkernel_low(unsigned long long low_size)
>  {
>  	unsigned long long low_base;
> @@ -456,6 +544,12 @@ void __init bootmem_init(void)
>  	 */
>  	reserve_crashkernel();
>  
> +	/*
> +	 * Reserving ramoops region resource dynamically in case it is
> +	 * requested from command line.
> +	 */
> +	reserve_dynamic_ramoops();
> +
>  	memblock_dump_all();
>  }
>  
> diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h
> index 9d65ff94e216..07d700b7649d 100644
> --- a/include/linux/pstore_ram.h
> +++ b/include/linux/pstore_ram.h
> @@ -10,6 +10,8 @@
>  
>  #include <linux/pstore.h>
>  
> +extern struct resource dyn_ramoops_res;
> +

What about other architectures?

>  struct persistent_ram_ecc_info {
>  	int block_size;
>  	int ecc_size;
> -- 
> 2.7.4
> 

Re: [PATCH v5 09/17] pstore/ram: Use dynamic ramoops reserve resource

[Pavan Kondeti]

On Sun, Sep 10, 2023 at 01:46:10AM +0530, Mukesh Ojha wrote:
> As dynamic ramoops command line parsing is now added, so
> lets add the support in ramoops driver to get the resource
> structure and add it during platform device registration.
> 
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
>  fs/pstore/ram.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 

Documentation/admin-guide/ramoops.rst might need an update as well.

> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index 2f625e1fa8d8..e73fbbc1b5b5 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -913,13 +913,17 @@ static void __init ramoops_register_dummy(void)
>  
>  	/*
>  	 * Prepare a dummy platform data structure to carry the module
> -	 * parameters. If mem_size isn't set, then there are no module
> -	 * parameters, and we can skip this.
> +	 * parameters. If mem_size isn't set, check for dynamic ramoops
> +	 * size and extract the information if it is set.
>  	 */
> -	if (!mem_size)
> +	if (!mem_size && !dyn_ramoops_res.end)
>  		return;
>  
>  	pr_info("using module parameters\n");
> +	if (dyn_ramoops_res.end) {
> +		mem_size = resource_size(&dyn_ramoops_res);
> +		mem_address = dyn_ramoops_res.start;
> +	}
>  
>  	memset(&pdata, 0, sizeof(pdata));
>  	pdata.mem_size = mem_size;

You might want to add "arch_" prefix to dyn_ramoops resource so that it
would be clear that it is coming from arch code. This code needs to
guard against arch not supplying this.

Thanks,
Pavan

Re: [PATCH v5 11/17] qcom_minidump: Register ramoops region with minidump

[Pavan Kondeti]

On Sun, Sep 10, 2023 at 01:46:12AM +0530, Mukesh Ojha wrote:
> Register all the pstore frontend with minidump, so that they can
> be dumped as default Linux minidump region to be collected on
> SoC where minidump is enabled.
> 
> Helper functions is written in separate file and built along with
> the minidump driver, since it is client of minidump and also call
> it at appropriate place from minidump probe so that they always
> get registered.
> 
> While at it also rename the out minidump module object name during
> build as qcom_apss_minidump which basically depicts as Qualcomm
> Application processor subsystem minidump.
> 
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
>  drivers/soc/qcom/Kconfig                 |  1 +
>  drivers/soc/qcom/Makefile                |  3 +-
>  drivers/soc/qcom/qcom_minidump.c         |  4 ++
>  drivers/soc/qcom/qcom_ramoops_minidump.c | 88 ++++++++++++++++++++++++++++++++
>  drivers/soc/qcom/qcom_ramoops_minidump.h | 10 ++++
>  5 files changed, 105 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/soc/qcom/qcom_ramoops_minidump.c
>  create mode 100644 drivers/soc/qcom/qcom_ramoops_minidump.h
> 
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index 4b36d46807bc..b3977f1687d8 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -305,6 +305,7 @@ config QCOM_MINIDUMP
>  	tristate "QCOM APSS Minidump driver"
>  	depends on ARCH_QCOM || COMPILE_TEST
>  	depends on QCOM_SMEM
> +	depends on PSTORE

Can't we make QC minidump available without PSTORE? PSTORE is another
cllient for minidump, so other clients can still use minidump right?
Where is this hard dependency coming from?

Thanks,
Pavan

Re: [PATCH v5 11/17] qcom_minidump: Register ramoops region with minidump

[Mukesh Ojha]

On 9/11/2023 11:31 AM, Pavan Kondeti wrote:
> On Sun, Sep 10, 2023 at 01:46:12AM +0530, Mukesh Ojha wrote:
>> Register all the pstore frontend with minidump, so that they can
>> be dumped as default Linux minidump region to be collected on
>> SoC where minidump is enabled.
>>
>> Helper functions is written in separate file and built along with
>> the minidump driver, since it is client of minidump and also call
>> it at appropriate place from minidump probe so that they always
>> get registered.
>>
>> While at it also rename the out minidump module object name during
>> build as qcom_apss_minidump which basically depicts as Qualcomm
>> Application processor subsystem minidump.
>>
>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>> ---
>>   drivers/soc/qcom/Kconfig                 |  1 +
>>   drivers/soc/qcom/Makefile                |  3 +-
>>   drivers/soc/qcom/qcom_minidump.c         |  4 ++
>>   drivers/soc/qcom/qcom_ramoops_minidump.c | 88 ++++++++++++++++++++++++++++++++
>>   drivers/soc/qcom/qcom_ramoops_minidump.h | 10 ++++
>>   5 files changed, 105 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/soc/qcom/qcom_ramoops_minidump.c
>>   create mode 100644 drivers/soc/qcom/qcom_ramoops_minidump.h
>>
>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>> index 4b36d46807bc..b3977f1687d8 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -305,6 +305,7 @@ config QCOM_MINIDUMP
>>   	tristate "QCOM APSS Minidump driver"
>>   	depends on ARCH_QCOM || COMPILE_TEST
>>   	depends on QCOM_SMEM
>> +	depends on PSTORE
> 
> Can't we make QC minidump available without PSTORE? PSTORE is another
> cllient for minidump, so other clients can still use minidump right?
> Where is this hard dependency coming from?

Thanks for asking this question, this was intentionally put here to
continue the discussion forward about minidump existence in kernel tree
and how community want to see it in kernel tree.

Actually, there is no hard dependency of minidump on pstore and  as i 
have said in cover-letter about minidump is going to collect what 
ramoops is offering now or going to offer in future as a default
regions.

So, main minidump driver does not depends on pstore since, i have 
integrated even the minidump client with this
qcom_ramoops_minidump_{register|unregister}()

I may guard this entire qcom_ramoops_minidump_{register|unregister}()
function under CONFIG_PSTORE_RAM, if everyone agree ?

-Mukesh
> 
> Thanks,
> Pavan

Re: [PATCH v5 00/17] Add Qualcomm Minidump kernel driver related support

[Kathiravan Thirumoorthy]

On 9/10/2023 1:46 AM, Mukesh Ojha wrote:
> Hi All,
>
> This is to continuation from the conversation happened at v4
>
> https://lore.kernel.org/lkml/632c5b97-4a91-c3e8-1e6c-33d6c4f6454f@quicinc.com/
>
> https://lore.kernel.org/lkml/695133e6-105f-de2a-5559-555cea0a0462@quicinc.com/
>
> We have put abstract on LPC on this topic as well as initiated a mail thread
> with other SoC vendors but did not get much traction on it.
>
> https://lore.kernel.org/lkml/0199db00-1b1d-0c63-58ff-03efae02cb21@quicinc.com/
>
> We explored most of possiblity present in kernel to address this issue[1] but
> solution like kdump/fadump does not seems safe/secure/performant from our
> perspective.
>
> Hence, with this series we tried to make the minidump kernel driver, simple
> and tied with pstore frontends, so that it collects the present available
> frontends data like dmesg, ftrace, pmsg, ftrace., Also, we will be working
> towards enhancing generic pstore to capture more debug data which will be
> helpful for first hand of debugging that can benefit both other pstore users
> as well as us as minidump users.
>
> One of the proposal made here,
> https://lore.kernel.org/lkml/1683561060-2197-1-git-send-email-quic_mojha@quicinc.com/
>
> Looking forward for your comments.
>
> Thanks,
> Mukesh
>
> [1]
> 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 SMEM based 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 SMEM
> based 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.
>
> Changes in v5:
>   - On suggestion from Pavan.k, to have single function call for minidump collection
>     from remoteproc driver, separated the logic to have separate minidump file called
>     qcom_rproc_minidump.c and also renamed the function from qcom_minidump() to
>     qcom_rproc_minidump(); however, dropped his suggestion about rework on lazy deletion
>     during region unregister in this series, will pursue it in next series.
>
>   - To simplify the minidump driver, removed the complication for frontend and different
>     backend from Greg suggestion, will pursue this once main driver gets mainlined.
>
>   - Move the dynamic ramoops region allocation from Device tree approach to command line
>     approch with the introduction command line parsing and memblock reservation during
>     early boot up; Not added documentation about it yet, will add if it gets positive
>     response.
>
>   - Exporting linux banner from kernel to make minidump build also as module, however,
>     minidump is a debug module and should be kernel built to get most debug information
>     from kernel.
>
>   - Tried to address comments given on dload patch series.
>
> Changes in v4: https://lore.kernel.org/lkml/1687955688-20809-1-git-send-email-quic_mojha@quicinc.com/
>   - Redesigned the driver and divided the driver into front end and backend (smem) so
>     that any new backend can be attached easily to avoid code duplication.
>   - Patch reordering as per the driver and subsystem to easier review of the code.
>   - Removed minidump specific code from remoteproc to minidump smem based driver.
>   - Enabled the all the driver as modules.
>   - Address comments made on documentation and yaml and Device tree file [Krzysztof/Konrad]
>   - Address comments made qcom_pstore_minidump driver and given its Device tree
>     same set of properties as ramoops. [Luca/Kees]
>   - Added patch for MAINTAINER file.
>   - Include defconfig change as one patch as per [Krzysztof] suggestion.
>   - Tried to remove the redundant file scope variables from the module as per [Krzysztof] suggestion.
>   - Addressed comments made on dload mode patch v6 version
>     https://lore.kernel.org/lkml/1680076012-10785-1-git-send-email-quic_mojha@quicinc.com/
>
> Changes in v3: https://lore.kernel.org/lkml/1683133352-10046-1-git-send-email-quic_mojha@quicinc.com/
>   - Addressed most of the comments by Srini on v2 and refactored the minidump driver.
>      - Added platform device support
>      - Unregister region support.
>   - Added update region for clients.
>   - Added pending region support.
>   - Modified the documentation guide accordingly.
>   - Added qcom_pstore_ramdump client driver which happen to add ramoops platform
>     device and also registers ramoops region with minidump.
>   - Added download mode patch series with this minidump series.
>      https://lore.kernel.org/lkml/1680076012-10785-1-git-send-email-quic_mojha@quicinc.com/
>
> Changes in v2: https://lore.kernel.org/lkml/1679491817-2498-1-git-send-email-quic_mojha@quicinc.com/
>   - 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/
>
> Testing of the patches has been done on sm8450 target after enabling config like
> CONFIG_PSTORE_RAM and CONFIG_PSTORE_CONSOLE and once the device boots up.
>
>   echo mini > /sys/module/qcom_scm/parameters/download_mode
>
> Try crashing it via devmem2 0xf11c000(this is known to create xpu violation and
> and put the device in download mode) on command prompt.
>
> Default storage type is set to via USB, so minidump would be downloaded with the
> help of x86_64 machine (running PCAT tool) attached to Qualcomm device which has
> backed minidump boot firmware support.
>
> This will make the device go to download mode and collect the minidump on to the
> attached x86 machine running the Qualcomm PCAT tool(This comes as part Qualcomm
> package manager kit).
>
> After that we will see a bunch of predefined registered region as binary blobs files
> starts with md_* downloaded on the x86 machine on given location in PCAT tool from
> the target device, more about this can be found in qualcomm minidump guide patch.
>
> Mukesh Ojha (17):
>    docs: qcom: Add qualcomm minidump guide
>    soc: qcom: Add qcom_rproc_minidump module
>    remoteproc: qcom_q6v5_pas: Use qcom_rproc_minidump()
>    remoteproc: qcom: Remove minidump related data from qcom_common.c
>    init: export linux_banner data variable
>    soc: qcom: Add Qualcomm APSS minidump kernel driver
>    soc: qcom: minidump: Add pending region registration
>    arm64: mm: Add dynamic ramoops region support through command line
>    pstore/ram: Use dynamic ramoops reserve resource
>    pstore: Add pstore_region_defined() helper and export it
>    qcom_minidump: Register ramoops region with minidump
>    MAINTAINERS: Add entry for minidump related files


Based on last discussion[1], I posted the V5 of the below three 
patches[2] separately. I don't mind clubbing these again back to 
Minidump series as long as three patches can be picked up separately.


>    firmware: qcom_scm: provide a read-modify-write function
>    pinctrl: qcom: Use qcom_scm_io_update_field()
>    firmware: scm: Modify only the download bits in TCSR register

[1] 
https://lore.kernel.org/linux-arm-msm/d77f5601-2b08-a7c7-1400-7ab68b8add3a@quicinc.com/

[2] 
https://lore.kernel.org/linux-arm-msm/20230720070408.1093698-1-quic_kathirav@quicinc.com/


>    firmware: qcom_scm: Refactor code to support multiple download mode
>    firmware: qcom_scm: Add multiple download mode support
>
>   Documentation/admin-guide/index.rst         |   1 +
>   Documentation/admin-guide/qcom_minidump.rst | 272 +++++++++++
>   MAINTAINERS                                 |  10 +
>   arch/arm64/mm/init.c                        |  94 ++++
>   drivers/firmware/Kconfig                    |  11 -
>   drivers/firmware/qcom_scm.c                 |  90 +++-
>   drivers/pinctrl/qcom/pinctrl-msm.c          |  10 +-
>   drivers/remoteproc/Kconfig                  |   1 +
>   drivers/remoteproc/qcom_common.c            | 150 ------
>   drivers/remoteproc/qcom_q6v5_pas.c          |   3 +-
>   drivers/soc/qcom/Kconfig                    |  24 +
>   drivers/soc/qcom/Makefile                   |   3 +
>   drivers/soc/qcom/qcom_minidump.c            | 727 ++++++++++++++++++++++++++++
>   drivers/soc/qcom/qcom_minidump_internal.h   |  74 +++
>   drivers/soc/qcom/qcom_ramoops_minidump.c    |  88 ++++
>   drivers/soc/qcom/qcom_ramoops_minidump.h    |  10 +
>   drivers/soc/qcom/qcom_rproc_minidump.c      | 111 +++++
>   drivers/soc/qcom/smem.c                     |  18 +
>   fs/pstore/platform.c                        |  15 +
>   fs/pstore/ram.c                             |  52 +-
>   include/linux/firmware/qcom/qcom_scm.h      |   2 +
>   include/linux/init.h                        |   3 +
>   include/linux/pstore.h                      |   6 +
>   include/linux/pstore_ram.h                  |   2 +
>   include/linux/soc/qcom/smem.h               |   2 +
>   include/soc/qcom/qcom_minidump.h            |  56 +++
>   init/version-timestamp.c                    |   3 +
>   27 files changed, 1659 insertions(+), 179 deletions(-)
>   create mode 100644 Documentation/admin-guide/qcom_minidump.rst
>   create mode 100644 drivers/soc/qcom/qcom_minidump.c
>   create mode 100644 drivers/soc/qcom/qcom_minidump_internal.h
>   create mode 100644 drivers/soc/qcom/qcom_ramoops_minidump.c
>   create mode 100644 drivers/soc/qcom/qcom_ramoops_minidump.h
>   create mode 100644 drivers/soc/qcom/qcom_rproc_minidump.c
>   create mode 100644 include/soc/qcom/qcom_minidump.h
>

Re: [PATCH v5 13/17] firmware: qcom_scm: provide a read-modify-write function

[Kathiravan Thirumoorthy]

On 9/10/2023 1:46 AM, Mukesh Ojha wrote:
> It was realized by Srinivas K. that there is a need of
> read-modify-write scm exported function so that it can
> be used by multiple clients.
>
> Let's introduce qcom_scm_io_update_field() which masks
> out the bits and write the passed value to that
> bit-offset. Subsequent patch will use this function.
>
> Suggested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---


Validated this change on IPQ9574 and IPQ53332 and system is entering 
into the download mode.

Tested-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> # IP9574 
and IPQ5332


>   drivers/firmware/qcom_scm.c            | 20 ++++++++++++++++++++
>   include/linux/firmware/qcom/qcom_scm.h |  2 ++
>   2 files changed, 22 insertions(+)
>
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index fde33acd46b7..5ea8fc4fd4e8 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -78,6 +78,7 @@ static const char * const qcom_scm_convention_names[] = {
>   };
>   
>   static struct qcom_scm *__scm;
> +static DEFINE_SPINLOCK(lock);
>   
>   static int qcom_scm_clk_enable(void)
>   {
> @@ -407,6 +408,25 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
>   }
>   EXPORT_SYMBOL(qcom_scm_set_remote_state);
>   
> +int qcom_scm_io_update_field(phys_addr_t addr, unsigned int mask, unsigned int val)
> +{
> +	unsigned int old, new;
> +	int ret;
> +
> +	spin_lock(&lock);
> +	ret = qcom_scm_io_readl(addr, &old);
> +	if (ret)
> +		goto unlock;
> +
> +	new = (old & ~mask) | (val & mask);
> +
> +	ret = qcom_scm_io_writel(addr, new);
> +unlock:
> +	spin_unlock(&lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(qcom_scm_io_update_field);
> +
>   static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
>   {
>   	struct qcom_scm_desc desc = {
> diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
> index 250ea4efb7cb..ca41e4eb33ad 100644
> --- a/include/linux/firmware/qcom/qcom_scm.h
> +++ b/include/linux/firmware/qcom/qcom_scm.h
> @@ -84,6 +84,8 @@ extern bool qcom_scm_pas_supported(u32 peripheral);
>   
>   extern int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val);
>   extern int qcom_scm_io_writel(phys_addr_t addr, unsigned int val);
> +extern int qcom_scm_io_update_field(phys_addr_t addr, unsigned int mask,
> +				    unsigned int val);
>   
>   extern bool qcom_scm_restore_sec_cfg_available(void);
>   extern int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare);

Re: [PATCH v5 15/17] firmware: scm: Modify only the download bits in TCSR register

[Kathiravan Thirumoorthy]

On 9/10/2023 1:46 AM, Mukesh Ojha wrote:
> Crashdump collection is based on the DLOAD bit of TCSR register.
> To retain other bits, we read the register and modify only the
> DLOAD bit as the other bits have their own significance.
>
> Co-developed-by: Poovendhan Selvaraj <quic_poovendh@quicinc.com>
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---


This change doesn't cleanly apply on next-20230911. Please rebase it.

Validated this change on IPQ9574 and IPQ5332 and system is entering into 
the download mode.

Tested-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> # IPQ9574 
and IPQ5332


>   drivers/firmware/qcom_scm.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index 5ea8fc4fd4e8..eda92f713019 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -5,6 +5,8 @@
>   #include <linux/platform_device.h>
>   #include <linux/init.h>
>   #include <linux/interrupt.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
>   #include <linux/completion.h>
>   #include <linux/cpumask.h>
>   #include <linux/export.h>
> @@ -30,6 +32,10 @@ module_param(download_mode, bool, 0);
>   #define SCM_HAS_IFACE_CLK	BIT(1)
>   #define SCM_HAS_BUS_CLK		BIT(2)
>   
> +#define QCOM_DLOAD_MASK		GENMASK(5, 4)
> +#define QCOM_DLOAD_FULLDUMP	0x1
> +#define QCOM_DLOAD_NODUMP	0x0
> +
>   struct qcom_scm {
>   	struct device *dev;
>   	struct clk *core_clk;
> @@ -444,6 +450,7 @@ static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
>   
>   static void qcom_scm_set_download_mode(bool enable)
>   {
> +	u32 val = enable ? QCOM_DLOAD_FULLDUMP : QCOM_DLOAD_NODUMP;
>   	bool avail;
>   	int ret = 0;
>   
> @@ -453,8 +460,9 @@ static void qcom_scm_set_download_mode(bool enable)
>   	if (avail) {
>   		ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
>   	} else if (__scm->dload_mode_addr) {
> -		ret = qcom_scm_io_writel(__scm->dload_mode_addr,
> -				enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0);
> +		ret = qcom_scm_io_update_field(__scm->dload_mode_addr,
> +					       QCOM_DLOAD_MASK,
> +					       FIELD_PREP(QCOM_DLOAD_MASK, val));
>   	} else {
>   		dev_err(__scm->dev,
>   			"No available mechanism for setting download mode\n");

Re: [PATCH v5 13/17] firmware: qcom_scm: provide a read-modify-write function

[Kathiravan Thirumoorthy]

On 9/11/2023 1:38 PM, Kathiravan Thirumoorthy wrote:
>
> On 9/10/2023 1:46 AM, Mukesh Ojha wrote:
>> It was realized by Srinivas K. that there is a need of
>> read-modify-write scm exported function so that it can
>> be used by multiple clients.
>>
>> Let's introduce qcom_scm_io_update_field() which masks
>> out the bits and write the passed value to that
>> bit-offset. Subsequent patch will use this function.
>>
>> Suggested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>> ---
>
>
> Validated this change on IPQ9574 and IPQ53332 and system is entering 
> into the download mode.
>
> Tested-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> # 
> IP9574 and IPQ5332


Couple of typos. Apologize...

Validated this change on IPQ9574 and IPQ5332 and system is entering into 
the download mode.

Tested-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> # IPQ9574 
and IPQ5332


>
>
>>   drivers/firmware/qcom_scm.c            | 20 ++++++++++++++++++++
>>   include/linux/firmware/qcom/qcom_scm.h |  2 ++
>>   2 files changed, 22 insertions(+)
>>
>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
>> index fde33acd46b7..5ea8fc4fd4e8 100644
>> --- a/drivers/firmware/qcom_scm.c
>> +++ b/drivers/firmware/qcom_scm.c
>> @@ -78,6 +78,7 @@ static const char * const 
>> qcom_scm_convention_names[] = {
>>   };
>>     static struct qcom_scm *__scm;
>> +static DEFINE_SPINLOCK(lock);
>>     static int qcom_scm_clk_enable(void)
>>   {
>> @@ -407,6 +408,25 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
>>   }
>>   EXPORT_SYMBOL(qcom_scm_set_remote_state);
>>   +int qcom_scm_io_update_field(phys_addr_t addr, unsigned int mask, 
>> unsigned int val)
>> +{
>> +    unsigned int old, new;
>> +    int ret;
>> +
>> +    spin_lock(&lock);
>> +    ret = qcom_scm_io_readl(addr, &old);
>> +    if (ret)
>> +        goto unlock;
>> +
>> +    new = (old & ~mask) | (val & mask);
>> +
>> +    ret = qcom_scm_io_writel(addr, new);
>> +unlock:
>> +    spin_unlock(&lock);
>> +    return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(qcom_scm_io_update_field);
>> +
>>   static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
>>   {
>>       struct qcom_scm_desc desc = {
>> diff --git a/include/linux/firmware/qcom/qcom_scm.h 
>> b/include/linux/firmware/qcom/qcom_scm.h
>> index 250ea4efb7cb..ca41e4eb33ad 100644
>> --- a/include/linux/firmware/qcom/qcom_scm.h
>> +++ b/include/linux/firmware/qcom/qcom_scm.h
>> @@ -84,6 +84,8 @@ extern bool qcom_scm_pas_supported(u32 peripheral);
>>     extern int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val);
>>   extern int qcom_scm_io_writel(phys_addr_t addr, unsigned int val);
>> +extern int qcom_scm_io_update_field(phys_addr_t addr, unsigned int 
>> mask,
>> +                    unsigned int val);
>>     extern bool qcom_scm_restore_sec_cfg_available(void);
>>   extern int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare);

Re: [PATCH v5 15/17] firmware: scm: Modify only the download bits in TCSR register

[Kathiravan Thirumoorthy]

On 9/10/2023 1:46 AM, Mukesh Ojha wrote:
> Crashdump collection is based on the DLOAD bit of TCSR register.
> To retain other bits, we read the register and modify only the
> DLOAD bit as the other bits have their own significance.
>
> Co-developed-by: Poovendhan Selvaraj <quic_poovendh@quicinc.com>


Poovendhan's signed-off is also required. Please refer [1]

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/process/submitting-patches.rst#n489


> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
>   drivers/firmware/qcom_scm.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index 5ea8fc4fd4e8..eda92f713019 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -5,6 +5,8 @@
>   #include <linux/platform_device.h>
>   #include <linux/init.h>
>   #include <linux/interrupt.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
>   #include <linux/completion.h>
>   #include <linux/cpumask.h>
>   #include <linux/export.h>
> @@ -30,6 +32,10 @@ module_param(download_mode, bool, 0);
>   #define SCM_HAS_IFACE_CLK	BIT(1)
>   #define SCM_HAS_BUS_CLK		BIT(2)
>   
> +#define QCOM_DLOAD_MASK		GENMASK(5, 4)
> +#define QCOM_DLOAD_FULLDUMP	0x1
> +#define QCOM_DLOAD_NODUMP	0x0
> +
>   struct qcom_scm {
>   	struct device *dev;
>   	struct clk *core_clk;
> @@ -444,6 +450,7 @@ static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
>   
>   static void qcom_scm_set_download_mode(bool enable)
>   {
> +	u32 val = enable ? QCOM_DLOAD_FULLDUMP : QCOM_DLOAD_NODUMP;
>   	bool avail;
>   	int ret = 0;
>   
> @@ -453,8 +460,9 @@ static void qcom_scm_set_download_mode(bool enable)
>   	if (avail) {
>   		ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
>   	} else if (__scm->dload_mode_addr) {
> -		ret = qcom_scm_io_writel(__scm->dload_mode_addr,
> -				enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0);
> +		ret = qcom_scm_io_update_field(__scm->dload_mode_addr,
> +					       QCOM_DLOAD_MASK,
> +					       FIELD_PREP(QCOM_DLOAD_MASK, val));
>   	} else {
>   		dev_err(__scm->dev,
>   			"No available mechanism for setting download mode\n");

Re: [PATCH v5 00/17] Add Qualcomm Minidump kernel driver related support

[Bagas Sanjaya]

[-- Attachment #1: Type: text/plain, Size: 8232 bytes --]

On Sun, Sep 10, 2023 at 01:46:01AM +0530, Mukesh Ojha wrote:
> Hi All,
> 
> This is to continuation from the conversation happened at v4
> 
> https://lore.kernel.org/lkml/632c5b97-4a91-c3e8-1e6c-33d6c4f6454f@quicinc.com/
> 
> https://lore.kernel.org/lkml/695133e6-105f-de2a-5559-555cea0a0462@quicinc.com/
> 
> We have put abstract on LPC on this topic as well as initiated a mail thread
> with other SoC vendors but did not get much traction on it.
> 
> https://lore.kernel.org/lkml/0199db00-1b1d-0c63-58ff-03efae02cb21@quicinc.com/
> 
> We explored most of possiblity present in kernel to address this issue[1] but
> solution like kdump/fadump does not seems safe/secure/performant from our
> perspective.
> 
> Hence, with this series we tried to make the minidump kernel driver, simple
> and tied with pstore frontends, so that it collects the present available
> frontends data like dmesg, ftrace, pmsg, ftrace., Also, we will be working
> towards enhancing generic pstore to capture more debug data which will be
> helpful for first hand of debugging that can benefit both other pstore users
> as well as us as minidump users.
> 
> One of the proposal made here,
> https://lore.kernel.org/lkml/1683561060-2197-1-git-send-email-quic_mojha@quicinc.com/
> 
> Looking forward for your comments.
> 
> Thanks,
> Mukesh
> 
> [1]
> 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 SMEM based 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 SMEM
> based 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.
> 
> Changes in v5:
>  - On suggestion from Pavan.k, to have single function call for minidump collection
>    from remoteproc driver, separated the logic to have separate minidump file called
>    qcom_rproc_minidump.c and also renamed the function from qcom_minidump() to 
>    qcom_rproc_minidump(); however, dropped his suggestion about rework on lazy deletion
>    during region unregister in this series, will pursue it in next series.
> 
>  - To simplify the minidump driver, removed the complication for frontend and different
>    backend from Greg suggestion, will pursue this once main driver gets mainlined.
> 
>  - Move the dynamic ramoops region allocation from Device tree approach to command line
>    approch with the introduction command line parsing and memblock reservation during
>    early boot up; Not added documentation about it yet, will add if it gets positive
>    response.
> 
>  - Exporting linux banner from kernel to make minidump build also as module, however,
>    minidump is a debug module and should be kernel built to get most debug information
>    from kernel.
> 
>  - Tried to address comments given on dload patch series. 
> 
> Changes in v4: https://lore.kernel.org/lkml/1687955688-20809-1-git-send-email-quic_mojha@quicinc.com/
>  - Redesigned the driver and divided the driver into front end and backend (smem) so
>    that any new backend can be attached easily to avoid code duplication.
>  - Patch reordering as per the driver and subsystem to easier review of the code.
>  - Removed minidump specific code from remoteproc to minidump smem based driver.
>  - Enabled the all the driver as modules.
>  - Address comments made on documentation and yaml and Device tree file [Krzysztof/Konrad]
>  - Address comments made qcom_pstore_minidump driver and given its Device tree
>    same set of properties as ramoops. [Luca/Kees]
>  - Added patch for MAINTAINER file.
>  - Include defconfig change as one patch as per [Krzysztof] suggestion.
>  - Tried to remove the redundant file scope variables from the module as per [Krzysztof] suggestion.
>  - Addressed comments made on dload mode patch v6 version
>    https://lore.kernel.org/lkml/1680076012-10785-1-git-send-email-quic_mojha@quicinc.com/
> 
> Changes in v3: https://lore.kernel.org/lkml/1683133352-10046-1-git-send-email-quic_mojha@quicinc.com/
>  - Addressed most of the comments by Srini on v2 and refactored the minidump driver.
>     - Added platform device support
>     - Unregister region support.
>  - Added update region for clients.
>  - Added pending region support.
>  - Modified the documentation guide accordingly.
>  - Added qcom_pstore_ramdump client driver which happen to add ramoops platform
>    device and also registers ramoops region with minidump.
>  - Added download mode patch series with this minidump series.
>     https://lore.kernel.org/lkml/1680076012-10785-1-git-send-email-quic_mojha@quicinc.com/
> 
> Changes in v2: https://lore.kernel.org/lkml/1679491817-2498-1-git-send-email-quic_mojha@quicinc.com/
>  - 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/
> 
> Testing of the patches has been done on sm8450 target after enabling config like
> CONFIG_PSTORE_RAM and CONFIG_PSTORE_CONSOLE and once the device boots up.
> 
>  echo mini > /sys/module/qcom_scm/parameters/download_mode
> 
> Try crashing it via devmem2 0xf11c000(this is known to create xpu violation and
> and put the device in download mode) on command prompt.
> 
> Default storage type is set to via USB, so minidump would be downloaded with the
> help of x86_64 machine (running PCAT tool) attached to Qualcomm device which has
> backed minidump boot firmware support.
> 
> This will make the device go to download mode and collect the minidump on to the
> attached x86 machine running the Qualcomm PCAT tool(This comes as part Qualcomm
> package manager kit).
> 
> After that we will see a bunch of predefined registered region as binary blobs files
> starts with md_* downloaded on the x86 machine on given location in PCAT tool from
> the target device, more about this can be found in qualcomm minidump guide patch.
> 

I tried to apply this series on top of 535a265d7f0dd50 (as suggested by
`b4 am -l -g`), but it conflicts on patch [04/17]. Please specify the
exact base commit or another series for which this series is based on.

Thanks.

-- 
An old man doll... just what I always wanted! - Clara

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v5 00/17] Add Qualcomm Minidump kernel driver related support

[Mukesh Ojha]

On 9/11/2023 2:22 PM, Bagas Sanjaya wrote:
> On Sun, Sep 10, 2023 at 01:46:01AM +0530, Mukesh Ojha wrote:
>> Hi All,
>>
>> This is to continuation from the conversation happened at v4
>>
>> https://lore.kernel.org/lkml/632c5b97-4a91-c3e8-1e6c-33d6c4f6454f@quicinc.com/
>>
>> https://lore.kernel.org/lkml/695133e6-105f-de2a-5559-555cea0a0462@quicinc.com/
>>
>> We have put abstract on LPC on this topic as well as initiated a mail thread
>> with other SoC vendors but did not get much traction on it.
>>
>> https://lore.kernel.org/lkml/0199db00-1b1d-0c63-58ff-03efae02cb21@quicinc.com/
>>
>> We explored most of possiblity present in kernel to address this issue[1] but
>> solution like kdump/fadump does not seems safe/secure/performant from our
>> perspective.
>>
>> Hence, with this series we tried to make the minidump kernel driver, simple
>> and tied with pstore frontends, so that it collects the present available
>> frontends data like dmesg, ftrace, pmsg, ftrace., Also, we will be working
>> towards enhancing generic pstore to capture more debug data which will be
>> helpful for first hand of debugging that can benefit both other pstore users
>> as well as us as minidump users.
>>
>> One of the proposal made here,
>> https://lore.kernel.org/lkml/1683561060-2197-1-git-send-email-quic_mojha@quicinc.com/
>>
>> Looking forward for your comments.
>>
>> Thanks,
>> Mukesh
>>
>> [1]
>> 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 SMEM based 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 SMEM
>> based 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.
>>
>> Changes in v5:
>>   - On suggestion from Pavan.k, to have single function call for minidump collection
>>     from remoteproc driver, separated the logic to have separate minidump file called
>>     qcom_rproc_minidump.c and also renamed the function from qcom_minidump() to
>>     qcom_rproc_minidump(); however, dropped his suggestion about rework on lazy deletion
>>     during region unregister in this series, will pursue it in next series.
>>
>>   - To simplify the minidump driver, removed the complication for frontend and different
>>     backend from Greg suggestion, will pursue this once main driver gets mainlined.
>>
>>   - Move the dynamic ramoops region allocation from Device tree approach to command line
>>     approch with the introduction command line parsing and memblock reservation during
>>     early boot up; Not added documentation about it yet, will add if it gets positive
>>     response.
>>
>>   - Exporting linux banner from kernel to make minidump build also as module, however,
>>     minidump is a debug module and should be kernel built to get most debug information
>>     from kernel.
>>
>>   - Tried to address comments given on dload patch series.
>>
>> Changes in v4: https://lore.kernel.org/lkml/1687955688-20809-1-git-send-email-quic_mojha@quicinc.com/
>>   - Redesigned the driver and divided the driver into front end and backend (smem) so
>>     that any new backend can be attached easily to avoid code duplication.
>>   - Patch reordering as per the driver and subsystem to easier review of the code.
>>   - Removed minidump specific code from remoteproc to minidump smem based driver.
>>   - Enabled the all the driver as modules.
>>   - Address comments made on documentation and yaml and Device tree file [Krzysztof/Konrad]
>>   - Address comments made qcom_pstore_minidump driver and given its Device tree
>>     same set of properties as ramoops. [Luca/Kees]
>>   - Added patch for MAINTAINER file.
>>   - Include defconfig change as one patch as per [Krzysztof] suggestion.
>>   - Tried to remove the redundant file scope variables from the module as per [Krzysztof] suggestion.
>>   - Addressed comments made on dload mode patch v6 version
>>     https://lore.kernel.org/lkml/1680076012-10785-1-git-send-email-quic_mojha@quicinc.com/
>>
>> Changes in v3: https://lore.kernel.org/lkml/1683133352-10046-1-git-send-email-quic_mojha@quicinc.com/
>>   - Addressed most of the comments by Srini on v2 and refactored the minidump driver.
>>      - Added platform device support
>>      - Unregister region support.
>>   - Added update region for clients.
>>   - Added pending region support.
>>   - Modified the documentation guide accordingly.
>>   - Added qcom_pstore_ramdump client driver which happen to add ramoops platform
>>     device and also registers ramoops region with minidump.
>>   - Added download mode patch series with this minidump series.
>>      https://lore.kernel.org/lkml/1680076012-10785-1-git-send-email-quic_mojha@quicinc.com/
>>
>> Changes in v2: https://lore.kernel.org/lkml/1679491817-2498-1-git-send-email-quic_mojha@quicinc.com/
>>   - 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/
>>
>> Testing of the patches has been done on sm8450 target after enabling config like
>> CONFIG_PSTORE_RAM and CONFIG_PSTORE_CONSOLE and once the device boots up.
>>
>>   echo mini > /sys/module/qcom_scm/parameters/download_mode
>>
>> Try crashing it via devmem2 0xf11c000(this is known to create xpu violation and
>> and put the device in download mode) on command prompt.
>>
>> Default storage type is set to via USB, so minidump would be downloaded with the
>> help of x86_64 machine (running PCAT tool) attached to Qualcomm device which has
>> backed minidump boot firmware support.
>>
>> This will make the device go to download mode and collect the minidump on to the
>> attached x86 machine running the Qualcomm PCAT tool(This comes as part Qualcomm
>> package manager kit).
>>
>> After that we will see a bunch of predefined registered region as binary blobs files
>> starts with md_* downloaded on the x86 machine on given location in PCAT tool from
>> the target device, more about this can be found in qualcomm minidump guide patch.
>>
> 
> I tried to apply this series on top of 535a265d7f0dd50 (as suggested by
> `b4 am -l -g`), but it conflicts on patch [04/17]. Please specify the
> exact base commit or another series for which this series is based on.

Apologies !
I just realized, it was 6.5-rc7, but let me rebase version of the series;

Sorry, for all the reviewed done so far, i will definitely take care of 
them or reply.

-Mukesh
> 
> Thanks.
> 

Re: [PATCH v5 09/17] pstore/ram: Use dynamic ramoops reserve resource

[Mukesh Ojha]

On 9/11/2023 11:03 AM, Pavan Kondeti wrote:
> On Sun, Sep 10, 2023 at 01:46:10AM +0530, Mukesh Ojha wrote:
>> As dynamic ramoops command line parsing is now added, so
>> lets add the support in ramoops driver to get the resource
>> structure and add it during platform device registration.
>>
>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>> ---
>>   fs/pstore/ram.c | 10 +++++++---
>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>
> 
> Documentation/admin-guide/ramoops.rst might need an update as well.

I have said in the cover-letter under changes in v5, it is open for
comment and not yet documented it yet.

> 
>> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
>> index 2f625e1fa8d8..e73fbbc1b5b5 100644
>> --- a/fs/pstore/ram.c
>> +++ b/fs/pstore/ram.c
>> @@ -913,13 +913,17 @@ static void __init ramoops_register_dummy(void)
>>   
>>   	/*
>>   	 * Prepare a dummy platform data structure to carry the module
>> -	 * parameters. If mem_size isn't set, then there are no module
>> -	 * parameters, and we can skip this.
>> +	 * parameters. If mem_size isn't set, check for dynamic ramoops
>> +	 * size and extract the information if it is set.
>>   	 */
>> -	if (!mem_size)
>> +	if (!mem_size && !dyn_ramoops_res.end)
>>   		return;
>>   
>>   	pr_info("using module parameters\n");
>> +	if (dyn_ramoops_res.end) {
>> +		mem_size = resource_size(&dyn_ramoops_res);
>> +		mem_address = dyn_ramoops_res.start;
>> +	}
>>   
>>   	memset(&pdata, 0, sizeof(pdata));
>>   	pdata.mem_size = mem_size;
> 
> You might want to add "arch_" prefix to dyn_ramoops resource so that it
> would be clear that it is coming from arch code. This code needs to
> guard against arch not supplying this.

Sure, thanks for pointing this.
Agree, if we finally decide to keep it in arch/arm64 .

-Mukesh
> 
> Thanks,
> Pavan

Re: [PATCH v5 06/17] soc: qcom: Add Qualcomm APSS minidump kernel driver

[Krzysztof Kozlowski]

On 09/09/2023 22:16, 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.

...

> +static int qcom_apss_md_table_init(struct minidump *md,
> +				   struct minidump_subsystem *mdss_toc)
> +{
> +	struct minidump_ss_data *mdss_data;
> +
> +	mdss_data = devm_kzalloc(md->dev, sizeof(*mdss_data), GFP_KERNEL);
> +	if (!mdss_data)
> +		return -ENOMEM;
> +
> +	mdss_data->md_ss_toc = mdss_toc;
> +	mdss_data->md_regions = devm_kcalloc(md->dev, MAX_NUM_ENTRIES,
> +					     sizeof(struct minidump_region),
> +					     GFP_KERNEL);
> +	if (!mdss_data->md_regions)
> +		return -ENOMEM;
> +
> +	mdss_toc = mdss_data->md_ss_toc;
> +	mdss_toc->regions_baseptr = cpu_to_le64(virt_to_phys(mdss_data->md_regions));
> +	mdss_toc->enabled = cpu_to_le32(MINIDUMP_SS_ENABLED);
> +	mdss_toc->status = cpu_to_le32(1);
> +	mdss_toc->region_count = cpu_to_le32(0);
> +
> +	/* Tell bootloader not to encrypt the regions of this subsystem */
> +	mdss_toc->encryption_status = cpu_to_le32(MINIDUMP_SS_ENCR_DONE);
> +	mdss_toc->encryption_required = cpu_to_le32(MINIDUMP_SS_ENCR_NOTREQ);
> +
> +	md->apss_data = mdss_data;
> +
> +	return 0;
> +}
> +
> +static int qcom_apss_minidump_probe(struct platform_device *pdev)
> +{
> +	struct minidump_global_toc *mdgtoc;
> +	struct minidump *md;
> +	size_t size;
> +	int ret;
> +
> +	md = devm_kzalloc(&pdev->dev, sizeof(struct minidump), GFP_KERNEL);

sizeof(*)

Didn't you get such comments already?


> +	if (!md)
> +		return -ENOMEM;
> +
> +	md->dev = &pdev->dev;
> +	mdgtoc = qcom_smem_get(QCOM_SMEM_HOST_ANY, SBL_MINIDUMP_SMEM_ID, &size);
> +	if (IS_ERR(mdgtoc)) {
> +		ret = PTR_ERR(mdgtoc);
> +		dev_err(md->dev, "Couldn't find minidump smem item: %d\n", ret);
> +		return ret;

The syntax is:
return dev_err_probe

> +	}
> +
> +	if (size < sizeof(*mdgtoc) || !mdgtoc->status) {
> +		dev_err(md->dev, "minidump table is not initialized: %d\n", ret);

ret is uninitialized here. Please use automated tools for checking your
code:
coccinelle, smatch and sparse

> +		return -EINVAL;
> +	}
> +
> +	mutex_init(&md->md_lock);
> +	ret = qcom_apss_md_table_init(md, &mdgtoc->subsystems[MINIDUMP_APSS_DESC]);
> +	if (ret) {
> +		dev_err(md->dev, "apss minidump initialization failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* First entry would be ELF header */
> +	ret = qcom_md_add_elfheader(md);
> +	if (ret) {
> +		dev_err(md->dev, "Failed to add elf header: %d\n", ret);
> +		memset(md->apss_data->md_ss_toc, 0, sizeof(struct minidump_subsystem));

Why do you need it?

> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, md);
> +
> +	return ret;
> +}
> +
> +static int qcom_apss_minidump_remove(struct platform_device *pdev)
> +{
> +	struct minidump *md = platform_get_drvdata(pdev);
> +	struct minidump_ss_data *mdss_data;
> +
> +	mdss_data = md->apss_data;
> +	memset(mdss_data->md_ss_toc, cpu_to_le32(0), sizeof(struct minidump_subsystem));

Why do you need it?

> +	md = NULL;

That's useless assignment.

> +
> +	return 0;
> +}
> +
> +static struct platform_driver qcom_minidump_driver = {
> +	.probe = qcom_apss_minidump_probe,
> +	.remove = qcom_apss_minidump_remove,
> +	.driver  = {
> +		.name = "qcom-minidump-smem",
> +	},
> +};
> +
> +module_platform_driver(qcom_minidump_driver);
> +
> +MODULE_DESCRIPTION("Qualcomm APSS minidump driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:qcom-minidump-smem");

Add a proper ID table instead of re-inventing it with module aliases.

Best regards,
Krzysztof

Re: [PATCH v5 06/17] soc: qcom: Add Qualcomm APSS minidump kernel driver

[Krzysztof Kozlowski]

On 09/09/2023 22:16, Mukesh Ojha wrote:
> +/**
> + * qcom_minidump_region_register() - Register region in APSS Minidump table.
> + * @region: minidump region.
> + *
> + * Return: On success, it returns 0 and negative error value on failure.
> + */
> +int qcom_minidump_region_register(const struct qcom_minidump_region *region)
> +{
> +	struct minidump *md;
> +	int ret;
> +
> +	md = qcom_smem_minidump_ready();
> +	if (!md)
> +		return -EPROBE_DEFER;
> +
> +	if (!qcom_minidump_valid_region(region))
> +		return -EINVAL;
> +
> +	mutex_lock(&md->md_lock);
> +	ret = qcom_md_region_register(md, region);
> +	if (ret)
> +		goto unlock;
> +
> +	qcom_md_update_elfheader(md, region);
> +unlock:
> +	mutex_unlock(&md->md_lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(qcom_minidump_region_register);

NAK, there is no user for this.

Drop all exports from minidump drivers. Your upstream driver *must not*
expose stuff to your vendor drivers.

Best regards,
Krzysztof

Re: [PATCH v5 07/17] soc: qcom: minidump: Add pending region registration

[Krzysztof Kozlowski]

On 09/09/2023 22:16, Mukesh Ojha wrote:
>  static int qcom_apss_minidump_probe(struct platform_device *pdev)
>  {
>  	struct minidump_global_toc *mdgtoc;
> @@ -571,7 +688,10 @@ static int qcom_apss_minidump_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	mutex_lock(&md_plist.plock);
>  	platform_set_drvdata(pdev, md);

Why this is locked?

> +	qcom_apss_register_pending_regions(md);

Why this one is locked? It seems ordering of your operations is not
correct if you need to lock the providers probe().

> +	mutex_unlock(&md_plist.plock);


Best regards,
Krzysztof

Re: [PATCH v5 11/17] qcom_minidump: Register ramoops region with minidump

[Krzysztof Kozlowski]

On 09/09/2023 22:16, Mukesh Ojha wrote:
> Register all the pstore frontend with minidump, so that they can
> be dumped as default Linux minidump region to be collected on
> SoC where minidump is enabled.
> 

...

> +
> +	record.type = type;
> +	record.id = 0;
> +	max_dump_cnt = 0;
> +	name = pstore_type_to_name(record.type);
> +	do {
> +		ret = pstore_region_defined(&record, &virt, &phys, &size, &max_dump_cnt);
> +		if (ret < 0)
> +			break;
> +
> +		mdr_list = devm_kzalloc(dev, sizeof(struct md_region_list), GFP_KERNEL);

sizeof(*)

Please fix it everywhere in your code.

> +		if (!mdr_list)
> +			return -ENOMEM;
> +
> +		md_region = &mdr_list->md_region;
> +		scnprintf(md_region->name, sizeof(md_region->name) - 1, "K%s%llu", name, record.id);
> +		md_region->virt_addr = virt;
> +		md_region->phys_addr = phys;
> +		md_region->size = size;
> +		ret = qcom_minidump_region_register(md_region);
> +		if (ret) {
> +			pr_err("failed to register minidump region\n");
> +			break;
> +		}
> +
> +		list_add(&mdr_list->list, &ramoops_region_list);
> +	} while (record.id < max_dump_cnt && ++record.id);
> +
> +	return ret;
> +}


Best regards,
Krzysztof

Re: [PATCH v5 00/17] Add Qualcomm Minidump kernel driver related support

[Bagas Sanjaya]

On 11/09/2023 17:39, Mukesh Ojha wrote:
> 
> 
> On 9/11/2023 2:22 PM, Bagas Sanjaya wrote:
>> On Sun, Sep 10, 2023 at 01:46:01AM +0530, Mukesh Ojha wrote:
>>> Hi All,
>>>
>>> This is to continuation from the conversation happened at v4
>>>
>>> https://lore.kernel.org/lkml/632c5b97-4a91-c3e8-1e6c-33d6c4f6454f@quicinc.com/
>>>
>>> https://lore.kernel.org/lkml/695133e6-105f-de2a-5559-555cea0a0462@quicinc.com/
>>>
>>> We have put abstract on LPC on this topic as well as initiated a mail thread
>>> with other SoC vendors but did not get much traction on it.
>>>
>>> https://lore.kernel.org/lkml/0199db00-1b1d-0c63-58ff-03efae02cb21@quicinc.com/
>>>
>>> We explored most of possiblity present in kernel to address this issue[1] but
>>> solution like kdump/fadump does not seems safe/secure/performant from our
>>> perspective.
>>>
>>> Hence, with this series we tried to make the minidump kernel driver, simple
>>> and tied with pstore frontends, so that it collects the present available
>>> frontends data like dmesg, ftrace, pmsg, ftrace., Also, we will be working
>>> towards enhancing generic pstore to capture more debug data which will be
>>> helpful for first hand of debugging that can benefit both other pstore users
>>> as well as us as minidump users.
>>>
>>> One of the proposal made here,
>>> https://lore.kernel.org/lkml/1683561060-2197-1-git-send-email-quic_mojha@quicinc.com/
>>>
>>> Looking forward for your comments.
>>>
>>> Thanks,
>>> Mukesh
>>>
>>> [1]
>>> 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 SMEM based 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 SMEM
>>> based 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.
>>>
>>> Changes in v5:
>>>   - On suggestion from Pavan.k, to have single function call for minidump collection
>>>     from remoteproc driver, separated the logic to have separate minidump file called
>>>     qcom_rproc_minidump.c and also renamed the function from qcom_minidump() to
>>>     qcom_rproc_minidump(); however, dropped his suggestion about rework on lazy deletion
>>>     during region unregister in this series, will pursue it in next series.
>>>
>>>   - To simplify the minidump driver, removed the complication for frontend and different
>>>     backend from Greg suggestion, will pursue this once main driver gets mainlined.
>>>
>>>   - Move the dynamic ramoops region allocation from Device tree approach to command line
>>>     approch with the introduction command line parsing and memblock reservation during
>>>     early boot up; Not added documentation about it yet, will add if it gets positive
>>>     response.
>>>
>>>   - Exporting linux banner from kernel to make minidump build also as module, however,
>>>     minidump is a debug module and should be kernel built to get most debug information
>>>     from kernel.
>>>
>>>   - Tried to address comments given on dload patch series.
>>>
>>> Changes in v4: https://lore.kernel.org/lkml/1687955688-20809-1-git-send-email-quic_mojha@quicinc.com/
>>>   - Redesigned the driver and divided the driver into front end and backend (smem) so
>>>     that any new backend can be attached easily to avoid code duplication.
>>>   - Patch reordering as per the driver and subsystem to easier review of the code.
>>>   - Removed minidump specific code from remoteproc to minidump smem based driver.
>>>   - Enabled the all the driver as modules.
>>>   - Address comments made on documentation and yaml and Device tree file [Krzysztof/Konrad]
>>>   - Address comments made qcom_pstore_minidump driver and given its Device tree
>>>     same set of properties as ramoops. [Luca/Kees]
>>>   - Added patch for MAINTAINER file.
>>>   - Include defconfig change as one patch as per [Krzysztof] suggestion.
>>>   - Tried to remove the redundant file scope variables from the module as per [Krzysztof] suggestion.
>>>   - Addressed comments made on dload mode patch v6 version
>>>     https://lore.kernel.org/lkml/1680076012-10785-1-git-send-email-quic_mojha@quicinc.com/
>>>
>>> Changes in v3: https://lore.kernel.org/lkml/1683133352-10046-1-git-send-email-quic_mojha@quicinc.com/
>>>   - Addressed most of the comments by Srini on v2 and refactored the minidump driver.
>>>      - Added platform device support
>>>      - Unregister region support.
>>>   - Added update region for clients.
>>>   - Added pending region support.
>>>   - Modified the documentation guide accordingly.
>>>   - Added qcom_pstore_ramdump client driver which happen to add ramoops platform
>>>     device and also registers ramoops region with minidump.
>>>   - Added download mode patch series with this minidump series.
>>>      https://lore.kernel.org/lkml/1680076012-10785-1-git-send-email-quic_mojha@quicinc.com/
>>>
>>> Changes in v2: https://lore.kernel.org/lkml/1679491817-2498-1-git-send-email-quic_mojha@quicinc.com/
>>>   - 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/
>>>
>>> Testing of the patches has been done on sm8450 target after enabling config like
>>> CONFIG_PSTORE_RAM and CONFIG_PSTORE_CONSOLE and once the device boots up.
>>>
>>>   echo mini > /sys/module/qcom_scm/parameters/download_mode
>>>
>>> Try crashing it via devmem2 0xf11c000(this is known to create xpu violation and
>>> and put the device in download mode) on command prompt.
>>>
>>> Default storage type is set to via USB, so minidump would be downloaded with the
>>> help of x86_64 machine (running PCAT tool) attached to Qualcomm device which has
>>> backed minidump boot firmware support.
>>>
>>> This will make the device go to download mode and collect the minidump on to the
>>> attached x86 machine running the Qualcomm PCAT tool(This comes as part Qualcomm
>>> package manager kit).
>>>
>>> After that we will see a bunch of predefined registered region as binary blobs files
>>> starts with md_* downloaded on the x86 machine on given location in PCAT tool from
>>> the target device, more about this can be found in qualcomm minidump guide patch.
>>>
>>
>> I tried to apply this series on top of 535a265d7f0dd50 (as suggested by
>> `b4 am -l -g`), but it conflicts on patch [04/17]. Please specify the
>> exact base commit or another series for which this series is based on.
> 
> Apologies !
> I just realized, it was 6.5-rc7, but let me rebase version of the series;
> 
> Sorry, for all the reviewed done so far, i will definitely take care of them or reply.
> 

OK, see you in v6!

-- 
An old man doll... just what I always wanted! - Clara

Re: [PATCH v5 07/17] soc: qcom: minidump: Add pending region registration

[Jeff Johnson]

On 9/9/2023 1:16 PM, Mukesh Ojha wrote:
> +/**
> + * struct minidump_pregion - Minidump pending region
> + * @list       : Pending region list pointer
> + * @region     : APSS minidump client region

does kernel-doc parse this correctly? should not be whitespace between 
@ID and ":"

refer to 
<https://static.lwn.net/kerneldoc/doc-guide/kernel-doc.html#structure-union-and-enumeration-documentation>

Re: [PATCH v5 06/17] soc: qcom: Add Qualcomm APSS minidump kernel driver

[Jeff Johnson]

On 9/11/2023 4:07 AM, Krzysztof Kozlowski wrote:
> On 09/09/2023 22:16, Mukesh Ojha wrote:
>> +/**
>> + * qcom_minidump_region_register() - Register region in APSS Minidump table.
>> + * @region: minidump region.
>> + *
>> + * Return: On success, it returns 0 and negative error value on failure.
>> + */
>> +int qcom_minidump_region_register(const struct qcom_minidump_region *region)
>> +{
>> +	struct minidump *md;
>> +	int ret;
>> +
>> +	md = qcom_smem_minidump_ready();
>> +	if (!md)
>> +		return -EPROBE_DEFER;
>> +
>> +	if (!qcom_minidump_valid_region(region))
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&md->md_lock);
>> +	ret = qcom_md_region_register(md, region);
>> +	if (ret)
>> +		goto unlock;
>> +
>> +	qcom_md_update_elfheader(md, region);
>> +unlock:
>> +	mutex_unlock(&md->md_lock);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(qcom_minidump_region_register);
> 
> NAK, there is no user for this.
> 
> Drop all exports from minidump drivers. Your upstream driver *must not*
> expose stuff to your vendor drivers.

Do we not expect that upstream drivers would want to register?
Mind you, in the downstream code the following was a bad limitation:
#define MAX_NUM_OF_SS           10

Re: [PATCH v5 06/17] soc: qcom: Add Qualcomm APSS minidump kernel driver

[Konrad Dybcio]

On 11.09.2023 21:09, Jeff Johnson wrote:
> On 9/11/2023 4:07 AM, Krzysztof Kozlowski wrote:
>> On 09/09/2023 22:16, Mukesh Ojha wrote:
>>> +/**
>>> + * qcom_minidump_region_register() - Register region in APSS Minidump table.
>>> + * @region: minidump region.
>>> + *
>>> + * Return: On success, it returns 0 and negative error value on failure.
>>> + */
>>> +int qcom_minidump_region_register(const struct qcom_minidump_region *region)
>>> +{
>>> +    struct minidump *md;
>>> +    int ret;
>>> +
>>> +    md = qcom_smem_minidump_ready();
>>> +    if (!md)
>>> +        return -EPROBE_DEFER;
>>> +
>>> +    if (!qcom_minidump_valid_region(region))
>>> +        return -EINVAL;
>>> +
>>> +    mutex_lock(&md->md_lock);
>>> +    ret = qcom_md_region_register(md, region);
>>> +    if (ret)
>>> +        goto unlock;
>>> +
>>> +    qcom_md_update_elfheader(md, region);
>>> +unlock:
>>> +    mutex_unlock(&md->md_lock);
>>> +    return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(qcom_minidump_region_register);
>>
>> NAK, there is no user for this.
>>
>> Drop all exports from minidump drivers. Your upstream driver *must not*
>> expose stuff to your vendor drivers.
> 
> Do we not expect that upstream drivers would want to register?
> Mind you, in the downstream code the following was a bad limitation:
> #define MAX_NUM_OF_SS           10
No, Krzysztof meant that you are not allowed to export symbols
without immediately providing a user for them - meaning if the
functions are not going to be used upstream, this change will
not be accepted.

Konrad

Re: [PATCH v5 09/17] pstore/ram: Use dynamic ramoops reserve resource

[Pavan Kondeti]

On Mon, Sep 11, 2023 at 04:21:44PM +0530, Mukesh Ojha wrote:
> 
> 
> On 9/11/2023 11:03 AM, Pavan Kondeti wrote:
> > On Sun, Sep 10, 2023 at 01:46:10AM +0530, Mukesh Ojha wrote:
> > > As dynamic ramoops command line parsing is now added, so
> > > lets add the support in ramoops driver to get the resource
> > > structure and add it during platform device registration.
> > > 
> > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> > > ---
> > >   fs/pstore/ram.c | 10 +++++++---
> > >   1 file changed, 7 insertions(+), 3 deletions(-)
> > > 
> > 
> > Documentation/admin-guide/ramoops.rst might need an update as well.
> 
> I have said in the cover-letter under changes in v5, it is open for
> comment and not yet documented it yet.
> 
Sure.

To easy on the reviewers, the under cut portion of a specific patch could be
used to add footer notes like TODO/Testing etc. In this case, I was lazy to 
read the loong cover letter posted in this series ;-)

Thanks,
Pavan

Re: [PATCH v5 06/17] soc: qcom: Add Qualcomm APSS minidump kernel driver

[Mukesh Ojha]

Thanks for your time in reviewing this.

On 9/11/2023 4:31 PM, Krzysztof Kozlowski wrote:
> On 09/09/2023 22:16, 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.
> 
> ...
> 
>> +static int qcom_apss_md_table_init(struct minidump *md,
>> +				   struct minidump_subsystem *mdss_toc)
>> +{
>> +	struct minidump_ss_data *mdss_data;
>> +
>> +	mdss_data = devm_kzalloc(md->dev, sizeof(*mdss_data), GFP_KERNEL);
>> +	if (!mdss_data)
>> +		return -ENOMEM;
>> +
>> +	mdss_data->md_ss_toc = mdss_toc;
>> +	mdss_data->md_regions = devm_kcalloc(md->dev, MAX_NUM_ENTRIES,
>> +					     sizeof(struct minidump_region),
>> +					     GFP_KERNEL);
>> +	if (!mdss_data->md_regions)
>> +		return -ENOMEM;
>> +
>> +	mdss_toc = mdss_data->md_ss_toc;
>> +	mdss_toc->regions_baseptr = cpu_to_le64(virt_to_phys(mdss_data->md_regions));
>> +	mdss_toc->enabled = cpu_to_le32(MINIDUMP_SS_ENABLED);
>> +	mdss_toc->status = cpu_to_le32(1);
>> +	mdss_toc->region_count = cpu_to_le32(0);
>> +
>> +	/* Tell bootloader not to encrypt the regions of this subsystem */
>> +	mdss_toc->encryption_status = cpu_to_le32(MINIDUMP_SS_ENCR_DONE);
>> +	mdss_toc->encryption_required = cpu_to_le32(MINIDUMP_SS_ENCR_NOTREQ);
>> +
>> +	md->apss_data = mdss_data;
>> +
>> +	return 0;
>> +}
>> +
>> +static int qcom_apss_minidump_probe(struct platform_device *pdev)
>> +{
>> +	struct minidump_global_toc *mdgtoc;
>> +	struct minidump *md;
>> +	size_t size;
>> +	int ret;
>> +
>> +	md = devm_kzalloc(&pdev->dev, sizeof(struct minidump), GFP_KERNEL);
> 
> sizeof(*)
> 
> Didn't you get such comments already?

Ok, will fix this, no i have not got such comments as of yet.
Any reason of using this way?

> 
> 
>> +	if (!md)
>> +		return -ENOMEM;
>> +
>> +	md->dev = &pdev->dev;
>> +	mdgtoc = qcom_smem_get(QCOM_SMEM_HOST_ANY, SBL_MINIDUMP_SMEM_ID, &size);
>> +	if (IS_ERR(mdgtoc)) {
>> +		ret = PTR_ERR(mdgtoc);
>> +		dev_err(md->dev, "Couldn't find minidump smem item: %d\n", ret);
>> +		return ret;
> 
> The syntax is:
> return dev_err_probe

ACK.

> 
>> +	}
>> +
>> +	if (size < sizeof(*mdgtoc) || !mdgtoc->status) {
>> +		dev_err(md->dev, "minidump table is not initialized: %d\n", ret);
> 
> ret is uninitialized here. Please use automated tools for checking your
> code:
> coccinelle, smatch and sparse

Thanks.

> 
>> +		return -EINVAL;
>> +	}
>> +
>> +	mutex_init(&md->md_lock);
>> +	ret = qcom_apss_md_table_init(md, &mdgtoc->subsystems[MINIDUMP_APSS_DESC]);
>> +	if (ret) {
>> +		dev_err(md->dev, "apss minidump initialization failed: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	/* First entry would be ELF header */
>> +	ret = qcom_md_add_elfheader(md);
>> +	if (ret) {
>> +		dev_err(md->dev, "Failed to add elf header: %d\n", ret);
>> +		memset(md->apss_data->md_ss_toc, 0, sizeof(struct minidump_subsystem));
> 
> Why do you need it?

Earlier, i got comment about clearing the SS TOC(subsystem table of 
content) which is shared with other SS and it will have stale values.

> 
>> +		return ret;
>> +	}
>> +
>> +	platform_set_drvdata(pdev, md);
>> +
>> +	return ret;
>> +}
>> +
>> +static int qcom_apss_minidump_remove(struct platform_device *pdev)
>> +{
>> +	struct minidump *md = platform_get_drvdata(pdev);
>> +	struct minidump_ss_data *mdss_data;
>> +
>> +	mdss_data = md->apss_data;
>> +	memset(mdss_data->md_ss_toc, cpu_to_le32(0), sizeof(struct minidump_subsystem));
> 
> Why do you need it?

Same as above.

> 
>> +	md = NULL;
> 
> That's useless assignment.

Ok.

> 
>> +
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver qcom_minidump_driver = {
>> +	.probe = qcom_apss_minidump_probe,
>> +	.remove = qcom_apss_minidump_remove,
>> +	.driver  = {
>> +		.name = "qcom-minidump-smem",
>> +	},
>> +};
>> +
>> +module_platform_driver(qcom_minidump_driver);
>> +
>> +MODULE_DESCRIPTION("Qualcomm APSS minidump driver");
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS("platform:qcom-minidump-smem");
> 
> Add a proper ID table instead of re-inventing it with module aliases.

Ok.

-Mukesh

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v5 09/17] pstore/ram: Use dynamic ramoops reserve resource

[Mukesh Ojha]

On 9/12/2023 6:09 AM, Pavan Kondeti wrote:
> On Mon, Sep 11, 2023 at 04:21:44PM +0530, Mukesh Ojha wrote:
>>
>>
>> On 9/11/2023 11:03 AM, Pavan Kondeti wrote:
>>> On Sun, Sep 10, 2023 at 01:46:10AM +0530, Mukesh Ojha wrote:
>>>> As dynamic ramoops command line parsing is now added, so
>>>> lets add the support in ramoops driver to get the resource
>>>> structure and add it during platform device registration.
>>>>
>>>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>>>> ---
>>>>    fs/pstore/ram.c | 10 +++++++---
>>>>    1 file changed, 7 insertions(+), 3 deletions(-)
>>>>
>>>
>>> Documentation/admin-guide/ramoops.rst might need an update as well.
>>
>> I have said in the cover-letter under changes in v5, it is open for
>> comment and not yet documented it yet.
>>
> Sure.
> 
> To easy on the reviewers, the under cut portion of a specific patch could be
> used to add footer notes like TODO/Testing etc. In this case, I was lazy to
> read the loong cover letter posted in this series ;-)

I have seen it, will comment related to particular patch under --- .
Thanks for suggestion.

-Mukesh

> 
> Thanks,
> Pavan

Re: [PATCH v5 06/17] soc: qcom: Add Qualcomm APSS minidump kernel driver

[Krzysztof Kozlowski]

On 12/09/2023 11:26, Mukesh Ojha wrote:
>>
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	mutex_init(&md->md_lock);
>>> +	ret = qcom_apss_md_table_init(md, &mdgtoc->subsystems[MINIDUMP_APSS_DESC]);
>>> +	if (ret) {
>>> +		dev_err(md->dev, "apss minidump initialization failed: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	/* First entry would be ELF header */
>>> +	ret = qcom_md_add_elfheader(md);
>>> +	if (ret) {
>>> +		dev_err(md->dev, "Failed to add elf header: %d\n", ret);
>>> +		memset(md->apss_data->md_ss_toc, 0, sizeof(struct minidump_subsystem));
>>
>> Why do you need it?
> 
> Earlier, i got comment about clearing the SS TOC(subsystem table of 
> content) which is shared with other SS and it will have stale values.

OK, but then the entire code is poorly readable. First, any cleanup of
qcom_apss_md_table_init() should be named similarly, e.g.
qcom_apss_md_table_clean() or qcom_apss_md_table_exit() or whatever
seems feasible.

Second, shouldn't writing to shared memory be the last step? Step which
cannot fail and there is no cleanup afterwards (like
platform_set_drvdata)? I don't enjoy looking at this interface...



Best regards,
Krzysztof

Re: [PATCH v5 06/17] soc: qcom: Add Qualcomm APSS minidump kernel driver

[Mukesh Ojha]

On 9/12/2023 2:56 PM, Mukesh Ojha wrote:
> Thanks for your time in reviewing this.
> 
> On 9/11/2023 4:31 PM, Krzysztof Kozlowski wrote:
>> On 09/09/2023 22:16, 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.
>>
>> ...
>>
>>> +static int qcom_apss_md_table_init(struct minidump *md,
>>> +                   struct minidump_subsystem *mdss_toc)
>>> +{
>>> +    struct minidump_ss_data *mdss_data;
>>> +
>>> +    mdss_data = devm_kzalloc(md->dev, sizeof(*mdss_data), GFP_KERNEL);
>>> +    if (!mdss_data)
>>> +        return -ENOMEM;
>>> +
>>> +    mdss_data->md_ss_toc = mdss_toc;
>>> +    mdss_data->md_regions = devm_kcalloc(md->dev, MAX_NUM_ENTRIES,
>>> +                         sizeof(struct minidump_region),
>>> +                         GFP_KERNEL);
>>> +    if (!mdss_data->md_regions)
>>> +        return -ENOMEM;
>>> +
>>> +    mdss_toc = mdss_data->md_ss_toc;
>>> +    mdss_toc->regions_baseptr = 
>>> cpu_to_le64(virt_to_phys(mdss_data->md_regions));
>>> +    mdss_toc->enabled = cpu_to_le32(MINIDUMP_SS_ENABLED);
>>> +    mdss_toc->status = cpu_to_le32(1);
>>> +    mdss_toc->region_count = cpu_to_le32(0);
>>> +
>>> +    /* Tell bootloader not to encrypt the regions of this subsystem */
>>> +    mdss_toc->encryption_status = cpu_to_le32(MINIDUMP_SS_ENCR_DONE);
>>> +    mdss_toc->encryption_required = 
>>> cpu_to_le32(MINIDUMP_SS_ENCR_NOTREQ);
>>> +
>>> +    md->apss_data = mdss_data;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int qcom_apss_minidump_probe(struct platform_device *pdev)
>>> +{
>>> +    struct minidump_global_toc *mdgtoc;
>>> +    struct minidump *md;
>>> +    size_t size;
>>> +    int ret;
>>> +
>>> +    md = devm_kzalloc(&pdev->dev, sizeof(struct minidump), GFP_KERNEL);
>>
>> sizeof(*)
>>
>> Didn't you get such comments already?
> 
> Ok, will fix this, no i have not got such comments as of yet.
> Any reason of using this way?

Got the reason, thanks.

-Mukesh

> 
>>
>>
>>> +    if (!md)
>>> +        return -ENOMEM;
>>> +
>>> +    md->dev = &pdev->dev;
>>> +    mdgtoc = qcom_smem_get(QCOM_SMEM_HOST_ANY, SBL_MINIDUMP_SMEM_ID, 
>>> &size);
>>> +    if (IS_ERR(mdgtoc)) {
>>> +        ret = PTR_ERR(mdgtoc);
>>> +        dev_err(md->dev, "Couldn't find minidump smem item: %d\n", 
>>> ret);
>>> +        return ret;
>>
>> The syntax is:
>> return dev_err_probe
> 
> ACK.
> 
>>
>>> +    }
>>> +
>>> +    if (size < sizeof(*mdgtoc) || !mdgtoc->status) {
>>> +        dev_err(md->dev, "minidump table is not initialized: %d\n", 
>>> ret);
>>
>> ret is uninitialized here. Please use automated tools for checking your
>> code:
>> coccinelle, smatch and sparse
> 
> Thanks.
> 
>>
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    mutex_init(&md->md_lock);
>>> +    ret = qcom_apss_md_table_init(md, 
>>> &mdgtoc->subsystems[MINIDUMP_APSS_DESC]);
>>> +    if (ret) {
>>> +        dev_err(md->dev, "apss minidump initialization failed: 
>>> %d\n", ret);
>>> +        return ret;
>>> +    }
>>> +
>>> +    /* First entry would be ELF header */
>>> +    ret = qcom_md_add_elfheader(md);
>>> +    if (ret) {
>>> +        dev_err(md->dev, "Failed to add elf header: %d\n", ret);
>>> +        memset(md->apss_data->md_ss_toc, 0, sizeof(struct 
>>> minidump_subsystem));
>>
>> Why do you need it?
> 
> Earlier, i got comment about clearing the SS TOC(subsystem table of 
> content) which is shared with other SS and it will have stale values.
> 
>>
>>> +        return ret;
>>> +    }
>>> +
>>> +    platform_set_drvdata(pdev, md);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static int qcom_apss_minidump_remove(struct platform_device *pdev)
>>> +{
>>> +    struct minidump *md = platform_get_drvdata(pdev);
>>> +    struct minidump_ss_data *mdss_data;
>>> +
>>> +    mdss_data = md->apss_data;
>>> +    memset(mdss_data->md_ss_toc, cpu_to_le32(0), sizeof(struct 
>>> minidump_subsystem));
>>
>> Why do you need it?
> 
> Same as above.
> 
>>
>>> +    md = NULL;
>>
>> That's useless assignment.
> 
> Ok.
> 
>>
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static struct platform_driver qcom_minidump_driver = {
>>> +    .probe = qcom_apss_minidump_probe,
>>> +    .remove = qcom_apss_minidump_remove,
>>> +    .driver  = {
>>> +        .name = "qcom-minidump-smem",
>>> +    },
>>> +};
>>> +
>>> +module_platform_driver(qcom_minidump_driver);
>>> +
>>> +MODULE_DESCRIPTION("Qualcomm APSS minidump driver");
>>> +MODULE_LICENSE("GPL");
>>> +MODULE_ALIAS("platform:qcom-minidump-smem");
>>
>> Add a proper ID table instead of re-inventing it with module aliases.
> 
> Ok.
> 
> -Mukesh
> 
>>
>> Best regards,
>> Krzysztof
>>

Re: [PATCH v5 06/17] soc: qcom: Add Qualcomm APSS minidump kernel driver

[Mukesh Ojha]

On 9/12/2023 3:24 PM, Krzysztof Kozlowski wrote:
> On 12/09/2023 11:26, Mukesh Ojha wrote:
>>>
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	mutex_init(&md->md_lock);
>>>> +	ret = qcom_apss_md_table_init(md, &mdgtoc->subsystems[MINIDUMP_APSS_DESC]);
>>>> +	if (ret) {
>>>> +		dev_err(md->dev, "apss minidump initialization failed: %d\n", ret);
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	/* First entry would be ELF header */
>>>> +	ret = qcom_md_add_elfheader(md);
>>>> +	if (ret) {
>>>> +		dev_err(md->dev, "Failed to add elf header: %d\n", ret);
>>>> +		memset(md->apss_data->md_ss_toc, 0, sizeof(struct minidump_subsystem));
>>>
>>> Why do you need it?
>>
>> Earlier, i got comment about clearing the SS TOC(subsystem table of
>> content) which is shared with other SS and it will have stale values.
> 
> OK, but then the entire code is poorly readable. First, any cleanup of
> qcom_apss_md_table_init() should be named similarly, e.g.
> qcom_apss_md_table_clean() or qcom_apss_md_table_exit() or whatever
> seems feasible.

ACK on this.

> 
> Second, shouldn't writing to shared memory be the last step? Step which
> cannot fail and there is no cleanup afterwards (like
> platform_set_drvdata)? I don't enjoy looking at this interface...

It can be done, if i shift adding elf header as first thing to first
caller of qcom_minidump_region_register() but then i would have to 
remove qcom_ramoops_minidump() from this probe in 11/17 patch.

-Mukesh
> 
> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v5 06/17] soc: qcom: Add Qualcomm APSS minidump kernel driver

[Mukesh Ojha]

On 9/12/2023 12:39 AM, Jeff Johnson wrote:
> On 9/11/2023 4:07 AM, Krzysztof Kozlowski wrote:
>> On 09/09/2023 22:16, Mukesh Ojha wrote:
>>> +/**
>>> + * qcom_minidump_region_register() - Register region in APSS 
>>> Minidump table.
>>> + * @region: minidump region.
>>> + *
>>> + * Return: On success, it returns 0 and negative error value on 
>>> failure.
>>> + */
>>> +int qcom_minidump_region_register(const struct qcom_minidump_region 
>>> *region)
>>> +{
>>> +    struct minidump *md;
>>> +    int ret;
>>> +
>>> +    md = qcom_smem_minidump_ready();
>>> +    if (!md)
>>> +        return -EPROBE_DEFER;
>>> +
>>> +    if (!qcom_minidump_valid_region(region))
>>> +        return -EINVAL;
>>> +
>>> +    mutex_lock(&md->md_lock);
>>> +    ret = qcom_md_region_register(md, region);
>>> +    if (ret)
>>> +        goto unlock;
>>> +
>>> +    qcom_md_update_elfheader(md, region);
>>> +unlock:
>>> +    mutex_unlock(&md->md_lock);
>>> +    return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(qcom_minidump_region_register);
>>
>> NAK, there is no user for this.
>>
>> Drop all exports from minidump drivers. Your upstream driver *must not*
>> expose stuff to your vendor drivers.
> 
> Do we not expect that upstream drivers would want to register?

As of current version of patch, there is no user. Let's avoid till
any upstream QCOM driver uses it .


> Mind you, in the downstream code the following was a bad limitation:
> #define MAX_NUM_OF_SS           10

I don't think, there is any problem with above macro, instead there is
restriction on total number of APSS region can be registered.

#define MAX_NUM_ENTRIES	  201

-Mukesh
> 
>