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
