On Tue, Feb 27, 2024 at 5:04 AM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote: > > Hi Matt, > > On Tue, Feb 27, 2024 at 10:31 AM Matt Coster <Matt.Coster@xxxxxxxxxx> wrote: > > > > Hi Adam, > > > > Thanks for these patches! I'll just reply to this one patch, but my > > comments apply to them all. > > > > On 27/02/2024 03:45, Adam Ford wrote: > > > The GPU on the RZ/G2M is a Rogue GX6250 which uses firmware > > > rogue_4.45.2.58_v1.fw available from Imagination. > > > > > > When enumerated, it appears as: > > > powervr fd000000.gpu: [drm] loaded firmware powervr/rogue_4.45.2.58_v1.fw > > > powervr fd000000.gpu: [drm] FW version v1.0 (build 6513336 OS) > > > > These messages are printed after verifying the firmware blob’s headers, > > *before* attempting to upload it to the device. Just because they appear > > in dmesg does *not* imply the device is functional beyond the handful of > > register reads in pvr_load_gpu_id(). > > > > Since Mesa does not yet have support for this GPU, there’s not a lot > > that can be done to actually test these bindings. > > OK. > > > When we added upstream support for the first GPU (the AXE core in TI’s > > AM62), we opted to wait until userspace was sufficiently progressed to > > the point it could be used for testing. This thought process still > > applies when adding new GPUs. > > > > Our main concern is that adding bindings for GPUs implies a level of > > support that cannot be tested. That in turn may make it challenging to > > justify UAPI changes if/when they’re needed to actually make these GPUs > > functional. > > I guess that applies to "[PATCH 00/11] Device tree support for > Imagination Series5 GPU", too, which has been in linux-next for about > a month? > https://lore.kernel.org/all/20240109171950.31010-1-afd@xxxxxx/ > > > > Signed-off-by: Adam Ford <aford173@xxxxxxxxx> > > > > > > diff --git a/arch/arm64/boot/dts/renesas/r8a774a1.dtsi b/arch/arm64/boot/dts/renesas/r8a774a1.dtsi > > > index a8a44fe5e83b..8923d9624b39 100644 > > > --- a/arch/arm64/boot/dts/renesas/r8a774a1.dtsi > > > +++ b/arch/arm64/boot/dts/renesas/r8a774a1.dtsi > > > @@ -2352,6 +2352,16 @@ gic: interrupt-controller@f1010000 { > > > resets = <&cpg 408>; > > > }; > > > > > > + gpu: gpu@fd000000 { > > > + compatible = "renesas,r8a774a1-gpu", "img,img-axe"; > > > > The GX6250 is *not* an AXE core - it shouldn’t be listed as compatible > > with one. For prior art, see [1] where we added support for the MT8173 > > found in Elm Chromebooks R13 (also a Series6XT GPU). > > IC. And the bindings in [2]. > > > > > > + reg = <0 0xfd000000 0 0x20000>; > > > + clocks = <&cpg CPG_MOD 112>; > > > + clock-names = "core"; > > > > Series6XT cores have three clocks (see [1] again). I don’t have a > > Renesas TRM to hand – do you know if their docs go into detail on the > > GPU integration? > > Not really. The diagram in the Hardware User's Manual just shows the > following clock inputs: > - Clock (ZGϕ) from CPG, > - Clock (S3D1ϕ) from CPG, > - MSTP (ST112) from CPG. > > ZG is the main (programmable) 3DGE clock, running at up to 600 MHz. > S3D1 is the fixed 266 MHz AXI bus clock. > MSTP112 is the gateable module clock (part of the SYSC/CPG clock > domain), and its parent is ZG. > > According to the sources: > - "core" is the primary clock used by the entire GPU core, so we use > MSTP112 for that. > - "sys" is the optional system bus clock, so that could be S3D1, > - "mem" is the optional memory clock, no idea what that would map to. > > But IMHO the two optional clocks do not matter at all (the driver > doesn't care about their rates, and just enables them together with > the core clock), and S3D1 is always-on, so I'd just limit clocks to > a single item. Matt, When the time is right, and the driver is ready for Series 6XT-based systems, would Geert's rationale for supporting one clock be acceptable if I added his clock description to the commit message? > > Just wondering: is the availability of 1 clock specific to AXE, or to > the AXE integration on AM62x? > > > + interrupts = <GIC_SPI 119 IRQ_TYPE_LEVEL_HIGH>; > > + power-domains = <&sysc R8A774A1_PD_3DG_B>; > > + resets = <&cpg 112>; > > + }; > > > [1]: https://gitlab.freedesktop.org/imagination/linux/-/blob/b3506b8bc45ed6d4005eb32a994df0e33d6613f1/arch/arm64/boot/dts/mediatek/mt8173.dtsi#L993-1006 > > [2] https://gitlab.freedesktop.org/imagination/linux/-/blob/b3506b8bc45ed6d4005eb32a994df0e33d6613f1/Documentation/devicetree/bindings/gpu/img,powervr.yaml > > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
