lkml.org 
[lkml]   [2020]   [Feb]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v3 1/3] pwm: jz4740: Use clocks from TCU driver
    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
    >> having
    >> 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
    >> with
    >> 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
    >> 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.
    >> v3: - Use %pe printf specifier to print error
    >> - Update commit message
    >> - Removed call to jz4740_timer_set_ctrl() in jz4740_pwm_free()
    >> which
    >> was reseting the clock's parent.
    >>
    >> drivers/pwm/Kconfig | 1 +
    >> drivers/pwm/pwm-jz4740.c | 53
    >> +++++++++++++++++++++++++++++-----------
    >> 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
    >> *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,32 @@ 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", 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)
    >> {
    >> - 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
    >> pwm_device *pwm)
    >> @@ -91,17 +111,22 @@ 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;
    >> + int err;
    >>
    >> - 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 +142,18 @@ static int jz4740_pwm_apply(struct pwm_chip
    >> *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
    > the
    > 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
    > the
    > 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.

    >
    >> +
    >> 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 +187,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);
    >
    > 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


    \
     
     \ /
      Last update: 2020-02-11 18:09    [W:3.890 / U:0.032 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site