Message-ID: <20190812083556.dvprpwv6mjy3cjae@pengutronix.de>
Date:   Mon, 12 Aug 2019 10:35:56 +0200
From:   Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
To:     Baolin Wang <baolin.wang@...aro.org>
Cc:     Thierry Reding <thierry.reding@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Orson Zhai <orsonzhai@...il.com>,
        Chunyan Zhang <zhang.lyra@...il.com>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        linux-pwm@...r.kernel.org, DTML <devicetree@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>, kernel@...gutronix.de
Subject: Re: [PATCH 2/2] pwm: sprd: Add Spreadtrum PWM support

Hello Baolin,

On Mon, Aug 12, 2019 at 03:29:07PM +0800, Baolin Wang wrote:
> Hi Uwe,
> 
> On Fri, 9 Aug 2019 at 22:41, Uwe Kleine-König
> <u.kleine-koenig@...gutronix.de> wrote:
> > On Fri, Aug 09, 2019 at 06:06:21PM +0800, Baolin Wang wrote:
> > > On Fri, 9 Aug 2019 at 17:10, Uwe Kleine-König
> > > <u.kleine-koenig@...gutronix.de> wrote:
> > > > On Thu, Aug 08, 2019 at 04:59:39PM +0800, Baolin Wang wrote:
> > > > > +static void sprd_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > > +                            struct pwm_state *state)
> > > > > +{
> > > > > +     struct sprd_pwm_chip *spc =
> > > > > +             container_of(chip, struct sprd_pwm_chip, chip);
> > > > > +     struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
> > > > > +     u32 enabled, duty, prescale;
> > > > > +     u64 tmp;
> > > > > +     int ret;
> > > > > +
> > > > > +     ret = clk_bulk_prepare_enable(SPRD_PWM_NUM_CLKS, chn->clks);
> > > > > +     if (ret) {
> > > > > +             dev_err(spc->dev, "failed to enable pwm%u clocks\n",
> > > > > +                     pwm->hwpwm);
> > > > > +             return;
> > > > > +     }
> > > > > +
> > > > > +     chn->clk_enabled = true;
> > > > > +
> > > > > +     duty = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_DUTY) & SPRD_PWM_REG_MSK;
> > > > > +     prescale = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_PRESCALE) & SPRD_PWM_REG_MSK;
> > > > > +     enabled = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_ENABLE) & SPRD_PWM_ENABLE_BIT;
> > > > > +
> > > > > +     /*
> > > > > +      * According to the datasheet, the period_ns and duty_ns calculation
> > > > > +      * formula should be:
> > > > > +      * period_ns = 10^9 * (prescale + 1) * mod / clk_rate
> > > > > +      * duty_ns = 10^9 * (prescale + 1) * duty / clk_rate
> > > > > +      */
> > > > > +     tmp = (prescale + 1) * 1000000000ULL * SPRD_PWM_MOD_MAX;
> > > > > +     state->period = div64_u64(tmp, chn->clk_rate);
> > > >
> > > > This is not idempotent. If you apply the configuration that is returned
> > > > here this shouldn't result in a reconfiguration.
> > >
> > > Since we may configure the  PWM in bootloader, so in kernel part we
> > > should get current PWM state to avoid reconfiguration if state
> > > configuration are same.
> >
> > This is also important as some consumers might do something like:
> >
> >         state = pwm_get_state(mypwm)
> >         if (something):
> >                 state->duty = 0
> >         else:
> >                 state->duty = state->period / 2
> >         pwm_set_state(mypwm, state)
> >
> > and then period shouldn't get smaller in each step.
> > (This won't happen as of now because the PWM framework caches the last
> > state that was set and returns this for pwm_get_state. Still getting
> > this right would be good.)
> 
> I understood your concern, but the period can be configured in
> bootloader, we have no software things to save the accurate period.

I don't understand what you're saying here. The bootloader configuring
the hardware is a usual use-case. That's why we have the .get_state
callback in the first place.

> Moreover I think I can change to use DIV_ROUND_CLOSET_ULL to keep the
> accuracy.

DIV_ROUND_CLOSEST_ULL still doesn't match what the apply callback uses.
With the lack of an official statement from the maintainer I'd prefer
.apply to round down and implement .get_state such that

	pwm_apply(pwm, pwm_get_state(pwm))

is a no-op.
 
> > > > > +
> > > > > +                     dev_err(spc->dev, "failed to get channel clocks\n");
> > > > > +                     return ret;
> > > > > +             }
> > > > > +
> > > > > +             clk_pwm = chn->clks[1].clk;
> > > >
> > > > This 1 looks suspicious. Are you using all clocks provided in the dtb at
> > > > all? You're not using i in the loop at all, this doesn't look right.
> > >
> > > Like I said above, each channel has 2 clocks: enable clock and pwm
> > > clock, the 2nd clock of each channel's bulk clocks is the pwm clock,
> > > which is used to set the source clock. I know this's not easy to read,
> > > so do you have any good suggestion?
> >
> > Not sure this is easily possible to rework to make this clearer.
> >
> > Do these clks have different uses? e.g. one to enable register access
> > and the other to enable the pwm output? If so just using
> 
> Yes.

So assuming one of the clocks is for operation of the output and the
other for accessing the registers, the latter can be disabled at the end
of each callback?

> > devm_clk_bulk_get isn't the right thing because you should be able know
> > if clks[0] or clks[1] is the one you need to enable the output (or
> > register access).
> 
> We've fixed the clock order in bulk clocks by the array
> 'sprd_pwm_clks', maybe I should define one readable macro instead of
> magic number.

ack.

> > > > > +             if (!clk_set_parent(clk_pwm, clk_parent))
> > > > > +                     chn->clk_rate = clk_get_rate(clk_pwm);
> > > > > +             else
> > > > > +                     chn->clk_rate = SPRD_PWM_DEFAULT_CLK;
> > > >
> > > > I don't know all the clock framework details, but I think there are
> > > > better ways to ensure that a given clock is used as parent for another
> > > > given clock. Please read the chapter about "Assigned clock parents and
> > > > rates" in the clock bindings and check if this could be used for the
> > > > purpose here and so simplify the driver.
> > >
> > > Actually there are many other drivers set the parent clock like this,
> > > and we want a default clock if failed to set the parent clock.
> >
> > These might be older than the clk framework capabilities, or the
> > reviewers didn't pay attention to this detail; both shouldn't be a
> > reason to not make it better here.
> 
> The clock framework supplies 'assigned-clocks' and
> 'assigned-clock-parents' properties to set parent, but for our case we
> still want to set a default clock rate if failed to set parent when
> met some abnormal things.

Without understanding the complete problem I'd say this is out of the
area the driver should care about.
 
Best regards
Uwe

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

Powered by blists - more mailing lists