AB8500 Regulators

10 messages, 2 authors, 2012-07-31 · open the first message on its own page

AB8500 Regulators

From: Lee Jones <hidden>
Date: 2012-07-30 13:49:43

Hi Mark,

I've taken some time to review the way we currently initialise the 
AB8500 regulators. I don't know if you recall, but in platform code we 
have a struct full of initial values, which sets up lots of bespoke 
options. These options require lots of non-standard register writes in 
order to configure the regulators in the way which best suits 
ST-Ericsson's needs.

I wrote a patch which pulled each of these initial values into Device 
Tree, but you NACK:ed it believing that most of the options could be 
dealt with using the API. There is currently no API to set up 
ST-Ericsson's configuration. So what's the best thing to do? How do all 
of the other regulator drivers deal with non-standard configuration? Or 
is it unheard of to use non-standard start-up values?

Kind regards,
Lee

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

AB8500 Regulators

From: Mark Brown <hidden>
Date: 2012-07-30 15:26:18

On Mon, Jul 30, 2012 at 02:49:43PM +0100, Lee Jones wrote:
I wrote a patch which pulled each of these initial values into
Device Tree, but you NACK:ed it believing that most of the options
could be dealt with using the API. There is currently no API to set
up ST-Ericsson's configuration. So what's the best thing to do? How
do all of the other regulator drivers deal with non-standard
configuration? Or is it unheard of to use non-standard start-up
values?
Well, a good start would be to work out what those register writes
actually do and then we can have a discussion on how to do whatever
that is.

AB8500 Regulators

From: Lee Jones <hidden>
Date: 2012-07-30 16:32:10

On 30/07/12 16:26, Mark Brown wrote:
On Mon, Jul 30, 2012 at 02:49:43PM +0100, Lee Jones wrote:
quoted
I wrote a patch which pulled each of these initial values into
Device Tree, but you NACK:ed it believing that most of the options
could be dealt with using the API. There is currently no API to set
up ST-Ericsson's configuration. So what's the best thing to do? How
do all of the other regulator drivers deal with non-standard
configuration? Or is it unheard of to use non-standard start-up
values?
Well, a good start would be to work out what those register writes
actually do and then we can have a discussion on how to do whatever
that is.
Most of the configuration seems to pertain to voltage selection and 
making regulators valid. I assume this means you can enable them via 
their own registers and the enablement will take hold. This seems like 
pretty routine stuff as far as regulator configuration goes.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

AB8500 Regulators

From: Mark Brown <hidden>
Date: 2012-07-30 17:48:31

On Mon, Jul 30, 2012 at 05:32:10PM +0100, Lee Jones wrote:
On 30/07/12 16:26, Mark Brown wrote:
quoted
On Mon, Jul 30, 2012 at 02:49:43PM +0100, Lee Jones wrote:
quoted
quoted
could be dealt with using the API. There is currently no API to set
up ST-Ericsson's configuration. So what's the best thing to do? How
quoted
Well, a good start would be to work out what those register writes
actually do and then we can have a discussion on how to do whatever
that is.
Most of the configuration seems to pertain to voltage selection and
So what makes you say there's no API for any of this then?  Clearly
setting voltages is something that's supported.
making regulators valid. I assume this means you can enable them via
their own registers and the enablement will take hold. This seems
Please be more specific, frankly I'm having a really hard time working
out what the above means.  Obviously most of what drivers do involves
writing to registers...
like pretty routine stuff as far as regulator configuration goes.
Which seems like a very good reason why it shouldn't just be done by a
magic sequence of undocumented register writes.

AB8500 Regulators

From: Lee Jones <hidden>
Date: 2012-07-31 08:46:28

On 30/07/12 18:48, Mark Brown wrote:
On Mon, Jul 30, 2012 at 05:32:10PM +0100, Lee Jones wrote:
quoted
On 30/07/12 16:26, Mark Brown wrote:
quoted
On Mon, Jul 30, 2012 at 02:49:43PM +0100, Lee Jones wrote:
quoted
quoted
quoted
could be dealt with using the API. There is currently no API to set
up ST-Ericsson's configuration. So what's the best thing to do? How
quoted
quoted
Well, a good start would be to work out what those register writes
actually do and then we can have a discussion on how to do whatever
that is.
quoted
Most of the configuration seems to pertain to voltage selection and
So what makes you say there's no API for any of this then?  Clearly
setting voltages is something that's supported.
quoted
making regulators valid. I assume this means you can enable them via
their own registers and the enablement will take hold. This seems
Please be more specific, frankly I'm having a really hard time working
out what the above means.  Obviously most of what drivers do involves
writing to registers...
quoted
like pretty routine stuff as far as regulator configuration goes.
Which seems like a very good reason why it shouldn't just be done by a
magic sequence of undocumented register writes.
Please listen to what I'm saying as a whole. Breaking that paragraph up 
takes it entirely out of context. I'm mostly agreeing with what you're 
saying, and asking you some advice.

Let me attempt to break it down, so there's no more ambiguity:

- I know that Device Drivers write to registers!
- I agree that magic-number blasting is the wrong thing to do here
- My aim is to convert to using whatever current APIs there are
- I'm happy to extend APIs or write new ones as required
- I now know how the changing voltage API works
   - What I don't know is where we'd call it from to initialise them
   - Can we do that from the ab8500 regulator driver's init() or probe()?
- WRT the 'valid' registers, I'm not sure what they do. The doc says:
   "Supply control thru Hardware signal and SysClkReq balls validation."
   - More comments on what the writes do individually can be seen in [1]
   - Perhaps we can extend the API thus:
quoted hunk
diff --git a/drivers/regulator/ab8500.c b/drivers/regulator/ab8500.c
index 2dac0d6..87fb141 100644
--- a/drivers/regulator/ab8500.c
+++ b/drivers/regulator/ab8500.c
@@ -51,6 +51,10 @@ struct ab8500_regulator_info {
        int max_uV;
        int min_uV;
        int fixed_uV;
+       u8 valid_bank;
+       u8 valid_reg;
+       u8 valid_mask;
+       u8 valid_val_enable;
        u8 update_bank;
        u8 update_reg;
        u8 update_mask;
Then I think most (if not all) of our bases are covered.

[1] arch/arm/mach-ux500/board-mop500-regulators.c
     'struct ab8500_regulator_reg_init ab8500_regulator_reg_init[]'

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

AB8500 Regulators

From: Mark Brown <hidden>
Date: 2012-07-31 11:08:30

On Tue, Jul 31, 2012 at 09:46:28AM +0100, Lee Jones wrote:
Please listen to what I'm saying as a whole. Breaking that paragraph
up takes it entirely out of context. I'm mostly agreeing with what
you're saying, and asking you some advice.
That's unfortunately not how it comes over (especially in the context of
some of the other serieses), it really feels like a lot of work to find
out what these magic register writes are supposed to do.
- I now know how the changing voltage API works
  - What I don't know is where we'd call it from to initialise them
  - Can we do that from the ab8500 regulator driver's init() or probe()?
I'm surprised you've managed to miss the existing interfaces:

   include/linux/regulator/machine.h
   Documentation/devicetree/bindings/regulator/regulator.txt
- WRT the 'valid' registers, I'm not sure what they do. The doc says:
  "Supply control thru Hardware signal and SysClkReq balls validation."
  - More comments on what the writes do individually can be seen in [1]
  - Perhaps we can extend the API thus:
These things don't tie very well.  The comments in the sequences look
reasonably meaningful but you're saying the datasheet doesn't document
anything at all.
quoted
+       u8 valid_bank;
+       u8 valid_reg;
+       u8 valid_mask;
+       u8 valid_val_enable;
Then I think most (if not all) of our bases are covered.
No, there's a whole series of problems here.  The most serious one is
that "valid" means nothing, we need *some* effort at semantics here.
We're also going to run into trouble as soon as we've got to update two
different registers.

It looks like you're trying to put something vaugely like regmap patches
into the regulator API, but it seems fairly clear that this stuff is
board specific configuration rather than things that should be blasted
in unconditionally on any board.

AB8500 Regulators

From: Lee Jones <hidden>
Date: 2012-07-31 12:26:12

On 31/07/12 12:08, Mark Brown wrote:
On Tue, Jul 31, 2012 at 09:46:28AM +0100, Lee Jones wrote:
quoted
Please listen to what I'm saying as a whole. Breaking that paragraph
up takes it entirely out of context. I'm mostly agreeing with what
you're saying, and asking you some advice.
That's unfortunately not how it comes over (especially in the context of
some of the other serieses), it really feels like a lot of work to find
out what these magic register writes are supposed to do.
This isn't the same context as the other series. Previously I was trying 
to reproduce what the platform was doing to get Device Tree up and 
running. After you informed me that register bashing was not only the 
correct thing do with regards to Device Tree, the platform shouldn't be 
doing it either, I took a side-action to come back to it and do it 
correctly on both counts. Well, here we are!
quoted
- I now know how the changing voltage API works
   - What I don't know is where we'd call it from to initialise them
   - Can we do that from the ab8500 regulator driver's init() or probe()?
I'm surprised you've managed to miss the existing interfaces:

    include/linux/regulator/machine.h
    Documentation/devicetree/bindings/regulator/regulator.txt
I saw the interfaces, I just don't know what's the correct thing to do. 
I'm leaving Device Tree for the moment. I'm more concerned about 
addressing the problem in the AB8500 Regulator Driver and it's Platform 
counterpart.

The only thing of interest that I could see was the regulator_init() 
call-back. Is this what you were alluding to in the first link?
quoted
- WRT the 'valid' registers, I'm not sure what they do. The doc says:
   "Supply control thru Hardware signal and SysClkReq balls validation."
   - More comments on what the writes do individually can be seen in [1]
   - Perhaps we can extend the API thus:
These things don't tie very well.  The comments in the sequences look
reasonably meaningful but you're saying the datasheet doesn't document
anything at all.
If it does explain what these 'valid' settings do, I can't seem to find 
where. The quote a provided is about as close to explanatory as I can 
find, and I'm buggered if I know what it means.
quoted
quoted
+       u8 valid_bank;
+       u8 valid_reg;
+       u8 valid_mask;
+       u8 valid_val_enable;
quoted
Then I think most (if not all) of our bases are covered.
No, there's a whole series of problems here.  The most serious one is
that "valid" means nothing, we need *some* effort at semantics here.
We're also going to run into trouble as soon as we've got to update two
different registers.

It looks like you're trying to put something vaugely like regmap patches
into the regulator API, but it seems fairly clear that this stuff is
board specific configuration rather than things that should be blasted
in unconditionally on any board.
I guess we're going to have to take a rain-check until I can find 
someone who can translate the documentation for me. I just can't seem to 
find even a small paragraph or such which can provide me with enough 
information to make any decisions.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

AB8500 Regulators

From: Mark Brown <hidden>
Date: 2012-07-31 13:47:16

On Tue, Jul 31, 2012 at 01:26:12PM +0100, Lee Jones wrote:
On 31/07/12 12:08, Mark Brown wrote:
quoted
quoted
- I now know how the changing voltage API works
  - What I don't know is where we'd call it from to initialise them
  - Can we do that from the ab8500 regulator driver's init() or probe()?
quoted
I'm surprised you've managed to miss the existing interfaces:
quoted
   include/linux/regulator/machine.h
   Documentation/devicetree/bindings/regulator/regulator.txt
I saw the interfaces, I just don't know what's the correct thing to
do. I'm leaving Device Tree for the moment. I'm more concerned about
addressing the problem in the AB8500 Regulator Driver and it's
Platform counterpart.
The only thing of interest that I could see was the regulator_init()
call-back. Is this what you were alluding to in the first link?
No.  apply_uV.
I guess we're going to have to take a rain-check until I can find
someone who can translate the documentation for me. I just can't
seem to find even a small paragraph or such which can provide me
with enough information to make any decisions.
It does seem that way, yes.

AB8500 Regulators

From: Lee Jones <hidden>
Date: 2012-07-31 14:21:53

On 31/07/12 14:47, Mark Brown wrote:
On Tue, Jul 31, 2012 at 01:26:12PM +0100, Lee Jones wrote:
quoted
On 31/07/12 12:08, Mark Brown wrote:
quoted
quoted
quoted
- I now know how the changing voltage API works
   - What I don't know is where we'd call it from to initialise them
   - Can we do that from the ab8500 regulator driver's init() or probe()?
quoted
quoted
I'm surprised you've managed to miss the existing interfaces:
quoted
quoted
    include/linux/regulator/machine.h
    Documentation/devicetree/bindings/regulator/regulator.txt
quoted
I saw the interfaces, I just don't know what's the correct thing to
do. I'm leaving Device Tree for the moment. I'm more concerned about
addressing the problem in the AB8500 Regulator Driver and it's
Platform counterpart.
quoted
The only thing of interest that I could see was the regulator_init()
call-back. Is this what you were alluding to in the first link?
No.  apply_uV.
Ah, nice.

So is it worth me making at least those changes, so we can start 
thinning down our platform data overhead?

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

AB8500 Regulators

From: Mark Brown <hidden>
Date: 2012-07-31 14:28:52

On Tue, Jul 31, 2012 at 03:21:53PM +0100, Lee Jones wrote:
On 31/07/12 14:47, Mark Brown wrote:
quoted
quoted
The only thing of interest that I could see was the regulator_init()
call-back. Is this what you were alluding to in the first link?
quoted
No.  apply_uV.
Ah, nice.
So is it worth me making at least those changes, so we can start
thinning down our platform data overhead?
Implementing support for framework features is of course useful.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help