Qualcomm's proprietary Android boot-loaders capture boot time
stats, like the time when the bootloader started execution and at what
point the bootloader handed over control to the kernel etc. in the IMEM
region. This information is captured in a specific format by this driver
by mapping a structure to the IMEM memory region and then accessing the
members of the structure to log the information in a debugfs file.
This information is useful in verifying if existing boot KPIs have
regressed or not.
A sample log in SM8450(waipio) device is as follows:-
/sys/kernel/debug/146aa6b0.boot_stats # cat abl_time
17898 ms
/sys/kernel/debug/146aa6b0.boot_stats # cat pre_abl_time
2879 ms
The Module Power Manager(MPM) sleep counter starts ticking at the PBL
stage and the timestamp generated by the sleep counter is logged by
the Qualcomm proprietary bootloader(ABL) at two points-> First when it
starts execution which is logged here as "pre_abl_time" and the second
when it is about to load the kernel logged as "abl_time". Both these
values are read up by the driver from IMEM region and printed as above.
Souradeep Chowdhury (3):
dt-bindings: sram: qcom,imem: Add Boot Stat region within IMEM
soc: qcom: boot_stat: Add Driver Support for Boot Stats
MAINTAINERS: Add the entry for boot_stats driver support
Documentation/ABI/testing/debugfs-driver-bootstat | 17 ++++
.../devicetree/bindings/sram/qcom,imem.yaml | 22 +++++
MAINTAINERS | 7 ++
drivers/soc/qcom/Kconfig | 9 ++
drivers/soc/qcom/Makefile | 1 +
drivers/soc/qcom/boot_stats.c | 109 +++++++++++++++++++++
6 files changed, 165 insertions(+)
create mode 100644 Documentation/ABI/testing/debugfs-driver-bootstat
create mode 100644 drivers/soc/qcom/boot_stats.c
--
2.7.4
All Qualcomm bootloaders log useful timestamp information related
to bootloader stats in the IMEM region. Add the child node within
IMEM for the boot stat region containing register address and
compatible string.
Signed-off-by: Souradeep Chowdhury <[email protected]>
---
.../devicetree/bindings/sram/qcom,imem.yaml | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
index ba694ce..c1e1e9b 100644
--- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
+++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
@@ -49,6 +49,28 @@ patternProperties:
$ref: /schemas/remoteproc/qcom,pil-info.yaml#
description: Peripheral image loader relocation region
+ "^stats@[0-9a-f]+$":
+ type: object
+ description:
+ Imem region dedicated for storing timestamps related
+ information regarding bootstats.
+
+ properties:
+ compatible:
+ items:
+ - enum:
+ - qcom,sm8450-bootstats
+ - const: qcom,imem-bootstats
+
+ reg:
+ maxItems: 1
+
+ required:
+ - compatible
+ - reg
+
+ additionalProperties: false
+
required:
- compatible
- reg
--
2.7.4
All of Qualcomm's proprietary Android boot-loaders capture boot time
stats, like the time when the bootloader started execution and at what
point the bootloader handed over control to the kernel etc. in the IMEM
region. This information is captured in a specific format by this driver
by mapping a structure to the IMEM memory region and then accessing the
members of the structure to show the information within debugfs file.
This information is useful in verifying if the existing boot KPIs have
regressed or not. The information is shown in milliseconds, a sample
log from sm8450(waipio) device is as follows:-
/sys/kernel/debug/146aa6b0.boot_stats # cat abl_time
17898 ms
/sys/kernel/debug/146aa6b0.boot_stats # cat pre_abl_time
2879 ms
The Module Power Manager(MPM) sleep counter starts ticking at the PBL
stage and the timestamp generated by the sleep counter is logged by
the Qualcomm proprietary bootloader(ABL) at two points-> First when it
starts execution which is logged here as "pre_abl_time" and the second
when it is about to load the kernel logged as "abl_time". Documentation
details are also added in Documentation/ABI/testing/debugfs-driver-bootstat
Signed-off-by: Souradeep Chowdhury <[email protected]>
---
Documentation/ABI/testing/debugfs-driver-bootstat | 17 ++++
drivers/soc/qcom/Kconfig | 9 ++
drivers/soc/qcom/Makefile | 1 +
drivers/soc/qcom/boot_stats.c | 109 ++++++++++++++++++++++
4 files changed, 136 insertions(+)
create mode 100644 Documentation/ABI/testing/debugfs-driver-bootstat
create mode 100644 drivers/soc/qcom/boot_stats.c
diff --git a/Documentation/ABI/testing/debugfs-driver-bootstat b/Documentation/ABI/testing/debugfs-driver-bootstat
new file mode 100644
index 0000000..37d8e32
--- /dev/null
+++ b/Documentation/ABI/testing/debugfs-driver-bootstat
@@ -0,0 +1,17 @@
+What: /sys/kernel/debug/...stats/pre_abl_time
+Date: April 2023
+Contact: Souradeep Chowdhury <[email protected]>
+Description:
+ This file is used to read the KPI value pre abl time.
+ It shows the time in milliseconds from the starting
+ point of PBL to the point when the control shifted
+ to ABL(Qualcomm proprietary bootloader).
+
+What: /sys/kernel/debug/...stats/abl_time
+Date: April 2023
+Contact: Souradeep Chowdhury <[email protected]>
+Description:
+ This file is used to read the KPI value abl time.
+ It show the duration in milliseconds from the
+ time control switched to ABL to the point when
+ the linux kernel started getting loaded.
diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index a8f2830..0d2cbd3 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -16,6 +16,15 @@ config QCOM_AOSS_QMP
subsystems as well as controlling the debug clocks exposed by the Always On
Subsystem (AOSS) using Qualcomm Messaging Protocol (QMP).
+config QCOM_BOOTSTAT
+ tristate "Qualcomm Technologies, Boot Stat driver"
+ depends on ARCH_QCOM || COMPILE_TEST
+ help
+ This option enables driver support for boot stats. Boot stat driver logs
+ the kernel bootloader information by accessing the imem region. These
+ information is exposed in the form of debugfs files. This is used to
+ determine if there is any regression in boot timings.
+
config QCOM_COMMAND_DB
tristate "Qualcomm Command DB"
depends on ARCH_QCOM || COMPILE_TEST
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 6e88da8..bdaa41a 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -1,6 +1,7 @@
# SPDX-License-Identifier: GPL-2.0
CFLAGS_rpmh-rsc.o := -I$(src)
obj-$(CONFIG_QCOM_AOSS_QMP) += qcom_aoss.o
+obj-$(CONFIG_QCOM_BOOTSTAT) += boot_stats.o
obj-$(CONFIG_QCOM_GENI_SE) += qcom-geni-se.o
obj-$(CONFIG_QCOM_COMMAND_DB) += cmd-db.o
obj-$(CONFIG_QCOM_CPR) += cpr.o
diff --git a/drivers/soc/qcom/boot_stats.c b/drivers/soc/qcom/boot_stats.c
new file mode 100644
index 0000000..9b65aa1
--- /dev/null
+++ b/drivers/soc/qcom/boot_stats.c
@@ -0,0 +1,109 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2013-2019, 2021 The Linux Foundation. All rights reserved.
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/debugfs.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+
+#define TO_MS(timestamp) ((timestamp * 1000) / 32768)
+
+/**
+ * struct boot_stats - timestamp information related to boot stats
+ * @abl_start: Time for the starting point of the abl
+ * @abl_end: Time when the kernel starts loading from abl
+ */
+struct boot_stats {
+ u32 abl_start;
+ u32 abl_end;
+} __packed;
+
+static struct boot_stats __iomem *boot_stats;
+static struct dentry *dbg_dir;
+
+static ssize_t abl_read(struct file *filp, char __user *userbuf,
+ size_t count, loff_t *ppos)
+{
+ char buf[20];
+
+ u32 abl_time = TO_MS(boot_stats->abl_end) - TO_MS(boot_stats->abl_start);
+
+ sprintf(buf, "%u ms\n", abl_time);
+ return simple_read_from_buffer(userbuf, count, ppos, buf, strlen(buf));
+}
+
+static const struct file_operations abl_fops = {
+ .read = abl_read,
+ .open = simple_open,
+ .llseek = generic_file_llseek,
+};
+
+static ssize_t pre_abl_read(struct file *filp, char __user *userbuf,
+ size_t count, loff_t *ppos)
+{
+ char buf[20];
+
+ sprintf(buf, "%u ms\n", TO_MS(boot_stats->abl_start));
+ return simple_read_from_buffer(userbuf, count, ppos, buf, strlen(buf));
+}
+
+static const struct file_operations pre_abl_fops = {
+ .read = pre_abl_read,
+ .open = simple_open,
+ .llseek = generic_file_llseek,
+};
+
+static int boot_stats_probe(struct platform_device *pdev)
+{
+ struct device *boot_stat = &pdev->dev;
+
+ dbg_dir = debugfs_create_dir(dev_name(&pdev->dev), NULL);
+ if (IS_ERR(dbg_dir))
+ return dev_err_probe(&pdev->dev, -ENOENT,
+ "failed to create debugfs directory");
+
+ boot_stats = of_iomap(boot_stat->of_node, 0);
+ if (!boot_stats)
+ return dev_err_probe(&pdev->dev, -ENOMEM,
+ "failed to map imem region\n");
+
+ debugfs_create_file("pre_abl_time", 0200, dbg_dir, NULL, &pre_abl_fops);
+ debugfs_create_file("abl_time", 0200, dbg_dir, NULL, &abl_fops);
+
+ return 0;
+}
+
+static int boot_stats_remove(struct platform_device *pdev)
+{
+ debugfs_remove_recursive(dbg_dir);
+ iounmap(boot_stats);
+
+ return 0;
+}
+
+static const struct of_device_id boot_stats_dt_match[] = {
+ { .compatible = "qcom,sm8450-bootstats" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, boot_stats_dt_match);
+
+static struct platform_driver boot_stat_driver = {
+ .probe = boot_stats_probe,
+ .remove = boot_stats_remove,
+ .driver = {
+ .name = "qcom-boot-stats",
+ .of_match_table = boot_stats_dt_match,
+ },
+};
+module_platform_driver(boot_stat_driver);
+
+MODULE_DESCRIPTION("Qualcomm Technologies Inc. Boot Stat driver");
+MODULE_LICENSE("GPL");
--
2.7.4
Add the entries for all the files added as a part of driver support for
boot stats.
Signed-off-by: Souradeep Chowdhury <[email protected]>
---
MAINTAINERS | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 0e64787..20b96e9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17055,6 +17055,13 @@ L: [email protected]
S: Supported
F: drivers/net/ipa/
+QCOM BOOT_STATS DRIVER
+M: Souradeep Chowdhury <[email protected]>
+L: [email protected]
+S: Maintained
+F: Documentation/ABI/testing/debugfs-driver-bootstat
+F: drivers/soc/qcom/boot_stats.c
+
QEMU MACHINE EMULATOR AND VIRTUALIZER SUPPORT
M: Gabriel Somlo <[email protected]>
M: "Michael S. Tsirkin" <[email protected]>
--
2.7.4
On 13/04/2023 11:28, Souradeep Chowdhury wrote:
Thank you for your patch. There is something to discuss/improve.
> + "^stats@[0-9a-f]+$":
> + type: object
> + description:
> + Imem region dedicated for storing timestamps related
> + information regarding bootstats.
> +
> + properties:
> + compatible:
> + items:
> + - enum:
> + - qcom,sm8450-bootstats
> + - const: qcom,imem-bootstats
> +
> + reg:
> + maxItems: 1
> +
> + required:
> + - compatible
> + - reg
> +
> + additionalProperties: false
The feedback about additionalProperties was given in different place. On
purpose, because it is easier to read when it is placed before
properties for indented cases. Therefore move it after description, how
I originally asked.
> +
> required:
> - compatible
> - reg
Best regards,
Krzysztof
On 13/04/2023 10:28, Souradeep Chowdhury wrote:
> All of Qualcomm's proprietary Android boot-loaders capture boot time
> stats, like the time when the bootloader started execution and at what
> point the bootloader handed over control to the kernel etc. in the IMEM
> region. This information is captured in a specific format by this driver
> by mapping a structure to the IMEM memory region and then accessing the
> members of the structure to show the information within debugfs file.
> This information is useful in verifying if the existing boot KPIs have
> regressed or not. The information is shown in milliseconds, a sample
> log from sm8450(waipio) device is as follows:-
>
> /sys/kernel/debug/146aa6b0.boot_stats # cat abl_time
> 17898 ms
> /sys/kernel/debug/146aa6b0.boot_stats # cat pre_abl_time
> 2879 ms
>
> The Module Power Manager(MPM) sleep counter starts ticking at the PBL
> stage and the timestamp generated by the sleep counter is logged by
> the Qualcomm proprietary bootloader(ABL) at two points-> First when it
> starts execution which is logged here as "pre_abl_time" and the second
> when it is about to load the kernel logged as "abl_time". Documentation
> details are also added in Documentation/ABI/testing/debugfs-driver-bootstat
>
> Signed-off-by: Souradeep Chowdhury <[email protected]>
> ---
> Documentation/ABI/testing/debugfs-driver-bootstat | 17 ++++
> drivers/soc/qcom/Kconfig | 9 ++
> drivers/soc/qcom/Makefile | 1 +
> drivers/soc/qcom/boot_stats.c | 109 ++++++++++++++++++++++
> 4 files changed, 136 insertions(+)
> create mode 100644 Documentation/ABI/testing/debugfs-driver-bootstat
> create mode 100644 drivers/soc/qcom/boot_stats.c
>
> diff --git a/Documentation/ABI/testing/debugfs-driver-bootstat b/Documentation/ABI/testing/debugfs-driver-bootstat
> new file mode 100644
> index 0000000..37d8e32
> --- /dev/null
> +++ b/Documentation/ABI/testing/debugfs-driver-bootstat
> @@ -0,0 +1,17 @@
> +What: /sys/kernel/debug/...stats/pre_abl_time
> +Date: April 2023
> +Contact: Souradeep Chowdhury <[email protected]>
> +Description:
> + This file is used to read the KPI value pre abl time.
> + It shows the time in milliseconds from the starting
> + point of PBL to the point when the control shifted
> + to ABL(Qualcomm proprietary bootloader).
> +
> +What: /sys/kernel/debug/...stats/abl_time
> +Date: April 2023
> +Contact: Souradeep Chowdhury <[email protected]>
> +Description:
> + This file is used to read the KPI value abl time.
> + It show the duration in milliseconds from the
> + time control switched to ABL to the point when
> + the linux kernel started getting loaded.
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index a8f2830..0d2cbd3 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -16,6 +16,15 @@ config QCOM_AOSS_QMP
> subsystems as well as controlling the debug clocks exposed by the Always On
> Subsystem (AOSS) using Qualcomm Messaging Protocol (QMP).
>
> +config QCOM_BOOTSTAT
> + tristate "Qualcomm Technologies, Boot Stat driver"
> + depends on ARCH_QCOM || COMPILE_TEST
> + help
> + This option enables driver support for boot stats. Boot stat driver logs
> + the kernel bootloader information by accessing the imem region. These
> + information is exposed in the form of debugfs files. This is used to
> + determine if there is any regression in boot timings.
> +
> config QCOM_COMMAND_DB
> tristate "Qualcomm Command DB"
> depends on ARCH_QCOM || COMPILE_TEST
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index 6e88da8..bdaa41a 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -1,6 +1,7 @@
> # SPDX-License-Identifier: GPL-2.0
> CFLAGS_rpmh-rsc.o := -I$(src)
> obj-$(CONFIG_QCOM_AOSS_QMP) += qcom_aoss.o
> +obj-$(CONFIG_QCOM_BOOTSTAT) += boot_stats.o
> obj-$(CONFIG_QCOM_GENI_SE) += qcom-geni-se.o
> obj-$(CONFIG_QCOM_COMMAND_DB) += cmd-db.o
> obj-$(CONFIG_QCOM_CPR) += cpr.o
> diff --git a/drivers/soc/qcom/boot_stats.c b/drivers/soc/qcom/boot_stats.c
> new file mode 100644
> index 0000000..9b65aa1
> --- /dev/null
> +++ b/drivers/soc/qcom/boot_stats.c
> @@ -0,0 +1,109 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2013-2019, 2021 The Linux Foundation. All rights reserved.
> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/debugfs.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +
> +#define TO_MS(timestamp) ((timestamp * 1000) / 32768)
> +
> +/**
> + * struct boot_stats - timestamp information related to boot stats
> + * @abl_start: Time for the starting point of the abl
> + * @abl_end: Time when the kernel starts loading from abl
> + */
> +struct boot_stats {
> + u32 abl_start;
> + u32 abl_end;
> +} __packed;
> +
> +static struct boot_stats __iomem *boot_stats;
> +static struct dentry *dbg_dir;
> +
I would avoid global variables where possible, and allocate them
dynamically.
In this case you could do that in probe and pass it(boot_stats) as
argument to debugfs_create_file() and also set dbg_dir as
dev_set_drvdata to be able to use it in remove.
----------------->cut<--------------
> +static ssize_t abl_read(struct file *filp, char __user *userbuf,
> + size_t count, loff_t *ppos)
> +{
> + char buf[20];
> +
> + u32 abl_time = TO_MS(boot_stats->abl_end) - TO_MS(boot_stats->abl_start);
> +
> + sprintf(buf, "%u ms\n", abl_time);
> + return simple_read_from_buffer(userbuf, count, ppos, buf, strlen(buf));
> +}
> +
> +static const struct file_operations abl_fops = {
> + .read = abl_read,
> + .open = simple_open,
> + .llseek = generic_file_llseek,
> +};
> +
--------------->cut<----------------
can be replaced with.
static int abl_show(struct seq_file *s_file, void *data)
{
struct boot_stats *boot_stats = s_file->private;
...
}
DEFINE_SHOW_ATTRIBUTE(abl);
> +static ssize_t pre_abl_read(struct file *filp, char __user *userbuf,
> + size_t count, loff_t *ppos)
> +{
> + char buf[20];
> +
> + sprintf(buf, "%u ms\n", TO_MS(boot_stats->abl_start));
> + return simple_read_from_buffer(userbuf, count, ppos, buf, strlen(buf));
> +}
> +
> +static const struct file_operations pre_abl_fops = {
> + .read = pre_abl_read,
> + .open = simple_open,
> + .llseek = generic_file_llseek,
> +};
> +
same here.
> +static int boot_stats_probe(struct platform_device *pdev)
> +{
> + struct device *boot_stat = &pdev->dev;
> +
> + dbg_dir = debugfs_create_dir(dev_name(&pdev->dev), NULL);
> + if (IS_ERR(dbg_dir))
> + return dev_err_probe(&pdev->dev, -ENOENT,
> + "failed to create debugfs directory");
> +
> + boot_stats = of_iomap(boot_stat->of_node, 0);
may be devm_of_iomap?
> + if (!boot_stats)
> + return dev_err_probe(&pdev->dev, -ENOMEM,
> + "failed to map imem region\n");
> +
> + debugfs_create_file("pre_abl_time", 0200, dbg_dir, NULL, &pre_abl_fops);
> + debugfs_create_file("abl_time", 0200, dbg_dir, NULL, &abl_fops);
> +
> + return 0;
> +}
> +
> +static int boot_stats_remove(struct platform_device *pdev)
> +{
> + debugfs_remove_recursive(dbg_dir);
> + iounmap(boot_stats);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id boot_stats_dt_match[] = {
> + { .compatible = "qcom,sm8450-bootstats" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, boot_stats_dt_match);
> +
> +static struct platform_driver boot_stat_driver = {
> + .probe = boot_stats_probe,
> + .remove = boot_stats_remove,
consider using remove_new as its suggested that new drivers implement
.remove_new as it returns void.
more details in include/linux/platform_device.h
-srini
> + .driver = {
> + .name = "qcom-boot-stats",
> + .of_match_table = boot_stats_dt_match,
> + },
> +};
> +module_platform_driver(boot_stat_driver);
> +
> +MODULE_DESCRIPTION("Qualcomm Technologies Inc. Boot Stat driver");
> +MODULE_LICENSE("GPL");
> --
> 2.7.4
>
On 4/14/2023 5:04 PM, Srinivas Kandagatla wrote:
>
>
> On 13/04/2023 10:28, Souradeep Chowdhury wrote:
>> All of Qualcomm's proprietary Android boot-loaders capture boot time
>> stats, like the time when the bootloader started execution and at what
>> point the bootloader handed over control to the kernel etc. in the IMEM
>> region. This information is captured in a specific format by this driver
>> by mapping a structure to the IMEM memory region and then accessing the
>> members of the structure to show the information within debugfs file.
>> This information is useful in verifying if the existing boot KPIs have
>> regressed or not. The information is shown in milliseconds, a sample
>> log from sm8450(waipio) device is as follows:-
>>
>> /sys/kernel/debug/146aa6b0.boot_stats # cat abl_time
>> 17898 ms
>> /sys/kernel/debug/146aa6b0.boot_stats # cat pre_abl_time
>> 2879 ms
>>
>> The Module Power Manager(MPM) sleep counter starts ticking at the PBL
>> stage and the timestamp generated by the sleep counter is logged by
>> the Qualcomm proprietary bootloader(ABL) at two points-> First when it
>> starts execution which is logged here as "pre_abl_time" and the second
>> when it is about to load the kernel logged as "abl_time". Documentation
>> details are also added in
>> Documentation/ABI/testing/debugfs-driver-bootstat
>>
>> Signed-off-by: Souradeep Chowdhury <[email protected]>
>> ---
>> Documentation/ABI/testing/debugfs-driver-bootstat | 17 ++++
>> drivers/soc/qcom/Kconfig | 9 ++
>> drivers/soc/qcom/Makefile | 1 +
>> drivers/soc/qcom/boot_stats.c | 109
>> ++++++++++++++++++++++
>> 4 files changed, 136 insertions(+)
>> create mode 100644 Documentation/ABI/testing/debugfs-driver-bootstat
>> create mode 100644 drivers/soc/qcom/boot_stats.c
>>
>> diff --git a/Documentation/ABI/testing/debugfs-driver-bootstat
>> b/Documentation/ABI/testing/debugfs-driver-bootstat
>> new file mode 100644
>> index 0000000..37d8e32
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/debugfs-driver-bootstat
>> @@ -0,0 +1,17 @@
>> +What: /sys/kernel/debug/...stats/pre_abl_time
>> +Date: April 2023
>> +Contact: Souradeep Chowdhury <[email protected]>
>> +Description:
>> + This file is used to read the KPI value pre abl time.
>> + It shows the time in milliseconds from the starting
>> + point of PBL to the point when the control shifted
>> + to ABL(Qualcomm proprietary bootloader).
>> +
>> +What: /sys/kernel/debug/...stats/abl_time
>> +Date: April 2023
>> +Contact: Souradeep Chowdhury <[email protected]>
>> +Description:
>> + This file is used to read the KPI value abl time.
>> + It show the duration in milliseconds from the
>> + time control switched to ABL to the point when
>> + the linux kernel started getting loaded.
>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>> index a8f2830..0d2cbd3 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -16,6 +16,15 @@ config QCOM_AOSS_QMP
>> subsystems as well as controlling the debug clocks exposed by
>> the Always On
>> Subsystem (AOSS) using Qualcomm Messaging Protocol (QMP).
>>
>> +config QCOM_BOOTSTAT
>> + tristate "Qualcomm Technologies, Boot Stat driver"
>> + depends on ARCH_QCOM || COMPILE_TEST
>> + help
>> + This option enables driver support for boot stats. Boot stat
>> driver logs
>> + the kernel bootloader information by accessing the imem region.
>> These
>> + information is exposed in the form of debugfs files. This is
>> used to
>> + determine if there is any regression in boot timings.
>> +
>> config QCOM_COMMAND_DB
>> tristate "Qualcomm Command DB"
>> depends on ARCH_QCOM || COMPILE_TEST
>> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
>> index 6e88da8..bdaa41a 100644
>> --- a/drivers/soc/qcom/Makefile
>> +++ b/drivers/soc/qcom/Makefile
>> @@ -1,6 +1,7 @@
>> # SPDX-License-Identifier: GPL-2.0
>> CFLAGS_rpmh-rsc.o := -I$(src)
>> obj-$(CONFIG_QCOM_AOSS_QMP) += qcom_aoss.o
>> +obj-$(CONFIG_QCOM_BOOTSTAT) += boot_stats.o
>> obj-$(CONFIG_QCOM_GENI_SE) += qcom-geni-se.o
>> obj-$(CONFIG_QCOM_COMMAND_DB) += cmd-db.o
>> obj-$(CONFIG_QCOM_CPR) += cpr.o
>> diff --git a/drivers/soc/qcom/boot_stats.c
>> b/drivers/soc/qcom/boot_stats.c
>> new file mode 100644
>> index 0000000..9b65aa1
>> --- /dev/null
>> +++ b/drivers/soc/qcom/boot_stats.c
>> @@ -0,0 +1,109 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2013-2019, 2021 The Linux Foundation. All rights
>> reserved.
>> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights
>> reserved.
>> + */
>> +
>> +#include <linux/debugfs.h>
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/init.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +
>> +#define TO_MS(timestamp) ((timestamp * 1000) / 32768)
>> +
>> +/**
>> + * struct boot_stats - timestamp information related to boot stats
>> + * @abl_start: Time for the starting point of the abl
>> + * @abl_end: Time when the kernel starts loading from abl
>> + */
>> +struct boot_stats {
>> + u32 abl_start;
>> + u32 abl_end;
>> +} __packed;
>> +
>> +static struct boot_stats __iomem *boot_stats;
>> +static struct dentry *dbg_dir;
>> +
> I would avoid global variables where possible, and allocate them
> dynamically.
>
> In this case you could do that in probe and pass it(boot_stats) as
> argument to debugfs_create_file() and also set dbg_dir as
> dev_set_drvdata to be able to use it in remove.
Ack
>
>
> ----------------->cut<--------------
>> +static ssize_t abl_read(struct file *filp, char __user *userbuf,
>> + size_t count, loff_t *ppos)
>> +{
>> + char buf[20];
>> +
>> + u32 abl_time = TO_MS(boot_stats->abl_end) -
>> TO_MS(boot_stats->abl_start);
>> +
>> + sprintf(buf, "%u ms\n", abl_time);
>> + return simple_read_from_buffer(userbuf, count, ppos, buf,
>> strlen(buf));
>> +}
>> +
>> +static const struct file_operations abl_fops = {
>> + .read = abl_read,
>> + .open = simple_open,
>> + .llseek = generic_file_llseek,
>> +};
>> +
> --------------->cut<----------------
> can be replaced with.
>
> static int abl_show(struct seq_file *s_file, void *data)
> {
> struct boot_stats *boot_stats = s_file->private;
> ...
> }
>
> DEFINE_SHOW_ATTRIBUTE(abl);
Ack
>
>> +static ssize_t pre_abl_read(struct file *filp, char __user *userbuf,
>> + size_t count, loff_t *ppos)
>> +{
>> + char buf[20];
>> +
>> + sprintf(buf, "%u ms\n", TO_MS(boot_stats->abl_start));
>> + return simple_read_from_buffer(userbuf, count, ppos, buf,
>> strlen(buf));
>> +}
>> +
>> +static const struct file_operations pre_abl_fops = {
>> + .read = pre_abl_read,
>> + .open = simple_open,
>> + .llseek = generic_file_llseek,
>> +};
>> +
> same here.
Ack
>
>> +static int boot_stats_probe(struct platform_device *pdev)
>> +{
>> + struct device *boot_stat = &pdev->dev;
>> +
>> + dbg_dir = debugfs_create_dir(dev_name(&pdev->dev), NULL);
>> + if (IS_ERR(dbg_dir))
>> + return dev_err_probe(&pdev->dev, -ENOENT,
>> + "failed to create debugfs directory");
>> +
>> + boot_stats = of_iomap(boot_stat->of_node, 0);
>
> may be devm_of_iomap?
Ack
>
>> + if (!boot_stats)
>> + return dev_err_probe(&pdev->dev, -ENOMEM,
>> + "failed to map imem region\n");
>> +
>> + debugfs_create_file("pre_abl_time", 0200, dbg_dir, NULL,
>> &pre_abl_fops);
>> + debugfs_create_file("abl_time", 0200, dbg_dir, NULL, &abl_fops);
>> +
>> + return 0;
>> +}
>> +
>> +static int boot_stats_remove(struct platform_device *pdev)
>> +{
>> + debugfs_remove_recursive(dbg_dir);
>> + iounmap(boot_stats);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id boot_stats_dt_match[] = {
>> + { .compatible = "qcom,sm8450-bootstats" },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(of, boot_stats_dt_match);
>> +
>> +static struct platform_driver boot_stat_driver = {
>> + .probe = boot_stats_probe,
>> + .remove = boot_stats_remove,
>
> consider using remove_new as its suggested that new drivers implement
> .remove_new as it returns void.
> more details in include/linux/platform_device.h
Ack
>
>
> -srini
>
>> + .driver = {
>> + .name = "qcom-boot-stats",
>> + .of_match_table = boot_stats_dt_match,
>> + },
>> +};
>> +module_platform_driver(boot_stat_driver);
>> +
>> +MODULE_DESCRIPTION("Qualcomm Technologies Inc. Boot Stat driver");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.7.4
>>
On 4/13/2023 10:11 PM, Krzysztof Kozlowski wrote:
> On 13/04/2023 11:28, Souradeep Chowdhury wrote:
>
> Thank you for your patch. There is something to discuss/improve.
>
>> + "^stats@[0-9a-f]+$":
>> + type: object
>> + description:
>> + Imem region dedicated for storing timestamps related
>> + information regarding bootstats.
>> +
>> + properties:
>> + compatible:
>> + items:
>> + - enum:
>> + - qcom,sm8450-bootstats
>> + - const: qcom,imem-bootstats
>> +
>> + reg:
>> + maxItems: 1
>> +
>> + required:
>> + - compatible
>> + - reg
>> +
>> + additionalProperties: false
>
> The feedback about additionalProperties was given in different place. On
> purpose, because it is easier to read when it is placed before
> properties for indented cases. Therefore move it after description, how
> I originally asked.
Ack
>
>> +
>> required:
>> - compatible
>> - reg
>
> Best regards,
> Krzysztof
>