Hello Vincent, On 03.05.21 09:26, Vincent Chen wrote: > The pwm_sifive_set_config() and pwm_sifive_set_enable() cannot work > properly due to the wrong implementations. It will cause the u-boot > PWM command to not work as expected. The bugs will be resolved in this > patch. > > Signed-off-by: Vincent Chen <vincent.c...@sifive.com> > --- > drivers/pwm/pwm-sifive.c | 21 +++++++++++---------- > 1 file changed, 11 insertions(+), 10 deletions(-) > > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c > index 01212d6..b9813a3 100644 > --- a/drivers/pwm/pwm-sifive.c > +++ b/drivers/pwm/pwm-sifive.c > @@ -38,6 +38,9 @@ > #define PWM_SIFIVE_SIZE_PWMCMP 4 > #define PWM_SIFIVE_CMPWIDTH 16 > > +#define PWM_SIFIVE_CHANNEL_ENABLE_VAL 0 > +#define PWM_SIFIVE_CHANNEL_DISABLE_VAL 0xffff > + > DECLARE_GLOBAL_DATA_PTR; > > struct pwm_sifive_regs { > @@ -77,7 +80,7 @@ static int pwm_sifive_set_config(struct udevice *dev, uint > channel, > */ > scale_pow = lldiv((uint64_t)priv->freq * period_ns, 1000000000); > scale = clamp(ilog2(scale_pow) - PWM_SIFIVE_CMPWIDTH, 0, 0xf); > - val |= FIELD_PREP(PWM_SIFIVE_PWMCFG_SCALE, scale); > + val |= (FIELD_PREP(PWM_SIFIVE_PWMCFG_SCALE, scale) | > PWM_SIFIVE_PWMCFG_EN_ALWAYS);
Ok, for this as it seems the same as in linux: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pwm/pwm-sifive.c#n96 > /* > * The problem of output producing mixed setting as mentioned at top, > @@ -88,6 +91,7 @@ static int pwm_sifive_set_config(struct udevice *dev, uint > channel, > num = (u64)duty_ns * (1U << PWM_SIFIVE_CMPWIDTH); > frac = DIV_ROUND_CLOSEST_ULL(num, period_ns); > frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1); > + frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac; I just looked into linux code, and current code is the same as in linux, see: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pwm/pwm-sifive.c#n186 May you can describe what problem you exactly fix? May this part is not needed? I wonder, why this is not also a problem than in linux? Thanks! bye, Heiko > > writel(val, priv->base + regs->cfg); > writel(frac, priv->base + regs->cmp0 + channel * > @@ -100,18 +104,15 @@ static int pwm_sifive_set_enable(struct udevice *dev, > uint channel, bool enable) > { > struct pwm_sifive_priv *priv = dev_get_priv(dev); > const struct pwm_sifive_regs *regs = &priv->data->regs; > - u32 val; > > debug("%s: Enable '%s'\n", __func__, dev->name); > > - if (enable) { > - val = readl(priv->base + regs->cfg); > - val |= PWM_SIFIVE_PWMCFG_EN_ALWAYS; > - writel(val, priv->base + regs->cfg); > - } else { > - writel(0, priv->base + regs->cmp0 + channel * > - PWM_SIFIVE_SIZE_PWMCMP); > - } > + if (enable) > + writel(PWM_SIFIVE_CHANNEL_ENABLE_VAL, priv->base + > + regs->cmp0 + channel * PWM_SIFIVE_SIZE_PWMCMP); > + else > + writel(PWM_SIFIVE_CHANNEL_DISABLE_VAL, priv->base + > + regs->cmp0 + channel * PWM_SIFIVE_SIZE_PWMCMP); > > return 0; > } > -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: h...@denx.de