| 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
| ||
|
Message-ID: <5477B413.80207@broadcom.com>
Date: Thu, 27 Nov 2014 15:30:27 -0800
From: Scott Branden <sbranden@...adcom.com>
To: Tim Kryger <tim.kryger@...il.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
Hi Tim,
Thanks for all your comments on the patchset. We are in the process of
reviewing and will provide feedback when that is completed.
Scott
On 14-11-25 11:29 PM, Tim Kryger wrote:
> 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