[v2,4/7] pwm: sun4i: Add support to output source clock directly
diff mbox series

Message ID 20191103203334.10539-5-peron.clem@gmail.com
State New
Headers show
Series
  • Add support for H6 PWM
Related show

Commit Message

Clément Péron Nov. 3, 2019, 8:33 p.m. UTC
From: Jernej Skrabec <jernej.skrabec@siol.net>

PWM core has an option to bypass whole logic and output unchanged source
clock as PWM output. This is achieved by enabling bypass bit.

Note that when bypass is enabled, no other setting has any meaning, not
even enable bit.

This mode of operation is needed to achieve high enough frequency to
serve as clock source for AC200 chip, which is integrated into same
package as H6 SoC.

Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
 drivers/pwm/pwm-sun4i.c | 39 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

Comments

Clément Péron Nov. 3, 2019, 10:41 p.m. UTC | #1
Hi,

On Sun, 3 Nov 2019 at 23:30, kbuild test robot <lkp@intel.com> wrote:
>
> Hi "Clément,
>
> I love your patch! Yet something to improve:
>
> [auto build test ERROR on sunxi/sunxi/for-next]
> [also build test ERROR on v5.4-rc5 next-20191031]
> [if your patch is applied to the wrong git tree, please drop us a note to help
> improve the system. BTW, we also suggest to use '--base' option to specify the
> base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
>
> url:    https://github.com/0day-ci/linux/commits/Cl-ment-P-ron/Add-support-for-H6-PWM/20191104-043621
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/sunxi/linux.git sunxi/for-next
> config: riscv-allmodconfig (attached as .config)
> compiler: riscv64-linux-gcc (GCC) 7.4.0
> reproduce:
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         GCC_VERSION=7.4.0 make.cross ARCH=riscv
>
> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot <lkp@intel.com>
>
> All errors (new ones prefixed by >>):
>
>    drivers//pwm/pwm-sun4i.c: In function 'sun4i_pwm_get_state':
> >> drivers//pwm/pwm-sun4i.c:132:6: error: 'data' undeclared (first use in this function)
>          data->has_direct_mod_clk_output) {
>          ^~~~

Arg, bad last minute indent fix :
This should be "sun4i_pwm->data->has_direct_mod_clk_output"

Sorry for that,
Clément

>    drivers//pwm/pwm-sun4i.c:132:6: note: each undeclared identifier is reported only once for each function it appears in
>
> vim +/data +132 drivers//pwm/pwm-sun4i.c
>
>    112
>    113  static void sun4i_pwm_get_state(struct pwm_chip *chip,
>    114                                  struct pwm_device *pwm,
>    115                                  struct pwm_state *state)
>    116  {
>    117          struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip);
>    118          u64 clk_rate, tmp;
>    119          u32 val;
>    120          unsigned int prescaler;
>    121
>    122          clk_rate = clk_get_rate(sun4i_pwm->clk);
>    123
>    124          val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
>    125
>    126          /*
>    127           * PWM chapter in H6 manual has a diagram which explains that if bypass
>    128           * bit is set, no other setting has any meaning. Even more, experiment
>    129           * proved that also enable bit is ignored in this case.
>    130           */
>    131          if ((val & BIT_CH(PWM_BYPASS, pwm->hwpwm)) &&
>  > 132              data->has_direct_mod_clk_output) {
>    133                  state->period = DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, clk_rate);
>    134                  state->duty_cycle = state->period / 2;
>    135                  state->polarity = PWM_POLARITY_NORMAL;
>    136                  state->enabled = true;
>    137                  return;
>    138          }
>    139
>    140          if ((PWM_REG_PRESCAL(val, pwm->hwpwm) == PWM_PRESCAL_MASK) &&
>    141              sun4i_pwm->data->has_prescaler_bypass)
>    142                  prescaler = 1;
>    143          else
>    144                  prescaler = prescaler_table[PWM_REG_PRESCAL(val, pwm->hwpwm)];
>    145
>    146          if (prescaler == 0)
>    147                  return;
>    148
>    149          if (val & BIT_CH(PWM_ACT_STATE, pwm->hwpwm))
>    150                  state->polarity = PWM_POLARITY_NORMAL;
>    151          else
>    152                  state->polarity = PWM_POLARITY_INVERSED;
>    153
>    154          if ((val & BIT_CH(PWM_CLK_GATING | PWM_EN, pwm->hwpwm)) ==
>    155              BIT_CH(PWM_CLK_GATING | PWM_EN, pwm->hwpwm))
>    156                  state->enabled = true;
>    157          else
>    158                  state->enabled = false;
>    159
>    160          val = sun4i_pwm_readl(sun4i_pwm, PWM_CH_PRD(pwm->hwpwm));
>    161
>    162          tmp = prescaler * NSEC_PER_SEC * PWM_REG_DTY(val);
>    163          state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, clk_rate);
>    164
>    165          tmp = prescaler * NSEC_PER_SEC * PWM_REG_PRD(val);
>    166          state->period = DIV_ROUND_CLOSEST_ULL(tmp, clk_rate);
>    167  }
>    168
>
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Uwe Kleine-König Nov. 4, 2019, 8:38 a.m. UTC | #2
On Sun, Nov 03, 2019 at 09:33:31PM +0100, Clément Péron wrote:
> From: Jernej Skrabec <jernej.skrabec@siol.net>
> 
> PWM core has an option to bypass whole logic and output unchanged source
> clock as PWM output. This is achieved by enabling bypass bit.
> 
> Note that when bypass is enabled, no other setting has any meaning, not
> even enable bit.
> 
> This mode of operation is needed to achieve high enough frequency to
> serve as clock source for AC200 chip, which is integrated into same
> package as H6 SoC.

I think the , should be dropped.

> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> Signed-off-by: Clément Péron <peron.clem@gmail.com>
> ---
>  drivers/pwm/pwm-sun4i.c | 39 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> index b5e7ac364f59..2441574674d9 100644
> --- a/drivers/pwm/pwm-sun4i.c
> +++ b/drivers/pwm/pwm-sun4i.c
> @@ -3,6 +3,10 @@
>   * Driver for Allwinner sun4i Pulse Width Modulation Controller
>   *
>   * Copyright (C) 2014 Alexandre Belloni <alexandre.belloni@free-electrons.com>
> + *
> + * Limitations:
> + * - When outputing the source clock directly, the PWM logic will be bypassed
> + *   and the currently running period is not guaranted to be completed

Typo: guaranted  -> guaranteed

>   */
>  
>  #include <linux/bitops.h>
> @@ -73,6 +77,7 @@ static const u32 prescaler_table[] = {
>  
>  struct sun4i_pwm_data {
>  	bool has_prescaler_bypass;
> +	bool has_direct_mod_clk_output;
>  	unsigned int npwm;
>  };
>  
> @@ -118,6 +123,20 @@ static void sun4i_pwm_get_state(struct pwm_chip *chip,
>  
>  	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
>  
> +	/*
> +	 * PWM chapter in H6 manual has a diagram which explains that if bypass
> +	 * bit is set, no other setting has any meaning. Even more, experiment
> +	 * proved that also enable bit is ignored in this case.
> +	 */
> +	if ((val & BIT_CH(PWM_BYPASS, pwm->hwpwm)) &&
> +	    data->has_direct_mod_clk_output) {
> +		state->period = DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, clk_rate);
> +		state->duty_cycle = state->period / 2;
> +		state->polarity = PWM_POLARITY_NORMAL;
> +		state->enabled = true;
> +		return;
> +	}

Not sure how the rest of sun4i_pwm_get_state behaves, but I would prefer
to let .get_state() round up which together with .apply_state() rounding
down yields sound behaviour.

> +
>  	if ((PWM_REG_PRESCAL(val, pwm->hwpwm) == PWM_PRESCAL_MASK) &&
>  	    sun4i_pwm->data->has_prescaler_bypass)
>  		prescaler = 1;
> @@ -203,7 +222,8 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  {
>  	struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip);
>  	struct pwm_state cstate;
> -	u32 ctrl;
> +	u32 ctrl, clk_rate;
> +	bool bypass;
>  	int ret;
>  	unsigned int delay_us;
>  	unsigned long now;
> @@ -218,6 +238,16 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  		}
>  	}
>  
> +	/*
> +	 * Although it would make much more sense to check for bypass in
> +	 * sun4i_pwm_calculate(), value of bypass bit also depends on "enabled".
> +	 * Period is allowed to be rounded up or down.
> +	 */
> +	clk_rate = clk_get_rate(sun4i_pwm->clk);
> +	bypass = ((state->period * clk_rate >= NSEC_PER_SEC &&
> +		   state->period * clk_rate < NSEC_PER_SEC + clk_rate) &&
> +		  state->enabled);

I guess the compiler is smart enough here, but checking for
state->enabled is cheaper than the other checks, so putting this at the
start of the expression seems sensible.

The comment doesn't match the code. You don't round up state->period.
(This is good, please fix the comment.) I think dropping the check

	state->period * clk_rate < NSEC_PER_SEC + clk_rate

would be fine, too.

I'd like to have a check for

	state->duty_cycle * clk_rate >= NSEC_PER_SEC / 2 &&
	state->duty_cycle * clk_rate < NSEC_PER_SEC

here. If this isn't true rather disable the PWM or output a 100% duty
cycle with a larger period.

> +
>  	spin_lock(&sun4i_pwm->ctrl_lock);
>  	ctrl = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
>  

Best regards
Uwe
Clément Péron Nov. 4, 2019, 9:28 p.m. UTC | #3
Hi Uwe

On Mon, 4 Nov 2019 at 09:38, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> On Sun, Nov 03, 2019 at 09:33:31PM +0100, Clément Péron wrote:
> > From: Jernej Skrabec <jernej.skrabec@siol.net>
> >
> > PWM core has an option to bypass whole logic and output unchanged source
> > clock as PWM output. This is achieved by enabling bypass bit.
> >
> > Note that when bypass is enabled, no other setting has any meaning, not
> > even enable bit.
> >
> > This mode of operation is needed to achieve high enough frequency to
> > serve as clock source for AC200 chip, which is integrated into same
> > package as H6 SoC.
>
> I think the , should be dropped.
>
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > ---
> >  drivers/pwm/pwm-sun4i.c | 39 ++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 38 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > index b5e7ac364f59..2441574674d9 100644
> > --- a/drivers/pwm/pwm-sun4i.c
> > +++ b/drivers/pwm/pwm-sun4i.c
> > @@ -3,6 +3,10 @@
> >   * Driver for Allwinner sun4i Pulse Width Modulation Controller
> >   *
> >   * Copyright (C) 2014 Alexandre Belloni <alexandre.belloni@free-electrons.com>
> > + *
> > + * Limitations:
> > + * - When outputing the source clock directly, the PWM logic will be bypassed
> > + *   and the currently running period is not guaranted to be completed
>
> Typo: guaranted  -> guaranteed
>
> >   */
> >
> >  #include <linux/bitops.h>
> > @@ -73,6 +77,7 @@ static const u32 prescaler_table[] = {
> >
> >  struct sun4i_pwm_data {
> >       bool has_prescaler_bypass;
> > +     bool has_direct_mod_clk_output;
> >       unsigned int npwm;
> >  };
> >
> > @@ -118,6 +123,20 @@ static void sun4i_pwm_get_state(struct pwm_chip *chip,
> >
> >       val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
> >
> > +     /*
> > +      * PWM chapter in H6 manual has a diagram which explains that if bypass
> > +      * bit is set, no other setting has any meaning. Even more, experiment
> > +      * proved that also enable bit is ignored in this case.
> > +      */
> > +     if ((val & BIT_CH(PWM_BYPASS, pwm->hwpwm)) &&
> > +         data->has_direct_mod_clk_output) {
> > +             state->period = DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, clk_rate);
> > +             state->duty_cycle = state->period / 2;
> > +             state->polarity = PWM_POLARITY_NORMAL;
> > +             state->enabled = true;
> > +             return;
> > +     }
>
> Not sure how the rest of sun4i_pwm_get_state behaves, but I would prefer
> to let .get_state() round up which together with .apply_state() rounding
> down yields sound behaviour.
Ok
>
> > +
> >       if ((PWM_REG_PRESCAL(val, pwm->hwpwm) == PWM_PRESCAL_MASK) &&
> >           sun4i_pwm->data->has_prescaler_bypass)
> >               prescaler = 1;
> > @@ -203,7 +222,8 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >  {
> >       struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip);
> >       struct pwm_state cstate;
> > -     u32 ctrl;
> > +     u32 ctrl, clk_rate;
> > +     bool bypass;
> >       int ret;
> >       unsigned int delay_us;
> >       unsigned long now;
> > @@ -218,6 +238,16 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >               }
> >       }
> >
> > +     /*
> > +      * Although it would make much more sense to check for bypass in
> > +      * sun4i_pwm_calculate(), value of bypass bit also depends on "enabled".
> > +      * Period is allowed to be rounded up or down.
> > +      */
> > +     clk_rate = clk_get_rate(sun4i_pwm->clk);
> > +     bypass = ((state->period * clk_rate >= NSEC_PER_SEC &&
> > +                state->period * clk_rate < NSEC_PER_SEC + clk_rate) &&
> > +               state->enabled);
>
> I guess the compiler is smart enough here, but checking for
> state->enabled is cheaper than the other checks, so putting this at the
> start of the expression seems sensible.
>
> The comment doesn't match the code. You don't round up state->period.
> (This is good, please fix the comment.) I think dropping the check
>
>         state->period * clk_rate < NSEC_PER_SEC + clk_rate
>
> would be fine, too.
Ok

>
> I'd like to have a check for
>
>         state->duty_cycle * clk_rate >= NSEC_PER_SEC / 2 &&
>         state->duty_cycle * clk_rate < NSEC_PER_SEC
>
> here. If this isn't true rather disable the PWM or output a 100% duty
> cycle with a larger period.

Why not just having the duty_cycle is 50% only ?
state->duty_cycle * 2 == state->period;

Regards,
Clement

>
> > +
> >       spin_lock(&sun4i_pwm->ctrl_lock);
> >       ctrl = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
> >
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Uwe Kleine-König Nov. 5, 2019, 7:29 a.m. UTC | #4
Hi Clément,

On Mon, Nov 04, 2019 at 10:28:54PM +0100, Clément Péron wrote:
> On Mon, 4 Nov 2019 at 09:38, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Sun, Nov 03, 2019 at 09:33:31PM +0100, Clément Péron wrote:
> > > From: Jernej Skrabec <jernej.skrabec@siol.net>
> > >
> > > PWM core has an option to bypass whole logic and output unchanged source
> > > clock as PWM output. This is achieved by enabling bypass bit.
> > >
> > > Note that when bypass is enabled, no other setting has any meaning, not
> > > even enable bit.
> > >
> > > This mode of operation is needed to achieve high enough frequency to
> > > serve as clock source for AC200 chip, which is integrated into same
> > > package as H6 SoC.
> >
> > I think the , should be dropped.
> >
> > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > > ---
> > >  drivers/pwm/pwm-sun4i.c | 39 ++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 38 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > > index b5e7ac364f59..2441574674d9 100644
> > > --- a/drivers/pwm/pwm-sun4i.c
> > > +++ b/drivers/pwm/pwm-sun4i.c
> > > @@ -3,6 +3,10 @@
> > >   * Driver for Allwinner sun4i Pulse Width Modulation Controller
> > >   *
> > >   * Copyright (C) 2014 Alexandre Belloni <alexandre.belloni@free-electrons.com>
> > > + *
> > > + * Limitations:
> > > + * - When outputing the source clock directly, the PWM logic will be bypassed
> > > + *   and the currently running period is not guaranted to be completed
> >
> > Typo: guaranted  -> guaranteed
> >
> > >   */
> > >
> > >  #include <linux/bitops.h>
> > > @@ -73,6 +77,7 @@ static const u32 prescaler_table[] = {
> > >
> > >  struct sun4i_pwm_data {
> > >       bool has_prescaler_bypass;
> > > +     bool has_direct_mod_clk_output;
> > >       unsigned int npwm;
> > >  };
> > >
> > > @@ -118,6 +123,20 @@ static void sun4i_pwm_get_state(struct pwm_chip *chip,
> > >
> > >       val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
> > >
> > > +     /*
> > > +      * PWM chapter in H6 manual has a diagram which explains that if bypass
> > > +      * bit is set, no other setting has any meaning. Even more, experiment
> > > +      * proved that also enable bit is ignored in this case.
> > > +      */
> > > +     if ((val & BIT_CH(PWM_BYPASS, pwm->hwpwm)) &&
> > > +         data->has_direct_mod_clk_output) {
> > > +             state->period = DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, clk_rate);
> > > +             state->duty_cycle = state->period / 2;
> > > +             state->polarity = PWM_POLARITY_NORMAL;
> > > +             state->enabled = true;
> > > +             return;
> > > +     }
> >
> > Not sure how the rest of sun4i_pwm_get_state behaves, but I would prefer
> > to let .get_state() round up which together with .apply_state() rounding
> > down yields sound behaviour.
> Ok
> >
> > > +
> > >       if ((PWM_REG_PRESCAL(val, pwm->hwpwm) == PWM_PRESCAL_MASK) &&
> > >           sun4i_pwm->data->has_prescaler_bypass)
> > >               prescaler = 1;
> > > @@ -203,7 +222,8 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > >  {
> > >       struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip);
> > >       struct pwm_state cstate;
> > > -     u32 ctrl;
> > > +     u32 ctrl, clk_rate;
> > > +     bool bypass;
> > >       int ret;
> > >       unsigned int delay_us;
> > >       unsigned long now;
> > > @@ -218,6 +238,16 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > >               }
> > >       }
> > >
> > > +     /*
> > > +      * Although it would make much more sense to check for bypass in
> > > +      * sun4i_pwm_calculate(), value of bypass bit also depends on "enabled".
> > > +      * Period is allowed to be rounded up or down.
> > > +      */
> > > +     clk_rate = clk_get_rate(sun4i_pwm->clk);
> > > +     bypass = ((state->period * clk_rate >= NSEC_PER_SEC &&
> > > +                state->period * clk_rate < NSEC_PER_SEC + clk_rate) &&
> > > +               state->enabled);
> >
> > I guess the compiler is smart enough here, but checking for
> > state->enabled is cheaper than the other checks, so putting this at the
> > start of the expression seems sensible.
> >
> > The comment doesn't match the code. You don't round up state->period.
> > (This is good, please fix the comment.) I think dropping the check
> >
> >         state->period * clk_rate < NSEC_PER_SEC + clk_rate
> >
> > would be fine, too.
> Ok
> 
> >
> > I'd like to have a check for
> >
> >         state->duty_cycle * clk_rate >= NSEC_PER_SEC / 2 &&
> >         state->duty_cycle * clk_rate < NSEC_PER_SEC
> >
> > here. If this isn't true rather disable the PWM or output a 100% duty
> > cycle with a larger period.
> 
> Why not just having the duty_cycle is 50% only ?
> state->duty_cycle * 2 == state->period;

Yeah, for the bypass case you can only provide a 50% duty cycle. The
problem you have to address is that you cannot rely on your consumer to
request only 50% duty cycles. So you have to implement some behaviour if
your consumer requests period = 1 / clk_rate and 20% duty cycle.

Where I want to get the pwm framework as a whole is to let lowlevel
drivers round down both duty_cycle and period to the next possible values
in their .apply callback to be able to provide a more uniform behaviour
for consumers. So here this would mean:

 - 1 / clk_rate <= state->period < smallest value without bypass &&
   0 <= state->duty_cycle < state->period / 2
   	=> provide a constant 0

 - 1 / clk_rate <= state->period < smallest value without bypass &&
   state->period / 2 <= state->duty_cycle < state->period
   	=> use bypass mode providing 50% duty cycle

 - 1 / clk_rate <= state->period < smallest value without bypass &&
   state->period == state->duty_cycle
   	=> provide a constant 1

Best regards
Uwe
Clément Péron Nov. 5, 2019, 12:58 p.m. UTC | #5
Hi Uwe,

On Tue, 5 Nov 2019 at 08:29, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hi Clément,
>
> On Mon, Nov 04, 2019 at 10:28:54PM +0100, Clément Péron wrote:
> > On Mon, 4 Nov 2019 at 09:38, Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Sun, Nov 03, 2019 at 09:33:31PM +0100, Clément Péron wrote:
> > > > From: Jernej Skrabec <jernej.skrabec@siol.net>
> > > >
> > > > PWM core has an option to bypass whole logic and output unchanged source
> > > > clock as PWM output. This is achieved by enabling bypass bit.
> > > >
> > > > Note that when bypass is enabled, no other setting has any meaning, not
> > > > even enable bit.
> > > >
> > > > This mode of operation is needed to achieve high enough frequency to
> > > > serve as clock source for AC200 chip, which is integrated into same
> > > > package as H6 SoC.
> > >
> > > I think the , should be dropped.
> > >
> > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > > > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > > > ---
> > > >  drivers/pwm/pwm-sun4i.c | 39 ++++++++++++++++++++++++++++++++++++++-
> > > >  1 file changed, 38 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > > > index b5e7ac364f59..2441574674d9 100644
> > > > --- a/drivers/pwm/pwm-sun4i.c
> > > > +++ b/drivers/pwm/pwm-sun4i.c
> > > > @@ -3,6 +3,10 @@
> > > >   * Driver for Allwinner sun4i Pulse Width Modulation Controller
> > > >   *
> > > >   * Copyright (C) 2014 Alexandre Belloni <alexandre.belloni@free-electrons.com>
> > > > + *
> > > > + * Limitations:
> > > > + * - When outputing the source clock directly, the PWM logic will be bypassed
> > > > + *   and the currently running period is not guaranted to be completed
> > >
> > > Typo: guaranted  -> guaranteed
> > >
> > > >   */
> > > >
> > > >  #include <linux/bitops.h>
> > > > @@ -73,6 +77,7 @@ static const u32 prescaler_table[] = {
> > > >
> > > >  struct sun4i_pwm_data {
> > > >       bool has_prescaler_bypass;
> > > > +     bool has_direct_mod_clk_output;
> > > >       unsigned int npwm;
> > > >  };
> > > >
> > > > @@ -118,6 +123,20 @@ static void sun4i_pwm_get_state(struct pwm_chip *chip,
> > > >
> > > >       val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
> > > >
> > > > +     /*
> > > > +      * PWM chapter in H6 manual has a diagram which explains that if bypass
> > > > +      * bit is set, no other setting has any meaning. Even more, experiment
> > > > +      * proved that also enable bit is ignored in this case.
> > > > +      */
> > > > +     if ((val & BIT_CH(PWM_BYPASS, pwm->hwpwm)) &&
> > > > +         data->has_direct_mod_clk_output) {
> > > > +             state->period = DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, clk_rate);
> > > > +             state->duty_cycle = state->period / 2;
> > > > +             state->polarity = PWM_POLARITY_NORMAL;
> > > > +             state->enabled = true;
> > > > +             return;
> > > > +     }
> > >
> > > Not sure how the rest of sun4i_pwm_get_state behaves, but I would prefer
> > > to let .get_state() round up which together with .apply_state() rounding
> > > down yields sound behaviour.
> > Ok
> > >
> > > > +
> > > >       if ((PWM_REG_PRESCAL(val, pwm->hwpwm) == PWM_PRESCAL_MASK) &&
> > > >           sun4i_pwm->data->has_prescaler_bypass)
> > > >               prescaler = 1;
> > > > @@ -203,7 +222,8 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > >  {
> > > >       struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip);
> > > >       struct pwm_state cstate;
> > > > -     u32 ctrl;
> > > > +     u32 ctrl, clk_rate;
> > > > +     bool bypass;
> > > >       int ret;
> > > >       unsigned int delay_us;
> > > >       unsigned long now;
> > > > @@ -218,6 +238,16 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > >               }
> > > >       }
> > > >
> > > > +     /*
> > > > +      * Although it would make much more sense to check for bypass in
> > > > +      * sun4i_pwm_calculate(), value of bypass bit also depends on "enabled".
> > > > +      * Period is allowed to be rounded up or down.
> > > > +      */
> > > > +     clk_rate = clk_get_rate(sun4i_pwm->clk);
> > > > +     bypass = ((state->period * clk_rate >= NSEC_PER_SEC &&
> > > > +                state->period * clk_rate < NSEC_PER_SEC + clk_rate) &&
> > > > +               state->enabled);
> > >
> > > I guess the compiler is smart enough here, but checking for
> > > state->enabled is cheaper than the other checks, so putting this at the
> > > start of the expression seems sensible.
> > >
> > > The comment doesn't match the code. You don't round up state->period.
> > > (This is good, please fix the comment.) I think dropping the check
> > >
> > >         state->period * clk_rate < NSEC_PER_SEC + clk_rate
> > >
> > > would be fine, too.
> > Ok
> >
> > >
> > > I'd like to have a check for
> > >
> > >         state->duty_cycle * clk_rate >= NSEC_PER_SEC / 2 &&
> > >         state->duty_cycle * clk_rate < NSEC_PER_SEC
> > >
> > > here. If this isn't true rather disable the PWM or output a 100% duty
> > > cycle with a larger period.
> >
> > Why not just having the duty_cycle is 50% only ?
> > state->duty_cycle * 2 == state->period;
>
> Yeah, for the bypass case you can only provide a 50% duty cycle. The
> problem you have to address is that you cannot rely on your consumer to
> request only 50% duty cycles. So you have to implement some behaviour if
> your consumer requests period = 1 / clk_rate and 20% duty cycle.

So you request to add a new patch in this series for fixing the actual
PWM behavior at corner case?

This series just want to add a new device and a new bypass
functionality and I can't measure the output of PWM and testing it
properly.
Can this be done in another patch/series ?

Regards,
Clément

>
> Where I want to get the pwm framework as a whole is to let lowlevel
> drivers round down both duty_cycle and period to the next possible values
> in their .apply callback to be able to provide a more uniform behaviour
> for consumers. So here this would mean:
>
>  - 1 / clk_rate <= state->period < smallest value without bypass &&
>    0 <= state->duty_cycle < state->period / 2
>         => provide a constant 0
>
>  - 1 / clk_rate <= state->period < smallest value without bypass &&
>    state->period / 2 <= state->duty_cycle < state->period
>         => use bypass mode providing 50% duty cycle
>
>  - 1 / clk_rate <= state->period < smallest value without bypass &&
>    state->period == state->duty_cycle
>         => provide a constant 1
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Uwe Kleine-König Nov. 5, 2019, 1:12 p.m. UTC | #6
Hello Clément,

On Tue, Nov 05, 2019 at 01:58:31PM +0100, Clément Péron wrote:
> This series just want to add a new device and a new bypass
> functionality and I can't measure the output of PWM and testing it
> properly.
> Can this be done in another patch/series ?

I'm fine if you implement the bypass stuff with this logic and keep the
other stuff as is.

Best regards
Uwe
Clément Péron Nov. 5, 2019, 1:12 p.m. UTC | #7
On Tue, 5 Nov 2019 at 14:12, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello Clément,
>
> On Tue, Nov 05, 2019 at 01:58:31PM +0100, Clément Péron wrote:
> > This series just want to add a new device and a new bypass
> > functionality and I can't measure the output of PWM and testing it
> > properly.
> > Can this be done in another patch/series ?
>
> I'm fine if you implement the bypass stuff with this logic and keep the
> other stuff as is.

Thanks,
Clément

>
> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Patch
diff mbox series

diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index b5e7ac364f59..2441574674d9 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -3,6 +3,10 @@ 
  * Driver for Allwinner sun4i Pulse Width Modulation Controller
  *
  * Copyright (C) 2014 Alexandre Belloni <alexandre.belloni@free-electrons.com>
+ *
+ * Limitations:
+ * - When outputing the source clock directly, the PWM logic will be bypassed
+ *   and the currently running period is not guaranted to be completed
  */
 
 #include <linux/bitops.h>
@@ -73,6 +77,7 @@  static const u32 prescaler_table[] = {
 
 struct sun4i_pwm_data {
 	bool has_prescaler_bypass;
+	bool has_direct_mod_clk_output;
 	unsigned int npwm;
 };
 
@@ -118,6 +123,20 @@  static void sun4i_pwm_get_state(struct pwm_chip *chip,
 
 	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
 
+	/*
+	 * PWM chapter in H6 manual has a diagram which explains that if bypass
+	 * bit is set, no other setting has any meaning. Even more, experiment
+	 * proved that also enable bit is ignored in this case.
+	 */
+	if ((val & BIT_CH(PWM_BYPASS, pwm->hwpwm)) &&
+	    data->has_direct_mod_clk_output) {
+		state->period = DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, clk_rate);
+		state->duty_cycle = state->period / 2;
+		state->polarity = PWM_POLARITY_NORMAL;
+		state->enabled = true;
+		return;
+	}
+
 	if ((PWM_REG_PRESCAL(val, pwm->hwpwm) == PWM_PRESCAL_MASK) &&
 	    sun4i_pwm->data->has_prescaler_bypass)
 		prescaler = 1;
@@ -203,7 +222,8 @@  static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 {
 	struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip);
 	struct pwm_state cstate;
-	u32 ctrl;
+	u32 ctrl, clk_rate;
+	bool bypass;
 	int ret;
 	unsigned int delay_us;
 	unsigned long now;
@@ -218,6 +238,16 @@  static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		}
 	}
 
+	/*
+	 * Although it would make much more sense to check for bypass in
+	 * sun4i_pwm_calculate(), value of bypass bit also depends on "enabled".
+	 * Period is allowed to be rounded up or down.
+	 */
+	clk_rate = clk_get_rate(sun4i_pwm->clk);
+	bypass = ((state->period * clk_rate >= NSEC_PER_SEC &&
+		   state->period * clk_rate < NSEC_PER_SEC + clk_rate) &&
+		  state->enabled);
+
 	spin_lock(&sun4i_pwm->ctrl_lock);
 	ctrl = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
 
@@ -265,6 +295,13 @@  static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		ctrl &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
 	}
 
+	if (sun4i_pwm->data->has_direct_mod_clk_output) {
+		if (bypass)
+			ctrl |= BIT_CH(PWM_BYPASS, pwm->hwpwm);
+		else
+			ctrl &= ~BIT_CH(PWM_BYPASS, pwm->hwpwm);
+	}
+
 	sun4i_pwm_writel(sun4i_pwm, ctrl, PWM_CTRL_REG);
 
 	spin_unlock(&sun4i_pwm->ctrl_lock);