| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 25 Nov 2014 23:29:41 -0800
From: Tim Kryger <tim.kryger@...il.com>
To: Scott Branden <sbranden@...adcom.com>
Cc: Thierry Reding <thierry.reding@...il.com>,
Ray Jui <rjui@...adcom.com>,
Arun Ramamurthy <arunrama@...adcom.com>,
bcm-kernel-feedback-list@...adcom.com, linux-pwm@...r.kernel.org,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 3/4] pwm: kona: Fix enable, disable and config procedures
On Tue, Nov 25, 2014 at 11:40 AM, Scott Branden <sbranden@...adcom.com> wrote:
> From: Arun Ramamurthy <arunrama@...adcom.com>
>
> - Added helper functions to set and clear smooth and trigger bits
> - Added 400ns delays when clearing and setting trigger bit as requied
> by spec
> - Added helper function to write prescale and other settings
> - Updated config procedure to match spec
> - Added code to handle pwn config when channel is disabled
> - Updated disable procedure to match spec
>
> Signed-off-by: Arun Ramamurthy <arunrama@...adcom.com>
> Reviewed-by: Ray Jui <rjui@...adcom.com>
> Signed-off-by: Scott Branden <sbranden@...adcom.com>
> ---
> drivers/pwm/pwm-bcm-kona.c | 100 +++++++++++++++++++++++++++++++++++----------
> 1 file changed, 78 insertions(+), 22 deletions(-)
The driver is fairly small and this change rewrites a considerable amount of it.
Is there a actually specific deficiency that this change is intended to address?
I'm not sure all the extra helper functions improve readability.
>
> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
> index fa0b5bf..06fa983 100644
> --- a/drivers/pwm/pwm-bcm-kona.c
> +++ b/drivers/pwm/pwm-bcm-kona.c
> @@ -65,6 +65,10 @@
> #define DUTY_CYCLE_HIGH_MIN (0x00000000)
> #define DUTY_CYCLE_HIGH_MAX (0x00ffffff)
>
> +/* The delay required after clearing or setting
> + PWMOUT_ENABLE*/
> +#define PWMOUT_ENABLE_HOLD_DELAY 400
> +
> struct kona_pwmc {
> struct pwm_chip chip;
> void __iomem *base;
> @@ -76,28 +80,70 @@ static inline struct kona_pwmc *to_kona_pwmc(struct pwm_chip *_chip)
> return container_of(_chip, struct kona_pwmc, chip);
> }
>
> -static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
> +static inline void kona_pwmc_set_trigger(struct kona_pwmc *kp,
> + unsigned int chan)
> {
> unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
>
> - /* Clear trigger bit but set smooth bit to maintain old output */
> - value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan);
> + /* set trigger bit to enable channel */
> + value |= 1 << PWM_CONTROL_TRIGGER_SHIFT(chan);
> + writel(value, kp->base + PWM_CONTROL_OFFSET);
> + ndelay(PWMOUT_ENABLE_HOLD_DELAY);
> +}
> +static inline void kona_pwmc_clear_trigger(struct kona_pwmc *kp,
> + unsigned int chan)
> +{
> + unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
> +
> + /* Clear trigger bit */
> value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
> writel(value, kp->base + PWM_CONTROL_OFFSET);
> + ndelay(PWMOUT_ENABLE_HOLD_DELAY);
> +}
>
> - /* Set trigger bit and clear smooth bit to apply new settings */
> +static inline void kona_pwmc_clear_smooth(struct kona_pwmc *kp,
> + unsigned int chan)
> +{
> + unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
> +
> + /* Clear smooth bit */
> value &= ~(1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
> - value |= 1 << PWM_CONTROL_TRIGGER_SHIFT(chan);
> writel(value, kp->base + PWM_CONTROL_OFFSET);
> }
>
> +static inline void kona_pwmc_set_smooth(struct kona_pwmc *kp, unsigned int chan)
> +{
> + unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
> +
> + /* set smooth bit to maintain old output */
> + value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan);
> + writel(value, kp->base + PWM_CONTROL_OFFSET);
> +}
> +
> +static void kona_pwmc_write_settings(struct kona_pwmc *kp, unsigned int chan,
> + unsigned long prescale, unsigned long pc,
> + unsigned long dc)
> +{
> + unsigned int value;
> +
> + 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));
> +
> +}
> +
> static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
> int duty_ns, int period_ns)
> {
> struct kona_pwmc *kp = to_kona_pwmc(chip);
> u64 val, div, rate;
> unsigned long prescale = PRESCALE_MIN, pc, dc;
> - unsigned int value, chan = pwm->hwpwm;
> + unsigned int ret, chan = pwm->hwpwm;
>
> /*
> * Find period count, duty count and prescale to suit duty_ns and
> @@ -133,19 +179,30 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
> 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));
> + /* If the PWM channel is not enabled, enable the clock */
> + if (!test_bit(PWMF_ENABLED, &pwm->flags)) {
> + ret = clk_prepare_enable(kp->clk);
> + if (ret < 0) {
> + dev_err(chip->dev, "failed to enable clock: %d\n", ret);
> + return ret;
> + }
> + }
>
> - writel(dc, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
> + /* Set smooth bit to maintain old output */
> + kona_pwmc_set_smooth(kp, chan);
> + kona_pwmc_clear_trigger(kp, chan);
> +
> + /* apply new settings */
> + kona_pwmc_write_settings(kp, chan, prescale, pc, dc);
> +
> + /*If the PWM is enabled, enable the channel with the new settings
> + and if not disable the clock*/
> + if (test_bit(PWMF_ENABLED, &pwm->flags))
> + kona_pwmc_set_trigger(kp, chan);
> + else
> + clk_disable_unprepare(kp->clk);
>
> - kona_pwmc_apply_settings(kp, chan);
> - }
>
> return 0;
> }
> @@ -188,7 +245,6 @@ static int kona_pwmc_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> dev_err(chip->dev, "failed to enable clock: %d\n", ret);
> return ret;
> }
> -
> ret = kona_pwmc_config(chip, pwm, pwm->duty_cycle, pwm->period);
> if (ret < 0) {
> clk_disable_unprepare(kp->clk);
> @@ -203,12 +259,12 @@ static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> struct kona_pwmc *kp = to_kona_pwmc(chip);
> unsigned int chan = pwm->hwpwm;
>
> + kona_pwmc_clear_smooth(kp, chan);
> + kona_pwmc_clear_trigger(kp, chan);
I believe the output will spike high here. Likely not what you want...
> /* Simulate a disable by configuring for zero duty */
> - writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
> - kona_pwmc_apply_settings(kp, chan);
> -
> - /* Wait for waveform to settle before gating off the clock */
> - ndelay(400);
> + kona_pwmc_write_settings(kp, chan, 0, 0, 0);
> + kona_pwmc_set_polarity(chip, pwm, PWM_POLARITY_NORMAL);
This is wrong. You shouldn't change the polarity when the PWM is disabled.
The original polarity isn't even restored when it is re-enabled...
> + kona_pwmc_set_trigger(kp, chan);
>
> clk_disable_unprepare(kp->clk);
> }
> --
> 2.1.3
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists