[PATCH V1 0/4] soc: qcom: boot_stats: Add driver support for boot_stats

[PATCH V1 0/4] soc: qcom: boot_stats: Add driver support for boot_stats

[Souradeep Chowdhury]

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

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

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

Souradeep Chowdhury (4):
  dt-bindings: sram: qcom,imem: Add Boot Stat region within IMEM
  dt-bindings: soc: qcom,mpm-sleep-counter: Add the dtschema
  soc: qcom: boot_stat: Add Driver Support for Boot Stats
  MAINTAINERS: Add the entry for boot_stats driver support

 .../bindings/soc/qcom/qcom,mpm-sleep-counter.yaml  |  40 ++++++++
 .../devicetree/bindings/sram/qcom,imem.yaml        |  20 ++++
 MAINTAINERS                                        |   7 ++
 drivers/soc/qcom/Kconfig                           |   7 ++
 drivers/soc/qcom/Makefile                          |   1 +
 drivers/soc/qcom/boot_stats.c                      | 108 +++++++++++++++++++++
 6 files changed, 183 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
 create mode 100644 drivers/soc/qcom/boot_stats.c

-- 
2.7.4

[PATCH V1 0/4] soc: qcom: boot_stats: Add driver support for boot_stats

[Souradeep Chowdhury]

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

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

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

Souradeep Chowdhury (4):
  dt-bindings: sram: qcom,imem: Add Boot Stat region within IMEM
  dt-bindings: soc: qcom,mpm-sleep-counter: Add the dtschema
  soc: qcom: boot_stat: Add Driver Support for Boot Stats
  MAINTAINERS: Add the entry for boot_stats driver support

 .../bindings/soc/qcom/qcom,mpm-sleep-counter.yaml  |  40 ++++++++
 .../devicetree/bindings/sram/qcom,imem.yaml        |  20 ++++
 MAINTAINERS                                        |   7 ++
 drivers/soc/qcom/Kconfig                           |   7 ++
 drivers/soc/qcom/Makefile                          |   1 +
 drivers/soc/qcom/boot_stats.c                      | 108 +++++++++++++++++++++
 6 files changed, 183 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
 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 V1 1/4] 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          | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
index 665c06e..c8c3890 100644
--- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
+++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
@@ -48,6 +48,26 @@ patternProperties:
     $ref: /schemas/remoteproc/qcom,pil-info.yaml#
     description: Peripheral image loader relocation region
 
+  "^boot-stat@[0-9a-f]+$":
+    type: object
+    description:
+      node for boot stat.
+
+    properties:
+      compatible:
+        items:
+          - const: qcom,imem-boot_stats
+
+      reg:
+        maxItems: 1
+        description:
+          The base address of the register region in case of
+          imem boot stats.
+
+    required:
+      - compatible
+      - reg
+
 required:
   - compatible
   - reg
-- 
2.7.4

[PATCH V1 1/4] 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          | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
index 665c06e..c8c3890 100644
--- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
+++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
@@ -48,6 +48,26 @@ patternProperties:
     $ref: /schemas/remoteproc/qcom,pil-info.yaml#
     description: Peripheral image loader relocation region
 
+  "^boot-stat@[0-9a-f]+$":
+    type: object
+    description:
+      node for boot stat.
+
+    properties:
+      compatible:
+        items:
+          - const: qcom,imem-boot_stats
+
+      reg:
+        maxItems: 1
+        description:
+          The base address of the register region in case of
+          imem boot stats.
+
+    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 V1 2/4] dt-bindings: soc: qcom,mpm-sleep-counter: Add the dtschema

[Souradeep Chowdhury]

Add the device tree bindings for the module power manager sleep
counter.

Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
---
 .../bindings/soc/qcom/qcom,mpm-sleep-counter.yaml  | 40 ++++++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml

diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
new file mode 100644
index 0000000..f9f46b7
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
@@ -0,0 +1,40 @@
+# SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/soc/qcom/qcom,mpm-sleep-counter.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MPM Sleep Counter
+
+maintainers:
+  - Souradeep Chowdhury <quic_schowdhu@quicinc.com>
+
+description: |
+    MPM(Module Power Manager) has a sleep counter which is used to track
+    various stages of the boot process in Qualcomm.
+
+properties:
+  compatible:
+    items:
+      - const: qcom,mpm2-sleep-counter
+
+  reg:
+    items:
+      - description: MPM Sleep Counter Base
+
+  clock-frequency:
+    description: Frequency for the sleep counter
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    mpm2-sleep-counter@c221000{
+       compatible = "qcom,mpm2-sleep-counter";
+       reg = <0xc221000 0x1000>;
+       clock-frequency = <32768>;
+    };
-- 
2.7.4

[PATCH V1 2/4] dt-bindings: soc: qcom,mpm-sleep-counter: Add the dtschema

[Souradeep Chowdhury]

Add the device tree bindings for the module power manager sleep
counter.

Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
---
 .../bindings/soc/qcom/qcom,mpm-sleep-counter.yaml  | 40 ++++++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml

diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
new file mode 100644
index 0000000..f9f46b7
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
@@ -0,0 +1,40 @@
+# SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/soc/qcom/qcom,mpm-sleep-counter.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MPM Sleep Counter
+
+maintainers:
+  - Souradeep Chowdhury <quic_schowdhu@quicinc.com>
+
+description: |
+    MPM(Module Power Manager) has a sleep counter which is used to track
+    various stages of the boot process in Qualcomm.
+
+properties:
+  compatible:
+    items:
+      - const: qcom,mpm2-sleep-counter
+
+  reg:
+    items:
+      - description: MPM Sleep Counter Base
+
+  clock-frequency:
+    description: Frequency for the sleep counter
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    mpm2-sleep-counter@c221000{
+       compatible = "qcom,mpm2-sleep-counter";
+       reg = <0xc221000 0x1000>;
+       clock-frequency = <32768>;
+    };
-- 
2.7.4


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

[PATCH V1 3/4] soc: qcom: boot_stat: Add Driver Support for Boot Stats

[Souradeep Chowdhury]

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

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

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

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

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index d11bda2..2cfdbb7 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -79,6 +79,13 @@ config QCOM_DCC
 	  driver provides interface to configure DCC block and read back
 	  captured data from DCC's internal SRAM.

+config QCOM_BOOTSTAT
+	tristate "Qualcomm Technologies, Boot Stat driver"
+	depends on ARCH_QCOM || COMPILE_TEST
+	help
+	  This option enables driver for boot stats. Boot stat driver prints
+	  the kernel bootloader information by accessing the imem region.
+
 config QCOM_KRYO_L2_ACCESSORS
 	bool
 	depends on ARCH_QCOM && ARM64 || COMPILE_TEST
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 3b92c6c..e9b1e52 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_QCOM_GENI_SE) +=	qcom-geni-se.o
 obj-$(CONFIG_QCOM_COMMAND_DB) += cmd-db.o
 obj-$(CONFIG_QCOM_CPR)		+= cpr.o
 obj-$(CONFIG_QCOM_DCC) += dcc.o
+obj-$(CONFIG_QCOM_BOOTSTAT) += boot_stats.o
 obj-$(CONFIG_QCOM_GSBI)	+=	qcom_gsbi.o
 obj-$(CONFIG_QCOM_MDT_LOADER)	+= mdt_loader.o
 obj-$(CONFIG_QCOM_OCMEM)	+= ocmem.o
diff --git a/drivers/soc/qcom/boot_stats.c b/drivers/soc/qcom/boot_stats.c
new file mode 100644
index 0000000..59b5ab6
--- /dev/null
+++ b/drivers/soc/qcom/boot_stats.c
@@ -0,0 +1,108 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2013-2019, 2021 The Linux Foundation. All rights reserved.
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+
+/**
+ *  struct boot_stats - timestamp information related to boot stats
+ *  @bootloader_start:	Time for the starting point of the abl bootloader
+ *  @bootloader_end:	Time when the kernel starts loading from abl bootloader
+ */
+struct boot_stats {
+	u32 bootloader_start;
+	u32 bootloader_end;
+} __packed;
+
+static struct boot_stats __iomem *boot_stats;
+static void __iomem *mpm_counter_base;
+static u32 mpm_counter_freq;
+
+static int mpm_parse_dt(void)
+{
+	struct device_node *np_imem, *np_mpm2;
+
+	np_imem = of_find_compatible_node(NULL, NULL,
+					  "qcom,imem-boot_stats");
+	if (!np_imem) {
+		pr_err("can't find qcom,imem node\n");
+		return -ENODEV;
+	}
+	boot_stats = of_iomap(np_imem, 0);
+	if (!boot_stats) {
+		pr_err("boot_stats: Can't map imem\n");
+		goto err1;
+	}
+
+	np_mpm2 = of_find_compatible_node(NULL, NULL,
+					  "qcom,mpm2-sleep-counter");
+	if (!np_mpm2) {
+		pr_err("mpm_counter: can't find DT node\n");
+		goto err2;
+	}
+
+	if (of_property_read_u32(np_mpm2, "clock-frequency", &mpm_counter_freq))
+		goto err2;
+
+	if (of_get_address(np_mpm2, 0, NULL, NULL)) {
+		mpm_counter_base = of_iomap(np_mpm2, 0);
+		if (!mpm_counter_base) {
+			pr_err("mpm_counter: can't map counter base\n");
+			goto err2;
+		}
+	} else {
+		goto err2;
+	}
+
+	return 0;
+
+err2:
+	of_node_put(np_mpm2);
+err1:
+	of_node_put(np_imem);
+	return -ENODEV;
+}
+
+static void print_boot_stats(void)
+{
+	u32 pre_abl_time = readl_relaxed(&boot_stats->bootloader_start) / mpm_counter_freq;
+	u32 abl_time = readl_relaxed(&boot_stats->bootloader_end) / mpm_counter_freq;
+
+	pr_info("KPI: Pre ABL Time = %us\n", pre_abl_time);
+	pr_info("KPI: ABL Time = %us\n", abl_time);
+	pr_info("KPI: Kernel MPM timestamp = %u\n", readl_relaxed(mpm_counter_base));
+}
+
+static int __init boot_stats_init(void)
+{
+	int ret;
+
+	ret = mpm_parse_dt();
+	if (ret < 0)
+		return -ENODEV;
+
+	print_boot_stats();
+
+	iounmap(boot_stats);
+	iounmap(mpm_counter_base);
+
+	return 0;
+}
+module_init(boot_stats_init);
+
+static void __exit boot_stats_exit(void)
+{
+}
+module_exit(boot_stats_exit)
+
+MODULE_DESCRIPTION("Qualcomm Technologies Inc. Boot Stat driver");
+MODULE_LICENSE("GPL");
--
2.7.4

[PATCH V1 3/4] soc: qcom: boot_stat: Add Driver Support for Boot Stats

[Souradeep Chowdhury]

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

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

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

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

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index d11bda2..2cfdbb7 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -79,6 +79,13 @@ config QCOM_DCC
 	  driver provides interface to configure DCC block and read back
 	  captured data from DCC's internal SRAM.

+config QCOM_BOOTSTAT
+	tristate "Qualcomm Technologies, Boot Stat driver"
+	depends on ARCH_QCOM || COMPILE_TEST
+	help
+	  This option enables driver for boot stats. Boot stat driver prints
+	  the kernel bootloader information by accessing the imem region.
+
 config QCOM_KRYO_L2_ACCESSORS
 	bool
 	depends on ARCH_QCOM && ARM64 || COMPILE_TEST
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 3b92c6c..e9b1e52 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_QCOM_GENI_SE) +=	qcom-geni-se.o
 obj-$(CONFIG_QCOM_COMMAND_DB) += cmd-db.o
 obj-$(CONFIG_QCOM_CPR)		+= cpr.o
 obj-$(CONFIG_QCOM_DCC) += dcc.o
+obj-$(CONFIG_QCOM_BOOTSTAT) += boot_stats.o
 obj-$(CONFIG_QCOM_GSBI)	+=	qcom_gsbi.o
 obj-$(CONFIG_QCOM_MDT_LOADER)	+= mdt_loader.o
 obj-$(CONFIG_QCOM_OCMEM)	+= ocmem.o
diff --git a/drivers/soc/qcom/boot_stats.c b/drivers/soc/qcom/boot_stats.c
new file mode 100644
index 0000000..59b5ab6
--- /dev/null
+++ b/drivers/soc/qcom/boot_stats.c
@@ -0,0 +1,108 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2013-2019, 2021 The Linux Foundation. All rights reserved.
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+
+/**
+ *  struct boot_stats - timestamp information related to boot stats
+ *  @bootloader_start:	Time for the starting point of the abl bootloader
+ *  @bootloader_end:	Time when the kernel starts loading from abl bootloader
+ */
+struct boot_stats {
+	u32 bootloader_start;
+	u32 bootloader_end;
+} __packed;
+
+static struct boot_stats __iomem *boot_stats;
+static void __iomem *mpm_counter_base;
+static u32 mpm_counter_freq;
+
+static int mpm_parse_dt(void)
+{
+	struct device_node *np_imem, *np_mpm2;
+
+	np_imem = of_find_compatible_node(NULL, NULL,
+					  "qcom,imem-boot_stats");
+	if (!np_imem) {
+		pr_err("can't find qcom,imem node\n");
+		return -ENODEV;
+	}
+	boot_stats = of_iomap(np_imem, 0);
+	if (!boot_stats) {
+		pr_err("boot_stats: Can't map imem\n");
+		goto err1;
+	}
+
+	np_mpm2 = of_find_compatible_node(NULL, NULL,
+					  "qcom,mpm2-sleep-counter");
+	if (!np_mpm2) {
+		pr_err("mpm_counter: can't find DT node\n");
+		goto err2;
+	}
+
+	if (of_property_read_u32(np_mpm2, "clock-frequency", &mpm_counter_freq))
+		goto err2;
+
+	if (of_get_address(np_mpm2, 0, NULL, NULL)) {
+		mpm_counter_base = of_iomap(np_mpm2, 0);
+		if (!mpm_counter_base) {
+			pr_err("mpm_counter: can't map counter base\n");
+			goto err2;
+		}
+	} else {
+		goto err2;
+	}
+
+	return 0;
+
+err2:
+	of_node_put(np_mpm2);
+err1:
+	of_node_put(np_imem);
+	return -ENODEV;
+}
+
+static void print_boot_stats(void)
+{
+	u32 pre_abl_time = readl_relaxed(&boot_stats->bootloader_start) / mpm_counter_freq;
+	u32 abl_time = readl_relaxed(&boot_stats->bootloader_end) / mpm_counter_freq;
+
+	pr_info("KPI: Pre ABL Time = %us\n", pre_abl_time);
+	pr_info("KPI: ABL Time = %us\n", abl_time);
+	pr_info("KPI: Kernel MPM timestamp = %u\n", readl_relaxed(mpm_counter_base));
+}
+
+static int __init boot_stats_init(void)
+{
+	int ret;
+
+	ret = mpm_parse_dt();
+	if (ret < 0)
+		return -ENODEV;
+
+	print_boot_stats();
+
+	iounmap(boot_stats);
+	iounmap(mpm_counter_base);
+
+	return 0;
+}
+module_init(boot_stats_init);
+
+static void __exit boot_stats_exit(void)
+{
+}
+module_exit(boot_stats_exit)
+
+MODULE_DESCRIPTION("Qualcomm Technologies Inc. Boot Stat driver");
+MODULE_LICENSE("GPL");
--
2.7.4


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

[PATCH V1 4/4] 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 f2c3d47..6a55577 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17071,6 +17071,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/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
+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 V1 4/4] 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 f2c3d47..6a55577 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17071,6 +17071,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/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
+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 V1 1/4] dt-bindings: sram: qcom,imem: Add Boot Stat region within IMEM

[Krzysztof Kozlowski]

On 21/03/2023 14:51, 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          | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> index 665c06e..c8c3890 100644
> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> @@ -48,6 +48,26 @@ patternProperties:
>      $ref: /schemas/remoteproc/qcom,pil-info.yaml#
>      description: Peripheral image loader relocation region
>  
> +  "^boot-stat@[0-9a-f]+$":
> +    type: object
> +    description:
> +      node for boot stat.

This basically copies the name of node, so not really helpful. Describe
what's this.

> +

additionalProperties: false

> +    properties:
> +      compatible:
> +        items:

Drop items.

> +          - const: qcom,imem-boot_stats

No underscores in compatibles. Why this is not SoC specific compatible?

> +
> +      reg:
> +        maxItems: 1
> +        description:
> +          The base address of the register region in case of
> +          imem boot stats.

Drop description, it's obvious.
> +
> +    required:
> +      - compatible
> +      - reg
> +
Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 21/03/2023 14:51, 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          | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> index 665c06e..c8c3890 100644
> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> @@ -48,6 +48,26 @@ patternProperties:
>      $ref: /schemas/remoteproc/qcom,pil-info.yaml#
>      description: Peripheral image loader relocation region
>  
> +  "^boot-stat@[0-9a-f]+$":
> +    type: object
> +    description:
> +      node for boot stat.

This basically copies the name of node, so not really helpful. Describe
what's this.

> +

additionalProperties: false

> +    properties:
> +      compatible:
> +        items:

Drop items.

> +          - const: qcom,imem-boot_stats

No underscores in compatibles. Why this is not SoC specific compatible?

> +
> +      reg:
> +        maxItems: 1
> +        description:
> +          The base address of the register region in case of
> +          imem boot stats.

Drop description, it's obvious.
> +
> +    required:
> +      - compatible
> +      - reg
> +
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 V1 2/4] dt-bindings: soc: qcom,mpm-sleep-counter: Add the dtschema

[Krzysztof Kozlowski]

On 21/03/2023 14:51, Souradeep Chowdhury wrote:
> Add the device tree bindings for the module power manager sleep
> counter.
> 
> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> ---
>  .../bindings/soc/qcom/qcom,mpm-sleep-counter.yaml  | 40 ++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
> 
> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
> new file mode 100644
> index 0000000..f9f46b7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
> @@ -0,0 +1,40 @@
> +# SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/soc/qcom/qcom,mpm-sleep-counter.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MPM Sleep Counter
> +
> +maintainers:
> +  - Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> +
> +description: |
> +    MPM(Module Power Manager) has a sleep counter which is used to track

Missing space:
MPM (Module

> +    various stages of the boot process in Qualcomm.

in Qualcomm SoC. Because you do not track it in the company...

> +
> +properties:
> +  compatible:
> +    items:

Drop items.

> +      - const: qcom,mpm2-sleep-counter

SoC specific compatible.

> +
> +  reg:
> +    items:
> +      - description: MPM Sleep Counter Base

just maxItems: 1

> +
> +  clock-frequency:
> +    description: Frequency for the sleep counter

Since this does not have clocks, what frequency you are setting here?

> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    mpm2-sleep-counter@c221000{

Node names should be generic.
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

> +       compatible = "qcom,mpm2-sleep-counter";
> +       reg = <0xc221000 0x1000>;
> +       clock-frequency = <32768>;
> +    };

Best regards,
Krzysztof

Re: [PATCH V1 2/4] dt-bindings: soc: qcom,mpm-sleep-counter: Add the dtschema

[Krzysztof Kozlowski]

On 21/03/2023 14:51, Souradeep Chowdhury wrote:
> Add the device tree bindings for the module power manager sleep
> counter.
> 
> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> ---
>  .../bindings/soc/qcom/qcom,mpm-sleep-counter.yaml  | 40 ++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
> 
> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
> new file mode 100644
> index 0000000..f9f46b7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
> @@ -0,0 +1,40 @@
> +# SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/soc/qcom/qcom,mpm-sleep-counter.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MPM Sleep Counter
> +
> +maintainers:
> +  - Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> +
> +description: |
> +    MPM(Module Power Manager) has a sleep counter which is used to track

Missing space:
MPM (Module

> +    various stages of the boot process in Qualcomm.

in Qualcomm SoC. Because you do not track it in the company...

> +
> +properties:
> +  compatible:
> +    items:

Drop items.

> +      - const: qcom,mpm2-sleep-counter

SoC specific compatible.

> +
> +  reg:
> +    items:
> +      - description: MPM Sleep Counter Base

just maxItems: 1

> +
> +  clock-frequency:
> +    description: Frequency for the sleep counter

Since this does not have clocks, what frequency you are setting here?

> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    mpm2-sleep-counter@c221000{

Node names should be generic.
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

> +       compatible = "qcom,mpm2-sleep-counter";
> +       reg = <0xc221000 0x1000>;
> +       clock-frequency = <32768>;
> +    };

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

[Krzysztof Kozlowski]

On 21/03/2023 14:51, Souradeep Chowdhury wrote:
> All of Qualcomm's proprietary Android boot-loaders capture boot time
> stats, like the time when the bootloader started execution and at what
> point the bootloader handed over control to the kernel etc. in the IMEM
> region. This information is captured in a specific format by this driver
> by mapping a structure to the IMEM memory region and then accessing the
> members of the structure to print the information. This information is
> useful in verifying if the existing boot KPIs have regre


> +/**
> + *  struct boot_stats - timestamp information related to boot stats
> + *  @bootloader_start:	Time for the starting point of the abl bootloader
> + *  @bootloader_end:	Time when the kernel starts loading from abl bootloader
> + */
> +struct boot_stats {
> +	u32 bootloader_start;
> +	u32 bootloader_end;
> +} __packed;
> +
> +static struct boot_stats __iomem *boot_stats;
> +static void __iomem *mpm_counter_base;
> +static u32 mpm_counter_freq;

No file-scope variables. Does not scale, not easy for review and
maintenance. Avoid such code.

> +
> +static int mpm_parse_dt(void)
> +{
> +	struct device_node *np_imem, *np_mpm2;
> +
> +	np_imem = of_find_compatible_node(NULL, NULL,
> +					  "qcom,imem-boot_stats");
> +	if (!np_imem) {
> +		pr_err("can't find qcom,imem node\n");

So you are printing errors everywhere, on every soc and with compile
test on every platform there is in the world... sorry, it does not work
like that.

> +		return -ENODEV;
> +	}
> +	boot_stats = of_iomap(np_imem, 0);
> +	if (!boot_stats) {
> +		pr_err("boot_stats: Can't map imem\n");
> +		goto err1;
> +	}


> +
> +static void __exit boot_stats_exit(void)
> +{
> +}
> +module_exit(boot_stats_exit)


I don't think this is some special code which deserves init calls. Make
it module_platform_driver().


Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 21/03/2023 14:51, Souradeep Chowdhury wrote:
> All of Qualcomm's proprietary Android boot-loaders capture boot time
> stats, like the time when the bootloader started execution and at what
> point the bootloader handed over control to the kernel etc. in the IMEM
> region. This information is captured in a specific format by this driver
> by mapping a structure to the IMEM memory region and then accessing the
> members of the structure to print the information. This information is
> useful in verifying if the existing boot KPIs have regre


> +/**
> + *  struct boot_stats - timestamp information related to boot stats
> + *  @bootloader_start:	Time for the starting point of the abl bootloader
> + *  @bootloader_end:	Time when the kernel starts loading from abl bootloader
> + */
> +struct boot_stats {
> +	u32 bootloader_start;
> +	u32 bootloader_end;
> +} __packed;
> +
> +static struct boot_stats __iomem *boot_stats;
> +static void __iomem *mpm_counter_base;
> +static u32 mpm_counter_freq;

No file-scope variables. Does not scale, not easy for review and
maintenance. Avoid such code.

> +
> +static int mpm_parse_dt(void)
> +{
> +	struct device_node *np_imem, *np_mpm2;
> +
> +	np_imem = of_find_compatible_node(NULL, NULL,
> +					  "qcom,imem-boot_stats");
> +	if (!np_imem) {
> +		pr_err("can't find qcom,imem node\n");

So you are printing errors everywhere, on every soc and with compile
test on every platform there is in the world... sorry, it does not work
like that.

> +		return -ENODEV;
> +	}
> +	boot_stats = of_iomap(np_imem, 0);
> +	if (!boot_stats) {
> +		pr_err("boot_stats: Can't map imem\n");
> +		goto err1;
> +	}


> +
> +static void __exit boot_stats_exit(void)
> +{
> +}
> +module_exit(boot_stats_exit)


I don't think this is some special code which deserves init calls. Make
it module_platform_driver().


Best regards,
Krzysztof


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

Re: [PATCH V1 2/4] dt-bindings: soc: qcom,mpm-sleep-counter: Add the dtschema

[Krzysztof Kozlowski]

On 21/03/2023 14:51, Souradeep Chowdhury wrote:
> Add the device tree bindings for the module power manager sleep
> counter.
> 
> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> ---
>  .../bindings/soc/qcom/qcom,mpm-sleep-counter.yaml  | 40 ++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
> 
> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
> new file mode 100644
> index 0000000..f9f46b7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml

Your code a bit explains this... this is not a device but some memory
region. Does not look like suitable for bindings as separate device.

Best regards,
Krzysztof

Re: [PATCH V1 2/4] dt-bindings: soc: qcom,mpm-sleep-counter: Add the dtschema

[Krzysztof Kozlowski]

On 21/03/2023 14:51, Souradeep Chowdhury wrote:
> Add the device tree bindings for the module power manager sleep
> counter.
> 
> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> ---
>  .../bindings/soc/qcom/qcom,mpm-sleep-counter.yaml  | 40 ++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
> 
> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
> new file mode 100644
> index 0000000..f9f46b7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml

Your code a bit explains this... this is not a device but some memory
region. Does not look like suitable for bindings as separate device.

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 V1 1/4] dt-bindings: sram: qcom,imem: Add Boot Stat region within IMEM

[Souradeep Chowdhury]

On 3/21/2023 11:01 PM, Krzysztof Kozlowski wrote:
> On 21/03/2023 14:51, 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          | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>> index 665c06e..c8c3890 100644
>> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>> @@ -48,6 +48,26 @@ patternProperties:
>>       $ref: /schemas/remoteproc/qcom,pil-info.yaml#
>>       description: Peripheral image loader relocation region
>>   
>> +  "^boot-stat@[0-9a-f]+$":
>> +    type: object
>> +    description:
>> +      node for boot stat.
> 
> This basically copies the name of node, so not really helpful. Describe
> what's this.

Ack
> 
>> +
> 
> additionalProperties: false
> 
>> +    properties:
>> +      compatible:
>> +        items:
> 
> Drop items.
> 
>> +          - const: qcom,imem-boot_stats
> 
> No underscores in compatibles. Why this is not SoC specific compatible?

Ack. The boot_stats module is not specific to a device. It is written to
read some values from this imem region which is present for almost all 
QCOM SoCs. So SoC specific compatible is not given in this case.

> 
>> +
>> +      reg:
>> +        maxItems: 1
>> +        description:
>> +          The base address of the register region in case of
>> +          imem boot stats.
> 
> Drop description, it's obvious.

Ack
>> +
>> +    required:
>> +      - compatible
>> +      - reg
>> +
> Best regards,
> Krzysztof
> 

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

[Souradeep Chowdhury]

On 3/21/2023 11:01 PM, Krzysztof Kozlowski wrote:
> On 21/03/2023 14:51, 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          | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>> index 665c06e..c8c3890 100644
>> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>> @@ -48,6 +48,26 @@ patternProperties:
>>       $ref: /schemas/remoteproc/qcom,pil-info.yaml#
>>       description: Peripheral image loader relocation region
>>   
>> +  "^boot-stat@[0-9a-f]+$":
>> +    type: object
>> +    description:
>> +      node for boot stat.
> 
> This basically copies the name of node, so not really helpful. Describe
> what's this.

Ack
> 
>> +
> 
> additionalProperties: false
> 
>> +    properties:
>> +      compatible:
>> +        items:
> 
> Drop items.
> 
>> +          - const: qcom,imem-boot_stats
> 
> No underscores in compatibles. Why this is not SoC specific compatible?

Ack. The boot_stats module is not specific to a device. It is written to
read some values from this imem region which is present for almost all 
QCOM SoCs. So SoC specific compatible is not given in this case.

> 
>> +
>> +      reg:
>> +        maxItems: 1
>> +        description:
>> +          The base address of the register region in case of
>> +          imem boot stats.
> 
> Drop description, it's obvious.

Ack
>> +
>> +    required:
>> +      - compatible
>> +      - reg
>> +
> 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 V1 2/4] dt-bindings: soc: qcom,mpm-sleep-counter: Add the dtschema

[Souradeep Chowdhury]

On 3/21/2023 11:03 PM, Krzysztof Kozlowski wrote:
> On 21/03/2023 14:51, Souradeep Chowdhury wrote:
>> Add the device tree bindings for the module power manager sleep
>> counter.
>>
>> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
>> ---
>>   .../bindings/soc/qcom/qcom,mpm-sleep-counter.yaml  | 40 ++++++++++++++++++++++
>>   1 file changed, 40 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
>> new file mode 100644
>> index 0000000..f9f46b7
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
>> @@ -0,0 +1,40 @@
>> +# SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/soc/qcom/qcom,mpm-sleep-counter.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: MPM Sleep Counter
>> +
>> +maintainers:
>> +  - Souradeep Chowdhury <quic_schowdhu@quicinc.com>
>> +
>> +description: |
>> +    MPM(Module Power Manager) has a sleep counter which is used to track
> 
> Missing space:
> MPM (Module
> 

Ack
>> +    various stages of the boot process in Qualcomm.
> 
> in Qualcomm SoC. Because you do not track it in the company...
> 

Ack
>> +
>> +properties:
>> +  compatible:
>> +    items:
> 
> Drop items.
> 

Ack
>> +      - const: qcom,mpm2-sleep-counter
> 
> SoC specific compatible.
> 

This is a generic Module Power Manager Sleep Counter which is present in 
all Qcom Socs, so SoC specific compatible is not given here.

>> +
>> +  reg:
>> +    items:
>> +      - description: MPM Sleep Counter Base
> 
> just maxItems: 1
> 

Ack
>> +
>> +  clock-frequency:
>> +    description: Frequency for the sleep counter
> 
> Since this does not have clocks, what frequency you are setting here?

Module Power Manager(MPM) Sleep Counter is a clock that starts ticking 
from Primary Boot Loader(PBL) Stage. This is usually a 32 Khz clock and 
the frequency for the same is stored here.

> 
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    mpm2-sleep-counter@c221000{
> 
> Node names should be generic.
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Ack
> 
>> +       compatible = "qcom,mpm2-sleep-counter";
>> +       reg = <0xc221000 0x1000>;
>> +       clock-frequency = <32768>;
>> +    };
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH V1 2/4] dt-bindings: soc: qcom,mpm-sleep-counter: Add the dtschema

[Souradeep Chowdhury]

On 3/21/2023 11:03 PM, Krzysztof Kozlowski wrote:
> On 21/03/2023 14:51, Souradeep Chowdhury wrote:
>> Add the device tree bindings for the module power manager sleep
>> counter.
>>
>> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
>> ---
>>   .../bindings/soc/qcom/qcom,mpm-sleep-counter.yaml  | 40 ++++++++++++++++++++++
>>   1 file changed, 40 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
>> new file mode 100644
>> index 0000000..f9f46b7
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
>> @@ -0,0 +1,40 @@
>> +# SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/soc/qcom/qcom,mpm-sleep-counter.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: MPM Sleep Counter
>> +
>> +maintainers:
>> +  - Souradeep Chowdhury <quic_schowdhu@quicinc.com>
>> +
>> +description: |
>> +    MPM(Module Power Manager) has a sleep counter which is used to track
> 
> Missing space:
> MPM (Module
> 

Ack
>> +    various stages of the boot process in Qualcomm.
> 
> in Qualcomm SoC. Because you do not track it in the company...
> 

Ack
>> +
>> +properties:
>> +  compatible:
>> +    items:
> 
> Drop items.
> 

Ack
>> +      - const: qcom,mpm2-sleep-counter
> 
> SoC specific compatible.
> 

This is a generic Module Power Manager Sleep Counter which is present in 
all Qcom Socs, so SoC specific compatible is not given here.

>> +
>> +  reg:
>> +    items:
>> +      - description: MPM Sleep Counter Base
> 
> just maxItems: 1
> 

Ack
>> +
>> +  clock-frequency:
>> +    description: Frequency for the sleep counter
> 
> Since this does not have clocks, what frequency you are setting here?

Module Power Manager(MPM) Sleep Counter is a clock that starts ticking 
from Primary Boot Loader(PBL) Stage. This is usually a 32 Khz clock and 
the frequency for the same is stored here.

> 
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    mpm2-sleep-counter@c221000{
> 
> Node names should be generic.
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Ack
> 
>> +       compatible = "qcom,mpm2-sleep-counter";
>> +       reg = <0xc221000 0x1000>;
>> +       clock-frequency = <32768>;
>> +    };
> 
> 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 V1 3/4] soc: qcom: boot_stat: Add Driver Support for Boot Stats

[Souradeep Chowdhury]

On 3/21/2023 11:07 PM, Krzysztof Kozlowski wrote:
> On 21/03/2023 14:51, Souradeep Chowdhury wrote:
>> All of Qualcomm's proprietary Android boot-loaders capture boot time
>> stats, like the time when the bootloader started execution and at what
>> point the bootloader handed over control to the kernel etc. in the IMEM
>> region. This information is captured in a specific format by this driver
>> by mapping a structure to the IMEM memory region and then accessing the
>> members of the structure to print the information. This information is
>> useful in verifying if the existing boot KPIs have regre
> 
> 
>> +/**
>> + *  struct boot_stats - timestamp information related to boot stats
>> + *  @bootloader_start:	Time for the starting point of the abl bootloader
>> + *  @bootloader_end:	Time when the kernel starts loading from abl bootloader
>> + */
>> +struct boot_stats {
>> +	u32 bootloader_start;
>> +	u32 bootloader_end;
>> +} __packed;
>> +
>> +static struct boot_stats __iomem *boot_stats;
>> +static void __iomem *mpm_counter_base;
>> +static u32 mpm_counter_freq;
> 
> No file-scope variables. Does not scale, not easy for review and
> maintenance. Avoid such code.

Ack
> 
>> +
>> +static int mpm_parse_dt(void)
>> +{
>> +	struct device_node *np_imem, *np_mpm2;
>> +
>> +	np_imem = of_find_compatible_node(NULL, NULL,
>> +					  "qcom,imem-boot_stats");
>> +	if (!np_imem) {
>> +		pr_err("can't find qcom,imem node\n");
> 
> So you are printing errors everywhere, on every soc and with compile
> test on every platform there is in the world... sorry, it does not work
> like that.

Ack
> 
>> +		return -ENODEV;
>> +	}
>> +	boot_stats = of_iomap(np_imem, 0);
>> +	if (!boot_stats) {
>> +		pr_err("boot_stats: Can't map imem\n");
>> +		goto err1;
>> +	}
> 
> 
>> +
>> +static void __exit boot_stats_exit(void)
>> +{
>> +}
>> +module_exit(boot_stats_exit)
> 
> 
> I don't think this is some special code which deserves init calls. Make
> it module_platform_driver().

Since this just reads some values from the Imem region and prints it to 
the user and doesn't have a specific device associated with it, a 
generic module code is written for it and not a module_platform_driver().

> 
> 
> Best regards,
> Krzysztof
> 

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

[Souradeep Chowdhury]

On 3/21/2023 11:07 PM, Krzysztof Kozlowski wrote:
> On 21/03/2023 14:51, Souradeep Chowdhury wrote:
>> All of Qualcomm's proprietary Android boot-loaders capture boot time
>> stats, like the time when the bootloader started execution and at what
>> point the bootloader handed over control to the kernel etc. in the IMEM
>> region. This information is captured in a specific format by this driver
>> by mapping a structure to the IMEM memory region and then accessing the
>> members of the structure to print the information. This information is
>> useful in verifying if the existing boot KPIs have regre
> 
> 
>> +/**
>> + *  struct boot_stats - timestamp information related to boot stats
>> + *  @bootloader_start:	Time for the starting point of the abl bootloader
>> + *  @bootloader_end:	Time when the kernel starts loading from abl bootloader
>> + */
>> +struct boot_stats {
>> +	u32 bootloader_start;
>> +	u32 bootloader_end;
>> +} __packed;
>> +
>> +static struct boot_stats __iomem *boot_stats;
>> +static void __iomem *mpm_counter_base;
>> +static u32 mpm_counter_freq;
> 
> No file-scope variables. Does not scale, not easy for review and
> maintenance. Avoid such code.

Ack
> 
>> +
>> +static int mpm_parse_dt(void)
>> +{
>> +	struct device_node *np_imem, *np_mpm2;
>> +
>> +	np_imem = of_find_compatible_node(NULL, NULL,
>> +					  "qcom,imem-boot_stats");
>> +	if (!np_imem) {
>> +		pr_err("can't find qcom,imem node\n");
> 
> So you are printing errors everywhere, on every soc and with compile
> test on every platform there is in the world... sorry, it does not work
> like that.

Ack
> 
>> +		return -ENODEV;
>> +	}
>> +	boot_stats = of_iomap(np_imem, 0);
>> +	if (!boot_stats) {
>> +		pr_err("boot_stats: Can't map imem\n");
>> +		goto err1;
>> +	}
> 
> 
>> +
>> +static void __exit boot_stats_exit(void)
>> +{
>> +}
>> +module_exit(boot_stats_exit)
> 
> 
> I don't think this is some special code which deserves init calls. Make
> it module_platform_driver().

Since this just reads some values from the Imem region and prints it to 
the user and doesn't have a specific device associated with it, a 
generic module code is written for it and not a module_platform_driver().

> 
> 
> Best regards,
> Krzysztof
> 

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

Re: [PATCH V1 2/4] dt-bindings: soc: qcom,mpm-sleep-counter: Add the dtschema

[Souradeep Chowdhury]

On 3/21/2023 11:09 PM, Krzysztof Kozlowski wrote:
> On 21/03/2023 14:51, Souradeep Chowdhury wrote:
>> Add the device tree bindings for the module power manager sleep
>> counter.
>>
>> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
>> ---
>>   .../bindings/soc/qcom/qcom,mpm-sleep-counter.yaml  | 40 ++++++++++++++++++++++
>>   1 file changed, 40 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
>> new file mode 100644
>> index 0000000..f9f46b7
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
> 
> Your code a bit explains this... this is not a device but some memory
> region. Does not look like suitable for bindings as separate device.

Ack. Can you let me know the suitable place to put the bindings for this?

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH V1 2/4] dt-bindings: soc: qcom,mpm-sleep-counter: Add the dtschema

[Souradeep Chowdhury]

On 3/21/2023 11:09 PM, Krzysztof Kozlowski wrote:
> On 21/03/2023 14:51, Souradeep Chowdhury wrote:
>> Add the device tree bindings for the module power manager sleep
>> counter.
>>
>> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
>> ---
>>   .../bindings/soc/qcom/qcom,mpm-sleep-counter.yaml  | 40 ++++++++++++++++++++++
>>   1 file changed, 40 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
>> new file mode 100644
>> index 0000000..f9f46b7
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
> 
> Your code a bit explains this... this is not a device but some memory
> region. Does not look like suitable for bindings as separate device.

Ack. Can you let me know the suitable place to put the bindings for this?

> 
> Best regards,
> Krzysztof
> 

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

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

[Krzysztof Kozlowski]

On 22/03/2023 14:54, Souradeep Chowdhury wrote:
> 
> 
> On 3/21/2023 11:07 PM, Krzysztof Kozlowski wrote:
>> On 21/03/2023 14:51, Souradeep Chowdhury wrote:
>>> All of Qualcomm's proprietary Android boot-loaders capture boot time
>>> stats, like the time when the bootloader started execution and at what
>>> point the bootloader handed over control to the kernel etc. in the IMEM
>>> region. This information is captured in a specific format by this driver
>>> by mapping a structure to the IMEM memory region and then accessing the
>>> members of the structure to print the information. This information is
>>> useful in verifying if the existing boot KPIs have regre
>>
>>
>>> +/**
>>> + *  struct boot_stats - timestamp information related to boot stats
>>> + *  @bootloader_start:	Time for the starting point of the abl bootloader
>>> + *  @bootloader_end:	Time when the kernel starts loading from abl bootloader
>>> + */
>>> +struct boot_stats {
>>> +	u32 bootloader_start;
>>> +	u32 bootloader_end;
>>> +} __packed;
>>> +
>>> +static struct boot_stats __iomem *boot_stats;
>>> +static void __iomem *mpm_counter_base;
>>> +static u32 mpm_counter_freq;
>>
>> No file-scope variables. Does not scale, not easy for review and
>> maintenance. Avoid such code.
> 
> Ack
>>
>>> +
>>> +static int mpm_parse_dt(void)
>>> +{
>>> +	struct device_node *np_imem, *np_mpm2;
>>> +
>>> +	np_imem = of_find_compatible_node(NULL, NULL,
>>> +					  "qcom,imem-boot_stats");
>>> +	if (!np_imem) {
>>> +		pr_err("can't find qcom,imem node\n");
>>
>> So you are printing errors everywhere, on every soc and with compile
>> test on every platform there is in the world... sorry, it does not work
>> like that.
> 
> Ack
>>
>>> +		return -ENODEV;
>>> +	}
>>> +	boot_stats = of_iomap(np_imem, 0);
>>> +	if (!boot_stats) {
>>> +		pr_err("boot_stats: Can't map imem\n");
>>> +		goto err1;
>>> +	}
>>
>>
>>> +
>>> +static void __exit boot_stats_exit(void)
>>> +{
>>> +}
>>> +module_exit(boot_stats_exit)
>>
>>
>> I don't think this is some special code which deserves init calls. Make
>> it module_platform_driver().
> 
> Since this just reads some values from the Imem region and prints it to 
> the user and doesn't have a specific device associated with it, a 

Which is not really an argument for such antipattern, but okay...

> generic module code is written for it and not a module_platform_driver().

... so how do you handle deferred probe?

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 22/03/2023 14:54, Souradeep Chowdhury wrote:
> 
> 
> On 3/21/2023 11:07 PM, Krzysztof Kozlowski wrote:
>> On 21/03/2023 14:51, Souradeep Chowdhury wrote:
>>> All of Qualcomm's proprietary Android boot-loaders capture boot time
>>> stats, like the time when the bootloader started execution and at what
>>> point the bootloader handed over control to the kernel etc. in the IMEM
>>> region. This information is captured in a specific format by this driver
>>> by mapping a structure to the IMEM memory region and then accessing the
>>> members of the structure to print the information. This information is
>>> useful in verifying if the existing boot KPIs have regre
>>
>>
>>> +/**
>>> + *  struct boot_stats - timestamp information related to boot stats
>>> + *  @bootloader_start:	Time for the starting point of the abl bootloader
>>> + *  @bootloader_end:	Time when the kernel starts loading from abl bootloader
>>> + */
>>> +struct boot_stats {
>>> +	u32 bootloader_start;
>>> +	u32 bootloader_end;
>>> +} __packed;
>>> +
>>> +static struct boot_stats __iomem *boot_stats;
>>> +static void __iomem *mpm_counter_base;
>>> +static u32 mpm_counter_freq;
>>
>> No file-scope variables. Does not scale, not easy for review and
>> maintenance. Avoid such code.
> 
> Ack
>>
>>> +
>>> +static int mpm_parse_dt(void)
>>> +{
>>> +	struct device_node *np_imem, *np_mpm2;
>>> +
>>> +	np_imem = of_find_compatible_node(NULL, NULL,
>>> +					  "qcom,imem-boot_stats");
>>> +	if (!np_imem) {
>>> +		pr_err("can't find qcom,imem node\n");
>>
>> So you are printing errors everywhere, on every soc and with compile
>> test on every platform there is in the world... sorry, it does not work
>> like that.
> 
> Ack
>>
>>> +		return -ENODEV;
>>> +	}
>>> +	boot_stats = of_iomap(np_imem, 0);
>>> +	if (!boot_stats) {
>>> +		pr_err("boot_stats: Can't map imem\n");
>>> +		goto err1;
>>> +	}
>>
>>
>>> +
>>> +static void __exit boot_stats_exit(void)
>>> +{
>>> +}
>>> +module_exit(boot_stats_exit)
>>
>>
>> I don't think this is some special code which deserves init calls. Make
>> it module_platform_driver().
> 
> Since this just reads some values from the Imem region and prints it to 
> the user and doesn't have a specific device associated with it, a 

Which is not really an argument for such antipattern, but okay...

> generic module code is written for it and not a module_platform_driver().

... so how do you handle deferred probe?

Best regards,
Krzysztof


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

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

[Krzysztof Kozlowski]

On 22/03/2023 14:34, Souradeep Chowdhury wrote:
>>
>>> +          - const: qcom,imem-boot_stats
>>
>> No underscores in compatibles. Why this is not SoC specific compatible?
> 
> Ack. The boot_stats module is not specific to a device. It is written to
> read some values from this imem region which is present for almost all 
> QCOM SoCs. So SoC specific compatible is not given in this case.

Yeah, but the generic rule is that we always want SoC specific
compatibles. If this is not specific to a device, then you do not need
anything in DT and just instantiate it from some soc-driver...

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 22/03/2023 14:34, Souradeep Chowdhury wrote:
>>
>>> +          - const: qcom,imem-boot_stats
>>
>> No underscores in compatibles. Why this is not SoC specific compatible?
> 
> Ack. The boot_stats module is not specific to a device. It is written to
> read some values from this imem region which is present for almost all 
> QCOM SoCs. So SoC specific compatible is not given in this case.

Yeah, but the generic rule is that we always want SoC specific
compatibles. If this is not specific to a device, then you do not need
anything in DT and just instantiate it from some soc-driver...

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 V1 2/4] dt-bindings: soc: qcom,mpm-sleep-counter: Add the dtschema

[Krzysztof Kozlowski]

On 22/03/2023 14:46, Souradeep Chowdhury wrote:
>>> +      - const: qcom,mpm2-sleep-counter
>>
>> SoC specific compatible.
>>
> 
> This is a generic Module Power Manager Sleep Counter which is present in 
> all Qcom Socs, so SoC specific compatible is not given here.

Not really a good reason and it is actually very difficult to verify
this. If I understand correctly any moment FW can change and this will
stop being valid, so no.

> 
>>> +
>>> +  reg:
>>> +    items:
>>> +      - description: MPM Sleep Counter Base
>>
>> just maxItems: 1
>>
> 
> Ack
>>> +
>>> +  clock-frequency:
>>> +    description: Frequency for the sleep counter
>>
>> Since this does not have clocks, what frequency you are setting here?
> 
> Module Power Manager(MPM) Sleep Counter is a clock that starts ticking 
> from Primary Boot Loader(PBL) Stage. This is usually a 32 Khz clock and 
> the frequency for the same is stored here.

You just said all devices have the same MPM, so I would argue that all
devices have the same clock.

Anyway, this is a legacy property.

Best regards,
Krzysztof

Re: [PATCH V1 2/4] dt-bindings: soc: qcom,mpm-sleep-counter: Add the dtschema

[Krzysztof Kozlowski]

On 22/03/2023 14:46, Souradeep Chowdhury wrote:
>>> +      - const: qcom,mpm2-sleep-counter
>>
>> SoC specific compatible.
>>
> 
> This is a generic Module Power Manager Sleep Counter which is present in 
> all Qcom Socs, so SoC specific compatible is not given here.

Not really a good reason and it is actually very difficult to verify
this. If I understand correctly any moment FW can change and this will
stop being valid, so no.

> 
>>> +
>>> +  reg:
>>> +    items:
>>> +      - description: MPM Sleep Counter Base
>>
>> just maxItems: 1
>>
> 
> Ack
>>> +
>>> +  clock-frequency:
>>> +    description: Frequency for the sleep counter
>>
>> Since this does not have clocks, what frequency you are setting here?
> 
> Module Power Manager(MPM) Sleep Counter is a clock that starts ticking 
> from Primary Boot Loader(PBL) Stage. This is usually a 32 Khz clock and 
> the frequency for the same is stored here.

You just said all devices have the same MPM, so I would argue that all
devices have the same clock.

Anyway, this is a legacy property.

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 V1 2/4] dt-bindings: soc: qcom,mpm-sleep-counter: Add the dtschema

[Krzysztof Kozlowski]

On 22/03/2023 15:02, Souradeep Chowdhury wrote:
> 
> 
> On 3/21/2023 11:09 PM, Krzysztof Kozlowski wrote:
>> On 21/03/2023 14:51, Souradeep Chowdhury wrote:
>>> Add the device tree bindings for the module power manager sleep
>>> counter.
>>>
>>> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
>>> ---
>>>   .../bindings/soc/qcom/qcom,mpm-sleep-counter.yaml  | 40 ++++++++++++++++++++++
>>>   1 file changed, 40 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
>>> new file mode 100644
>>> index 0000000..f9f46b7
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
>>
>> Your code a bit explains this... this is not a device but some memory
>> region. Does not look like suitable for bindings as separate device.
> 
> Ack. Can you let me know the suitable place to put the bindings for this?

Reserved memory region? Or drop from bindings entirely.

Best regards,
Krzysztof

Re: [PATCH V1 2/4] dt-bindings: soc: qcom,mpm-sleep-counter: Add the dtschema

[Krzysztof Kozlowski]

On 22/03/2023 15:02, Souradeep Chowdhury wrote:
> 
> 
> On 3/21/2023 11:09 PM, Krzysztof Kozlowski wrote:
>> On 21/03/2023 14:51, Souradeep Chowdhury wrote:
>>> Add the device tree bindings for the module power manager sleep
>>> counter.
>>>
>>> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
>>> ---
>>>   .../bindings/soc/qcom/qcom,mpm-sleep-counter.yaml  | 40 ++++++++++++++++++++++
>>>   1 file changed, 40 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
>>> new file mode 100644
>>> index 0000000..f9f46b7
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
>>
>> Your code a bit explains this... this is not a device but some memory
>> region. Does not look like suitable for bindings as separate device.
> 
> Ack. Can you let me know the suitable place to put the bindings for this?

Reserved memory region? Or drop from bindings entirely.

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

[Souradeep Chowdhury]

On 3/22/2023 8:23 PM, Krzysztof Kozlowski wrote:
> On 22/03/2023 14:54, Souradeep Chowdhury wrote:
>>
>>
>> On 3/21/2023 11:07 PM, Krzysztof Kozlowski wrote:
>>> On 21/03/2023 14:51, Souradeep Chowdhury wrote:
>>>> All of Qualcomm's proprietary Android boot-loaders capture boot time
>>>> stats, like the time when the bootloader started execution and at what
>>>> point the bootloader handed over control to the kernel etc. in the IMEM
>>>> region. This information is captured in a specific format by this driver
>>>> by mapping a structure to the IMEM memory region and then accessing the
>>>> members of the structure to print the information. This information is
>>>> useful in verifying if the existing boot KPIs have regre
>>>
>>>
>>>> +/**
>>>> + *  struct boot_stats - timestamp information related to boot stats
>>>> + *  @bootloader_start:	Time for the starting point of the abl bootloader
>>>> + *  @bootloader_end:	Time when the kernel starts loading from abl bootloader
>>>> + */
>>>> +struct boot_stats {
>>>> +	u32 bootloader_start;
>>>> +	u32 bootloader_end;
>>>> +} __packed;
>>>> +
>>>> +static struct boot_stats __iomem *boot_stats;
>>>> +static void __iomem *mpm_counter_base;
>>>> +static u32 mpm_counter_freq;
>>>
>>> No file-scope variables. Does not scale, not easy for review and
>>> maintenance. Avoid such code.
>>
>> Ack
>>>
>>>> +
>>>> +static int mpm_parse_dt(void)
>>>> +{
>>>> +	struct device_node *np_imem, *np_mpm2;
>>>> +
>>>> +	np_imem = of_find_compatible_node(NULL, NULL,
>>>> +					  "qcom,imem-boot_stats");
>>>> +	if (!np_imem) {
>>>> +		pr_err("can't find qcom,imem node\n");
>>>
>>> So you are printing errors everywhere, on every soc and with compile
>>> test on every platform there is in the world... sorry, it does not work
>>> like that.
>>
>> Ack
>>>
>>>> +		return -ENODEV;
>>>> +	}
>>>> +	boot_stats = of_iomap(np_imem, 0);
>>>> +	if (!boot_stats) {
>>>> +		pr_err("boot_stats: Can't map imem\n");
>>>> +		goto err1;
>>>> +	}
>>>
>>>
>>>> +
>>>> +static void __exit boot_stats_exit(void)
>>>> +{
>>>> +}
>>>> +module_exit(boot_stats_exit)
>>>
>>>
>>> I don't think this is some special code which deserves init calls. Make
>>> it module_platform_driver().
>>
>> Since this just reads some values from the Imem region and prints it to
>> the user and doesn't have a specific device associated with it, a
> 
> Which is not really an argument for such antipattern, but okay...
> 
>> generic module code is written for it and not a module_platform_driver().
> 
> ... so how do you handle deferred probe?

This has no dependency on other resources except that it parses some 
information from DT nodes, so deferred probe handling is not needed
in this case.

> 
> Best regards,
> Krzysztof
> 

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

[Souradeep Chowdhury]

On 3/22/2023 8:23 PM, Krzysztof Kozlowski wrote:
> On 22/03/2023 14:54, Souradeep Chowdhury wrote:
>>
>>
>> On 3/21/2023 11:07 PM, Krzysztof Kozlowski wrote:
>>> On 21/03/2023 14:51, Souradeep Chowdhury wrote:
>>>> All of Qualcomm's proprietary Android boot-loaders capture boot time
>>>> stats, like the time when the bootloader started execution and at what
>>>> point the bootloader handed over control to the kernel etc. in the IMEM
>>>> region. This information is captured in a specific format by this driver
>>>> by mapping a structure to the IMEM memory region and then accessing the
>>>> members of the structure to print the information. This information is
>>>> useful in verifying if the existing boot KPIs have regre
>>>
>>>
>>>> +/**
>>>> + *  struct boot_stats - timestamp information related to boot stats
>>>> + *  @bootloader_start:	Time for the starting point of the abl bootloader
>>>> + *  @bootloader_end:	Time when the kernel starts loading from abl bootloader
>>>> + */
>>>> +struct boot_stats {
>>>> +	u32 bootloader_start;
>>>> +	u32 bootloader_end;
>>>> +} __packed;
>>>> +
>>>> +static struct boot_stats __iomem *boot_stats;
>>>> +static void __iomem *mpm_counter_base;
>>>> +static u32 mpm_counter_freq;
>>>
>>> No file-scope variables. Does not scale, not easy for review and
>>> maintenance. Avoid such code.
>>
>> Ack
>>>
>>>> +
>>>> +static int mpm_parse_dt(void)
>>>> +{
>>>> +	struct device_node *np_imem, *np_mpm2;
>>>> +
>>>> +	np_imem = of_find_compatible_node(NULL, NULL,
>>>> +					  "qcom,imem-boot_stats");
>>>> +	if (!np_imem) {
>>>> +		pr_err("can't find qcom,imem node\n");
>>>
>>> So you are printing errors everywhere, on every soc and with compile
>>> test on every platform there is in the world... sorry, it does not work
>>> like that.
>>
>> Ack
>>>
>>>> +		return -ENODEV;
>>>> +	}
>>>> +	boot_stats = of_iomap(np_imem, 0);
>>>> +	if (!boot_stats) {
>>>> +		pr_err("boot_stats: Can't map imem\n");
>>>> +		goto err1;
>>>> +	}
>>>
>>>
>>>> +
>>>> +static void __exit boot_stats_exit(void)
>>>> +{
>>>> +}
>>>> +module_exit(boot_stats_exit)
>>>
>>>
>>> I don't think this is some special code which deserves init calls. Make
>>> it module_platform_driver().
>>
>> Since this just reads some values from the Imem region and prints it to
>> the user and doesn't have a specific device associated with it, a
> 
> Which is not really an argument for such antipattern, but okay...
> 
>> generic module code is written for it and not a module_platform_driver().
> 
> ... so how do you handle deferred probe?

This has no dependency on other resources except that it parses some 
information from DT nodes, so deferred probe handling is not needed
in this case.

> 
> Best regards,
> Krzysztof
> 

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

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

[Souradeep Chowdhury]

On 3/22/2023 9:57 PM, Krzysztof Kozlowski wrote:
> On 22/03/2023 14:34, Souradeep Chowdhury wrote:
>>>
>>>> +          - const: qcom,imem-boot_stats
>>>
>>> No underscores in compatibles. Why this is not SoC specific compatible?
>>
>> Ack. The boot_stats module is not specific to a device. It is written to
>> read some values from this imem region which is present for almost all
>> QCOM SoCs. So SoC specific compatible is not given in this case.
> 
> Yeah, but the generic rule is that we always want SoC specific
> compatibles. If this is not specific to a device, then you do not need
> anything in DT and just instantiate it from some soc-driver...

Ack. Will add SoC specific compatible here.

> 
> Best regards,
> Krzysztof
> 

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

[Souradeep Chowdhury]

On 3/22/2023 9:57 PM, Krzysztof Kozlowski wrote:
> On 22/03/2023 14:34, Souradeep Chowdhury wrote:
>>>
>>>> +          - const: qcom,imem-boot_stats
>>>
>>> No underscores in compatibles. Why this is not SoC specific compatible?
>>
>> Ack. The boot_stats module is not specific to a device. It is written to
>> read some values from this imem region which is present for almost all
>> QCOM SoCs. So SoC specific compatible is not given in this case.
> 
> Yeah, but the generic rule is that we always want SoC specific
> compatibles. If this is not specific to a device, then you do not need
> anything in DT and just instantiate it from some soc-driver...

Ack. Will add SoC specific compatible here.

> 
> 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 V1 2/4] dt-bindings: soc: qcom,mpm-sleep-counter: Add the dtschema

[Souradeep Chowdhury]

On 3/22/2023 9:59 PM, Krzysztof Kozlowski wrote:
> On 22/03/2023 14:46, Souradeep Chowdhury wrote:
>>>> +      - const: qcom,mpm2-sleep-counter
>>>
>>> SoC specific compatible.
>>>
>>
>> This is a generic Module Power Manager Sleep Counter which is present in
>> all Qcom Socs, so SoC specific compatible is not given here.
> 
> Not really a good reason and it is actually very difficult to verify
> this. If I understand correctly any moment FW can change and this will
> stop being valid, so no.

Ack
> 
>>
>>>> +
>>>> +  reg:
>>>> +    items:
>>>> +      - description: MPM Sleep Counter Base
>>>
>>> just maxItems: 1
>>>
>>
>> Ack
>>>> +
>>>> +  clock-frequency:
>>>> +    description: Frequency for the sleep counter
>>>
>>> Since this does not have clocks, what frequency you are setting here?
>>
>> Module Power Manager(MPM) Sleep Counter is a clock that starts ticking
>> from Primary Boot Loader(PBL) Stage. This is usually a 32 Khz clock and
>> the frequency for the same is stored here.
> 
> You just said all devices have the same MPM, so I would argue that all
> devices have the same clock.
> 
> Anyway, this is a legacy property.

Ack. Will drop clock frequency.
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH V1 2/4] dt-bindings: soc: qcom,mpm-sleep-counter: Add the dtschema

[Souradeep Chowdhury]

On 3/22/2023 9:59 PM, Krzysztof Kozlowski wrote:
> On 22/03/2023 14:46, Souradeep Chowdhury wrote:
>>>> +      - const: qcom,mpm2-sleep-counter
>>>
>>> SoC specific compatible.
>>>
>>
>> This is a generic Module Power Manager Sleep Counter which is present in
>> all Qcom Socs, so SoC specific compatible is not given here.
> 
> Not really a good reason and it is actually very difficult to verify
> this. If I understand correctly any moment FW can change and this will
> stop being valid, so no.

Ack
> 
>>
>>>> +
>>>> +  reg:
>>>> +    items:
>>>> +      - description: MPM Sleep Counter Base
>>>
>>> just maxItems: 1
>>>
>>
>> Ack
>>>> +
>>>> +  clock-frequency:
>>>> +    description: Frequency for the sleep counter
>>>
>>> Since this does not have clocks, what frequency you are setting here?
>>
>> Module Power Manager(MPM) Sleep Counter is a clock that starts ticking
>> from Primary Boot Loader(PBL) Stage. This is usually a 32 Khz clock and
>> the frequency for the same is stored here.
> 
> You just said all devices have the same MPM, so I would argue that all
> devices have the same clock.
> 
> Anyway, this is a legacy property.

Ack. Will drop clock frequency.
> 
> 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 V1 2/4] dt-bindings: soc: qcom,mpm-sleep-counter: Add the dtschema

[Souradeep Chowdhury]

On 3/22/2023 10:01 PM, Krzysztof Kozlowski wrote:
> On 22/03/2023 15:02, Souradeep Chowdhury wrote:
>>
>>
>> On 3/21/2023 11:09 PM, Krzysztof Kozlowski wrote:
>>> On 21/03/2023 14:51, Souradeep Chowdhury wrote:
>>>> Add the device tree bindings for the module power manager sleep
>>>> counter.
>>>>
>>>> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
>>>> ---
>>>>    .../bindings/soc/qcom/qcom,mpm-sleep-counter.yaml  | 40 ++++++++++++++++++++++
>>>>    1 file changed, 40 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
>>>> new file mode 100644
>>>> index 0000000..f9f46b7
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
>>>
>>> Your code a bit explains this... this is not a device but some memory
>>> region. Does not look like suitable for bindings as separate device.
>>
>> Ack. Can you let me know the suitable place to put the bindings for this?
> 
> Reserved memory region? Or drop from bindings entirely.

Ack. Will drop this for now.
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH V1 2/4] dt-bindings: soc: qcom,mpm-sleep-counter: Add the dtschema

[Souradeep Chowdhury]

On 3/22/2023 10:01 PM, Krzysztof Kozlowski wrote:
> On 22/03/2023 15:02, Souradeep Chowdhury wrote:
>>
>>
>> On 3/21/2023 11:09 PM, Krzysztof Kozlowski wrote:
>>> On 21/03/2023 14:51, Souradeep Chowdhury wrote:
>>>> Add the device tree bindings for the module power manager sleep
>>>> counter.
>>>>
>>>> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
>>>> ---
>>>>    .../bindings/soc/qcom/qcom,mpm-sleep-counter.yaml  | 40 ++++++++++++++++++++++
>>>>    1 file changed, 40 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
>>>> new file mode 100644
>>>> index 0000000..f9f46b7
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,mpm-sleep-counter.yaml
>>>
>>> Your code a bit explains this... this is not a device but some memory
>>> region. Does not look like suitable for bindings as separate device.
>>
>> Ack. Can you let me know the suitable place to put the bindings for this?
> 
> Reserved memory region? Or drop from bindings entirely.

Ack. Will drop this for now.
> 
> 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 V1 3/4] soc: qcom: boot_stat: Add Driver Support for Boot Stats

[Krzysztof Kozlowski]

On 23/03/2023 14:45, Souradeep Chowdhury wrote:
> 
> 
> On 3/22/2023 8:23 PM, Krzysztof Kozlowski wrote:
>> On 22/03/2023 14:54, Souradeep Chowdhury wrote:
>>>
>>>
>>> On 3/21/2023 11:07 PM, Krzysztof Kozlowski wrote:
>>>> On 21/03/2023 14:51, Souradeep Chowdhury wrote:
>>>>> All of Qualcomm's proprietary Android boot-loaders capture boot time
>>>>> stats, like the time when the bootloader started execution and at what
>>>>> point the bootloader handed over control to the kernel etc. in the IMEM
>>>>> region. This information is captured in a specific format by this driver
>>>>> by mapping a structure to the IMEM memory region and then accessing the
>>>>> members of the structure to print the information. This information is
>>>>> useful in verifying if the existing boot KPIs have regre
>>>>
>>>>
>>>>> +/**
>>>>> + *  struct boot_stats - timestamp information related to boot stats
>>>>> + *  @bootloader_start:	Time for the starting point of the abl bootloader
>>>>> + *  @bootloader_end:	Time when the kernel starts loading from abl bootloader
>>>>> + */
>>>>> +struct boot_stats {
>>>>> +	u32 bootloader_start;
>>>>> +	u32 bootloader_end;
>>>>> +} __packed;
>>>>> +
>>>>> +static struct boot_stats __iomem *boot_stats;
>>>>> +static void __iomem *mpm_counter_base;
>>>>> +static u32 mpm_counter_freq;
>>>>
>>>> No file-scope variables. Does not scale, not easy for review and
>>>> maintenance. Avoid such code.
>>>
>>> Ack
>>>>
>>>>> +
>>>>> +static int mpm_parse_dt(void)
>>>>> +{
>>>>> +	struct device_node *np_imem, *np_mpm2;
>>>>> +
>>>>> +	np_imem = of_find_compatible_node(NULL, NULL,
>>>>> +					  "qcom,imem-boot_stats");
>>>>> +	if (!np_imem) {
>>>>> +		pr_err("can't find qcom,imem node\n");
>>>>
>>>> So you are printing errors everywhere, on every soc and with compile
>>>> test on every platform there is in the world... sorry, it does not work
>>>> like that.
>>>
>>> Ack
>>>>
>>>>> +		return -ENODEV;
>>>>> +	}
>>>>> +	boot_stats = of_iomap(np_imem, 0);
>>>>> +	if (!boot_stats) {
>>>>> +		pr_err("boot_stats: Can't map imem\n");
>>>>> +		goto err1;
>>>>> +	}
>>>>
>>>>
>>>>> +
>>>>> +static void __exit boot_stats_exit(void)
>>>>> +{
>>>>> +}
>>>>> +module_exit(boot_stats_exit)
>>>>
>>>>
>>>> I don't think this is some special code which deserves init calls. Make
>>>> it module_platform_driver().
>>>
>>> Since this just reads some values from the Imem region and prints it to
>>> the user and doesn't have a specific device associated with it, a
>>
>> Which is not really an argument for such antipattern, but okay...
>>
>>> generic module code is written for it and not a module_platform_driver().
>>
>> ... so how do you handle deferred probe?
> 
> This has no dependency on other resources except that it parses some 
> information from DT nodes, so deferred probe handling is not needed
> in this case.

Yes, I know, but if we would ever add it how this driver can handle it?
This is antipattern.

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 23/03/2023 14:45, Souradeep Chowdhury wrote:
> 
> 
> On 3/22/2023 8:23 PM, Krzysztof Kozlowski wrote:
>> On 22/03/2023 14:54, Souradeep Chowdhury wrote:
>>>
>>>
>>> On 3/21/2023 11:07 PM, Krzysztof Kozlowski wrote:
>>>> On 21/03/2023 14:51, Souradeep Chowdhury wrote:
>>>>> All of Qualcomm's proprietary Android boot-loaders capture boot time
>>>>> stats, like the time when the bootloader started execution and at what
>>>>> point the bootloader handed over control to the kernel etc. in the IMEM
>>>>> region. This information is captured in a specific format by this driver
>>>>> by mapping a structure to the IMEM memory region and then accessing the
>>>>> members of the structure to print the information. This information is
>>>>> useful in verifying if the existing boot KPIs have regre
>>>>
>>>>
>>>>> +/**
>>>>> + *  struct boot_stats - timestamp information related to boot stats
>>>>> + *  @bootloader_start:	Time for the starting point of the abl bootloader
>>>>> + *  @bootloader_end:	Time when the kernel starts loading from abl bootloader
>>>>> + */
>>>>> +struct boot_stats {
>>>>> +	u32 bootloader_start;
>>>>> +	u32 bootloader_end;
>>>>> +} __packed;
>>>>> +
>>>>> +static struct boot_stats __iomem *boot_stats;
>>>>> +static void __iomem *mpm_counter_base;
>>>>> +static u32 mpm_counter_freq;
>>>>
>>>> No file-scope variables. Does not scale, not easy for review and
>>>> maintenance. Avoid such code.
>>>
>>> Ack
>>>>
>>>>> +
>>>>> +static int mpm_parse_dt(void)
>>>>> +{
>>>>> +	struct device_node *np_imem, *np_mpm2;
>>>>> +
>>>>> +	np_imem = of_find_compatible_node(NULL, NULL,
>>>>> +					  "qcom,imem-boot_stats");
>>>>> +	if (!np_imem) {
>>>>> +		pr_err("can't find qcom,imem node\n");
>>>>
>>>> So you are printing errors everywhere, on every soc and with compile
>>>> test on every platform there is in the world... sorry, it does not work
>>>> like that.
>>>
>>> Ack
>>>>
>>>>> +		return -ENODEV;
>>>>> +	}
>>>>> +	boot_stats = of_iomap(np_imem, 0);
>>>>> +	if (!boot_stats) {
>>>>> +		pr_err("boot_stats: Can't map imem\n");
>>>>> +		goto err1;
>>>>> +	}
>>>>
>>>>
>>>>> +
>>>>> +static void __exit boot_stats_exit(void)
>>>>> +{
>>>>> +}
>>>>> +module_exit(boot_stats_exit)
>>>>
>>>>
>>>> I don't think this is some special code which deserves init calls. Make
>>>> it module_platform_driver().
>>>
>>> Since this just reads some values from the Imem region and prints it to
>>> the user and doesn't have a specific device associated with it, a
>>
>> Which is not really an argument for such antipattern, but okay...
>>
>>> generic module code is written for it and not a module_platform_driver().
>>
>> ... so how do you handle deferred probe?
> 
> This has no dependency on other resources except that it parses some 
> information from DT nodes, so deferred probe handling is not needed
> in this case.

Yes, I know, but if we would ever add it how this driver can handle it?
This is antipattern.

Best regards,
Krzysztof


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