Re: [PATCH] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes
On Wed 18 Feb 18:29 PST 2015, Stephen Boyd wrote: > On 02/18/15 13:08, Bjorn Andersson wrote: > > On Wed, Feb 18, 2015 at 12:28 PM, Stephen Boyd wrote: After taking a deeper look at this I've come to the following conclusion: We can save 2100 bytes of data by spreading out the magic numbers from the rpm resource tables into the regulator, clock and bus drivers. At the cost of having this rpm-specific information spread out. Separate of that we could rewrite the entire regulator implementation to define all regulators in platform specific tables in the regulators. This would save about 12-15k of ram. This can however be done in two ways: 1) We modify the mfd driver to instatiate 3 mfd_cells; one of them being "qcom,rpm-regulators". We modify the regulator driver to have tables of the regulators for each platform and matching regulator parameters by of_node name and registering these. 2) We stick with this binding, we then go ahead and do the same modification to the mfd as above - passing the rpm of_node to the regulator driver, that walks the children and match that to the current compatible list. (in other words, we replace of_platform_populate with our own mechamism). The benefit of 2 is that we can use the code that was posted 10 months ago and merged 3 months ago until someone prioritize those 12kb. To take either of these paths the issue at the bottom has to be resolved first. More answers follows inline: > > We're already maintaining these tables in qcom-rpm.c. I'm advocating for > removing those tables from the rpm driver and putting the data into the > regulator driver. Then we don't have to index into a sparse table to > figure out what values the RPM wants to use. Instead we have the data at > hand for a particular regulator based on the of_regulator_match. > I do like the "separation of concerns" between the rpm driver and the children, but you're right in that we're wasting almost 3kb in doing so: (gdb) p sizeof(apq8064_rpm_resource_table) + sizeof(msm8660_rpm_resource_table) + sizeof(msm8960_rpm_resource_table) $2 = 6384 vs (gdb) p sizeof(struct qcom_rpm_resource) * (77 + 74 + 73) $3 = 3584 The alternative would be to spread those magic constants out in the three client drivers. Looking at this from a software design perspective I stand by the argument that the register layout of the rpm is not a regulator driver implementation detail and is better kept separate. As we decided to define the regulators in code but the actual composition in dt it was not possible to put the individual numbers in DT. Having reg = <165 68 48> does not make any sense, unless we perhaps maintain said table in the dt binding documentation. > I don't understand the maintenance argument either. The data has to go > somewhere so the maintenance is always there. > Well, you used exactly that argument when you questioned the way I did pinctrl-msm, with a table per platform. > From what I can tell, that data is split in two places. The regulator > type knows how big the data we want to send is (1 or 2) and that is > checked in the RPM driver to make sure that the two agree (this part > looks like over-defensive coding). Yeah, after debugging production code for years I like to be slightly on the defensive side. But the size could of course be dropped from the resource-tables; reducing the savings of your suggestion by another 800 bytes... > Then the DT has a made up number that > maps to 3 different numbers in the RPM driver. Reading the following snippet in my dts file makes imidiate sense: reg = the following doesn't make any sense: reg = <116 31 30>; Maybe it's write-once in a dts so it doesn't matter that much - as long as the node is descriptive. But I like the properties to be human understandable. > The same could be done > for b-family, where we have two numbers that just so happen to be user > friendly enough to make sense on their own. Instead of being 165, 68, > and 48, they're a #define SMPS and 1. We could make a #define SMPS1 and > map it to a tuple with #define SMPS and 1. It looks like that was how > you had b-family prototyped at one point. > In family B things are completely different. The reason why I proposed the similar setup was simply because I wanted to keep the api similar (or even the same). But the mechanism is completely different; The regulator driver builds up a key-value-pair list and encapsulte this in a packet with a header that includes 'type' and 'id' of the resource to be modified. This is then put in the packet-channel (smd) that goes to the rpm. The types are magic numbers but the id correlates to the resource id of that type - so it doesn't give anything to maintain any tables for family B. Family A and B are so different that it doesn't make sense to share any code, it was however my indention to have the dt bindings follow the same structure! > > > > So for family B, where this is not necessary for the rpm driver, I > > have revised this
Re: [PATCH] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes
On Wed 18 Feb 18:29 PST 2015, Stephen Boyd wrote: On 02/18/15 13:08, Bjorn Andersson wrote: On Wed, Feb 18, 2015 at 12:28 PM, Stephen Boyd sb...@codeaurora.org wrote: After taking a deeper look at this I've come to the following conclusion: We can save 2100 bytes of data by spreading out the magic numbers from the rpm resource tables into the regulator, clock and bus drivers. At the cost of having this rpm-specific information spread out. Separate of that we could rewrite the entire regulator implementation to define all regulators in platform specific tables in the regulators. This would save about 12-15k of ram. This can however be done in two ways: 1) We modify the mfd driver to instatiate 3 mfd_cells; one of them being qcom,rpm-regulators. We modify the regulator driver to have tables of the regulators for each platform and matching regulator parameters by of_node name and registering these. 2) We stick with this binding, we then go ahead and do the same modification to the mfd as above - passing the rpm of_node to the regulator driver, that walks the children and match that to the current compatible list. (in other words, we replace of_platform_populate with our own mechamism). The benefit of 2 is that we can use the code that was posted 10 months ago and merged 3 months ago until someone prioritize those 12kb. To take either of these paths the issue at the bottom has to be resolved first. More answers follows inline: We're already maintaining these tables in qcom-rpm.c. I'm advocating for removing those tables from the rpm driver and putting the data into the regulator driver. Then we don't have to index into a sparse table to figure out what values the RPM wants to use. Instead we have the data at hand for a particular regulator based on the of_regulator_match. I do like the separation of concerns between the rpm driver and the children, but you're right in that we're wasting almost 3kb in doing so: (gdb) p sizeof(apq8064_rpm_resource_table) + sizeof(msm8660_rpm_resource_table) + sizeof(msm8960_rpm_resource_table) $2 = 6384 vs (gdb) p sizeof(struct qcom_rpm_resource) * (77 + 74 + 73) $3 = 3584 The alternative would be to spread those magic constants out in the three client drivers. Looking at this from a software design perspective I stand by the argument that the register layout of the rpm is not a regulator driver implementation detail and is better kept separate. As we decided to define the regulators in code but the actual composition in dt it was not possible to put the individual numbers in DT. Having reg = 165 68 48 does not make any sense, unless we perhaps maintain said table in the dt binding documentation. I don't understand the maintenance argument either. The data has to go somewhere so the maintenance is always there. Well, you used exactly that argument when you questioned the way I did pinctrl-msm, with a table per platform. From what I can tell, that data is split in two places. The regulator type knows how big the data we want to send is (1 or 2) and that is checked in the RPM driver to make sure that the two agree (this part looks like over-defensive coding). Yeah, after debugging production code for years I like to be slightly on the defensive side. But the size could of course be dropped from the resource-tables; reducing the savings of your suggestion by another 800 bytes... Then the DT has a made up number that maps to 3 different numbers in the RPM driver. Reading the following snippet in my dts file makes imidiate sense: reg = QCOM_RPM_PM8921_SMPS1 the following doesn't make any sense: reg = 116 31 30; Maybe it's write-once in a dts so it doesn't matter that much - as long as the node is descriptive. But I like the properties to be human understandable. The same could be done for b-family, where we have two numbers that just so happen to be user friendly enough to make sense on their own. Instead of being 165, 68, and 48, they're a #define SMPS and 1. We could make a #define SMPS1 and map it to a tuple with #define SMPS and 1. It looks like that was how you had b-family prototyped at one point. In family B things are completely different. The reason why I proposed the similar setup was simply because I wanted to keep the api similar (or even the same). But the mechanism is completely different; The regulator driver builds up a key-value-pair list and encapsulte this in a packet with a header that includes 'type' and 'id' of the resource to be modified. This is then put in the packet-channel (smd) that goes to the rpm. The types are magic numbers but the id correlates to the resource id of that type - so it doesn't give anything to maintain any tables for family B. Family A and B are so different that it doesn't make sense to share any code, it was however my indention to have the dt bindings follow the same structure! So for family B, where this is not necessary for the rpm driver, I have revised this
Re: [PATCH] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes
On 02/18/15 13:08, Bjorn Andersson wrote:
> On Wed, Feb 18, 2015 at 12:28 PM, Stephen Boyd wrote:
>>
>> MFD name matching isn't required. All we need to do is have a regulators
>> node and put a compatible = "qcom,rpm-msm-regulators" in there. Then
>> of_platform_populate() does most of the work and we rework the RPM
>> driver to match on this compatible. Thus the regulator stuff is
>> encapsulated in the drivers/regulator/ directory.
>>
> Right, this would only affect the mfd children...
>
>
>> e.g.
>>
>> rpm@108000 {
>> compatible = "qcom,rpm-msm8960";
>> reg = <0x108000 0x1000>;
>> qcom,ipc = < 0x8 2>;
>>
>> interrupts = <0 19 0>, <0 21 0>, <0 22 0>;
>> interrupt-names = "ack", "err", "wakeup";
>>
>> regulators {
>> compatible = "qcom,rpm-msm8960-regulators";
>>
>> s1: s1 {
>> regulator-min-microvolt = <1225000>;
>> regulator-max-microvolt = <1225000>;
>> bias-pull-down;
>> qcom,switch-mode-frequency = <320>;
>> };
> This means that the regulator driver needs to have a list of
> regulators per platform supported.
>
> If we keep the compatible in the regulator nodes, we can use that for
> matching with an implementation - still without using the
> platform_device model for instantiating them. We would not need said
> list in that case.
Agreed. It was intentional because the type of regulator is another
static implementation detail.
>
>> ...
>> };
>>
>> rpmcc: clock-controller (or clocks?) {
>> compatible = "qcom,rpm-msm8960-clocks";
>> #clock-cells = <1>;
>> };
>> };
>>
>> This is probably missing a size-cells somewhere, but you get the idea. I
>> intentionally named the node "s1" in the hopes of the compiler
>> consolidating the multiple "s1" strings for all the different pmic match
>> tables into one string in some literal pool somewhere. Also, I removed
>> reg from the regulator nodes to stay flexible in case we want to change
>> the rpm write API in the future (it would go into the match table as
>> driver data).
> If we fall back to define the regulators in the code and map the
> property nodes by name, then we can just add the content of the reg
> property in those tables as well.
Yep.
>
>> (Goes to look at the RPM write API...)
>>
>> BTW, some of those rpm tables are going to be huge when you consider
>> that the flat number space of QCOM_RPM_ is monotonically
>> increasing but the actual resources used by a particular PMIC is only a
>> subset of that space. For example, some arrays might only have resources
>> that start at 100, so the first 100 entries in the array are wasted
>> space. Maybe the rpm write API shouldn't be doing this fake resource
>> numbering thing. Instead it should rely on the client drivers to pack up
>> a structure that the write API can interpret, i.e. push the resource
>> tables out to the drivers.
>>
> Correct, but David Collins comment on the subject was clear; we don't
> want these tables in the code. And after playing around with various
> options I came to the same conclusion - maintaining the tables will be
> a mess.
We're already maintaining these tables in qcom-rpm.c. I'm advocating for
removing those tables from the rpm driver and putting the data into the
regulator driver. Then we don't have to index into a sparse table to
figure out what values the RPM wants to use. Instead we have the data at
hand for a particular regulator based on the of_regulator_match.
I don't understand the maintenance argument either. The data has to go
somewhere so the maintenance is always there.
>From what I can tell, that data is split in two places. The regulator
type knows how big the data we want to send is (1 or 2) and that is
checked in the RPM driver to make sure that the two agree (this part
looks like over-defensive coding). Then the DT has a made up number that
maps to 3 different numbers in the RPM driver. The same could be done
for b-family, where we have two numbers that just so happen to be user
friendly enough to make sense on their own. Instead of being 165, 68,
and 48, they're a #define SMPS and 1. We could make a #define SMPS1 and
map it to a tuple with #define SMPS and 1. It looks like that was how
you had b-family prototyped at one point.
>
> So for family B, where this is not necessary for the rpm driver, I
> have revised this to instead be:
>
> #address-cells = <2>;
> smps1 {
>reg = ;
> }
>
> With #define QCOM_SMD_RPM_SMPA 0x61706d73, this information goes
> straight into the smd packet and we don't have any tables at all.
How does the rpm write API look there? Does it take two numbers
(resource type and resource id) instead of 1 (enum)? Is the plan to keep
the write API the same as what's in qcom-rpm.c today?
>
>
> Josh made an attempt to encode the data needed for family A in the
Re: [PATCH] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes
On Wed, Feb 18, 2015 at 12:28 PM, Stephen Boyd wrote:
> On 02/18/15 10:37, Bjorn Andersson wrote:
>> On Tue, Feb 17, 2015 at 6:52 PM, Stephen Boyd wrote:
>>> The main benefit I can think of is we cut down on runtime memory bloat.
>>>
>>> (gdb) p sizeof(struct platform_device)
>>> $1 = 624
>>>
>>> Multiply that by 20 regulators and you get 624 * 20 = 12480 bytes in
>>> platform devices. If we had one platform_device for all RPM controlled
>>> regulators that would reduce this number from ~12k to ~0.5K. It would
>>> require of_regulator_match() and the (undesirable) lists of regulator
>>> names for all the different pmic variants, or we would need to pick out
>>> the regulator nodes based on compatible matching. Is that so bad? In the
>>> other cases we were putting lots of data in the driver purely for
>>> debugging, whereas in this case we're doing it to find nodes that we
>>> need to hook up with regulators in software and any associated data for
>>> that regulator.
>>>
>> That is indeed a bunch of memory.
>>
>> I think that if we instantiate the rpm-regulator driver by name from
>> the mfd driver and then loop over all the children and match against
>> our compatible list we would come down to 1 platform driver that
>> instantiate all our regulators. It's going to require some surgery and
>> will make the regulator driver less simple, but can be done.
>
> MFD name matching isn't required. All we need to do is have a regulators
> node and put a compatible = "qcom,rpm-msm-regulators" in there. Then
> of_platform_populate() does most of the work and we rework the RPM
> driver to match on this compatible. Thus the regulator stuff is
> encapsulated in the drivers/regulator/ directory.
>
Right, this would only affect the mfd children...
>>
>> With this we can go for the proposed binding and later alter the
>> implementation to save the memory. The cost of not encapsulating the
>> regulators/clocks/msm_busses are the extra loops in above search. The
>> benefit is cleaner bindings (and something that works as of today).
>>
>>
>> With the alternative of using the existing infrastructure of matching
>> regulators by name we need to change the binding to require certain
>> naming as well as maintain lists of the resources within the
>> regulator, clock & msm_bus drivers - something that has been objected
>> to several times already.
>
> For clocks I don't plan on us putting anything besides #clock-cells=<1>
> in DT. To mimic the regulators we can have a clock-controller node that
> has compatible = "qcom,rpm-msm-clocks" so that we don't have to do
> anything in the mfd driver itself and just fork the control over to a
> driver in drivers/clk/qcom.
That's fine and I'm glad you're looking into it as I don't have
documentation for those.
>
> e.g.
>
> rpm@108000 {
> compatible = "qcom,rpm-msm8960";
> reg = <0x108000 0x1000>;
> qcom,ipc = < 0x8 2>;
>
> interrupts = <0 19 0>, <0 21 0>, <0 22 0>;
> interrupt-names = "ack", "err", "wakeup";
>
> regulators {
> compatible = "qcom,rpm-msm8960-regulators";
>
> s1: s1 {
> regulator-min-microvolt = <1225000>;
> regulator-max-microvolt = <1225000>;
> bias-pull-down;
> qcom,switch-mode-frequency = <320>;
> };
This means that the regulator driver needs to have a list of
regulators per platform supported.
If we keep the compatible in the regulator nodes, we can use that for
matching with an implementation - still without using the
platform_device model for instantiating them. We would not need said
list in that case.
> ...
> };
>
> rpmcc: clock-controller (or clocks?) {
> compatible = "qcom,rpm-msm8960-clocks";
> #clock-cells = <1>;
> };
> };
>
> This is probably missing a size-cells somewhere, but you get the idea. I
> intentionally named the node "s1" in the hopes of the compiler
> consolidating the multiple "s1" strings for all the different pmic match
> tables into one string in some literal pool somewhere. Also, I removed
> reg from the regulator nodes to stay flexible in case we want to change
> the rpm write API in the future (it would go into the match table as
> driver data).
If we fall back to define the regulators in the code and map the
property nodes by name, then we can just add the content of the reg
property in those tables as well.
>
> (Goes to look at the RPM write API...)
>
> BTW, some of those rpm tables are going to be huge when you consider
> that the flat number space of QCOM_RPM_ is monotonically
> increasing but the actual resources used by a particular PMIC is only a
> subset of that space. For example, some arrays might only have resources
> that start at 100, so the first 100 entries in the array are wasted
> space. Maybe the rpm write API shouldn't be doing this fake
Re: [PATCH] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes
On 02/18/15 10:37, Bjorn Andersson wrote:
> On Tue, Feb 17, 2015 at 6:52 PM, Stephen Boyd wrote:
>> The main benefit I can think of is we cut down on runtime memory bloat.
>>
>> (gdb) p sizeof(struct platform_device)
>> $1 = 624
>>
>> Multiply that by 20 regulators and you get 624 * 20 = 12480 bytes in
>> platform devices. If we had one platform_device for all RPM controlled
>> regulators that would reduce this number from ~12k to ~0.5K. It would
>> require of_regulator_match() and the (undesirable) lists of regulator
>> names for all the different pmic variants, or we would need to pick out
>> the regulator nodes based on compatible matching. Is that so bad? In the
>> other cases we were putting lots of data in the driver purely for
>> debugging, whereas in this case we're doing it to find nodes that we
>> need to hook up with regulators in software and any associated data for
>> that regulator.
>>
> That is indeed a bunch of memory.
>
> I think that if we instantiate the rpm-regulator driver by name from
> the mfd driver and then loop over all the children and match against
> our compatible list we would come down to 1 platform driver that
> instantiate all our regulators. It's going to require some surgery and
> will make the regulator driver less simple, but can be done.
MFD name matching isn't required. All we need to do is have a regulators
node and put a compatible = "qcom,rpm-msm-regulators" in there. Then
of_platform_populate() does most of the work and we rework the RPM
driver to match on this compatible. Thus the regulator stuff is
encapsulated in the drivers/regulator/ directory.
>
> With this we can go for the proposed binding and later alter the
> implementation to save the memory. The cost of not encapsulating the
> regulators/clocks/msm_busses are the extra loops in above search. The
> benefit is cleaner bindings (and something that works as of today).
>
>
> With the alternative of using the existing infrastructure of matching
> regulators by name we need to change the binding to require certain
> naming as well as maintain lists of the resources within the
> regulator, clock & msm_bus drivers - something that has been objected
> to several times already.
For clocks I don't plan on us putting anything besides #clock-cells=<1>
in DT. To mimic the regulators we can have a clock-controller node that
has compatible = "qcom,rpm-msm-clocks" so that we don't have to do
anything in the mfd driver itself and just fork the control over to a
driver in drivers/clk/qcom.
e.g.
rpm@108000 {
compatible = "qcom,rpm-msm8960";
reg = <0x108000 0x1000>;
qcom,ipc = < 0x8 2>;
interrupts = <0 19 0>, <0 21 0>, <0 22 0>;
interrupt-names = "ack", "err", "wakeup";
regulators {
compatible = "qcom,rpm-msm8960-regulators";
s1: s1 {
regulator-min-microvolt = <1225000>;
regulator-max-microvolt = <1225000>;
bias-pull-down;
qcom,switch-mode-frequency = <320>;
};
...
};
rpmcc: clock-controller (or clocks?) {
compatible = "qcom,rpm-msm8960-clocks";
#clock-cells = <1>;
};
};
This is probably missing a size-cells somewhere, but you get the idea. I
intentionally named the node "s1" in the hopes of the compiler
consolidating the multiple "s1" strings for all the different pmic match
tables into one string in some literal pool somewhere. Also, I removed
reg from the regulator nodes to stay flexible in case we want to change
the rpm write API in the future (it would go into the match table as
driver data).
(Goes to look at the RPM write API...)
BTW, some of those rpm tables are going to be huge when you consider
that the flat number space of QCOM_RPM_ is monotonically
increasing but the actual resources used by a particular PMIC is only a
subset of that space. For example, some arrays might only have resources
that start at 100, so the first 100 entries in the array are wasted
space. Maybe the rpm write API shouldn't be doing this fake resource
numbering thing. Instead it should rely on the client drivers to pack up
a structure that the write API can interpret, i.e. push the resource
tables out to the drivers.
>
>
> A drawback of both these solutions is that supplies are specified on
> the device's of_node, and hence it is no longer possible to describe
> the supply dependency between our regulators - something that have
> shown to be needed. Maybe we can open code the supply logic in the
> regulator driver or we have to convince Mark about the necessity of
> this.
The supply dependencies are a detail of the PMIC implementation and it
isn't configurable on a board-by-board basis. If we did the individual
nodes and had the container regulators "device" we could specify the
dependencies in C and then vin-supply is not necessary.
Re: [PATCH] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes
On Tue, Feb 17, 2015 at 6:52 PM, Stephen Boyd wrote:
> On 02/17/15 13:48, Bjorn Andersson wrote:
>> On Fri, Feb 13, 2015 at 2:13 PM, Andy Gross wrote:
>>> On Thu, Jan 29, 2015 at 05:51:06PM -0800, Bjorn Andersson wrote:
Add the regulator subnodes to the Qualcomm RPM MFD device tree bindings.
Signed-off-by: Bjorn Andersson
---
#include
@@ -66,5 +237,18 @@ frequencies.
#address-cells = <1>;
#size-cells = <0>;
+
+ pm8921_smps1: pm8921-smps1 {
+ compatible = "qcom,rpm-pm8921-smps";
+ reg = ;
+
+ regulator-min-microvolt = <1225000>;
+ regulator-max-microvolt = <1225000>;
+ regulator-always-on;
+
+ bias-pull-down;
+
+ qcom,switch-mode-frequency = <320>;
+ };
};
>>> My only comment here is that most (all but one) of the other mfd regulator
>>> devices use regulators {}. Still wonder if that's what we should do.
>>>
>> Looking at the existing mfds they all have a list in the code of the
>> regulators supported on a certain mfd. Through the use of
>> regulator_desc.{of_match,regulators_node} these regulators are
>> populated with properties from of_nodes matched by name (of_match)
>> under the specified node (regulators_node).
>> But as we've discussed in other cases it's not desirable to maintain
>> these lists for the various variants of Qualcomm platforms, so I did
>> choose to go with "standalone" platform devices - with matching
>> through compatible and all.
>>
>> But that's all implementation, looking at the binding itself a
>> regulator {} (clocks{} and msm_bus{}) would serve as a sort of
>> grouping of children based on type. Except for the implications this
>> has on the implementation I don't see much benefit of this (and in our
>> case the implementation would suffer from the extra grouping).
>>
>>
>> Let me know what you think, I based these ideas on just reading the
>> existing code and bindings, and might very well have missed something.
>>
>
> The main benefit I can think of is we cut down on runtime memory bloat.
>
> (gdb) p sizeof(struct platform_device)
> $1 = 624
>
> Multiply that by 20 regulators and you get 624 * 20 = 12480 bytes in
> platform devices. If we had one platform_device for all RPM controlled
> regulators that would reduce this number from ~12k to ~0.5K. It would
> require of_regulator_match() and the (undesirable) lists of regulator
> names for all the different pmic variants, or we would need to pick out
> the regulator nodes based on compatible matching. Is that so bad? In the
> other cases we were putting lots of data in the driver purely for
> debugging, whereas in this case we're doing it to find nodes that we
> need to hook up with regulators in software and any associated data for
> that regulator.
>
That is indeed a bunch of memory.
I think that if we instantiate the rpm-regulator driver by name from
the mfd driver and then loop over all the children and match against
our compatible list we would come down to 1 platform driver that
instantiate all our regulators. It's going to require some surgery and
will make the regulator driver less simple, but can be done.
With this we can go for the proposed binding and later alter the
implementation to save the memory. The cost of not encapsulating the
regulators/clocks/msm_busses are the extra loops in above search. The
benefit is cleaner bindings (and something that works as of today).
With the alternative of using the existing infrastructure of matching
regulators by name we need to change the binding to require certain
naming as well as maintain lists of the resources within the
regulator, clock & msm_bus drivers - something that has been objected
to several times already.
A drawback of both these solutions is that supplies are specified on
the device's of_node, and hence it is no longer possible to describe
the supply dependency between our regulators - something that have
shown to be needed. Maybe we can open code the supply logic in the
regulator driver or we have to convince Mark about the necessity of
this.
So I would prefer to merge the binding as is and then put an
optimisation effort of the implementation on the todo list.
Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes
On Tue, Feb 17, 2015 at 6:52 PM, Stephen Boyd sb...@codeaurora.org wrote:
On 02/17/15 13:48, Bjorn Andersson wrote:
On Fri, Feb 13, 2015 at 2:13 PM, Andy Gross agr...@codeaurora.org wrote:
On Thu, Jan 29, 2015 at 05:51:06PM -0800, Bjorn Andersson wrote:
Add the regulator subnodes to the Qualcomm RPM MFD device tree bindings.
Signed-off-by: Bjorn Andersson bjorn.anders...@sonymobile.com
---
#include dt-bindings/mfd/qcom-rpm.h
@@ -66,5 +237,18 @@ frequencies.
#address-cells = 1;
#size-cells = 0;
+
+ pm8921_smps1: pm8921-smps1 {
+ compatible = qcom,rpm-pm8921-smps;
+ reg = QCOM_RPM_PM8921_SMPS1;
+
+ regulator-min-microvolt = 1225000;
+ regulator-max-microvolt = 1225000;
+ regulator-always-on;
+
+ bias-pull-down;
+
+ qcom,switch-mode-frequency = 320;
+ };
};
My only comment here is that most (all but one) of the other mfd regulator
devices use regulators {}. Still wonder if that's what we should do.
Looking at the existing mfds they all have a list in the code of the
regulators supported on a certain mfd. Through the use of
regulator_desc.{of_match,regulators_node} these regulators are
populated with properties from of_nodes matched by name (of_match)
under the specified node (regulators_node).
But as we've discussed in other cases it's not desirable to maintain
these lists for the various variants of Qualcomm platforms, so I did
choose to go with standalone platform devices - with matching
through compatible and all.
But that's all implementation, looking at the binding itself a
regulator {} (clocks{} and msm_bus{}) would serve as a sort of
grouping of children based on type. Except for the implications this
has on the implementation I don't see much benefit of this (and in our
case the implementation would suffer from the extra grouping).
Let me know what you think, I based these ideas on just reading the
existing code and bindings, and might very well have missed something.
The main benefit I can think of is we cut down on runtime memory bloat.
(gdb) p sizeof(struct platform_device)
$1 = 624
Multiply that by 20 regulators and you get 624 * 20 = 12480 bytes in
platform devices. If we had one platform_device for all RPM controlled
regulators that would reduce this number from ~12k to ~0.5K. It would
require of_regulator_match() and the (undesirable) lists of regulator
names for all the different pmic variants, or we would need to pick out
the regulator nodes based on compatible matching. Is that so bad? In the
other cases we were putting lots of data in the driver purely for
debugging, whereas in this case we're doing it to find nodes that we
need to hook up with regulators in software and any associated data for
that regulator.
That is indeed a bunch of memory.
I think that if we instantiate the rpm-regulator driver by name from
the mfd driver and then loop over all the children and match against
our compatible list we would come down to 1 platform driver that
instantiate all our regulators. It's going to require some surgery and
will make the regulator driver less simple, but can be done.
With this we can go for the proposed binding and later alter the
implementation to save the memory. The cost of not encapsulating the
regulators/clocks/msm_busses are the extra loops in above search. The
benefit is cleaner bindings (and something that works as of today).
With the alternative of using the existing infrastructure of matching
regulators by name we need to change the binding to require certain
naming as well as maintain lists of the resources within the
regulator, clock msm_bus drivers - something that has been objected
to several times already.
A drawback of both these solutions is that supplies are specified on
the device's of_node, and hence it is no longer possible to describe
the supply dependency between our regulators - something that have
shown to be needed. Maybe we can open code the supply logic in the
regulator driver or we have to convince Mark about the necessity of
this.
So I would prefer to merge the binding as is and then put an
optimisation effort of the implementation on the todo list.
Regards,
Bjorn
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes
On 02/18/15 10:37, Bjorn Andersson wrote:
On Tue, Feb 17, 2015 at 6:52 PM, Stephen Boyd sb...@codeaurora.org wrote:
The main benefit I can think of is we cut down on runtime memory bloat.
(gdb) p sizeof(struct platform_device)
$1 = 624
Multiply that by 20 regulators and you get 624 * 20 = 12480 bytes in
platform devices. If we had one platform_device for all RPM controlled
regulators that would reduce this number from ~12k to ~0.5K. It would
require of_regulator_match() and the (undesirable) lists of regulator
names for all the different pmic variants, or we would need to pick out
the regulator nodes based on compatible matching. Is that so bad? In the
other cases we were putting lots of data in the driver purely for
debugging, whereas in this case we're doing it to find nodes that we
need to hook up with regulators in software and any associated data for
that regulator.
That is indeed a bunch of memory.
I think that if we instantiate the rpm-regulator driver by name from
the mfd driver and then loop over all the children and match against
our compatible list we would come down to 1 platform driver that
instantiate all our regulators. It's going to require some surgery and
will make the regulator driver less simple, but can be done.
MFD name matching isn't required. All we need to do is have a regulators
node and put a compatible = qcom,rpm-msm-regulators in there. Then
of_platform_populate() does most of the work and we rework the RPM
driver to match on this compatible. Thus the regulator stuff is
encapsulated in the drivers/regulator/ directory.
With this we can go for the proposed binding and later alter the
implementation to save the memory. The cost of not encapsulating the
regulators/clocks/msm_busses are the extra loops in above search. The
benefit is cleaner bindings (and something that works as of today).
With the alternative of using the existing infrastructure of matching
regulators by name we need to change the binding to require certain
naming as well as maintain lists of the resources within the
regulator, clock msm_bus drivers - something that has been objected
to several times already.
For clocks I don't plan on us putting anything besides #clock-cells=1
in DT. To mimic the regulators we can have a clock-controller node that
has compatible = qcom,rpm-msm-clocks so that we don't have to do
anything in the mfd driver itself and just fork the control over to a
driver in drivers/clk/qcom.
e.g.
rpm@108000 {
compatible = qcom,rpm-msm8960;
reg = 0x108000 0x1000;
qcom,ipc = apcs 0x8 2;
interrupts = 0 19 0, 0 21 0, 0 22 0;
interrupt-names = ack, err, wakeup;
regulators {
compatible = qcom,rpm-msm8960-regulators;
s1: s1 {
regulator-min-microvolt = 1225000;
regulator-max-microvolt = 1225000;
bias-pull-down;
qcom,switch-mode-frequency = 320;
};
...
};
rpmcc: clock-controller (or clocks?) {
compatible = qcom,rpm-msm8960-clocks;
#clock-cells = 1;
};
};
This is probably missing a size-cells somewhere, but you get the idea. I
intentionally named the node s1 in the hopes of the compiler
consolidating the multiple s1 strings for all the different pmic match
tables into one string in some literal pool somewhere. Also, I removed
reg from the regulator nodes to stay flexible in case we want to change
the rpm write API in the future (it would go into the match table as
driver data).
(Goes to look at the RPM write API...)
BTW, some of those rpm tables are going to be huge when you consider
that the flat number space of QCOM_RPM_RESOURCE is monotonically
increasing but the actual resources used by a particular PMIC is only a
subset of that space. For example, some arrays might only have resources
that start at 100, so the first 100 entries in the array are wasted
space. Maybe the rpm write API shouldn't be doing this fake resource
numbering thing. Instead it should rely on the client drivers to pack up
a structure that the write API can interpret, i.e. push the resource
tables out to the drivers.
A drawback of both these solutions is that supplies are specified on
the device's of_node, and hence it is no longer possible to describe
the supply dependency between our regulators - something that have
shown to be needed. Maybe we can open code the supply logic in the
regulator driver or we have to convince Mark about the necessity of
this.
The supply dependencies are a detail of the PMIC implementation and it
isn't configurable on a board-by-board basis. If we did the individual
nodes and had the container regulators device we could specify the
dependencies in C and then vin-supply is not necessary. That sounds like
a win to me because we sidestep adding a new property.
Re: [PATCH] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes
On Wed, Feb 18, 2015 at 12:28 PM, Stephen Boyd sb...@codeaurora.org wrote:
On 02/18/15 10:37, Bjorn Andersson wrote:
On Tue, Feb 17, 2015 at 6:52 PM, Stephen Boyd sb...@codeaurora.org wrote:
The main benefit I can think of is we cut down on runtime memory bloat.
(gdb) p sizeof(struct platform_device)
$1 = 624
Multiply that by 20 regulators and you get 624 * 20 = 12480 bytes in
platform devices. If we had one platform_device for all RPM controlled
regulators that would reduce this number from ~12k to ~0.5K. It would
require of_regulator_match() and the (undesirable) lists of regulator
names for all the different pmic variants, or we would need to pick out
the regulator nodes based on compatible matching. Is that so bad? In the
other cases we were putting lots of data in the driver purely for
debugging, whereas in this case we're doing it to find nodes that we
need to hook up with regulators in software and any associated data for
that regulator.
That is indeed a bunch of memory.
I think that if we instantiate the rpm-regulator driver by name from
the mfd driver and then loop over all the children and match against
our compatible list we would come down to 1 platform driver that
instantiate all our regulators. It's going to require some surgery and
will make the regulator driver less simple, but can be done.
MFD name matching isn't required. All we need to do is have a regulators
node and put a compatible = qcom,rpm-msm-regulators in there. Then
of_platform_populate() does most of the work and we rework the RPM
driver to match on this compatible. Thus the regulator stuff is
encapsulated in the drivers/regulator/ directory.
Right, this would only affect the mfd children...
With this we can go for the proposed binding and later alter the
implementation to save the memory. The cost of not encapsulating the
regulators/clocks/msm_busses are the extra loops in above search. The
benefit is cleaner bindings (and something that works as of today).
With the alternative of using the existing infrastructure of matching
regulators by name we need to change the binding to require certain
naming as well as maintain lists of the resources within the
regulator, clock msm_bus drivers - something that has been objected
to several times already.
For clocks I don't plan on us putting anything besides #clock-cells=1
in DT. To mimic the regulators we can have a clock-controller node that
has compatible = qcom,rpm-msm-clocks so that we don't have to do
anything in the mfd driver itself and just fork the control over to a
driver in drivers/clk/qcom.
That's fine and I'm glad you're looking into it as I don't have
documentation for those.
e.g.
rpm@108000 {
compatible = qcom,rpm-msm8960;
reg = 0x108000 0x1000;
qcom,ipc = apcs 0x8 2;
interrupts = 0 19 0, 0 21 0, 0 22 0;
interrupt-names = ack, err, wakeup;
regulators {
compatible = qcom,rpm-msm8960-regulators;
s1: s1 {
regulator-min-microvolt = 1225000;
regulator-max-microvolt = 1225000;
bias-pull-down;
qcom,switch-mode-frequency = 320;
};
This means that the regulator driver needs to have a list of
regulators per platform supported.
If we keep the compatible in the regulator nodes, we can use that for
matching with an implementation - still without using the
platform_device model for instantiating them. We would not need said
list in that case.
...
};
rpmcc: clock-controller (or clocks?) {
compatible = qcom,rpm-msm8960-clocks;
#clock-cells = 1;
};
};
This is probably missing a size-cells somewhere, but you get the idea. I
intentionally named the node s1 in the hopes of the compiler
consolidating the multiple s1 strings for all the different pmic match
tables into one string in some literal pool somewhere. Also, I removed
reg from the regulator nodes to stay flexible in case we want to change
the rpm write API in the future (it would go into the match table as
driver data).
If we fall back to define the regulators in the code and map the
property nodes by name, then we can just add the content of the reg
property in those tables as well.
(Goes to look at the RPM write API...)
BTW, some of those rpm tables are going to be huge when you consider
that the flat number space of QCOM_RPM_RESOURCE is monotonically
increasing but the actual resources used by a particular PMIC is only a
subset of that space. For example, some arrays might only have resources
that start at 100, so the first 100 entries in the array are wasted
space. Maybe the rpm write API shouldn't be doing this fake resource
numbering thing. Instead it should rely on the client drivers to pack up
a structure that the write API can interpret, i.e. push
Re: [PATCH] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes
On 02/18/15 13:08, Bjorn Andersson wrote:
On Wed, Feb 18, 2015 at 12:28 PM, Stephen Boyd sb...@codeaurora.org wrote:
MFD name matching isn't required. All we need to do is have a regulators
node and put a compatible = qcom,rpm-msm-regulators in there. Then
of_platform_populate() does most of the work and we rework the RPM
driver to match on this compatible. Thus the regulator stuff is
encapsulated in the drivers/regulator/ directory.
Right, this would only affect the mfd children...
e.g.
rpm@108000 {
compatible = qcom,rpm-msm8960;
reg = 0x108000 0x1000;
qcom,ipc = apcs 0x8 2;
interrupts = 0 19 0, 0 21 0, 0 22 0;
interrupt-names = ack, err, wakeup;
regulators {
compatible = qcom,rpm-msm8960-regulators;
s1: s1 {
regulator-min-microvolt = 1225000;
regulator-max-microvolt = 1225000;
bias-pull-down;
qcom,switch-mode-frequency = 320;
};
This means that the regulator driver needs to have a list of
regulators per platform supported.
If we keep the compatible in the regulator nodes, we can use that for
matching with an implementation - still without using the
platform_device model for instantiating them. We would not need said
list in that case.
Agreed. It was intentional because the type of regulator is another
static implementation detail.
...
};
rpmcc: clock-controller (or clocks?) {
compatible = qcom,rpm-msm8960-clocks;
#clock-cells = 1;
};
};
This is probably missing a size-cells somewhere, but you get the idea. I
intentionally named the node s1 in the hopes of the compiler
consolidating the multiple s1 strings for all the different pmic match
tables into one string in some literal pool somewhere. Also, I removed
reg from the regulator nodes to stay flexible in case we want to change
the rpm write API in the future (it would go into the match table as
driver data).
If we fall back to define the regulators in the code and map the
property nodes by name, then we can just add the content of the reg
property in those tables as well.
Yep.
(Goes to look at the RPM write API...)
BTW, some of those rpm tables are going to be huge when you consider
that the flat number space of QCOM_RPM_RESOURCE is monotonically
increasing but the actual resources used by a particular PMIC is only a
subset of that space. For example, some arrays might only have resources
that start at 100, so the first 100 entries in the array are wasted
space. Maybe the rpm write API shouldn't be doing this fake resource
numbering thing. Instead it should rely on the client drivers to pack up
a structure that the write API can interpret, i.e. push the resource
tables out to the drivers.
Correct, but David Collins comment on the subject was clear; we don't
want these tables in the code. And after playing around with various
options I came to the same conclusion - maintaining the tables will be
a mess.
We're already maintaining these tables in qcom-rpm.c. I'm advocating for
removing those tables from the rpm driver and putting the data into the
regulator driver. Then we don't have to index into a sparse table to
figure out what values the RPM wants to use. Instead we have the data at
hand for a particular regulator based on the of_regulator_match.
I don't understand the maintenance argument either. The data has to go
somewhere so the maintenance is always there.
From what I can tell, that data is split in two places. The regulator
type knows how big the data we want to send is (1 or 2) and that is
checked in the RPM driver to make sure that the two agree (this part
looks like over-defensive coding). Then the DT has a made up number that
maps to 3 different numbers in the RPM driver. The same could be done
for b-family, where we have two numbers that just so happen to be user
friendly enough to make sense on their own. Instead of being 165, 68,
and 48, they're a #define SMPS and 1. We could make a #define SMPS1 and
map it to a tuple with #define SMPS and 1. It looks like that was how
you had b-family prototyped at one point.
So for family B, where this is not necessary for the rpm driver, I
have revised this to instead be:
#address-cells = 2;
smps1 {
reg = QCOM_SMD_RPM_SMPA 1;
}
With #define QCOM_SMD_RPM_SMPA 0x61706d73, this information goes
straight into the smd packet and we don't have any tables at all.
How does the rpm write API look there? Does it take two numbers
(resource type and resource id) instead of 1 (enum)? Is the plan to keep
the write API the same as what's in qcom-rpm.c today?
Josh made an attempt to encode the data needed for family A in the DT
in a similar fasion, but we never found a reasonable way to do so.
With Josh's attempt would this be?
#address-cells = 3;
Re: [PATCH] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes
On 02/17/15 13:48, Bjorn Andersson wrote:
> On Fri, Feb 13, 2015 at 2:13 PM, Andy Gross wrote:
>> On Thu, Jan 29, 2015 at 05:51:06PM -0800, Bjorn Andersson wrote:
>>> Add the regulator subnodes to the Qualcomm RPM MFD device tree bindings.
>>>
>>> Signed-off-by: Bjorn Andersson
>>> ---
>>> #include
>>> @@ -66,5 +237,18 @@ frequencies.
>>>
>>> #address-cells = <1>;
>>> #size-cells = <0>;
>>> +
>>> + pm8921_smps1: pm8921-smps1 {
>>> + compatible = "qcom,rpm-pm8921-smps";
>>> + reg = ;
>>> +
>>> + regulator-min-microvolt = <1225000>;
>>> + regulator-max-microvolt = <1225000>;
>>> + regulator-always-on;
>>> +
>>> + bias-pull-down;
>>> +
>>> + qcom,switch-mode-frequency = <320>;
>>> + };
>>> };
>> My only comment here is that most (all but one) of the other mfd regulator
>> devices use regulators {}. Still wonder if that's what we should do.
>>
> Looking at the existing mfds they all have a list in the code of the
> regulators supported on a certain mfd. Through the use of
> regulator_desc.{of_match,regulators_node} these regulators are
> populated with properties from of_nodes matched by name (of_match)
> under the specified node (regulators_node).
> But as we've discussed in other cases it's not desirable to maintain
> these lists for the various variants of Qualcomm platforms, so I did
> choose to go with "standalone" platform devices - with matching
> through compatible and all.
>
> But that's all implementation, looking at the binding itself a
> regulator {} (clocks{} and msm_bus{}) would serve as a sort of
> grouping of children based on type. Except for the implications this
> has on the implementation I don't see much benefit of this (and in our
> case the implementation would suffer from the extra grouping).
>
>
> Let me know what you think, I based these ideas on just reading the
> existing code and bindings, and might very well have missed something.
>
The main benefit I can think of is we cut down on runtime memory bloat.
(gdb) p sizeof(struct platform_device)
$1 = 624
Multiply that by 20 regulators and you get 624 * 20 = 12480 bytes in
platform devices. If we had one platform_device for all RPM controlled
regulators that would reduce this number from ~12k to ~0.5K. It would
require of_regulator_match() and the (undesirable) lists of regulator
names for all the different pmic variants, or we would need to pick out
the regulator nodes based on compatible matching. Is that so bad? In the
other cases we were putting lots of data in the driver purely for
debugging, whereas in this case we're doing it to find nodes that we
need to hook up with regulators in software and any associated data for
that regulator.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes
On Fri, Feb 13, 2015 at 2:13 PM, Andy Gross wrote:
> On Thu, Jan 29, 2015 at 05:51:06PM -0800, Bjorn Andersson wrote:
>> Add the regulator subnodes to the Qualcomm RPM MFD device tree bindings.
>>
>> Signed-off-by: Bjorn Andersson
>> ---
>
>> #include
>> @@ -66,5 +237,18 @@ frequencies.
>>
>> #address-cells = <1>;
>> #size-cells = <0>;
>> +
>> + pm8921_smps1: pm8921-smps1 {
>> + compatible = "qcom,rpm-pm8921-smps";
>> + reg = ;
>> +
>> + regulator-min-microvolt = <1225000>;
>> + regulator-max-microvolt = <1225000>;
>> + regulator-always-on;
>> +
>> + bias-pull-down;
>> +
>> + qcom,switch-mode-frequency = <320>;
>> + };
>> };
>
> My only comment here is that most (all but one) of the other mfd regulator
> devices use regulators {}. Still wonder if that's what we should do.
>
Looking at the existing mfds they all have a list in the code of the
regulators supported on a certain mfd. Through the use of
regulator_desc.{of_match,regulators_node} these regulators are
populated with properties from of_nodes matched by name (of_match)
under the specified node (regulators_node).
But as we've discussed in other cases it's not desirable to maintain
these lists for the various variants of Qualcomm platforms, so I did
choose to go with "standalone" platform devices - with matching
through compatible and all.
But that's all implementation, looking at the binding itself a
regulator {} (clocks{} and msm_bus{}) would serve as a sort of
grouping of children based on type. Except for the implications this
has on the implementation I don't see much benefit of this (and in our
case the implementation would suffer from the extra grouping).
Let me know what you think, I based these ideas on just reading the
existing code and bindings, and might very well have missed something.
> Otherwise,
>
> Reviewed-by: Andy Gross
>
Thanks
Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes
On 02/17/15 13:48, Bjorn Andersson wrote:
On Fri, Feb 13, 2015 at 2:13 PM, Andy Gross agr...@codeaurora.org wrote:
On Thu, Jan 29, 2015 at 05:51:06PM -0800, Bjorn Andersson wrote:
Add the regulator subnodes to the Qualcomm RPM MFD device tree bindings.
Signed-off-by: Bjorn Andersson bjorn.anders...@sonymobile.com
---
#include dt-bindings/mfd/qcom-rpm.h
@@ -66,5 +237,18 @@ frequencies.
#address-cells = 1;
#size-cells = 0;
+
+ pm8921_smps1: pm8921-smps1 {
+ compatible = qcom,rpm-pm8921-smps;
+ reg = QCOM_RPM_PM8921_SMPS1;
+
+ regulator-min-microvolt = 1225000;
+ regulator-max-microvolt = 1225000;
+ regulator-always-on;
+
+ bias-pull-down;
+
+ qcom,switch-mode-frequency = 320;
+ };
};
My only comment here is that most (all but one) of the other mfd regulator
devices use regulators {}. Still wonder if that's what we should do.
Looking at the existing mfds they all have a list in the code of the
regulators supported on a certain mfd. Through the use of
regulator_desc.{of_match,regulators_node} these regulators are
populated with properties from of_nodes matched by name (of_match)
under the specified node (regulators_node).
But as we've discussed in other cases it's not desirable to maintain
these lists for the various variants of Qualcomm platforms, so I did
choose to go with standalone platform devices - with matching
through compatible and all.
But that's all implementation, looking at the binding itself a
regulator {} (clocks{} and msm_bus{}) would serve as a sort of
grouping of children based on type. Except for the implications this
has on the implementation I don't see much benefit of this (and in our
case the implementation would suffer from the extra grouping).
Let me know what you think, I based these ideas on just reading the
existing code and bindings, and might very well have missed something.
The main benefit I can think of is we cut down on runtime memory bloat.
(gdb) p sizeof(struct platform_device)
$1 = 624
Multiply that by 20 regulators and you get 624 * 20 = 12480 bytes in
platform devices. If we had one platform_device for all RPM controlled
regulators that would reduce this number from ~12k to ~0.5K. It would
require of_regulator_match() and the (undesirable) lists of regulator
names for all the different pmic variants, or we would need to pick out
the regulator nodes based on compatible matching. Is that so bad? In the
other cases we were putting lots of data in the driver purely for
debugging, whereas in this case we're doing it to find nodes that we
need to hook up with regulators in software and any associated data for
that regulator.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes
On Fri, Feb 13, 2015 at 2:13 PM, Andy Gross agr...@codeaurora.org wrote:
On Thu, Jan 29, 2015 at 05:51:06PM -0800, Bjorn Andersson wrote:
Add the regulator subnodes to the Qualcomm RPM MFD device tree bindings.
Signed-off-by: Bjorn Andersson bjorn.anders...@sonymobile.com
---
#include dt-bindings/mfd/qcom-rpm.h
@@ -66,5 +237,18 @@ frequencies.
#address-cells = 1;
#size-cells = 0;
+
+ pm8921_smps1: pm8921-smps1 {
+ compatible = qcom,rpm-pm8921-smps;
+ reg = QCOM_RPM_PM8921_SMPS1;
+
+ regulator-min-microvolt = 1225000;
+ regulator-max-microvolt = 1225000;
+ regulator-always-on;
+
+ bias-pull-down;
+
+ qcom,switch-mode-frequency = 320;
+ };
};
My only comment here is that most (all but one) of the other mfd regulator
devices use regulators {}. Still wonder if that's what we should do.
Looking at the existing mfds they all have a list in the code of the
regulators supported on a certain mfd. Through the use of
regulator_desc.{of_match,regulators_node} these regulators are
populated with properties from of_nodes matched by name (of_match)
under the specified node (regulators_node).
But as we've discussed in other cases it's not desirable to maintain
these lists for the various variants of Qualcomm platforms, so I did
choose to go with standalone platform devices - with matching
through compatible and all.
But that's all implementation, looking at the binding itself a
regulator {} (clocks{} and msm_bus{}) would serve as a sort of
grouping of children based on type. Except for the implications this
has on the implementation I don't see much benefit of this (and in our
case the implementation would suffer from the extra grouping).
Let me know what you think, I based these ideas on just reading the
existing code and bindings, and might very well have missed something.
Otherwise,
Reviewed-by: Andy Gross agr...@codeaurora.org
Thanks
Regards,
Bjorn
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes
On Thu, Jan 29, 2015 at 05:51:06PM -0800, Bjorn Andersson wrote:
> Add the regulator subnodes to the Qualcomm RPM MFD device tree bindings.
>
> Signed-off-by: Bjorn Andersson
> ---
> #include
> @@ -66,5 +237,18 @@ frequencies.
>
> #address-cells = <1>;
> #size-cells = <0>;
> +
> + pm8921_smps1: pm8921-smps1 {
> + compatible = "qcom,rpm-pm8921-smps";
> + reg = ;
> +
> + regulator-min-microvolt = <1225000>;
> + regulator-max-microvolt = <1225000>;
> + regulator-always-on;
> +
> + bias-pull-down;
> +
> + qcom,switch-mode-frequency = <320>;
> + };
> };
My only comment here is that most (all but one) of the other mfd regulator
devices use regulators {}. Still wonder if that's what we should do.
Otherwise,
Reviewed-by: Andy Gross
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes
On Thu, Jan 29, 2015 at 05:51:06PM -0800, Bjorn Andersson wrote:
Add the regulator subnodes to the Qualcomm RPM MFD device tree bindings.
Signed-off-by: Bjorn Andersson bjorn.anders...@sonymobile.com
---
#include dt-bindings/mfd/qcom-rpm.h
@@ -66,5 +237,18 @@ frequencies.
#address-cells = 1;
#size-cells = 0;
+
+ pm8921_smps1: pm8921-smps1 {
+ compatible = qcom,rpm-pm8921-smps;
+ reg = QCOM_RPM_PM8921_SMPS1;
+
+ regulator-min-microvolt = 1225000;
+ regulator-max-microvolt = 1225000;
+ regulator-always-on;
+
+ bias-pull-down;
+
+ qcom,switch-mode-frequency = 320;
+ };
};
My only comment here is that most (all but one) of the other mfd regulator
devices use regulators {}. Still wonder if that's what we should do.
Otherwise,
Reviewed-by: Andy Gross agr...@codeaurora.org
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes
On Tue, 10 Feb 2015, Lee Jones wrote: > On Mon, 09 Feb 2015, Bjorn Andersson wrote: > > > On Thu, Jan 29, 2015 at 5:51 PM, Bjorn Andersson > > wrote: > > > Add the regulator subnodes to the Qualcomm RPM MFD device tree bindings. > > > > > > Signed-off-by: Bjorn Andersson > > > --- > > > After discussing this back and forth we've concluded that we should be > > > future compatible with these bindings, so let's make an attempt to make > > > it possible to use the Qualcomm family A regulators. > > > > > > > ping? > > Pings don't work. If I haven't replied, there is a reason. > > I've been hectic at 'proper work' lately -- I'll get to you. In fact, this needs a DT Ack. -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes
On Mon, 09 Feb 2015, Bjorn Andersson wrote: > On Thu, Jan 29, 2015 at 5:51 PM, Bjorn Andersson > wrote: > > Add the regulator subnodes to the Qualcomm RPM MFD device tree bindings. > > > > Signed-off-by: Bjorn Andersson > > --- > > After discussing this back and forth we've concluded that we should be > > future compatible with these bindings, so let's make an attempt to make > > it possible to use the Qualcomm family A regulators. > > > > ping? Pings don't work. If I haven't replied, there is a reason. I've been hectic at 'proper work' lately -- I'll get to you. -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes
On Thu, Jan 29, 2015 at 5:51 PM, Bjorn Andersson wrote: > Add the regulator subnodes to the Qualcomm RPM MFD device tree bindings. > > Signed-off-by: Bjorn Andersson > --- > After discussing this back and forth we've concluded that we should be > future compatible with these bindings, so let's make an attempt to make > it possible to use the Qualcomm family A regulators. > ping? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes
On Thu, Jan 29, 2015 at 5:51 PM, Bjorn Andersson bjorn.anders...@sonymobile.com wrote: Add the regulator subnodes to the Qualcomm RPM MFD device tree bindings. Signed-off-by: Bjorn Andersson bjorn.anders...@sonymobile.com --- After discussing this back and forth we've concluded that we should be future compatible with these bindings, so let's make an attempt to make it possible to use the Qualcomm family A regulators. ping? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes
On Tue, 10 Feb 2015, Lee Jones wrote: On Mon, 09 Feb 2015, Bjorn Andersson wrote: On Thu, Jan 29, 2015 at 5:51 PM, Bjorn Andersson bjorn.anders...@sonymobile.com wrote: Add the regulator subnodes to the Qualcomm RPM MFD device tree bindings. Signed-off-by: Bjorn Andersson bjorn.anders...@sonymobile.com --- After discussing this back and forth we've concluded that we should be future compatible with these bindings, so let's make an attempt to make it possible to use the Qualcomm family A regulators. ping? Pings don't work. If I haven't replied, there is a reason. I've been hectic at 'proper work' lately -- I'll get to you. In fact, this needs a DT Ack. -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes
On Mon, 09 Feb 2015, Bjorn Andersson wrote: On Thu, Jan 29, 2015 at 5:51 PM, Bjorn Andersson bjorn.anders...@sonymobile.com wrote: Add the regulator subnodes to the Qualcomm RPM MFD device tree bindings. Signed-off-by: Bjorn Andersson bjorn.anders...@sonymobile.com --- After discussing this back and forth we've concluded that we should be future compatible with these bindings, so let's make an attempt to make it possible to use the Qualcomm family A regulators. ping? Pings don't work. If I haven't replied, there is a reason. I've been hectic at 'proper work' lately -- I'll get to you. -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes
Add the regulator subnodes to the Qualcomm RPM MFD device tree bindings. Signed-off-by: Bjorn Andersson --- After discussing this back and forth we've concluded that we should be future compatible with these bindings, so let's make an attempt to make it possible to use the Qualcomm family A regulators. Documentation/devicetree/bindings/mfd/qcom-rpm.txt | 184 + 1 file changed, 184 insertions(+) diff --git a/Documentation/devicetree/bindings/mfd/qcom-rpm.txt b/Documentation/devicetree/bindings/mfd/qcom-rpm.txt index 85e3198..816acda 100644 --- a/Documentation/devicetree/bindings/mfd/qcom-rpm.txt +++ b/Documentation/devicetree/bindings/mfd/qcom-rpm.txt @@ -52,6 +52,177 @@ frequencies. - u32 representing the ipc bit within the register += SUBDEVICES + +The RPM exposes resources to its subnodes. The below bindings specify the set +of valid subnodes that can operate on these resources. + +== Switch-mode Power Supply regulator + +- compatible: + Usage: required + Value type: + Definition: must be one of: + "qcom,rpm-pm8058-smps" + "qcom,rpm-pm8901-ftsmps" + "qcom,rpm-pm8921-smps" + "qcom,rpm-pm8921-ftsmps" + +- reg: + Usage: required + Value type: + Definition: resource as defined in + must be one of: + QCOM_RPM_PM8058_SMPS0 - QCOM_RPM_PM8058_SMPS4, + QCOM_RPM_PM8821_SMPS1 - QCOM_RPM_PM8821_SMPS2, + QCOM_RPM_PM8901_SMPS0 - QCOM_RPM_PM8901_SMPS4, + QCOM_RPM_PM8921_SMPS1 - QCOM_RPM_PM8921_SMPS8 + +- bias-pull-down: + Usage: optional + Value type: + Definition: enable pull down of the regulator when inactive + +- qcom,switch-mode-frequency: + Usage: required + Value type: + Definition: Frequency (Hz) of the switch-mode power supply; + must be one of: + 1920, 960, 640, 480, 384, 320, + 274, 240, 213, 192, 175, 160, + 148, 137, 128, 120 + +- qcom,force-mode: + Usage: optional (default if no other qcom,force-mode is specified) + Value type: + Definition: indicates that the regulator should be forced to a + particular mode, valid values are: + QCOM_RPM_FORCE_MODE_NONE - do not force any mode + QCOM_RPM_FORCE_MODE_LPM - force into low power mode + QCOM_RPM_FORCE_MODE_HPM - force into high power mode + QCOM_RPM_FORCE_MODE_AUTO - allow regulator to automatically + select its own mode based on + realtime current draw, only for: + qcom,rpm-pm8921-smps, + qcom,rpm-pm8921-ftsmps + +- qcom,power-mode-hysteretic: + Usage: optional + Value type: + Definition: select that the power supply should operate in hysteretic + mode, instead of the default pwm mode + +Standard regulator bindings are used inside switch mode power supply subnodes. +Check Documentation/devicetree/bindings/regulator/regulator.txt for more +details. + +== Low-dropout regulator + +- compatible: + Usage: required + Value type: + Definition: must be one of: + "qcom,rpm-pm8058-pldo" + "qcom,rpm-pm8058-nldo" + "qcom,rpm-pm8901-pldo" + "qcom,rpm-pm8901-nldo" + "qcom,rpm-pm8921-pldo" + "qcom,rpm-pm8921-nldo" + "qcom,rpm-pm8921-nldo1200" + +- reg: + Usage: required + Value type: + Definition: resource as defined in + must be one of: + QCOM_RPM_PM8058_LDO0 - QCOM_RPM_PM8058_LDO25, + QCOM_RPM_PM8821_LDO1, + QCOM_RPM_PM8901_LDO0 - QCOM_RPM_PM8901_LDO6, + QCOM_RPM_PM8921_LDO1 - QCOM_RPM_PM8921_LDO29 + +- bias-pull-down: + Usage: optional + Value type: + Definition: enable pull down of the regulator when inactive + +- qcom,force-mode: + Usage: optional + Value type: + Definition: indicates that the regulator should not be forced to any + particular mode, valid values are: + QCOM_RPM_FORCE_MODE_NONE - do not force any mode + QCOM_RPM_FORCE_MODE_LPM - force into low power mode + QCOM_RPM_FORCE_MODE_HPM - force into high power mode + QCOM_RPM_FORCE_MODE_BYPASS - set regulator to use bypass +mode, i.e. to act as a switch +and not regulate, only for: +
[PATCH] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes
Add the regulator subnodes to the Qualcomm RPM MFD device tree bindings. Signed-off-by: Bjorn Andersson bjorn.anders...@sonymobile.com --- After discussing this back and forth we've concluded that we should be future compatible with these bindings, so let's make an attempt to make it possible to use the Qualcomm family A regulators. Documentation/devicetree/bindings/mfd/qcom-rpm.txt | 184 + 1 file changed, 184 insertions(+) diff --git a/Documentation/devicetree/bindings/mfd/qcom-rpm.txt b/Documentation/devicetree/bindings/mfd/qcom-rpm.txt index 85e3198..816acda 100644 --- a/Documentation/devicetree/bindings/mfd/qcom-rpm.txt +++ b/Documentation/devicetree/bindings/mfd/qcom-rpm.txt @@ -52,6 +52,177 @@ frequencies. - u32 representing the ipc bit within the register += SUBDEVICES + +The RPM exposes resources to its subnodes. The below bindings specify the set +of valid subnodes that can operate on these resources. + +== Switch-mode Power Supply regulator + +- compatible: + Usage: required + Value type: string + Definition: must be one of: + qcom,rpm-pm8058-smps + qcom,rpm-pm8901-ftsmps + qcom,rpm-pm8921-smps + qcom,rpm-pm8921-ftsmps + +- reg: + Usage: required + Value type: u32 + Definition: resource as defined in dt-bindings/mfd/qcom-rpm.h + must be one of: + QCOM_RPM_PM8058_SMPS0 - QCOM_RPM_PM8058_SMPS4, + QCOM_RPM_PM8821_SMPS1 - QCOM_RPM_PM8821_SMPS2, + QCOM_RPM_PM8901_SMPS0 - QCOM_RPM_PM8901_SMPS4, + QCOM_RPM_PM8921_SMPS1 - QCOM_RPM_PM8921_SMPS8 + +- bias-pull-down: + Usage: optional + Value type: empty + Definition: enable pull down of the regulator when inactive + +- qcom,switch-mode-frequency: + Usage: required + Value type: u32 + Definition: Frequency (Hz) of the switch-mode power supply; + must be one of: + 1920, 960, 640, 480, 384, 320, + 274, 240, 213, 192, 175, 160, + 148, 137, 128, 120 + +- qcom,force-mode: + Usage: optional (default if no other qcom,force-mode is specified) + Value type: u32 + Definition: indicates that the regulator should be forced to a + particular mode, valid values are: + QCOM_RPM_FORCE_MODE_NONE - do not force any mode + QCOM_RPM_FORCE_MODE_LPM - force into low power mode + QCOM_RPM_FORCE_MODE_HPM - force into high power mode + QCOM_RPM_FORCE_MODE_AUTO - allow regulator to automatically + select its own mode based on + realtime current draw, only for: + qcom,rpm-pm8921-smps, + qcom,rpm-pm8921-ftsmps + +- qcom,power-mode-hysteretic: + Usage: optional + Value type: empty + Definition: select that the power supply should operate in hysteretic + mode, instead of the default pwm mode + +Standard regulator bindings are used inside switch mode power supply subnodes. +Check Documentation/devicetree/bindings/regulator/regulator.txt for more +details. + +== Low-dropout regulator + +- compatible: + Usage: required + Value type: string + Definition: must be one of: + qcom,rpm-pm8058-pldo + qcom,rpm-pm8058-nldo + qcom,rpm-pm8901-pldo + qcom,rpm-pm8901-nldo + qcom,rpm-pm8921-pldo + qcom,rpm-pm8921-nldo + qcom,rpm-pm8921-nldo1200 + +- reg: + Usage: required + Value type: u32 + Definition: resource as defined in dt-bindings/mfd/qcom-rpm.h + must be one of: + QCOM_RPM_PM8058_LDO0 - QCOM_RPM_PM8058_LDO25, + QCOM_RPM_PM8821_LDO1, + QCOM_RPM_PM8901_LDO0 - QCOM_RPM_PM8901_LDO6, + QCOM_RPM_PM8921_LDO1 - QCOM_RPM_PM8921_LDO29 + +- bias-pull-down: + Usage: optional + Value type: empty + Definition: enable pull down of the regulator when inactive + +- qcom,force-mode: + Usage: optional + Value type: u32 + Definition: indicates that the regulator should not be forced to any + particular mode, valid values are: + QCOM_RPM_FORCE_MODE_NONE - do not force any mode + QCOM_RPM_FORCE_MODE_LPM - force into low power mode + QCOM_RPM_FORCE_MODE_HPM - force into high power mode + QCOM_RPM_FORCE_MODE_BYPASS - set regulator to use bypass +mode, i.e. to act as a
