| Message ID | 1394655346-30048-3-git-send-email-tim.kryger@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 12, 2014 at 01:15:43PM -0700, Tim Kryger wrote: [...] > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index 22f2f28..1fd42af 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -62,6 +62,16 @@ config PWM_ATMEL_TCB > To compile this driver as a module, choose M here: the module > will be called pwm-atmel-tcb. > > +config PWM_BCM_KONA > + tristate "Kona PWM support" > + depends on ARCH_BCM_MOBILE > + default y No "default y", please. > diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c [...] > +static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan) > +{ > + unsigned long value = readl(kp->base + PWM_CONTROL_OFFSET); > + > + /* > + * New duty and period settings are only reflected in the PWM output > + * after a rising edge of the enable bit. When smooth bit is set, the > + * new settings are delayed until the prior PWM period has completed. > + * Furthermore, if the smooth bit is set, the PWM continues to output a > + * waveform based on the old settings during the time that the enable > + * bit is low. Otherwise the output is a constant high signal while > + * the enable bit is low. > + */ > + > + /* clear enable bit but set smooth bit to maintain old output */ > + value |= (1 << PWM_CONTROL_SMOOTH_SHIFT(chan)); There's no need for the parentheses here. > + value &= ~(1 << PWM_CONTROL_ENABLE_SHIFT(chan)); > + writel(value, kp->base + PWM_CONTROL_OFFSET); > + > + /* set enable bit and clear smooth bit to apply new settings */ > + value &= ~(1 << PWM_CONTROL_SMOOTH_SHIFT(chan)); > + value |= (1 << PWM_CONTROL_ENABLE_SHIFT(chan)); Nor here. > + writel(value, kp->base + PWM_CONTROL_OFFSET); I'm curious about how this work. The above writes the control register once with smooth set and enable cleared, then immediately rewrites it again with smooth cleared and enable set. Are these writes somehow queued in hardware, so that the subsequent write becomes active only after the current period? > +static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm, > + int duty_ns, int period_ns) Please align arguments on subsequent lines with those on the first line. > +{ > + struct kona_pwmc *kp = dev_get_drvdata(chip->dev); The proper way to do this would be upcasting using container_of(). Better yet, define a static inline function that wraps container_of(), just like very other driver does. > + u64 val, div, clk_rate; > + unsigned long prescale = PRESCALE_MIN, pc, dc; > + unsigned int value, chan = pwm->hwpwm; > + > + /* > + * Find period count, duty count and prescale to suit duty_ns and > + * period_ns. This is done according to formulas described below: > + * > + * period_ns = 10^9 * (PRESCALE + 1) * PC / PWM_CLK_RATE > + * duty_ns = 10^9 * (PRESCALE + 1) * DC / PWM_CLK_RATE > + * > + * PC = (PWM_CLK_RATE * period_ns) / (10^9 * (PRESCALE + 1)) > + * DC = (PWM_CLK_RATE * duty_ns) / (10^9 * (PRESCALE + 1)) > + */ > + > + clk_rate = clk_get_rate(kp->clk); "rate" is 50% shorter and would do equally well. > + > + /* There is polarity support in HW but it is easier to manage in SW */ > + if (pwm->polarity == PWM_POLARITY_INVERSED) > + duty_ns = period_ns - duty_ns; No, this is wrong. You're not actually changing the *polarity* here. Also I think this is missing a pair of clk_prepare_enable() and clk_disable_unprepare(). > +static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm, > + enum pwm_polarity polarity) > +{ > + /* > + * The framework only allows the polarity to be changed when a PWM is > + * disabled so no immediate action is required here. When a channel is > + * enabled, the polarity gets handled as part of the re-config step. > + */ > + > + return 0; > +} See above. If you don't want to implement the hardware support for inversed polarity, then simply don't implement this. > +static int kona_pwmc_enable(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + struct kona_pwmc *kp = dev_get_drvdata(chip->dev); > + int ret; > + > + /* > + * The PWM framework does not clear the enable bit in the flags if an > + * error is returned from a PWM driver's enable function so it must be > + * cleared here if any trouble is encountered. > + */ > + > + ret = clk_prepare_enable(kp->clk); > + if (ret < 0) { > + dev_err(chip->dev, "failed to enable clock: %d\n", ret); > + clear_bit(PWMF_ENABLED, &pwm->flags); You're not supposed to touch these. This is a bug in the core, and it should be fixed in the core. > + return ret; > + } > + > + ret = kona_pwmc_config(chip, pwm, pwm->duty_cycle, pwm->period); > + if (ret < 0) { > + clk_disable_unprepare(kp->clk); > + clear_bit(PWMF_ENABLED, &pwm->flags); > + return ret; > + } Why are you doing this? .config() should be setting everything up according to the given duty cycle and period and .enable() should only be enabling a specific channel. Please don't conflate the two. > +static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + struct kona_pwmc *kp = dev_get_drvdata(chip->dev); > + unsigned int chan = pwm->hwpwm; > + > + /* > + * The "enable" bits in the control register only affect when settings > + * start to take effect so the only real way to disable the PWM output > + * is to program a zero duty cycle. > + */ What's wrong about waiting for the settings to take effect? There's nothing about disable that says it should happen *immediately*. > + > + writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan)); > + kona_pwmc_apply_settings(kp, chan); > + > + /* > + * When the PWM clock is disabled, the output is pegged high or low > + * depending on its state at that instant. To guarantee that the new > + * settings have taken effect and the output is low a delay of 400ns is > + * required. > + */ > + > + ndelay(400); Where does this number come from? > +static int kona_pwmc_probe(struct platform_device *pdev) > +{ > + struct kona_pwmc *kp; > + struct resource *res; > + unsigned int chan, value; [...] > + /* Set smooth mode, push/pull, and normal polarity for all channels */ > + for (value = 0, chan = 0; chan < kp->chip.npwm; chan++) { Can't you initialize value to 0 when you declare it? That way the for loop becomes much more idiomatic. > + value |= (1 << PWM_CONTROL_SMOOTH_SHIFT(chan)); > + value |= (1 << PWM_CONTROL_TYPE_SHIFT(chan)); > + value |= (1 << PWM_CONTROL_POLARITY_SHIFT(chan)); > + } > + writel(value, kp->base + PWM_CONTROL_OFFSET); I'd prefer a blank line between the above two for readability. > +static struct platform_driver kona_pwmc_driver = { > + Gratuitous blank line. > + .driver = { > + .name = "bcm-kona-pwm", > + .of_match_table = bcm_kona_pwmc_dt, > + }, > + .probe = kona_pwmc_probe, > + .remove = kona_pwmc_remove, > +}; > + And here. > +module_platform_driver(kona_pwmc_driver); > + > +MODULE_AUTHOR("Broadcom Corporation <bcm-kernel-feedback-list@broadcom.com>"); > +MODULE_AUTHOR("Tim Kryger <tkryger@broadcom.com>"); > +MODULE_DESCRIPTION("Driver for Kona PWM controller"); Perhaps "Broadcom Kona PWM"? Thierry
On Tue, Mar 18, 2014 at 2:52 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Wed, Mar 12, 2014 at 01:15:43PM -0700, Tim Kryger wrote: > [...] >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig >> index 22f2f28..1fd42af 100644 >> --- a/drivers/pwm/Kconfig >> +++ b/drivers/pwm/Kconfig >> @@ -62,6 +62,16 @@ config PWM_ATMEL_TCB >> To compile this driver as a module, choose M here: the module >> will be called pwm-atmel-tcb. >> >> +config PWM_BCM_KONA >> + tristate "Kona PWM support" >> + depends on ARCH_BCM_MOBILE >> + default y > > No "default y", please. Even though it depends on ARCH_BCM_MOBILE? It seems like a reasonable default. These SoCs target mobile phones which almost always have a backlight controlled by the PWM. > >> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c > [...] >> +static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan) >> +{ >> + unsigned long value = readl(kp->base + PWM_CONTROL_OFFSET); >> + >> + /* >> + * New duty and period settings are only reflected in the PWM output >> + * after a rising edge of the enable bit. When smooth bit is set, the >> + * new settings are delayed until the prior PWM period has completed. >> + * Furthermore, if the smooth bit is set, the PWM continues to output a >> + * waveform based on the old settings during the time that the enable >> + * bit is low. Otherwise the output is a constant high signal while >> + * the enable bit is low. >> + */ >> + >> + /* clear enable bit but set smooth bit to maintain old output */ >> + value |= (1 << PWM_CONTROL_SMOOTH_SHIFT(chan)); > > There's no need for the parentheses here. Ok > >> + value &= ~(1 << PWM_CONTROL_ENABLE_SHIFT(chan)); >> + writel(value, kp->base + PWM_CONTROL_OFFSET); >> + >> + /* set enable bit and clear smooth bit to apply new settings */ >> + value &= ~(1 << PWM_CONTROL_SMOOTH_SHIFT(chan)); >> + value |= (1 << PWM_CONTROL_ENABLE_SHIFT(chan)); > > Nor here. Ok > >> + writel(value, kp->base + PWM_CONTROL_OFFSET); > > I'm curious about how this work. The above writes the control register > once with smooth set and enable cleared, then immediately rewrites it > again with smooth cleared and enable set. Are these writes somehow > queued in hardware, so that the subsequent write becomes active only > after the current period? It isn't exactly queuing up the writes but if smooth mode is set, it will wait to apply the new settings. > >> +static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm, >> + int duty_ns, int period_ns) > > Please align arguments on subsequent lines with those on the first line. Sure > >> +{ >> + struct kona_pwmc *kp = dev_get_drvdata(chip->dev); > > The proper way to do this would be upcasting using container_of(). > Better yet, define a static inline function that wraps container_of(), > just like very other driver does. > I will convert to use this approach (except in kona_pwmc_remove) >> + u64 val, div, clk_rate; >> + unsigned long prescale = PRESCALE_MIN, pc, dc; >> + unsigned int value, chan = pwm->hwpwm; >> + >> + /* >> + * Find period count, duty count and prescale to suit duty_ns and >> + * period_ns. This is done according to formulas described below: >> + * >> + * period_ns = 10^9 * (PRESCALE + 1) * PC / PWM_CLK_RATE >> + * duty_ns = 10^9 * (PRESCALE + 1) * DC / PWM_CLK_RATE >> + * >> + * PC = (PWM_CLK_RATE * period_ns) / (10^9 * (PRESCALE + 1)) >> + * DC = (PWM_CLK_RATE * duty_ns) / (10^9 * (PRESCALE + 1)) >> + */ >> + >> + clk_rate = clk_get_rate(kp->clk); > > "rate" is 50% shorter and would do equally well. That works for me. > >> + >> + /* There is polarity support in HW but it is easier to manage in SW */ >> + if (pwm->polarity == PWM_POLARITY_INVERSED) >> + duty_ns = period_ns - duty_ns; > > No, this is wrong. You're not actually changing the *polarity* here. Please elaborate. I don't understand what is wrong here. Does polarity influence the output while a PWM is disabled? > > Also I think this is missing a pair of clk_prepare_enable() and > clk_disable_unprepare(). There is no issue here since the hardware registers are only touched in kona_pwmc_config if the channel was already enabled. > >> +static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm, >> + enum pwm_polarity polarity) >> +{ >> + /* >> + * The framework only allows the polarity to be changed when a PWM is >> + * disabled so no immediate action is required here. When a channel is >> + * enabled, the polarity gets handled as part of the re-config step. >> + */ >> + >> + return 0; >> +} > > See above. If you don't want to implement the hardware support for > inversed polarity, then simply don't implement this. I had originally planned to omit polarity support but because it affects the binding (which is treated as ABI), it wouldn't be possible to add it in later without defining a new compatible string. > >> +static int kona_pwmc_enable(struct pwm_chip *chip, struct pwm_device *pwm) >> +{ >> + struct kona_pwmc *kp = dev_get_drvdata(chip->dev); >> + int ret; >> + >> + /* >> + * The PWM framework does not clear the enable bit in the flags if an >> + * error is returned from a PWM driver's enable function so it must be >> + * cleared here if any trouble is encountered. >> + */ >> + >> + ret = clk_prepare_enable(kp->clk); >> + if (ret < 0) { >> + dev_err(chip->dev, "failed to enable clock: %d\n", ret); >> + clear_bit(PWMF_ENABLED, &pwm->flags); > > You're not supposed to touch these. This is a bug in the core, and it > should be fixed in the core. Okay. I'll drop the clear_bit lines from this patch. > >> + return ret; >> + } >> + >> + ret = kona_pwmc_config(chip, pwm, pwm->duty_cycle, pwm->period); >> + if (ret < 0) { >> + clk_disable_unprepare(kp->clk); >> + clear_bit(PWMF_ENABLED, &pwm->flags); >> + return ret; >> + } > > Why are you doing this? .config() should be setting everything up > according to the given duty cycle and period and .enable() should only > be enabling a specific channel. Please don't conflate the two. The hardware only supports a configure operation. Thus disable has to be simulated by configuring zero duty. During an enable operation we have to program the last configuration. > >> +static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm) >> +{ >> + struct kona_pwmc *kp = dev_get_drvdata(chip->dev); >> + unsigned int chan = pwm->hwpwm; >> + >> + /* >> + * The "enable" bits in the control register only affect when settings >> + * start to take effect so the only real way to disable the PWM output >> + * is to program a zero duty cycle. >> + */ > > What's wrong about waiting for the settings to take effect? There's > nothing about disable that says it should happen *immediately*. The current code does wait for the settings to take effect. Can you clarify what you mean? > >> + >> + writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan)); >> + kona_pwmc_apply_settings(kp, chan); >> + >> + /* >> + * When the PWM clock is disabled, the output is pegged high or low >> + * depending on its state at that instant. To guarantee that the new >> + * settings have taken effect and the output is low a delay of 400ns is >> + * required. >> + */ >> + >> + ndelay(400); > > Where does this number come from? Hardware documentation > >> +static int kona_pwmc_probe(struct platform_device *pdev) >> +{ >> + struct kona_pwmc *kp; >> + struct resource *res; >> + unsigned int chan, value; > [...] >> + /* Set smooth mode, push/pull, and normal polarity for all channels */ >> + for (value = 0, chan = 0; chan < kp->chip.npwm; chan++) { > > Can't you initialize value to 0 when you declare it? That way the for > loop becomes much more idiomatic. Sure. > >> + value |= (1 << PWM_CONTROL_SMOOTH_SHIFT(chan)); >> + value |= (1 << PWM_CONTROL_TYPE_SHIFT(chan)); >> + value |= (1 << PWM_CONTROL_POLARITY_SHIFT(chan)); >> + } >> + writel(value, kp->base + PWM_CONTROL_OFFSET); > > I'd prefer a blank line between the above two for readability. Ok > >> +static struct platform_driver kona_pwmc_driver = { >> + > > Gratuitous blank line. Ok > >> + .driver = { >> + .name = "bcm-kona-pwm", >> + .of_match_table = bcm_kona_pwmc_dt, >> + }, >> + .probe = kona_pwmc_probe, >> + .remove = kona_pwmc_remove, >> +}; >> + > > And here. Ok > >> +module_platform_driver(kona_pwmc_driver); >> + >> +MODULE_AUTHOR("Broadcom Corporation <bcm-kernel-feedback-list@broadcom.com>"); >> +MODULE_AUTHOR("Tim Kryger <tkryger@broadcom.com>"); >> +MODULE_DESCRIPTION("Driver for Kona PWM controller"); > > Perhaps "Broadcom Kona PWM"? Sure Thanks for the review. -Tim
On Tue, Mar 18, 2014 at 4:47 PM, Tim Kryger <tim.kryger@linaro.org> wrote: > On Tue, Mar 18, 2014 at 2:52 PM, Thierry Reding > <thierry.reding@gmail.com> wrote: >> On Wed, Mar 12, 2014 at 01:15:43PM -0700, Tim Kryger wrote: >>> + >>> + /* There is polarity support in HW but it is easier to manage in SW */ >>> + if (pwm->polarity == PWM_POLARITY_INVERSED) >>> + duty_ns = period_ns - duty_ns; >> >> No, this is wrong. You're not actually changing the *polarity* here. > > Please elaborate. I don't understand what is wrong here. > > Does polarity influence the output while a PWM is disabled? > >>> +static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm, >>> + enum pwm_polarity polarity) >>> +{ >>> + /* >>> + * The framework only allows the polarity to be changed when a PWM is >>> + * disabled so no immediate action is required here. When a channel is >>> + * enabled, the polarity gets handled as part of the re-config step. >>> + */ >>> + >>> + return 0; >>> +} >> >> See above. If you don't want to implement the hardware support for >> inversed polarity, then simply don't implement this. > > I had originally planned to omit polarity support but because it > affects the binding (which is treated as ABI), it wouldn't be possible > to add it in later without defining a new compatible string. I would like to get this right but it occurred to me that there may be a way to defer the implementation of this feature without disrupting the binding. Would it be acceptable to continue using #pwm-cells = <3> and of_pwm_xlate_with_flags but return -EINVAL from kona_pwmc_set_polarity if PWM_POLARITY_INVERSED is specified?
On Tue, Mar 18, 2014 at 06:06:03PM -0700, Tim Kryger wrote: > On Tue, Mar 18, 2014 at 4:47 PM, Tim Kryger <tim.kryger@linaro.org> wrote: > > On Tue, Mar 18, 2014 at 2:52 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > >> On Wed, Mar 12, 2014 at 01:15:43PM -0700, Tim Kryger wrote: [...] > >>> +static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm, > >>> + enum pwm_polarity polarity) > >>> +{ > >>> + /* > >>> + * The framework only allows the polarity to be changed when a PWM is > >>> + * disabled so no immediate action is required here. When a channel is > >>> + * enabled, the polarity gets handled as part of the re-config step. > >>> + */ > >>> + > >>> + return 0; > >>> +} > >> > >> See above. If you don't want to implement the hardware support for > >> inversed polarity, then simply don't implement this. > > > > I had originally planned to omit polarity support but because it > > affects the binding (which is treated as ABI), it wouldn't be possible > > to add it in later without defining a new compatible string. > > I would like to get this right but it occurred to me that there may be > a way to defer the implementation of this feature without disrupting > the binding. > > Would it be acceptable to continue using #pwm-cells = <3> and > of_pwm_xlate_with_flags but return -EINVAL from kona_pwmc_set_polarity > if PWM_POLARITY_INVERSED is specified? This was recently discussed for the pwm-imx driver. And you can easily support #pwm-cells = <2> and #pwm-cells = <3> with the same binding. So you could start with #pwm-cells = <2>, leaving out .set_polarity() and implement it later on, extending the binding in a backwards-compatible way to support the polarity flag. Thierry
On Tue, Mar 18, 2014 at 04:47:36PM -0700, Tim Kryger wrote: > On Tue, Mar 18, 2014 at 2:52 PM, Thierry Reding > <thierry.reding@gmail.com> wrote: > > On Wed, Mar 12, 2014 at 01:15:43PM -0700, Tim Kryger wrote: > > [...] > >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > >> index 22f2f28..1fd42af 100644 > >> --- a/drivers/pwm/Kconfig > >> +++ b/drivers/pwm/Kconfig > >> @@ -62,6 +62,16 @@ config PWM_ATMEL_TCB > >> To compile this driver as a module, choose M here: the module > >> will be called pwm-atmel-tcb. > >> > >> +config PWM_BCM_KONA > >> + tristate "Kona PWM support" > >> + depends on ARCH_BCM_MOBILE > >> + default y > > > > No "default y", please. > > Even though it depends on ARCH_BCM_MOBILE? It seems like a reasonable > default. Well, that's only true until somebody decides to turn that dependency into something like: depends on ARCH_BCM_MOBILE || COMPILE_TEST > These SoCs target mobile phones which almost always have a backlight > controlled by the PWM. And you can easily turn it on in bcm_defconfig and/or multi_v7_defconfig if you want it enabled "by default". > >> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c [...] > >> + > >> + /* There is polarity support in HW but it is easier to manage in SW */ > >> + if (pwm->polarity == PWM_POLARITY_INVERSED) > >> + duty_ns = period_ns - duty_ns; > > > > No, this is wrong. You're not actually changing the *polarity* here. > > Please elaborate. I don't understand what is wrong here. Perhaps the comment for enum pwm_polarity in include/linux/pwm.h makes this more obvious. What you do here is invert the duty cycle, rather than the polarity. While it is true that the result is the same for things like LEDs or backlight (because the signal power remains the same), but there's a slight difference to what the PWM signal looks like. > Does polarity influence the output while a PWM is disabled? Yes, there is apparently hardware where the polarity causes the PWM pin to be 1 when the PWM is disabled. But that's really a separate issue. > >> +static int kona_pwmc_enable(struct pwm_chip *chip, struct pwm_device *pwm) > >> +{ > >> + struct kona_pwmc *kp = dev_get_drvdata(chip->dev); > >> + int ret; > >> + > >> + /* > >> + * The PWM framework does not clear the enable bit in the flags if an > >> + * error is returned from a PWM driver's enable function so it must be > >> + * cleared here if any trouble is encountered. > >> + */ > >> + > >> + ret = clk_prepare_enable(kp->clk); > >> + if (ret < 0) { > >> + dev_err(chip->dev, "failed to enable clock: %d\n", ret); > >> + clear_bit(PWMF_ENABLED, &pwm->flags); > > > > You're not supposed to touch these. This is a bug in the core, and it > > should be fixed in the core. > > Okay. I'll drop the clear_bit lines from this patch. I was sort of hoping you would post a patch which fixes this in the core instead of just dropping those lines here and ignoring the issue. That would've earned you bonus points. =) > >> + return ret; > >> + } > >> + > >> + ret = kona_pwmc_config(chip, pwm, pwm->duty_cycle, pwm->period); > >> + if (ret < 0) { > >> + clk_disable_unprepare(kp->clk); > >> + clear_bit(PWMF_ENABLED, &pwm->flags); > >> + return ret; > >> + } > > > > Why are you doing this? .config() should be setting everything up > > according to the given duty cycle and period and .enable() should only > > be enabling a specific channel. Please don't conflate the two. > > The hardware only supports a configure operation. > > Thus disable has to be simulated by configuring zero duty. > > During an enable operation we have to program the last configuration. My worry is that by calling one public function from another you'll be mixing contexts. For instance, with the PWMF_ENABLED issue you mention one solution to fix this in the core would be to delay setting the flag until the driver's .enable() implementation actually succeeded. If such an implementation was chosen, then your driver would be broken, because PWMF_ENABLED wouldn't be set when .enable() is called, and therefore calling the kona_pwmc_config() wouldn't have any effect. > >> +static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm) > >> +{ > >> + struct kona_pwmc *kp = dev_get_drvdata(chip->dev); > >> + unsigned int chan = pwm->hwpwm; > >> + > >> + /* > >> + * The "enable" bits in the control register only affect when settings > >> + * start to take effect so the only real way to disable the PWM output > >> + * is to program a zero duty cycle. > >> + */ > > > > What's wrong about waiting for the settings to take effect? There's > > nothing about disable that says it should happen *immediately*. > > The current code does wait for the settings to take effect. > > Can you clarify what you mean? Things are starting to get confusing here. Looking at the register definitions, there's PWM_CONTROL_ENABLE_SHIFT(chan), which I suspect controls whether or not a channel is enabled (if that's not the case then please add a comment that explains what it does). But a little further up you said that the hardware does only support a configure operation and not an enable/disable. The comment above further confuses me. What I read from it is that you can in fact disable a channel by clearing the "enable" bit in the control register. But the reason why you don't do it that way is because that change won't take effect until "settings start to take effect". So in order to disable the PWM immediately you resort to writing a 0 duty cycle. Perhaps I misunderstood, in which case it might be good to revise that comment to be more explicit or accurate. Thierry
On Thu, Mar 20, 2014 at 8:57 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Tue, Mar 18, 2014 at 04:47:36PM -0700, Tim Kryger wrote: > Perhaps the comment for enum pwm_polarity in include/linux/pwm.h makes > this more obvious. What you do here is invert the duty cycle, rather > than the polarity. While it is true that the result is the same for > things like LEDs or backlight (because the signal power remains the > same), but there's a slight difference to what the PWM signal looks > like. Thanks, I missed that comment before. >> Does polarity influence the output while a PWM is disabled? > > Yes, there is apparently hardware where the polarity causes the PWM pin > to be 1 when the PWM is disabled. But that's really a separate issue. Do you have a preference on how this should be handled? > Things are starting to get confusing here. Looking at the register > definitions, there's PWM_CONTROL_ENABLE_SHIFT(chan), which I suspect > controls whether or not a channel is enabled (if that's not the case > then please add a comment that explains what it does). I've tried to do this but the unfortunate name for these bits and their nuanced behavior makes it difficult. > > But a little further up you said that the hardware does only support a > configure operation and not an enable/disable. If you define disabled as zero duty output, then this is true. When the smooth bit is off and the "enable" bit is off output is 100% duty. > > The comment above further confuses me. What I read from it is that you > can in fact disable a channel by clearing the "enable" bit in the > control register. But the reason why you don't do it that way is because > that change won't take effect until "settings start to take effect". So > in order to disable the PWM immediately you resort to writing a 0 duty > cycle. Sorry if my comments were confusing. New settings are only applied on a rising edge of the "enable" bit. You should think of it more as a trigger bit than an enable. > > Perhaps I misunderstood, in which case it might be good to revise that > comment to be more explicit or accurate. Perhaps it would be clearest to deviate from the hw docs and rename PWM_CONTROL_ENABLE_SHIFT to PWM_CONTROL_TRIGGER_SHIFT to more closely match its function. What do you think of the following? /* * New duty and period settings are only reflected in the PWM output * after a rising edge of the trigger bit. After a rising edge, if the * smooth bit is set, the settings are also delayed until the current * period has completed. Furthermore, if the smooth bit is set, the PWM * continues to output a waveform based on the old settings during the * time that the trigger bit is low. Otherwise the output is a constant * high signal while the trigger bit is low. */
On Thu, Mar 20, 2014 at 8:57 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Tue, Mar 18, 2014 at 04:47:36PM -0700, Tim Kryger wrote: >> On Tue, Mar 18, 2014 at 2:52 PM, Thierry Reding >> <thierry.reding@gmail.com> wrote: >> > On Wed, Mar 12, 2014 at 01:15:43PM -0700, Tim Kryger wrote: >> >> + tristate "Kona PWM support" >> >> + depends on ARCH_BCM_MOBILE >> >> + default y >> > >> > No "default y", please. >> >> Even though it depends on ARCH_BCM_MOBILE? It seems like a reasonable >> default. > > Well, that's only true until somebody decides to turn that dependency > into something like: > > depends on ARCH_BCM_MOBILE || COMPILE_TEST Do you expect someone would actually make such a change? > >> These SoCs target mobile phones which almost always have a backlight >> controlled by the PWM. > > And you can easily turn it on in bcm_defconfig and/or multi_v7_defconfig > if you want it enabled "by default". Except that is not what default means. Having a sensible default value in the Kconfig is convenient and enables savedefconfig to create minimal configuration files. Are you firmly opposed to this? Thanks Again, Tim
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index 22f2f28..1fd42af 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -62,6 +62,16 @@ config PWM_ATMEL_TCB To compile this driver as a module, choose M here: the module will be called pwm-atmel-tcb. +config PWM_BCM_KONA + tristate "Kona PWM support" + depends on ARCH_BCM_MOBILE + default y + help + Generic PWM framework driver for Broadcom Kona PWM block. + + To compile this driver as a module, choose M here: the module + will be called pwm-bcm-kona. + config PWM_BFIN tristate "Blackfin PWM support" depends on BFIN_GPTIMERS diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index d8906ec..7413090 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -3,6 +3,7 @@ obj-$(CONFIG_PWM_SYSFS) += sysfs.o obj-$(CONFIG_PWM_AB8500) += pwm-ab8500.o obj-$(CONFIG_PWM_ATMEL) += pwm-atmel.o obj-$(CONFIG_PWM_ATMEL_TCB) += pwm-atmel-tcb.o +obj-$(CONFIG_PWM_BCM_KONA) += pwm-bcm-kona.o obj-$(CONFIG_PWM_BFIN) += pwm-bfin.o obj-$(CONFIG_PWM_EP93XX) += pwm-ep93xx.o obj-$(CONFIG_PWM_IMX) += pwm-imx.o diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c new file mode 100644 index 0000000..374e9ad --- /dev/null +++ b/drivers/pwm/pwm-bcm-kona.c @@ -0,0 +1,304 @@ +/* + * Copyright (C) 2014 Broadcom Corporation + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation version 2. + * + * This program is distributed "as is" WITHOUT ANY WARRANTY of any + * kind, whether express or implied; without even the implied warranty + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/clk.h> +#include <linux/delay.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/ioport.h> +#include <linux/math64.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/pwm.h> +#include <linux/slab.h> +#include <linux/types.h> + +#define PWM_CONTROL_OFFSET (0x00000000) +#define PWM_CONTROL_SMOOTH_SHIFT(chan) (24 + (chan)) +#define PWM_CONTROL_TYPE_SHIFT(chan) (16 + (chan)) +#define PWM_CONTROL_POLARITY_SHIFT(chan) (8 + (chan)) +#define PWM_CONTROL_ENABLE_SHIFT(chan) (chan) + +#define PRESCALE_OFFSET (0x00000004) +#define PRESCALE_SHIFT(chan) ((chan) << 2) +#define PRESCALE_MASK(chan) (0x7 << PRESCALE_SHIFT(chan)) +#define PRESCALE_MIN (0x00000000) +#define PRESCALE_MAX (0x00000007) + +#define PERIOD_COUNT_OFFSET(chan) (0x00000008 + ((chan) << 3)) +#define PERIOD_COUNT_MIN (0x00000002) +#define PERIOD_COUNT_MAX (0x00ffffff) + +#define DUTY_CYCLE_HIGH_OFFSET(chan) (0x0000000c + ((chan) << 3)) +#define DUTY_CYCLE_HIGH_MIN (0x00000000) +#define DUTY_CYCLE_HIGH_MAX (0x00ffffff) + +struct kona_pwmc { + struct pwm_chip chip; + void __iomem *base; + struct clk *clk; +}; + +static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan) +{ + unsigned long value = readl(kp->base + PWM_CONTROL_OFFSET); + + /* + * New duty and period settings are only reflected in the PWM output + * after a rising edge of the enable bit. When smooth bit is set, the + * new settings are delayed until the prior PWM period has completed. + * Furthermore, if the smooth bit is set, the PWM continues to output a + * waveform based on the old settings during the time that the enable + * bit is low. Otherwise the output is a constant high signal while + * the enable bit is low. + */ + + /* clear enable bit but set smooth bit to maintain old output */ + value |= (1 << PWM_CONTROL_SMOOTH_SHIFT(chan)); + value &= ~(1 << PWM_CONTROL_ENABLE_SHIFT(chan)); + writel(value, kp->base + PWM_CONTROL_OFFSET); + + /* set enable bit and clear smooth bit to apply new settings */ + value &= ~(1 << PWM_CONTROL_SMOOTH_SHIFT(chan)); + value |= (1 << PWM_CONTROL_ENABLE_SHIFT(chan)); + writel(value, kp->base + PWM_CONTROL_OFFSET); +} + +static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm, + int duty_ns, int period_ns) +{ + struct kona_pwmc *kp = dev_get_drvdata(chip->dev); + u64 val, div, clk_rate; + unsigned long prescale = PRESCALE_MIN, pc, dc; + unsigned int value, chan = pwm->hwpwm; + + /* + * Find period count, duty count and prescale to suit duty_ns and + * period_ns. This is done according to formulas described below: + * + * period_ns = 10^9 * (PRESCALE + 1) * PC / PWM_CLK_RATE + * duty_ns = 10^9 * (PRESCALE + 1) * DC / PWM_CLK_RATE + * + * PC = (PWM_CLK_RATE * period_ns) / (10^9 * (PRESCALE + 1)) + * DC = (PWM_CLK_RATE * duty_ns) / (10^9 * (PRESCALE + 1)) + */ + + clk_rate = clk_get_rate(kp->clk); + + /* There is polarity support in HW but it is easier to manage in SW */ + if (pwm->polarity == PWM_POLARITY_INVERSED) + duty_ns = period_ns - duty_ns; + + while (1) { + div = 1000000000; + div *= 1 + prescale; + val = clk_rate * period_ns; + pc = div64_u64(val, div); + val = clk_rate * duty_ns; + dc = div64_u64(val, div); + + /* If duty_ns or period_ns are not achievable then return */ + if (pc < PERIOD_COUNT_MIN || dc < DUTY_CYCLE_HIGH_MIN) + return -EINVAL; + + /* If pc and dc are in bounds, the calculation is done */ + if (pc <= PERIOD_COUNT_MAX && dc <= DUTY_CYCLE_HIGH_MAX) + break; + + /* Otherwise, increase prescale and recalculate pc and dc */ + if (++prescale > PRESCALE_MAX) + return -EINVAL; + } + + /* If the PWM channel is enabled, write the settings to the HW */ + if (test_bit(PWMF_ENABLED, &pwm->flags)) { + value = readl(kp->base + PRESCALE_OFFSET); + value &= ~PRESCALE_MASK(chan); + value |= prescale << PRESCALE_SHIFT(chan); + writel(value, kp->base + PRESCALE_OFFSET); + + writel(pc, kp->base + PERIOD_COUNT_OFFSET(chan)); + + writel(dc, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan)); + + kona_pwmc_apply_settings(kp, chan); + } + + return 0; +} + +static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm, + enum pwm_polarity polarity) +{ + /* + * The framework only allows the polarity to be changed when a PWM is + * disabled so no immediate action is required here. When a channel is + * enabled, the polarity gets handled as part of the re-config step. + */ + + return 0; +} + +static int kona_pwmc_enable(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct kona_pwmc *kp = dev_get_drvdata(chip->dev); + int ret; + + /* + * The PWM framework does not clear the enable bit in the flags if an + * error is returned from a PWM driver's enable function so it must be + * cleared here if any trouble is encountered. + */ + + ret = clk_prepare_enable(kp->clk); + if (ret < 0) { + dev_err(chip->dev, "failed to enable clock: %d\n", ret); + clear_bit(PWMF_ENABLED, &pwm->flags); + return ret; + } + + ret = kona_pwmc_config(chip, pwm, pwm->duty_cycle, pwm->period); + if (ret < 0) { + clk_disable_unprepare(kp->clk); + clear_bit(PWMF_ENABLED, &pwm->flags); + return ret; + } + + return 0; +} + +static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct kona_pwmc *kp = dev_get_drvdata(chip->dev); + unsigned int chan = pwm->hwpwm; + + /* + * The "enable" bits in the control register only affect when settings + * start to take effect so the only real way to disable the PWM output + * is to program a zero duty cycle. + */ + + writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan)); + kona_pwmc_apply_settings(kp, chan); + + /* + * When the PWM clock is disabled, the output is pegged high or low + * depending on its state at that instant. To guarantee that the new + * settings have taken effect and the output is low a delay of 400ns is + * required. + */ + + ndelay(400); + + clk_disable_unprepare(kp->clk); +} + +static const struct pwm_ops kona_pwm_ops = { + .config = kona_pwmc_config, + .set_polarity = kona_pwmc_set_polarity, + .enable = kona_pwmc_enable, + .disable = kona_pwmc_disable, + .owner = THIS_MODULE, +}; + +static int kona_pwmc_probe(struct platform_device *pdev) +{ + struct kona_pwmc *kp; + struct resource *res; + unsigned int chan, value; + int ret = 0; + + kp = devm_kzalloc(&pdev->dev, sizeof(*kp), GFP_KERNEL); + if (kp == NULL) + return -ENOMEM; + + platform_set_drvdata(pdev, kp); + + kp->chip.dev = &pdev->dev; + kp->chip.ops = &kona_pwm_ops; + kp->chip.base = -1; + kp->chip.npwm = 6; + kp->chip.of_xlate = of_pwm_xlate_with_flags; + kp->chip.of_pwm_n_cells = 3; + kp->chip.can_sleep = true; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + kp->base = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(kp->base)) + return PTR_ERR(kp->base); + + kp->clk = devm_clk_get(&pdev->dev, NULL); + if (IS_ERR(kp->clk)) { + dev_err(&pdev->dev, "failed to get clock: %ld\n", + PTR_ERR(kp->clk)); + return PTR_ERR(kp->clk); + } + + ret = clk_prepare_enable(kp->clk); + if (ret < 0) { + dev_err(&pdev->dev, "failed to enable clock: %d\n", ret); + return ret; + } + + /* Set smooth mode, push/pull, and normal polarity for all channels */ + for (value = 0, chan = 0; chan < kp->chip.npwm; chan++) { + value |= (1 << PWM_CONTROL_SMOOTH_SHIFT(chan)); + value |= (1 << PWM_CONTROL_TYPE_SHIFT(chan)); + value |= (1 << PWM_CONTROL_POLARITY_SHIFT(chan)); + } + writel(value, kp->base + PWM_CONTROL_OFFSET); + + clk_disable_unprepare(kp->clk); + + ret = pwmchip_add(&kp->chip); + if (ret < 0) + dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret); + + return ret; +} + +static int kona_pwmc_remove(struct platform_device *pdev) +{ + struct kona_pwmc *kp = platform_get_drvdata(pdev); + unsigned int chan; + + for (chan = 0; chan < kp->chip.npwm; chan++) + if (test_bit(PWMF_ENABLED, &kp->chip.pwms[chan].flags)) + clk_disable_unprepare(kp->clk); + + return pwmchip_remove(&kp->chip); +} + +static const struct of_device_id bcm_kona_pwmc_dt[] = { + { .compatible = "brcm,kona-pwm" }, + { }, +}; +MODULE_DEVICE_TABLE(of, bcm_kona_pwmc_dt); + +static struct platform_driver kona_pwmc_driver = { + + .driver = { + .name = "bcm-kona-pwm", + .of_match_table = bcm_kona_pwmc_dt, + }, + .probe = kona_pwmc_probe, + .remove = kona_pwmc_remove, +}; + +module_platform_driver(kona_pwmc_driver); + +MODULE_AUTHOR("Broadcom Corporation <bcm-kernel-feedback-list@broadcom.com>"); +MODULE_AUTHOR("Tim Kryger <tkryger@broadcom.com>"); +MODULE_DESCRIPTION("Driver for Kona PWM controller"); +MODULE_LICENSE("GPL v2");