From: Steven Price <steven.price@arm.com>
To: "Clément Péron" <peron.clem@gmail.com>
Cc: Rob Herring <robh@kernel.org>,
Tomeu Vizoso <tomeu.vizoso@collabora.com>,
Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>,
Viresh Kumar <vireshk@kernel.org>, Nishanth Menon <nm@ti.com>,
Stephen Boyd <sboyd@kernel.org>,
dri-devel <dri-devel@lists.freedesktop.org>,
linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/2] drm/panfrost: add devfreq regulator support
Date: Tue, 14 Apr 2020 14:10:36 +0100 [thread overview]
Message-ID: <000f26f4-3640-797f-c7f6-4b31a5e2669e@arm.com> (raw)
In-Reply-To: <CAJiuCcfa9ro1V4nBzfD48cCuHpEsLaNA5P0bb-tQ3hcWUCtpkA@mail.gmail.com>
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@gmail.com> wrote:
>>
>> Hi Steven,
>>
>> On Mon, 13 Apr 2020 at 17:55, Steven Price <steven.price@arm.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@gmail.com> wrote:
>>>>>
>>>>> Hi Steven,
>>>>>
>>>>> On Mon, 13 Apr 2020 at 15:18, Steven Price <steven.price@arm.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@gmail.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
> 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
>
> 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
next prev parent reply other threads:[~2020-04-14 13:10 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-11 20:06 [PATCH 1/2] drm/panfrost: missing remove opp table in case of failure Clément Péron
2020-04-11 20:06 ` [PATCH 2/2] drm/panfrost: add devfreq regulator support Clément Péron
2020-04-13 11:33 ` Clément Péron
2020-04-13 13:18 ` Steven Price
2020-04-13 14:18 ` Clément Péron
2020-04-13 14:31 ` Clément Péron
2020-04-13 15:55 ` Steven Price
2020-04-13 16:35 ` Clément Péron
2020-04-13 17:28 ` Clément Péron
2020-04-14 13:10 ` Steven Price [this message]
2020-04-14 18:20 ` Clément Péron
2020-04-14 18:55 ` Mark Brown
2020-04-14 19:16 ` Clément Péron
2020-04-16 13:42 ` Multiple regulators for one device [was drm/panfrost: add devfreq regulator support] Steven Price
2020-04-16 14:04 ` Mark Brown
2020-04-17 11:10 ` Robin Murphy
2020-04-17 12:33 ` Clément Péron
2020-04-19 9:25 ` Clément Péron
2020-04-20 12:32 ` Mark Brown
2020-04-16 13:44 ` [PATCH 2/2] drm/panfrost: add devfreq regulator support Mark Brown
2020-05-02 22:07 ` Clément Péron
2020-05-07 14:30 ` Steven Price
2020-05-09 16:28 ` Clément Péron
2020-05-09 19:40 ` Clément Péron
2020-04-14 13:09 ` Robin Murphy
2020-04-13 13:07 ` [PATCH 1/2] drm/panfrost: missing remove opp table in case of failure Steven Price
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=000f26f4-3640-797f-c7f6-4b31a5e2669e@arm.com \
--to=steven.price@arm.com \
--cc=alyssa.rosenzweig@collabora.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=linux-kernel@vger.kernel.org \
--cc=nm@ti.com \
--cc=peron.clem@gmail.com \
--cc=robh@kernel.org \
--cc=sboyd@kernel.org \
--cc=tomeu.vizoso@collabora.com \
--cc=vireshk@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).