| Message ID | 20220720043444.1289952-4-martin.kepplinger@puri.sm (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | power: domain: handle power supplies that need interrupts | expand |
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. Regards, Lucas > ret = pm_genpd_init(&domain->genpd, NULL, true); > if (ret) { > dev_err(domain->dev, "Failed to init power domain\n");
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"); > >
Am Mittwoch, dem 20.07.2022 um 10:05 +0200 schrieb Martin Kepplinger: > 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. > I'm pretty sure that this holds for all boards. Yes, it might introduce some more runtime changes than your option, but it will be more consistent. One could possibly have a simple GPIO regulator, which could work in noirq if the GPIO is internal MMIO, but it already breaks when the GPIO is from an i2c attached GPIO expander. This might even be a good example where your DT parsing breaks: a GPIO regulator is not necessarily a child device of the i2c GPIO expander, so the DT walking will miss that IRQs need to be functional in order to toggle the GPIO. Just keying the IRQ_ON flag on the presence of the power-supply property will have less surprises, I think. > > > 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). > Yea, one could make this even more complex to preserve the current behavior as much as possible, but I just don't think that the current behavior is relevant enough to warrant the complexity and possible inconsistent behavior on different systems. Thanks for working on this! Regards, Lucas > 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"); > > > > > >
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; + } + } + ret = pm_genpd_init(&domain->genpd, NULL, true); if (ret) { dev_err(domain->dev, "Failed to init power domain\n");
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(+)