Regulator supplies when using Device Tree

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.

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

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

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.

I have no idea what this means, sorry.

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.

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.

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...

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.

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

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.

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.

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

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...

...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.

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

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 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.

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.

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.

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

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.

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.

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.

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.

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.