[PATCH v2 0/3] pwm: jz4740: Update to use TCU clocks/regmap

[PATCH v2 0/3] pwm: jz4740: Update to use TCU clocks/regmap

[Paul Cercueil]

Hi,

Patchset v1 had 7 patches. Some of them require more feedback before I'm
comfortable sending them upstream again; so I just cut the previous
patchset in half, and will upstream the second half in due time.

Cheers,
-Paul

[PATCH v2 1/3] pwm: jz4740: Use clocks from TCU driver

[Paul Cercueil]

The ingenic-timer "TCU" driver 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>
Tested-by: Mathieu Malaterre <malat@debian.org>
Tested-by: Artur Rojek <contact@artur-rojek.eu>
---

Notes:
    v2: This patch is now before the patch introducing regmap, so the code
        has changed a bit.

 drivers/pwm/Kconfig      |  1 +
 drivers/pwm/pwm-jz4740.c | 45 ++++++++++++++++++++++++++++------------
 2 files changed, 33 insertions(+), 13 deletions(-)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index e3a2518503ed..e998e5cb01b0 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -225,6 +225,7 @@ config PWM_IMX_TPM
 config PWM_JZ4740
 	tristate "Ingenic JZ47xx PWM support"
 	depends on MACH_INGENIC
+	depends on COMMON_CLK
 	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 9d78cc21cb12..fd83644f9323 100644
--- a/drivers/pwm/pwm-jz4740.c
+++ b/drivers/pwm/pwm-jz4740.c
@@ -24,7 +24,6 @@
 
 struct jz4740_pwm_chip {
 	struct pwm_chip chip;
-	struct clk *clk;
 };
 
 static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip *chip)
@@ -34,6 +33,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.
@@ -41,16 +45,31 @@ 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;
+	}
+
+	pwm_set_chip_data(pwm, clk);
 
 	return 0;
 }
 
 static void jz4740_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 {
+	struct clk *clk = pwm_get_chip_data(pwm);
+
 	jz4740_timer_set_ctrl(pwm->hwpwm, 0);
 
-	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)
@@ -91,17 +110,21 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			    const struct pwm_state *state)
 {
 	struct jz4740_pwm_chip *jz4740 = to_jz4740(pwm->chip);
+	struct clk *clk = pwm_get_chip_data(pwm),
+		   *parent_clk = clk_get_parent(clk);
+	unsigned long rate, period, duty;
 	unsigned long long tmp;
-	unsigned long period, duty;
 	unsigned int prescaler = 0;
 	uint16_t ctrl;
 
-	tmp = (unsigned long long)clk_get_rate(jz4740->clk) * state->period;
+	rate = clk_get_rate(parent_clk);
+	tmp = (unsigned long long)rate * state->period;
 	do_div(tmp, 1000000000);
 	period = tmp;
 
 	while (period > 0xffff && prescaler < 6) {
 		period >>= 2;
+		rate >>= 2;
 		++prescaler;
 	}
 
@@ -117,14 +140,14 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	jz4740_pwm_disable(chip, pwm);
 
+	clk_set_rate(clk, rate);
+
 	jz4740_timer_set_count(pwm->hwpwm, 0);
 	jz4740_timer_set_duty(pwm->hwpwm, duty);
 	jz4740_timer_set_period(pwm->hwpwm, period);
 
-	ctrl = JZ_TIMER_CTRL_PRESCALER(prescaler) | JZ_TIMER_CTRL_SRC_EXT |
-		JZ_TIMER_CTRL_PWM_ABBRUPT_SHUTDOWN;
-
-	jz4740_timer_set_ctrl(pwm->hwpwm, ctrl);
+	ctrl = jz4740_timer_get_ctrl(pwm->hwpwm);
+	ctrl |= JZ_TIMER_CTRL_PWM_ABBRUPT_SHUTDOWN;
 
 	switch (state->polarity) {
 	case PWM_POLARITY_NORMAL:
@@ -158,10 +181,6 @@ static int jz4740_pwm_probe(struct platform_device *pdev)
 	if (!jz4740)
 		return -ENOMEM;
 
-	jz4740->clk = devm_clk_get(&pdev->dev, "ext");
-	if (IS_ERR(jz4740->clk))
-		return PTR_ERR(jz4740->clk);
-
 	jz4740->chip.dev = &pdev->dev;
 	jz4740->chip.ops = &jz4740_pwm_ops;
 	jz4740->chip.npwm = NUM_PWM;
-- 
2.24.0

Re: [PATCH v2 1/3] pwm: jz4740: Use clocks from TCU driver

[Uwe Kleine-König]

On Sat, Nov 16, 2019 at 06:36:11PM +0100, Paul Cercueil wrote:
> The ingenic-timer "TCU" driver 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>
> Tested-by: Mathieu Malaterre <malat@debian.org>
> Tested-by: Artur Rojek <contact@artur-rojek.eu>
> ---
> 
> Notes:
>     v2: This patch is now before the patch introducing regmap, so the code
>         has changed a bit.
> 
>  drivers/pwm/Kconfig      |  1 +
>  drivers/pwm/pwm-jz4740.c | 45 ++++++++++++++++++++++++++++------------
>  2 files changed, 33 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index e3a2518503ed..e998e5cb01b0 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -225,6 +225,7 @@ config PWM_IMX_TPM
>  config PWM_JZ4740
>  	tristate "Ingenic JZ47xx PWM support"
>  	depends on MACH_INGENIC
> +	depends on COMMON_CLK
>  	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 9d78cc21cb12..fd83644f9323 100644
> --- a/drivers/pwm/pwm-jz4740.c
> +++ b/drivers/pwm/pwm-jz4740.c
> @@ -24,7 +24,6 @@
>  
>  struct jz4740_pwm_chip {
>  	struct pwm_chip chip;
> -	struct clk *clk;

What is the motivation to go away from this approach to store the clock?

>  };
>  
>  static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip *chip)
> @@ -34,6 +33,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.
> @@ -41,16 +45,31 @@ 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)) 

		if (PTR_ERR(clk) != -EPROBE_DEFER)
			dev_err(chip->dev, "Failed to get clock: %pe\n", clk);

> +		return PTR_ERR(clk);
> +
> +	ret = clk_prepare_enable(clk);
> +	if (ret) {
> +		clk_put(clk);
> +		return ret;
> +	}
> +
> +	pwm_set_chip_data(pwm, clk);
>  
>  	return 0;
>  }
>  
>  static void jz4740_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
> +	struct clk *clk = pwm_get_chip_data(pwm);
> +
>  	jz4740_timer_set_ctrl(pwm->hwpwm, 0);

What is the purpose of this call? I would have expected that all these
would go away when converting to the clk stuff?!

> -	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)
> @@ -91,17 +110,21 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  			    const struct pwm_state *state)
>  {
>  	struct jz4740_pwm_chip *jz4740 = to_jz4740(pwm->chip);
> +	struct clk *clk = pwm_get_chip_data(pwm),
> +		   *parent_clk = clk_get_parent(clk);
> +	unsigned long rate, period, duty;
>  	unsigned long long tmp;
> -	unsigned long period, duty;
>  	unsigned int prescaler = 0;
>  	uint16_t ctrl;
>  
> -	tmp = (unsigned long long)clk_get_rate(jz4740->clk) * state->period;
> +	rate = clk_get_rate(parent_clk);

Why is it the parent's rate that is relevant here?

> +	tmp = (unsigned long long)rate * state->period;
>  	do_div(tmp, 1000000000);
>  	period = tmp;
>  
>  	while (period > 0xffff && prescaler < 6) {
>  		period >>= 2;
> +		rate >>= 2;
>  		++prescaler;
>  	}
>  
> @@ -117,14 +140,14 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  
>  	jz4740_pwm_disable(chip, pwm);
>  
> +	clk_set_rate(clk, rate);

This function's return code must be checked.

>  	jz4740_timer_set_count(pwm->hwpwm, 0);
>  	jz4740_timer_set_duty(pwm->hwpwm, duty);
>  	jz4740_timer_set_period(pwm->hwpwm, period);
>  
> -	ctrl = JZ_TIMER_CTRL_PRESCALER(prescaler) | JZ_TIMER_CTRL_SRC_EXT |
> -		JZ_TIMER_CTRL_PWM_ABBRUPT_SHUTDOWN;
> -
> -	jz4740_timer_set_ctrl(pwm->hwpwm, ctrl);
> +	ctrl = jz4740_timer_get_ctrl(pwm->hwpwm);
> +	ctrl |= JZ_TIMER_CTRL_PWM_ABBRUPT_SHUTDOWN;
>  
>  	switch (state->polarity) {
>  	case PWM_POLARITY_NORMAL:

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH v2 1/3] pwm: jz4740: Use clocks from TCU driver

[Paul Cercueil]

Hi Uwe,


Le dim., nov. 17, 2019 at 21:20, Uwe Kleine-König 
<u.kleine-koenig@pengutronix.de> a écrit :
> On Sat, Nov 16, 2019 at 06:36:11PM +0100, Paul Cercueil wrote:
>>  The ingenic-timer "TCU" driver 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>
>>  Tested-by: Mathieu Malaterre <malat@debian.org>
>>  Tested-by: Artur Rojek <contact@artur-rojek.eu>
>>  ---
>> 
>>  Notes:
>>      v2: This patch is now before the patch introducing regmap, so 
>> the code
>>          has changed a bit.
>> 
>>   drivers/pwm/Kconfig      |  1 +
>>   drivers/pwm/pwm-jz4740.c | 45 
>> ++++++++++++++++++++++++++++------------
>>   2 files changed, 33 insertions(+), 13 deletions(-)
>> 
>>  diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>>  index e3a2518503ed..e998e5cb01b0 100644
>>  --- a/drivers/pwm/Kconfig
>>  +++ b/drivers/pwm/Kconfig
>>  @@ -225,6 +225,7 @@ config PWM_IMX_TPM
>>   config PWM_JZ4740
>>   	tristate "Ingenic JZ47xx PWM support"
>>   	depends on MACH_INGENIC
>>  +	depends on COMMON_CLK
>>   	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 9d78cc21cb12..fd83644f9323 100644
>>  --- a/drivers/pwm/pwm-jz4740.c
>>  +++ b/drivers/pwm/pwm-jz4740.c
>>  @@ -24,7 +24,6 @@
>> 
>>   struct jz4740_pwm_chip {
>>   	struct pwm_chip chip;
>>  -	struct clk *clk;
> 
> What is the motivation to go away from this approach to store the 
> clock?

It's actually not the same clock. Instead of obtaining "ext" clock from 
the probe, we obtain "timerX" clocks (X being the PWM channel) from the 
request callback.


>>   };
>> 
>>   static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip 
>> *chip)
>>  @@ -34,6 +33,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.
>>  @@ -41,16 +45,31 @@ 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))
> 
> 		if (PTR_ERR(clk) != -EPROBE_DEFER)
> 			dev_err(chip->dev, "Failed to get clock: %pe\n", clk);

Never heard about that %pe. Will do that.


> 
>>  +		return PTR_ERR(clk);
>>  +
>>  +	ret = clk_prepare_enable(clk);
>>  +	if (ret) {
>>  +		clk_put(clk);
>>  +		return ret;
>>  +	}
>>  +
>>  +	pwm_set_chip_data(pwm, clk);
>> 
>>   	return 0;
>>   }
>> 
>>   static void jz4740_pwm_free(struct pwm_chip *chip, struct 
>> pwm_device *pwm)
>>   {
>>  +	struct clk *clk = pwm_get_chip_data(pwm);
>>  +
>>   	jz4740_timer_set_ctrl(pwm->hwpwm, 0);
> 
> What is the purpose of this call? I would have expected that all these
> would go away when converting to the clk stuff?!

Some go away in patch [1/3] as they are clock-related, this one will go 
away in patch [2/3] when the driver is converted to use regmap.

> 
>>  -	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)
>>  @@ -91,17 +110,21 @@ static int jz4740_pwm_apply(struct pwm_chip 
>> *chip, struct pwm_device *pwm,
>>   			    const struct pwm_state *state)
>>   {
>>   	struct jz4740_pwm_chip *jz4740 = to_jz4740(pwm->chip);
>>  +	struct clk *clk = pwm_get_chip_data(pwm),
>>  +		   *parent_clk = clk_get_parent(clk);
>>  +	unsigned long rate, period, duty;
>>   	unsigned long long tmp;
>>  -	unsigned long period, duty;
>>   	unsigned int prescaler = 0;
>>   	uint16_t ctrl;
>> 
>>  -	tmp = (unsigned long long)clk_get_rate(jz4740->clk) * 
>> state->period;
>>  +	rate = clk_get_rate(parent_clk);
> 
> Why is it the parent's rate that is relevant here?

We calculate the divider to be used for the "timerX" clock, so we need 
to know the parent clock.


>>  +	tmp = (unsigned long long)rate * state->period;
>>   	do_div(tmp, 1000000000);
>>   	period = tmp;
>> 
>>   	while (period > 0xffff && prescaler < 6) {
>>   		period >>= 2;
>>  +		rate >>= 2;
>>   		++prescaler;
>>   	}
>> 
>>  @@ -117,14 +140,14 @@ static int jz4740_pwm_apply(struct pwm_chip 
>> *chip, struct pwm_device *pwm,
>> 
>>   	jz4740_pwm_disable(chip, pwm);
>> 
>>  +	clk_set_rate(clk, rate);
> 
> This function's return code must be checked.

In practice this will never fail, but OK, will do.

Cheers,
-Paul

> 
>>   	jz4740_timer_set_count(pwm->hwpwm, 0);
>>   	jz4740_timer_set_duty(pwm->hwpwm, duty);
>>   	jz4740_timer_set_period(pwm->hwpwm, period);
>> 
>>  -	ctrl = JZ_TIMER_CTRL_PRESCALER(prescaler) | JZ_TIMER_CTRL_SRC_EXT 
>> |
>>  -		JZ_TIMER_CTRL_PWM_ABBRUPT_SHUTDOWN;
>>  -
>>  -	jz4740_timer_set_ctrl(pwm->hwpwm, ctrl);
>>  +	ctrl = jz4740_timer_get_ctrl(pwm->hwpwm);
>>  +	ctrl |= JZ_TIMER_CTRL_PWM_ABBRUPT_SHUTDOWN;
>> 
>>   	switch (state->polarity) {
>>   	case PWM_POLARITY_NORMAL:
> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König        
>     |
> Industrial Linux Solutions                 | 
> https://www.pengutronix.de/ |

Re: [PATCH v2 1/3] pwm: jz4740: Use clocks from TCU driver

[Uwe Kleine-König]

Hello Paul,

On Sun, Nov 17, 2019 at 11:58:43PM +0100, Paul Cercueil wrote:
> Le dim., nov. 17, 2019 at 21:20, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> a écrit :
> > On Sat, Nov 16, 2019 at 06:36:11PM +0100, Paul Cercueil wrote:
> > >  The ingenic-timer "TCU" driver 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>
> > >  Tested-by: Mathieu Malaterre <malat@debian.org>
> > >  Tested-by: Artur Rojek <contact@artur-rojek.eu>
> > >  ---
> > > 
> > >  Notes:
> > >      v2: This patch is now before the patch introducing regmap, so
> > > the code
> > >          has changed a bit.
> > > 
> > >   drivers/pwm/Kconfig      |  1 +
> > >   drivers/pwm/pwm-jz4740.c | 45 ++++++++++++++++++++++++++++------------
> > >   2 files changed, 33 insertions(+), 13 deletions(-)
> > > 
> > >  diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > >  index e3a2518503ed..e998e5cb01b0 100644
> > >  --- a/drivers/pwm/Kconfig
> > >  +++ b/drivers/pwm/Kconfig
> > >  @@ -225,6 +225,7 @@ config PWM_IMX_TPM
> > >   config PWM_JZ4740
> > >   	tristate "Ingenic JZ47xx PWM support"
> > >   	depends on MACH_INGENIC
> > >  +	depends on COMMON_CLK
> > >   	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 9d78cc21cb12..fd83644f9323 100644
> > >  --- a/drivers/pwm/pwm-jz4740.c
> > >  +++ b/drivers/pwm/pwm-jz4740.c
> > >  @@ -24,7 +24,6 @@
> > > 
> > >   struct jz4740_pwm_chip {
> > >   	struct pwm_chip chip;
> > >  -	struct clk *clk;
> > 
> > What is the motivation to go away from this approach to store the clock?
> 
> It's actually not the same clock. Instead of obtaining "ext" clock from the
> probe, we obtain "timerX" clocks (X being the PWM channel) from the request
> callback.

Before you used driver data and container_of to get it, now you used
pwm_set_chip_data. I wondered why you changed the approach to store
data. That the actual data is different now is another thing (and
obviously ok).

> > >   };
> > > 
> > >   static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip *chip)
> > >  @@ -34,6 +33,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.
> > >  @@ -41,16 +45,31 @@ 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))
> > 
> > 		if (PTR_ERR(clk) != -EPROBE_DEFER)
> > 			dev_err(chip->dev, "Failed to get clock: %pe\n", clk);
> 
> Never heard about that %pe. Will do that.

Yeah, that's new and IMHO quite nice.
 
> > >  +		return PTR_ERR(clk);
> > >  +
> > >  +	ret = clk_prepare_enable(clk);
> > >  +	if (ret) {
> > >  +		clk_put(clk);
> > >  +		return ret;
> > >  +	}
> > >  +
> > >  +	pwm_set_chip_data(pwm, clk);
> > > 
> > >   	return 0;
> > >   }
> > > 
> > >   static void jz4740_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> > >   {
> > >  +	struct clk *clk = pwm_get_chip_data(pwm);
> > >  +
> > >   	jz4740_timer_set_ctrl(pwm->hwpwm, 0);
> > 
> > What is the purpose of this call? I would have expected that all these
> > would go away when converting to the clk stuff?!
> 
> Some go away in patch [1/3] as they are clock-related, this one will go away
> in patch [2/3] when the driver is converted to use regmap.

I'd like to understand what it does. Judging from the name I expect this
is somehow related to the clock stuff and so I wonder if the conversion
to the clk API is as complete as it should be.

> > >  -	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)
> > >  @@ -91,17 +110,21 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > >   			    const struct pwm_state *state)
> > >   {
> > >   	struct jz4740_pwm_chip *jz4740 = to_jz4740(pwm->chip);
> > >  +	struct clk *clk = pwm_get_chip_data(pwm),
> > >  +		   *parent_clk = clk_get_parent(clk);
> > >  +	unsigned long rate, period, duty;
> > >   	unsigned long long tmp;
> > >  -	unsigned long period, duty;
> > >   	unsigned int prescaler = 0;
> > >   	uint16_t ctrl;
> > > 
> > >  -	tmp = (unsigned long long)clk_get_rate(jz4740->clk) * state->period;
> > >  +	rate = clk_get_rate(parent_clk);
> > 
> > Why is it the parent's rate that is relevant here?
> 
> We calculate the divider to be used for the "timerX" clock, so we need to
> know the parent clock.

Then the approach here is wrong. You should not assume anything about
the internal details of the clock, that's the task of the clock driver.
As a consumer of the clock just request a rate (or use clk_round_rate to
find a good setting first) and use that.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH v2 1/3] pwm: jz4740: Use clocks from TCU driver

[Paul Cercueil]

Hi Uwe,


Le lun., nov. 18, 2019 at 08:15, Uwe Kleine-König 
<u.kleine-koenig@pengutronix.de> a écrit :
> Hello Paul,
> 
> On Sun, Nov 17, 2019 at 11:58:43PM +0100, Paul Cercueil wrote:
>>  Le dim., nov. 17, 2019 at 21:20, Uwe Kleine-König
>>  <u.kleine-koenig@pengutronix.de> a écrit :
>>  > On Sat, Nov 16, 2019 at 06:36:11PM +0100, Paul Cercueil wrote:
>>  > >  The ingenic-timer "TCU" driver 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>
>>  > >  Tested-by: Mathieu Malaterre <malat@debian.org>
>>  > >  Tested-by: Artur Rojek <contact@artur-rojek.eu>
>>  > >  ---
>>  > >
>>  > >  Notes:
>>  > >      v2: This patch is now before the patch introducing regmap, 
>> so
>>  > > the code
>>  > >          has changed a bit.
>>  > >
>>  > >   drivers/pwm/Kconfig      |  1 +
>>  > >   drivers/pwm/pwm-jz4740.c | 45 
>> ++++++++++++++++++++++++++++------------
>>  > >   2 files changed, 33 insertions(+), 13 deletions(-)
>>  > >
>>  > >  diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>>  > >  index e3a2518503ed..e998e5cb01b0 100644
>>  > >  --- a/drivers/pwm/Kconfig
>>  > >  +++ b/drivers/pwm/Kconfig
>>  > >  @@ -225,6 +225,7 @@ config PWM_IMX_TPM
>>  > >   config PWM_JZ4740
>>  > >   	tristate "Ingenic JZ47xx PWM support"
>>  > >   	depends on MACH_INGENIC
>>  > >  +	depends on COMMON_CLK
>>  > >   	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 9d78cc21cb12..fd83644f9323 100644
>>  > >  --- a/drivers/pwm/pwm-jz4740.c
>>  > >  +++ b/drivers/pwm/pwm-jz4740.c
>>  > >  @@ -24,7 +24,6 @@
>>  > >
>>  > >   struct jz4740_pwm_chip {
>>  > >   	struct pwm_chip chip;
>>  > >  -	struct clk *clk;
>>  >
>>  > What is the motivation to go away from this approach to store the 
>> clock?
>> 
>>  It's actually not the same clock. Instead of obtaining "ext" clock 
>> from the
>>  probe, we obtain "timerX" clocks (X being the PWM channel) from the 
>> request
>>  callback.
> 
> Before you used driver data and container_of to get it, now you used
> pwm_set_chip_data. I wondered why you changed the approach to store
> data. That the actual data is different now is another thing (and
> obviously ok).

Thierry suggested it: https://lkml.org/lkml/2019/3/4/486

> 
>>  > >   };
>>  > >
>>  > >   static inline struct jz4740_pwm_chip *to_jz4740(struct 
>> pwm_chip *chip)
>>  > >  @@ -34,6 +33,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.
>>  > >  @@ -41,16 +45,31 @@ 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))
>>  >
>>  > 		if (PTR_ERR(clk) != -EPROBE_DEFER)
>>  > 			dev_err(chip->dev, "Failed to get clock: %pe\n", clk);
>> 
>>  Never heard about that %pe. Will do that.
> 
> Yeah, that's new and IMHO quite nice.
> 
>>  > >  +		return PTR_ERR(clk);
>>  > >  +
>>  > >  +	ret = clk_prepare_enable(clk);
>>  > >  +	if (ret) {
>>  > >  +		clk_put(clk);
>>  > >  +		return ret;
>>  > >  +	}
>>  > >  +
>>  > >  +	pwm_set_chip_data(pwm, clk);
>>  > >
>>  > >   	return 0;
>>  > >   }
>>  > >
>>  > >   static void jz4740_pwm_free(struct pwm_chip *chip, struct 
>> pwm_device *pwm)
>>  > >   {
>>  > >  +	struct clk *clk = pwm_get_chip_data(pwm);
>>  > >  +
>>  > >   	jz4740_timer_set_ctrl(pwm->hwpwm, 0);
>>  >
>>  > What is the purpose of this call? I would have expected that all 
>> these
>>  > would go away when converting to the clk stuff?!
>> 
>>  Some go away in patch [1/3] as they are clock-related, this one 
>> will go away
>>  in patch [2/3] when the driver is converted to use regmap.
> 
> I'd like to understand what it does. Judging from the name I expect 
> this
> is somehow related to the clock stuff and so I wonder if the 
> conversion
> to the clk API is as complete as it should be.

It clears the PWM channel's CTRL register. That's the register used for 
instance to enable the PWM function of a TCU channel.

> 
>>  > >  -	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)
>>  > >  @@ -91,17 +110,21 @@ static int jz4740_pwm_apply(struct 
>> pwm_chip *chip, struct pwm_device *pwm,
>>  > >   			    const struct pwm_state *state)
>>  > >   {
>>  > >   	struct jz4740_pwm_chip *jz4740 = to_jz4740(pwm->chip);
>>  > >  +	struct clk *clk = pwm_get_chip_data(pwm),
>>  > >  +		   *parent_clk = clk_get_parent(clk);
>>  > >  +	unsigned long rate, period, duty;
>>  > >   	unsigned long long tmp;
>>  > >  -	unsigned long period, duty;
>>  > >   	unsigned int prescaler = 0;
>>  > >   	uint16_t ctrl;
>>  > >
>>  > >  -	tmp = (unsigned long long)clk_get_rate(jz4740->clk) * 
>> state->period;
>>  > >  +	rate = clk_get_rate(parent_clk);
>>  >
>>  > Why is it the parent's rate that is relevant here?
>> 
>>  We calculate the divider to be used for the "timerX" clock, so we 
>> need to
>>  know the parent clock.
> 
> Then the approach here is wrong. You should not assume anything about
> the internal details of the clock, that's the task of the clock 
> driver.
> As a consumer of the clock just request a rate (or use clk_round_rate 
> to
> find a good setting first) and use that.

Totally agreed. I wanted to do that, but you were fighting tooth and 
nails against my patch "Improve algorithm of clock calculation", 
remember?

-Paul

Re: [PATCH v2 1/3] pwm: jz4740: Use clocks from TCU driver

[Uwe Kleine-König]

Hello Paul,

On Mon, Nov 18, 2019 at 11:55:56AM +0100, Paul Cercueil wrote:
> Le lun., nov. 18, 2019 at 08:15, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> a écrit :
> > On Sun, Nov 17, 2019 at 11:58:43PM +0100, Paul Cercueil wrote:
> > >  Le dim., nov. 17, 2019 at 21:20, Uwe Kleine-König
> > >  <u.kleine-koenig@pengutronix.de> a écrit :
> > >  > On Sat, Nov 16, 2019 at 06:36:11PM +0100, Paul Cercueil wrote:
> > >  > >   struct jz4740_pwm_chip {
> > >  > >   	struct pwm_chip chip;
> > >  > >  -	struct clk *clk;
> > >  >
> > >  > What is the motivation to go away from this approach to store the
> > > clock?
> > > 
> > >  It's actually not the same clock. Instead of obtaining "ext" clock
> > > from the
> > >  probe, we obtain "timerX" clocks (X being the PWM channel) from the
> > > request
> > >  callback.
> > 
> > Before you used driver data and container_of to get it, now you used
> > pwm_set_chip_data. I wondered why you changed the approach to store
> > data. That the actual data is different now is another thing (and
> > obviously ok).
> 
> Thierry suggested it: https://lkml.org/lkml/2019/3/4/486

If you motivate that in the commit log (preferably with a better
rationale than "Thierry suggested it") that's fine for. (Do I claim now
without having read the rationale :-)

> > >  > >   static void jz4740_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> > >  > >   {
> > >  > >  +	struct clk *clk = pwm_get_chip_data(pwm);
> > >  > >  +
> > >  > >   	jz4740_timer_set_ctrl(pwm->hwpwm, 0);
> > >  >
> > >  > What is the purpose of this call? I would have expected that all these
> > >  > would go away when converting to the clk stuff?!
> > > 
> > >  Some go away in patch [1/3] as they are clock-related, this one will go away
> > >  in patch [2/3] when the driver is converted to use regmap.
> > 
> > I'd like to understand what it does. Judging from the name I expect this
> > is somehow related to the clock stuff and so I wonder if the conversion
> > to the clk API is as complete as it should be.
> 
> It clears the PWM channel's CTRL register. That's the register used for
> instance to enable the PWM function of a TCU channel.

OK, so this is a register in a different register range than the PWM
related registers to set duty and period, right? Looking at the code,
this register has a bit to enable PWM mode and other than that bit
fields to tune the clock feeding the PWM counters, right?

This probably explains my resistance because such a setup if really hard
to map to nice code. At least the "PWM enable" bit doesn't fit the clk
abstraction, no good idea here. Maybe it's easier and more straight
forward to not wrap that register in a clock driver and only use a clk
for the parent? What is the motivation to convert this piece of hardware
to a clk driver? Or abstract it as a proper clk and provide a function
to enable PWM mode for channel X?

> > >  > >  -	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)
> > >  > >  @@ -91,17 +110,21 @@ static int jz4740_pwm_apply(struct
> > > pwm_chip *chip, struct pwm_device *pwm,
> > >  > >   			    const struct pwm_state *state)
> > >  > >   {
> > >  > >   	struct jz4740_pwm_chip *jz4740 = to_jz4740(pwm->chip);
> > >  > >  +	struct clk *clk = pwm_get_chip_data(pwm),
> > >  > >  +		   *parent_clk = clk_get_parent(clk);
> > >  > >  +	unsigned long rate, period, duty;
> > >  > >   	unsigned long long tmp;
> > >  > >  -	unsigned long period, duty;
> > >  > >   	unsigned int prescaler = 0;
> > >  > >   	uint16_t ctrl;
> > >  > >
> > >  > >  -	tmp = (unsigned long long)clk_get_rate(jz4740->clk) *
> > > state->period;
> > >  > >  +	rate = clk_get_rate(parent_clk);
> > >  >
> > >  > Why is it the parent's rate that is relevant here?
> > > 
> > >  We calculate the divider to be used for the "timerX" clock, so we
> > > need to
> > >  know the parent clock.
> > 
> > Then the approach here is wrong. You should not assume anything about
> > the internal details of the clock, that's the task of the clock driver.
> > As a consumer of the clock just request a rate (or use clk_round_rate to
> > find a good setting first) and use that.
> 
> Totally agreed. I wanted to do that, but you were fighting tooth and nails
> against my patch "Improve algorithm of clock calculation", remember?

No, I don't, but I looked that up :-) And I fighted because I thought
the clk API isn't used properly (and I think your problem is that the
clk API as is today doesn't give you what you want, so there is more
work to do on the clk side of the problem).

The conceptual problem I see is that currently the code uses some
internal knowledge about how this timer clock works but as soon as you
use the clk abstraction it's wrong to use such internal knowledge.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH v2 1/3] pwm: jz4740: Use clocks from TCU driver

[Paul Cercueil]

Hi Uwe,


Le lun., nov. 18, 2019 at 12:19, Uwe Kleine-König 
<u.kleine-koenig@pengutronix.de> a écrit :
> Hello Paul,
> 
> On Mon, Nov 18, 2019 at 11:55:56AM +0100, Paul Cercueil wrote:
>>  Le lun., nov. 18, 2019 at 08:15, Uwe Kleine-König
>>  <u.kleine-koenig@pengutronix.de> a écrit :
>>  > On Sun, Nov 17, 2019 at 11:58:43PM +0100, Paul Cercueil wrote:
>>  > >  Le dim., nov. 17, 2019 at 21:20, Uwe Kleine-König
>>  > >  <u.kleine-koenig@pengutronix.de> a écrit :
>>  > >  > On Sat, Nov 16, 2019 at 06:36:11PM +0100, Paul Cercueil 
>> wrote:
>>  > >  > >   struct jz4740_pwm_chip {
>>  > >  > >   	struct pwm_chip chip;
>>  > >  > >  -	struct clk *clk;
>>  > >  >
>>  > >  > What is the motivation to go away from this approach to 
>> store the
>>  > > clock?
>>  > >
>>  > >  It's actually not the same clock. Instead of obtaining "ext" 
>> clock
>>  > > from the
>>  > >  probe, we obtain "timerX" clocks (X being the PWM channel) 
>> from the
>>  > > request
>>  > >  callback.
>>  >
>>  > Before you used driver data and container_of to get it, now you 
>> used
>>  > pwm_set_chip_data. I wondered why you changed the approach to 
>> store
>>  > data. That the actual data is different now is another thing (and
>>  > obviously ok).
>> 
>>  Thierry suggested it: https://lkml.org/lkml/2019/3/4/486
> 
> If you motivate that in the commit log (preferably with a better
> rationale than "Thierry suggested it") that's fine for. (Do I claim 
> now
> without having read the rationale :-)

I don't really have a better rationale. The alternative was to have a 
"struct clk[NB_PWMS];" in the struct jz4740_pwm_chip, so this is 
arguably better. I'm not sure it's worth mentioning in the commit 
message, is it?


>>  > >  > >   static void jz4740_pwm_free(struct pwm_chip *chip, 
>> struct pwm_device *pwm)
>>  > >  > >   {
>>  > >  > >  +	struct clk *clk = pwm_get_chip_data(pwm);
>>  > >  > >  +
>>  > >  > >   	jz4740_timer_set_ctrl(pwm->hwpwm, 0);
>>  > >  >
>>  > >  > What is the purpose of this call? I would have expected that 
>> all these
>>  > >  > would go away when converting to the clk stuff?!
>>  > >
>>  > >  Some go away in patch [1/3] as they are clock-related, this 
>> one will go away
>>  > >  in patch [2/3] when the driver is converted to use regmap.
>>  >
>>  > I'd like to understand what it does. Judging from the name I 
>> expect this
>>  > is somehow related to the clock stuff and so I wonder if the 
>> conversion
>>  > to the clk API is as complete as it should be.
>> 
>>  It clears the PWM channel's CTRL register. That's the register used 
>> for
>>  instance to enable the PWM function of a TCU channel.
> 
> OK, so this is a register in a different register range than the PWM
> related registers to set duty and period, right? Looking at the code,
> this register has a bit to enable PWM mode and other than that bit
> fields to tune the clock feeding the PWM counters, right?

They are actually all in the same register range. Each channel has 4 
32-bit registers, the first one is the CTRL (aka. TCSR) register which 
is written to here. The following two configure the duty/period values, 
the last one is the counter. The 'timer enable' bit is however in the 
global TCU registers area.

The clock bits of the TCSRs registers (including the TCSR registers of 
the watchdog and 64-bit OS timer) are controlled by the clocks driver. 
All register accesses are properly handled thanks to regmap, that we 
add in patch [2/3].


> This probably explains my resistance because such a setup if really 
> hard
> to map to nice code. At least the "PWM enable" bit doesn't fit the clk
> abstraction, no good idea here. Maybe it's easier and more straight
> forward to not wrap that register in a clock driver and only use a clk
> for the parent? What is the motivation to convert this piece of 
> hardware
> to a clk driver? Or abstract it as a proper clk and provide a function
> to enable PWM mode for channel X?

The motivation behind converting it to a clocks driver, is that it's 
not only used for PWM, but for system timers, the watchdog, and the 
64-bit OS timer.

There is some overview of the TCU hardware here: 
linux/Documentation/mips/ingenic-tcu.rst (and yes, that hardware is a 
mess).

All the bits have been merged or accepted upstream minus the OST driver 
which is still under review, and this patchset.


>>  > >  > >  -	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)
>>  > >  > >  @@ -91,17 +110,21 @@ static int jz4740_pwm_apply(struct
>>  > > pwm_chip *chip, struct pwm_device *pwm,
>>  > >  > >   			    const struct pwm_state *state)
>>  > >  > >   {
>>  > >  > >   	struct jz4740_pwm_chip *jz4740 = to_jz4740(pwm->chip);
>>  > >  > >  +	struct clk *clk = pwm_get_chip_data(pwm),
>>  > >  > >  +		   *parent_clk = clk_get_parent(clk);
>>  > >  > >  +	unsigned long rate, period, duty;
>>  > >  > >   	unsigned long long tmp;
>>  > >  > >  -	unsigned long period, duty;
>>  > >  > >   	unsigned int prescaler = 0;
>>  > >  > >   	uint16_t ctrl;
>>  > >  > >
>>  > >  > >  -	tmp = (unsigned long long)clk_get_rate(jz4740->clk) *
>>  > > state->period;
>>  > >  > >  +	rate = clk_get_rate(parent_clk);
>>  > >  >
>>  > >  > Why is it the parent's rate that is relevant here?
>>  > >
>>  > >  We calculate the divider to be used for the "timerX" clock, so 
>> we
>>  > > need to
>>  > >  know the parent clock.
>>  >
>>  > Then the approach here is wrong. You should not assume anything 
>> about
>>  > the internal details of the clock, that's the task of the clock 
>> driver.
>>  > As a consumer of the clock just request a rate (or use 
>> clk_round_rate to
>>  > find a good setting first) and use that.
>> 
>>  Totally agreed. I wanted to do that, but you were fighting tooth 
>> and nails
>>  against my patch "Improve algorithm of clock calculation", remember?
> 
> No, I don't, but I looked that up :-) And I fighted because I thought
> the clk API isn't used properly (and I think your problem is that the
> clk API as is today doesn't give you what you want, so there is more
> work to do on the clk side of the problem).

That's a question of point of view, really. I need to specify the 
maximum clock rate that still gives me a valid value, the clk API gives 
me clk_set_max_rate(). Doesn't look like I'm doing something out of 
bounds.

> The conceptual problem I see is that currently the code uses some
> internal knowledge about how this timer clock works but as soon as you
> use the clk abstraction it's wrong to use such internal knowledge.

Well, this patch is still a step in the right direction. I too would 
like to see a better solution, but I don't want to hold it until we fix 
the problem mentioned above. Right now jz4740-pwm is broken upstream 
(incompatible with the documented DT bindings) and that's something I 
want fixed ASAP, hence this reduced patchset.

Best regards,
-Paul

Re: [PATCH v2 1/3] pwm: jz4740: Use clocks from TCU driver

[Uwe Kleine-König]

Hi Paul,

On Mon, Nov 18, 2019 at 02:42:54PM +0100, Paul Cercueil wrote:
> Le lun., nov. 18, 2019 at 12:19, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> a écrit :
> > On Mon, Nov 18, 2019 at 11:55:56AM +0100, Paul Cercueil wrote:
> > >  Le lun., nov. 18, 2019 at 08:15, Uwe Kleine-König
> > >  <u.kleine-koenig@pengutronix.de> a écrit :
> > >  > On Sun, Nov 17, 2019 at 11:58:43PM +0100, Paul Cercueil wrote:
> > >  > >  Le dim., nov. 17, 2019 at 21:20, Uwe Kleine-König
> > >  > >  <u.kleine-koenig@pengutronix.de> a écrit :
> > >  > >  > On Sat, Nov 16, 2019 at 06:36:11PM +0100, Paul Cercueil wrote:
> > >  > >  > >   struct jz4740_pwm_chip {
> > >  > >  > >   	struct pwm_chip chip;
> > >  > >  > >  -	struct clk *clk;
> > >  > >  >
> > >  > >  > What is the motivation to go away from this approach to store the clock?
> > >  > >
> > >  > >  It's actually not the same clock. Instead of obtaining "ext" clock from the
> > >  > >  probe, we obtain "timerX" clocks (X being the PWM channel) from the request
> > >  > >  callback.
> > >  >
> > >  > Before you used driver data and container_of to get it, now you used
> > >  > pwm_set_chip_data. I wondered why you changed the approach to store
> > >  > data. That the actual data is different now is another thing (and
> > >  > obviously ok).
> > > 
> > >  Thierry suggested it: https://lkml.org/lkml/2019/3/4/486
> > 
> > If you motivate that in the commit log (preferably with a better
> > rationale than "Thierry suggested it") that's fine for. (Do I claim now
> > without having read the rationale :-)
> 
> I don't really have a better rationale. The alternative was to have a
> "struct clk[NB_PWMS];" in the struct jz4740_pwm_chip, so this is arguably
> better. I'm not sure it's worth mentioning in the commit message, is it?

It depends what you want to achieve. An array in the chip data might be
too big, with per-pwm data your memory fragments. Still worth to mention
something like:

	The new code now uses a clk pointer per PWM (instead of a clk
	per pwm-chip before). So the pointer is stored in per-pwm data
	now.
 
> They are actually all in the same register range. Each channel has 4 32-bit
> registers, the first one is the CTRL (aka. TCSR) register which is written
> to here. The following two configure the duty/period values, the last one is
> the counter. The 'timer enable' bit is however in the global TCU registers
> area.
> 
> The clock bits of the TCSRs registers (including the TCSR registers of the
> watchdog and 64-bit OS timer) are controlled by the clocks driver. All
> register accesses are properly handled thanks to regmap, that we add in
> patch [2/3].

You reshuffled in v3 which obsoletes some of the statements here I
think. Will take a look there.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[PATCH v2 2/3] pwm: jz4740: Obtain regmap from parent node

[Paul Cercueil]

The TCU registers are shared between a handful of drivers, accessing
them through the same regmap.

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>
Tested-by: Mathieu Malaterre <malat@debian.org>
Tested-by: Artur Rojek <contact@artur-rojek.eu>
---

Notes:
    v2: This patch is now after the patch introducing TCU clocks, so the code
        changed a bit.

 drivers/pwm/Kconfig      |  1 +
 drivers/pwm/pwm-jz4740.c | 70 ++++++++++++++++++++++++++--------------
 2 files changed, 46 insertions(+), 25 deletions(-)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index e998e5cb01b0..b6cbfb97e85f 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -226,6 +226,7 @@ config PWM_JZ4740
 	tristate "Ingenic JZ47xx PWM support"
 	depends on MACH_INGENIC
 	depends on COMMON_CLK
+	select MFD_SYSCON
 	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 fd83644f9323..a5368b98652d 100644
--- a/drivers/pwm/pwm-jz4740.c
+++ b/drivers/pwm/pwm-jz4740.c
@@ -13,17 +13,19 @@
 #include <linux/err.h>
 #include <linux/gpio.h>
 #include <linux/kernel.h>
+#include <linux/mfd/ingenic-tcu.h>
+#include <linux/mfd/syscon.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 regmap *map;
 };
 
 static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip *chip)
@@ -64,9 +66,10 @@ static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 
 static void jz4740_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 {
+	struct jz4740_pwm_chip *jz = to_jz4740(chip);
 	struct clk *clk = pwm_get_chip_data(pwm);
 
-	jz4740_timer_set_ctrl(pwm->hwpwm, 0);
+	regmap_write(jz->map, TCU_REG_TCSRc(pwm->hwpwm), 0);
 
 	clk_disable_unprepare(clk);
 	clk_put(clk);
@@ -74,36 +77,39 @@ static void jz4740_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 
 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);
 
 	/*
 	 * Set duty > period. This trick allows the TCU channels in TCU2 mode to
 	 * properly return to their init level.
 	 */
-	jz4740_timer_set_duty(pwm->hwpwm, 0xffff);
-	jz4740_timer_set_period(pwm->hwpwm, 0x0);
+	regmap_write(jz->map, TCU_REG_TDHRc(pwm->hwpwm), 0xffff);
+	regmap_write(jz->map, TCU_REG_TDFRc(pwm->hwpwm), 0x0);
 
 	/*
 	 * 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_apply(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -115,7 +121,6 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	unsigned long rate, period, duty;
 	unsigned long long tmp;
 	unsigned int prescaler = 0;
-	uint16_t ctrl;
 
 	rate = clk_get_rate(parent_clk);
 	tmp = (unsigned long long)rate * state->period;
@@ -142,24 +147,32 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	clk_set_rate(clk, rate);
 
-	jz4740_timer_set_count(pwm->hwpwm, 0);
-	jz4740_timer_set_duty(pwm->hwpwm, duty);
-	jz4740_timer_set_period(pwm->hwpwm, period);
+	/* Reset counter to 0 */
+	regmap_write(jz4740->map, TCU_REG_TCNTc(pwm->hwpwm), 0);
+
+	/* Set duty */
+	regmap_write(jz4740->map, TCU_REG_TDHRc(pwm->hwpwm), duty);
 
-	ctrl = jz4740_timer_get_ctrl(pwm->hwpwm);
-	ctrl |= JZ_TIMER_CTRL_PWM_ABBRUPT_SHUTDOWN;
+	/* Set period */
+	regmap_write(jz4740->map, TCU_REG_TDFRc(pwm->hwpwm), period);
 
+	/* Set abrupt shutdown */
+	regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
+			   TCU_TCSR_PWM_SD, TCU_TCSR_PWM_SD);
+
+	/* Set polarity */
 	switch (state->polarity) {
 	case PWM_POLARITY_NORMAL:
-		ctrl &= ~JZ_TIMER_CTRL_PWM_ACTIVE_LOW;
+		regmap_update_bits(jz4740->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(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
+				   TCU_TCSR_PWM_INITL_HIGH,
+				   TCU_TCSR_PWM_INITL_HIGH);
 		break;
 	}
 
-	jz4740_timer_set_ctrl(pwm->hwpwm, ctrl);
-
 	if (state->enabled)
 		jz4740_pwm_enable(chip, pwm);
 
@@ -176,12 +189,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->chip.dev = &pdev->dev;
+	jz4740->map = device_node_to_regmap(dev->parent->of_node);
+	if (!jz4740->map) {
+		dev_err(dev, "regmap not found\n");
+		return -EINVAL;
+	}
+
+	jz4740->chip.dev = dev;
 	jz4740->chip.ops = &jz4740_pwm_ops;
 	jz4740->chip.npwm = NUM_PWM;
 	jz4740->chip.base = -1;
-- 
2.24.0

[PATCH v2 3/3] pwm: jz4740: Allow selection of PWM channels 0 and 1

[Paul Cercueil]

The TCU channels 0 and 1 were previously reserved for system tasks, and
thus unavailable for PWM.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Tested-by: Mathieu Malaterre <malat@debian.org>
Tested-by: Artur Rojek <contact@artur-rojek.eu>
---

Notes:
    v2: No change

 drivers/pwm/pwm-jz4740.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
index a5368b98652d..801dfabfa6f4 100644
--- a/drivers/pwm/pwm-jz4740.c
+++ b/drivers/pwm/pwm-jz4740.c
@@ -33,6 +33,19 @@ static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip *chip)
 	return container_of(chip, struct jz4740_pwm_chip, chip);
 }
 
+static bool jz4740_pwm_can_use_chn(struct jz4740_pwm_chip *jz,
+				   unsigned int channel)
+{
+	/* Enable all TCU channels for PWM use by default except channels 0/1 */
+	u32 pwm_channels_mask = GENMASK(NUM_PWM - 1, 2);
+
+	device_property_read_u32(jz->chip.dev->parent,
+				 "ingenic,pwm-channels-mask",
+				 &pwm_channels_mask);
+
+	return !!(pwm_channels_mask & BIT(channel));
+}
+
 static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct jz4740_pwm_chip *jz = to_jz4740(chip);
@@ -40,11 +53,7 @@ static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 	char clk_name[16];
 	int ret;
 
-	/*
-	 * Timers 0 and 1 are used for system tasks, so they are unavailable
-	 * for use as PWMs.
-	 */
-	if (pwm->hwpwm < 2)
+	if (!jz4740_pwm_can_use_chn(jz, pwm->hwpwm))
 		return -EBUSY;
 
 	snprintf(clk_name, sizeof(clk_name), "timer%u", pwm->hwpwm);
-- 
2.24.0