| Message ID | 20191103203334.10539-5-peron.clem@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for H6 PWM | expand |
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
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
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/ |
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
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/ |
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
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/ |
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);