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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux