RE: [PATCH v3 4/5] drm/panfrost: devfreq: set opp to the recommended one to configure and enable regulator

[Biju Das]

Hi Clement,

> 
> Hi,
> 
> On Mon, 5 Sept 2022 at 20:17, Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> wrote:
> >
> > Hi,
> >
> > Thanks for the patch.
> >
> > > Subject: [PATCH v3 4/5] drm/panfrost: devfreq: set opp to the
> > > recommended one to configure and enable regulator
> > >
> > > devm_pm_opp_set_regulators() doesn't enable regulator, which make
> > > regulator framework switching it off during regulator_late_cleanup().
> >
> > In that case, why not regulator_get()for Dynamic regulator(non fixed
> > regulator)??
> 
> Sorry I don't understand, what do you mean?

Normally we need to turn on regulator and clock only when needed.
I am not sure with your new code, will make it always on and
drains the power unnecessarily and does it set lower opp or higher
opp at the start??

Compared to the fixed regulator, you have voltage regulator to
control that is the difference between my environment and
Your environment.

I am not sure any other SoC is using voltage regulator??
If yes, thenthere should be some bug or some difference in HW
which is giving different behaviour??

If you are the first one using voltage regulator with mali gpu,
Then Your implementation may be correct, as you have proper
HW to check.

> 
> >
> > >
> > > Call dev_pm_opp_set_opp() with the recommend OPP in
> > > panfrost_devfreq_init() to enable the regulator and avoid any switch
> > > off by regulator_late_cleanup().
> > >
> > > Suggested-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> > > Signed-off-by: Clément Péron <peron.clem@xxxxxxxxx>
> > > ---
> > >  drivers/gpu/drm/panfrost/panfrost_devfreq.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > > b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > > index 5110cd9b2425..67b242407156 100644
> > > --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > > +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > > @@ -131,6 +131,14 @@ int panfrost_devfreq_init(struct
> > > panfrost_device
> > > *pfdev)
> > >               return PTR_ERR(opp);
> > >
> > >       panfrost_devfreq_profile.initial_freq = cur_freq;
> > > +
> > > +     /* Setup and enable regulator */
> > > +     ret = dev_pm_opp_set_opp(dev, opp);
> > > +     if (ret) {
> > > +             DRM_DEV_ERROR(dev, "Couldn't set recommended OPP\n");
> > > +             return ret;
> > > +     }
> >
> >
> > FYI,
> > On RZ/G2L mali gpu, we have fixed regulator and I was able to do GPU
> > OPP transition without any issues previously.
> 
> rzg2l-smarc-som.dtsi uses regulator reg_1p1v; which is marked as
> regulator-always-on; that's why
> regulator_late_cleanup() doesn't switch it off.

Yes that is correct. It is fixed regulator and always on.
We control only frequency.

Cheers,
Biju

> 
> >
> > root@smarc-rzg2l:~# cat /sys/class/devfreq/11840000.gpu/trans_stat
> >      From  :   To
> >            :  50000000  62500000 100000000 125000000 200000000
> 250000000 400000000 500000000   time(ms)
> > *  50000000:         0         0         0         0         0
> 0         0         1       144
> >    62500000:         0         0         0         0         0
> 0         0         0         0
> >   100000000:         0         0         0         0         0
> 0         0         9       524
> >   125000000:         0         0         9         0         0
> 0         0         3      2544
> >   200000000:         0         0         0        11         0
> 0         0        46      3304
> >   250000000:         1         0         0         0        33
> 0         0         0      7496
> >   400000000:         0         0         0         0        16
> 19         0         0      2024
> >   500000000:         1         0         0         1         8
> 15        35         0      4032
> > Total transition : 208
> >
> > Cheers,
> > Biju
> >