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: <3109fb7a-0050-1b9b-d2aa-e00e26937b72@quicinc.com>
Date:   Tue, 9 May 2023 09:50:33 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.1
Subject: Re: [PATCH V4 2/3] soc: qcom: boot_stat: Add Driver Support for Boot
 Stats
To:     Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
        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>
 <f42637c8-400b-e39f-412d-60328e176585@quicinc.com>
 <35ac64ab-512d-1425-7a1b-6e8d3806c8a8@linaro.org>
Content-Language: en-US
From:   Souradeep Chowdhury <quic_schowdhu@quicinc.com>
In-Reply-To: <35ac64ab-512d-1425-7a1b-6e8d3806c8a8@linaro.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


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