Re: [PATCH v9 1/3] soc: qcom: Add qcom_rproc_minidump module
On 4/30/2024 7:08 PM, Bjorn Andersson wrote: On Tue, Mar 26, 2024 at 07:43:12PM +0530, Mukesh Ojha wrote: 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(). I'd prefer to see this enter the git history as one patch, extracting this logic from the remoteproc into its own driver - rather than as presented here give a sense that it's a new thing added. (I'll take care of the maintainer sync...) I also would prefer for this to include a problem description, documenting why this is done. I've not compared patch 1 and 3, but I'd also like a statement in the commit message telling if there are any changes, or if the functions are cleanly moved from one place to another. Thanks for the review, addressed the comments here in v10. https://lore.kernel.org/lkml/1714724287-12518-1-git-send-email-quic_mo...@quicinc.com/ -Mukesh
Re: [PATCH v9 1/3] soc: qcom: Add qcom_rproc_minidump module
On Tue, Mar 26, 2024 at 07:43:12PM +0530, Mukesh Ojha wrote:
> 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().
>
I'd prefer to see this enter the git history as one patch, extracting
this logic from the remoteproc into its own driver - rather than as
presented here give a sense that it's a new thing added. (I'll take care
of the maintainer sync...)
I also would prefer for this to include a problem description,
documenting why this is done.
I've not compared patch 1 and 3, but I'd also like a statement in the
commit message telling if there are any changes, or if the functions are
cleanly moved from one place to another.
Regards,
Bjorn
> Signed-off-by: Mukesh Ojha
> ---
> Changes in v9:
> - Added source file driver/remoteproc/qcom_common.c copyright
>to qcom_rproc_minidump.c
> - Dissociated it from minidump series as this can go separately
>and minidump can put it dependency for the data structure files.
>
> Nothing much changed in these three patches from previous version,
> However, giving the link of their older versions.
>
> v8:
> https://lore.kernel.org/lkml/20240131105734.13090-1-quic_mo...@quicinc.com/
> v7:
> https://lore.kernel.org/lkml/20240109153200.12848-1-quic_mo...@quicinc.com/
> v6:
> https://lore.kernel.org/lkml/1700864395-1479-1-git-send-email-quic_mo...@quicinc.com/
> v5:
> https://lore.kernel.org/lkml/1694429639-21484-1-git-send-email-quic_mo...@quicinc.com/
> v4:
> https://lore.kernel.org/lkml/1687955688-20809-1-git-send-email-quic_mo...@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| 115
> ++
> include/soc/qcom/qcom_minidump.h | 23 ++
> 5 files changed, 213 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 5af33b0e3470..ed23e0275c22 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -277,4 +277,14 @@ config QCOM_PBS
> This module provides the APIs to the client drivers that wants to
> send the
> PBS trigger event to the PBS RAM.
>
> +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 ca0bece0dfff..44664589263d 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_PBS) +=qcom-pbs.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 ..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
> +
Re: [PATCH v9 1/3] soc: qcom: Add qcom_rproc_minidump module
Gentle ping..
-Mukesh
On 3/26/2024 7:43 PM, Mukesh Ojha wrote:
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
---
Changes in v9:
- Added source file driver/remoteproc/qcom_common.c copyright
to qcom_rproc_minidump.c
- Dissociated it from minidump series as this can go separately
and minidump can put it dependency for the data structure files.
Nothing much changed in these three patches from previous version,
However, giving the link of their older versions.
v8: https://lore.kernel.org/lkml/20240131105734.13090-1-quic_mo...@quicinc.com/
v7: https://lore.kernel.org/lkml/20240109153200.12848-1-quic_mo...@quicinc.com/
v6:
https://lore.kernel.org/lkml/1700864395-1479-1-git-send-email-quic_mo...@quicinc.com/
v5:
https://lore.kernel.org/lkml/1694429639-21484-1-git-send-email-quic_mo...@quicinc.com/
v4:
https://lore.kernel.org/lkml/1687955688-20809-1-git-send-email-quic_mo...@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| 115 ++
include/soc/qcom/qcom_minidump.h | 23 ++
5 files changed, 213 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 5af33b0e3470..ed23e0275c22 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -277,4 +277,14 @@ config QCOM_PBS
This module provides the APIs to the client drivers that wants to
send the
PBS trigger event to the PBS RAM.
+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 ca0bece0dfff..44664589263d 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_PBS) += qcom-pbs.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 ..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 {
+ charname[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;
+
[PATCH v9 1/3] soc: qcom: Add qcom_rproc_minidump module
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
---
Changes in v9:
- Added source file driver/remoteproc/qcom_common.c copyright
to qcom_rproc_minidump.c
- Dissociated it from minidump series as this can go separately
and minidump can put it dependency for the data structure files.
Nothing much changed in these three patches from previous version,
However, giving the link of their older versions.
v8: https://lore.kernel.org/lkml/20240131105734.13090-1-quic_mo...@quicinc.com/
v7: https://lore.kernel.org/lkml/20240109153200.12848-1-quic_mo...@quicinc.com/
v6:
https://lore.kernel.org/lkml/1700864395-1479-1-git-send-email-quic_mo...@quicinc.com/
v5:
https://lore.kernel.org/lkml/1694429639-21484-1-git-send-email-quic_mo...@quicinc.com/
v4:
https://lore.kernel.org/lkml/1687955688-20809-1-git-send-email-quic_mo...@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| 115 ++
include/soc/qcom/qcom_minidump.h | 23 ++
5 files changed, 213 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 5af33b0e3470..ed23e0275c22 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -277,4 +277,14 @@ config QCOM_PBS
This module provides the APIs to the client drivers that wants to
send the
PBS trigger event to the PBS RAM.
+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 ca0bece0dfff..44664589263d 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_PBS) += qcom-pbs.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 ..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 {
+ charname[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
