| Message ID | 3f385562845ae26d519940ca8098fde89282991b.1679403696.git.quic_schowdhu@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | soc: qcom: boot_stats: Add driver support for boot_stats | expand |
On 21/03/2023 14:51, 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 print the information. This information is > useful in verifying if the existing boot KPIs have regre > +/** > + * struct boot_stats - timestamp information related to boot stats > + * @bootloader_start: Time for the starting point of the abl bootloader > + * @bootloader_end: Time when the kernel starts loading from abl bootloader > + */ > +struct boot_stats { > + u32 bootloader_start; > + u32 bootloader_end; > +} __packed; > + > +static struct boot_stats __iomem *boot_stats; > +static void __iomem *mpm_counter_base; > +static u32 mpm_counter_freq; No file-scope variables. Does not scale, not easy for review and maintenance. Avoid such code. > + > +static int mpm_parse_dt(void) > +{ > + struct device_node *np_imem, *np_mpm2; > + > + np_imem = of_find_compatible_node(NULL, NULL, > + "qcom,imem-boot_stats"); > + if (!np_imem) { > + pr_err("can't find qcom,imem node\n"); So you are printing errors everywhere, on every soc and with compile test on every platform there is in the world... sorry, it does not work like that. > + return -ENODEV; > + } > + boot_stats = of_iomap(np_imem, 0); > + if (!boot_stats) { > + pr_err("boot_stats: Can't map imem\n"); > + goto err1; > + } > + > +static void __exit boot_stats_exit(void) > +{ > +} > +module_exit(boot_stats_exit) I don't think this is some special code which deserves init calls. Make it module_platform_driver(). Best regards, Krzysztof
On 3/21/2023 11:07 PM, Krzysztof Kozlowski wrote: > On 21/03/2023 14:51, 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 print the information. This information is >> useful in verifying if the existing boot KPIs have regre > > >> +/** >> + * struct boot_stats - timestamp information related to boot stats >> + * @bootloader_start: Time for the starting point of the abl bootloader >> + * @bootloader_end: Time when the kernel starts loading from abl bootloader >> + */ >> +struct boot_stats { >> + u32 bootloader_start; >> + u32 bootloader_end; >> +} __packed; >> + >> +static struct boot_stats __iomem *boot_stats; >> +static void __iomem *mpm_counter_base; >> +static u32 mpm_counter_freq; > > No file-scope variables. Does not scale, not easy for review and > maintenance. Avoid such code. Ack > >> + >> +static int mpm_parse_dt(void) >> +{ >> + struct device_node *np_imem, *np_mpm2; >> + >> + np_imem = of_find_compatible_node(NULL, NULL, >> + "qcom,imem-boot_stats"); >> + if (!np_imem) { >> + pr_err("can't find qcom,imem node\n"); > > So you are printing errors everywhere, on every soc and with compile > test on every platform there is in the world... sorry, it does not work > like that. Ack > >> + return -ENODEV; >> + } >> + boot_stats = of_iomap(np_imem, 0); >> + if (!boot_stats) { >> + pr_err("boot_stats: Can't map imem\n"); >> + goto err1; >> + } > > >> + >> +static void __exit boot_stats_exit(void) >> +{ >> +} >> +module_exit(boot_stats_exit) > > > I don't think this is some special code which deserves init calls. Make > it module_platform_driver(). Since this just reads some values from the Imem region and prints it to the user and doesn't have a specific device associated with it, a generic module code is written for it and not a module_platform_driver(). > > > Best regards, > Krzysztof >
On 22/03/2023 14:54, Souradeep Chowdhury wrote: > > > On 3/21/2023 11:07 PM, Krzysztof Kozlowski wrote: >> On 21/03/2023 14:51, 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 print the information. This information is >>> useful in verifying if the existing boot KPIs have regre >> >> >>> +/** >>> + * struct boot_stats - timestamp information related to boot stats >>> + * @bootloader_start: Time for the starting point of the abl bootloader >>> + * @bootloader_end: Time when the kernel starts loading from abl bootloader >>> + */ >>> +struct boot_stats { >>> + u32 bootloader_start; >>> + u32 bootloader_end; >>> +} __packed; >>> + >>> +static struct boot_stats __iomem *boot_stats; >>> +static void __iomem *mpm_counter_base; >>> +static u32 mpm_counter_freq; >> >> No file-scope variables. Does not scale, not easy for review and >> maintenance. Avoid such code. > > Ack >> >>> + >>> +static int mpm_parse_dt(void) >>> +{ >>> + struct device_node *np_imem, *np_mpm2; >>> + >>> + np_imem = of_find_compatible_node(NULL, NULL, >>> + "qcom,imem-boot_stats"); >>> + if (!np_imem) { >>> + pr_err("can't find qcom,imem node\n"); >> >> So you are printing errors everywhere, on every soc and with compile >> test on every platform there is in the world... sorry, it does not work >> like that. > > Ack >> >>> + return -ENODEV; >>> + } >>> + boot_stats = of_iomap(np_imem, 0); >>> + if (!boot_stats) { >>> + pr_err("boot_stats: Can't map imem\n"); >>> + goto err1; >>> + } >> >> >>> + >>> +static void __exit boot_stats_exit(void) >>> +{ >>> +} >>> +module_exit(boot_stats_exit) >> >> >> I don't think this is some special code which deserves init calls. Make >> it module_platform_driver(). > > Since this just reads some values from the Imem region and prints it to > the user and doesn't have a specific device associated with it, a Which is not really an argument for such antipattern, but okay... > generic module code is written for it and not a module_platform_driver(). ... so how do you handle deferred probe? Best regards, Krzysztof
On 3/22/2023 8:23 PM, Krzysztof Kozlowski wrote: > On 22/03/2023 14:54, Souradeep Chowdhury wrote: >> >> >> On 3/21/2023 11:07 PM, Krzysztof Kozlowski wrote: >>> On 21/03/2023 14:51, 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 print the information. This information is >>>> useful in verifying if the existing boot KPIs have regre >>> >>> >>>> +/** >>>> + * struct boot_stats - timestamp information related to boot stats >>>> + * @bootloader_start: Time for the starting point of the abl bootloader >>>> + * @bootloader_end: Time when the kernel starts loading from abl bootloader >>>> + */ >>>> +struct boot_stats { >>>> + u32 bootloader_start; >>>> + u32 bootloader_end; >>>> +} __packed; >>>> + >>>> +static struct boot_stats __iomem *boot_stats; >>>> +static void __iomem *mpm_counter_base; >>>> +static u32 mpm_counter_freq; >>> >>> No file-scope variables. Does not scale, not easy for review and >>> maintenance. Avoid such code. >> >> Ack >>> >>>> + >>>> +static int mpm_parse_dt(void) >>>> +{ >>>> + struct device_node *np_imem, *np_mpm2; >>>> + >>>> + np_imem = of_find_compatible_node(NULL, NULL, >>>> + "qcom,imem-boot_stats"); >>>> + if (!np_imem) { >>>> + pr_err("can't find qcom,imem node\n"); >>> >>> So you are printing errors everywhere, on every soc and with compile >>> test on every platform there is in the world... sorry, it does not work >>> like that. >> >> Ack >>> >>>> + return -ENODEV; >>>> + } >>>> + boot_stats = of_iomap(np_imem, 0); >>>> + if (!boot_stats) { >>>> + pr_err("boot_stats: Can't map imem\n"); >>>> + goto err1; >>>> + } >>> >>> >>>> + >>>> +static void __exit boot_stats_exit(void) >>>> +{ >>>> +} >>>> +module_exit(boot_stats_exit) >>> >>> >>> I don't think this is some special code which deserves init calls. Make >>> it module_platform_driver(). >> >> Since this just reads some values from the Imem region and prints it to >> the user and doesn't have a specific device associated with it, a > > Which is not really an argument for such antipattern, but okay... > >> generic module code is written for it and not a module_platform_driver(). > > ... so how do you handle deferred probe? This has no dependency on other resources except that it parses some information from DT nodes, so deferred probe handling is not needed in this case. > > Best regards, > Krzysztof >
On 23/03/2023 14:45, Souradeep Chowdhury wrote: > > > On 3/22/2023 8:23 PM, Krzysztof Kozlowski wrote: >> On 22/03/2023 14:54, Souradeep Chowdhury wrote: >>> >>> >>> On 3/21/2023 11:07 PM, Krzysztof Kozlowski wrote: >>>> On 21/03/2023 14:51, 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 print the information. This information is >>>>> useful in verifying if the existing boot KPIs have regre >>>> >>>> >>>>> +/** >>>>> + * struct boot_stats - timestamp information related to boot stats >>>>> + * @bootloader_start: Time for the starting point of the abl bootloader >>>>> + * @bootloader_end: Time when the kernel starts loading from abl bootloader >>>>> + */ >>>>> +struct boot_stats { >>>>> + u32 bootloader_start; >>>>> + u32 bootloader_end; >>>>> +} __packed; >>>>> + >>>>> +static struct boot_stats __iomem *boot_stats; >>>>> +static void __iomem *mpm_counter_base; >>>>> +static u32 mpm_counter_freq; >>>> >>>> No file-scope variables. Does not scale, not easy for review and >>>> maintenance. Avoid such code. >>> >>> Ack >>>> >>>>> + >>>>> +static int mpm_parse_dt(void) >>>>> +{ >>>>> + struct device_node *np_imem, *np_mpm2; >>>>> + >>>>> + np_imem = of_find_compatible_node(NULL, NULL, >>>>> + "qcom,imem-boot_stats"); >>>>> + if (!np_imem) { >>>>> + pr_err("can't find qcom,imem node\n"); >>>> >>>> So you are printing errors everywhere, on every soc and with compile >>>> test on every platform there is in the world... sorry, it does not work >>>> like that. >>> >>> Ack >>>> >>>>> + return -ENODEV; >>>>> + } >>>>> + boot_stats = of_iomap(np_imem, 0); >>>>> + if (!boot_stats) { >>>>> + pr_err("boot_stats: Can't map imem\n"); >>>>> + goto err1; >>>>> + } >>>> >>>> >>>>> + >>>>> +static void __exit boot_stats_exit(void) >>>>> +{ >>>>> +} >>>>> +module_exit(boot_stats_exit) >>>> >>>> >>>> I don't think this is some special code which deserves init calls. Make >>>> it module_platform_driver(). >>> >>> Since this just reads some values from the Imem region and prints it to >>> the user and doesn't have a specific device associated with it, a >> >> Which is not really an argument for such antipattern, but okay... >> >>> generic module code is written for it and not a module_platform_driver(). >> >> ... so how do you handle deferred probe? > > This has no dependency on other resources except that it parses some > information from DT nodes, so deferred probe handling is not needed > in this case. Yes, I know, but if we would ever add it how this driver can handle it? This is antipattern. Best regards, Krzysztof
diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig index d11bda2..2cfdbb7 100644 --- a/drivers/soc/qcom/Kconfig +++ b/drivers/soc/qcom/Kconfig @@ -79,6 +79,13 @@ config QCOM_DCC driver provides interface to configure DCC block and read back captured data from DCC's internal SRAM. +config QCOM_BOOTSTAT + tristate "Qualcomm Technologies, Boot Stat driver" + depends on ARCH_QCOM || COMPILE_TEST + help + This option enables driver for boot stats. Boot stat driver prints + the kernel bootloader information by accessing the imem region. + config QCOM_KRYO_L2_ACCESSORS bool depends on ARCH_QCOM && ARM64 || COMPILE_TEST diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile index 3b92c6c..e9b1e52 100644 --- a/drivers/soc/qcom/Makefile +++ b/drivers/soc/qcom/Makefile @@ -5,6 +5,7 @@ obj-$(CONFIG_QCOM_GENI_SE) += qcom-geni-se.o obj-$(CONFIG_QCOM_COMMAND_DB) += cmd-db.o obj-$(CONFIG_QCOM_CPR) += cpr.o obj-$(CONFIG_QCOM_DCC) += dcc.o +obj-$(CONFIG_QCOM_BOOTSTAT) += boot_stats.o obj-$(CONFIG_QCOM_GSBI) += qcom_gsbi.o obj-$(CONFIG_QCOM_MDT_LOADER) += mdt_loader.o obj-$(CONFIG_QCOM_OCMEM) += ocmem.o diff --git a/drivers/soc/qcom/boot_stats.c b/drivers/soc/qcom/boot_stats.c new file mode 100644 index 0000000..59b5ab6 --- /dev/null +++ b/drivers/soc/qcom/boot_stats.c @@ -0,0 +1,108 @@ +// 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/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> + +/** + * struct boot_stats - timestamp information related to boot stats + * @bootloader_start: Time for the starting point of the abl bootloader + * @bootloader_end: Time when the kernel starts loading from abl bootloader + */ +struct boot_stats { + u32 bootloader_start; + u32 bootloader_end; +} __packed; + +static struct boot_stats __iomem *boot_stats; +static void __iomem *mpm_counter_base; +static u32 mpm_counter_freq; + +static int mpm_parse_dt(void) +{ + struct device_node *np_imem, *np_mpm2; + + np_imem = of_find_compatible_node(NULL, NULL, + "qcom,imem-boot_stats"); + if (!np_imem) { + pr_err("can't find qcom,imem node\n"); + return -ENODEV; + } + boot_stats = of_iomap(np_imem, 0); + if (!boot_stats) { + pr_err("boot_stats: Can't map imem\n"); + goto err1; + } + + np_mpm2 = of_find_compatible_node(NULL, NULL, + "qcom,mpm2-sleep-counter"); + if (!np_mpm2) { + pr_err("mpm_counter: can't find DT node\n"); + goto err2; + } + + if (of_property_read_u32(np_mpm2, "clock-frequency", &mpm_counter_freq)) + goto err2; + + if (of_get_address(np_mpm2, 0, NULL, NULL)) { + mpm_counter_base = of_iomap(np_mpm2, 0); + if (!mpm_counter_base) { + pr_err("mpm_counter: can't map counter base\n"); + goto err2; + } + } else { + goto err2; + } + + return 0; + +err2: + of_node_put(np_mpm2); +err1: + of_node_put(np_imem); + return -ENODEV; +} + +static void print_boot_stats(void) +{ + u32 pre_abl_time = readl_relaxed(&boot_stats->bootloader_start) / mpm_counter_freq; + u32 abl_time = readl_relaxed(&boot_stats->bootloader_end) / mpm_counter_freq; + + pr_info("KPI: Pre ABL Time = %us\n", pre_abl_time); + pr_info("KPI: ABL Time = %us\n", abl_time); + pr_info("KPI: Kernel MPM timestamp = %u\n", readl_relaxed(mpm_counter_base)); +} + +static int __init boot_stats_init(void) +{ + int ret; + + ret = mpm_parse_dt(); + if (ret < 0) + return -ENODEV; + + print_boot_stats(); + + iounmap(boot_stats); + iounmap(mpm_counter_base); + + return 0; +} +module_init(boot_stats_init); + +static void __exit boot_stats_exit(void) +{ +} +module_exit(boot_stats_exit) + +MODULE_DESCRIPTION("Qualcomm Technologies Inc. Boot Stat driver"); +MODULE_LICENSE("GPL");
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 print the information. This information is useful in verifying if the existing boot KPIs have regressed or not. A sample log in SM8450(waipio) device is as follows:- KPI: Pre ABL Time = 3s KPI: ABL Time = 14s KPI: Kernel MPM timestamp = 890206 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 are logged in the unit of seconds. The current kernel timestamp is printed by the boot_stats driver as well. Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com> --- drivers/soc/qcom/Kconfig | 7 +++ drivers/soc/qcom/Makefile | 1 + drivers/soc/qcom/boot_stats.c | 108 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 116 insertions(+) create mode 100644 drivers/soc/qcom/boot_stats.c -- 2.7.4