Re: [PATCH 3/4] firmware: qcom: scm: Add minidump SRAM support

[Dmitry Baryshkov]

On Thu, May 07, 2026 at 01:37:19PM +0530, Mukesh Ojha wrote:
> On most Qualcomm SoCs where minidump is supported, a word in always-on
> SRAM is shared between the kernel and boot firmware. Before DDR is
> initialised on the warm reset following a crash, firmware reads this
> word to decide if minidump is enabled and collect a minidump and where
> to deliver it (USB upload to a host, or save to local storage).
> 
> The SRAM region is described by a 'sram'/'sram-names' phandle pair on
> the SCM DT node. If the property is absent the feature is silently
> disabled, keeping existing SoCs unaffected.
> 
> Expose a 'minidump_dest' module parameter (default: usb) so the user can
> select the destination. Only the string names "usb" or "storage" are
> accepted; an invalid value is rejected with -EINVAL. Changing the
> destination while minidump mode is already active updates SRAM immediately.
> 
> Suggested-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxxxxxxxx>
> Signed-off-by: Mukesh Ojha <mukesh.ojha@xxxxxxxxxxxxxxxx>
> ---
>  drivers/firmware/qcom/qcom_scm.c | 95 ++++++++++++++++++++++++++++++++
>  1 file changed, 95 insertions(+)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index f65b132004a5..b57f8cce7a8c 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -57,6 +57,7 @@ struct qcom_scm {
>  	int scm_vote_count;
>  
>  	u64 dload_mode_addr;
> +	void __iomem *minidump_sram;
>  
>  	struct qcom_tzmem_pool *mempool;
>  	unsigned int wq_cnt;
> @@ -141,6 +142,18 @@ static const u8 qcom_scm_cpu_warm_bits[QCOM_SCM_BOOT_MAX_CPUS] = {
>  #define QCOM_DLOAD_MINIDUMP	2
>  #define QCOM_DLOAD_BOTHDUMP	3
>  
> +/* Minidump destination values written to always-on SRAM for boot firmware */
> +#define QCOM_MINIDUMP_DEST_USB		0x0
> +#define QCOM_MINIDUMP_DEST_STORAGE	0x2

This makes one wonder, what is 0x01

> +
> +static u32 minidump_dest = QCOM_MINIDUMP_DEST_USB;
> +
> +static const char * const minidump_dest_name[] = { "usb", "storage" };
> +static const u32 minidump_dest_val[] = {
> +	QCOM_MINIDUMP_DEST_USB,
> +	QCOM_MINIDUMP_DEST_STORAGE,
> +};
> +
>  #define QCOM_SCM_DEFAULT_WAITQ_COUNT 1
>  
>  static const char * const qcom_scm_convention_names[] = {
> @@ -568,6 +581,17 @@ static void qcom_scm_set_download_mode(u32 dload_mode)
>  
>  	if (ret)
>  		dev_err(__scm->dev, "failed to set download mode: %d\n", ret);
> +
> +	/*
> +	 * Mirror the destination into the always-on SRAM so boot firmware
> +	 * can read it before DDR is initialised on the next warm reset.
> +	 * Only written when minidump is active; skip if SRAM already holds
> +	 * the requested destination to avoid unnecessary writes.

And writes are bad, because?

> +	 */
> +	if (__scm->minidump_sram && (dload_mode & QCOM_DLOAD_MINIDUMP)) {
> +		if (readl_relaxed(__scm->minidump_sram) != minidump_dest)
> +			writel_relaxed(minidump_dest, __scm->minidump_sram);
> +	}
>  }
>  
>  /**
> @@ -2055,6 +2079,37 @@ int qcom_scm_gpu_init_regs(u32 gpu_req)
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_gpu_init_regs);
>  
> +static int qcom_scm_map_minidump_sram(struct device *dev, void __iomem **out)
> +{
> +	struct device_node *np = dev->of_node;
> +	struct device_node *sram_np;
> +	struct resource res;
> +	int ret;
> +
> +	if (of_property_match_string(np, "sram-names", "minidump") < 0)
> +		return 0;

Do you actually need sram-names? Just to verify that it has one string?

> +
> +	sram_np = of_parse_phandle(np, "sram", 0);
> +	if (!sram_np)
> +		return -EINVAL;
> +
> +	ret = of_address_to_resource(sram_np, 0, &res);
> +	of_node_put(sram_np);
> +	if (ret)
> +		return ret;
> +
> +	if (resource_size(&res) < sizeof(u32)) {
> +		dev_err(dev, "minidump SRAM region too small\n");
> +		return -EINVAL;
> +	}
> +
> +	*out = devm_ioremap(dev, res.start, resource_size(&res));
> +	if (!*out)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
>  static int qcom_scm_find_dload_address(struct device *dev, u64 *addr)
>  {
>  	struct device_node *tcsr;
> @@ -2748,6 +2803,41 @@ 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/0/N for no dump mode, full/on/1/Y for full dump mode, mini for minidump mode and full,mini for both full and minidump mode together are acceptable values");
>  
> +static int get_minidump_dest(char *buffer, const struct kernel_param *kp)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(minidump_dest_val); i++)
> +		if (minidump_dest == minidump_dest_val[i])
> +			return sysfs_emit(buffer, "%s\n", minidump_dest_name[i]);
> +
> +	return sysfs_emit(buffer, "unknown\n");
> +}
> +
> +static int set_minidump_dest(const char *val, const struct kernel_param *kp)
> +{
> +	int i;
> +
> +	i = sysfs_match_string(minidump_dest_name, val);
> +	if (i < 0)
> +		return -EINVAL;
> +
> +	minidump_dest = minidump_dest_val[i];
> +	if (__scm && __scm->minidump_sram && (download_mode & QCOM_DLOAD_MINIDUMP) &&
> +	    readl_relaxed(__scm->minidump_sram) != minidump_dest)
> +		writel_relaxed(minidump_dest, __scm->minidump_sram);
> +
> +	return 0;
> +}
> +
> +static const struct kernel_param_ops minidump_dest_param_ops = {
> +	.get = get_minidump_dest,
> +	.set = set_minidump_dest,
> +};
> +
> +module_param_cb(minidump_dest, &minidump_dest_param_ops, NULL, 0644);
> +MODULE_PARM_DESC(minidump_dest, "Minidump SRAM destination: usb (default) or storage");
> +
>  static int qcom_scm_probe(struct platform_device *pdev)
>  {
>  	struct qcom_tzmem_pool_config pool_config;
> @@ -2765,6 +2855,11 @@ static int qcom_scm_probe(struct platform_device *pdev)
>  		return dev_err_probe(&pdev->dev, ret,
>  				     "Failed to find download mode address\n");
>  
> +	ret = qcom_scm_map_minidump_sram(&pdev->dev, &scm->minidump_sram);
> +	if (ret < 0)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "Failed to map minidump SRAM\n");
> +
>  	mutex_init(&scm->scm_bw_lock);
>  
>  	scm->path = devm_of_icc_get(&pdev->dev, NULL);
> -- 
> 2.53.0
> 

-- 
With best wishes
Dmitry