diff mbox series

[V1,1/4] dt-bindings: sram: qcom,imem: Add Boot Stat region within IMEM

Message ID 1589f33deda07cb9f9e6c3c26bce6e02e53c168e.1679403696.git.quic_schowdhu@quicinc.com
State New
Headers
Series soc: qcom: boot_stats: Add driver support for boot_stats |

Commit Message

Souradeep Chowdhury March 21, 2023, 1:51 p.m. UTC 
  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          | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Comments

Krzysztof Kozlowski March 21, 2023, 5:31 p.m. UTC | #1 
On 21/03/2023 14:51, 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          | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> index 665c06e..c8c3890 100644
> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> @@ -48,6 +48,26 @@ patternProperties:
>      $ref: /schemas/remoteproc/qcom,pil-info.yaml#
>      description: Peripheral image loader relocation region
>  
> +  "^boot-stat@[0-9a-f]+$":
> +    type: object
> +    description:
> +      node for boot stat.

This basically copies the name of node, so not really helpful. Describe
what's this.

> +

additionalProperties: false

> +    properties:
> +      compatible:
> +        items:

Drop items.

> +          - const: qcom,imem-boot_stats

No underscores in compatibles. Why this is not SoC specific compatible?

> +
> +      reg:
> +        maxItems: 1
> +        description:
> +          The base address of the register region in case of
> +          imem boot stats.

Drop description, it's obvious.
> +
> +    required:
> +      - compatible
> +      - reg
> +
Best regards,
Krzysztof
Souradeep Chowdhury March 22, 2023, 1:34 p.m. UTC | #2 
On 3/21/2023 11:01 PM, Krzysztof Kozlowski wrote:
> On 21/03/2023 14:51, 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          | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>> index 665c06e..c8c3890 100644
>> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>> @@ -48,6 +48,26 @@ patternProperties:
>>       $ref: /schemas/remoteproc/qcom,pil-info.yaml#
>>       description: Peripheral image loader relocation region
>>   
>> +  "^boot-stat@[0-9a-f]+$":
>> +    type: object
>> +    description:
>> +      node for boot stat.
> 
> This basically copies the name of node, so not really helpful. Describe
> what's this.

Ack
> 
>> +
> 
> additionalProperties: false
> 
>> +    properties:
>> +      compatible:
>> +        items:
> 
> Drop items.
> 
>> +          - const: qcom,imem-boot_stats
> 
> No underscores in compatibles. Why this is not SoC specific compatible?

Ack. The boot_stats module is not specific to a device. It is written to
read some values from this imem region which is present for almost all 
QCOM SoCs. So SoC specific compatible is not given in this case.

> 
>> +
>> +      reg:
>> +        maxItems: 1
>> +        description:
>> +          The base address of the register region in case of
>> +          imem boot stats.
> 
> Drop description, it's obvious.

Ack
>> +
>> +    required:
>> +      - compatible
>> +      - reg
>> +
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski March 22, 2023, 4:27 p.m. UTC | #3 
On 22/03/2023 14:34, Souradeep Chowdhury wrote:
>>
>>> +          - const: qcom,imem-boot_stats
>>
>> No underscores in compatibles. Why this is not SoC specific compatible?
> 
> Ack. The boot_stats module is not specific to a device. It is written to
> read some values from this imem region which is present for almost all 
> QCOM SoCs. So SoC specific compatible is not given in this case.

Yeah, but the generic rule is that we always want SoC specific
compatibles. If this is not specific to a device, then you do not need
anything in DT and just instantiate it from some soc-driver...

Best regards,
Krzysztof
Souradeep Chowdhury March 23, 2023, 1:46 p.m. UTC | #4 
On 3/22/2023 9:57 PM, Krzysztof Kozlowski wrote:
> On 22/03/2023 14:34, Souradeep Chowdhury wrote:
>>>
>>>> +          - const: qcom,imem-boot_stats
>>>
>>> No underscores in compatibles. Why this is not SoC specific compatible?
>>
>> Ack. The boot_stats module is not specific to a device. It is written to
>> read some values from this imem region which is present for almost all
>> QCOM SoCs. So SoC specific compatible is not given in this case.
> 
> Yeah, but the generic rule is that we always want SoC specific
> compatibles. If this is not specific to a device, then you do not need
> anything in DT and just instantiate it from some soc-driver...

Ack. Will add SoC specific compatible here.

> 
> Best regards,
> Krzysztof
>
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
index 665c06e..c8c3890 100644
--- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
+++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
@@ -48,6 +48,26 @@  patternProperties:
     $ref: /schemas/remoteproc/qcom,pil-info.yaml#
     description: Peripheral image loader relocation region
 
+  "^boot-stat@[0-9a-f]+$":
+    type: object
+    description:
+      node for boot stat.
+
+    properties:
+      compatible:
+        items:
+          - const: qcom,imem-boot_stats
+
+      reg:
+        maxItems: 1
+        description:
+          The base address of the register region in case of
+          imem boot stats.
+
+    required:
+      - compatible
+      - reg
+
 required:
   - compatible
   - reg