| Message ID | 20250227170012.124768-5-marex@denx.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: dts: imx95: Add support for Mali G310 GPU | expand |
Hi Marek, Am Donnerstag, 27. Februar 2025, 17:58:04 CET schrieb Marek Vasut: > The instance of the GPU populated in Freescale i.MX95 does require > release from reset by writing into a single GPUMIX block controller > GPURESET register bit 0. Implement support for one optional reset. > > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: Boris Brezillon <boris.brezillon@collabora.com> > Cc: Conor Dooley <conor+dt@kernel.org> > Cc: David Airlie <airlied@gmail.com> > Cc: Fabio Estevam <festevam@gmail.com> > Cc: Krzysztof Kozlowski <krzk+dt@kernel.org> > Cc: Liviu Dudau <liviu.dudau@arm.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Maxime Ripard <mripard@kernel.org> > Cc: Pengutronix Kernel Team <kernel@pengutronix.de> > Cc: Philipp Zabel <p.zabel@pengutronix.de> > Cc: Rob Herring <robh@kernel.org> > Cc: Sascha Hauer <s.hauer@pengutronix.de> > Cc: Sebastian Reichel <sre@kernel.org> > Cc: Shawn Guo <shawnguo@kernel.org> > Cc: Simona Vetter <simona@ffwll.ch> > Cc: Steven Price <steven.price@arm.com> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: devicetree@vger.kernel.org > Cc: dri-devel@lists.freedesktop.org > Cc: imx@lists.linux.dev > Cc: linux-arm-kernel@lists.infradead.org > --- > drivers/gpu/drm/panthor/Kconfig | 1 + > drivers/gpu/drm/panthor/panthor_device.c | 23 +++++++++++++++++++++++ > drivers/gpu/drm/panthor/panthor_device.h | 3 +++ > 3 files changed, 27 insertions(+) > > diff --git a/drivers/gpu/drm/panthor/Kconfig b/drivers/gpu/drm/panthor/Kconfig > index 55b40ad07f3b0..ab62bd6a0750f 100644 > --- a/drivers/gpu/drm/panthor/Kconfig > +++ b/drivers/gpu/drm/panthor/Kconfig > @@ -14,6 +14,7 @@ config DRM_PANTHOR > select IOMMU_IO_PGTABLE_LPAE > select IOMMU_SUPPORT > select PM_DEVFREQ > + select RESET_SIMPLE if SOC_IMX9 > help > DRM driver for ARM Mali CSF-based GPUs. > > diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c > index a9da1d1eeb707..51ee9cae94504 100644 > --- a/drivers/gpu/drm/panthor/panthor_device.c > +++ b/drivers/gpu/drm/panthor/panthor_device.c > @@ -64,6 +64,17 @@ static int panthor_clk_init(struct panthor_device *ptdev) > return 0; > } > > +static int panthor_reset_init(struct panthor_device *ptdev) > +{ > + ptdev->resets = devm_reset_control_get_optional_exclusive_deasserted(ptdev->base.dev, NULL); If the description as a write-once register is true, wouldn't this already write to it? > + if (IS_ERR(ptdev->resets)) > + return dev_err_probe(ptdev->base.dev, > + PTR_ERR(ptdev->resets), > + "get reset failed"); > + > + return 0; > +} > + > void panthor_device_unplug(struct panthor_device *ptdev) > { > /* This function can be called from two different path: the reset work > @@ -217,6 +228,10 @@ int panthor_device_init(struct panthor_device *ptdev) > if (ret) > return ret; > > + ret = panthor_reset_init(ptdev); > + if (ret) > + return ret; > + > ret = panthor_devfreq_init(ptdev); > if (ret) > return ret; > @@ -470,6 +485,10 @@ int panthor_device_resume(struct device *dev) > if (ret) > goto err_disable_stacks_clk; > > + ret = reset_control_deassert(ptdev->resets); > + if (ret) > + goto err_disable_coregroup_clk; > + This wouldn't work at all on a write-once register, no? Same for resume. Best regards Alexander > panthor_devfreq_resume(ptdev); > > if (panthor_device_is_initialized(ptdev) && > @@ -512,6 +531,9 @@ int panthor_device_resume(struct device *dev) > > err_suspend_devfreq: > panthor_devfreq_suspend(ptdev); > + reset_control_assert(ptdev->resets); > + > +err_disable_coregroup_clk: > clk_disable_unprepare(ptdev->clks.coregroup); > > err_disable_stacks_clk: > @@ -563,6 +585,7 @@ int panthor_device_suspend(struct device *dev) > > panthor_devfreq_suspend(ptdev); > > + reset_control_assert(ptdev->resets); > clk_disable_unprepare(ptdev->clks.coregroup); > clk_disable_unprepare(ptdev->clks.stacks); > clk_disable_unprepare(ptdev->clks.core); > diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h > index da6574021664b..fea3a05778e2e 100644 > --- a/drivers/gpu/drm/panthor/panthor_device.h > +++ b/drivers/gpu/drm/panthor/panthor_device.h > @@ -111,6 +111,9 @@ struct panthor_device { > struct clk *coregroup; > } clks; > > + /** @resets: GPU reset. */ > + struct reset_control *resets; Your commit message says "one optional reset", so I would name this just reset. > + > /** @coherent: True if the CPU/GPU are memory coherent. */ > bool coherent; > >
On 2/28/25 11:06 AM, Alexander Stein wrote: Hi, >> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c >> index a9da1d1eeb707..51ee9cae94504 100644 >> --- a/drivers/gpu/drm/panthor/panthor_device.c >> +++ b/drivers/gpu/drm/panthor/panthor_device.c >> @@ -64,6 +64,17 @@ static int panthor_clk_init(struct panthor_device *ptdev) >> return 0; >> } >> >> +static int panthor_reset_init(struct panthor_device *ptdev) >> +{ >> + ptdev->resets = devm_reset_control_get_optional_exclusive_deasserted(ptdev->base.dev, NULL); > > If the description as a write-once register is true, wouldn't this > already write to it? It would. I believe the reset handling in the GPU driver should be generic. The reset has to be deasserted here to access the GPU registers. The question is, whether the GPUMIX GPURESET bit should be modeled as simple reset, even if it really behaves as a set-and-never-can-be-cleared latched bit. >> + if (IS_ERR(ptdev->resets)) >> + return dev_err_probe(ptdev->base.dev, >> + PTR_ERR(ptdev->resets), >> + "get reset failed"); >> + >> + return 0; >> +} >> + >> void panthor_device_unplug(struct panthor_device *ptdev) >> { >> /* This function can be called from two different path: the reset work >> @@ -217,6 +228,10 @@ int panthor_device_init(struct panthor_device *ptdev) >> if (ret) >> return ret; >> >> + ret = panthor_reset_init(ptdev); >> + if (ret) >> + return ret; >> + >> ret = panthor_devfreq_init(ptdev); >> if (ret) >> return ret; >> @@ -470,6 +485,10 @@ int panthor_device_resume(struct device *dev) >> if (ret) >> goto err_disable_stacks_clk; >> >> + ret = reset_control_deassert(ptdev->resets); >> + if (ret) >> + goto err_disable_coregroup_clk; >> + > > This wouldn't work at all on a write-once register, no? Same for resume. See above and also my reply to 2/9 .
Hi Marek,
kernel test robot noticed the following build warnings:
[auto build test WARNING on shawnguo/for-next]
[also build test WARNING on robh/for-next arm64/for-next/core kvmarm/next rockchip/for-next soc/for-next linus/master v6.14-rc4 next-20250228]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Marek-Vasut/dt-bindings-reset-imx95-gpu-blk-ctrl-Document-Freescale-i-MX95-GPU-reset/20250228-011031
base: https://git.kernel.org/pub/scm/linux/kernel/git/shawnguo/linux.git for-next
patch link: https://lore.kernel.org/r/20250227170012.124768-5-marex%40denx.de
patch subject: [PATCH 4/9] drm/panthor: Implement optional reset
config: x86_64-kismet-CONFIG_RESET_SIMPLE-CONFIG_DRM_PANTHOR-0-0 (https://download.01.org/0day-ci/archive/20250302/202503020521.MBUsoVte-lkp@intel.com/config)
reproduce: (https://download.01.org/0day-ci/archive/20250302/202503020521.MBUsoVte-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503020521.MBUsoVte-lkp@intel.com/
kismet warnings: (new ones prefixed by >>)
>> kismet: WARNING: unmet direct dependencies detected for RESET_SIMPLE when selected by DRM_PANTHOR
WARNING: unmet direct dependencies detected for RESET_SIMPLE
Depends on [n]: RESET_CONTROLLER [=n] && HAS_IOMEM [=y]
Selected by [y]:
- DRM_PANTHOR [=y] && HAS_IOMEM [=y] && DRM [=y] && (ARM || ARM64 || COMPILE_TEST [=y]) && !GENERIC_ATOMIC64 [=n] && MMU [=y] && SOC_IMX9 [=y]
diff --git a/drivers/gpu/drm/panthor/Kconfig b/drivers/gpu/drm/panthor/Kconfig index 55b40ad07f3b0..ab62bd6a0750f 100644 --- a/drivers/gpu/drm/panthor/Kconfig +++ b/drivers/gpu/drm/panthor/Kconfig @@ -14,6 +14,7 @@ config DRM_PANTHOR select IOMMU_IO_PGTABLE_LPAE select IOMMU_SUPPORT select PM_DEVFREQ + select RESET_SIMPLE if SOC_IMX9 help DRM driver for ARM Mali CSF-based GPUs. diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c index a9da1d1eeb707..51ee9cae94504 100644 --- a/drivers/gpu/drm/panthor/panthor_device.c +++ b/drivers/gpu/drm/panthor/panthor_device.c @@ -64,6 +64,17 @@ static int panthor_clk_init(struct panthor_device *ptdev) return 0; } +static int panthor_reset_init(struct panthor_device *ptdev) +{ + ptdev->resets = devm_reset_control_get_optional_exclusive_deasserted(ptdev->base.dev, NULL); + if (IS_ERR(ptdev->resets)) + return dev_err_probe(ptdev->base.dev, + PTR_ERR(ptdev->resets), + "get reset failed"); + + return 0; +} + void panthor_device_unplug(struct panthor_device *ptdev) { /* This function can be called from two different path: the reset work @@ -217,6 +228,10 @@ int panthor_device_init(struct panthor_device *ptdev) if (ret) return ret; + ret = panthor_reset_init(ptdev); + if (ret) + return ret; + ret = panthor_devfreq_init(ptdev); if (ret) return ret; @@ -470,6 +485,10 @@ int panthor_device_resume(struct device *dev) if (ret) goto err_disable_stacks_clk; + ret = reset_control_deassert(ptdev->resets); + if (ret) + goto err_disable_coregroup_clk; + panthor_devfreq_resume(ptdev); if (panthor_device_is_initialized(ptdev) && @@ -512,6 +531,9 @@ int panthor_device_resume(struct device *dev) err_suspend_devfreq: panthor_devfreq_suspend(ptdev); + reset_control_assert(ptdev->resets); + +err_disable_coregroup_clk: clk_disable_unprepare(ptdev->clks.coregroup); err_disable_stacks_clk: @@ -563,6 +585,7 @@ int panthor_device_suspend(struct device *dev) panthor_devfreq_suspend(ptdev); + reset_control_assert(ptdev->resets); clk_disable_unprepare(ptdev->clks.coregroup); clk_disable_unprepare(ptdev->clks.stacks); clk_disable_unprepare(ptdev->clks.core); diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h index da6574021664b..fea3a05778e2e 100644 --- a/drivers/gpu/drm/panthor/panthor_device.h +++ b/drivers/gpu/drm/panthor/panthor_device.h @@ -111,6 +111,9 @@ struct panthor_device { struct clk *coregroup; } clks; + /** @resets: GPU reset. */ + struct reset_control *resets; + /** @coherent: True if the CPU/GPU are memory coherent. */ bool coherent;
The instance of the GPU populated in Freescale i.MX95 does require release from reset by writing into a single GPUMIX block controller GPURESET register bit 0. Implement support for one optional reset. Signed-off-by: Marek Vasut <marex@denx.de> --- Cc: Boris Brezillon <boris.brezillon@collabora.com> Cc: Conor Dooley <conor+dt@kernel.org> Cc: David Airlie <airlied@gmail.com> Cc: Fabio Estevam <festevam@gmail.com> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org> Cc: Liviu Dudau <liviu.dudau@arm.com> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Maxime Ripard <mripard@kernel.org> Cc: Pengutronix Kernel Team <kernel@pengutronix.de> Cc: Philipp Zabel <p.zabel@pengutronix.de> Cc: Rob Herring <robh@kernel.org> Cc: Sascha Hauer <s.hauer@pengutronix.de> Cc: Sebastian Reichel <sre@kernel.org> Cc: Shawn Guo <shawnguo@kernel.org> Cc: Simona Vetter <simona@ffwll.ch> Cc: Steven Price <steven.price@arm.com> Cc: Thomas Zimmermann <tzimmermann@suse.de> Cc: devicetree@vger.kernel.org Cc: dri-devel@lists.freedesktop.org Cc: imx@lists.linux.dev Cc: linux-arm-kernel@lists.infradead.org --- drivers/gpu/drm/panthor/Kconfig | 1 + drivers/gpu/drm/panthor/panthor_device.c | 23 +++++++++++++++++++++++ drivers/gpu/drm/panthor/panthor_device.h | 3 +++ 3 files changed, 27 insertions(+)