Messages in this thread | | | Date | Tue, 17 Aug 2021 20:04:07 +0200 | From | Uwe Kleine-König <> | Subject | Re: [PATCH v5 3/3] pwm: Add support for Xilinx AXI Timer |
| |
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.
> 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.
Best regards Uwe
-- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [unhandled content-type:application/pgp-signature] | |