From: Boris Brezillon <hidden> Date: 2016-06-08 16:35:00
Hello,
This patch series series aims at adding two important features to the
pwm-regulator driver.
The first one is the support for 'smooth handover' between the
bootloader and the kernel. This is mainly solving problems we have when
the PWM is controlling a critical regulator (like the one powering the
DDR chip). Currently, when the PWM regulator acquire the PWM device it
assumes it was off and it's safe to change the configuration before
enabling it, which can generate glitches on the PWM signal which in turn
generated glitches on the output voltage.
To solve that we've introduced support for hardware readout to the
PWM framework, so that the PWM regulator driver can adjust the PWM
a probe time and avoid glitches.
Atomic update is also helping in this regard.
Patch 1 is adding convenient helpers (at the PWM level) that will be
used by the PWM regulator driver.
Patches 2 to 7 are preparing everything on the PWM driver side to make
hardware readout available to all platforms using the PWM regulator
driver (rockchip and sti).
Patches 8 to 11 are making use of the atomic update and hardware readout
features to implement this smooth handover.
The second feature we add to the driver is the capability of using
a sub duty_cycle range in continuous mode. By default the regulator
is assuming that min_uV is achieved with a 0% dutycyle and max_uV
with a 100% dutycycle, but this is not necessarily true.
Moreover, in some cases (when the PWM device does not support
polarity inversion), we might have min_uV at 100% and max_uV at 0%.
Hence the addition of new properties to the existing DT bindings.
The feature is added in patch 12 and 13.
Best Regards,
Boris
Changes since v1:
- dropped already applied patches
- added R-b/A-b/T-b tags
- s/readl/readl_relaxed/ in patch 3 (as suggested by Brian)
- fixed pwm-regulator DT binding doc
- added some comments in the code
- replaced pwm_get_state() + if (state.enabled) by if (pwm_is_enabled())
Boris Brezillon (13):
pwm: Add new helpers to create/manipulate PWM states
pwm: rockchip: Fix period and duty_cycle approximation
pwm: rockchip: Add support for hardware readout
pwm: rockchip: Avoid glitches on already running PWMs
pwm: rockchip: Add support for atomic update
pwm: sti: Add support for hardware readout
pwm: sti: Avoid glitches on already running PWMs
regulator: pwm: Adjust PWM config at probe time
regulator: pwm: Switch to the atomic PWM API
regulator: pwm: Properly initialize the ->state field
regulator: pwm: Retrieve correct voltage
regulator: pwm: Support extra continuous mode cases
regulator: pwm: Document pwm-dutycycle-unit and pwm-dutycycle-range
.../bindings/regulator/pwm-regulator.txt | 19 +++
drivers/pwm/pwm-rockchip.c | 178 +++++++++++++++------
drivers/pwm/pwm-sti.c | 52 +++++-
drivers/regulator/pwm-regulator.c | 162 ++++++++++++++-----
include/linux/pwm.h | 81 ++++++++++
5 files changed, 401 insertions(+), 91 deletions(-)
--
2.7.4
From: Boris Brezillon <hidden> Date: 2016-06-08 16:35:04
The current implementation always round down the duty and period
values, while it would be better to round them to the closest integer.
These changes are needed in preparation of atomic update support to
prevent a period/duty cycle drift when executing several time the
'pwm_get_state() / modify / pwm_apply_state()' sequence.
Say you have an expected period of 3.333 us and a clk rate of
112.666667 MHz -- the clock frequency doesn't divide evenly,
so the period (stashed in nanoseconds) shrinks when we convert to
the register value and back, as follows:
pwm_apply_state(): register = period * 112666667 / 1000000000;
pwm_get_state(): period = register * 1000000000 / 112666667;
or in other words:
period = period * 112666667 / 1000000000 * 1000000000 / 112666667;
which yields a sequence like:
3333 -> 3328
3328 -> 3319
3319 -> 3310
3310 -> 3301
3301 -> 3292
3292 -> ... (etc) ...
With this patch, we'd see instead:
period = div_round_closest(period * 112666667, 1000000000) *
1000000000 / 112666667;
which yields a stable sequence:
3333 -> 3337
3337 -> 3337
3337 -> ... (etc) ...
Signed-off-by: Boris Brezillon <redacted>
Reviewed-by: Brian Norris <briannorris@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>
---
drivers/pwm/pwm-rockchip.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
From: Boris Brezillon <hidden> Date: 2016-06-08 16:35:07
The ->state field is currently initialized to 0, thus referencing the
voltage selector at index 0, which might not reflect the current voltage
value.
If possible, retrieve the current voltage selector from the PWM state, else
return -EINVAL.
Signed-off-by: Boris Brezillon <redacted>
Tested-by: Brian Norris <briannorris@chromium.org>
---
Heiko, Mark,
I know you already added your Tested-by/Acked-by tags on this patch
but this version has slightly change and is now making use of the
pwm_get_relative_duty_cycle() helper instead of manually converting
the absolute duty_cycle value into a relative one.
---
drivers/regulator/pwm-regulator.c | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
From: Boris Brezillon <hidden> Date: 2016-06-08 16:35:10
The pwm_prepare_new_state() helper prepares a new state object
containing the current PWM state except for the polarity and period
fields which are set to the reference values.
This is particurly useful for PWM users who want to apply a new
duty-cycle expressed relatively to the reference period without
changing the enable state.
The PWM framework expect PWM users to configure the duty cycle in
nanosecond, but most users just want to express this duty cycle
relatively to the period value (i.e. duty_cycle = 33% of the period).
Add pwm_{get,set}_relative_duty_cycle() helpers to ease this kind of
conversion.
Signed-off-by: Boris Brezillon <redacted>
---
include/linux/pwm.h | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 81 insertions(+)
@@ -148,6 +148,87 @@ static inline void pwm_get_args(const struct pwm_device *pwm,}/**+*pwm_prepare_new_state()-prepareanewstatetobeappliedwith+*pwm_apply_state()+*@pwm:PWMdevice+*@state:statetofillwiththepreparedPWMstate+*+*Thisfunctionspreparesastatethatcanlaterbetweakedandapplied+*tothePWMdevicewithpwm_apply_state().Thisisaconvenientfunction+*thatfirstretrievesthecurrentPWMstateandthereplacestheperiod+*andpolarityfieldswiththereferencevaluesdefinedinpwm->args.+*Oncethenewstateiscreatedyoucanadjustthe->enableand->duty_cycle+*accordingtoyourneedbeforecallingpwm_apply_state().+*/+staticinlinevoidpwm_prepare_new_state(conststructpwm_device*pwm,+structpwm_state*state)+{+structpwm_argsargs;++/* First get the current state. */+pwm_get_state(pwm,state);++/* Then fill it with the reference config */+pwm_get_args(pwm,&args);++state->period=args.period;+state->polarity=args.polarity;+state->duty_cycle=0;+}++/**+*pwm_get_relative_duty_cycle()-Getarelativeduty_cyclevalue+*@state:thePWMstatetoextractperiodandduty_cyclefrom+*@scale:thescaleyouwanttouseforyourelativeduty_cyclevalue+*+*Thisfunctionsconvertstheabsoluteduty_cyclestoredin@state+*(expressedinnanosecond)intoarelativevalue.+*Forexampleifyouwanttogettheduty_cycleexpressedinnanosecond,+*call:+*+*pwm_get_state(pwm,&state);+*duty=pwm_get_relative_duty_cycle(&state,100);+*/+staticinlineunsignedint+pwm_get_relative_duty_cycle(conststructpwm_state*state,unsignedintscale)+{+if(!state->period)+return0;++returnDIV_ROUND_CLOSEST_ULL((u64)state->duty_cycle*scale,+state->period);+}++/**+*pwm_set_relative_duty_cycle()-Setarelativeduty_cyclevalue+*@state:thePWMstatetofill+*@val:therelativeduty_cyclevalue+*@scale:thescaleyouuseforyourelativeduty_cyclevalue+*+*Thisfunctionsconvertsarelativeduty_cyclestoredintoanabsolute+*one(expressedinnanoseconds),andputtheresultinstate->duty_cycle.+*Forexampleifyouwanttochangeconfigurea50%duty_cycle,call:+*+*pwm_prepare_new_state(pwm,&state);+*pwm_set_relative_duty_cycle(&state,50,100);+*pwm_apply_state(pwm,&state);+*/+staticinlinevoid+pwm_set_relative_duty_cycle(structpwm_state*state,unsignedintval,+unsignedintscale)+{+if(!scale)+return;++/* Make sure we don't have duty_cycle > period */+if(val>scale)+val=scale;++state->duty_cycle=DIV_ROUND_CLOSEST_ULL((u64)val*state->period,+scale);+}++/***structpwm_ops-PWMcontrolleroperations*@request:optionalhookforrequestingaPWM*@free:optionalhookforfreeingaPWM
On Wed, Jun 08, 2016 at 06:34:36PM +0200, Boris Brezillon wrote:
The pwm_prepare_new_state() helper prepares a new state object
containing the current PWM state except for the polarity and period
fields which are set to the reference values.
This is particurly useful for PWM users who want to apply a new
duty-cycle expressed relatively to the reference period without
changing the enable state.
The PWM framework expect PWM users to configure the duty cycle in
nanosecond, but most users just want to express this duty cycle
relatively to the period value (i.e. duty_cycle = 33% of the period).
Add pwm_{get,set}_relative_duty_cycle() helpers to ease this kind of
conversion.
This looks pretty useful and good, though I think these could be two
separate patches. A couple of suggestions on wording and such below.
+ * @pwm: PWM device
+ * @state: state to fill with the prepared PWM state
+ *
+ * This functions prepares a state that can later be tweaked and applied
+ * to the PWM device with pwm_apply_state(). This is a convenient function
+ * that first retrieves the current PWM state and the replaces the period
+ * and polarity fields with the reference values defined in pwm->args.
+ * Once the new state is created you can adjust the ->enable and ->duty_cycle
"created" has the connotation of allocating. Perhaps: "Once the function
returns, you can adjust the ->enabled and ->duty_cycle fields according
to your needs before calling pwm_apply_state()."?
Also note that the field is called ->enabled.
+ * according to your need before calling pwm_apply_state().
Maybe mention that the ->duty_cycle field is explicitly zeroed. Then
again, do we really need it? If users are going to overwrite it anyway,
do we even need to bother? I suppose it makes some sense because the
current duty cycle is stale when the ->period gets set to the value from
args. I think the documentation should mention this in some way.
Perhaps choose a shorter name, such as: pwm_init_state()?
+{
+ struct pwm_args args;
+
+ /* First get the current state. */
+ pwm_get_state(pwm, state);
+
+ /* Then fill it with the reference config */
+ pwm_get_args(pwm, &args);
+
+ state->period = args.period;
+ state->polarity = args.polarity;
+ state->duty_cycle = 0;
+}
+
+/**
+ * pwm_get_relative_duty_cycle() - Get a relative duty_cycle value
+ * @state: the PWM state to extract period and duty_cycle from
+ * @scale: the scale you want to use for you relative duty_cycle value
Other kerneldoc comments in this file don't prefix the description with
"the".
Also, perhaps the following descriptions would be slightly less
confusing:
@state: PWM state to extract the duty cycle from
We don't extract (i.e. return) period from the state, so it's a little
confusing to mention it here.
@scale: scale in which to return the relative duty cycle
This is slightly more explicit in that it says what the returned duty
cycle will be in. Perhaps as an alternative:
@scale: target scale of the relative duty cycle
+ *
+ * This functions converts the absolute duty_cycle stored in @state
+ * (expressed in nanosecond) into a relative value.
Perhaps: "... into a value relative to the period."?
+ * For example if you want to get the duty_cycle expressed in nanosecond,
The example below would give you the duty cycle in percent, not
nanoseconds, right?
+ * call:
+ *
+ * pwm_get_state(pwm, &state);
+ * duty = pwm_get_relative_duty_cycle(&state, 100);
+ */
+static inline unsigned int
+pwm_get_relative_duty_cycle(const struct pwm_state *state, unsigned int scale)
+{
+ if (!state->period)
+ return 0;
+
+ return DIV_ROUND_CLOSEST_ULL((u64)state->duty_cycle * scale,
+ state->period);
+}
+
+/**
+ * pwm_set_relative_duty_cycle() - Set a relative duty_cycle value
+ * @state: the PWM state to fill
+ * @val: the relative duty_cycle value
+ * @scale: the scale you use for you relative duty_cycle value
"scale in which @duty_cycle is expressed"?
+ *
+ * This functions converts a relative duty_cycle stored into an absolute
+ * one (expressed in nanoseconds), and put the result in state->duty_cycle.
+ * For example if you want to change configure a 50% duty_cycle, call:
+ *
+ * pwm_prepare_new_state(pwm, &state);
+ * pwm_set_relative_duty_cycle(&state, 50, 100);
+ * pwm_apply_state(pwm, &state);
+ */
+static inline void
+pwm_set_relative_duty_cycle(struct pwm_state *state, unsigned int val,
Any reason why we can't call "val" "duty_cycle"? That's what it is,
after all.
+ unsigned int scale)
+{
+ if (!scale)
+ return;
+
+ /* Make sure we don't have duty_cycle > period */
+ if (val > scale)
+ val = scale;
Can we return -EINVAL for both of the above checks? I don't like
silently clamping the duty cycle that the user passed in.
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160610/a1491895/attachment.sig>
From: Boris Brezillon <hidden> Date: 2016-06-10 16:29:59
Hi Thierry,
On Fri, 10 Jun 2016 17:21:09 +0200
Thierry Reding [off-list ref] wrote:
On Wed, Jun 08, 2016 at 06:34:36PM +0200, Boris Brezillon wrote:
quoted
The pwm_prepare_new_state() helper prepares a new state object
containing the current PWM state except for the polarity and period
fields which are set to the reference values.
This is particurly useful for PWM users who want to apply a new
duty-cycle expressed relatively to the reference period without
changing the enable state.
The PWM framework expect PWM users to configure the duty cycle in
nanosecond, but most users just want to express this duty cycle
relatively to the period value (i.e. duty_cycle = 33% of the period).
Add pwm_{get,set}_relative_duty_cycle() helpers to ease this kind of
conversion.
This looks pretty useful and good, though I think these could be two
separate patches. A couple of suggestions on wording and such below.
+ * @pwm: PWM device
+ * @state: state to fill with the prepared PWM state
+ *
+ * This functions prepares a state that can later be tweaked and applied
+ * to the PWM device with pwm_apply_state(). This is a convenient function
+ * that first retrieves the current PWM state and the replaces the period
+ * and polarity fields with the reference values defined in pwm->args.
+ * Once the new state is created you can adjust the ->enable and ->duty_cycle
"created" has the connotation of allocating. Perhaps: "Once the function
returns, you can adjust the ->enabled and ->duty_cycle fields according
to your needs before calling pwm_apply_state()."?
Okay.
Also note that the field is called ->enabled.
quoted
+ * according to your need before calling pwm_apply_state().
Maybe mention that the ->duty_cycle field is explicitly zeroed. Then
again, do we really need it? If users are going to overwrite it anyway,
do we even need to bother? I suppose it makes some sense because the
current duty cycle is stale when the ->period gets set to the value from
args. I think the documentation should mention this in some way.
Yes, if we keep the current duty_cycle it can exceed the period value.
I'm fine dropping the ->duty_cycle = 0 assignment and documenting the
behavior.
Perhaps choose a shorter name, such as: pwm_init_state()?
Sure.
quoted
+{
+ struct pwm_args args;
+
+ /* First get the current state. */
+ pwm_get_state(pwm, state);
+
+ /* Then fill it with the reference config */
+ pwm_get_args(pwm, &args);
+
+ state->period = args.period;
+ state->polarity = args.polarity;
+ state->duty_cycle = 0;
+}
+
+/**
+ * pwm_get_relative_duty_cycle() - Get a relative duty_cycle value
+ * @state: the PWM state to extract period and duty_cycle from
+ * @scale: the scale you want to use for you relative duty_cycle value
Other kerneldoc comments in this file don't prefix the description with
"the".
Also, perhaps the following descriptions would be slightly less
confusing:
@state: PWM state to extract the duty cycle from
Agreed.
We don't extract (i.e. return) period from the state, so it's a little
confusing to mention it here.
@scale: scale in which to return the relative duty cycle
This is slightly more explicit in that it says what the returned duty
cycle will be in. Perhaps as an alternative:
@scale: target scale of the relative duty cycle
I'll go for this one.
quoted
+ *
+ * This functions converts the absolute duty_cycle stored in @state
+ * (expressed in nanosecond) into a relative value.
Perhaps: "... into a value relative to the period."?
Okay.
quoted
+ * For example if you want to get the duty_cycle expressed in nanosecond,
The example below would give you the duty cycle in percent, not
nanoseconds, right?
Absolutely. I'll fix that.
quoted
+ * call:
+ *
+ * pwm_get_state(pwm, &state);
+ * duty = pwm_get_relative_duty_cycle(&state, 100);
+ */
+static inline unsigned int
+pwm_get_relative_duty_cycle(const struct pwm_state *state, unsigned int scale)
+{
+ if (!state->period)
+ return 0;
+
+ return DIV_ROUND_CLOSEST_ULL((u64)state->duty_cycle * scale,
+ state->period);
+}
+
+/**
+ * pwm_set_relative_duty_cycle() - Set a relative duty_cycle value
+ * @state: the PWM state to fill
+ * @val: the relative duty_cycle value
+ * @scale: the scale you use for you relative duty_cycle value
"scale in which @duty_cycle is expressed"?
Yep.
quoted
+ *
+ * This functions converts a relative duty_cycle stored into an absolute
+ * one (expressed in nanoseconds), and put the result in state->duty_cycle.
+ * For example if you want to change configure a 50% duty_cycle, call:
+ *
+ * pwm_prepare_new_state(pwm, &state);
+ * pwm_set_relative_duty_cycle(&state, 50, 100);
+ * pwm_apply_state(pwm, &state);
+ */
+static inline void
+pwm_set_relative_duty_cycle(struct pwm_state *state, unsigned int val,
Any reason why we can't call "val" "duty_cycle"? That's what it is,
after all.
Nope, I'll switch to duty_cycle.
quoted
+ unsigned int scale)
+{
+ if (!scale)
+ return;
+
+ /* Make sure we don't have duty_cycle > period */
+ if (val > scale)
+ val = scale;
Can we return -EINVAL for both of the above checks? I don't like
silently clamping the duty cycle that the user passed in.
I'm fine returning -EINVAL in this case.
Thanks for rewording some of my sentences and reviewing the patch.
Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
On Fri, Jun 10, 2016 at 06:29:42PM +0200, Boris Brezillon wrote:
On Fri, 10 Jun 2016 17:21:09 +0200, Thierry Reding [off-list ref] wrote:
quoted
On Wed, Jun 08, 2016 at 06:34:36PM +0200, Boris Brezillon wrote:
[...]
quoted
quoted
+ * according to your need before calling pwm_apply_state().
Maybe mention that the ->duty_cycle field is explicitly zeroed. Then
again, do we really need it? If users are going to overwrite it anyway,
do we even need to bother? I suppose it makes some sense because the
current duty cycle is stale when the ->period gets set to the value from
args. I think the documentation should mention this in some way.
Yes, if we keep the current duty_cycle it can exceed the period value.
I'm fine dropping the ->duty_cycle = 0 assignment and documenting the
behavior.
Actually what I was trying to suggest is that we keep the code as-is but
document the behaviour (and rationale behind it).
I think it's fine to zero out the value precisely because it could
become invalid after the function (and there's no other reasonable value
to set it to). Just wanted to make sure it's all properly documented.
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160610/8be32721/attachment.sig>
From: Boris Brezillon <hidden> Date: 2016-06-08 16:35:13
Use the atomic API wherever appropriate and get rid of pwm_apply_args()
call (the reference period and polarity are now explicitly set when
calling pwm_apply_state()).
We also make use of the pwm_set_relative_duty_cycle() helper to ease
relative to absolute duty_cycle conversion.
Note that changes introduced by commit fd786fb0276a ("regulator: pwm:
Try to avoid voltage error in duty cycle calculation") are no longer
needed because pwm_set_relative_duty_cycle() takes care of all rounding
approximation for us.
Signed-off-by: Boris Brezillon <redacted>
Reviewed-by: Brian Norris <briannorris@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>
Acked-by: Laxman Dewangan <ldewangan@nvidia.com>
---
drivers/regulator/pwm-regulator.c | 38 ++++++++++----------------------------
1 file changed, 10 insertions(+), 28 deletions(-)
@@ -59,16 +59,14 @@ static int pwm_regulator_set_voltage_sel(struct regulator_dev *rdev,unsignedselector){structpwm_regulator_data*drvdata=rdev_get_drvdata(rdev);-structpwm_argspargs;-intdutycycle;+structpwm_statepstate;intret;-pwm_get_args(drvdata->pwm,&pargs);+pwm_prepare_new_state(drvdata->pwm,&pstate);+pwm_set_relative_duty_cycle(&pstate,+drvdata->duty_cycle_table[selector].dutycycle,100);-dutycycle=(pargs.period*-drvdata->duty_cycle_table[selector].dutycycle)/100;--ret=pwm_config(drvdata->pwm,dutycycle,pargs.period);+ret=pwm_apply_state(drvdata->pwm,&pstate);if(ret){dev_err(&rdev->dev,"Failed to configure PWM: %d\n",ret);returnret;
@@ -126,34 +124,18 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev,{structpwm_regulator_data*drvdata=rdev_get_drvdata(rdev);unsignedintramp_delay=rdev->constraints->ramp_delay;-structpwm_argspargs;unsignedintreq_diff=min_uV-rdev->constraints->min_uV;+structpwm_statepstate;unsignedintdiff;-unsignedintduty_pulse;-u64req_period;-u32rem;intret;-pwm_get_args(drvdata->pwm,&pargs);+pwm_prepare_new_state(drvdata->pwm,&pstate);diff=rdev->constraints->max_uV-rdev->constraints->min_uV;-/* First try to find out if we get the iduty cycle time which is-*factorofPWMperiodtime.If(request_diff_to_min*pwm_period)-*isperfectdividedbyvoltage_range_diffthenitispossibleto-*getdutycycletimewhichisfactorofPWMperiod.Thiswillhelp-*togetoutputvoltagenearertorequestedvalueasthereisno-*calculationloss.-*/-req_period=req_diff*pargs.period;-div_u64_rem(req_period,diff,&rem);-if(!rem){-do_div(req_period,diff);-duty_pulse=(unsignedint)req_period;-}else{-duty_pulse=(pargs.period/100)*((req_diff*100)/diff);-}+/* We pass diff as the scale to get a uV precision. */+pwm_set_relative_duty_cycle(&pstate,req_diff,diff);-ret=pwm_config(drvdata->pwm,duty_pulse,pargs.period);+ret=pwm_apply_state(drvdata->pwm,&pstate);if(ret){dev_err(&rdev->dev,"Failed to configure PWM: %d\n",ret);returnret;
@@ -34,6 +34,18 @@ Only required for Voltage Table Mode: First cell is voltage in microvolts (uV) Second cell is duty-cycle in percent (%)+Optional properties for Continuous mode:+- pwm-dutycycle-unit: Integer value encoding the duty cycle unit. If not+ defined, <100> is assumed, meaning that+ pwm-dutycycle-range contains values expressed in+ percent.++- pwm-dutycycle-range: Should contain 2 entries. The first entry is encoding+ the dutycycle for regulator-min-microvolt and the+ second one the dutycycle for regulator-max-microvolt.+ Duty cycle values are expressed in pwm-dutycycle-unit.+ If not defined, <0 100> is assumed.+ NB: To be clear, if voltage-table is provided, then the device will be used in Voltage Table Mode. If no voltage-table is provided, then the device will be used in Continuous Voltage Mode.
@@ -48,6 +60,13 @@ Continuous Voltage Example: regulator-min-microvolt = <1016000>; regulator-max-microvolt = <1114000>; regulator-name = "vdd_logic";+ /* unit == per-mille */+ pwm-dutycycle-unit = <1000>;+ /*+ * Inverted PWM logic, and the duty cycle range is limited+ * to 30%-70%.+ */+ pwm-dutycycle-range <700 300>; /* */ }; Voltage Table Example:
From: Boris Brezillon <hidden> Date: 2016-06-08 16:36:41
The continuous mode allows one to declare a PWM regulator without having
to declare the voltage <-> dutycycle association table. It works fine as
long as your voltage(dutycycle) function is linear, but also has the
following constraints:
- dutycycle for min_uV = 0%
- dutycycle for max_uV = 100%
- dutycycle for min_uV < dutycycle for max_uV
While the linearity constraint is acceptable for now, we sometimes need to
restrict of the PWM range (to limit the maximum/minimum voltage for
example) or have a min_uV_dutycycle > max_uV_dutycycle (this could be
tweaked with PWM polarity, but not all PWMs support inverted polarity).
Add the pwm-dutycycle-range and pwm-dutycycle-unit DT properties to define
such constraints. If those properties are not defined, the PWM regulator
use the default pwm-dutycycle-range = <0 100> and
pwm-dutycycle-unit = <100> values (existing behavior).
Signed-off-by: Boris Brezillon <redacted>
Reviewed-by: Brian Norris <briannorris@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>
---
drivers/regulator/pwm-regulator.c | 92 +++++++++++++++++++++++++++++++++++----
1 file changed, 83 insertions(+), 9 deletions(-)
@@ -28,6 +34,9 @@ struct pwm_regulator_data {/* Voltage table */structpwm_voltages*duty_cycle_table;+/* Continuous mode info */+structpwm_continuous_reg_datacontinuous;+/* regulator descriptor */structregulator_descdesc;
@@ -132,31 +141,79 @@ static int pwm_regulator_is_enabled(struct regulator_dev *dev)staticintpwm_regulator_get_voltage(structregulator_dev*rdev){structpwm_regulator_data*drvdata=rdev_get_drvdata(rdev);+unsignedintmin_uV_duty=drvdata->continuous.min_uV_dutycycle;+unsignedintmax_uV_duty=drvdata->continuous.max_uV_dutycycle;+unsignedintduty_unit=drvdata->continuous.dutycycle_unit;intmin_uV=rdev->constraints->min_uV;-intdiff=rdev->constraints->max_uV-min_uV;+intmax_uV=rdev->constraints->max_uV;+intdiff_uV=max_uV-min_uV;structpwm_statepstate;+unsignedintdiff_duty;+unsignedintvoltage;pwm_get_state(drvdata->pwm,&pstate);-returnmin_uV+pwm_get_relative_duty_cycle(&pstate,diff);+voltage=pwm_get_relative_duty_cycle(&pstate,duty_unit);++/*+*Thedutycycleforthemin_uVvoltagemightbegreaterthanthe+*oneforthemax_uV.Thisishappeningwhentheuserneedsan+*inversedpolarity,butthePWMdevicedoesnotsupportinversing+*itinhardware.+*/+if(max_uV_duty<min_uV_duty){+voltage=min_uV_duty-voltage;+diff_duty=min_uV_duty-max_uV_duty;+}else{+voltage=voltage-min_uV_duty;+diff_duty=max_uV_duty-min_uV_duty;+}++voltage=DIV_ROUND_CLOSEST_ULL((u64)voltage*diff_uV,diff_duty);++returnvoltage+min_uV;}staticintpwm_regulator_set_voltage(structregulator_dev*rdev,-intmin_uV,intmax_uV,-unsigned*selector)+intreq_min_uV,intreq_max_uV,+unsignedint*selector){structpwm_regulator_data*drvdata=rdev_get_drvdata(rdev);+unsignedintmin_uV_duty=drvdata->continuous.min_uV_dutycycle;+unsignedintmax_uV_duty=drvdata->continuous.max_uV_dutycycle;+unsignedintduty_unit=drvdata->continuous.dutycycle_unit;unsignedintramp_delay=rdev->constraints->ramp_delay;-unsignedintreq_diff=min_uV-rdev->constraints->min_uV;+intmin_uV=rdev->constraints->min_uV;+intmax_uV=rdev->constraints->max_uV;+intdiff_uV=max_uV-min_uV;structpwm_statepstate;-unsignedintdiff;+unsignedintdiff_duty;+unsignedintdutycycle;intret;pwm_prepare_new_state(drvdata->pwm,&pstate);-diff=rdev->constraints->max_uV-rdev->constraints->min_uV;-/* We pass diff as the scale to get a uV precision. */-pwm_set_relative_duty_cycle(&pstate,req_diff,diff);+/*+*Thedutycycleforthemin_uVvoltagemightbegreaterthanthe+*oneforthemax_uV.Thisishappeningwhentheuserneedsan+*inversedpolarity,butthePWMdevicedoesnotsupportinversing+*itinhardware.+*/+if(max_uV_duty<min_uV_duty)+diff_duty=min_uV_duty-max_uV_duty;+else+diff_duty=max_uV_duty-min_uV_duty;++dutycycle=DIV_ROUND_CLOSEST_ULL((u64)(req_min_uV-min_uV)*+diff_duty,+diff_uV);++if(max_uV_duty<min_uV_duty)+dutycycle=min_uV_duty-dutycycle;+else+dutycycle=min_uV_duty+dutycycle;++pwm_set_relative_duty_cycle(&pstate,dutycycle,duty_unit);ret=pwm_apply_state(drvdata->pwm,&pstate);if(ret){
@@ -237,11 +294,28 @@ static int pwm_regulator_init_table(struct platform_device *pdev,staticintpwm_regulator_init_continuous(structplatform_device*pdev,structpwm_regulator_data*drvdata){+u32dutycycle_range[2]={0,100};+u32dutycycle_unit=100;+memcpy(&drvdata->ops,&pwm_regulator_voltage_continuous_ops,sizeof(drvdata->ops));drvdata->desc.ops=&drvdata->ops;drvdata->desc.continuous_voltage_range=true;+of_property_read_u32_array(pdev->dev.of_node,+"pwm-dutycycle-range",+dutycycle_range,2);+of_property_read_u32(pdev->dev.of_node,"pwm-dutycycle-unit",+&dutycycle_unit);++if(dutycycle_range[0]>dutycycle_unit||+dutycycle_range[1]>dutycycle_unit)+return-EINVAL;++drvdata->continuous.dutycycle_unit=dutycycle_unit;+drvdata->continuous.min_uV_dutycycle=dutycycle_range[0];+drvdata->continuous.max_uV_dutycycle=dutycycle_range[1];+return0;}
From: Boris Brezillon <hidden> Date: 2016-06-08 16:37:00
The continuous PWM voltage regulator is caching the voltage value in
the ->volt_uV field. While most of the time this value should reflect the
real voltage, sometime it can be sightly different if the PWM device
rounded the set_duty_cycle request.
Moreover, this value is not valid until someone has modified the regulator
output.
Remove the ->volt_uV field and always rely on the PWM state to calculate
the regulator output.
Signed-off-by: Boris Brezillon <redacted>
Reviewed-by: Brian Norris <briannorris@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>
---
Mark,
I know you already added your Tested-by/Acked-by tags on this patch
but this version has slightly change and is now making use of the
pwm_get_relative_duty_cycle() helper instead of manually converting
the absolute duty_cycle value into a relative one.
---
drivers/regulator/pwm-regulator.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
@@ -35,9 +35,6 @@ struct pwm_regulator_data {structregulator_opsops;intstate;--/* Continuous voltage */-intvolt_uV;};structpwm_voltages{
@@ -135,8 +132,13 @@ static int pwm_regulator_is_enabled(struct regulator_dev *dev)staticintpwm_regulator_get_voltage(structregulator_dev*rdev){structpwm_regulator_data*drvdata=rdev_get_drvdata(rdev);+intmin_uV=rdev->constraints->min_uV;+intdiff=rdev->constraints->max_uV-min_uV;+structpwm_statepstate;-returndrvdata->volt_uV;+pwm_get_state(drvdata->pwm,&pstate);++returnmin_uV+pwm_get_relative_duty_cycle(&pstate,diff);}staticintpwm_regulator_set_voltage(structregulator_dev*rdev,
@@ -162,8 +164,6 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev,returnret;}-drvdata->volt_uV=min_uV;-/* Delay required by PWM regulator to settle to the new voltage */usleep_range(ramp_delay,ramp_delay+1000);
From: Boris Brezillon <hidden> Date: 2016-06-08 16:37:31
The PWM attached to a PWM regulator device might have been previously
configured by the bootloader.
Make sure the bootloader and linux config are in sync, and adjust the PWM
config if that's not the case.
Signed-off-by: Boris Brezillon <redacted>
Acked-by: Mark Brown <broonie@kernel.org>
Acked-by: Brian Norris <briannorris@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>
---
drivers/regulator/pwm-regulator.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
From: Boris Brezillon <hidden> Date: 2016-06-08 16:37:35
The current logic will disable the PWM clk even if a PWM was left
enabled by the bootloader (because it's controlling a critical device
like a regulator for example).
Keep the PWM clk enabled if at least one PWM is enabled to avoid any
glitches.
Signed-off-by: Boris Brezillon <redacted>
---
drivers/pwm/pwm-sti.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
From: Boris Brezillon <hidden> Date: 2016-06-08 16:37:37
Implement the ->apply() function to add support for atomic update.
Signed-off-by: Boris Brezillon <redacted>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Reviewed-by: Brian Norris <briannorris@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>
---
drivers/pwm/pwm-rockchip.c | 84 ++++++++++++++++++++++++----------------------
1 file changed, 43 insertions(+), 41 deletions(-)
From: Boris Brezillon <hidden> Date: 2016-06-08 16:37:39
Implement ->get_state() to provide support for initial state retrieval.
Signed-off-by: Boris Brezillon <redacted>
---
drivers/pwm/pwm-sti.c | 38 ++++++++++++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)
@@ -238,6 +238,43 @@ static void sti_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)mutex_unlock(&pc->sti_pwm_lock);}+staticvoidsti_pwm_get_state(structpwm_chip*chip,+structpwm_device*pwm,+structpwm_state*state)+{+structsti_pwm_chip*pc=to_sti_pwmchip(chip);+unsignedintregval,prescaler;+intret;++/* The clock has to be enabled to access PWM registers */+ret=clk_enable(pc->clk);+if(ret){+dev_err(chip->dev,"Failed to enable PWM clk");+return;+}++regmap_field_read(pc->prescale_high,®val);+prescaler=regval<<4;+regmap_field_read(pc->prescale_low,®val);+prescaler|=regval;+state->period=DIV_ROUND_CLOSEST_ULL((u64)(prescaler+1)*+NSEC_PER_SEC*+(pc->cdata->max_pwm_cnt+1),+pc->clk_rate);++regmap_read(pc->regmap,STI_DS_REG(pwm->hwpwm),®val);+state->duty_cycle=DIV_ROUND_CLOSEST_ULL((u64)(regval+1)*+state->period,+pc->cdata->max_pwm_cnt+1);++regmap_field_read(pc->pwm_en,®val);+state->enabled=regval;++state->polarity=PWM_POLARITY_NORMAL;++clk_disable(pc->clk);+}+staticvoidsti_pwm_free(structpwm_chip*chip,structpwm_device*pwm){structsti_pwm_chip*pc=to_sti_pwmchip(chip);
From: Boris Brezillon <hidden> Date: 2016-06-08 16:38:46
The current logic will disable the PWM clk even if the PWM was left
enabled by the bootloader (because it's controlling a critical device
like a regulator for example).
Keep the PWM clk enabled if the PWM is enabled to avoid any glitches.
Signed-off-by: Boris Brezillon <redacted>
Reviewed-by: Brian Norris <briannorris@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>
---
drivers/pwm/pwm-rockchip.c | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
@@ -319,7 +319,7 @@ static int rockchip_pwm_probe(struct platform_device *pdev)if(IS_ERR(pc->clk))returnPTR_ERR(pc->clk);-ret=clk_prepare(pc->clk);+ret=clk_prepare_enable(pc->clk);if(ret)returnret;
@@ -342,6 +342,10 @@ static int rockchip_pwm_probe(struct platform_device *pdev)dev_err(&pdev->dev,"pwmchip_add() failed: %d\n",ret);}+/* Keep the PWM clk enabled if the PWM appears to be up and running. */+if(!pwm_is_enabled(pc->chip.pwms))+clk_disable(pc->clk);+returnret;}
@@ -349,6 +353,20 @@ static int rockchip_pwm_remove(struct platform_device *pdev){structrockchip_pwm_chip*pc=platform_get_drvdata(pdev);+/*+*DisablethePWMclkbeforeunpreparingitifthePWMdeviceisstill+*running.ThisshouldonlyhappenwhenthelastPWMuserleftit+*enabled,orwhennobodyrequestedaPWMthatwaspreviouslyenabled+*bythebootloader.+*+*FIXME:MaybethecoreshoulddisableallPWMdevicesin+*pwmchip_remove().Inthiscasewe'donlyhavetocall+*clk_unprepare()afterpwmchip_remove().+*+*/+if(pwm_is_enabled(pc->chip.pwms))+clk_disable(pc->clk);+clk_unprepare(pc->clk);returnpwmchip_remove(&pc->chip);
From: Boris Brezillon <hidden> Date: 2016-06-08 16:39:16
Implement the ->get_state() function to expose initial state.
Signed-off-by: Boris Brezillon <redacted>
Reviewed-by: Brian Norris <briannorris@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>
---
drivers/pwm/pwm-rockchip.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 67 insertions(+)
Hi Boris,
Am Mittwoch, 8. Juni 2016, 18:34:35 schrieb Boris Brezillon:
This patch series series aims at adding two important features to the
pwm-regulator driver.
The first one is the support for 'smooth handover' between the
bootloader and the kernel. This is mainly solving problems we have when
the PWM is controlling a critical regulator (like the one powering the
DDR chip). Currently, when the PWM regulator acquire the PWM device it
assumes it was off and it's safe to change the configuration before
enabling it, which can generate glitches on the PWM signal which in turn
generated glitches on the output voltage.
To solve that we've introduced support for hardware readout to the
PWM framework, so that the PWM regulator driver can adjust the PWM
a probe time and avoid glitches.
Atomic update is also helping in this regard.
[...]
The second feature we add to the driver is the capability of using
a sub duty_cycle range in continuous mode. By default the regulator
is assuming that min_uV is achieved with a 0% dutycyle and max_uV
with a 100% dutycycle, but this is not necessarily true.
Moreover, in some cases (when the PWM device does not support
polarity inversion), we might have min_uV at 100% and max_uV at 0%.
Hence the addition of new properties to the existing DT bindings.
The feature is added in patch 12 and 13.
I've tested this series on a rk3288-veyron chromebook and both the backlight-
pwm as well as the pwm-regulator still work as expected, so for everything
except the STI parts:
Tested-by: Heiko Stuebner <heiko@sntech.de>