[U-Boot] [PATCH] power: regulator: pwm: support pwm polarity setting
The latest kernel PWM drivers enable the polarity settings. When system run from U-Boot to kerenl, if there are differences in polarity set or duty cycle, the PMW will re-init: close -> set polarity and duty cycle -> enable the PWM. The power supply controled by pwm regulator may have voltage shaking, which lead to the system not stable.
Signed-off-by: Kever Yang kever.yang@rock-chips.com ---
drivers/power/regulator/pwm_regulator.c | 12 ++++++++++-- drivers/pwm/pwm-uclass.c | 10 ++++++++++ drivers/pwm/rk_pwm.c | 17 ++++++++++++++++- include/pwm.h | 9 +++++++++ 4 files changed, 45 insertions(+), 3 deletions(-)
diff --git a/drivers/power/regulator/pwm_regulator.c b/drivers/power/regulator/pwm_regulator.c index 4875238..f8a6712 100644 --- a/drivers/power/regulator/pwm_regulator.c +++ b/drivers/power/regulator/pwm_regulator.c @@ -24,6 +24,8 @@ struct pwm_regulator_info { int pwm_id; /* the period of one PWM cycle */ int period_ns; + /* the polarity of one PWM */ + int polarity; struct udevice *pwm; /* initialize voltage of regulator */ unsigned int init_voltage; @@ -49,7 +51,7 @@ static int pwm_voltage_to_duty_cycle_percentage(struct udevice *dev, int req_uV) int max_uV = priv->max_voltage; int diff = max_uV - min_uV;
- return 100 - (((req_uV * 100) - (min_uV * 100)) / diff); + return ((req_uV * 100) - (min_uV * 100)) / diff; }
static int pwm_regulator_get_voltage(struct udevice *dev) @@ -67,6 +69,12 @@ static int pwm_regulator_set_voltage(struct udevice *dev, int uvolt)
duty_cycle = pwm_voltage_to_duty_cycle_percentage(dev, uvolt);
+ ret = pwm_set_init(priv->pwm, priv->pwm_id, priv->polarity); + if (ret) { + dev_err(dev, "Failed to init PWM\n"); + return ret; + } + ret = pwm_set_config(priv->pwm, priv->pwm_id, (priv->period_ns / 100) * duty_cycle, priv->period_ns); if (ret) { @@ -97,9 +105,9 @@ static int pwm_regulator_ofdata_to_platdata(struct udevice *dev) debug("%s: Cannot get PWM phandle: ret=%d\n", __func__, ret); return ret; } - /* TODO: pwm_id here from device tree if needed */
priv->period_ns = args.args[1]; + priv->polarity = args.args[2];
priv->init_voltage = fdtdec_get_int(blob, node, "regulator-init-microvolt", -1); diff --git a/drivers/pwm/pwm-uclass.c b/drivers/pwm/pwm-uclass.c index c2200af..287a7f3 100644 --- a/drivers/pwm/pwm-uclass.c +++ b/drivers/pwm/pwm-uclass.c @@ -9,6 +9,16 @@ #include <dm.h> #include <pwm.h>
+int pwm_set_init(struct udevice *dev, uint channel, uint polarity) +{ + struct pwm_ops *ops = pwm_get_ops(dev); + + if (!ops->set_init) + return -ENOSYS; + + return ops->set_init(dev, channel, polarity); +} + int pwm_set_config(struct udevice *dev, uint channel, uint period_ns, uint duty_ns) { diff --git a/drivers/pwm/rk_pwm.c b/drivers/pwm/rk_pwm.c index 9254f5b..5ff1e00 100644 --- a/drivers/pwm/rk_pwm.c +++ b/drivers/pwm/rk_pwm.c @@ -21,8 +21,22 @@ DECLARE_GLOBAL_DATA_PTR; struct rk_pwm_priv { struct rk3288_pwm *regs; ulong freq; + uint enable_conf; };
+static int rk_pwm_set_init(struct udevice *dev, uint channel, uint polarity) +{ + struct rk_pwm_priv *priv = dev_get_priv(dev); + + debug("%s: polarity=%u\n", __func__, polarity); + if (polarity) + priv->enable_conf |= PWM_DUTY_NEGATIVE | PWM_INACTIVE_POSTIVE; + else + priv->enable_conf |= PWM_DUTY_POSTIVE | PWM_INACTIVE_NEGATIVE; + + return 0; +} + static int rk_pwm_set_config(struct udevice *dev, uint channel, uint period_ns, uint duty_ns) { @@ -32,7 +46,7 @@ static int rk_pwm_set_config(struct udevice *dev, uint channel, uint period_ns,
debug("%s: period_ns=%u, duty_ns=%u\n", __func__, period_ns, duty_ns); writel(PWM_SEL_SRC_CLK | PWM_OUTPUT_LEFT | PWM_LP_DISABLE | - PWM_CONTINUOUS | PWM_DUTY_POSTIVE | PWM_INACTIVE_POSTIVE | + PWM_CONTINUOUS | priv->enable_conf | RK_PWM_DISABLE, ®s->ctrl);
@@ -83,6 +97,7 @@ static int rk_pwm_probe(struct udevice *dev) }
static const struct pwm_ops rk_pwm_ops = { + .set_init = rk_pwm_set_init, .set_config = rk_pwm_set_config, .set_enable = rk_pwm_set_enable, }; diff --git a/include/pwm.h b/include/pwm.h index 851915e..a66ee1f 100644 --- a/include/pwm.h +++ b/include/pwm.h @@ -14,6 +14,15 @@ /* struct pwm_ops: Operations for the PWM uclass */ struct pwm_ops { /** + * set_init() - Set the PWM invert + * + * @dev: PWM device to update + * @channel: PWM channel to update + * @polarity: PWM invert polarity + * @return 0 if OK, -ve on error + */ + int (*set_init)(struct udevice *dev, uint channel, uint polarity); + /** * set_config() - Set the PWM configuration * * @dev: PWM device to update
Hi Kever,
On 04/07/2017 08:02 PM, Kever Yang wrote:
The latest kernel PWM drivers enable the polarity settings. When system run from U-Boot to kerenl, if there are differences in polarity set or duty cycle, the PMW will re-init: close -> set polarity and duty cycle -> enable the PWM. The power supply controled by pwm regulator may have voltage shaking, which lead to the system not stable.
Signed-off-by: Kever Yang kever.yang@rock-chips.com
drivers/power/regulator/pwm_regulator.c | 12 ++++++++++-- drivers/pwm/pwm-uclass.c | 10 ++++++++++ drivers/pwm/rk_pwm.c | 17 ++++++++++++++++- include/pwm.h | 9 +++++++++ 4 files changed, 45 insertions(+), 3 deletions(-)
diff --git a/drivers/power/regulator/pwm_regulator.c b/drivers/power/regulator/pwm_regulator.c index 4875238..f8a6712 100644 --- a/drivers/power/regulator/pwm_regulator.c +++ b/drivers/power/regulator/pwm_regulator.c @@ -24,6 +24,8 @@ struct pwm_regulator_info { int pwm_id; /* the period of one PWM cycle */ int period_ns;
- /* the polarity of one PWM */
- int polarity; struct udevice *pwm; /* initialize voltage of regulator */ unsigned int init_voltage;
@@ -49,7 +51,7 @@ static int pwm_voltage_to_duty_cycle_percentage(struct udevice *dev, int req_uV) int max_uV = priv->max_voltage; int diff = max_uV - min_uV;
- return 100 - (((req_uV * 100) - (min_uV * 100)) / diff);
- return ((req_uV * 100) - (min_uV * 100)) / diff;
}
static int pwm_regulator_get_voltage(struct udevice *dev) @@ -67,6 +69,12 @@ static int pwm_regulator_set_voltage(struct udevice *dev, int uvolt)
duty_cycle = pwm_voltage_to_duty_cycle_percentage(dev, uvolt);
- ret = pwm_set_init(priv->pwm, priv->pwm_id, priv->polarity);
- if (ret) {
dev_err(dev, "Failed to init PWM\n");return ret;- }
- ret = pwm_set_config(priv->pwm, priv->pwm_id, (priv->period_ns / 100) * duty_cycle, priv->period_ns); if (ret) {
@@ -97,9 +105,9 @@ static int pwm_regulator_ofdata_to_platdata(struct udevice *dev) debug("%s: Cannot get PWM phandle: ret=%d\n", __func__, ret); return ret; }
/* TODO: pwm_id here from device tree if needed */
priv->period_ns = args.args[1];
priv->polarity = args.args[2];
priv->init_voltage = fdtdec_get_int(blob, node, "regulator-init-microvolt", -1);
diff --git a/drivers/pwm/pwm-uclass.c b/drivers/pwm/pwm-uclass.c index c2200af..287a7f3 100644 --- a/drivers/pwm/pwm-uclass.c +++ b/drivers/pwm/pwm-uclass.c @@ -9,6 +9,16 @@ #include <dm.h> #include <pwm.h>
+int pwm_set_init(struct udevice *dev, uint channel, uint polarity) +{
- struct pwm_ops *ops = pwm_get_ops(dev);
- if (!ops->set_init)
return -ENOSYS;- return ops->set_init(dev, channel, polarity);
+}
int pwm_set_config(struct udevice *dev, uint channel, uint period_ns, uint duty_ns) { diff --git a/drivers/pwm/rk_pwm.c b/drivers/pwm/rk_pwm.c index 9254f5b..5ff1e00 100644 --- a/drivers/pwm/rk_pwm.c +++ b/drivers/pwm/rk_pwm.c @@ -21,8 +21,22 @@ DECLARE_GLOBAL_DATA_PTR; struct rk_pwm_priv { struct rk3288_pwm *regs; ulong freq;
- uint enable_conf;
};
+static int rk_pwm_set_init(struct udevice *dev, uint channel, uint polarity) +{
"channel" is need?
- struct rk_pwm_priv *priv = dev_get_priv(dev);
- debug("%s: polarity=%u\n", __func__, polarity);
- if (polarity)
priv->enable_conf |= PWM_DUTY_NEGATIVE | PWM_INACTIVE_POSTIVE;- else
priv->enable_conf |= PWM_DUTY_POSTIVE | PWM_INACTIVE_NEGATIVE;- return 0;
+}
static int rk_pwm_set_config(struct udevice *dev, uint channel, uint period_ns, uint duty_ns) { @@ -32,7 +46,7 @@ static int rk_pwm_set_config(struct udevice *dev, uint channel, uint period_ns,
debug("%s: period_ns=%u, duty_ns=%u\n", __func__, period_ns, duty_ns); writel(PWM_SEL_SRC_CLK | PWM_OUTPUT_LEFT | PWM_LP_DISABLE |
PWM_CONTINUOUS | PWM_DUTY_POSTIVE | PWM_INACTIVE_POSTIVE |
PWM_CONTINUOUS | priv->enable_conf |@@ -83,6 +97,7 @@ static int rk_pwm_probe(struct udevice *dev) }
static const struct pwm_ops rk_pwm_ops = {
- .set_init = rk_pwm_set_init, .set_config = rk_pwm_set_config, .set_enable = rk_pwm_set_enable,
}; diff --git a/include/pwm.h b/include/pwm.h index 851915e..a66ee1f 100644 --- a/include/pwm.h +++ b/include/pwm.h @@ -14,6 +14,15 @@ /* struct pwm_ops: Operations for the PWM uclass */ struct pwm_ops { /**
* set_init() - Set the PWM invert** @dev: PWM device to update* @channel: PWM channel to update* @polarity: PWM invert polarity* @return 0 if OK, -ve on error*/- int (*set_init)(struct udevice *dev, uint channel, uint polarity);
Is there a reason about using int type? It's always return 0... Otherwise do you have a plan to use "int" type?
Best Regards, Jaehoon Chung
- /**
- set_config() - Set the PWM configuration
- @dev: PWM device to update
On 04/07/2017 07:28 PM, Jaehoon Chung wrote:
Hi Kever,
On 04/07/2017 08:02 PM, Kever Yang wrote:
The latest kernel PWM drivers enable the polarity settings. When system run from U-Boot to kerenl, if there are differences in polarity set or duty cycle, the PMW will re-init: close -> set polarity and duty cycle -> enable the PWM. The power supply controled by pwm regulator may have voltage shaking, which lead to the system not stable.
Signed-off-by: Kever Yang kever.yang@rock-chips.com
drivers/power/regulator/pwm_regulator.c | 12 ++++++++++-- drivers/pwm/pwm-uclass.c | 10 ++++++++++ drivers/pwm/rk_pwm.c | 17 ++++++++++++++++- include/pwm.h | 9 +++++++++ 4 files changed, 45 insertions(+), 3 deletions(-)
diff --git a/drivers/power/regulator/pwm_regulator.c b/drivers/power/regulator/pwm_regulator.c index 4875238..f8a6712 100644 --- a/drivers/power/regulator/pwm_regulator.c +++ b/drivers/power/regulator/pwm_regulator.c @@ -24,6 +24,8 @@ struct pwm_regulator_info { int pwm_id; /* the period of one PWM cycle */ int period_ns;
- /* the polarity of one PWM */
- int polarity; struct udevice *pwm; /* initialize voltage of regulator */ unsigned int init_voltage;
@@ -49,7 +51,7 @@ static int pwm_voltage_to_duty_cycle_percentage(struct udevice *dev, int req_uV) int max_uV = priv->max_voltage; int diff = max_uV - min_uV;
- return 100 - (((req_uV * 100) - (min_uV * 100)) / diff);
return ((req_uV * 100) - (min_uV * 100)) / diff; }
static int pwm_regulator_get_voltage(struct udevice *dev)
@@ -67,6 +69,12 @@ static int pwm_regulator_set_voltage(struct udevice *dev, int uvolt)
duty_cycle = pwm_voltage_to_duty_cycle_percentage(dev, uvolt);
- ret = pwm_set_init(priv->pwm, priv->pwm_id, priv->polarity);
- if (ret) {
dev_err(dev, "Failed to init PWM\n");return ret;- }
- ret = pwm_set_config(priv->pwm, priv->pwm_id, (priv->period_ns / 100) * duty_cycle, priv->period_ns); if (ret) {
@@ -97,9 +105,9 @@ static int pwm_regulator_ofdata_to_platdata(struct udevice *dev) debug("%s: Cannot get PWM phandle: ret=%d\n", __func__, ret); return ret; }
/* TODO: pwm_id here from device tree if needed */
priv->period_ns = args.args[1];
priv->polarity = args.args[2];
priv->init_voltage = fdtdec_get_int(blob, node, "regulator-init-microvolt", -1);
diff --git a/drivers/pwm/pwm-uclass.c b/drivers/pwm/pwm-uclass.c index c2200af..287a7f3 100644 --- a/drivers/pwm/pwm-uclass.c +++ b/drivers/pwm/pwm-uclass.c @@ -9,6 +9,16 @@ #include <dm.h> #include <pwm.h>
+int pwm_set_init(struct udevice *dev, uint channel, uint polarity) +{
- struct pwm_ops *ops = pwm_get_ops(dev);
- if (!ops->set_init)
return -ENOSYS;- return ops->set_init(dev, channel, polarity);
+}
- int pwm_set_config(struct udevice *dev, uint channel, uint period_ns, uint duty_ns) {
diff --git a/drivers/pwm/rk_pwm.c b/drivers/pwm/rk_pwm.c index 9254f5b..5ff1e00 100644 --- a/drivers/pwm/rk_pwm.c +++ b/drivers/pwm/rk_pwm.c @@ -21,8 +21,22 @@ DECLARE_GLOBAL_DATA_PTR; struct rk_pwm_priv { struct rk3288_pwm *regs; ulong freq;
- uint enable_conf; };
+static int rk_pwm_set_init(struct udevice *dev, uint channel, uint polarity) +{
"channel" is need?
Don't need.
- struct rk_pwm_priv *priv = dev_get_priv(dev);
- debug("%s: polarity=%u\n", __func__, polarity);
- if (polarity)
priv->enable_conf |= PWM_DUTY_NEGATIVE | PWM_INACTIVE_POSTIVE;- else
priv->enable_conf |= PWM_DUTY_POSTIVE | PWM_INACTIVE_NEGATIVE;- return 0;
+}
- static int rk_pwm_set_config(struct udevice *dev, uint channel, uint period_ns, uint duty_ns) {
@@ -32,7 +46,7 @@ static int rk_pwm_set_config(struct udevice *dev, uint channel, uint period_ns,
debug("%s: period_ns=%u, duty_ns=%u\n", __func__, period_ns, duty_ns); writel(PWM_SEL_SRC_CLK | PWM_OUTPUT_LEFT | PWM_LP_DISABLE |
PWM_CONTINUOUS | PWM_DUTY_POSTIVE | PWM_INACTIVE_POSTIVE |
PWM_CONTINUOUS | priv->enable_conf |@@ -83,6 +97,7 @@ static int rk_pwm_probe(struct udevice *dev) }
static const struct pwm_ops rk_pwm_ops = {
- .set_init = rk_pwm_set_init, .set_config = rk_pwm_set_config, .set_enable = rk_pwm_set_enable, };
diff --git a/include/pwm.h b/include/pwm.h index 851915e..a66ee1f 100644 --- a/include/pwm.h +++ b/include/pwm.h @@ -14,6 +14,15 @@ /* struct pwm_ops: Operations for the PWM uclass */ struct pwm_ops { /**
* set_init() - Set the PWM invert** @dev: PWM device to update* @channel: PWM channel to update* @polarity: PWM invert polarity* @return 0 if OK, -ve on error*/- int (*set_init)(struct udevice *dev, uint channel, uint polarity);
Is there a reason about using int type? It's always return 0... Otherwise do you have a plan to use "int" type?
No particular reason. just follow as set_config and set_enable.
Best Regards, Jaehoon Chung
- /**
- set_config() - Set the PWM configuration
- @dev: PWM device to update
Hi,
On 7 April 2017 at 05:02, Kever Yang kever.yang@rock-chips.com wrote:
The latest kernel PWM drivers enable the polarity settings. When system run from U-Boot to kerenl, if there are differences in polarity set or duty cycle, the PMW will re-init: close -> set polarity and duty cycle -> enable the PWM. The power supply controled by pwm regulator may have voltage shaking, which lead to the system not stable.
Signed-off-by: Kever Yang kever.yang@rock-chips.com
drivers/power/regulator/pwm_regulator.c | 12 ++++++++++-- drivers/pwm/pwm-uclass.c | 10 ++++++++++ drivers/pwm/rk_pwm.c | 17 ++++++++++++++++- include/pwm.h | 9 +++++++++ 4 files changed, 45 insertions(+), 3 deletions(-)
If the generic uclass change and the rk change can be separated, it is good to do it.
Also we should have a test for pwm (test/dm/pwm.c). Are you able to do that? If not I could give it a try.
diff --git a/drivers/power/regulator/pwm_regulator.c b/drivers/power/regulator/pwm_regulator.c index 4875238..f8a6712 100644 --- a/drivers/power/regulator/pwm_regulator.c +++ b/drivers/power/regulator/pwm_regulator.c @@ -24,6 +24,8 @@ struct pwm_regulator_info { int pwm_id; /* the period of one PWM cycle */ int period_ns;
/* the polarity of one PWM */int polarity;
Can you update the comment to indicate what the values mean? E.g. is 0 the normal polarity and 1 inverted?
struct udevice *pwm; /* initialize voltage of regulator */ unsigned int init_voltage;@@ -49,7 +51,7 @@ static int pwm_voltage_to_duty_cycle_percentage(struct udevice *dev, int req_uV) int max_uV = priv->max_voltage; int diff = max_uV - min_uV;
return 100 - (((req_uV * 100) - (min_uV * 100)) / diff);
return ((req_uV * 100) - (min_uV * 100)) / diff;}
static int pwm_regulator_get_voltage(struct udevice *dev) @@ -67,6 +69,12 @@ static int pwm_regulator_set_voltage(struct udevice *dev, int uvolt)
duty_cycle = pwm_voltage_to_duty_cycle_percentage(dev, uvolt);
ret = pwm_set_init(priv->pwm, priv->pwm_id, priv->polarity);
I wonder if it would be better to combine the polarity into the pwm_set_config() call? Then we can do everything in one call. If not then I think pwm_set_invert() would be a better name.
if (ret) {dev_err(dev, "Failed to init PWM\n");return ret;}ret = pwm_set_config(priv->pwm, priv->pwm_id, (priv->period_ns / 100) * duty_cycle, priv->period_ns); if (ret) {@@ -97,9 +105,9 @@ static int pwm_regulator_ofdata_to_platdata(struct udevice *dev) debug("%s: Cannot get PWM phandle: ret=%d\n", __func__, ret); return ret; }
/* TODO: pwm_id here from device tree if needed */ priv->period_ns = args.args[1];
priv->polarity = args.args[2];
Does this mean that the binding has this argument and we have been ignoring it?
Can you bring in the DT binding file from Linux to U-Boot also?
priv->init_voltage = fdtdec_get_int(blob, node, "regulator-init-microvolt", -1);diff --git a/drivers/pwm/pwm-uclass.c b/drivers/pwm/pwm-uclass.c index c2200af..287a7f3 100644 --- a/drivers/pwm/pwm-uclass.c +++ b/drivers/pwm/pwm-uclass.c @@ -9,6 +9,16 @@ #include <dm.h> #include <pwm.h>
+int pwm_set_init(struct udevice *dev, uint channel, uint polarity) +{
struct pwm_ops *ops = pwm_get_ops(dev);if (!ops->set_init)return -ENOSYS;return ops->set_init(dev, channel, polarity);+}
int pwm_set_config(struct udevice *dev, uint channel, uint period_ns, uint duty_ns) { diff --git a/drivers/pwm/rk_pwm.c b/drivers/pwm/rk_pwm.c index 9254f5b..5ff1e00 100644 --- a/drivers/pwm/rk_pwm.c +++ b/drivers/pwm/rk_pwm.c @@ -21,8 +21,22 @@ DECLARE_GLOBAL_DATA_PTR; struct rk_pwm_priv { struct rk3288_pwm *regs; ulong freq;
uint enable_conf;};
+static int rk_pwm_set_init(struct udevice *dev, uint channel, uint polarity) +{
struct rk_pwm_priv *priv = dev_get_priv(dev);debug("%s: polarity=%u\n", __func__, polarity);if (polarity)priv->enable_conf |= PWM_DUTY_NEGATIVE | PWM_INACTIVE_POSTIVE;elsepriv->enable_conf |= PWM_DUTY_POSTIVE | PWM_INACTIVE_NEGATIVE;return 0;+}
static int rk_pwm_set_config(struct udevice *dev, uint channel, uint period_ns, uint duty_ns) { @@ -32,7 +46,7 @@ static int rk_pwm_set_config(struct udevice *dev, uint channel, uint period_ns,
debug("%s: period_ns=%u, duty_ns=%u\n", __func__, period_ns, duty_ns); writel(PWM_SEL_SRC_CLK | PWM_OUTPUT_LEFT | PWM_LP_DISABLE |
PWM_CONTINUOUS | PWM_DUTY_POSTIVE | PWM_INACTIVE_POSTIVE |
PWM_CONTINUOUS | priv->enable_conf | RK_PWM_DISABLE, ®s->ctrl);@@ -83,6 +97,7 @@ static int rk_pwm_probe(struct udevice *dev) }
static const struct pwm_ops rk_pwm_ops = {
.set_init = rk_pwm_set_init, .set_config = rk_pwm_set_config, .set_enable = rk_pwm_set_enable,}; diff --git a/include/pwm.h b/include/pwm.h index 851915e..a66ee1f 100644 --- a/include/pwm.h +++ b/include/pwm.h @@ -14,6 +14,15 @@ /* struct pwm_ops: Operations for the PWM uclass */ struct pwm_ops { /**
* set_init() - Set the PWM invert** @dev: PWM device to update* @channel: PWM channel to update* @polarity: PWM invert polarity* @return 0 if OK, -ve on error*/int (*set_init)(struct udevice *dev, uint channel, uint polarity);/** * set_config() - Set the PWM configuration * * @dev: PWM device to update-- 1.9.1
Regards, Simon
On 04/10/2017 03:28 AM, Simon Glass wrote:
Hi,
On 7 April 2017 at 05:02, Kever Yang kever.yang@rock-chips.com wrote:
The latest kernel PWM drivers enable the polarity settings. When system run from U-Boot to kerenl, if there are differences in polarity set or duty cycle, the PMW will re-init: close -> set polarity and duty cycle -> enable the PWM. The power supply controled by pwm regulator may have voltage shaking, which lead to the system not stable.
Signed-off-by: Kever Yang kever.yang@rock-chips.com
drivers/power/regulator/pwm_regulator.c | 12 ++++++++++-- drivers/pwm/pwm-uclass.c | 10 ++++++++++ drivers/pwm/rk_pwm.c | 17 ++++++++++++++++- include/pwm.h | 9 +++++++++ 4 files changed, 45 insertions(+), 3 deletions(-)
If the generic uclass change and the rk change can be separated, it is good to do it.
Also we should have a test for pwm (test/dm/pwm.c). Are you able to do that? If not I could give it a try.
we have no plan to do it.
diff --git a/drivers/power/regulator/pwm_regulator.c b/drivers/power/regulator/pwm_regulator.c index 4875238..f8a6712 100644 --- a/drivers/power/regulator/pwm_regulator.c +++ b/drivers/power/regulator/pwm_regulator.c @@ -24,6 +24,8 @@ struct pwm_regulator_info { int pwm_id; /* the period of one PWM cycle */ int period_ns;
/* the polarity of one PWM */int polarity;Can you update the comment to indicate what the values mean? E.g. is 0 the normal polarity and 1 inverted?
0 : normal polarity 1 : inverted polarity I will update the comment next version.
struct udevice *pwm; /* initialize voltage of regulator */ unsigned int init_voltage;@@ -49,7 +51,7 @@ static int pwm_voltage_to_duty_cycle_percentage(struct udevice *dev, int req_uV) int max_uV = priv->max_voltage; int diff = max_uV - min_uV;
return 100 - (((req_uV * 100) - (min_uV * 100)) / diff);
return ((req_uV * 100) - (min_uV * 100)) / diff;}
static int pwm_regulator_get_voltage(struct udevice *dev)
@@ -67,6 +69,12 @@ static int pwm_regulator_set_voltage(struct udevice *dev, int uvolt)
duty_cycle = pwm_voltage_to_duty_cycle_percentage(dev, uvolt);
ret = pwm_set_init(priv->pwm, priv->pwm_id, priv->polarity);I wonder if it would be better to combine the polarity into the pwm_set_config() call? Then we can do everything in one call. If not then I think pwm_set_invert() would be a better name.
The polarity set only once, so we set it in pwm_set_init() call. Using pwm_set_invert, of course, is also possible.
if (ret) {dev_err(dev, "Failed to init PWM\n");return ret;}ret = pwm_set_config(priv->pwm, priv->pwm_id, (priv->period_ns / 100) * duty_cycle, priv->period_ns); if (ret) {@@ -97,9 +105,9 @@ static int pwm_regulator_ofdata_to_platdata(struct udevice *dev) debug("%s: Cannot get PWM phandle: ret=%d\n", __func__, ret); return ret; }
/* TODO: pwm_id here from device tree if needed */ priv->period_ns = args.args[1];
priv->polarity = args.args[2];Does this mean that the binding has this argument and we have been ignoring it?
Can you bring in the DT binding file from Linux to U-Boot also?
priv->init_voltage = fdtdec_get_int(blob, node, "regulator-init-microvolt", -1);diff --git a/drivers/pwm/pwm-uclass.c b/drivers/pwm/pwm-uclass.c index c2200af..287a7f3 100644 --- a/drivers/pwm/pwm-uclass.c +++ b/drivers/pwm/pwm-uclass.c @@ -9,6 +9,16 @@ #include <dm.h> #include <pwm.h>
+int pwm_set_init(struct udevice *dev, uint channel, uint polarity) +{
struct pwm_ops *ops = pwm_get_ops(dev);if (!ops->set_init)return -ENOSYS;return ops->set_init(dev, channel, polarity);+}
- int pwm_set_config(struct udevice *dev, uint channel, uint period_ns, uint duty_ns) {
diff --git a/drivers/pwm/rk_pwm.c b/drivers/pwm/rk_pwm.c index 9254f5b..5ff1e00 100644 --- a/drivers/pwm/rk_pwm.c +++ b/drivers/pwm/rk_pwm.c @@ -21,8 +21,22 @@ DECLARE_GLOBAL_DATA_PTR; struct rk_pwm_priv { struct rk3288_pwm *regs; ulong freq;
uint enable_conf;+static int rk_pwm_set_init(struct udevice *dev, uint channel, uint polarity) +{
struct rk_pwm_priv *priv = dev_get_priv(dev);debug("%s: polarity=%u\n", __func__, polarity);if (polarity)priv->enable_conf |= PWM_DUTY_NEGATIVE | PWM_INACTIVE_POSTIVE;elsepriv->enable_conf |= PWM_DUTY_POSTIVE | PWM_INACTIVE_NEGATIVE;return 0;+}
- static int rk_pwm_set_config(struct udevice *dev, uint channel, uint period_ns, uint duty_ns) {
@@ -32,7 +46,7 @@ static int rk_pwm_set_config(struct udevice *dev, uint channel, uint period_ns,
debug("%s: period_ns=%u, duty_ns=%u\n", __func__, period_ns, duty_ns); writel(PWM_SEL_SRC_CLK | PWM_OUTPUT_LEFT | PWM_LP_DISABLE |
PWM_CONTINUOUS | PWM_DUTY_POSTIVE | PWM_INACTIVE_POSTIVE |
PWM_CONTINUOUS | priv->enable_conf | RK_PWM_DISABLE, ®s->ctrl);@@ -83,6 +97,7 @@ static int rk_pwm_probe(struct udevice *dev) }
static const struct pwm_ops rk_pwm_ops = {
.set_init = rk_pwm_set_init, .set_config = rk_pwm_set_config, .set_enable = rk_pwm_set_enable,diff --git a/include/pwm.h b/include/pwm.h index 851915e..a66ee1f 100644 --- a/include/pwm.h +++ b/include/pwm.h @@ -14,6 +14,15 @@ /* struct pwm_ops: Operations for the PWM uclass */ struct pwm_ops { /**
* set_init() - Set the PWM invert** @dev: PWM device to update* @channel: PWM channel to update* @polarity: PWM invert polarity* @return 0 if OK, -ve on error*/int (*set_init)(struct udevice *dev, uint channel, uint polarity);/** * set_config() - Set the PWM configuration * * @dev: PWM device to update-- 1.9.1
Regards, Simon
Hi Elaine,
On 9 April 2017 at 20:09, Elaine Zhang zhangqing@rock-chips.com wrote:
On 04/10/2017 03:28 AM, Simon Glass wrote:
Hi,
On 7 April 2017 at 05:02, Kever Yang kever.yang@rock-chips.com wrote:
The latest kernel PWM drivers enable the polarity settings. When system run from U-Boot to kerenl, if there are differences in polarity set or duty cycle, the PMW will re-init: close -> set polarity and duty cycle -> enable the PWM. The power supply controled by pwm regulator may have voltage shaking, which lead to the system not stable.
Signed-off-by: Kever Yang kever.yang@rock-chips.com
drivers/power/regulator/pwm_regulator.c | 12 ++++++++++-- drivers/pwm/pwm-uclass.c | 10 ++++++++++ drivers/pwm/rk_pwm.c | 17 ++++++++++++++++- include/pwm.h | 9 +++++++++ 4 files changed, 45 insertions(+), 3 deletions(-)
If the generic uclass change and the rk change can be separated, it is good to do it.
Also we should have a test for pwm (test/dm/pwm.c). Are you able to do that? If not I could give it a try.
we have no plan to do it.
I've created something simple here. You can base your next series on u-boot-dm/pwm-working.
http://patchwork.ozlabs.org/patch/751254/
diff --git a/drivers/power/regulator/pwm_regulator.c b/drivers/power/regulator/pwm_regulator.c index 4875238..f8a6712 100644 --- a/drivers/power/regulator/pwm_regulator.c +++ b/drivers/power/regulator/pwm_regulator.c @@ -24,6 +24,8 @@ struct pwm_regulator_info { int pwm_id; /* the period of one PWM cycle */ int period_ns;
/* the polarity of one PWM */int polarity;Can you update the comment to indicate what the values mean? E.g. is 0 the normal polarity and 1 inverted?
0 : normal polarity 1 : inverted polarity I will update the comment next version.
struct udevice *pwm; /* initialize voltage of regulator */ unsigned int init_voltage;@@ -49,7 +51,7 @@ static int pwm_voltage_to_duty_cycle_percentage(struct udevice *dev, int req_uV) int max_uV = priv->max_voltage; int diff = max_uV - min_uV;
return 100 - (((req_uV * 100) - (min_uV * 100)) / diff);
return ((req_uV * 100) - (min_uV * 100)) / diff;}
static int pwm_regulator_get_voltage(struct udevice *dev)
@@ -67,6 +69,12 @@ static int pwm_regulator_set_voltage(struct udevice *dev, int uvolt)
duty_cycle = pwm_voltage_to_duty_cycle_percentage(dev, uvolt);
ret = pwm_set_init(priv->pwm, priv->pwm_id, priv->polarity);I wonder if it would be better to combine the polarity into the pwm_set_config() call? Then we can do everything in one call. If not then I think pwm_set_invert() would be a better name.
The polarity set only once, so we set it in pwm_set_init() call. Using pwm_set_invert, of course, is also possible.
OK, so that's why it should a separate call. Seems OK to me.
if (ret) {dev_err(dev, "Failed to init PWM\n");return ret;}ret = pwm_set_config(priv->pwm, priv->pwm_id, (priv->period_ns / 100) * duty_cycle,priv->period_ns); if (ret) { @@ -97,9 +105,9 @@ static int pwm_regulator_ofdata_to_platdata(struct udevice *dev) debug("%s: Cannot get PWM phandle: ret=%d\n", __func__, ret); return ret; }
/* TODO: pwm_id here from device tree if needed */ priv->period_ns = args.args[1];
priv->polarity = args.args[2];Does this mean that the binding has this argument and we have been ignoring it?
Can you bring in the DT binding file from Linux to U-Boot also?
Did you see this one?
Regards, Simon
participants (4)
-
Elaine Zhang -
Jaehoon Chung -
Kever Yang -
Simon Glass