Fix bugs in kona pwm driver and pwm core
Subsystems (2)
v8 cover Fix bugs in kona pwm driver and pwm core
This patchset fixes a number of bugs in the Broadcom Kona pwm driver. It also fixes a bug in the pwm core where the enabled state was incorrect on failed calls to enable. Also, a new function was added to the pwm core to add pwm chips with inversed polarity for chips that have a different default polarity than the core. The prevents incorrect polarity being reported.
Debug info has been added to help debug configuring duty cycle and period.
These changes are required for the Kona PWM driver to work on Cygnus. The same PWM IP is being used.
Changes from v7:
- Polarity changes take effect immediately instead of being deferred until enable is called.
Changes from v6:
- Move new debugging info for duty cycle and period in config function into its own commit.
- Add kona_pwmc_prepare_for_settings() function to remove duplicated code.
Changes from v5:
- Address Dmitry's comment on code cleanup of pwm_enable() in pwm core.
- Including all patches from different contributors in this patchset. Some were left out in v4.
Changes from v4:
- Rebased to Tim Kryger's patch that adds support in pwm core to add driver with inversed polarity.
- Removed patch 2 that resolved difference between hardware default polarity and pwm framework default polarity. The default polarity is set properly now when the pwm driver is registered with the pwm framework.
Arun Ramamurthy (1):
drivers: pwm: bcm-kona: Dont set polarity in probe
Jonathan Richardson (3):
pwm: kona: Fix incorrect config, disable, and polarity procedures
pwm: kona: Add debug info to config function
pwm: core: Set enable state properly on failed call to enable
Tim Kryger (1):
drivers: pwm: core: Add pwmchip_add_inversed
drivers/pwm/core.c | 52 ++++++++++++++++++++++------ drivers/pwm/pwm-bcm-kona.c | 81 ++++++++++++++++++++++++++++++++++---------- include/linux/pwm.h | 6 ++++ 3 files changed, 111 insertions(+), 28 deletions(-)
Signature · 1 line
1.7.9.5
PATCH v8 1/5 drivers: pwm: core: Add pwmchip_add_inversed
From: Tim Kryger <[email protected]>
Add a new function to register a PWM chip with channels that have their initial polarity as inversed. This benefits drivers of controllers that by default operate with inversed polarity by removing the need to modify the polarity during initialization.
--- drivers/pwm/core.c | 36 ++++++++++++++++++++++++++++-------- include/linux/pwm.h | 6 ++++++ 2 files changed, 34 insertions(+), 8 deletions(-)
drivers/pwm/core.c +28 -8
@@ -222,14 +222,8 @@ void *pwm_get_chip_data(struct pwm_device *pwm) }EXPORT_SYMBOL_GPL (pwm_get_chip_data); -/** -* pwmchip_add() - register a new PWM chip -* @chip: the PWM chip to add -* -* Register a new PWM chip. If chip->base < 0 then a dynamically assigned base -* will be used. -*/ -int pwmchip_add (struct pwm_chip *chip) +static int pwmchip_add_with_polarity (struct pwm_chip *chip, +enum pwm_polarity polarity) {struct pwm_device *pwm;unsigned int i;
@@ -259,6 +253,7 @@ int pwmchip_add(struct pwm_chip *chip) pwm->chip = chip; pwm->pwm = chip->base + i; pwm->hwpwm = i; + pwm->polarity = polarity;radix_tree_insert (&pwm_tree, pwm->pwm, pwm); }
@@ -279,9 +274,34 @@ out:mutex_unlock (&pwm_lock);return ret; } + +/** +* pwmchip_add() - register a new PWM chip +* @chip: the PWM chip to add +* +* Register a new PWM chip. If chip->base < 0 then a dynamically assigned base +* will be used. The initial polarity for all channels is normal. +*/ +int pwmchip_add (struct pwm_chip *chip) +{ +return pwmchip_add_with_polarity (chip,PWM_POLARITY_NORMAL ); +}EXPORT_SYMBOL_GPL (pwmchip_add);/** +* pwmchip_add_inversed() - register a new PWM chip +* @chip: the PWM chip to add +* +* Register a new PWM chip. If chip->base < 0 then a dynamically assigned base +* will be used. The initial polarity for all channels is inversed. +*/ +int pwmchip_add_inversed (struct pwm_chip *chip) +{ +return pwmchip_add_with_polarity (chip,PWM_POLARITY_INVERSED ); +} +EXPORT_SYMBOL_GPL (pwmchip_add_inversed); + +/** * pwmchip_remove() - remove a PWM chip * @chip: the PWM chip to remove *
include/linux/pwm.h +6 -0
@@ -183,6 +183,7 @@ int pwm_set_chip_data(struct pwm_device *pwm, void *data);void *pwm_get_chip_data (struct pwm_device *pwm);int pwmchip_add (struct pwm_chip *chip); +int pwmchip_add_inversed (struct pwm_chip *chip);int pwmchip_remove (struct pwm_chip *chip); struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip, unsigned int index,
@@ -217,6 +218,11 @@ static inline int pwmchip_add(struct pwm_chip *chip)return -EINVAL ; } +static inline int pwmchip_add_inversed (struct pwm_chip *chip) +{ +return -EINVAL ; +} +static inline int pwmchip_remove (struct pwm_chip *chip) {return -EINVAL ;
Signature · 1 line
1.7.9.5
Re: [PATCH v8 1/5] drivers: pwm: core: Add pwmchip_add_inversed
Quoted 7 lines · Jonathan Richardson
From: Tim Kryger <[email protected]>
Add a new function to register a PWM chip with channels that have their initial polarity as inversed. This benefits drivers of controllers that by default operate with inversed polarity by removing the need to modify the polarity during initialization.
Quoted 6 lines
Signed-off-by Tim Kryger <[email protected]>Signed-off-by Jonathan Richardson <[email protected]>--- drivers/pwm/core.c | 36 ++++++++++++++++++++++++++++-------- include/linux/pwm.h | 6 ++++++ 2 files changed, 34 insertions(+), 8 deletions(-)
I had to bikeshed this a little, so I ended up applying a variant that exports pwmchip_add_with_polarity() instead of having the additional wrapper. The rationale here is that pwmchip_add_with_polarity() is more explicit than pwmchip_add_inversed().
Thierry
Re: [PATCH v8 1/5] drivers: pwm: core: Add pwmchip_add_inversed
Quoted 15 lines · Thierry Reding
Quoted 7 lines · Jonathan Richardson
From: Tim Kryger <[email protected]>
Add a new function to register a PWM chip with channels that have their initial polarity as inversed. This benefits drivers of controllers that by default operate with inversed polarity by removing the need to modify the polarity during initialization.
Quoted 6 lines
Signed-off-by Tim Kryger <[email protected]>Signed-off-by Jonathan Richardson <[email protected]>--- drivers/pwm/core.c | 36 ++++++++++++++++++++++++++++-------- include/linux/pwm.h | 6 ++++++ 2 files changed, 34 insertions(+), 8 deletions(-)
Quoted 6 lines
I had to bikeshed this a little, so I ended up applying a variant that exports
pwmchip_add_with_polarity()instead of having the additional wrapper. The rationale here is thatpwmchip_add_with_polarity()is more explicit thanpwmchip_add_inversed().Thierry
Sounds good. Thanks.
-Tim
PATCH v8 2/5 drivers: pwm: bcm-kona: Dont set polarity in probe
In reply to Jonathan RichardsonFrom: Arun Ramamurthy <[email protected]>
Omit setting the polarity to normal during probe and instead use the new pwmchip_add_inversed function to register a PWM chip with default polarity of inversed for all channels as this is the actual hardware default.
--- drivers/pwm/pwm-bcm-kona.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
drivers/pwm/pwm-bcm-kona.c +3 -6
@@ -266,18 +266,15 @@ static int kona_pwmc_probe(struct platform_device *pdev)return ret; } -/* Set smooth mode, push/pull, and normal polarity for all channels */ -for (chan =0 ; chan < kp->chip.npwm; chan++) { - value |= (1 <<PWM_CONTROL_SMOOTH_SHIFT (chan)); +/* Set push/pull for all channels */ +for (chan =0 ; chan < kp->chip.npwm; chan++) value |= (1 <<PWM_CONTROL_TYPE_SHIFT (chan)); - value |= (1 <<PWM_CONTROL_POLARITY_SHIFT (chan)); - }writel (value, kp->base +PWM_CONTROL_OFFSET );clk_disable_unprepare (kp->clk); - ret =pwmchip_add (&kp->chip); + ret =pwmchip_add_inversed (&kp->chip);if (ret <0 )dev_err (&pdev->dev,"failed to add PWM chip: %d\n" , ret);
Signature · 1 line
1.7.9.5
Thierry Reding
AppliedRe: [PATCH v8 2/5] drivers: pwm: bcm-kona: Dont set polarity in probe
Quoted 7 lines · Jonathan Richardson
From: Arun Ramamurthy <[email protected]>
Omit setting the polarity to normal during probe and instead use the new
pwmchip_add_inversedfunction to register a PWM chip with default polarity of inversed for all channels as this is the actual hardware default.
Quoted 8 lines
Signed-off-by Arun Ramamurthy <[email protected]>Reviewed-by Ray Jui <[email protected]>Signed-off-by Scott Branden <[email protected]>Signed-off-by Tim Kryger <[email protected]>Signed-off-by Jonathan Richardson <[email protected]>--- drivers/pwm/pwm-bcm-kona.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
Applied, after adapting to the pwmchip_add_with_polarity() API, thanks.
Thierry
PATCH v8 3/5 pwm: kona: Fix incorrect config, disable, and polarity procedures
In reply to Jonathan RichardsonThe config procedure didn't follow the spec which periodically resulted in failing to enable the output signal. This happened one in ten or twenty attempts. Following the spec and adding a 400ns delay in the appropriate locations resolves this problem.
The disable and polarity procedures now also follow the spec. The old procedures would result in no change in signal when called.
--- drivers/pwm/pwm-bcm-kona.c | 47 +++++++++++++++++++++++++++++++++++--------- 1 file changed, 38 insertions(+), 9 deletions(-)
drivers/pwm/pwm-bcm-kona.c +38 -9
@@ -76,19 +76,36 @@ 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) +/* +* Clear trigger bit but set smooth bit to maintain old output. +*/ +static void kona_pwmc_prepare_for_settings (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); value &= ~(1 <<PWM_CONTROL_TRIGGER_SHIFT (chan));writel (value, kp->base +PWM_CONTROL_OFFSET ); +/* +* There must be a min 400ns delay between clearing enable and setting +* it. Failing to do this may result in no PWM signal. +*/ +ndelay (400 ); +} + +static void kona_pwmc_apply_settings (struct kona_pwmc *kp,unsigned int chan) +{ +unsigned int value =readl (kp->base +PWM_CONTROL_OFFSET ); +/* Set trigger bit and clear smooth bit to apply new settings */ value &= ~(1 <<PWM_CONTROL_SMOOTH_SHIFT (chan)); value |=1 <<PWM_CONTROL_TRIGGER_SHIFT (chan);writel (value, kp->base +PWM_CONTROL_OFFSET ); + +/* PWMOUT_ENABLE must be held high for at least 400 ns. */ +ndelay (400 ); } static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -133,8 +150,14 @@ 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 */ +/* +* Don't apply settings if disabled. The period and duty cycle are +* always calculated above to ensure the new values are +* validated immediately instead of on enable. +*/ if (test_bit (PWMF_ENABLED , &pwm->flags)) { +kona_pwmc_prepare_for_settings (kp, chan); + value =readl (kp->base +PRESCALE_OFFSET ); value &= ~PRESCALE_MASK (chan); value |= prescale <<PRESCALE_SHIFT (chan);
@@ -164,6 +187,8 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,return ret; } +kona_pwmc_prepare_for_settings (kp, chan); + value =readl (kp->base +PWM_CONTROL_OFFSET );if (polarity ==PWM_POLARITY_NORMAL )
@@ -175,9 +200,6 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,kona_pwmc_apply_settings (kp, chan); -/* Wait for waveform to settle before gating off the clock */ -ndelay (400 ); -clk_disable_unprepare (kp->clk);return 0 ;
@@ -207,13 +229,20 @@ 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; +unsigned int value; + +kona_pwmc_prepare_for_settings (kp, chan);/* Simulate a disable by configuring for zero duty */ writel (0 , kp->base +DUTY_CYCLE_HIGH_OFFSET (chan)); -kona_pwmc_apply_settings (kp, chan); +writel (0 , kp->base +PERIOD_COUNT_OFFSET (chan)); -/* Wait for waveform to settle before gating off the clock */ -ndelay (400 ); +/* Set prescale to 0 for this channel */ + value =readl (kp->base +PRESCALE_OFFSET ); + value &= ~PRESCALE_MASK (chan); +writel (value, kp->base +PRESCALE_OFFSET ); + +kona_pwmc_apply_settings (kp, chan);clk_disable_unprepare (kp->clk); }
Signature · 1 line
1.7.9.5
Re: [PATCH v8 3/5] pwm: kona: Fix incorrect config, disable, and polarity procedures
Quoted 7 lines · Jonathan Richardson
The config procedure didn't follow the spec which periodically resulted in failing to enable the output signal. This happened one in ten or twenty attempts. Following the spec and adding a 400ns delay in the appropriate locations resolves this problem.
The disable and polarity procedures now also follow the spec. The old procedures would result in no change in signal when called.
I think you may want to adjust your commit title and message to more clearly describe what this change is doing. Perhaps something like:
pwm: kona: Modify settings application sequence
Update the driver so that settings are applied in accordance with the most recent version of the hardware spec. The revised sequence clears the trigger bit, waits 400ns, writes settings, sets the trigger bit, and waits another 400ns. This corrects an issue where occasionally a requested change was not properly reflected in the PWM output.
Otherwise, this patch looks reasonable so
Quoted 27 lines
Reviewed-by Arun Ramamurthy <[email protected]>Reviewed-by Scott Branden <[email protected]>Tested-by Scott Branden <[email protected]>Signed-off-by Jonathan Richardson <[email protected]>--- drivers/pwm/pwm-bcm-kona.c | 47 +++++++++++++++++++++++++++++++++++--------- 1 file changed, 38 insertions(+), 9 deletions(-)drivers/pwm/pwm-bcm-kona.c +5
-2@@ -76,19 +76,36 @@ 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) +/* +* Clear trigger bit but set smooth bit to maintain old output. +*/ +static void kona_pwmc_prepare_for_settings (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 */
drivers/pwm/pwm-bcm-kona.c static inline struct kona_pwmc *to_kona_pwmc(struct pwm_chip *_chip) · line 96
Quoted 10 lines
drivers/pwm/pwm-bcm-kona.c +6
-0value |=1 <<PWM_CONTROL_SMOOTH_SHIFT (chan); value &= ~(1 <<PWM_CONTROL_TRIGGER_SHIFT (chan));writel (value, kp->base +PWM_CONTROL_OFFSET ); +/* +* There must be a min 400ns delay between clearing enable and setting +* it. Failing to do this may result in no PWM signal. +*/ +ndelay (400 ); +}
Since it doesn't function as an enable, please call it the trigger bit.
drivers/pwm/pwm-bcm-kona.c static inline struct kona_pwmc *to_kona_pwmc(struct pwm_chip *_chip) · line 109
Quoted 13 lines
drivers/pwm/pwm-bcm-kona.c +8
-0+ +static void kona_pwmc_apply_settings (struct kona_pwmc *kp,unsigned int chan) +{ +unsigned int value =readl (kp->base +PWM_CONTROL_OFFSET ); +/* Set trigger bit and clear smooth bit to apply new settings */ value &= ~(1 <<PWM_CONTROL_SMOOTH_SHIFT (chan)); value |=1 <<PWM_CONTROL_TRIGGER_SHIFT (chan);writel (value, kp->base +PWM_CONTROL_OFFSET ); + +/* PWMOUT_ENABLE must be held high for at least 400 ns. */ +ndelay (400 ); }
Same here.
Quoted 64 lines
drivers/pwm/pwm-bcm-kona.c +19
-8static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,@@ -133,8 +150,14 @@ 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 */ +/* +* Don't apply settings if disabled. The period and duty cycle are +* always calculated above to ensure the new values are +* validated immediately instead of on enable. +*/ if (test_bit (PWMF_ENABLED , &pwm->flags)) { +kona_pwmc_prepare_for_settings (kp, chan); + value =readl (kp->base +PRESCALE_OFFSET ); value &= ~PRESCALE_MASK (chan); value |= prescale <<PRESCALE_SHIFT (chan);@@ -164,6 +187,8 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,return ret; } +kona_pwmc_prepare_for_settings (kp, chan); + value =readl (kp->base +PWM_CONTROL_OFFSET );if (polarity ==PWM_POLARITY_NORMAL )@@ -175,9 +200,6 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,kona_pwmc_apply_settings (kp, chan); -/* Wait for waveform to settle before gating off the clock */ -ndelay (400 ); -clk_disable_unprepare (kp->clk);return 0 ;@@ -207,13 +229,20 @@ 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; +unsigned int value; + +kona_pwmc_prepare_for_settings (kp, chan);/* Simulate a disable by configuring for zero duty */ writel (0 , kp->base +DUTY_CYCLE_HIGH_OFFSET (chan)); -kona_pwmc_apply_settings (kp, chan); +writel (0 , kp->base +PERIOD_COUNT_OFFSET (chan)); -/* Wait for waveform to settle before gating off the clock */ -ndelay (400 ); +/* Set prescale to 0 for this channel */ + value =readl (kp->base +PRESCALE_OFFSET ); + value &= ~PRESCALE_MASK (chan); +writel (value, kp->base +PRESCALE_OFFSET ); + +kona_pwmc_apply_settings (kp, chan);clk_disable_unprepare (kp->clk); } --1.7.9.5
Re: [PATCH v8 3/5] pwm: kona: Fix incorrect config, disable, and polarity procedures
Quoted 21 lines · Tim Kryger
Quoted 7 lines · Jonathan Richardson
The config procedure didn't follow the spec which periodically resulted in failing to enable the output signal. This happened one in ten or twenty attempts. Following the spec and adding a 400ns delay in the appropriate locations resolves this problem.
The disable and polarity procedures now also follow the spec. The old procedures would result in no change in signal when called.
I think you may want to adjust your commit title and message to more clearly describe what this change is doing. Perhaps something like:
pwm: kona: Modify settings application sequence
Update the driver so that settings are applied in accordance with the most recent version of the hardware spec. The revised sequence clears the trigger bit, waits 400ns, writes settings, sets the trigger bit, and waits another 400ns. This corrects an issue where occasionally a requested change was not properly reflected in the PWM output.
Quoted 3 lines
Otherwise, this patch looks reasonable so
Reviewed-by Tim Kryger <[email protected]>
Fine with me. Same with two comments below. Will re-spin when I can see Thierry's modification to use pwmchip_add_with_polarity() instead of pwmchip_add_inversed().
Thanks.
Quoted 121 lines
Quoted 28 lines
Reviewed-by Arun Ramamurthy <[email protected]>Reviewed-by Scott Branden <[email protected]>Tested-by Scott Branden <[email protected]>Signed-off-by Jonathan Richardson <[email protected]>--- drivers/pwm/pwm-bcm-kona.c | 47 +++++++++++++++++++++++++++++++++++--------- 1 file changed, 38 insertions(+), 9 deletions(-)drivers/pwm/pwm-bcm-kona.c +5
-2@@ -76,19 +76,36 @@ 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) +/* +* Clear trigger bit but set smooth bit to maintain old output. +*/ +static void kona_pwmc_prepare_for_settings (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 */ drivers/pwm/pwm-bcm-kona.c static inline struct kona_pwmc *to_kona_pwmc(struct pwm_chip *_chip) · line 96
Quoted 10 lines
drivers/pwm/pwm-bcm-kona.c +6
-0value |=1 <<PWM_CONTROL_SMOOTH_SHIFT (chan); value &= ~(1 <<PWM_CONTROL_TRIGGER_SHIFT (chan));writel (value, kp->base +PWM_CONTROL_OFFSET ); +/* +* There must be a min 400ns delay between clearing enable and setting +* it. Failing to do this may result in no PWM signal. +*/ +ndelay (400 ); +}Since it doesn't function as an enable, please call it the trigger bit.
drivers/pwm/pwm-bcm-kona.c static inline struct kona_pwmc *to_kona_pwmc(struct pwm_chip *_chip) · line 109
Quoted 13 lines
drivers/pwm/pwm-bcm-kona.c +8
-0+ +static void kona_pwmc_apply_settings (struct kona_pwmc *kp,unsigned int chan) +{ +unsigned int value =readl (kp->base +PWM_CONTROL_OFFSET ); +/* Set trigger bit and clear smooth bit to apply new settings */ value &= ~(1 <<PWM_CONTROL_SMOOTH_SHIFT (chan)); value |=1 <<PWM_CONTROL_TRIGGER_SHIFT (chan);writel (value, kp->base +PWM_CONTROL_OFFSET ); + +/* PWMOUT_ENABLE must be held high for at least 400 ns. */ +ndelay (400 ); }Same here.
Quoted 64 lines
drivers/pwm/pwm-bcm-kona.c +19
-8static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,@@ -133,8 +150,14 @@ 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 */ +/* +* Don't apply settings if disabled. The period and duty cycle are +* always calculated above to ensure the new values are +* validated immediately instead of on enable. +*/ if (test_bit (PWMF_ENABLED , &pwm->flags)) { +kona_pwmc_prepare_for_settings (kp, chan); + value =readl (kp->base +PRESCALE_OFFSET ); value &= ~PRESCALE_MASK (chan); value |= prescale <<PRESCALE_SHIFT (chan);@@ -164,6 +187,8 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,return ret; } +kona_pwmc_prepare_for_settings (kp, chan); + value =readl (kp->base +PWM_CONTROL_OFFSET );if (polarity ==PWM_POLARITY_NORMAL )@@ -175,9 +200,6 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,kona_pwmc_apply_settings (kp, chan); -/* Wait for waveform to settle before gating off the clock */ -ndelay (400 ); -clk_disable_unprepare (kp->clk);return 0 ;@@ -207,13 +229,20 @@ 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; +unsigned int value; + +kona_pwmc_prepare_for_settings (kp, chan);/* Simulate a disable by configuring for zero duty */ writel (0 , kp->base +DUTY_CYCLE_HIGH_OFFSET (chan)); -kona_pwmc_apply_settings (kp, chan); +writel (0 , kp->base +PERIOD_COUNT_OFFSET (chan)); -/* Wait for waveform to settle before gating off the clock */ -ndelay (400 ); +/* Set prescale to 0 for this channel */ + value =readl (kp->base +PRESCALE_OFFSET ); + value &= ~PRESCALE_MASK (chan); +writel (value, kp->base +PRESCALE_OFFSET ); + +kona_pwmc_apply_settings (kp, chan);clk_disable_unprepare (kp->clk); } --1.7.9.5
PATCH v8 4/5 pwm: kona: Add debug info to config function
In reply to Jonathan RichardsonAdds debugging info to config function where duty cycle and period are calculated and verified.
--- drivers/pwm/pwm-bcm-kona.c | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-)
drivers/pwm/pwm-bcm-kona.c +23 -2
@@ -138,18 +138,39 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm, dc =div64_u64 (val, div);/* If duty_ns or period_ns are not achievable then return */ -if (pc <PERIOD_COUNT_MIN || dc <DUTY_CYCLE_HIGH_MIN ) +if (pc <PERIOD_COUNT_MIN ) { +dev_warn (chip->dev, +"%s: pwm[%d]: period=%d is not achievable, pc=%lu, prescale=%lu\n" , + __func__, chan, period_ns, pc, prescale);return -EINVAL ; + } + +if (dc <DUTY_CYCLE_HIGH_MIN ) { +if (0 != duty_ns) { +dev_warn (chip->dev, +"%s: pwm[%d]: duty cycle=%d is not achievable, dc=%lu, prescale=%lu\n" , + __func__, chan, duty_ns, dc, prescale); + } +return -EINVAL ; + }/* If pc and dc are in bounds, the calculation is done */ if (pc <=PERIOD_COUNT_MAX && dc <=DUTY_CYCLE_HIGH_MAX )break ;/* Otherwise, increase prescale and recalculate pc and dc */ -if (++prescale >PRESCALE_MAX ) +if (++prescale >PRESCALE_MAX ) { +dev_warn (chip->dev, +"%s: pwm[%d]: Prescale (=%lu) within max (=%d) for period=%d and duty cycle=%d is not achievable\n" , + __func__, chan, prescale,PRESCALE_MAX , + period_ns, duty_ns);return -EINVAL ; + } } +dev_dbg (chip->dev,"pwm[%d]: period=%lu, duty_high=%lu, prescale=%lu\n" , + chan, pc, dc, prescale); +/* * Don't apply settings if disabled. The period and duty cycle are * always calculated above to ensure the new values are
Signature · 1 line
1.7.9.5
Re: [PATCH v8 4/5] pwm: kona: Add debug info to config function
Quoted 13 lines · Jonathan Richardson
Adds debugging info to config function where duty cycle and period are calculated and verified.
Signed-off-by Jonathan Richardson <[email protected]>--- drivers/pwm/pwm-bcm-kona.c | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-)drivers/pwm/pwm-bcm-kona.c +0
-0
drivers/pwm/pwm-bcm-kona.c static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 141
Quoted 4 lines
drivers/pwm/pwm-bcm-kona.c +0
-1@@ -138,18 +138,39 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm, dc =div64_u64 (val, div);/* If duty_ns or period_ns are not achievable then return */ -if (pc <PERIOD_COUNT_MIN || dc <DUTY_CYCLE_HIGH_MIN )
The original code was based on the SPEAr PWM driver which has a non-zero PWMDCR_MIN_DUTY such that the second condition here can evaluate to true.
This isn't the case for the Kona PWM where DUTY_CYCLE_HIGH_MIN is zero.
drivers/pwm/pwm-bcm-kona.c static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 146
Quoted 6 lines
drivers/pwm/pwm-bcm-kona.c +5
-0+if (pc <PERIOD_COUNT_MIN ) { +dev_warn (chip->dev, +"%s: pwm[%d]: period=%d is not achievable, pc=%lu, prescale=%lu\n" , + __func__, chan, period_ns, pc, prescale);return -EINVAL ; + }
Why not just print the minimum allowable period with the provided clock?
I don't think pc and prescale will be particularly helpful to users.
Also, do we really need to print __func__ here?
drivers/pwm/pwm-bcm-kona.c static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 155
Quoted 9 lines
drivers/pwm/pwm-bcm-kona.c +9
-0+ +if (dc <DUTY_CYCLE_HIGH_MIN ) { +if (0 != duty_ns) { +dev_warn (chip->dev, +"%s: pwm[%d]: duty cycle=%d is not achievable, dc=%lu, prescale=%lu\n" , + __func__, chan, duty_ns, dc, prescale); + } +return -EINVAL ; + }
The above block is unreachable code.
Quoted 5 lines
drivers/pwm/pwm-bcm-kona.c +0
-0/* If pc and dc are in bounds, the calculation is done */ if (pc <=PERIOD_COUNT_MAX && dc <=DUTY_CYCLE_HIGH_MAX )break ;
drivers/pwm/pwm-bcm-kona.c static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 169
Quoted 10 lines
drivers/pwm/pwm-bcm-kona.c +6
-1/* Otherwise, increase prescale and recalculate pc and dc */ -if (++prescale >PRESCALE_MAX ) +if (++prescale >PRESCALE_MAX ) { +dev_warn (chip->dev, +"%s: pwm[%d]: Prescale (=%lu) within max (=%d) for period=%d and duty cycle=%d is not achievable\n" , + __func__, chan, prescale,PRESCALE_MAX , + period_ns, duty_ns);return -EINVAL ; + } }
The user got here because they specified a period larger than the maximum supported so why not tell them largest value that can be supported instead of confusing them with prescale and PRESCALE_MAX?
drivers/pwm/pwm-bcm-kona.c static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 173
Quoted 4 lines
drivers/pwm/pwm-bcm-kona.c +3
-0+dev_dbg (chip->dev,"pwm[%d]: period=%lu, duty_high=%lu, prescale=%lu\n" , + chan, pc, dc, prescale); +
This could be more clear. It prints pc but calls it period.
Quoted 6 lines
drivers/pwm/pwm-bcm-kona.c +0
-1/* * Don't apply settings if disabled. The period and duty cycle are * always calculated above to ensure the new values are -- 1.7.9.5
Re: [PATCH v8 4/5] pwm: kona: Add debug info to config function
Quoted 71 lines · Tim Kryger
Quoted 13 lines · Jonathan Richardson
Adds debugging info to config function where duty cycle and period are calculated and verified.
Signed-off-by Jonathan Richardson <[email protected]>--- drivers/pwm/pwm-bcm-kona.c | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-)drivers/pwm/pwm-bcm-kona.c +0
-0drivers/pwm/pwm-bcm-kona.c static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 141
Quoted 4 lines
drivers/pwm/pwm-bcm-kona.c +0
-1@@ -138,18 +138,39 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm, dc =div64_u64 (val, div);/* If duty_ns or period_ns are not achievable then return */ -if (pc <PERIOD_COUNT_MIN || dc <DUTY_CYCLE_HIGH_MIN )The original code was based on the SPEAr PWM driver which has a non-zero
PWMDCR_MIN_DUTYsuch that the second condition here can evaluate to true.This isn't the case for the Kona PWM where
DUTY_CYCLE_HIGH_MINis zero.drivers/pwm/pwm-bcm-kona.c static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 146
Quoted 6 lines
drivers/pwm/pwm-bcm-kona.c +5
-0+if (pc <PERIOD_COUNT_MIN ) { +dev_warn (chip->dev, +"%s: pwm[%d]: period=%d is not achievable, pc=%lu, prescale=%lu\n" , + __func__, chan, period_ns, pc, prescale);return -EINVAL ; + }Why not just print the minimum allowable period with the provided clock?
I don't think pc and prescale will be particularly helpful to users.
Also, do we really need to print
__func__here?drivers/pwm/pwm-bcm-kona.c static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 155
Quoted 9 lines
drivers/pwm/pwm-bcm-kona.c +9
-0+ +if (dc <DUTY_CYCLE_HIGH_MIN ) { +if (0 != duty_ns) { +dev_warn (chip->dev, +"%s: pwm[%d]: duty cycle=%d is not achievable, dc=%lu, prescale=%lu\n" , + __func__, chan, duty_ns, dc, prescale); + } +return -EINVAL ; + }The above block is unreachable code.
Quoted 5 lines
drivers/pwm/pwm-bcm-kona.c +0
-0/* If pc and dc are in bounds, the calculation is done */ if (pc <=PERIOD_COUNT_MAX && dc <=DUTY_CYCLE_HIGH_MAX )break ;drivers/pwm/pwm-bcm-kona.c static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 169
Quoted 10 lines
drivers/pwm/pwm-bcm-kona.c +6
-1/* Otherwise, increase prescale and recalculate pc and dc */ -if (++prescale >PRESCALE_MAX ) +if (++prescale >PRESCALE_MAX ) { +dev_warn (chip->dev, +"%s: pwm[%d]: Prescale (=%lu) within max (=%d) for period=%d and duty cycle=%d is not achievable\n" , + __func__, chan, prescale,PRESCALE_MAX , + period_ns, duty_ns);return -EINVAL ; + } }The user got here because they specified a period larger than the maximum supported so why not tell them largest value that can be supported instead of confusing them with prescale and
PRESCALE_MAX?Quoted 1 line
Quoted 12 lines
drivers/pwm/pwm-bcm-kona.c static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 173
Quoted 3 lines
drivers/pwm/pwm-bcm-kona.c +3
-0+dev_dbg (chip->dev,"pwm[%d]: period=%lu, duty_high=%lu, prescale=%lu\n" , + chan, pc, dc, prescale); +This could be more clear. It prints pc but calls it period.
Quoted 6 lines
/*
* Don't apply settings if disabled. The period and duty cycle are
* always calculated above to ensure the new values are
--
1.7.9.5
We can defer this for now until I can look into it further. It's not a priority. I'm more concerned with core changes and kona pwm fix.
Thanks,
Jon
PATCH v8 5/5 pwm: core: Set enable state properly on failed call to enable
In reply to Jonathan RichardsonThe pwm_enable function didn't clear the enabled bit if a call to a clients enable function returned an error. The result was that the state of the pwm core was wrong. Clearing the bit when enable returns an error ensures the state is properly set.
--- drivers/pwm/core.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)
drivers/pwm/core.c +13 -3
@@ -477,10 +477,20 @@ EXPORT_SYMBOL_GPL(pwm_set_polarity);*/ int pwm_enable (struct pwm_device *pwm) { -if (pwm && !test_and_set_bit (PWMF_ENABLED , &pwm->flags)) -return pwm->chip->ops->enable (pwm->chip, pwm); +int err; + +if (!pwm) +return -EINVAL ; + +if (!test_and_set_bit (PWMF_ENABLED , &pwm->flags)) { + err = pwm->chip->ops->enable (pwm->chip, pwm); +if (err) { +clear_bit (PWMF_ENABLED , &pwm->flags); +return err; + } + } -return pwm ?0 : -EINVAL ; +return 0 ; }EXPORT_SYMBOL_GPL (pwm_enable);
Signature · 1 line
1.7.9.5
Re: [PATCH v8 5/5] pwm: core: Set enable state properly on failed call to enable
Quoted 24 lines · Jonathan Richardson
The
pwm_enablefunction didn't clear the enabled bit if a call to a clients enable function returned an error. The result was that the state of the pwm core was wrong. Clearing the bit when enable returns an error ensures the state is properly set.Tested-by Jonathan Richardson <[email protected]>Reviewed-by Dmitry Torokhov <[email protected]>Signed-off-by Jonathan Richardson <[email protected]>--- drivers/pwm/core.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)drivers/pwm/core.c +2
-2@@ -477,10 +477,20 @@ EXPORT_SYMBOL_GPL(pwm_set_polarity);*/ int pwm_enable (struct pwm_device *pwm) { -if (pwm && !test_and_set_bit (PWMF_ENABLED , &pwm->flags)) -return pwm->chip->ops->enable (pwm->chip, pwm); +int err; +
drivers/pwm/core.c EXPORT_SYMBOL_GPL(pwm_set_polarity); · line 491
Quoted 10 lines
drivers/pwm/core.c +10
-0+if (!pwm) +return -EINVAL ; + +if (!test_and_set_bit (PWMF_ENABLED , &pwm->flags)) { + err = pwm->chip->ops->enable (pwm->chip, pwm); +if (err) { +clear_bit (PWMF_ENABLED , &pwm->flags); +return err; + } + }
I think with this new pattern we're now actually going to need a lock to make sure pwm->flags doesn't change between the test_and_set_bit() and clear_bit() calls.
Thierry
Re: [PATCH v8 5/5] pwm: core: Set enable state properly on failed call to enable
Quoted 40 lines · Thierry Reding
Quoted 24 lines · Jonathan Richardson
The
pwm_enablefunction didn't clear the enabled bit if a call to a clients enable function returned an error. The result was that the state of the pwm core was wrong. Clearing the bit when enable returns an error ensures the state is properly set.Tested-by Jonathan Richardson <[email protected]>Reviewed-by Dmitry Torokhov <[email protected]>Signed-off-by Jonathan Richardson <[email protected]>--- drivers/pwm/core.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)drivers/pwm/core.c +2
-2@@ -477,10 +477,20 @@ EXPORT_SYMBOL_GPL(pwm_set_polarity);*/ int pwm_enable (struct pwm_device *pwm) { -if (pwm && !test_and_set_bit (PWMF_ENABLED , &pwm->flags)) -return pwm->chip->ops->enable (pwm->chip, pwm); +int err; +drivers/pwm/core.c EXPORT_SYMBOL_GPL(pwm_set_polarity); · line 491
Quoted 10 lines
drivers/pwm/core.c +10
-0+if (!pwm) +return -EINVAL ; + +if (!test_and_set_bit (PWMF_ENABLED , &pwm->flags)) { + err = pwm->chip->ops->enable (pwm->chip, pwm); +if (err) { +clear_bit (PWMF_ENABLED , &pwm->flags); +return err; + } + }I think with this new pattern we're now actually going to need a lock to make sure
pwm->flagsdoesn't change between thetest_and_set_bit()andclear_bit()calls.
Quoted 2 lines
Thierry
Ok. I'll add a lock per pwm_device and re-send the patch.
Jon