Re: [PATCH v4 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support

[Uwe Kleine-König]

On Mon, Apr 12, 2021 at 11:55:36AM +0900, Nobuhiro Iwamatsu wrote:
> Hi Uwe,
> 
> Thanks for your review.
> 
> On Sat, Apr 10, 2021 at 03:53:21PM +0200, Uwe Kleine-König wrote:
> > Hello,
> > 
> > just a few small details left to criticize ...
> > 
> > On Sat, Apr 10, 2021 at 08:08:37AM +0900, Nobuhiro Iwamatsu wrote:
> > > diff --git a/drivers/pwm/pwm-visconti.c b/drivers/pwm/pwm-visconti.c
> > > new file mode 100644
> > > index 000000000000..99d83f94ed86
> > > --- /dev/null
> > > +++ b/drivers/pwm/pwm-visconti.c
> > > @@ -0,0 +1,188 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Toshiba Visconti pulse-width-modulation controller driver
> > > + *
> > > + * Copyright (c) 2020 TOSHIBA CORPORATION
> > > + * Copyright (c) 2020 Toshiba Electronic Devices & Storage Corporation
> > > + *
> > > + * Authors: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@xxxxxxxxxxxxx>
> > > + *
> > > + */
> > > +
> > > +#include <linux/err.h>
> > > +#include <linux/io.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/pwm.h>
> > > +
> > > +#define PIPGM_PCSR(ch) (0x400 + 4 * (ch))
> > > +#define PIPGM_PDUT(ch) (0x420 + 4 * (ch))
> > > +#define PIPGM_PWMC(ch) (0x440 + 4 * (ch))
> > > +
> > > +#define PIPGM_PWMC_PWMACT		BIT(5)
> > > +#define PIPGM_PWMC_CLK_MASK		GENMASK(1, 0)
> > > +#define PIPGM_PWMC_POLARITY_MASK	GENMASK(5, 5)
> > > +
> > > +struct visconti_pwm_chip {
> > > +	struct pwm_chip chip;
> > > +	void __iomem *base;
> > > +};
> > > +
> > > +static inline struct visconti_pwm_chip *to_visconti_chip(struct pwm_chip *chip)
> > > +{
> > > +	return container_of(chip, struct visconti_pwm_chip, chip);
> > > +}
> > > +
> > > +static int visconti_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > +			      const struct pwm_state *state)
> > > +{
> > > +	struct visconti_pwm_chip *priv = to_visconti_chip(chip);
> > > +	u32 period, duty_cycle, pwmc0;
> > > +
> > > +	/*
> > > +	 * pwmc is a 2-bit divider for the input clock running at 1 MHz.
> > > +	 * When the settings of the PWM are modified, the new values are shadowed in hardware until
> > > +	 * the period register (PCSR) is written and the currently running period is completed. This
> > > +	 * way the hardware switches atomically from the old setting to the new.
> > > +	 * Also, disabling the hardware completes the currently running period and keeps the output
> > > +	 * at low level at all times.
> > 
> > Can you please put a paragraph analogous to the one in pwm-sifive in the
> > same format. This simplified keeping an overview about the oddities of
> > the various supported chips.
> 
> OK, I will check pwm-sifive's, and add.
> 
> > 
> > > +	 */
> > > +	if (!state->enabled) {
> > > +		writel(0, priv->base + PIPGM_PCSR(pwm->hwpwm));
> > > +		return 0;
> > > +	}
> > > +
> > > [...]
> > > +
> > > +static void visconti_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > > +				   struct pwm_state *state)
> > > +{
> > > +	struct visconti_pwm_chip *priv = chip_to_priv(chip);
> > > +	u32 period, duty, pwmc0, pwmc0_clk;
> > > +
> > > +	period = readl(priv->base + PIPGM_PCSR(pwm->hwpwm));
> > > +	if (period)
> > > +		state->enabled = true;
> > > +	else
> > > +		state->enabled = false;
> > 
> > If PIPGM_PCSR is 0 the hardware is still active and setting a new
> > configuration completes the currently running period, right? Then I
> > think having always state->enabled = true is a better match.
>
> If PIPGM_PCSR is 0, the hardware is stopped. Even if the settings of
> other registers are written, the values of other registers will not work
> unless PIPGM_PCSR is written.

Correct me if I'm wrong, but how I understand it, PCSR is special as the
other registers are shadow until PCSR is written. (And that is
irrespective of which value is active in PCSR or what is written to
PCSR.)
 
> However, as a logic, if PIPGM_PCSR becomes 0, it is only
> visconti_pwm_apply () in this driver,
> so I think that this process should always be state-> enabled = true
> as you pointed out.
> I wil drop this, thanks.

For me the critical (and only) difference between "off" and
"duty cycle = 0" is that when a new configuration is to be applied. In
the "off" state a new period can (and should) start immediately, while
with "duty_cycle = 0" the rising edge should be delayed until the
currently running period is over.[1]

So the thing to do here (IMHO) is:

Iff with PIPGM_PCSR = 0 configuring a new setting (that is finalized
with writing a non-zero value to PIPGM_PCSR) completes the currently
running period, then always assume the PWM as enabled.

And so if the hardware is stopped and the counter is reset when 0 is
written to PIPGM_PCSR, model that as enabled = false.

Best regards
Uwe

[1] In practise this is more difficult because several PWMs don't
complete periods in general. But the hardware under discussion luckily
isn't one of these. And (worse) there are other hardware implementations
where off doesn't emit an inactive level.

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

Attachment: signature.asc
Description: PGP signature