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

[PATCH V2 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 print the information. This information is
useful in verifying if existing boot KPIs have regressed or not.
A sample log in SM8450(waipio) device is as follows:-

KPI: Pre ABL Time = 3s
KPI: ABL Time = 14s
KPI: Kernel MPM timestamp = 890206

The Module Power Manager(MPM) sleep counter starts ticking at the PBL
stage and the timestamp generated by the sleep counter is logged by
the Qualcomm proprietary bootloader(ABL) at two points-> First when it
starts execution which is logged here as "Pre ABL Time" and the second
when it is about to load the kernel logged as "ABL Time". Both these
values are read up by the driver from IMEM region and printed as above.
The current sleep counter timestamp is also logged by the driver.

Changes in V2

*Implemented comments on version 1 of the patch

*Changed the boot_stats driver to module_platform_driver

*Dropped the DT binding for mpm sleep counter

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

 .../devicetree/bindings/sram/qcom,imem.yaml        | 21 +++++
 MAINTAINERS                                        |  6 ++
 drivers/soc/qcom/Kconfig                           |  7 ++
 drivers/soc/qcom/Makefile                          |  1 +
 drivers/soc/qcom/boot_stats.c                      | 95 ++++++++++++++++++++++
 5 files changed, 130 insertions(+)
 create mode 100644 drivers/soc/qcom/boot_stats.c

-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V2 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>
---
 .../devicetree/bindings/sram/qcom,imem.yaml         | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
index 665c06e..9998d65 100644
--- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
+++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
@@ -26,6 +26,7 @@ properties:
           - qcom,sdm845-imem
           - qcom,sdx55-imem
           - qcom,sdx65-imem
+          - qcom,sm8450-imem
       - const: syscon
       - const: simple-mfd
 
@@ -48,6 +49,26 @@ patternProperties:
     $ref: /schemas/remoteproc/qcom,pil-info.yaml#
     description: Peripheral image loader relocation region
 
+  "^boot-stat@[0-9a-f]+$":
+    type: object
+    description:
+      Imem region dedicated for storing timestamps related
+      information regarding bootstats.
+
+    properties:
+      compatible:
+        items:
+          - enum:
+              - qcom,sm8450-bootstats
+          - const: qcom,imem-bootstats
+
+      reg:
+        maxItems: 1
+
+    required:
+      - compatible
+      - reg
+
 required:
   - compatible
   - reg
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V2 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 print the information. This information is
useful in verifying if the existing boot KPIs have regressed or not.
A sample log in SM8450(waipio) device is as follows:-

KPI: Pre ABL Time = 3s
KPI: ABL Time = 14s
KPI: Kernel MPM timestamp = 890206

The Module Power Manager(MPM) sleep counter starts ticking at the PBL
stage and the timestamp generated by the sleep counter is logged by
the Qualcomm proprietary bootloader(ABL) at two points-> First when it
starts execution which is logged here as "Pre ABL Time" and the second
when it is about to load the kernel logged as "ABL Time". Both are
logged in the unit of seconds. The current kernel timestamp is
printed by the boot_stats driver as well.

Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
---
 drivers/soc/qcom/Kconfig      |  7 ++++
 drivers/soc/qcom/Makefile     |  1 +
 drivers/soc/qcom/boot_stats.c | 95 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 103 insertions(+)
 create mode 100644 drivers/soc/qcom/boot_stats.c

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index d11bda2..2cfdbb7 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -79,6 +79,13 @@ config QCOM_DCC
 	  driver provides interface to configure DCC block and read back
 	  captured data from DCC's internal SRAM.
 
+config QCOM_BOOTSTAT
+	tristate "Qualcomm Technologies, Boot Stat driver"
+	depends on ARCH_QCOM || COMPILE_TEST
+	help
+	  This option enables driver for boot stats. Boot stat driver prints
+	  the kernel bootloader information by accessing the imem region.
+
 config QCOM_KRYO_L2_ACCESSORS
 	bool
 	depends on ARCH_QCOM && ARM64 || COMPILE_TEST
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 3b92c6c..8a9d995 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_QCOM_GENI_SE) +=	qcom-geni-se.o
 obj-$(CONFIG_QCOM_COMMAND_DB) += cmd-db.o
 obj-$(CONFIG_QCOM_CPR)		+= cpr.o
 obj-$(CONFIG_QCOM_DCC) += dcc.o
+obj-$(CONFIG_QCOM_BOOTSTAT) += boot_stats_new.o
 obj-$(CONFIG_QCOM_GSBI)	+=	qcom_gsbi.o
 obj-$(CONFIG_QCOM_MDT_LOADER)	+= mdt_loader.o
 obj-$(CONFIG_QCOM_OCMEM)	+= ocmem.o
diff --git a/drivers/soc/qcom/boot_stats.c b/drivers/soc/qcom/boot_stats.c
new file mode 100644
index 0000000..080e820
--- /dev/null
+++ b/drivers/soc/qcom/boot_stats.c
@@ -0,0 +1,95 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2013-2019, 2021 The Linux Foundation. All rights reserved.
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+
+#define MPM_COUNTER_FREQ 32768
+
+/**
+ *  struct boot_stats - timestamp information related to boot stats
+ *  @bootloader_start:  Time for the starting point of the abl bootloader
+ *  @bootloader_end:    Time when the kernel starts loading from abl bootloader
+ */
+struct boot_stats {
+	u32 bootloader_start;
+	u32 bootloader_end;
+} __packed;
+
+struct boot_stats __iomem *boot_stats;
+void __iomem *mpm_counter_base;
+
+static void print_boot_stats(void)
+{
+	u32 pre_abl_time = readl_relaxed(&boot_stats->bootloader_start) / MPM_COUNTER_FREQ;
+	u32 abl_time = readl_relaxed(&boot_stats->bootloader_end) / MPM_COUNTER_FREQ;
+
+	pr_info("KPI: Pre ABL Time = %us\n", pre_abl_time);
+	pr_info("KPI: ABL Time = %us\n", abl_time);
+	pr_info("KPI: Kernel MPM timestamp = %u\n", readl_relaxed(mpm_counter_base));
+}
+
+static int boot_stats_probe(struct platform_device *pdev)
+{
+	struct device_node *np_mpm2;
+	struct device *boot_stat = &pdev->dev;
+
+	boot_stats = of_iomap(boot_stat->of_node->child, 0);
+	if (!boot_stats)
+		return dev_err_probe(&pdev->dev, -ENOMEM,
+					"failed to map imem region\n");
+
+	np_mpm2 = of_find_compatible_node(NULL, NULL,
+					  "qcom,mpm2-sleep-counter");
+	if (!np_mpm2) {
+		return dev_err_probe(&pdev->dev, -EINVAL,
+				     "failed to get the counter node\n");
+	}
+
+	if (of_get_address(np_mpm2, 0, NULL, NULL)) {
+		mpm_counter_base = of_iomap(np_mpm2, 0);
+		if (!mpm_counter_base) {
+			return dev_err_probe(&pdev->dev, -ENOMEM,
+					     "failed to map the counter\n");
+		}
+	}
+	print_boot_stats();
+
+	return 0;
+}
+
+static int boot_stats_remove(struct platform_device *pdev)
+{
+	iounmap(boot_stats);
+	iounmap(mpm_counter_base);
+
+	return 0;
+}
+
+static const struct of_device_id boot_stats_dt_match[] = {
+	{ .compatible = "qcom,sm8450-imem" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, boot_stats_dt_match);
+
+static struct platform_driver boot_stat_driver = {
+	.probe  = boot_stats_probe,
+	.remove = boot_stats_remove,
+	.driver = {
+		.name = "qcom-boot-stats",
+		.of_match_table = boot_stats_dt_match,
+	},
+};
+module_platform_driver(boot_stat_driver);
+
+MODULE_DESCRIPTION("Qualcomm Technologies Inc. Boot Stat driver");
+MODULE_LICENSE("GPL");
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V2 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 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index f2c3d47..c591dff 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17071,6 +17071,12 @@ 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:	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.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Bjorn Andersson]

On Fri, Apr 07, 2023 at 07:34:36PM +0530, Souradeep Chowdhury wrote:
> All of Qualcomm's proprietary Android boot-loaders capture boot time
> stats, like the time when the bootloader started execution and at what
> point the bootloader handed over control to the kernel etc. in the IMEM
> region. This information is captured in a specific format by this driver
> by mapping a structure to the IMEM memory region and then accessing the
> members of the structure to print the information. This information is
> useful in verifying if the existing boot KPIs have regressed or not.
> A sample log in SM8450(waipio) device is as follows:-
> 
> KPI: Pre ABL Time = 3s
> KPI: ABL Time = 14s

Why are these in whole seconds?

> KPI: Kernel MPM timestamp = 890206

And why is this presented in cycles?

> 
> The Module Power Manager(MPM) sleep counter starts ticking at the PBL
> stage and the timestamp generated by the sleep counter is logged by
> the Qualcomm proprietary bootloader(ABL) at two points-> First when it
> starts execution which is logged here as "Pre ABL Time" and the second
> when it is about to load the kernel logged as "ABL Time". Both are
> logged in the unit of seconds.

We have a policy to not taint the kernel log with "useless" information,
for kernel developers this seems to add no value and for end users
there's no benefit to this.

> The current kernel timestamp is
> printed by the boot_stats driver as well.
> 

Why?

> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> ---
>  drivers/soc/qcom/Kconfig      |  7 ++++
>  drivers/soc/qcom/Makefile     |  1 +
>  drivers/soc/qcom/boot_stats.c | 95 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 103 insertions(+)
>  create mode 100644 drivers/soc/qcom/boot_stats.c
> 
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index d11bda2..2cfdbb7 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -79,6 +79,13 @@ config QCOM_DCC
>  	  driver provides interface to configure DCC block and read back
>  	  captured data from DCC's internal SRAM.
>  
> +config QCOM_BOOTSTAT
> +	tristate "Qualcomm Technologies, Boot Stat driver"
> +	depends on ARCH_QCOM || COMPILE_TEST
> +	help
> +	  This option enables driver for boot stats. Boot stat driver prints
> +	  the kernel bootloader information by accessing the imem region.
> +
>  config QCOM_KRYO_L2_ACCESSORS
>  	bool
>  	depends on ARCH_QCOM && ARM64 || COMPILE_TEST
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index 3b92c6c..8a9d995 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -5,6 +5,7 @@ obj-$(CONFIG_QCOM_GENI_SE) +=	qcom-geni-se.o
>  obj-$(CONFIG_QCOM_COMMAND_DB) += cmd-db.o
>  obj-$(CONFIG_QCOM_CPR)		+= cpr.o
>  obj-$(CONFIG_QCOM_DCC) += dcc.o
> +obj-$(CONFIG_QCOM_BOOTSTAT) += boot_stats_new.o

Most other entries here are sorted alphabetically.

>  obj-$(CONFIG_QCOM_GSBI)	+=	qcom_gsbi.o
>  obj-$(CONFIG_QCOM_MDT_LOADER)	+= mdt_loader.o
>  obj-$(CONFIG_QCOM_OCMEM)	+= ocmem.o
> diff --git a/drivers/soc/qcom/boot_stats.c b/drivers/soc/qcom/boot_stats.c
> new file mode 100644
> index 0000000..080e820
> --- /dev/null
> +++ b/drivers/soc/qcom/boot_stats.c
> @@ -0,0 +1,95 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2013-2019, 2021 The Linux Foundation. All rights reserved.
> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +
> +#define MPM_COUNTER_FREQ 32768
> +
> +/**
> + *  struct boot_stats - timestamp information related to boot stats
> + *  @bootloader_start:  Time for the starting point of the abl bootloader
> + *  @bootloader_end:    Time when the kernel starts loading from abl bootloader
> + */
> +struct boot_stats {
> +	u32 bootloader_start;
> +	u32 bootloader_end;

bootloader != abl

> +} __packed;
> +
> +struct boot_stats __iomem *boot_stats;
> +void __iomem *mpm_counter_base;

Why are these non-static global variables?

> +
> +static void print_boot_stats(void)
> +{
> +	u32 pre_abl_time = readl_relaxed(&boot_stats->bootloader_start) / MPM_COUNTER_FREQ;
> +	u32 abl_time = readl_relaxed(&boot_stats->bootloader_end) / MPM_COUNTER_FREQ;
> +
> +	pr_info("KPI: Pre ABL Time = %us\n", pre_abl_time);
> +	pr_info("KPI: ABL Time = %us\n", abl_time);
> +	pr_info("KPI: Kernel MPM timestamp = %u\n", readl_relaxed(mpm_counter_base));

This number is completely dependent on link order and other things in
happening in the kernel, so what trust do you give in this number going
up or down?

As above, why is this presented in ticks?

> +}
> +
> +static int boot_stats_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np_mpm2;
> +	struct device *boot_stat = &pdev->dev;
> +
> +	boot_stats = of_iomap(boot_stat->of_node->child, 0);

You can't just do ->child here, what if boot stats isn't the first item
in the list?

> +	if (!boot_stats)
> +		return dev_err_probe(&pdev->dev, -ENOMEM,
> +					"failed to map imem region\n");
> +
> +	np_mpm2 = of_find_compatible_node(NULL, NULL,
> +					  "qcom,mpm2-sleep-counter");
> +	if (!np_mpm2) {
> +		return dev_err_probe(&pdev->dev, -EINVAL,
> +				     "failed to get the counter node\n");
> +	}
> +
> +	if (of_get_address(np_mpm2, 0, NULL, NULL)) {
> +		mpm_counter_base = of_iomap(np_mpm2, 0);

Isn't this region going to be also accessed by some sleep stats driver?

> +		if (!mpm_counter_base) {
> +			return dev_err_probe(&pdev->dev, -ENOMEM,
> +					     "failed to map the counter\n");
> +		}
> +	}
> +	print_boot_stats();

You're done with your platform_device, and your two ioremap regions
here. But you're holding on to those 10kb of memory for the rest of the
systems runtime.

> +
> +	return 0;
> +}
> +
> +static int boot_stats_remove(struct platform_device *pdev)
> +{
> +	iounmap(boot_stats);
> +	iounmap(mpm_counter_base);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id boot_stats_dt_match[] = {
> +	{ .compatible = "qcom,sm8450-imem" },

You're binding to the root imem node, rather than your boot stats child
node.

How about just exposing the boot stats imem region to userspace, through
debugfs or similar and then you can have a userspace tool that digs out
and reports this information when profiling is relevant?

Regards,
Bjorn

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, boot_stats_dt_match);
> +
> +static struct platform_driver boot_stat_driver = {
> +	.probe  = boot_stats_probe,
> +	.remove = boot_stats_remove,
> +	.driver = {
> +		.name = "qcom-boot-stats",
> +		.of_match_table = boot_stats_dt_match,
> +	},
> +};
> +module_platform_driver(boot_stat_driver);
> +
> +MODULE_DESCRIPTION("Qualcomm Technologies Inc. Boot Stat driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.7.4
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Bjorn Andersson]

On Fri, Apr 07, 2023 at 07:34:34PM +0530, Souradeep Chowdhury wrote:
> Qualcomm's proprietary Android boot-loaders capture boot time
> stats, like the time when the bootloader started execution and at what
> point the bootloader handed over control to the kernel etc. in the IMEM
> region. This information is captured in a specific format by this driver
> by mapping a structure to the IMEM memory region and then accessing the
> members of the structure to print the information. This information is
> useful in verifying if existing boot KPIs have regressed or not.
> A sample log in SM8450(waipio) device is as follows:-
> 
> KPI: Pre ABL Time = 3s
> KPI: ABL Time = 14s
> KPI: Kernel MPM timestamp = 890206
> 
> The Module Power Manager(MPM) sleep counter starts ticking at the PBL
> stage and the timestamp generated by the sleep counter is logged by
> the Qualcomm proprietary bootloader(ABL) at two points-> First when it
> starts execution which is logged here as "Pre ABL Time" and the second
> when it is about to load the kernel logged as "ABL Time". Both these
> values are read up by the driver from IMEM region and printed as above.
> The current sleep counter timestamp is also logged by the driver.
> 
> Changes in V2
> 
> *Implemented comments on version 1 of the patch

This doesn't give a particular good indication of what you actually
changed. So please spell it out going forward.

Thanks,
Bjorn

> 
> *Changed the boot_stats driver to module_platform_driver
> 
> *Dropped the DT binding for mpm sleep counter
> 
> 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
> 
>  .../devicetree/bindings/sram/qcom,imem.yaml        | 21 +++++
>  MAINTAINERS                                        |  6 ++
>  drivers/soc/qcom/Kconfig                           |  7 ++
>  drivers/soc/qcom/Makefile                          |  1 +
>  drivers/soc/qcom/boot_stats.c                      | 95 ++++++++++++++++++++++
>  5 files changed, 130 insertions(+)
>  create mode 100644 drivers/soc/qcom/boot_stats.c
> 
> -- 
> 2.7.4
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Souradeep Chowdhury]

On 4/7/2023 9:11 PM, Bjorn Andersson wrote:
> On Fri, Apr 07, 2023 at 07:34:36PM +0530, Souradeep Chowdhury wrote:
>> All of Qualcomm's proprietary Android boot-loaders capture boot time
>> stats, like the time when the bootloader started execution and at what
>> point the bootloader handed over control to the kernel etc. in the IMEM
>> region. This information is captured in a specific format by this driver
>> by mapping a structure to the IMEM memory region and then accessing the
>> members of the structure to print the information. This information is
>> useful in verifying if the existing boot KPIs have regressed or not.
>> A sample log in SM8450(waipio) device is as follows:-
>>
>> KPI: Pre ABL Time = 3s
>> KPI: ABL Time = 14s
> 
> Why are these in whole seconds?

This is to give a granular view of time.

> 
>> KPI: Kernel MPM timestamp = 890206
> 
> And why is this presented in cycles?

This timestamp is used as an intermediate value for calculating one of 
the KPIs. Can be changed to seconds as well for consistency.

> 
>>
>> The Module Power Manager(MPM) sleep counter starts ticking at the PBL
>> stage and the timestamp generated by the sleep counter is logged by
>> the Qualcomm proprietary bootloader(ABL) at two points-> First when it
>> starts execution which is logged here as "Pre ABL Time" and the second
>> when it is about to load the kernel logged as "ABL Time". Both are
>> logged in the unit of seconds.
> 
> We have a policy to not taint the kernel log with "useless" information,
> for kernel developers this seems to add no value and for end users
> there's no benefit to this.
> 
>> The current kernel timestamp is
>> printed by the boot_stats driver as well.
>>
> 
> Why?

Same as stated above.


> 
>> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
>> ---
>>   drivers/soc/qcom/Kconfig      |  7 ++++
>>   drivers/soc/qcom/Makefile     |  1 +
>>   drivers/soc/qcom/boot_stats.c | 95 +++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 103 insertions(+)
>>   create mode 100644 drivers/soc/qcom/boot_stats.c
>>
>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>> index d11bda2..2cfdbb7 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -79,6 +79,13 @@ config QCOM_DCC
>>   	  driver provides interface to configure DCC block and read back
>>   	  captured data from DCC's internal SRAM.
>>   
>> +config QCOM_BOOTSTAT
>> +	tristate "Qualcomm Technologies, Boot Stat driver"
>> +	depends on ARCH_QCOM || COMPILE_TEST
>> +	help
>> +	  This option enables driver for boot stats. Boot stat driver prints
>> +	  the kernel bootloader information by accessing the imem region.
>> +
>>   config QCOM_KRYO_L2_ACCESSORS
>>   	bool
>>   	depends on ARCH_QCOM && ARM64 || COMPILE_TEST
>> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
>> index 3b92c6c..8a9d995 100644
>> --- a/drivers/soc/qcom/Makefile
>> +++ b/drivers/soc/qcom/Makefile
>> @@ -5,6 +5,7 @@ obj-$(CONFIG_QCOM_GENI_SE) +=	qcom-geni-se.o
>>   obj-$(CONFIG_QCOM_COMMAND_DB) += cmd-db.o
>>   obj-$(CONFIG_QCOM_CPR)		+= cpr.o
>>   obj-$(CONFIG_QCOM_DCC) += dcc.o
>> +obj-$(CONFIG_QCOM_BOOTSTAT) += boot_stats_new.o
> 
> Most other entries here are sorted alphabetically.

Ack

> 
>>   obj-$(CONFIG_QCOM_GSBI)	+=	qcom_gsbi.o
>>   obj-$(CONFIG_QCOM_MDT_LOADER)	+= mdt_loader.o
>>   obj-$(CONFIG_QCOM_OCMEM)	+= ocmem.o
>> diff --git a/drivers/soc/qcom/boot_stats.c b/drivers/soc/qcom/boot_stats.c
>> new file mode 100644
>> index 0000000..080e820
>> --- /dev/null
>> +++ b/drivers/soc/qcom/boot_stats.c
>> @@ -0,0 +1,95 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2013-2019, 2021 The Linux Foundation. All rights reserved.
>> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/init.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +
>> +#define MPM_COUNTER_FREQ 32768
>> +
>> +/**
>> + *  struct boot_stats - timestamp information related to boot stats
>> + *  @bootloader_start:  Time for the starting point of the abl bootloader
>> + *  @bootloader_end:    Time when the kernel starts loading from abl bootloader
>> + */
>> +struct boot_stats {
>> +	u32 bootloader_start;
>> +	u32 bootloader_end;
> 
> bootloader != abl

Ack. Will change these names based on abl.

> 
>> +} __packed;
>> +
>> +struct boot_stats __iomem *boot_stats;
>> +void __iomem *mpm_counter_base;
> 
> Why are these non-static global variables?

This was removed as per the comment on version 1 of the patch. Will 
reinstate it.

> 
>> +
>> +static void print_boot_stats(void)
>> +{
>> +	u32 pre_abl_time = readl_relaxed(&boot_stats->bootloader_start) / MPM_COUNTER_FREQ;
>> +	u32 abl_time = readl_relaxed(&boot_stats->bootloader_end) / MPM_COUNTER_FREQ;
>> +
>> +	pr_info("KPI: Pre ABL Time = %us\n", pre_abl_time);
>> +	pr_info("KPI: ABL Time = %us\n", abl_time);
>> +	pr_info("KPI: Kernel MPM timestamp = %u\n", readl_relaxed(mpm_counter_base));
> 
> This number is completely dependent on link order and other things in
> happening in the kernel, so what trust do you give in this number going
> up or down?
> 
> As above, why is this presented in ticks?

This is the sleep counter for module power manager unit which starts 
ticking from PBL stage, it will keep on ticking till the system shuts 
down. As mentioned above this timestamp value is used at an intermediate
point for calculating one of the KPIs, we can change it to seconds for 
consistency.

> 
>> +}
>> +
>> +static int boot_stats_probe(struct platform_device *pdev)
>> +{
>> +	struct device_node *np_mpm2;
>> +	struct device *boot_stat = &pdev->dev;
>> +
>> +	boot_stats = of_iomap(boot_stat->of_node->child, 0);
> 
> You can't just do ->child here, what if boot stats isn't the first item
> in the list?

Ack

> 
>> +	if (!boot_stats)
>> +		return dev_err_probe(&pdev->dev, -ENOMEM,
>> +					"failed to map imem region\n");
>> +
>> +	np_mpm2 = of_find_compatible_node(NULL, NULL,
>> +					  "qcom,mpm2-sleep-counter");
>> +	if (!np_mpm2) {
>> +		return dev_err_probe(&pdev->dev, -EINVAL,
>> +				     "failed to get the counter node\n");
>> +	}
>> +
>> +	if (of_get_address(np_mpm2, 0, NULL, NULL)) {
>> +		mpm_counter_base = of_iomap(np_mpm2, 0);
> 
> Isn't this region going to be also accessed by some sleep stats driver?

Currently there is no sleep stat driver present for this upstream.

> 
>> +		if (!mpm_counter_base) {
>> +			return dev_err_probe(&pdev->dev, -ENOMEM,
>> +					     "failed to map the counter\n");
>> +		}
>> +	}
>> +	print_boot_stats();
> 
> You're done with your platform_device, and your two ioremap regions
> here. But you're holding on to those 10kb of memory for the rest of the
> systems runtime.

Ack

> 
>> +
>> +	return 0;
>> +}
>> +
>> +static int boot_stats_remove(struct platform_device *pdev)
>> +{
>> +	iounmap(boot_stats);
>> +	iounmap(mpm_counter_base);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id boot_stats_dt_match[] = {
>> +	{ .compatible = "qcom,sm8450-imem" },
> 
> You're binding to the root imem node, rather than your boot stats child
> node.

Ack

> 
> How about just exposing the boot stats imem region to userspace, through
> debugfs or similar and then you can have a userspace tool that digs out
> and reports this information when profiling is relevant?

Ack. This can be exposed to user as a part of debugfs interface.

> 
> Regards,
> Bjorn
> 
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, boot_stats_dt_match);
>> +
>> +static struct platform_driver boot_stat_driver = {
>> +	.probe  = boot_stats_probe,
>> +	.remove = boot_stats_remove,
>> +	.driver = {
>> +		.name = "qcom-boot-stats",
>> +		.of_match_table = boot_stats_dt_match,
>> +	},
>> +};
>> +module_platform_driver(boot_stat_driver);
>> +
>> +MODULE_DESCRIPTION("Qualcomm Technologies Inc. Boot Stat driver");
>> +MODULE_LICENSE("GPL");
>> -- 
>> 2.7.4
>>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Krzysztof Kozlowski]

On 07/04/2023 16:04, Souradeep Chowdhury 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.
> 
> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> ---
>  .../devicetree/bindings/sram/qcom,imem.yaml         | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> index 665c06e..9998d65 100644
> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> @@ -26,6 +26,7 @@ properties:
>            - qcom,sdm845-imem
>            - qcom,sdx55-imem
>            - qcom,sdx65-imem
> +          - qcom,sm8450-imem
>        - const: syscon
>        - const: simple-mfd
>  
> @@ -48,6 +49,26 @@ patternProperties:
>      $ref: /schemas/remoteproc/qcom,pil-info.yaml#
>      description: Peripheral image loader relocation region
>  
> +  "^boot-stat@[0-9a-f]+$":

Konrad,
Just like for RPM Master stats, didn't we want to call these just "stats"?

https://lore.kernel.org/linux-arm-msm/20230405-topic-master_stats-v2-1-51c304ecb610@linaro.org/

> +    type: object
> +    description:
> +      Imem region dedicated for storing timestamps related
> +      information regarding bootstats.

Description is okay, but you ignored the rest.

This is a friendly reminder during the review process.

It seems my previous comments were not fully addressed. Maybe my
feedback got lost between the quotes, maybe you just forgot to apply it.
Please go back to the previous discussion and either implement all
requested changes or keep discussing them.

Thank you.


Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Krzysztof Kozlowski]

On 10/04/2023 08:48, Souradeep Chowdhury wrote:
> 
> 
> On 4/7/2023 9:11 PM, Bjorn Andersson wrote:
>> On Fri, Apr 07, 2023 at 07:34:36PM +0530, Souradeep Chowdhury wrote:
>>> All of Qualcomm's proprietary Android boot-loaders capture boot time
>>> stats, like the time when the bootloader started execution and at what
>>> point the bootloader handed over control to the kernel etc. in the IMEM
>>> region. This information is captured in a specific format by this driver
>>> by mapping a structure to the IMEM memory region and then accessing the
>>> members of the structure to print the information. This information is
>>> useful in verifying if the existing boot KPIs have regressed or not.
>>> A sample log in SM8450(waipio) device is as follows:-
>>>
>>> KPI: Pre ABL Time = 3s
>>> KPI: ABL Time = 14s
>>
>> Why are these in whole seconds?
> 
> This is to give a granular view of time.
> 
>>
>>> KPI: Kernel MPM timestamp = 890206
>>
>> And why is this presented in cycles?
> 
> This timestamp is used as an intermediate value for calculating one of 
> the KPIs. Can be changed to seconds as well for consistency.
> 
>>
>>>
>>> The Module Power Manager(MPM) sleep counter starts ticking at the PBL
>>> stage and the timestamp generated by the sleep counter is logged by
>>> the Qualcomm proprietary bootloader(ABL) at two points-> First when it
>>> starts execution which is logged here as "Pre ABL Time" and the second
>>> when it is about to load the kernel logged as "ABL Time". Both are
>>> logged in the unit of seconds.
>>
>> We have a policy to not taint the kernel log with "useless" information,
>> for kernel developers this seems to add no value and for end users
>> there's no benefit to this.
>>
>>> The current kernel timestamp is
>>> printed by the boot_stats driver as well.
>>>
>>
>> Why?
> 
> Same as stated above.

You did not answer. The question is "why do you think printing this
during boot is suitable for wide usage?". I don't find answer "give a
granular view of time" anyway related.

Please come with rationale why such printing should be used in Linux
kernel at all, given that as Bjorn said - we do not print
debugging/profiling information.

You should probably come with a debugfs interface for this.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Krzysztof Kozlowski]

On 07/04/2023 16:04, Souradeep Chowdhury 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.
> 
> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> ---
>  .../devicetree/bindings/sram/qcom,imem.yaml         | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> index 665c06e..9998d65 100644
> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> @@ -26,6 +26,7 @@ properties:
>            - qcom,sdm845-imem
>            - qcom,sdx55-imem
>            - qcom,sdx65-imem
> +          - qcom,sm8450-imem

Use recent Linux kernel for your work.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Krzysztof Kozlowski]

On 07/04/2023 16:04, Souradeep Chowdhury wrote:
> All of Qualcomm's proprietary Android boot-loaders capture boot time
> stats, like the time when the bootloader started execution and at what
> point the bootloader handed over control to the kernel etc. in the IMEM
> region. This information is captured in a specific format by this driver
> by mapping a structure to the IMEM memory region and then accessing the
> members of the structure to print the information. This information is
> useful in verifying if the existing boot KPIs have regressed or not.
> A sample log in SM8450(waipio) device is as follows:-
> 


> +
> +static int boot_stats_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np_mpm2;
> +	struct device *boot_stat = &pdev->dev;
> +
> +	boot_stats = of_iomap(boot_stat->of_node->child, 0);
> +	if (!boot_stats)
> +		return dev_err_probe(&pdev->dev, -ENOMEM,
> +					"failed to map imem region\n");
> +
> +	np_mpm2 = of_find_compatible_node(NULL, NULL,
> +					  "qcom,mpm2-sleep-counter");

This is undocumented and non-existing compatible. No, we cannot take this.

Also, referencing other nodes should be with phandles, not compatibles.


Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Souradeep Chowdhury]

On 4/12/2023 1:45 PM, Krzysztof Kozlowski wrote:
> On 07/04/2023 16:04, Souradeep Chowdhury 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.
>>
>> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
>> ---
>>   .../devicetree/bindings/sram/qcom,imem.yaml         | 21 +++++++++++++++++++++
>>   1 file changed, 21 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>> index 665c06e..9998d65 100644
>> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>> @@ -26,6 +26,7 @@ properties:
>>             - qcom,sdm845-imem
>>             - qcom,sdx55-imem
>>             - qcom,sdx65-imem
>> +          - qcom,sm8450-imem
>>         - const: syscon
>>         - const: simple-mfd
>>   
>> @@ -48,6 +49,26 @@ patternProperties:
>>       $ref: /schemas/remoteproc/qcom,pil-info.yaml#
>>       description: Peripheral image loader relocation region
>>   
>> +  "^boot-stat@[0-9a-f]+$":
> 
> Konrad,
> Just like for RPM Master stats, didn't we want to call these just "stats"?
> 
> https://lore.kernel.org/linux-arm-msm/20230405-topic-master_stats-v2-1-51c304ecb610@linaro.org
> 
>> +    type: object
>> +    description:
>> +      Imem region dedicated for storing timestamps related
>> +      information regarding bootstats.
> 
> Description is okay, but you ignored the rest.
> 
> This is a friendly reminder during the review process.
> 
> It seems my previous comments were not fully addressed. Maybe my
> feedback got lost between the quotes, maybe you just forgot to apply it.
> Please go back to the previous discussion and either implement all
> requested changes or keep discussing them.
> 
> Thank you.

Ack. Will be implemented in the next version.
> 
> 
> Best regards,
> Krzysztof
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Souradeep Chowdhury]

On 4/12/2023 1:47 PM, Krzysztof Kozlowski wrote:
> On 10/04/2023 08:48, Souradeep Chowdhury wrote:
>>
>>
>> On 4/7/2023 9:11 PM, Bjorn Andersson wrote:
>>> On Fri, Apr 07, 2023 at 07:34:36PM +0530, Souradeep Chowdhury wrote:
>>>> All of Qualcomm's proprietary Android boot-loaders capture boot time
>>>> stats, like the time when the bootloader started execution and at what
>>>> point the bootloader handed over control to the kernel etc. in the IMEM
>>>> region. This information is captured in a specific format by this driver
>>>> by mapping a structure to the IMEM memory region and then accessing the
>>>> members of the structure to print the information. This information is
>>>> useful in verifying if the existing boot KPIs have regressed or not.
>>>> A sample log in SM8450(waipio) device is as follows:-
>>>>
>>>> KPI: Pre ABL Time = 3s
>>>> KPI: ABL Time = 14s
>>>
>>> Why are these in whole seconds?
>>
>> This is to give a granular view of time.
>>
>>>
>>>> KPI: Kernel MPM timestamp = 890206
>>>
>>> And why is this presented in cycles?
>>
>> This timestamp is used as an intermediate value for calculating one of
>> the KPIs. Can be changed to seconds as well for consistency.
>>
>>>
>>>>
>>>> The Module Power Manager(MPM) sleep counter starts ticking at the PBL
>>>> stage and the timestamp generated by the sleep counter is logged by
>>>> the Qualcomm proprietary bootloader(ABL) at two points-> First when it
>>>> starts execution which is logged here as "Pre ABL Time" and the second
>>>> when it is about to load the kernel logged as "ABL Time". Both are
>>>> logged in the unit of seconds.
>>>
>>> We have a policy to not taint the kernel log with "useless" information,
>>> for kernel developers this seems to add no value and for end users
>>> there's no benefit to this.
>>>
>>>> The current kernel timestamp is
>>>> printed by the boot_stats driver as well.
>>>>
>>>
>>> Why?
>>
>> Same as stated above.
> 
> You did not answer. The question is "why do you think printing this
> during boot is suitable for wide usage?". I don't find answer "give a
> granular view of time" anyway related.
> 
> Please come with rationale why such printing should be used in Linux
> kernel at all, given that as Bjorn said - we do not print
> debugging/profiling information.
> 
> You should probably come with a debugfs interface for this.

Ack. The debugfs interface for this will be present in the next version.

> 
> Best regards,
> Krzysztof
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Souradeep Chowdhury]

On 4/12/2023 1:51 PM, Krzysztof Kozlowski wrote:
> On 07/04/2023 16:04, Souradeep Chowdhury wrote:
>> All of Qualcomm's proprietary Android boot-loaders capture boot time
>> stats, like the time when the bootloader started execution and at what
>> point the bootloader handed over control to the kernel etc. in the IMEM
>> region. This information is captured in a specific format by this driver
>> by mapping a structure to the IMEM memory region and then accessing the
>> members of the structure to print the information. This information is
>> useful in verifying if the existing boot KPIs have regressed or not.
>> A sample log in SM8450(waipio) device is as follows:-
>>
> 
> 
>> +
>> +static int boot_stats_probe(struct platform_device *pdev)
>> +{
>> +	struct device_node *np_mpm2;
>> +	struct device *boot_stat = &pdev->dev;
>> +
>> +	boot_stats = of_iomap(boot_stat->of_node->child, 0);
>> +	if (!boot_stats)
>> +		return dev_err_probe(&pdev->dev, -ENOMEM,
>> +					"failed to map imem region\n");
>> +
>> +	np_mpm2 = of_find_compatible_node(NULL, NULL,
>> +					  "qcom,mpm2-sleep-counter");
> 
> This is undocumented and non-existing compatible. No, we cannot take this.
> 
> Also, referencing other nodes should be with phandles, not compatibles.

Ack. This will be dropped in the next version as we are not printing the
current kernel timestamp.

> 
> 
> Best regards,
> Krzysztof
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel