Re: [PATCH] pwm: sifive: make set_config() and set_enable() work properly
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
> ---
> 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 0x
> +
> 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, 10);
> 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 = >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
Re: FW: [PATCH] pwm: sifive: make set_config() and set_enable() work properly
> From: U-Boot On Behalf Of Vincent Chen > Sent: Monday, May 03, 2021 3:27 PM > To: s...@chromium.org; h...@denx.de > Cc: u-boot@lists.denx.de; Vincent Chen > 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 > --- > drivers/pwm/pwm-sifive.c | 21 +++-- > 1 file changed, 11 insertions(+), 10 deletions(-) Reviewed-by: Rick Chen
[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
---
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 0x
+
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, 10);
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 = >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
[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
---
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 0x
+
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, 10);
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 = >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
