diff mbox series

[v3,1/8] dt-bindings: clock: qcom: Allow VDD_GFX supply to GX

Message ID 20240123-sa8295p-gpu-v3-1-d5b4474c8f33@quicinc.com
State New
Headers
Series arm64: dts: qcom: sa8295p: Enable GPU |

Commit Message

Bjorn Andersson Jan. 24, 2024, 4:25 a.m. UTC 
  In some designs the SoC's VDD_GFX pads are supplied by an external
regulator, rather than a power-domain. Allow this to be described in the
GPU clock controller binding.

Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
 Documentation/devicetree/bindings/clock/qcom,gpucc.yaml | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Krzysztof Kozlowski Jan. 24, 2024, 6:31 a.m. UTC | #1 
On 24/01/2024 05:25, Bjorn Andersson wrote:
> +# Allow either power-domains or vdd-gfx-supply, not both
> +oneOf:
> +  - required:
> +      - power-domains
> +  - required:
> +      - vdd-gfx-supply
> +  - not:
> +      anyOf:
> +        - required:
> +            - power-domains
> +        - required:
> +            - vdd-gfx-supply

I don't fully understand what you want to achieve here. If only "allow
either", so not a "require either", then simpler:

https://lore.kernel.org/all/20230118163208.GA117919-robh@kernel.org/


Best regards,
Krzysztof
Bjorn Andersson Jan. 24, 2024, 9:21 p.m. UTC | #2 
On Wed, Jan 24, 2024 at 07:31:34AM +0100, Krzysztof Kozlowski wrote:
> On 24/01/2024 05:25, Bjorn Andersson wrote:
> > +# Allow either power-domains or vdd-gfx-supply, not both
> > +oneOf:
> > +  - required:
> > +      - power-domains
> > +  - required:
> > +      - vdd-gfx-supply
> > +  - not:
> > +      anyOf:
> > +        - required:
> > +            - power-domains
> > +        - required:
> > +            - vdd-gfx-supply
> 
> I don't fully understand what you want to achieve here. If only "allow
> either", so not a "require either", then simpler:
> 
> https://lore.kernel.org/all/20230118163208.GA117919-robh@kernel.org/
> 

As discussed in v2, power-domains is currently an optional property in
this binding and I'm adding vdd-gfx-supply as an alternative to that.

As it's optional, barely any of our platforms define the property, so
requiring this would not be compatible with existing DT source.

It's clear that this does not accurately represent the power situation
for the block, so we should fix this. But I'd prefer to see that as a
separate task.


Implementation-wise, we need to figure how to consume multiple
power-domains in the GPUCC drivers in Linux, because the correct
definition seems to be to add both CX and GX/GFX domains here - and if
we just add them to the DT node Linux will break.

Regards,
Bjorn
Krzysztof Kozlowski Jan. 25, 2024, 7:39 a.m. UTC | #3 
On 24/01/2024 22:21, Bjorn Andersson wrote:
> On Wed, Jan 24, 2024 at 07:31:34AM +0100, Krzysztof Kozlowski wrote:
>> On 24/01/2024 05:25, Bjorn Andersson wrote:
>>> +# Allow either power-domains or vdd-gfx-supply, not both
>>> +oneOf:
>>> +  - required:
>>> +      - power-domains
>>> +  - required:
>>> +      - vdd-gfx-supply
>>> +  - not:
>>> +      anyOf:
>>> +        - required:
>>> +            - power-domains
>>> +        - required:
>>> +            - vdd-gfx-supply
>>
>> I don't fully understand what you want to achieve here. If only "allow
>> either", so not a "require either", then simpler:
>>
>> https://lore.kernel.org/all/20230118163208.GA117919-robh@kernel.org/
>>
> 
> As discussed in v2, power-domains is currently an optional property in
> this binding and I'm adding vdd-gfx-supply as an alternative to that.
> 

Then go with Rob's syntax - not:required: Much easier code.

Best regards,
Krzysztof
Bjorn Andersson Jan. 25, 2024, 9 p.m. UTC | #4 
On Thu, Jan 25, 2024 at 08:39:15AM +0100, Krzysztof Kozlowski wrote:
> On 24/01/2024 22:21, Bjorn Andersson wrote:
> > On Wed, Jan 24, 2024 at 07:31:34AM +0100, Krzysztof Kozlowski wrote:
> >> On 24/01/2024 05:25, Bjorn Andersson wrote:
> >>> +# Allow either power-domains or vdd-gfx-supply, not both
> >>> +oneOf:
> >>> +  - required:
> >>> +      - power-domains
> >>> +  - required:
> >>> +      - vdd-gfx-supply
> >>> +  - not:
> >>> +      anyOf:
> >>> +        - required:
> >>> +            - power-domains
> >>> +        - required:
> >>> +            - vdd-gfx-supply
> >>
> >> I don't fully understand what you want to achieve here. If only "allow
> >> either", so not a "require either", then simpler:
> >>
> >> https://lore.kernel.org/all/20230118163208.GA117919-robh@kernel.org/
> >>
> > 
> > As discussed in v2, power-domains is currently an optional property in
> > this binding and I'm adding vdd-gfx-supply as an alternative to that.
> > 
> 
> Then go with Rob's syntax - not:required: Much easier code.
> 

I looked at it, but was not able to understand that it expressed my
desired result. Now I do, and I agree with you, so will update it.

Thanks,
Bjorn
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/clock/qcom,gpucc.yaml b/Documentation/devicetree/bindings/clock/qcom,gpucc.yaml
index f369fa34e00c..c0dd24c9dcb3 100644
--- a/Documentation/devicetree/bindings/clock/qcom,gpucc.yaml
+++ b/Documentation/devicetree/bindings/clock/qcom,gpucc.yaml
@@ -53,6 +53,9 @@  properties:
   power-domains:
     maxItems: 1
 
+  vdd-gfx-supply:
+    description: Regulator supply for the VDD_GFX pads
+
   '#clock-cells':
     const: 1
 
@@ -74,6 +77,19 @@  required:
   - '#reset-cells'
   - '#power-domain-cells'
 
+# Allow either power-domains or vdd-gfx-supply, not both
+oneOf:
+  - required:
+      - power-domains
+  - required:
+      - vdd-gfx-supply
+  - not:
+      anyOf:
+        - required:
+            - power-domains
+        - required:
+            - vdd-gfx-supply
+
 additionalProperties: false
 
 examples: