| Message ID | 20250321200625.132494-5-marex@denx.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: dts: imx95: Add support for Mali G310 GPU | expand |
On Fri, 21 Mar 2025 21:05:54 +0100 Marek Vasut <marex@denx.de> wrote: > 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 > --- > V2: Drop the select RESET_SIMPLE from Kconfig > --- > drivers/gpu/drm/panthor/panthor_device.c | 23 +++++++++++++++++++++++ > drivers/gpu/drm/panthor/panthor_device.h | 3 +++ > 2 files changed, 26 insertions(+) > > 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); Hm, that might be the cause of the fast reset issue (which is a fast resume more than a fast reset BTW): if you re-assert the reset line on runtime suspend, I guess this causes a full GPU reset, and the MCU ends up in a state where it needs a slow reset (all data sections reset to their initial state). Can you try to move the reset_control_[de]assert to the unplug/init functions? > 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; >
On 3/24/25 9:43 AM, Boris Brezillon wrote: [...] >> @@ -563,6 +585,7 @@ int panthor_device_suspend(struct device *dev) >> >> panthor_devfreq_suspend(ptdev); >> >> + reset_control_assert(ptdev->resets); > > Hm, that might be the cause of the fast reset issue (which is a fast > resume more than a fast reset BTW): if you re-assert the reset line on > runtime suspend, I guess this causes a full GPU reset, and the MCU ends > up in a state where it needs a slow reset (all data sections reset to > their initial state). Can you try to move the reset_control_[de]assert > to the unplug/init functions? The reset on the MX95 is not really a reset, it is clear-only set-never-again bit which goes only one way, the "unreset" way, so I don't think this has any impact. Also, I commented this out already and it made no difference. I will give the second part of our suggestion a try in the next few days though, and also try the updated downstream firmware blobs (sigh).
On 3/24/25 9:43 AM, Boris Brezillon wrote: [...] >> @@ -563,6 +585,7 @@ int panthor_device_suspend(struct device *dev) >> >> panthor_devfreq_suspend(ptdev); >> >> + reset_control_assert(ptdev->resets); > > Hm, that might be the cause of the fast reset issue (which is a fast > resume more than a fast reset BTW): if you re-assert the reset line on > runtime suspend, I guess this causes a full GPU reset, and the MCU ends > up in a state where it needs a slow reset (all data sections reset to > their initial state). Can you try to move the reset_control_[de]assert > to the unplug/init functions? Is it correct to assume , that if I remove all reset_control_assert() calls (and keep only the _deassert() calls), the slow resume problem should go away too ?
On Tue, 25 Mar 2025 00:37:59 +0100 Marek Vasut <marex@denx.de> wrote: > On 3/24/25 9:43 AM, Boris Brezillon wrote: > > [...] > > >> @@ -563,6 +585,7 @@ int panthor_device_suspend(struct device *dev) > >> > >> panthor_devfreq_suspend(ptdev); > >> > >> + reset_control_assert(ptdev->resets); > > > > Hm, that might be the cause of the fast reset issue (which is a fast > > resume more than a fast reset BTW): if you re-assert the reset line on > > runtime suspend, I guess this causes a full GPU reset, and the MCU ends > > up in a state where it needs a slow reset (all data sections reset to > > their initial state). Can you try to move the reset_control_[de]assert > > to the unplug/init functions? > Is it correct to assume , that if I remove all reset_control_assert() > calls (and keep only the _deassert() calls), the slow resume problem > should go away too ? Yeah, dropping the _assert()s should do the trick.
On 3/25/25 8:43 AM, Boris Brezillon wrote: > On Tue, 25 Mar 2025 00:37:59 +0100 > Marek Vasut <marex@denx.de> wrote: > >> On 3/24/25 9:43 AM, Boris Brezillon wrote: >> >> [...] >> >>>> @@ -563,6 +585,7 @@ int panthor_device_suspend(struct device *dev) >>>> >>>> panthor_devfreq_suspend(ptdev); >>>> >>>> + reset_control_assert(ptdev->resets); >>> >>> Hm, that might be the cause of the fast reset issue (which is a fast >>> resume more than a fast reset BTW): if you re-assert the reset line on >>> runtime suspend, I guess this causes a full GPU reset, and the MCU ends >>> up in a state where it needs a slow reset (all data sections reset to >>> their initial state). Can you try to move the reset_control_[de]assert >>> to the unplug/init functions? >> Is it correct to assume , that if I remove all reset_control_assert() >> calls (and keep only the _deassert() calls), the slow resume problem >> should go away too ? > > Yeah, dropping the _assert()s should do the trick. Hmmm, no, that does not help. I was hoping maybe NXP can chime in and suggest something too ?
On Mo, 2025-03-24 at 20:05 +0100, Marek Vasut wrote: > On 3/24/25 9:43 AM, Boris Brezillon wrote: > > [...] > > > > @@ -563,6 +585,7 @@ int panthor_device_suspend(struct device *dev) > > > > > > panthor_devfreq_suspend(ptdev); > > > > > > + reset_control_assert(ptdev->resets); > > > > Hm, that might be the cause of the fast reset issue (which is a fast > > resume more than a fast reset BTW): if you re-assert the reset line on > > runtime suspend, I guess this causes a full GPU reset, and the MCU ends > > up in a state where it needs a slow reset (all data sections reset to > > their initial state). Can you try to move the reset_control_[de]assert > > to the unplug/init functions? > The reset on the MX95 is not really a reset, it is clear-only > set-never-again bit which goes only one way, the "unreset" way, so I > don't think this has any impact. This sounds like the bit is initially set to 1 (reset asserted) and can be cleared by writing 0 (once, to deassert the reset). But in the reset-simple patch it looks to be the other way around (active_low = true)? If the reset control can't be asserted again on this hardware, it would be better to have custom driver that doesn't implement the .assert() callback at all. regards Philipp
On 3/25/25 3:12 PM, Philipp Zabel wrote: > On Mo, 2025-03-24 at 20:05 +0100, Marek Vasut wrote: >> On 3/24/25 9:43 AM, Boris Brezillon wrote: >> >> [...] >> >>>> @@ -563,6 +585,7 @@ int panthor_device_suspend(struct device *dev) >>>> >>>> panthor_devfreq_suspend(ptdev); >>>> >>>> + reset_control_assert(ptdev->resets); >>> >>> Hm, that might be the cause of the fast reset issue (which is a fast >>> resume more than a fast reset BTW): if you re-assert the reset line on >>> runtime suspend, I guess this causes a full GPU reset, and the MCU ends >>> up in a state where it needs a slow reset (all data sections reset to >>> their initial state). Can you try to move the reset_control_[de]assert >>> to the unplug/init functions? >> The reset on the MX95 is not really a reset, it is clear-only >> set-never-again bit which goes only one way, the "unreset" way, so I >> don't think this has any impact. > > This sounds like the bit is initially set to 1 (reset asserted) and can > be cleared by writing 0 (once, to deassert the reset). But in the > reset-simple patch it looks to be the other way around (active_low = > true)? > > If the reset control can't be asserted again on this hardware, it would > be better to have custom driver that doesn't implement the .assert() > callback at all. Maybe the reset-simple driver can be extended with some mode of operation like this instead , to make it skip assert once deasserted , for specific reset controllers ?
On Tue, 25 Mar 2025 14:50:32 +0100 Marek Vasut <marex@denx.de> wrote: > On 3/25/25 8:43 AM, Boris Brezillon wrote: > > On Tue, 25 Mar 2025 00:37:59 +0100 > > Marek Vasut <marex@denx.de> wrote: > > > >> On 3/24/25 9:43 AM, Boris Brezillon wrote: > >> > >> [...] > >> > >>>> @@ -563,6 +585,7 @@ int panthor_device_suspend(struct device *dev) > >>>> > >>>> panthor_devfreq_suspend(ptdev); > >>>> > >>>> + reset_control_assert(ptdev->resets); > >>> > >>> Hm, that might be the cause of the fast reset issue (which is a fast > >>> resume more than a fast reset BTW): if you re-assert the reset line on > >>> runtime suspend, I guess this causes a full GPU reset, and the MCU ends > >>> up in a state where it needs a slow reset (all data sections reset to > >>> their initial state). Can you try to move the reset_control_[de]assert > >>> to the unplug/init functions? > >> Is it correct to assume , that if I remove all reset_control_assert() > >> calls (and keep only the _deassert() calls), the slow resume problem > >> should go away too ? > > > > Yeah, dropping the _assert()s should do the trick. > Hmmm, no, that does not help. I was hoping maybe NXP can chime in and > suggest something too ? Can you try keep all the clks/regulators/power-domains/... on after init, and see if the fast resume works with that. If it does, re-introduce one resource at a time to find out which one causes the MCU to lose its state.
On 3/25/25 3:35 PM, Boris Brezillon wrote: > On Tue, 25 Mar 2025 14:50:32 +0100 > Marek Vasut <marex@denx.de> wrote: > >> On 3/25/25 8:43 AM, Boris Brezillon wrote: >>> On Tue, 25 Mar 2025 00:37:59 +0100 >>> Marek Vasut <marex@denx.de> wrote: >>> >>>> On 3/24/25 9:43 AM, Boris Brezillon wrote: >>>> >>>> [...] >>>> >>>>>> @@ -563,6 +585,7 @@ int panthor_device_suspend(struct device *dev) >>>>>> >>>>>> panthor_devfreq_suspend(ptdev); >>>>>> >>>>>> + reset_control_assert(ptdev->resets); >>>>> >>>>> Hm, that might be the cause of the fast reset issue (which is a fast >>>>> resume more than a fast reset BTW): if you re-assert the reset line on >>>>> runtime suspend, I guess this causes a full GPU reset, and the MCU ends >>>>> up in a state where it needs a slow reset (all data sections reset to >>>>> their initial state). Can you try to move the reset_control_[de]assert >>>>> to the unplug/init functions? >>>> Is it correct to assume , that if I remove all reset_control_assert() >>>> calls (and keep only the _deassert() calls), the slow resume problem >>>> should go away too ? >>> >>> Yeah, dropping the _assert()s should do the trick. >> Hmmm, no, that does not help. I was hoping maybe NXP can chime in and >> suggest something too ? > > Can you try keep all the clks/regulators/power-domains/... on after > init, and see if the fast resume works with that. If it does, > re-introduce one resource at a time to find out which one causes the > MCU to lose its state. I already tried that too . I spent quite a while until I reached that L2 workaround in fact.
On Tue, 25 Mar 2025 15:37:01 +0100 Marek Vasut <marex@denx.de> wrote: > On 3/25/25 3:35 PM, Boris Brezillon wrote: > > On Tue, 25 Mar 2025 14:50:32 +0100 > > Marek Vasut <marex@denx.de> wrote: > > > >> On 3/25/25 8:43 AM, Boris Brezillon wrote: > >>> On Tue, 25 Mar 2025 00:37:59 +0100 > >>> Marek Vasut <marex@denx.de> wrote: > >>> > >>>> On 3/24/25 9:43 AM, Boris Brezillon wrote: > >>>> > >>>> [...] > >>>> > >>>>>> @@ -563,6 +585,7 @@ int panthor_device_suspend(struct device *dev) > >>>>>> > >>>>>> panthor_devfreq_suspend(ptdev); > >>>>>> > >>>>>> + reset_control_assert(ptdev->resets); > >>>>> > >>>>> Hm, that might be the cause of the fast reset issue (which is a fast > >>>>> resume more than a fast reset BTW): if you re-assert the reset line on > >>>>> runtime suspend, I guess this causes a full GPU reset, and the MCU ends > >>>>> up in a state where it needs a slow reset (all data sections reset to > >>>>> their initial state). Can you try to move the reset_control_[de]assert > >>>>> to the unplug/init functions? > >>>> Is it correct to assume , that if I remove all reset_control_assert() > >>>> calls (and keep only the _deassert() calls), the slow resume problem > >>>> should go away too ? > >>> > >>> Yeah, dropping the _assert()s should do the trick. > >> Hmmm, no, that does not help. I was hoping maybe NXP can chime in and > >> suggest something too ? > > > > Can you try keep all the clks/regulators/power-domains/... on after > > init, and see if the fast resume works with that. If it does, > > re-introduce one resource at a time to find out which one causes the > > MCU to lose its state. > > I already tried that too . I spent quite a while until I reached that L2 > workaround in fact. So, with your RPM suspend/resume being NOPs, it still doesn't work? Unless the FW is doing something behind our back, I don't really see why this would fail on your platform, but not on the rk3588. Are you sure the power domains are kept on at all times. I'm asking, because if you linked all the PDs, the on/off sequence is automatically handled by the RPM core at suspend/resume time.
On Di, 2025-03-25 at 15:27 +0100, Marek Vasut wrote: > On 3/25/25 3:12 PM, Philipp Zabel wrote: > > On Mo, 2025-03-24 at 20:05 +0100, Marek Vasut wrote: > > > On 3/24/25 9:43 AM, Boris Brezillon wrote: > > > > > > [...] > > > > > > > > @@ -563,6 +585,7 @@ int panthor_device_suspend(struct device *dev) > > > > > > > > > > panthor_devfreq_suspend(ptdev); > > > > > > > > > > + reset_control_assert(ptdev->resets); > > > > > > > > Hm, that might be the cause of the fast reset issue (which is a fast > > > > resume more than a fast reset BTW): if you re-assert the reset line on > > > > runtime suspend, I guess this causes a full GPU reset, and the MCU ends > > > > up in a state where it needs a slow reset (all data sections reset to > > > > their initial state). Can you try to move the reset_control_[de]assert > > > > to the unplug/init functions? > > > The reset on the MX95 is not really a reset, it is clear-only > > > set-never-again bit which goes only one way, the "unreset" way, so I > > > don't think this has any impact. > > > > This sounds like the bit is initially set to 1 (reset asserted) and can > > be cleared by writing 0 (once, to deassert the reset). But in the > > reset-simple patch it looks to be the other way around (active_low = > > true)? > > > > If the reset control can't be asserted again on this hardware, it would > > be better to have custom driver that doesn't implement the .assert() > > callback at all. > Maybe the reset-simple driver can be extended with some mode of > operation like this instead , to make it skip assert once deasserted , > for specific reset controllers ? How about a second reset_control_ops struct without .assert, selected in reset_simple_probe() by a new flag in reset_simple_devdata. regards Philipp
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 --- V2: Drop the select RESET_SIMPLE from Kconfig --- drivers/gpu/drm/panthor/panthor_device.c | 23 +++++++++++++++++++++++ drivers/gpu/drm/panthor/panthor_device.h | 3 +++ 2 files changed, 26 insertions(+)