Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <5a6bfd6827f8ad838bdab8dfb208753ad258b1ec.camel@puri.sm>
Subject: Re: [PATCH v4 3/3] soc: imx: gpcv2: fix suspend/resume by setting
 GENPD_FLAG_IRQ_ON
From:   Martin Kepplinger <martin.kepplinger@puri.sm>
To:     Lucas Stach <l.stach@pengutronix.de>, rafael@kernel.org,
        khilman@kernel.org, ulf.hansson@linaro.org, robh@kernel.org,
        krzysztof.kozlowski@linaro.org, shawnguo@kernel.org,
        s.hauer@pengutronix.de, festevam@gmail.com, pavel@ucw.cz
Cc:     kernel@puri.sm, linux-imx@nxp.com, broonie@kernel.org,
        aford173@gmail.com, linux-pm@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org
Date:   Wed, 20 Jul 2022 10:05:57 +0200
In-Reply-To: <a7d51c154693881523e5d96c443a7dc9b3cc216d.camel@pengutronix.de>
References: <20220720043444.1289952-1-martin.kepplinger@puri.sm>
         <20220720043444.1289952-4-martin.kepplinger@puri.sm>
         <a7d51c154693881523e5d96c443a7dc9b3cc216d.camel@pengutronix.de>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.38.3-1 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

Am Mittwoch, dem 20.07.2022 um 09:53 +0200 schrieb Lucas Stach:
> Am Mittwoch, dem 20.07.2022 um 06:34 +0200 schrieb Martin Kepplinger:
> > For boards that use power-domains' power-supplies that need
> > interrupts
> > to work (like regulator over i2c), set GENPD_FLAG_IRQ_ON.
> > This will tell genpd to adjust accordingly. Currently it "only"
> > sets the
> > correct suspend/resume callbacks.
> > 
> > This fixes suspend/resume on imx8mq-librem5 boards (tested) and
> > imx8mq-evk (by looking at dts) and possibly more.
> > 
> > Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
> > ---
> >  drivers/soc/imx/gpcv2.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
> > index 85aa86e1338a..46d2ead2352b 100644
> > --- a/drivers/soc/imx/gpcv2.c
> > +++ b/drivers/soc/imx/gpcv2.c
> > @@ -1303,6 +1303,7 @@ static const struct imx_pgc_domain_data
> > imx8mn_pgc_domain_data = {
> >  static int imx_pgc_domain_probe(struct platform_device *pdev)
> >  {
> >         struct imx_pgc_domain *domain = pdev->dev.platform_data;
> > +       struct device_node *dn;
> >         int ret;
> >  
> >         domain->dev = &pdev->dev;
> > @@ -1333,6 +1334,14 @@ static int imx_pgc_domain_probe(struct
> > platform_device *pdev)
> >                 regmap_update_bits(domain->regmap, domain->regs-
> > >map,
> >                                    domain->bits.map, domain-
> > >bits.map);
> >  
> > +       dn = of_parse_phandle(domain->dev->of_node, "power-supply",
> > 0);
> > +       if (dn) {
> > +               while ((dn = of_get_next_parent(dn))) {
> > +                       if (of_get_property(dn, "interrupts",
> > NULL))
> > +                               domain->genpd.flags |=
> > GENPD_FLAG_IRQ_ON;
> > +               }
> > +       }
> > +
> While I understand the intention, I think the DT walking is overkill.
> I
> believe that there are no cases where we have a external regulator
> attached to the PD and the devices in the domain needing noirq
> support.
> I think it's sufficient to simply set the IRQ_ON flag based on
> presence
> of the power-supply property on the domain DT node.

Are you sure? Can't boards just *describe* a power-supply that doesn't
really do much, where noirq would work? looking for "interrupts" in any
parent feels very stable and makes sure we only change behaviour when
really needed. But for the boards I'm looking at, I have to admit it
wouldn't change anything afaik. So if you insist, I'll happily remove
that.

Also, I forgot to say earlier: We could even add "if not regulator-
always-on" to the DT parsing above, because in that case noirq is fine
even for external regulators. Should I add that? I'd like as little
runtime change as possible so I would add that (and keep the
"interrupts" search above for the same reason). 

thanks for looking at this,

                             martin


> 
> Regards,
> Lucas
> 
> >         ret = pm_genpd_init(&domain->genpd, NULL, true);
> >         if (ret) {
> >                 dev_err(domain->dev, "Failed to init power
> > domain\n");
> 
>