linux-kernel - Re: [PATCH v4 3/3] soc: imx: gpcv2: fix suspend/resume by setting GENPD_FLAG_IRQ

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5a6bfd6827f8ad838bdab8dfb208753ad258b1ec.camel@puri.sm>
Date:   Wed, 20 Jul 2022 10:05:57 +0200
From:   Martin Kepplinger <martin.kepplinger@...i.sm>
To:     Lucas Stach <l.stach@...gutronix.de>, rafael@...nel.org,
        khilman@...nel.org, ulf.hansson@...aro.org, robh@...nel.org,
        krzysztof.kozlowski@...aro.org, shawnguo@...nel.org,
        s.hauer@...gutronix.de, festevam@...il.com, pavel@....cz
Cc:     kernel@...i.sm, linux-imx@....com, broonie@...nel.org,
        aford173@...il.com, linux-pm@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v4 3/3] soc: imx: gpcv2: fix suspend/resume by setting
 GENPD_FLAG_IRQ_ON

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@...i.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");
> 
>