Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932880AbaLBE3F (ORCPT ); Mon, 1 Dec 2014 23:29:05 -0500 Received: from mail-ob0-f175.google.com ([209.85.214.175]:52444 "EHLO mail-ob0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753880AbaLBE3D (ORCPT ); Mon, 1 Dec 2014 23:29:03 -0500 MIME-Version: 1.0 In-Reply-To: <547CC392.3070508@broadcom.com> References: <1416944441-12066-1-git-send-email-sbranden@broadcom.com> <1416944441-12066-4-git-send-email-sbranden@broadcom.com> <54790A0F.2010506@broadcom.com> <547CC392.3070508@broadcom.com> Date: Mon, 1 Dec 2014 20:29:02 -0800 Message-ID: Subject: Re: [PATCH v2 3/4] pwm: kona: Fix enable, disable and config procedures From: Tim Kryger To: Arun Ramamurthy Cc: Scott Branden , Thierry Reding , Ray Jui , Arun Ramamurthy , bcm-kernel-feedback-list@broadcom.com, linux-pwm@vger.kernel.org, Linux Kernel Mailing List Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Dec 1, 2014 at 11:37 AM, Arun Ramamurthy wrote: > > > On 14-11-28 06:30 PM, Tim Kryger wrote: >> >> On Fri, Nov 28, 2014 at 3:49 PM, Arun Ramamurthy >> wrote: >>> >>> >>> >>> On 14-11-25 11:29 PM, Tim Kryger wrote: >>>> >>>> >>>> On Tue, Nov 25, 2014 at 11:40 AM, Scott Branden >>>> wrote: >>>>> >>>>> >>>>> From: Arun Ramamurthy >>>>> >>>>> - 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 >>>>> Reviewed-by: Ray Jui >>>>> Signed-off-by: Scott Branden >>>>> --- >>>>> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/