Received-SPF: pass (google.com: domain of linux-kernel+bounces-83149-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240227034539.193573-1-aford173@gmail.com> <20240227034539.193573-3-aford173@gmail.com>
 <39aead3b-b809-4c9c-8a5d-c0be2b36ea47@imgtec.com> <CAMuHMdW5vWg=tpB9PCRXmdBmLtt0wNN9dOEN1Lp_N7R68jz0tA@mail.gmail.com>
In-Reply-To: <CAMuHMdW5vWg=tpB9PCRXmdBmLtt0wNN9dOEN1Lp_N7R68jz0tA@mail.gmail.com>
From: Adam Ford <aford173@gmail.com>
Date: Tue, 27 Feb 2024 05:54:49 -0600
Message-ID: <CAHCN7x+EV8rw-Ya=wHOr6jsFAg47DDwKg3FZ9p=W-zDaBQEByw@mail.gmail.com>
Subject: Re: [PATCH 2/6] arm64: dts: renesas: r8a774a1: Enable GPU
To: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Matt Coster <Matt.Coster@imgtec.com>, 
	"dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>, 
	"linux-renesas-soc@vger.kernel.org" <linux-renesas-soc@vger.kernel.org>, Adam Ford <aford@beaconembedded.com>, 
	Frank Binns <Frank.Binns@imgtec.com>, David Airlie <airlied@gmail.com>, 
	Daniel Vetter <daniel@ffwll.ch>, Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, 
	Maxime Ripard <mripard@kernel.org>, Thomas Zimmermann <tzimmermann@suse.de>, Rob Herring <robh@kernel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, 
	Geert Uytterhoeven <geert+renesas@glider.be>, Magnus Damm <magnus.damm@gmail.com>, 
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>, 
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Feb 27, 2024 at 5:04=E2=80=AFAM Geert Uytterhoeven <geert@linux-m68=
k.org> wrote:
>
> Hi Matt,
>
> On Tue, Feb 27, 2024 at 10:31=E2=80=AFAM Matt Coster <Matt.Coster@imgtec.=
com> 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=E2=80=99s =
headers,
> > *before* attempting to upload it to the device. Just because they appea=
r
> > in dmesg does *not* imply the device is functional beyond the handful o=
f
> > register reads in pvr_load_gpu_id().
> >
> > Since Mesa does not yet have support for this GPU, there=E2=80=99s 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=E2=
=80=99s
> > 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=E2=80=99re needed to actually make th=
ese 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@ti.com/
>
> > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > >
> > > diff --git a/arch/arm64/boot/dts/renesas/r8a774a1.dtsi b/arch/arm64/b=
oot/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 =3D <&cpg 408>;
> > >               };
> > >
> > > +             gpu: gpu@fd000000 {
> > > +                     compatible =3D "renesas,r8a774a1-gpu", "img,img=
-axe";
> >
> > The GX6250 is *not* an AXE core - it shouldn=E2=80=99t be listed as com=
patible
> > 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 =3D <0 0xfd000000 0 0x20000>;
> > > +                     clocks =3D <&cpg CPG_MOD 112>;
> > > +                     clock-names =3D "core";
> >
> > Series6XT cores have three clocks (see [1] again). I don=E2=80=99t have=
 a
> > Renesas TRM to hand =E2=80=93 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=CF=95) from CPG,
>   - Clock (S3D1=CF=95) 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 =3D <GIC_SPI 119 IRQ_TYPE_LEVEL_HIGH>;
> > +                     power-domains =3D <&sysc R8A774A1_PD_3DG_B>;
> > +                     resets =3D <&cpg 112>;
> > +             };
>
> > [1]: https://gitlab.freedesktop.org/imagination/linux/-/blob/b3506b8bc4=
5ed6d4005eb32a994df0e33d6613f1/arch/arm64/boot/dts/mediatek/mt8173.dtsi#L99=
3-1006
>
> [2] https://gitlab.freedesktop.org/imagination/linux/-/blob/b3506b8bc45ed=
6d4005eb32a994df0e33d6613f1/Documentation/devicetree/bindings/gpu/img,power=
vr.yaml
>
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m6=
8k.org
>
> 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