Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <f42637c8-400b-e39f-412d-60328e176585@quicinc.com>
Date:   Thu, 4 May 2023 12:05:56 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.1
Subject: Re: [PATCH V4 2/3] soc: qcom: boot_stat: Add Driver Support for Boot
 Stats
Content-Language: en-US
To:     Caleb Connolly <caleb.connolly@linaro.org>,
        Andy Gross <agross@kernel.org>,
        Konrad Dybcio <konrad.dybcio@somainline.org>,
        "Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Rob Herring <robh+dt@kernel.org>
CC:     <linux-arm-kernel@lists.infradead.org>,
        <linux-kernel@vger.kernel.org>, <linux-arm-msm@vger.kernel.org>,
        <devicetree@vger.kernel.org>, Sibi Sankar <quic_sibis@quicinc.com>,
        Rajendra Nayak <quic_rjendra@quicinc.com>
References: <cover.1681742910.git.quic_schowdhu@quicinc.com>
 <C1eDJi-H9uWRAtbInRclmCgPb4EcgaeS3sk5FKO9cw8KscgMCH8dxRSvdPGUMwDFKpte7cBVeaqPhlLog-CRrg==@protonmail.internalid>
 <2ef76ce292c059c144e559123a9a54201ae2d0cf.1681742910.git.quic_schowdhu@quicinc.com>
 <575ee047-c6ce-95c3-8781-8c9a78534bb1@linaro.org>
From:   Souradeep Chowdhury <quic_schowdhu@quicinc.com>
In-Reply-To: <575ee047-c6ce-95c3-8781-8c9a78534bb1@linaro.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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 
of the debugfs file with the device. Will create the root dir as
"qcom_boot_stats" and push the dev_name dir inside it.

> 
> [...]
>> +
>> +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.


>> +	{ }
>> +};
>> +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
>>
>