| Message ID | 20211208134725.3328030-1-martin.kepplinger@puri.sm (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hack: soc: imx: gpcv2: avoid unbalanced powering off of one device | expand |
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. 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);
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); > >
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);
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). Makes sense functinally, but I wonder if we could implement it a bit clearer? Anyway I'm also not sure whether imx8mq might be different than your imx8mm system. 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? thanks a lot for working on this! martin --- drivers/soc/imx/gpcv2.c | 3 +++ 1 file changed, 3 insertions(+)