Re: [PATCH 2/2] drm/panfrost: add devfreq regulator support

[ClÃment PÃron]

Hi Steven,

On Mon, 13 Apr 2020 at 17:55, Steven Price <steven.price@xxxxxxx> wrote:
>
> On 13/04/2020 15:31, ClÃment PÃron wrote:
> > Hi,
> >
> > On Mon, 13 Apr 2020 at 16:18, ClÃment PÃron <peron.clem@xxxxxxxxx> wrote:
> >>
> >> Hi Steven,
> >>
> >> On Mon, 13 Apr 2020 at 15:18, Steven Price <steven.price@xxxxxxx> wrote:
> >>>
> >>> On 11/04/2020 21:06, ClÃment PÃron wrote:
> >>>> OPP table can defined both frequency and voltage.
> >>>>
> >>>> Register the mali regulator if it exist.
> >>>>
> >>>> Signed-off-by: ClÃment PÃron <peron.clem@xxxxxxxxx>
> >>>> ---
> >>>>    drivers/gpu/drm/panfrost/panfrost_devfreq.c | 34 ++++++++++++++++++---
> >>>>    drivers/gpu/drm/panfrost/panfrost_device.h  |  1 +
> >>>>    2 files changed, 31 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> >>>> index 62541f4edd81..2dc8e2355358 100644
> >>>> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> >>>> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> >>>> @@ -78,12 +78,26 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
> >>>>        struct device *dev = &pfdev->pdev->dev;
> >>>>        struct devfreq *devfreq;
> >>>>        struct thermal_cooling_device *cooling;
> >>>> +     const char *mali = "mali";
> >>>> +     struct opp_table *opp_table = NULL;
> >>>> +
> >>>> +     /* Regulator is optional */
> >>>> +     opp_table = dev_pm_opp_set_regulators(dev, &mali, 1);
> >>>
> >>> This looks like it applies before 3e1399bccf51 ("drm/panfrost: Add
> >>> support for multiple regulators") which is currently in drm-misc-next
> >>> (and linux-next). You want something more like:
> >>
> >> Thanks for you review, indeed I didn't see that multiple regulators
> >> support has been added.
> >> Will update in v2.
> >>
> >>>
> >>>       opp_table = dev_pm_opp_set_regulators(dev,
> >>>                                             pfdev->comp->supply_names,
> >>>                                             pfdev->comp->num_supplies);
> >>>
> >>> Otherwise a platform with multiple regulators won't work correctly.
> >>>
> >>> Also running on my firefly (RK3288) board I get the following warning:
> >>>
> >>>      debugfs: Directory 'ffa30000.gpu-mali' with parent 'vdd_gpu' already
> >>> present!
> >>>
> >>> This is due to the regulator debugfs entries getting created twice (once
> >>> in panfrost_regulator_init() and once here).
> >>
> >> Is it a warning that should be consider as an error? Look's more an info no?
> >> What should be the correct behavior if a device want to register two
> >> times the same regulator?
> >
> > Or we can change the name from vdd_XXX to opp_vdd_XXX ?
> > https://elixir.bootlin.com/linux/latest/source/drivers/opp/debugfs.c#L45
>
> Yes, I'm not sure that it's actually a problem in practice. And it may
> well be correct to change this in the generic code rather than try to
> work around it in Panfrost. But we shouldn't spam the user with warnings
> as that makes real issues harder to see.
>
> Your suggestion to change the name seems reasonable to me, but I don't
> fully understand the opp code, so we'd need some review from the OPP
> maintainers. Hopefully Viresh, Nishanth or Stephen can provide some insight.

Agree, I will send a v2 with the rename and see if OPP Maintainers agree.

Regards,
Clement

>
> Steve