[linux-sunxi] Re: [PATCH v3 00/12] pwm: add support for atomic update

['Doug Anderson' via linux-sunxi]

Thierry,

On Thu, Feb 25, 2016 at 3:14 PM, Doug Anderson  wrote:
> So just to summarize:
>
> * Add pwm_get_state(), pwm_apply_state(), pwm_get_args().
> pwm_get_state() initially returns 0 for duty cycle if driver doesn't
> support readout.
>
> * Re-implement pwm_get_period() (and maybe other similar functions)
> atop pwm_get_state() as you describe earlier in the thread.
>
> * Document pwm_get_period() (and maybe other similar functions) as deprecated.
>
> * Fix drivers for all current 2 users of PWM regulator to support
> hardware readout.
>
> * Update PWM regulator as you described earlier in the thread (Feb 23).
>
> * If PWM regulator is ever used on a new board whose PWM doesn't
> support hardware readout, the voltage will change at probe time.
>
>
> Did I get all that right?  Thanks!

Can you provide a "yes, you got that right" or a "no, you didn't
understand"?  That will unblock Boris, I think.

-Doug

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[linux-sunxi] Re: [PATCH v3 00/12] pwm: add support for atomic update

['Doug Anderson' via linux-sunxi]

Thierry,

On Tue, Feb 23, 2016 at 10:42 AM, Doug Anderson  wrote:
> Thierry,
>
> On Tue, Feb 23, 2016 at 10:14 AM, Thierry Reding
>  wrote:
>>> pwm_get_period(): get the period of the PWM; if the PWM has not yet
>>> been configured by software this gets the default period (possibly
>>> specified by the device tree).
>>
>> No. I think we'll need a different construct for the period defined by
>> DT or board files. pwm_get_period() is the legacy API to retrieve the
>> "current" period, even if it was lying a little before the atomic API.
>
> Ah, got it.  I think I missed that you considered pwm_get_period()
> legacy and that you eventually wanted to get rid of it.  OK, then what
> you say makes sense.
>
>
>>> That should work with one minor problem.  If HW readout isn't
>>> supported then pwm_get_state() in probe will presumably return 0 for
>>> the duty cycle.  That means it will change the voltage.  That's in
>>> contrast to how I think things work today where the voltage isn't
>>> changed until the first set_voltage() call.  At least the last time I
>>> tested things get_voltage() would simply report an incorrect value
>>> until the first set_voltage().  I think existing behavior (reporting
>>> the wrong value) is better than new behavior (change the value at
>>> probe).
>>
>> That's exactly the point. Reporting a wrong value isn't really a good
>> option. Changing the voltage on boot is the only way to make the logical
>> state match the hardware state on boot. Chances are that if you don't
>> have hardware readout support you probably don't care what state your
>> regulator will be in.
>>
>> Then again, if we don't support hardware readout, setting up the logical
>> state with data from DT (or board files) and defaulting the duty cycle
>> to 0, we end up with exactly what we had before, even with the atomic
>> API, right? Maybe that's okay, too.
>
> IMHO this is a change in behavior that will break existing users.
> Anyone using a PWM regulator will suddenly find their voltage changing
> at bootup.  Certainly today all users of the PWM regulator don't seem
> to mind (apparently) the the voltage is reported incorrectly at bootup
> but I bet they'd mind if the voltage suddenly started changing for
> them at bootup.
>
> It seems better to preserve existing behavior and print a warning that
> the voltage will be reported incorrectly until HW Readout is
> supported.
>
> Of course, we're only talking about two real users in mainline here:
> Rockchip boards and the "stih407-family".  If we just fix both of
> those to support HW Readout before landing the change then I'm fine
> with doing what you say.
>
>
>>> ...and if set_voltage() remains untouched then we can solve my probe
>>> problem by renaming pwm_get_state() to pwm_get_hw_state() and having
>>> it return an error if HW readout is not supported.  Then we only call
>>> pwm_get_args() / pwm_apply_state() when we support HW readout.
>>
>> The problem is that we make the API clumsy to use. If we don't sync the
>> "initial" state (as defined by DT or board files) to hardware at any
>> point, then we need to add the pwm_args construct and always stick to
>> it. I think it weird to have to use the pwm_args.period instead of the
>> current period.
>>
>> So we're back to square one, really. That's exactly what Mark brought up
>> originally.
>
> I had missed the part where you wanted to deprecate pwm_get_period().
> Thus my points here aren't really valid.
>
> In my mind the old API was perfectly fine (and actually quite clean /
> simple to use) except in the special case of avoiding the PWM
> regulator glitches.  With that mindset I think my previous email make
> sense.  However, this is your subsystem to maintain and if you think
> moving everyone to a new atomic API makes more sense then you're in
> the best position to make that decision.  :)

So just to summarize:

* Add pwm_get_state(), pwm_apply_state(), pwm_get_args().
pwm_get_state() initially returns 0 for duty cycle if driver doesn't
support readout.

* Re-implement pwm_get_period() (and maybe other similar functions)
atop pwm_get_state() as you describe earlier in the thread.

* Document pwm_get_period() (and maybe other similar functions) as deprecated.

* Fix drivers for all current 2 users of PWM regulator to support
hardware readout.

* Update PWM regulator as you described earlier in the thread (Feb 23).

* If PWM regulator is ever used on a new board whose PWM doesn't
support hardware readout, the voltage will change at probe time.


Did I get all that right?  Thanks!

-Doug

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[linux-sunxi] Re: [PATCH v3 00/12] pwm: add support for atomic update

['Doug Anderson' via linux-sunxi]

Thierry,

On Tue, Feb 23, 2016 at 10:14 AM, Thierry Reding
 wrote:
>> pwm_get_period(): get the period of the PWM; if the PWM has not yet
>> been configured by software this gets the default period (possibly
>> specified by the device tree).
>
> No. I think we'll need a different construct for the period defined by
> DT or board files. pwm_get_period() is the legacy API to retrieve the
> "current" period, even if it was lying a little before the atomic API.

Ah, got it.  I think I missed that you considered pwm_get_period()
legacy and that you eventually wanted to get rid of it.  OK, then what
you say makes sense.


>> That should work with one minor problem.  If HW readout isn't
>> supported then pwm_get_state() in probe will presumably return 0 for
>> the duty cycle.  That means it will change the voltage.  That's in
>> contrast to how I think things work today where the voltage isn't
>> changed until the first set_voltage() call.  At least the last time I
>> tested things get_voltage() would simply report an incorrect value
>> until the first set_voltage().  I think existing behavior (reporting
>> the wrong value) is better than new behavior (change the value at
>> probe).
>
> That's exactly the point. Reporting a wrong value isn't really a good
> option. Changing the voltage on boot is the only way to make the logical
> state match the hardware state on boot. Chances are that if you don't
> have hardware readout support you probably don't care what state your
> regulator will be in.
>
> Then again, if we don't support hardware readout, setting up the logical
> state with data from DT (or board files) and defaulting the duty cycle
> to 0, we end up with exactly what we had before, even with the atomic
> API, right? Maybe that's okay, too.

IMHO this is a change in behavior that will break existing users.
Anyone using a PWM regulator will suddenly find their voltage changing
at bootup.  Certainly today all users of the PWM regulator don't seem
to mind (apparently) the the voltage is reported incorrectly at bootup
but I bet they'd mind if the voltage suddenly started changing for
them at bootup.

It seems better to preserve existing behavior and print a warning that
the voltage will be reported incorrectly until HW Readout is
supported.

Of course, we're only talking about two real users in mainline here:
Rockchip boards and the "stih407-family".  If we just fix both of
those to support HW Readout before landing the change then I'm fine
with doing what you say.


>> ...and if set_voltage() remains untouched then we can solve my probe
>> problem by renaming pwm_get_state() to pwm_get_hw_state() and having
>> it return an error if HW readout is not supported.  Then we only call
>> pwm_get_args() / pwm_apply_state() when we support HW readout.
>
> The problem is that we make the API clumsy to use. If we don't sync the
> "initial" state (as defined by DT or board files) to hardware at any
> point, then we need to add the pwm_args construct and always stick to
> it. I think it weird to have to use the pwm_args.period instead of the
> current period.
>
> So we're back to square one, really. That's exactly what Mark brought up
> originally.

I had missed the part where you wanted to deprecate pwm_get_period().
Thus my points here aren't really valid.

In my mind the old API was perfectly fine (and actually quite clean /
simple to use) except in the special case of avoiding the PWM
regulator glitches.  With that mindset I think my previous email make
sense.  However, this is your subsystem to maintain and if you think
moving everyone to a new atomic API makes more sense then you're in
the best position to make that decision.  :)


-Doug

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[linux-sunxi] Re: [PATCH v3 00/12] pwm: add support for atomic update

['Doug Anderson' via linux-sunxi]

Thierry,

On Tue, Feb 23, 2016 at 6:38 AM, Thierry Reding
 wrote:
>> > Furthermore it's out of the question that changes to the API will be
>> > required. That's precisely the reason why the atomic PWM proposal came
>> > about. It's an attempt to solve the shortcomings of the current API for
>> > cases such as Rockchip.
>>
>> I _think_ we're on the same page here.  If there are shortcomings with
>> the current API that make it impossible to implement a feature, we've
>> got to change and/or add to the existing API.  ...but we don't want to
>> break existing users / drivers.
>>
>> Note that historically I remember that Linus Torvalds has stated that
>> there is no stable API within the Linux kernel and that forcing the
>> in-kernel API to never change was bad for software development.  I
>> tracked down my memory and found
>> .  Linus is rabid about not
>> breaking userspace, but in general there's no strong requirement to
>> never change the driver API inside the kernel.  That being said,
>> changing the driver API causes a lot of churn, so presumably changing
>> it in a backward compatible way (like adding to the API instead of
>> changing it) will make things happier.
>
> I didn't say anything about stable API. All I said is that new API
> should be well-thought-out. Those are two very different things.

I guess I just misunderstood "it's out of the question that changes to
the API will be required".  In any case, I think everyone's on the
same page here, so no need to debate further.  :)


>> >> So all we need is a new API call that lets you read the hardware
>> >> values and make sure that the PWM regulator calls that before anyone
>> >> calls pwm_config().  That's roughly B) above.
>> >
>> > Yes. I'm thinking that we should have a pwm_get_state() which retrieves
>> > the current state of the PWM. For drivers that support hardware readout
>> > this state should match the hardware state. For other drivers it should
>> > reflect whatever was specified in DT; essentially what pwm_get_period()
>> > and friends return today.
>>
>> Excellent, so pwm_get_period() gets the period as specified in the
>> device tree (or other board config) and pwm_get_state() returns the
>> hardware state.  SGTM.
>
> That's not quite what I was thinking. If hardware readout is supported
> then whatever we report back should be the current hardware state unless
> we're explicitly asked for something else. If we start mixing the state
> and legacy APIs this way, we'll get into a situation where drivers that
> support hardware readout behave differently than drivers that don't.
>
> For example: A PWM device that's controlled by a driver that supports
> hardware readout has a current period of 5 ns and the firmware set
> the period to 25000 ns. pwm_get_period() for this PWM device will return
> 5 ns. If you reconfigure the PWM to generate a PWM signal with a
> period of 3 ns, pwm_get_period() would still return 5 ns.
>
> A driver that doesn't support hardware readout, on the contrary, would
> return 5 ns from pwm_get_period() on the first call, but after you
> have reconfigured it using pwm_config() it will return the new period.

Ah, right!  I forgot that the existing API will be updated if you've
reconfigured the period via pwm_config().  Ugh, you're right that's a
little ugly then.

So do we define it as:

pwm_get_state(): always get the hardware state w/ no caching (maybe
even pwm_get_raw_state() or pwm_get_hw_state())

pwm_get_period(): get the period of the PWM; if the PWM has not yet
been configured by software this gets the default period (possibly
specified by the device tree).


Is that OK?  That is well defined and doesn't change the existing
behavior of pwm_get_period().


>> > That way if you want to get the current voltage in the regulator-pwm
>> > driver you'd simply do a pwm_get_state() and compute the voltage from
>> > the period and duty cycle. If the PWM driver that you happen to use
>> > doesn't support hardware readout, you'll get an initial output voltage
>> > of 0, which is as good as any, really.
>>
>> Sounds fine to me.  PWM regulator is in charge of calling
>> pwm_get_state(), which can return 0 (or an error?) if driver (or
>> underlying hardware) doesn't support hardware readout.  PWM regulator
>> is in charge of using the resulting period / duty cycle to calculate a
>> percentage.
>
> I'm not sure if pwm_get_state() should ever return an error. For drivers
> that support hardware readout, the resulting state should match what's
> programmed to the hardware.
>
> But for drivers without hardware readout support pwm_get_state() still
> makes sense because after a pwm_apply_state() the internal logical state
> would again match hardware.

I guess it depends on how you define things.  With my above
definitions it seems clearest if pwm_get_state() returns an error if
hardware readout is not supported.  If we call it 

[linux-sunxi] Re: [PATCH v3 00/12] pwm: add support for atomic update

['Doug Anderson' via linux-sunxi]

Mark,

On Mon, Feb 22, 2016 at 1:24 PM, Mark Brown  wrote:
> On Mon, Feb 22, 2016 at 11:15:09AM -0800, Doug Anderson wrote:
>
>> Note that historically I remember that Linus Torvalds has stated that
>> there is no stable API within the Linux kernel and that forcing the
>> in-kernel API to never change was bad for software development.  I
>> tracked down my memory and found
>> .  Linus is rabid about not
>> breaking userspace, but in general there's no strong requirement to
>> never change the driver API inside the kernel.  That being said,
>> changing the driver API causes a lot of churn, so presumably changing
>> it in a backward compatible way (like adding to the API instead of
>> changing it) will make things happier.
>
> You do need to fix the users though, change is fine but you can't cause
> people's systems to break.

Yes, of course!  :)  Thanks for clarifying.

-Doug

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[linux-sunxi] Re: [PATCH v3 00/12] pwm: add support for atomic update

['Doug Anderson' via linux-sunxi]

Thierry,

On Mon, Feb 22, 2016 at 9:59 AM, Thierry Reding
 wrote:
>> This is because only some drivers would be able to read the hardware
>> state?  I'm not sure how we can get away from that.  In all proposals
>> we've talked about (including what you propose below, right?) the PWM
>> regulator will need a PWM driver that can read hardware state.  Only
>> PWM drivers that have been upgraded to support reading hardware state
>> can use the PWM regulator (or at least only those drivers will be able
>> to use the PWM regulator glitch-free).
>
> Yes, the key here is glitch-free. There's no reason whatsoever that the
> rugaltor-pwm driver should be limited to usage with a hardware readout-
> capable PWM driver. If you don't care about glitches, likely because no
> critical components depend on the regulator, you can simply force what
> state you choose on boot.
>
> As a matter of fact, I think that's how regulators work already. If the
> current output voltage doesn't match the specified constraints, then a
> valid value will be forced by the regulator core. If the voltage lies
> within the constraints the core won't touch the regulator. Is this not
> going to "just work" with the PWM regulator?
>
> The problem is somewhat simplified if that's the case. An implementation
> could then fail the regulator_get_voltage() if hardware readout is not
> supported and return the current voltage when readout is possible.

Based on looking at the current code, I believe it just returns 0V
until you call regulator_set_voltage() today.  I don't think that was
always the case.  Back before it was made continuous I think it
returned the voltage that matched with 0% duty cycle.

I haven't dug into what the regulator framework does in the current
system nor what happens if regulator_get_voltage() returns an error.
Perhaps Boris can dig / comment?


>> When we add a new feature then it's expected that only updated drivers
>> will support that feature.
>>
>> We need to make sure that we don't regress / negatively change the
>> behavior for anyone running non-updated drivers.  ...and we should
>> strive to require as few changes to drivers as possible.  ...but if
>> the best we can do requires changes to the PWM driver API then we will
>> certainly have differences depending on the PWM driver.
>
> How so? Drivers should behave consistently, irrespective of the API. Of
> course if you need to change behaviour of the user driver depending on
> the availability of a certain feature, that's perfectly fine.
>
> Furthermore it's out of the question that changes to the API will be
> required. That's precisely the reason why the atomic PWM proposal came
> about. It's an attempt to solve the shortcomings of the current API for
> cases such as Rockchip.

I _think_ we're on the same page here.  If there are shortcomings with
the current API that make it impossible to implement a feature, we've
got to change and/or add to the existing API.  ...but we don't want to
break existing users / drivers.

Note that historically I remember that Linus Torvalds has stated that
there is no stable API within the Linux kernel and that forcing the
in-kernel API to never change was bad for software development.  I
tracked down my memory and found
.  Linus is rabid about not
breaking userspace, but in general there's no strong requirement to
never change the driver API inside the kernel.  That being said,
changing the driver API causes a lot of churn, so presumably changing
it in a backward compatible way (like adding to the API instead of
changing it) will make things happier.


>> That means that if you call pwm_get_period() right away at boot time
>> you're not getting the current period of the hardware but the period
>> that was specified in the device tree.
>
> Yes.
>
>> So all we need is a new API call that lets you read the hardware
>> values and make sure that the PWM regulator calls that before anyone
>> calls pwm_config().  That's roughly B) above.
>
> Yes. I'm thinking that we should have a pwm_get_state() which retrieves
> the current state of the PWM. For drivers that support hardware readout
> this state should match the hardware state. For other drivers it should
> reflect whatever was specified in DT; essentially what pwm_get_period()
> and friends return today.

Excellent, so pwm_get_period() gets the period as specified in the
device tree (or other board config) and pwm_get_state() returns the
hardware state.  SGTM.


> That way if you want to get the current voltage in the regulator-pwm
> driver you'd simply do a pwm_get_state() and compute the voltage from
> the period and duty cycle. If the PWM driver that you happen to use
> doesn't support hardware readout, you'll get an initial output voltage
> of 0, which is as good as any, really.

Sounds fine to me.  PWM regulator is in charge of calling
pwm_get_state(), which can return 0 (or an error?) if driver (or
underlying 

[linux-sunxi] Re: [PATCH v3 00/12] pwm: add support for atomic update

['Doug Anderson' via linux-sunxi]

Thierry,

On Wed, Feb 3, 2016 at 11:04 AM, Doug Anderson  wrote:
> Thierry
>
> On Wed, Feb 3, 2016 at 6:53 AM, Thierry Reding  
> wrote:
>>> A) The software state here is the period and flags (AKA "inverted),
>>> right?  It does seem possible that you could apply the period and
>>> flags while keeping the calculated bootup duty cycle percentage
>>> (presuming that the PWM was actually enabled at probe time and there
>>> was a bootup duty cycle at all).  That would basically say that
>>> whenever you set the period of a PWM then the duty cycle of the PWM
>>> should remain the same percentage.  That actually seems quite sane
>>> IMHO.  It seems much saner than trying to keep the duty cycle "ns"
>>> when the period changes or resetting the PWM to some default when the
>>> period changes.
>>
>> That really depends on the use-case. If you're interested in the output
>> power of the PWM then, yes, this is sane. But it might not be the right
>> answer in other cases.
>
> Ah, I see.  You're envisioning a device where active time in "ns" is
> more important than the percentage of active time.  Perhaps an LED
> where it's more important to have it on for no more than .1 seconds
> (so we don't drive it too long and burn it out?).  If we slowed down
> the duty cycle and adjusted the period to match, we could get into a
> bad state.
>
>
>>> B) Alternatively, I'd also say that setting a period without a duty
>>> cycle doesn't make a lot of sense.  ...so you could just apply the
>>> period at the same time that you apply the duty cycle the first time.
>>> Presumably you'd want to "lie" to the callers of the PWM subsystem and
>>> tell them that you already changed the period even though the change
>>> won't really take effect until they actually set the duty cycle.  If
>>> anyone cared to find out the true hardware period we could add a new
>>> pwm_get_hw_period().  ...or since the only reason you'd want to know
>>> the hardware period would be if you're trying to read the current duty
>>> cycle percentage, you could instead add "pwm_get_hw_state()" and have
>>> that return both the hardware period ns and duty cycle ns (which is
>>> the most accurate way to return the "percentage" without using fix or
>>> floating point math).
>>
>> But then you get into a situation where behaviour is dependent on the
>> PWM driver, whereas this is really very specific to one specific use-
>> case.
>
> This is because only some drivers would be able to read the hardware
> state?  I'm not sure how we can get away from that.  In all proposals
> we've talked about (including what you propose below, right?) the PWM
> regulator will need a PWM driver that can read hardware state.  Only
> PWM drivers that have been upgraded to support reading hardware state
> can use the PWM regulator (or at least only those drivers will be able
> to use the PWM regulator glitch-free).
>
> When we add a new feature then it's expected that only updated drivers
> will support that feature.
>
> We need to make sure that we don't regress / negatively change the
> behavior for anyone running non-updated drivers.  ...and we should
> strive to require as few changes to drivers as possible.  ...but if
> the best we can do requires changes to the PWM driver API then we will
> certainly have differences depending on the PWM driver.
>
>
>> In the end the PWM API is as low-level as it is because it needs to be
>> flexible enough to cope with other use-cases. In the general case the
>> simple truth is that it doesn't make sense to set a period without the
>> duty cycle and vice versa. That's why pwm_config() takes both as input
>> parameters. The atomic API is going to take that one step further in
>> that you need to specify the complete state of the PWM when applying.
>
> I guess this is still showing the strangeness of the device tree
> specifying a period without a duty cycle.  You just said it doesn't
> make sense, but that's exactly what the device tree has.
>
> I'd imagine that the only reason that the device tree specifies just
> the period is that it's intended to be there only for clients that
> don't care about the specific duty cycle in terms of seconds but
> _only_ care about the duty cycle in terms of percentage.  Anyone who
> cared about the duty cycle in terms of seconds would presumably also
> care about specifying the period (in terms of seconds) in the same
> place they specify the duty cycle.
>
>
>>> I think this is like my suggestion B), right?  AKA the PWM regulator
>>> would be the sole caller of pwm_get_hw_state() and it would use this
>>> to figure out the existing duty cycle percentage.  Then it would
>>> translate that into "ns" and would set the duty cycle.  Upon the first
>>> set of the duty cycle both the period and duty cycle would be applied
>>> at the same time.
>>
>> Yes and no. I think the PWM regulator would need to get the current
>> hardware state and derive the output power from duty 

[linux-sunxi] Re: [PATCH v3 00/12] pwm: add support for atomic update

['Doug Anderson' via linux-sunxi]

Thierry

On Wed, Feb 3, 2016 at 6:53 AM, Thierry Reding  wrote:
>> A) The software state here is the period and flags (AKA "inverted),
>> right?  It does seem possible that you could apply the period and
>> flags while keeping the calculated bootup duty cycle percentage
>> (presuming that the PWM was actually enabled at probe time and there
>> was a bootup duty cycle at all).  That would basically say that
>> whenever you set the period of a PWM then the duty cycle of the PWM
>> should remain the same percentage.  That actually seems quite sane
>> IMHO.  It seems much saner than trying to keep the duty cycle "ns"
>> when the period changes or resetting the PWM to some default when the
>> period changes.
>
> That really depends on the use-case. If you're interested in the output
> power of the PWM then, yes, this is sane. But it might not be the right
> answer in other cases.

Ah, I see.  You're envisioning a device where active time in "ns" is
more important than the percentage of active time.  Perhaps an LED
where it's more important to have it on for no more than .1 seconds
(so we don't drive it too long and burn it out?).  If we slowed down
the duty cycle and adjusted the period to match, we could get into a
bad state.


>> B) Alternatively, I'd also say that setting a period without a duty
>> cycle doesn't make a lot of sense.  ...so you could just apply the
>> period at the same time that you apply the duty cycle the first time.
>> Presumably you'd want to "lie" to the callers of the PWM subsystem and
>> tell them that you already changed the period even though the change
>> won't really take effect until they actually set the duty cycle.  If
>> anyone cared to find out the true hardware period we could add a new
>> pwm_get_hw_period().  ...or since the only reason you'd want to know
>> the hardware period would be if you're trying to read the current duty
>> cycle percentage, you could instead add "pwm_get_hw_state()" and have
>> that return both the hardware period ns and duty cycle ns (which is
>> the most accurate way to return the "percentage" without using fix or
>> floating point math).
>
> But then you get into a situation where behaviour is dependent on the
> PWM driver, whereas this is really very specific to one specific use-
> case.

This is because only some drivers would be able to read the hardware
state?  I'm not sure how we can get away from that.  In all proposals
we've talked about (including what you propose below, right?) the PWM
regulator will need a PWM driver that can read hardware state.  Only
PWM drivers that have been upgraded to support reading hardware state
can use the PWM regulator (or at least only those drivers will be able
to use the PWM regulator glitch-free).

When we add a new feature then it's expected that only updated drivers
will support that feature.

We need to make sure that we don't regress / negatively change the
behavior for anyone running non-updated drivers.  ...and we should
strive to require as few changes to drivers as possible.  ...but if
the best we can do requires changes to the PWM driver API then we will
certainly have differences depending on the PWM driver.


> In the end the PWM API is as low-level as it is because it needs to be
> flexible enough to cope with other use-cases. In the general case the
> simple truth is that it doesn't make sense to set a period without the
> duty cycle and vice versa. That's why pwm_config() takes both as input
> parameters. The atomic API is going to take that one step further in
> that you need to specify the complete state of the PWM when applying.

I guess this is still showing the strangeness of the device tree
specifying a period without a duty cycle.  You just said it doesn't
make sense, but that's exactly what the device tree has.

I'd imagine that the only reason that the device tree specifies just
the period is that it's intended to be there only for clients that
don't care about the specific duty cycle in terms of seconds but
_only_ care about the duty cycle in terms of percentage.  Anyone who
cared about the duty cycle in terms of seconds would presumably also
care about specifying the period (in terms of seconds) in the same
place they specify the duty cycle.


>> I think this is like my suggestion B), right?  AKA the PWM regulator
>> would be the sole caller of pwm_get_hw_state() and it would use this
>> to figure out the existing duty cycle percentage.  Then it would
>> translate that into "ns" and would set the duty cycle.  Upon the first
>> set of the duty cycle both the period and duty cycle would be applied
>> at the same time.
>
> Yes and no. I think the PWM regulator would need to get the current
> hardware state and derive the output power from duty cycle and period.
> It would then need to rescale to whatever new period it wants to use
> to ensure the same output power is used.

Right.  We all agree that somehow this needs to happen, it's just a
question of the 

[linux-sunxi] Re: [PATCH v3 00/12] pwm: add support for atomic update

['Doug Anderson' via linux-sunxi]

Thierry and Boris,

On Tue, Nov 10, 2015 at 9:34 AM, Thierry Reding
 wrote:
> On Mon, Oct 19, 2015 at 12:12:12PM +0200, Heiko Stübner wrote:
>> Hi Thierry,
>>
>> Am Montag, 21. September 2015, 11:33:17 schrieb Boris Brezillon:
>> > Hello,
>> >
>> > This series adds support for atomic PWM update, or IOW, the capability
>> > to update all the parameters of a PWM device (enabled/disabled, period,
>> > duty and polarity) in one go.
>>
>> is anything more blocking this series? It's now sitting on the lists for
>> nearly a month and everybody seems happy with it, so it would be really nice
>> to have in mainline :-) .
>>
>> Especially as this also makes it possible for Rockchip Chromebooks to 
>> actually
>> control the logic-regulator that is implemented as pwm-regulator there.
>
> Last time I tried to put this into linux-next I got immediately
> bombarded by a number of build failures, so I backed things out. The
> current plan is to give this another try after v4.4-rc1.

We're now into the 4.5 timeframe.  Does anyone have a concrete set of
things that need to happen before this patch series makes it into
mainline?

>From searching I see that the latest version of this series is v4 and
there are a smattering of comments on the 24-patch series.  Presumably
a v5 needs to be posted to address those things.

...but it looks like the big sticking point is that Boris is waiting
for a response to his questions in
.  Thierry: can you give
Boris some direction for what else he needs to do?  We need to come up
with _some_ solution since this series gets us much better support for
PWM regulators.  Without this series or some other solution, PWM
regulators aren't usable in mainline on any system that uses them for
system-critical rails.  Nearly all Rockchip reference boards and
shipping devices uses a PWM regulator for the system-critidal "logic"
rail.  That means any patches which need to change this rail in Linux
are blocked.

If there's already been off-list discussion and Boris already knows
what the next steps are then my apologies and I'll wait patiently for
the next series.  ;)

Thanks!

-Doug

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[linux-sunxi] Re: [PATCH v3 00/12] pwm: add support for atomic update

['Doug Anderson' via linux-sunxi]

Hi,

On Mon, Jan 25, 2016 at 9:08 AM, Thierry Reding
 wrote:
> I really don't understand this design decision. I presume that the PWM
> controlling this system-critical logic is driven by the SoC? So if the
> regulator is system-critical, doesn't that make it a chicken and egg
> problem? How can the SoC turn the PWM on if it doesn't have power? But
> perhaps I'm completely misunderstanding what you're saying. Perhaps if
> somebody could summarize how exactly this works, it would help better
> understand the requirements or what's the correct thing to do.

Sure, here's how the dang thing works, as I understand it.

First, an overview of PWM regulator in general (maybe you know this,
but to get us on the same page).  There's an external regulator on the
system.  Looking on at least one board I see a TLV62565 specifically.

>From the docs of TLV62565, I see it describe the situation as the chip
being able to provide an adjustable output voltage configurable via an
external resistor divider.  In simplified terms words you can adjust
the output voltage of the regulator by tweaking the inputs to one of
its pins.  I'm just a software guy so I can't explain all the details
of it, but the net-net of the situation is is that you can hook this
configuration pin up to the output of a PWM (with a bunch of well
balanced resistors and capacitors) and then you can set the voltage
based on the output of the PWM.


OK, so what happens at bootup?  At bootup most of the pins of the
rk3288 (including the PWM) are configured as inputs with a pull.  The
particular pin hooked up to this PWM has a pulldown.  Remember that
we've got this nicely balanced set of resistors and capacitors hooked
up to the output of our PWM pin?  So what happens when we have this
pin configured as an input?  As I understand it / remember it:

* input w/ no pull: equivalent to 50% duty cycle on the PWM
* input w/ pull down: equivalent to slightly higher voltage than 50%
duty cycle on the PWM
* input w/ pull up: equivalent to slightly lower voltage than 50% duty
cycle on the PWM

On our particular board that means that the rail comes up with roughly
1.1V.  If you drive the PWM at 100% (or set the pin to output high)
you get .86V and if you drive the PWM at 0% (or set the pin to output
low) you get 1.36V.

Now, 1.1V is plenty of voltage to boot the system.  In fact most of
the logic within the SoC can run as low as 0.95V I think.  ...but 0.86
V is not enough to run the logic parts of the system (even at their
default bootup frequencies) 1.1V is _definitely_ not enough to run the
SDRAM memory controller at full speed.


So the bootloader wants to run the system fast so it can boot fast.
It increases the CPU rails (as is typical for a bootloader) and moves
the ARM CPU to 1.8GHz (from the relatively slow boot frequency) and
also raises the logic rail to 1.2V (or I think 1.15 V on systems w/
different memory configs) and inits the SDRAM controller to run at
full speed.  Then it boots Linux.

Note: apparently in U-Boot they actually boot system slower (this was
at least true 1.5 years ago with some reference U-Boot Rockchip
provided).  If I understand correctly they _didn't_ init the SDRAM
controller as full speed in the bootloader and just left the logic
rail at its bootup default.  If everyone had done that then our job
would be "easier" because we wouldn't need to read in the voltage
provided by the bootloader (by reading the PWM and cros-referencing
with our table), though even in that case we'd have to be very careful
not to glitch the line (since .86 V is too low).  Of course all of
those systems are stuck running at a very slow memory speed until
Linux gets DDR Frequency support for Rockchip whereas systems with our
bootloader not only boot faster but also get to use the full memory
speed even without any Linux DDRFreq drivers.


In any case: I think I've demonstrated how a critical system rail can
be using a PWM regulator and how glitching that PWM regulator at boot
time can be catastrophic.  Possibly it's not critical to be able to
"read" the voltage that that bootloader left things configured at
(it's mostly nice for debugging purposes), but it's definitely
important to make sure we don't set it to some default and important
to never glitch it.  Said another way, presumably a DDR Freq driver
would be able to switch the memory controller frequency sanely by
reading the memory controller frequency and using that to figure out
whether it needed to up the logic rail before or after the DDR Freq
change.


>> If there's already been off-list discussion and Boris already knows
>> what the next steps are then my apologies and I'll wait patiently for
>> the next series.  ;)
>
> I don't think we reached a conclusion on this. And to be honest, I'm not
> sure what the right way forward is in this situation. So in order to
> make some forward progress I suggest we start a discussion, hopefully
> that will clarify the situation and help