[2/6] arm64: dts: renesas: r8a774a1: Enable GPU

  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)

Signed-off-by: Adam Ford <aford173@gmail.com>
  

On Tue, Feb 27, 2024 at 4:46 AM Adam Ford <aford173@gmail.com> 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)
>
> Signed-off-by: Adam Ford <aford173@gmail.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert
  

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.

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.

> Signed-off-by: Adam Ford <aford173@gmail.com>
> 
> 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).

> +			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?

> +			interrupts = <GIC_SPI 119 IRQ_TYPE_LEVEL_HIGH>;
> +			power-domains = <&sysc R8A774A1_PD_3DG_B>;
> +			resets = <&cpg 112>;
> +		};
> +
>  		pciec0: pcie@fe000000 {
>  			compatible = "renesas,pcie-r8a774a1",
>  				     "renesas,pcie-rcar-gen3";

As you probably expect by this point, I have to nack this series for
now. I appreciate your effort here and I’ll be happy to help you land
these once Mesa gains some form of usable support to allow testing.

Cheers,
Matt

[1]: https://gitlab.freedesktop.org/imagination/linux/-/blob/b3506b8bc45ed6d4005eb32a994df0e33d6613f1/arch/arm64/boot/dts/mediatek/mt8173.dtsi#L993-1006
  

Hi Matt,

On Tue, Feb 27, 2024 at 10:31 AM 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’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@ti.com/

> > Signed-off-by: Adam Ford <aford173@gmail.com>
> >
> > 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.

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,powervryaml


Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg

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
  

On Tue, Feb 27, 2024 at 3:31 AM 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’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.
>
> 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 wrongly assumed that when the firmware was ready, there was some
preliminary functionality, but it sounds like we need to work for
Series6XT support to be added to the driver.  I only used the AXE
compatible since it appeared to the be the only one and the existing
binding document stated "model/revision is fully discoverable" which I
interpreted to mean that the AXE compatible was sufficient.
>
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> >
> > 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).
>
> > +                     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?
>
> > +                     interrupts = <GIC_SPI 119 IRQ_TYPE_LEVEL_HIGH>;
> > +                     power-domains = <&sysc R8A774A1_PD_3DG_B>;
> > +                     resets = <&cpg 112>;
> > +             };
> > +
> >               pciec0: pcie@fe000000 {
> >                       compatible = "renesas,pcie-r8a774a1",
> >                                    "renesas,pcie-rcar-gen3";
>
> As you probably expect by this point, I have to nack this series for
> now. I appreciate your effort here and I’ll be happy to help you land

I get that.  I wasn't sure if I should have even pushed this, but I
wanted to get a little traction, because I know there are people like
myself who want to use the 3D in the Renesas boards, but don't want to
use the closed-source blobs tied to EULA and NDA documents.

> these once Mesa gains some form of usable support to allow testing.

Is there a way for your group to add me to the CC list when future
updates are submitted?  I'd like to follow this and resubmit when it's
ready.
>
> Cheers,
> Matt
>
> [1]: https://gitlab.freedesktop.org/imagination/linux/-/blob/b3506b8bc45ed6d4005eb32a994df0e33d6613f1/arch/arm64/boot/dts/mediatek/mt8173.dtsi#L993-1006
  

On Tue, Feb 27, 2024 at 5:04 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Matt,
>
> On Tue, Feb 27, 2024 at 10:31 AM 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’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@ti.com/
>
> > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > >
> > > 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@linux-m68k.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