mbox series

[V2,0/4] Add support for PWM Configure and stablize for PWM kona

Message ID 1547669716-20070-1-git-send-email-sheetal.tigadoli@broadcom.com
Headers show
Series Add support for PWM Configure and stablize for PWM kona | expand

Message

Sheetal Tigadoli Jan. 16, 2019, 8:15 p.m. UTC 
Hi,
	This patchset contain support to make PWM changes configure
	and stablize
	Following are brief changes done
	a. Add support for version2 compatible string
	b. Change PWM config and stablize delay in PWM Kona

Changes since V1:
	- Addressed review comments

Praveen Kumar B (3):
  dt-bindings: pwm: kona: Add new compatible for new version pwm-cygnus
  drivers: pwm: pwm-bcm-kona: Add cygnus-pwm support
  ARM: dts: cygnus: Change pwm compatible to new version

Sheetal Tigadoli (1):
  drivers: pwm: pwm-bcm-kona: Switch to using atomic PWM Framework

 .../devicetree/bindings/pwm/brcm,kona-pwm.txt      |   2 +-
 arch/arm/boot/dts/bcm-cygnus.dtsi                  |   2 +-
 drivers/pwm/pwm-bcm-kona.c                         | 290 +++++++++++----------
 3 files changed, 158 insertions(+), 136 deletions(-)

Comments

Uwe Kleine-König Jan. 21, 2019, 6:46 p.m. UTC | #1 
Hello,

On Thu, Jan 17, 2019 at 01:45:14AM +0530, Sheetal Tigadoli wrote:
> Switch to using atomic PWM Framework on broadcom PWM kona driver
> 
> Signed-off-by: Sheetal Tigadoli <sheetal.tigadoli@broadcom.com>
> ---
>  drivers/pwm/pwm-bcm-kona.c | 201 +++++++++++++++++++--------------------------
>  1 file changed, 83 insertions(+), 118 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
> index 09a95ae..fe63289 100644
> --- a/drivers/pwm/pwm-bcm-kona.c
> +++ b/drivers/pwm/pwm-bcm-kona.c
> @@ -108,151 +108,116 @@ static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
>  	ndelay(400);
>  }
>  
> -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;
> -
> -	/*
> -	 * Find period count, duty count and prescale to suit duty_ns and
> -	 * period_ns. This is done according to formulas described below:
> -	 *
> -	 * period_ns = 10^9 * (PRESCALE + 1) * PC / PWM_CLK_RATE
> -	 * duty_ns = 10^9 * (PRESCALE + 1) * DC / PWM_CLK_RATE
> -	 *
> -	 * PC = (PWM_CLK_RATE * period_ns) / (10^9 * (PRESCALE + 1))
> -	 * DC = (PWM_CLK_RATE * duty_ns) / (10^9 * (PRESCALE + 1))
> -	 */
> -
> -	rate = clk_get_rate(kp->clk);
> -
> -	while (1) {
> -		div = 1000000000;
> -		div *= 1 + prescale;
> -		val = rate * period_ns;
> -		pc = div64_u64(val, div);
> -		val = rate * duty_ns;
> -		dc = div64_u64(val, div);
> -
> -		/* If duty_ns or period_ns are not achievable then return */
> -		if (pc < PERIOD_COUNT_MIN || dc < DUTY_CYCLE_HIGH_MIN)
> -			return -EINVAL;
> -
> -		/* If pc and dc are in bounds, the calculation is done */
> -		if (pc <= PERIOD_COUNT_MAX && dc <= DUTY_CYCLE_HIGH_MAX)
> -			break;
> -
> -		/* Otherwise, increase prescale and recalculate pc and dc */
> -		if (++prescale > PRESCALE_MAX)
> -			return -EINVAL;
> -	}
> -
> -	/*
> -	 * Don't apply settings if disabled. The period and duty cycle are
> -	 * always calculated above to ensure the new values are
> -	 * validated immediately instead of on enable.
> -	 */
> -	if (pwm_is_enabled(pwm)) {
> -		kona_pwmc_prepare_for_settings(kp, chan);
> -
> -		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));
> -
> -		kona_pwmc_apply_settings(kp, chan);
> -	}
> -
> -	return 0;
> -}
> -
> -static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
> -				  enum pwm_polarity polarity)
> +static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
>  	struct kona_pwmc *kp = to_kona_pwmc(chip);
>  	unsigned int chan = pwm->hwpwm;
>  	unsigned int value;
> -	int ret;
> -
> -	ret = clk_prepare_enable(kp->clk);
> -	if (ret < 0) {
> -		dev_err(chip->dev, "failed to enable clock: %d\n", ret);
> -		return ret;
> -	}
>  
>  	kona_pwmc_prepare_for_settings(kp, chan);
>  
> -	value = readl(kp->base + PWM_CONTROL_OFFSET);
> -
> -	if (polarity == PWM_POLARITY_NORMAL)
> -		value |= 1 << PWM_CONTROL_POLARITY_SHIFT(chan);
> -	else
> -		value &= ~(1 << PWM_CONTROL_POLARITY_SHIFT(chan));
> +	/* Simulate a disable by configuring for zero duty */
> +	writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
> +	writel(0, kp->base + PERIOD_COUNT_OFFSET(chan));
>  
> -	writel(value, kp->base + PWM_CONTROL_OFFSET);
> +	/* Set prescale to 0 for this channel */

kona_pwmc_apply uses PRESCALE_MIN instead of a plain 0. To make the
comment more helpful tell *why* you do it instead of stating the obvious
for the fluent C programmer.

> +	value = readl(kp->base + PRESCALE_OFFSET);
> +	value &= ~PRESCALE_MASK(chan);
> +	writel(value, kp->base + PRESCALE_OFFSET);
>  
>  	kona_pwmc_apply_settings(kp, chan);
>  
>  	clk_disable_unprepare(kp->clk);
> -
> -	return 0;
>  }
>  
> -static int kona_pwmc_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +static int kona_pwmc_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			   struct pwm_state *state)
>  {
>  	struct kona_pwmc *kp = to_kona_pwmc(chip);
> +	struct pwm_state cstate;
> +	u64 val, div, rate;
> +	unsigned long prescale = PRESCALE_MIN, pc, dc;
> +	unsigned int value, chan = pwm->hwpwm;
>  	int ret;
>  
> -	ret = clk_prepare_enable(kp->clk);
> -	if (ret < 0) {
> -		dev_err(chip->dev, "failed to enable clock: %d\n", ret);
> -		return ret;
> -	}
> -
> -	ret = kona_pwmc_config(chip, pwm, pwm_get_duty_cycle(pwm),
> -			       pwm_get_period(pwm));
> -	if (ret < 0) {
> -		clk_disable_unprepare(kp->clk);
> -		return ret;
> -	}
> +	pwm_get_state(pwm, &cstate);

The pwm_get_state function is designed for PWM-consumers. It is an
implementation detail that it also works for drivers. So I'd like to see
its usage dropped in drivers. (Note that Thierry might not agree here.)

> +
> +	if (state->enabled) {
> +		/*
> +		 * Find period count, duty count and prescale to suit duty_ns
> +		 * and period_ns. This is done according to formulas described
> +		 * below:
> +		 *
> +		 * period_ns = 10^9 * (PRESCALE + 1) * PC / PWM_CLK_RATE
> +		 * duty_ns = 10^9 * (PRESCALE + 1) * DC / PWM_CLK_RATE
> +		 *
> +		 * PC = (PWM_CLK_RATE * period_ns) / (10^9 * (PRESCALE + 1))
> +		 * DC = (PWM_CLK_RATE * duty_ns) / (10^9 * (PRESCALE + 1))
> +		 */
> +		rate = clk_get_rate(kp->clk);
> +
> +		while (1) {
> +			div = 1000000000;
> +			div *= 1 + prescale;
> +			val = rate * state->period;
> +			pc = div64_u64(val, div);
> +			val = rate * state->duty_cycle;
> +			dc = div64_u64(val, div);
> +
> +			/* If duty_ns or period_ns are not achievable then

Please stick to the usual style for multi-line comments, i.e. "/*" on a
separate line.

> +			 * return
> +			 */
> +			if (pc < PERIOD_COUNT_MIN || dc < DUTY_CYCLE_HIGH_MIN)
> +				return -EINVAL;
> +
> +			/* If pc & dc are in bounds, the calculation is done */
> +			if (pc <= PERIOD_COUNT_MAX && dc <= DUTY_CYCLE_HIGH_MAX)
> +				break;
> +
> +			/* Otherwise, increase prescale & recalculate pc & dc */
> +			if (++prescale > PRESCALE_MAX)
> +				return -EINVAL;
> +		}
> +
> +		if (!cstate.enabled) {
> +			ret = clk_prepare_enable(kp->clk);
> +			if (ret < 0) {
> +				dev_err(chip->dev,
> +					"failed to enable clock: %d\n", ret);
> +				return ret;
> +			}
> +		}
>  
> -	return 0;
> -}
> -
> -static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> -{
> -	struct kona_pwmc *kp = to_kona_pwmc(chip);
> -	unsigned int chan = pwm->hwpwm;
> -	unsigned int value;
> +		kona_pwmc_prepare_for_settings(kp, chan);
>  
> -	kona_pwmc_prepare_for_settings(kp, chan);
> +		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));
>  
> -	/* Simulate a disable by configuring for zero duty */
> -	writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
> -	writel(0, kp->base + PERIOD_COUNT_OFFSET(chan));
> +		if (cstate.polarity != state->polarity) {
> +			value = readl(kp->base + PWM_CONTROL_OFFSET);
> +			if (state->polarity == PWM_POLARITY_NORMAL)
> +				value |= 1 << PWM_CONTROL_POLARITY_SHIFT(chan);
> +			else
> +				value &= ~(1 <<
> +					   PWM_CONTROL_POLARITY_SHIFT(chan));
>  
> -	/* Set prescale to 0 for this channel */
> -	value = readl(kp->base + PRESCALE_OFFSET);
> -	value &= ~PRESCALE_MASK(chan);
> -	writel(value, kp->base + PRESCALE_OFFSET);
> +			writel(value, kp->base + PWM_CONTROL_OFFSET);
> +		}
>  
> -	kona_pwmc_apply_settings(kp, chan);
> +		kona_pwmc_apply_settings(kp, chan);
> +	} else if (cstate.enabled) {
> +		kona_pwmc_disable(chip, pwm);

If I do:

	pwm_apply_state(pwm, { .polarity = PWM_POLARITY_NORMAL, .enabled = true, ... });
	pwm_apply_state(pwm, { .polarity = PWM_POLARITY_INVERSED, .enabled = false, ... });

the output is constant low, which is wrong.

Best regards
Uwe
Uwe Kleine-König Jan. 21, 2019, 6:53 p.m. UTC | #2 
Hello,

On Thu, Jan 17, 2019 at 01:45:15AM +0530, Sheetal Tigadoli wrote:
> From: Praveen Kumar B <praveen.b@broadcom.com>
> 
> Add support for new version of pwm-cygnus.
> Add support to make PWM changes configured and stable.
> 
> Signed-off-by: Praveen Kumar B <praveen.b@broadcom.com>
> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
> Signed-off-by: Sheetal Tigadoli <sheetal.tigadoli@broadcom.com>
> ---
>  drivers/pwm/pwm-bcm-kona.c | 95 ++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 76 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
> index fe63289..143843f 100644
> --- a/drivers/pwm/pwm-bcm-kona.c
> +++ b/drivers/pwm/pwm-bcm-kona.c
> @@ -15,6 +15,7 @@
>  #include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/io.h>
> +#include <linux/iopoll.h>
>  #include <linux/ioport.h>
>  #include <linux/math64.h>
>  #include <linux/module.h>
> @@ -65,10 +66,19 @@
>  #define DUTY_CYCLE_HIGH_MIN			(0x00000000)
>  #define DUTY_CYCLE_HIGH_MAX			(0x00ffffff)
>  
> +#define PWM_MONITOR_OFFSET			0xb0
> +#define PWM_MONITOR_TIMEOUT_US			5
> +
> +enum kona_pwmc_ver {
> +	KONA_PWM = 1,
> +	CYGNUS_PWM
> +};
> +
>  struct kona_pwmc {
>  	struct pwm_chip chip;
>  	void __iomem *base;
>  	struct clk *clk;
> +	enum kona_pwmc_ver version;
>  };
>  
>  static inline struct kona_pwmc *to_kona_pwmc(struct pwm_chip *_chip)
> @@ -76,10 +86,36 @@ static inline struct kona_pwmc *to_kona_pwmc(struct pwm_chip *_chip)
>  	return container_of(_chip, struct kona_pwmc, chip);
>  }
>  
> +static int kona_pwmc_wait_stable(struct pwm_chip *chip, unsigned int chan,
> +				 unsigned int kona_ver)
> +{
> +	struct kona_pwmc *kp = to_kona_pwmc(chip);
> +	unsigned int value;
> +
> +	switch (kona_ver) {
> +	case KONA_PWM:
> +		/*
> +		 * There must be a min 400ns delay between clearing trigger and
> +		 * settingit. Failing to do this may result in no PWM signal.
> +		 */
> +		ndelay(400);
> +		return 0;
> +
> +	case CYGNUS_PWM:
> +		return readl_poll_timeout(kp->base + PWM_MONITOR_OFFSET, value,
> +					  !(value & (BIT(chan))), 0,
> +					  PWM_MONITOR_TIMEOUT_US);
> +
> +	default:
> +		return -ENODEV;
> +
> +	}
> +}

This function is the only difference between these two otherwise similar
implementations. If you do the following instead:

	static int kona_pwmc_wait_stable(struct pwm_chip *chip, unsigned int chan)
	{
		ndelay(400);
		return 0;
	}

	static int cygnus_pwmc_wait_stable(struct pwm_chip *chip, unsigned int chan)
	{
		return readl_poll_timeout(...);
	}

and then maintain a

	int (*wait_stable)(struct pwm_chip *, unsigned int);

in struct kona_pwmc that is initialized in .probe you save quite some
checks for the version.

> +
>  /*
>   * Clear trigger bit but set smooth bit to maintain old output.
>   */
> -static void kona_pwmc_prepare_for_settings(struct kona_pwmc *kp,
> +static int kona_pwmc_prepare_for_settings(struct kona_pwmc *kp,
>  	unsigned int chan)
>  {
>  	unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
> @@ -88,14 +124,10 @@ static void kona_pwmc_prepare_for_settings(struct kona_pwmc *kp,
>  	value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
>  	writel(value, kp->base + PWM_CONTROL_OFFSET);
>  
> -	/*
> -	 * There must be a min 400ns delay between clearing trigger and setting
> -	 * it. Failing to do this may result in no PWM signal.
> -	 */
> -	ndelay(400);
> +	return kona_pwmc_wait_stable(&kp->chip, chan, kp->version);
>  }
>  
> -static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
> +static int kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
>  {
>  	unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
>  
> @@ -104,8 +136,7 @@ static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
>  	value |= 1 << PWM_CONTROL_TRIGGER_SHIFT(chan);
>  	writel(value, kp->base + PWM_CONTROL_OFFSET);
>  
> -	/* Trigger bit must be held high for at least 400 ns. */
> -	ndelay(400);
> +	return kona_pwmc_wait_stable(&kp->chip, chan, kp->version);
>  }
>  
>  static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> @@ -113,8 +144,13 @@ static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  	struct kona_pwmc *kp = to_kona_pwmc(chip);
>  	unsigned int chan = pwm->hwpwm;
>  	unsigned int value;
> +	int ret;
>  
> -	kona_pwmc_prepare_for_settings(kp, chan);
> +	ret = kona_pwmc_prepare_for_settings(kp, chan);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "failed to prepare pwm settings: %d\n", ret);
> +		return;
> +	}
>  
>  	/* Simulate a disable by configuring for zero duty */
>  	writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
> @@ -125,7 +161,11 @@ static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  	value &= ~PRESCALE_MASK(chan);
>  	writel(value, kp->base + PRESCALE_OFFSET);
>  
> -	kona_pwmc_apply_settings(kp, chan);
> +	ret = kona_pwmc_apply_settings(kp, chan);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "failed to apply pwm settings: %d\n", ret);
> +		return;

I think it would make sense to change kona_pwmc_disable to return an
error code and then use that in the callers.

> +	}
>  
>  	clk_disable_unprepare(kp->clk);
>  }
> @@ -188,7 +228,12 @@ static int kona_pwmc_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  			}
>  		}
>  
> -		kona_pwmc_prepare_for_settings(kp, chan);
> +		ret = kona_pwmc_prepare_for_settings(kp, chan);
> +		if (ret < 0) {
> +			dev_err(chip->dev,
> +				"failed to prepare pwm settings: %d\n", ret);
> +			return ret;
> +		}
>  
>  		value = readl(kp->base + PRESCALE_OFFSET);
>  		value &= ~PRESCALE_MASK(chan);
> @@ -208,7 +253,12 @@ static int kona_pwmc_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  			writel(value, kp->base + PWM_CONTROL_OFFSET);
>  		}
>  
> -		kona_pwmc_apply_settings(kp, chan);
> +		ret = kona_pwmc_apply_settings(kp, chan);
> +		if (ret < 0) {
> +			dev_err(chip->dev,
> +				"failed to prepare pwm settings: %d\n", ret);
> +			return ret;
> +		}
>  	} else if (cstate.enabled) {
>  		kona_pwmc_disable(chip, pwm);
>  	}
> @@ -221,14 +271,26 @@ static int kona_pwmc_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	.owner = THIS_MODULE,
>  };
>  
> +static const struct of_device_id bcm_kona_pwmc_dt[] = {
> +	{ .compatible = "brcm,kona-pwm", .data = (void *)KONA_PWM},
> +	{ .compatible = "brcm,cygnus-pwm", .data = (void *)CYGNUS_PWM},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, bcm_kona_pwmc_dt);
> +
>  static int kona_pwmc_probe(struct platform_device *pdev)
>  {
>  	struct kona_pwmc *kp;
>  	struct resource *res;
> +	const struct of_device_id *of_id;
>  	unsigned int chan;
>  	unsigned int value = 0;
>  	int ret = 0;
>  
> +	of_id = of_match_node(bcm_kona_pwmc_dt, pdev->dev.of_node);
> +	if (!of_id)
> +		return -ENODEV;
> +
>  	kp = devm_kzalloc(&pdev->dev, sizeof(*kp), GFP_KERNEL);
>  	if (kp == NULL)
>  		return -ENOMEM;
> @@ -241,6 +303,7 @@ static int kona_pwmc_probe(struct platform_device *pdev)
>  	kp->chip.npwm = 6;
>  	kp->chip.of_xlate = of_pwm_xlate_with_flags;
>  	kp->chip.of_pwm_n_cells = 3;
> +	kp->version = (enum kona_pwmc_ver)of_id->data;
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	kp->base = devm_ioremap_resource(&pdev->dev, res);
> @@ -287,12 +350,6 @@ static int kona_pwmc_remove(struct platform_device *pdev)
>  	return pwmchip_remove(&kp->chip);
>  }
>  
> -static const struct of_device_id bcm_kona_pwmc_dt[] = {
> -	{ .compatible = "brcm,kona-pwm" },
> -	{ },
> -};
> -MODULE_DEVICE_TABLE(of, bcm_kona_pwmc_dt);
> -
>  static struct platform_driver kona_pwmc_driver = {
>  	.driver = {
>  		.name = "bcm-kona-pwm",

Best regards
Uwe