diff mbox series

[v8,11/26] pwm: jz4740: Use regmap and clocks from TCU driver

Message ID 20181212220922.18759-12-paul@crapouillou.net
State Superseded
Headers show
Series Ingenic TCU patchset v8 | expand

Commit Message

Paul Cercueil Dec. 12, 2018, 10:09 p.m. UTC 
The ingenic-timer "TCU" driver provides us with a regmap, that we can
use to safely access the TCU registers.

It also provides us with clocks, that can be (un)gated, reparented or
reclocked from devicetree, instead of having these settings hardcoded in
this driver.

While this driver is devicetree-compatible, it is never (as of now)
probed from devicetree, so this change does not introduce a ABI problem
with current devicetree files.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---

Notes:
     v5: New patch
    
     v6: Drop requirement of probing from devicetree
    
     v7: No change

     v8: Drop useless <linux/clk-provider.h> include

 drivers/pwm/Kconfig      |   2 +
 drivers/pwm/pwm-jz4740.c | 123 ++++++++++++++++++++++++++++++-----------------
 2 files changed, 82 insertions(+), 43 deletions(-)

Comments

Uwe Kleine-König Dec. 13, 2018, 9:30 a.m. UTC | #1 
Hello,

On Wed, Dec 12, 2018 at 11:09:06PM +0100, Paul Cercueil wrote:
> [...]
>  static int jz4740_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
> -	uint32_t ctrl = jz4740_timer_get_ctrl(pwm->pwm);
> +	struct jz4740_pwm_chip *jz = to_jz4740(chip);
>  
> -	ctrl |= JZ_TIMER_CTRL_PWM_ENABLE;
> -	jz4740_timer_set_ctrl(pwm->hwpwm, ctrl);
> -	jz4740_timer_enable(pwm->hwpwm);
> +	/* Enable PWM output */
> +	regmap_update_bits(jz->map, TCU_REG_TCSRc(pwm->hwpwm),
> +				TCU_TCSR_PWM_EN, TCU_TCSR_PWM_EN);

Usually follow-up lines are indented to the matching parenthesis.

> [...]
>  static int jz4740_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  			     int duty_ns, int period_ns)
>  {
>  	struct jz4740_pwm_chip *jz4740 = to_jz4740(pwm->chip);
> +	struct clk *clk = jz4740->clks[pwm->hwpwm];
> +	unsigned long rate, new_rate, period, duty;
>  	unsigned long long tmp;
> -	unsigned long period, duty;
> -	unsigned int prescaler = 0;
> -	uint16_t ctrl;
> +	unsigned int tcsr;
>  	bool is_enabled;
>  
> -	tmp = (unsigned long long)clk_get_rate(jz4740->clk) * period_ns;
> -	do_div(tmp, 1000000000);
> -	period = tmp;
> +	rate = clk_get_rate(clk);
> +
> +	for (;;) {
> +		tmp = (unsigned long long) rate * period_ns;
> +		do_div(tmp, 1000000000);
>  
> -	while (period > 0xffff && prescaler < 6) {
> -		period >>= 2;
> -		++prescaler;
> +		if (tmp <= 0xffff)
> +			break;
> +
> +		new_rate = clk_round_rate(clk, rate / 2);
> +
> +		if (new_rate < rate)
> +			rate = new_rate;
> +		else
> +			return -EINVAL;
>  	}
>  
> -	if (prescaler == 6)
> -		return -EINVAL;
> +	clk_set_rate(clk, rate);

Maybe this could better live in a separate patch. If you split still
further to have the conversion to regmap in a single patch, then the
conversion to the clk_* functions and then improve the algorithm for the
clk settings each of the patches is easier to review than this one patch
that does all three things at once.

Best regards
Uwe
Paul Cercueil Dec. 13, 2018, 2:34 p.m. UTC | #2 
Hi,

Le jeu. 13 déc. 2018 à 10:30, Uwe Kleine-König 
<u.kleine-koenig@pengutronix.de> a écrit :
> Hello,
> 
> On Wed, Dec 12, 2018 at 11:09:06PM +0100, Paul Cercueil wrote:
>>  [...]
>>   static int jz4740_pwm_enable(struct pwm_chip *chip, struct 
>> pwm_device *pwm)
>>   {
>>  -	uint32_t ctrl = jz4740_timer_get_ctrl(pwm->pwm);
>>  +	struct jz4740_pwm_chip *jz = to_jz4740(chip);
>> 
>>  -	ctrl |= JZ_TIMER_CTRL_PWM_ENABLE;
>>  -	jz4740_timer_set_ctrl(pwm->hwpwm, ctrl);
>>  -	jz4740_timer_enable(pwm->hwpwm);
>>  +	/* Enable PWM output */
>>  +	regmap_update_bits(jz->map, TCU_REG_TCSRc(pwm->hwpwm),
>>  +				TCU_TCSR_PWM_EN, TCU_TCSR_PWM_EN);
> 
> Usually follow-up lines are indented to the matching parenthesis.

OK.

>>  [...]
>>   static int jz4740_pwm_config(struct pwm_chip *chip, struct 
>> pwm_device *pwm,
>>   			     int duty_ns, int period_ns)
>>   {
>>   	struct jz4740_pwm_chip *jz4740 = to_jz4740(pwm->chip);
>>  +	struct clk *clk = jz4740->clks[pwm->hwpwm];
>>  +	unsigned long rate, new_rate, period, duty;
>>   	unsigned long long tmp;
>>  -	unsigned long period, duty;
>>  -	unsigned int prescaler = 0;
>>  -	uint16_t ctrl;
>>  +	unsigned int tcsr;
>>   	bool is_enabled;
>> 
>>  -	tmp = (unsigned long long)clk_get_rate(jz4740->clk) * period_ns;
>>  -	do_div(tmp, 1000000000);
>>  -	period = tmp;
>>  +	rate = clk_get_rate(clk);
>>  +
>>  +	for (;;) {
>>  +		tmp = (unsigned long long) rate * period_ns;
>>  +		do_div(tmp, 1000000000);
>> 
>>  -	while (period > 0xffff && prescaler < 6) {
>>  -		period >>= 2;
>>  -		++prescaler;
>>  +		if (tmp <= 0xffff)
>>  +			break;
>>  +
>>  +		new_rate = clk_round_rate(clk, rate / 2);
>>  +
>>  +		if (new_rate < rate)
>>  +			rate = new_rate;
>>  +		else
>>  +			return -EINVAL;
>>   	}
>> 
>>  -	if (prescaler == 6)
>>  -		return -EINVAL;
>>  +	clk_set_rate(clk, rate);
> 
> Maybe this could better live in a separate patch. If you split still
> further to have the conversion to regmap in a single patch, then the
> conversion to the clk_* functions and then improve the algorithm for 
> the
> clk settings each of the patches is easier to review than this one 
> patch
> that does all three things at once.

I can try.

> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König        
>     |
> Industrial Linux Solutions                 | 
> http://www.pengutronix.de/  |
diff mbox series

Patch

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 27e5dd47a01f..0343f0c1238e 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -202,6 +202,8 @@  config PWM_IMX
 config PWM_JZ4740
 	tristate "Ingenic JZ47xx PWM support"
 	depends on MACH_INGENIC
+	depends on COMMON_CLK
+	select INGENIC_TIMER
 	help
 	  Generic PWM framework driver for Ingenic JZ47xx based
 	  machines.
diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
index a7b134af5e04..6b865c14f789 100644
--- a/drivers/pwm/pwm-jz4740.c
+++ b/drivers/pwm/pwm-jz4740.c
@@ -15,20 +15,19 @@ 
 
 #include <linux/clk.h>
 #include <linux/err.h>
-#include <linux/gpio.h>
 #include <linux/kernel.h>
+#include <linux/mfd/ingenic-tcu.h>
 #include <linux/module.h>
-#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/pwm.h>
-
-#include <asm/mach-jz4740/timer.h>
+#include <linux/regmap.h>
 
 #define NUM_PWM 8
 
 struct jz4740_pwm_chip {
 	struct pwm_chip chip;
-	struct clk *clk;
+	struct clk *clks[NUM_PWM];
+	struct regmap *map;
 };
 
 static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip *chip)
@@ -38,6 +37,11 @@  static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip *chip)
 
 static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 {
+	struct jz4740_pwm_chip *jz = to_jz4740(chip);
+	struct clk *clk;
+	char clk_name[16];
+	int ret;
+
 	/*
 	 * Timers 0 and 1 are used for system tasks, so they are unavailable
 	 * for use as PWMs.
@@ -45,65 +49,89 @@  static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 	if (pwm->hwpwm < 2)
 		return -EBUSY;
 
-	jz4740_timer_start(pwm->hwpwm);
+	snprintf(clk_name, sizeof(clk_name), "timer%u", pwm->hwpwm);
+
+	clk = clk_get(chip->dev, clk_name);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
 
+	ret = clk_prepare_enable(clk);
+	if (ret) {
+		clk_put(clk);
+		return ret;
+	}
+
+	jz->clks[pwm->hwpwm] = clk;
 	return 0;
 }
 
 static void jz4740_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 {
-	jz4740_timer_set_ctrl(pwm->hwpwm, 0);
+	struct jz4740_pwm_chip *jz = to_jz4740(chip);
+	struct clk *clk = jz->clks[pwm->hwpwm];
 
-	jz4740_timer_stop(pwm->hwpwm);
+	clk_disable_unprepare(clk);
+	clk_put(clk);
 }
 
 static int jz4740_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
-	uint32_t ctrl = jz4740_timer_get_ctrl(pwm->pwm);
+	struct jz4740_pwm_chip *jz = to_jz4740(chip);
 
-	ctrl |= JZ_TIMER_CTRL_PWM_ENABLE;
-	jz4740_timer_set_ctrl(pwm->hwpwm, ctrl);
-	jz4740_timer_enable(pwm->hwpwm);
+	/* Enable PWM output */
+	regmap_update_bits(jz->map, TCU_REG_TCSRc(pwm->hwpwm),
+				TCU_TCSR_PWM_EN, TCU_TCSR_PWM_EN);
 
+	/* Start counter */
+	regmap_write(jz->map, TCU_REG_TESR, BIT(pwm->hwpwm));
 	return 0;
 }
 
 static void jz4740_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
-	uint32_t ctrl = jz4740_timer_get_ctrl(pwm->hwpwm);
+	struct jz4740_pwm_chip *jz = to_jz4740(chip);
 
 	/* Disable PWM output.
 	 * In TCU2 mode (channel 1/2 on JZ4750+), this must be done before the
 	 * counter is stopped, while in TCU1 mode the order does not matter.
 	 */
-	ctrl &= ~JZ_TIMER_CTRL_PWM_ENABLE;
-	jz4740_timer_set_ctrl(pwm->hwpwm, ctrl);
+	regmap_update_bits(jz->map, TCU_REG_TCSRc(pwm->hwpwm),
+				TCU_TCSR_PWM_EN, 0);
 
 	/* Stop counter */
-	jz4740_timer_disable(pwm->hwpwm);
+	regmap_write(jz->map, TCU_REG_TECR, BIT(pwm->hwpwm));
 }
 
 static int jz4740_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 			     int duty_ns, int period_ns)
 {
 	struct jz4740_pwm_chip *jz4740 = to_jz4740(pwm->chip);
+	struct clk *clk = jz4740->clks[pwm->hwpwm];
+	unsigned long rate, new_rate, period, duty;
 	unsigned long long tmp;
-	unsigned long period, duty;
-	unsigned int prescaler = 0;
-	uint16_t ctrl;
+	unsigned int tcsr;
 	bool is_enabled;
 
-	tmp = (unsigned long long)clk_get_rate(jz4740->clk) * period_ns;
-	do_div(tmp, 1000000000);
-	period = tmp;
+	rate = clk_get_rate(clk);
+
+	for (;;) {
+		tmp = (unsigned long long) rate * period_ns;
+		do_div(tmp, 1000000000);
 
-	while (period > 0xffff && prescaler < 6) {
-		period >>= 2;
-		++prescaler;
+		if (tmp <= 0xffff)
+			break;
+
+		new_rate = clk_round_rate(clk, rate / 2);
+
+		if (new_rate < rate)
+			rate = new_rate;
+		else
+			return -EINVAL;
 	}
 
-	if (prescaler == 6)
-		return -EINVAL;
+	clk_set_rate(clk, rate);
+
+	period = tmp;
 
 	tmp = (unsigned long long)period * duty_ns;
 	do_div(tmp, period_ns);
@@ -112,18 +140,23 @@  static int jz4740_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (duty >= period)
 		duty = period - 1;
 
-	is_enabled = jz4740_timer_is_enabled(pwm->hwpwm);
+	regmap_read(jz4740->map, TCU_REG_TER, &tcsr);
+	is_enabled = tcsr & BIT(pwm->hwpwm);
 	if (is_enabled)
 		jz4740_pwm_disable(chip, pwm);
 
-	jz4740_timer_set_count(pwm->hwpwm, 0);
-	jz4740_timer_set_duty(pwm->hwpwm, duty);
-	jz4740_timer_set_period(pwm->hwpwm, period);
+	/* Set abrupt shutdown */
+	regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
+				TCU_TCSR_PWM_SD, TCU_TCSR_PWM_SD);
+
+	/* Reset counter to 0 */
+	regmap_write(jz4740->map, TCU_REG_TCNTc(pwm->hwpwm), 0);
 
-	ctrl = JZ_TIMER_CTRL_PRESCALER(prescaler) | JZ_TIMER_CTRL_SRC_EXT |
-		JZ_TIMER_CTRL_PWM_ABBRUPT_SHUTDOWN;
+	/* Set duty */
+	regmap_write(jz4740->map, TCU_REG_TDHRc(pwm->hwpwm), duty);
 
-	jz4740_timer_set_ctrl(pwm->hwpwm, ctrl);
+	/* Set period */
+	regmap_write(jz4740->map, TCU_REG_TDFRc(pwm->hwpwm), period);
 
 	if (is_enabled)
 		jz4740_pwm_enable(chip, pwm);
@@ -134,18 +167,19 @@  static int jz4740_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 static int jz4740_pwm_set_polarity(struct pwm_chip *chip,
 		struct pwm_device *pwm, enum pwm_polarity polarity)
 {
-	uint32_t ctrl = jz4740_timer_get_ctrl(pwm->pwm);
+	struct jz4740_pwm_chip *jz = to_jz4740(chip);
 
 	switch (polarity) {
 	case PWM_POLARITY_NORMAL:
-		ctrl &= ~JZ_TIMER_CTRL_PWM_ACTIVE_LOW;
+		regmap_update_bits(jz->map, TCU_REG_TCSRc(pwm->hwpwm),
+				TCU_TCSR_PWM_INITL_HIGH, 0);
 		break;
 	case PWM_POLARITY_INVERSED:
-		ctrl |= JZ_TIMER_CTRL_PWM_ACTIVE_LOW;
+		regmap_update_bits(jz->map, TCU_REG_TCSRc(pwm->hwpwm),
+			TCU_TCSR_PWM_INITL_HIGH, TCU_TCSR_PWM_INITL_HIGH);
 		break;
 	}
 
-	jz4740_timer_set_ctrl(pwm->hwpwm, ctrl);
 	return 0;
 }
 
@@ -162,16 +196,19 @@  static const struct pwm_ops jz4740_pwm_ops = {
 static int jz4740_pwm_probe(struct platform_device *pdev)
 {
 	struct jz4740_pwm_chip *jz4740;
+	struct device *dev = &pdev->dev;
 
-	jz4740 = devm_kzalloc(&pdev->dev, sizeof(*jz4740), GFP_KERNEL);
+	jz4740 = devm_kzalloc(dev, sizeof(*jz4740), GFP_KERNEL);
 	if (!jz4740)
 		return -ENOMEM;
 
-	jz4740->clk = devm_clk_get(&pdev->dev, "ext");
-	if (IS_ERR(jz4740->clk))
-		return PTR_ERR(jz4740->clk);
+	jz4740->map = dev_get_regmap(dev->parent, NULL);
+	if (!jz4740->map) {
+		dev_err(dev, "regmap not found\n");
+		return -EINVAL;
+	}
 
-	jz4740->chip.dev = &pdev->dev;
+	jz4740->chip.dev = dev;
 	jz4740->chip.ops = &jz4740_pwm_ops;
 	jz4740->chip.npwm = NUM_PWM;
 	jz4740->chip.base = -1;