[PATCH V6 0/3] soc: qcom: boot_stats: Add driver support for boot_stats

[PATCH V6 0/3] soc: qcom: boot_stats: Add driver support for boot_stats

[Souradeep Chowdhury]

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/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". Both these
values are read up by the driver from IMEM region and printed as above.

Changes in V6

*Implemented the comments on V4 and V5 of the patch

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

 .../ABI/testing/debugfs-driver-bootstat       |  17 +++
 .../devicetree/bindings/sram/qcom,imem.yaml   |  22 ++++
 MAINTAINERS                                   |   7 ++
 drivers/soc/qcom/Kconfig                      |  10 ++
 drivers/soc/qcom/Makefile                     |   1 +
 drivers/soc/qcom/boot_stats.c                 | 100 ++++++++++++++++++
 6 files changed, 157 insertions(+)
 create mode 100644 Documentation/ABI/testing/debugfs-driver-bootstat
 create mode 100644 drivers/soc/qcom/boot_stats.c

-- 
2.17.1

[PATCH V6 1/3] dt-bindings: sram: qcom,imem: Add Boot Stat region within IMEM

[Souradeep Chowdhury]

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 <quic_schowdhu@quicinc.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../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 0548e8e0d30b..bb884c5c8952 100644
--- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
+++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
@@ -50,6 +50,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.
+
+    additionalProperties: false
+
+    properties:
+      compatible:
+        items:
+          - enum:
+              - qcom,sm8450-bootstats
+          - const: qcom,imem-bootstats
+
+      reg:
+        maxItems: 1
+
+    required:
+      - compatible
+      - reg
+
 required:
   - compatible
   - reg
-- 
2.17.1

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

[Souradeep Chowdhury]

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);
+
+	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

[PATCH V6 3/3] MAINTAINERS: Add the entry for boot_stats driver support

[Souradeep Chowdhury]

Add the entries for all the files added as a part of driver support for
boot stats.

Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 7e0b87d5aa2e..076891632f68 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17182,6 +17182,13 @@ L:	netdev@vger.kernel.org
 S:	Supported
 F:	drivers/net/ipa/
 
+QCOM BOOT_STATS DRIVER
+M:	Souradeep Chowdhury <quic_schowdhu@quicinc.com>
+L:	linux-arm-msm@vger.kernel.org
+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 <somlo@cmu.edu>
 M:	"Michael S. Tsirkin" <mst@redhat.com>
-- 
2.17.1

Re: [PATCH V6 1/3] dt-bindings: sram: qcom,imem: Add Boot Stat region within IMEM

[Dmitry Baryshkov]

On Tue, 9 May 2023 at 13:53, Souradeep Chowdhury
<quic_schowdhu@quicinc.com> wrote:
>
> 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.

I might have a minor vote here. Is there any reason why you have to
instantiate the device from DT?
It looks like a software interface. Ideally software should not be
described in DT (e.g. this can be instantiated from imem
driver-to-be).
Or we can follow the RPM master-stats approach, where the device is a
top-level device, having handle pointers to the sram regions.

>
> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
>  .../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 0548e8e0d30b..bb884c5c8952 100644
> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> @@ -50,6 +50,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.
> +
> +    additionalProperties: false
> +
> +    properties:
> +      compatible:
> +        items:
> +          - enum:
> +              - qcom,sm8450-bootstats
> +          - const: qcom,imem-bootstats
> +
> +      reg:
> +        maxItems: 1
> +
> +    required:
> +      - compatible
> +      - reg
> +
>  required:
>    - compatible
>    - reg
> --
> 2.17.1
>


-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

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
>


-- 
With best wishes
Dmitry

Re: [PATCH V6 1/3] dt-bindings: sram: qcom,imem: Add Boot Stat region within IMEM

[Souradeep Chowdhury]

On 5/9/2023 5:05 PM, Dmitry Baryshkov wrote:
> On Tue, 9 May 2023 at 13:53, Souradeep Chowdhury
> <quic_schowdhu@quicinc.com> wrote:
>>
>> 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.
> 
> I might have a minor vote here. Is there any reason why you have to
> instantiate the device from DT?
> It looks like a software interface. Ideally software should not be
> described in DT (e.g. this can be instantiated from imem
> driver-to-be).
> Or we can follow the RPM master-stats approach, where the device is a
> top-level device, having handle pointers to the sram regions.

This is a dedicated region of IMEM reserved for storing stats related 
information. So it is represented as a child of IMEM, please
refer to Documentation/devicetree/bindings/sram/sram.yaml which
follows a similar philosophy. Also since this is a child of IMEM with
a specific purpose, does it not warrant a dedicated driver?

> 
>>
>> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> ---
>>   .../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 0548e8e0d30b..bb884c5c8952 100644
>> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>> @@ -50,6 +50,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.
>> +
>> +    additionalProperties: false
>> +
>> +    properties:
>> +      compatible:
>> +        items:
>> +          - enum:
>> +              - qcom,sm8450-bootstats
>> +          - const: qcom,imem-bootstats
>> +
>> +      reg:
>> +        maxItems: 1
>> +
>> +    required:
>> +      - compatible
>> +      - reg
>> +
>>   required:
>>     - compatible
>>     - reg
>> --
>> 2.17.1
>>
> 
> 

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

[Souradeep Chowdhury]

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
>>
> 
> 

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

[Dmitry Baryshkov]

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
> >>
> >
> >



-- 
With best wishes
Dmitry

Re: [PATCH V6 1/3] dt-bindings: sram: qcom,imem: Add Boot Stat region within IMEM

[Dmitry Baryshkov]

On Tue, 9 May 2023 at 15:21, Souradeep Chowdhury
<quic_schowdhu@quicinc.com> wrote:
>
>
>
> On 5/9/2023 5:05 PM, Dmitry Baryshkov wrote:
> > On Tue, 9 May 2023 at 13:53, Souradeep Chowdhury
> > <quic_schowdhu@quicinc.com> wrote:
> >>
> >> 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.
> >
> > I might have a minor vote here. Is there any reason why you have to
> > instantiate the device from DT?
> > It looks like a software interface. Ideally software should not be
> > described in DT (e.g. this can be instantiated from imem
> > driver-to-be).
> > Or we can follow the RPM master-stats approach, where the device is a
> > top-level device, having handle pointers to the sram regions.
>
> This is a dedicated region of IMEM reserved for storing stats related
> information. So it is represented as a child of IMEM, please
> refer to Documentation/devicetree/bindings/sram/sram.yaml which
> follows a similar philosophy. Also since this is a child of IMEM with
> a specific purpose, does it not warrant a dedicated driver?

I do not question a dedicated driver. I was asking about the DT node.
Even the mentioned bindings file describes the SRAM regions inside the
SRAM, rather than a proper device to be instantiated in the SRAM node.
I'd point to the boot_stats discussions (present on the list in the
last several months).

>
> >
> >>
> >> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> >> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >> ---
> >>   .../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 0548e8e0d30b..bb884c5c8952 100644
> >> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> >> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> >> @@ -50,6 +50,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.
> >> +
> >> +    additionalProperties: false
> >> +
> >> +    properties:
> >> +      compatible:
> >> +        items:
> >> +          - enum:
> >> +              - qcom,sm8450-bootstats
> >> +          - const: qcom,imem-bootstats
> >> +
> >> +      reg:
> >> +        maxItems: 1
> >> +
> >> +    required:
> >> +      - compatible
> >> +      - reg
> >> +
> >>   required:
> >>     - compatible
> >>     - reg
> >> --
> >> 2.17.1
> >>
> >
> >



-- 
With best wishes
Dmitry

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

[kernel test robot]

Hi Souradeep,

kernel test robot noticed the following build warnings:

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

url:    https://github.com/intel-lab-lkp/linux/commits/Souradeep-Chowdhury/dt-bindings-sram-qcom-imem-Add-Boot-Stat-region-within-IMEM/20230509-185332
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/35863b47c04c2edd7ae49c57d23682aba6111d4f.1683628357.git.quic_schowdhu%40quicinc.com
patch subject: [PATCH V6 2/3] soc: qcom: boot_stat: Add Driver Support for Boot Stats
config: sparc-allyesconfig (https://download.01.org/0day-ci/archive/20230509/202305092107.0Wy1eeCn-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/7883e99da179c8b49f6834b84a91259b03679e56
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Souradeep-Chowdhury/dt-bindings-sram-qcom-imem-Add-Boot-Stat-region-within-IMEM/20230509-185332
        git checkout 7883e99da179c8b49f6834b84a91259b03679e56
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc SHELL=/bin/bash drivers/soc/qcom/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202305092107.0Wy1eeCn-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/soc/qcom/boot_stats.c:76:6: warning: no previous prototype for 'boot_stats_remove' [-Wmissing-prototypes]
      76 | void boot_stats_remove(struct platform_device *pdev)
         |      ^~~~~~~~~~~~~~~~~


vim +/boot_stats_remove +76 drivers/soc/qcom/boot_stats.c

    75	
  > 76	void boot_stats_remove(struct platform_device *pdev)
    77	{
    78		struct bs_data *drvdata = platform_get_drvdata(pdev);
    79	
    80		debugfs_remove_recursive(drvdata->dbg_dir);
    81	}
    82	

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

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

[kernel test robot]

Hi Souradeep,

kernel test robot noticed the following build warnings:

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

url:    https://github.com/intel-lab-lkp/linux/commits/Souradeep-Chowdhury/dt-bindings-sram-qcom-imem-Add-Boot-Stat-region-within-IMEM/20230509-185332
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/35863b47c04c2edd7ae49c57d23682aba6111d4f.1683628357.git.quic_schowdhu%40quicinc.com
patch subject: [PATCH V6 2/3] soc: qcom: boot_stat: Add Driver Support for Boot Stats
config: hexagon-allyesconfig (https://download.01.org/0day-ci/archive/20230510/202305101517.ld1hBheg-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project b0fb98227c90adf2536c9ad644a74d5e92961111)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/7883e99da179c8b49f6834b84a91259b03679e56
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Souradeep-Chowdhury/dt-bindings-sram-qcom-imem-Add-Boot-Stat-region-within-IMEM/20230509-185332
        git checkout 7883e99da179c8b49f6834b84a91259b03679e56
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/hwmon/ drivers/net/can/usb/ drivers/soc/qcom/ drivers/watchdog/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202305101517.ld1hBheg-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/soc/qcom/boot_stats.c:9:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
                                                     ^
   In file included from drivers/soc/qcom/boot_stats.c:9:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
                                                     ^
   In file included from drivers/soc/qcom/boot_stats.c:9:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
>> drivers/soc/qcom/boot_stats.c:76:6: warning: no previous prototype for function 'boot_stats_remove' [-Wmissing-prototypes]
   void boot_stats_remove(struct platform_device *pdev)
        ^
   drivers/soc/qcom/boot_stats.c:76:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void boot_stats_remove(struct platform_device *pdev)
   ^
   static 
   7 warnings generated.


vim +/boot_stats_remove +76 drivers/soc/qcom/boot_stats.c

    75	
  > 76	void boot_stats_remove(struct platform_device *pdev)
    77	{
    78		struct bs_data *drvdata = platform_get_drvdata(pdev);
    79	
    80		debugfs_remove_recursive(drvdata->dbg_dir);
    81	}
    82	

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

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

[Souradeep Chowdhury]

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
>>>>
>>>
>>>
> 
> 
> 

Re: [PATCH V6 1/3] dt-bindings: sram: qcom,imem: Add Boot Stat region within IMEM

[Souradeep Chowdhury]

On 5/9/2023 6:35 PM, Dmitry Baryshkov wrote:
> On Tue, 9 May 2023 at 15:21, Souradeep Chowdhury
> <quic_schowdhu@quicinc.com> wrote:
>>
>>
>>
>> On 5/9/2023 5:05 PM, Dmitry Baryshkov wrote:
>>> On Tue, 9 May 2023 at 13:53, Souradeep Chowdhury
>>> <quic_schowdhu@quicinc.com> wrote:
>>>>
>>>> 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.
>>>
>>> I might have a minor vote here. Is there any reason why you have to
>>> instantiate the device from DT?
>>> It looks like a software interface. Ideally software should not be
>>> described in DT (e.g. this can be instantiated from imem
>>> driver-to-be).
>>> Or we can follow the RPM master-stats approach, where the device is a
>>> top-level device, having handle pointers to the sram regions.
>>
>> This is a dedicated region of IMEM reserved for storing stats related
>> information. So it is represented as a child of IMEM, please
>> refer to Documentation/devicetree/bindings/sram/sram.yaml which
>> follows a similar philosophy. Also since this is a child of IMEM with
>> a specific purpose, does it not warrant a dedicated driver?
> 
> I do not question a dedicated driver. I was asking about the DT node.
> Even the mentioned bindings file describes the SRAM regions inside the
> SRAM, rather than a proper device to be instantiated in the SRAM node.
> I'd point to the boot_stats discussions (present on the list in the
> last several months).
> 

Ack. Will instantiate the device from the parent node in the driver and
access the stats region to print the boot_stats information.


>>
>>>
>>>>
>>>> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
>>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>> ---
>>>>    .../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 0548e8e0d30b..bb884c5c8952 100644
>>>> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>>>> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>>>> @@ -50,6 +50,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.
>>>> +
>>>> +    additionalProperties: false
>>>> +
>>>> +    properties:
>>>> +      compatible:
>>>> +        items:
>>>> +          - enum:
>>>> +              - qcom,sm8450-bootstats
>>>> +          - const: qcom,imem-bootstats
>>>> +
>>>> +      reg:
>>>> +        maxItems: 1
>>>> +
>>>> +    required:
>>>> +      - compatible
>>>> +      - reg
>>>> +
>>>>    required:
>>>>      - compatible
>>>>      - reg
>>>> --
>>>> 2.17.1
>>>>
>>>
>>>
> 
> 
> 

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

[Bjorn Andersson]

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
> 

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

[Trilok Soni]

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

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

[Souradeep Chowdhury]

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
>>

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

[Satya Durga Srinivasu Prabhala]

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");

Re: [PATCH V6 1/3] dt-bindings: sram: qcom,imem: Add Boot Stat region within IMEM

[Arnd Bergmann]

On Tue, May 9, 2023, at 13:35, Dmitry Baryshkov wrote:
> On Tue, 9 May 2023 at 13:53, Souradeep Chowdhury
> <quic_schowdhu@quicinc.com> wrote:
>>
>> 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.
>
> I might have a minor vote here. Is there any reason why you have to
> instantiate the device from DT?
> It looks like a software interface. Ideally software should not be
> described in DT (e.g. this can be instantiated from imem
> driver-to-be).

There is nothing wrong with describing firmware in DT, if that
firmware is part of the platform, we do that for a lot of
other bits of firmware.

However, in this specific case, many things are wrong with the
implementation, and neither the DT binding nor the driver
makes sense to me in its current state.

>> +  "^stats@[0-9a-f]+$":
>> +    type: object
>> +    description:
>> +      Imem region dedicated for storing timestamps related
>> +      information regarding bootstats.
>> +
>> +    additionalProperties: false
>> +
>> +    properties:
>> +      compatible:
>> +        items:
>> +          - enum:
>> +              - qcom,sm8450-bootstats
>> +          - const: qcom,imem-bootstats
>> +
>> +      reg:
>> +        maxItems: 1

If I understand this right, this "qcom,imem-bootstats"
device serves as an indirection to store additional
properties of the system in a memory area, but the description
of that area is more complex than its contents, which
makes no sense to me.

Just create a binding for a firmware node in the devicetree
itself, and put the values in properties of that. The first
stage firmware can still use the same interface, but the
actual loader that assembles the DT can get it out of that
and store it in the properties. With that done, there is also
no need for a kernel driver, as userspace can just get the
values from /sys/firmware/devicetree/ directly.

      Arnd

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

[Arnd Bergmann]

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

Re: [PATCH V6 1/3] dt-bindings: sram: qcom,imem: Add Boot Stat region within IMEM

[Trilok Soni]

On 5/16/2023 1:16 AM, Arnd Bergmann wrote:
> On Tue, May 9, 2023, at 13:35, Dmitry Baryshkov wrote:
>> On Tue, 9 May 2023 at 13:53, Souradeep Chowdhury
>> <quic_schowdhu@quicinc.com> wrote:
>>>
>>> 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.
>>
>> I might have a minor vote here. Is there any reason why you have to
>> instantiate the device from DT?
>> It looks like a software interface. Ideally software should not be
>> described in DT (e.g. this can be instantiated from imem
>> driver-to-be).
> 
> There is nothing wrong with describing firmware in DT, if that
> firmware is part of the platform, we do that for a lot of
> other bits of firmware.
> 
> However, in this specific case, many things are wrong with the
> implementation, and neither the DT binding nor the driver
> makes sense to me in its current state.
> 
>>> +  "^stats@[0-9a-f]+$":
>>> +    type: object
>>> +    description:
>>> +      Imem region dedicated for storing timestamps related
>>> +      information regarding bootstats.
>>> +
>>> +    additionalProperties: false
>>> +
>>> +    properties:
>>> +      compatible:
>>> +        items:
>>> +          - enum:
>>> +              - qcom,sm8450-bootstats
>>> +          - const: qcom,imem-bootstats
>>> +
>>> +      reg:
>>> +        maxItems: 1
> 
> If I understand this right, this "qcom,imem-bootstats"
> device serves as an indirection to store additional
> properties of the system in a memory area, but the description
> of that area is more complex than its contents, which
> makes no sense to me.
> 
> Just create a binding for a firmware node in the devicetree
> itself, and put the values in properties of that. The first
> stage firmware can still use the same interface, but the
> actual loader that assembles the DT can get it out of that
> and store it in the properties. With that done, there is also
> no need for a kernel driver, as userspace can just get the
> values from /sys/firmware/devicetree/ directly.
> 

To understand bit better here, what you are suggesting here that
application bootloader (e.g., UEFI app for Linux) can read these
boot values from the IMEM and encode them into the devicetree properties
which can be later retrieved directly from the userspace.

I am fine with this approach as well and in this case we just
need to submit the bindings document, right?

---Trilok Soni

Re: [PATCH V6 1/3] dt-bindings: sram: qcom,imem: Add Boot Stat region within IMEM

[Arnd Bergmann]

On Tue, May 16, 2023, at 20:34, Trilok Soni wrote:
> On 5/16/2023 1:16 AM, Arnd Bergmann wrote:
>
> To understand bit better here, what you are suggesting here that
> application bootloader (e.g., UEFI app for Linux) can read these
> boot values from the IMEM and encode them into the devicetree properties
> which can be later retrieved directly from the userspace.

I'm not entirely sure I understand the concept of "application bootloader",
but in principle this can be anything that runs before the devicetree
is handed off from the final boot loader to the kernel.

> I am fine with this approach as well and in this case we just
> need to submit the bindings document, right?

Yes, exactly.

     Arnd

Re: [PATCH V6 1/3] dt-bindings: sram: qcom,imem: Add Boot Stat region within IMEM

[Dmitry Baryshkov]

On Tue, 16 May 2023 at 11:16, Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Tue, May 9, 2023, at 13:35, Dmitry Baryshkov wrote:
> > On Tue, 9 May 2023 at 13:53, Souradeep Chowdhury
> > <quic_schowdhu@quicinc.com> wrote:
> >>
> >> 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.
> >
> > I might have a minor vote here. Is there any reason why you have to
> > instantiate the device from DT?
> > It looks like a software interface. Ideally software should not be
> > described in DT (e.g. this can be instantiated from imem
> > driver-to-be).
>
> There is nothing wrong with describing firmware in DT, if that
> firmware is part of the platform, we do that for a lot of
> other bits of firmware.
>
> However, in this specific case, many things are wrong with the
> implementation, and neither the DT binding nor the driver
> makes sense to me in its current state.
>
> >> +  "^stats@[0-9a-f]+$":
> >> +    type: object
> >> +    description:
> >> +      Imem region dedicated for storing timestamps related
> >> +      information regarding bootstats.
> >> +
> >> +    additionalProperties: false
> >> +
> >> +    properties:
> >> +      compatible:
> >> +        items:
> >> +          - enum:
> >> +              - qcom,sm8450-bootstats
> >> +          - const: qcom,imem-bootstats
> >> +
> >> +      reg:
> >> +        maxItems: 1
>
> If I understand this right, this "qcom,imem-bootstats"
> device serves as an indirection to store additional
> properties of the system in a memory area, but the description
> of that area is more complex than its contents, which
> makes no sense to me.
>
> Just create a binding for a firmware node in the devicetree
> itself, and put the values in properties of that. The first
> stage firmware can still use the same interface, but the
> actual loader that assembles the DT can get it out of that
> and store it in the properties. With that done, there is also
> no need for a kernel driver, as userspace can just get the
> values from /sys/firmware/devicetree/ directly.

This sounds good, except the always-present issue of the devices which
have already been released. Usually we can not expect a bootloader
update for these devices.

-- 
With best wishes
Dmitry

Re: [PATCH V6 1/3] dt-bindings: sram: qcom,imem: Add Boot Stat region within IMEM

[Trilok Soni]

On 5/16/2023 12:17 PM, Dmitry Baryshkov wrote:
> On Tue, 16 May 2023 at 11:16, Arnd Bergmann <arnd@arndb.de> wrote:
>>
>> On Tue, May 9, 2023, at 13:35, Dmitry Baryshkov wrote:
>>> On Tue, 9 May 2023 at 13:53, Souradeep Chowdhury
>>> <quic_schowdhu@quicinc.com> wrote:
>>>>
>>>> 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.
>>>
>>> I might have a minor vote here. Is there any reason why you have to
>>> instantiate the device from DT?
>>> It looks like a software interface. Ideally software should not be
>>> described in DT (e.g. this can be instantiated from imem
>>> driver-to-be).
>>
>> There is nothing wrong with describing firmware in DT, if that
>> firmware is part of the platform, we do that for a lot of
>> other bits of firmware.
>>
>> However, in this specific case, many things are wrong with the
>> implementation, and neither the DT binding nor the driver
>> makes sense to me in its current state.
>>
>>>> +  "^stats@[0-9a-f]+$":
>>>> +    type: object
>>>> +    description:
>>>> +      Imem region dedicated for storing timestamps related
>>>> +      information regarding bootstats.
>>>> +
>>>> +    additionalProperties: false
>>>> +
>>>> +    properties:
>>>> +      compatible:
>>>> +        items:
>>>> +          - enum:
>>>> +              - qcom,sm8450-bootstats
>>>> +          - const: qcom,imem-bootstats
>>>> +
>>>> +      reg:
>>>> +        maxItems: 1
>>
>> If I understand this right, this "qcom,imem-bootstats"
>> device serves as an indirection to store additional
>> properties of the system in a memory area, but the description
>> of that area is more complex than its contents, which
>> makes no sense to me.
>>
>> Just create a binding for a firmware node in the devicetree
>> itself, and put the values in properties of that. The first
>> stage firmware can still use the same interface, but the
>> actual loader that assembles the DT can get it out of that
>> and store it in the properties. With that done, there is also
>> no need for a kernel driver, as userspace can just get the
>> values from /sys/firmware/devicetree/ directly.
> 
> This sounds good, except the always-present issue of the devices which
> have already been released. Usually we can not expect a bootloader
> update for these devices.

Valid point. I don't expect current SOCs supported at upstream to update 
with the newer bootloader having this feature done through bootloader.

---Trilok Soni

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

[Dmitry Baryshkov]

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.

-- 
With best wishes
Dmitry