diff mbox series

[v8,01/18] dt-bindings: display/msm: gpu: Document GMU wrapper-equipped A6xx

Message ID 20230223-topic-gmuwrapper-v8-1-69c68206609e@linaro.org
State New
Headers
Series GMU-less A6xx support (A610, A619_holi) |

Commit Message

Konrad Dybcio May 29, 2023, 1:52 p.m. UTC 
  The "GMU Wrapper" is Qualcomm's name for "let's treat the GPU blocks
we'd normally assign to the GMU as if they were a part of the GMU, even
though they are not". It's a (good) software representation of the GMU_CX
and GMU_GX register spaces within the GPUSS that helps us programatically
treat these de-facto GMU-less parts in a way that's very similar to their
GMU-equipped cousins, massively saving up on code duplication.

The "wrapper" register space was specifically designed to mimic the layout
of a real GMU, though it rather obviously does not have the M3 core et al.

GMU wrapper-equipped A6xx GPUs require clocks and clock-names to be
specified under the GPU node, just like their older cousins. Account
for that.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 .../devicetree/bindings/display/msm/gpu.yaml       | 61 ++++++++++++++++++----
 1 file changed, 52 insertions(+), 9 deletions(-)

Comments

Krzysztof Kozlowski May 30, 2023, 12:26 p.m. UTC | #1 
On Mon, 29 May 2023 15:52:20 +0200, Konrad Dybcio wrote:
> The "GMU Wrapper" is Qualcomm's name for "let's treat the GPU blocks
> we'd normally assign to the GMU as if they were a part of the GMU, even
> though they are not". It's a (good) software representation of the GMU_CX
> and GMU_GX register spaces within the GPUSS that helps us programatically
> treat these de-facto GMU-less parts in a way that's very similar to their
> GMU-equipped cousins, massively saving up on code duplication.
> 
> The "wrapper" register space was specifically designed to mimic the layout
> of a real GMU, though it rather obviously does not have the M3 core et al.
> 
> GMU wrapper-equipped A6xx GPUs require clocks and clock-names to be
> specified under the GPU node, just like their older cousins. Account
> for that.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  .../devicetree/bindings/display/msm/gpu.yaml       | 61 ++++++++++++++++++----
>  1 file changed, 52 insertions(+), 9 deletions(-)
> 

Running 'make dtbs_check' with the schema in this patch gives the
following warnings. Consider if they are expected or the schema is
incorrect. These may not be new warnings.

Note that it is not yet a requirement to have 0 warnings for dtbs_check.
This will change in the future.

Full log is available here: https://patchwork.ozlabs.org/patch/1787121


gpu@2c00000: compatible: 'oneOf' conditional failed, one must be fixed:
	arch/arm64/boot/dts/qcom/sm8150-hdk.dtb
	arch/arm64/boot/dts/qcom/sm8150-mtp.dtb
Konrad Dybcio May 30, 2023, 1:35 p.m. UTC | #2 
On 30.05.2023 14:26, Krzysztof Kozlowski wrote:
> On Mon, 29 May 2023 15:52:20 +0200, Konrad Dybcio wrote:
>> The "GMU Wrapper" is Qualcomm's name for "let's treat the GPU blocks
>> we'd normally assign to the GMU as if they were a part of the GMU, even
>> though they are not". It's a (good) software representation of the GMU_CX
>> and GMU_GX register spaces within the GPUSS that helps us programatically
>> treat these de-facto GMU-less parts in a way that's very similar to their
>> GMU-equipped cousins, massively saving up on code duplication.
>>
>> The "wrapper" register space was specifically designed to mimic the layout
>> of a real GMU, though it rather obviously does not have the M3 core et al.
>>
>> GMU wrapper-equipped A6xx GPUs require clocks and clock-names to be
>> specified under the GPU node, just like their older cousins. Account
>> for that.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>  .../devicetree/bindings/display/msm/gpu.yaml       | 61 ++++++++++++++++++----
>>  1 file changed, 52 insertions(+), 9 deletions(-)
>>
> 
> Running 'make dtbs_check' with the schema in this patch gives the
> following warnings. Consider if they are expected or the schema is
> incorrect. These may not be new warnings.
I think it'd be beneficial if the bot diffed the output of checks pre-
and post- patch.

Konrad
> 
> Note that it is not yet a requirement to have 0 warnings for dtbs_check.
> This will change in the future.
> 
> Full log is available here: https://patchwork.ozlabs.org/patch/1787121
> 
> 
> gpu@2c00000: compatible: 'oneOf' conditional failed, one must be fixed:
> 	arch/arm64/boot/dts/qcom/sm8150-hdk.dtb
> 	arch/arm64/boot/dts/qcom/sm8150-mtp.dtb
Rob Herring June 8, 2023, 8:58 p.m. UTC | #3 
On Tue, May 30, 2023 at 03:35:09PM +0200, Konrad Dybcio wrote:
> 
> 
> On 30.05.2023 14:26, Krzysztof Kozlowski wrote:
> > On Mon, 29 May 2023 15:52:20 +0200, Konrad Dybcio wrote:
> >> The "GMU Wrapper" is Qualcomm's name for "let's treat the GPU blocks
> >> we'd normally assign to the GMU as if they were a part of the GMU, even
> >> though they are not". It's a (good) software representation of the GMU_CX
> >> and GMU_GX register spaces within the GPUSS that helps us programatically
> >> treat these de-facto GMU-less parts in a way that's very similar to their
> >> GMU-equipped cousins, massively saving up on code duplication.
> >>
> >> The "wrapper" register space was specifically designed to mimic the layout
> >> of a real GMU, though it rather obviously does not have the M3 core et al.
> >>
> >> GMU wrapper-equipped A6xx GPUs require clocks and clock-names to be
> >> specified under the GPU node, just like their older cousins. Account
> >> for that.
> >>
> >> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> >> ---
> >>  .../devicetree/bindings/display/msm/gpu.yaml       | 61 ++++++++++++++++++----
> >>  1 file changed, 52 insertions(+), 9 deletions(-)
> >>
> > 
> > Running 'make dtbs_check' with the schema in this patch gives the
> > following warnings. Consider if they are expected or the schema is
> > incorrect. These may not be new warnings.
> I think it'd be beneficial if the bot diffed the output of checks pre-
> and post- patch.

Fix all the warnings and it will. ;) Care to donate h/w to run the build 
twice every time?

Really what I care about on these is when I keep getting changes to a 
schema and the list of warnings remains long and not getting fixed.

This case was less than useful with just the oneOf warning.

Rob
Rob Herring June 8, 2023, 9 p.m. UTC | #4 
On Mon, 29 May 2023 15:52:20 +0200, Konrad Dybcio wrote:
> The "GMU Wrapper" is Qualcomm's name for "let's treat the GPU blocks
> we'd normally assign to the GMU as if they were a part of the GMU, even
> though they are not". It's a (good) software representation of the GMU_CX
> and GMU_GX register spaces within the GPUSS that helps us programatically
> treat these de-facto GMU-less parts in a way that's very similar to their
> GMU-equipped cousins, massively saving up on code duplication.
> 
> The "wrapper" register space was specifically designed to mimic the layout
> of a real GMU, though it rather obviously does not have the M3 core et al.
> 
> GMU wrapper-equipped A6xx GPUs require clocks and clock-names to be
> specified under the GPU node, just like their older cousins. Account
> for that.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  .../devicetree/bindings/display/msm/gpu.yaml       | 61 ++++++++++++++++++----
>  1 file changed, 52 insertions(+), 9 deletions(-)
> 

Acked-by: Rob Herring <robh@kernel.org>
Konrad Dybcio June 9, 2023, 9:12 a.m. UTC | #5 
On 8.06.2023 22:58, Rob Herring wrote:
> On Tue, May 30, 2023 at 03:35:09PM +0200, Konrad Dybcio wrote:
>>
>>
>> On 30.05.2023 14:26, Krzysztof Kozlowski wrote:
>>> On Mon, 29 May 2023 15:52:20 +0200, Konrad Dybcio wrote:
>>>> The "GMU Wrapper" is Qualcomm's name for "let's treat the GPU blocks
>>>> we'd normally assign to the GMU as if they were a part of the GMU, even
>>>> though they are not". It's a (good) software representation of the GMU_CX
>>>> and GMU_GX register spaces within the GPUSS that helps us programatically
>>>> treat these de-facto GMU-less parts in a way that's very similar to their
>>>> GMU-equipped cousins, massively saving up on code duplication.
>>>>
>>>> The "wrapper" register space was specifically designed to mimic the layout
>>>> of a real GMU, though it rather obviously does not have the M3 core et al.
>>>>
>>>> GMU wrapper-equipped A6xx GPUs require clocks and clock-names to be
>>>> specified under the GPU node, just like their older cousins. Account
>>>> for that.
>>>>
>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>> ---
>>>>  .../devicetree/bindings/display/msm/gpu.yaml       | 61 ++++++++++++++++++----
>>>>  1 file changed, 52 insertions(+), 9 deletions(-)
>>>>
>>>
>>> Running 'make dtbs_check' with the schema in this patch gives the
>>> following warnings. Consider if they are expected or the schema is
>>> incorrect. These may not be new warnings.
>> I think it'd be beneficial if the bot diffed the output of checks pre-
>> and post- patch.
> 
> Fix all the warnings and it will. ;)
Nice one :P

Care to donate h/w to run the build 
> twice every time?
Personally that might be a bit difficult, but I'm pretty sure KernelCI
farms don't run at full throttle 24/7, perpaps some of their capacity
could be borrowed?

> 
> Really what I care about on these is when I keep getting changes to a 
> schema and the list of warnings remains long and not getting fixed.
> 
> This case was less than useful with just the oneOf warning.
Ack

Konrad
> 
> Rob
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/display/msm/gpu.yaml b/Documentation/devicetree/bindings/display/msm/gpu.yaml
index 5dabe7b6794b..58ca8912a8c3 100644
--- a/Documentation/devicetree/bindings/display/msm/gpu.yaml
+++ b/Documentation/devicetree/bindings/display/msm/gpu.yaml
@@ -36,10 +36,7 @@  properties:
 
   reg-names:
     minItems: 1
-    items:
-      - const: kgsl_3d0_reg_memory
-      - const: cx_mem
-      - const: cx_dbgc
+    maxItems: 3
 
   interrupts:
     maxItems: 1
@@ -157,16 +154,62 @@  allOf:
       required:
         - clocks
         - clock-names
+
   - if:
       properties:
         compatible:
           contains:
-            pattern: '^qcom,adreno-6[0-9][0-9]\.[0-9]$'
-
-    then: # Since Adreno 6xx series clocks should be defined in GMU
+            enum:
+              - qcom,adreno-610.0
+              - qcom,adreno-619.1
+    then:
       properties:
-        clocks: false
-        clock-names: false
+        clocks:
+          minItems: 6
+          maxItems: 6
+
+        clock-names:
+          items:
+            - const: core
+              description: GPU Core clock
+            - const: iface
+              description: GPU Interface clock
+            - const: mem_iface
+              description: GPU Memory Interface clock
+            - const: alt_mem_iface
+              description: GPU Alternative Memory Interface clock
+            - const: gmu
+              description: CX GMU clock
+            - const: xo
+              description: GPUCC clocksource clock
+
+        reg-names:
+          minItems: 1
+          items:
+            - const: kgsl_3d0_reg_memory
+            - const: cx_dbgc
+
+      required:
+        - clocks
+        - clock-names
+    else:
+      if:
+        properties:
+          compatible:
+            contains:
+              pattern: '^qcom,adreno-6[0-9][0-9]\.[0-9]$'
+
+      then: # Starting with A6xx, the clocks are usually defined in the GMU node
+        properties:
+          clocks: false
+          clock-names: false
+
+          reg-names:
+            minItems: 1
+            items:
+              - const: kgsl_3d0_reg_memory
+              - const: cx_mem
+              - const: cx_dbgc
 
 examples:
   - |