Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1524984 > unrolled thread

Re: [PATCH V3 0/9] PM / OPP: Multiple regulator support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH V3 0/9] PM / OPP: Multiple regulator support Viresh Kumar <viresh.kumar@linaro.org> - 2016-11-18 04:10 +0100
    Re: [PATCH V3 0/9] PM / OPP: Multiple regulator support Mark Brown <broonie@kernel.org> - 2016-11-18 11:50 +0100
      Re: [PATCH V3 0/9] PM / OPP: Multiple regulator support Viresh Kumar <viresh.kumar@linaro.org> - 2016-11-22 04:50 +0100
        Re: [PATCH V3 0/9] PM / OPP: Multiple regulator support Mark Brown <broonie@kernel.org> - 2016-11-22 19:50 +0100
          Re: [PATCH V3 0/9] PM / OPP: Multiple regulator support Viresh Kumar <viresh.kumar@linaro.org> - 2016-11-23 05:00 +0100
            Re: [PATCH V3 0/9] PM / OPP: Multiple regulator support Mark Brown <broonie@kernel.org> - 2016-11-23 13:40 +0100
              Re: [PATCH V3 0/9] PM / OPP: Multiple regulator support Viresh Kumar <viresh.kumar@linaro.org> - 2016-11-24 06:10 +0100
                Re: [PATCH V3 0/9] PM / OPP: Multiple regulator support Mark Brown <broonie@kernel.org> - 2016-11-24 11:20 +0100

#1524984 — Re: [PATCH V3 0/9] PM / OPP: Multiple regulator support

On 26-10-16, 12:02, Viresh Kumar wrote:
> Hi,
> 
> Some platforms (like TI) have complex DVFS configuration for CPU
> devices, where multiple regulators are required to be configured to
> change DVFS state of the device. This was explained well by Nishanth
> earlier [1].
> 
> One of the major complaints around multiple regulators case was that the
> DT isn't responsible in any way to represent the ordering in which
> multiple supplies need to be programmed, before or after frequency
> change. It was considered in this patch and such information is left to
> the platform specific OPP driver now, which can register its own
> opp_set_rate() callback with the OPP core and the OPP core will then
> call it during DVFS.
> 
> The patches are tested on Exynos5250 (Dual A15). I have hacked around DT
> and code to pass values for multiple regulators and verified that they
> are all properly read by the kernel (using debugfs interface).
> 
> Dave Gerlach has already tested it on the real TI platforms and it works
> well for him.
> 
> This is rebased over: linux-next branch in the PM tree.
> 
> V2->V3:
> - The last patch is new
> - Removed a debug leftover pr_info() message
> - Renamed few names as s/set_rate/set_opp
> - Removed a TODO comment (as it is done now with this series)
> - created struct for min_uV and max_uV
> - kerneldoc comments for structures in pm_opp.h
> - s/const char */const char * const
> - use kasprintf()
> - Some more minor reformatting
> - More Ack/RBY tags added

Hi guys,

Can we please get this series reviewed quickly and come to a conclusion? It has
already taken a lot of time getting this merged and the present code seems to be
the best possible shot we have, AFAIU.

-- 
viresh

[toc] | [next] | [standalone]

#1525171

[Multipart message — attachments visible in raw view] — view raw

On Fri, Nov 18, 2016 at 08:36:36AM +0530, Viresh Kumar wrote:

> Can we please get this series reviewed quickly and come to a conclusion? It has
> already taken a lot of time getting this merged and the present code seems to be
> the best possible shot we have, AFAIU.

There already seems to be extensive, ongoing discusion about this...

[toc] | [prev] | [next] | [standalone]

#1527205

On 18-11-16, 10:43, Mark Brown wrote:
> On Fri, Nov 18, 2016 at 08:36:36AM +0530, Viresh Kumar wrote:
> 
> > Can we please get this series reviewed quickly and come to a conclusion? It has
> > already taken a lot of time getting this merged and the present code seems to be
> > the best possible shot we have, AFAIU.
> 
> There already seems to be extensive, ongoing discusion about this...

And I am quite sure we are stuck again :)

I just wanted to say that we should get it to some sort of conclusion. And yes I
want to say thanks to all who invested their time on this thread :)

So the LAST remaining question is:

"How do we know (from the DT) the order in which entries for multiple regulators
are present in the OPP table?"

And I am not sure if we can do that without having a property like:

+               supply-names = "vcc0", "vcc1", "vcc2";

in the OPP table or the consumer device. And surely it isn't a clean enough
solution and that's why this series relied on the code to get such details.

Does someone have an alternative? If NO, can we go ahead with this series as is?

-- 
viresh

[toc] | [prev] | [next] | [standalone]

#1527843

[Multipart message — attachments visible in raw view] — view raw

On Tue, Nov 22, 2016 at 09:19:22AM +0530, Viresh Kumar wrote:

> So the LAST remaining question is:

> "How do we know (from the DT) the order in which entries for multiple regulators
> are present in the OPP table?"
> 
> And I am not sure if we can do that without having a property like:
> 
> +               supply-names = "vcc0", "vcc1", "vcc2";
> 
> in the OPP table or the consumer device. And surely it isn't a clean enough
> solution and that's why this series relied on the code to get such details.
> 
> Does someone have an alternative? If NO, can we go ahead with this series as is?

I'm really not at all clear why this has to be in DT.  My understanding
was that this is basically a helper library for more specific bindings
which already have to hard code things like sequencing so surely they'd
be specifying the ordering to be used when supplying data?

[toc] | [prev] | [next] | [standalone]

#1528116

On 22-11-16, 18:41, Mark Brown wrote:
> On Tue, Nov 22, 2016 at 09:19:22AM +0530, Viresh Kumar wrote:
> > "How do we know (from the DT) the order in which entries for multiple regulators
> > are present in the OPP table?"
> > 
> > And I am not sure if we can do that without having a property like:
> > 
> > +               supply-names = "vcc0", "vcc1", "vcc2";
> > 
> > in the OPP table or the consumer device. And surely it isn't a clean enough
> > solution and that's why this series relied on the code to get such details.
> > 
> > Does someone have an alternative? If NO, can we go ahead with this series as is?
> 
> I'm really not at all clear why this has to be in DT.  My understanding
> was that this is basically a helper library for more specific bindings
> which already have to hard code things like sequencing so surely they'd
> be specifying the ordering to be used when supplying data?

I am a bit confused and perhaps I am misreading your feedback.

Are you saying that:

"we don't need to identify which microVolts value in the OPP table corresponds
to which supply from the DT itself and we can do that with some hard coded
stuff" ?

If yes, then below is from an earlier email from you, which I feel is opposite
of what you are suggesting now.

On 09-11-16, 14:58, Mark Brown wrote:
> On Wed, Oct 26, 2016 at 12:02:56PM +0530, Viresh Kumar wrote:

> > The platform driver is responsible to identify the order and pass it on to the
> > OPP core. And the platform driver needs to have that hard coded.
>
> That *really* should be in the binding.  Honestly if the binding is this
> vague I'm not even clear that it's worth documenting these properties at
> this level, might be better to just put the documentation in the
> platform driver bindings.

-- 
viresh

[toc] | [prev] | [next] | [standalone]

#1528373

[Multipart message — attachments visible in raw view] — view raw

On Wed, Nov 23, 2016 at 09:16:57AM +0530, Viresh Kumar wrote:
> On 22-11-16, 18:41, Mark Brown wrote:

> > I'm really not at all clear why this has to be in DT.  My understanding
> > was that this is basically a helper library for more specific bindings
> > which already have to hard code things like sequencing so surely they'd
> > be specifying the ordering to be used when supplying data?

> I am a bit confused and perhaps I am misreading your feedback.

> Are you saying that:

> "we don't need to identify which microVolts value in the OPP table corresponds
> to which supply from the DT itself and we can do that with some hard coded
> stuff" ?

No, of course not.  That would be completely incoherent, there would be
no way for anything to use the data if the values can just be in any
random order.

> If yes, then below is from an earlier email from you, which I feel is opposite
> of what you are suggesting now.

> > That *really* should be in the binding.  Honestly if the binding is this
> > vague I'm not even clear that it's worth documenting these properties at
> > this level, might be better to just put the documentation in the
> > platform driver bindings.

The "platform driver bindings" bit of this is very important here.  This
is a generic binding that is going to be used by platform specific
drivers (as I understand it).  I would therefore expect that these
things can be described in the platform specific bindings.

Please, take a step back and think about what what the binding means and
how it's going to be used.  Not only is this a DT binding and therefore
an ABI it's also a generic binding that's going to affect a lot of
systems probably for a long time.  This means it is really important to
think things through and make sure we understand what they're doing.
When working on kernel internal code it's relatively easy to fix things
if we realize later that they don't work well so it's easier to just
work quickly but when we're making ABIs that's not possible.

[toc] | [prev] | [next] | [standalone]

#1528989

Hi Mark,

On 23-11-16, 12:29, Mark Brown wrote:
> On Wed, Nov 23, 2016 at 09:16:57AM +0530, Viresh Kumar wrote:
> > Are you saying that:
> 
> > "we don't need to identify which microVolts value in the OPP table corresponds
> > to which supply from the DT itself and we can do that with some hard coded
> > stuff" ?
> 
> No, of course not.  That would be completely incoherent, there would be
> no way for anything to use the data if the values can just be in any
> random order.

With the current implementation in this patchset, the order in which entries are
present in the OPP node is _assumed_ to be known to the platform specific code,
which will pass it on to the OPP core with some callbacks. So the order will not
be completely random.

> > If yes, then below is from an earlier email from you, which I feel is opposite
> > of what you are suggesting now.
> 
> > > That *really* should be in the binding.  Honestly if the binding is this
> > > vague I'm not even clear that it's worth documenting these properties at
> > > this level, might be better to just put the documentation in the
> > > platform driver bindings.
> 
> The "platform driver bindings" bit of this is very important here.  This
> is a generic binding that is going to be used by platform specific
> drivers (as I understand it).

There is no platform specific binding here.

For example in case of a single regulator for the device (CPU), the platform
specific DT file contains the CPU nodes (using generic bindings) and an OPP
table node (again generic bindings only). The OPP core reads both these nodes
for the device and constructs the OPP table.

Now in case of multiple regulators for the device, as you already know, the only
unanswered detail (apart from the order in which the regulators need to be
programmed) is to link which entries in the OPP table are for which regulator.

We can either get this information from DT (somehow) or hardcode it in platform
specific code. This patch provided infrastructure for the later one.

If we indeed want to get this information from the DT then there are two
options:

- Create platform specific binding:

  foo-platform,supply-names = "vcc0", "vcc1", "vcc2";

- Create common binding that can be used by all platforms:

  supply-names = "vcc0", "vcc1", "vcc2";


Such bindings will end up either in the consumer device node (like CPU0 node) or
in the OPP table itself. I am personally inclined towards the common
supply-names bindings, otherwise every user platform will end up creating very
similar bindings.

> I would therefore expect that these
> things can be described in the platform specific bindings.
> 
> Please, take a step back and think about what what the binding means and
> how it's going to be used.  Not only is this a DT binding and therefore
> an ABI it's also a generic binding that's going to affect a lot of
> systems probably for a long time.  This means it is really important to
> think things through and make sure we understand what they're doing.
> When working on kernel internal code it's relatively easy to fix things
> if we realize later that they don't work well so it's easier to just
> work quickly but when we're making ABIs that's not possible.

I agree and I completely understand your concerns here and it is surely very
important to get the bindings right as they will last for very long.

But I am still unsure about what's the best way of doing this. The new bindings
are rejected by almost everyone as they contain some of the information already
contained in the consumer node while the regulators are defined.

-- 
viresh

[toc] | [prev] | [next] | [standalone]

#1529128

[Multipart message — attachments visible in raw view] — view raw

On Thu, Nov 24, 2016 at 10:37:24AM +0530, Viresh Kumar wrote:
> On 23-11-16, 12:29, Mark Brown wrote:

> > No, of course not.  That would be completely incoherent, there would be
> > no way for anything to use the data if the values can just be in any
> > random order.

> With the current implementation in this patchset, the order in which entries are
> present in the OPP node is _assumed_ to be known to the platform specific code,
> which will pass it on to the OPP core with some callbacks. So the order will not
> be completely random.

What we're reviewing here is the DT binding and the DT binding
explicitly said the order doesn't matter.  The DT binding is OS neutral
so it needs to make sense without the code.

> > The "platform driver bindings" bit of this is very important here.  This
> > is a generic binding that is going to be used by platform specific
> > drivers (as I understand it).

> There is no platform specific binding here.

It seems like we're going to need one for this to be a comprehensible
binding.

> We can either get this information from DT (somehow) or hardcode it in platform
> specific code. This patch provided infrastructure for the later one.

> If we indeed want to get this information from the DT then there are two
> options:

Why would we want to get it from DT when we can't get half the other
information we need to make the data useful from DT?

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web