[PATCH] hack: soc: imx: gpcv2: avoid unbalanced powering off of one device

[Martin Kepplinger]

Am Mittwoch, dem 08.12.2021 um 15:05 +0100 schrieb Lucas Stach:
> Hi Martin,
> 
> Am Mittwoch, dem 08.12.2021 um 14:47 +0100 schrieb Martin Kepplinger:
> > Hi Lucas,
> > 
> > I've posted this hack with a report here a few days back:
> > https://lore.kernel.org/linux-arm-kernel/20211122115145.177196-1-martin.kepplinger@puri.sm/
> > 
> > But now that I see these suspend/resume callback additions things
> > again go wrong on my imx8mq system.
> > 
> > With a v5.16-rc4 based tree and printing on regulator
> > enable/disable,
> > system suspend + resume looks like so:
> > 
> > [   47.559681] imx-pgc imx-pgc-domain.5: enable
> > [   47.584679] imx-pgc imx-pgc-domain.0: disable
> > [   47.646592] imx-pgc imx-pgc-domain.5: disable
> > [   47.823627] imx-pgc imx-pgc-domain.5: enable
> > [   47.994805] imx-pgc imx-pgc-domain.5: disable
> > [   48.664018] imx-pgc imx-pgc-domain.5: enable
> > [   48.805828] imx-pgc imx-pgc-domain.5: disable
> > [   49.909579] imx-pgc imx-pgc-domain.6: enable
> > [   50.013079] imx-pgc imx-pgc-domain.6: failed to enable
> > regulator: -110
> > [   50.013686] imx-pgc imx-pgc-domain.5: enable
> > [   50.120224] imx-pgc imx-pgc-domain.5: failed to enable
> > regulator: -110
> > [   50.120324] imx-pgc imx-pgc-domain.0: enable
> > [   53.703468] imx-pgc imx-pgc-domain.0: disable
> > [   53.746368] imx-pgc imx-pgc-domain.5: disable
> > [   53.754452] imx-pgc imx-pgc-domain.5: failed to disable
> > regulator: -5
> > [   53.765045] imx-pgc imx-pgc-domain.6: disable
> > [   53.822269] imx-pgc imx-pgc-domain.6: failed to disable
> > regulator: -5
> > 
> > 
> > But my main point is that the situation is a bit hard to understand
> > right now. when transitioning to system suspend we expect (if
> > disabled)
> > enable+disable to happen, right? and after resuming: enable (+
> > runtime disable).
> 
> Right.
> 
> > Makes sense functinally, but I wonder if we could implement it a
> > bit clearer?
> 
> Unfortunately, I don't think there is a way to do this in a much
> cleaner way. 
> > 
> > Anyway I'm also not sure whether imx8mq might be different than
> > your
> > imx8mm system.
> 
> imx8mq, without the upcoming VPU blk-ctrl, has no nested power
> domains,
> which are the main reason for the power domain runtime resume before
> the system suspend. If they aren't resumed before the suspend the
> nested domains will not be able to power up their parent domains on
> resume, due to runtime PM being unavailable at this stage. All of
> 8mm/8mn/8mp have some sorts of nested power domains.
> 
> > 
> > When I revert your one patch and add my hack below again, things
> > work again and the system resumes without errors.
> > 
> > Can you imagine what might be missing here?
> > 
> I would like to understand why the runtime resume of the power domain
> isn't working for you. Is this a i2c attached regulator? There might
> be
> some RPM dependency handling (device link) missing to keep the i2c
> bus
> alive until the power domains finished their suspend handling.

domain 5 is gpu, domain 6 is vpu. gpu has power-supply described to be
the "buck3_reg" regulator:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/freescale/imx8mq-librem5.dtsi#n760

vpu has power-supply described as "buck4_reg" on the board. So no
regulators that control a gpio. They are handled by the pmic though
that is attached to i2c. Maybe I should trace what the pmic does a bit
closer...

> 
> Regards,
> Lucas
> 
> > thanks a lot for working on this!
> > 
> >                                martin
> > ---
> >  drivers/soc/imx/gpcv2.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
> > index 07610bf87854..898886c9d799 100644
> > --- a/drivers/soc/imx/gpcv2.c
> > +++ b/drivers/soc/imx/gpcv2.c
> > @@ -319,6 +319,9 @@ static int imx_pgc_power_down(struct
> > generic_pm_domain *genpd)
> >         u32 reg_val, pgc;
> >         int ret;
> >  
> > +       if (pm_runtime_suspended(domain->dev))
> > +               return 0;
> > +
> >         /* Enable reset clocks for all devices in the domain */
> >         if (!domain->keep_clocks) {
> >                 ret = clk_bulk_prepare_enable(domain->num_clks,
> > domain->clks);
> 
>