Re: [PATCH v3 09/18] soc: qcom: Add qcom's pstore minidump driver support

[Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>]

On 03/05/2023 19:02, Mukesh Ojha wrote:
> This driver was inspired from the fact pstore ram region should be
> fixed and boot firmware need to have awarness about this region,
> so that it will be persistent across boot. But, there are many
> QCOM SoC which does not support warm boot from hardware but they
> have minidump support from the software, and for them, there is
> no need of this pstore ram region to be fixed, but at the same
> time have interest in the pstore frontends. So, this driver
> get the dynamic reserved region from the ram and register the
> ramoops platform device.
> 
>  +---------+     +---------+   +--------+     +---------+
>  | console |     | pmsg    |   | ftrace |     | dmesg   |
>  +---------+     +---------+   +--------+     +---------+
>        |             |             |              |
>        |             |             |              |
>        +------------------------------------------+
>                           |
>                          \ /
>                   +----------------+
>             (1)   |pstore frontends|
>                   +----------------+
>                           |
>                          \ /
>                  +------------------- +
>             (2)  | pstore backend(ram)|
>                  +--------------------+
>                           |
>                          \ /
>                  +--------------------+
>             (3)  |qcom_pstore_minidump|
>                  +--------------------+
>                           |
>                          \ /
>                    +---------------+
>             (4)    | qcom_minidump |
>                    +---------------+
> 
> This driver will route all the pstore front data to the stored
> in qcom pstore reserved region and the reason of showing an
> arrow from (3) to (4) as qcom_pstore_minidump driver will register
> all the available frontends region with qcom minidump driver
> in upcoming patch.
> 
> Signed-off-by: Mukesh Ojha <quic_mojha@xxxxxxxxxxx>
> ---
>  drivers/soc/qcom/Kconfig                |  11 +++
>  drivers/soc/qcom/Makefile               |   1 +
>  drivers/soc/qcom/qcom_pstore_minidump.c | 116 ++++++++++++++++++++++++++++++++
>  3 files changed, 128 insertions(+)
>  create mode 100644 drivers/soc/qcom/qcom_pstore_minidump.c
> 
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index 15c931e..afdc634 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -293,4 +293,15 @@ config QCOM_MINIDUMP
>  	  these selective regions will be dumped instead of the entire DDR.
>  	  This saves significant amount of time and/or storage space.
>  
> +config QCOM_PSTORE_MINIDUMP
> +	tristate "Pstore support for QCOM Minidump"
> +	depends on ARCH_QCOM
> +	depends on PSTORE_RAM
> +	depends on QCOM_MINIDUMP
> +	help
> +	  Enablement of this driver ensures that ramoops region can be anywhere
> +	  reserved in ram instead of being fixed address which needs boot firmware
> +	  awareness. So, this driver creates plaform device and registers available
> +	  frontend region with the Qualcomm's minidump driver.
> +
>  endmenu
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index 1ebe081..02d30d7 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -34,3 +34,4 @@ obj-$(CONFIG_QCOM_KRYO_L2_ACCESSORS) +=	kryo-l2-accessors.o
>  obj-$(CONFIG_QCOM_ICC_BWMON)	+= icc-bwmon.o
>  obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE)	+= ice.o
>  obj-$(CONFIG_QCOM_MINIDUMP) += qcom_minidump.o
> +obj-$(CONFIG_QCOM_PSTORE_MINIDUMP) += qcom_pstore_minidump.o
> diff --git a/drivers/soc/qcom/qcom_pstore_minidump.c b/drivers/soc/qcom/qcom_pstore_minidump.c
> new file mode 100644
> index 0000000..8d58500
> --- /dev/null
> +++ b/drivers/soc/qcom/qcom_pstore_minidump.c
> @@ -0,0 +1,116 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +/*
> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/of_reserved_mem.h>
> +#include <linux/platform_device.h>
> +#include <linux/pstore_ram.h>
> +#include <soc/qcom/qcom_minidump.h>
> +
> +struct qcom_ramoops_config {
> +	unsigned long	record_size;
> +	unsigned long	console_size;
> +	unsigned long	ftrace_size;
> +	unsigned long	pmsg_size;
> +	unsigned int	mem_type;
> +	unsigned int	flags;
> +	int		max_reason;
> +};
> +
> +struct qcom_ramoops_dd {
> +	struct ramoops_platform_data qcom_ramoops_pdata;
> +	struct platform_device *ramoops_pdev;
> +};
> +
> +static struct qcom_ramoops_config default_ramoops_config = {

Cannot this be const?


> +	.mem_type = 2,
> +	.record_size = 0x0,
> +	.console_size = 0x200000,
> +	.ftrace_size = 0x0,
> +	.pmsg_size = 0x0,
> +};
> +
> +static struct qcom_ramoops_dd *qcom_rdd;

Drop file scope variable. It's not even used.

> +static int qcom_ramoops_probe(struct platform_device *pdev)
> +{
> +	struct device_node *of_node = pdev->dev.of_node;
> +	struct device_node *node;
> +	const struct qcom_ramoops_config *cfg;
> +	struct ramoops_platform_data *pdata;
> +	struct reserved_mem *rmem;
> +	long ret;
> +
> +	node = of_parse_phandle(of_node, "memory-region", 0);
> +	if (!node)
> +		return -ENODEV;
> +
> +	rmem = of_reserved_mem_lookup(node);
> +	of_node_put(node);
> +	if (!rmem) {
> +		dev_err(&pdev->dev, "failed to locate DT /reserved-memory resource\n");
> +		return -EINVAL;
> +	}
> +
> +	qcom_rdd = devm_kzalloc(&pdev->dev, sizeof(*qcom_rdd), GFP_KERNEL);
> +	if (!qcom_rdd)
> +		return -ENOMEM;
> +
> +	cfg = of_device_get_match_data(&pdev->dev);
> +	if (!cfg) {
> +		dev_err(&pdev->dev, "failed to get supported matched data\n");
> +		return -ENOENT;
> +	}
> +
> +	pdata = &qcom_rdd->qcom_ramoops_pdata;
> +	pdata->mem_size = rmem->size;
> +	pdata->mem_address = rmem->base;
> +	pdata->mem_type = cfg->mem_type;
> +	pdata->record_size = cfg->record_size;
> +	pdata->console_size = cfg->console_size;
> +	pdata->ftrace_size = cfg->ftrace_size;
> +	pdata->pmsg_size = cfg->pmsg_size;
> +	pdata->max_reason = KMSG_DUMP_PANIC;
> +
> +	qcom_rdd->ramoops_pdev = platform_device_register_data(NULL, "ramoops", -1,
> +							       pdata, sizeof(*pdata));
> +	if (IS_ERR(qcom_rdd->ramoops_pdev)) {
> +		ret = PTR_ERR(qcom_rdd->ramoops_pdev);
> +		dev_err(&pdev->dev, "could not create platform device: %ld\n", ret);
> +		qcom_rdd->ramoops_pdev = NULL;
> +	}
> +
> +	return ret;
> +}
> +
> +static int qcom_ramoops_remove(struct platform_device *pdev)

Use instead .remove_new callback.

> +{
> +	platform_device_unregister(qcom_rdd->ramoops_pdev);
> +	qcom_rdd->ramoops_pdev = NULL;
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id qcom_ramoops_of_match[] = {
> +	{ .compatible = "qcom,sm8450-ramoops-minidump", .data = &default_ramoops_config },

You don't need this entry.

> +	{ .compatible = "qcom,ramoops-minidump", .data = &default_ramoops_config },
> +	{}
> +};
> +
> +MODULE_DEVICE_TABLE(of, qcom_ramoops_of_match);

Blank line goes after the MODULE_DEVICE_TABLE, not before.


Best regards,
Krzysztof