Re: Multiple regulators for one device [was drm/panfrost: add devfreq regulator support]
On Sun, Apr 19, 2020 at 11:25:08AM +0200, Clément Péron wrote: > Just saw that a Lima devfreq[0] has been also introduced with I think > exactly the same logic. > Is this something that hasn't been triggered by Maintainer or I am > missing something? My understanding is that there is very little use of any of this upstream since it's all pretty new, some platforms have OPPs but use a firmware interface rather than the OS to control clocks and regulators while most other platforms don't have OPPs defined and it's only platforms with both regulators and OPPs that are affected. signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Multiple regulators for one device [was drm/panfrost: add devfreq regulator support]
Hi, On Fri, 17 Apr 2020 at 14:33, Clément Péron wrote: > > Hi Robin, > > On Fri, 17 Apr 2020 at 13:10, Robin Murphy wrote: > > > > On 2020-04-16 2:42 pm, Steven Price wrote: > > [...] > > > Perhaps a better approach would be for Panfrost to hand over the struct > > > regulator objects it has already got to the OPP framework. I.e. open > > > code dev_pm_opp_set_regulators(), but instead of calling > > > regulator_get_optional() simply populate the regulators we already have? Just saw that a Lima devfreq[0] has been also introduced with I think exactly the same logic. Is this something that hasn't been triggered by Maintainer or I am missing something? I will backport some remarks made on the lima devfreq to improve panfrost one. They are almost identical. Regards, Clement 0: https://cgit.freedesktop.org/drm-misc/commit/?id=1996970773a323533e1cc1b6b97f00a95d675f32 > > > > > > The other benefit of that is it would provide a clear hand-over of > > > responsibility between Panfrost handling it's own regulators and the OPP > > > framework picking up the work. The disadvantage is that Panfrost would > > > have to track whether the regulators have been handed over or not. > > > > Sounds like the most logical thing to do is to shuffle things around so > > we start by trying to set up an OPP table, then fall back to explicitly > > claiming clocks and regulators if necessary. Then we can easily make the > > devfreq decision later in probe based on how that turned out. > > Ok I will propose a new serie with this behavior, > > Thanks > Clement > > > > > Robin. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Multiple regulators for one device [was drm/panfrost: add devfreq regulator support]
Hi Robin, On Fri, 17 Apr 2020 at 13:10, Robin Murphy wrote: > > On 2020-04-16 2:42 pm, Steven Price wrote: > [...] > > Perhaps a better approach would be for Panfrost to hand over the struct > > regulator objects it has already got to the OPP framework. I.e. open > > code dev_pm_opp_set_regulators(), but instead of calling > > regulator_get_optional() simply populate the regulators we already have? > > > > The other benefit of that is it would provide a clear hand-over of > > responsibility between Panfrost handling it's own regulators and the OPP > > framework picking up the work. The disadvantage is that Panfrost would > > have to track whether the regulators have been handed over or not. > > Sounds like the most logical thing to do is to shuffle things around so > we start by trying to set up an OPP table, then fall back to explicitly > claiming clocks and regulators if necessary. Then we can easily make the > devfreq decision later in probe based on how that turned out. Ok I will propose a new serie with this behavior, Thanks Clement > > Robin. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Multiple regulators for one device [was drm/panfrost: add devfreq regulator support]
On 2020-04-16 2:42 pm, Steven Price wrote: [...] Perhaps a better approach would be for Panfrost to hand over the struct regulator objects it has already got to the OPP framework. I.e. open code dev_pm_opp_set_regulators(), but instead of calling regulator_get_optional() simply populate the regulators we already have? The other benefit of that is it would provide a clear hand-over of responsibility between Panfrost handling it's own regulators and the OPP framework picking up the work. The disadvantage is that Panfrost would have to track whether the regulators have been handed over or not. Sounds like the most logical thing to do is to shuffle things around so we start by trying to set up an OPP table, then fall back to explicitly claiming clocks and regulators if necessary. Then we can easily make the devfreq decision later in probe based on how that turned out. Robin. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Multiple regulators for one device [was drm/panfrost: add devfreq regulator support]
On Thu, Apr 16, 2020 at 02:42:13PM +0100, Steven Price wrote: > On 14/04/2020 20:16, Clément Péron wrote: > > That's can be reworked and Panfrost can only probe regulator if there > > is no opp-table. > This is what I was thinking about looking at. But it may make sense instead > to extend the regulator API to allow multiple regualtor_get() calls for a > single device. I haven't had time to dig into how difficult this would be. To repeat what I said before we don't actively stop this, it's just not something that seems particularly tasteful and the warning does find actual errors. I definitely don't think it's a good idea to extend the API for this. > Ideally calling regulator_get a second time for the same device would simply > return the same struct regulator object (with a reference count increment). One of the goals with the distinct struct regulator is to make sure that we track all the user's activity together - if we mix multiple users in there it becomes harder to tell if something is going wrong. signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Multiple regulators for one device [was drm/panfrost: add devfreq regulator support]
On 14/04/2020 20:16, Clément Péron wrote: Hi Mark, On Tue, 14 Apr 2020 at 20:55, Mark Brown wrote: On Tue, Apr 14, 2020 at 08:20:23PM +0200, Clément Péron wrote: Hi Liam and Mark, You might want to flag stuff like this in the subject line, I very nearly deleted this without opening it since most of the email I get about panfrost appears to be coming from me having sent patches rather than being relevant. Ok will do next time, 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. We don't actively prevent it and I can't think what other than debugfs might run into problems (and that's just a warning) but it does seem like a weird thing to want to do and like it's pointing to some confusion in your code with two different parts of the device controlling the same supply independently. What's the use case here? Panfrost first probe clock, reset and regulator in device_init: https://elixir.bootlin.com/linux/v5.7-rc1/source/drivers/gpu/drm/panfrost/panfrost_drv.c#L602 Then it probe for optional devfreq, devfreq will get opp tables and also get regulator again. https://elixir.bootlin.com/linux/v5.7-rc1/source/drivers/gpu/drm/panfrost/panfrost_drv.c#L609 That's can be reworked and Panfrost can only probe regulator if there is no opp-table. This is what I was thinking about looking at. But it may make sense instead to extend the regulator API to allow multiple regualtor_get() calls for a single device. I haven't had time to dig into how difficult this would be. But if multiple regulator is not an issue and as each request is logic. The first in device_init assure to enable the regulator and the second in OPP assure the voltage level. Maybe we can just fix this warning? From what I can see in the code, just silencing the warning would lead to 'odd' behaviour with debugfs. The first struct regulator Panfrost acquires is the one that is used purely for turning the GPU on (no voltage scaling). The second struct regulator is the one which is obtained by the OPP framework. However the debugfs entries point into the actual struct regulator, so it would be far more logical/useful if those were pointing into the second struct regulator. Ideally calling regulator_get a second time for the same device would simply return the same struct regulator object (with a reference count increment). Perhaps a better approach would be for Panfrost to hand over the struct regulator objects it has already got to the OPP framework. I.e. open code dev_pm_opp_set_regulators(), but instead of calling regulator_get_optional() simply populate the regulators we already have? The other benefit of that is it would provide a clear hand-over of responsibility between Panfrost handling it's own regulators and the OPP framework picking up the work. The disadvantage is that Panfrost would have to track whether the regulators have been handed over or not. Steve ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
