[RFC PATCH 0/1] Add driver for bootstage stash

[RFC PATCH 0/1] Add driver for bootstage stash

[Francesco Valla]

Hello,

after the discussion on the "Unified Boot Log" topic during the latest
Boot Time SIG special meeting [1], I tried to mock up a driver that
reads a bootstage stash saved by the U-Boot bootloader in a given memory
area and exposes the data in a user- and machine- friendly through both
sysfs and debugfs attributes. Details on the interfaces, as well as
example output for the debugfs interfaces, can be found on the
documentation that is part of the patchset.

To use this driver, a memory area shall be reserved inside the Linux
kernel devicetree as follows (possibly changing the address and the size
of the memory area):

    bootstage@a4300000 {
        compatible = "bootstage";
        reg = <0 0xa4300000 0 0x1000>;
        no-map;
    };

At U-Boot side, following configuration shall then be set:

    CONFIG_BOOTSTAGE=y
    CONFIG_BOOTSTAGE_STASH_ADDR=0xa4300000
    CONFIG_BOOTSTAGE_STASH_SIZE=0x1000

Once booted, the bootstage data can will be found at:

    - /sys/devices/platform/a4300000.bootstage/
    - /sys/kernel/debug/bootstage/a4300000.bootstage/

The device name is purposely part of the sysfs and debugfs paths to
support multiple bootstage areas, as this _might_ then be used for
multiple bootstage sources, e.g. bootloaders running on different
cores inside a SoC with different architectures.

Note that this is not really meant to be integrated as-is, not only
because it's a single patch including code, documentation and devicetree
bindings, but also because the bootstage stash format itself may need to
be touched up a bit. In particular, fixed data type should probably be
evaluated for the bootstage record, in order to increase compatibility
with different data sources.


Comments are of course welcome.

Regards,

Francesco

[1] https://lore.kernel.org/linux-embedded/MW5PR13MB5632B8FA3279D77F2F9217BBFD9CA@MW5PR13MB5632.namprd13.prod.outlook.com/

Francesco Valla (1):
  drivers: misc: add driver for bootstage stash

 .../bindings/reserved-memory/bootstage.yaml   |  44 +++
 Documentation/misc-devices/bootstage.rst      |  53 ++++
 Documentation/misc-devices/index.rst          |   1 +
 MAINTAINERS                                   |   7 +
 drivers/misc/Kconfig                          |  10 +
 drivers/misc/Makefile                         |   1 +
 drivers/misc/bootstage.c                      | 292 ++++++++++++++++++
 drivers/of/platform.c                         |   1 +
 8 files changed, 409 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reserved-memory/bootstage.yaml
 create mode 100644 Documentation/misc-devices/bootstage.rst
 create mode 100644 drivers/misc/bootstage.c

-- 
2.49.0

[PATCH 1/1] drivers: misc: add driver for bootstage stash

[Francesco Valla]

Add support for bootstage stash areas containing boot time data
created by some bootloader (e.g. U-Boot). The driver provides generic
time information through sysfs and platform-specific one through
debugfs.

Signed-off-by: Francesco Valla <francesco@valla.it>
---
 .../bindings/reserved-memory/bootstage.yaml   |  44 +++
 Documentation/misc-devices/bootstage.rst      |  53 ++++
 Documentation/misc-devices/index.rst          |   1 +
 MAINTAINERS                                   |   7 +
 drivers/misc/Kconfig                          |  10 +
 drivers/misc/Makefile                         |   1 +
 drivers/misc/bootstage.c                      | 292 ++++++++++++++++++
 drivers/of/platform.c                         |   1 +
 8 files changed, 409 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reserved-memory/bootstage.yaml
 create mode 100644 Documentation/misc-devices/bootstage.rst
 create mode 100644 drivers/misc/bootstage.c

diff --git a/Documentation/devicetree/bindings/reserved-memory/bootstage.yaml b/Documentation/devicetree/bindings/reserved-memory/bootstage.yaml
new file mode 100644
index 000000000000..e71d85c5c2ce
--- /dev/null
+++ b/Documentation/devicetree/bindings/reserved-memory/bootstage.yaml
@@ -0,0 +1,44 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/reserved-memory/bootstage.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Bootstage stash
+
+description: |
+  This binding represents a reserved memory region containing bootstage stash
+  data generated by a previous bootloader stage.
+
+maintainers:
+  - Francesco Valla <francesco@valla.it>
+
+allOf:
+  - $ref: reserved-memory.yaml
+
+properties:
+  compatible:
+    const: bootstage
+
+  reg:
+    description: page-aligned region of memory containing bootstage stash data
+
+required:
+  - compatible
+  - reg
+  - no-map
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    reserved-memory {
+        #address-cells = <2>;
+        #size-cells = <1>;
+
+        bootstage: bootstage@12340000 {
+            compatible = "bootstage";
+            reg = <0x00 0x12340000 0x2000>;
+            no-map;
+        };
+    };
diff --git a/Documentation/misc-devices/bootstage.rst b/Documentation/misc-devices/bootstage.rst
new file mode 100644
index 000000000000..2e1bbd31aab8
--- /dev/null
+++ b/Documentation/misc-devices/bootstage.rst
@@ -0,0 +1,53 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+================
+Bootstage driver
+================
+
+The bootstage driver exports interfaces to read from a bootstage stash area
+saved by a bootloader (e.g.: U-Boot) that ran before the Linux kernel.
+
+Two kind of interfaces are exported:
+
+- a sysfs interface for bootloader- and platform-agnostic data
+- a debugfs interface for bootloader- and platform-specific data
+
+
+The sysfs interface
+-------------------
+
+Following sysfs attributes can be found at /sys/devices/platform/<device-name>/:
+
+- start_time_us: bootloader start time in microseconds
+- end_time_us: bootloader end time in microseconds
+
+
+The debugfs interface
+---------------------
+
+Following debugfs interfaces can be found at
+/sys/kernel/debug/bootstage/<device-name>/:
+
+- stages: details on staged bootloader stages, with start time and duration.
+  Example output::
+
+    Mark (us)  Elapsed (us)  Stage
+            0             0  reset
+       183689        183689  SPL
+       489247        305558  end phase
+       506987         17740  board_init_f
+      1257880        750893  board_init_r
+      1622303        364423  eth_common_init
+      1888033        265730  eth_initialize
+      1893077          5044  main_loop
+      4204282       2311205  cli_loop
+
+- accumulated_time: time accumulated during certain bootloader stages.
+  Example output::
+
+    Time (us)  Stage
+         4902  dm_spl
+       322719  dm_f
+         9527  dm_r
+
+The number and type of staged stages are bootloader- and platform-specific.
diff --git a/Documentation/misc-devices/index.rst b/Documentation/misc-devices/index.rst
index 8c5b226d8313..c5ebb3d44505 100644
--- a/Documentation/misc-devices/index.rst
+++ b/Documentation/misc-devices/index.rst
@@ -28,3 +28,4 @@ fit into other categories.
    tps6594-pfsm
    uacce
    xilinx_sdfec
+   bootstage
diff --git a/MAINTAINERS b/MAINTAINERS
index f21f1dabb5fe..0bdecd07023a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4203,6 +4203,13 @@ F:	Documentation/ABI/stable/sysfs-class-bluetooth
 F:	include/net/bluetooth/
 F:	net/bluetooth/
 
+BOOTSTAGE DRIVER
+M:	Francesco Valla <francesco@valla.it>
+L:	linux-embedded@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/reserved-memory/bootstage.yaml
+F:	drivers/misc/bootstage.c
+
 BONDING DRIVER
 M:	Jay Vosburgh <jv@jvosburgh.net>
 L:	netdev@vger.kernel.org
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 6b37d61150ee..97cdfa241c0c 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -632,6 +632,16 @@ config MCHP_LAN966X_PCI
 	    - lan966x-miim (MDIO_MSCC_MIIM)
 	    - lan966x-switch (LAN966X_SWITCH)
 
+config BOOTSTAGE
+	tristate "Bootstage stash support"
+	depends on OF_RESERVED_MEM
+	help
+	  This enables the support for a bootstage stash.
+
+	  A bootstage stash can be created by some bootloaders (e.g.: U-Boot) to
+	  store information on its boot timings. This driver provides access to
+	  these information through sysfs and debugsfs interfaces.
+
 source "drivers/misc/c2port/Kconfig"
 source "drivers/misc/eeprom/Kconfig"
 source "drivers/misc/cb710/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index d6c917229c45..3562c1bf701f 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -74,3 +74,4 @@ lan966x-pci-objs		:= lan966x_pci.o
 lan966x-pci-objs		+= lan966x_pci.dtbo.o
 obj-$(CONFIG_MCHP_LAN966X_PCI)	+= lan966x-pci.o
 obj-y				+= keba/
+obj-$(CONFIG_BOOTSTAGE)		+= bootstage.o
diff --git a/drivers/misc/bootstage.c b/drivers/misc/bootstage.c
new file mode 100644
index 000000000000..a106410a56ee
--- /dev/null
+++ b/drivers/misc/bootstage.c
@@ -0,0 +1,292 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2025 - Francesco Valla <francesco@valla.it>
+ *
+ * Driver for bootstage stash.
+ *
+ * This driver exposes bootstage stash generated by bootloader and/or firmware
+ * stages that run before the Linux kernel.
+ *
+ */
+
+#include <linux/debugfs.h>
+#include <linux/io.h>
+#include <linux/mm.h>
+#include <linux/module.h>
+#include <linux/of_reserved_mem.h>
+#include <linux/platform_device.h>
+#include <linux/seq_file.h>
+
+#define BOOTSTAGE_MAGIC		0xb00757a3
+#define BOOTSTAGE_MAX_VERSION	0
+
+enum bootstage_id {
+	BOOTSTAGE_ID_START = 0,
+};
+
+enum bootstage_flags {
+	BOOTSTAGEF_ERROR	= 1 << 0,	/* Error record */
+	BOOTSTAGEF_ALLOC	= 1 << 1,	/* Allocate an id */
+};
+
+struct bootstage_record {
+	ulong time_us;
+	u32 start_us;
+	const char *name;
+	int flags;
+	enum bootstage_id id;
+};
+
+struct bootstage_hdr {
+	u32 version;		/* Boostage stash version */
+	u32 count;		/* Number of records */
+	u32 size;		/* Total data size (non-zero if valid) */
+	u32 magic;		/* Magic number */
+	u32 next_id;		/* Next ID to use for bootstage */
+};
+
+struct bootstage_drvdata {
+	struct bootstage_hdr *hdr;
+	struct bootstage_record *records;
+
+	u32 start_time_us;
+	u32 end_time_us;
+
+	struct dentry *debugfs_dir;
+};
+
+static struct dentry *bootstage_debugfs_dir;
+
+static int stages_show(struct seq_file *m, void *d)
+{
+	struct bootstage_drvdata *drvdata = m->private;
+	struct bootstage_hdr *hdr = drvdata->hdr;
+	struct bootstage_record *rec;
+	u32 prev = 0;
+	int i;
+
+	seq_printf(m, "%13s %13s  %s\n", "Mark (us)", "Elapsed (us)", "Stage");
+
+	for (i = 0, rec = drvdata->records; i < hdr->count; i++, rec++) {
+		if ((rec->id && !rec->start_us) || (i == 0)) {
+			if (prev > rec->time_us)
+				prev = 0;
+			seq_printf(m, "%13lu %13lu  %s\n", rec->time_us,
+				   rec->time_us - prev, rec->name);
+			prev = rec->time_us;
+		}
+	}
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(stages);
+
+static int accumulated_time_show(struct seq_file *m, void *d)
+{
+	struct bootstage_drvdata *drvdata = m->private;
+	struct bootstage_hdr *hdr = drvdata->hdr;
+	struct bootstage_record *rec;
+	int i;
+
+	seq_printf(m, "%13s  %s\n", "Time (us)", "Stage");
+
+	for (i = 0, rec = drvdata->records; i < hdr->count; i++, rec++) {
+		if (rec->start_us)
+			seq_printf(m, "%13lu  %s\n", rec->time_us, rec->name);
+	}
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(accumulated_time);
+
+static void bootstage_debugfs_init(struct device *dev, struct bootstage_drvdata *drvdata)
+{
+	drvdata->debugfs_dir = debugfs_create_dir(dev_name(dev), bootstage_debugfs_dir);
+	if (IS_ERR(drvdata->debugfs_dir))
+		return;
+
+	debugfs_create_file("stages", 0444, drvdata->debugfs_dir, drvdata, &stages_fops);
+	debugfs_create_file("accumulated_time", 0444, drvdata->debugfs_dir, drvdata,
+			    &accumulated_time_fops);
+}
+
+static void bootstage_debugfs_exit(struct device *dev, struct bootstage_drvdata *drvdata)
+{
+	debugfs_remove_recursive(drvdata->debugfs_dir);
+}
+
+static ssize_t start_time_us_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct bootstage_drvdata *drvdata = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%u\n", drvdata->start_time_us);
+}
+static DEVICE_ATTR_RO(start_time_us);
+
+static ssize_t end_time_us_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct bootstage_drvdata *drvdata = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%u\n", drvdata->end_time_us);
+}
+static DEVICE_ATTR_RO(end_time_us);
+
+static struct attribute *bootstage_attrs[] = {
+	&dev_attr_start_time_us.attr,
+	&dev_attr_end_time_us.attr,
+	NULL,
+};
+
+static const struct attribute_group bootstage_attr_group = {
+	.attrs = bootstage_attrs,
+};
+
+static int bootstage_parse(struct device *dev, struct bootstage_drvdata *drvdata,
+			   resource_size_t size)
+{
+	const char *str_ptr = (const char *)(drvdata->records + drvdata->hdr->count);
+	const resource_size_t calc_size = (resource_size_t)((void *)str_ptr - (void *)drvdata->hdr);
+	struct bootstage_record *rec;
+	u32 r;
+
+	// Sanity checks on bootstage header
+	if (drvdata->hdr->magic != BOOTSTAGE_MAGIC) {
+		dev_err(dev, "wrong bootstage magic number %08Xh\n", drvdata->hdr->magic);
+		return -EINVAL;
+	} else if (drvdata->hdr->version > BOOTSTAGE_MAX_VERSION) {
+		dev_err(dev, "bootstage version %u not supported\n", drvdata->hdr->version);
+		return -EOPNOTSUPP;
+	} else if (drvdata->hdr->size == 0) {
+		dev_err(dev, "invalid bootstage stash (declared size is zero)\n");
+		return -EINVAL;
+	} else if (drvdata->hdr->size > size) {
+		dev_err(dev, "invalid declared stash size %u (expected: <= %llu)\n",
+			drvdata->hdr->size, size);
+		return -EINVAL;
+	} else if (calc_size > size) {
+		dev_err(dev, "invalid calculated stash size %llu (expected: <= %llu)\n",
+			calc_size, size);
+		return -EINVAL;
+	} else if (drvdata->hdr->count == 0) {
+		dev_info(dev, "bootstage stash has no records\n");
+		return 0;
+	}
+
+	// Set start time to invalid
+	drvdata->start_time_us = 0xFFFFFFFF;
+
+	// Associate names to records, which are placed at the end of the record area
+	for (r = 0, rec = drvdata->records; r < drvdata->hdr->count; r++, rec++) {
+		// Save minimum time, will be used as bootloader enter time
+		if (rec->start_us < drvdata->start_time_us)
+			drvdata->start_time_us = rec->time_us;
+
+		// Save maximum time, will be used as bootloader exit time
+		if (rec->time_us > drvdata->end_time_us)
+			drvdata->end_time_us = rec->time_us;
+
+		if (str_ptr > ((const char *)drvdata->hdr + size)) {
+			dev_err(dev, "name for record %u is corrupted\n", r);
+			return -ENODATA;
+		}
+
+		rec->name = str_ptr;
+		str_ptr += strlen(rec->name) + 1;
+	}
+
+	return 0;
+}
+
+static int bootstage_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct reserved_mem *rmem;
+	struct bootstage_drvdata *drvdata;
+	int ret;
+
+	rmem = of_reserved_mem_lookup(dev->of_node);
+	if (!rmem) {
+		dev_err(dev, "failed to lookup reserved memory\n");
+		return -EINVAL;
+	}
+
+	if (!rmem->size || (rmem->size > ULONG_MAX) ||
+	    (rmem->size < sizeof(struct bootstage_hdr))) {
+		dev_err(dev, "invalid memory region size\n");
+		return -EINVAL;
+	}
+
+	if (!PAGE_ALIGNED(rmem->base) || !PAGE_ALIGNED(rmem->size)) {
+		dev_err(dev, "memory region must be page-aligned\n");
+		return -EINVAL;
+	}
+
+	drvdata = devm_kmalloc(dev, sizeof(*drvdata), GFP_KERNEL);
+	if (!drvdata)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, drvdata);
+
+	drvdata->hdr = devm_memremap(dev, rmem->base, rmem->size, MEMREMAP_WB);
+	if (IS_ERR(drvdata->hdr)) {
+		dev_err(dev, "failed to remap bootstage region\n");
+		return PTR_ERR(drvdata->hdr);
+	}
+
+	drvdata->records =
+		(struct bootstage_record *)((void *)drvdata->hdr + sizeof(struct bootstage_hdr));
+
+	ret = bootstage_parse(dev, drvdata, rmem->size);
+	if (ret)
+		return ret;
+
+	ret = sysfs_create_group(&pdev->dev.kobj, &bootstage_attr_group);
+	if (ret) {
+		dev_err(dev, "failed to create sysfs group\n");
+		return ret;
+	}
+
+	bootstage_debugfs_init(dev, drvdata);
+
+	return 0;
+}
+
+static void bootstage_remove(struct platform_device *pdev)
+{
+	struct bootstage_drvdata *drvdata = platform_get_drvdata(pdev);
+
+	bootstage_debugfs_exit(&pdev->dev, drvdata);
+	sysfs_remove_group(&pdev->dev.kobj, &bootstage_attr_group);
+}
+
+static const struct of_device_id bootstage_of_match[] = {
+	{ .compatible = "bootstage" },
+	{},
+};
+
+static struct platform_driver bootstage_driver = {
+	.probe = bootstage_probe,
+	.remove = bootstage_remove,
+	.driver = {
+		.name = "bootstage",
+		.of_match_table = bootstage_of_match,
+	},
+};
+
+static int __init bootstage_init(void)
+{
+	bootstage_debugfs_dir = debugfs_create_dir(bootstage_driver.driver.name, NULL);
+	return platform_driver_register(&bootstage_driver);
+}
+arch_initcall(bootstage_init);
+
+static void __exit bootstage_exit(void)
+{
+	debugfs_remove_recursive(bootstage_debugfs_dir);
+	platform_driver_unregister(&bootstage_driver);
+}
+module_exit(bootstage_exit)
+
+MODULE_DESCRIPTION("Driver for Bootstage stash.");
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Francesco Valla <francesco@valla.it>");
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index f77cb19973a5..e19b04733584 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -496,6 +496,7 @@ static const struct of_device_id reserved_mem_matches[] = {
 	{ .compatible = "ramoops" },
 	{ .compatible = "nvmem-rmem" },
 	{ .compatible = "google,open-dice" },
+	{ .compatible = "bootstage" },
 	{}
 };
 
-- 
2.49.0

Re: [PATCH 1/1] drivers: misc: add driver for bootstage stash

[Krzysztof Kozlowski]

On 23/05/2025 00:42, Francesco Valla wrote:
> Add support for bootstage stash areas containing boot time data
> created by some bootloader (e.g. U-Boot). The driver provides generic
> time information through sysfs and platform-specific one through
> debugfs.
> 
> Signed-off-by: Francesco Valla <francesco@valla.it>

Your Cc list is so incomplete I really do not understand which project
you target and this popped up in my lei filters. If this is not for
Linux kernel, please ignore the rest.

If this is for Linux kernel then:

Please run scripts/checkpatch.pl on the patches and fix reported
warnings. After that, run also 'scripts/checkpatch.pl --strict' on the
patches and (probably) fix more warnings. Some warnings can be ignored,
especially from --strict run, but the code here looks like it needs a
fix. Feel free to get in touch if the warning is not clear.


Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline) or work on fork of kernel
(don't, instead use mainline). Just use b4 and everything should be
fine, although remember about `b4 prep --auto-to-cc` if you added new
patches to the patchset.

You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling. Performing review on untested code might be
a waste of time.

Please kindly resend and include all necessary To/Cc entries.


Best regards,
Krzysztof

Re: [PATCH 1/1] drivers: misc: add driver for bootstage stash

[Rob Landley]

On 5/23/25 01:29, Krzysztof Kozlowski wrote:
> On 23/05/2025 00:42, Francesco Valla wrote:
>> Add support for bootstage stash areas containing boot time data
>> created by some bootloader (e.g. U-Boot). The driver provides generic
>> time information through sysfs and platform-specific one through
>> debugfs.
>>
>> Signed-off-by: Francesco Valla <francesco@valla.it>
> 
> Your Cc list is so incomplete I really do not understand which project
> you target and this popped up in my lei filters. If this is not for
> Linux kernel, please ignore the rest.

The discussion is happening on the linux-embedded list you cc'd in your 
reply.

https://www.spinics.net/lists/linux-embedded/msg04429.html

https://www.spinics.net/lists/linux-embedded/msg04435.html

Rob

Re: [PATCH 1/1] drivers: misc: add driver for bootstage stash

[Krzysztof Kozlowski]

On 23/05/2025 21:34, Rob Landley wrote:
> On 5/23/25 01:29, Krzysztof Kozlowski wrote:
>> On 23/05/2025 00:42, Francesco Valla wrote:
>>> Add support for bootstage stash areas containing boot time data
>>> created by some bootloader (e.g. U-Boot). The driver provides generic
>>> time information through sysfs and platform-specific one through
>>> debugfs.
>>>
>>> Signed-off-by: Francesco Valla <francesco@valla.it>
>>
>> Your Cc list is so incomplete I really do not understand which project
>> you target and this popped up in my lei filters. If this is not for
>> Linux kernel, please ignore the rest.
> 
> The discussion is happening on the linux-embedded list you cc'd in your 
> reply.
> 
> https://www.spinics.net/lists/linux-embedded/msg04429.html
> 
> https://www.spinics.net/lists/linux-embedded/msg04435.html

No, that's not the place where we discuss patches. Use
get_maintainers.pl to find the right place.

Best regards,
Krzysztof

Re: [PATCH 1/1] drivers: misc: add driver for bootstage stash

[Francesco Valla]

Hello Krzysztof,

On Friday, 23 May 2025 at 08:29:28 Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On 23/05/2025 00:42, Francesco Valla wrote:
> > Add support for bootstage stash areas containing boot time data
> > created by some bootloader (e.g. U-Boot). The driver provides generic
> > time information through sysfs and platform-specific one through
> > debugfs.
> > 
> > Signed-off-by: Francesco Valla <francesco@valla.it>
> 
> Your Cc list is so incomplete I really do not understand which project
> you target and this popped up in my lei filters. If this is not for
> Linux kernel, please ignore the rest.
> 

Yes, target is the Linux kernel, the patch contains a mock-up driver
generated by a discussion [1] of the Boot Time SIG held on last Tuesday.
I should have marked it as proof-of-concept rather than RFC, probably.

> If this is for Linux kernel then:
> 
> Please run scripts/checkpatch.pl on the patches and fix reported
> warnings. After that, run also 'scripts/checkpatch.pl --strict' on the
> patches and (probably) fix more warnings. Some warnings can be ignored,
> especially from --strict run, but the code here looks like it needs a
> fix. Feel free to get in touch if the warning is not clear.
> 

The only warning, if we ignore three on unnecessary parentheses raised
by --strict, is the one about the DT bindings that need to be submitted
separately. That was expected and somewhat specified in the cover letter,
but I honestly did not think of the automated tooling.

> 
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC. It might happen, that command when run on an older
> kernel, gives you outdated entries. Therefore please be sure you base
> your patches on recent Linux kernel.
> 
> Tools like b4 or scripts/get_maintainer.pl provide you proper list of
> people, so fix your workflow. Tools might also fail if you work on some
> ancient tree (don't, instead use mainline) or work on fork of kernel
> (don't, instead use mainline). Just use b4 and everything should be
> fine, although remember about `b4 prep --auto-to-cc` if you added new
> patches to the patchset.
> 

I used scripts/get_maintainer.pl in the past but I wasn't aware of this
use of b4. I'll check it out.

> You missed at least devicetree list (maybe more), so this won't be
> tested by automated tooling. Performing review on untested code might be
> a waste of time.
> 
> Please kindly resend and include all necessary To/Cc entries.
> 


I did not want to unnecessary bother people that might not be interested,
as the current form is not intended for mainline. I'll however be more
scrupulous in the future.

> 
> Best regards,
> Krzysztof
> 
> 

Thank you for the guidance!

Regards,
Francesco

[1] https://docs.google.com/document/d/1XAufoTT6VVJOTMzKMoz8SyOss-JA9H4J1_yVXQq5mN0/edit?tab=t.0

Re: [PATCH 1/1] drivers: misc: add driver for bootstage stash

[Krzysztof Kozlowski]

On 23/05/2025 21:43, Francesco Valla wrote:
>> tested by automated tooling. Performing review on untested code might be
>> a waste of time.
>>
>> Please kindly resend and include all necessary To/Cc entries.
>>
> 
> 
> I did not want to unnecessary bother people that might not be interested,
> as the current form is not intended for mainline. I'll however be more
> scrupulous in the future.

Then mark your patches clearly like that. Usually this is RFC with
explanation what you mean by that in cover letter.


Best regards,
Krzysztof

RE: [PATCH 1/1] drivers: misc: add driver for bootstage stash

[Bird, Tim]

Francesco,

Thanks for this prototype!!

> -----Original Message-----
> From: Francesco Valla <francesco@valla.it>
> Add support for bootstage stash areas containing boot time data
> created by some bootloader (e.g. U-Boot). The driver provides generic
> time information through sysfs and platform-specific one through
> debugfs.
> 
> Signed-off-by: Francesco Valla <francesco@valla.it>
> ---
>  .../bindings/reserved-memory/bootstage.yaml   |  44 +++
>  Documentation/misc-devices/bootstage.rst      |  53 ++++
>  Documentation/misc-devices/index.rst          |   1 +
>  MAINTAINERS                                   |   7 +
>  drivers/misc/Kconfig                          |  10 +
>  drivers/misc/Makefile                         |   1 +
>  drivers/misc/bootstage.c                      | 292 ++++++++++++++++++
>  drivers/of/platform.c                         |   1 +
>  8 files changed, 409 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reserved-memory/bootstage.yaml
>  create mode 100644 Documentation/misc-devices/bootstage.rst
>  create mode 100644 drivers/misc/bootstage.c
> 
> diff --git a/Documentation/devicetree/bindings/reserved-memory/bootstage.yaml b/Documentation/devicetree/bindings/reserved-
> memory/bootstage.yaml
> new file mode 100644
> index 000000000000..e71d85c5c2ce
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reserved-memory/bootstage.yaml
> @@ -0,0 +1,44 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/reserved-memory/bootstage.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Bootstage stash
> +
> +description: |
> +  This binding represents a reserved memory region containing bootstage stash
> +  data generated by a previous bootloader stage.
> +
> +maintainers:
> +  - Francesco Valla <francesco@valla.it>
> +
> +allOf:
> +  - $ref: reserved-memory.yaml
> +
> +properties:
> +  compatible:
> +    const: bootstage
> +
> +  reg:
> +    description: page-aligned region of memory containing bootstage stash data
> +
> +required:
> +  - compatible
> +  - reg
> +  - no-map
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    reserved-memory {
> +        #address-cells = <2>;
> +        #size-cells = <1>;
> +
> +        bootstage: bootstage@12340000 {
> +            compatible = "bootstage";
> +            reg = <0x00 0x12340000 0x2000>;
> +            no-map;
> +        };
> +    };
> diff --git a/Documentation/misc-devices/bootstage.rst b/Documentation/misc-devices/bootstage.rst
> new file mode 100644
> index 000000000000..2e1bbd31aab8
> --- /dev/null
> +++ b/Documentation/misc-devices/bootstage.rst
> @@ -0,0 +1,53 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +================
> +Bootstage driver
> +================
> +
> +The bootstage driver exports interfaces to read from a bootstage stash area
> +saved by a bootloader (e.g.: U-Boot) that ran before the Linux kernel.
> +
> +Two kind of interfaces are exported:
> +
> +- a sysfs interface for bootloader- and platform-agnostic data
> +- a debugfs interface for bootloader- and platform-specific data
> +
> +
> +The sysfs interface
> +-------------------
> +
> +Following sysfs attributes can be found at /sys/devices/platform/<device-name>/:
> +
> +- start_time_us: bootloader start time in microseconds
> +- end_time_us: bootloader end time in microseconds
> +
> +
> +The debugfs interface
> +---------------------
> +
> +Following debugfs interfaces can be found at
> +/sys/kernel/debug/bootstage/<device-name>/:
> +
> +- stages: details on staged bootloader stages, with start time and duration.
> +  Example output::
> +
> +    Mark (us)  Elapsed (us)  Stage
> +            0             0  reset
> +       183689        183689  SPL
> +       489247        305558  end phase
> +       506987         17740  board_init_f
> +      1257880        750893  board_init_r
> +      1622303        364423  eth_common_init
> +      1888033        265730  eth_initialize
> +      1893077          5044  main_loop
> +      4204282       2311205  cli_loop
> +
> +- accumulated_time: time accumulated during certain bootloader stages.
> +  Example output::
> +
> +    Time (us)  Stage
> +         4902  dm_spl
> +       322719  dm_f
> +         9527  dm_r
> +
> +The number and type of staged stages are bootloader- and platform-specific.

I think it would be good to indicate in this document the required U-Boot CONFIG
settings for bootstage to be enabled.  We don't need to replicate detailed U-Boot
documentation, but maybe just refer to the required configs.  (e.g. CONFIG_BOOTSTAGE,
CONFIG_BOOTSTAGE_STASH, etc.)

Note: I looked in the U-boot documentation for documentation about Bootstage,
and I couldn't find much.  There might be an opportunity to add some documentation
there as well.

> diff --git a/Documentation/misc-devices/index.rst b/Documentation/misc-devices/index.rst
> index 8c5b226d8313..c5ebb3d44505 100644
> --- a/Documentation/misc-devices/index.rst
> +++ b/Documentation/misc-devices/index.rst
> @@ -28,3 +28,4 @@ fit into other categories.
>     tps6594-pfsm
>     uacce
>     xilinx_sdfec
> +   bootstage
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f21f1dabb5fe..0bdecd07023a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4203,6 +4203,13 @@ F:	Documentation/ABI/stable/sysfs-class-bluetooth
>  F:	include/net/bluetooth/
>  F:	net/bluetooth/
> 
> +BOOTSTAGE DRIVER
> +M:	Francesco Valla <francesco@valla.it>
> +L:	linux-embedded@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/reserved-memory/bootstage.yaml
> +F:	drivers/misc/bootstage.c
> +
>  BONDING DRIVER
>  M:	Jay Vosburgh <jv@jvosburgh.net>
>  L:	netdev@vger.kernel.org
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 6b37d61150ee..97cdfa241c0c 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -632,6 +632,16 @@ config MCHP_LAN966X_PCI
>  	    - lan966x-miim (MDIO_MSCC_MIIM)
>  	    - lan966x-switch (LAN966X_SWITCH)
> 
> +config BOOTSTAGE
> +	tristate "Bootstage stash support"
> +	depends on OF_RESERVED_MEM
> +	help
> +	  This enables the support for a bootstage stash.
> +
> +	  A bootstage stash can be created by some bootloaders (e.g.: U-Boot) to
> +	  store information on its boot timings. This driver provides access to
> +	  these information through sysfs and debugsfs interfaces.
> +
>  source "drivers/misc/c2port/Kconfig"
>  source "drivers/misc/eeprom/Kconfig"
>  source "drivers/misc/cb710/Kconfig"
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index d6c917229c45..3562c1bf701f 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -74,3 +74,4 @@ lan966x-pci-objs		:= lan966x_pci.o
>  lan966x-pci-objs		+= lan966x_pci.dtbo.o
>  obj-$(CONFIG_MCHP_LAN966X_PCI)	+= lan966x-pci.o
>  obj-y				+= keba/
> +obj-$(CONFIG_BOOTSTAGE)		+= bootstage.o
> diff --git a/drivers/misc/bootstage.c b/drivers/misc/bootstage.c
> new file mode 100644
> index 000000000000..a106410a56ee
> --- /dev/null
> +++ b/drivers/misc/bootstage.c
> @@ -0,0 +1,292 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2025 - Francesco Valla <francesco@valla.it>
> + *
> + * Driver for bootstage stash.
> + *
> + * This driver exposes bootstage stash generated by bootloader and/or firmware
> + * stages that run before the Linux kernel.
> + *
> + */
> +
> +#include <linux/debugfs.h>
> +#include <linux/io.h>
> +#include <linux/mm.h>
> +#include <linux/module.h>
> +#include <linux/of_reserved_mem.h>
> +#include <linux/platform_device.h>
> +#include <linux/seq_file.h>
> +
> +#define BOOTSTAGE_MAGIC		0xb00757a3
> +#define BOOTSTAGE_MAX_VERSION	0
> +
> +enum bootstage_id {
> +	BOOTSTAGE_ID_START = 0,
> +};
> +
> +enum bootstage_flags {
> +	BOOTSTAGEF_ERROR	= 1 << 0,	/* Error record */
> +	BOOTSTAGEF_ALLOC	= 1 << 1,	/* Allocate an id */
> +};
> +
> +struct bootstage_record {
> +	ulong time_us;
> +	u32 start_us;
> +	const char *name;
> +	int flags;
> +	enum bootstage_id id;
> +};
> +
> +struct bootstage_hdr {
> +	u32 version;		/* Boostage stash version */
> +	u32 count;		/* Number of records */
> +	u32 size;		/* Total data size (non-zero if valid) */
> +	u32 magic;		/* Magic number */
> +	u32 next_id;		/* Next ID to use for bootstage */
> +};
> +
> +struct bootstage_drvdata {
> +	struct bootstage_hdr *hdr;
> +	struct bootstage_record *records;
> +
> +	u32 start_time_us;
> +	u32 end_time_us;
> +
> +	struct dentry *debugfs_dir;
> +};
> +
> +static struct dentry *bootstage_debugfs_dir;
> +
> +static int stages_show(struct seq_file *m, void *d)
> +{
> +	struct bootstage_drvdata *drvdata = m->private;
> +	struct bootstage_hdr *hdr = drvdata->hdr;
> +	struct bootstage_record *rec;
> +	u32 prev = 0;
> +	int i;
> +
> +	seq_printf(m, "%13s %13s  %s\n", "Mark (us)", "Elapsed (us)", "Stage");
> +
> +	for (i = 0, rec = drvdata->records; i < hdr->count; i++, rec++) {
> +		if ((rec->id && !rec->start_us) || (i == 0)) {
> +			if (prev > rec->time_us)
> +				prev = 0;
> +			seq_printf(m, "%13lu %13lu  %s\n", rec->time_us,
> +				   rec->time_us - prev, rec->name);
> +			prev = rec->time_us;
> +		}
> +	}
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(stages);
> +
> +static int accumulated_time_show(struct seq_file *m, void *d)
> +{
> +	struct bootstage_drvdata *drvdata = m->private;
> +	struct bootstage_hdr *hdr = drvdata->hdr;
> +	struct bootstage_record *rec;
> +	int i;
> +
> +	seq_printf(m, "%13s  %s\n", "Time (us)", "Stage");
> +
> +	for (i = 0, rec = drvdata->records; i < hdr->count; i++, rec++) {
> +		if (rec->start_us)
> +			seq_printf(m, "%13lu  %s\n", rec->time_us, rec->name);
> +	}
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(accumulated_time);
> +
> +static void bootstage_debugfs_init(struct device *dev, struct bootstage_drvdata *drvdata)
> +{
> +	drvdata->debugfs_dir = debugfs_create_dir(dev_name(dev), bootstage_debugfs_dir);
> +	if (IS_ERR(drvdata->debugfs_dir))
> +		return;
> +
> +	debugfs_create_file("stages", 0444, drvdata->debugfs_dir, drvdata, &stages_fops);
> +	debugfs_create_file("accumulated_time", 0444, drvdata->debugfs_dir, drvdata,
> +			    &accumulated_time_fops);
> +}
> +
> +static void bootstage_debugfs_exit(struct device *dev, struct bootstage_drvdata *drvdata)
> +{
> +	debugfs_remove_recursive(drvdata->debugfs_dir);
> +}
> +
> +static ssize_t start_time_us_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct bootstage_drvdata *drvdata = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%u\n", drvdata->start_time_us);
> +}
> +static DEVICE_ATTR_RO(start_time_us);
> +
> +static ssize_t end_time_us_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct bootstage_drvdata *drvdata = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%u\n", drvdata->end_time_us);
> +}
> +static DEVICE_ATTR_RO(end_time_us);
> +
> +static struct attribute *bootstage_attrs[] = {
> +	&dev_attr_start_time_us.attr,
> +	&dev_attr_end_time_us.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group bootstage_attr_group = {
> +	.attrs = bootstage_attrs,
> +};
> +
> +static int bootstage_parse(struct device *dev, struct bootstage_drvdata *drvdata,
> +			   resource_size_t size)
> +{
> +	const char *str_ptr = (const char *)(drvdata->records + drvdata->hdr->count);
> +	const resource_size_t calc_size = (resource_size_t)((void *)str_ptr - (void *)drvdata->hdr);
> +	struct bootstage_record *rec;
> +	u32 r;
> +
> +	// Sanity checks on bootstage header
> +	if (drvdata->hdr->magic != BOOTSTAGE_MAGIC) {
> +		dev_err(dev, "wrong bootstage magic number %08Xh\n", drvdata->hdr->magic);
> +		return -EINVAL;
> +	} else if (drvdata->hdr->version > BOOTSTAGE_MAX_VERSION) {
> +		dev_err(dev, "bootstage version %u not supported\n", drvdata->hdr->version);
> +		return -EOPNOTSUPP;
> +	} else if (drvdata->hdr->size == 0) {
> +		dev_err(dev, "invalid bootstage stash (declared size is zero)\n");
> +		return -EINVAL;
> +	} else if (drvdata->hdr->size > size) {
> +		dev_err(dev, "invalid declared stash size %u (expected: <= %llu)\n",
> +			drvdata->hdr->size, size);
> +		return -EINVAL;
> +	} else if (calc_size > size) {
> +		dev_err(dev, "invalid calculated stash size %llu (expected: <= %llu)\n",
> +			calc_size, size);
> +		return -EINVAL;
> +	} else if (drvdata->hdr->count == 0) {
> +		dev_info(dev, "bootstage stash has no records\n");
> +		return 0;
> +	}
> +
> +	// Set start time to invalid
> +	drvdata->start_time_us = 0xFFFFFFFF;
> +
> +	// Associate names to records, which are placed at the end of the record area
> +	for (r = 0, rec = drvdata->records; r < drvdata->hdr->count; r++, rec++) {
> +		// Save minimum time, will be used as bootloader enter time
> +		if (rec->start_us < drvdata->start_time_us)
> +			drvdata->start_time_us = rec->time_us;
> +
> +		// Save maximum time, will be used as bootloader exit time
> +		if (rec->time_us > drvdata->end_time_us)
> +			drvdata->end_time_us = rec->time_us;
> +
> +		if (str_ptr > ((const char *)drvdata->hdr + size)) {
> +			dev_err(dev, "name for record %u is corrupted\n", r);
> +			return -ENODATA;
> +		}
> +
> +		rec->name = str_ptr;
> +		str_ptr += strlen(rec->name) + 1;
> +	}
> +
> +	return 0;
> +}
> +
> +static int bootstage_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct reserved_mem *rmem;
> +	struct bootstage_drvdata *drvdata;
> +	int ret;
> +
> +	rmem = of_reserved_mem_lookup(dev->of_node);
> +	if (!rmem) {
> +		dev_err(dev, "failed to lookup reserved memory\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!rmem->size || (rmem->size > ULONG_MAX) ||
> +	    (rmem->size < sizeof(struct bootstage_hdr))) {
> +		dev_err(dev, "invalid memory region size\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!PAGE_ALIGNED(rmem->base) || !PAGE_ALIGNED(rmem->size)) {
> +		dev_err(dev, "memory region must be page-aligned\n");
> +		return -EINVAL;
> +	}
> +
> +	drvdata = devm_kmalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> +	if (!drvdata)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, drvdata);
> +
> +	drvdata->hdr = devm_memremap(dev, rmem->base, rmem->size, MEMREMAP_WB);
> +	if (IS_ERR(drvdata->hdr)) {
> +		dev_err(dev, "failed to remap bootstage region\n");
> +		return PTR_ERR(drvdata->hdr);
> +	}
> +
> +	drvdata->records =
> +		(struct bootstage_record *)((void *)drvdata->hdr + sizeof(struct bootstage_hdr));
> +
> +	ret = bootstage_parse(dev, drvdata, rmem->size);
> +	if (ret)
> +		return ret;
> +
> +	ret = sysfs_create_group(&pdev->dev.kobj, &bootstage_attr_group);
> +	if (ret) {
> +		dev_err(dev, "failed to create sysfs group\n");
> +		return ret;
> +	}
> +
> +	bootstage_debugfs_init(dev, drvdata);
> +
> +	return 0;
> +}
> +
> +static void bootstage_remove(struct platform_device *pdev)
> +{
> +	struct bootstage_drvdata *drvdata = platform_get_drvdata(pdev);
> +
> +	bootstage_debugfs_exit(&pdev->dev, drvdata);
> +	sysfs_remove_group(&pdev->dev.kobj, &bootstage_attr_group);
> +}
> +
> +static const struct of_device_id bootstage_of_match[] = {
> +	{ .compatible = "bootstage" },
> +	{},
> +};
> +
> +static struct platform_driver bootstage_driver = {
> +	.probe = bootstage_probe,
> +	.remove = bootstage_remove,
> +	.driver = {
> +		.name = "bootstage",
> +		.of_match_table = bootstage_of_match,
> +	},
> +};
> +
> +static int __init bootstage_init(void)
> +{
> +	bootstage_debugfs_dir = debugfs_create_dir(bootstage_driver.driver.name, NULL);
> +	return platform_driver_register(&bootstage_driver);
> +}
> +arch_initcall(bootstage_init);
> +
> +static void __exit bootstage_exit(void)
> +{
> +	debugfs_remove_recursive(bootstage_debugfs_dir);
> +	platform_driver_unregister(&bootstage_driver);
> +}
> +module_exit(bootstage_exit)
> +
> +MODULE_DESCRIPTION("Driver for Bootstage stash.");
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Francesco Valla <francesco@valla.it>");

OK - I have to admit now that I am confused.  I haven't actually run this code,
or the U-boot code, but in examining the code at:
https://elixir.bootlin.com/u-boot/v2025.04/source/common/bootstage.c#L269
it seems like U-boot adds each individual record as a device tree property.

If that code (add_bootstage_devicetree) is putting info in the device tree as properties,
why is this kernel code reading bootstage stash entries as binary structured data
from the bootstage stash memory.

I see the 'bootstage_stash()' function in that same U-boot code, which presumably
is storing U-boot entries into the stash area, to be passed to the kernel.
Is there code in U-boot to add the stash element address property to the flattened
device tree that it passes to the kernel, or is this just manually configured
in 1) the U-boot config, and 2) the kernel device tree file?

Or maybe, the bootstage_stash() in the U-boot source code is there to copy
records from a secondary bootloader into the primary U-boot bootstage
stash?  I can't figure out what's going on.

Are these two things related or completely separate?  (That is, if the flattened
device tree has a bootstage node with sub-notes having 'name' and either 'accum' or 'mark'
records with a time_us element, is that completely different from the bootstage stash
memory area?

Just trying to get a handle on what's going on here and how the data is being transferred.

> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index f77cb19973a5..e19b04733584 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -496,6 +496,7 @@ static const struct of_device_id reserved_mem_matches[] = {
>  	{ .compatible = "ramoops" },
>  	{ .compatible = "nvmem-rmem" },
>  	{ .compatible = "google,open-dice" },
> +	{ .compatible = "bootstage" },
>  	{}
>  };
> 
> --
> 2.49.0
> 

Re: [PATCH 1/1] drivers: misc: add driver for bootstage stash

[kernel test robot]

Hi Francesco,

kernel test robot noticed the following build warnings:

[auto build test WARNING on char-misc/char-misc-linus]
[also build test WARNING on robh/for-next linus/master v6.15-rc7]
[cannot apply to char-misc/char-misc-testing char-misc/char-misc-next next-20250523]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Francesco-Valla/drivers-misc-add-driver-for-bootstage-stash/20250523-122757
base:   char-misc/char-misc-linus
patch link:    https://lore.kernel.org/r/20250522224223.358881-3-francesco%40valla.it
patch subject: [PATCH 1/1] drivers: misc: add driver for bootstage stash
config: hexagon-allyesconfig (https://download.01.org/0day-ci/archive/20250524/202505241547.wVTtLz71-lkp@intel.com/config)
compiler: clang version 21.0.0git (https://github.com/llvm/llvm-project f819f46284f2a79790038e1f6649172789734ae8)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250524/202505241547.wVTtLz71-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202505241547.wVTtLz71-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/misc/bootstage.c:164:24: warning: format specifies type 'unsigned long long' but the argument has type 'resource_size_t' (aka 'unsigned int') [-Wformat]
     163 |                 dev_err(dev, "invalid declared stash size %u (expected: <= %llu)\n",
         |                                                                            ~~~~
         |                                                                            %u
     164 |                         drvdata->hdr->size, size);
         |                                             ^~~~
   include/linux/dev_printk.h:154:65: note: expanded from macro 'dev_err'
     154 |         dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                                                                ~~~     ^~~~~~~~~~~
   include/linux/dev_printk.h:110:23: note: expanded from macro 'dev_printk_index_wrap'
     110 |                 _p_func(dev, fmt, ##__VA_ARGS__);                       \
         |                              ~~~    ^~~~~~~~~~~
   drivers/misc/bootstage.c:168:4: warning: format specifies type 'unsigned long long' but the argument has type 'resource_size_t' (aka 'unsigned int') [-Wformat]
     167 |                 dev_err(dev, "invalid calculated stash size %llu (expected: <= %llu)\n",
         |                                                             ~~~~
         |                                                             %u
     168 |                         calc_size, size);
         |                         ^~~~~~~~~
   include/linux/dev_printk.h:154:65: note: expanded from macro 'dev_err'
     154 |         dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                                                                ~~~     ^~~~~~~~~~~
   include/linux/dev_printk.h:110:23: note: expanded from macro 'dev_printk_index_wrap'
     110 |                 _p_func(dev, fmt, ##__VA_ARGS__);                       \
         |                              ~~~    ^~~~~~~~~~~
   drivers/misc/bootstage.c:168:15: warning: format specifies type 'unsigned long long' but the argument has type 'resource_size_t' (aka 'unsigned int') [-Wformat]
     167 |                 dev_err(dev, "invalid calculated stash size %llu (expected: <= %llu)\n",
         |                                                                                ~~~~
         |                                                                                %u
     168 |                         calc_size, size);
         |                                    ^~~~
   include/linux/dev_printk.h:154:65: note: expanded from macro 'dev_err'
     154 |         dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                                                                ~~~     ^~~~~~~~~~~
   include/linux/dev_printk.h:110:23: note: expanded from macro 'dev_printk_index_wrap'
     110 |                 _p_func(dev, fmt, ##__VA_ARGS__);                       \
         |                              ~~~    ^~~~~~~~~~~
   3 warnings generated.


vim +164 drivers/misc/bootstage.c

   143	
   144	static int bootstage_parse(struct device *dev, struct bootstage_drvdata *drvdata,
   145				   resource_size_t size)
   146	{
   147		const char *str_ptr = (const char *)(drvdata->records + drvdata->hdr->count);
   148		const resource_size_t calc_size = (resource_size_t)((void *)str_ptr - (void *)drvdata->hdr);
   149		struct bootstage_record *rec;
   150		u32 r;
   151	
   152		// Sanity checks on bootstage header
   153		if (drvdata->hdr->magic != BOOTSTAGE_MAGIC) {
   154			dev_err(dev, "wrong bootstage magic number %08Xh\n", drvdata->hdr->magic);
   155			return -EINVAL;
   156		} else if (drvdata->hdr->version > BOOTSTAGE_MAX_VERSION) {
   157			dev_err(dev, "bootstage version %u not supported\n", drvdata->hdr->version);
   158			return -EOPNOTSUPP;
   159		} else if (drvdata->hdr->size == 0) {
   160			dev_err(dev, "invalid bootstage stash (declared size is zero)\n");
   161			return -EINVAL;
   162		} else if (drvdata->hdr->size > size) {
   163			dev_err(dev, "invalid declared stash size %u (expected: <= %llu)\n",
 > 164				drvdata->hdr->size, size);
   165			return -EINVAL;
   166		} else if (calc_size > size) {
   167			dev_err(dev, "invalid calculated stash size %llu (expected: <= %llu)\n",
   168				calc_size, size);
   169			return -EINVAL;
   170		} else if (drvdata->hdr->count == 0) {
   171			dev_info(dev, "bootstage stash has no records\n");
   172			return 0;
   173		}
   174	
   175		// Set start time to invalid
   176		drvdata->start_time_us = 0xFFFFFFFF;
   177	
   178		// Associate names to records, which are placed at the end of the record area
   179		for (r = 0, rec = drvdata->records; r < drvdata->hdr->count; r++, rec++) {
   180			// Save minimum time, will be used as bootloader enter time
   181			if (rec->start_us < drvdata->start_time_us)
   182				drvdata->start_time_us = rec->time_us;
   183	
   184			// Save maximum time, will be used as bootloader exit time
   185			if (rec->time_us > drvdata->end_time_us)
   186				drvdata->end_time_us = rec->time_us;
   187	
   188			if (str_ptr > ((const char *)drvdata->hdr + size)) {
   189				dev_err(dev, "name for record %u is corrupted\n", r);
   190				return -ENODATA;
   191			}
   192	
   193			rec->name = str_ptr;
   194			str_ptr += strlen(rec->name) + 1;
   195		}
   196	
   197		return 0;
   198	}
   199	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [RFC PATCH 0/1] Add driver for bootstage stash

[Geert Uytterhoeven]

Hi Francesco,

On Fri, 23 May 2025 at 02:25, Francesco Valla <francesco@valla.it> wrote:
> after the discussion on the "Unified Boot Log" topic during the latest
> Boot Time SIG special meeting [1], I tried to mock up a driver that
> reads a bootstage stash saved by the U-Boot bootloader in a given memory
> area and exposes the data in a user- and machine- friendly through both
> sysfs and debugfs attributes. Details on the interfaces, as well as
> example output for the debugfs interfaces, can be found on the
> documentation that is part of the patchset.

Thanks for your work!

> To use this driver, a memory area shall be reserved inside the Linux
> kernel devicetree as follows (possibly changing the address and the size
> of the memory area):
>
>     bootstage@a4300000 {
>         compatible = "bootstage";
>         reg = <0 0xa4300000 0 0x1000>;
>         no-map;
>     };
>
> At U-Boot side, following configuration shall then be set:
>
>     CONFIG_BOOTSTAGE=y
>     CONFIG_BOOTSTAGE_STASH_ADDR=0xa4300000
>     CONFIG_BOOTSTAGE_STASH_SIZE=0x1000

I think this can be simplified further, using either of these two options:
  1. If the bootstage@a4300000 node would already be present in the
     DTB used by U-Boot, the two CONFIG_BOOTSTAGE_STASH_*
     options would no longer be needed.
  2. U-Boot could add the bootstage@a4300000 to the DTB that is
     passed to the kernel, just like it already adds/updates the memory
     nodes.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC PATCH 0/1] Add driver for bootstage stash

[Francesco Valla]

Hi Geert,

On Friday, 23 May 2025 at 09:04:48 Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >
> > At U-Boot side, following configuration shall then be set:
> >
> >     CONFIG_BOOTSTAGE=y
> >     CONFIG_BOOTSTAGE_STASH_ADDR=0xa4300000
> >     CONFIG_BOOTSTAGE_STASH_SIZE=0x1000
> 
> I think this can be simplified further, using either of these two options:
>   1. If the bootstage@a4300000 node would already be present in the
>      DTB used by U-Boot, the two CONFIG_BOOTSTAGE_STASH_*
>      options would no longer be needed.
>   2. U-Boot could add the bootstage@a4300000 to the DTB that is
>      passed to the kernel, just like it already adds/updates the memory
>      nodes.
> 

The second option is probably the best one, as it aligns with other
similar usecases (e.g.: OP-TEE node [1]). Once it has been determined that
this driver is the right approach, it should be pretty simple to do.

[1] https://elixir.bootlin.com/u-boot/v2025.04/source/lib/optee/optee.c#L112

> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> 

Regards,

Francesco

Re: [RFC PATCH 0/1] Add driver for bootstage stash

[Federico Giovanardi]

Hello,

The note about the data format also was my initial thought, by just 
copying a C structure we might have issues as soon one party changes it, 
and they might not be perfectly aligned.

To avoid inventing yet-another-data-format I've used msgpack in the past 
for that (the format 
https://github.com/msgpack/msgpack/blob/master/spec.md, not the library  
); because the specs are so simple they can be implemented in a few 
lines, and it's something with a reference. But I don't have a lot of 
experience in upstreaming stuff on the kernel, so I don't know if it 
might cause someone to don't be happy. Anyway, I can contribute the 
implementation if needed.

Something as simple as an array of fixarray will give extensibility with 
only a few bytes of overhead.

Which gets encoded as:

0xdc # lenght 16 bit << array header
      # 0xB << 4 | ( array_size & 0xF) << fixarray header ( 3 elements, 
simplest case)
            # 0xce # time_us
            # 0xce # start_us
            # 0xc << 4 | strlen(name) # name
            /*no flags, no id*/
      # 0xB << 4 | ( array_size & 0xF) << fixarray header ( 5 elements 
bigger case)
            # 0xce # time_us
            # 0xce # start_us
            # 0xc << 4 | strlen(name) # name
            # 0xcc # flags
            # 0xcc # id
      .. repeat ...


Since the goal is to use that in many different contexts, defining the 
fields that we need early is important.

Bye
Federico

> -------------------------
> 
> Da: Francesco Valla <francesco@valla.it>
> Inviato: venerdì 23 maggio 2025 00:42
> A: linux-embedded@vger.kernel.org <linux-embedded@vger.kernel.org>
> Oggetto: [RFC PATCH 0/1] Add driver for bootstage stash
> 
> Questo messaggio proviene da un mittente esterno: fai attenzione ad
> allegati e link.
> 
> Hello,
> 
> after the discussion on the "Unified Boot Log" topic during the latest
> Boot Time SIG special meeting [1], I tried to mock up a driver that
> reads a bootstage stash saved by the U-Boot bootloader in a given
> memory
> area and exposes the data in a user- and machine- friendly through
> both
> sysfs and debugfs attributes. Details on the interfaces, as well as
> example output for the debugfs interfaces, can be found on the
> documentation that is part of the patchset.
> 
> To use this driver, a memory area shall be reserved inside the Linux
> kernel devicetree as follows (possibly changing the address and the
> size
> of the memory area):
> 
>     bootstage@a4300000 {
>         compatible = "bootstage";
>         reg = <0 0xa4300000 0 0x1000>;
>         no-map;
>     };
> 
> At U-Boot side, following configuration shall then be set:
> 
>     CONFIG_BOOTSTAGE=y
>     CONFIG_BOOTSTAGE_STASH_ADDR=0xa4300000
>     CONFIG_BOOTSTAGE_STASH_SIZE=0x1000
> 
> Once booted, the bootstage data can will be found at:
> 
>     - /sys/devices/platform/a4300000.bootstage/
>     - /sys/kernel/debug/bootstage/a4300000.bootstage/
> 
> The device name is purposely part of the sysfs and debugfs paths to
> support multiple bootstage areas, as this _might_ then be used for
> multiple bootstage sources, e.g. bootloaders running on different
> cores inside a SoC with different architectures.
> 
> Note that this is not really meant to be integrated as-is, not only
> because it's a single patch including code, documentation and
> devicetree
> bindings, but also because the bootstage stash format itself may need
> to
> be touched up a bit. In particular, fixed data type should probably be
> evaluated for the bootstage record, in order to increase compatibility
> with different data sources.
> 
> Comments are of course welcome.
> 
> Regards,
> 
> Francesco
> 
> [1]
> https://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flinux-embedded%2FMW5PR13MB5632B8FA3279D77F2F9217BBFD9CA%40MW5PR13MB5632.namprd13.prod.outlook.com%2F&data=05%7C02%7Cfederico.giovanardi%40cnh.com%7C68aacb29d80340fc5d3208dd99904415%7C79310fb0d39b486bb77b25f3e0c82a0e%7C0%7C0%7C638835567100175385%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=Hrd3rjtJ7sLciUzkFymL7y2agCAMZSAKKF7evt20LQU%3D&reserved=0
> [1]
> 
> Francesco Valla (1):
>   drivers: misc: add driver for bootstage stash
> 
>  .../bindings/reserved-memory/bootstage.yaml   |  44 +++
>  Documentation/misc-devices/bootstage.rst      |  53 ++++
>  Documentation/misc-devices/index.rst          |   1 +
>  MAINTAINERS                                   |   7 +
>  drivers/misc/Kconfig                          |  10 +
>  drivers/misc/Makefile                         |   1 +
>  drivers/misc/bootstage.c                      | 292
> ++++++++++++++++++
>  drivers/of/platform.c                         |   1 +
>  8 files changed, 409 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/reserved-memory/bootstage.yaml
>  create mode 100644 Documentation/misc-devices/bootstage.rst
>  create mode 100644 drivers/misc/bootstage.c
> 
> --
> 2.49.0
> 
> 
> 
> Links:
> ------
> [1] 
> https://lore.kernel.org/linux-embedded/MW5PR13MB5632B8FA3279D77F2F9217BBFD9CA@MW5PR13MB5632.namprd13.prod.outlook.com/

Re: [RFC PATCH 0/1] Add driver for bootstage stash

[Rob Landley]

On 5/23/25 02:34, Federico Giovanardi wrote:
> Hello,
> 
> The note about the data format also was my initial thought, by just 
> copying a C structure we might have issues as soon one party changes it, 
> and they might not be perfectly aligned.
> 
> To avoid inventing yet-another-data-format I've used msgpack in the past 
> for that (the format https://github.com/msgpack/msgpack/blob/master/ 
> spec.md, not the library ); because the specs are so simple they can be 
> implemented in a few lines, and it's something with a reference. But I 
> don't have a lot of experience in upstreaming stuff on the kernel, so I 
> don't know if it might cause someone to don't be happy. Anyway, I can 
> contribute the implementation if needed.

I note that if it has an external build-time dependency, I will not only 
never enable it, I will go to great lengths to _disable_ it. I maintain 
a patch to build x86-64 without that ridiculous fourth ELF library, for 
example:

https://landley.net/bin/mkroot/latest/linux-patches/0005-x86-64-elfcrap.patch

The traditional approach to exporting data was to have synthetic 
filesystems like /sys and /blah produce human readable ascii output that 
could be easily parsed, because unix is textual.

Ala "Write programs to handle text streams, because that is a universal 
interface." from https://en.wikipedia.org/wiki/Unix_philosophy (quoting 
Peter Salus's "a quarter century of unix" in 1994 which was itself 
quoting... I think Doug McIlroy?

Rob

P.S. The whole digression into Plan 9 muddied the waters, not because 
they backed off on it but because the original authors of unix doubled 
down on their sequel project, and then unix copied things like the 
concept of synthetic filesystems from that. The difference is Unix was 
published freely in 1974 and Plan 9 was closed source proprietary until 
2000, so the first changed the world and the second is a footnote must 
people weren't aware existed until it had basically died, and then mined 
it for ideas they could port to what they were using without ever 
touching the incompatibly licensed code...

Re: [RFC PATCH 0/1] Add driver for bootstage stash

[Francesco Valla]

Hi Federico,

On Friday, 23 May 2025 at 09:34:09 Federico Giovanardi <fede.dev@giovanardi.dev> wrote:
> Hello,
> 
> The note about the data format also was my initial thought, by just 
> copying a C structure we might have issues as soon one party changes it, 
> and they might not be perfectly aligned.
> 

Definitely - before a proper implementation, a formal specification with
a versioning should probably be defined.

> To avoid inventing yet-another-data-format I've used msgpack in the past 
> for that (the format 
> https://github.com/msgpack/msgpack/blob/master/spec.md, not the library  
> ); because the specs are so simple they can be implemented in a few 
> lines, and it's something with a reference. But I don't have a lot of 
> experience in upstreaming stuff on the kernel, so I don't know if it 
> might cause someone to don't be happy. Anyway, I can contribute the 
> implementation if needed.
> 

I'd avoid a serialization technology here, as it would be another
(somewhat costly) step before booting, while re-using the memory area
that the bootloader itself uses as stash is basically free.


Regards,
Francesco

RE: [RFC PATCH 0/1] Add driver for bootstage stash

[Bird, Tim]

> -----Original Message-----
> From: Federico Giovanardi <fede.dev@giovanardi.dev>
> Hello,
> 
> The note about the data format also was my initial thought, by just
> copying a C structure we might have issues as soon one party changes it,
> and they might not be perfectly aligned.
> 
> To avoid inventing yet-another-data-format I've used msgpack in the past
> for that (the format
> https://github.com/msgpack/msgpack/blob/master/spec.md, not the library
> ); because the specs are so simple they can be implemented in a few
> lines, and it's something with a reference. But I don't have a lot of
> experience in upstreaming stuff on the kernel, so I don't know if it
> might cause someone to don't be happy. Anyway, I can contribute the
> implementation if needed.
> 
> Something as simple as an array of fixarray will give extensibility with
> only a few bytes of overhead.
> 
> Which gets encoded as:
> 
> 0xdc # lenght 16 bit << array header
>       # 0xB << 4 | ( array_size & 0xF) << fixarray header ( 3 elements,
> simplest case)
>             # 0xce # time_us
>             # 0xce # start_us
>             # 0xc << 4 | strlen(name) # name
>             /*no flags, no id*/
>       # 0xB << 4 | ( array_size & 0xF) << fixarray header ( 5 elements
> bigger case)
>             # 0xce # time_us
>             # 0xce # start_us
>             # 0xc << 4 | strlen(name) # name
>             # 0xcc # flags
>             # 0xcc # id
>       .. repeat ...
> 
> 
> Since the goal is to use that in many different contexts, defining the
> fields that we need early is important.

My own opinion is that the current bootstash format is too complicated, and that the
Unified Boot Log record format should be simplified to just
name, counter_id, and counter value, with fixed limits on the size of each.

struct UBL_entry {
  char name[7];
  u8 counter_id;
  u64 counter_value;
}

None of this looking up names in a separate table business.

One counter_id could be 0 - indicating that the counter is time in microseconds
(for U-boot bootstage data, where the values are already converted to microseconds).
I would prefer doing that counter conversion at boot-time, when the cycle counter
values can (almost) as easily be normalized to microseconds at report time.

That is, other cycle-counters used in a single Unified Boot Log could be used, and
instrumentation can just store their value, without doing the conversion.  The reporting
tool would need to know the conversion rate for each counter_id.  But it
should be able to calculate that at report time.

It would be good to discuss whether that reporting (and conversion) should be in the kernel
or a separate user-space tool.  The current patch puts reporting into the kernel.
   -- Tim

RE: [RFC PATCH 0/1] Add driver for bootstage stash

[Bird, Tim]

Correction...

> -----Original Message-----
> From: Bird, Tim <Tim.Bird@sony.com>
> One counter_id could be 0 - indicating that the counter is time in microseconds
> (for U-boot bootstage data, where the values are already converted to microseconds).
> I would prefer doing that counter conversion at boot-time, when the cycle counter
I would prefer NOT doing that counter conversion at boot-time...

> values can (almost) as easily be normalized to microseconds at report time.
>