[PATCH V7 0/2] firmware: Add support for boot_stats

[PATCH V7 0/2] firmware: Add support for boot_stats

[Souradeep Chowdhury]

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 by the bootloader and populated on
the device tree properties "pre-abl-time" and "abl-time" which can be
be viewed from the user space like as follows:-

/sys/firmware/devicetree/base/bootstats # cat abl_time
17898

As a part of this patch series added the bindings for boot-stats which
was suggested in the v6 of the series. The link for the same is as
follows:-

https://lore.kernel.org/linux-arm-kernel/7d397e67-5d56-4975-98af-1ac9746c07f4@app.fastmail.com/

Souradeep Chowdhury (2):
  dt-bindings: firmware: bootstats: Add the dtschema
  MAINTAINERS: Add the entry for boot_stats support

 .../bindings/firmware/qcom,bootstats.yaml     | 38 +++++++++++++++++++
 MAINTAINERS                                   |  6 +++
 2 files changed, 44 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/firmware/qcom,bootstats.yaml

--
2.17.1

[PATCH V7 1/2] dt-bindings: firmware: bootstats: Add the dtschema

[Souradeep Chowdhury]

Add the device tree bindings for boot stats. This has 2 properties
pre-abl-time and abl-time which records the timestamps for boot
stats.

Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
Link: https://lore.kernel.org/linux-arm-kernel/7d397e67-5d56-4975-98af-1ac9746c07f4@app.fastmail.com/
---
 .../bindings/firmware/qcom,bootstats.yaml     | 38 +++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/firmware/qcom,bootstats.yaml

diff --git a/Documentation/devicetree/bindings/firmware/qcom,bootstats.yaml b/Documentation/devicetree/bindings/firmware/qcom,bootstats.yaml
new file mode 100644
index 000000000000..22e697524058
--- /dev/null
+++ b/Documentation/devicetree/bindings/firmware/qcom,bootstats.yaml
@@ -0,0 +1,38 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/firmware/qcom,bootstats.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Boot Stats
+
+maintainers:
+  - Souradeep Chowdhury <quic_schowdhu@quicinc.com>
+
+description: |
+    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.
+    This is captured in the unit of ms in devicetree property.
+
+properties:
+  pre-abl-time:
+    description: The property to store the starting point of abl in ms.
+    $ref: /schemas/types.yaml#/definitions/string-array
+
+  abl-time:
+    description: The property to store the duration of abl in ms.
+    $ref: /schemas/types.yaml#/definitions/string-array
+
+required:
+  - pre-abl-time
+  - abl-time
+
+additionalProperties: false
+
+examples:
+  - |
+    bootstats {
+        pre-abl-time="17627";
+        abl-time="26748";
+    };
-- 
2.17.1

Re: [PATCH V7 1/2] dt-bindings: firmware: bootstats: Add the dtschema

[Krzysztof Kozlowski]

On 05/07/2023 07:32, Souradeep Chowdhury wrote:
> Add the device tree bindings for boot stats. This has 2 properties
> pre-abl-time and abl-time which records the timestamps for boot
> stats.
> 
> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> Link: https://lore.kernel.org/linux-arm-kernel/7d397e67-5d56-4975-98af-1ac9746c07f4@app.fastmail.com/
> ---
>  .../bindings/firmware/qcom,bootstats.yaml     | 38 +++++++++++++++++++
>  1 file changed, 38 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/firmware/qcom,bootstats.yaml
> 
> diff --git a/Documentation/devicetree/bindings/firmware/qcom,bootstats.yaml b/Documentation/devicetree/bindings/firmware/qcom,bootstats.yaml
> new file mode 100644
> index 000000000000..22e697524058
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/firmware/qcom,bootstats.yaml
> @@ -0,0 +1,38 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/firmware/qcom,bootstats.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Boot Stats

This explains nothing...

> +
> +maintainers:
> +  - Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> +
> +description: |

Drop |

> +    Qualcomm's proprietary Android boot-loaders capture boot time

Wrong indentation.

> +    stats, like the time when the bootloader started execution and
> +    at what point the bootloader handed over control to the kernel.
> +    This is captured in the unit of ms in devicetree property.
> +
> +properties:
> +  pre-abl-time:
> +    description: The property to store the starting point of abl in ms.

String instead of ms? No, this is the craziest idea I saw recently. Use
-ms suffix.

> +    $ref: /schemas/types.yaml#/definitions/string-array
> +
> +  abl-time:
> +    description: The property to store the duration of abl in ms.
> +    $ref: /schemas/types.yaml#/definitions/string-array

I have no clue what this entire binding is about. Nothing can bind to
it, no usage explained. Properties are not used to "store the duration".
This does not look like suitable for DT, drop entire binding.

> +
> +required:
> +  - pre-abl-time
> +  - abl-time
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    bootstats {
> +        pre-abl-time="17627";
> +        abl-time="26748";

Missing spaces. Open existing DTS and look at existing coding style.

Best regards,
Krzysztof

Re: [PATCH V7 1/2] dt-bindings: firmware: bootstats: Add the dtschema

[Souradeep Chowdhury]

On 7/5/2023 11:47 AM, Krzysztof Kozlowski wrote:
> On 05/07/2023 07:32, Souradeep Chowdhury wrote:
>> Add the device tree bindings for boot stats. This has 2 properties
>> pre-abl-time and abl-time which records the timestamps for boot
>> stats.
>>
>> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
>> Link: https://lore.kernel.org/linux-arm-kernel/7d397e67-5d56-4975-98af-1ac9746c07f4@app.fastmail.com/
>> ---
>>   .../bindings/firmware/qcom,bootstats.yaml     | 38 +++++++++++++++++++
>>   1 file changed, 38 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/firmware/qcom,bootstats.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/firmware/qcom,bootstats.yaml b/Documentation/devicetree/bindings/firmware/qcom,bootstats.yaml
>> new file mode 100644
>> index 000000000000..22e697524058
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/firmware/qcom,bootstats.yaml
>> @@ -0,0 +1,38 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/firmware/qcom,bootstats.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Boot Stats
> 
> This explains nothing...

Ack

> 
>> +
>> +maintainers:
>> +  - Souradeep Chowdhury <quic_schowdhu@quicinc.com>
>> +
>> +description: |
> 
> Drop |

Ack

> 
>> +    Qualcomm's proprietary Android boot-loaders capture boot time
> 
> Wrong indentation.

Ack
> 
>> +    stats, like the time when the bootloader started execution and
>> +    at what point the bootloader handed over control to the kernel.
>> +    This is captured in the unit of ms in devicetree property.
>> +
>> +properties:
>> +  pre-abl-time:
>> +    description: The property to store the starting point of abl in ms.
> 
> String instead of ms? No, this is the craziest idea I saw recently. Use
> -ms suffix.
> 
>> +    $ref: /schemas/types.yaml#/definitions/string-array
>> +
>> +  abl-time:
>> +    description: The property to store the duration of abl in ms.
>> +    $ref: /schemas/types.yaml#/definitions/string-array
> 
> I have no clue what this entire binding is about. Nothing can bind to
> it, no usage explained. Properties are not used to "store the duration".
> This does not look like suitable for DT, drop entire binding.

This binding was created as per the suggestion on version 6 of the patch 
by Arnd. The idea was that these 2 devicetree properties will be used to 
populate the bootstat values from the bootloader and exposed to the user 
via /sys/firmware/devicetree/ directly.

Details in the link below:-

https://lore.kernel.org/lkml/7d397e67-5d56-4975-98af-1ac9746c07f4@app.fastmail.com/T/#mbdc9ad95fcbb5ad7b56c6996a3933899b42d982c

Can you suggest any alternative way to represent this as a binding?

> 
>> +
>> +required:
>> +  - pre-abl-time
>> +  - abl-time
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    bootstats {
>> +        pre-abl-time="17627";
>> +        abl-time="26748";
> 
> Missing spaces. Open existing DTS and look at existing coding style.

Ack

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH V7 1/2] dt-bindings: firmware: bootstats: Add the dtschema

[Krzysztof Kozlowski]

On 05/07/2023 10:33, Souradeep Chowdhury wrote:
>>> +    $ref: /schemas/types.yaml#/definitions/string-array
>>> +
>>> +  abl-time:
>>> +    description: The property to store the duration of abl in ms.
>>> +    $ref: /schemas/types.yaml#/definitions/string-array
>>
>> I have no clue what this entire binding is about. Nothing can bind to
>> it, no usage explained. Properties are not used to "store the duration".
>> This does not look like suitable for DT, drop entire binding.
> 
> This binding was created as per the suggestion on version 6 of the patch 
> by Arnd. The idea was that these 2 devicetree properties will be used to 
> populate the bootstat values from the bootloader and exposed to the user 
> via /sys/firmware/devicetree/ directly.
> 
> Details in the link below:-
> 
> https://lore.kernel.org/lkml/7d397e67-5d56-4975-98af-1ac9746c07f4@app.fastmail.com/T/#mbdc9ad95fcbb5ad7b56c6996a3933899b42d982c
> 
> Can you suggest any alternative way to represent this as a binding?

Then you should clearly state in the binding how this is going to be
used and who is going to populate it. Not only in the binding but also
in commit msg which currently has 0 rationale and answers to "why". Your
commit msg explained only "what", which is usually obvious and much less
important. Your commit should stand on its own and should clearly
explain why we need this feature at all, what problem it solves.

And before you claim that there is some discussion under link or some
cover letter - these do not matter. Commit and bindings matter.

What's more, I don't think that Arnd's advice is correct here - DT is
suppose to describe hardware or firmware. These properties are coming
from firmware but they are not describing any firmware or hardware
characteristics. Instead they are debugging of current boot status.

I will leave the decision on that for Rob, however anyway binding is
very vague and incorrect, so I would expect he will come with the same
concerns regardless whether it is suitable to DT or is not.



Best regards,
Krzysztof

Re: [PATCH V7 1/2] dt-bindings: firmware: bootstats: Add the dtschema

[Rob Herring]

On Wed, Jul 05, 2023 at 11:34:35AM +0200, Krzysztof Kozlowski wrote:
> On 05/07/2023 10:33, Souradeep Chowdhury wrote:
> >>> +    $ref: /schemas/types.yaml#/definitions/string-array
> >>> +
> >>> +  abl-time:
> >>> +    description: The property to store the duration of abl in ms.
> >>> +    $ref: /schemas/types.yaml#/definitions/string-array
> >>
> >> I have no clue what this entire binding is about. Nothing can bind to
> >> it, no usage explained. Properties are not used to "store the duration".
> >> This does not look like suitable for DT, drop entire binding.
> > 
> > This binding was created as per the suggestion on version 6 of the patch 
> > by Arnd. The idea was that these 2 devicetree properties will be used to 
> > populate the bootstat values from the bootloader and exposed to the user 
> > via /sys/firmware/devicetree/ directly.
> > 
> > Details in the link below:-
> > 
> > https://lore.kernel.org/lkml/7d397e67-5d56-4975-98af-1ac9746c07f4@app.fastmail.com/T/#mbdc9ad95fcbb5ad7b56c6996a3933899b42d982c
> > 
> > Can you suggest any alternative way to represent this as a binding?
> 
> Then you should clearly state in the binding how this is going to be
> used and who is going to populate it. Not only in the binding but also
> in commit msg which currently has 0 rationale and answers to "why". Your
> commit msg explained only "what", which is usually obvious and much less
> important. Your commit should stand on its own and should clearly
> explain why we need this feature at all, what problem it solves.
> 
> And before you claim that there is some discussion under link or some
> cover letter - these do not matter. Commit and bindings matter.
> 
> What's more, I don't think that Arnd's advice is correct here - DT is
> suppose to describe hardware or firmware. These properties are coming
> from firmware but they are not describing any firmware or hardware
> characteristics. Instead they are debugging of current boot status.
> 
> I will leave the decision on that for Rob, however anyway binding is
> very vague and incorrect, so I would expect he will come with the same
> concerns regardless whether it is suitable to DT or is not.

My main concern here is not so much having this info in DT, but whether 
it's just the start of various properties. Either because there's already 
more data and these are just the 2 things you care about now, or because 
once we enable this it's an invitation to add more properties.

Boot timing information seems like something multiple platforms might 
want and only having 2 stages isn't extensible.

Rob

Re: [PATCH V7 1/2] dt-bindings: firmware: bootstats: Add the dtschema

[Dmitry Baryshkov]

On 05/07/2023 22:30, Rob Herring wrote:
> On Wed, Jul 05, 2023 at 11:34:35AM +0200, Krzysztof Kozlowski wrote:
>> On 05/07/2023 10:33, Souradeep Chowdhury wrote:
>>>>> +    $ref: /schemas/types.yaml#/definitions/string-array
>>>>> +
>>>>> +  abl-time:
>>>>> +    description: The property to store the duration of abl in ms.
>>>>> +    $ref: /schemas/types.yaml#/definitions/string-array
>>>>
>>>> I have no clue what this entire binding is about. Nothing can bind to
>>>> it, no usage explained. Properties are not used to "store the duration".
>>>> This does not look like suitable for DT, drop entire binding.
>>>
>>> This binding was created as per the suggestion on version 6 of the patch
>>> by Arnd. The idea was that these 2 devicetree properties will be used to
>>> populate the bootstat values from the bootloader and exposed to the user
>>> via /sys/firmware/devicetree/ directly.
>>>
>>> Details in the link below:-
>>>
>>> https://lore.kernel.org/lkml/7d397e67-5d56-4975-98af-1ac9746c07f4@app.fastmail.com/T/#mbdc9ad95fcbb5ad7b56c6996a3933899b42d982c
>>>
>>> Can you suggest any alternative way to represent this as a binding?
>>
>> Then you should clearly state in the binding how this is going to be
>> used and who is going to populate it. Not only in the binding but also
>> in commit msg which currently has 0 rationale and answers to "why". Your
>> commit msg explained only "what", which is usually obvious and much less
>> important. Your commit should stand on its own and should clearly
>> explain why we need this feature at all, what problem it solves.
>>
>> And before you claim that there is some discussion under link or some
>> cover letter - these do not matter. Commit and bindings matter.
>>
>> What's more, I don't think that Arnd's advice is correct here - DT is
>> suppose to describe hardware or firmware. These properties are coming
>> from firmware but they are not describing any firmware or hardware
>> characteristics. Instead they are debugging of current boot status.
>>
>> I will leave the decision on that for Rob, however anyway binding is
>> very vague and incorrect, so I would expect he will come with the same
>> concerns regardless whether it is suitable to DT or is not.
> 
> My main concern here is not so much having this info in DT, but whether
> it's just the start of various properties. Either because there's already
> more data and these are just the 2 things you care about now, or because
> once we enable this it's an invitation to add more properties.
> 
> Boot timing information seems like something multiple platforms might
> want and only having 2 stages isn't extensible.

I preferred the previous implementation idea, where the Linux driver 
will parse firmware data, instead of bootloader doing something for us.

Not to mention that that approach would allow us to get boot time stats 
on older platforms, without waiting (indefinitely) for the platform 
vendor to update the bootloader.

> 
> Rob

-- 
With best wishes
Dmitry

Re: [PATCH V7 1/2] dt-bindings: firmware: bootstats: Add the dtschema

[Souradeep Chowdhury]

On 7/6/2023 2:44 AM, Dmitry Baryshkov wrote:
> On 05/07/2023 22:30, Rob Herring wrote:
>> On Wed, Jul 05, 2023 at 11:34:35AM +0200, Krzysztof Kozlowski wrote:
>>> On 05/07/2023 10:33, Souradeep Chowdhury wrote:
>>>>>> +    $ref: /schemas/types.yaml#/definitions/string-array
>>>>>> +
>>>>>> +  abl-time:
>>>>>> +    description: The property to store the duration of abl in ms.
>>>>>> +    $ref: /schemas/types.yaml#/definitions/string-array
>>>>>
>>>>> I have no clue what this entire binding is about. Nothing can bind to
>>>>> it, no usage explained. Properties are not used to "store the 
>>>>> duration".
>>>>> This does not look like suitable for DT, drop entire binding.
>>>>
>>>> This binding was created as per the suggestion on version 6 of the 
>>>> patch
>>>> by Arnd. The idea was that these 2 devicetree properties will be 
>>>> used to
>>>> populate the bootstat values from the bootloader and exposed to the 
>>>> user
>>>> via /sys/firmware/devicetree/ directly.
>>>>
>>>> Details in the link below:-
>>>>
>>>> https://lore.kernel.org/lkml/7d397e67-5d56-4975-98af-1ac9746c07f4@app.fastmail.com/T/#mbdc9ad95fcbb5ad7b56c6996a3933899b42d982c
>>>>
>>>> Can you suggest any alternative way to represent this as a binding?
>>>
>>> Then you should clearly state in the binding how this is going to be
>>> used and who is going to populate it. Not only in the binding but also
>>> in commit msg which currently has 0 rationale and answers to "why". Your
>>> commit msg explained only "what", which is usually obvious and much less
>>> important. Your commit should stand on its own and should clearly
>>> explain why we need this feature at all, what problem it solves.
>>>
>>> And before you claim that there is some discussion under link or some
>>> cover letter - these do not matter. Commit and bindings matter.
>>>
>>> What's more, I don't think that Arnd's advice is correct here - DT is
>>> suppose to describe hardware or firmware. These properties are coming
>>> from firmware but they are not describing any firmware or hardware
>>> characteristics. Instead they are debugging of current boot status.
>>>
>>> I will leave the decision on that for Rob, however anyway binding is
>>> very vague and incorrect, so I would expect he will come with the same
>>> concerns regardless whether it is suitable to DT or is not.
>>
>> My main concern here is not so much having this info in DT, but whether
>> it's just the start of various properties. Either because there's already
>> more data and these are just the 2 things you care about now, or because
>> once we enable this it's an invitation to add more properties.
>>
>> Boot timing information seems like something multiple platforms might
>> want and only having 2 stages isn't extensible.
> 
> I preferred the previous implementation idea, where the Linux driver 
> will parse firmware data, instead of bootloader doing something for us.
> 
> Not to mention that that approach would allow us to get boot time stats 
> on older platforms, without waiting (indefinitely) for the platform 
> vendor to update the bootloader.
> 
>>
>> Rob

Hi Rob/Krzysztof/Arnd,

We will work on describing the bindings better but can you first give us
clarity on whether this is something that should be represented on the
devicetree and what should be the final approach we need to take for 
boot_stats?

Thanks,
Souradeep

> 

[PATCH V7 2/2] MAINTAINERS: Add the entry for boot_stats support

[Souradeep Chowdhury]

Add the entry for the file added as a part of support for
boot stats.

Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 4eba5e88c6b0..ee64b9e7b7cc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17173,6 +17173,12 @@ F:	sound/soc/codecs/wsa881x.c
 F:	sound/soc/codecs/wsa883x.c
 F:	sound/soc/qcom/
 
+QCOM BOOT_STATS DRIVER
+M:	Souradeep Chowdhury <quic_schowdhu@quicinc.com>
+L:	linux-arm-msm@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/firmware/qcom,bootstats.yaml
+
 QCOM EMBEDDED USB DEBUGGER (EUD)
 M:	Souradeep Chowdhury <quic_schowdhu@quicinc.com>
 L:	linux-arm-msm@vger.kernel.org
-- 
2.17.1

Re: [PATCH V7 0/2] firmware: Add support for boot_stats

[Brian Masney]

On Wed, Jul 05, 2023 at 11:02:30AM +0530, Souradeep Chowdhury wrote:
> 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 by the bootloader and populated on
> the device tree properties "pre-abl-time" and "abl-time" which can be
> be viewed from the user space like as follows:-
> 
> /sys/firmware/devicetree/base/bootstats # cat abl_time
> 17898
> 
> As a part of this patch series added the bindings for boot-stats which
> was suggested in the v6 of the series. The link for the same is as
> follows:-

Should boot statistics be exposed to userspace through the qcom socinfo
driver? I see that the socinfo v19 metadata has fields for the boot core
and boot cluster [1], so boot timing wouldn't be out of place.

Brian

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/soc/qcom/socinfo.c#n572

Re: [PATCH V7 0/2] firmware: Add support for boot_stats

[Souradeep Chowdhury]

On 7/10/2023 8:50 PM, Brian Masney wrote:
> On Wed, Jul 05, 2023 at 11:02:30AM +0530, Souradeep Chowdhury wrote:
>> 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 by the bootloader and populated on
>> the device tree properties "pre-abl-time" and "abl-time" which can be
>> be viewed from the user space like as follows:-
>>
>> /sys/firmware/devicetree/base/bootstats # cat abl_time
>> 17898
>>
>> As a part of this patch series added the bindings for boot-stats which
>> was suggested in the v6 of the series. The link for the same is as
>> follows:-
> 
> Should boot statistics be exposed to userspace through the qcom socinfo
> driver? I see that the socinfo v19 metadata has fields for the boot core
> and boot cluster [1], so boot timing wouldn't be out of place.

"Boot Cluster" and "boot core" are soc specific info which are fixed for 
a particular qcom soc recorded in smem region of memory. "Boot_stats" is
not a property of a soc as such, they are KPI measurements in the boot 
time path stored in the imem region. So I not sure if this is the right 
home for it.

> 
> Brian
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/soc/qcom/socinfo.c#n572
>