[PATCH v4 7/7] RFC: drm/panfrost: devfreq: Add support for 2 regulators

[Nick Fan (范哲維)]

An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20200214/25b84188/attachment-0001.htm>
-------------- next part --------------
+JB and SJ for info sync

Hi Nicolas,

Please see my inline reply.
Thanks

Hi Weiyi,

Could you please help to comment on the second question?
Thanks

Warmest regards,
Nick Fan 范哲維

-----Original Message-----
From: Nicolas Boichat [mailto:drinkcat at chromium.org] 
Sent: Thursday, February 13, 2020 3:58 PM
To: Rob Herring <robh+dt at kernel.org>; Weiyi Lu (呂威儀) <Weiyi.Lu at mediatek.com>; Nick Fan (范哲維) <Nick.Fan at mediatek.com>
Cc: David Airlie <airlied at linux.ie>; Daniel Vetter <daniel at ffwll.ch>; Mark Rutland <mark.rutland at arm.com>; Matthias Brugger <matthias.bgg at gmail.com>; Tomeu Vizoso <tomeu.vizoso at collabora.com>; Steven Price <steven.price at arm.com>; Alyssa Rosenzweig <alyssa.rosenzweig at collabora.com>; Liam Girdwood <lgirdwood at gmail.com>; Mark Brown <broonie at kernel.org>; dri-devel <dri-devel at lists.freedesktop.org>; Devicetree List <devicetree at vger.kernel.org>; lkml <linux-kernel at vger.kernel.org>; linux-arm Mailing List <linux-arm-kernel at lists.infradead.org>; moderated list:ARM/Mediatek SoC support <linux-mediatek at lists.infradead.org>; Hsin-Yi Wang <hsinyi at chromium.org>; Ulf Hansson <ulf.hansson at linaro.org>; Viresh Kumar <viresh.kumar at linaro.org>; Stephen Boyd <sboyd at kernel.org>
Subject: Re: [PATCH v4 7/7] RFC: drm/panfrost: devfreq: Add support for 2 regulators

+Weiyi Lu +Nick Fan @MTK who may have more ideas.

On Thu, Feb 13, 2020 at 2:14 AM Rob Herring <robh+dt at kernel.org> wrote:
>
> On Wed, Feb 12, 2020 at 2:49 AM Nicolas Boichat <drinkcat at chromium.org> wrote:
> >
> > +Viresh Kumar +Stephen Boyd for clock advice.
> >
> > On Fri, Feb 7, 2020 at 1:27 PM Nicolas Boichat <drinkcat at chromium.org> wrote:
> > >
> > > The Bifrost GPU on MT8183 uses 2 regulators (core and SRAM) for 
> > > devfreq, and provides OPP table with 2 sets of voltages.
> > >
> > > TODO: This is incomplete as we'll need add support for setting a 
> > > pair of voltages as well.
> >
> > So all we need for this to work (at least apparently, that is, I can 
> > change frequency) is this:
> > https://lore.kernel.org/patchwork/patch/1192945/
> > (ah well, Viresh just replied, so, probably not, I'll check that out 
> > and use the correct API)
> >
> > But then there's a slight problem: panfrost_devfreq uses a bunch of 
> > clk_get_rate calls, and the clock PLLs (at least on MTK platform) 
> > are never fully precise, so we get back 299999955 for 300 Mhz and
> > 799999878 for 800 Mhz. That means that the kernel is unable to keep 
> > devfreq stats as neither of these values are in the table:
> > [ 4802.470952] devfreq devfreq1: Couldn't update frequency 
> > transition information.
> > The kbase driver fixes this by remembering the last set frequency, 
> > and reporting that to devfreq. Should we do that as well or is there 
> > a better fix?
> >
> > Another thing that I'm not implementing is the dance that Mediatek 
> > does in their kbase driver when changing the clock (described in 
> > patch
> > 2/7):
> > ""
> > The binding we use with out-of-tree Mali drivers includes more 
> > clocks, this is used for devfreq: the out-of-tree driver switches 
> > clk_mux to clk_sub_parent (26Mhz), adjusts clk_main_parent, then 
> > switches clk_mux back to clk_main_parent:
> > (see 
> > https://chromium.googlesource.com/chromiumos/third_party/kernel/+/ch
> > romeos-4.19/drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_run
> > time_pm.c#423)
> > clocks =
> >         <&topckgen CLK_TOP_MFGPLL_CK>,
> >         <&topckgen CLK_TOP_MUX_MFG>,
> >         <&clk26m>,
> >         <&mfgcfg CLK_MFG_BG3D>;
> > clock-names =
> >         "clk_main_parent",
> >         "clk_mux",
> >         "clk_sub_parent",
> >         "subsys_mfg_cg";
> > ""
> > Is there a clean/simple way to implement this in the clock 
> > framework/device tree? Or should we implement something in the 
> > panfrost driver?
>
> Putting parent clocks into 'clocks' for a device is a pretty common 
> abuse. The 'assigned-clocks' binding is what's used for parent clock 
> setup. Not sure that's going to help here though. Is this dance 
> because the parent clock frequency can't be changed cleanly?

Nick/Weiyi, any idea why we do that dance in the first place? (maybe the PLL clock is unstable while it's being changed?)

Clock source may become unstable during clock frequency changes, so it is always safer to switch to a more reliable clock source.
Otherwise, it may cause some problem in some corner case.
I would suggest to keep it.

If we really need it, can we move that logic to the clock core?

> If up to
> me, I'd put that dance in the clock driver. The GPU shouldn't have to 
> care.
>
> Rob