Re: [PATCH V3 2/3] soc: qcom: boot_stat: Add Driver Support for Boot Stats

[Souradeep Chowdhury <quic_schowdhu@xxxxxxxxxxx>]

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 <quic_schowdhu@xxxxxxxxxxx>
> > ---
> >   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 <quic_schowdhu@xxxxxxxxxxx>
> > +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 <quic_schowdhu@xxxxxxxxxxx>
> > +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