Re: [PATCH v5 3/3] pwm: Add support for Xilinx AXI Timer

[Sean Anderson]

On 8/17/21 2:04 PM, Uwe Kleine-König wrote:
> On Mon, Aug 16, 2021 at 07:51:17PM -0400, Sean Anderson wrote:
> > On 8/14/21 4:47 PM, Uwe Kleine-König wrote:
> > > Hello Sean,
> > >
> > > sorry for having you let waiting so long. Now here some more feedback:
> > >
> > > On Mon, Jul 19, 2021 at 06:13:22PM -0400, Sean Anderson wrote:
> > > > +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused,
> > > > +			    const struct pwm_state *state)
> > > > +{
> > > > +	bool enabled;
> > > > +	struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
> > > > +	u32 tlr0, tlr1, tcsr0, tcsr1;
> > > > +	u64 period_cycles, duty_cycles;
> > > > +	unsigned long rate;
> > > > +
> > > > +	if (state->polarity != PWM_POLARITY_NORMAL)
> > > > +		return -EINVAL;
> > > > +
> > > > +	/*
> > > > +	 * To be representable by TLR, cycles must be between 2 and
> > > > +	 * priv->max + 2. To enforce this we can reduce the duty
> > > > +	 * cycle, but we may not increase it.
> > > > +	 */
> > > > +	rate = clk_get_rate(priv->clk);
> > > > +	period_cycles = mul_u64_u32_div(state->period, rate, NSEC_PER_SEC);
> > >
> > > cool, I didn't know mul_u64_u32_div.
> >
> > I didn't either. Alas, many useful functions like these have no
> > documentation...
> >
> > >
> > > Hmm, we still have a problem here if
> > >
> > > 	state->period * rate > 1000000000 * U64_MAX.
> >
> > Note that this can only occur with rate > 1GHz (and period = U64_MAX).
> > The highest fmax in the datasheet is 300 MHz (on a very expensive FPGA).
> >
> > Maybe it is more prudent to do
> >
> > 	period = min(state->period, ULONG_MAX * NSEC_PER_SEC)
>
> Together with a check for rate being <= 300 MHz to be safe that's fine.

That's what the ULONG_MAX is for; whatever we get back from
clk_get_rate, it has to fit in a ulong.

> > I think a period of 136 years is adequate :) This comparison also has
> > the advantage of being against const values.
>
> *nod*
>
> > > > +static void xilinx_pwm_get_state(struct pwm_chip *chip,
> > > > +				 struct pwm_device *unused,
> > > > +				 struct pwm_state *state)
> > > > +{
> > > > +	struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
> > > > +	u32 tlr0, tlr1, tcsr0, tcsr1;
> > > > +
> > > > +	regmap_read(priv->map, TLR0, &tlr0);
> > > > +	regmap_read(priv->map, TLR1, &tlr1);
> > > > +	regmap_read(priv->map, TCSR0, &tcsr0);
> > > > +	regmap_read(priv->map, TCSR1, &tcsr1);
> > > > +	state->period = xilinx_timer_get_period(priv, tlr0, tcsr0);
> > >
> > > xilinx_timer_get_period rounds down, this is however wrong for
> > > .get_state().
> >
> > Why is this wrong? I thought get_state should return values which would
> > not be rounded if passed to apply_state.
>
> Consider a PWM that yields a period of π * $regval ns when a certain
> register is programmed with the value $regval.
>
> Consider the HW is programmed with regval = 317. The exact period is
> 995.8848711879644. If now .get_state() rounds down and returns 995 ns and
> you feed that value back into .apply the new regval (assuming round down
> in .apply(), too) this yields regval = 316. If however .get_state()
> rounds up and returns 996, putting this value back into .apply() you get
> the desired 317.

Will fix for v6, but please document this somewhere :)

--Sean