This patchset contains fixes for Broadcom's Kona PWM driver.
These changes fix glitch issues when changing settings on different channels.
Kconfig change made to allow the driver to work on any Broadcom SoC rather
than just mobile devices.
Arun Ramamurthy (4):
pwm: kona: Remove setting default smooth type and polarity for all
channels
pwm: kona: Fix incorrect enable after channel polarity change
pwm: kona: Fix enable, disable and config procedures
pwm: kona: Update dependency to ARCH_BCM
drivers/pwm/Kconfig | 2 +-
drivers/pwm/pwm-bcm-kona.c | 112 ++++++++++++++++++++++++++++++++-------------
2 files changed, 81 insertions(+), 33 deletions(-)
--
2.1.3
From: Arun Ramamurthy <[email protected]>
change kona driver depends on so to it is available for any Broadcom SoC
Signed-off-by: Arun Ramamurthy <[email protected]>
Reviewed-by: Ray Jui <[email protected]>
Signed-off-by: Scott Branden <[email protected]>
---
drivers/pwm/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index ef2dd2e..186080e 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -64,7 +64,7 @@ config PWM_ATMEL_TCB
config PWM_BCM_KONA
tristate "Kona PWM support"
- depends on ARCH_BCM_MOBILE
+ depends on ARCH_BCM
help
Generic PWM framework driver for Broadcom Kona PWM block.
--
2.1.3
From: Arun Ramamurthy <[email protected]>
- 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 <[email protected]>
Reviewed-by: Ray Jui <[email protected]>
Signed-off-by: Scott Branden <[email protected]>
---
drivers/pwm/pwm-bcm-kona.c | 100 +++++++++++++++++++++++++++++++++++----------
1 file changed, 78 insertions(+), 22 deletions(-)
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);
/* 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);
+ kona_pwmc_set_trigger(kp, chan);
clk_disable_unprepare(kp->clk);
}
--
2.1.3
From: Arun Ramamurthy <[email protected]>
The pwm core code requires a separate call for enabling the channel
and hence the driver does not need to set pwm_trigger after a
polarity change
Signed-off-by: Arun Ramamurthy <[email protected]>
Reviewed-by: Ray Jui <[email protected]>
Signed-off-by: Scott Branden <[email protected]>
---
drivers/pwm/pwm-bcm-kona.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index 29eef9e..fa0b5bf 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -173,11 +173,6 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
writel(value, kp->base + PWM_CONTROL_OFFSET);
- 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;
--
2.1.3
From: Arun Ramamurthy <[email protected]>
The probe routine unnecessarily sets the smooth type and polarity for
all channels. This causes the channel for the speaker to click at the same
time the backlight turns on. The smooth type and polarity should be set individually
for each channel as required and no defaults need to be set.
Signed-off-by: Arun Ramamurthy <[email protected]>
Reviewed-by: Ray Jui <[email protected]>
Signed-off-by: Scott Branden <[email protected]>
---
drivers/pwm/pwm-bcm-kona.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index 02bc048..29eef9e 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -266,12 +266,9 @@ 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);
--
2.1.3
On Tue, Nov 25, 2014 at 11:40 AM, Scott Branden <[email protected]> wrote:
> From: Arun Ramamurthy <[email protected]>
>
> The probe routine unnecessarily sets the smooth type and polarity for
> all channels. This causes the channel for the speaker to click at the same
> time the backlight turns on. The smooth type and polarity should be set individually
> for each channel as required and no defaults need to be set.
I am guessing you are talking about a PWM controlled beeper/buzzer.
Can you mention what board you are observing this issue on?
Also please explain why setting these bits result in an audible click.
>
> Signed-off-by: Arun Ramamurthy <[email protected]>
> Reviewed-by: Ray Jui <[email protected]>
> Signed-off-by: Scott Branden <[email protected]>
> ---
> drivers/pwm/pwm-bcm-kona.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
> index 02bc048..29eef9e 100644
> --- a/drivers/pwm/pwm-bcm-kona.c
> +++ b/drivers/pwm/pwm-bcm-kona.c
> @@ -266,12 +266,9 @@ 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);
While the smooth bit need not be set here, it is important that the
polarity bit be set.
Otherwise software will report the polarity as normal when it it is
actually inversed.
Consider the case where a userspace process is controlling the PWM via sysfs.
On Tue, Nov 25, 2014 at 11:40 AM, Scott Branden <[email protected]> wrote:
> From: Arun Ramamurthy <[email protected]>
>
> The pwm core code requires a separate call for enabling the channel
> and hence the driver does not need to set pwm_trigger after a
> polarity change
The framework does restrict when polarity changes can occur but it
isn't clear to me that there is any reason to delay applying the
polarity change. Keep in mind that polarity matters even when a PWM
is disabled. While disabled, the output should be equivalent to an
enabled configuration with zero duty. Thus for normal polarity the
output is constant low and for inversed polarity the output is
constant high. I believe there is an expectation that the output is
updated to reflect the requested polarity change prior to returning to
the caller.
>
> Signed-off-by: Arun Ramamurthy <[email protected]>
> Reviewed-by: Ray Jui <[email protected]>
> Signed-off-by: Scott Branden <[email protected]>
> ---
> drivers/pwm/pwm-bcm-kona.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
> index 29eef9e..fa0b5bf 100644
> --- a/drivers/pwm/pwm-bcm-kona.c
> +++ b/drivers/pwm/pwm-bcm-kona.c
> @@ -173,11 +173,6 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
>
> writel(value, kp->base + PWM_CONTROL_OFFSET);
>
> - 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;
> --
> 2.1.3
>
On Tue, Nov 25, 2014 at 11:40 AM, Scott Branden <[email protected]> wrote:
> From: Arun Ramamurthy <[email protected]>
>
> - 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 <[email protected]>
> Reviewed-by: Ray Jui <[email protected]>
> Signed-off-by: Scott Branden <[email protected]>
> ---
> 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
>
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 <[email protected]> wrote:
>> From: Arun Ramamurthy <[email protected]>
>>
>> - 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 <[email protected]>
>> Reviewed-by: Ray Jui <[email protected]>
>> Signed-off-by: Scott Branden <[email protected]>
>> ---
>> 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
>>
On 14-11-25 09:51 PM, Tim Kryger wrote:
> On Tue, Nov 25, 2014 at 11:40 AM, Scott Branden <[email protected]> wrote:
>> From: Arun Ramamurthy <[email protected]>
>>
>> The probe routine unnecessarily sets the smooth type and polarity for
>> all channels. This causes the channel for the speaker to click at the same
>> time the backlight turns on. The smooth type and polarity should be set individually
>> for each channel as required and no defaults need to be set.
>
> I am guessing you are talking about a PWM controlled beeper/buzzer.
>
This change is more so to remove setting smooth type and polarity for
all channels during probe and to leave them as their default values.
Infact, setting the PWM_CONTROL_TYPE_SHIT is also redundant cause the
default value is already 1 for all channels. We can remove that loop
entirely and this will be done in the next patch set. The smooth type
and polarity are only changed when the particular pwm channel is enabled
or polarity is changed.
> Can you mention what board you are observing this issue on?
>
> Also please explain why setting these bits result in an audible click.
>
We observe this on the bcm958300K board where one of the
PWM channels is connected to the buzzer and changing the
smooth type and polarity from its default values causes a click
>>
>> Signed-off-by: Arun Ramamurthy <[email protected]>
>> Reviewed-by: Ray Jui <[email protected]>
>> Signed-off-by: Scott Branden <[email protected]>
>> ---
>> drivers/pwm/pwm-bcm-kona.c | 7 ++-----
>> 1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
>> index 02bc048..29eef9e 100644
>> --- a/drivers/pwm/pwm-bcm-kona.c
>> +++ b/drivers/pwm/pwm-bcm-kona.c
>> @@ -266,12 +266,9 @@ 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);
>
> While the smooth bit need not be set here, it is important that the
> polarity bit be set.
>
The default value for polarity is 0 which is normal polarity, so setting
it to 1 here in the probe function without a sysfs call is
when the software will report the polarity as normal when it is actually
inversed.
> Otherwise software will report the polarity as normal when it it is
> actually inversed.
>
> Consider the case where a userspace process is controlling the PWM via sysfs.
>
I agree with you about the sysfs case Tim, but since this is the probe
function and not a sysfs callback, should we not leave it as the default
value?
On 14-11-25 10:22 PM, Tim Kryger wrote:
> On Tue, Nov 25, 2014 at 11:40 AM, Scott Branden <[email protected]> wrote:
>> From: Arun Ramamurthy <[email protected]>
>>
>> The pwm core code requires a separate call for enabling the channel
>> and hence the driver does not need to set pwm_trigger after a
>> polarity change
>
> The framework does restrict when polarity changes can occur but it
> isn't clear to me that there is any reason to delay applying the
> polarity change.
I examined several other drivers such as pwm-atmel-tcb.c, pwm-ep93xx.c,
pwm-renesas-tpu.c, pwm-samsung.c in the 3.17 kernel tree and none of
them enable the channel after changing polarity. We would be the first
driver to do so.
> Keep in mind that polarity matters even when a PWM
> is disabled. While disabled, the output should be equivalent to an
> enabled configuration with zero duty. Thus for normal polarity the
> output is constant low and for inversed polarity the output is
> constant high.
The driver does set the duty cycle to zero when disabling the pwm
channel.However since the frame work prevents polarity change when the
pwm is enabled, I don’t see how one could expect the polarity change to
be reflected immediately without a separate call to pwm enable.
I believe there is an expectation that the output is
> updated to reflect the requested polarity change prior to returning to
> the caller.
Once again I disagree with this based on other pwm drivers which only
change the polarity and do not enable the channel when their set
polarity functions are called.
>
>>
>> Signed-off-by: Arun Ramamurthy <[email protected]>
>> Reviewed-by: Ray Jui <[email protected]>
>> Signed-off-by: Scott Branden <[email protected]>
>> ---
>> drivers/pwm/pwm-bcm-kona.c | 5 -----
>> 1 file changed, 5 deletions(-)
>>
>> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
>> index 29eef9e..fa0b5bf 100644
>> --- a/drivers/pwm/pwm-bcm-kona.c
>> +++ b/drivers/pwm/pwm-bcm-kona.c
>> @@ -173,11 +173,6 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
>>
>> writel(value, kp->base + PWM_CONTROL_OFFSET);
>>
>> - 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;
>> --
>> 2.1.3
>>
On 14-11-25 11:29 PM, Tim Kryger wrote:
> On Tue, Nov 25, 2014 at 11:40 AM, Scott Branden <[email protected]> wrote:
>> From: Arun Ramamurthy <[email protected]>
>>
>> - 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 <[email protected]>
>> Reviewed-by: Ray Jui <[email protected]>
>> Signed-off-by: Scott Branden <[email protected]>
>> ---
>> 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
>>
On Fri, Nov 28, 2014 at 3:47 PM, Arun Ramamurthy
<[email protected]> wrote:
>
>
> On 14-11-25 09:51 PM, Tim Kryger wrote:
>>
>> On Tue, Nov 25, 2014 at 11:40 AM, Scott Branden <[email protected]>
>> wrote:
>>>
>>> From: Arun Ramamurthy <[email protected]>
>>>
>>> The probe routine unnecessarily sets the smooth type and polarity for
>>> all channels. This causes the channel for the speaker to click at the
>>> same
>>> time the backlight turns on. The smooth type and polarity should be set
>>> individually
>>> for each channel as required and no defaults need to be set.
>>
>>
>> I am guessing you are talking about a PWM controlled beeper/buzzer.
>>
> This change is more so to remove setting smooth type and polarity for all
> channels during probe and to leave them as their default values. Infact,
> setting the PWM_CONTROL_TYPE_SHIT is also redundant cause the default value
> is already 1 for all channels. We can remove that loop entirely and this
> will be done in the next patch set. The smooth type and polarity are only
> changed when the particular pwm channel is enabled or polarity is changed.
>
>> Can you mention what board you are observing this issue on?
>>
>> Also please explain why setting these bits result in an audible click.
>>
> We observe this on the bcm958300K board where one of the
> PWM channels is connected to the buzzer and changing the
> smooth type and polarity from its default values causes a click
>
Which of these two bits is causing the click?
I've already said that I'm open to removing the smooth bit here if that helps.
>>>
>>> Signed-off-by: Arun Ramamurthy <[email protected]>
>>> Reviewed-by: Ray Jui <[email protected]>
>>> Signed-off-by: Scott Branden <[email protected]>
>>> ---
>>> drivers/pwm/pwm-bcm-kona.c | 7 ++-----
>>> 1 file changed, 2 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
>>> index 02bc048..29eef9e 100644
>>> --- a/drivers/pwm/pwm-bcm-kona.c
>>> +++ b/drivers/pwm/pwm-bcm-kona.c
>>> @@ -266,12 +266,9 @@ 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);
>>
>>
>> While the smooth bit need not be set here, it is important that the
>> polarity bit be set.
>>
> The default value for polarity is 0 which is normal polarity, so setting it
> to 1 here in the probe function without a sysfs call is
> when the software will report the polarity as normal when it is actually
> inversed.
Please double check the meaning of the polarity bits for the revision
of PWM IP in your chip. I suspect you are mistaken here.
This driver is for PWM blocks compatible those found in bcm28145,
bcm28155, bcm21664, and other mobile chips of that generation.
Apparently in contrast to the chip you are using, a set polarity bit
in the control register means normal polarity for the chips I
mentioned.
A default of zero for these bits means they must be set to meet the
PWM framework's expectation that channels begin with normal polarity.
>>
>> Otherwise software will report the polarity as normal when it it is
>> actually inversed.
>>
>> Consider the case where a userspace process is controlling the PWM via
>> sysfs.
>>
> I agree with you about the sysfs case Tim, but since this is the probe
> function and not a sysfs callback, should we not leave it as the default
> value?
On 14-11-28 05:08 PM, Tim Kryger wrote:
> On Fri, Nov 28, 2014 at 3:47 PM, Arun Ramamurthy
> <[email protected]> wrote:
>>
>>
>> On 14-11-25 09:51 PM, Tim Kryger wrote:
>>>
>>> On Tue, Nov 25, 2014 at 11:40 AM, Scott Branden <[email protected]>
>>> wrote:
>>>>
>>>> From: Arun Ramamurthy <[email protected]>
>>>>
>>>> The probe routine unnecessarily sets the smooth type and polarity for
>>>> all channels. This causes the channel for the speaker to click at the
>>>> same
>>>> time the backlight turns on. The smooth type and polarity should be set
>>>> individually
>>>> for each channel as required and no defaults need to be set.
>>>
>>>
>>> I am guessing you are talking about a PWM controlled beeper/buzzer.
>>>
>> This change is more so to remove setting smooth type and polarity for all
>> channels during probe and to leave them as their default values. Infact,
>> setting the PWM_CONTROL_TYPE_SHIT is also redundant cause the default value
>> is already 1 for all channels. We can remove that loop entirely and this
>> will be done in the next patch set. The smooth type and polarity are only
>> changed when the particular pwm channel is enabled or polarity is changed.
>>
>>> Can you mention what board you are observing this issue on?
>>>
>>> Also please explain why setting these bits result in an audible click.
>>>
>> We observe this on the bcm958300K board where one of the
>> PWM channels is connected to the buzzer and changing the
>> smooth type and polarity from its default values causes a click
>>
>
> Which of these two bits is causing the click?
>
> I've already said that I'm open to removing the smooth bit here if that helps.
>
Thank you for your quick reply Tim. It is setting the polarity bit that
causes the click. I am planning on removing this entire loop in the next
patch set, are you okay with that?
>>>>
>>>> Signed-off-by: Arun Ramamurthy <[email protected]>
>>>> Reviewed-by: Ray Jui <[email protected]>
>>>> Signed-off-by: Scott Branden <[email protected]>
>>>> ---
>>>> drivers/pwm/pwm-bcm-kona.c | 7 ++-----
>>>> 1 file changed, 2 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
>>>> index 02bc048..29eef9e 100644
>>>> --- a/drivers/pwm/pwm-bcm-kona.c
>>>> +++ b/drivers/pwm/pwm-bcm-kona.c
>>>> @@ -266,12 +266,9 @@ 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);
>>>
>>>
>>> While the smooth bit need not be set here, it is important that the
>>> polarity bit be set.
>>>
>> The default value for polarity is 0 which is normal polarity, so setting it
>> to 1 here in the probe function without a sysfs call is
>> when the software will report the polarity as normal when it is actually
>> inversed.
>
> Please double check the meaning of the polarity bits for the revision
> of PWM IP in your chip. I suspect you are mistaken here.
>
> This driver is for PWM blocks compatible those found in bcm28145,
> bcm28155, bcm21664, and other mobile chips of that generation.
>
> Apparently in contrast to the chip you are using, a set polarity bit
> in the control register means normal polarity for the chips I
> mentioned.
>
> A default of zero for these bits means they must be set to meet the
> PWM framework's expectation that channels begin with normal polarity.
>
Tim, this is from the RDB of our new chip which is supposed to have the
same IP as the mobile chip sets you mentioned:
When set to 1 the output polarity for the PWM Output signal will be
active hight; When set to 0, the output polarity for the PWM Output
signal will be active low. Default State is 0.
My understanding is that the frameworks normal polarity means active
low, am I mistaken in that?
>>>
>>> Otherwise software will report the polarity as normal when it it is
>>> actually inversed.
>>>
>>> Consider the case where a userspace process is controlling the PWM via
>>> sysfs.
>>>
>> I agree with you about the sysfs case Tim, but since this is the probe
>> function and not a sysfs callback, should we not leave it as the default
>> value?
On Fri, Nov 28, 2014 at 3:48 PM, Arun Ramamurthy
<[email protected]> wrote:
>
>
> On 14-11-25 10:22 PM, Tim Kryger wrote:
>>
>> On Tue, Nov 25, 2014 at 11:40 AM, Scott Branden <[email protected]>
>> wrote:
>>>
>>> From: Arun Ramamurthy <[email protected]>
>>>
>>> The pwm core code requires a separate call for enabling the channel
>>> and hence the driver does not need to set pwm_trigger after a
>>> polarity change
>>
>>
>> The framework does restrict when polarity changes can occur but it
>> isn't clear to me that there is any reason to delay applying the
>> polarity change.
>
> I examined several other drivers such as pwm-atmel-tcb.c, pwm-ep93xx.c,
> pwm-renesas-tpu.c, pwm-samsung.c in the 3.17 kernel tree and none of them
> enable the channel after changing polarity. We would be the first driver to
> do so.
We are not "enabling" the channel as much as we are "triggering an
application of settings" by hardware.
Both pwm-ep93xx and pwm-samsung write polarity settings to the
hardware immediately, presumably resulting in an immediate change in
output.
Alternatively, the pwm-atmel-tcb and pwm-renesas-tpu drivers save the
polarity and write it to hardware later.
There may be advantages to deferring the application of the polarity
till a subsequent enable but both approaches appear to be acceptable.
>
>> Keep in mind that polarity matters even when a PWM
>> is disabled. While disabled, the output should be equivalent to an
>> enabled configuration with zero duty. Thus for normal polarity the
>> output is constant low and for inversed polarity the output is
>> constant high.
>
> The driver does set the duty cycle to zero when disabling the pwm
> channel.However since the frame work prevents polarity change when the pwm
> is enabled, I don’t see how one could expect the polarity change to be
> reflected immediately without a separate call to pwm enable.
>
>
>> I believe there is an expectation that the output is
>> updated to reflect the requested polarity change prior to returning to
>> the caller.
>
>
> Once again I disagree with this based on other pwm drivers which only change
> the polarity and do not enable the channel when their set polarity functions
> are called.
I don't know why you keep calling this an enable. Its not an enable,
it is only a trigger.
Perhaps this would be best explained with an example:
# Export PWM for access by userspace
cd /sys/class/pwm/pwmchip0
echo 0 > export
cd pwm0
# Request 50% duty output when PWM is enabled
echo 50000 > duty_cycle
echo 100000 > period
# Command Inversed Polarity
echo inversed > polarity
# Command Normal Polarity
echo normal > polarity
# Enable PWM
echo 1 > enable
The polarity changes trigger immediate output updates but the PWM is
not enabled until the end.
Prior to the last step the output is either a constant high or low
signal, not the 50% duty waveform.
>
>
>>
>>>
>>> Signed-off-by: Arun Ramamurthy <[email protected]>
>>> Reviewed-by: Ray Jui <[email protected]>
>>> Signed-off-by: Scott Branden <[email protected]>
>>> ---
>>> drivers/pwm/pwm-bcm-kona.c | 5 -----
>>> 1 file changed, 5 deletions(-)
>>>
>>> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
>>> index 29eef9e..fa0b5bf 100644
>>> --- a/drivers/pwm/pwm-bcm-kona.c
>>> +++ b/drivers/pwm/pwm-bcm-kona.c
>>> @@ -173,11 +173,6 @@ static int kona_pwmc_set_polarity(struct pwm_chip
>>> *chip, struct pwm_device *pwm,
>>>
>>> writel(value, kp->base + PWM_CONTROL_OFFSET);
>>>
>>> - 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;
>>> --
>>> 2.1.3
>>>
>
On Fri, Nov 28, 2014 at 3:49 PM, Arun Ramamurthy
<[email protected]> wrote:
>
>
> On 14-11-25 11:29 PM, Tim Kryger wrote:
>>
>> On Tue, Nov 25, 2014 at 11:40 AM, Scott Branden <[email protected]>
>> wrote:
>>>
>>> From: Arun Ramamurthy <[email protected]>
>>>
>>> - 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 <[email protected]>
>>> Reviewed-by: Ray Jui <[email protected]>
>>> Signed-off-by: Scott Branden <[email protected]>
>>> ---
>>> 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.
>
>> 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.
> 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
>>>
>
On Fri, Nov 28, 2014 at 5:19 PM, Arun Ramamurthy
<[email protected]> wrote:
>
>
> On 14-11-28 05:08 PM, Tim Kryger wrote:
>>
>> On Fri, Nov 28, 2014 at 3:47 PM, Arun Ramamurthy
>> <[email protected]> wrote:
>>>
>>>
>>>
>>> On 14-11-25 09:51 PM, Tim Kryger wrote:
>>>>
>>>>
>>>> On Tue, Nov 25, 2014 at 11:40 AM, Scott Branden <[email protected]>
>>>> wrote:
>>>>>
>>>>>
>>>>> From: Arun Ramamurthy <[email protected]>
>>>>>
>>>>> The probe routine unnecessarily sets the smooth type and polarity for
>>>>> all channels. This causes the channel for the speaker to click at the
>>>>> same
>>>>> time the backlight turns on. The smooth type and polarity should be set
>>>>> individually
>>>>> for each channel as required and no defaults need to be set.
>>>>
>>>>
>>>>
>>>> I am guessing you are talking about a PWM controlled beeper/buzzer.
>>>>
>>> This change is more so to remove setting smooth type and polarity for all
>>> channels during probe and to leave them as their default values. Infact,
>>> setting the PWM_CONTROL_TYPE_SHIT is also redundant cause the default
>>> value
>>> is already 1 for all channels. We can remove that loop entirely and this
>>> will be done in the next patch set. The smooth type and polarity are only
>>> changed when the particular pwm channel is enabled or polarity is
>>> changed.
>>>
>>>> Can you mention what board you are observing this issue on?
>>>>
>>>> Also please explain why setting these bits result in an audible click.
>>>>
>>> We observe this on the bcm958300K board where one of the
>>> PWM channels is connected to the buzzer and changing the
>>> smooth type and polarity from its default values causes a click
>>>
>>
>> Which of these two bits is causing the click?
>>
>> I've already said that I'm open to removing the smooth bit here if that
>> helps.
>>
> Thank you for your quick reply Tim. It is setting the polarity bit that
> causes the click. I am planning on removing this entire loop in the next
> patch set, are you okay with that?
Does it cause a click at this moment or at a later point in time?
Is the click your sole motivation for this series? If so, can you
propose a more targeted fix?
I would be amenable to deferring polarity changes until subsequent
enable operations:
- kona_pwmc_probe doesn't do any hardware writes
- kona_pwmc_set_polarity only updates a polarity variable
- kona_pwmc_enable sets hardware polarity according to the variable
Would this be sufficient to satisfy your needs?
I am still worried that deferring polarity changes may negatively
impact some PWM users who set polarity but don't immediately enable
the PWM.
>>>>>
>>>>> Signed-off-by: Arun Ramamurthy <[email protected]>
>>>>> Reviewed-by: Ray Jui <[email protected]>
>>>>> Signed-off-by: Scott Branden <[email protected]>
>>>>> ---
>>>>> drivers/pwm/pwm-bcm-kona.c | 7 ++-----
>>>>> 1 file changed, 2 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
>>>>> index 02bc048..29eef9e 100644
>>>>> --- a/drivers/pwm/pwm-bcm-kona.c
>>>>> +++ b/drivers/pwm/pwm-bcm-kona.c
>>>>> @@ -266,12 +266,9 @@ 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);
>>>>
>>>>
>>>>
>>>> While the smooth bit need not be set here, it is important that the
>>>> polarity bit be set.
>>>>
>>> The default value for polarity is 0 which is normal polarity, so setting
>>> it
>>> to 1 here in the probe function without a sysfs call is
>>> when the software will report the polarity as normal when it is actually
>>> inversed.
>>
>>
>> Please double check the meaning of the polarity bits for the revision
>> of PWM IP in your chip. I suspect you are mistaken here.
>>
>> This driver is for PWM blocks compatible those found in bcm28145,
>> bcm28155, bcm21664, and other mobile chips of that generation.
>>
>> Apparently in contrast to the chip you are using, a set polarity bit
>> in the control register means normal polarity for the chips I
>> mentioned.
>>
>> A default of zero for these bits means they must be set to meet the
>> PWM framework's expectation that channels begin with normal polarity.
>>
> Tim, this is from the RDB of our new chip which is supposed to have the same
> IP as the mobile chip sets you mentioned:
>
> When set to 1 the output polarity for the PWM Output signal will be active
> hight; When set to 0, the output polarity for the PWM Output signal will be
> active low. Default State is 0.
>
> My understanding is that the frameworks normal polarity means active low, am
> I mistaken in that?
That is not how I would interpret things.
Perhaps this paragraph from Documentation/pwm.txt will help you:
When implementing polarity support in a PWM driver, make sure to respect the
signal conventions in the PWM framework. By definition, normal polarity
characterizes a signal starts high for the duration of the duty cycle and
goes low for the remainder of the period. Conversely, a signal with inversed
polarity starts low for the duration of the duty cycle and goes high for the
remainder of the period.
>
>>>>
>>>> Otherwise software will report the polarity as normal when it it is
>>>> actually inversed.
>>>>
>>>> Consider the case where a userspace process is controlling the PWM via
>>>> sysfs.
>>>>
>>> I agree with you about the sysfs case Tim, but since this is the probe
>>> function and not a sysfs callback, should we not leave it as the default
>>> value?
On 14-11-28 06:30 PM, Tim Kryger wrote:
> On Fri, Nov 28, 2014 at 3:49 PM, Arun Ramamurthy
> <[email protected]> wrote:
>>
>>
>> On 14-11-25 11:29 PM, Tim Kryger wrote:
>>>
>>> On Tue, Nov 25, 2014 at 11:40 AM, Scott Branden <[email protected]>
>>> wrote:
>>>>
>>>> From: Arun Ramamurthy <[email protected]>
>>>>
>>>> - 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 <[email protected]>
>>>> Reviewed-by: Ray Jui <[email protected]>
>>>> Signed-off-by: Scott Branden <[email protected]>
>>>> ---
>>>> 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.
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.
>
>> 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
>>>>
>>
On 14-11-28 07:19 PM, Tim Kryger wrote:
> On Fri, Nov 28, 2014 at 5:19 PM, Arun Ramamurthy
> <[email protected]> wrote:
>>
>>
>> On 14-11-28 05:08 PM, Tim Kryger wrote:
>>>
>>> On Fri, Nov 28, 2014 at 3:47 PM, Arun Ramamurthy
>>> <[email protected]> wrote:
>>>>
>>>>
>>>>
>>>> On 14-11-25 09:51 PM, Tim Kryger wrote:
>>>>>
>>>>>
>>>>> On Tue, Nov 25, 2014 at 11:40 AM, Scott Branden <[email protected]>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>> From: Arun Ramamurthy <[email protected]>
>>>>>>
>>>>>> The probe routine unnecessarily sets the smooth type and polarity for
>>>>>> all channels. This causes the channel for the speaker to click at the
>>>>>> same
>>>>>> time the backlight turns on. The smooth type and polarity should be set
>>>>>> individually
>>>>>> for each channel as required and no defaults need to be set.
>>>>>
>>>>>
>>>>>
>>>>> I am guessing you are talking about a PWM controlled beeper/buzzer.
>>>>>
>>>> This change is more so to remove setting smooth type and polarity for all
>>>> channels during probe and to leave them as their default values. Infact,
>>>> setting the PWM_CONTROL_TYPE_SHIT is also redundant cause the default
>>>> value
>>>> is already 1 for all channels. We can remove that loop entirely and this
>>>> will be done in the next patch set. The smooth type and polarity are only
>>>> changed when the particular pwm channel is enabled or polarity is
>>>> changed.
>>>>
>>>>> Can you mention what board you are observing this issue on?
>>>>>
>>>>> Also please explain why setting these bits result in an audible click.
>>>>>
>>>> We observe this on the bcm958300K board where one of the
>>>> PWM channels is connected to the buzzer and changing the
>>>> smooth type and polarity from its default values causes a click
>>>>
>>>
>>> Which of these two bits is causing the click?
>>>
>>> I've already said that I'm open to removing the smooth bit here if that
>>> helps.
>>>
>> Thank you for your quick reply Tim. It is setting the polarity bit that
>> causes the click. I am planning on removing this entire loop in the next
>> patch set, are you okay with that?
>
> Does it cause a click at this moment or at a later point in time?
>
> Is the click your sole motivation for this series? If so, can you
> propose a more targeted fix?
>
> I would be amenable to deferring polarity changes until subsequent
> enable operations:
>
> - kona_pwmc_probe doesn't do any hardware writes
> - kona_pwmc_set_polarity only updates a polarity variable
> - kona_pwmc_enable sets hardware polarity according to the variable
>
> Would this be sufficient to satisfy your needs?
>
It clicks at time of boot up, I can agree to the above changes and will
update the next patch set accordingly.
> I am still worried that deferring polarity changes may negatively
> impact some PWM users who set polarity but don't immediately enable
> the PWM.
>
>>>>>>
>>>>>> Signed-off-by: Arun Ramamurthy <[email protected]>
>>>>>> Reviewed-by: Ray Jui <[email protected]>
>>>>>> Signed-off-by: Scott Branden <[email protected]>
>>>>>> ---
>>>>>> drivers/pwm/pwm-bcm-kona.c | 7 ++-----
>>>>>> 1 file changed, 2 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
>>>>>> index 02bc048..29eef9e 100644
>>>>>> --- a/drivers/pwm/pwm-bcm-kona.c
>>>>>> +++ b/drivers/pwm/pwm-bcm-kona.c
>>>>>> @@ -266,12 +266,9 @@ 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);
>>>>>
>>>>>
>>>>>
>>>>> While the smooth bit need not be set here, it is important that the
>>>>> polarity bit be set.
>>>>>
>>>> The default value for polarity is 0 which is normal polarity, so setting
>>>> it
>>>> to 1 here in the probe function without a sysfs call is
>>>> when the software will report the polarity as normal when it is actually
>>>> inversed.
>>>
>>>
>>> Please double check the meaning of the polarity bits for the revision
>>> of PWM IP in your chip. I suspect you are mistaken here.
>>>
>>> This driver is for PWM blocks compatible those found in bcm28145,
>>> bcm28155, bcm21664, and other mobile chips of that generation.
>>>
>>> Apparently in contrast to the chip you are using, a set polarity bit
>>> in the control register means normal polarity for the chips I
>>> mentioned.
>>>
>>> A default of zero for these bits means they must be set to meet the
>>> PWM framework's expectation that channels begin with normal polarity.
>>>
>> Tim, this is from the RDB of our new chip which is supposed to have the same
>> IP as the mobile chip sets you mentioned:
>>
>> When set to 1 the output polarity for the PWM Output signal will be active
>> hight; When set to 0, the output polarity for the PWM Output signal will be
>> active low. Default State is 0.
>>
>> My understanding is that the frameworks normal polarity means active low, am
>> I mistaken in that?
>
> That is not how I would interpret things.
>
> Perhaps this paragraph from Documentation/pwm.txt will help you:
>
> When implementing polarity support in a PWM driver, make sure to respect the
> signal conventions in the PWM framework. By definition, normal polarity
> characterizes a signal starts high for the duration of the duty cycle and
> goes low for the remainder of the period. Conversely, a signal with inversed
> polarity starts low for the duration of the duty cycle and goes high for the
> remainder of the period.
>
I had it mixed up, I will update the polarity to match the PWM framework.
>>
>>>>>
>>>>> Otherwise software will report the polarity as normal when it it is
>>>>> actually inversed.
>>>>>
>>>>> Consider the case where a userspace process is controlling the PWM via
>>>>> sysfs.
>>>>>
>>>> I agree with you about the sysfs case Tim, but since this is the probe
>>>> function and not a sysfs callback, should we not leave it as the default
>>>> value?
On Mon, Dec 1, 2014 at 11:37 AM, Arun Ramamurthy
<[email protected]> wrote:
>
>
> On 14-11-28 06:30 PM, Tim Kryger wrote:
>>
>> On Fri, Nov 28, 2014 at 3:49 PM, Arun Ramamurthy
>> <[email protected]> wrote:
>>>
>>>
>>>
>>> On 14-11-25 11:29 PM, Tim Kryger wrote:
>>>>
>>>>
>>>> On Tue, Nov 25, 2014 at 11:40 AM, Scott Branden <[email protected]>
>>>> wrote:
>>>>>
>>>>>
>>>>> From: Arun Ramamurthy <[email protected]>
>>>>>
>>>>> - 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 <[email protected]>
>>>>> Reviewed-by: Ray Jui <[email protected]>
>>>>> Signed-off-by: Scott Branden <[email protected]>
>>>>> ---
>>>>> 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
>>>>>
>>>
>
Hi Tim,
I'm going to take over this submission because I made the changes to the
driver. Arun was filling in for me while I was on leave. Now I'm back
and I think I can help clarify why these changes were made. A summary
for all the changes should help.
There were two problems. First was a problem caused by setting the
polarity in probe. It caused the speaker to click when the polarity was
set so we took that out as it didn't seem to serve any useful purpose
that I could see. The polarity changes should be made in the polarity
callback.
The second and bigger problem was the smooth type sequence. If it isn't
done according to the spec then one in ten or twenty times you won't
even get a signal when it's enabled. Following the correct sequence with
the 400 ns delays solves this problem.
Additionally, by not following the sequence you won't get a smooth
transition. You'll get a change in the settings (duty cycle, period) but
may get a non smooth transition. So it's important to follow the spec
here. We don't want non-smooth transitions.
More comments below.
On 14-11-28 07:19 PM, Tim Kryger wrote:
> On Fri, Nov 28, 2014 at 5:19 PM, Arun Ramamurthy
> <[email protected]> wrote:
>>
>>
>> On 14-11-28 05:08 PM, Tim Kryger wrote:
>>>
>>> On Fri, Nov 28, 2014 at 3:47 PM, Arun Ramamurthy
>>> <[email protected]> wrote:
>>>>
>>>>
>>>>
>>>> On 14-11-25 09:51 PM, Tim Kryger wrote:
>>>>>
>>>>>
>>>>> On Tue, Nov 25, 2014 at 11:40 AM, Scott Branden <[email protected]>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>> From: Arun Ramamurthy <[email protected]>
>>>>>>
>>>>>> The probe routine unnecessarily sets the smooth type and polarity for
>>>>>> all channels. This causes the channel for the speaker to click at the
>>>>>> same
>>>>>> time the backlight turns on. The smooth type and polarity should be set
>>>>>> individually
>>>>>> for each channel as required and no defaults need to be set.
>>>>>
>>>>>
>>>>>
>>>>> I am guessing you are talking about a PWM controlled beeper/buzzer.
>>>>>
>>>> This change is more so to remove setting smooth type and polarity for all
>>>> channels during probe and to leave them as their default values. Infact,
>>>> setting the PWM_CONTROL_TYPE_SHIT is also redundant cause the default
>>>> value
>>>> is already 1 for all channels. We can remove that loop entirely and this
>>>> will be done in the next patch set. The smooth type and polarity are only
>>>> changed when the particular pwm channel is enabled or polarity is
>>>> changed.
>>>>
>>>>> Can you mention what board you are observing this issue on?
>>>>>
>>>>> Also please explain why setting these bits result in an audible click.
>>>>>
>>>> We observe this on the bcm958300K board where one of the
>>>> PWM channels is connected to the buzzer and changing the
>>>> smooth type and polarity from its default values causes a click
>>>>
>>>
>>> Which of these two bits is causing the click?
>>>
>>> I've already said that I'm open to removing the smooth bit here if that
>>> helps.
>>>
>> Thank you for your quick reply Tim. It is setting the polarity bit that
>> causes the click. I am planning on removing this entire loop in the next
>> patch set, are you okay with that?
>
> Does it cause a click at this moment or at a later point in time?
>
> Is the click your sole motivation for this series? If so, can you
> propose a more targeted fix?
>
> I would be amenable to deferring polarity changes until subsequent
> enable operations:
>
> - kona_pwmc_probe doesn't do any hardware writes
> - kona_pwmc_set_polarity only updates a polarity variable
> - kona_pwmc_enable sets hardware polarity according to the variable
>
> Would this be sufficient to satisfy your needs?
>
> I am still worried that deferring polarity changes may negatively
> impact some PWM users who set polarity but don't immediately enable
> the PWM.
>
>>>>>>
>>>>>> Signed-off-by: Arun Ramamurthy <[email protected]>
>>>>>> Reviewed-by: Ray Jui <[email protected]>
>>>>>> Signed-off-by: Scott Branden <[email protected]>
>>>>>> ---
>>>>>> drivers/pwm/pwm-bcm-kona.c | 7 ++-----
>>>>>> 1 file changed, 2 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
>>>>>> index 02bc048..29eef9e 100644
>>>>>> --- a/drivers/pwm/pwm-bcm-kona.c
>>>>>> +++ b/drivers/pwm/pwm-bcm-kona.c
>>>>>> @@ -266,12 +266,9 @@ 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);
>>>>>
>>>>>
>>>>>
>>>>> While the smooth bit need not be set here, it is important that the
>>>>> polarity bit be set.
>>>>>
>>>> The default value for polarity is 0 which is normal polarity, so setting
>>>> it
>>>> to 1 here in the probe function without a sysfs call is
>>>> when the software will report the polarity as normal when it is actually
>>>> inversed.
>>>
>>>
>>> Please double check the meaning of the polarity bits for the revision
>>> of PWM IP in your chip. I suspect you are mistaken here.
>>>
>>> This driver is for PWM blocks compatible those found in bcm28145,
>>> bcm28155, bcm21664, and other mobile chips of that generation.
>>>
>>> Apparently in contrast to the chip you are using, a set polarity bit
>>> in the control register means normal polarity for the chips I
>>> mentioned.
>>>
>>> A default of zero for these bits means they must be set to meet the
>>> PWM framework's expectation that channels begin with normal polarity.
>>>
>> Tim, this is from the RDB of our new chip which is supposed to have the same
>> IP as the mobile chip sets you mentioned:
>>
>> When set to 1 the output polarity for the PWM Output signal will be active
>> hight; When set to 0, the output polarity for the PWM Output signal will be
>> active low. Default State is 0.
>>
>> My understanding is that the frameworks normal polarity means active low, am
>> I mistaken in that?
>
> That is not how I would interpret things.
>
> Perhaps this paragraph from Documentation/pwm.txt will help you:
>
> When implementing polarity support in a PWM driver, make sure to respect the
> signal conventions in the PWM framework. By definition, normal polarity
> characterizes a signal starts high for the duration of the duty cycle and
> goes low for the remainder of the period. Conversely, a signal with inversed
> polarity starts low for the duration of the duty cycle and goes high for the
> remainder of the period.
>
Agreed. When using DT, the polarity will be set properly. When using
sysfs there is a discrepancy and this needs to be addressed. Because the
hw default is active low, the polarity will be inversed when the sysfs
says polarity is "normal". I'll have to account for this in the driver.
>>
>>>>>
>>>>> Otherwise software will report the polarity as normal when it it is
>>>>> actually inversed.
>>>>>
>>>>> Consider the case where a userspace process is controlling the PWM via
>>>>> sysfs.
>>>>>
>>>> I agree with you about the sysfs case Tim, but since this is the probe
>>>> function and not a sysfs callback, should we not leave it as the default
>>>> value?
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
Hi Tim,
Comments below.
On 14-11-25 11:29 PM, Tim Kryger wrote:
> On Tue, Nov 25, 2014 at 11:40 AM, Scott Branden <[email protected]> wrote:
>> From: Arun Ramamurthy <[email protected]>
>>
>> - 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 <[email protected]>
>> Reviewed-by: Ray Jui <[email protected]>
>> Signed-off-by: Scott Branden <[email protected]>
>> ---
>> 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.
>
Hopefully my summary in the previous reply helps clarify. I'm going to
remove all the helper functions to make the changes as simple as possible.
>>
>> 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.
Agreed. No need to set polarity on disable.
>
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
Comments below.
On 14-11-28 06:02 PM, Tim Kryger wrote:
> On Fri, Nov 28, 2014 at 3:48 PM, Arun Ramamurthy
> <[email protected]> wrote:
>>
>>
>> On 14-11-25 10:22 PM, Tim Kryger wrote:
>>>
>>> On Tue, Nov 25, 2014 at 11:40 AM, Scott Branden <[email protected]>
>>> wrote:
>>>>
>>>> From: Arun Ramamurthy <[email protected]>
>>>>
>>>> The pwm core code requires a separate call for enabling the channel
>>>> and hence the driver does not need to set pwm_trigger after a
>>>> polarity change
>>>
>>>
>>> The framework does restrict when polarity changes can occur but it
>>> isn't clear to me that there is any reason to delay applying the
>>> polarity change.
>>
>> I examined several other drivers such as pwm-atmel-tcb.c, pwm-ep93xx.c,
>> pwm-renesas-tpu.c, pwm-samsung.c in the 3.17 kernel tree and none of them
>> enable the channel after changing polarity. We would be the first driver to
>> do so.
>
> We are not "enabling" the channel as much as we are "triggering an
> application of settings" by hardware.
>
> Both pwm-ep93xx and pwm-samsung write polarity settings to the
> hardware immediately, presumably resulting in an immediate change in
> output.
>
> Alternatively, the pwm-atmel-tcb and pwm-renesas-tpu drivers save the
> polarity and write it to hardware later.
>
> There may be advantages to deferring the application of the polarity
> till a subsequent enable but both approaches appear to be acceptable.
>
>>
>>> Keep in mind that polarity matters even when a PWM
>>> is disabled. While disabled, the output should be equivalent to an
>>> enabled configuration with zero duty. Thus for normal polarity the
>>> output is constant low and for inversed polarity the output is
>>> constant high.
>>
>> The driver does set the duty cycle to zero when disabling the pwm
>> channel.However since the frame work prevents polarity change when the pwm
>> is enabled, I don’t see how one could expect the polarity change to be
>> reflected immediately without a separate call to pwm enable.
>>
>>
>>> I believe there is an expectation that the output is
>>> updated to reflect the requested polarity change prior to returning to
>>> the caller.
>>
>>
>> Once again I disagree with this based on other pwm drivers which only change
>> the polarity and do not enable the channel when their set polarity functions
>> are called.
>
> I don't know why you keep calling this an enable. Its not an enable,
> it is only a trigger.
>
> Perhaps this would be best explained with an example:
>
> # Export PWM for access by userspace
> cd /sys/class/pwm/pwmchip0
> echo 0 > export
> cd pwm0
>
> # Request 50% duty output when PWM is enabled
> echo 50000 > duty_cycle
> echo 100000 > period
>
> # Command Inversed Polarity
> echo inversed > polarity
>
> # Command Normal Polarity
> echo normal > polarity
>
> # Enable PWM
> echo 1 > enable
>
> The polarity changes trigger immediate output updates but the PWM is
> not enabled until the end.
>
> Prior to the last step the output is either a constant high or low
> signal, not the 50% duty waveform.
>
I just want to clarify so we're on the same page here. The pwm channel
needs to be disabled when polarity is set. When the set polarity
callback is called it just needs to update the polarity so that when the
channel is enabled again the new polarity value is used. We write this
to the polarity register in the callback which seems appropriate. Also
note that the clock is disabled at the end of the routine so the signal
is just going to be floating anyway. Hopefully this makes sense.
>>
>>
>>>
>>>>
>>>> Signed-off-by: Arun Ramamurthy <[email protected]>
>>>> Reviewed-by: Ray Jui <[email protected]>
>>>> Signed-off-by: Scott Branden <[email protected]>
>>>> ---
>>>> drivers/pwm/pwm-bcm-kona.c | 5 -----
>>>> 1 file changed, 5 deletions(-)
>>>>
>>>> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
>>>> index 29eef9e..fa0b5bf 100644
>>>> --- a/drivers/pwm/pwm-bcm-kona.c
>>>> +++ b/drivers/pwm/pwm-bcm-kona.c
>>>> @@ -173,11 +173,6 @@ static int kona_pwmc_set_polarity(struct pwm_chip
>>>> *chip, struct pwm_device *pwm,
>>>>
>>>> writel(value, kp->base + PWM_CONTROL_OFFSET);
>>>>
>>>> - 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;
>>>> --
>>>> 2.1.3
>>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
On Thu, Dec 4, 2014 at 12:22 PM, Jonathan Richardson
<[email protected]> wrote:
> Hi Tim,
>
> I'm going to take over this submission because I made the changes to the
> driver. Arun was filling in for me while I was on leave. Now I'm back
> and I think I can help clarify why these changes were made. A summary
> for all the changes should help.
>
> There were two problems. First was a problem caused by setting the
> polarity in probe. It caused the speaker to click when the polarity was
> set so we took that out as it didn't seem to serve any useful purpose
> that I could see. The polarity changes should be made in the polarity
> callback.
Please provide more details about your configuration.
Are you using the pwm-beeper driver with a piezo?
After a reset, all PWM output will be low until the PWM clock is
enabled at which point it will be constant high. Are you confident
that this transition is not responsible for the click you are hearing?
> The second and bigger problem was the smooth type sequence. If it isn't
> done according to the spec then one in ten or twenty times you won't
> even get a signal when it's enabled. Following the correct sequence with
> the 400 ns delays solves this problem.
Would the following minor change be sufficient to fix the issue?
diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index 02bc048..c537efd 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -85,6 +85,9 @@ static void kona_pwmc_apply_settings(struct kona_pwmc *kp, uns
value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
writel(value, kp->base + PWM_CONTROL_OFFSET);
+ ndelay(400);
+
/* 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);
>
> Additionally, by not following the sequence you won't get a smooth
> transition. You'll get a change in the settings (duty cycle, period) but
> may get a non smooth transition. So it's important to follow the spec
> here. We don't want non-smooth transitions.
Please provide your rationale for requiring smooth transitions.
Thanks,
Tim Kryger
On 14-12-06 03:13 PM, Tim Kryger wrote:
> On Thu, Dec 4, 2014 at 12:22 PM, Jonathan Richardson
> <[email protected]> wrote:
>> Hi Tim,
>>
>> I'm going to take over this submission because I made the changes to the
>> driver. Arun was filling in for me while I was on leave. Now I'm back
>> and I think I can help clarify why these changes were made. A summary
>> for all the changes should help.
>>
>> There were two problems. First was a problem caused by setting the
>> polarity in probe. It caused the speaker to click when the polarity was
>> set so we took that out as it didn't seem to serve any useful purpose
>> that I could see. The polarity changes should be made in the polarity
>> callback.
>
> Please provide more details about your configuration.
>
> Are you using the pwm-beeper driver with a piezo?
Probably, but I don't know. We don't care about it and aren't using it
which underscores why we don't want to initialize pwm channels we aren't
using.
>
> After a reset, all PWM output will be low until the PWM clock is
> enabled at which point it will be constant high. Are you confident
> that this transition is not responsible for the click you are hearing?
>
>> The second and bigger problem was the smooth type sequence. If it isn't
>> done according to the spec then one in ten or twenty times you won't
>> even get a signal when it's enabled. Following the correct sequence with
>> the 400 ns delays solves this problem.
>
> Would the following minor change be sufficient to fix the issue?
Not quite but hopefully I can send a patch soon for your review.
>
> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
> index 02bc048..c537efd 100644
> --- a/drivers/pwm/pwm-bcm-kona.c
> +++ b/drivers/pwm/pwm-bcm-kona.c
> @@ -85,6 +85,9 @@ static void kona_pwmc_apply_settings(struct kona_pwmc *kp, uns
> value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
> writel(value, kp->base + PWM_CONTROL_OFFSET);
>
> + ndelay(400);
> +
> /* 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);
>
>>
>> Additionally, by not following the sequence you won't get a smooth
>> transition. You'll get a change in the settings (duty cycle, period) but
>> may get a non smooth transition. So it's important to follow the spec
>> here. We don't want non-smooth transitions.
>
> Please provide your rationale for requiring smooth transitions.
The rationale is we don't want to give customers a bad experience when
we can provide them with a better one. The driver attempts to use smooth
transitions already but due to the quirks in the procedure which were
sent out previously it doesn't quite work as intended. When changing the
waveform we want to always have smooth transitions. There are unused pwm
channels that vendors can hook up whatever they want.
Thanks,
Jon
>
> Thanks,
> Tim Kryger
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>