Re: [PATCH 3/3] pwm: imx: Implement get_state() function for hardware readout

[Uwe Kleine-König]

Hello Thierry,

On Thu, Dec 13, 2018 at 06:00:00PM +0100, Thierry Reding wrote:
> On Thu, Dec 13, 2018 at 09:52:13AM +0100, Uwe Kleine-KÃnig wrote:
> > On Wed, Dec 12, 2018 at 11:54:32AM +0100, Thierry Reding wrote:
> > > On Mon, Oct 01, 2018 at 04:19:48PM +0200, Michal VokÃÄ wrote:
> > > > Implement the get_state() function and set the initial state to reflect
> > > > real state of the hardware. This allows to keep the PWM running if it was
> > > > enabled in bootloader. It is very similar to the GPIO behavior. GPIO pin
> > > > set as output in bootloader keep the same setting in Linux unless it is
> > > > reconfigured.
> > > > 
> > > > If we find the PWM block enabled we need to prepare and enable its source
> > > > clock otherwise the clock will be disabled late in the boot as unused.
> > > > That will leave the PWM in enabled state but with disabled clock. That has
> > > > a side effect that the PWM output is left at its current level at which
> > > > the clock was disabled. It is totally non-deterministic and it may be LOW
> > > > or HIGH.
> > > > 
> > > > Signed-off-by: Michal VokÃÄ <michal.vokac@xxxxxxxxx>
> > > > ---
> > > >  drivers/pwm/pwm-imx.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 53 insertions(+)
> > > 
> > > Applied, thanks.
> > 
> > Did you miss my feedback for this patch or did you choose to ignore it?
> 
> Don't have anything in my inbox, but I see that it's there on patchwork,
> so I suspect it was eaten by the spam filter.

:-|

> Let me copy-paste here and go through it.
> 
> > > @@ -93,6 +96,55 @@ struct imx_chip {
> > >
> > >  #define to_imx_chip(chip) container_of(chip, struct imx_chip, chip)
> > >
> > > +static void imx_pwm_get_state(struct pwm_chip *chip,
> > > +  struct pwm_device *pwm, struct pwm_state *state)
> > 
> > broken alignment.
> 
> I can fix that up, no need to resend for that.

fine.

> > > +{
> > > + struct imx_chip *imx = to_imx_chip(chip);
> > > + u32 period, prescaler, pwm_clk, ret, val;
> > > + u64 tmp;
> > > +
> > > + val = readl(imx->mmio_base + MX3_PWMCR);
> > > +
> > > + if (val & MX3_PWMCR_EN) {
> > > +  state->enabled = true;
> > > +  ret = clk_prepare_enable(imx->clk_per);
> > > +  if (ret)
> > > +   return;
> > > + } else {
> > > +  state->enabled = false;
> > > + }
> > > +
> > > + switch (FIELD_GET(MX3_PWMCR_POUTC, val)) {
> > > + case MX3_PWMCR_POUTC_NORMAL:
> > > +  state->polarity = PWM_POLARITY_NORMAL;
> > > +  break;
> > > + case MX3_PWMCR_POUTC_INVERTED:
> > > +  state->polarity = PWM_POLARITY_INVERSED;
> > > +  break;
> > > + default:
> > > +  dev_warn(chip->dev, "can't set polarity, output disconnected");
> > 
> > Should we return an error here?
> 
> We can't return an error here because the function has a void return
> type. I'm not sure what it means if the POUTC is neither "normal" nor
> "inverted", but perhaps a good idea would be to default to either of
> those two in that case, or perhaps forcibly disable the PWM so that
> we get known-good values in the registers?

The manual says "PWM output is disconnected". So I think the right thing
to do is to consider the pwm as disabled. (But checking how the output
actually behaves would be great.) (Maybe this setting is even the
solution to our "Make the output high-Z on disable" discussion?)

> I'm tempted not to treat this as a blocker because it's not actually
> a bug or anything. Prior to this patch we also ignore whatever this
> field was set to.

Prior to this patch this field was never read as this is only needed in
.get_state. And in .apply the value is correctly written.

> > > + }
> > > +
> > > + prescaler = MX3_PWMCR_PRESCALER_GET(val);
> > > + pwm_clk = clk_get_rate(imx->clk_per);
> > > + pwm_clk = DIV_ROUND_CLOSEST_ULL(pwm_clk, prescaler);
> > > + val = readl(imx->mmio_base + MX3_PWMPR);
> > 
> > It would be more cautionous to not rely on the reserved bits to read as
> > zero. So I suggest to mask the value with 0xffff.
> 
> If that's what the documentation says I think it's okay to rely on it.
> But I can add the mask if we want to play it extra safe.

The PERIOD field only covers bits 0 to 15 and the upper 16 bits of this
register are reserved. The registers are documented to read as 0, still
I'd prefer to mask them out for maximal correctness.

> > > + period = val >= MX3_PWMPR_MAX ? MX3_PWMPR_MAX : val;
> > > +
> > > + /* PWMOUT (Hz) = PWMCLK / (PWMPR + 2) */
> > > + tmp = NSEC_PER_SEC * (u64)(period + 2);
> > > + state->period = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk);
> > 
> > Would it make sense to introduce a policy about how to round in this
> > case? (Similarily for .apply?) This is of course out of scope for this
> > patch.
> 
> I'm not sure what you means by policy. The above already does round to
> closest. Is that not an appropriate policy?

I mean to document the expectation of the pwm framework how the driver
is supposed to configure the hardware when it should program the PWM
with say duty_cycle=100ns/period=300ns and but the hardware can only
implement both duty_cycle and period in steps of 3ns. So the (IMHO) good
candidates are:

	duty_cycle = 99ns / period = 297ns
	duty_cycle = 99ns / period = 300ns
	duty_cycle = 102ns / period = 300ns
	duty_cycle = 102ns / period = 306ns

As it's not obvious which is the best here (or is it?) and taking into
account that the PWM user might care, it would be right to specify
something like:

  The driver is supposed to configure the biggest duty_cycle that
  is not greater than the requested duty_cycle. Similarily it should
  pick the biggest possible period that is not greater than the
  requested period.

To actually allow the user to find the setting that he or she prefers
something like the clk framework provides with clk_round_rate() would be
needed of course.

And for .get_state I'd specify something similar that ensures that in
the sequence

	myops->get_state(chip, pwm, state);
	myops->apply(chip, pwm, state);

the latter is a noop.

Best regards
Uwe

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