Messages in this thread | | | Date | Wed, 22 Mar 2023 19:24:16 +0530 | Subject | Re: [PATCH V1 3/4] soc: qcom: boot_stat: Add Driver Support for Boot Stats | From | 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 >
| |