| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <fd46f443-b1f9-4f82-8d73-117cda093315@imgtec.com>
Date: Fri, 31 Jan 2025 15:39:31 +0000
From: Matt Coster <Matt.Coster@...tec.com>
To: Michal Wilczynski <m.wilczynski@...sung.com>,
"mturquette@...libre.com"
<mturquette@...libre.com>,
"sboyd@...nel.org" <sboyd@...nel.org>,
"robh@...nel.org" <robh@...nel.org>,
"krzk+dt@...nel.org"
<krzk+dt@...nel.org>,
"conor+dt@...nel.org" <conor+dt@...nel.org>,
"drew@...7.com" <drew@...7.com>,
"guoren@...nel.org" <guoren@...nel.org>,
"wefu@...hat.com" <wefu@...hat.com>,
"jassisinghbrar@...il.com"
<jassisinghbrar@...il.com>,
"paul.walmsley@...ive.com"
<paul.walmsley@...ive.com>,
"palmer@...belt.com" <palmer@...belt.com>,
"aou@...s.berkeley.edu" <aou@...s.berkeley.edu>,
Frank Binns
<Frank.Binns@...tec.com>,
"maarten.lankhorst@...ux.intel.com"
<maarten.lankhorst@...ux.intel.com>,
"mripard@...nel.org"
<mripard@...nel.org>,
"tzimmermann@...e.de" <tzimmermann@...e.de>,
"airlied@...il.com" <airlied@...il.com>,
"simona@...ll.ch" <simona@...ll.ch>,
"ulf.hansson@...aro.org" <ulf.hansson@...aro.org>,
"jszhang@...nel.org"
<jszhang@...nel.org>,
"p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
"m.szyprowski@...sung.com" <m.szyprowski@...sung.com>
CC: "linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-riscv@...ts.infradead.org" <linux-riscv@...ts.infradead.org>,
"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>
Subject: Re: [PATCH v4 10/18] drm/imagination: Add reset controller support
for GPU initialization
On 28/01/2025 19:48, Michal Wilczynski wrote:
> Certain platforms, such as the T-Head TH1520 and Banana Pi BPI-F3,
> require a controlled GPU reset sequence during the power-up procedure
> to ensure proper initialization. Without this reset, the GPU may remain
> in an undefined state, potentially leading to stability or performance
> issues.
Can you reword this to clarify that _all_ IMG Rogue GPUs have a reset
line that participates in the power-up sequence but some SoCs handle
this in silicon and/or firmware without exposing the reset line
directly (as the currently supported TI SoC does).
>
> This commit integrates a dedicated reset controller within the
> drm/imagination driver. By doing so, the driver can coordinate the
> necessary reset operations as part of the normal GPU bring-up process,
> improving reliability and ensuring that the hardware is ready for
> operation.
>
> Signed-off-by: Michal Wilczynski <m.wilczynski@...sung.com>
> ---
> drivers/gpu/drm/imagination/pvr_device.c | 21 +++++++++++++++++++++
> drivers/gpu/drm/imagination/pvr_device.h | 9 +++++++++
> drivers/gpu/drm/imagination/pvr_power.c | 12 +++++++++++-
> 3 files changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/imagination/pvr_device.c b/drivers/gpu/drm/imagination/pvr_device.c
> index 1704c0268589..ef73e95157ee 100644
> --- a/drivers/gpu/drm/imagination/pvr_device.c
> +++ b/drivers/gpu/drm/imagination/pvr_device.c
> @@ -25,6 +25,7 @@
> #include <linux/interrupt.h>
> #include <linux/platform_device.h>
> #include <linux/pm_runtime.h>
> +#include <linux/reset.h>
> #include <linux/slab.h>
> #include <linux/stddef.h>
> #include <linux/types.h>
> @@ -120,6 +121,21 @@ static int pvr_device_clk_init(struct pvr_device *pvr_dev)
> return 0;
> }
>
> +static int pvr_device_reset_init(struct pvr_device *pvr_dev)
> +{
> + struct drm_device *drm_dev = from_pvr_device(pvr_dev);
> + struct reset_control *reset;
> +
> + reset = devm_reset_control_get_optional_exclusive(drm_dev->dev, NULL);
> + if (IS_ERR(reset))
> + return dev_err_probe(drm_dev->dev, PTR_ERR(reset),
> + "failed to get gpu reset line\n");
> +
> + pvr_dev->reset = reset;
> +
> + return 0;
> +}
> +
> /**
> * pvr_device_process_active_queues() - Process all queue related events.
> * @pvr_dev: PowerVR device to check
> @@ -509,6 +525,11 @@ pvr_device_init(struct pvr_device *pvr_dev)
> if (err)
> return err;
>
> + /* Get the reset line for the GPU */
> + err = pvr_device_reset_init(pvr_dev);
> + if (err)
> + return err;
> +
> /* Explicitly power the GPU so we can access control registers before the FW is booted. */
> err = pm_runtime_resume_and_get(dev);
> if (err)
> diff --git a/drivers/gpu/drm/imagination/pvr_device.h b/drivers/gpu/drm/imagination/pvr_device.h
> index 6d0dfacb677b..f6576c08111c 100644
> --- a/drivers/gpu/drm/imagination/pvr_device.h
> +++ b/drivers/gpu/drm/imagination/pvr_device.h
> @@ -131,6 +131,15 @@ struct pvr_device {
> */
> struct clk *mem_clk;
>
> + /**
> + * @reset: Optional reset line.
> + *
> + * This may be used on some platforms to provide a reset line that needs to be de-asserted
> + * after power-up procedure. It would also need to be asserted after the power-down
> + * procedure.
> + */
> + struct reset_control *reset;
> +
> /** @irq: IRQ number. */
> int irq;
>
> diff --git a/drivers/gpu/drm/imagination/pvr_power.c b/drivers/gpu/drm/imagination/pvr_power.c
> index ba7816fd28ec..e39460d594bd 100644
> --- a/drivers/gpu/drm/imagination/pvr_power.c
> +++ b/drivers/gpu/drm/imagination/pvr_power.c
> @@ -15,6 +15,7 @@
> #include <linux/mutex.h>
> #include <linux/platform_device.h>
> #include <linux/pm_runtime.h>
> +#include <linux/reset.h>
> #include <linux/timer.h>
> #include <linux/types.h>
> #include <linux/workqueue.h>
> @@ -252,6 +253,8 @@ pvr_power_device_suspend(struct device *dev)
> clk_disable_unprepare(pvr_dev->sys_clk);
> clk_disable_unprepare(pvr_dev->core_clk);
>
> + err = reset_control_assert(pvr_dev->reset);
> +
> err_drm_dev_exit:
> drm_dev_exit(idx);
>
> @@ -282,16 +285,23 @@ pvr_power_device_resume(struct device *dev)
> if (err)
> goto err_sys_clk_disable;
This is where I'd expect to see the 32 cycle delay that's currently in
P9 ("reset: thead: Add TH1520 reset controller driver").
If it turns out that delay is required in the reset driver, would you be
opposed to adding it here as well? It's a very small amount of time and
would make this codepath more versatile to future reset controllers.
Cheers,
Matt
>
> + err = reset_control_deassert(pvr_dev->reset);
> + if (err)
> + goto err_mem_clk_disable;
> +
> if (pvr_dev->fw_dev.booted) {
> err = pvr_power_fw_enable(pvr_dev);
> if (err)
> - goto err_mem_clk_disable;
> + goto err_reset_assert;
> }
>
> drm_dev_exit(idx);
>
> return 0;
>
> +err_reset_assert:
> + reset_control_assert(pvr_dev->reset);
> +
> err_mem_clk_disable:
> clk_disable_unprepare(pvr_dev->mem_clk);
>
--
Matt Coster
E: matt.coster@...tec.com
Download attachment "OpenPGP_signature.asc" of type "application/pgp-signature" (237 bytes)
Powered by blists - more mailing lists