Regulator supplies when using Device Tree

Regulator supplies when using Device Tree

[Michael Bohan]

Hi,

I'm curious if there was a reason we didn't standardize on a binding 
name for regulator supplies when using Device Tree. This appears to 
cause duplicated code for regulator drivers that support devices that 
may or may not have supplies specified.

For example, if one were to set rdesc->supply_name to a chosen supply 
name "parent" and that particular device does not exist in the Device 
Tree topology, then regulator_register() will fail. So in the driver, we 
have to first use of_get_property() to verify that "parent-supply" is 
defined. Only then do we set rdesc->supply_name. Since I have multiple 
regulator drivers that may or may not have supplies, each has to 
duplicate this check.

But it seems like if we agreed on a standardized supply name for 
regulators, then we could move that complexity to 
of_get_regulator_init_data(). Having multiple supply names sure makes 
sense for consumers, but for supplies, there can ever only be one.

I guess another alternative is to have the driver unconditionally assign 
rdesc->supply_name, but change the framework to not fail the 
regulator_register() if the supply is not specified in the topology. If 
it is specified but fails either a phandle lookup or the targeted supply 
regulator is not valid, then fail only in those cases.

Also, I'm curious why we need two pointers for the supply name. There's 
currently regulator_desc->supply_name, recently added for Device Tree, 
and then the old init_data->supply_regulator. Is there a need for both?

Thanks,
Mike

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: Regulator supplies when using Device Tree

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 767 bytes --]

On Thu, Mar 22, 2012 at 06:17:59PM -0700, Michael Bohan wrote:

> I'm curious if there was a reason we didn't standardize on a binding
> name for regulator supplies when using Device Tree. This appears to
> cause duplicated code for regulator drivers that support devices
> that may or may not have supplies specified.

Supplies are *always* specified using the name from the part data sheet,
anything to do with regulator-regulator supplies is a Linux
implementation detail.

> Also, I'm curious why we need two pointers for the supply name.
> There's currently regulator_desc->supply_name, recently added for
> Device Tree, and then the old init_data->supply_regulator. Is there
> a need for both?

We can't just break the build for systems using supply_regulator.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Regulator supplies when using Device Tree

[Michael Bohan]

On 3/26/2012 6:00 AM, Mark Brown wrote:
> On Thu, Mar 22, 2012 at 06:17:59PM -0700, Michael Bohan wrote:
>
>> I'm curious if there was a reason we didn't standardize on a binding
>> name for regulator supplies when using Device Tree. This appears to
>> cause duplicated code for regulator drivers that support devices
>> that may or may not have supplies specified.
>
> Supplies are *always* specified using the name from the part data sheet,
> anything to do with regulator-regulator supplies is a Linux
> implementation detail.

So before filling out the supply_name when calling regulator_register(), 
does that mean we should expect regulator drivers that optionally 
support supplies to always check with of_get_property()? And which name 
should we check? It sounds like the answer is that we should invent 
another binding to portray the name of the supply the driver should be 
checking against. But then it would seem silly to have two bindings that 
pertain to supply names.

Thanks,
Mike

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: Regulator supplies when using Device Tree

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 933 bytes --]

On Tue, Mar 27, 2012 at 06:38:30PM -0700, Michael Bohan wrote:
> On 3/26/2012 6:00 AM, Mark Brown wrote:

> >Supplies are *always* specified using the name from the part data sheet,
> >anything to do with regulator-regulator supplies is a Linux
> >implementation detail.

> So before filling out the supply_name when calling
> regulator_register(), does that mean we should expect regulator
> drivers that optionally support supplies to always check with
> of_get_property()? And which name should we check? It sounds like

I have no idea what this means, sorry.

> the answer is that we should invent another binding to portray the
> name of the supply the driver should be checking against. But then
> it would seem silly to have two bindings that pertain to supply
> names.

Absolutely not, that would be broken.  The whole point here is that
supplies of all kinds are always requested with the name the chip uses
for the supply.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Regulator supplies when using Device Tree

[Michael Bohan]

On 3/28/2012 3:09 AM, Mark Brown wrote:
> On Tue, Mar 27, 2012 at 06:38:30PM -0700, Michael Bohan wrote:
>> the answer is that we should invent another binding to portray the
>> name of the supply the driver should be checking against. But then
>> it would seem silly to have two bindings that pertain to supply
>> names.
>
> Absolutely not, that would be broken.  The whole point here is that
> supplies of all kinds are always requested with the name the chip uses
> for the supply.

Let's consider an example with two regulators:

regulator@0 {
   compatible = "ldo_driver";
   foo-supply = <&smps1>;
};

regulator@1 {
   compatible = "ldo_driver";
};

How do we write a single regulator driver that supports both of these 
regulator devices?

Within the regulator driver, we currently have to do an 
of_get_property(of_node, "foo-supply", NULL) to determine whether the 
device has a supply, and thus whether we should assign 
rdesc->supply_name to "foo" or not when calling regulator_register(). Is 
there a better way to do this? If we don't do this check for the case 
where a device does not have a supply specified in the Device Tree, then 
regulator_register() will fail.

Before Device Tree, regulators could get their supply names directly 
from the board file like so:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=Documentation/power/regulator/machine.txt;h=ce63af0a8e35ecab32e2f326d13a9a2b33b62909;hb=refs/heads/master#l67

Thus the regulator driver was never concerned about the specifics of a 
supply name. And if the supply name was not specified, then the 
regulator_register() would happily succeed without any checks in the 
driver.

Thanks,
Mike

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: Regulator supplies when using Device Tree

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1229 bytes --]

On Wed, Mar 28, 2012 at 12:19:45PM -0700, Michael Bohan wrote:
> On 3/28/2012 3:09 AM, Mark Brown wrote:
> >On Tue, Mar 27, 2012 at 06:38:30PM -0700, Michael Bohan wrote:

> >Absolutely not, that would be broken.  The whole point here is that
> >supplies of all kinds are always requested with the name the chip uses
> >for the supply.

> Within the regulator driver, we currently have to do an
> of_get_property(of_node, "foo-supply", NULL) to determine whether
> the device has a supply, and thus whether we should assign
> rdesc->supply_name to "foo" or not when calling

No, this would be completely idiotic.  Please think about what I'm
saying here.  To repeat, supplies of all kinds are always requested with
the name the chip uses for the supply.  This means that any machine
binding is *totally* irrelevant to the regulator driver.

> regulator_register(). Is there a better way to do this? If we don't
> do this check for the case where a device does not have a supply
> specified in the Device Tree, then regulator_register() will fail.

Obviously there's a better way to do this.  For one thing as soon as we
find ourselves saying "let's open code this in every single driver"
we've got a clear abstraction problem...

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Regulator supplies when using Device Tree

[Michael Bohan]

On 3/28/2012 12:33 PM, Mark Brown wrote:
> On Wed, Mar 28, 2012 at 12:19:45PM -0700, Michael Bohan wrote:
>> Within the regulator driver, we currently have to do an
>> of_get_property(of_node, "foo-supply", NULL) to determine whether
>> the device has a supply, and thus whether we should assign
>> rdesc->supply_name to "foo" or not when calling
>
> No, this would be completely idiotic.  Please think about what I'm
> saying here.  To repeat, supplies of all kinds are always requested with
> the name the chip uses for the supply.  This means that any machine
> binding is *totally* irrelevant to the regulator driver.

I'm not sure we're even talking about the same problem here. I agree 
that the name used for the supply should correspond with the data sheet 
- I just don't know how that's relevant.

Put simply, whose responsibility is it to assign the 
regulator_desc->supply_name pointer before registering a regulator 
device added from Device Tree? And do you agree that if you assign this 
pointer to a name for which there isn't a Device Tree property specified 
in that device_node, then regulator_register() will fail? This happens 
because of_get_regulator() tacks on a "-supply" at the end and then 
calls of_get_property() on the resulting string. That call will fail 
since there is no property specified in the device_node. This is the 
scenario I'm trying to avoid. Thus I add an additional check with 
of_get_property() in my driver to see if the property does exist before 
registering the regulator device, since both cases are reasonable. One 
is a regulator device with a supply, and one is a regulator device 
without a supply.

Are you aware of any other examples of submitted drivers with Device 
Tree support that implement regulator devices that optionally have an 
upstream supply? I looked at your tree recently and couldn't see any 
such cases.

Thanks,
Mike

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: Regulator supplies when using Device Tree

[Rajendra Nayak]

On Thursday 29 March 2012 05:36 AM, Michael Bohan wrote:
> Put simply, whose responsibility is it to assign the
> regulator_desc->supply_name pointer before registering a regulator
> device added from Device Tree?

The drivers, and its expected to know, and not lookup something from the 
DT node to figure out what regulator_desc->supply_name should be.

You can read through the discussion when the bindings were being
proposed.
http://www.spinics.net/lists/linux-omap/msg58395.html

Re: Regulator supplies when using Device Tree

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 639 bytes --]

On Thu, Mar 29, 2012 at 10:14:05AM +0530, Rajendra Nayak wrote:
> On Thursday 29 March 2012 05:36 AM, Michael Bohan wrote:
> >Put simply, whose responsibility is it to assign the
> >regulator_desc->supply_name pointer before registering a regulator
> >device added from Device Tree?

> The drivers, and its expected to know, and not lookup something from
> the DT node to figure out what regulator_desc->supply_name should
> be.

Also note that this stuff is essentially unrelated to device tree,
exactly the same mechanism should be used by all drivers it's just that
we happened to improve the framework when adding device tree support.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Regulator supplies when using Device Tree

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 430 bytes --]

On Wed, Mar 28, 2012 at 05:06:48PM -0700, Michael Bohan wrote:

> Are you aware of any other examples of submitted drivers with Device
> Tree support that implement regulator devices that optionally have
> an upstream supply? I looked at your tree recently and couldn't see
> any such cases.

No, pretty much any drivers which optionally have an upstream supply
would be buggy and should therefore run into trouble during review.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Regulator supplies when using Device Tree

[Michael Bohan]

On 3/29/2012 4:11 AM, Mark Brown wrote:
> On Wed, Mar 28, 2012 at 05:06:48PM -0700, Michael Bohan wrote:
>
>> Are you aware of any other examples of submitted drivers with Device
>> Tree support that implement regulator devices that optionally have
>> an upstream supply? I looked at your tree recently and couldn't see
>> any such cases.
>
> No, pretty much any drivers which optionally have an upstream supply
> would be buggy and should therefore run into trouble during review.

Can you please elaborate on why this is a bad design? We have always 
used this model in the past, and the regulator framework is happy to 
support it.

We have identical hardware blocks that can be positioned to take a 
supply or not. So are you proposing we copy / paste our driver so that 
in one case rdesc->supply_name is NULL and the other case a valid supply 
name? I'm guessing you aren't implying this, but what alternatives do 
you suggest to support such a model?

In the thread mentioned by Rajendra, part of the motivation for putting 
the supply_name in the driver struct was 'no code changes in the 
individual drivers'. But that's exactly what we have here.

http://www.spinics.net/lists/linux-omap/msg58673.html

I was hoping that we could continue to treat regulator supplies as 
normal supplies, but somehow allow the framework to determine whether a 
regulator supply exists or not in the Device Tree configuration so the 
driver doesn't have to.

Thanks,
Mike

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: Regulator supplies when using Device Tree

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1969 bytes --]

On Thu, Mar 29, 2012 at 06:18:44PM -0700, Michael Bohan wrote:
> On 3/29/2012 4:11 AM, Mark Brown wrote:
> >On Wed, Mar 28, 2012 at 05:06:48PM -0700, Michael Bohan wrote:

> >>Are you aware of any other examples of submitted drivers with Device
> >>Tree support that implement regulator devices that optionally have
> >>an upstream supply? I looked at your tree recently and couldn't see
> >>any such cases.

> >No, pretty much any drivers which optionally have an upstream supply
> >would be buggy and should therefore run into trouble during review.

> Can you please elaborate on why this is a bad design? We have always
> used this model in the past, and the regulator framework is happy to
> support it.

The support for regulator-regulator supplies has always been problematic
and infrequently used -

> We have identical hardware blocks that can be positioned to take a
> supply or not. So are you proposing we copy / paste our driver so
> that in one case rdesc->supply_name is NULL and the other case a
> valid supply name? I'm guessing you aren't implying this, but what
> alternatives do you suggest to support such a model?

Please be more specific about what you're actually trying to physically
accomplish here.  It would be *extremely* unusual to see a regulator
which was able to do something useful without any input power and if you
genuinely have this need whatever you're doing probably needs the
framework to actually be aware of what's going on.  However...

> I was hoping that we could continue to treat regulator supplies as
> normal supplies, but somehow allow the framework to determine
> whether a regulator supply exists or not in the Device Tree
> configuration so the driver doesn't have to.

...this doesn't really make any sense - the new scheme makes regulator
supplies *more* like normal supplies, not less, since they're now
specified in exactly the same way as other supplies and are just assumed
to be provided by the leaf driver.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Regulator supplies when using Device Tree

[Michael Bohan]

On 3/30/2012 3:36 AM, Mark Brown wrote:
> On Thu, Mar 29, 2012 at 06:18:44PM -0700, Michael Bohan wrote:
>> Can you please elaborate on why this is a bad design? We have always
>> used this model in the past, and the regulator framework is happy to
>> support it.
>
> The support for regulator-regulator supplies has always been problematic
> and infrequently used -

Hmm, I'd be interested to know the history there.

> Please be more specific about what you're actually trying to physically
> accomplish here.  It would be *extremely* unusual to see a regulator
> which was able to do something useful without any input power and if you
> genuinely have this need whatever you're doing probably needs the
> framework to actually be aware of what's going on.  However...

Some of our regulators take inputs from other regulators. Some 
regulators take their input from the battery. We support both types in 
the same driver, since the hardware is the same. Thus this supply 
configuration is natural and matches our hardware closely. The actual 
connections of our regulators varies between different board designs.

If you're interested, you can checkout some sample driver code:

https://www.codeaurora.org/gitweb/quic/la/?p=kernel/msm.git;a=blob;f=drivers/regulator/pm8xxx-regulator.c;h=fd691f051428158d3543dd7562e4f5e26e905962;hb=msm-3.0

And see how we pass the supply names and other parameters here:

https://www.codeaurora.org/gitweb/quic/la/?p=kernel/msm.git;a=blob;f=arch/arm/mach-msm/board-8960-regulator.c;h=23e3e690ff0e582fd947553a51015f5059750cdf;hb=msm-3.0

We have a table of devices with associated data attributes -- one of 
them being supplies. For devices with supplies, the name is specified. 
For devices without supplies, nothing is specified. This works well 
since then the driver doesn't need to check whether there exists a 
supply or not.

>> I was hoping that we could continue to treat regulator supplies as
>> normal supplies, but somehow allow the framework to determine
>> whether a regulator supply exists or not in the Device Tree
>> configuration so the driver doesn't have to.
>
> ...this doesn't really make any sense - the new scheme makes regulator
> supplies *more* like normal supplies, not less, since they're now
> specified in exactly the same way as other supplies and are just assumed
> to be provided by the leaf driver.

I don't disagree that the new scheme makes supplies more like normal 
supplies. I'm just pointing out the solution doesn't appear perfect 
since extraneous code is put in the driver in this case.

Thanks,
Mike

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: Regulator supplies when using Device Tree

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1248 bytes --]

On Mon, Apr 02, 2012 at 10:35:22AM -0700, Michael Bohan wrote:
> On 3/30/2012 3:36 AM, Mark Brown wrote:

> >The support for regulator-regulator supplies has always been problematic
> >and infrequently used -

> Hmm, I'd be interested to know the history there.

git logs should be instructive.  The use part is just because most board
designs end up thinking of better things to do with their DCDCs than
drop the supply for LDOs.

> >Please be more specific about what you're actually trying to physically
> >accomplish here.  It would be *extremely* unusual to see a regulator

> Some of our regulators take inputs from other regulators. Some
> regulators take their input from the battery. We support both types

Oh, if that's all it is that's totally normal and unsurprising.  Just
tell the regulator API about the battery supply, typically people use
a fixed regulator for this.  You don't have to specify a voltage when
you use them.

> I don't disagree that the new scheme makes supplies more like normal
> supplies. I'm just pointing out the solution doesn't appear perfect
> since extraneous code is put in the driver in this case.

To repeat yet again if you're putting code for this in the driver you've
got a clear abstraction failure.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Regulator supplies when using Device Tree

[Michael Bohan]

On 4/2/2012 2:22 PM, Mark Brown wrote:
> On Mon, Apr 02, 2012 at 10:35:22AM -0700, Michael Bohan wrote:
>> Some of our regulators take inputs from other regulators. Some
>> regulators take their input from the battery. We support both types
>
> Oh, if that's all it is that's totally normal and unsurprising.  Just
> tell the regulator API about the battery supply, typically people use
> a fixed regulator for this.  You don't have to specify a voltage when
> you use them.

One problem here is the added run-time overhead. This includes the 
callbacks for the battery device which serve no functional purpose. And 
then there's the contention for a single mutex on the battery regulator, 
which all regulators using it as a supply would have to contend for. 
That is actually a sizable number of devices in our configuration; And 
these operations are frequent and time sensitive.

Plus, then there's other complexities that arise here by introducing yet 
another regulator driver into the configuration. The 'fixed' regulator 
driver initializes at subsys_initcall, but our other regulator drivers 
are registered at arch_initcall. This is because with Device Tree 
configurations we are forced to explicitly register drivers in a precise 
sequence in order to satisfy device dependencies and early probe times. 
But using 'fixed' would complicate this process, since we don't have 
control over it.

And generally speaking, why incur a larger memory footprint by linking 
another driver when there's no technical need to?

Thus it seems like there are use cases to have a single driver that can 
support regulators with or without supplies specified. I see adding a 
few lines of ugly code in the driver more ideal than accepting the 
problems above. But it would be nice if the regulator Device Tree 
support could handle it natively.

Thanks,
Mike

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: Regulator supplies when using Device Tree

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 4193 bytes --]

On Mon, Apr 02, 2012 at 06:53:33PM -0700, Michael Bohan wrote:
> On 4/2/2012 2:22 PM, Mark Brown wrote:

> >Oh, if that's all it is that's totally normal and unsurprising.  Just
> >tell the regulator API about the battery supply, typically people use
> >a fixed regulator for this.  You don't have to specify a voltage when
> >you use them.

> One problem here is the added run-time overhead. This includes the
> callbacks for the battery device which serve no functional purpose.
> And then there's the contention for a single mutex on the battery
> regulator, which all regulators using it as a supply would have to
> contend for. That is actually a sizable number of devices in our
> configuration; And these operations are frequent and time sensitive.

None of this is particularly related to telling the system the regulator
has a parent, it's all to do with the fact that there's room for
improvement in the handling of simple regulators.  Right now we're just
doing the simplest thing possible most of the time, generally the CPU
overhead is trivial compared to the amount of time it takes to actually
do anything to the hardware so it's not much of a problem.

> Plus, then there's other complexities that arise here by introducing
> yet another regulator driver into the configuration. The 'fixed'
> regulator driver initializes at subsys_initcall, but our other

All the init ordering stuff should be dealt with using the probe
deferral stuff.  The main trick is making sure that cpufreq (and any
other deviceless consumers) are registered after all the regulators come
up, though obviously what we really want there is to make sure we don't
have any consumers (or drivers generally) that don't have a struct
device associated with them so we can actually do things like this.

> And generally speaking, why incur a larger memory footprint by
> linking another driver when there's no technical need to?

I'm finding it hard to worry, frankly - the overhead is small enough in
the grand scheme of things and there's nothing magic or especially
resource intensive about regulator-regulator supplies which means that
they're the only place where it's worth saving memory.

> Thus it seems like there are use cases to have a single driver that
> can support regulators with or without supplies specified. I see
> adding a few lines of ugly code in the driver more ideal than
> accepting the problems above. But it would be nice if the regulator
> Device Tree support could handle it natively.

To repeat, open coding anything like this in every single driver (which
is what you're suggesting) is obviously an abstraction failure.  Please
stop suggesting this.

In general a lack of thought for abstraction seems to be your issue here
- you've thought of one particular optimisation for a very specific case
and you're focusing too much on how to deploy that rather than thinking
more generally.  We've not even tried to improve the speed yet so we
shouldn't be looking at microoptimisations yet, they'll just add
complicaton.  This has to work well on other people's systems too and
things have to be maintainable going forward.

The most obvious thing (which I keep meaning to do anyway for
diagnostics) is to reintroduce the per consumer and per regulator
reference counts then use those to avoid going up the tree when we can.
If always on and other stub regulators are a bottleneck there's plenty
of optimisation we can do there too - for example, we can use the fact
that the parent is always on to avoid propagating things up the tree (or
avoid doing so expensively), or if we notice that the regulator is just
a stub when it's being registered we could replace it with a null
pointer and then handle that.

These are just the things I thought of within a few moments and will
bring benefits to any supply that fits, not just those where one
regulator happens to supply another.  I'm sure that if we thought a bit
more we could come up with plenty more optimisations.

In short, just describe your hardware accurately in the device tree and
then optimise the software to deal with it better.  There's plenty of
low hanging fruit that haven't been touched yet, don't microoptimise.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]