[RFC PATCH 0/1] Add driver to read secondary bootloader (XBL) log

[RFC PATCH 0/1] Add driver to read secondary bootloader (XBL) log

[Ninad Naik]

Boot time logs for Qualcomm secondary boot-loader or XBL can help to
identify different set of information regarding firmware configuration,
SoC boot KPIs. A dedicated region is carved out in the memory map in order
to store this log in the memory.

The objective of this driver is to read the XBL boot log stored in
this memory region post boot-up, and provide an entry in debugfs, which
can be used to read back the logs and print them on to the console.

Further, this log can be parsed to extract different set of information
as required.

For example, one use case can be parsing boot time KPIs to calculate,
 - PBL End time
 - SBL End time
 - UEFI Start/Stop time

Corresponding log contents as captured in /sys/kernel/debug/xbl_log entry
which prints the above counters,

        S -     59378 - PBL, End
        B -     68198 - SBL1, Start
        B -   1308877 - SBL1, End
        UEFI Start     [ 3370]
         - 0x0AF001000 [ 3375] Sec.efi
        UEFI Total : 466 m
        POST Time      [ 3836] OS Loader
         - 0x0ABE10000 [ 3838] LinuxLoader.efi
        Exit EBS        [ 4878] UEFI End

        /* Note - these are few excerpts picked from the complete XBL log
         *        as tested on SA8775p
         */

Ninad Naik (1):
  soc: qcom: Add driver to read secondary bootloader (XBL) log

 drivers/soc/qcom/Kconfig        |  13 +++
 drivers/soc/qcom/Makefile       |   1 +
 drivers/soc/qcom/dump_xbl_log.c | 139 ++++++++++++++++++++++++++++++++
 3 files changed, 153 insertions(+)
 create mode 100644 drivers/soc/qcom/dump_xbl_log.c

-- 
2.41.0

[RFC PATCH 1/1] soc: qcom: Add driver to read secondary bootloader (XBL) log

[Ninad Naik]

Qualcomm secondary bootloader (XBL) boot log holds information to
identify various firmware configuration currently set on the SoC.
The XBL log is stored in a predefined reserved memory region.

This drivers provides a way to print XBL logs on the console. To
do so, it provides a debugfs entry which captures the logs stored
in this reserved memory region. This entry can now be used to read
and print the XBL logs to console.

User can use the below command to print XBL log to console:
        cat /sys/kernel/debug/xbl_log

Signed-off-by: Ninad Naik <quic_ninanaik@quicinc.com>
---
 drivers/soc/qcom/Kconfig        |  13 +++
 drivers/soc/qcom/Makefile       |   1 +
 drivers/soc/qcom/dump_xbl_log.c | 139 ++++++++++++++++++++++++++++++++
 3 files changed, 153 insertions(+)
 create mode 100644 drivers/soc/qcom/dump_xbl_log.c

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 715348869d04..4489d37e924d 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -291,4 +291,17 @@ config QCOM_INLINE_CRYPTO_ENGINE
 	tristate
 	select QCOM_SCM
 
+config QCOM_DUMP_XBL_LOG
+	tristate "Qualcomm driver to print XBL logs on console from debugfs"
+	help
+	  This driver is used to capture secondary bootloader (xbl) log
+	  from a reserved memory region and provide a debugfs entry to read
+	  logs captured from this memory region and print them on console.
+	  User can use below command to print the xbl log on console:
+
+                cat /sys/kernel/debug/xbl_log
+
+	  These logs help to identify firmware configuration information on
+	  the SoC. The name of the built module will be dump_xbl_log
+
 endmenu
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index bbca2e1e55bb..aac088a1a0b6 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -32,3 +32,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_DUMP_XBL_LOG)	+= dump_xbl_log.o
diff --git a/drivers/soc/qcom/dump_xbl_log.c b/drivers/soc/qcom/dump_xbl_log.c
new file mode 100644
index 000000000000..ea335a5e660b
--- /dev/null
+++ b/drivers/soc/qcom/dump_xbl_log.c
@@ -0,0 +1,139 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2021, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/memblock.h>
+#include <linux/of_address.h>
+#include <linux/err.h>
+#include <linux/debugfs.h>
+
+struct xbl_log_data {
+	struct device *dev;
+	size_t buf_size;
+	void __iomem *xbl_buf;
+	struct dentry *dbg_file;
+	struct debugfs_blob_wrapper dbg_data;
+};
+
+static int map_addr_range(struct device_node **parent, const char *name,
+			  struct xbl_log_data *xbl_data)
+{
+	struct device_node *node;
+	struct resource res;
+	int ret;
+
+	node = of_find_node_by_name(*parent, name);
+	if (!node)
+		return -ENODEV;
+
+	ret = of_address_to_resource(node, 0, &res);
+	if (ret) {
+		dev_err(xbl_data->dev, "Failed to parse memory region\n");
+		return ret;
+	}
+	of_node_put(node);
+
+	if (!resource_size(&res)) {
+		dev_err(xbl_data->dev, "Failed to parse memory region size\n");
+		return -ENODEV;
+	}
+
+	xbl_data->buf_size = resource_size(&res) - 1;
+	xbl_data->xbl_buf = devm_memremap(xbl_data->dev, res.start,
+					  xbl_data->buf_size, MEMREMAP_WB);
+	if (!xbl_data->xbl_buf) {
+		dev_err(xbl_data->dev, "%s: memory remap failed\n", name);
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static int xbl_log_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct xbl_log_data *xbl_data;
+	struct device_node *parent;
+	int ret;
+
+	xbl_data = devm_kzalloc(dev, sizeof(*xbl_data), GFP_KERNEL);
+	if (!xbl_data)
+		return -ENOMEM;
+
+	xbl_data->dev = &pdev->dev;
+	platform_set_drvdata(pdev, xbl_data);
+
+	parent = of_find_node_by_path("/reserved-memory");
+	if (!parent) {
+		dev_err(xbl_data->dev, "reserved-memory node missing\n");
+		return -ENODEV;
+	}
+
+	ret = map_addr_range(&parent, "uefi-log", xbl_data);
+	if (ret)
+		goto put_node;
+
+	xbl_data->dbg_data.data = xbl_data->xbl_buf;
+	xbl_data->dbg_data.size = xbl_data->buf_size;
+	xbl_data->dbg_file = debugfs_create_blob("xbl_log", 0400, NULL,
+						 &xbl_data->dbg_data);
+	if (IS_ERR(xbl_data->dbg_file)) {
+		dev_err(xbl_data->dev, "failed to create debugfs entry\n");
+		ret = PTR_ERR(xbl_data->dbg_file);
+	}
+
+put_node:
+	of_node_put(parent);
+	return ret;
+}
+
+static int xbl_log_remove(struct platform_device *pdev)
+{
+	struct xbl_log_data *xbl_data = platform_get_drvdata(pdev);
+
+	debugfs_remove_recursive(xbl_data->dbg_file);
+	return 0;
+}
+
+static struct platform_driver xbl_log_driver = {
+	.probe = xbl_log_probe,
+	.remove = xbl_log_remove,
+	.driver = {
+		   .name = "xbl-log",
+		   },
+};
+
+static struct platform_device xbl_log_device = {
+	.name = "xbl-log",
+};
+
+static int __init xbl_log_init(void)
+{
+	int ret = 0;
+
+	ret = platform_driver_register(&xbl_log_driver);
+	if (!ret) {
+		ret = platform_device_register(&xbl_log_device);
+		if (ret)
+			platform_driver_unregister(&xbl_log_driver);
+	}
+	return ret;
+}
+
+static void __exit xbl_log_exit(void)
+{
+	platform_device_unregister(&xbl_log_device);
+	platform_driver_unregister(&xbl_log_driver);
+}
+
+module_init(xbl_log_init);
+module_exit(xbl_log_exit);
+
+MODULE_DESCRIPTION("Qualcomm Technologies, Inc. (QTI) XBL log driver");
+MODULE_LICENSE("GPL");
-- 
2.41.0

Re: [RFC PATCH 1/1] soc: qcom: Add driver to read secondary bootloader (XBL) log

[Pavan Kondeti]

On Tue, Aug 22, 2023 at 05:45:12PM +0530, Ninad Naik wrote:
> Qualcomm secondary bootloader (XBL) boot log holds information to
> identify various firmware configuration currently set on the SoC.
> The XBL log is stored in a predefined reserved memory region.
> 
> This drivers provides a way to print XBL logs on the console. To
> do so, it provides a debugfs entry which captures the logs stored
> in this reserved memory region. This entry can now be used to read
> and print the XBL logs to console.
> 
> User can use the below command to print XBL log to console:
>         cat /sys/kernel/debug/xbl_log
> 
> Signed-off-by: Ninad Naik <quic_ninanaik@quicinc.com>
> ---

For a single patch, cover letter may not be needed. The under cut
portion (this area) of the patch can be used to present the additional
details.

>  drivers/soc/qcom/Kconfig        |  13 +++
>  drivers/soc/qcom/Makefile       |   1 +
>  drivers/soc/qcom/dump_xbl_log.c | 139 ++++++++++++++++++++++++++++++++
>  3 files changed, 153 insertions(+)
>  create mode 100644 drivers/soc/qcom/dump_xbl_log.c
> 

[...]

> +static int map_addr_range(struct device_node **parent, const char *name,
> +			  struct xbl_log_data *xbl_data)
> +{
> +	struct device_node *node;
> +	struct resource res;
> +	int ret;
> +
> +	node = of_find_node_by_name(*parent, name);
> +	if (!node)
> +		return -ENODEV;
> +
> +	ret = of_address_to_resource(node, 0, &res);
> +	if (ret) {
> +		dev_err(xbl_data->dev, "Failed to parse memory region\n");
> +		return ret;
> +	}
> +	of_node_put(node);
> +
> +	if (!resource_size(&res)) {
> +		dev_err(xbl_data->dev, "Failed to parse memory region size\n");
> +		return -ENODEV;
> +	}
> +
> +	xbl_data->buf_size = resource_size(&res) - 1;
> +	xbl_data->xbl_buf = devm_memremap(xbl_data->dev, res.start,
> +					  xbl_data->buf_size, MEMREMAP_WB);
> +	if (!xbl_data->xbl_buf) {
> +		dev_err(xbl_data->dev, "%s: memory remap failed\n", name);
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +static int xbl_log_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct xbl_log_data *xbl_data;
> +	struct device_node *parent;
> +	int ret;
> +
> +	xbl_data = devm_kzalloc(dev, sizeof(*xbl_data), GFP_KERNEL);
> +	if (!xbl_data)
> +		return -ENOMEM;
> +
> +	xbl_data->dev = &pdev->dev;
> +	platform_set_drvdata(pdev, xbl_data);
> +
> +	parent = of_find_node_by_path("/reserved-memory");
> +	if (!parent) {
> +		dev_err(xbl_data->dev, "reserved-memory node missing\n");
> +		return -ENODEV;
> +	}
> +

You would need to present the Device Tree binding document for this. For
ex: pls see 
Documentation/devicetree/bindings/reserved-memory/qcom,cmd-db.yaml 

> +	ret = map_addr_range(&parent, "uefi-log", xbl_data);
> +	if (ret)
> +		goto put_node;
> +
> +	xbl_data->dbg_data.data = xbl_data->xbl_buf;
> +	xbl_data->dbg_data.size = xbl_data->buf_size;
> +	xbl_data->dbg_file = debugfs_create_blob("xbl_log", 0400, NULL,
> +						 &xbl_data->dbg_data);
> +	if (IS_ERR(xbl_data->dbg_file)) {
> +		dev_err(xbl_data->dev, "failed to create debugfs entry\n");
> +		ret = PTR_ERR(xbl_data->dbg_file);
> +	}
> +
> +put_node:
> +	of_node_put(parent);
> +	return ret;
> +}
> +
> +static int xbl_log_remove(struct platform_device *pdev)
> +{
> +	struct xbl_log_data *xbl_data = platform_get_drvdata(pdev);
> +
> +	debugfs_remove_recursive(xbl_data->dbg_file);
> +	return 0;
> +}
> +
> +static struct platform_driver xbl_log_driver = {
> +	.probe = xbl_log_probe,
> +	.remove = xbl_log_remove,
> +	.driver = {
> +		   .name = "xbl-log",
> +		   },
> +};
> +
> +static struct platform_device xbl_log_device = {
> +	.name = "xbl-log",
> +};
> +
> +static int __init xbl_log_init(void)
> +{
> +	int ret = 0;
> +
> +	ret = platform_driver_register(&xbl_log_driver);
> +	if (!ret) {
> +		ret = platform_device_register(&xbl_log_device);
> +		if (ret)
> +			platform_driver_unregister(&xbl_log_driver);
> +	}
> +	return ret;
> +}
> +

The platform device registration, the resource parsing can be completely
avoided by adding your compatible entry to reserved_mem_matches
structure defined in drivers/of/platform.c . There are some Qualcomm SoC
devices also present in that list.

> +static void __exit xbl_log_exit(void)
> +{
> +	platform_device_unregister(&xbl_log_device);
> +	platform_driver_unregister(&xbl_log_driver);
> +}
> +
> +module_init(xbl_log_init);
> +module_exit(xbl_log_exit);
> +
> +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. (QTI) XBL log driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.41.0
> 

Re: [RFC PATCH 1/1] soc: qcom: Add driver to read secondary bootloader (XBL) log

[Bryan O'Donoghue]

On 22/08/2023 13:15, Ninad Naik wrote:
> Qualcomm secondary bootloader (XBL) boot log holds information to
> identify various firmware configuration currently set on the SoC.
> The XBL log is stored in a predefined reserved memory region.
> 
> This drivers provides a way to print XBL logs on the console. To
> do so, it provides a debugfs entry which captures the logs stored
> in this reserved memory region. This entry can now be used to read
> and print the XBL logs to console.
> 
> User can use the below command to print XBL log to console:
>          cat /sys/kernel/debug/xbl_log
> 
> Signed-off-by: Ninad Naik <quic_ninanaik@quicinc.com>
> ---
>   drivers/soc/qcom/Kconfig        |  13 +++
>   drivers/soc/qcom/Makefile       |   1 +
>   drivers/soc/qcom/dump_xbl_log.c | 139 ++++++++++++++++++++++++++++++++
>   3 files changed, 153 insertions(+)
>   create mode 100644 drivers/soc/qcom/dump_xbl_log.c
> 
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index 715348869d04..4489d37e924d 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -291,4 +291,17 @@ config QCOM_INLINE_CRYPTO_ENGINE
>   	tristate
>   	select QCOM_SCM
>   
> +config QCOM_DUMP_XBL_LOG
> +	tristate "Qualcomm driver to print XBL logs on console from debugfs"
> +	help
> +	  This driver is used to capture secondary bootloader (xbl) log
> +	  from a reserved memory region and provide a debugfs entry to read
> +	  logs captured from this memory region and print them on console.
> +	  User can use below command to print the xbl log on console:
> +
> +                cat /sys/kernel/debug/xbl_log
> +
> +	  These logs help to identify firmware configuration information on
> +	  the SoC. The name of the built module will be dump_xbl_log
> +
>   endmenu
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index bbca2e1e55bb..aac088a1a0b6 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -32,3 +32,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_DUMP_XBL_LOG)	+= dump_xbl_log.o
> diff --git a/drivers/soc/qcom/dump_xbl_log.c b/drivers/soc/qcom/dump_xbl_log.c
> new file mode 100644
> index 000000000000..ea335a5e660b
> --- /dev/null
> +++ b/drivers/soc/qcom/dump_xbl_log.c
> @@ -0,0 +1,139 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2021, The Linux Foundation. All rights reserved.
> + */

2023

> +
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include <linux/memblock.h>
> +#include <linux/of_address.h>
> +#include <linux/err.h>
> +#include <linux/debugfs.h>

It would be nice to alphetise this include list.

> +struct xbl_log_data {
> +	struct device *dev;
> +	size_t buf_size;
> +	void __iomem *xbl_buf;
> +	struct dentry *dbg_file;
> +	struct debugfs_blob_wrapper dbg_data;
> +};
> +
> +static int map_addr_range(struct device_node **parent, const char *name,
> +			  struct xbl_log_data *xbl_data)
> +{
> +	struct device_node *node;
> +	struct resource res;
> +	int ret;
> +
> +	node = of_find_node_by_name(*parent, name);
> +	if (!node)
> +		return -ENODEV;
> +
> +	ret = of_address_to_resource(node, 0, &res);
> +	if (ret) {
> +		dev_err(xbl_data->dev, "Failed to parse memory region\n");
> +		return ret;
> +	}
> +	of_node_put(node);
> +
> +	if (!resource_size(&res)) {
> +		dev_err(xbl_data->dev, "Failed to parse memory region size\n");
> +		return -ENODEV;
> +	}
> +
> +	xbl_data->buf_size = resource_size(&res) - 1;
> +	xbl_data->xbl_buf = devm_memremap(xbl_data->dev, res.start,
> +					  xbl_data->buf_size, MEMREMAP_WB);
> +	if (!xbl_data->xbl_buf) {
> +		dev_err(xbl_data->dev, "%s: memory remap failed\n", name);
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +static int xbl_log_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct xbl_log_data *xbl_data;
> +	struct device_node *parent;
> +	int ret;
> +
> +	xbl_data = devm_kzalloc(dev, sizeof(*xbl_data), GFP_KERNEL);
> +	if (!xbl_data)
> +		return -ENOMEM;
> +
> +	xbl_data->dev = &pdev->dev;
> +	platform_set_drvdata(pdev, xbl_data);
> +
> +	parent = of_find_node_by_path("/reserved-memory");
> +	if (!parent) {
> +		dev_err(xbl_data->dev, "reserved-memory node missing\n");
> +		return -ENODEV;
> +	}
> +
> +	ret = map_addr_range(&parent, "uefi-log", xbl_data);
> +	if (ret)
> +		goto put_node;
> +
> +	xbl_data->dbg_data.data = xbl_data->xbl_buf;
> +	xbl_data->dbg_data.size = xbl_data->buf_size;
> +	xbl_data->dbg_file = debugfs_create_blob("xbl_log", 0400, NULL,
> +						 &xbl_data->dbg_data);
> +	if (IS_ERR(xbl_data->dbg_file)) {
> +		dev_err(xbl_data->dev, "failed to create debugfs entry\n");
> +		ret = PTR_ERR(xbl_data->dbg_file);
> +	}
> +
> +put_node:
> +	of_node_put(parent);
> +	return ret;
> +}
> +
> +static int xbl_log_remove(struct platform_device *pdev)
> +{
> +	struct xbl_log_data *xbl_data = platform_get_drvdata(pdev);
> +
> +	debugfs_remove_recursive(xbl_data->dbg_file);
> +	return 0;
> +}
> +
> +static struct platform_driver xbl_log_driver = {
> +	.probe = xbl_log_probe,
> +	.remove = xbl_log_remove,
> +	.driver = {
> +		   .name = "xbl-log",
> +		   },
Is that indentation correct ?

> +};
> +
> +static struct platform_device xbl_log_device = {
> +	.name = "xbl-log",
> +};
> +
> +static int __init xbl_log_init(void)
> +{
> +	int ret = 0;
> +
> +	ret = platform_driver_register(&xbl_log_driver);
> +	if (!ret) {
> +		ret = platform_device_register(&xbl_log_device);
> +		if (ret)
> +			platform_driver_unregister(&xbl_log_driver);
> +	}
> +	return ret;
> +}
> +
> +static void __exit xbl_log_exit(void)
> +{
> +	platform_device_unregister(&xbl_log_device);
> +	platform_driver_unregister(&xbl_log_driver);
> +}
> +
> +module_init(xbl_log_init);
> +module_exit(xbl_log_exit);
> +
> +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. (QTI) XBL log driver");
> +MODULE_LICENSE("GPL");

---
bod

Re: [RFC PATCH 1/1] soc: qcom: Add driver to read secondary bootloader (XBL) log

[Srinivas Kandagatla]

On 22/08/2023 13:15, Ninad Naik wrote:
> Qualcomm secondary bootloader (XBL) boot log holds information to
> identify various firmware configuration currently set on the SoC.
> The XBL log is stored in a predefined reserved memory region.
> 
> This drivers provides a way to print XBL logs on the console. To
> do so, it provides a debugfs entry which captures the logs stored
> in this reserved memory region. This entry can now be used to read
> and print the XBL logs to console.
> 
> User can use the below command to print XBL log to console:
>          cat /sys/kernel/debug/xbl_log
> 
> Signed-off-by: Ninad Naik <quic_ninanaik@quicinc.com>
> ---
>   drivers/soc/qcom/Kconfig        |  13 +++
>   drivers/soc/qcom/Makefile       |   1 +
>   drivers/soc/qcom/dump_xbl_log.c | 139 ++++++++++++++++++++++++++++++++
>   3 files changed, 153 insertions(+)
>   create mode 100644 drivers/soc/qcom/dump_xbl_log.c
> 
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index 715348869d04..4489d37e924d 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -291,4 +291,17 @@ config QCOM_INLINE_CRYPTO_ENGINE
>   	tristate
>   	select QCOM_SCM
>   
> +config QCOM_DUMP_XBL_LOG
> +	tristate "Qualcomm driver to print XBL logs on console from debugfs"
> +	help
> +	  This driver is used to capture secondary bootloader (xbl) log
> +	  from a reserved memory region and provide a debugfs entry to read
> +	  logs captured from this memory region and print them on console.
> +	  User can use below command to print the xbl log on console:
> +
> +                cat /sys/kernel/debug/xbl_log
> +
> +	  These logs help to identify firmware configuration information on
> +	  the SoC. The name of the built module will be dump_xbl_log
> +
>   endmenu
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index bbca2e1e55bb..aac088a1a0b6 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -32,3 +32,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_DUMP_XBL_LOG)	+= dump_xbl_log.o
> diff --git a/drivers/soc/qcom/dump_xbl_log.c b/drivers/soc/qcom/dump_xbl_log.c
> new file mode 100644
> index 000000000000..ea335a5e660b
> --- /dev/null
> +++ b/drivers/soc/qcom/dump_xbl_log.c
> @@ -0,0 +1,139 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2021, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include <linux/memblock.h>
> +#include <linux/of_address.h>
> +#include <linux/err.h>
> +#include <linux/debugfs.h>
> +
> +struct xbl_log_data {
> +	struct device *dev;
> +	size_t buf_size;
> +	void __iomem *xbl_buf;
> +	struct dentry *dbg_file;
> +	struct debugfs_blob_wrapper dbg_data;
> +};
> +
> +static int map_addr_range(struct device_node **parent, const char *name,
> +			  struct xbl_log_data *xbl_data)
> +{
> +	struct device_node *node;
> +	struct resource res;
> +	int ret;
> +
> +	node = of_find_node_by_name(*parent, name);
> +	if (!node)
> +		return -ENODEV;
> +
> +	ret = of_address_to_resource(node, 0, &res);
> +	if (ret) {
> +		dev_err(xbl_data->dev, "Failed to parse memory region\n");
> +		return ret;
> +	}
> +	of_node_put(node);
> +
> +	if (!resource_size(&res)) {
> +		dev_err(xbl_data->dev, "Failed to parse memory region size\n");
> +		return -ENODEV;
> +	}
> +
> +	xbl_data->buf_size = resource_size(&res) - 1;
> +	xbl_data->xbl_buf = devm_memremap(xbl_data->dev, res.start,
> +					  xbl_data->buf_size, MEMREMAP_WB);
> +	if (!xbl_data->xbl_buf) {
> +		dev_err(xbl_data->dev, "%s: memory remap failed\n", name);
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +static int xbl_log_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct xbl_log_data *xbl_data;
> +	struct device_node *parent;
> +	int ret;
> +
> +	xbl_data = devm_kzalloc(dev, sizeof(*xbl_data), GFP_KERNEL);
> +	if (!xbl_data)
> +		return -ENOMEM;
> +
> +	xbl_data->dev = &pdev->dev;
> +	platform_set_drvdata(pdev, xbl_data);
> +
> +	parent = of_find_node_by_path("/reserved-memory");
> +	if (!parent) {
> +		dev_err(xbl_data->dev, "reserved-memory node missing\n");
> +		return -ENODEV;
> +	}
> +
> +	ret = map_addr_range(&parent, "uefi-log", xbl_data);

This driver really needs a proper device tree bindings, rather than 
parsing the device tree manually here in the driver.

Pl look at ./Documentation/devicetree/bindings/reserved-memory/qcom*

This should also help you getting rid of all these parsing code.

--srini



> +	if (ret)
> +		goto put_node;
> +
> +	xbl_data->dbg_data.data = xbl_data->xbl_buf;
> +	xbl_data->dbg_data.size = xbl_data->buf_size;
> +	xbl_data->dbg_file = debugfs_create_blob("xbl_log", 0400, NULL,
> +						 &xbl_data->dbg_data);
> +	if (IS_ERR(xbl_data->dbg_file)) {
> +		dev_err(xbl_data->dev, "failed to create debugfs entry\n");
> +		ret = PTR_ERR(xbl_data->dbg_file);
> +	}
> +
> +put_node:
> +	of_node_put(parent);
> +	return ret;
> +}
> +
> +static int xbl_log_remove(struct platform_device *pdev)
> +{
> +	struct xbl_log_data *xbl_data = platform_get_drvdata(pdev);
> +
> +	debugfs_remove_recursive(xbl_data->dbg_file);
> +	return 0;
> +}
> +
> +static struct platform_driver xbl_log_driver = {
> +	.probe = xbl_log_probe,
> +	.remove = xbl_log_remove,
> +	.driver = {
> +		   .name = "xbl-log",
> +		   },
> +};
> +
> +static struct platform_device xbl_log_device = {
> +	.name = "xbl-log",
> +};
> +
> +static int __init xbl_log_init(void)
> +{
> +	int ret = 0;
> +
> +	ret = platform_driver_register(&xbl_log_driver);
> +	if (!ret) {
> +		ret = platform_device_register(&xbl_log_device);
> +		if (ret)
> +			platform_driver_unregister(&xbl_log_driver);
> +	}
> +	return ret;
> +}
> +
> +static void __exit xbl_log_exit(void)
> +{
> +	platform_device_unregister(&xbl_log_device);
> +	platform_driver_unregister(&xbl_log_driver);
> +}
> +
> +module_init(xbl_log_init);
> +module_exit(xbl_log_exit);
> +
> +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. (QTI) XBL log driver");
> +MODULE_LICENSE("GPL");

Re: [RFC PATCH 1/1] soc: qcom: Add driver to read secondary bootloader (XBL) log

[Jeff Johnson]

On 8/22/2023 5:43 AM, Bryan O'Donoghue wrote:
> On 22/08/2023 13:15, Ninad Naik wrote:
..snip..
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2021, The Linux Foundation. All rights reserved.
>> + */
> 
> 2023

if this is derived work based upon existing downstream code then I 
suspect the LF 2021 copyright is correct and should be maintained, and a 
QuIC 2023 copyright should be added

Re: [RFC PATCH 1/1] soc: qcom: Add driver to read secondary bootloader (XBL) log

[Trilok Soni]

On 8/22/2023 5:15 AM, Ninad Naik wrote:
> Qualcomm secondary bootloader (XBL) boot log holds information to
> identify various firmware configuration currently set on the SoC.
> The XBL log is stored in a predefined reserved memory region.

What does "X" stands for here? From what you have described above it 
looks like SBL and not XBL.

> 
> This drivers provides a way to print XBL logs on the console. To
> do so, it provides a debugfs entry which captures the logs stored
> in this reserved memory region. This entry can now be used to read
> and print the XBL logs to console.
> 
> User can use the below command to print XBL log to console:
>          cat /sys/kernel/debug/xbl_log


It is not clear to me why these patches are posted as RFC. Please 
clarify. Are they not tested properly or just seeking some feedback and 
driver is not ready w/ all the features?

> 
> Signed-off-by: Ninad Naik <quic_ninanaik@quicinc.com>
> ---
>   drivers/soc/qcom/Kconfig        |  13 +++
>   drivers/soc/qcom/Makefile       |   1 +
>   drivers/soc/qcom/dump_xbl_log.c | 139 ++++++++++++++++++++++++++++++++
>   3 files changed, 153 insertions(+)
>   create mode 100644 drivers/soc/qcom/dump_xbl_log.c
> 
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index 715348869d04..4489d37e924d 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -291,4 +291,17 @@ config QCOM_INLINE_CRYPTO_ENGINE
>   	tristate
>   	select QCOM_SCM
>   
> +config QCOM_DUMP_XBL_LOG
> +	tristate "Qualcomm driver to print XBL logs on console from debugfs"

Why you want to print these logs from the debugfs? What is the format of 
the logs? Can you post an example log?

> +	help
> +	  This driver is used to capture secondary bootloader (xbl) log
> +	  from a reserved memory region and provide a debugfs entry to read
> +	  logs captured from this memory region and print them on console.
> +	  User can use below command to print the xbl log on console:
> +
> +                cat /sys/kernel/debug/xbl_log
> +
> +	  These logs help to identify firmware configuration information on
> +	  the SoC. The name of the built module will be dump_xbl_log
> +
>   endmenu
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index bbca2e1e55bb..aac088a1a0b6 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -32,3 +32,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_DUMP_XBL_LOG)	+= dump_xbl_log.o
> diff --git a/drivers/soc/qcom/dump_xbl_log.c b/drivers/soc/qcom/dump_xbl_log.c
> new file mode 100644
> index 000000000000..ea335a5e660b
> --- /dev/null
> +++ b/drivers/soc/qcom/dump_xbl_log.c
> @@ -0,0 +1,139 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2021, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include <linux/memblock.h>
> +#include <linux/of_address.h>
> +#include <linux/err.h>
> +#include <linux/debugfs.h>
> +
> +struct xbl_log_data {
> +	struct device *dev;
> +	size_t buf_size;
> +	void __iomem *xbl_buf;
> +	struct dentry *dbg_file;
> +	struct debugfs_blob_wrapper dbg_data;
> +};
> +
> +static int map_addr_range(struct device_node **parent, const char *name,
> +			  struct xbl_log_data *xbl_data)
> +{
> +	struct device_node *node;
> +	struct resource res;
> +	int ret;
> +
> +	node = of_find_node_by_name(*parent, name);
> +	if (!node)
> +		return -ENODEV;
> +
> +	ret = of_address_to_resource(node, 0, &res);
> +	if (ret) {
> +		dev_err(xbl_data->dev, "Failed to parse memory region\n");
> +		return ret;
> +	}
> +	of_node_put(node);
> +
> +	if (!resource_size(&res)) {
> +		dev_err(xbl_data->dev, "Failed to parse memory region size\n");
> +		return -ENODEV;
> +	}
> +
> +	xbl_data->buf_size = resource_size(&res) - 1;
> +	xbl_data->xbl_buf = devm_memremap(xbl_data->dev, res.start,
> +					  xbl_data->buf_size, MEMREMAP_WB);
> +	if (!xbl_data->xbl_buf) {
> +		dev_err(xbl_data->dev, "%s: memory remap failed\n", name);
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +static int xbl_log_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct xbl_log_data *xbl_data;
> +	struct device_node *parent;
> +	int ret;
> +
> +	xbl_data = devm_kzalloc(dev, sizeof(*xbl_data), GFP_KERNEL);
> +	if (!xbl_data)
> +		return -ENOMEM;
> +
> +	xbl_data->dev = &pdev->dev;
> +	platform_set_drvdata(pdev, xbl_data);
> +
> +	parent = of_find_node_by_path("/reserved-memory");
> +	if (!parent) {
> +		dev_err(xbl_data->dev, "reserved-memory node missing\n");
> +		return -ENODEV;
> +	}
> +
> +	ret = map_addr_range(&parent, "uefi-log", xbl_data);

Here you are calling it as uefi-log. Is it xbl-log or uefi-log? Please 
decide first.


> +	if (ret)
> +		goto put_node;
> +
> +	xbl_data->dbg_data.data = xbl_data->xbl_buf;
> +	xbl_data->dbg_data.size = xbl_data->buf_size;
> +	xbl_data->dbg_file = debugfs_create_blob("xbl_log", 0400, NULL,
> +						 &xbl_data->dbg_data);
> +	if (IS_ERR(xbl_data->dbg_file)) {
> +		dev_err(xbl_data->dev, "failed to create debugfs entry\n");
> +		ret = PTR_ERR(xbl_data->dbg_file);
> +	}
> +
> +put_node:
> +	of_node_put(parent);
> +	return ret;
> +}
> +
> +static int xbl_log_remove(struct platform_device *pdev)
> +{
> +	struct xbl_log_data *xbl_data = platform_get_drvdata(pdev);
> +
> +	debugfs_remove_recursive(xbl_data->dbg_file);
> +	return 0;
> +}
> +
> +static struct platform_driver xbl_log_driver = {
> +	.probe = xbl_log_probe,
> +	.remove = xbl_log_remove,
> +	.driver = {
> +		   .name = "xbl-log",
> +		   },
> +};
> +
> +static struct platform_device xbl_log_device = {
> +	.name = "xbl-log",
> +};
> +
> +static int __init xbl_log_init(void)
> +{
> +	int ret = 0;
> +
> +	ret = platform_driver_register(&xbl_log_driver);
> +	if (!ret) {
> +		ret = platform_device_register(&xbl_log_device);

I am puzzled here. Why?

> +		if (ret)
> +			platform_driver_unregister(&xbl_log_driver);
> +	}
> +	return ret;
> +}
> +
> +static void __exit xbl_log_exit(void)
> +{
> +	platform_device_unregister(&xbl_log_device);
> +	platform_driver_unregister(&xbl_log_driver);
> +}
> +
> +module_init(xbl_log_init);
> +module_exit(xbl_log_exit);
> +
> +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. (QTI) XBL log driver");
> +MODULE_LICENSE("GPL");
-- 
---Trilok Soni

Re: [RFC PATCH 1/1] soc: qcom: Add driver to read secondary bootloader (XBL) log

[Ninad Naik]

Hi Pavan,

On 8/22/2023 6:09 PM, Pavan Kondeti wrote:
> On Tue, Aug 22, 2023 at 05:45:12PM +0530, Ninad Naik wrote:
>> Qualcomm secondary bootloader (XBL) boot log holds information to
>> identify various firmware configuration currently set on the SoC.
>> The XBL log is stored in a predefined reserved memory region.
>>
>> This drivers provides a way to print XBL logs on the console. To
>> do so, it provides a debugfs entry which captures the logs stored
>> in this reserved memory region. This entry can now be used to read
>> and print the XBL logs to console.
>>
>> User can use the below command to print XBL log to console:
>>          cat /sys/kernel/debug/xbl_log
>>
>> Signed-off-by: Ninad Naik <quic_ninanaik@quicinc.com>
>> ---
> 
> For a single patch, cover letter may not be needed. The under cut
> portion (this area) of the patch can be used to present the additional
> details.
> 
Ack.
>>   drivers/soc/qcom/Kconfig        |  13 +++
>>   drivers/soc/qcom/Makefile       |   1 +
>>   drivers/soc/qcom/dump_xbl_log.c | 139 ++++++++++++++++++++++++++++++++
>>   3 files changed, 153 insertions(+)
>>   create mode 100644 drivers/soc/qcom/dump_xbl_log.c
>>
> 
> [...]
> 
>> +static int map_addr_range(struct device_node **parent, const char *name,
>> +			  struct xbl_log_data *xbl_data)
>> +{
>> +	struct device_node *node;
>> +	struct resource res;
>> +	int ret;
>> +
>> +	node = of_find_node_by_name(*parent, name);
>> +	if (!node)
>> +		return -ENODEV;
>> +
>> +	ret = of_address_to_resource(node, 0, &res);
>> +	if (ret) {
>> +		dev_err(xbl_data->dev, "Failed to parse memory region\n");
>> +		return ret;
>> +	}
>> +	of_node_put(node);
>> +
>> +	if (!resource_size(&res)) {
>> +		dev_err(xbl_data->dev, "Failed to parse memory region size\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	xbl_data->buf_size = resource_size(&res) - 1;
>> +	xbl_data->xbl_buf = devm_memremap(xbl_data->dev, res.start,
>> +					  xbl_data->buf_size, MEMREMAP_WB);
>> +	if (!xbl_data->xbl_buf) {
>> +		dev_err(xbl_data->dev, "%s: memory remap failed\n", name);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int xbl_log_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct xbl_log_data *xbl_data;
>> +	struct device_node *parent;
>> +	int ret;
>> +
>> +	xbl_data = devm_kzalloc(dev, sizeof(*xbl_data), GFP_KERNEL);
>> +	if (!xbl_data)
>> +		return -ENOMEM;
>> +
>> +	xbl_data->dev = &pdev->dev;
>> +	platform_set_drvdata(pdev, xbl_data);
>> +
>> +	parent = of_find_node_by_path("/reserved-memory");
>> +	if (!parent) {
>> +		dev_err(xbl_data->dev, "reserved-memory node missing\n");
>> +		return -ENODEV;
>> +	}
>> +
> 
> You would need to present the Device Tree binding document for this. For
> ex: pls see
> Documentation/devicetree/bindings/reserved-memory/qcom,cmd-db.yaml
> 
Ack. Will add a corresponding DT binding in the next version
>> +	ret = map_addr_range(&parent, "uefi-log", xbl_data);
>> +	if (ret)
>> +		goto put_node;
>> +
>> +	xbl_data->dbg_data.data = xbl_data->xbl_buf;
>> +	xbl_data->dbg_data.size = xbl_data->buf_size;
>> +	xbl_data->dbg_file = debugfs_create_blob("xbl_log", 0400, NULL,
>> +						 &xbl_data->dbg_data);
>> +	if (IS_ERR(xbl_data->dbg_file)) {
>> +		dev_err(xbl_data->dev, "failed to create debugfs entry\n");
>> +		ret = PTR_ERR(xbl_data->dbg_file);
>> +	}
>> +
>> +put_node:
>> +	of_node_put(parent);
>> +	return ret;
>> +}
>> +
>> +static int xbl_log_remove(struct platform_device *pdev)
>> +{
>> +	struct xbl_log_data *xbl_data = platform_get_drvdata(pdev);
>> +
>> +	debugfs_remove_recursive(xbl_data->dbg_file);
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver xbl_log_driver = {
>> +	.probe = xbl_log_probe,
>> +	.remove = xbl_log_remove,
>> +	.driver = {
>> +		   .name = "xbl-log",
>> +		   },
>> +};
>> +
>> +static struct platform_device xbl_log_device = {
>> +	.name = "xbl-log",
>> +};
>> +
>> +static int __init xbl_log_init(void)
>> +{
>> +	int ret = 0;
>> +
>> +	ret = platform_driver_register(&xbl_log_driver);
>> +	if (!ret) {
>> +		ret = platform_device_register(&xbl_log_device);
>> +		if (ret)
>> +			platform_driver_unregister(&xbl_log_driver);
>> +	}
>> +	return ret;
>> +}
>> +
> 
> The platform device registration, the resource parsing can be completely
> avoided by adding your compatible entry to reserved_mem_matches
> structure defined in drivers/of/platform.c . There are some Qualcomm SoC
> devices also present in that list.
> 
Ack. So once the compatible entry is added to reserved_mem_matches 
structure here, a platform_device is created for this node to which this 
driver can bind to. Thank you for the suggestion, I'll make the 
necessary corrections in the next revision.

>> +static void __exit xbl_log_exit(void)
>> +{
>> +	platform_device_unregister(&xbl_log_device);
>> +	platform_driver_unregister(&xbl_log_driver);
>> +}
>> +
>> +module_init(xbl_log_init);
>> +module_exit(xbl_log_exit);
>> +
>> +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. (QTI) XBL log driver");
>> +MODULE_LICENSE("GPL");
>> -- 
>> 2.41.0
>>
Thanks,
Ninad

Re: [RFC PATCH 1/1] soc: qcom: Add driver to read secondary bootloader (XBL) log

[Ninad Naik]

Hi Bryan,

On 8/22/2023 6:13 PM, Bryan O'Donoghue wrote:
> On 22/08/2023 13:15, Ninad Naik wrote:
>> Qualcomm secondary bootloader (XBL) boot log holds information to
>> identify various firmware configuration currently set on the SoC.
>> The XBL log is stored in a predefined reserved memory region.
>>
>> This drivers provides a way to print XBL logs on the console. To
>> do so, it provides a debugfs entry which captures the logs stored
>> in this reserved memory region. This entry can now be used to read
>> and print the XBL logs to console.
>>
>> User can use the below command to print XBL log to console:
>>          cat /sys/kernel/debug/xbl_log
>>
>> Signed-off-by: Ninad Naik <quic_ninanaik@quicinc.com>
>> ---
>>   drivers/soc/qcom/Kconfig        |  13 +++
>>   drivers/soc/qcom/Makefile       |   1 +
>>   drivers/soc/qcom/dump_xbl_log.c | 139 ++++++++++++++++++++++++++++++++
>>   3 files changed, 153 insertions(+)
>>   create mode 100644 drivers/soc/qcom/dump_xbl_log.c
>>
>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>> index 715348869d04..4489d37e924d 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -291,4 +291,17 @@ config QCOM_INLINE_CRYPTO_ENGINE
>>       tristate
>>       select QCOM_SCM
>> +config QCOM_DUMP_XBL_LOG
>> +    tristate "Qualcomm driver to print XBL logs on console from debugfs"
>> +    help
>> +      This driver is used to capture secondary bootloader (xbl) log
>> +      from a reserved memory region and provide a debugfs entry to read
>> +      logs captured from this memory region and print them on console.
>> +      User can use below command to print the xbl log on console:
>> +
>> +                cat /sys/kernel/debug/xbl_log
>> +
>> +      These logs help to identify firmware configuration information on
>> +      the SoC. The name of the built module will be dump_xbl_log
>> +
>>   endmenu
>> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
>> index bbca2e1e55bb..aac088a1a0b6 100644
>> --- a/drivers/soc/qcom/Makefile
>> +++ b/drivers/soc/qcom/Makefile
>> @@ -32,3 +32,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_DUMP_XBL_LOG)    += dump_xbl_log.o
>> diff --git a/drivers/soc/qcom/dump_xbl_log.c 
>> b/drivers/soc/qcom/dump_xbl_log.c
>> new file mode 100644
>> index 000000000000..ea335a5e660b
>> --- /dev/null
>> +++ b/drivers/soc/qcom/dump_xbl_log.c
>> @@ -0,0 +1,139 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2021, The Linux Foundation. All rights reserved.
>> + */
> 
> 2023
> 
>> +
>> +#include <linux/init.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/memblock.h>
>> +#include <linux/of_address.h>
>> +#include <linux/err.h>
>> +#include <linux/debugfs.h>
> 
> It would be nice to alphetise this include list.
> 
Ack. Sure, I'll make this fix. Thank you.
>> +struct xbl_log_data {
>> +    struct device *dev;
>> +    size_t buf_size;
>> +    void __iomem *xbl_buf;
>> +    struct dentry *dbg_file;
>> +    struct debugfs_blob_wrapper dbg_data;
>> +};
>> +
>> +static int map_addr_range(struct device_node **parent, const char *name,
>> +              struct xbl_log_data *xbl_data)
>> +{
>> +    struct device_node *node;
>> +    struct resource res;
>> +    int ret;
>> +
>> +    node = of_find_node_by_name(*parent, name);
>> +    if (!node)
>> +        return -ENODEV;
>> +
>> +    ret = of_address_to_resource(node, 0, &res);
>> +    if (ret) {
>> +        dev_err(xbl_data->dev, "Failed to parse memory region\n");
>> +        return ret;
>> +    }
>> +    of_node_put(node);
>> +
>> +    if (!resource_size(&res)) {
>> +        dev_err(xbl_data->dev, "Failed to parse memory region size\n");
>> +        return -ENODEV;
>> +    }
>> +
>> +    xbl_data->buf_size = resource_size(&res) - 1;
>> +    xbl_data->xbl_buf = devm_memremap(xbl_data->dev, res.start,
>> +                      xbl_data->buf_size, MEMREMAP_WB);
>> +    if (!xbl_data->xbl_buf) {
>> +        dev_err(xbl_data->dev, "%s: memory remap failed\n", name);
>> +        return -ENOMEM;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int xbl_log_probe(struct platform_device *pdev)
>> +{
>> +    struct device *dev = &pdev->dev;
>> +    struct xbl_log_data *xbl_data;
>> +    struct device_node *parent;
>> +    int ret;
>> +
>> +    xbl_data = devm_kzalloc(dev, sizeof(*xbl_data), GFP_KERNEL);
>> +    if (!xbl_data)
>> +        return -ENOMEM;
>> +
>> +    xbl_data->dev = &pdev->dev;
>> +    platform_set_drvdata(pdev, xbl_data);
>> +
>> +    parent = of_find_node_by_path("/reserved-memory");
>> +    if (!parent) {
>> +        dev_err(xbl_data->dev, "reserved-memory node missing\n");
>> +        return -ENODEV;
>> +    }
>> +
>> +    ret = map_addr_range(&parent, "uefi-log", xbl_data);
>> +    if (ret)
>> +        goto put_node;
>> +
>> +    xbl_data->dbg_data.data = xbl_data->xbl_buf;
>> +    xbl_data->dbg_data.size = xbl_data->buf_size;
>> +    xbl_data->dbg_file = debugfs_create_blob("xbl_log", 0400, NULL,
>> +                         &xbl_data->dbg_data);
>> +    if (IS_ERR(xbl_data->dbg_file)) {
>> +        dev_err(xbl_data->dev, "failed to create debugfs entry\n");
>> +        ret = PTR_ERR(xbl_data->dbg_file);
>> +    }
>> +
>> +put_node:
>> +    of_node_put(parent);
>> +    return ret;
>> +}
>> +
>> +static int xbl_log_remove(struct platform_device *pdev)
>> +{
>> +    struct xbl_log_data *xbl_data = platform_get_drvdata(pdev);
>> +
>> +    debugfs_remove_recursive(xbl_data->dbg_file);
>> +    return 0;
>> +}
>> +
>> +static struct platform_driver xbl_log_driver = {
>> +    .probe = xbl_log_probe,
>> +    .remove = xbl_log_remove,
>> +    .driver = {
>> +           .name = "xbl-log",
>> +           },
> Is that indentation correct ?
>
No, the closing parenthesis should be fixed. Will correct this, thank you.

>> +};
>> +
>> +static struct platform_device xbl_log_device = {
>> +    .name = "xbl-log",
>> +};
>> +
>> +static int __init xbl_log_init(void)
>> +{
>> +    int ret = 0;
>> +
>> +    ret = platform_driver_register(&xbl_log_driver);
>> +    if (!ret) {
>> +        ret = platform_device_register(&xbl_log_device);
>> +        if (ret)
>> +            platform_driver_unregister(&xbl_log_driver);
>> +    }
>> +    return ret;
>> +}
>> +
>> +static void __exit xbl_log_exit(void)
>> +{
>> +    platform_device_unregister(&xbl_log_device);
>> +    platform_driver_unregister(&xbl_log_driver);
>> +}
>> +
>> +module_init(xbl_log_init);
>> +module_exit(xbl_log_exit);
>> +
>> +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. (QTI) XBL log driver");
>> +MODULE_LICENSE("GPL");
> 
> ---
> bod
Thanks,
Ninad

Re: [RFC PATCH 1/1] soc: qcom: Add driver to read secondary bootloader (XBL) log

[Ninad Naik]

Hi Jeff,

On 8/22/2023 8:44 PM, Jeff Johnson wrote:
> On 8/22/2023 5:43 AM, Bryan O'Donoghue wrote:
>> On 22/08/2023 13:15, Ninad Naik wrote:
> ..snip..
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * Copyright (c) 2021, The Linux Foundation. All rights reserved.
>>> + */
>>
>> 2023
> 
> if this is derived work based upon existing downstream code then I 
> suspect the LF 2021 copyright is correct and should be maintained, and a
Yes, this driver is derived from an existing downstream code and hence 
2021 license is carried forward> QuIC 2023 copyright should be added
> 
Ack.

Thanks,
Ninad

Re: [RFC PATCH 1/1] soc: qcom: Add driver to read secondary bootloader (XBL) log

[Ninad Naik]

Hi Srinivas,

On 8/22/2023 7:11 PM, Srinivas Kandagatla wrote:
> 
> 
> On 22/08/2023 13:15, Ninad Naik wrote:
>> Qualcomm secondary bootloader (XBL) boot log holds information to
>> identify various firmware configuration currently set on the SoC.
>> The XBL log is stored in a predefined reserved memory region.
>>
>> This drivers provides a way to print XBL logs on the console. To
>> do so, it provides a debugfs entry which captures the logs stored
>> in this reserved memory region. This entry can now be used to read
>> and print the XBL logs to console.
>>
>> User can use the below command to print XBL log to console:
>>          cat /sys/kernel/debug/xbl_log
>>
>> Signed-off-by: Ninad Naik <quic_ninanaik@quicinc.com>
>> ---
>>   drivers/soc/qcom/Kconfig        |  13 +++
>>   drivers/soc/qcom/Makefile       |   1 +
>>   drivers/soc/qcom/dump_xbl_log.c | 139 ++++++++++++++++++++++++++++++++
>>   3 files changed, 153 insertions(+)
>>   create mode 100644 drivers/soc/qcom/dump_xbl_log.c
>>
>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>> index 715348869d04..4489d37e924d 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -291,4 +291,17 @@ config QCOM_INLINE_CRYPTO_ENGINE
>>       tristate
>>       select QCOM_SCM
>> +config QCOM_DUMP_XBL_LOG
>> +    tristate "Qualcomm driver to print XBL logs on console from debugfs"
>> +    help
>> +      This driver is used to capture secondary bootloader (xbl) log
>> +      from a reserved memory region and provide a debugfs entry to read
>> +      logs captured from this memory region and print them on console.
>> +      User can use below command to print the xbl log on console:
>> +
>> +                cat /sys/kernel/debug/xbl_log
>> +
>> +      These logs help to identify firmware configuration information on
>> +      the SoC. The name of the built module will be dump_xbl_log
>> +
>>   endmenu
>> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
>> index bbca2e1e55bb..aac088a1a0b6 100644
>> --- a/drivers/soc/qcom/Makefile
>> +++ b/drivers/soc/qcom/Makefile
>> @@ -32,3 +32,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_DUMP_XBL_LOG)    += dump_xbl_log.o
>> diff --git a/drivers/soc/qcom/dump_xbl_log.c 
>> b/drivers/soc/qcom/dump_xbl_log.c
>> new file mode 100644
>> index 000000000000..ea335a5e660b
>> --- /dev/null
>> +++ b/drivers/soc/qcom/dump_xbl_log.c
>> @@ -0,0 +1,139 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2021, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include <linux/init.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/memblock.h>
>> +#include <linux/of_address.h>
>> +#include <linux/err.h>
>> +#include <linux/debugfs.h>
>> +
>> +struct xbl_log_data {
>> +    struct device *dev;
>> +    size_t buf_size;
>> +    void __iomem *xbl_buf;
>> +    struct dentry *dbg_file;
>> +    struct debugfs_blob_wrapper dbg_data;
>> +};
>> +
>> +static int map_addr_range(struct device_node **parent, const char *name,
>> +              struct xbl_log_data *xbl_data)
>> +{
>> +    struct device_node *node;
>> +    struct resource res;
>> +    int ret;
>> +
>> +    node = of_find_node_by_name(*parent, name);
>> +    if (!node)
>> +        return -ENODEV;
>> +
>> +    ret = of_address_to_resource(node, 0, &res);
>> +    if (ret) {
>> +        dev_err(xbl_data->dev, "Failed to parse memory region\n");
>> +        return ret;
>> +    }
>> +    of_node_put(node);
>> +
>> +    if (!resource_size(&res)) {
>> +        dev_err(xbl_data->dev, "Failed to parse memory region size\n");
>> +        return -ENODEV;
>> +    }
>> +
>> +    xbl_data->buf_size = resource_size(&res) - 1;
>> +    xbl_data->xbl_buf = devm_memremap(xbl_data->dev, res.start,
>> +                      xbl_data->buf_size, MEMREMAP_WB);
>> +    if (!xbl_data->xbl_buf) {
>> +        dev_err(xbl_data->dev, "%s: memory remap failed\n", name);
>> +        return -ENOMEM;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int xbl_log_probe(struct platform_device *pdev)
>> +{
>> +    struct device *dev = &pdev->dev;
>> +    struct xbl_log_data *xbl_data;
>> +    struct device_node *parent;
>> +    int ret;
>> +
>> +    xbl_data = devm_kzalloc(dev, sizeof(*xbl_data), GFP_KERNEL);
>> +    if (!xbl_data)
>> +        return -ENOMEM;
>> +
>> +    xbl_data->dev = &pdev->dev;
>> +    platform_set_drvdata(pdev, xbl_data);
>> +
>> +    parent = of_find_node_by_path("/reserved-memory");
>> +    if (!parent) {
>> +        dev_err(xbl_data->dev, "reserved-memory node missing\n");
>> +        return -ENODEV;
>> +    }
>> +
>> +    ret = map_addr_range(&parent, "uefi-log", xbl_data);
> 
> This driver really needs a proper device tree bindings, rather than 
> parsing the device tree manually here in the driver.
> 
> Pl look at ./Documentation/devicetree/bindings/reserved-memory/qcom*
> 
> This should also help you getting rid of all these parsing code.
> 
> --srini
>
Ack. I'll add a device tree binding and make corresponding fixes without 
the parsing and device registration, taking this link as well as Pavan's 
suggestion of adding the compatible entry to the reserved_mem_matches 
table in consideration. Thank you.>
> 
>> +    if (ret)
>> +        goto put_node;
>> +
>> +    xbl_data->dbg_data.data = xbl_data->xbl_buf;
>> +    xbl_data->dbg_data.size = xbl_data->buf_size;
>> +    xbl_data->dbg_file = debugfs_create_blob("xbl_log", 0400, NULL,
>> +                         &xbl_data->dbg_data);
>> +    if (IS_ERR(xbl_data->dbg_file)) {
>> +        dev_err(xbl_data->dev, "failed to create debugfs entry\n");
>> +        ret = PTR_ERR(xbl_data->dbg_file);
>> +    }
>> +
>> +put_node:
>> +    of_node_put(parent);
>> +    return ret;
>> +}
>> +
>> +static int xbl_log_remove(struct platform_device *pdev)
>> +{
>> +    struct xbl_log_data *xbl_data = platform_get_drvdata(pdev);
>> +
>> +    debugfs_remove_recursive(xbl_data->dbg_file);
>> +    return 0;
>> +}
>> +
>> +static struct platform_driver xbl_log_driver = {
>> +    .probe = xbl_log_probe,
>> +    .remove = xbl_log_remove,
>> +    .driver = {
>> +           .name = "xbl-log",
>> +           },
>> +};
>> +
>> +static struct platform_device xbl_log_device = {
>> +    .name = "xbl-log",
>> +};
>> +
>> +static int __init xbl_log_init(void)
>> +{
>> +    int ret = 0;
>> +
>> +    ret = platform_driver_register(&xbl_log_driver);
>> +    if (!ret) {
>> +        ret = platform_device_register(&xbl_log_device);
>> +        if (ret)
>> +            platform_driver_unregister(&xbl_log_driver);
>> +    }
>> +    return ret;
>> +}
>> +
>> +static void __exit xbl_log_exit(void)
>> +{
>> +    platform_device_unregister(&xbl_log_device);
>> +    platform_driver_unregister(&xbl_log_driver);
>> +}
>> +
>> +module_init(xbl_log_init);
>> +module_exit(xbl_log_exit);
>> +
>> +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. (QTI) XBL log driver");
>> +MODULE_LICENSE("GPL");

Thanks,
Ninad

Re: [RFC PATCH 1/1] soc: qcom: Add driver to read secondary bootloader (XBL) log

[Ninad Naik]

Hi Trilok,

On 8/22/2023 9:38 PM, Trilok Soni wrote:
> On 8/22/2023 5:15 AM, Ninad Naik wrote:
>> Qualcomm secondary bootloader (XBL) boot log holds information to
>> identify various firmware configuration currently set on the SoC.
>> The XBL log is stored in a predefined reserved memory region.
> 
> What does "X" stands for here? From what you have described above it 
> looks like SBL and not XBL.
>
Ack. I will change the commit text from "Secondary" to "eXtensible". 
Thank you.
>>
>> This drivers provides a way to print XBL logs on the console. To
>> do so, it provides a debugfs entry which captures the logs stored
>> in this reserved memory region. This entry can now be used to read
>> and print the XBL logs to console.
>>
>> User can use the below command to print XBL log to console:
>>          cat /sys/kernel/debug/xbl_log
> 
> 
> It is not clear to me why these patches are posted as RFC. Please 
> clarify. Are they not tested properly or just seeking some feedback and 
> driver is not ready w/ all the features?
>
Hi Trilok, the driver is tested on sa8775 platform.The reason for 
posting as RFC is to seek feedback on possible improvements. One was 
primarily on the probing mechanism which I have pointers now from Pavan 
and Srini's suggestions.
>>
>> Signed-off-by: Ninad Naik <quic_ninanaik@quicinc.com>
>> ---
>>   drivers/soc/qcom/Kconfig        |  13 +++
>>   drivers/soc/qcom/Makefile       |   1 +
>>   drivers/soc/qcom/dump_xbl_log.c | 139 ++++++++++++++++++++++++++++++++
>>   3 files changed, 153 insertions(+)
>>   create mode 100644 drivers/soc/qcom/dump_xbl_log.c
>>
>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>> index 715348869d04..4489d37e924d 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -291,4 +291,17 @@ config QCOM_INLINE_CRYPTO_ENGINE
>>       tristate
>>       select QCOM_SCM
>> +config QCOM_DUMP_XBL_LOG
>> +    tristate "Qualcomm driver to print XBL logs on console from debugfs"
> 
> Why you want to print these logs from the debugfs? What is the format of 
> the logs? Can you post an example log?
> 
This log is printed in plain ascii format and are accessed post boot up, 
so chose to access them through debugfs. I have uploaded the full logs 
at [1]

[1] https://gist.github.com/ninadnaik-quic/914e86f6a0acadade632dc45b727d87b
>> +    help
>> +      This driver is used to capture secondary bootloader (xbl) log
>> +      from a reserved memory region and provide a debugfs entry to read
>> +      logs captured from this memory region and print them on console.
>> +      User can use below command to print the xbl log on console:
>> +
>> +                cat /sys/kernel/debug/xbl_log
>> +
>> +      These logs help to identify firmware configuration information on
>> +      the SoC. The name of the built module will be dump_xbl_log
>> +
>>   endmenu
>> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
>> index bbca2e1e55bb..aac088a1a0b6 100644
>> --- a/drivers/soc/qcom/Makefile
>> +++ b/drivers/soc/qcom/Makefile
>> @@ -32,3 +32,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_DUMP_XBL_LOG)    += dump_xbl_log.o
>> diff --git a/drivers/soc/qcom/dump_xbl_log.c 
>> b/drivers/soc/qcom/dump_xbl_log.c
>> new file mode 100644
>> index 000000000000..ea335a5e660b
>> --- /dev/null
>> +++ b/drivers/soc/qcom/dump_xbl_log.c
>> @@ -0,0 +1,139 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2021, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include <linux/init.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/memblock.h>
>> +#include <linux/of_address.h>
>> +#include <linux/err.h>
>> +#include <linux/debugfs.h>
>> +
>> +struct xbl_log_data {
>> +    struct device *dev;
>> +    size_t buf_size;
>> +    void __iomem *xbl_buf;
>> +    struct dentry *dbg_file;
>> +    struct debugfs_blob_wrapper dbg_data;
>> +};
>> +
>> +static int map_addr_range(struct device_node **parent, const char *name,
>> +              struct xbl_log_data *xbl_data)
>> +{
>> +    struct device_node *node;
>> +    struct resource res;
>> +    int ret;
>> +
>> +    node = of_find_node_by_name(*parent, name);
>> +    if (!node)
>> +        return -ENODEV;
>> +
>> +    ret = of_address_to_resource(node, 0, &res);
>> +    if (ret) {
>> +        dev_err(xbl_data->dev, "Failed to parse memory region\n");
>> +        return ret;
>> +    }
>> +    of_node_put(node);
>> +
>> +    if (!resource_size(&res)) {
>> +        dev_err(xbl_data->dev, "Failed to parse memory region size\n");
>> +        return -ENODEV;
>> +    }
>> +
>> +    xbl_data->buf_size = resource_size(&res) - 1;
>> +    xbl_data->xbl_buf = devm_memremap(xbl_data->dev, res.start,
>> +                      xbl_data->buf_size, MEMREMAP_WB);
>> +    if (!xbl_data->xbl_buf) {
>> +        dev_err(xbl_data->dev, "%s: memory remap failed\n", name);
>> +        return -ENOMEM;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int xbl_log_probe(struct platform_device *pdev)
>> +{
>> +    struct device *dev = &pdev->dev;
>> +    struct xbl_log_data *xbl_data;
>> +    struct device_node *parent;
>> +    int ret;
>> +
>> +    xbl_data = devm_kzalloc(dev, sizeof(*xbl_data), GFP_KERNEL);
>> +    if (!xbl_data)
>> +        return -ENOMEM;
>> +
>> +    xbl_data->dev = &pdev->dev;
>> +    platform_set_drvdata(pdev, xbl_data);
>> +
>> +    parent = of_find_node_by_path("/reserved-memory");
>> +    if (!parent) {
>> +        dev_err(xbl_data->dev, "reserved-memory node missing\n");
>> +        return -ENODEV;
>> +    }
>> +
>> +    ret = map_addr_range(&parent, "uefi-log", xbl_data);
> 
> Here you are calling it as uefi-log. Is it xbl-log or uefi-log? Please 
> decide first.
> 
> 
The reason for using "uefi-log" here is because this node name is in 
accordance to the sa8775p.dtsi as seen in [2]

[2] 
https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/qcom/sa8775p.dtsi#L354

So, in the next revision while adding the device tree bindings and 
corresponding compatible string, should this node name be changed as well?
>> +    if (ret)
>> +        goto put_node;
>> +
>> +    xbl_data->dbg_data.data = xbl_data->xbl_buf;
>> +    xbl_data->dbg_data.size = xbl_data->buf_size;
>> +    xbl_data->dbg_file = debugfs_create_blob("xbl_log", 0400, NULL,
>> +                         &xbl_data->dbg_data);
>> +    if (IS_ERR(xbl_data->dbg_file)) {
>> +        dev_err(xbl_data->dev, "failed to create debugfs entry\n");
>> +        ret = PTR_ERR(xbl_data->dbg_file);
>> +    }
>> +
>> +put_node:
>> +    of_node_put(parent);
>> +    return ret;
>> +}
>> +
>> +static int xbl_log_remove(struct platform_device *pdev)
>> +{
>> +    struct xbl_log_data *xbl_data = platform_get_drvdata(pdev);
>> +
>> +    debugfs_remove_recursive(xbl_data->dbg_file);
>> +    return 0;
>> +}
>> +
>> +static struct platform_driver xbl_log_driver = {
>> +    .probe = xbl_log_probe,
>> +    .remove = xbl_log_remove,
>> +    .driver = {
>> +           .name = "xbl-log",
>> +           },
>> +};
>> +
>> +static struct platform_device xbl_log_device = {
>> +    .name = "xbl-log",
>> +};
>> +
>> +static int __init xbl_log_init(void)
>> +{
>> +    int ret = 0;
>> +
>> +    ret = platform_driver_register(&xbl_log_driver);
>> +    if (!ret) {
>> +        ret = platform_device_register(&xbl_log_device);
> 
> I am puzzled here. Why?
>
Ack. I was registering a platform device to get the driver probed when 
built as module. I'll correct this according to comments from Srinivas 
and Pavan.

>> +        if (ret)
>> +            platform_driver_unregister(&xbl_log_driver);
>> +    }
>> +    return ret;
>> +}
>> +
>> +static void __exit xbl_log_exit(void)
>> +{
>> +    platform_device_unregister(&xbl_log_device);
>> +    platform_driver_unregister(&xbl_log_driver);
>> +}
>> +
>> +module_init(xbl_log_init);
>> +module_exit(xbl_log_exit);
>> +
>> +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. (QTI) XBL log driver");
>> +MODULE_LICENSE("GPL");

Thanks,
Ninad

Re: [RFC PATCH 1/1] soc: qcom: Add driver to read secondary bootloader (XBL) log

[Trilok Soni]

On 8/23/2023 4:16 AM, Ninad Naik wrote:
>>>   ret = map_addr_range(&parent, "uefi-log", xbl_data);
>>
>> Here you are calling it as uefi-log. Is it xbl-log or uefi-log? Please 
>> decide first.
>>
>>
> The reason for using "uefi-log" here is because this node name is in 
> accordance to the sa8775p.dtsi as seen in [2]
> 
> [2] 
> https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/qcom/sa8775p.dtsi#L354
> 
> So, in the next revision while adding the device tree bindings and 
> corresponding compatible string, should this node name be changed as well?

Yes, please keep it consistent.

-- 
---Trilok Soni

Re: [RFC PATCH 0/1] Add driver to read secondary bootloader (XBL) log

[Arun Kumar Neelakantam]

On 8/22/2023 5:45 PM, Ninad Naik wrote:
> Boot time logs for Qualcomm secondary boot-loader or XBL can help to
> identify different set of information regarding firmware configuration,
> SoC boot KPIs. A dedicated region is carved out in the memory map in order
> to store this log in the memory.
> 
> The objective of this driver is to read the XBL boot log stored in
> this memory region post boot-up, and provide an entry in debugfs, which
> can be used to read back the logs and print them on to the console.
> 

I see couple of use cases for this kind of logging like logs from boot, 
Hypervisor, Trusted Execution environments and also one in upstream for 
chromeos EC console. Can this be made a generic driver which take log 
name, log memory buffer address and size to read from debugfs.
> Further, this log can be parsed to extract different set of information
> as required.
> 
> For example, one use case can be parsing boot time KPIs to calculate,
>   - PBL End time
>   - SBL End time
>   - UEFI Start/Stop time
> 
> Corresponding log contents as captured in /sys/kernel/debug/xbl_log entry
> which prints the above counters,
> 
>          S -     59378 - PBL, End
>          B -     68198 - SBL1, Start
>          B -   1308877 - SBL1, End
>          UEFI Start     [ 3370]
>           - 0x0AF001000 [ 3375] Sec.efi
>          UEFI Total : 466 m
>          POST Time      [ 3836] OS Loader
>           - 0x0ABE10000 [ 3838] LinuxLoader.efi
>          Exit EBS        [ 4878] UEFI End
> 
>          /* Note - these are few excerpts picked from the complete XBL log
>           *        as tested on SA8775p
>           */
> 
> Ninad Naik (1):
>    soc: qcom: Add driver to read secondary bootloader (XBL) log
> 
>   drivers/soc/qcom/Kconfig        |  13 +++
>   drivers/soc/qcom/Makefile       |   1 +
>   drivers/soc/qcom/dump_xbl_log.c | 139 ++++++++++++++++++++++++++++++++
>   3 files changed, 153 insertions(+)
>   create mode 100644 drivers/soc/qcom/dump_xbl_log.c
> 

Re: [RFC PATCH 0/1] Add driver to read secondary bootloader (XBL) log

[Trilok Soni]

On 8/28/2023 10:19 AM, Arun Kumar Neelakantam wrote:
> 
> 
> On 8/22/2023 5:45 PM, Ninad Naik wrote:
>> Boot time logs for Qualcomm secondary boot-loader or XBL can help to
>> identify different set of information regarding firmware configuration,
>> SoC boot KPIs. A dedicated region is carved out in the memory map in order
>> to store this log in the memory.
>>
>> The objective of this driver is to read the XBL boot log stored in
>> this memory region post boot-up, and provide an entry in debugfs, which
>> can be used to read back the logs and print them on to the console.
>>
> 
> I see couple of use cases for this kind of logging like logs from boot, Hypervisor, Trusted Execution environments and also one in upstream for chromeos EC console. Can this be made a generic driver which take log name, log memory buffer address and size to read from debugfs.

The one downside of generic solution here is that log format may not be consistent. Some may have binary format of logs which will need further parsing in kernel or userspace. 

If we need to make such feature generic then it needs to be generic across arm64 / arm32 then and not SOC specific. 


-- 
---Trilok Soni

Re: [RFC PATCH 0/1] Add driver to read secondary bootloader (XBL) log

[Bjorn Andersson]

On Mon, Aug 28, 2023 at 11:44:08AM -0700, Trilok Soni wrote:
> On 8/28/2023 10:19 AM, Arun Kumar Neelakantam wrote:
> > 
> > 
> > On 8/22/2023 5:45 PM, Ninad Naik wrote:
> >> Boot time logs for Qualcomm secondary boot-loader or XBL can help to
> >> identify different set of information regarding firmware configuration,
> >> SoC boot KPIs. A dedicated region is carved out in the memory map in order
> >> to store this log in the memory.
> >>
> >> The objective of this driver is to read the XBL boot log stored in
> >> this memory region post boot-up, and provide an entry in debugfs, which
> >> can be used to read back the logs and print them on to the console.
> >>
> > 
> > I see couple of use cases for this kind of logging like logs from
> > boot, Hypervisor, Trusted Execution environments and also one in
> > upstream for chromeos EC console. Can this be made a generic driver
> > which take log name, log memory buffer address and size to read from
> > debugfs.
> 
> The one downside of generic solution here is that log format may not
> be consistent. Some may have binary format of logs which will need
> further parsing in kernel or userspace. 
> 
> If we need to make such feature generic then it needs to be generic
> across arm64 / arm32 then and not SOC specific. 
> 

More importantly, it should provide a consistent interface towards the
consumer of the data, so that one wouldn't need a custom app to collect
every type of data.

Regards,
Bjorn

Re: [RFC PATCH 0/1] Add driver to read secondary bootloader (XBL) log

[Nikunj Kela]

From: Nikunj Kela <quic_nkela@quicinc.com

I was planning something similar for dumping firmware VM's logs. These are in
ascii format too hence doesn't require any parsing/post processing. Can this
be little generic so I can use this driver?

Re: [RFC PATCH 0/1] Add driver to read secondary bootloader (XBL) log

[Trilok Soni]

On 9/12/2023 9:08 AM, Nikunj Kela wrote:
> From: Nikunj Kela <quic_nkela@quicinc.com
> 
> I was planning something similar for dumping firmware VM's logs. These are in
> ascii format too hence doesn't require any parsing/post processing. Can this
> be little generic so I can use this driver?

This is exactly I want to avoid too. The discussion internally lead us to look at the UEFI/EDK2 work done by Alexander Graf [1] and TPM EFI log driver. For EDK2 we should have generic approach coming from the EDK2 and not just SOC vendor specific approaches. 


[1]
https://edk2.groups.io/g/devel/message/90067
https://github.com/agraf/linux/commit/b1fe0c296ec923e9b1f544862b0eb9365a8da7cb

--
---Trilok Soni