From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by smtp.subspace.kernel.org (Postfix) with ESMTP id B75781E0E13 for ; Mon, 31 Mar 2025 11:13:52 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.140.110.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1743419634; cv=none; b=NYbf+C5Eshxts9pQ/BXwa1hPPY3phMSbk33ZV8N8R2LaBD9+kjiTy7YpExOnZwhrKsZnLJOAAkSdRvSMYtcHE2ORtjXsEWSnqfXBBPrXVgjgV8V6hWg9zqjYquvhxhxw+uq/cWAvK7uQq1kq3OhRWRD/zs1rZ2nazw04CUGqYdw= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1743419634; c=relaxed/simple; bh=HC26LdIq2V2ccwTjv1vB6lt05uNFtHGl0Y+NFc5qdMg=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=EEcJFwytkboFBRy8eLHuo/vJfIotYSMtCSpqAaJK+ZPuV5PuD5mrrM8mAsuTDoYv111kcoUFOvakrXdzC/GPuPSp0hEP8iJ6hdI3aowYF+wL5CO4BaaWGcn2WX/zKhp5hyHb3JFBbCJpA4qpFcvhGyi33anS2/yyWq4V62MhA54= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com; spf=pass smtp.mailfrom=arm.com; arc=none smtp.client-ip=217.140.110.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id DC9A31F02; Mon, 31 Mar 2025 04:13:55 -0700 (PDT) Received: from donnerap.manchester.arm.com (usa-sjc-imap-foss1.foss.arm.com [10.121.207.14]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 50AA33F694; Mon, 31 Mar 2025 04:13:50 -0700 (PDT) Date: Mon, 31 Mar 2025 12:13:47 +0100 From: Andre Przywara To: Steven Price Cc: Boris Brezillon , Rob Herring , Philippe Simons , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , David Airlie , Simona Vetter , Philipp Zabel , , , , Jernej =?UTF-8?B?xaBrcmFiZWM=?= Subject: Re: [PATCH 1/2] drm/panfrost: Add PM runtime flags Message-ID: <20250331121347.150d4f4f@donnerap.manchester.arm.com> In-Reply-To: References: <20250312232319.25712-1-simons.philippe@gmail.com> <20250312232319.25712-2-simons.philippe@gmail.com> <20250327123628.3d33c68e@donnerap.manchester.arm.com> Organization: ARM X-Mailer: Claws Mail 3.18.0 (GTK+ 2.24.32; aarch64-unknown-linux-gnu) Precedence: bulk X-Mailing-List: linux-sunxi@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit On Mon, 31 Mar 2025 11:32:58 +0100 Steven Price wrote: > On 27/03/2025 12:36, Andre Przywara wrote: > > On Thu, 13 Mar 2025 00:23:18 +0100 > > Philippe Simons 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 > > > 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 > >> --- > >> 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 { > > >