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

[Paul Cercueil <paul@crapouillou.net>]

Hi Uwe,

Le mar., févr. 11, 2020 at 14:07, Paul Cercueil <paul@crapouillou.net> 
a écrit :
> Hi Uwe,
> 
> 
> Le mar., févr. 11, 2020 at 17:46, Uwe Kleine-König 
> <u.kleine-koenig@pengutronix.de> a écrit :
>> hello Paul,
>> 
>> thanks for the ping, I lost your series from my radar.
>> 
>> On Tue, Dec 10, 2019 at 04:27:32PM +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 
>>> \x7f\x7fhaving
>>>  these settings hardcoded in this driver.
>>> 
>>>  Each PWM channel's clk pointer is stored in PWM chip data to keep
>>>  the code simple. The calls to arch-specific timer code is replaced 
>>> \x7f\x7fwith
>>>  standard clock API calls to start and stop each channel's clock.
>>> 
>>>  While this driver is devicetree-compatible, it is never (as of now)
>>>  probed from devicetree, so this change does not introduce a ABI 
>>> \x7f\x7fproblem
>>>  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 
>>> \x7f\x7fthe code
>>>      	has changed a bit.
>>>      v3: - Use %pe printf specifier to print error
>>>      	- Update commit message
>>>      	- Removed call to jz4740_timer_set_ctrl() in 
>>> jz4740_pwm_free() \x7f\x7fwhich
>>>      	  was reseting the clock's parent.
>>> 
>>>   drivers/pwm/Kconfig      |  1 +
>>>   drivers/pwm/pwm-jz4740.c | 53 
>>> \x7f\x7f+++++++++++++++++++++++++++++-----------
>>>   2 files changed, 40 insertions(+), 14 deletions(-)
>>> 
>>>  diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>>>  index bd21655c37a6..15d3816341f3 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..ee50ac5fabb6 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 
>>> \x7f\x7f*chip)
>>>  @@ -34,6 +33,11 @@ static inline struct jz4740_pwm_chip 
>>> \x7f\x7f*to_jz4740(struct pwm_chip *chip)
>>> 
>>>   static int jz4740_pwm_request(struct pwm_chip *chip, struct 
>>> \x7f\x7fpwm_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 
>>> \x7f\x7funavailable
>>>   	 * for use as PWMs.
>>>  @@ -41,16 +45,32 @@ static int jz4740_pwm_request(struct pwm_chip 
>>> \x7f\x7f*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", 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 
>>> \x7f\x7fpwm_device *pwm)
>>>   {
>>>  -	jz4740_timer_set_ctrl(pwm->hwpwm, 0);
>>>  +	struct clk *clk = pwm_get_chip_data(pwm);
>>> 
>>>  -	jz4740_timer_stop(pwm->hwpwm);
>>>  +	clk_disable_unprepare(clk);
>>>  +	clk_put(clk);
>>>   }
>>> 
>>>   static int jz4740_pwm_enable(struct pwm_chip *chip, struct 
>>> \x7f\x7fpwm_device *pwm)
>>>  @@ -91,17 +111,22 @@ static int jz4740_pwm_apply(struct pwm_chip 
>>> \x7f\x7f*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;
>>>  +	int err;
>>> 
>>>  -	tmp = (unsigned long long)clk_get_rate(jz4740->clk) * 
>>> \x7f\x7fstate->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 +142,18 @@ static int jz4740_pwm_apply(struct pwm_chip 
>>> \x7f\x7f*chip, struct pwm_device *pwm,
>>> 
>>>   	jz4740_pwm_disable(chip, pwm);
>>> 
>>>  +	err = clk_set_rate(clk, rate);
>>>  +	if (err) {
>>>  +		dev_err(chip->dev, "Unable to set rate: %d", err);
>>>  +		return err;
>>>  +	}
>> 
>> What I don't like here is that you use internal knowledge about the
>> clock to set a specific rate. Given this is a 1:1 conversion to hide 
>> \x7fthe
>> previously contained register accesses behind the clk API (which is a
>> fine move) this is ok. But IMHO this should be cleaned up to rely on 
>> \x7fthe
>> clk code. The more natural thing to do here would be to calculate the
>> rate you can work with and request that. Maybe add a todo-comment to
>> clean this up?
> 
> Yes, I am very aware of that - I had a patch to clean it up but we 
> disagreed on the algorithm, so I left it for later, since these three 
> patches are much more important for now. Not sure it needs a TODO, I 
> plan to submit the cleanup patch once these are merged, but I can 
> send a v4 if you prefer.

Should I respin this?

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 \x7f\x7f|
>>>  -		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 +187,6 @@ static int jz4740_pwm_probe(struct 
>>> \x7f\x7fplatform_device *pdev)
>>>   	if (!jz4740)
>>>   		return -ENOMEM;
>>> 
>>>  -	jz4740->clk = devm_clk_get(&pdev->dev, "ext");
>>>  -	if (IS_ERR(jz4740->clk))
>>>  -		return PTR_ERR(jz4740->clk);
>> 
>> Huh, jz4740->clk was never enabled?
> 
> Correct. The "ext" clock is an external oscillator that is always 
> enabled, so the code just didn't bother calling clk_enable (but it's 
> a bug of course).
> 
> Cheers,
> -Paul
>