[v2,14/15] dt-bindings: gpu: mali-valhall-csf: Add initial bindings for panthor driver

Commit Message

Boris Brezillon Aug. 9, 2023, 4:53 p.m. UTC

From: Liviu Dudau <[e-mail]>

Arm has introduced a new v10 GPU architecture that replaces the Job Manager
interface with a new Command Stream Frontend. It adds firmware driven
command stream queues that can be used by kernel and user space to submit
jobs to the GPU.

Add the initial schema for the device tree that is based on support for
RK3588 SoC. The minimum number of clocks is one for the IP, but on Rockchip
platforms they will tend to expose the semi-independent clocks for better
power management.

v2:
- New commit

Signed-off-by: Liviu Dudau <[e-mail]>
Cc: Krzysztof Kozlowski <[e-mail]>
Cc: Rob Herring <[e-mail]>
Cc: Conor Dooley <[e-mail]>
Cc: [e-mail]
---
 .../bindings/gpu/arm,mali-valhall-csf.yaml    | 148 ++++++++++++++++++
 1 file changed, 148 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpu/arm,mali-valhall-csf.yaml

Comments

Krzysztof Kozlowski Aug. 20, 2023, 8:01 a.m. UTC | #1

On 09/08/2023 18:53, Boris Brezillon wrote:
> From: Liviu Dudau <[e-mail]>
> 
> Arm has introduced a new v10 GPU architecture that replaces the Job Manager
> interface with a new Command Stream Frontend. It adds firmware driven
> command stream queues that can be used by kernel and user space to submit
> jobs to the GPU.
> 
> Add the initial schema for the device tree that is based on support for
> RK3588 SoC. The minimum number of clocks is one for the IP, but on Rockchip
> platforms they will tend to expose the semi-independent clocks for better
> power management.

A nit, subject: drop second/last, redundant "bindings for". The
"dt-bindings" prefix is already stating that these are bindings.

Also drop "driver" form the subject. Bindings are for hardware, not drivers.

> 
> v2:
> - New commit
> 
> Signed-off-by: Liviu Dudau <[e-mail]>

SoB chain is incomplete.

> Cc: Krzysztof Kozlowski <[e-mail]>
> Cc: Rob Herring <[e-mail]>
> Cc: Conor Dooley <[e-mail]>
> Cc: [e-mail]
> ---
>  .../bindings/gpu/arm,mali-valhall-csf.yaml    | 148 ++++++++++++++++++
>  1 file changed, 148 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpu/arm,mali-valhall-csf.yaml
> 
> diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-valhall-csf.yaml b/Documentation/devicetree/bindings/gpu/arm,mali-valhall-csf.yaml
> new file mode 100644
> index 000000000000..2b9f77aa0b7a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpu/arm,mali-valhall-csf.yaml
> @@ -0,0 +1,148 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpu/arm,mali-valhall-csf.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ARM Mali Valhall GPU
> +
> +maintainers:
> +  - Liviu Dudau <[e-mail]>
> +  - Boris Brezillon <[e-mail]>
> +
> +properties:
> +  $nodename:
> +    pattern: '^gpu@[a-f0-9]+$'
> +
> +  compatible:
> +    oneOf:

Drop oneOf.

> +      - items:
> +          - enum:
> +              - rockchip,rk3588-mali
> +          - const: arm,mali-valhall-csf   # Mali Valhall GPU model/revision is fully discoverable
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    items:
> +      - description: Job interrupt
> +      - description: MMU interrupt
> +      - description: GPU interrupt
> +
> +  interrupt-names:
> +    items:
> +      - const: job
> +      - const: mmu
> +      - const: gpu
> +
> +  clocks:
> +    minItems: 1
> +    maxItems: 3
> +
> +  clock-names:
> +    minItems: 1
> +    items:
> +      - const: core
> +      - const: coregroup
> +      - const: stacks
> +
> +  mali-supply: true
> +
> +  sram-supply: true
> +
> +  operating-points-v2: true

Missing opp-table.

> +
> +  power-domains:
> +    minItems: 1
> +    maxItems: 5
> +
> +  power-domain-names:
> +    minItems: 1
> +    maxItems: 5
> +
> +  "#cooling-cells":
> +    const: 2
> +
> +  dynamic-power-coefficient:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      A u32 value that represents the running time dynamic
> +      power coefficient in units of uW/MHz/V^2. The
> +      coefficient can either be calculated from power
> +      measurements or derived by analysis.
> +
> +      The dynamic power consumption of the GPU is
> +      proportional to the square of the Voltage (V) and
> +      the clock frequency (f). The coefficient is used to
> +      calculate the dynamic power as below -
> +
> +      Pdyn = dynamic-power-coefficient * V^2 * f
> +
> +      where voltage is in V, frequency is in MHz.
> +
> +  dma-coherent: true
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - interrupt-names
> +  - clocks
> +  - mali-supply
> +
> +additionalProperties: false
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: rockchip,rk3588-mali
> +    then:
> +      properties:
> +        clocks:
> +          minItems: 3
> +        clock-names:
> +          items:
> +            - const: core
> +            - const: coregroup
> +            - const: stacks

This duplicates top-level. Just minItems: 3.

Please describe also power domains - constrains and names.

> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/rockchip,rk3588-cru.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/power/rk3588-power.h>
> +
> +    gpu: gpu@fb000000 {
> +        compatible = "rockchip,rk3588-mali", "arm,mali-valhall-csf";
> +        reg = <0xfb000000 0x200000>;
> +        interrupts = <GIC_SPI 92 IRQ_TYPE_LEVEL_HIGH 0>,
> +                     <GIC_SPI 93 IRQ_TYPE_LEVEL_HIGH 0>,
> +                     <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH 0>;
> +        interrupt-names = "job", "mmu", "gpu";
> +        clock-names = "core", "coregroup", "stacks";
> +        clocks = <&cru CLK_GPU>, <&cru CLK_GPU_COREGROUP>,
> +                 <&cru CLK_GPU_STACKS>;
> +        power-domains = <&power RK3588_PD_GPU>;
> +        operating-points-v2 = <&gpu_opp_table>;
> +        mali-supply = <&vdd_gpu_s0>;
> +        sram-supply = <&vdd_gpu_mem_s0>;
> +        status = "disabled";

Drop status.

> +    };
> +
> +    gpu_opp_table: opp-table {

Opp table should be inside the device node.

> +        compatible = "operating-points-v2";
> +        opp-300000000 {
> +            opp-hz = /bits/ 64 <300000000>;
> +            opp-microvolt = <675000 675000 850000>;
> +        };

Best regards,
Krzysztof

Liviu Dudau Sept. 20, 2023, 1:41 p.m. UTC | #2

Hi Krzysztof,

Thanks for taking the time to review this patch. I'm about to update it
to address your comments and I need some clarifications from you.

On Sun, Aug 20, 2023 at 10:01:25AM +0200, Krzysztof Kozlowski wrote:
> On 09/08/2023 18:53, Boris Brezillon wrote:
> > From: Liviu Dudau <[e-mail]>
> > 
> > Arm has introduced a new v10 GPU architecture that replaces the Job Manager
> > interface with a new Command Stream Frontend. It adds firmware driven
> > command stream queues that can be used by kernel and user space to submit
> > jobs to the GPU.
> > 
> > Add the initial schema for the device tree that is based on support for
> > RK3588 SoC. The minimum number of clocks is one for the IP, but on Rockchip
> > platforms they will tend to expose the semi-independent clocks for better
> > power management.
> 
> A nit, subject: drop second/last, redundant "bindings for". The
> "dt-bindings" prefix is already stating that these are bindings.
> 
> Also drop "driver" form the subject. Bindings are for hardware, not drivers.

Not exactly true as the 'compatible' string is for the driver, but I understand
your point.

> 
> > 
> > v2:
> > - New commit
> > 
> > Signed-off-by: Liviu Dudau <[e-mail]>
> 
> SoB chain is incomplete.
> 
> > Cc: Krzysztof Kozlowski <[e-mail]>
> > Cc: Rob Herring <[e-mail]>
> > Cc: Conor Dooley <[e-mail]>
> > Cc: [e-mail]
> > ---
> >  .../bindings/gpu/arm,mali-valhall-csf.yaml    | 148 ++++++++++++++++++
> >  1 file changed, 148 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/gpu/arm,mali-valhall-csf.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-valhall-csf.yaml b/Documentation/devicetree/bindings/gpu/arm,mali-valhall-csf.yaml
> > new file mode 100644
> > index 000000000000..2b9f77aa0b7a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpu/arm,mali-valhall-csf.yaml
> > @@ -0,0 +1,148 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/gpu/arm,mali-valhall-csf.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: ARM Mali Valhall GPU
> > +
> > +maintainers:
> > +  - Liviu Dudau <[e-mail]>
> > +  - Boris Brezillon <[e-mail]>
> > +
> > +properties:
> > +  $nodename:
> > +    pattern: '^gpu@[a-f0-9]+$'
> > +
> > +  compatible:
> > +    oneOf:
> 
> Drop oneOf.

The idea was to allow for future compatible strings to be added later, but
I guess we can re-introduce the oneOf entry later. Will remove it.

> 
> > +      - items:
> > +          - enum:
> > +              - rockchip,rk3588-mali
> > +          - const: arm,mali-valhall-csf   # Mali Valhall GPU model/revision is fully discoverable
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    items:
> > +      - description: Job interrupt
> > +      - description: MMU interrupt
> > +      - description: GPU interrupt
> > +
> > +  interrupt-names:
> > +    items:
> > +      - const: job
> > +      - const: mmu
> > +      - const: gpu
> > +
> > +  clocks:
> > +    minItems: 1
> > +    maxItems: 3
> > +
> > +  clock-names:
> > +    minItems: 1
> > +    items:
> > +      - const: core
> > +      - const: coregroup
> > +      - const: stacks
> > +
> > +  mali-supply: true
> > +
> > +  sram-supply: true
> > +
> > +  operating-points-v2: true
> 
> Missing opp-table.

This is the main topic I want to clarify. See further down for the main comment,
but I would like to understand what you are asking here. To copy the schema
from bindings/opp/opp-v2.yaml and bindings/opp/opp-v2-base.yaml?

> 
> > +
> > +  power-domains:
> > +    minItems: 1
> > +    maxItems: 5
> > +
> > +  power-domain-names:
> > +    minItems: 1
> > +    maxItems: 5
> > +
> > +  "#cooling-cells":
> > +    const: 2
> > +
> > +  dynamic-power-coefficient:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description:
> > +      A u32 value that represents the running time dynamic
> > +      power coefficient in units of uW/MHz/V^2. The
> > +      coefficient can either be calculated from power
> > +      measurements or derived by analysis.
> > +
> > +      The dynamic power consumption of the GPU is
> > +      proportional to the square of the Voltage (V) and
> > +      the clock frequency (f). The coefficient is used to
> > +      calculate the dynamic power as below -
> > +
> > +      Pdyn = dynamic-power-coefficient * V^2 * f
> > +
> > +      where voltage is in V, frequency is in MHz.
> > +
> > +  dma-coherent: true
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - interrupt-names
> > +  - clocks
> > +  - mali-supply
> > +
> > +additionalProperties: false
> > +
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: rockchip,rk3588-mali
> > +    then:
> > +      properties:
> > +        clocks:
> > +          minItems: 3
> > +        clock-names:
> > +          items:
> > +            - const: core
> > +            - const: coregroup
> > +            - const: stacks
> 
> This duplicates top-level. Just minItems: 3.

Will remove the duplicated names.

> 
> Please describe also power domains - constrains and names.

I'm not sure the power domains and how to handle them have been
entirely settled for Rockchip, hence why they were not included. Will
check with Collabora to see if they have anything to add here, but
for non-Rockchip platforms (like Juno with FPGAs) the constraints
are going to be different.

> 
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/rockchip,rk3588-cru.h>
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    #include <dt-bindings/power/rk3588-power.h>
> > +
> > +    gpu: gpu@fb000000 {
> > +        compatible = "rockchip,rk3588-mali", "arm,mali-valhall-csf";
> > +        reg = <0xfb000000 0x200000>;
> > +        interrupts = <GIC_SPI 92 IRQ_TYPE_LEVEL_HIGH 0>,
> > +                     <GIC_SPI 93 IRQ_TYPE_LEVEL_HIGH 0>,
> > +                     <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH 0>;
> > +        interrupt-names = "job", "mmu", "gpu";
> > +        clock-names = "core", "coregroup", "stacks";
> > +        clocks = <&cru CLK_GPU>, <&cru CLK_GPU_COREGROUP>,
> > +                 <&cru CLK_GPU_STACKS>;
> > +        power-domains = <&power RK3588_PD_GPU>;
> > +        operating-points-v2 = <&gpu_opp_table>;
> > +        mali-supply = <&vdd_gpu_s0>;
> > +        sram-supply = <&vdd_gpu_mem_s0>;
> > +        status = "disabled";
> 
> Drop status.

Will do.

> 
> > +    };
> > +
> > +    gpu_opp_table: opp-table {
> 
> Opp table should be inside the device node.

I cannot find any device tree that supports your suggested usage. Most (all?) of
the device trees that I can find have the opp table as a separate node from
the gpu and make use of the 'operating-points-v2 = <&opp_node_name>' reference
in the board fragment. To me that makes more sense as different boards can have
different operating points and is no reason to make them sub-nodes of the gpu.

> 
> > +        compatible = "operating-points-v2";
> > +        opp-300000000 {
> > +            opp-hz = /bits/ 64 <300000000>;
> > +            opp-microvolt = <675000 675000 850000>;
> > +        };
> 
> Best regards,
> Krzysztof
> 

Thanks again for your review!

Best regards,
Liviu

Krzysztof Kozlowski Sept. 20, 2023, 1:51 p.m. UTC | #3

On 20/09/2023 15:41, Liviu Dudau wrote:
>>> +properties:
>>> +  $nodename:
>>> +    pattern: '^gpu@[a-f0-9]+$'
>>> +
>>> +  compatible:
>>> +    oneOf:
>>
>> Drop oneOf.
> 
> The idea was to allow for future compatible strings to be added later, but
> I guess we can re-introduce the oneOf entry later. Will remove it.

If you already predict that new list will be added (so new fallback
compatible!), then it's fine.

> 
>>
>>> +      - items:
>>> +          - enum:
>>> +              - rockchip,rk3588-mali
>>> +          - const: arm,mali-valhall-csf   # Mali Valhall GPU model/revision is fully discoverable
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  interrupts:
>>> +    items:
>>> +      - description: Job interrupt
>>> +      - description: MMU interrupt
>>> +      - description: GPU interrupt
>>> +
>>> +  interrupt-names:
>>> +    items:
>>> +      - const: job
>>> +      - const: mmu
>>> +      - const: gpu
>>> +
>>> +  clocks:
>>> +    minItems: 1
>>> +    maxItems: 3
>>> +
>>> +  clock-names:
>>> +    minItems: 1
>>> +    items:
>>> +      - const: core
>>> +      - const: coregroup
>>> +      - const: stacks
>>> +
>>> +  mali-supply: true
>>> +
>>> +  sram-supply: true
>>> +
>>> +  operating-points-v2: true
>>
>> Missing opp-table.
> 
> This is the main topic I want to clarify. See further down for the main comment,
> but I would like to understand what you are asking here. To copy the schema
> from bindings/opp/opp-v2.yaml and bindings/opp/opp-v2-base.yaml?

No, "opp-table" property.
git grep "opp-table:"

> 
>>
>>> +
>>> +  power-domains:
>>> +    minItems: 1
>>> +    maxItems: 5
>>> +
>>> +  power-domain-names:
>>> +    minItems: 1
>>> +    maxItems: 5
>>> +
>>> +  "#cooling-cells":
>>> +    const: 2
>>> +
>>> +  dynamic-power-coefficient:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    description:
>>> +      A u32 value that represents the running time dynamic
>>> +      power coefficient in units of uW/MHz/V^2. The
>>> +      coefficient can either be calculated from power
>>> +      measurements or derived by analysis.
>>> +
>>> +      The dynamic power consumption of the GPU is
>>> +      proportional to the square of the Voltage (V) and
>>> +      the clock frequency (f). The coefficient is used to
>>> +      calculate the dynamic power as below -
>>> +
>>> +      Pdyn = dynamic-power-coefficient * V^2 * f
>>> +
>>> +      where voltage is in V, frequency is in MHz.
>>> +
>>> +  dma-coherent: true
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - interrupts
>>> +  - interrupt-names
>>> +  - clocks
>>> +  - mali-supply
>>> +
>>> +additionalProperties: false
>>> +
>>> +allOf:
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            const: rockchip,rk3588-mali
>>> +    then:
>>> +      properties:
>>> +        clocks:
>>> +          minItems: 3
>>> +        clock-names:
>>> +          items:
>>> +            - const: core
>>> +            - const: coregroup
>>> +            - const: stacks
>>
>> This duplicates top-level. Just minItems: 3.
> 
> Will remove the duplicated names.
> 
>>
>> Please describe also power domains - constrains and names.
> 
> I'm not sure the power domains and how to handle them have been
> entirely settled for Rockchip, hence why they were not included. Will
> check with Collabora to see if they have anything to add here, but
> for non-Rockchip platforms (like Juno with FPGAs) the constraints
> are going to be different.
> 
>>
>>> +
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/clock/rockchip,rk3588-cru.h>
>>> +    #include <dt-bindings/interrupt-controller/irq.h>
>>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>>> +    #include <dt-bindings/power/rk3588-power.h>
>>> +
>>> +    gpu: gpu@fb000000 {
>>> +        compatible = "rockchip,rk3588-mali", "arm,mali-valhall-csf";
>>> +        reg = <0xfb000000 0x200000>;
>>> +        interrupts = <GIC_SPI 92 IRQ_TYPE_LEVEL_HIGH 0>,
>>> +                     <GIC_SPI 93 IRQ_TYPE_LEVEL_HIGH 0>,
>>> +                     <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH 0>;
>>> +        interrupt-names = "job", "mmu", "gpu";
>>> +        clock-names = "core", "coregroup", "stacks";
>>> +        clocks = <&cru CLK_GPU>, <&cru CLK_GPU_COREGROUP>,
>>> +                 <&cru CLK_GPU_STACKS>;
>>> +        power-domains = <&power RK3588_PD_GPU>;
>>> +        operating-points-v2 = <&gpu_opp_table>;
>>> +        mali-supply = <&vdd_gpu_s0>;
>>> +        sram-supply = <&vdd_gpu_mem_s0>;
>>> +        status = "disabled";
>>
>> Drop status.
> 
> Will do.
> 
>>
>>> +    };
>>> +
>>> +    gpu_opp_table: opp-table {
>>
>> Opp table should be inside the device node.
> 
> I cannot find any device tree that supports your suggested usage. Most (all?) of

Really? All Qcom have it embedded.

> the device trees that I can find have the opp table as a separate node from
> the gpu and make use of the 'operating-points-v2 = <&opp_node_name>' reference

operating-points-v2 is needed anyway, I am not suggesting to drop it.

> in the board fragment. To me that makes more sense as different boards can have
> different operating points and is no reason to make them sub-nodes of the gpu.

How boards do it, is independent. They can keep it inside, outside,
override etc.

For majority of simple cases, the OPPs come from the SoC, thus they are
in DTSI.

Best regards,
Krzysztof

Boris Brezillon Sept. 20, 2023, 1:56 p.m. UTC | #4

On Wed, 20 Sep 2023 14:41:05 +0100
Liviu Dudau <[e-mail]> wrote:

> > 
> > Please describe also power domains - constrains and names.  
> 
> I'm not sure the power domains and how to handle them have been
> entirely settled for Rockchip, hence why they were not included. Will
> check with Collabora to see if they have anything to add here,

On rk3588, it's just one power domain feeding all GPU blocks.

> but
> for non-Rockchip platforms (like Juno with FPGAs) the constraints
> are going to be different.

Liviu Dudau Sept. 20, 2023, 2:03 p.m. UTC | #5

On Wed, Sep 20, 2023 at 03:56:24PM +0200, Boris Brezillon wrote:
> On Wed, 20 Sep 2023 14:41:05 +0100
> Liviu Dudau <[e-mail]> wrote:
> 
> > > 
> > > Please describe also power domains - constrains and names.  
> > 
> > I'm not sure the power domains and how to handle them have been
> > entirely settled for Rockchip, hence why they were not included. Will
> > check with Collabora to see if they have anything to add here,
> 
> On rk3588, it's just one power domain feeding all GPU blocks.

Not sure if this has been missed, but earlier in the file there is an
entry for power-domains and power-domain-names, but with only a minimum
of requirements and no mandate on the actual names.

Best regards,
Liviu

> 
> > but
> > for non-Rockchip platforms (like Juno with FPGAs) the constraints
> > are going to be different.

Liviu Dudau Sept. 20, 2023, 2:25 p.m. UTC | #6

On Wed, Sep 20, 2023 at 03:51:36PM +0200, Krzysztof Kozlowski wrote:
> On 20/09/2023 15:41, Liviu Dudau wrote:
> >>> +properties:
> >>> +  $nodename:
> >>> +    pattern: '^gpu@[a-f0-9]+$'
> >>> +
> >>> +  compatible:
> >>> +    oneOf:
> >>
> >> Drop oneOf.
> > 
> > The idea was to allow for future compatible strings to be added later, but
> > I guess we can re-introduce the oneOf entry later. Will remove it.
> 
> If you already predict that new list will be added (so new fallback
> compatible!), then it's fine.
> 
> > 
> >>
> >>> +      - items:
> >>> +          - enum:
> >>> +              - rockchip,rk3588-mali
> >>> +          - const: arm,mali-valhall-csf   # Mali Valhall GPU model/revision is fully discoverable
> >>> +
> >>> +  reg:
> >>> +    maxItems: 1
> >>> +
> >>> +  interrupts:
> >>> +    items:
> >>> +      - description: Job interrupt
> >>> +      - description: MMU interrupt
> >>> +      - description: GPU interrupt
> >>> +
> >>> +  interrupt-names:
> >>> +    items:
> >>> +      - const: job
> >>> +      - const: mmu
> >>> +      - const: gpu
> >>> +
> >>> +  clocks:
> >>> +    minItems: 1
> >>> +    maxItems: 3
> >>> +
> >>> +  clock-names:
> >>> +    minItems: 1
> >>> +    items:
> >>> +      - const: core
> >>> +      - const: coregroup
> >>> +      - const: stacks
> >>> +
> >>> +  mali-supply: true
> >>> +
> >>> +  sram-supply: true
> >>> +
> >>> +  operating-points-v2: true
> >>
> >> Missing opp-table.
> > 
> > This is the main topic I want to clarify. See further down for the main comment,
> > but I would like to understand what you are asking here. To copy the schema
> > from bindings/opp/opp-v2.yaml and bindings/opp/opp-v2-base.yaml?
> 
> No, "opp-table" property.
> git grep "opp-table:"

You mean adding

     opp-table:
       type: object

as property? What's the difference between opp-table: true (like in
'display/msm/dp-controller.yaml') and 'opp-table: type: object' like in other
places that I can find? Does that mean you need to have an opp-table node somewhere
but it doesn't have to be inside the gpu node? 'arm,mali-midgard.yaml' that was
my original source of inspiration has an 'opp-table: type: object' in the properties
but the example still shows a separate node for table.

> 
> > 
> >>
> >>> +
> >>> +  power-domains:
> >>> +    minItems: 1
> >>> +    maxItems: 5
> >>> +
> >>> +  power-domain-names:
> >>> +    minItems: 1
> >>> +    maxItems: 5
> >>> +
> >>> +  "#cooling-cells":
> >>> +    const: 2
> >>> +
> >>> +  dynamic-power-coefficient:
> >>> +    $ref: /schemas/types.yaml#/definitions/uint32
> >>> +    description:
> >>> +      A u32 value that represents the running time dynamic
> >>> +      power coefficient in units of uW/MHz/V^2. The
> >>> +      coefficient can either be calculated from power
> >>> +      measurements or derived by analysis.
> >>> +
> >>> +      The dynamic power consumption of the GPU is
> >>> +      proportional to the square of the Voltage (V) and
> >>> +      the clock frequency (f). The coefficient is used to
> >>> +      calculate the dynamic power as below -
> >>> +
> >>> +      Pdyn = dynamic-power-coefficient * V^2 * f
> >>> +
> >>> +      where voltage is in V, frequency is in MHz.
> >>> +
> >>> +  dma-coherent: true
> >>> +
> >>> +required:
> >>> +  - compatible
> >>> +  - reg
> >>> +  - interrupts
> >>> +  - interrupt-names
> >>> +  - clocks
> >>> +  - mali-supply
> >>> +
> >>> +additionalProperties: false
> >>> +
> >>> +allOf:
> >>> +  - if:
> >>> +      properties:
> >>> +        compatible:
> >>> +          contains:
> >>> +            const: rockchip,rk3588-mali
> >>> +    then:
> >>> +      properties:
> >>> +        clocks:
> >>> +          minItems: 3
> >>> +        clock-names:
> >>> +          items:
> >>> +            - const: core
> >>> +            - const: coregroup
> >>> +            - const: stacks
> >>
> >> This duplicates top-level. Just minItems: 3.
> > 
> > Will remove the duplicated names.
> > 
> >>
> >> Please describe also power domains - constrains and names.
> > 
> > I'm not sure the power domains and how to handle them have been
> > entirely settled for Rockchip, hence why they were not included. Will
> > check with Collabora to see if they have anything to add here, but
> > for non-Rockchip platforms (like Juno with FPGAs) the constraints
> > are going to be different.
> > 
> >>
> >>> +
> >>> +examples:
> >>> +  - |
> >>> +    #include <dt-bindings/clock/rockchip,rk3588-cru.h>
> >>> +    #include <dt-bindings/interrupt-controller/irq.h>
> >>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> >>> +    #include <dt-bindings/power/rk3588-power.h>
> >>> +
> >>> +    gpu: gpu@fb000000 {
> >>> +        compatible = "rockchip,rk3588-mali", "arm,mali-valhall-csf";
> >>> +        reg = <0xfb000000 0x200000>;
> >>> +        interrupts = <GIC_SPI 92 IRQ_TYPE_LEVEL_HIGH 0>,
> >>> +                     <GIC_SPI 93 IRQ_TYPE_LEVEL_HIGH 0>,
> >>> +                     <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH 0>;
> >>> +        interrupt-names = "job", "mmu", "gpu";
> >>> +        clock-names = "core", "coregroup", "stacks";
> >>> +        clocks = <&cru CLK_GPU>, <&cru CLK_GPU_COREGROUP>,
> >>> +                 <&cru CLK_GPU_STACKS>;
> >>> +        power-domains = <&power RK3588_PD_GPU>;
> >>> +        operating-points-v2 = <&gpu_opp_table>;
> >>> +        mali-supply = <&vdd_gpu_s0>;
> >>> +        sram-supply = <&vdd_gpu_mem_s0>;
> >>> +        status = "disabled";
> >>
> >> Drop status.
> > 
> > Will do.
> > 
> >>
> >>> +    };
> >>> +
> >>> +    gpu_opp_table: opp-table {
> >>
> >> Opp table should be inside the device node.
> > 
> > I cannot find any device tree that supports your suggested usage. Most (all?) of
> 
> Really? All Qcom have it embedded.

The arm,mali-* ones seem to have them outside the gpu node. See "arm,mali-midgard.yaml"

Best regards,
Liviu

> 
> > the device trees that I can find have the opp table as a separate node from
> > the gpu and make use of the 'operating-points-v2 = <&opp_node_name>' reference
> 
> operating-points-v2 is needed anyway, I am not suggesting to drop it.
> 
> > in the board fragment. To me that makes more sense as different boards can have
> > different operating points and is no reason to make them sub-nodes of the gpu.
> 
> How boards do it, is independent. They can keep it inside, outside,
> override etc.
> 
> For majority of simple cases, the OPPs come from the SoC, thus they are
> in DTSI.
> 
> Best regards,
> Krzysztof
>

Krzysztof Kozlowski Sept. 20, 2023, 3:31 p.m. UTC | #7

On 20/09/2023 16:25, Liviu Dudau wrote:
> On Wed, Sep 20, 2023 at 03:51:36PM +0200, Krzysztof Kozlowski wrote:
>> On 20/09/2023 15:41, Liviu Dudau wrote:
>>>>> +properties:
>>>>> +  $nodename:
>>>>> +    pattern: '^gpu@[a-f0-9]+$'
>>>>> +
>>>>> +  compatible:
>>>>> +    oneOf:
>>>>
>>>> Drop oneOf.
>>>
>>> The idea was to allow for future compatible strings to be added later, but
>>> I guess we can re-introduce the oneOf entry later. Will remove it.
>>
>> If you already predict that new list will be added (so new fallback
>> compatible!), then it's fine.
>>
>>>
>>>>
>>>>> +      - items:
>>>>> +          - enum:
>>>>> +              - rockchip,rk3588-mali
>>>>> +          - const: arm,mali-valhall-csf   # Mali Valhall GPU model/revision is fully discoverable
>>>>> +
>>>>> +  reg:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  interrupts:
>>>>> +    items:
>>>>> +      - description: Job interrupt
>>>>> +      - description: MMU interrupt
>>>>> +      - description: GPU interrupt
>>>>> +
>>>>> +  interrupt-names:
>>>>> +    items:
>>>>> +      - const: job
>>>>> +      - const: mmu
>>>>> +      - const: gpu
>>>>> +
>>>>> +  clocks:
>>>>> +    minItems: 1
>>>>> +    maxItems: 3
>>>>> +
>>>>> +  clock-names:
>>>>> +    minItems: 1
>>>>> +    items:
>>>>> +      - const: core
>>>>> +      - const: coregroup
>>>>> +      - const: stacks
>>>>> +
>>>>> +  mali-supply: true
>>>>> +
>>>>> +  sram-supply: true
>>>>> +
>>>>> +  operating-points-v2: true
>>>>
>>>> Missing opp-table.
>>>
>>> This is the main topic I want to clarify. See further down for the main comment,
>>> but I would like to understand what you are asking here. To copy the schema
>>> from bindings/opp/opp-v2.yaml and bindings/opp/opp-v2-base.yaml?
>>
>> No, "opp-table" property.
>> git grep "opp-table:"
> 
> You mean adding
> 
>      opp-table:
>        type: object
> 
> as property? What's the difference between opp-table: true (like in
> 'display/msm/dp-controller.yaml') and 'opp-table: type: object' like in other

There is no opp-table: true. Nowhere.

...

>>>
>>>>
>>>>> +    };
>>>>> +
>>>>> +    gpu_opp_table: opp-table {
>>>>
>>>> Opp table should be inside the device node.
>>>
>>> I cannot find any device tree that supports your suggested usage. Most (all?) of
>>
>> Really? All Qcom have it embedded.
> 
> The arm,mali-* ones seem to have them outside the gpu node. See "arm,mali-midgard.yaml"

You said you cannot find any, so I pointed out that's not true.

Best regards,
Krzysztof

Patch

diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-valhall-csf.yaml b/Documentation/devicetree/bindings/gpu/arm,mali-valhall-csf.yaml
new file mode 100644
index 000000000000..2b9f77aa0b7a
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpu/arm,mali-valhall-csf.yaml
@@ -0,0 +1,148 @@ 
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpu/arm,mali-valhall-csf.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ARM Mali Valhall GPU
+
+maintainers:
+  - Liviu Dudau <[e-mail]>
+  - Boris Brezillon <[e-mail]>
+
+properties:
+  $nodename:
+    pattern: '^gpu@[a-f0-9]+$'
+
+  compatible:
+    oneOf:
+      - items:
+          - enum:
+              - rockchip,rk3588-mali
+          - const: arm,mali-valhall-csf   # Mali Valhall GPU model/revision is fully discoverable
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    items:
+      - description: Job interrupt
+      - description: MMU interrupt
+      - description: GPU interrupt
+
+  interrupt-names:
+    items:
+      - const: job
+      - const: mmu
+      - const: gpu
+
+  clocks:
+    minItems: 1
+    maxItems: 3
+
+  clock-names:
+    minItems: 1
+    items:
+      - const: core
+      - const: coregroup
+      - const: stacks
+
+  mali-supply: true
+
+  sram-supply: true
+
+  operating-points-v2: true
+
+  power-domains:
+    minItems: 1
+    maxItems: 5
+
+  power-domain-names:
+    minItems: 1
+    maxItems: 5
+
+  "#cooling-cells":
+    const: 2
+
+  dynamic-power-coefficient:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      A u32 value that represents the running time dynamic
+      power coefficient in units of uW/MHz/V^2. The
+      coefficient can either be calculated from power
+      measurements or derived by analysis.
+
+      The dynamic power consumption of the GPU is
+      proportional to the square of the Voltage (V) and
+      the clock frequency (f). The coefficient is used to
+      calculate the dynamic power as below -
+
+      Pdyn = dynamic-power-coefficient * V^2 * f
+
+      where voltage is in V, frequency is in MHz.
+
+  dma-coherent: true
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - interrupt-names
+  - clocks
+  - mali-supply
+
+additionalProperties: false
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: rockchip,rk3588-mali
+    then:
+      properties:
+        clocks:
+          minItems: 3
+        clock-names:
+          items:
+            - const: core
+            - const: coregroup
+            - const: stacks
+
+examples:
+  - |
+    #include <dt-bindings/clock/rockchip,rk3588-cru.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/power/rk3588-power.h>
+
+    gpu: gpu@fb000000 {
+        compatible = "rockchip,rk3588-mali", "arm,mali-valhall-csf";
+        reg = <0xfb000000 0x200000>;
+        interrupts = <GIC_SPI 92 IRQ_TYPE_LEVEL_HIGH 0>,
+                     <GIC_SPI 93 IRQ_TYPE_LEVEL_HIGH 0>,
+                     <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH 0>;
+        interrupt-names = "job", "mmu", "gpu";
+        clock-names = "core", "coregroup", "stacks";
+        clocks = <&cru CLK_GPU>, <&cru CLK_GPU_COREGROUP>,
+                 <&cru CLK_GPU_STACKS>;
+        power-domains = <&power RK3588_PD_GPU>;
+        operating-points-v2 = <&gpu_opp_table>;
+        mali-supply = <&vdd_gpu_s0>;
+        sram-supply = <&vdd_gpu_mem_s0>;
+        status = "disabled";
+    };
+
+    gpu_opp_table: opp-table {
+        compatible = "operating-points-v2";
+        opp-300000000 {
+            opp-hz = /bits/ 64 <300000000>;
+            opp-microvolt = <675000 675000 850000>;
+        };
+        opp-400000000 {
+            opp-hz = /bits/ 64 <400000000>;
+            opp-microvolt = <675000 675000 850000>;
+        };
+    };
+
+...