Messages in this thread | | | Date | Tue, 9 May 2023 09:50:33 +0530 | Subject | Re: [PATCH V4 2/3] soc: qcom: boot_stat: Add Driver Support for Boot Stats | From | 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 >>>> >>> >
| |