| Message ID | 20220711094549.3445566-1-martin.kepplinger@puri.sm |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/3] power: domain: handle power supplies that need irq | expand |
Am Montag, dem 11.07.2022 um 12:38 +0200 schrieb Krzysztof Kozlowski: > On 11/07/2022 11:45, Martin Kepplinger wrote: > > Add the power-supply-needs-irq board description property for power > > domains. > > Where is a board description here? I think you just meant > "power-supply-needs-irq property"? > > > > Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm> > > --- > > .../devicetree/bindings/power/power-domain.yaml | 10 > > ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/power/power- > > domain.yaml b/Documentation/devicetree/bindings/power/power- > > domain.yaml > > index 889091b9814f..e82c2f7ccb97 100644 > > --- a/Documentation/devicetree/bindings/power/power-domain.yaml > > +++ b/Documentation/devicetree/bindings/power/power-domain.yaml > > @@ -70,6 +70,16 @@ properties: > > by the given provider should be subdomains of the domain > > specified > > by this binding. > > > > + power-supply: true > > This is a new property not described in the commit msg. true, I think it's missing and could be added as a separate patch. > > > + > > + power-supply-needs-irq: > > + type: boolean > > + description: > > + A power-supply can link for example to a regulator > > controlled via > > + i2c or otherwise needing interrupts enabled to be able to > > enable and > > + disable. > > Not really a property of power domain. How the regulator supply works > is > entirely up to regulator. Otherwise such property should appear for > every device. you're right. The power-domain driver could read the power-supply regulator node directly. Still, I think then a new regulator property is needed instead, or is it? > > > This property makes various callbacks usually run in the > > + noirq phase, being run when interrupts are available. > > Last sentence does not fit - you embed Linux implementation into DT > bindings. noirq phase is Linux specific. oh I keep making this mistake. thanks for the fast review! > > > + > > required: > > - "#power-domain-cells" > > > > > Best regards, > Krzysztof
On 11/07/2022 15:17, Martin Kepplinger wrote: > Am Montag, dem 11.07.2022 um 12:38 +0200 schrieb Krzysztof Kozlowski: >> On 11/07/2022 11:45, Martin Kepplinger wrote: >>> Add the power-supply-needs-irq board description property for power >>> domains. >> >> Where is a board description here? I think you just meant >> "power-supply-needs-irq property"? >>> >>> Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm> >>> --- >>> .../devicetree/bindings/power/power-domain.yaml | 10 >>> ++++++++++ >>> 1 file changed, 10 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/power/power- >>> domain.yaml b/Documentation/devicetree/bindings/power/power- >>> domain.yaml >>> index 889091b9814f..e82c2f7ccb97 100644 >>> --- a/Documentation/devicetree/bindings/power/power-domain.yaml >>> +++ b/Documentation/devicetree/bindings/power/power-domain.yaml >>> @@ -70,6 +70,16 @@ properties: >>> by the given provider should be subdomains of the domain >>> specified >>> by this binding. >>> >>> + power-supply: true >> >> This is a new property not described in the commit msg. > > true, I think it's missing and could be added as a separate patch. > >> >>> + >>> + power-supply-needs-irq: >>> + type: boolean >>> + description: >>> + A power-supply can link for example to a regulator >>> controlled via >>> + i2c or otherwise needing interrupts enabled to be able to >>> enable and >>> + disable. >> >> Not really a property of power domain. How the regulator supply works >> is >> entirely up to regulator. Otherwise such property should appear for >> every device. > > you're right. The power-domain driver could read the power-supply > regulator node directly. Still, I think then a new regulator property > is needed instead, or is it? In case of regulator, I am not so sure it needs a dedicated property of DT. If it is I2C regulator - the parent node is I2C bus and regulator device is some child of I2C controller (could be via a MFD device), so no need for dedicated property. If it uses interrupts, then: 1. The presence of interrupts is already known - "interrupts" property. 2. The actual use of interrupts is DT independent and only driver knows it. Best regards, Krzysztof
Am Dienstag, dem 12.07.2022 um 09:01 +0200 schrieb Krzysztof Kozlowski: > On 11/07/2022 15:17, Martin Kepplinger wrote: > > Am Montag, dem 11.07.2022 um 12:38 +0200 schrieb Krzysztof > > Kozlowski: > > > On 11/07/2022 11:45, Martin Kepplinger wrote: > > > > Add the power-supply-needs-irq board description property for > > > > power > > > > domains. > > > > > > Where is a board description here? I think you just meant > > > "power-supply-needs-irq property"? > > > > > > > > Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm> > > > > --- > > > > .../devicetree/bindings/power/power-domain.yaml | 10 > > > > ++++++++++ > > > > 1 file changed, 10 insertions(+) > > > > > > > > diff --git a/Documentation/devicetree/bindings/power/power- > > > > domain.yaml b/Documentation/devicetree/bindings/power/power- > > > > domain.yaml > > > > index 889091b9814f..e82c2f7ccb97 100644 > > > > --- a/Documentation/devicetree/bindings/power/power-domain.yaml > > > > +++ b/Documentation/devicetree/bindings/power/power-domain.yaml > > > > @@ -70,6 +70,16 @@ properties: > > > > by the given provider should be subdomains of the domain > > > > specified > > > > by this binding. > > > > > > > > + power-supply: true > > > > > > This is a new property not described in the commit msg. > > > > true, I think it's missing and could be added as a separate patch. > > > > > > > > > + > > > > + power-supply-needs-irq: > > > > + type: boolean > > > > + description: > > > > + A power-supply can link for example to a regulator > > > > controlled via > > > > + i2c or otherwise needing interrupts enabled to be able > > > > to > > > > enable and > > > > + disable. > > > > > > Not really a property of power domain. How the regulator supply > > > works > > > is > > > entirely up to regulator. Otherwise such property should appear > > > for > > > every device. > > > > you're right. The power-domain driver could read the power-supply > > regulator node directly. Still, I think then a new regulator > > property > > is needed instead, or is it? > > In case of regulator, I am not so sure it needs a dedicated property > of > DT. If it is I2C regulator - the parent node is I2C bus and regulator > device is some child of I2C controller (could be via a MFD device), > so > no need for dedicated property. > > If it uses interrupts, then: > 1. The presence of interrupts is already known - "interrupts" > property. > 2. The actual use of interrupts is DT independent and only driver > knows it. thanks for this great suggestion! for the imx8mq devices this (1.) indeed is the case for exactly the 3 regulators I manually describe here. v2 of this patch looks very elegant (and I guess I could have removed the DT people from the email, I forgot). here it is: https://lore.kernel.org/linux-arm-kernel/20220712121832.3659769-1-martin.kepplinger@puri.sm/T/#u > > Best regards, > Krzysztof
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 3e86772d5fac..c8fecba2c191 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -2298,6 +2298,16 @@ static bool genpd_present(const struct generic_pm_domain *genpd) return ret; } +static void of_genpd_get_power_supply_irq(struct generic_pm_domain *pd) +{ + if (of_property_read_bool(pd->dev.of_node, "power-supply-needs-irq")) { + pd->domain.ops.suspend = genpd_suspend_noirq; + pd->domain.ops.resume = genpd_resume_noirq; + pd->domain.ops.suspend_noirq = NULL; + pd->domain.ops.resume_noirq = NULL; + } +} + /** * of_genpd_add_provider_simple() - Register a simple PM domain provider * @np: Device node pointer associated with the PM domain provider. @@ -2343,6 +2353,8 @@ int of_genpd_add_provider_simple(struct device_node *np, genpd->provider = &np->fwnode; genpd->has_provider = true; + of_genpd_get_power_supply_irq(genpd); + return 0; } EXPORT_SYMBOL_GPL(of_genpd_add_provider_simple); @@ -2394,6 +2406,8 @@ int of_genpd_add_provider_onecell(struct device_node *np, genpd->provider = &np->fwnode; genpd->has_provider = true; + + of_genpd_get_power_supply_irq(genpd); } ret = genpd_add_provider(np, data->xlate, data);
Handle a new DT property power-supply-needs-irq that boards can use if the power-domains' power-supply node (regulator) needs interrupts to function. An example is a pmic regulator on i2c. Since suspend/resume doesn't work in these cases currently, switch to the normal, outer suspend/resume callbacks. Usually such a regulator already times out during suspend_noirq: [ 41.024193] buck4: failed to disable: -ETIMEDOUT Initially systemd suspend problems had been discussed at https://lore.kernel.org/linux-arm-kernel/20211002005954.1367653-8-l.stach@pengutronix.de/ which led to discussing the pmic that contains the regulators which serve as power-domain power-supplies: https://lore.kernel.org/linux-pm/573166b75e524517782471c2b7f96e03fd93d175.camel@puri.sm/T/ So this enables boards to use power-supply-needs-irq and fix systemd suspend and resume. Possibly one can find more changes this property should configure. They can be added later when testing them. Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm> --- drivers/base/power/domain.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)