Re: [PATCH v8 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
On Mon, Mar 11, 2019 at 6:59 PM Uwe Kleine-König
wrote:
>
> Hello,
>
> On Mon, Mar 11, 2019 at 05:10:17PM +0530, Yash Shah wrote:
> > On Fri, Mar 8, 2019 at 5:27 PM Uwe Kleine-König
> > wrote:
> > > On Fri, Mar 08, 2019 at 04:59:36PM +0530, Yash Shah wrote:
> > > > On Thu, Mar 7, 2019 at 8:57 PM Uwe Kleine-König
> > > > wrote:
> > > > > > + if (state->period != cur_state.period) {
> > > > >
> > > > > Did you test this with more than one consumer? For sure the following
> > > > > should work:
> > > > >
> > > > > pwm1 = pwm_get(.. the first ..);
> > > > > pwm_apply_state(pwm1, { .enabled = true, .period = 1000,
> > > > > });
> > > > >
> > > > > pwm2 = pwm_get(.. the second ..);
> > > > > pwm_apply_state(pwm2, { .enabled = true, .period = 1000,
> > > > > });
> > > > >
> > > > > but for the second pwm_apply_state() run state->period is likely not
> > > > > exactly 1000.
> > > >
> > > > Yes, I have tested multiple consumers using sysfs interface. It is
> > > > working.
> > >
> > > Can you provide details about your testing here? What is the parent clk
> > > rate? Which settings did you test? Can you confirm my claim that the
> > > above sequence would fail or point out my error in reasoning?
> > >
> >
> > I have tested on HiFive Unleashed board using sysfs interface.
> > Parent clk rate is around 512 Mhz.
> > I have tested scenarios as you mentioned above with various period and
> > duty_cycle values.
> >
> > After considering your below suggestion,
> > | To get the result independent of the prior configuration you better use
> > | the real targeted period length as input instead of the last configured
> > | approximation
> > I will introduce approx_period feild, which will be used as the
> > targeted period length.
> > Also, in pwm_sifive_get_state, I will make below change
> > - state->period = pwm->real_period;
> > + state->period = pwm->approx_period.
> > So with this change in place, I believe the cur_state.period for the
> > second pwm_apply_state() above (pwm2) will be exactly 1000
>
> I don't understand your intention completely. Just send a new patch
> round, then I will gladly take another look.
I was planning to go with above-mentioned change but then I realized
that pwm state should always reflect the current hardware state, so I
have dropped the above idea.
Coming back to your concern on
| if (state->period != cur_state.period) {
It is not failing for the scenario you mentioned but failing for
another way around.
pwm1 = pwm_get(.. the first ..);
pwm_apply_state(pwm1, { .enabled = true, .period = 1000, });
pwm2 = pwm_get(.. the second ..);
pwm_apply_state(pwm2, { .enabled = true, .period = 2000, });
The pwm2 should get an error for period mismatch but with v8 patch, it
is not getting any error.
I am sending a v9 patch which has the fix for this.
With v9 changes, I have tested all scenarios and it is working fine.
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König|
> Industrial Linux Solutions | http://www.pengutronix.de/ |
Re: [PATCH v8 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
Hello,
On Mon, Mar 11, 2019 at 05:10:17PM +0530, Yash Shah wrote:
> On Fri, Mar 8, 2019 at 5:27 PM Uwe Kleine-König
> wrote:
> > On Fri, Mar 08, 2019 at 04:59:36PM +0530, Yash Shah wrote:
> > > On Thu, Mar 7, 2019 at 8:57 PM Uwe Kleine-König
> > > wrote:
> > > > > + if (state->period != cur_state.period) {
> > > >
> > > > Did you test this with more than one consumer? For sure the following
> > > > should work:
> > > >
> > > > pwm1 = pwm_get(.. the first ..);
> > > > pwm_apply_state(pwm1, { .enabled = true, .period = 1000,
> > > > });
> > > >
> > > > pwm2 = pwm_get(.. the second ..);
> > > > pwm_apply_state(pwm2, { .enabled = true, .period = 1000,
> > > > });
> > > >
> > > > but for the second pwm_apply_state() run state->period is likely not
> > > > exactly 1000.
> > >
> > > Yes, I have tested multiple consumers using sysfs interface. It is
> > > working.
> >
> > Can you provide details about your testing here? What is the parent clk
> > rate? Which settings did you test? Can you confirm my claim that the
> > above sequence would fail or point out my error in reasoning?
> >
>
> I have tested on HiFive Unleashed board using sysfs interface.
> Parent clk rate is around 512 Mhz.
> I have tested scenarios as you mentioned above with various period and
> duty_cycle values.
>
> After considering your below suggestion,
> | To get the result independent of the prior configuration you better use
> | the real targeted period length as input instead of the last configured
> | approximation
> I will introduce approx_period feild, which will be used as the
> targeted period length.
> Also, in pwm_sifive_get_state, I will make below change
> - state->period = pwm->real_period;
> + state->period = pwm->approx_period.
> So with this change in place, I believe the cur_state.period for the
> second pwm_apply_state() above (pwm2) will be exactly 1000
I don't understand your intention completely. Just send a new patch
round, then I will gladly take another look.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/ |
Re: [PATCH v8 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
On Fri, Mar 8, 2019 at 5:27 PM Uwe Kleine-König
wrote:
>
> Hello,
>
> On Fri, Mar 08, 2019 at 04:59:36PM +0530, Yash Shah wrote:
> > On Thu, Mar 7, 2019 at 8:57 PM Uwe Kleine-König
> > wrote:
> > > > +static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device
> > > > *dev,
> > > > + struct pwm_state *state)
> > > > +{
> > > > + struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> > > > + unsigned int duty_cycle, x;
> > > > + u32 frac;
> > > > + struct pwm_state cur_state;
> > > > + bool enabled;
> > > > + int ret = 0;
> > > > + unsigned long num;
> > > > +
> > > > + if (state->polarity != PWM_POLARITY_INVERSED)
> > > > + return -EINVAL;
> > > > +
> > > > + mutex_lock(>lock);
> > > > + pwm_get_state(dev, _state);
> > > > + enabled = cur_state.enabled;
> > > > +
> > > > + if (state->period != cur_state.period) {
> > >
> > > Did you test this with more than one consumer? For sure the following
> > > should work:
> > >
> > > pwm1 = pwm_get(.. the first ..);
> > > pwm_apply_state(pwm1, { .enabled = true, .period = 1000,
> > > });
> > >
> > > pwm2 = pwm_get(.. the second ..);
> > > pwm_apply_state(pwm2, { .enabled = true, .period = 1000,
> > > });
> > >
> > > but for the second pwm_apply_state() run state->period is likely not
> > > exactly 1000.
> >
> > Yes, I have tested multiple consumers using sysfs interface. It is working.
>
> Can you provide details about your testing here? What is the parent clk
> rate? Which settings did you test? Can you confirm my claim that the
> above sequence would fail or point out my error in reasoning?
>
I have tested on HiFive Unleashed board using sysfs interface.
Parent clk rate is around 512 Mhz.
I have tested scenarios as you mentioned above with various period and
duty_cycle values.
After considering your below suggestion,
| To get the result independent of the prior configuration you better use
| the real targeted period length as input instead of the last configured
| approximation
I will introduce approx_period feild, which will be used as the
targeted period length.
Also, in pwm_sifive_get_state, I will make below change
- state->period = pwm->real_period;
+ state->period = pwm->approx_period.
So with this change in place, I believe the cur_state.period for the
second pwm_apply_state() above (pwm2) will be exactly 1000
> > > > + x = 1U << PWM_SIFIVE_CMPWIDTH;
> > > > + num = (u64)duty_cycle * x + x / 2;
> > > > + frac = div_u64(num, state->period);
> > >
> > > I don't understand the "+ x / 2" part. Should this better be
> > > "+ state->period / 2"? Something like
> >
> > This eqn is as per your comments against v5 of this patch series.
> > frac = (duty_cycle * (1 << PWM_SIFIVE_CMPWIDTH) + (1 <<
> > PWM_SIFIVE_CMPWIDTH) / 2) / period;
>
> OK, then not only the code is wrong, but also my suggestion was. :-)
>
> > > #define div_u64_round(a, b) ({typeof(b) __b = b; div_u64(a + __b / 2,
> > > __b)})
> > >
> > > would make this less error prone.
>
> This still stands. It makes it easier to get the code right and makes it
> easier to understand.
Sure, will implement this.
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König|
> Industrial Linux Solutions | http://www.pengutronix.de/ |
Re: [PATCH v8 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
Hello,
On Fri, Mar 08, 2019 at 04:59:36PM +0530, Yash Shah wrote:
> On Thu, Mar 7, 2019 at 8:57 PM Uwe Kleine-König
> wrote:
> > > +static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device
> > > *dev,
> > > + struct pwm_state *state)
> > > +{
> > > + struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> > > + unsigned int duty_cycle, x;
> > > + u32 frac;
> > > + struct pwm_state cur_state;
> > > + bool enabled;
> > > + int ret = 0;
> > > + unsigned long num;
> > > +
> > > + if (state->polarity != PWM_POLARITY_INVERSED)
> > > + return -EINVAL;
> > > +
> > > + mutex_lock(>lock);
> > > + pwm_get_state(dev, _state);
> > > + enabled = cur_state.enabled;
> > > +
> > > + if (state->period != cur_state.period) {
> >
> > Did you test this with more than one consumer? For sure the following
> > should work:
> >
> > pwm1 = pwm_get(.. the first ..);
> > pwm_apply_state(pwm1, { .enabled = true, .period = 1000,
> > });
> >
> > pwm2 = pwm_get(.. the second ..);
> > pwm_apply_state(pwm2, { .enabled = true, .period = 1000,
> > });
> >
> > but for the second pwm_apply_state() run state->period is likely not
> > exactly 1000.
>
> Yes, I have tested multiple consumers using sysfs interface. It is working.
Can you provide details about your testing here? What is the parent clk
rate? Which settings did you test? Can you confirm my claim that the
above sequence would fail or point out my error in reasoning?
> > > + x = 1U << PWM_SIFIVE_CMPWIDTH;
> > > + num = (u64)duty_cycle * x + x / 2;
> > > + frac = div_u64(num, state->period);
> >
> > I don't understand the "+ x / 2" part. Should this better be
> > "+ state->period / 2"? Something like
>
> This eqn is as per your comments against v5 of this patch series.
> frac = (duty_cycle * (1 << PWM_SIFIVE_CMPWIDTH) + (1 <<
> PWM_SIFIVE_CMPWIDTH) / 2) / period;
OK, then not only the code is wrong, but also my suggestion was. :-)
> > #define div_u64_round(a, b) ({typeof(b) __b = b; div_u64(a + __b / 2, __b)})
> >
> > would make this less error prone.
This still stands. It makes it easier to get the code right and makes it
easier to understand.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/ |
Re: [PATCH v8 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
On Thu, Mar 7, 2019 at 8:57 PM Uwe Kleine-König
wrote:
>
> Hello,
>
> On Fri, Mar 01, 2019 at 04:23:19PM +0530, Yash Shah wrote:
> > Adds a PWM driver for PWM chip present in SiFive's HiFive Unleashed SoC.
> >
> > Signed-off-by: Wesley W. Terpstra
> > [Atish: Various fixes and code cleanup]
> > Signed-off-by: Atish Patra
> > Signed-off-by: Yash Shah
> > ---
> > drivers/pwm/Kconfig | 11 ++
> > drivers/pwm/Makefile | 1 +
> > drivers/pwm/pwm-sifive.c | 345
> > +++
> > 3 files changed, 357 insertions(+)
> > create mode 100644 drivers/pwm/pwm-sifive.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index a8f47df..4a61d1a 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -380,6 +380,17 @@ config PWM_SAMSUNG
> > To compile this driver as a module, choose M here: the module
> > will be called pwm-samsung.
> >
> > +config PWM_SIFIVE
> > + tristate "SiFive PWM support"
> > + depends on OF
> > + depends on COMMON_CLK
> > + depends on RISCV || COMPILE_TEST
> > + help
> > + Generic PWM framework driver for SiFive SoCs.
> > +
> > + To compile this driver as a module, choose M here: the module
> > + will be called pwm-sifive.
> > +
> > config PWM_SPEAR
> > tristate "STMicroelectronics SPEAr PWM support"
> > depends on PLAT_SPEAR
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > index 9c676a0..30089ca 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -37,6 +37,7 @@ obj-$(CONFIG_PWM_RCAR) += pwm-rcar.o
> > obj-$(CONFIG_PWM_RENESAS_TPU)+= pwm-renesas-tpu.o
> > obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o
> > obj-$(CONFIG_PWM_SAMSUNG)+= pwm-samsung.o
> > +obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o
> > obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o
> > obj-$(CONFIG_PWM_STI)+= pwm-sti.o
> > obj-$(CONFIG_PWM_STM32) += pwm-stm32.o
> > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> > new file mode 100644
> > index 000..6679ec7
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-sifive.c
> > @@ -0,0 +1,345 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2017-2018 SiFive
> > + * For SiFive's PWM IP block documentation please refer Chapter 14 of
> > + * Reference Manual : https://static.dev.sifive.com/FU540-C000-v1.0.pdf
> > + *
> > + * Limitations:
> > + * - When changing both duty cycle and period, we cannot prevent in
> > + * software that the output might produce a period with mixed
> > + * settings (new period length and old duty cycle).
> > + * - The hardware cannot generate a 100% duty cycle.
> > + * - The hardware generates only inverted output.
> > + */
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +/* Register offsets */
> > +#define PWM_SIFIVE_PWMCFG0x0
> > +#define PWM_SIFIVE_PWMCOUNT 0x8
> > +#define PWM_SIFIVE_PWMS 0x10
> > +#define PWM_SIFIVE_PWMCMP0 0x20
> > +
> > +/* PWMCFG fields */
> > +#define PWM_SIFIVE_PWMCFG_SCALE 0
> > +#define PWM_SIFIVE_PWMCFG_STICKY 8
> > +#define PWM_SIFIVE_PWMCFG_ZERO_CMP 9
> > +#define PWM_SIFIVE_PWMCFG_DEGLITCH 10
> > +#define PWM_SIFIVE_PWMCFG_EN_ALWAYS BIT(12)
> > +#define PWM_SIFIVE_PWMCFG_EN_ONCE13
> > +#define PWM_SIFIVE_PWMCFG_CENTER 16
> > +#define PWM_SIFIVE_PWMCFG_GANG 24
> > +#define PWM_SIFIVE_PWMCFG_IP 28
>
> It's a bit inconsistent to have one of them use BIT and the others not.
> For consistency please use BIT for all defines. (They are unused
> anyhow.) For PWM_SIFIVE_PWMCFG_SCALE use:
>
> #define PWM_SIFIVE_PWMCFG_SCALE GENMASK(3, 0)
>
> and then FIELD_GET and FIELD_PREP to access the values.
Sure will do that.
>
> > +/* PWM_SIFIVE_SIZE_PWMCMP is used to calculate offset for pwmcmpX
> > registers */
> > +#define PWM_SIFIVE_SIZE_PWMCMP 4
> > +#define PWM_SIFIVE_CMPWIDTH 16
> > +#define PWM_SIFIVE_DEFAULT_PERIOD1000
> > +
> > +struct pwm_sifive_ddata {
> > + struct pwm_chip chip;
> > + struct mutex lock; /* lock to protect user_count */
> > + struct notifier_block notifier;
> > + struct clk *clk;
> > + void __iomem *regs;
> > + unsigned int real_period;
> > + int user_count;
> > +};
> > +
> > +static inline
> > +struct pwm_sifive_ddata *pwm_sifive_chip_to_ddata(struct pwm_chip *c)
> > +{
> > + return container_of(c, struct pwm_sifive_ddata, chip);
> > +}
> > +
> > +static int pwm_sifive_request(struct pwm_chip *chip, struct pwm_device
> > *dev)
> > +{
> > + struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> > +
> > + mutex_lock(>lock);
> > + pwm->user_count++;
> > + mutex_unlock(>lock);
> > +
> > + return 0;
> > +}
> > +
> > +static void pwm_sifive_free(struct pwm_chip *chip, struct
Re: [PATCH v8 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
Hello,
On Fri, Mar 01, 2019 at 04:23:19PM +0530, Yash Shah wrote:
> Adds a PWM driver for PWM chip present in SiFive's HiFive Unleashed SoC.
>
> Signed-off-by: Wesley W. Terpstra
> [Atish: Various fixes and code cleanup]
> Signed-off-by: Atish Patra
> Signed-off-by: Yash Shah
> ---
> drivers/pwm/Kconfig | 11 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-sifive.c | 345
> +++
> 3 files changed, 357 insertions(+)
> create mode 100644 drivers/pwm/pwm-sifive.c
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index a8f47df..4a61d1a 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -380,6 +380,17 @@ config PWM_SAMSUNG
> To compile this driver as a module, choose M here: the module
> will be called pwm-samsung.
>
> +config PWM_SIFIVE
> + tristate "SiFive PWM support"
> + depends on OF
> + depends on COMMON_CLK
> + depends on RISCV || COMPILE_TEST
> + help
> + Generic PWM framework driver for SiFive SoCs.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called pwm-sifive.
> +
> config PWM_SPEAR
> tristate "STMicroelectronics SPEAr PWM support"
> depends on PLAT_SPEAR
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 9c676a0..30089ca 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -37,6 +37,7 @@ obj-$(CONFIG_PWM_RCAR) += pwm-rcar.o
> obj-$(CONFIG_PWM_RENESAS_TPU)+= pwm-renesas-tpu.o
> obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o
> obj-$(CONFIG_PWM_SAMSUNG)+= pwm-samsung.o
> +obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o
> obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o
> obj-$(CONFIG_PWM_STI)+= pwm-sti.o
> obj-$(CONFIG_PWM_STM32) += pwm-stm32.o
> diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> new file mode 100644
> index 000..6679ec7
> --- /dev/null
> +++ b/drivers/pwm/pwm-sifive.c
> @@ -0,0 +1,345 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2017-2018 SiFive
> + * For SiFive's PWM IP block documentation please refer Chapter 14 of
> + * Reference Manual : https://static.dev.sifive.com/FU540-C000-v1.0.pdf
> + *
> + * Limitations:
> + * - When changing both duty cycle and period, we cannot prevent in
> + * software that the output might produce a period with mixed
> + * settings (new period length and old duty cycle).
> + * - The hardware cannot generate a 100% duty cycle.
> + * - The hardware generates only inverted output.
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +/* Register offsets */
> +#define PWM_SIFIVE_PWMCFG0x0
> +#define PWM_SIFIVE_PWMCOUNT 0x8
> +#define PWM_SIFIVE_PWMS 0x10
> +#define PWM_SIFIVE_PWMCMP0 0x20
> +
> +/* PWMCFG fields */
> +#define PWM_SIFIVE_PWMCFG_SCALE 0
> +#define PWM_SIFIVE_PWMCFG_STICKY 8
> +#define PWM_SIFIVE_PWMCFG_ZERO_CMP 9
> +#define PWM_SIFIVE_PWMCFG_DEGLITCH 10
> +#define PWM_SIFIVE_PWMCFG_EN_ALWAYS BIT(12)
> +#define PWM_SIFIVE_PWMCFG_EN_ONCE13
> +#define PWM_SIFIVE_PWMCFG_CENTER 16
> +#define PWM_SIFIVE_PWMCFG_GANG 24
> +#define PWM_SIFIVE_PWMCFG_IP 28
It's a bit inconsistent to have one of them use BIT and the others not.
For consistency please use BIT for all defines. (They are unused
anyhow.) For PWM_SIFIVE_PWMCFG_SCALE use:
#define PWM_SIFIVE_PWMCFG_SCALE GENMASK(3, 0)
and then FIELD_GET and FIELD_PREP to access the values.
> +/* PWM_SIFIVE_SIZE_PWMCMP is used to calculate offset for pwmcmpX registers
> */
> +#define PWM_SIFIVE_SIZE_PWMCMP 4
> +#define PWM_SIFIVE_CMPWIDTH 16
> +#define PWM_SIFIVE_DEFAULT_PERIOD1000
> +
> +struct pwm_sifive_ddata {
> + struct pwm_chip chip;
> + struct mutex lock; /* lock to protect user_count */
> + struct notifier_block notifier;
> + struct clk *clk;
> + void __iomem *regs;
> + unsigned int real_period;
> + int user_count;
> +};
> +
> +static inline
> +struct pwm_sifive_ddata *pwm_sifive_chip_to_ddata(struct pwm_chip *c)
> +{
> + return container_of(c, struct pwm_sifive_ddata, chip);
> +}
> +
> +static int pwm_sifive_request(struct pwm_chip *chip, struct pwm_device *dev)
> +{
> + struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> +
> + mutex_lock(>lock);
> + pwm->user_count++;
> + mutex_unlock(>lock);
> +
> + return 0;
> +}
> +
> +static void pwm_sifive_free(struct pwm_chip *chip, struct pwm_device *dev)
> +{
> + struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> +
> + mutex_lock(>lock);
> + pwm->user_count--;
> + mutex_unlock(>lock);
> +}
> +
> +static void pwm_sifive_update_clock(struct pwm_sifive_ddata *pwm,
> + unsigned long rate)
> +{
> + u32 val;
> +
