[PATCH] pwm: sifive: make set_config() and set_enable() work properly

[PATCH] pwm: sifive: make set_config() and set_enable() work properly

[Vincent Chen]

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);
 
 	/*
 	 * 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;
 }
-- 
2.7.4

FW: [PATCH] pwm: sifive: make set_config() and set_enable() work properly

[Rick Chen]

> From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Vincent Chen
> Sent: Monday, May 03, 2021 3:27 PM
> To: sjg at chromium.org; hs at denx.de
> Cc: u-boot at 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>

[PATCH] pwm: sifive: make set_config() and set_enable() work properly

[Heiko Schocher]

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;
>  }
> 

-- 
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: hs at denx.de