| Message ID | cover.1683628357.git.quic_schowdhu@quicinc.com |
|---|---|
| Headers | show |
| Series | soc: qcom: boot_stats: Add driver support for boot_stats | expand |
On Tue, 9 May 2023 at 13:53, Souradeep Chowdhury <quic_schowdhu@quicinc.com> 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/qcom_boot_stats # cat abl_time > 17898 ms > /sys/kernel/debug/qcom_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 <quic_schowdhu@quicinc.com> > --- > .../ABI/testing/debugfs-driver-bootstat | 17 +++ > drivers/soc/qcom/Kconfig | 10 ++ > drivers/soc/qcom/Makefile | 1 + > drivers/soc/qcom/boot_stats.c | 100 ++++++++++++++++++ > 4 files changed, 128 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 000000000000..7127d15d9f15 > --- /dev/null > +++ b/Documentation/ABI/testing/debugfs-driver-bootstat > @@ -0,0 +1,17 @@ > +What: /sys/kernel/debug/qcom_boot_stats/pre_abl_time Could you please change these bindings to be generic? s/qcom_boot_stats/boot_stats/ s/pre_abl_time/pre_bootloader_msec/ s/abl_time/bootloader_msec/ This way other platforms might also use the same file structure. > +Date: May 2023 > +Contact: Souradeep Chowdhury <quic_schowdhu@quicinc.com> > +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/qcom_boot_stats/abl_time > +Date: May 2023 > +Contact: Souradeep Chowdhury <quic_schowdhu@quicinc.com> > +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 a491718f8064..04141236dcdd 100644 > --- a/drivers/soc/qcom/Kconfig > +++ b/drivers/soc/qcom/Kconfig > @@ -16,6 +16,16 @@ 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 > + depends on DEBUG_FS > + help > + This option enables driver support for boot stats. Boot stat driver logs > + the kernel bootloader information by accessing the imem region. These > + information are 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 0f43a88b4894..ae7bda96a539 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 000000000000..ca67b6b5d8eb > --- /dev/null > +++ b/drivers/soc/qcom/boot_stats.c > @@ -0,0 +1,100 @@ > +// 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) Quoting v4 question, which got no answer: Some of the platforms DTs define 32KHz clock instead of 32.768 KHz What should be the divisor in this case? > + > +/** > + * 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; > + > +struct bs_data { > + struct boot_stats __iomem *b_stats; > + struct dentry *dbg_dir; > +}; > + > +static void populate_boot_stats(char *abl_str, char *pre_abl_str, struct bs_data *drvdata) > +{ > + u32 abl_time, pre_abl_time; > + > + abl_time = TO_MS(drvdata->b_stats->abl_end) - TO_MS(drvdata->b_stats->abl_start); > + sprintf(abl_str, "%u ms", abl_time); > + > + pre_abl_time = TO_MS(drvdata->b_stats->abl_start); > + sprintf(pre_abl_str, "%u ms", pre_abl_time); Another point from v4: It would be better to move the unit to the file name and include just the number. > +} > + > +static int boot_stats_probe(struct platform_device *pdev) > +{ > + char abl_str[20], pre_abl_str[20], *abl, *pre_abl; > + struct device *bootstat_dev = &pdev->dev; > + struct bs_data *drvdata; > + > + drvdata = devm_kzalloc(bootstat_dev, sizeof(*drvdata), GFP_KERNEL); > + if (!drvdata) > + return dev_err_probe(bootstat_dev, -ENOMEM, "failed to allocate memory"); > + platform_set_drvdata(pdev, drvdata); > + > + drvdata->b_stats = devm_of_iomap(bootstat_dev, bootstat_dev->of_node, 0, NULL); > + if (IS_ERR(drvdata->b_stats)) > + return dev_err_probe(bootstat_dev, PTR_ERR(drvdata->b_stats), > + "failed to map imem region"); > + > + drvdata->dbg_dir = debugfs_create_dir("qcom_boot_stats", NULL); > + if (IS_ERR(drvdata->dbg_dir)) > + return dev_err_probe(bootstat_dev, PTR_ERR(drvdata->dbg_dir), > + "failed to create debugfs directory"); > + > + populate_boot_stats(abl_str, pre_abl_str, drvdata); > + abl = abl_str; > + pre_abl = pre_abl_str; > + > + debugfs_create_str("pre_abl_time", 0400, drvdata->dbg_dir, (char **)&pre_abl); > + debugfs_create_str("abl_time", 0400, drvdata->dbg_dir, (char **)&abl); > + > + return 0; > +} > + > +void boot_stats_remove(struct platform_device *pdev) > +{ > + struct bs_data *drvdata = platform_get_drvdata(pdev); > + > + debugfs_remove_recursive(drvdata->dbg_dir); > +} > + > +static const struct of_device_id boot_stats_dt_match[] = { > + { .compatible = "qcom,imem-bootstats" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, boot_stats_dt_match); > + > +static struct platform_driver boot_stat_driver = { > + .probe = boot_stats_probe, > + .remove_new = 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.17.1 >
On 5/9/2023 5:09 PM, Dmitry Baryshkov wrote: > On Tue, 9 May 2023 at 13:53, Souradeep Chowdhury > <quic_schowdhu@quicinc.com> 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/qcom_boot_stats # cat abl_time >> 17898 ms >> /sys/kernel/debug/qcom_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 <quic_schowdhu@quicinc.com> >> --- >> .../ABI/testing/debugfs-driver-bootstat | 17 +++ >> drivers/soc/qcom/Kconfig | 10 ++ >> drivers/soc/qcom/Makefile | 1 + >> drivers/soc/qcom/boot_stats.c | 100 ++++++++++++++++++ >> 4 files changed, 128 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 000000000000..7127d15d9f15 >> --- /dev/null >> +++ b/Documentation/ABI/testing/debugfs-driver-bootstat >> @@ -0,0 +1,17 @@ >> +What: /sys/kernel/debug/qcom_boot_stats/pre_abl_time > > Could you please change these bindings to be generic? > > s/qcom_boot_stats/boot_stats/ > s/pre_abl_time/pre_bootloader_msec/ > s/abl_time/bootloader_msec/ > > This way other platforms might also use the same file structure. Ack > >> +Date: May 2023 >> +Contact: Souradeep Chowdhury <quic_schowdhu@quicinc.com> >> +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/qcom_boot_stats/abl_time >> +Date: May 2023 >> +Contact: Souradeep Chowdhury <quic_schowdhu@quicinc.com> >> +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 a491718f8064..04141236dcdd 100644 >> --- a/drivers/soc/qcom/Kconfig >> +++ b/drivers/soc/qcom/Kconfig >> @@ -16,6 +16,16 @@ 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 >> + depends on DEBUG_FS >> + help >> + This option enables driver support for boot stats. Boot stat driver logs >> + the kernel bootloader information by accessing the imem region. These >> + information are 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 0f43a88b4894..ae7bda96a539 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 000000000000..ca67b6b5d8eb >> --- /dev/null >> +++ b/drivers/soc/qcom/boot_stats.c >> @@ -0,0 +1,100 @@ >> +// 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) > > Quoting v4 question, which got no answer: > > Some of the platforms DTs define 32KHz clock instead of 32.768 KHz > What should be the divisor in this case? This is the standard divisor used to calculate the pre_abl and abl times across most QCOM SoCs. Can you give an example where the sleep_stat counter has a different frequency? > >> + >> +/** >> + * 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; >> + >> +struct bs_data { >> + struct boot_stats __iomem *b_stats; >> + struct dentry *dbg_dir; >> +}; >> + >> +static void populate_boot_stats(char *abl_str, char *pre_abl_str, struct bs_data *drvdata) >> +{ >> + u32 abl_time, pre_abl_time; >> + >> + abl_time = TO_MS(drvdata->b_stats->abl_end) - TO_MS(drvdata->b_stats->abl_start); >> + sprintf(abl_str, "%u ms", abl_time); >> + >> + pre_abl_time = TO_MS(drvdata->b_stats->abl_start); >> + sprintf(pre_abl_str, "%u ms", pre_abl_time); > > Another point from v4: > > It would be better to move the unit to the file name and include just > the number. Clarified from your first comment > >> +} >> + >> +static int boot_stats_probe(struct platform_device *pdev) >> +{ >> + char abl_str[20], pre_abl_str[20], *abl, *pre_abl; >> + struct device *bootstat_dev = &pdev->dev; >> + struct bs_data *drvdata; >> + >> + drvdata = devm_kzalloc(bootstat_dev, sizeof(*drvdata), GFP_KERNEL); >> + if (!drvdata) >> + return dev_err_probe(bootstat_dev, -ENOMEM, "failed to allocate memory"); >> + platform_set_drvdata(pdev, drvdata); >> + >> + drvdata->b_stats = devm_of_iomap(bootstat_dev, bootstat_dev->of_node, 0, NULL); >> + if (IS_ERR(drvdata->b_stats)) >> + return dev_err_probe(bootstat_dev, PTR_ERR(drvdata->b_stats), >> + "failed to map imem region"); >> + >> + drvdata->dbg_dir = debugfs_create_dir("qcom_boot_stats", NULL); >> + if (IS_ERR(drvdata->dbg_dir)) >> + return dev_err_probe(bootstat_dev, PTR_ERR(drvdata->dbg_dir), >> + "failed to create debugfs directory"); >> + >> + populate_boot_stats(abl_str, pre_abl_str, drvdata); >> + abl = abl_str; >> + pre_abl = pre_abl_str; >> + >> + debugfs_create_str("pre_abl_time", 0400, drvdata->dbg_dir, (char **)&pre_abl); >> + debugfs_create_str("abl_time", 0400, drvdata->dbg_dir, (char **)&abl); >> + >> + return 0; >> +} >> + >> +void boot_stats_remove(struct platform_device *pdev) >> +{ >> + struct bs_data *drvdata = platform_get_drvdata(pdev); >> + >> + debugfs_remove_recursive(drvdata->dbg_dir); >> +} >> + >> +static const struct of_device_id boot_stats_dt_match[] = { >> + { .compatible = "qcom,imem-bootstats" }, >> + { } >> +}; >> +MODULE_DEVICE_TABLE(of, boot_stats_dt_match); >> + >> +static struct platform_driver boot_stat_driver = { >> + .probe = boot_stats_probe, >> + .remove_new = 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.17.1 >> > >
On Tue, 9 May 2023 at 15:27, Souradeep Chowdhury <quic_schowdhu@quicinc.com> wrote: > > > > On 5/9/2023 5:09 PM, Dmitry Baryshkov wrote: > > On Tue, 9 May 2023 at 13:53, Souradeep Chowdhury > > <quic_schowdhu@quicinc.com> wrote: > >> diff --git a/drivers/soc/qcom/boot_stats.c b/drivers/soc/qcom/boot_stats.c > >> new file mode 100644 > >> index 000000000000..ca67b6b5d8eb > >> --- /dev/null > >> +++ b/drivers/soc/qcom/boot_stats.c > >> @@ -0,0 +1,100 @@ > >> +// 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) > > > > Quoting v4 question, which got no answer: > > > > Some of the platforms DTs define 32KHz clock instead of 32.768 KHz > > What should be the divisor in this case? > > This is the standard divisor used to calculate the pre_abl and abl times > across most QCOM SoCs. Can you give an example where the sleep_stat > counter has a different frequency? Following SoCs declare 37000 as sleep_clk frequency: ipq6018, qdu1000, qru1000, sc7280, sm6125, sm6375, sm8350, sm8450, sm8550. This might be an error in the dtsi, or might be not. > > > > >> + > >> +/** > >> + * 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; > >> + > >> +struct bs_data { > >> + struct boot_stats __iomem *b_stats; > >> + struct dentry *dbg_dir; > >> +}; > >> + > >> +static void populate_boot_stats(char *abl_str, char *pre_abl_str, struct bs_data *drvdata) > >> +{ > >> + u32 abl_time, pre_abl_time; > >> + > >> + abl_time = TO_MS(drvdata->b_stats->abl_end) - TO_MS(drvdata->b_stats->abl_start); > >> + sprintf(abl_str, "%u ms", abl_time); > >> + > >> + pre_abl_time = TO_MS(drvdata->b_stats->abl_start); > >> + sprintf(pre_abl_str, "%u ms", pre_abl_time); > > > > Another point from v4: > > > > It would be better to move the unit to the file name and include just > > the number. > > Clarified from your first comment > > > > >> +} > >> + > >> +static int boot_stats_probe(struct platform_device *pdev) > >> +{ > >> + char abl_str[20], pre_abl_str[20], *abl, *pre_abl; > >> + struct device *bootstat_dev = &pdev->dev; > >> + struct bs_data *drvdata; > >> + > >> + drvdata = devm_kzalloc(bootstat_dev, sizeof(*drvdata), GFP_KERNEL); > >> + if (!drvdata) > >> + return dev_err_probe(bootstat_dev, -ENOMEM, "failed to allocate memory"); > >> + platform_set_drvdata(pdev, drvdata); > >> + > >> + drvdata->b_stats = devm_of_iomap(bootstat_dev, bootstat_dev->of_node, 0, NULL); > >> + if (IS_ERR(drvdata->b_stats)) > >> + return dev_err_probe(bootstat_dev, PTR_ERR(drvdata->b_stats), > >> + "failed to map imem region"); > >> + > >> + drvdata->dbg_dir = debugfs_create_dir("qcom_boot_stats", NULL); > >> + if (IS_ERR(drvdata->dbg_dir)) > >> + return dev_err_probe(bootstat_dev, PTR_ERR(drvdata->dbg_dir), > >> + "failed to create debugfs directory"); > >> + > >> + populate_boot_stats(abl_str, pre_abl_str, drvdata); > >> + abl = abl_str; > >> + pre_abl = pre_abl_str; > >> + > >> + debugfs_create_str("pre_abl_time", 0400, drvdata->dbg_dir, (char **)&pre_abl); > >> + debugfs_create_str("abl_time", 0400, drvdata->dbg_dir, (char **)&abl); > >> + > >> + return 0; > >> +} > >> + > >> +void boot_stats_remove(struct platform_device *pdev) > >> +{ > >> + struct bs_data *drvdata = platform_get_drvdata(pdev); > >> + > >> + debugfs_remove_recursive(drvdata->dbg_dir); > >> +} > >> + > >> +static const struct of_device_id boot_stats_dt_match[] = { > >> + { .compatible = "qcom,imem-bootstats" }, > >> + { } > >> +}; > >> +MODULE_DEVICE_TABLE(of, boot_stats_dt_match); > >> + > >> +static struct platform_driver boot_stat_driver = { > >> + .probe = boot_stats_probe, > >> + .remove_new = 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.17.1 > >> > > > >
On 5/9/2023 6:31 PM, Dmitry Baryshkov wrote: > On Tue, 9 May 2023 at 15:27, Souradeep Chowdhury > <quic_schowdhu@quicinc.com> wrote: >> >> >> >> On 5/9/2023 5:09 PM, Dmitry Baryshkov wrote: >>> On Tue, 9 May 2023 at 13:53, Souradeep Chowdhury >>> <quic_schowdhu@quicinc.com> wrote: > > >>>> diff --git a/drivers/soc/qcom/boot_stats.c b/drivers/soc/qcom/boot_stats.c >>>> new file mode 100644 >>>> index 000000000000..ca67b6b5d8eb >>>> --- /dev/null >>>> +++ b/drivers/soc/qcom/boot_stats.c >>>> @@ -0,0 +1,100 @@ >>>> +// 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) >>> >>> Quoting v4 question, which got no answer: >>> >>> Some of the platforms DTs define 32KHz clock instead of 32.768 KHz >>> What should be the divisor in this case? >> >> This is the standard divisor used to calculate the pre_abl and abl times >> across most QCOM SoCs. Can you give an example where the sleep_stat >> counter has a different frequency? > > Following SoCs declare 37000 as sleep_clk frequency: > ipq6018, qdu1000, qru1000, sc7280, sm6125, sm6375, sm8350, sm8450, sm8550. > > This might be an error in the dtsi, or might be not. This sleep_clk is different from the sleep_stats counter of the module power manager(MPM) which is used to log the timestamps. This driver is tested and verified with sm8450(waipio) which uses the same frequency(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; >>>> + >>>> +struct bs_data { >>>> + struct boot_stats __iomem *b_stats; >>>> + struct dentry *dbg_dir; >>>> +}; >>>> + >>>> +static void populate_boot_stats(char *abl_str, char *pre_abl_str, struct bs_data *drvdata) >>>> +{ >>>> + u32 abl_time, pre_abl_time; >>>> + >>>> + abl_time = TO_MS(drvdata->b_stats->abl_end) - TO_MS(drvdata->b_stats->abl_start); >>>> + sprintf(abl_str, "%u ms", abl_time); >>>> + >>>> + pre_abl_time = TO_MS(drvdata->b_stats->abl_start); >>>> + sprintf(pre_abl_str, "%u ms", pre_abl_time); >>> >>> Another point from v4: >>> >>> It would be better to move the unit to the file name and include just >>> the number. >> >> Clarified from your first comment >> >>> >>>> +} >>>> + >>>> +static int boot_stats_probe(struct platform_device *pdev) >>>> +{ >>>> + char abl_str[20], pre_abl_str[20], *abl, *pre_abl; >>>> + struct device *bootstat_dev = &pdev->dev; >>>> + struct bs_data *drvdata; >>>> + >>>> + drvdata = devm_kzalloc(bootstat_dev, sizeof(*drvdata), GFP_KERNEL); >>>> + if (!drvdata) >>>> + return dev_err_probe(bootstat_dev, -ENOMEM, "failed to allocate memory"); >>>> + platform_set_drvdata(pdev, drvdata); >>>> + >>>> + drvdata->b_stats = devm_of_iomap(bootstat_dev, bootstat_dev->of_node, 0, NULL); >>>> + if (IS_ERR(drvdata->b_stats)) >>>> + return dev_err_probe(bootstat_dev, PTR_ERR(drvdata->b_stats), >>>> + "failed to map imem region"); >>>> + >>>> + drvdata->dbg_dir = debugfs_create_dir("qcom_boot_stats", NULL); >>>> + if (IS_ERR(drvdata->dbg_dir)) >>>> + return dev_err_probe(bootstat_dev, PTR_ERR(drvdata->dbg_dir), >>>> + "failed to create debugfs directory"); >>>> + >>>> + populate_boot_stats(abl_str, pre_abl_str, drvdata); >>>> + abl = abl_str; >>>> + pre_abl = pre_abl_str; >>>> + >>>> + debugfs_create_str("pre_abl_time", 0400, drvdata->dbg_dir, (char **)&pre_abl); >>>> + debugfs_create_str("abl_time", 0400, drvdata->dbg_dir, (char **)&abl); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +void boot_stats_remove(struct platform_device *pdev) >>>> +{ >>>> + struct bs_data *drvdata = platform_get_drvdata(pdev); >>>> + >>>> + debugfs_remove_recursive(drvdata->dbg_dir); >>>> +} >>>> + >>>> +static const struct of_device_id boot_stats_dt_match[] = { >>>> + { .compatible = "qcom,imem-bootstats" }, >>>> + { } >>>> +}; >>>> +MODULE_DEVICE_TABLE(of, boot_stats_dt_match); >>>> + >>>> +static struct platform_driver boot_stat_driver = { >>>> + .probe = boot_stats_probe, >>>> + .remove_new = 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.17.1 >>>> >>> >>> > > >
On Tue, May 09, 2023 at 03:52:22AM -0700, 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/qcom_boot_stats # cat abl_time > 17898 ms > /sys/kernel/debug/qcom_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 > I would have preferred some way to implement this without spending countless kB of RAM to occasionally read out two u32 values... But pulling them out of /dev/mem is the only suggestion that comes to mind... Perhaps dropping the MODULE_DEVICE_TABLE() to rely on an explicit modprobe/insmod in the few cases where it's needed? @Arnd, do you have any suggestion about how to handle this kind of debug drivers? > Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com> > --- > .../ABI/testing/debugfs-driver-bootstat | 17 +++ > drivers/soc/qcom/Kconfig | 10 ++ > drivers/soc/qcom/Makefile | 1 + > drivers/soc/qcom/boot_stats.c | 100 ++++++++++++++++++ > 4 files changed, 128 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 000000000000..7127d15d9f15 > --- /dev/null > +++ b/Documentation/ABI/testing/debugfs-driver-bootstat > @@ -0,0 +1,17 @@ > +What: /sys/kernel/debug/qcom_boot_stats/pre_abl_time > +Date: May 2023 > +Contact: Souradeep Chowdhury <quic_schowdhu@quicinc.com> > +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/qcom_boot_stats/abl_time > +Date: May 2023 > +Contact: Souradeep Chowdhury <quic_schowdhu@quicinc.com> > +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 a491718f8064..04141236dcdd 100644 > --- a/drivers/soc/qcom/Kconfig > +++ b/drivers/soc/qcom/Kconfig > @@ -16,6 +16,16 @@ 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 > + depends on DEBUG_FS > + help > + This option enables driver support for boot stats. Boot stat driver logs > + the kernel bootloader information by accessing the imem region. These > + information are 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 0f43a88b4894..ae7bda96a539 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 000000000000..ca67b6b5d8eb > --- /dev/null > +++ b/drivers/soc/qcom/boot_stats.c > @@ -0,0 +1,100 @@ > +// 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; > + > +struct bs_data { > + struct boot_stats __iomem *b_stats; > + struct dentry *dbg_dir; > +}; > + > +static void populate_boot_stats(char *abl_str, char *pre_abl_str, struct bs_data *drvdata) > +{ > + u32 abl_time, pre_abl_time; > + > + abl_time = TO_MS(drvdata->b_stats->abl_end) - TO_MS(drvdata->b_stats->abl_start); > + sprintf(abl_str, "%u ms", abl_time); > + > + pre_abl_time = TO_MS(drvdata->b_stats->abl_start); > + sprintf(pre_abl_str, "%u ms", pre_abl_time); > +} > + > +static int boot_stats_probe(struct platform_device *pdev) > +{ > + char abl_str[20], pre_abl_str[20], *abl, *pre_abl; > + struct device *bootstat_dev = &pdev->dev; > + struct bs_data *drvdata; > + > + drvdata = devm_kzalloc(bootstat_dev, sizeof(*drvdata), GFP_KERNEL); > + if (!drvdata) > + return dev_err_probe(bootstat_dev, -ENOMEM, "failed to allocate memory"); > + platform_set_drvdata(pdev, drvdata); > + > + drvdata->b_stats = devm_of_iomap(bootstat_dev, bootstat_dev->of_node, 0, NULL); You don't use this region past probe, so no need to keep it mapped, or hang onto the pointer. This means that you don't need struct bs_data, you can just stuff the dentry pointer directly in the drvdata. > + if (IS_ERR(drvdata->b_stats)) > + return dev_err_probe(bootstat_dev, PTR_ERR(drvdata->b_stats), > + "failed to map imem region"); > + > + drvdata->dbg_dir = debugfs_create_dir("qcom_boot_stats", NULL); > + if (IS_ERR(drvdata->dbg_dir)) Please omit error handling in the debugfs api. > + return dev_err_probe(bootstat_dev, PTR_ERR(drvdata->dbg_dir), > + "failed to create debugfs directory"); > + > + populate_boot_stats(abl_str, pre_abl_str, drvdata); > + abl = abl_str; > + pre_abl = pre_abl_str; > + > + debugfs_create_str("pre_abl_time", 0400, drvdata->dbg_dir, (char **)&pre_abl); abl lives on the stack, pre_abl is a pointer to the stack, &pre_abl is a pointer to this pointer and if I read the code correctly, in __debugfs_create_file this value is stored in inode->i_private. So I think this will only work if your stack isn't resused... Regards, Bjorn > + debugfs_create_str("abl_time", 0400, drvdata->dbg_dir, (char **)&abl); > + > + return 0; > +} > + > +void boot_stats_remove(struct platform_device *pdev) > +{ > + struct bs_data *drvdata = platform_get_drvdata(pdev); > + > + debugfs_remove_recursive(drvdata->dbg_dir); > +} > + > +static const struct of_device_id boot_stats_dt_match[] = { > + { .compatible = "qcom,imem-bootstats" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, boot_stats_dt_match); > + > +static struct platform_driver boot_stat_driver = { > + .probe = boot_stats_probe, > + .remove_new = 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.17.1 >
On 5/11/2023 10:07 AM, Bjorn Andersson wrote: > On Tue, May 09, 2023 at 03:52:22AM -0700, 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/qcom_boot_stats # cat abl_time >> 17898 ms >> /sys/kernel/debug/qcom_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 >> > > I would have preferred some way to implement this without spending > countless kB of RAM to occasionally read out two u32 values... If this is just for the debug build, why we are caring of it. There are various others kernel features we enable to debug the system crashes which consumes lot of RAM as well. Optimizations are good, but not sure if we need to overdo it when it is just for debugging. ---Trilok Soni
On 5/11/2023 10:37 PM, Bjorn Andersson wrote: > On Tue, May 09, 2023 at 03:52:22AM -0700, 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/qcom_boot_stats # cat abl_time >> 17898 ms >> /sys/kernel/debug/qcom_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 >> > > I would have preferred some way to implement this without spending > countless kB of RAM to occasionally read out two u32 values... > > But pulling them out of /dev/mem is the only suggestion that comes to > mind... Perhaps dropping the MODULE_DEVICE_TABLE() to rely on an > explicit modprobe/insmod in the few cases where it's needed? > > @Arnd, do you have any suggestion about how to handle this kind of debug > drivers? > >> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com> >> --- >> .../ABI/testing/debugfs-driver-bootstat | 17 +++ >> drivers/soc/qcom/Kconfig | 10 ++ >> drivers/soc/qcom/Makefile | 1 + >> drivers/soc/qcom/boot_stats.c | 100 ++++++++++++++++++ >> 4 files changed, 128 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 000000000000..7127d15d9f15 >> --- /dev/null >> +++ b/Documentation/ABI/testing/debugfs-driver-bootstat >> @@ -0,0 +1,17 @@ >> +What: /sys/kernel/debug/qcom_boot_stats/pre_abl_time >> +Date: May 2023 >> +Contact: Souradeep Chowdhury <quic_schowdhu@quicinc.com> >> +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/qcom_boot_stats/abl_time >> +Date: May 2023 >> +Contact: Souradeep Chowdhury <quic_schowdhu@quicinc.com> >> +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 a491718f8064..04141236dcdd 100644 >> --- a/drivers/soc/qcom/Kconfig >> +++ b/drivers/soc/qcom/Kconfig >> @@ -16,6 +16,16 @@ 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 >> + depends on DEBUG_FS >> + help >> + This option enables driver support for boot stats. Boot stat driver logs >> + the kernel bootloader information by accessing the imem region. These >> + information are 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 0f43a88b4894..ae7bda96a539 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 000000000000..ca67b6b5d8eb >> --- /dev/null >> +++ b/drivers/soc/qcom/boot_stats.c >> @@ -0,0 +1,100 @@ >> +// 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; >> + >> +struct bs_data { >> + struct boot_stats __iomem *b_stats; >> + struct dentry *dbg_dir; >> +}; >> + >> +static void populate_boot_stats(char *abl_str, char *pre_abl_str, struct bs_data *drvdata) >> +{ >> + u32 abl_time, pre_abl_time; >> + >> + abl_time = TO_MS(drvdata->b_stats->abl_end) - TO_MS(drvdata->b_stats->abl_start); >> + sprintf(abl_str, "%u ms", abl_time); >> + >> + pre_abl_time = TO_MS(drvdata->b_stats->abl_start); >> + sprintf(pre_abl_str, "%u ms", pre_abl_time); >> +} >> + >> +static int boot_stats_probe(struct platform_device *pdev) >> +{ >> + char abl_str[20], pre_abl_str[20], *abl, *pre_abl; >> + struct device *bootstat_dev = &pdev->dev; >> + struct bs_data *drvdata; >> + >> + drvdata = devm_kzalloc(bootstat_dev, sizeof(*drvdata), GFP_KERNEL); >> + if (!drvdata) >> + return dev_err_probe(bootstat_dev, -ENOMEM, "failed to allocate memory"); >> + platform_set_drvdata(pdev, drvdata); >> + >> + drvdata->b_stats = devm_of_iomap(bootstat_dev, bootstat_dev->of_node, 0, NULL); > > You don't use this region past probe, so no need to keep it mapped, or > hang onto the pointer. > > This means that you don't need struct bs_data, you can just stuff the > dentry pointer directly in the drvdata. Ack > >> + if (IS_ERR(drvdata->b_stats)) >> + return dev_err_probe(bootstat_dev, PTR_ERR(drvdata->b_stats), >> + "failed to map imem region"); >> + >> + drvdata->dbg_dir = debugfs_create_dir("qcom_boot_stats", NULL); >> + if (IS_ERR(drvdata->dbg_dir)) > > Please omit error handling in the debugfs api. Ack > >> + return dev_err_probe(bootstat_dev, PTR_ERR(drvdata->dbg_dir), >> + "failed to create debugfs directory"); >> + >> + populate_boot_stats(abl_str, pre_abl_str, drvdata); >> + abl = abl_str; >> + pre_abl = pre_abl_str; >> + >> + debugfs_create_str("pre_abl_time", 0400, drvdata->dbg_dir, (char **)&pre_abl); > > abl lives on the stack, pre_abl is a pointer to the stack, &pre_abl is a > pointer to this pointer and if I read the code correctly, in > __debugfs_create_file this value is stored in inode->i_private. > > So I think this will only work if your stack isn't resused... Ack > > Regards, > Bjorn > >> + debugfs_create_str("abl_time", 0400, drvdata->dbg_dir, (char **)&abl); >> + >> + return 0; >> +} >> + >> +void boot_stats_remove(struct platform_device *pdev) >> +{ >> + struct bs_data *drvdata = platform_get_drvdata(pdev); >> + >> + debugfs_remove_recursive(drvdata->dbg_dir); >> +} >> + >> +static const struct of_device_id boot_stats_dt_match[] = { >> + { .compatible = "qcom,imem-bootstats" }, >> + { } >> +}; >> +MODULE_DEVICE_TABLE(of, boot_stats_dt_match); >> + >> +static struct platform_driver boot_stat_driver = { >> + .probe = boot_stats_probe, >> + .remove_new = 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.17.1 >>
On 5/9/2023 3:52 AM, 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/qcom_boot_stats # cat abl_time > 17898 ms > /sys/kernel/debug/qcom_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 <quic_schowdhu@quicinc.com> > --- > .../ABI/testing/debugfs-driver-bootstat | 17 +++ > drivers/soc/qcom/Kconfig | 10 ++ > drivers/soc/qcom/Makefile | 1 + > drivers/soc/qcom/boot_stats.c | 100 ++++++++++++++++++ > 4 files changed, 128 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 000000000000..7127d15d9f15 > --- /dev/null > +++ b/Documentation/ABI/testing/debugfs-driver-bootstat > @@ -0,0 +1,17 @@ > +What: /sys/kernel/debug/qcom_boot_stats/pre_abl_time > +Date: May 2023 > +Contact: Souradeep Chowdhury <quic_schowdhu@quicinc.com> > +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/qcom_boot_stats/abl_time > +Date: May 2023 > +Contact: Souradeep Chowdhury <quic_schowdhu@quicinc.com> > +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 a491718f8064..04141236dcdd 100644 > --- a/drivers/soc/qcom/Kconfig > +++ b/drivers/soc/qcom/Kconfig > @@ -16,6 +16,16 @@ 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 > + depends on DEBUG_FS > + help > + This option enables driver support for boot stats. Boot stat > driver logs > + the kernel bootloader information by accessing the imem region. > These > + information are 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 0f43a88b4894..ae7bda96a539 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 000000000000..ca67b6b5d8eb > --- /dev/null > +++ b/drivers/soc/qcom/boot_stats.c > @@ -0,0 +1,100 @@ > +// 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; > + > +struct bs_data { > + struct boot_stats __iomem *b_stats; > + struct dentry *dbg_dir; > +}; > + > +static void populate_boot_stats(char *abl_str, char *pre_abl_str, struct > bs_data *drvdata) > +{ > + u32 abl_time, pre_abl_time; > + > + abl_time = TO_MS(drvdata->b_stats->abl_end) - > TO_MS(drvdata->b_stats->abl_start); > + sprintf(abl_str, "%u ms", abl_time); > + > + pre_abl_time = TO_MS(drvdata->b_stats->abl_start); > + sprintf(pre_abl_str, "%u ms", pre_abl_time); > +} > + > +static int boot_stats_probe(struct platform_device *pdev) > +{ > + char abl_str[20], pre_abl_str[20], *abl, *pre_abl; > + struct device *bootstat_dev = &pdev->dev; > + struct bs_data *drvdata; > + > + drvdata = devm_kzalloc(bootstat_dev, sizeof(*drvdata), > GFP_KERNEL); > + if (!drvdata) > + return dev_err_probe(bootstat_dev, -ENOMEM, "failed to > allocate memory"); > + platform_set_drvdata(pdev, drvdata); > + > + drvdata->b_stats = devm_of_iomap(bootstat_dev, > bootstat_dev->of_node, 0, NULL); > + if (IS_ERR(drvdata->b_stats)) > + return dev_err_probe(bootstat_dev, > PTR_ERR(drvdata->b_stats), > + "failed to map imem region"); > + > + drvdata->dbg_dir = debugfs_create_dir("qcom_boot_stats", NULL); > + if (IS_ERR(drvdata->dbg_dir)) > + return dev_err_probe(bootstat_dev, > PTR_ERR(drvdata->dbg_dir), > + "failed to create debugfs > directory"); > + > + populate_boot_stats(abl_str, pre_abl_str, drvdata); > + abl = abl_str; > + pre_abl = pre_abl_str; > + > + debugfs_create_str("pre_abl_time", 0400, drvdata->dbg_dir, (char > **)&pre_abl); > + debugfs_create_str("abl_time", 0400, drvdata->dbg_dir, (char > **)&abl); We would need these stats in Android "user" builds as well and debugfs won't be available in Android "user" builds. Please see below article. Can we move to sysfs instead of debugfs? https://source.android.com/docs/core/architecture/kernel/using-debugfs-12 > + > + return 0; > +} > + > +void boot_stats_remove(struct platform_device *pdev) > +{ > + struct bs_data *drvdata = platform_get_drvdata(pdev); > + > + debugfs_remove_recursive(drvdata->dbg_dir); > +} > + > +static const struct of_device_id boot_stats_dt_match[] = { > + { .compatible = "qcom,imem-bootstats" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, boot_stats_dt_match); > + > +static struct platform_driver boot_stat_driver = { > + .probe = boot_stats_probe, > + .remove_new = 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");
On Tue, May 9, 2023, at 12:52, 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/qcom_boot_stats # cat abl_time > 17898 ms > /sys/kernel/debug/qcom_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 <quic_schowdhu@quicinc.com> > --- > .../ABI/testing/debugfs-driver-bootstat | 17 +++ > drivers/soc/qcom/Kconfig | 10 ++ > drivers/soc/qcom/Makefile | 1 + > drivers/soc/qcom/boot_stats.c | 100 ++++++++++++++++++ As mentioned in my reply to the binding, I don't think this should be a driver at all. On top of that, even if it was a driver, it is clearly not a "soc" driver since nothing in it has any relevance to the hardware, rather than the first-stage loader, and drivers/soc/ drivers should never have their own user space interface either. Arnd
On 16/05/2023 11:19, Arnd Bergmann wrote: > On Tue, May 9, 2023, at 12:52, 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/qcom_boot_stats # cat abl_time >> 17898 ms >> /sys/kernel/debug/qcom_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 <quic_schowdhu@quicinc.com> >> --- >> .../ABI/testing/debugfs-driver-bootstat | 17 +++ >> drivers/soc/qcom/Kconfig | 10 ++ >> drivers/soc/qcom/Makefile | 1 + >> drivers/soc/qcom/boot_stats.c | 100 ++++++++++++++++++ > > As mentioned in my reply to the binding, I don't think this should > be a driver at all. On top of that, even if it was a driver, it is > clearly not a "soc" driver since nothing in it has any relevance to > the hardware, rather than the first-stage loader, and drivers/soc/ > drivers should never have their own user space interface either. I suppose that we should add a proper driver for imem rather than always using it through syscon.