| Message ID | 1582789610-23133-2-git-send-email-sam.shih@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pwm: mediatek: add longer period support | expand |
On Thu, Feb 27, 2020 at 03:46:50PM +0800, Sam Shih wrote: > The pwm clock source could be divided by 1625 with PWM_CON > BIT(3) setting in mediatek hardware. > > This patch add support for longer pwm period configuration, > which allowing blinking LEDs via pwm interface. > > Signed-off-by: Sam Shih <sam.shih@mediatek.com> > --- > drivers/pwm/pwm-mediatek.c | 21 +++++++++++++++++---- > 1 file changed, 17 insertions(+), 4 deletions(-) > > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c > index b94e0d09c300..9af309bea01a 100644 > --- a/drivers/pwm/pwm-mediatek.c > +++ b/drivers/pwm/pwm-mediatek.c > @@ -121,8 +121,8 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm, > int duty_ns, int period_ns) > { > struct pwm_mediatek_chip *pc = to_pwm_mediatek_chip(chip); > - u32 clkdiv = 0, cnt_period, cnt_duty, reg_width = PWMDWIDTH, > - reg_thres = PWMTHRES; > + u32 clkdiv = 0, clksel = 0, cnt_period, cnt_duty, > + reg_width = PWMDWIDTH, reg_thres = PWMTHRES; > u64 resolution; > int ret; > Adding some more context: > @@ -139,11 +139,20 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm, > while (cnt_period > 8191) { > resolution *= 2; > clkdiv++; > cnt_period = DIV_ROUND_CLOSEST_ULL((u64)period_ns * 1000, > resolution); > + if (clkdiv > PWM_CLK_DIV_MAX && !clksel) { > + clksel = 1; > + clkdiv = 0; > + resolution = (u64)NSEC_PER_SEC * 1000 * 1625; > + do_div(resolution, > + clk_get_rate(pc->clk_pwms[pwm->hwpwm])); > + cnt_period = DIV_ROUND_CLOSEST_ULL( > + (u64)period_ns * 1000, resolution); The assignment is a repetition from just above the if. Maybe just put it once after this if block? > + } > } > > - if (clkdiv > PWM_CLK_DIV_MAX) { > + if (clkdiv > PWM_CLK_DIV_MAX && clksel) { Is this change actually relevant? If the while loop that starts at line 139 is never run (because cnt_period is <= 8191) clkdiv is zero and so the condition is false with and without "&& clksel". If however the while loop is entered and clkdiv becomes bigger than PWM_CLK_DIV_MAX clksel is 1 and the "&& clksel" doesn't make a difference, too. The code is hard to follow, I wonder if this could be cleaned up with some comments added that explain the hardware details enough to be able to actually understand the code without having the hardware reference manual handy. > pwm_mediatek_clk_disable(chip, pwm); > dev_err(chip->dev, "period %d not supported\n", period_ns); > return -EINVAL; > @@ -159,7 +168,11 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm, > } > > cnt_duty = DIV_ROUND_CLOSEST_ULL((u64)duty_ns * 1000, resolution); > - pwm_mediatek_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | clkdiv); > + if (clksel) > + pwm_mediatek_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | BIT(3) | > + clkdiv); > + else > + pwm_mediatek_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | clkdiv); > pwm_mediatek_writel(pc, pwm->hwpwm, reg_width, cnt_period); > pwm_mediatek_writel(pc, pwm->hwpwm, reg_thres, cnt_duty); > > -- > 2.17.1 Best regards Uwe
On Thu, 2020-02-27 at 09:04 +0100, Uwe Kleine-König wrote: On Thu, Feb 27, 2020 at 03:46:50PM +0800, Sam Shih wrote: > > The pwm clock source could be divided by 1625 with PWM_CON > > BIT(3) setting in mediatek hardware. > > > > This patch add support for longer pwm period configuration, > > which allowing blinking LEDs via pwm interface. > > > > Signed-off-by: Sam Shih <sam.shih@mediatek.com> > > --- > > drivers/pwm/pwm-mediatek.c | 21 +++++++++++++++++---- > > 1 file changed, 17 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c > > index b94e0d09c300..9af309bea01a 100644 > > --- a/drivers/pwm/pwm-mediatek.c > > +++ b/drivers/pwm/pwm-mediatek.c > > @@ -121,8 +121,8 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm, > > int duty_ns, int period_ns) > > { > > struct pwm_mediatek_chip *pc = to_pwm_mediatek_chip(chip); > > - u32 clkdiv = 0, cnt_period, cnt_duty, reg_width = PWMDWIDTH, > > - reg_thres = PWMTHRES; > > + u32 clkdiv = 0, clksel = 0, cnt_period, cnt_duty, > > + reg_width = PWMDWIDTH, reg_thres = PWMTHRES; > > u64 resolution; > > int ret; > > > Adding some more context: > + /* The pwm source clock can be divided by 2^clkdiv. When the clksel + * bit is set to 1, The final clock output needs to be divided by an + * extra 1625. + */ Is this ok ? > > @@ -139,11 +139,20 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm, > > while (cnt_period > 8191) { > > resolution *= 2; > > clkdiv++; > > cnt_period = DIV_ROUND_CLOSEST_ULL((u64)period_ns * 1000, > > resolution); > > + if (clkdiv > PWM_CLK_DIV_MAX && !clksel) { > > + clksel = 1; > > + clkdiv = 0; > > + resolution = (u64)NSEC_PER_SEC * 1000 * 1625; > > + do_div(resolution, > > + clk_get_rate(pc->clk_pwms[pwm->hwpwm])); > > + cnt_period = DIV_ROUND_CLOSEST_ULL( > > + (u64)period_ns * 1000, resolution); > > The assignment is a repetition from just above the if. Maybe just put it > once after this if block? The cnt_period represents the effective range of the PWM period counter, when we need changing the pwm output period to a longer value at the same clock frequency, we can setting a larger cnt_period, but the width of the cnt_peroid register is 12 bits, When the request period is too long, we need to divide the clock source and then recalculate cnt_period outputs the correct waveform. As mentioned above, when changing clkdiv, we need to recalculate cnt_period immediately. If the request period is very long (for example, LED blinking), clkdiv may be insufficient. In this case, we will use clksel to divide the pwm source clock by an additional 1625, and recalculate clkdiv and cnt_period. I don't think we can just place assignments after the if block. > > > + } > > } > > > > - if (clkdiv > PWM_CLK_DIV_MAX) { > > + if (clkdiv > PWM_CLK_DIV_MAX && clksel) { > > Is this change actually relevant? If the while loop that starts at line > 139 is never run (because cnt_period is <= 8191) clkdiv is zero and so > the condition is false with and without "&& clksel". If however the > while loop is entered and clkdiv becomes bigger than PWM_CLK_DIV_MAX > clksel is 1 and the "&& clksel" doesn't make a difference, too. > You are right, I will remove this. > The code is hard to follow, I wonder if this could be cleaned up with > some comments added that explain the hardware details enough to be able > to actually understand the code without having the hardware reference > manual handy. > Is it sufficient to add some context into comment like the response of the second question? > > pwm_mediatek_clk_disable(chip, pwm); > > dev_err(chip->dev, "period %d not supported\n", period_ns); > > return -EINVAL; > > @@ -159,7 +168,11 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm, > > } > > > > cnt_duty = DIV_ROUND_CLOSEST_ULL((u64)duty_ns * 1000, resolution); > > - pwm_mediatek_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | clkdiv); > > + if (clksel) > > + pwm_mediatek_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | BIT(3) | > > + clkdiv); > > + else > > + pwm_mediatek_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | clkdiv); > > pwm_mediatek_writel(pc, pwm->hwpwm, reg_width, cnt_period); > > pwm_mediatek_writel(pc, pwm->hwpwm, reg_thres, cnt_duty); > > > > -- > > 2.17.1 > > Best regards > Uwe > > Best Regards, Sam
Hello, On Thu, Feb 27, 2020 at 05:59:50PM +0800, Sam Shih wrote: > On Thu, 2020-02-27 at 09:04 +0100, Uwe Kleine-König wrote: > On Thu, Feb 27, 2020 at 03:46:50PM +0800, Sam Shih wrote: > > > The pwm clock source could be divided by 1625 with PWM_CON > > > BIT(3) setting in mediatek hardware. > > > > > > This patch add support for longer pwm period configuration, > > > which allowing blinking LEDs via pwm interface. > > > > > > Signed-off-by: Sam Shih <sam.shih@mediatek.com> > > > --- > > > drivers/pwm/pwm-mediatek.c | 21 +++++++++++++++++---- > > > 1 file changed, 17 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c > > > index b94e0d09c300..9af309bea01a 100644 > > > --- a/drivers/pwm/pwm-mediatek.c > > > +++ b/drivers/pwm/pwm-mediatek.c > > > @@ -121,8 +121,8 @@ static int pwm_mediatek_config(struct pwm_chip > *chip, struct pwm_device *pwm, > > > int duty_ns, int period_ns) > > > { > > > struct pwm_mediatek_chip *pc = to_pwm_mediatek_chip(chip); > > > - u32 clkdiv = 0, cnt_period, cnt_duty, reg_width = PWMDWIDTH, > > > - reg_thres = PWMTHRES; > > > + u32 clkdiv = 0, clksel = 0, cnt_period, cnt_duty, > > > + reg_width = PWMDWIDTH, reg_thres = PWMTHRES; > > > u64 resolution; > > > int ret; > > > > > Adding some more context: > > > > + /* The pwm source clock can be divided by 2^clkdiv. When the clksel + > * bit is set to 1, The final clock output needs to be divided by an + * > extra 1625. > + */ I'd write: The source clock is divided by 2^clkdiv or iff the clksel bit is set by 2^clkdiv + 1625. > > Is this ok ? > > > > > @@ -139,11 +139,20 @@ static int pwm_mediatek_config(struct pwm_chip > *chip, struct pwm_device *pwm, > > > while (cnt_period > 8191) { > > > resolution *= 2; > > > clkdiv++; > > > cnt_period = DIV_ROUND_CLOSEST_ULL((u64)period_ns * 1000, > > > resolution); > > > + if (clkdiv > PWM_CLK_DIV_MAX && !clksel) { > > > + clksel = 1; > > > + clkdiv = 0; > > > + resolution = (u64)NSEC_PER_SEC * 1000 * 1625; > > > + do_div(resolution, > > > + clk_get_rate(pc->clk_pwms[pwm->hwpwm])); > > > + cnt_period = DIV_ROUND_CLOSEST_ULL( > > > + (u64)period_ns * 1000, resolution); > > > > The assignment is a repetition from just above the if. Maybe just put > it > > once after this if block? > > The cnt_period represents the effective range of the PWM period counter, > when we need changing the pwm output period to a longer value at the > same clock frequency, we can setting a larger cnt_period, but the width > of the cnt_peroid register is 12 bits, > When the request period is too long, we need to divide the clock source > and then recalculate cnt_period outputs the correct waveform. > As mentioned above, when changing clkdiv, we need to recalculate > cnt_period immediately. > > If the request period is very long (for example, LED blinking), clkdiv > may be insufficient. > In this case, we will use clksel to divide the pwm source clock by an > additional 1625, and recalculate clkdiv and cnt_period. > > I don't think we can just place assignments after the if block. I didn't care enough to read your reasoning and retry to convince you with mine: With your patch you have: cnt_period = someexpression; if (somecondition) { ... cnt_period = someexpression; } As somecondition doesn't make use of cnt_period this is equivalent to: if (somecondition) { ... } cnt_period = someexpression; isn't it? > > The code is hard to follow, I wonder if this could be cleaned up with > > some comments added that explain the hardware details enough to be able > > to actually understand the code without having the hardware reference > > manual handy. > > Is it sufficient to add some context into comment like the response of > the second question? I didn't check but I wouldn't be surprised if the code is more complicated than necessary. If you don't see something to simplify, go for adding an explanation as suggested and I will take a look in a quiet moment. Not sure I already pointed out that having a link to a publicly available reference manual in the driver's header is useful. If there is such a manual, please add a link there. Your benefit is that you simplify others to improve your driver. Best regards Uwe
Hello, > > On Thu, Feb 27, 2020 at 05:59:50PM +0800, Sam Shih wrote: > > On Thu, 2020-02-27 at 09:04 +0100, Uwe Kleine-König wrote: > > On Thu, Feb 27, 2020 at 03:46:50PM +0800, Sam Shih wrote: > > > > The pwm clock source could be divided by 1625 with PWM_CON > > > > BIT(3) setting in mediatek hardware. > > > > > > > > This patch add support for longer pwm period configuration, > > > > which allowing blinking LEDs via pwm interface. > > > > > > > > Signed-off-by: Sam Shih <sam.shih@mediatek.com> > > > > --- > > > > drivers/pwm/pwm-mediatek.c | 21 +++++++++++++++++---- > > > > 1 file changed, 17 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c > > > > index b94e0d09c300..9af309bea01a 100644 > > > > --- a/drivers/pwm/pwm-mediatek.c > > > > +++ b/drivers/pwm/pwm-mediatek.c > > > > @@ -121,8 +121,8 @@ static int pwm_mediatek_config(struct pwm_chip > > *chip, struct pwm_device *pwm, > > > > int duty_ns, int period_ns) > > > > { > > > > struct pwm_mediatek_chip *pc = to_pwm_mediatek_chip(chip); > > > > - u32 clkdiv = 0, cnt_period, cnt_duty, reg_width = PWMDWIDTH, > > > > - reg_thres = PWMTHRES; > > > > + u32 clkdiv = 0, clksel = 0, cnt_period, cnt_duty, > > > > + reg_width = PWMDWIDTH, reg_thres = PWMTHRES; > > > > u64 resolution; > > > > int ret; > > > > > > > Adding some more context: > > > > > > > + /* The pwm source clock can be divided by 2^clkdiv. When the clksel + > > * bit is set to 1, The final clock output needs to be divided by an + * > > extra 1625. > > + */ > > I'd write: > > The source clock is divided by 2^clkdiv or iff the clksel bit is set by > 2^clkdiv + 1625. > Great, the comment is short and clear. But maybe change “2^clkdiv + 1625” to “the product of 2^clkdiv and 1625” is clearer ? > > > > Is this ok ? > > > > > > > > @@ -139,11 +139,20 @@ static int pwm_mediatek_config(struct pwm_chip > > *chip, struct pwm_device *pwm, > > > > while (cnt_period > 8191) { > > > > resolution *= 2; > > > > clkdiv++; > > > > cnt_period = DIV_ROUND_CLOSEST_ULL((u64)period_ns * 1000, > > > > resolution); > > > > + if (clkdiv > PWM_CLK_DIV_MAX && !clksel) { > > > > + clksel = 1; > > > > + clkdiv = 0; > > > > + resolution = (u64)NSEC_PER_SEC * 1000 * 1625; > > > > + do_div(resolution, > > > > + clk_get_rate(pc->clk_pwms[pwm->hwpwm])); > > > > + cnt_period = DIV_ROUND_CLOSEST_ULL( > > > > + (u64)period_ns * 1000, resolution); > > > > > > The assignment is a repetition from just above the if. Maybe just put > > it > > > once after this if block? > > > > The cnt_period represents the effective range of the PWM period counter, > > when we need changing the pwm output period to a longer value at the > > same clock frequency, we can setting a larger cnt_period, but the width > > of the cnt_peroid register is 12 bits, > > When the request period is too long, we need to divide the clock source > > and then recalculate cnt_period outputs the correct waveform. > > As mentioned above, when changing clkdiv, we need to recalculate > > cnt_period immediately. > > > > If the request period is very long (for example, LED blinking), clkdiv > > may be insufficient. > > In this case, we will use clksel to divide the pwm source clock by an > > additional 1625, and recalculate clkdiv and cnt_period. > > > > I don't think we can just place assignments after the if block. > > I didn't care enough to read your reasoning and retry to convince you > with mine: > > With your patch you have: > > cnt_period = someexpression; > > if (somecondition) { > ... > cnt_period = someexpression; > } > > As somecondition doesn't make use of cnt_period this is equivalent to: > > if (somecondition) { > ... > } > cnt_period = someexpression; > > isn't it? > Yes, you're right, I misunderstood. Your code clearly reminded me. I just want to explain that the re-calculation of the cnt_period is important. However, after reading your code, I think the program logic will not be break and the duplicates can be removed. > > > The code is hard to follow, I wonder if this could be cleaned up with > > > some comments added that explain the hardware details enough to be able > > > to actually understand the code without having the hardware reference > > > manual handy. > > > > Is it sufficient to add some context into comment like the response of > > the second question? > > I didn't check but I wouldn't be surprised if the code is more > complicated than necessary. If you don't see something to simplify, go > for adding an explanation as suggested and I will take a look in a quiet > moment. > I will send v2 patch to remove the repetition of “cnt_period = someexpression”, and add some comment to clksel and cnt_period. > Not sure I already pointed out that having a link to a publicly > available reference manual in the driver's header is useful. If there is > such a manual, please add a link there. Your benefit is that you > simplify others to improve your driver. > > > Best regards > Uwe > > Best Regards. Sam
Hello Sam, On Thu, Feb 27, 2020 at 08:27:07PM +0800, Sam Shih wrote: > > > > > > + /* The pwm source clock can be divided by 2^clkdiv. When the clksel + > > > * bit is set to 1, The final clock output needs to be divided by an + * > > > extra 1625. > > > + */ > > > > I'd write: > > > > The source clock is divided by 2^clkdiv or iff the clksel bit is set by > > 2^clkdiv + 1625. > > > > Great, the comment is short and clear. > But maybe change “2^clkdiv + 1625” to “the product of 2^clkdiv and 1625” > is clearer ? Writing a formula in words isn't helpful. If my formula was wrong use the right one. I wrote 2^clkdiv + 1625 (which implicitly means (2^clkdiv) + 1625), if this is wrong write 2^clkdiv * 1625 or whatever is the right one then. And use parenthesis if you doubt clearness. Best regards Uwe
diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c index b94e0d09c300..9af309bea01a 100644 --- a/drivers/pwm/pwm-mediatek.c +++ b/drivers/pwm/pwm-mediatek.c @@ -121,8 +121,8 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm, int duty_ns, int period_ns) { struct pwm_mediatek_chip *pc = to_pwm_mediatek_chip(chip); - u32 clkdiv = 0, cnt_period, cnt_duty, reg_width = PWMDWIDTH, - reg_thres = PWMTHRES; + u32 clkdiv = 0, clksel = 0, cnt_period, cnt_duty, + reg_width = PWMDWIDTH, reg_thres = PWMTHRES; u64 resolution; int ret; @@ -141,9 +141,18 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm, clkdiv++; cnt_period = DIV_ROUND_CLOSEST_ULL((u64)period_ns * 1000, resolution); + if (clkdiv > PWM_CLK_DIV_MAX && !clksel) { + clksel = 1; + clkdiv = 0; + resolution = (u64)NSEC_PER_SEC * 1000 * 1625; + do_div(resolution, + clk_get_rate(pc->clk_pwms[pwm->hwpwm])); + cnt_period = DIV_ROUND_CLOSEST_ULL( + (u64)period_ns * 1000, resolution); + } } - if (clkdiv > PWM_CLK_DIV_MAX) { + if (clkdiv > PWM_CLK_DIV_MAX && clksel) { pwm_mediatek_clk_disable(chip, pwm); dev_err(chip->dev, "period %d not supported\n", period_ns); return -EINVAL; @@ -159,7 +168,11 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm, } cnt_duty = DIV_ROUND_CLOSEST_ULL((u64)duty_ns * 1000, resolution); - pwm_mediatek_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | clkdiv); + if (clksel) + pwm_mediatek_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | BIT(3) | + clkdiv); + else + pwm_mediatek_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | clkdiv); pwm_mediatek_writel(pc, pwm->hwpwm, reg_width, cnt_period); pwm_mediatek_writel(pc, pwm->hwpwm, reg_thres, cnt_duty);
The pwm clock source could be divided by 1625 with PWM_CON BIT(3) setting in mediatek hardware. This patch add support for longer pwm period configuration, which allowing blinking LEDs via pwm interface. Signed-off-by: Sam Shih <sam.shih@mediatek.com> --- drivers/pwm/pwm-mediatek.c | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-)