| 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: <CAD7vxxK3+uXF1Eqb=8xq+kuR7yUOvzyGUhhxM2-C_Nqq+RRpLQ@mail.gmail.com>
Date: Mon, 1 Dec 2014 20:29:02 -0800
From: Tim Kryger <tim.kryger@...il.com>
To: Arun Ramamurthy <arun.ramamurthy@...adcom.com>
Cc: Scott Branden <sbranden@...adcom.com>,
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 Mon, Dec 1, 2014 at 11:37 AM, Arun Ramamurthy
<arun.ramamurthy@...adcom.com> wrote:
>
>
> On 14-11-28 06:30 PM, Tim Kryger wrote:
>>
>> On Fri, Nov 28, 2014 at 3:49 PM, Arun Ramamurthy
>> <arun.ramamurthy@...adcom.com> wrote:
>>>
>>>
>>>
>>> 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.
>>
>>
>> So what you are saying is that there isn't any known issue that this
>> resolves.
>>
>> This only changes the driver to use an alternate programming sequence?
>>
>> The benefit here seems uncertain.
>>
> Actually Tim, it addresses the bug where period and duty cycle are not set
> if the PWM channel is not enabled in sysfs. Following up on your example you
> provided in your other email:
>
> # Disable PWM
> echo 0 > enable
>
> # Change to a 25% duty output when PWM is enabled
> echo 25000 > duty_cycle
>
> The original code would not write this duty cycle change to the registers as
> the PWmF_ENABLED bit is not set and the clocks are turned off.
You are mistaken. There is no bug here. This is by design.
A subsequent enable results in the requested duty and period being set.
> I can remove all the helper functions and address only this particular bug
> if you prefer that.
>
>>>
>>>> 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.
>>
>>
>> This is exactly what I was saying before. You glitch the output high
>> for no good reason.
>>
>> The sequence in the document isn't gospel. From what I recall, it was
>> just a verification engineer's best guess at how to get a very unusual
>> PWM controller to do the normal PWM things.
>
>
> So to summarize, you would prefer that the disable procedure be left
> unchanged which is to set the duty cycle to zero.
I would be much more likely to give my approval to a patch that
identified a specific issue and resolved it without unnecessarily
rewriting large parts of the driver.
>
>>
>>> 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