| Message ID | 1583230755-25986-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 Tue, Mar 03, 2020 at 06:19:15PM +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 | 34 ++++++++++++++++++++++++++++++---- > 1 file changed, 30 insertions(+), 4 deletions(-) > > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c > index b94e0d09c300..c64ecff6c550 100644 > --- a/drivers/pwm/pwm-mediatek.c > +++ b/drivers/pwm/pwm-mediatek.c > @@ -121,8 +121,11 @@ 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; > + /* The source clock is divided by 2^clkdiv or iff the clksel bit > + * is set by (2^clkdiv*1625) > + */ Please put the /* on it's own line. See https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting > + u32 clkdiv = 0, clksel = 0, cnt_period, cnt_duty, > + reg_width = PWMDWIDTH, reg_thres = PWMTHRES; > u64 resolution; > int ret; > > @@ -133,12 +136,30 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm, > > /* Using resolution in picosecond gets accuracy higher */ > resolution = (u64)NSEC_PER_SEC * 1000; > + /* Calculate resolution based on current clock frequency */ > do_div(resolution, clk_get_rate(pc->clk_pwms[pwm->hwpwm])); > - > + /* Using resolution to calculate cnt_period which represents > + * the effective range of the PWM period counter > + */ > cnt_period = DIV_ROUND_CLOSEST_ULL((u64)period_ns * 1000, resolution); The existing code is rather bad. A better approach to calulate cnt_period (with a single division and higher accuracy): cnt_period = DIV_ROUND_CLOSEST_ULL((u64)period_ns * clk_get_rate(..), NSEC_PER_SEC); > while (cnt_period > 8191) { > + /* Using clkdiv to reduce clock frequency and calculate > + * new resolution based on new clock speed > + */ > resolution *= 2; > clkdiv++; > + if (clkdiv > PWM_CLK_DIV_MAX && !clksel) { > + /* Using clksel to divide the pwm source clock by > + * an additional 1625, and recalculate new clkdiv > + * and resolution > + */ > + clksel = 1; > + clkdiv = 0; > + resolution = (u64)NSEC_PER_SEC * 1000 * 1625; > + do_div(resolution, > + clk_get_rate(pc->clk_pwms[pwm->hwpwm])); > + } > + /* Calculate cnt_period based on resolution */ > cnt_period = DIV_ROUND_CLOSEST_ULL((u64)period_ns * 1000, > resolution); > } > @@ -158,8 +179,13 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm, > reg_thres = PWM45THRES_FIXUP; > } > > + /* Calculate cnt_duty based on resolution */ > cnt_duty = DIV_ROUND_CLOSEST_ULL((u64)duty_ns * 1000, resolution); Using resolution as divisor is bad here, too, so the way to calculate cnt_duty should be changed accordingly. I think if the driver is simplified first, maybe even getting rid of the while loop, your change will get considerably easier, too. > - 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); Best regards Uwe
On 03/03/2020 11:19, 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. Is this a fix? In this case please provide a Fixes tag with the commit ID which introduced the bug. Thanks Matthias > > Signed-off-by: Sam Shih <sam.shih@mediatek.com> > --- > drivers/pwm/pwm-mediatek.c | 34 ++++++++++++++++++++++++++++++---- > 1 file changed, 30 insertions(+), 4 deletions(-) > > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c > index b94e0d09c300..c64ecff6c550 100644 > --- a/drivers/pwm/pwm-mediatek.c > +++ b/drivers/pwm/pwm-mediatek.c > @@ -121,8 +121,11 @@ 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; > + /* The source clock is divided by 2^clkdiv or iff the clksel bit > + * is set by (2^clkdiv*1625) > + */ > + u32 clkdiv = 0, clksel = 0, cnt_period, cnt_duty, > + reg_width = PWMDWIDTH, reg_thres = PWMTHRES; > u64 resolution; > int ret; > > @@ -133,12 +136,30 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm, > > /* Using resolution in picosecond gets accuracy higher */ > resolution = (u64)NSEC_PER_SEC * 1000; > + /* Calculate resolution based on current clock frequency */ > do_div(resolution, clk_get_rate(pc->clk_pwms[pwm->hwpwm])); > - > + /* Using resolution to calculate cnt_period which represents > + * the effective range of the PWM period counter > + */ > cnt_period = DIV_ROUND_CLOSEST_ULL((u64)period_ns * 1000, resolution); > while (cnt_period > 8191) { > + /* Using clkdiv to reduce clock frequency and calculate > + * new resolution based on new clock speed > + */ > resolution *= 2; > clkdiv++; > + if (clkdiv > PWM_CLK_DIV_MAX && !clksel) { > + /* Using clksel to divide the pwm source clock by > + * an additional 1625, and recalculate new clkdiv > + * and resolution > + */ > + clksel = 1; > + clkdiv = 0; > + resolution = (u64)NSEC_PER_SEC * 1000 * 1625; > + do_div(resolution, > + clk_get_rate(pc->clk_pwms[pwm->hwpwm])); > + } > + /* Calculate cnt_period based on resolution */ > cnt_period = DIV_ROUND_CLOSEST_ULL((u64)period_ns * 1000, > resolution); > } > @@ -158,8 +179,13 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm, > reg_thres = PWM45THRES_FIXUP; > } > > + /* Calculate cnt_duty based on resolution */ > 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); > >
Hello, On Sat, Mar 07, 2020 at 10:28:36PM +0100, Matthias Brugger wrote: > On 03/03/2020 11:19, 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. > > Is this a fix? In this case please provide a Fixes tag with the commit ID which > introduced the bug. I'd say it qualifies as a fix if without it a request with a long period returns success but isn't properly implemented. Otherwise it's only a new feature. Best regards Uwe
Hello, On Sun, 2020-03-08 at 21:18 +0100, Uwe Kleine-König wrote: > Hello, > > On Sat, Mar 07, 2020 at 10:28:36PM +0100, Matthias Brugger wrote: > > On 03/03/2020 11:19, 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. > > > > Is this a fix? In this case please provide a Fixes tag with the commit ID which > > introduced the bug. > > I'd say it qualifies as a fix if without it a request with a long period > returns success but isn't properly implemented. Otherwise it's only a > new feature. > It's only a new feature. Without this patch, pwm_mediatek_config return -EINVAL when receive a long period request. I will send v3 to reply Uwe's comment. Best Regards, Sam Shih
diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c index b94e0d09c300..c64ecff6c550 100644 --- a/drivers/pwm/pwm-mediatek.c +++ b/drivers/pwm/pwm-mediatek.c @@ -121,8 +121,11 @@ 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; + /* The source clock is divided by 2^clkdiv or iff the clksel bit + * is set by (2^clkdiv*1625) + */ + u32 clkdiv = 0, clksel = 0, cnt_period, cnt_duty, + reg_width = PWMDWIDTH, reg_thres = PWMTHRES; u64 resolution; int ret; @@ -133,12 +136,30 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm, /* Using resolution in picosecond gets accuracy higher */ resolution = (u64)NSEC_PER_SEC * 1000; + /* Calculate resolution based on current clock frequency */ do_div(resolution, clk_get_rate(pc->clk_pwms[pwm->hwpwm])); - + /* Using resolution to calculate cnt_period which represents + * the effective range of the PWM period counter + */ cnt_period = DIV_ROUND_CLOSEST_ULL((u64)period_ns * 1000, resolution); while (cnt_period > 8191) { + /* Using clkdiv to reduce clock frequency and calculate + * new resolution based on new clock speed + */ resolution *= 2; clkdiv++; + if (clkdiv > PWM_CLK_DIV_MAX && !clksel) { + /* Using clksel to divide the pwm source clock by + * an additional 1625, and recalculate new clkdiv + * and resolution + */ + clksel = 1; + clkdiv = 0; + resolution = (u64)NSEC_PER_SEC * 1000 * 1625; + do_div(resolution, + clk_get_rate(pc->clk_pwms[pwm->hwpwm])); + } + /* Calculate cnt_period based on resolution */ cnt_period = DIV_ROUND_CLOSEST_ULL((u64)period_ns * 1000, resolution); } @@ -158,8 +179,13 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm, reg_thres = PWM45THRES_FIXUP; } + /* Calculate cnt_duty based on resolution */ 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 | 34 ++++++++++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 4 deletions(-)