Message-ID: <2015216.PYKUYFuaPT@workhorse>
Date: Thu, 25 Sep 2025 16:04:59 +0200
From: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
To: Ulf Hansson <ulf.hansson@...aro.org>
Cc: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
 Boris Brezillon <boris.brezillon@...labora.com>,
 Jassi Brar <jassisinghbrar@...il.com>, Chia-I Wu <olvaffe@...il.com>,
 Chen-Yu Tsai <wenst@...omium.org>, Steven Price <steven.price@....com>,
 Liviu Dudau <liviu.dudau@....com>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>,
 Matthias Brugger <matthias.bgg@...il.com>, Kees Cook <kees@...nel.org>,
 "Gustavo A. R. Silva" <gustavoars@...nel.org>, kernel@...labora.com,
 dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-mediatek@...ts.infradead.org, linux-hardening@...r.kernel.org,
 linux-pm@...r.kernel.org
Subject: Re: [PATCH v4 8/8] pmdomain: mediatek: Add support for MFlexGraphics

On Tuesday, 23 September 2025 16:24:13 Central European Summer Time Ulf Hansson wrote:
> On Tue, 23 Sept 2025 at 13:41, Nicolas Frattaroli
> <nicolas.frattaroli@...labora.com> wrote:
> >
> > Various MediaTek SoCs use GPU integration silicon named "MFlexGraphics"
> > by MediaTek. On the MT8196 and MT6991 SoCs, interacting with this
> > integration silicon is required to power on the GPU.
> >
> > This glue silicon is in the form of an embedded microcontroller running
> > special-purpose firmware, which autonomously adjusts clocks and
> > regulators.
> >
> > Implement a driver, modelled as a pmdomain driver with a
> > set_performance_state operation, to support these SoCs.
> >
> > The driver also exposes the actual achieved clock rate, as read back
> > from the MCU, as common clock framework clocks, by acting as a clock
> > provider as well.
> >
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
> > ---
> >  drivers/pmdomain/mediatek/Kconfig            |  16 +
> >  drivers/pmdomain/mediatek/Makefile           |   1 +
> >  drivers/pmdomain/mediatek/mtk-mfg-pmdomain.c | 928 +++++++++++++++++++++++++++
> >  3 files changed, 945 insertions(+)
> 
> [...]
> 
> > +
> > +static int mtk_mfg_set_performance(struct generic_pm_domain *pd,
> > +                                  unsigned int state)
> > +{
> > +       struct mtk_mfg *mfg = mtk_mfg_from_genpd(pd);
> > +
> > +       /*
> > +        * Occasionally, we're asked to set OPPs when we're off. This will fail,
> > +        * so don't do it at all. We do foo != GENPD_STATE_ON instead of !foo
> > +        * as to not depend on the actual value of the enum.
> > +        */
> > +       if (mfg->pd.status != GENPD_STATE_ON)
> > +               return 0;
> 
> Returning 0 here, means that we may end up never restoring the
> performance state for a device and its genpd, when the device is
> getting runtime resumed. In genpd_runtime_resume() we are calling
> genpd_restore_performance_state() before calling genpd_power_on().
> This is deliberate, see commit ae8ac19655e0.
> 
> That said, I think we need to manage the restore in the ->power_on()
> callback. In principle, it means we should call
> mtk_mfg_set_oppidx(mfg, genpd->performance_state) from there.
> 

Thanks for pointing this out, yeah setting the oppidx in power_on
does look like the right choice.

> > +
> > +       return mtk_mfg_set_oppidx(mfg, state);
> > +}
> > +
> > +static int mtk_mfg_power_on(struct generic_pm_domain *pd)
> > +{
> > +       struct mtk_mfg *mfg = mtk_mfg_from_genpd(pd);
> > +       int ret;
> > +
> > +       ret = regulator_bulk_enable(mfg->variant->num_regulators,
> > +                                   mfg->gpu_regs);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = clk_prepare_enable(mfg->clk_eb);
> > +       if (ret)
> > +               goto err_disable_regulators;
> > +
> > +       ret = clk_bulk_prepare_enable(mfg->variant->num_clks, mfg->gpu_clks);
> > +       if (ret)
> > +               goto err_disable_eb_clk;
> > +
> > +       ret = mtk_mfg_eb_on(mfg);
> > +       if (ret)
> > +               goto err_disable_clks;
> > +
> > +       ret = mtk_mfg_power_control(mfg, true);
> > +       if (ret)
> > +               goto err_eb_off;
> > +
> > +       return 0;
> > +
> > +err_eb_off:
> > +       mtk_mfg_eb_off(mfg);
> > +err_disable_clks:
> > +       clk_bulk_disable_unprepare(mfg->variant->num_clks, mfg->gpu_clks);
> > +err_disable_eb_clk:
> > +       clk_disable_unprepare(mfg->clk_eb);
> > +err_disable_regulators:
> > +       regulator_bulk_disable(mfg->variant->num_regulators, mfg->gpu_regs);
> > +
> > +       return ret;
> > +}
> 
> [...]
> 
> Note, I intend to have a bit closer look at this soon, but I just
> observed the issue I pointed out above from my first quick look.

I actually have a question about a mystery that has been puzzling
me as I work on v5: when unloading the driver that uses the PD
(panthor) and my driver using `modprobe -r panthor mtk_mfg_pmdomain`,
I see that sometimes detach_dev is called with the pd status
reportedly being off, but according to my own power-on/power-off
counting I hacked in, it actually being on. It then proceeds to
call my remove, which results in three splats from the regulator
subsystem as the regulators weren't balanced with disables before
they were freed, which isn't the main problem but more a symptom
of the bigger problem that power_off and power_on calls don't
appear to be balanced by the pmdomain subsystem when a driver is
removed under certain circumstances.

Did I run into a core pmdomain bug here, or do I have to balance
the power_on/off myself somehow in detach_dev? If the latter, I'm
struggling to figure out how I can determine that the PD is still
on without doing my own bookkeeping, as pmdomain core seems to clear
these fields before my detach_dev gets to them :(

> 
> Kind regards
> Uffe
> 

Kind regards,
Nicolas Frattaroli

Powered by blists - more mailing lists