[PATCH V2 1/3] dt-bindings: sram: qcom,imem: Add Boot Stat region within IMEM

Souradeep Chowdhury posted 3 patches 2 years, 10 months ago
There is a newer version of this series
[PATCH V2 1/3] dt-bindings: sram: qcom,imem: Add Boot Stat region within IMEM
Posted by Souradeep Chowdhury 2 years, 10 months ago
All Qualcomm bootloaders log useful timestamp information related
to bootloader stats in the IMEM region. Add the child node within
IMEM for the boot stat region containing register address and
compatible string.

Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
---
 .../devicetree/bindings/sram/qcom,imem.yaml         | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
index 665c06e..9998d65 100644
--- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
+++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
@@ -26,6 +26,7 @@ properties:
           - qcom,sdm845-imem
           - qcom,sdx55-imem
           - qcom,sdx65-imem
+          - qcom,sm8450-imem
       - const: syscon
       - const: simple-mfd
 
@@ -48,6 +49,26 @@ patternProperties:
     $ref: /schemas/remoteproc/qcom,pil-info.yaml#
     description: Peripheral image loader relocation region
 
+  "^boot-stat@[0-9a-f]+$":
+    type: object
+    description:
+      Imem region dedicated for storing timestamps related
+      information regarding bootstats.
+
+    properties:
+      compatible:
+        items:
+          - enum:
+              - qcom,sm8450-bootstats
+          - const: qcom,imem-bootstats
+
+      reg:
+        maxItems: 1
+
+    required:
+      - compatible
+      - reg
+
 required:
   - compatible
   - reg
-- 
2.7.4
Re: [PATCH V2 1/3] dt-bindings: sram: qcom,imem: Add Boot Stat region within IMEM
Posted by Krzysztof Kozlowski 2 years, 10 months ago
On 07/04/2023 16:04, Souradeep Chowdhury wrote:
> All Qualcomm bootloaders log useful timestamp information related
> to bootloader stats in the IMEM region. Add the child node within
> IMEM for the boot stat region containing register address and
> compatible string.
> 
> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> ---
>  .../devicetree/bindings/sram/qcom,imem.yaml         | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> index 665c06e..9998d65 100644
> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> @@ -26,6 +26,7 @@ properties:
>            - qcom,sdm845-imem
>            - qcom,sdx55-imem
>            - qcom,sdx65-imem
> +          - qcom,sm8450-imem

Use recent Linux kernel for your work.

Best regards,
Krzysztof
Re: [PATCH V2 1/3] dt-bindings: sram: qcom,imem: Add Boot Stat region within IMEM
Posted by Krzysztof Kozlowski 2 years, 10 months ago
On 07/04/2023 16:04, Souradeep Chowdhury wrote:
> All Qualcomm bootloaders log useful timestamp information related
> to bootloader stats in the IMEM region. Add the child node within
> IMEM for the boot stat region containing register address and
> compatible string.
> 
> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> ---
>  .../devicetree/bindings/sram/qcom,imem.yaml         | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> index 665c06e..9998d65 100644
> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> @@ -26,6 +26,7 @@ properties:
>            - qcom,sdm845-imem
>            - qcom,sdx55-imem
>            - qcom,sdx65-imem
> +          - qcom,sm8450-imem
>        - const: syscon
>        - const: simple-mfd
>  
> @@ -48,6 +49,26 @@ patternProperties:
>      $ref: /schemas/remoteproc/qcom,pil-info.yaml#
>      description: Peripheral image loader relocation region
>  
> +  "^boot-stat@[0-9a-f]+$":

Konrad,
Just like for RPM Master stats, didn't we want to call these just "stats"?

https://lore.kernel.org/linux-arm-msm/20230405-topic-master_stats-v2-1-51c304ecb610@linaro.org/

> +    type: object
> +    description:
> +      Imem region dedicated for storing timestamps related
> +      information regarding bootstats.

Description is okay, but you ignored the rest.

This is a friendly reminder during the review process.

It seems my previous comments were not fully addressed. Maybe my
feedback got lost between the quotes, maybe you just forgot to apply it.
Please go back to the previous discussion and either implement all
requested changes or keep discussing them.

Thank you.


Best regards,
Krzysztof
Re: [PATCH V2 1/3] dt-bindings: sram: qcom,imem: Add Boot Stat region within IMEM
Posted by Souradeep Chowdhury 2 years, 10 months ago

On 4/12/2023 1:45 PM, Krzysztof Kozlowski wrote:
> On 07/04/2023 16:04, Souradeep Chowdhury wrote:
>> All Qualcomm bootloaders log useful timestamp information related
>> to bootloader stats in the IMEM region. Add the child node within
>> IMEM for the boot stat region containing register address and
>> compatible string.
>>
>> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
>> ---
>>   .../devicetree/bindings/sram/qcom,imem.yaml         | 21 +++++++++++++++++++++
>>   1 file changed, 21 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>> index 665c06e..9998d65 100644
>> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>> @@ -26,6 +26,7 @@ properties:
>>             - qcom,sdm845-imem
>>             - qcom,sdx55-imem
>>             - qcom,sdx65-imem
>> +          - qcom,sm8450-imem
>>         - const: syscon
>>         - const: simple-mfd
>>   
>> @@ -48,6 +49,26 @@ patternProperties:
>>       $ref: /schemas/remoteproc/qcom,pil-info.yaml#
>>       description: Peripheral image loader relocation region
>>   
>> +  "^boot-stat@[0-9a-f]+$":
> 
> Konrad,
> Just like for RPM Master stats, didn't we want to call these just "stats"?
> 
> https://lore.kernel.org/linux-arm-msm/20230405-topic-master_stats-v2-1-51c304ecb610@linaro.org
> 
>> +    type: object
>> +    description:
>> +      Imem region dedicated for storing timestamps related
>> +      information regarding bootstats.
> 
> Description is okay, but you ignored the rest.
> 
> This is a friendly reminder during the review process.
> 
> It seems my previous comments were not fully addressed. Maybe my
> feedback got lost between the quotes, maybe you just forgot to apply it.
> Please go back to the previous discussion and either implement all
> requested changes or keep discussing them.
> 
> Thank you.

Ack. Will be implemented in the next version.
> 
> 
> Best regards,
> Krzysztof
>