[PATCH v4 14/18] arm64: dts: imx8mm: add GPC node

[Tim Harvey]

On Sun, Oct 3, 2021 at 12:44 PM Lucas Stach <l.stach at pengutronix.de> wrote:
>
> Am Sonntag, dem 03.10.2021 um 10:17 -0700 schrieb Tim Harvey:
> > On Sat, Oct 2, 2021 at 5:51 AM Lucas Stach <l.stach at pengutronix.de> wrote:
> > >
> > > Hi Tim,
> > >
> > > Am Freitag, dem 01.10.2021 um 19:43 -0700 schrieb Tim Harvey:
> > > > > > > >
> > > [...]
> > > > > > > > > Lucas,
> > > > > > > > >
> > > > > > > > > I've been using your 'i.MX8MM GPC improvements and BLK_CTRL driver'
> > > > > > > > > series for imx8mm-venice* and imx8mn-venice* boards. Thank you for
> > > > > > > > > this work and I hope to see it merged soon!
> > > > > > > > >
> > > > > > > > > I have found that on the imx8mm-venice-gw7901 board which does not use
> > > > > > > > > MIPI and thus does not connect VDD_MIPI_1P8, VDD_MIPI_1P2,
> > > > > > > > > VDD_MIPI_0P9, MIPI_VREG_CAP pins on the IMX8MM hangs with this
> > > > > > > > > particular patch. If I comment out the pgc_mipi domain and subsequent
> > > > > > > > > disp_blk_ctrl node from a later patch it resolves the hang. Is this
> > > > > > > > > behavior expected and what would your recommendation be to work around
> > > > > > > > > it?
> > > > > > > >
> > > > > > > > No, this isn't expected. If there are no active devices in the MIPI
> > > > > > > > domain, the power domain should not be touched, as we treat all of them
> > > > > > > > as disabled initially. If we don't touch the domain I would expect that
> > > > > > > > the power supply not being present shouldn't be an issue.
> > > > > > > >
> > > > > > > > Can you check if something in your system causes this power domain to
> > > > > > > > be powered up? Easiest way is probably to sprinkle a
> > > > > > > > printk("%s\n, genpd->name) in both imx8m_blk_ctrl_power_on() and
> > > > > > > > imx_gpc_power_on().
> > > > > > > >
> > > > > > >
> > > > > > > Lucas,
> > > > > > >
> > > > > > > Here's what I see before I hang (debug print on both power on/off
> > > > > > > followed by a msleep(1000) to make sure I see it before I hang):
> > > > > > > [    0.518319] imx_pgc_power_up hsiomix
> > > > > > > [    0.624031] imx_pgc_power_down hsiomix
> > > > > > > [    0.731879] imx_pgc_power_up hsiomix
> > > > > > > [    0.839906] imx_pgc_power_down hsiomix
> > > > > > > [    0.947875] imx_pgc_power_up hsiomix
> > > > > > > [    1.055859] imx_pgc_power_down hsiomix
> > > > > > > [    1.057296] imx_pgc_power_up gpumix
> > > > > > > [    1.167884] imx_pgc_power_down gpumix
> > > > > > > [    0.518513] imx_pgc_power_up hsiomix
> > > > > > > [    0.519933] imx_pgc_power_up gpumix
> > > > > > >
> > > > > >
> > > > > > The board also has IMX8MM VDD_GPU pins not connected so it makes sense
> > > > > > that we hang here I suppose. Yet if I add the folloiwng to
> > > > > > imx8mm-venice-gw7901.dts it still tries to enable them and hangs:
> > > > > > &gpu_2d {
> > > > > >         status = "disabled";
> > > > > > };
> > > > > >
> > > > > > &gpu_3d {
> > > > > >         status = "disabled";
> > > > > > };
> > > > > >
> > > > > > &vpu_blk_ctrl {
> > > > > >         status = "disabled";
> > > > > > };
> > > > >
> > > > > The pgc_gpu is a "active" consumer of the pgc_gpumix domain while the
> > > > > driver gets probed, so the driver core will power up the gpumix domain
> > > > > for a moment during kernel init. To avoid this you must at least set
> > > > > the status of the pgc_gpu node to disabled.
> > > > >
> > > >
> > > > I've tried that and it doesn't work either.
> > > >
> > > > &gpu_2d {
> > > >         status = "disabled";
> > > > };
> > > >
> > > > &gpu_3d {
> > > >         status = "disabled";
> > > > };
> > > >
> > > > &vpu_blk_ctrl {
> > > >         status = "disabled";
> > > > };
> > > >
> > > > &pgc_gpumix {
> > > >         status = "disabled";
> > > > };
> > > >
> > > > &pgc_gpu {
> > > >         status = "disabled";
> > > > };
> > > >
> > > > The interesting thing is that the first patch that causes this is
> > > > 'arm64: dts: imx8mm: add GPC node' which just adds the gpc node and is
> > > > before the addition of the other nodes. Therefore these are being
> > > > enabled by default regardless of the above nodes being disabled (or
> > > > not even added yet to imx8mm.dtsi).
> > >
> > > My bad, I didn't think about the fact that the power domain devices are
> > > not instantiated via the common OF populate code. For the status
> > > property to work as expected the GPCv2 code needs to check this
> > > property. I've just sent a patch to make this happen. Can you give it
> > > another try with your DT changes and this patch applied?
> > >
> >
> > Yes, this makes sense and resolves it with only disabling pgc_gpumix in my dts.
> >
> > I believe it is fine for me to not specifically disable gpu_2d and
> > gpu_3d as they are not enabled by default in imx8mm.dtsi and I don't
> > believe that will change and I don't 'currently' need to disable
> > disp_blk_ctrl or pgc_mipi but wondering if I should as the MIPI rails
> > are not hooked up either? (ie will something added in the future try
> > to use them?)
> >
> I think it would be better to disable the GPU devices in your DT. They
> don't have a status property and thus are enabled by default. They
> won't probe without the power domain being available, but it will cause
> those devices to stay in probe deferred state forever, which may
> confuse some users. So I guess it would be good style to disable the
> GPU nodes.
>

it still tries to power the pgc_gpumix domain if I 'only' disable
gpu_2d and gpu_3d. I have to disable 'pgc_gpumix' specifically - is
that what you expect here?

> One consequence of disabling the MIPI power domain is that the disp-
> blk-ctrl driver won't probe anymore, as it needs all the referenced
> power domains to be available. Do you think this might be an issue? Is
> there a valid system configuration where you would use devices from
> from the display power domain, but not the MIPI domain? If that's a
> valid case we might need to make this configuration possible in the
> disp-blk-ctrl driver.

In the case of imx8mm I don't see that you can have any display
without MIPI however the imx8mp can have HDMI display without needing
MIPI so in general that should be allowed.

>
> > What needs to be done to get this series merged? I suppose it's too
> > late to get it into 5.13?
>
> 5.13? Way too late, even the 5.15 merge window is long closed. The
> series is almost completely reviewed and the DT bindings are also good
> to go. So if no one happens to run into any real showstopper, Shawn may
> be willing to pick up the series and stage it for the 5.16 merge
> window.

oops... I meant 5.15 but yes, seems too late for that. We seem to have
time to get this into 5.16 however. I'm not sure what Shawn's cut-off
is there.

Tim