On Mon, 31 Mar 2025 11:32:58 +0100
Steven Price <steven.pr...@arm.com> wrote:
> On 27/03/2025 12:36, Andre Przywara wrote:
> > On Thu, 13 Mar 2025 00:23:18 +0100
> > Philippe Simons <simons.phili...@gmail.com> wrote:
> >
> > Hi Rob, Boris, Steven,
> >
> >> When the GPU is the only device attached to a single power domain,
> >> core genpd disable and enable it when gpu enter and leave runtime suspend.
> >>
> >> Some power-domain requires a sequence before disabled,
> >> and the reverse when enabled.
> >>
> >> Add PM flags for CLK and RST, and implement in
> >> panfrost_device_runtime_suspend/resume.
> >
> > So some Mali configuration and integration manual I am looking at says
> > that this sequence should be always observed, as the powerdown sequence
> > would include disabling the clocks first, then asserting the reset, then
> > turning the power switches off (and the inverse sequence on powerup).
> >
> > So should we make this unconditional, not depending on implementation
> > specific flags?
>
> I think you're right, this probably should be unconditional. My only
> reservation is that "it works" currently and we'd need to test this
> doesn't cause regressions on existing platforms. So unless someone with
> a reasonable board farm is able to do that testing I think this solution
> is reasonable. So:
>
> Reviewed-by: Steven Price <steven.pr...@arm.com>
>
> > And also I am wondering if panfrost_device_init() gets this wrong as well?
> > As I see it enabling clock first, then reset, then pm_domain, where it
> > should be exactly the opposite?
>
> I agree, that looks very wrong - the power needs to be enabled before
> reset is deasserted. I'm somewhat surprised we've got away with that.
> Fancy writing a patch? ;)
Seems like Philippe volunteered ;-) (on IRC). Actually we tried this
already some weeks ago, but this alone didn't help. I think it's that this
power domain in panfrost_device_init() doesn't trigger for some reason,
but only in suspend()/resume(), so he came up with this patch here.
Thanks!
Andre
> Steve
>
> > Cheers,
> > Andre
> >
> >>
> >> Signed-off-by: Philippe Simons <simons.phili...@gmail.com>
> >> ---
> >> drivers/gpu/drm/panfrost/panfrost_device.c | 37 ++++++++++++++++++++++
> >> drivers/gpu/drm/panfrost/panfrost_device.h | 4 +++
> >> 2 files changed, 41 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c
> >> b/drivers/gpu/drm/panfrost/panfrost_device.c
> >> index a45e4addcc19..189ad2ad2b32 100644
> >> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> >> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> >> @@ -406,11 +406,38 @@ void panfrost_device_reset(struct panfrost_device
> >> *pfdev)
> >> static int panfrost_device_runtime_resume(struct device *dev)
> >> {
> >> struct panfrost_device *pfdev = dev_get_drvdata(dev);
> >> + int ret;
> >> +
> >> + if (pfdev->comp->pm_features & BIT(GPU_PM_RT_RST_ASRT)) {
> >> + ret = reset_control_deassert(pfdev->rstc);
> >> + if (ret)
> >> + return ret;
> >> + }
> >> +
> >> + if (pfdev->comp->pm_features & BIT(GPU_PM_RT_CLK_DIS)) {
> >> + ret = clk_enable(pfdev->clock);
> >> + if (ret)
> >> + goto err_clk;
> >> +
> >> + if (pfdev->bus_clock) {
> >> + ret = clk_enable(pfdev->bus_clock);
> >> + if (ret)
> >> + goto err_bus_clk;
> >> + }
> >> + }
> >>
> >> panfrost_device_reset(pfdev);
> >> panfrost_devfreq_resume(pfdev);
> >>
> >> return 0;
> >> +
> >> +err_bus_clk:
> >> + if (pfdev->comp->pm_features & BIT(GPU_PM_RT_CLK_DIS))
> >> + clk_disable(pfdev->clock);
> >> +err_clk:
> >> + if (pfdev->comp->pm_features & BIT(GPU_PM_RT_RST_ASRT))
> >> + reset_control_assert(pfdev->rstc);
> >> + return ret;
> >> }
> >>
> >> static int panfrost_device_runtime_suspend(struct device *dev)
> >> @@ -426,6 +453,16 @@ static int panfrost_device_runtime_suspend(struct
> >> device *dev)
> >> panfrost_gpu_suspend_irq(pfdev);
> >> panfrost_gpu_power_off(pfdev);
> >>
> >> + if (pfdev->comp->pm_features & BIT(GPU_PM_RT_CLK_DIS)) {
> >> + if (pfdev->bus_clock)
> >> + clk_disable(pfdev->bus_clock);
> >> +
> >> + clk_disable(pfdev->clock);
> >> + }
> >> +
> >> + if (pfdev->comp->pm_features & BIT(GPU_PM_RT_RST_ASRT))
> >> + reset_control_assert(pfdev->rstc);
> >> +
> >> return 0;
> >> }
> >>
> >> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h
> >> b/drivers/gpu/drm/panfrost/panfrost_device.h
> >> index cffcb0ac7c11..f372d4819262 100644
> >> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
> >> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
> >> @@ -36,10 +36,14 @@ enum panfrost_drv_comp_bits {
> >> * enum panfrost_gpu_pm - Supported kernel power management features
> >> * @GPU_PM_CLK_DIS: Allow disabling clocks during system suspend
> >> * @GPU_PM_VREG_OFF: Allow turning off regulators during system suspend
> >> + * @GPU_PM_RT_CLK_DIS: Allow disabling clocks during system runtime
> >> suspend
> >> + * @GPU_PM_RST_ASRT: Allow asserting the reset control during runtime
> >> suspend
> >> */
> >> enum panfrost_gpu_pm {
> >> GPU_PM_CLK_DIS,
> >> GPU_PM_VREG_OFF,
> >> + GPU_PM_RT_CLK_DIS,
> >> + GPU_PM_RT_RST_ASRT
> >> };
> >>
> >> struct panfrost_features {
> >
>
