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
>
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
