diff mbox series

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

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

Commit Message

Souradeep Chowdhury April 17, 2023, 3:08 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        | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

Comments

Krzysztof Kozlowski April 18, 2023, 7:25 a.m. UTC | #1 
On 17/04/2023 17:08, 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>

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof
Caleb Connolly May 3, 2023, 10:10 p.m. UTC | #2 
On 17/04/2023 16:08, 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        | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> index ba694ce..d028bed 100644
> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> @@ -49,6 +49,28 @@ patternProperties:
>      $ref: /schemas/remoteproc/qcom,pil-info.yaml#
>      description: Peripheral image loader relocation region
> 
> +  "^stats@[0-9a-f]+$":
> +    type: object
> +    description:
> +      Imem region dedicated for storing timestamps related
> +      information regarding bootstats.
> +
> +    additionalProperties: false
> +
> +    properties:
> +      compatible:
> +        items:
> +          - enum:
> +              - qcom,sm8450-bootstats

This region isn't exclusive to sm8450, it exists also on sdm845 and
presumably other platforms. Is there any need for an SoC specific
compatible?

> +          - const: qcom,imem-bootstats
> +
> +      reg:
> +        maxItems: 1
> +
> +    required:
> +      - compatible
> +      - reg
> +
>  required:
>    - compatible
>    - reg
> --
> 2.7.4
>
Souradeep Chowdhury May 4, 2023, 5:53 a.m. UTC | #3 
On 5/4/2023 3:40 AM, Caleb Connolly wrote:
> 
> 
> On 17/04/2023 16:08, 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        | 22 ++++++++++++++++++++++
>>   1 file changed, 22 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>> index ba694ce..d028bed 100644
>> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>> @@ -49,6 +49,28 @@ patternProperties:
>>       $ref: /schemas/remoteproc/qcom,pil-info.yaml#
>>       description: Peripheral image loader relocation region
>>
>> +  "^stats@[0-9a-f]+$":
>> +    type: object
>> +    description:
>> +      Imem region dedicated for storing timestamps related
>> +      information regarding bootstats.
>> +
>> +    additionalProperties: false
>> +
>> +    properties:
>> +      compatible:
>> +        items:
>> +          - enum:
>> +              - qcom,sm8450-bootstats
> 
> This region isn't exclusive to sm8450, it exists also on sdm845 and
> presumably other platforms. Is there any need for an SoC specific
> compatible?

Yes SOC specific compatibles are needed for device tree nodes. This has 
been clarified as per prior discussion on this patch series.

https://lore.kernel.org/lkml/e1d53083-82b6-d193-517e-02af281a066a@linaro.org/ 


> 
>> +          - const: qcom,imem-bootstats
>> +
>> +      reg:
>> +        maxItems: 1
>> +
>> +    required:
>> +      - compatible
>> +      - reg
>> +
>>   required:
>>     - compatible
>>     - reg
>> --
>> 2.7.4
>>
>
Krzysztof Kozlowski May 4, 2023, 6:26 a.m. UTC | #4 
On 04/05/2023 00:10, Caleb Connolly wrote:
> 
> 
> On 17/04/2023 16:08, 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        | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>> index ba694ce..d028bed 100644
>> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>> @@ -49,6 +49,28 @@ patternProperties:
>>      $ref: /schemas/remoteproc/qcom,pil-info.yaml#
>>      description: Peripheral image loader relocation region
>>
>> +  "^stats@[0-9a-f]+$":
>> +    type: object
>> +    description:
>> +      Imem region dedicated for storing timestamps related
>> +      information regarding bootstats.
>> +
>> +    additionalProperties: false
>> +
>> +    properties:
>> +      compatible:
>> +        items:
>> +          - enum:
>> +              - qcom,sm8450-bootstats
> 
> This region isn't exclusive to sm8450, it exists also on sdm845 and
> presumably other platforms. Is there any need for an SoC specific
> compatible?

Yes.
https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42

Also see many discussions on LKML about this.

Best regards,
Krzysztof
Dmitry Baryshkov May 4, 2023, 4:22 p.m. UTC | #5 
On 04/05/2023 09:26, Krzysztof Kozlowski wrote:
> On 04/05/2023 00:10, Caleb Connolly wrote:
>>
>>
>> On 17/04/2023 16:08, 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        | 22 ++++++++++++++++++++++
>>>   1 file changed, 22 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>>> index ba694ce..d028bed 100644
>>> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>>> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>>> @@ -49,6 +49,28 @@ patternProperties:
>>>       $ref: /schemas/remoteproc/qcom,pil-info.yaml#
>>>       description: Peripheral image loader relocation region
>>>
>>> +  "^stats@[0-9a-f]+$":
>>> +    type: object
>>> +    description:
>>> +      Imem region dedicated for storing timestamps related
>>> +      information regarding bootstats.
>>> +
>>> +    additionalProperties: false
>>> +
>>> +    properties:
>>> +      compatible:
>>> +        items:
>>> +          - enum:
>>> +              - qcom,sm8450-bootstats
>>
>> This region isn't exclusive to sm8450, it exists also on sdm845 and
>> presumably other platforms. Is there any need for an SoC specific
>> compatible?
> 
> Yes.
> https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42
> 
> Also see many discussions on LKML about this.


I checked the closest relative: qcom_stats.c driver. It defines several 
platform-specific overrides and also a generic "qcom,rpm-stats" / 
"qcom,rpmh-stats" drivers.
Dmitry Baryshkov May 4, 2023, 4:26 p.m. UTC | #6 
On 04/05/2023 09:26, Krzysztof Kozlowski wrote:
> On 04/05/2023 00:10, Caleb Connolly wrote:
>>
>>
>> On 17/04/2023 16:08, 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        | 22 ++++++++++++++++++++++
>>>   1 file changed, 22 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>>> index ba694ce..d028bed 100644
>>> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>>> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>>> @@ -49,6 +49,28 @@ patternProperties:
>>>       $ref: /schemas/remoteproc/qcom,pil-info.yaml#
>>>       description: Peripheral image loader relocation region
>>>
>>> +  "^stats@[0-9a-f]+$":
>>> +    type: object
>>> +    description:
>>> +      Imem region dedicated for storing timestamps related
>>> +      information regarding bootstats.
>>> +
>>> +    additionalProperties: false
>>> +
>>> +    properties:
>>> +      compatible:
>>> +        items:
>>> +          - enum:
>>> +              - qcom,sm8450-bootstats
>>
>> This region isn't exclusive to sm8450, it exists also on sdm845 and
>> presumably other platforms. Is there any need for an SoC specific
>> compatible?
> 
> Yes.
> https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42
> 
> Also see many discussions on LKML about this.

After taking another glance at the parent device (IMEM), I start to 
think that we should not be defining the device at all. The imem has the 
SoC name in it. So I think there should be a proper driver for IMEM. 
Then it will instantiate the ABL stats platform device depending on the 
SoC compat. Also this would allow us to rewrite qcom_pil_info_init() in 
a way to query IMEM instead of poking into DT directly.
Caleb Connolly May 4, 2023, 6:17 p.m. UTC | #7 
On 04/05/2023 17:26, Dmitry Baryshkov wrote:
> On 04/05/2023 09:26, Krzysztof Kozlowski wrote:
>> On 04/05/2023 00:10, Caleb Connolly wrote:
>>>
>>>
>>> On 17/04/2023 16:08, Souradeep Chowdhury wrote:
>>>> +
>>>> +    properties:
>>>> +      compatible:
>>>> +        items:
>>>> +          - enum:
>>>> +              - qcom,sm8450-bootstats
>>>
>>> This region isn't exclusive to sm8450, it exists also on sdm845 and
>>> presumably other platforms. Is there any need for an SoC specific
>>> compatible?
>>
>> Yes.
>> https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42
>>
>> Also see many discussions on LKML about this.

Ah, thanks both for the clarification and apologies for the confusion.
My main concern is that this binding and the associated driver work just
fine on sdm845 with no additional changes. Is it acceptable then to use
the qcom,sm8450-bootstats compatible there? If not, then for someone to
enable this driver on sdm845 would require not just a patch to add the
DT node, but also a patch to dt-bindings to add a compatible AND a patch
to the driver to use it.

Based on Dmitry's response on the driver patch [1], perhaps adding the
catch-all "qcom,imem-bootstats" compatible to the driver would be
suitable. If there are SoC specific parts in the future then match data
or gating with of_device_is_compatible() can be used with the SoC
specific compatible. This is how the qcom_stats driver handles things,
would this be an OK solution for everyone?

> 
> After taking another glance at the parent device (IMEM), I start to
> think that we should not be defining the device at all. The imem has the
> SoC name in it. So I think there should be a proper driver for IMEM.
> Then it will instantiate the ABL stats platform device depending on the
> SoC compat. Also this would allow us to rewrite qcom_pil_info_init() in
> a way to query IMEM instead of poking into DT directly.

+1 for handling this automagically in driver code, I'd be happy to look
into this in the future.

[1]
https://lore.kernel.org/linux-arm-msm/35ac64ab-512d-1425-7a1b-6e8d3806c8a8@linaro.org/
>
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
index ba694ce..d028bed 100644
--- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
+++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
@@ -49,6 +49,28 @@  patternProperties:
     $ref: /schemas/remoteproc/qcom,pil-info.yaml#
     description: Peripheral image loader relocation region
 
+  "^stats@[0-9a-f]+$":
+    type: object
+    description:
+      Imem region dedicated for storing timestamps related
+      information regarding bootstats.
+
+    additionalProperties: false
+
+    properties:
+      compatible:
+        items:
+          - enum:
+              - qcom,sm8450-bootstats
+          - const: qcom,imem-bootstats
+
+      reg:
+        maxItems: 1
+
+    required:
+      - compatible
+      - reg
+
 required:
   - compatible
   - reg