From: "Clément Péron" <peron.clem@gmail.com> To: Steven Price <steven.price@arm.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: Mon, 13 Apr 2020 18:35:23 +0200 [thread overview] Message-ID: <CAJiuCccv2XPLY6sjcgvvrG5a8ONYHa_xn9i-YUDKUDK5a0DY=A@mail.gmail.com> (raw) In-Reply-To: <d6465e7e-8e05-8b7d-16bd-f40877969089@arm.com> 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! > >>> > >>> 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
WARNING: multiple messages have this Message-ID (diff)
From: "Clément Péron" <peron.clem@gmail.com> To: Steven Price <steven.price@arm.com> Cc: Nishanth Menon <nm@ti.com>, Tomeu Vizoso <tomeu.vizoso@collabora.com>, Stephen Boyd <sboyd@kernel.org>, Viresh Kumar <vireshk@kernel.org>, linux-kernel <linux-kernel@vger.kernel.org>, dri-devel <dri-devel@lists.freedesktop.org>, Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com> Subject: Re: [PATCH 2/2] drm/panfrost: add devfreq regulator support Date: Mon, 13 Apr 2020 18:35:23 +0200 [thread overview] Message-ID: <CAJiuCccv2XPLY6sjcgvvrG5a8ONYHa_xn9i-YUDKUDK5a0DY=A@mail.gmail.com> (raw) In-Reply-To: <d6465e7e-8e05-8b7d-16bd-f40877969089@arm.com> 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! > >>> > >>> 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2020-04-13 16:35 UTC|newest] Thread overview: 52+ 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 ` Clément Péron 2020-04-11 20:06 ` [PATCH 2/2] drm/panfrost: add devfreq regulator support Clément Péron 2020-04-11 20:06 ` Clément Péron 2020-04-13 11:33 ` Clément Péron 2020-04-13 11:33 ` Clément Péron 2020-04-13 13:18 ` Steven Price 2020-04-13 13:18 ` Steven Price 2020-04-13 14:18 ` Clément Péron 2020-04-13 14:18 ` Clément Péron 2020-04-13 14:31 ` Clément Péron 2020-04-13 14:31 ` Clément Péron 2020-04-13 15:55 ` Steven Price 2020-04-13 15:55 ` Steven Price 2020-04-13 16:35 ` Clément Péron [this message] 2020-04-13 16:35 ` Clément Péron 2020-04-13 17:28 ` Clément Péron 2020-04-13 17:28 ` Clément Péron 2020-04-14 13:10 ` Steven Price 2020-04-14 13:10 ` Steven Price 2020-04-14 18:20 ` Clément Péron 2020-04-14 18:20 ` Clément Péron 2020-04-14 18:55 ` Mark Brown 2020-04-14 18:55 ` Mark Brown 2020-04-14 19:16 ` Clément Péron 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 13:42 ` Steven Price 2020-04-16 14:04 ` Mark Brown 2020-04-16 14:04 ` Mark Brown 2020-04-17 11:10 ` Robin Murphy 2020-04-17 11:10 ` Robin Murphy 2020-04-17 12:33 ` Clément Péron 2020-04-17 12:33 ` Clément Péron 2020-04-19 9:25 ` Clément Péron 2020-04-19 9:25 ` Clément Péron 2020-04-20 12:32 ` Mark Brown 2020-04-20 12:32 ` Mark Brown 2020-04-16 13:44 ` [PATCH 2/2] drm/panfrost: add devfreq regulator support Mark Brown 2020-04-16 13:44 ` Mark Brown 2020-05-02 22:07 ` Clément Péron 2020-05-02 22:07 ` Clément Péron 2020-05-07 14:30 ` Steven Price 2020-05-07 14:30 ` Steven Price 2020-05-09 16:28 ` Clément Péron 2020-05-09 16:28 ` Clément Péron 2020-05-09 19:40 ` Clément Péron 2020-05-09 19:40 ` Clément Péron 2020-04-14 13:09 ` Robin Murphy 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 2020-04-13 13:07 ` 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='CAJiuCccv2XPLY6sjcgvvrG5a8ONYHa_xn9i-YUDKUDK5a0DY=A@mail.gmail.com' \ --to=peron.clem@gmail.com \ --cc=alyssa.rosenzweig@collabora.com \ --cc=dri-devel@lists.freedesktop.org \ --cc=linux-kernel@vger.kernel.org \ --cc=nm@ti.com \ --cc=robh@kernel.org \ --cc=sboyd@kernel.org \ --cc=steven.price@arm.com \ --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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.