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 A6DD41DF97A for ; Mon, 31 Mar 2025 10:33:08 +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=1743417191; cv=none; b=fuxQMS4e3yfYRgGqJDZqJGLFZIbMCYu60LIHey6z/jHueHqRVSOUKl/+izxMOUliqNJ5K2UT0WerSpzBKqv0WtCQ5Bizyx02qmMhUsKxsFeCzrubJpqGYm0M/DCm+TVdw0AHAVvMMhX1U7yiz6yDHS5S//YoRvdLLbKqZTQUEaE= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1743417191; c=relaxed/simple; bh=HkkiXoc5dyS5CPC7iq6goh7COI2oAKYB1Qu7Dw8b40E=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=UBu8MYqqX7JVb/tF1IDqrYrgxeU5fSZKa4GSDtzCuFVGgwN7qcvStxQUGA8mhEuhC5j3te+e2flGSkh5nYfjFOg8SoJEPVu1sby1hiq2grJxhS7x5z4k3dogjapNRvjUZ3X3D2N05XoXMi0mYRrm9GzaiihZcxC5kcb5IGiIWJs= 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 5DE561F02; Mon, 31 Mar 2025 03:33:06 -0700 (PDT) Received: from [10.57.15.213] (unknown [10.57.15.213]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 2E24F3F63F; Mon, 31 Mar 2025 03:33:00 -0700 (PDT) Message-ID: Date: Mon, 31 Mar 2025 11:32:58 +0100 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 1/2] drm/panfrost: Add PM runtime flags To: Andre Przywara , Boris Brezillon , Rob Herring Cc: Philippe Simons , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , David Airlie , Simona Vetter , Philipp Zabel , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, linux-sunxi@lists.linux.dev, =?UTF-8?Q?Jernej_=C5=A0krabec?= References: <20250312232319.25712-1-simons.philippe@gmail.com> <20250312232319.25712-2-simons.philippe@gmail.com> <20250327123628.3d33c68e@donnerap.manchester.arm.com> From: Steven Price Content-Language: en-GB In-Reply-To: <20250327123628.3d33c68e@donnerap.manchester.arm.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit 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? ;) 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 { >