mbox series

[V3,0/9] arm64: imx8mn: Enable more imx8m Nano functions

Message ID 20211104161804.587250-1-aford173@gmail.com (mailing list archive)
Headers show
Series arm64: imx8mn: Enable more imx8m Nano functions | expand

Message

Adam Ford Nov. 4, 2021, 4:17 p.m. UTC
The i.MX8M Nano is similar to the i.MX8M Mini in some ways, but very 
different in others.  With the blk-ctrl driver for Mini in place, 
this series expands the blk-ctrl driver to support the Nano which
opens the door for additional functions in the future.  As part of
this series, it also addresses some issues in the GPCv2 driver and
finally adds support for enabling USB and GPU.

V3:  Fixes an the yaml example
V2:  Fixes the clock count in the blk-ctrl

Adam Ford (9):
  soc: imx: gpcv2: keep i.MX8MN gpumix bus clock enabled
  soc: imx: gpcv2: Add dispmix and mipi domains to imx8mn
  dt-bindings: power: imx8mn: add defines for DISP blk-ctrl domains
  dt-bindings: soc: add binding for i.MX8MN DISP blk-ctrl
  soc: imx: imx8m-blk-ctrl: add i.MX8MN DISP blk-ctrl
  arm64: dts: imx8mn: add GPC node
  arm64: dts: imx8mn: put USB controller into power-domains
  arm64: dts: imx8mn: add DISP blk-ctrl
  arm64: dts: imx8mn: Enable GPU

 .../soc/imx/fsl,imx8mn-disp-blk-ctrl.yaml     |  97 +++++++++++++++++
 arch/arm64/boot/dts/freescale/imx8mn.dtsi     | 103 ++++++++++++++++++
 drivers/soc/imx/gpcv2.c                       |  26 +++++
 drivers/soc/imx/imx8m-blk-ctrl.c              |  75 ++++++++++++-
 include/dt-bindings/power/imx8mn-power.h      |   5 +
 5 files changed, 305 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/soc/imx/fsl,imx8mn-disp-blk-ctrl.yaml

Comments

Tim Harvey Nov. 16, 2021, 5:57 p.m. UTC | #1
On Thu, Nov 4, 2021 at 9:18 AM Adam Ford <aford173@gmail.com> wrote:
>
> The i.MX8M Nano is similar to the i.MX8M Mini in some ways, but very
> different in others.  With the blk-ctrl driver for Mini in place,
> this series expands the blk-ctrl driver to support the Nano which
> opens the door for additional functions in the future.  As part of
> this series, it also addresses some issues in the GPCv2 driver and
> finally adds support for enabling USB and GPU.
>
> V3:  Fixes an the yaml example
> V2:  Fixes the clock count in the blk-ctrl
>
> Adam Ford (9):
>   soc: imx: gpcv2: keep i.MX8MN gpumix bus clock enabled
>   soc: imx: gpcv2: Add dispmix and mipi domains to imx8mn
>   dt-bindings: power: imx8mn: add defines for DISP blk-ctrl domains
>   dt-bindings: soc: add binding for i.MX8MN DISP blk-ctrl
>   soc: imx: imx8m-blk-ctrl: add i.MX8MN DISP blk-ctrl
>   arm64: dts: imx8mn: add GPC node
>   arm64: dts: imx8mn: put USB controller into power-domains
>   arm64: dts: imx8mn: add DISP blk-ctrl
>   arm64: dts: imx8mn: Enable GPU
>
>  .../soc/imx/fsl,imx8mn-disp-blk-ctrl.yaml     |  97 +++++++++++++++++
>  arch/arm64/boot/dts/freescale/imx8mn.dtsi     | 103 ++++++++++++++++++
>  drivers/soc/imx/gpcv2.c                       |  26 +++++
>  drivers/soc/imx/imx8m-blk-ctrl.c              |  75 ++++++++++++-
>  include/dt-bindings/power/imx8mn-power.h      |   5 +
>  5 files changed, 305 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/soc/imx/fsl,imx8mn-disp-blk-ctrl.yaml
>

Adam,

Thanks for the patches. I'm not sure how best to test this but on an
imx8mm-venice-gw7902 which has USB, but no display.

I find that if DRM_ETNAVIV is enabled I hang at 'etnaviv etnaviv:
bound 38000000.gpu (ops 0xffff800010964748)'.

If DRM_ETNAVIV is not enabled:
- boots fine
- usb works
- soft reboot works (does not hang)

Best regards,

Tim
Adam Ford Nov. 16, 2021, 6:04 p.m. UTC | #2
On Tue, Nov 16, 2021 at 11:57 AM Tim Harvey <tharvey@gateworks.com> wrote:
>
> On Thu, Nov 4, 2021 at 9:18 AM Adam Ford <aford173@gmail.com> wrote:
> >
> > The i.MX8M Nano is similar to the i.MX8M Mini in some ways, but very
> > different in others.  With the blk-ctrl driver for Mini in place,
> > this series expands the blk-ctrl driver to support the Nano which
> > opens the door for additional functions in the future.  As part of
> > this series, it also addresses some issues in the GPCv2 driver and
> > finally adds support for enabling USB and GPU.
> >
> > V3:  Fixes an the yaml example
> > V2:  Fixes the clock count in the blk-ctrl
> >
> > Adam Ford (9):
> >   soc: imx: gpcv2: keep i.MX8MN gpumix bus clock enabled
> >   soc: imx: gpcv2: Add dispmix and mipi domains to imx8mn
> >   dt-bindings: power: imx8mn: add defines for DISP blk-ctrl domains
> >   dt-bindings: soc: add binding for i.MX8MN DISP blk-ctrl
> >   soc: imx: imx8m-blk-ctrl: add i.MX8MN DISP blk-ctrl
> >   arm64: dts: imx8mn: add GPC node
> >   arm64: dts: imx8mn: put USB controller into power-domains
> >   arm64: dts: imx8mn: add DISP blk-ctrl
> >   arm64: dts: imx8mn: Enable GPU
> >
> >  .../soc/imx/fsl,imx8mn-disp-blk-ctrl.yaml     |  97 +++++++++++++++++
> >  arch/arm64/boot/dts/freescale/imx8mn.dtsi     | 103 ++++++++++++++++++
> >  drivers/soc/imx/gpcv2.c                       |  26 +++++
> >  drivers/soc/imx/imx8m-blk-ctrl.c              |  75 ++++++++++++-
> >  include/dt-bindings/power/imx8mn-power.h      |   5 +
> >  5 files changed, 305 insertions(+), 1 deletion(-)
> >  create mode 100644 Documentation/devicetree/bindings/soc/imx/fsl,imx8mn-disp-blk-ctrl.yaml
> >
>
> Adam,
>
> Thanks for the patches. I'm not sure how best to test this but on an
> imx8mm-venice-gw7902 which has USB, but no display.
>
> I find that if DRM_ETNAVIV is enabled I hang at 'etnaviv etnaviv:
> bound 38000000.gpu (ops 0xffff800010964748)'.

Thanks for testing this.

Does your board send power to the GPU?  I recall someone somewhere
didn't power theirGPU, but I can't remember who and/or what board was
being discussed.

I'll run some more tests on the latest 5.16-rc1 to see if I can
replicate your issue.

adam
>
> If DRM_ETNAVIV is not enabled:
> - boots fine
> - usb works
> - soft reboot works (does not hang)

At least we have some progress.  :-)

adam
>
> Best regards,
>
> Tim
Tim Harvey Nov. 16, 2021, 6:27 p.m. UTC | #3
On Tue, Nov 16, 2021 at 10:04 AM Adam Ford <aford173@gmail.com> wrote:
>
> On Tue, Nov 16, 2021 at 11:57 AM Tim Harvey <tharvey@gateworks.com> wrote:
> >
> > On Thu, Nov 4, 2021 at 9:18 AM Adam Ford <aford173@gmail.com> wrote:
> > >
> > > The i.MX8M Nano is similar to the i.MX8M Mini in some ways, but very
> > > different in others.  With the blk-ctrl driver for Mini in place,
> > > this series expands the blk-ctrl driver to support the Nano which
> > > opens the door for additional functions in the future.  As part of
> > > this series, it also addresses some issues in the GPCv2 driver and
> > > finally adds support for enabling USB and GPU.
> > >
> > > V3:  Fixes an the yaml example
> > > V2:  Fixes the clock count in the blk-ctrl
> > >
> > > Adam Ford (9):
> > >   soc: imx: gpcv2: keep i.MX8MN gpumix bus clock enabled
> > >   soc: imx: gpcv2: Add dispmix and mipi domains to imx8mn
> > >   dt-bindings: power: imx8mn: add defines for DISP blk-ctrl domains
> > >   dt-bindings: soc: add binding for i.MX8MN DISP blk-ctrl
> > >   soc: imx: imx8m-blk-ctrl: add i.MX8MN DISP blk-ctrl
> > >   arm64: dts: imx8mn: add GPC node
> > >   arm64: dts: imx8mn: put USB controller into power-domains
> > >   arm64: dts: imx8mn: add DISP blk-ctrl
> > >   arm64: dts: imx8mn: Enable GPU
> > >
> > >  .../soc/imx/fsl,imx8mn-disp-blk-ctrl.yaml     |  97 +++++++++++++++++
> > >  arch/arm64/boot/dts/freescale/imx8mn.dtsi     | 103 ++++++++++++++++++
> > >  drivers/soc/imx/gpcv2.c                       |  26 +++++
> > >  drivers/soc/imx/imx8m-blk-ctrl.c              |  75 ++++++++++++-
> > >  include/dt-bindings/power/imx8mn-power.h      |   5 +
> > >  5 files changed, 305 insertions(+), 1 deletion(-)
> > >  create mode 100644 Documentation/devicetree/bindings/soc/imx/fsl,imx8mn-disp-blk-ctrl.yaml
> > >
> >
> > Adam,
> >
> > Thanks for the patches. I'm not sure how best to test this but on an
> > imx8mm-venice-gw7902 which has USB, but no display.
> >
> > I find that if DRM_ETNAVIV is enabled I hang at 'etnaviv etnaviv:
> > bound 38000000.gpu (ops 0xffff800010964748)'.
>
> Thanks for testing this.
>
> Does your board send power to the GPU?  I recall someone somewhere
> didn't power theirGPU, but I can't remember who and/or what board was
> being discussed.

Yes, the imx8mm-venice-gw7902 does have the GPU powered (the
imx8mm-venice-gw7901 was the one that did not).

Tim

>
> I'll run some more tests on the latest 5.16-rc1 to see if I can
> replicate your issue.
>
> adam
> >
> > If DRM_ETNAVIV is not enabled:
> > - boots fine
> > - usb works
> > - soft reboot works (does not hang)
>
> At least we have some progress.  :-)
>
> adam
> >
> > Best regards,
> >
> > Tim
Adam Ford Nov. 21, 2021, 1:07 p.m. UTC | #4
On Tue, Nov 16, 2021 at 12:27 PM Tim Harvey <tharvey@gateworks.com> wrote:
>
> On Tue, Nov 16, 2021 at 10:04 AM Adam Ford <aford173@gmail.com> wrote:
> >
> > On Tue, Nov 16, 2021 at 11:57 AM Tim Harvey <tharvey@gateworks.com> wrote:
> > >
> > > On Thu, Nov 4, 2021 at 9:18 AM Adam Ford <aford173@gmail.com> wrote:
> > > >
> > > > The i.MX8M Nano is similar to the i.MX8M Mini in some ways, but very
> > > > different in others.  With the blk-ctrl driver for Mini in place,
> > > > this series expands the blk-ctrl driver to support the Nano which
> > > > opens the door for additional functions in the future.  As part of
> > > > this series, it also addresses some issues in the GPCv2 driver and
> > > > finally adds support for enabling USB and GPU.
> > > >
> > > > V3:  Fixes an the yaml example
> > > > V2:  Fixes the clock count in the blk-ctrl
> > > >
> > > > Adam Ford (9):
> > > >   soc: imx: gpcv2: keep i.MX8MN gpumix bus clock enabled
> > > >   soc: imx: gpcv2: Add dispmix and mipi domains to imx8mn
> > > >   dt-bindings: power: imx8mn: add defines for DISP blk-ctrl domains
> > > >   dt-bindings: soc: add binding for i.MX8MN DISP blk-ctrl
> > > >   soc: imx: imx8m-blk-ctrl: add i.MX8MN DISP blk-ctrl
> > > >   arm64: dts: imx8mn: add GPC node
> > > >   arm64: dts: imx8mn: put USB controller into power-domains
> > > >   arm64: dts: imx8mn: add DISP blk-ctrl
> > > >   arm64: dts: imx8mn: Enable GPU
> > > >
> > > >  .../soc/imx/fsl,imx8mn-disp-blk-ctrl.yaml     |  97 +++++++++++++++++
> > > >  arch/arm64/boot/dts/freescale/imx8mn.dtsi     | 103 ++++++++++++++++++
> > > >  drivers/soc/imx/gpcv2.c                       |  26 +++++
> > > >  drivers/soc/imx/imx8m-blk-ctrl.c              |  75 ++++++++++++-
> > > >  include/dt-bindings/power/imx8mn-power.h      |   5 +
> > > >  5 files changed, 305 insertions(+), 1 deletion(-)
> > > >  create mode 100644 Documentation/devicetree/bindings/soc/imx/fsl,imx8mn-disp-blk-ctrl.yaml
> > > >
> > >
> > > Adam,
> > >
> > > Thanks for the patches. I'm not sure how best to test this but on an
> > > imx8mm-venice-gw7902 which has USB, but no display.
> > >
> > > I find that if DRM_ETNAVIV is enabled I hang at 'etnaviv etnaviv:
> > > bound 38000000.gpu (ops 0xffff800010964748)'.
> >
> > Thanks for testing this.
> >
> > Does your board send power to the GPU?  I recall someone somewhere
> > didn't power theirGPU, but I can't remember who and/or what board was
> > being discussed.
>
> Yes, the imx8mm-venice-gw7902 does have the GPU powered (the
> imx8mm-venice-gw7901 was the one that did not).

I cannot replicate your issue.  I applied the patch series to
5.16-rc1, and it's still working for me.

root@beacon-imx8mn-kit:~# dmesg |grep viv
[   11.323660] etnaviv etnaviv: bound 38000000.gpu (ops gpu_ops [etnaviv])
[   11.480543] etnaviv-gpu 38000000.gpu: model: GC7000, revision: 6203
[   11.747576] [drm] Initialized etnaviv 1.3.0 20151214 for etnaviv on minor 0

After booting, I connected a USB drive and it enumerated.

[  152.328228] ci_hdrc ci_hdrc.0: new USB bus registered, assigned bus number 1
[  152.351885] ci_hdrc ci_hdrc.0: USB 2.0 started, EHCI 1.00
[  152.363859] hub 1-0:1.0: USB hub found
[  152.371560] hub 1-0:1.0: 1 port detected
[  153.075902] usb 1-1: new high-speed USB device number 2 using ci_hdrc
[  153.244124] usb-storage 1-1:1.0: USB Mass Storage device detected
[  153.263651] scsi host0: usb-storage 1-1:1.0
[  154.277997] scsi 0:0:0:0: Direct-Access     SanDisk  Cruzer
  1.00 PQ: 0 ANSI: 6
[  154.292357] sd 0:0:0:0: [sda] 247529472 512-byte logical blocks:
(127 GB/118 GiB)
[  154.309246] sd 0:0:0:0: [sda] Write Protect is off
[  154.319237] sd 0:0:0:0: [sda] Write cache: disabled, read cache:
enabled, doesn't support DPO or FUA
[  154.359113]  sda: sda1
[  154.366975] sd 0:0:0:0: [sda] Attached SCSI removable disk

root@beacon-imx8mn-kit:~# lsusb
Bus 001 Device 002: ID 0781:5530 SanDisk Corp. Cruzer
Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub

I am not sure where to go from here.

>
> Tim
>
> >
> > I'll run some more tests on the latest 5.16-rc1 to see if I can
> > replicate your issue.
> >
> > adam
> > >
> > > If DRM_ETNAVIV is not enabled:
> > > - boots fine
> > > - usb works
> > > - soft reboot works (does not hang)
> >
> > At least we have some progress.  :-)
> >
> > adam
> > >
> > > Best regards,
> > >
> > > Tim
Fabio Estevam Nov. 21, 2021, 2:11 p.m. UTC | #5
Hi Adam,

On Sun, Nov 21, 2021 at 10:07 AM Adam Ford <aford173@gmail.com> wrote:

> I cannot replicate your issue.  I applied the patch series to
> 5.16-rc1, and it's still working for me.

Could the different behavior be caused by different TF-A versions that
you and Tim used?

Which ATF version do you use? Is it TF-A v2.5?

Thanks
Adam Ford Nov. 21, 2021, 2:17 p.m. UTC | #6
On Sun, Nov 21, 2021 at 8:12 AM Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Adam,
>
> On Sun, Nov 21, 2021 at 10:07 AM Adam Ford <aford173@gmail.com> wrote:
>
> > I cannot replicate your issue.  I applied the patch series to
> > 5.16-rc1, and it's still working for me.
>
> Could the different behavior be caused by different TF-A versions that
> you and Tim used?
>
> Which ATF version do you use? Is it TF-A v2.5?

I am using https://source.codeaurora.org/external/imx/imx-atf/log/?h=lf_v2.4

Since the driver sending SMCC commands to ATF isn't doing that, I
assume it's safe to use the linux power-domain drivers with the ATF
from NXP's kernel.

If you can point me to the repo you think I should be using, I'll give it a try.

thanks,

adam

>
> Thanks
Fabio Estevam Nov. 21, 2021, 2:21 p.m. UTC | #7
Hi Adam,

On Sun, Nov 21, 2021 at 11:17 AM Adam Ford <aford173@gmail.com> wrote:

> I am using https://source.codeaurora.org/external/imx/imx-atf/log/?h=lf_v2.4
>
> Since the driver sending SMCC commands to ATF isn't doing that, I
> assume it's safe to use the linux power-domain drivers with the ATF
> from NXP's kernel.
>
> If you can point me to the repo you think I should be using, I'll give it a try.

Do you know if the mainline TF-A repo v2.5 works too?
https://github.com/ARM-software/arm-trusted-firmware/tree/v2.5

Thanks
Adam Ford Nov. 21, 2021, 2:34 p.m. UTC | #8
On Sun, Nov 21, 2021 at 8:21 AM Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Adam,
>
> On Sun, Nov 21, 2021 at 11:17 AM Adam Ford <aford173@gmail.com> wrote:
>
> > I am using https://source.codeaurora.org/external/imx/imx-atf/log/?h=lf_v2.4
> >
> > Since the driver sending SMCC commands to ATF isn't doing that, I
> > assume it's safe to use the linux power-domain drivers with the ATF
> > from NXP's kernel.
> >
> > If you can point me to the repo you think I should be using, I'll give it a try.
>
> Do you know if the mainline TF-A repo v2.5 works too?
> https://github.com/ARM-software/arm-trusted-firmware/tree/v2.5

That's good to know.

I just built it into U-Boot:

NOTICE:  BL31: v2.5(release):v2.5
NOTICE:  BL31: Built : 08:24:13, Nov 21 2021

The Etnaviv driver is still loading without hanging

root@beacon-imx8mn-kit:~# dmesg |grep -i etna
[   12.393936] etnaviv etnaviv: bound 38000000.gpu (ops gpu_ops [etnaviv])
[   12.400676] etnaviv-gpu 38000000.gpu: model: GC7000, revision: 6203
[   12.641297] [drm] Initialized etnaviv 1.3.0 20151214 for etnaviv on minor 0



>
> Thanks
Adam Ford Nov. 21, 2021, 3:25 p.m. UTC | #9
On Sun, Nov 21, 2021 at 8:34 AM Adam Ford <aford173@gmail.com> wrote:
>
> On Sun, Nov 21, 2021 at 8:21 AM Fabio Estevam <festevam@gmail.com> wrote:
> >
> > Hi Adam,
> >
> > On Sun, Nov 21, 2021 at 11:17 AM Adam Ford <aford173@gmail.com> wrote:
> >
> > > I am using https://source.codeaurora.org/external/imx/imx-atf/log/?h=lf_v2.4
> > >
> > > Since the driver sending SMCC commands to ATF isn't doing that, I
> > > assume it's safe to use the linux power-domain drivers with the ATF
> > > from NXP's kernel.
> > >
> > > If you can point me to the repo you think I should be using, I'll give it a try.
> >
> > Do you know if the mainline TF-A repo v2.5 works too?
> > https://github.com/ARM-software/arm-trusted-firmware/tree/v2.5
>
> That's good to know.
>
> I just built it into U-Boot:
>
> NOTICE:  BL31: v2.5(release):v2.5
> NOTICE:  BL31: Built : 08:24:13, Nov 21 2021
>
> The Etnaviv driver is still loading without hanging
>
> root@beacon-imx8mn-kit:~# dmesg |grep -i etna
> [   12.393936] etnaviv etnaviv: bound 38000000.gpu (ops gpu_ops [etnaviv])
> [   12.400676] etnaviv-gpu 38000000.gpu: model: GC7000, revision: 6203
> [   12.641297] [drm] Initialized etnaviv 1.3.0 20151214 for etnaviv on minor 0
>
>

Tim,

Which version of Nano do you have?  Not all Nano SoC's have a GPU from
looking at the datasheet [1] .  I am using MIMX8MN2CVTIZAA (Nano Solo)

[1] - https://www.nxp.com/docs/en/data-sheet/IMX8MNIEC.pdf

adam

>
> >
> > Thanks
Tim Harvey Nov. 22, 2021, 5:59 p.m. UTC | #10
On Sun, Nov 21, 2021 at 7:25 AM Adam Ford <aford173@gmail.com> wrote:
>
> On Sun, Nov 21, 2021 at 8:34 AM Adam Ford <aford173@gmail.com> wrote:
> >
> > On Sun, Nov 21, 2021 at 8:21 AM Fabio Estevam <festevam@gmail.com> wrote:
> > >
> > > Hi Adam,
> > >
> > > On Sun, Nov 21, 2021 at 11:17 AM Adam Ford <aford173@gmail.com> wrote:
> > >
> > > > I am using https://source.codeaurora.org/external/imx/imx-atf/log/?h=lf_v2.4
> > > >
> > > > Since the driver sending SMCC commands to ATF isn't doing that, I
> > > > assume it's safe to use the linux power-domain drivers with the ATF
> > > > from NXP's kernel.
> > > >
> > > > If you can point me to the repo you think I should be using, I'll give it a try.
> > >
> > > Do you know if the mainline TF-A repo v2.5 works too?
> > > https://github.com/ARM-software/arm-trusted-firmware/tree/v2.5
> >
> > That's good to know.
> >
> > I just built it into U-Boot:
> >
> > NOTICE:  BL31: v2.5(release):v2.5
> > NOTICE:  BL31: Built : 08:24:13, Nov 21 2021
> >
> > The Etnaviv driver is still loading without hanging
> >
> > root@beacon-imx8mn-kit:~# dmesg |grep -i etna
> > [   12.393936] etnaviv etnaviv: bound 38000000.gpu (ops gpu_ops [etnaviv])
> > [   12.400676] etnaviv-gpu 38000000.gpu: model: GC7000, revision: 6203
> > [   12.641297] [drm] Initialized etnaviv 1.3.0 20151214 for etnaviv on minor 0
> >
> >
>
> Tim,
>
> Which version of Nano do you have?  Not all Nano SoC's have a GPU from
> looking at the datasheet [1] .  I am using MIMX8MN2CVTIZAA (Nano Solo)
>
> [1] - https://www.nxp.com/docs/en/data-sheet/IMX8MNIEC.pdf
>

Adam,

The board I have here has MIMX8MN5CVTIZAA so i.MX 8M Nano QuadLite
with 'No GPU' as you expected.

So I have to add the following to keep my board from hanging after your series:
diff --git a/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
b/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
index 236f425e1570..0d256a607b7c 100644
--- a/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
@@ -251,6 +251,10 @@
        };
 };

+&gpu {
+       status = "disabled";
+};
+
 &i2c1 {
        clock-frequency = <100000>;
        pinctrl-names = "default";

This situation is similar to the one I encountered with the
imx8mm-venice-gw7901 where adding the GPC node caused my board (which
did not power the GPU) to hang until I added disables to the
device-tree with commit 7973009235e2 ("arm64: dts:
imx8mm-venice-gw7901.dts: disable pgc_gpumix"). It feels painful to
have to add patches to keep things from hanging after additional
functionality is added to dt but perhaps that is more common than I
think esp for SoC's like IMX8M which have a lot of lingering support
still coming in.

I don't mind at all submitting the above patch to fix my board after
your series is accepted as I think that having an IMX8MN with 'no gpu'
is perhaps less likely than having one with a GPU and thus we probably
shouldn't mark the node as disabled and force everyone that has a GPU
to go and enable it.

I wonder however if we should think about adding something to etnaviv
to check the capability so that the same dt could be used with both
CPU variants?

At any rate for now let's keep the ball rolling!

For the series:
Reviewed-by: Tim Harvey <tharvey@gateworks.com>
Tested-by: Tim Harvey <tharvey@gateworks.com> (tested on imx8mm-venice-gw7902)

Best regards,

Tim
Lucas Stach Nov. 22, 2021, 6:20 p.m. UTC | #11
Am Montag, dem 22.11.2021 um 09:59 -0800 schrieb Tim Harvey:
> On Sun, Nov 21, 2021 at 7:25 AM Adam Ford <aford173@gmail.com> wrote:
> > 
> > On Sun, Nov 21, 2021 at 8:34 AM Adam Ford <aford173@gmail.com> wrote:
> > > 
> > > On Sun, Nov 21, 2021 at 8:21 AM Fabio Estevam <festevam@gmail.com> wrote:
> > > > 
> > > > Hi Adam,
> > > > 
> > > > On Sun, Nov 21, 2021 at 11:17 AM Adam Ford <aford173@gmail.com> wrote:
> > > > 
> > > > > I am using https://source.codeaurora.org/external/imx/imx-atf/log/?h=lf_v2.4
> > > > > 
> > > > > Since the driver sending SMCC commands to ATF isn't doing that, I
> > > > > assume it's safe to use the linux power-domain drivers with the ATF
> > > > > from NXP's kernel.
> > > > > 
> > > > > If you can point me to the repo you think I should be using, I'll give it a try.
> > > > 
> > > > Do you know if the mainline TF-A repo v2.5 works too?
> > > > https://github.com/ARM-software/arm-trusted-firmware/tree/v2.5
> > > 
> > > That's good to know.
> > > 
> > > I just built it into U-Boot:
> > > 
> > > NOTICE:  BL31: v2.5(release):v2.5
> > > NOTICE:  BL31: Built : 08:24:13, Nov 21 2021
> > > 
> > > The Etnaviv driver is still loading without hanging
> > > 
> > > root@beacon-imx8mn-kit:~# dmesg |grep -i etna
> > > [   12.393936] etnaviv etnaviv: bound 38000000.gpu (ops gpu_ops [etnaviv])
> > > [   12.400676] etnaviv-gpu 38000000.gpu: model: GC7000, revision: 6203
> > > [   12.641297] [drm] Initialized etnaviv 1.3.0 20151214 for etnaviv on minor 0
> > > 
> > > 
> > 
> > Tim,
> > 
> > Which version of Nano do you have?  Not all Nano SoC's have a GPU from
> > looking at the datasheet [1] .  I am using MIMX8MN2CVTIZAA (Nano Solo)
> > 
> > [1] - https://www.nxp.com/docs/en/data-sheet/IMX8MNIEC.pdf
> > 
> 
> Adam,
> 
> The board I have here has MIMX8MN5CVTIZAA so i.MX 8M Nano QuadLite
> with 'No GPU' as you expected.
> 
> So I have to add the following to keep my board from hanging after your series:
> diff --git a/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
> b/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
> index 236f425e1570..0d256a607b7c 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
> +++ b/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
> @@ -251,6 +251,10 @@
>         };
>  };
> 
> +&gpu {
> +       status = "disabled";
> +};
> +
>  &i2c1 {
>         clock-frequency = <100000>;
>         pinctrl-names = "default";
> 
> This situation is similar to the one I encountered with the
> imx8mm-venice-gw7901 where adding the GPC node caused my board (which
> did not power the GPU) to hang until I added disables to the
> device-tree with commit 7973009235e2 ("arm64: dts:
> imx8mm-venice-gw7901.dts: disable pgc_gpumix"). It feels painful to
> have to add patches to keep things from hanging after additional
> functionality is added to dt but perhaps that is more common than I
> think esp for SoC's like IMX8M which have a lot of lingering support
> still coming in.
> 
Yea, it's unfortunate that those patches break your board, but I guess
we need to accept this, while there is still a lot of feature work
going on.

> I don't mind at all submitting the above patch to fix my board after
> your series is accepted as I think that having an IMX8MN with 'no gpu'
> is perhaps less likely than having one with a GPU and thus we probably
> shouldn't mark the node as disabled and force everyone that has a GPU
> to go and enable it.
> 
> I wonder however if we should think about adding something to etnaviv
> to check the capability so that the same dt could be used with both
> CPU variants?

etnaviv or really the kernel at all is not the place to handle this.
The DT is supposed to describe the hardware and the kernel should be
able to trust this description.

If there is some way to read the chip capabilities and avoid having too
much DT variants in the kernel, the right place to handle this is the
software running before the kernel is started, i.e. your bootloader.
Barebox for example reads the SCU fuses on i.MX6 and removes the DT
nodes for the fused off CPU cores on i.MX6S and i.MX6D.

Regards,
Lucas
Tim Harvey Nov. 22, 2021, 9:52 p.m. UTC | #12
On Mon, Nov 22, 2021 at 10:20 AM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Am Montag, dem 22.11.2021 um 09:59 -0800 schrieb Tim Harvey:
> > On Sun, Nov 21, 2021 at 7:25 AM Adam Ford <aford173@gmail.com> wrote:
> > >
> > > On Sun, Nov 21, 2021 at 8:34 AM Adam Ford <aford173@gmail.com> wrote:
> > > >
> > > > On Sun, Nov 21, 2021 at 8:21 AM Fabio Estevam <festevam@gmail.com> wrote:
> > > > >
> > > > > Hi Adam,
> > > > >
> > > > > On Sun, Nov 21, 2021 at 11:17 AM Adam Ford <aford173@gmail.com> wrote:
> > > > >
> > > > > > I am using https://source.codeaurora.org/external/imx/imx-atf/log/?h=lf_v2.4
> > > > > >
> > > > > > Since the driver sending SMCC commands to ATF isn't doing that, I
> > > > > > assume it's safe to use the linux power-domain drivers with the ATF
> > > > > > from NXP's kernel.
> > > > > >
> > > > > > If you can point me to the repo you think I should be using, I'll give it a try.
> > > > >
> > > > > Do you know if the mainline TF-A repo v2.5 works too?
> > > > > https://github.com/ARM-software/arm-trusted-firmware/tree/v2.5
> > > >
> > > > That's good to know.
> > > >
> > > > I just built it into U-Boot:
> > > >
> > > > NOTICE:  BL31: v2.5(release):v2.5
> > > > NOTICE:  BL31: Built : 08:24:13, Nov 21 2021
> > > >
> > > > The Etnaviv driver is still loading without hanging
> > > >
> > > > root@beacon-imx8mn-kit:~# dmesg |grep -i etna
> > > > [   12.393936] etnaviv etnaviv: bound 38000000.gpu (ops gpu_ops [etnaviv])
> > > > [   12.400676] etnaviv-gpu 38000000.gpu: model: GC7000, revision: 6203
> > > > [   12.641297] [drm] Initialized etnaviv 1.3.0 20151214 for etnaviv on minor 0
> > > >
> > > >
> > >
> > > Tim,
> > >
> > > Which version of Nano do you have?  Not all Nano SoC's have a GPU from
> > > looking at the datasheet [1] .  I am using MIMX8MN2CVTIZAA (Nano Solo)
> > >
> > > [1] - https://www.nxp.com/docs/en/data-sheet/IMX8MNIEC.pdf
> > >
> >
> > Adam,
> >
> > The board I have here has MIMX8MN5CVTIZAA so i.MX 8M Nano QuadLite
> > with 'No GPU' as you expected.
> >
> > So I have to add the following to keep my board from hanging after your series:
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
> > b/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
> > index 236f425e1570..0d256a607b7c 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
> > +++ b/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
> > @@ -251,6 +251,10 @@
> >         };
> >  };
> >
> > +&gpu {
> > +       status = "disabled";
> > +};
> > +
> >  &i2c1 {
> >         clock-frequency = <100000>;
> >         pinctrl-names = "default";
> >
> > This situation is similar to the one I encountered with the
> > imx8mm-venice-gw7901 where adding the GPC node caused my board (which
> > did not power the GPU) to hang until I added disables to the
> > device-tree with commit 7973009235e2 ("arm64: dts:
> > imx8mm-venice-gw7901.dts: disable pgc_gpumix"). It feels painful to
> > have to add patches to keep things from hanging after additional
> > functionality is added to dt but perhaps that is more common than I
> > think esp for SoC's like IMX8M which have a lot of lingering support
> > still coming in.
> >
> Yea, it's unfortunate that those patches break your board, but I guess
> we need to accept this, while there is still a lot of feature work
> going on.
>
> > I don't mind at all submitting the above patch to fix my board after
> > your series is accepted as I think that having an IMX8MN with 'no gpu'
> > is perhaps less likely than having one with a GPU and thus we probably
> > shouldn't mark the node as disabled and force everyone that has a GPU
> > to go and enable it.
> >
> > I wonder however if we should think about adding something to etnaviv
> > to check the capability so that the same dt could be used with both
> > CPU variants?
>
> etnaviv or really the kernel at all is not the place to handle this.
> The DT is supposed to describe the hardware and the kernel should be
> able to trust this description.
>
> If there is some way to read the chip capabilities and avoid having too
> much DT variants in the kernel, the right place to handle this is the
> software running before the kernel is started, i.e. your bootloader.
> Barebox for example reads the SCU fuses on i.MX6 and removes the DT
> nodes for the fused off CPU cores on i.MX6S and i.MX6D.
>

Lucas,

I agree - the boot firmware is an appropriate place for this. I
believe the correct course of action in the case of the IMX8M Nano
would be to do the following for no GPU:
- disable disp_blk_ctrl node
- disable gpu node
- disable pgc_gpumix node

What would you propose to do for detection of this in boot firmware?
The DIGPROG register is currently used in U-Boot to determine IMX8M
variants including Nano
Qad/Dual/Solo/QaudLite/DualLite/SoloLite/UltraLite Quad/UltraLite
Dual/UltraLite Solo. It would appear all the 'Lite' and 'UltraLite'
variants have no GPU.

Best regards,

Tim
Adam Ford Nov. 23, 2021, 2:08 p.m. UTC | #13
On Mon, Nov 22, 2021 at 3:52 PM Tim Harvey <tharvey@gateworks.com> wrote:
>
> On Mon, Nov 22, 2021 at 10:20 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> >
> > Am Montag, dem 22.11.2021 um 09:59 -0800 schrieb Tim Harvey:
> > > On Sun, Nov 21, 2021 at 7:25 AM Adam Ford <aford173@gmail.com> wrote:
> > > >
> > > > On Sun, Nov 21, 2021 at 8:34 AM Adam Ford <aford173@gmail.com> wrote:
> > > > >
> > > > > On Sun, Nov 21, 2021 at 8:21 AM Fabio Estevam <festevam@gmail.com> wrote:
> > > > > >
> > > > > > Hi Adam,
> > > > > >
> > > > > > On Sun, Nov 21, 2021 at 11:17 AM Adam Ford <aford173@gmail.com> wrote:
> > > > > >
> > > > > > > I am using https://source.codeaurora.org/external/imx/imx-atf/log/?h=lf_v2.4
> > > > > > >
> > > > > > > Since the driver sending SMCC commands to ATF isn't doing that, I
> > > > > > > assume it's safe to use the linux power-domain drivers with the ATF
> > > > > > > from NXP's kernel.
> > > > > > >
> > > > > > > If you can point me to the repo you think I should be using, I'll give it a try.
> > > > > >
> > > > > > Do you know if the mainline TF-A repo v2.5 works too?
> > > > > > https://github.com/ARM-software/arm-trusted-firmware/tree/v2.5
> > > > >
> > > > > That's good to know.
> > > > >
> > > > > I just built it into U-Boot:
> > > > >
> > > > > NOTICE:  BL31: v2.5(release):v2.5
> > > > > NOTICE:  BL31: Built : 08:24:13, Nov 21 2021
> > > > >
> > > > > The Etnaviv driver is still loading without hanging
> > > > >
> > > > > root@beacon-imx8mn-kit:~# dmesg |grep -i etna
> > > > > [   12.393936] etnaviv etnaviv: bound 38000000.gpu (ops gpu_ops [etnaviv])
> > > > > [   12.400676] etnaviv-gpu 38000000.gpu: model: GC7000, revision: 6203
> > > > > [   12.641297] [drm] Initialized etnaviv 1.3.0 20151214 for etnaviv on minor 0
> > > > >
> > > > >
> > > >
> > > > Tim,
> > > >
> > > > Which version of Nano do you have?  Not all Nano SoC's have a GPU from
> > > > looking at the datasheet [1] .  I am using MIMX8MN2CVTIZAA (Nano Solo)
> > > >
> > > > [1] - https://www.nxp.com/docs/en/data-sheet/IMX8MNIEC.pdf
> > > >
> > >
> > > Adam,
> > >
> > > The board I have here has MIMX8MN5CVTIZAA so i.MX 8M Nano QuadLite
> > > with 'No GPU' as you expected.
> > >
> > > So I have to add the following to keep my board from hanging after your series:
> > > diff --git a/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
> > > b/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
> > > index 236f425e1570..0d256a607b7c 100644
> > > --- a/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
> > > +++ b/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
> > > @@ -251,6 +251,10 @@
> > >         };
> > >  };
> > >
> > > +&gpu {
> > > +       status = "disabled";
> > > +};
> > > +
> > >  &i2c1 {
> > >         clock-frequency = <100000>;
> > >         pinctrl-names = "default";
> > >
> > > This situation is similar to the one I encountered with the
> > > imx8mm-venice-gw7901 where adding the GPC node caused my board (which
> > > did not power the GPU) to hang until I added disables to the
> > > device-tree with commit 7973009235e2 ("arm64: dts:
> > > imx8mm-venice-gw7901.dts: disable pgc_gpumix"). It feels painful to
> > > have to add patches to keep things from hanging after additional
> > > functionality is added to dt but perhaps that is more common than I
> > > think esp for SoC's like IMX8M which have a lot of lingering support
> > > still coming in.
> > >
> > Yea, it's unfortunate that those patches break your board, but I guess
> > we need to accept this, while there is still a lot of feature work
> > going on.

There are a significant number of peripherals which are defined and
marked as 'disabled' by default, so I don't think it's unreasonable to
do that here.
I'd like to propose we keep the default disabled and people who
need/want the GPU enabled can turn it on.  Why waste the power if it's
not needed?

> >
> > > I don't mind at all submitting the above patch to fix my board after
> > > your series is accepted as I think that having an IMX8MN with 'no gpu'
> > > is perhaps less likely than having one with a GPU and thus we probably
> > > shouldn't mark the node as disabled and force everyone that has a GPU
> > > to go and enable it.
> > >
> > > I wonder however if we should think about adding something to etnaviv
> > > to check the capability so that the same dt could be used with both
> > > CPU variants?
> >
> > etnaviv or really the kernel at all is not the place to handle this.
> > The DT is supposed to describe the hardware and the kernel should be
> > able to trust this description.
> >
> > If there is some way to read the chip capabilities and avoid having too
> > much DT variants in the kernel, the right place to handle this is the
> > software running before the kernel is started, i.e. your bootloader.
> > Barebox for example reads the SCU fuses on i.MX6 and removes the DT
> > nodes for the fused off CPU cores on i.MX6S and i.MX6D.
> >
>
> Lucas,
>
> I agree - the boot firmware is an appropriate place for this. I
> believe the correct course of action in the case of the IMX8M Nano
> would be to do the following for no GPU:
> - disable disp_blk_ctrl node

I don't think it's necessary to remove the disp_blk_ctrl node or
change it.  The GPU doesn't directly interact with it.  LCD, CSI, and
DSI do, but I don't think they are removed.  The gpu only interacts
with the pgc_gpumix and neither the gpu nor gpumix currently interact
with the disp_blk_ctrl.

adam



> - disable gpu node
> - disable pgc_gpumix node
>
> What would you propose to do for detection of this in boot firmware?
> The DIGPROG register is currently used in U-Boot to determine IMX8M
> variants including Nano
> Qad/Dual/Solo/QaudLite/DualLite/SoloLite/UltraLite Quad/UltraLite
> Dual/UltraLite Solo. It would appear all the 'Lite' and 'UltraLite'
> variants have no GPU.
>
> Best regards,
>
> Tim
Adam Ford Nov. 23, 2021, 2:16 p.m. UTC | #14
On Tue, Nov 23, 2021 at 8:08 AM Adam Ford <aford173@gmail.com> wrote:
>
> On Mon, Nov 22, 2021 at 3:52 PM Tim Harvey <tharvey@gateworks.com> wrote:
> >
> > On Mon, Nov 22, 2021 at 10:20 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > >
> > > Am Montag, dem 22.11.2021 um 09:59 -0800 schrieb Tim Harvey:
> > > > On Sun, Nov 21, 2021 at 7:25 AM Adam Ford <aford173@gmail.com> wrote:
> > > > >
> > > > > On Sun, Nov 21, 2021 at 8:34 AM Adam Ford <aford173@gmail.com> wrote:
> > > > > >
> > > > > > On Sun, Nov 21, 2021 at 8:21 AM Fabio Estevam <festevam@gmail.com> wrote:
> > > > > > >
> > > > > > > Hi Adam,
> > > > > > >
> > > > > > > On Sun, Nov 21, 2021 at 11:17 AM Adam Ford <aford173@gmail.com> wrote:
> > > > > > >
> > > > > > > > I am using https://source.codeaurora.org/external/imx/imx-atf/log/?h=lf_v2.4
> > > > > > > >
> > > > > > > > Since the driver sending SMCC commands to ATF isn't doing that, I
> > > > > > > > assume it's safe to use the linux power-domain drivers with the ATF
> > > > > > > > from NXP's kernel.
> > > > > > > >
> > > > > > > > If you can point me to the repo you think I should be using, I'll give it a try.
> > > > > > >
> > > > > > > Do you know if the mainline TF-A repo v2.5 works too?
> > > > > > > https://github.com/ARM-software/arm-trusted-firmware/tree/v2.5
> > > > > >
> > > > > > That's good to know.
> > > > > >
> > > > > > I just built it into U-Boot:
> > > > > >
> > > > > > NOTICE:  BL31: v2.5(release):v2.5
> > > > > > NOTICE:  BL31: Built : 08:24:13, Nov 21 2021
> > > > > >
> > > > > > The Etnaviv driver is still loading without hanging
> > > > > >
> > > > > > root@beacon-imx8mn-kit:~# dmesg |grep -i etna
> > > > > > [   12.393936] etnaviv etnaviv: bound 38000000.gpu (ops gpu_ops [etnaviv])
> > > > > > [   12.400676] etnaviv-gpu 38000000.gpu: model: GC7000, revision: 6203
> > > > > > [   12.641297] [drm] Initialized etnaviv 1.3.0 20151214 for etnaviv on minor 0
> > > > > >
> > > > > >
> > > > >
> > > > > Tim,
> > > > >
> > > > > Which version of Nano do you have?  Not all Nano SoC's have a GPU from
> > > > > looking at the datasheet [1] .  I am using MIMX8MN2CVTIZAA (Nano Solo)
> > > > >
> > > > > [1] - https://www.nxp.com/docs/en/data-sheet/IMX8MNIEC.pdf
> > > > >
> > > >
> > > > Adam,
> > > >
> > > > The board I have here has MIMX8MN5CVTIZAA so i.MX 8M Nano QuadLite
> > > > with 'No GPU' as you expected.
> > > >
> > > > So I have to add the following to keep my board from hanging after your series:
> > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
> > > > b/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
> > > > index 236f425e1570..0d256a607b7c 100644
> > > > --- a/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
> > > > +++ b/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
> > > > @@ -251,6 +251,10 @@
> > > >         };
> > > >  };
> > > >
> > > > +&gpu {
> > > > +       status = "disabled";
> > > > +};
> > > > +
> > > >  &i2c1 {
> > > >         clock-frequency = <100000>;
> > > >         pinctrl-names = "default";
> > > >
> > > > This situation is similar to the one I encountered with the
> > > > imx8mm-venice-gw7901 where adding the GPC node caused my board (which
> > > > did not power the GPU) to hang until I added disables to the
> > > > device-tree with commit 7973009235e2 ("arm64: dts:
> > > > imx8mm-venice-gw7901.dts: disable pgc_gpumix"). It feels painful to
> > > > have to add patches to keep things from hanging after additional
> > > > functionality is added to dt but perhaps that is more common than I
> > > > think esp for SoC's like IMX8M which have a lot of lingering support
> > > > still coming in.
> > > >
> > > Yea, it's unfortunate that those patches break your board, but I guess
> > > we need to accept this, while there is still a lot of feature work
> > > going on.
>
> There are a significant number of peripherals which are defined and
> marked as 'disabled' by default, so I don't think it's unreasonable to
> do that here.
> I'd like to propose we keep the default disabled and people who
> need/want the GPU enabled can turn it on.  Why waste the power if it's
> not needed?
>
> > >
> > > > I don't mind at all submitting the above patch to fix my board after
> > > > your series is accepted as I think that having an IMX8MN with 'no gpu'
> > > > is perhaps less likely than having one with a GPU and thus we probably
> > > > shouldn't mark the node as disabled and force everyone that has a GPU
> > > > to go and enable it.
> > > >
> > > > I wonder however if we should think about adding something to etnaviv
> > > > to check the capability so that the same dt could be used with both
> > > > CPU variants?
> > >
> > > etnaviv or really the kernel at all is not the place to handle this.
> > > The DT is supposed to describe the hardware and the kernel should be
> > > able to trust this description.
> > >
> > > If there is some way to read the chip capabilities and avoid having too
> > > much DT variants in the kernel, the right place to handle this is the
> > > software running before the kernel is started, i.e. your bootloader.
> > > Barebox for example reads the SCU fuses on i.MX6 and removes the DT
> > > nodes for the fused off CPU cores on i.MX6S and i.MX6D.
> > >
> >
> > Lucas,
> >
> > I agree - the boot firmware is an appropriate place for this. I
> > believe the correct course of action in the case of the IMX8M Nano
> > would be to do the following for no GPU:
> > - disable disp_blk_ctrl node
>
> I don't think it's necessary to remove the disp_blk_ctrl node or
> change it.  The GPU doesn't directly interact with it.  LCD, CSI, and
> DSI do, but I don't think they are removed.  The gpu only interacts
> with the pgc_gpumix and neither the gpu nor gpumix currently interact
> with the disp_blk_ctrl.
>

I went back and reviewed the datasheet, and the UltraLite models
remove the DSI, but I think it's still safe to leave the disp_blk_ctrl
untouched, because FWICT it's only activated from the corresponding
power-domain is requested, and without a MIPI_DSI the DSI power-domain
won't be requested.  The CSI appears to remain intact on the Lite and
UltraLite versions, so we still need some form of the disp_blk_ctrl to
manage it.

> adam
>
>
>
> > - disable gpu node
> > - disable pgc_gpumix node
> >
> > What would you propose to do for detection of this in boot firmware?
> > The DIGPROG register is currently used in U-Boot to determine IMX8M
> > variants including Nano
> > Qad/Dual/Solo/QaudLite/DualLite/SoloLite/UltraLite Quad/UltraLite
> > Dual/UltraLite Solo. It would appear all the 'Lite' and 'UltraLite'
> > variants have no GPU.
> >
> > Best regards,
> >
> > Tim
Lucas Stach Nov. 23, 2021, 2:24 p.m. UTC | #15
Am Dienstag, dem 23.11.2021 um 08:08 -0600 schrieb Adam Ford:
> On Mon, Nov 22, 2021 at 3:52 PM Tim Harvey <tharvey@gateworks.com> wrote:
> > 
> > On Mon, Nov 22, 2021 at 10:20 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > > 
> > > Am Montag, dem 22.11.2021 um 09:59 -0800 schrieb Tim Harvey:
> > > > On Sun, Nov 21, 2021 at 7:25 AM Adam Ford <aford173@gmail.com> wrote:
> > > > > 
> > > > > On Sun, Nov 21, 2021 at 8:34 AM Adam Ford <aford173@gmail.com> wrote:
> > > > > > 
> > > > > > On Sun, Nov 21, 2021 at 8:21 AM Fabio Estevam <festevam@gmail.com> wrote:
> > > > > > > 
> > > > > > > Hi Adam,
> > > > > > > 
> > > > > > > On Sun, Nov 21, 2021 at 11:17 AM Adam Ford <aford173@gmail.com> wrote:
> > > > > > > 
> > > > > > > > I am using https://source.codeaurora.org/external/imx/imx-atf/log/?h=lf_v2.4
> > > > > > > > 
> > > > > > > > Since the driver sending SMCC commands to ATF isn't doing that, I
> > > > > > > > assume it's safe to use the linux power-domain drivers with the ATF
> > > > > > > > from NXP's kernel.
> > > > > > > > 
> > > > > > > > If you can point me to the repo you think I should be using, I'll give it a try.
> > > > > > > 
> > > > > > > Do you know if the mainline TF-A repo v2.5 works too?
> > > > > > > https://github.com/ARM-software/arm-trusted-firmware/tree/v2.5
> > > > > > 
> > > > > > That's good to know.
> > > > > > 
> > > > > > I just built it into U-Boot:
> > > > > > 
> > > > > > NOTICE:  BL31: v2.5(release):v2.5
> > > > > > NOTICE:  BL31: Built : 08:24:13, Nov 21 2021
> > > > > > 
> > > > > > The Etnaviv driver is still loading without hanging
> > > > > > 
> > > > > > root@beacon-imx8mn-kit:~# dmesg |grep -i etna
> > > > > > [   12.393936] etnaviv etnaviv: bound 38000000.gpu (ops gpu_ops [etnaviv])
> > > > > > [   12.400676] etnaviv-gpu 38000000.gpu: model: GC7000, revision: 6203
> > > > > > [   12.641297] [drm] Initialized etnaviv 1.3.0 20151214 for etnaviv on minor 0
> > > > > > 
> > > > > > 
> > > > > 
> > > > > Tim,
> > > > > 
> > > > > Which version of Nano do you have?  Not all Nano SoC's have a GPU from
> > > > > looking at the datasheet [1] .  I am using MIMX8MN2CVTIZAA (Nano Solo)
> > > > > 
> > > > > [1] - https://www.nxp.com/docs/en/data-sheet/IMX8MNIEC.pdf
> > > > > 
> > > > 
> > > > Adam,
> > > > 
> > > > The board I have here has MIMX8MN5CVTIZAA so i.MX 8M Nano QuadLite
> > > > with 'No GPU' as you expected.
> > > > 
> > > > So I have to add the following to keep my board from hanging after your series:
> > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
> > > > b/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
> > > > index 236f425e1570..0d256a607b7c 100644
> > > > --- a/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
> > > > +++ b/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
> > > > @@ -251,6 +251,10 @@
> > > >         };
> > > >  };
> > > > 
> > > > +&gpu {
> > > > +       status = "disabled";
> > > > +};
> > > > +
> > > >  &i2c1 {
> > > >         clock-frequency = <100000>;
> > > >         pinctrl-names = "default";
> > > > 
> > > > This situation is similar to the one I encountered with the
> > > > imx8mm-venice-gw7901 where adding the GPC node caused my board (which
> > > > did not power the GPU) to hang until I added disables to the
> > > > device-tree with commit 7973009235e2 ("arm64: dts:
> > > > imx8mm-venice-gw7901.dts: disable pgc_gpumix"). It feels painful to
> > > > have to add patches to keep things from hanging after additional
> > > > functionality is added to dt but perhaps that is more common than I
> > > > think esp for SoC's like IMX8M which have a lot of lingering support
> > > > still coming in.
> > > > 
> > > Yea, it's unfortunate that those patches break your board, but I guess
> > > we need to accept this, while there is still a lot of feature work
> > > going on.
> 
> There are a significant number of peripherals which are defined and
> marked as 'disabled' by default, so I don't think it's unreasonable to
> do that here.
> I'd like to propose we keep the default disabled and people who
> need/want the GPU enabled can turn it on.  Why waste the power if it's
> not needed?
> 
Sure, if a significant number of chips has the GPU disabled, we might
want to keep it disabled in the base dtsi. With those variants it's
always a tradeoff, for example there are SKUs of the i.MX6 that had the
VPU disabled, but very few of those were in the field, so the VPUs are
enabled in the SoC base dtsi and only users of those special SKUs would
need to disable them in the board DT.

The power argument isn't valid, as the kernel driver will suspend the
device when not needed, so there is no wasted power (aside from the
sort moment while the driver probes) with the GPU enabled.

The rule of thumb for when a device is default enabled in the SoC dsti
has always been (at least for i.MX) that the peripheral must not have a
board level dependency. While a i2c controller obviously needs a i2c
bus connected on the board to fulfill its purpose, a GPU can be used as
color space converter or something like that with no board level
interaction. Now the line is a bit blurred by having multiple power
rails into the SoC, so one could argue that the GPUs and VPUs now have
some board level dependency on the i.MX8M*.

Regards,
Lucas
Adam Ford Nov. 23, 2021, 2:30 p.m. UTC | #16
On Tue, Nov 23, 2021 at 8:24 AM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Am Dienstag, dem 23.11.2021 um 08:08 -0600 schrieb Adam Ford:
> > On Mon, Nov 22, 2021 at 3:52 PM Tim Harvey <tharvey@gateworks.com> wrote:
> > >
> > > On Mon, Nov 22, 2021 at 10:20 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > > >
> > > > Am Montag, dem 22.11.2021 um 09:59 -0800 schrieb Tim Harvey:
> > > > > On Sun, Nov 21, 2021 at 7:25 AM Adam Ford <aford173@gmail.com> wrote:
> > > > > >
> > > > > > On Sun, Nov 21, 2021 at 8:34 AM Adam Ford <aford173@gmail.com> wrote:
> > > > > > >
> > > > > > > On Sun, Nov 21, 2021 at 8:21 AM Fabio Estevam <festevam@gmail.com> wrote:
> > > > > > > >
> > > > > > > > Hi Adam,
> > > > > > > >
> > > > > > > > On Sun, Nov 21, 2021 at 11:17 AM Adam Ford <aford173@gmail.com> wrote:
> > > > > > > >
> > > > > > > > > I am using https://source.codeaurora.org/external/imx/imx-atf/log/?h=lf_v2.4
> > > > > > > > >
> > > > > > > > > Since the driver sending SMCC commands to ATF isn't doing that, I
> > > > > > > > > assume it's safe to use the linux power-domain drivers with the ATF
> > > > > > > > > from NXP's kernel.
> > > > > > > > >
> > > > > > > > > If you can point me to the repo you think I should be using, I'll give it a try.
> > > > > > > >
> > > > > > > > Do you know if the mainline TF-A repo v2.5 works too?
> > > > > > > > https://github.com/ARM-software/arm-trusted-firmware/tree/v2.5
> > > > > > >
> > > > > > > That's good to know.
> > > > > > >
> > > > > > > I just built it into U-Boot:
> > > > > > >
> > > > > > > NOTICE:  BL31: v2.5(release):v2.5
> > > > > > > NOTICE:  BL31: Built : 08:24:13, Nov 21 2021
> > > > > > >
> > > > > > > The Etnaviv driver is still loading without hanging
> > > > > > >
> > > > > > > root@beacon-imx8mn-kit:~# dmesg |grep -i etna
> > > > > > > [   12.393936] etnaviv etnaviv: bound 38000000.gpu (ops gpu_ops [etnaviv])
> > > > > > > [   12.400676] etnaviv-gpu 38000000.gpu: model: GC7000, revision: 6203
> > > > > > > [   12.641297] [drm] Initialized etnaviv 1.3.0 20151214 for etnaviv on minor 0
> > > > > > >
> > > > > > >
> > > > > >
> > > > > > Tim,
> > > > > >
> > > > > > Which version of Nano do you have?  Not all Nano SoC's have a GPU from
> > > > > > looking at the datasheet [1] .  I am using MIMX8MN2CVTIZAA (Nano Solo)
> > > > > >
> > > > > > [1] - https://www.nxp.com/docs/en/data-sheet/IMX8MNIEC.pdf
> > > > > >
> > > > >
> > > > > Adam,
> > > > >
> > > > > The board I have here has MIMX8MN5CVTIZAA so i.MX 8M Nano QuadLite
> > > > > with 'No GPU' as you expected.
> > > > >
> > > > > So I have to add the following to keep my board from hanging after your series:
> > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
> > > > > b/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
> > > > > index 236f425e1570..0d256a607b7c 100644
> > > > > --- a/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
> > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
> > > > > @@ -251,6 +251,10 @@
> > > > >         };
> > > > >  };
> > > > >
> > > > > +&gpu {
> > > > > +       status = "disabled";
> > > > > +};
> > > > > +
> > > > >  &i2c1 {
> > > > >         clock-frequency = <100000>;
> > > > >         pinctrl-names = "default";
> > > > >
> > > > > This situation is similar to the one I encountered with the
> > > > > imx8mm-venice-gw7901 where adding the GPC node caused my board (which
> > > > > did not power the GPU) to hang until I added disables to the
> > > > > device-tree with commit 7973009235e2 ("arm64: dts:
> > > > > imx8mm-venice-gw7901.dts: disable pgc_gpumix"). It feels painful to
> > > > > have to add patches to keep things from hanging after additional
> > > > > functionality is added to dt but perhaps that is more common than I
> > > > > think esp for SoC's like IMX8M which have a lot of lingering support
> > > > > still coming in.
> > > > >
> > > > Yea, it's unfortunate that those patches break your board, but I guess
> > > > we need to accept this, while there is still a lot of feature work
> > > > going on.
> >
> > There are a significant number of peripherals which are defined and
> > marked as 'disabled' by default, so I don't think it's unreasonable to
> > do that here.
> > I'd like to propose we keep the default disabled and people who
> > need/want the GPU enabled can turn it on.  Why waste the power if it's
> > not needed?
> >
> Sure, if a significant number of chips has the GPU disabled, we might
> want to keep it disabled in the base dtsi. With those variants it's
> always a tradeoff, for example there are SKUs of the i.MX6 that had the
> VPU disabled, but very few of those were in the field, so the VPUs are
> enabled in the SoC base dtsi and only users of those special SKUs would
> need to disable them in the board DT.
>
> The power argument isn't valid, as the kernel driver will suspend the
> device when not needed, so there is no wasted power (aside from the
> sort moment while the driver probes) with the GPU enabled.
>
> The rule of thumb for when a device is default enabled in the SoC dsti
> has always been (at least for i.MX) that the peripheral must not have a
> board level dependency. While a i2c controller obviously needs a i2c
> bus connected on the board to fulfill its purpose, a GPU can be used as
> color space converter or something like that with no board level
> interaction. Now the line is a bit blurred by having multiple power
> rails into the SoC, so one could argue that the GPUs and VPUs now have
> some board level dependency on the i.MX8M*.

That makes sense.

Do we defer to Shawn as the final arbiter as to whether or not it's
enabled/disabled?  It would be nice to get Nano caught up in
functionality as much as possible.

adam

>
> Regards,
> Lucas
>
Adam Ford Nov. 23, 2021, 4:40 p.m. UTC | #17
On Tue, Nov 23, 2021 at 8:30 AM Adam Ford <aford173@gmail.com> wrote:
>
> On Tue, Nov 23, 2021 at 8:24 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> >
> > Am Dienstag, dem 23.11.2021 um 08:08 -0600 schrieb Adam Ford:
> > > On Mon, Nov 22, 2021 at 3:52 PM Tim Harvey <tharvey@gateworks.com> wrote:
> > > >
> > > > On Mon, Nov 22, 2021 at 10:20 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > > > >
> > > > > Am Montag, dem 22.11.2021 um 09:59 -0800 schrieb Tim Harvey:
> > > > > > On Sun, Nov 21, 2021 at 7:25 AM Adam Ford <aford173@gmail.com> wrote:
> > > > > > >
> > > > > > > On Sun, Nov 21, 2021 at 8:34 AM Adam Ford <aford173@gmail.com> wrote:
> > > > > > > >
> > > > > > > > On Sun, Nov 21, 2021 at 8:21 AM Fabio Estevam <festevam@gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > Hi Adam,
> > > > > > > > >
> > > > > > > > > On Sun, Nov 21, 2021 at 11:17 AM Adam Ford <aford173@gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > > I am using https://source.codeaurora.org/external/imx/imx-atf/log/?h=lf_v2.4
> > > > > > > > > >
> > > > > > > > > > Since the driver sending SMCC commands to ATF isn't doing that, I
> > > > > > > > > > assume it's safe to use the linux power-domain drivers with the ATF
> > > > > > > > > > from NXP's kernel.
> > > > > > > > > >
> > > > > > > > > > If you can point me to the repo you think I should be using, I'll give it a try.
> > > > > > > > >
> > > > > > > > > Do you know if the mainline TF-A repo v2.5 works too?
> > > > > > > > > https://github.com/ARM-software/arm-trusted-firmware/tree/v2.5
> > > > > > > >
> > > > > > > > That's good to know.
> > > > > > > >
> > > > > > > > I just built it into U-Boot:
> > > > > > > >
> > > > > > > > NOTICE:  BL31: v2.5(release):v2.5
> > > > > > > > NOTICE:  BL31: Built : 08:24:13, Nov 21 2021
> > > > > > > >
> > > > > > > > The Etnaviv driver is still loading without hanging
> > > > > > > >
> > > > > > > > root@beacon-imx8mn-kit:~# dmesg |grep -i etna
> > > > > > > > [   12.393936] etnaviv etnaviv: bound 38000000.gpu (ops gpu_ops [etnaviv])
> > > > > > > > [   12.400676] etnaviv-gpu 38000000.gpu: model: GC7000, revision: 6203
> > > > > > > > [   12.641297] [drm] Initialized etnaviv 1.3.0 20151214 for etnaviv on minor 0
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > > > Tim,
> > > > > > >
> > > > > > > Which version of Nano do you have?  Not all Nano SoC's have a GPU from
> > > > > > > looking at the datasheet [1] .  I am using MIMX8MN2CVTIZAA (Nano Solo)
> > > > > > >
> > > > > > > [1] - https://www.nxp.com/docs/en/data-sheet/IMX8MNIEC.pdf
> > > > > > >
> > > > > >
> > > > > > Adam,
> > > > > >
> > > > > > The board I have here has MIMX8MN5CVTIZAA so i.MX 8M Nano QuadLite
> > > > > > with 'No GPU' as you expected.
> > > > > >
> > > > > > So I have to add the following to keep my board from hanging after your series:
> > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
> > > > > > b/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
> > > > > > index 236f425e1570..0d256a607b7c 100644
> > > > > > --- a/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
> > > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
> > > > > > @@ -251,6 +251,10 @@
> > > > > >         };
> > > > > >  };
> > > > > >
> > > > > > +&gpu {
> > > > > > +       status = "disabled";
> > > > > > +};
> > > > > > +
> > > > > >  &i2c1 {
> > > > > >         clock-frequency = <100000>;
> > > > > >         pinctrl-names = "default";
> > > > > >
> > > > > > This situation is similar to the one I encountered with the
> > > > > > imx8mm-venice-gw7901 where adding the GPC node caused my board (which
> > > > > > did not power the GPU) to hang until I added disables to the
> > > > > > device-tree with commit 7973009235e2 ("arm64: dts:
> > > > > > imx8mm-venice-gw7901.dts: disable pgc_gpumix"). It feels painful to
> > > > > > have to add patches to keep things from hanging after additional
> > > > > > functionality is added to dt but perhaps that is more common than I
> > > > > > think esp for SoC's like IMX8M which have a lot of lingering support
> > > > > > still coming in.
> > > > > >
> > > > > Yea, it's unfortunate that those patches break your board, but I guess
> > > > > we need to accept this, while there is still a lot of feature work
> > > > > going on.
> > >
> > > There are a significant number of peripherals which are defined and
> > > marked as 'disabled' by default, so I don't think it's unreasonable to
> > > do that here.
> > > I'd like to propose we keep the default disabled and people who
> > > need/want the GPU enabled can turn it on.  Why waste the power if it's
> > > not needed?
> > >
> > Sure, if a significant number of chips has the GPU disabled, we might
> > want to keep it disabled in the base dtsi. With those variants it's
> > always a tradeoff, for example there are SKUs of the i.MX6 that had the
> > VPU disabled, but very few of those were in the field, so the VPUs are
> > enabled in the SoC base dtsi and only users of those special SKUs would
> > need to disable them in the board DT.
> >
> > The power argument isn't valid, as the kernel driver will suspend the
> > device when not needed, so there is no wasted power (aside from the
> > sort moment while the driver probes) with the GPU enabled.
> >
> > The rule of thumb for when a device is default enabled in the SoC dsti
> > has always been (at least for i.MX) that the peripheral must not have a
> > board level dependency. While a i2c controller obviously needs a i2c
> > bus connected on the board to fulfill its purpose, a GPU can be used as
> > color space converter or something like that with no board level
> > interaction. Now the line is a bit blurred by having multiple power
> > rails into the SoC, so one could argue that the GPUs and VPUs now have
> > some board level dependency on the i.MX8M*.
>
> That makes sense.
>
> Do we defer to Shawn as the final arbiter as to whether or not it's
> enabled/disabled?  It would be nice to get Nano caught up in
> functionality as much as possible.

We could add two more device trees, one for 8mnl (lite) and 8mnul (ulta-lite)

imx8mnl:

#include imx8mn.dtsi

&gpu {
    status = "disabled";
};


imx8mnul:

#include imx8mnl

&dsi {
    status = "disabled";
};

Then the boards using either lite or ultralite just include their
respective SoC.dtsi instead of imx8mn.dtsi.  This is similar to what
we do with the plethora of i.mx6 options.

Just a thought.  Although, I really like the idea of the bootloader
disabling the unavailable nodes.

adam
>
> adam
>
> >
> > Regards,
> > Lucas
> >
Lucas Stach Nov. 23, 2021, 5:03 p.m. UTC | #18
Am Montag, dem 22.11.2021 um 13:52 -0800 schrieb Tim Harvey:
> On Mon, Nov 22, 2021 at 10:20 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > 
> > Am Montag, dem 22.11.2021 um 09:59 -0800 schrieb Tim Harvey:
> > > On Sun, Nov 21, 2021 at 7:25 AM Adam Ford <aford173@gmail.com> wrote:
> > > > 
> > > > On Sun, Nov 21, 2021 at 8:34 AM Adam Ford <aford173@gmail.com> wrote:
> > > > > 
> > > > > On Sun, Nov 21, 2021 at 8:21 AM Fabio Estevam <festevam@gmail.com> wrote:
> > > > > > 
> > > > > > Hi Adam,
> > > > > > 
> > > > > > On Sun, Nov 21, 2021 at 11:17 AM Adam Ford <aford173@gmail.com> wrote:
> > > > > > 
> > > > > > > I am using https://source.codeaurora.org/external/imx/imx-atf/log/?h=lf_v2.4
> > > > > > > 
> > > > > > > Since the driver sending SMCC commands to ATF isn't doing that, I
> > > > > > > assume it's safe to use the linux power-domain drivers with the ATF
> > > > > > > from NXP's kernel.
> > > > > > > 
> > > > > > > If you can point me to the repo you think I should be using, I'll give it a try.
> > > > > > 
> > > > > > Do you know if the mainline TF-A repo v2.5 works too?
> > > > > > https://github.com/ARM-software/arm-trusted-firmware/tree/v2.5
> > > > > 
> > > > > That's good to know.
> > > > > 
> > > > > I just built it into U-Boot:
> > > > > 
> > > > > NOTICE:  BL31: v2.5(release):v2.5
> > > > > NOTICE:  BL31: Built : 08:24:13, Nov 21 2021
> > > > > 
> > > > > The Etnaviv driver is still loading without hanging
> > > > > 
> > > > > root@beacon-imx8mn-kit:~# dmesg |grep -i etna
> > > > > [   12.393936] etnaviv etnaviv: bound 38000000.gpu (ops gpu_ops [etnaviv])
> > > > > [   12.400676] etnaviv-gpu 38000000.gpu: model: GC7000, revision: 6203
> > > > > [   12.641297] [drm] Initialized etnaviv 1.3.0 20151214 for etnaviv on minor 0
> > > > > 
> > > > > 
> > > > 
> > > > Tim,
> > > > 
> > > > Which version of Nano do you have?  Not all Nano SoC's have a GPU from
> > > > looking at the datasheet [1] .  I am using MIMX8MN2CVTIZAA (Nano Solo)
> > > > 
> > > > [1] - https://www.nxp.com/docs/en/data-sheet/IMX8MNIEC.pdf
> > > > 
> > > 
> > > Adam,
> > > 
> > > The board I have here has MIMX8MN5CVTIZAA so i.MX 8M Nano QuadLite
> > > with 'No GPU' as you expected.
> > > 
> > > So I have to add the following to keep my board from hanging after your series:
> > > diff --git a/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
> > > b/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
> > > index 236f425e1570..0d256a607b7c 100644
> > > --- a/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
> > > +++ b/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
> > > @@ -251,6 +251,10 @@
> > >         };
> > >  };
> > > 
> > > +&gpu {
> > > +       status = "disabled";
> > > +};
> > > +
> > >  &i2c1 {
> > >         clock-frequency = <100000>;
> > >         pinctrl-names = "default";
> > > 
> > > This situation is similar to the one I encountered with the
> > > imx8mm-venice-gw7901 where adding the GPC node caused my board (which
> > > did not power the GPU) to hang until I added disables to the
> > > device-tree with commit 7973009235e2 ("arm64: dts:
> > > imx8mm-venice-gw7901.dts: disable pgc_gpumix"). It feels painful to
> > > have to add patches to keep things from hanging after additional
> > > functionality is added to dt but perhaps that is more common than I
> > > think esp for SoC's like IMX8M which have a lot of lingering support
> > > still coming in.
> > > 
> > Yea, it's unfortunate that those patches break your board, but I guess
> > we need to accept this, while there is still a lot of feature work
> > going on.
> > 
> > > I don't mind at all submitting the above patch to fix my board after
> > > your series is accepted as I think that having an IMX8MN with 'no gpu'
> > > is perhaps less likely than having one with a GPU and thus we probably
> > > shouldn't mark the node as disabled and force everyone that has a GPU
> > > to go and enable it.
> > > 
> > > I wonder however if we should think about adding something to etnaviv
> > > to check the capability so that the same dt could be used with both
> > > CPU variants?
> > 
> > etnaviv or really the kernel at all is not the place to handle this.
> > The DT is supposed to describe the hardware and the kernel should be
> > able to trust this description.
> > 
> > If there is some way to read the chip capabilities and avoid having too
> > much DT variants in the kernel, the right place to handle this is the
> > software running before the kernel is started, i.e. your bootloader.
> > Barebox for example reads the SCU fuses on i.MX6 and removes the DT
> > nodes for the fused off CPU cores on i.MX6S and i.MX6D.
> > 
> 
> Lucas,
> 
> I agree - the boot firmware is an appropriate place for this. I
> believe the correct course of action in the case of the IMX8M Nano
> would be to do the following for no GPU:
> - disable disp_blk_ctrl node
> - disable gpu node
> - disable pgc_gpumix node
> 
> What would you propose to do for detection of this in boot firmware?
> The DIGPROG register is currently used in U-Boot to determine IMX8M
> variants including Nano
> Qad/Dual/Solo/QaudLite/DualLite/SoloLite/UltraLite Quad/UltraLite
> Dual/UltraLite Solo. It would appear all the 'Lite' and 'UltraLite'
> variants have no GPU.

According to the fusemap there are even individual fuses to indicate
disabled GPU, DSI, CSI and other peripherals (table 6-31 in the 8MN RM,
0x450 range). I would expect that those are burned correctly on the
stripped down SKUs. Maybe you can validate this on your side?

Regards,
Lucas
Lucas Stach Nov. 23, 2021, 5:10 p.m. UTC | #19
Am Dienstag, dem 23.11.2021 um 10:40 -0600 schrieb Adam Ford:
> On Tue, Nov 23, 2021 at 8:30 AM Adam Ford <aford173@gmail.com> wrote:
> > 
> > On Tue, Nov 23, 2021 at 8:24 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > > 
> > > Am Dienstag, dem 23.11.2021 um 08:08 -0600 schrieb Adam Ford:
> > > > On Mon, Nov 22, 2021 at 3:52 PM Tim Harvey <tharvey@gateworks.com> wrote:
> > > > > 
> > > > > On Mon, Nov 22, 2021 at 10:20 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > > > > > 
> > > > > > Am Montag, dem 22.11.2021 um 09:59 -0800 schrieb Tim Harvey:
> > > > > > > On Sun, Nov 21, 2021 at 7:25 AM Adam Ford <aford173@gmail.com> wrote:
> > > > > > > > 
> > > > > > > > On Sun, Nov 21, 2021 at 8:34 AM Adam Ford <aford173@gmail.com> wrote:
> > > > > > > > > 
> > > > > > > > > On Sun, Nov 21, 2021 at 8:21 AM Fabio Estevam <festevam@gmail.com> wrote:
> > > > > > > > > > 
> > > > > > > > > > Hi Adam,
> > > > > > > > > > 
> > > > > > > > > > On Sun, Nov 21, 2021 at 11:17 AM Adam Ford <aford173@gmail.com> wrote:
> > > > > > > > > > 
> > > > > > > > > > > I am using https://source.codeaurora.org/external/imx/imx-atf/log/?h=lf_v2.4
> > > > > > > > > > > 
> > > > > > > > > > > Since the driver sending SMCC commands to ATF isn't doing that, I
> > > > > > > > > > > assume it's safe to use the linux power-domain drivers with the ATF
> > > > > > > > > > > from NXP's kernel.
> > > > > > > > > > > 
> > > > > > > > > > > If you can point me to the repo you think I should be using, I'll give it a try.
> > > > > > > > > > 
> > > > > > > > > > Do you know if the mainline TF-A repo v2.5 works too?
> > > > > > > > > > https://github.com/ARM-software/arm-trusted-firmware/tree/v2.5
> > > > > > > > > 
> > > > > > > > > That's good to know.
> > > > > > > > > 
> > > > > > > > > I just built it into U-Boot:
> > > > > > > > > 
> > > > > > > > > NOTICE:  BL31: v2.5(release):v2.5
> > > > > > > > > NOTICE:  BL31: Built : 08:24:13, Nov 21 2021
> > > > > > > > > 
> > > > > > > > > The Etnaviv driver is still loading without hanging
> > > > > > > > > 
> > > > > > > > > root@beacon-imx8mn-kit:~# dmesg |grep -i etna
> > > > > > > > > [   12.393936] etnaviv etnaviv: bound 38000000.gpu (ops gpu_ops [etnaviv])
> > > > > > > > > [   12.400676] etnaviv-gpu 38000000.gpu: model: GC7000, revision: 6203
> > > > > > > > > [   12.641297] [drm] Initialized etnaviv 1.3.0 20151214 for etnaviv on minor 0
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > Tim,
> > > > > > > > 
> > > > > > > > Which version of Nano do you have?  Not all Nano SoC's have a GPU from
> > > > > > > > looking at the datasheet [1] .  I am using MIMX8MN2CVTIZAA (Nano Solo)
> > > > > > > > 
> > > > > > > > [1] - https://www.nxp.com/docs/en/data-sheet/IMX8MNIEC.pdf
> > > > > > > > 
> > > > > > > 
> > > > > > > Adam,
> > > > > > > 
> > > > > > > The board I have here has MIMX8MN5CVTIZAA so i.MX 8M Nano QuadLite
> > > > > > > with 'No GPU' as you expected.
> > > > > > > 
> > > > > > > So I have to add the following to keep my board from hanging after your series:
> > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
> > > > > > > b/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
> > > > > > > index 236f425e1570..0d256a607b7c 100644
> > > > > > > --- a/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
> > > > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
> > > > > > > @@ -251,6 +251,10 @@
> > > > > > >         };
> > > > > > >  };
> > > > > > > 
> > > > > > > +&gpu {
> > > > > > > +       status = "disabled";
> > > > > > > +};
> > > > > > > +
> > > > > > >  &i2c1 {
> > > > > > >         clock-frequency = <100000>;
> > > > > > >         pinctrl-names = "default";
> > > > > > > 
> > > > > > > This situation is similar to the one I encountered with the
> > > > > > > imx8mm-venice-gw7901 where adding the GPC node caused my board (which
> > > > > > > did not power the GPU) to hang until I added disables to the
> > > > > > > device-tree with commit 7973009235e2 ("arm64: dts:
> > > > > > > imx8mm-venice-gw7901.dts: disable pgc_gpumix"). It feels painful to
> > > > > > > have to add patches to keep things from hanging after additional
> > > > > > > functionality is added to dt but perhaps that is more common than I
> > > > > > > think esp for SoC's like IMX8M which have a lot of lingering support
> > > > > > > still coming in.
> > > > > > > 
> > > > > > Yea, it's unfortunate that those patches break your board, but I guess
> > > > > > we need to accept this, while there is still a lot of feature work
> > > > > > going on.
> > > > 
> > > > There are a significant number of peripherals which are defined and
> > > > marked as 'disabled' by default, so I don't think it's unreasonable to
> > > > do that here.
> > > > I'd like to propose we keep the default disabled and people who
> > > > need/want the GPU enabled can turn it on.  Why waste the power if it's
> > > > not needed?
> > > > 
> > > Sure, if a significant number of chips has the GPU disabled, we might
> > > want to keep it disabled in the base dtsi. With those variants it's
> > > always a tradeoff, for example there are SKUs of the i.MX6 that had the
> > > VPU disabled, but very few of those were in the field, so the VPUs are
> > > enabled in the SoC base dtsi and only users of those special SKUs would
> > > need to disable them in the board DT.
> > > 
> > > The power argument isn't valid, as the kernel driver will suspend the
> > > device when not needed, so there is no wasted power (aside from the
> > > sort moment while the driver probes) with the GPU enabled.
> > > 
> > > The rule of thumb for when a device is default enabled in the SoC dsti
> > > has always been (at least for i.MX) that the peripheral must not have a
> > > board level dependency. While a i2c controller obviously needs a i2c
> > > bus connected on the board to fulfill its purpose, a GPU can be used as
> > > color space converter or something like that with no board level
> > > interaction. Now the line is a bit blurred by having multiple power
> > > rails into the SoC, so one could argue that the GPUs and VPUs now have
> > > some board level dependency on the i.MX8M*.
> > 
> > That makes sense.
> > 
> > Do we defer to Shawn as the final arbiter as to whether or not it's
> > enabled/disabled?  It would be nice to get Nano caught up in
> > functionality as much as possible.
> 
> We could add two more device trees, one for 8mnl (lite) and 8mnul (ulta-lite)
> 
> imx8mnl:
> 
> #include imx8mn.dtsi
> 
> &gpu {
>     status = "disabled";
> };
> 
> 
> imx8mnul:
> 
> #include imx8mnl
> 
> &dsi {
>     status = "disabled";
> };
> 
> Then the boards using either lite or ultralite just include their
> respective SoC.dtsi instead of imx8mn.dtsi.  This is similar to what
> we do with the plethora of i.mx6 options.
> 
Yes, that's an option but it quickly blows up in a combinatorial
explosion. As the chips are pin compatible there is a high probability
that hardware makers will start to offer boards with different feature
sets and then you need to have a number of board DTs just to include
the right dtsi for the chip.

> Just a thought.  Although, I really like the idea of the bootloader
> disabling the unavailable nodes.

Yea, it seems there are even fuses that allow to check those
capabilities, see my reply to Tim. So I think relying on the boot
firmware to fix things up would be the best option, as it allows to
keep the number of DTs small and does not place a big burden on the
boot firmware implementation.

Regards,
Lucas