Message-ID: <CAJiuCccF3tmbmMWNh0nC5WRJ1_iPdj6f1oH1zYMSue_pFrXsPQ@mail.gmail.com>
Date:   Tue, 14 Apr 2020 20:20:23 +0200
From:   Clément Péron <peron.clem@...il.com>
To:     Steven Price <steven.price@....com>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>
Cc:     Rob Herring <robh@...nel.org>,
        Tomeu Vizoso <tomeu.vizoso@...labora.com>,
        Alyssa Rosenzweig <alyssa.rosenzweig@...labora.com>,
        Viresh Kumar <vireshk@...nel.org>, Nishanth Menon <nm@...com>,
        Stephen Boyd <sboyd@...nel.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] drm/panfrost: add devfreq regulator support

Hi Liam and Mark,

On Tue, 14 Apr 2020 at 15:10, Steven Price <steven.price@....com> wrote:
>
> Hi Clément,
>
> On 13/04/2020 18:28, Clément Péron wrote:
> > Hi Steven,
> >
> > On Mon, 13 Apr 2020 at 18:35, Clément Péron <peron.clem@...il.com> wrote:
> >>
> >> Hi Steven,
> >>
> >> On Mon, 13 Apr 2020 at 17:55, Steven Price <steven.price@....com> 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@...il.com> wrote:
> >>>>>
> >>>>> Hi Steven,
> >>>>>
> >>>>> On Mon, 13 Apr 2020 at 15:18, Steven Price <steven.price@....com> 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@...il.com>
> >>>>>>> ---
> >>>>>>>     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!
> >
> > I try to reproduce but it can't

Sorry, you're right I have indeed the same issue:
[    2.903406] panfrost 1800000.gpu: Features: L2:0x07110206
Shader:0x00000000 Tiler:0x00000809 Mem:0x1 MMU:0x00002821 AS:0xf
JS:0x7
[    2.913297] sunxi-ir 7040000.ir: initialized sunXi IR driver
[    2.919901] panfrost 1800000.gpu: shader_present=0x3 l2_present=0x1
[    2.920497] panfrost 1800000.gpu: Looking up mali-supply from device tree
[    3.036568] vdd-gpu: could not add device link 1800000.gpu err -17
[    3.036580] debugfs: Directory '1800000.gpu-mali' with parent
'vdd-gpu' already present!
[    3.046312] panfrost 1800000.gpu: [drm:panfrost_devfreq_init
[panfrost]] Failed to register cooling device
[    3.056322] [drm] Initialized panfrost 1.1.0 20180908 for
1800000.gpu on minor 0

> > regulator is mount at :
> > ./regulator/vdd-gpu
> > whereas OPP is mount :
> > ./opp/soc-1800000.gpu/opp:756000000/supply-0/
>
> Getting a backtrace from the two occurrences, I see one added from:
>
>    (debugfs_create_dir) from [<c04633f8>] (create_regulator+0xe0/0x220)
>    (create_regulator) from [<c04681d8>] (_regulator_get+0x168/0x204)
>    (_regulator_get) from [<c04682e0>] (regulator_bulk_get+0x64/0xf4)
>    (regulator_bulk_get) from [<c04696f0>]
> (devm_regulator_bulk_get+0x40/0x74)
>    (devm_regulator_bulk_get) from [<bf00af44>]
> (panfrost_device_init+0x1b4/0x48c [panfrost])
>    (panfrost_device_init [panfrost]) from [<bf00a4d4>]
> (panfrost_probe+0x94/0x184 [panfrost])
>    (panfrost_probe [panfrost]) from [<c04ee694>]
> (platform_drv_probe+0x48/0x94)
>
> And the other:
>
>    (debugfs_create_dir) from [<c04633f8>] (create_regulator+0xe0/0x220)
>    (create_regulator) from [<c04681d8>] (_regulator_get+0x168/0x204)
>    (_regulator_get) from [<c05c1280>] (dev_pm_opp_set_regulators+0x6c/0x184)
>    (dev_pm_opp_set_regulators) from [<bf00b4ac>]
> (panfrost_devfreq_init+0x38/0x1ac [panfrost])
>    (panfrost_devfreq_init [panfrost]) from [<bf00a508>]
> (panfrost_probe+0xc8/0x184 [panfrost])
>    (panfrost_probe [panfrost]) from [<c04ee694>]
> (platform_drv_probe+0x48/0x94)
>
> Both are created at /regulator/vdd_gpu

We are having an issue with Panfrost driver registering two times the
same regulator and giving an error when trying to create the debugfs
folder.

Could you clarify if it is allowed for a device to register two times
the same regulator?

I check Documentation/power/regulator/regulator.rst but this point is
not specified.

Thanks,
Clement

>
> >
> > I see that firefly as 2 regulators with the same name :
> > vdd_gpu from syr828
> > (https://github.com/mopplayer/Firefly-RK3288-Kernel-With-Mali764/blob/master/arch/arm/boot/dts/firefly-rk3288.dts#L453)
> > vdd_gpu from rk808_dcdc2_reg
> > (https://github.com/mopplayer/Firefly-RK3288-Kernel-With-Mali764/blob/master/arch/arm/boot/dts/firefly-rk3288.dts#L841)
> >
> > So i think the issue is from the firefly device-tree.
>
> I'm using the DTB from the kernel tree
> (/arch/arm/boot/dts/rk3288-firefly.dts) which as far as I can see
> doesn't contain this problem. Certainly decompiling the DTB I can see
> only one mention of vdd_gpu (in syr828).
>
> Steve
>
> >
> > Regards,
> > Clement
> >
> >>>>>>
> >>>>>> 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
>

Powered by blists - more mailing lists