| Message ID | 1620026809-16227-1-git-send-email-vincent.chen@sifive.com |
|---|---|
| State | Accepted |
| Commit | cc25f346c9e58f02efb68003d0526825fee265ea |
| Delegated to: | Andes |
| Headers | show |
| Series | pwm: sifive: make set_config() and set_enable() work properly | expand |
> From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Vincent Chen > Sent: Monday, May 03, 2021 3:27 PM > To: sjg@chromium.org; hs@denx.de > Cc: u-boot@lists.denx.de; Vincent Chen <vincent.chen@sifive.com> > Subject: [PATCH] pwm: sifive: make set_config() and set_enable() work properly > > 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.chen@sifive.com> > --- > drivers/pwm/pwm-sifive.c | 21 +++++++++++---------- > 1 file changed, 11 insertions(+), 10 deletions(-) Reviewed-by: Rick Chen <rick@andestech.com>
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.chen@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; > } >
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); /* * 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; 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; }
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.chen@sifive.com> --- drivers/pwm/pwm-sifive.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)