| 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: <54790A0F.2010506@broadcom.com>
Date: Fri, 28 Nov 2014 15:49:35 -0800
From: Arun Ramamurthy <arun.ramamurthy@...adcom.com>
To: Tim Kryger <tim.kryger@...il.com>,
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 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?
>
The main issue this patchset addresses is setting the period and duty
cycle when the pwm is disabled. This is done by turning on the clock and
writing to the PWM registers. Additionally it also adds the 400ns
delays specified by the PWM spec when setting or clearing certain bits.
It also updates the PWM programming procedure to match the spec more
closely. Although there is considerable change, all of it addresses the
core functionality and it would not make sense to split it into multiple
patches.
> I'm not sure all the extra helper functions improve readability.
>
There was a lot of repeated code in various different functions. It
seemed more efficient to consolidate them into helper functions. It also
helped when comparing the spec to the code to check if we were
setting the bits in the right order.
>>
>> 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...
According to spec, this is the procedure to program the PWM and the code
follows that:
STEP0: Program SMOOTH_TYPE=1. That will only allow changing of PWM
setting at the PWM period boundary.
STEP1: Program PWMOUT_ENABLE=0. At this time, PWM internal logic will
continue to run with the previous settings. (i.e. If PWM is at 50Hz 40%
duty cycle before, during the time when PWMOUT_ENABLE=0, it will still
run at 50MHz 40% duty cycle.)
STEP2: Program PWM register for new setting (i.e. PRESCALE, DUTY,
PERIOD etc)
STEP3: Program PWMOUT_ENABLE=1. That will load the new PWM setting
from APB into PWM internal register. (Note. Minimum of 400ns is needed
between step1 and step3. )
STEP4: Keep PWMOUT ENABLE=1. (Note: If user didn't hold
PWMOUT_ENABLE=1 for longer than 400ns, PWM internal logic will discard
the new PWM setting in step2. User should hold the PWMOUT_ENABLE=1
unless new PWM settings is needed.)
>
>> /* 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...
>
this is procedure from the PWM spec to disable :
STEP0: Program SMOOTH_TYPE=0.
STEP1: Program PWMOUT_ENABLE=0. Now, PWM internal logic will be at
reset, PWM output will be default at 1.
STEP2: Program PWM register to these setting. PRESCALE=0, POLARITY=1,
DUTY=0, PERIOD=0.
STEP3: Program PWMOUT_ENABLE=1, and Keep SMOOTH_TYPE=0.
STEP4: Turn off PWM clock from CCU, and Keep PWMOUT ENABLE=1. (Note,
It takes 400ns from STEP3 to turn off the LCD backlight, and user should
guarantee that the PWM clock will not be disabled in less than 400ns
after STEP3.
I agree with you that the original polarity isnt restored. I will need
to add some code to check the syfs polarity value when the PWM is
enabled. However, if i was to comply with the above spec, I would still
have set the polarity. I just realized it should be set to inverted and
I will fix this in the next patchset
>> + 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