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

[Dmitry Baryshkov]

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