| Message ID | 1491562976-21917-1-git-send-email-kever.yang@rock-chips.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Jaehoon Chung |
| Headers | show |
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 | > 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); 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 >
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; > + 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 > -- > 1.9.1 > Regards, Simon
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 | >> 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); > > 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 >> > > > >
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; >> + 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 >> -- >> 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
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
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(-)