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

[PATCH V4 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/146aa6b0.boot_stats # cat abl_time
17898 ms
/sys/kernel/debug/146aa6b0.boot_stats # cat pre_abl_time
2879 ms

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

Changes in V4

*Implemented the comments on V3 of the patch series

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

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

--
2.7.4

[PATCH V4 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/146aa6b0.boot_stats # cat abl_time
17898 ms
/sys/kernel/debug/146aa6b0.boot_stats # cat pre_abl_time
2879 ms

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

Changes in V4

*Implemented the comments on V3 of the patch series

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

 Documentation/ABI/testing/debugfs-driver-bootstat  |  17 ++++
 .../devicetree/bindings/sram/qcom,imem.yaml        |  22 +++++
 MAINTAINERS                                        |   7 ++
 drivers/soc/qcom/Kconfig                           |   9 ++
 drivers/soc/qcom/Makefile                          |   1 +
 drivers/soc/qcom/boot_stats.c                      | 101 +++++++++++++++++++++
 6 files changed, 157 insertions(+)
 create mode 100644 Documentation/ABI/testing/debugfs-driver-bootstat
 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 V4 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        | 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 ba694ce..d028bed 100644
--- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
+++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
@@ -49,6 +49,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.7.4

[PATCH V4 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        | 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 ba694ce..d028bed 100644
--- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
+++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
@@ -49,6 +49,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.7.4


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

[PATCH V4 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/146aa6b0.boot_stats # cat abl_time
17898 ms
/sys/kernel/debug/146aa6b0.boot_stats # cat pre_abl_time
2879 ms

The Module Power Manager(MPM) sleep counter starts ticking at the PBL
stage and the timestamp generated by the sleep counter is logged by
the Qualcomm proprietary bootloader(ABL) at two points-> First when it
starts execution which is logged here as "pre_abl_time" and the second
when it is about to load the kernel logged as "abl_time". Documentation
details are also added in Documentation/ABI/testing/debugfs-driver-bootstat

Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
---
 Documentation/ABI/testing/debugfs-driver-bootstat |  17 ++++
 drivers/soc/qcom/Kconfig                          |   9 ++
 drivers/soc/qcom/Makefile                         |   1 +
 drivers/soc/qcom/boot_stats.c                     | 101 ++++++++++++++++++++++
 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 0000000..2543029
--- /dev/null
+++ b/Documentation/ABI/testing/debugfs-driver-bootstat
@@ -0,0 +1,17 @@
+What:		/sys/kernel/debug/...stats/pre_abl_time
+Date:           April 2023
+Contact:        Souradeep Chowdhury <quic_schowdhu@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/...stats/abl_time
+Date:           April 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 a8f2830..0d2cbd3 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -16,6 +16,15 @@ config QCOM_AOSS_QMP
 	  subsystems as well as controlling the debug clocks exposed by the Always On
 	  Subsystem (AOSS) using Qualcomm Messaging Protocol (QMP).
 
+config QCOM_BOOTSTAT
+	tristate "Qualcomm Technologies, Boot Stat driver"
+	depends on ARCH_QCOM || COMPILE_TEST
+	help
+	  This option enables driver support for boot stats. Boot stat driver logs
+	  the kernel bootloader information by accessing the imem region. These
+	  information 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 6e88da8..bdaa41a 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 CFLAGS_rpmh-rsc.o := -I$(src)
 obj-$(CONFIG_QCOM_AOSS_QMP) +=	qcom_aoss.o
+obj-$(CONFIG_QCOM_BOOTSTAT) += boot_stats.o
 obj-$(CONFIG_QCOM_GENI_SE) +=	qcom-geni-se.o
 obj-$(CONFIG_QCOM_COMMAND_DB) += cmd-db.o
 obj-$(CONFIG_QCOM_CPR)		+= cpr.o
diff --git a/drivers/soc/qcom/boot_stats.c b/drivers/soc/qcom/boot_stats.c
new file mode 100644
index 0000000..7fa8efb
--- /dev/null
+++ b/drivers/soc/qcom/boot_stats.c
@@ -0,0 +1,101 @@
+// 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 int abl_time_show(struct seq_file *seq, void *v)
+{
+	struct boot_stats *boot_stats = seq->private;
+	u32 abl_time = TO_MS(boot_stats->abl_end) - TO_MS(boot_stats->abl_start);
+
+	seq_printf(seq, "%u ms\n", abl_time);
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(abl_time);
+
+static int pre_abl_time_show(struct seq_file *seq, void *v)
+{
+	struct boot_stats *boot_stats = seq->private;
+
+	seq_printf(seq, "%u ms\n", TO_MS(boot_stats->abl_start));
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(pre_abl_time);
+
+static int boot_stats_probe(struct platform_device *pdev)
+{
+	struct device *bootstat_dev = &pdev->dev;
+	struct bs_data *drvdata;
+
+	drvdata = devm_kzalloc(bootstat_dev, sizeof(*drvdata), GFP_KERNEL);
+	platform_set_drvdata(pdev, drvdata);
+
+	drvdata->dbg_dir = debugfs_create_dir(dev_name(bootstat_dev), NULL);
+	if (IS_ERR(drvdata->dbg_dir))
+		return dev_err_probe(bootstat_dev, -ENOENT, "failed to create debugfs directory");
+
+	drvdata->b_stats = devm_of_iomap(bootstat_dev, bootstat_dev->of_node, 0, NULL);
+	if (!drvdata->b_stats)
+		return dev_err_probe(bootstat_dev, -ENOMEM, "failed to map imem region\n");
+
+	debugfs_create_file("pre_abl_time", 0200, drvdata->dbg_dir,
+			    drvdata->b_stats, &pre_abl_time_fops);
+	debugfs_create_file("abl_time", 0200, drvdata->dbg_dir, drvdata->b_stats, &abl_time_fops);
+
+	return 0;
+}
+
+void boot_stats_remove(struct platform_device *pdev)
+{
+	struct bs_data *drvdata = platform_get_drvdata(pdev);
+
+	debugfs_remove_recursive(drvdata->dbg_dir);
+	iounmap(drvdata->b_stats);
+}
+
+static const struct of_device_id boot_stats_dt_match[] = {
+	{ .compatible = "qcom,sm8450-bootstats" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, boot_stats_dt_match);
+
+static struct platform_driver boot_stat_driver = {
+	.probe  = boot_stats_probe,
+	.remove_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.7.4


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

[PATCH V4 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/146aa6b0.boot_stats # cat abl_time
17898 ms
/sys/kernel/debug/146aa6b0.boot_stats # cat pre_abl_time
2879 ms

The Module Power Manager(MPM) sleep counter starts ticking at the PBL
stage and the timestamp generated by the sleep counter is logged by
the Qualcomm proprietary bootloader(ABL) at two points-> First when it
starts execution which is logged here as "pre_abl_time" and the second
when it is about to load the kernel logged as "abl_time". Documentation
details are also added in Documentation/ABI/testing/debugfs-driver-bootstat

Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
---
 Documentation/ABI/testing/debugfs-driver-bootstat |  17 ++++
 drivers/soc/qcom/Kconfig                          |   9 ++
 drivers/soc/qcom/Makefile                         |   1 +
 drivers/soc/qcom/boot_stats.c                     | 101 ++++++++++++++++++++++
 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 0000000..2543029
--- /dev/null
+++ b/Documentation/ABI/testing/debugfs-driver-bootstat
@@ -0,0 +1,17 @@
+What:		/sys/kernel/debug/...stats/pre_abl_time
+Date:           April 2023
+Contact:        Souradeep Chowdhury <quic_schowdhu@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/...stats/abl_time
+Date:           April 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 a8f2830..0d2cbd3 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -16,6 +16,15 @@ config QCOM_AOSS_QMP
 	  subsystems as well as controlling the debug clocks exposed by the Always On
 	  Subsystem (AOSS) using Qualcomm Messaging Protocol (QMP).
 
+config QCOM_BOOTSTAT
+	tristate "Qualcomm Technologies, Boot Stat driver"
+	depends on ARCH_QCOM || COMPILE_TEST
+	help
+	  This option enables driver support for boot stats. Boot stat driver logs
+	  the kernel bootloader information by accessing the imem region. These
+	  information 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 6e88da8..bdaa41a 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 CFLAGS_rpmh-rsc.o := -I$(src)
 obj-$(CONFIG_QCOM_AOSS_QMP) +=	qcom_aoss.o
+obj-$(CONFIG_QCOM_BOOTSTAT) += boot_stats.o
 obj-$(CONFIG_QCOM_GENI_SE) +=	qcom-geni-se.o
 obj-$(CONFIG_QCOM_COMMAND_DB) += cmd-db.o
 obj-$(CONFIG_QCOM_CPR)		+= cpr.o
diff --git a/drivers/soc/qcom/boot_stats.c b/drivers/soc/qcom/boot_stats.c
new file mode 100644
index 0000000..7fa8efb
--- /dev/null
+++ b/drivers/soc/qcom/boot_stats.c
@@ -0,0 +1,101 @@
+// 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 int abl_time_show(struct seq_file *seq, void *v)
+{
+	struct boot_stats *boot_stats = seq->private;
+	u32 abl_time = TO_MS(boot_stats->abl_end) - TO_MS(boot_stats->abl_start);
+
+	seq_printf(seq, "%u ms\n", abl_time);
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(abl_time);
+
+static int pre_abl_time_show(struct seq_file *seq, void *v)
+{
+	struct boot_stats *boot_stats = seq->private;
+
+	seq_printf(seq, "%u ms\n", TO_MS(boot_stats->abl_start));
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(pre_abl_time);
+
+static int boot_stats_probe(struct platform_device *pdev)
+{
+	struct device *bootstat_dev = &pdev->dev;
+	struct bs_data *drvdata;
+
+	drvdata = devm_kzalloc(bootstat_dev, sizeof(*drvdata), GFP_KERNEL);
+	platform_set_drvdata(pdev, drvdata);
+
+	drvdata->dbg_dir = debugfs_create_dir(dev_name(bootstat_dev), NULL);
+	if (IS_ERR(drvdata->dbg_dir))
+		return dev_err_probe(bootstat_dev, -ENOENT, "failed to create debugfs directory");
+
+	drvdata->b_stats = devm_of_iomap(bootstat_dev, bootstat_dev->of_node, 0, NULL);
+	if (!drvdata->b_stats)
+		return dev_err_probe(bootstat_dev, -ENOMEM, "failed to map imem region\n");
+
+	debugfs_create_file("pre_abl_time", 0200, drvdata->dbg_dir,
+			    drvdata->b_stats, &pre_abl_time_fops);
+	debugfs_create_file("abl_time", 0200, drvdata->dbg_dir, drvdata->b_stats, &abl_time_fops);
+
+	return 0;
+}
+
+void boot_stats_remove(struct platform_device *pdev)
+{
+	struct bs_data *drvdata = platform_get_drvdata(pdev);
+
+	debugfs_remove_recursive(drvdata->dbg_dir);
+	iounmap(drvdata->b_stats);
+}
+
+static const struct of_device_id boot_stats_dt_match[] = {
+	{ .compatible = "qcom,sm8450-bootstats" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, boot_stats_dt_match);
+
+static struct platform_driver boot_stat_driver = {
+	.probe  = boot_stats_probe,
+	.remove_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.7.4

[PATCH V4 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 0e64787..20b96e9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17055,6 +17055,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.7.4

[PATCH V4 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 0e64787..20b96e9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17055,6 +17055,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.7.4


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

Re: [PATCH V4 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.3-rc7]
[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/20230417-231510
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/2ef76ce292c059c144e559123a9a54201ae2d0cf.1681742910.git.quic_schowdhu%40quicinc.com
patch subject: [PATCH V4 2/3] soc: qcom: boot_stat: Add Driver Support for Boot Stats
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20230418/202304180101.Wxf2Jbjq-lkp@intel.com/config)
compiler: sh4-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/4c8808184261e004f0b258e469415e4eea9c5ad9
        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/20230417-231510
        git checkout 4c8808184261e004f0b258e469415e4eea9c5ad9
        # 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=sh olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sh 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/202304180101.Wxf2Jbjq-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		iounmap(drvdata->b_stats);
    82	}
    83	

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

Re: [PATCH V4 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.3-rc7]
[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/20230417-231510
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/2ef76ce292c059c144e559123a9a54201ae2d0cf.1681742910.git.quic_schowdhu%40quicinc.com
patch subject: [PATCH V4 2/3] soc: qcom: boot_stat: Add Driver Support for Boot Stats
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20230418/202304180101.Wxf2Jbjq-lkp@intel.com/config)
compiler: sh4-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/4c8808184261e004f0b258e469415e4eea9c5ad9
        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/20230417-231510
        git checkout 4c8808184261e004f0b258e469415e4eea9c5ad9
        # 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=sh olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sh 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/202304180101.Wxf2Jbjq-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		iounmap(drvdata->b_stats);
    82	}
    83	

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

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

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

[Krzysztof Kozlowski]

On 17/04/2023 17:08, 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>

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 17/04/2023 17:08, 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>

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

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 V4 2/3] soc: qcom: boot_stat: Add Driver Support for Boot Stats

[Pavan Kondeti]

On Mon, Apr 17, 2023 at 08:38:15PM +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 show the information within debugfs file.
> This information is useful in verifying if the existing boot KPIs have
> regressed or not. The information is shown in milliseconds, a sample
> log from sm8450(waipio) device is as follows:-
> 
> /sys/kernel/debug/146aa6b0.boot_stats # cat abl_time
> 17898 ms
> /sys/kernel/debug/146aa6b0.boot_stats # cat pre_abl_time
> 2879 ms
> 
> The Module Power Manager(MPM) sleep counter starts ticking at the PBL
> stage and the timestamp generated by the sleep counter is logged by
> the Qualcomm proprietary bootloader(ABL) at two points-> First when it
> starts execution which is logged here as "pre_abl_time" and the second
> when it is about to load the kernel logged as "abl_time". Documentation
> details are also added in Documentation/ABI/testing/debugfs-driver-bootstat
> 
> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> ---
>  Documentation/ABI/testing/debugfs-driver-bootstat |  17 ++++
>  drivers/soc/qcom/Kconfig                          |   9 ++
>  drivers/soc/qcom/Makefile                         |   1 +
>  drivers/soc/qcom/boot_stats.c                     | 101 ++++++++++++++++++++++
>  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 0000000..2543029
> --- /dev/null
> +++ b/Documentation/ABI/testing/debugfs-driver-bootstat
> @@ -0,0 +1,17 @@
> +What:		/sys/kernel/debug/...stats/pre_abl_time
> +Date:           April 2023
> +Contact:        Souradeep Chowdhury <quic_schowdhu@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/...stats/abl_time
> +Date:           April 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 a8f2830..0d2cbd3 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -16,6 +16,15 @@ config QCOM_AOSS_QMP
>  	  subsystems as well as controlling the debug clocks exposed by the Always On
>  	  Subsystem (AOSS) using Qualcomm Messaging Protocol (QMP).
>  
> +config QCOM_BOOTSTAT
> +	tristate "Qualcomm Technologies, Boot Stat driver"
> +	depends on ARCH_QCOM || COMPILE_TEST
> +	help
> +	  This option enables driver support for boot stats. Boot stat driver logs
> +	  the kernel bootloader information by accessing the imem region. These
> +	  information are exposed in the form of debugfs files. This is used to
> +	  determine if there is any regression in boot timings.

should this driver depend on DEBUG_FS?

> +
>  config QCOM_COMMAND_DB
>  	tristate "Qualcomm Command DB"
>  	depends on ARCH_QCOM || COMPILE_TEST
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index 6e88da8..bdaa41a 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  CFLAGS_rpmh-rsc.o := -I$(src)
>  obj-$(CONFIG_QCOM_AOSS_QMP) +=	qcom_aoss.o
> +obj-$(CONFIG_QCOM_BOOTSTAT) += boot_stats.o
>  obj-$(CONFIG_QCOM_GENI_SE) +=	qcom-geni-se.o
>  obj-$(CONFIG_QCOM_COMMAND_DB) += cmd-db.o
>  obj-$(CONFIG_QCOM_CPR)		+= cpr.o
> diff --git a/drivers/soc/qcom/boot_stats.c b/drivers/soc/qcom/boot_stats.c
> new file mode 100644
> index 0000000..7fa8efb
> --- /dev/null
> +++ b/drivers/soc/qcom/boot_stats.c
> @@ -0,0 +1,101 @@
> +// 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 int abl_time_show(struct seq_file *seq, void *v)
> +{
> +	struct boot_stats *boot_stats = seq->private;
> +	u32 abl_time = TO_MS(boot_stats->abl_end) - TO_MS(boot_stats->abl_start);
> +
> +	seq_printf(seq, "%u ms\n", abl_time);
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(abl_time);
> +
> +static int pre_abl_time_show(struct seq_file *seq, void *v)
> +{
> +	struct boot_stats *boot_stats = seq->private;
> +
> +	seq_printf(seq, "%u ms\n", TO_MS(boot_stats->abl_start));
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(pre_abl_time);
> +
> +static int boot_stats_probe(struct platform_device *pdev)
> +{
> +	struct device *bootstat_dev = &pdev->dev;
> +	struct bs_data *drvdata;
> +
> +	drvdata = devm_kzalloc(bootstat_dev, sizeof(*drvdata), GFP_KERNEL);

Error handling missing.

> +	platform_set_drvdata(pdev, drvdata);
> +
> +	drvdata->dbg_dir = debugfs_create_dir(dev_name(bootstat_dev), NULL);
> +	if (IS_ERR(drvdata->dbg_dir))
> +		return dev_err_probe(bootstat_dev, -ENOENT, "failed to create debugfs directory");

%s/-ENOENT/PTR_ERR(drvdata->dbg_dir)

> +
> +	drvdata->b_stats = devm_of_iomap(bootstat_dev, bootstat_dev->of_node, 0, NULL);
> +	if (!drvdata->b_stats)
> +		return dev_err_probe(bootstat_dev, -ENOMEM, "failed to map imem region\n");

The error handling for devm_of_iomap() is different from usual
ioremap(). you should check for IS_ERR() and return the error code
approriately. Also when it fails, undo the stuff done above i.e delete
the debugfs directory.

> +
> +	debugfs_create_file("pre_abl_time", 0200, drvdata->dbg_dir,
> +			    drvdata->b_stats, &pre_abl_time_fops);
> +	debugfs_create_file("abl_time", 0200, drvdata->dbg_dir, drvdata->b_stats, &abl_time_fops);

Why 0200 permissions for read only entries?

Also consider using debugfs_create_str() which makes your driver
simpler.

> +
> +	return 0;
> +}
> +
> +void boot_stats_remove(struct platform_device *pdev)

static?

> +{
> +	struct bs_data *drvdata = platform_get_drvdata(pdev);
> +
> +	debugfs_remove_recursive(drvdata->dbg_dir);
> +	iounmap(drvdata->b_stats);

since you used devm_of_iomap(), explicit iounmap is not needed.

> +}
> +
> +static const struct of_device_id boot_stats_dt_match[] = {
> +	{ .compatible = "qcom,sm8450-bootstats" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, boot_stats_dt_match);
> +
> +static struct platform_driver boot_stat_driver = {
> +	.probe  = boot_stats_probe,
> +	.remove_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.7.4
> 
Thanks,
Pavan

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

[Pavan Kondeti]

On Mon, Apr 17, 2023 at 08:38:15PM +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 show the information within debugfs file.
> This information is useful in verifying if the existing boot KPIs have
> regressed or not. The information is shown in milliseconds, a sample
> log from sm8450(waipio) device is as follows:-
> 
> /sys/kernel/debug/146aa6b0.boot_stats # cat abl_time
> 17898 ms
> /sys/kernel/debug/146aa6b0.boot_stats # cat pre_abl_time
> 2879 ms
> 
> The Module Power Manager(MPM) sleep counter starts ticking at the PBL
> stage and the timestamp generated by the sleep counter is logged by
> the Qualcomm proprietary bootloader(ABL) at two points-> First when it
> starts execution which is logged here as "pre_abl_time" and the second
> when it is about to load the kernel logged as "abl_time". Documentation
> details are also added in Documentation/ABI/testing/debugfs-driver-bootstat
> 
> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> ---
>  Documentation/ABI/testing/debugfs-driver-bootstat |  17 ++++
>  drivers/soc/qcom/Kconfig                          |   9 ++
>  drivers/soc/qcom/Makefile                         |   1 +
>  drivers/soc/qcom/boot_stats.c                     | 101 ++++++++++++++++++++++
>  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 0000000..2543029
> --- /dev/null
> +++ b/Documentation/ABI/testing/debugfs-driver-bootstat
> @@ -0,0 +1,17 @@
> +What:		/sys/kernel/debug/...stats/pre_abl_time
> +Date:           April 2023
> +Contact:        Souradeep Chowdhury <quic_schowdhu@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/...stats/abl_time
> +Date:           April 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 a8f2830..0d2cbd3 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -16,6 +16,15 @@ config QCOM_AOSS_QMP
>  	  subsystems as well as controlling the debug clocks exposed by the Always On
>  	  Subsystem (AOSS) using Qualcomm Messaging Protocol (QMP).
>  
> +config QCOM_BOOTSTAT
> +	tristate "Qualcomm Technologies, Boot Stat driver"
> +	depends on ARCH_QCOM || COMPILE_TEST
> +	help
> +	  This option enables driver support for boot stats. Boot stat driver logs
> +	  the kernel bootloader information by accessing the imem region. These
> +	  information are exposed in the form of debugfs files. This is used to
> +	  determine if there is any regression in boot timings.

should this driver depend on DEBUG_FS?

> +
>  config QCOM_COMMAND_DB
>  	tristate "Qualcomm Command DB"
>  	depends on ARCH_QCOM || COMPILE_TEST
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index 6e88da8..bdaa41a 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  CFLAGS_rpmh-rsc.o := -I$(src)
>  obj-$(CONFIG_QCOM_AOSS_QMP) +=	qcom_aoss.o
> +obj-$(CONFIG_QCOM_BOOTSTAT) += boot_stats.o
>  obj-$(CONFIG_QCOM_GENI_SE) +=	qcom-geni-se.o
>  obj-$(CONFIG_QCOM_COMMAND_DB) += cmd-db.o
>  obj-$(CONFIG_QCOM_CPR)		+= cpr.o
> diff --git a/drivers/soc/qcom/boot_stats.c b/drivers/soc/qcom/boot_stats.c
> new file mode 100644
> index 0000000..7fa8efb
> --- /dev/null
> +++ b/drivers/soc/qcom/boot_stats.c
> @@ -0,0 +1,101 @@
> +// 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 int abl_time_show(struct seq_file *seq, void *v)
> +{
> +	struct boot_stats *boot_stats = seq->private;
> +	u32 abl_time = TO_MS(boot_stats->abl_end) - TO_MS(boot_stats->abl_start);
> +
> +	seq_printf(seq, "%u ms\n", abl_time);
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(abl_time);
> +
> +static int pre_abl_time_show(struct seq_file *seq, void *v)
> +{
> +	struct boot_stats *boot_stats = seq->private;
> +
> +	seq_printf(seq, "%u ms\n", TO_MS(boot_stats->abl_start));
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(pre_abl_time);
> +
> +static int boot_stats_probe(struct platform_device *pdev)
> +{
> +	struct device *bootstat_dev = &pdev->dev;
> +	struct bs_data *drvdata;
> +
> +	drvdata = devm_kzalloc(bootstat_dev, sizeof(*drvdata), GFP_KERNEL);

Error handling missing.

> +	platform_set_drvdata(pdev, drvdata);
> +
> +	drvdata->dbg_dir = debugfs_create_dir(dev_name(bootstat_dev), NULL);
> +	if (IS_ERR(drvdata->dbg_dir))
> +		return dev_err_probe(bootstat_dev, -ENOENT, "failed to create debugfs directory");

%s/-ENOENT/PTR_ERR(drvdata->dbg_dir)

> +
> +	drvdata->b_stats = devm_of_iomap(bootstat_dev, bootstat_dev->of_node, 0, NULL);
> +	if (!drvdata->b_stats)
> +		return dev_err_probe(bootstat_dev, -ENOMEM, "failed to map imem region\n");

The error handling for devm_of_iomap() is different from usual
ioremap(). you should check for IS_ERR() and return the error code
approriately. Also when it fails, undo the stuff done above i.e delete
the debugfs directory.

> +
> +	debugfs_create_file("pre_abl_time", 0200, drvdata->dbg_dir,
> +			    drvdata->b_stats, &pre_abl_time_fops);
> +	debugfs_create_file("abl_time", 0200, drvdata->dbg_dir, drvdata->b_stats, &abl_time_fops);

Why 0200 permissions for read only entries?

Also consider using debugfs_create_str() which makes your driver
simpler.

> +
> +	return 0;
> +}
> +
> +void boot_stats_remove(struct platform_device *pdev)

static?

> +{
> +	struct bs_data *drvdata = platform_get_drvdata(pdev);
> +
> +	debugfs_remove_recursive(drvdata->dbg_dir);
> +	iounmap(drvdata->b_stats);

since you used devm_of_iomap(), explicit iounmap is not needed.

> +}
> +
> +static const struct of_device_id boot_stats_dt_match[] = {
> +	{ .compatible = "qcom,sm8450-bootstats" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, boot_stats_dt_match);
> +
> +static struct platform_driver boot_stat_driver = {
> +	.probe  = boot_stats_probe,
> +	.remove_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.7.4
> 
Thanks,
Pavan

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

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

[Souradeep Chowdhury]

On 4/27/2023 9:41 AM, Pavan Kondeti wrote:
> On Mon, Apr 17, 2023 at 08:38:15PM +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 show the information within debugfs file.
>> This information is useful in verifying if the existing boot KPIs have
>> regressed or not. The information is shown in milliseconds, a sample
>> log from sm8450(waipio) device is as follows:-
>>
>> /sys/kernel/debug/146aa6b0.boot_stats # cat abl_time
>> 17898 ms
>> /sys/kernel/debug/146aa6b0.boot_stats # cat pre_abl_time
>> 2879 ms
>>
>> The Module Power Manager(MPM) sleep counter starts ticking at the PBL
>> stage and the timestamp generated by the sleep counter is logged by
>> the Qualcomm proprietary bootloader(ABL) at two points-> First when it
>> starts execution which is logged here as "pre_abl_time" and the second
>> when it is about to load the kernel logged as "abl_time". Documentation
>> details are also added in Documentation/ABI/testing/debugfs-driver-bootstat
>>
>> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
>> ---
>>   Documentation/ABI/testing/debugfs-driver-bootstat |  17 ++++
>>   drivers/soc/qcom/Kconfig                          |   9 ++
>>   drivers/soc/qcom/Makefile                         |   1 +
>>   drivers/soc/qcom/boot_stats.c                     | 101 ++++++++++++++++++++++
>>   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 0000000..2543029
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/debugfs-driver-bootstat
>> @@ -0,0 +1,17 @@
>> +What:		/sys/kernel/debug/...stats/pre_abl_time
>> +Date:           April 2023
>> +Contact:        Souradeep Chowdhury <quic_schowdhu@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/...stats/abl_time
>> +Date:           April 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 a8f2830..0d2cbd3 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -16,6 +16,15 @@ config QCOM_AOSS_QMP
>>   	  subsystems as well as controlling the debug clocks exposed by the Always On
>>   	  Subsystem (AOSS) using Qualcomm Messaging Protocol (QMP).
>>   
>> +config QCOM_BOOTSTAT
>> +	tristate "Qualcomm Technologies, Boot Stat driver"
>> +	depends on ARCH_QCOM || COMPILE_TEST
>> +	help
>> +	  This option enables driver support for boot stats. Boot stat driver logs
>> +	  the kernel bootloader information by accessing the imem region. These
>> +	  information are exposed in the form of debugfs files. This is used to
>> +	  determine if there is any regression in boot timings.
> 
> should this driver depend on DEBUG_FS?

Ack

> 
>> +
>>   config QCOM_COMMAND_DB
>>   	tristate "Qualcomm Command DB"
>>   	depends on ARCH_QCOM || COMPILE_TEST
>> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
>> index 6e88da8..bdaa41a 100644
>> --- a/drivers/soc/qcom/Makefile
>> +++ b/drivers/soc/qcom/Makefile
>> @@ -1,6 +1,7 @@
>>   # SPDX-License-Identifier: GPL-2.0
>>   CFLAGS_rpmh-rsc.o := -I$(src)
>>   obj-$(CONFIG_QCOM_AOSS_QMP) +=	qcom_aoss.o
>> +obj-$(CONFIG_QCOM_BOOTSTAT) += boot_stats.o
>>   obj-$(CONFIG_QCOM_GENI_SE) +=	qcom-geni-se.o
>>   obj-$(CONFIG_QCOM_COMMAND_DB) += cmd-db.o
>>   obj-$(CONFIG_QCOM_CPR)		+= cpr.o
>> diff --git a/drivers/soc/qcom/boot_stats.c b/drivers/soc/qcom/boot_stats.c
>> new file mode 100644
>> index 0000000..7fa8efb
>> --- /dev/null
>> +++ b/drivers/soc/qcom/boot_stats.c
>> @@ -0,0 +1,101 @@
>> +// 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 int abl_time_show(struct seq_file *seq, void *v)
>> +{
>> +	struct boot_stats *boot_stats = seq->private;
>> +	u32 abl_time = TO_MS(boot_stats->abl_end) - TO_MS(boot_stats->abl_start);
>> +
>> +	seq_printf(seq, "%u ms\n", abl_time);
>> +	return 0;
>> +}
>> +DEFINE_SHOW_ATTRIBUTE(abl_time);
>> +
>> +static int pre_abl_time_show(struct seq_file *seq, void *v)
>> +{
>> +	struct boot_stats *boot_stats = seq->private;
>> +
>> +	seq_printf(seq, "%u ms\n", TO_MS(boot_stats->abl_start));
>> +	return 0;
>> +}
>> +DEFINE_SHOW_ATTRIBUTE(pre_abl_time);
>> +
>> +static int boot_stats_probe(struct platform_device *pdev)
>> +{
>> +	struct device *bootstat_dev = &pdev->dev;
>> +	struct bs_data *drvdata;
>> +
>> +	drvdata = devm_kzalloc(bootstat_dev, sizeof(*drvdata), GFP_KERNEL);
> 
> Error handling missing.

Ack

> 
>> +	platform_set_drvdata(pdev, drvdata);
>> +
>> +	drvdata->dbg_dir = debugfs_create_dir(dev_name(bootstat_dev), NULL);
>> +	if (IS_ERR(drvdata->dbg_dir))
>> +		return dev_err_probe(bootstat_dev, -ENOENT, "failed to create debugfs directory");
> 
> %s/-ENOENT/PTR_ERR(drvdata->dbg_dir)

Ack

> 
>> +
>> +	drvdata->b_stats = devm_of_iomap(bootstat_dev, bootstat_dev->of_node, 0, NULL);
>> +	if (!drvdata->b_stats)
>> +		return dev_err_probe(bootstat_dev, -ENOMEM, "failed to map imem region\n");
> 
> The error handling for devm_of_iomap() is different from usual
> ioremap(). you should check for IS_ERR() and return the error code
> approriately. Also when it fails, undo the stuff done above i.e delete
> the debugfs directory.

Ack

> 
>> +
>> +	debugfs_create_file("pre_abl_time", 0200, drvdata->dbg_dir,
>> +			    drvdata->b_stats, &pre_abl_time_fops);
>> +	debugfs_create_file("abl_time", 0200, drvdata->dbg_dir, drvdata->b_stats, &abl_time_fops);
> 
> Why 0200 permissions for read only entries?
> 
> Also consider using debugfs_create_str() which makes your driver
> simpler.

Ack

> 
>> +
>> +	return 0;
>> +}
>> +
>> +void boot_stats_remove(struct platform_device *pdev)
> 
> static?

Ack

> 
>> +{
>> +	struct bs_data *drvdata = platform_get_drvdata(pdev);
>> +
>> +	debugfs_remove_recursive(drvdata->dbg_dir);
>> +	iounmap(drvdata->b_stats);
> 
> since you used devm_of_iomap(), explicit iounmap is not needed.

Ack

> 
>> +}
>> +
>> +static const struct of_device_id boot_stats_dt_match[] = {
>> +	{ .compatible = "qcom,sm8450-bootstats" },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, boot_stats_dt_match);
>> +
>> +static struct platform_driver boot_stat_driver = {
>> +	.probe  = boot_stats_probe,
>> +	.remove_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.7.4
>>
> Thanks,
> Pavan

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

[Souradeep Chowdhury]

On 4/27/2023 9:41 AM, Pavan Kondeti wrote:
> On Mon, Apr 17, 2023 at 08:38:15PM +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 show the information within debugfs file.
>> This information is useful in verifying if the existing boot KPIs have
>> regressed or not. The information is shown in milliseconds, a sample
>> log from sm8450(waipio) device is as follows:-
>>
>> /sys/kernel/debug/146aa6b0.boot_stats # cat abl_time
>> 17898 ms
>> /sys/kernel/debug/146aa6b0.boot_stats # cat pre_abl_time
>> 2879 ms
>>
>> The Module Power Manager(MPM) sleep counter starts ticking at the PBL
>> stage and the timestamp generated by the sleep counter is logged by
>> the Qualcomm proprietary bootloader(ABL) at two points-> First when it
>> starts execution which is logged here as "pre_abl_time" and the second
>> when it is about to load the kernel logged as "abl_time". Documentation
>> details are also added in Documentation/ABI/testing/debugfs-driver-bootstat
>>
>> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
>> ---
>>   Documentation/ABI/testing/debugfs-driver-bootstat |  17 ++++
>>   drivers/soc/qcom/Kconfig                          |   9 ++
>>   drivers/soc/qcom/Makefile                         |   1 +
>>   drivers/soc/qcom/boot_stats.c                     | 101 ++++++++++++++++++++++
>>   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 0000000..2543029
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/debugfs-driver-bootstat
>> @@ -0,0 +1,17 @@
>> +What:		/sys/kernel/debug/...stats/pre_abl_time
>> +Date:           April 2023
>> +Contact:        Souradeep Chowdhury <quic_schowdhu@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/...stats/abl_time
>> +Date:           April 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 a8f2830..0d2cbd3 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -16,6 +16,15 @@ config QCOM_AOSS_QMP
>>   	  subsystems as well as controlling the debug clocks exposed by the Always On
>>   	  Subsystem (AOSS) using Qualcomm Messaging Protocol (QMP).
>>   
>> +config QCOM_BOOTSTAT
>> +	tristate "Qualcomm Technologies, Boot Stat driver"
>> +	depends on ARCH_QCOM || COMPILE_TEST
>> +	help
>> +	  This option enables driver support for boot stats. Boot stat driver logs
>> +	  the kernel bootloader information by accessing the imem region. These
>> +	  information are exposed in the form of debugfs files. This is used to
>> +	  determine if there is any regression in boot timings.
> 
> should this driver depend on DEBUG_FS?

Ack

> 
>> +
>>   config QCOM_COMMAND_DB
>>   	tristate "Qualcomm Command DB"
>>   	depends on ARCH_QCOM || COMPILE_TEST
>> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
>> index 6e88da8..bdaa41a 100644
>> --- a/drivers/soc/qcom/Makefile
>> +++ b/drivers/soc/qcom/Makefile
>> @@ -1,6 +1,7 @@
>>   # SPDX-License-Identifier: GPL-2.0
>>   CFLAGS_rpmh-rsc.o := -I$(src)
>>   obj-$(CONFIG_QCOM_AOSS_QMP) +=	qcom_aoss.o
>> +obj-$(CONFIG_QCOM_BOOTSTAT) += boot_stats.o
>>   obj-$(CONFIG_QCOM_GENI_SE) +=	qcom-geni-se.o
>>   obj-$(CONFIG_QCOM_COMMAND_DB) += cmd-db.o
>>   obj-$(CONFIG_QCOM_CPR)		+= cpr.o
>> diff --git a/drivers/soc/qcom/boot_stats.c b/drivers/soc/qcom/boot_stats.c
>> new file mode 100644
>> index 0000000..7fa8efb
>> --- /dev/null
>> +++ b/drivers/soc/qcom/boot_stats.c
>> @@ -0,0 +1,101 @@
>> +// 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 int abl_time_show(struct seq_file *seq, void *v)
>> +{
>> +	struct boot_stats *boot_stats = seq->private;
>> +	u32 abl_time = TO_MS(boot_stats->abl_end) - TO_MS(boot_stats->abl_start);
>> +
>> +	seq_printf(seq, "%u ms\n", abl_time);
>> +	return 0;
>> +}
>> +DEFINE_SHOW_ATTRIBUTE(abl_time);
>> +
>> +static int pre_abl_time_show(struct seq_file *seq, void *v)
>> +{
>> +	struct boot_stats *boot_stats = seq->private;
>> +
>> +	seq_printf(seq, "%u ms\n", TO_MS(boot_stats->abl_start));
>> +	return 0;
>> +}
>> +DEFINE_SHOW_ATTRIBUTE(pre_abl_time);
>> +
>> +static int boot_stats_probe(struct platform_device *pdev)
>> +{
>> +	struct device *bootstat_dev = &pdev->dev;
>> +	struct bs_data *drvdata;
>> +
>> +	drvdata = devm_kzalloc(bootstat_dev, sizeof(*drvdata), GFP_KERNEL);
> 
> Error handling missing.

Ack

> 
>> +	platform_set_drvdata(pdev, drvdata);
>> +
>> +	drvdata->dbg_dir = debugfs_create_dir(dev_name(bootstat_dev), NULL);
>> +	if (IS_ERR(drvdata->dbg_dir))
>> +		return dev_err_probe(bootstat_dev, -ENOENT, "failed to create debugfs directory");
> 
> %s/-ENOENT/PTR_ERR(drvdata->dbg_dir)

Ack

> 
>> +
>> +	drvdata->b_stats = devm_of_iomap(bootstat_dev, bootstat_dev->of_node, 0, NULL);
>> +	if (!drvdata->b_stats)
>> +		return dev_err_probe(bootstat_dev, -ENOMEM, "failed to map imem region\n");
> 
> The error handling for devm_of_iomap() is different from usual
> ioremap(). you should check for IS_ERR() and return the error code
> approriately. Also when it fails, undo the stuff done above i.e delete
> the debugfs directory.

Ack

> 
>> +
>> +	debugfs_create_file("pre_abl_time", 0200, drvdata->dbg_dir,
>> +			    drvdata->b_stats, &pre_abl_time_fops);
>> +	debugfs_create_file("abl_time", 0200, drvdata->dbg_dir, drvdata->b_stats, &abl_time_fops);
> 
> Why 0200 permissions for read only entries?
> 
> Also consider using debugfs_create_str() which makes your driver
> simpler.

Ack

> 
>> +
>> +	return 0;
>> +}
>> +
>> +void boot_stats_remove(struct platform_device *pdev)
> 
> static?

Ack

> 
>> +{
>> +	struct bs_data *drvdata = platform_get_drvdata(pdev);
>> +
>> +	debugfs_remove_recursive(drvdata->dbg_dir);
>> +	iounmap(drvdata->b_stats);
> 
> since you used devm_of_iomap(), explicit iounmap is not needed.

Ack

> 
>> +}
>> +
>> +static const struct of_device_id boot_stats_dt_match[] = {
>> +	{ .compatible = "qcom,sm8450-bootstats" },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, boot_stats_dt_match);
>> +
>> +static struct platform_driver boot_stat_driver = {
>> +	.probe  = boot_stats_probe,
>> +	.remove_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.7.4
>>
> Thanks,
> Pavan

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

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

[Caleb Connolly]

On 17/04/2023 16:08, 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        | 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 ba694ce..d028bed 100644
> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> @@ -49,6 +49,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

This region isn't exclusive to sm8450, it exists also on sdm845 and
presumably other platforms. Is there any need for an SoC specific
compatible?

> +          - const: qcom,imem-bootstats
> +
> +      reg:
> +        maxItems: 1
> +
> +    required:
> +      - compatible
> +      - reg
> +
>  required:
>    - compatible
>    - reg
> --
> 2.7.4
> 

-- 
Kind Regards,
Caleb (they/them)

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

[Caleb Connolly]

On 17/04/2023 16:08, 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        | 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 ba694ce..d028bed 100644
> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> @@ -49,6 +49,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

This region isn't exclusive to sm8450, it exists also on sdm845 and
presumably other platforms. Is there any need for an SoC specific
compatible?

> +          - const: qcom,imem-bootstats
> +
> +      reg:
> +        maxItems: 1
> +
> +    required:
> +      - compatible
> +      - reg
> +
>  required:
>    - compatible
>    - reg
> --
> 2.7.4
> 

-- 
Kind Regards,
Caleb (they/them)

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

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

[Caleb Connolly]

On 17/04/2023 16:08, Souradeep Chowdhury wrote:
> All of Qualcomm's proprietary Android boot-loaders capture boot time
> stats, like the time when the bootloader started execution and at what
> point the bootloader handed over control to the kernel etc. in the IMEM
> region. This information is captured in a specific format by this driver
> by mapping a structure to the IMEM memory region and then accessing the
> members of the structure to show the information within debugfs file.
> This information is useful in verifying if the existing boot KPIs have
> regressed or not. The information is shown in milliseconds, a sample
> log from sm8450(waipio) device is as follows:-
> 
> /sys/kernel/debug/146aa6b0.boot_stats # cat abl_time
> 17898 ms
> /sys/kernel/debug/146aa6b0.boot_stats # cat pre_abl_time
> 2879 ms
> 
> The Module Power Manager(MPM) sleep counter starts ticking at the PBL
> stage and the timestamp generated by the sleep counter is logged by
> the Qualcomm proprietary bootloader(ABL) at two points-> First when it
> starts execution which is logged here as "pre_abl_time" and the second
> when it is about to load the kernel logged as "abl_time". Documentation
> details are also added in Documentation/ABI/testing/debugfs-driver-bootstat
> 
> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>

Hi,

[...]
> +
> +static int boot_stats_probe(struct platform_device *pdev)
> +{
> +	struct device *bootstat_dev = &pdev->dev;
> +	struct bs_data *drvdata;
> +
> +	drvdata = devm_kzalloc(bootstat_dev, sizeof(*drvdata), GFP_KERNEL);
> +	platform_set_drvdata(pdev, drvdata);
> +
> +	drvdata->dbg_dir = debugfs_create_dir(dev_name(bootstat_dev), NULL);

This might be better as just "qcom_boot_stats", rather than including
the address.

[...]
> +
> +static const struct of_device_id boot_stats_dt_match[] = {
> +	{ .compatible = "qcom,sm8450-bootstats" },

This driver doesn't only support sm8450, I've tested this on sdm845 and
it works just fine. Can we use a generic compatible here instead?
> +	{ }
> +};
> +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.7.4
> 

-- 
Kind Regards,
Caleb (they/them)

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

[Caleb Connolly]

On 17/04/2023 16:08, Souradeep Chowdhury wrote:
> All of Qualcomm's proprietary Android boot-loaders capture boot time
> stats, like the time when the bootloader started execution and at what
> point the bootloader handed over control to the kernel etc. in the IMEM
> region. This information is captured in a specific format by this driver
> by mapping a structure to the IMEM memory region and then accessing the
> members of the structure to show the information within debugfs file.
> This information is useful in verifying if the existing boot KPIs have
> regressed or not. The information is shown in milliseconds, a sample
> log from sm8450(waipio) device is as follows:-
> 
> /sys/kernel/debug/146aa6b0.boot_stats # cat abl_time
> 17898 ms
> /sys/kernel/debug/146aa6b0.boot_stats # cat pre_abl_time
> 2879 ms
> 
> The Module Power Manager(MPM) sleep counter starts ticking at the PBL
> stage and the timestamp generated by the sleep counter is logged by
> the Qualcomm proprietary bootloader(ABL) at two points-> First when it
> starts execution which is logged here as "pre_abl_time" and the second
> when it is about to load the kernel logged as "abl_time". Documentation
> details are also added in Documentation/ABI/testing/debugfs-driver-bootstat
> 
> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>

Hi,

[...]
> +
> +static int boot_stats_probe(struct platform_device *pdev)
> +{
> +	struct device *bootstat_dev = &pdev->dev;
> +	struct bs_data *drvdata;
> +
> +	drvdata = devm_kzalloc(bootstat_dev, sizeof(*drvdata), GFP_KERNEL);
> +	platform_set_drvdata(pdev, drvdata);
> +
> +	drvdata->dbg_dir = debugfs_create_dir(dev_name(bootstat_dev), NULL);

This might be better as just "qcom_boot_stats", rather than including
the address.

[...]
> +
> +static const struct of_device_id boot_stats_dt_match[] = {
> +	{ .compatible = "qcom,sm8450-bootstats" },

This driver doesn't only support sm8450, I've tested this on sdm845 and
it works just fine. Can we use a generic compatible here instead?
> +	{ }
> +};
> +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.7.4
> 

-- 
Kind Regards,
Caleb (they/them)

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

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

[Souradeep Chowdhury]

On 5/4/2023 3:40 AM, Caleb Connolly wrote:
> 
> 
> On 17/04/2023 16:08, 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        | 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 ba694ce..d028bed 100644
>> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>> @@ -49,6 +49,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
> 
> This region isn't exclusive to sm8450, it exists also on sdm845 and
> presumably other platforms. Is there any need for an SoC specific
> compatible?

Yes SOC specific compatibles are needed for device tree nodes. This has 
been clarified as per prior discussion on this patch series.

https://lore.kernel.org/lkml/e1d53083-82b6-d193-517e-02af281a066a@linaro.org/ 


> 
>> +          - const: qcom,imem-bootstats
>> +
>> +      reg:
>> +        maxItems: 1
>> +
>> +    required:
>> +      - compatible
>> +      - reg
>> +
>>   required:
>>     - compatible
>>     - reg
>> --
>> 2.7.4
>>
> 

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

[Souradeep Chowdhury]

On 5/4/2023 3:40 AM, Caleb Connolly wrote:
> 
> 
> On 17/04/2023 16:08, 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        | 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 ba694ce..d028bed 100644
>> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>> @@ -49,6 +49,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
> 
> This region isn't exclusive to sm8450, it exists also on sdm845 and
> presumably other platforms. Is there any need for an SoC specific
> compatible?

Yes SOC specific compatibles are needed for device tree nodes. This has 
been clarified as per prior discussion on this patch series.

https://lore.kernel.org/lkml/e1d53083-82b6-d193-517e-02af281a066a@linaro.org/ 


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

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

[Krzysztof Kozlowski]

On 04/05/2023 00:10, Caleb Connolly wrote:
> 
> 
> On 17/04/2023 16:08, 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        | 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 ba694ce..d028bed 100644
>> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>> @@ -49,6 +49,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
> 
> This region isn't exclusive to sm8450, it exists also on sdm845 and
> presumably other platforms. Is there any need for an SoC specific
> compatible?

Yes.
https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42

Also see many discussions on LKML about this.

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 04/05/2023 00:10, Caleb Connolly wrote:
> 
> 
> On 17/04/2023 16:08, 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        | 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 ba694ce..d028bed 100644
>> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>> @@ -49,6 +49,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
> 
> This region isn't exclusive to sm8450, it exists also on sdm845 and
> presumably other platforms. Is there any need for an SoC specific
> compatible?

Yes.
https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42

Also see many discussions on LKML about 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 V4 2/3] soc: qcom: boot_stat: Add Driver Support for Boot Stats

[Souradeep Chowdhury]

On 5/4/2023 4:23 AM, Caleb Connolly wrote:
> 
> 
> On 17/04/2023 16:08, Souradeep Chowdhury wrote:
>> All of Qualcomm's proprietary Android boot-loaders capture boot time
>> stats, like the time when the bootloader started execution and at what
>> point the bootloader handed over control to the kernel etc. in the IMEM
>> region. This information is captured in a specific format by this driver
>> by mapping a structure to the IMEM memory region and then accessing the
>> members of the structure to show the information within debugfs file.
>> This information is useful in verifying if the existing boot KPIs have
>> regressed or not. The information is shown in milliseconds, a sample
>> log from sm8450(waipio) device is as follows:-
>>
>> /sys/kernel/debug/146aa6b0.boot_stats # cat abl_time
>> 17898 ms
>> /sys/kernel/debug/146aa6b0.boot_stats # cat pre_abl_time
>> 2879 ms
>>
>> The Module Power Manager(MPM) sleep counter starts ticking at the PBL
>> stage and the timestamp generated by the sleep counter is logged by
>> the Qualcomm proprietary bootloader(ABL) at two points-> First when it
>> starts execution which is logged here as "pre_abl_time" and the second
>> when it is about to load the kernel logged as "abl_time". Documentation
>> details are also added in Documentation/ABI/testing/debugfs-driver-bootstat
>>
>> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> 
> Hi,
> 
> [...]
>> +
>> +static int boot_stats_probe(struct platform_device *pdev)
>> +{
>> +	struct device *bootstat_dev = &pdev->dev;
>> +	struct bs_data *drvdata;
>> +
>> +	drvdata = devm_kzalloc(bootstat_dev, sizeof(*drvdata), GFP_KERNEL);
>> +	platform_set_drvdata(pdev, drvdata);
>> +
>> +	drvdata->dbg_dir = debugfs_create_dir(dev_name(bootstat_dev), NULL);
> 
> This might be better as just "qcom_boot_stats", rather than including
> the address.

We usually use the dev_name to represent the one to one correspondence 
of the debugfs file with the device. Will create the root dir as
"qcom_boot_stats" and push the dev_name dir inside it.

> 
> [...]
>> +
>> +static const struct of_device_id boot_stats_dt_match[] = {
>> +	{ .compatible = "qcom,sm8450-bootstats" },
> 
> This driver doesn't only support sm8450, I've tested this on sdm845 and
> it works just fine. Can we use a generic compatible here instead?

We can add soc specific compatibles here to extend support for other 
socs. This also captures the SoCs for which the driver is supported 
which won't be the case if we use a generic compatible.


>> +	{ }
>> +};
>> +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.7.4
>>
> 

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

[Souradeep Chowdhury]

On 5/4/2023 4:23 AM, Caleb Connolly wrote:
> 
> 
> On 17/04/2023 16:08, Souradeep Chowdhury wrote:
>> All of Qualcomm's proprietary Android boot-loaders capture boot time
>> stats, like the time when the bootloader started execution and at what
>> point the bootloader handed over control to the kernel etc. in the IMEM
>> region. This information is captured in a specific format by this driver
>> by mapping a structure to the IMEM memory region and then accessing the
>> members of the structure to show the information within debugfs file.
>> This information is useful in verifying if the existing boot KPIs have
>> regressed or not. The information is shown in milliseconds, a sample
>> log from sm8450(waipio) device is as follows:-
>>
>> /sys/kernel/debug/146aa6b0.boot_stats # cat abl_time
>> 17898 ms
>> /sys/kernel/debug/146aa6b0.boot_stats # cat pre_abl_time
>> 2879 ms
>>
>> The Module Power Manager(MPM) sleep counter starts ticking at the PBL
>> stage and the timestamp generated by the sleep counter is logged by
>> the Qualcomm proprietary bootloader(ABL) at two points-> First when it
>> starts execution which is logged here as "pre_abl_time" and the second
>> when it is about to load the kernel logged as "abl_time". Documentation
>> details are also added in Documentation/ABI/testing/debugfs-driver-bootstat
>>
>> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> 
> Hi,
> 
> [...]
>> +
>> +static int boot_stats_probe(struct platform_device *pdev)
>> +{
>> +	struct device *bootstat_dev = &pdev->dev;
>> +	struct bs_data *drvdata;
>> +
>> +	drvdata = devm_kzalloc(bootstat_dev, sizeof(*drvdata), GFP_KERNEL);
>> +	platform_set_drvdata(pdev, drvdata);
>> +
>> +	drvdata->dbg_dir = debugfs_create_dir(dev_name(bootstat_dev), NULL);
> 
> This might be better as just "qcom_boot_stats", rather than including
> the address.

We usually use the dev_name to represent the one to one correspondence 
of the debugfs file with the device. Will create the root dir as
"qcom_boot_stats" and push the dev_name dir inside it.

> 
> [...]
>> +
>> +static const struct of_device_id boot_stats_dt_match[] = {
>> +	{ .compatible = "qcom,sm8450-bootstats" },
> 
> This driver doesn't only support sm8450, I've tested this on sdm845 and
> it works just fine. Can we use a generic compatible here instead?

We can add soc specific compatibles here to extend support for other 
socs. This also captures the SoCs for which the driver is supported 
which won't be the case if we use a generic compatible.


>> +	{ }
>> +};
>> +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.7.4
>>
> 

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

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

[Dmitry Baryshkov]

On 17/04/2023 18:08, Souradeep Chowdhury wrote:
> All of Qualcomm's proprietary Android boot-loaders capture boot time
> stats, like the time when the bootloader started execution and at what
> point the bootloader handed over control to the kernel etc. in the IMEM
> region. This information is captured in a specific format by this driver
> by mapping a structure to the IMEM memory region and then accessing the
> members of the structure to show the information within debugfs file.
> This information is useful in verifying if the existing boot KPIs have
> regressed or not. The information is shown in milliseconds, a sample
> log from sm8450(waipio) device is as follows:-
> 
> /sys/kernel/debug/146aa6b0.boot_stats # cat abl_time
> 17898 ms
> /sys/kernel/debug/146aa6b0.boot_stats # cat pre_abl_time
> 2879 ms
> 
> The Module Power Manager(MPM) sleep counter starts ticking at the PBL
> stage and the timestamp generated by the sleep counter is logged by
> the Qualcomm proprietary bootloader(ABL) at two points-> First when it
> starts execution which is logged here as "pre_abl_time" and the second
> when it is about to load the kernel logged as "abl_time". Documentation
> details are also added in Documentation/ABI/testing/debugfs-driver-bootstat
> 
> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> ---
>   Documentation/ABI/testing/debugfs-driver-bootstat |  17 ++++
>   drivers/soc/qcom/Kconfig                          |   9 ++
>   drivers/soc/qcom/Makefile                         |   1 +
>   drivers/soc/qcom/boot_stats.c                     | 101 ++++++++++++++++++++++
>   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 0000000..2543029
> --- /dev/null
> +++ b/Documentation/ABI/testing/debugfs-driver-bootstat
> @@ -0,0 +1,17 @@
> +What:		/sys/kernel/debug/...stats/pre_abl_time
> +Date:           April 2023
> +Contact:        Souradeep Chowdhury <quic_schowdhu@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/...stats/abl_time
> +Date:           April 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 a8f2830..0d2cbd3 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -16,6 +16,15 @@ config QCOM_AOSS_QMP
>   	  subsystems as well as controlling the debug clocks exposed by the Always On
>   	  Subsystem (AOSS) using Qualcomm Messaging Protocol (QMP).
>   
> +config QCOM_BOOTSTAT
> +	tristate "Qualcomm Technologies, Boot Stat driver"
> +	depends on ARCH_QCOM || COMPILE_TEST
> +	help
> +	  This option enables driver support for boot stats. Boot stat driver logs
> +	  the kernel bootloader information by accessing the imem region. These
> +	  information 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 6e88da8..bdaa41a 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -1,6 +1,7 @@
>   # SPDX-License-Identifier: GPL-2.0
>   CFLAGS_rpmh-rsc.o := -I$(src)
>   obj-$(CONFIG_QCOM_AOSS_QMP) +=	qcom_aoss.o
> +obj-$(CONFIG_QCOM_BOOTSTAT) += boot_stats.o
>   obj-$(CONFIG_QCOM_GENI_SE) +=	qcom-geni-se.o
>   obj-$(CONFIG_QCOM_COMMAND_DB) += cmd-db.o
>   obj-$(CONFIG_QCOM_CPR)		+= cpr.o
> diff --git a/drivers/soc/qcom/boot_stats.c b/drivers/soc/qcom/boot_stats.c
> new file mode 100644
> index 0000000..7fa8efb
> --- /dev/null
> +++ b/drivers/soc/qcom/boot_stats.c
> @@ -0,0 +1,101 @@
> +// 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)

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 int abl_time_show(struct seq_file *seq, void *v)
> +{
> +	struct boot_stats *boot_stats = seq->private;
> +	u32 abl_time = TO_MS(boot_stats->abl_end) - TO_MS(boot_stats->abl_start);
> +
> +	seq_printf(seq, "%u ms\n", abl_time);
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(abl_time);
> +
> +static int pre_abl_time_show(struct seq_file *seq, void *v)
> +{
> +	struct boot_stats *boot_stats = seq->private;
> +
> +	seq_printf(seq, "%u ms\n", TO_MS(boot_stats->abl_start));

It would be better to move the unit to the file name and include just 
the number.

> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(pre_abl_time);
> +
> +static int boot_stats_probe(struct platform_device *pdev)
> +{
> +	struct device *bootstat_dev = &pdev->dev;
> +	struct bs_data *drvdata;
> +
> +	drvdata = devm_kzalloc(bootstat_dev, sizeof(*drvdata), GFP_KERNEL);
> +	platform_set_drvdata(pdev, drvdata);
> +
> +	drvdata->dbg_dir = debugfs_create_dir(dev_name(bootstat_dev), NULL);
> +	if (IS_ERR(drvdata->dbg_dir))
> +		return dev_err_probe(bootstat_dev, -ENOENT, "failed to create debugfs directory");
> +
> +	drvdata->b_stats = devm_of_iomap(bootstat_dev, bootstat_dev->of_node, 0, NULL);
> +	if (!drvdata->b_stats)
> +		return dev_err_probe(bootstat_dev, -ENOMEM, "failed to map imem region\n");
> +
> +	debugfs_create_file("pre_abl_time", 0200, drvdata->dbg_dir,
> +			    drvdata->b_stats, &pre_abl_time_fops);
> +	debugfs_create_file("abl_time", 0200, drvdata->dbg_dir, drvdata->b_stats, &abl_time_fops);
> +
> +	return 0;
> +}
> +
> +void boot_stats_remove(struct platform_device *pdev)
> +{
> +	struct bs_data *drvdata = platform_get_drvdata(pdev);
> +
> +	debugfs_remove_recursive(drvdata->dbg_dir);
> +	iounmap(drvdata->b_stats);
> +}
> +
> +static const struct of_device_id boot_stats_dt_match[] = {
> +	{ .compatible = "qcom,sm8450-bootstats" },

Is it specific to sm8450 only?

> +	{ }
> +};
> +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");

-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On 17/04/2023 18:08, Souradeep Chowdhury wrote:
> All of Qualcomm's proprietary Android boot-loaders capture boot time
> stats, like the time when the bootloader started execution and at what
> point the bootloader handed over control to the kernel etc. in the IMEM
> region. This information is captured in a specific format by this driver
> by mapping a structure to the IMEM memory region and then accessing the
> members of the structure to show the information within debugfs file.
> This information is useful in verifying if the existing boot KPIs have
> regressed or not. The information is shown in milliseconds, a sample
> log from sm8450(waipio) device is as follows:-
> 
> /sys/kernel/debug/146aa6b0.boot_stats # cat abl_time
> 17898 ms
> /sys/kernel/debug/146aa6b0.boot_stats # cat pre_abl_time
> 2879 ms
> 
> The Module Power Manager(MPM) sleep counter starts ticking at the PBL
> stage and the timestamp generated by the sleep counter is logged by
> the Qualcomm proprietary bootloader(ABL) at two points-> First when it
> starts execution which is logged here as "pre_abl_time" and the second
> when it is about to load the kernel logged as "abl_time". Documentation
> details are also added in Documentation/ABI/testing/debugfs-driver-bootstat
> 
> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> ---
>   Documentation/ABI/testing/debugfs-driver-bootstat |  17 ++++
>   drivers/soc/qcom/Kconfig                          |   9 ++
>   drivers/soc/qcom/Makefile                         |   1 +
>   drivers/soc/qcom/boot_stats.c                     | 101 ++++++++++++++++++++++
>   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 0000000..2543029
> --- /dev/null
> +++ b/Documentation/ABI/testing/debugfs-driver-bootstat
> @@ -0,0 +1,17 @@
> +What:		/sys/kernel/debug/...stats/pre_abl_time
> +Date:           April 2023
> +Contact:        Souradeep Chowdhury <quic_schowdhu@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/...stats/abl_time
> +Date:           April 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 a8f2830..0d2cbd3 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -16,6 +16,15 @@ config QCOM_AOSS_QMP
>   	  subsystems as well as controlling the debug clocks exposed by the Always On
>   	  Subsystem (AOSS) using Qualcomm Messaging Protocol (QMP).
>   
> +config QCOM_BOOTSTAT
> +	tristate "Qualcomm Technologies, Boot Stat driver"
> +	depends on ARCH_QCOM || COMPILE_TEST
> +	help
> +	  This option enables driver support for boot stats. Boot stat driver logs
> +	  the kernel bootloader information by accessing the imem region. These
> +	  information 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 6e88da8..bdaa41a 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -1,6 +1,7 @@
>   # SPDX-License-Identifier: GPL-2.0
>   CFLAGS_rpmh-rsc.o := -I$(src)
>   obj-$(CONFIG_QCOM_AOSS_QMP) +=	qcom_aoss.o
> +obj-$(CONFIG_QCOM_BOOTSTAT) += boot_stats.o
>   obj-$(CONFIG_QCOM_GENI_SE) +=	qcom-geni-se.o
>   obj-$(CONFIG_QCOM_COMMAND_DB) += cmd-db.o
>   obj-$(CONFIG_QCOM_CPR)		+= cpr.o
> diff --git a/drivers/soc/qcom/boot_stats.c b/drivers/soc/qcom/boot_stats.c
> new file mode 100644
> index 0000000..7fa8efb
> --- /dev/null
> +++ b/drivers/soc/qcom/boot_stats.c
> @@ -0,0 +1,101 @@
> +// 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)

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 int abl_time_show(struct seq_file *seq, void *v)
> +{
> +	struct boot_stats *boot_stats = seq->private;
> +	u32 abl_time = TO_MS(boot_stats->abl_end) - TO_MS(boot_stats->abl_start);
> +
> +	seq_printf(seq, "%u ms\n", abl_time);
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(abl_time);
> +
> +static int pre_abl_time_show(struct seq_file *seq, void *v)
> +{
> +	struct boot_stats *boot_stats = seq->private;
> +
> +	seq_printf(seq, "%u ms\n", TO_MS(boot_stats->abl_start));

It would be better to move the unit to the file name and include just 
the number.

> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(pre_abl_time);
> +
> +static int boot_stats_probe(struct platform_device *pdev)
> +{
> +	struct device *bootstat_dev = &pdev->dev;
> +	struct bs_data *drvdata;
> +
> +	drvdata = devm_kzalloc(bootstat_dev, sizeof(*drvdata), GFP_KERNEL);
> +	platform_set_drvdata(pdev, drvdata);
> +
> +	drvdata->dbg_dir = debugfs_create_dir(dev_name(bootstat_dev), NULL);
> +	if (IS_ERR(drvdata->dbg_dir))
> +		return dev_err_probe(bootstat_dev, -ENOENT, "failed to create debugfs directory");
> +
> +	drvdata->b_stats = devm_of_iomap(bootstat_dev, bootstat_dev->of_node, 0, NULL);
> +	if (!drvdata->b_stats)
> +		return dev_err_probe(bootstat_dev, -ENOMEM, "failed to map imem region\n");
> +
> +	debugfs_create_file("pre_abl_time", 0200, drvdata->dbg_dir,
> +			    drvdata->b_stats, &pre_abl_time_fops);
> +	debugfs_create_file("abl_time", 0200, drvdata->dbg_dir, drvdata->b_stats, &abl_time_fops);
> +
> +	return 0;
> +}
> +
> +void boot_stats_remove(struct platform_device *pdev)
> +{
> +	struct bs_data *drvdata = platform_get_drvdata(pdev);
> +
> +	debugfs_remove_recursive(drvdata->dbg_dir);
> +	iounmap(drvdata->b_stats);
> +}
> +
> +static const struct of_device_id boot_stats_dt_match[] = {
> +	{ .compatible = "qcom,sm8450-bootstats" },

Is it specific to sm8450 only?

> +	{ }
> +};
> +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");

-- 
With best wishes
Dmitry


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

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

[Dmitry Baryshkov]

On 04/05/2023 09:35, Souradeep Chowdhury wrote:
> 
> 
> On 5/4/2023 4:23 AM, Caleb Connolly wrote:
>>
>>
>> On 17/04/2023 16:08, Souradeep Chowdhury wrote:
>>> All of Qualcomm's proprietary Android boot-loaders capture boot time
>>> stats, like the time when the bootloader started execution and at what
>>> point the bootloader handed over control to the kernel etc. in the IMEM
>>> region. This information is captured in a specific format by this driver
>>> by mapping a structure to the IMEM memory region and then accessing the
>>> members of the structure to show the information within debugfs file.
>>> This information is useful in verifying if the existing boot KPIs have
>>> regressed or not. The information is shown in milliseconds, a sample
>>> log from sm8450(waipio) device is as follows:-
>>>
>>> /sys/kernel/debug/146aa6b0.boot_stats # cat abl_time
>>> 17898 ms
>>> /sys/kernel/debug/146aa6b0.boot_stats # cat pre_abl_time
>>> 2879 ms
>>>
>>> The Module Power Manager(MPM) sleep counter starts ticking at the PBL
>>> stage and the timestamp generated by the sleep counter is logged by
>>> the Qualcomm proprietary bootloader(ABL) at two points-> First when it
>>> starts execution which is logged here as "pre_abl_time" and the second
>>> when it is about to load the kernel logged as "abl_time". Documentation
>>> details are also added in 
>>> Documentation/ABI/testing/debugfs-driver-bootstat
>>>
>>> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
>>
>> Hi,
>>
>> [...]
>>> +
>>> +static int boot_stats_probe(struct platform_device *pdev)
>>> +{
>>> +    struct device *bootstat_dev = &pdev->dev;
>>> +    struct bs_data *drvdata;
>>> +
>>> +    drvdata = devm_kzalloc(bootstat_dev, sizeof(*drvdata), GFP_KERNEL);
>>> +    platform_set_drvdata(pdev, drvdata);
>>> +
>>> +    drvdata->dbg_dir = debugfs_create_dir(dev_name(bootstat_dev), 
>>> NULL);
>>
>> This might be better as just "qcom_boot_stats", rather than including
>> the address.
> 
> We usually use the dev_name to represent the one to one correspondence 

Who is "we"?

> of the debugfs file with the device. Will create the root dir as
> "qcom_boot_stats" and push the dev_name dir inside it.

No, this doesn't sound logical. Please use just the "qcom_boot_stats" as 
Caleb suggested.

> 
>>
>> [...]
>>> +
>>> +static const struct of_device_id boot_stats_dt_match[] = {
>>> +    { .compatible = "qcom,sm8450-bootstats" },
>>
>> This driver doesn't only support sm8450, I've tested this on sdm845 and
>> it works just fine. Can we use a generic compatible here instead?
> 
> We can add soc specific compatibles here to extend support for other 
> socs. This also captures the SoCs for which the driver is supported 
> which won't be the case if we use a generic compatible.

No. If there is no difference between SoCs, please don't add 
soc-specific compatibles. They pollute the kernel and provide no 
additional benefits. Please use generic compatible and add 
platform-specific ones only if you have something to override.

> 
> 
>>> +    { }
>>> +};
>>> +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.7.4
>>>
>>

-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On 04/05/2023 09:35, Souradeep Chowdhury wrote:
> 
> 
> On 5/4/2023 4:23 AM, Caleb Connolly wrote:
>>
>>
>> On 17/04/2023 16:08, Souradeep Chowdhury wrote:
>>> All of Qualcomm's proprietary Android boot-loaders capture boot time
>>> stats, like the time when the bootloader started execution and at what
>>> point the bootloader handed over control to the kernel etc. in the IMEM
>>> region. This information is captured in a specific format by this driver
>>> by mapping a structure to the IMEM memory region and then accessing the
>>> members of the structure to show the information within debugfs file.
>>> This information is useful in verifying if the existing boot KPIs have
>>> regressed or not. The information is shown in milliseconds, a sample
>>> log from sm8450(waipio) device is as follows:-
>>>
>>> /sys/kernel/debug/146aa6b0.boot_stats # cat abl_time
>>> 17898 ms
>>> /sys/kernel/debug/146aa6b0.boot_stats # cat pre_abl_time
>>> 2879 ms
>>>
>>> The Module Power Manager(MPM) sleep counter starts ticking at the PBL
>>> stage and the timestamp generated by the sleep counter is logged by
>>> the Qualcomm proprietary bootloader(ABL) at two points-> First when it
>>> starts execution which is logged here as "pre_abl_time" and the second
>>> when it is about to load the kernel logged as "abl_time". Documentation
>>> details are also added in 
>>> Documentation/ABI/testing/debugfs-driver-bootstat
>>>
>>> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
>>
>> Hi,
>>
>> [...]
>>> +
>>> +static int boot_stats_probe(struct platform_device *pdev)
>>> +{
>>> +    struct device *bootstat_dev = &pdev->dev;
>>> +    struct bs_data *drvdata;
>>> +
>>> +    drvdata = devm_kzalloc(bootstat_dev, sizeof(*drvdata), GFP_KERNEL);
>>> +    platform_set_drvdata(pdev, drvdata);
>>> +
>>> +    drvdata->dbg_dir = debugfs_create_dir(dev_name(bootstat_dev), 
>>> NULL);
>>
>> This might be better as just "qcom_boot_stats", rather than including
>> the address.
> 
> We usually use the dev_name to represent the one to one correspondence 

Who is "we"?

> of the debugfs file with the device. Will create the root dir as
> "qcom_boot_stats" and push the dev_name dir inside it.

No, this doesn't sound logical. Please use just the "qcom_boot_stats" as 
Caleb suggested.

> 
>>
>> [...]
>>> +
>>> +static const struct of_device_id boot_stats_dt_match[] = {
>>> +    { .compatible = "qcom,sm8450-bootstats" },
>>
>> This driver doesn't only support sm8450, I've tested this on sdm845 and
>> it works just fine. Can we use a generic compatible here instead?
> 
> We can add soc specific compatibles here to extend support for other 
> socs. This also captures the SoCs for which the driver is supported 
> which won't be the case if we use a generic compatible.

No. If there is no difference between SoCs, please don't add 
soc-specific compatibles. They pollute the kernel and provide no 
additional benefits. Please use generic compatible and add 
platform-specific ones only if you have something to override.

> 
> 
>>> +    { }
>>> +};
>>> +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.7.4
>>>
>>

-- 
With best wishes
Dmitry


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

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

[Dmitry Baryshkov]

On 04/05/2023 09:26, Krzysztof Kozlowski wrote:
> On 04/05/2023 00:10, Caleb Connolly wrote:
>>
>>
>> On 17/04/2023 16:08, 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        | 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 ba694ce..d028bed 100644
>>> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>>> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>>> @@ -49,6 +49,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
>>
>> This region isn't exclusive to sm8450, it exists also on sdm845 and
>> presumably other platforms. Is there any need for an SoC specific
>> compatible?
> 
> Yes.
> https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42
> 
> Also see many discussions on LKML about this.


I checked the closest relative: qcom_stats.c driver. It defines several 
platform-specific overrides and also a generic "qcom,rpm-stats" / 
"qcom,rpmh-stats" drivers.

-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On 04/05/2023 09:26, Krzysztof Kozlowski wrote:
> On 04/05/2023 00:10, Caleb Connolly wrote:
>>
>>
>> On 17/04/2023 16:08, 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        | 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 ba694ce..d028bed 100644
>>> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>>> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>>> @@ -49,6 +49,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
>>
>> This region isn't exclusive to sm8450, it exists also on sdm845 and
>> presumably other platforms. Is there any need for an SoC specific
>> compatible?
> 
> Yes.
> https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42
> 
> Also see many discussions on LKML about this.


I checked the closest relative: qcom_stats.c driver. It defines several 
platform-specific overrides and also a generic "qcom,rpm-stats" / 
"qcom,rpmh-stats" drivers.

-- 
With best wishes
Dmitry


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

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

[Dmitry Baryshkov]

On 04/05/2023 09:26, Krzysztof Kozlowski wrote:
> On 04/05/2023 00:10, Caleb Connolly wrote:
>>
>>
>> On 17/04/2023 16:08, 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        | 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 ba694ce..d028bed 100644
>>> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>>> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>>> @@ -49,6 +49,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
>>
>> This region isn't exclusive to sm8450, it exists also on sdm845 and
>> presumably other platforms. Is there any need for an SoC specific
>> compatible?
> 
> Yes.
> https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42
> 
> Also see many discussions on LKML about this.

After taking another glance at the parent device (IMEM), I start to 
think that we should not be defining the device at all. The imem has the 
SoC name in it. So I think there should be a proper driver for IMEM. 
Then it will instantiate the ABL stats platform device depending on the 
SoC compat. Also this would allow us to rewrite qcom_pil_info_init() in 
a way to query IMEM instead of poking into DT directly.

-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On 04/05/2023 09:26, Krzysztof Kozlowski wrote:
> On 04/05/2023 00:10, Caleb Connolly wrote:
>>
>>
>> On 17/04/2023 16:08, 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        | 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 ba694ce..d028bed 100644
>>> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>>> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>>> @@ -49,6 +49,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
>>
>> This region isn't exclusive to sm8450, it exists also on sdm845 and
>> presumably other platforms. Is there any need for an SoC specific
>> compatible?
> 
> Yes.
> https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42
> 
> Also see many discussions on LKML about this.

After taking another glance at the parent device (IMEM), I start to 
think that we should not be defining the device at all. The imem has the 
SoC name in it. So I think there should be a proper driver for IMEM. 
Then it will instantiate the ABL stats platform device depending on the 
SoC compat. Also this would allow us to rewrite qcom_pil_info_init() in 
a way to query IMEM instead of poking into DT directly.

-- 
With best wishes
Dmitry


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

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

[Caleb Connolly]

On 04/05/2023 17:26, Dmitry Baryshkov wrote:
> On 04/05/2023 09:26, Krzysztof Kozlowski wrote:
>> On 04/05/2023 00:10, Caleb Connolly wrote:
>>>
>>>
>>> On 17/04/2023 16:08, Souradeep Chowdhury wrote:
>>>> +
>>>> +    properties:
>>>> +      compatible:
>>>> +        items:
>>>> +          - enum:
>>>> +              - qcom,sm8450-bootstats
>>>
>>> This region isn't exclusive to sm8450, it exists also on sdm845 and
>>> presumably other platforms. Is there any need for an SoC specific
>>> compatible?
>>
>> Yes.
>> https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42
>>
>> Also see many discussions on LKML about this.

Ah, thanks both for the clarification and apologies for the confusion.
My main concern is that this binding and the associated driver work just
fine on sdm845 with no additional changes. Is it acceptable then to use
the qcom,sm8450-bootstats compatible there? If not, then for someone to
enable this driver on sdm845 would require not just a patch to add the
DT node, but also a patch to dt-bindings to add a compatible AND a patch
to the driver to use it.

Based on Dmitry's response on the driver patch [1], perhaps adding the
catch-all "qcom,imem-bootstats" compatible to the driver would be
suitable. If there are SoC specific parts in the future then match data
or gating with of_device_is_compatible() can be used with the SoC
specific compatible. This is how the qcom_stats driver handles things,
would this be an OK solution for everyone?

> 
> After taking another glance at the parent device (IMEM), I start to
> think that we should not be defining the device at all. The imem has the
> SoC name in it. So I think there should be a proper driver for IMEM.
> Then it will instantiate the ABL stats platform device depending on the
> SoC compat. Also this would allow us to rewrite qcom_pil_info_init() in
> a way to query IMEM instead of poking into DT directly.

+1 for handling this automagically in driver code, I'd be happy to look
into this in the future.

[1]
https://lore.kernel.org/linux-arm-msm/35ac64ab-512d-1425-7a1b-6e8d3806c8a8@linaro.org/
> 

-- 
Kind Regards,
Caleb (they/them)

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

[Caleb Connolly]

On 04/05/2023 17:26, Dmitry Baryshkov wrote:
> On 04/05/2023 09:26, Krzysztof Kozlowski wrote:
>> On 04/05/2023 00:10, Caleb Connolly wrote:
>>>
>>>
>>> On 17/04/2023 16:08, Souradeep Chowdhury wrote:
>>>> +
>>>> +    properties:
>>>> +      compatible:
>>>> +        items:
>>>> +          - enum:
>>>> +              - qcom,sm8450-bootstats
>>>
>>> This region isn't exclusive to sm8450, it exists also on sdm845 and
>>> presumably other platforms. Is there any need for an SoC specific
>>> compatible?
>>
>> Yes.
>> https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42
>>
>> Also see many discussions on LKML about this.

Ah, thanks both for the clarification and apologies for the confusion.
My main concern is that this binding and the associated driver work just
fine on sdm845 with no additional changes. Is it acceptable then to use
the qcom,sm8450-bootstats compatible there? If not, then for someone to
enable this driver on sdm845 would require not just a patch to add the
DT node, but also a patch to dt-bindings to add a compatible AND a patch
to the driver to use it.

Based on Dmitry's response on the driver patch [1], perhaps adding the
catch-all "qcom,imem-bootstats" compatible to the driver would be
suitable. If there are SoC specific parts in the future then match data
or gating with of_device_is_compatible() can be used with the SoC
specific compatible. This is how the qcom_stats driver handles things,
would this be an OK solution for everyone?

> 
> After taking another glance at the parent device (IMEM), I start to
> think that we should not be defining the device at all. The imem has the
> SoC name in it. So I think there should be a proper driver for IMEM.
> Then it will instantiate the ABL stats platform device depending on the
> SoC compat. Also this would allow us to rewrite qcom_pil_info_init() in
> a way to query IMEM instead of poking into DT directly.

+1 for handling this automagically in driver code, I'd be happy to look
into this in the future.

[1]
https://lore.kernel.org/linux-arm-msm/35ac64ab-512d-1425-7a1b-6e8d3806c8a8@linaro.org/
> 

-- 
Kind Regards,
Caleb (they/them)

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

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

[Souradeep Chowdhury]

On 5/4/2023 9:49 PM, Dmitry Baryshkov wrote:
> On 04/05/2023 09:35, Souradeep Chowdhury wrote:
>>
>>
>> On 5/4/2023 4:23 AM, Caleb Connolly wrote:
>>>
>>>
>>> On 17/04/2023 16:08, Souradeep Chowdhury wrote:
>>>> All of Qualcomm's proprietary Android boot-loaders capture boot time
>>>> stats, like the time when the bootloader started execution and at what
>>>> point the bootloader handed over control to the kernel etc. in the IMEM
>>>> region. This information is captured in a specific format by this 
>>>> driver
>>>> by mapping a structure to the IMEM memory region and then accessing the
>>>> members of the structure to show the information within debugfs file.
>>>> This information is useful in verifying if the existing boot KPIs have
>>>> regressed or not. The information is shown in milliseconds, a sample
>>>> log from sm8450(waipio) device is as follows:-
>>>>
>>>> /sys/kernel/debug/146aa6b0.boot_stats # cat abl_time
>>>> 17898 ms
>>>> /sys/kernel/debug/146aa6b0.boot_stats # cat pre_abl_time
>>>> 2879 ms
>>>>
>>>> The Module Power Manager(MPM) sleep counter starts ticking at the PBL
>>>> stage and the timestamp generated by the sleep counter is logged by
>>>> the Qualcomm proprietary bootloader(ABL) at two points-> First when it
>>>> starts execution which is logged here as "pre_abl_time" and the second
>>>> when it is about to load the kernel logged as "abl_time". Documentation
>>>> details are also added in 
>>>> Documentation/ABI/testing/debugfs-driver-bootstat
>>>>
>>>> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
>>>
>>> Hi,
>>>
>>> [...]
>>>> +
>>>> +static int boot_stats_probe(struct platform_device *pdev)
>>>> +{
>>>> +    struct device *bootstat_dev = &pdev->dev;
>>>> +    struct bs_data *drvdata;
>>>> +
>>>> +    drvdata = devm_kzalloc(bootstat_dev, sizeof(*drvdata), 
>>>> GFP_KERNEL);
>>>> +    platform_set_drvdata(pdev, drvdata);
>>>> +
>>>> +    drvdata->dbg_dir = debugfs_create_dir(dev_name(bootstat_dev), 
>>>> NULL);
>>>
>>> This might be better as just "qcom_boot_stats", rather than including
>>> the address.
>>
>> We usually use the dev_name to represent the one to one correspondence 
> 
> Who is "we"?
> 
>> of the debugfs file with the device. Will create the root dir as
>> "qcom_boot_stats" and push the dev_name dir inside it.
> 
> No, this doesn't sound logical. Please use just the "qcom_boot_stats" as 
> Caleb suggested.

Ack

> 
>>
>>>
>>> [...]
>>>> +
>>>> +static const struct of_device_id boot_stats_dt_match[] = {
>>>> +    { .compatible = "qcom,sm8450-bootstats" },
>>>
>>> This driver doesn't only support sm8450, I've tested this on sdm845 and
>>> it works just fine. Can we use a generic compatible here instead?
>>
>> We can add soc specific compatibles here to extend support for other 
>> socs. This also captures the SoCs for which the driver is supported 
>> which won't be the case if we use a generic compatible.
> 
> No. If there is no difference between SoCs, please don't add 
> soc-specific compatibles. They pollute the kernel and provide no 
> additional benefits. Please use generic compatible and add 
> platform-specific ones only if you have something to override.

Ack
> 
>>
>>
>>>> +    { }
>>>> +};
>>>> +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.7.4
>>>>
>>>
>