Messages in this thread | | | From | Sean Anderson <> | Subject | Re: [PATCH v5 3/3] pwm: Add support for Xilinx AXI Timer | Date | Mon, 16 Aug 2021 19:51:17 -0400 |
| |
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)
I think a period of 136 years is adequate :) This comparison also has the advantage of being against const values.
> So to be entirely save, we either need: > > /* > * To ensure that period * rate / NSEC_PER_SEC fits into an u64 > * we need: > * U64_MAX * NSEC_PER_SEC > * period < ---------------------- > * rate > * > * . If rate is not bigger than NSEC_PER_SEC this is true for > * sure as the RHS is bigger than U64_MAX. Otherwise we can > * calculate the RHS using mul_u64_u32_div. > */ > if (rate > NSEC_PER_SEC) > period = min(state->period, mul_u64_u32_div(U64_MAX, NSEC_PER_SEC, rate); > else > period = state->period; > > or we go a step further and check the priv->max limit in the same step: > > period = min(state->period, ((u64)priv->max + 2) * NSEC_PER_SEC / rate) > > . The latter is simpler and it's safe as priv->max is an u32 and so > there is no overflow. > >> + if (period_cycles - 2 > priv->max || period_cycles < 2) > > I'd check for period_cycles < 2 first, because otherwise period_cycles - > 2 might underflow. Nothing bad happens in this case, but reading from > left to right my first thought was I found a bug. Also please decrease > period_cycles if it's bigger than priv->max + 2. (With the suggestion > above you don't need to check for period_cycles - 2 > priv->max any more > however.)
Ok, will swap.
>> + return -ERANGE; >> + duty_cycles = mul_u64_u32_div(state->duty_cycle, rate, NSEC_PER_SEC); >> + >> + /* >> + * If we specify 100% duty cycle, we will get 0% instead, so decrease >> + * the duty cycle count by one. >> + */ >> + if (period_cycles == duty_cycles) >> + duty_cycles--; >> + >> + /* Round down to 0% duty cycle for unrepresentable duty cycles */ >> + if (duty_cycles < 2) >> + duty_cycles = period_cycles; >> + >> + regmap_read(priv->map, TCSR0, &tcsr0); >> + regmap_read(priv->map, TCSR1, &tcsr1); >> + tlr0 = xilinx_timer_tlr_cycles(priv, tcsr0, period_cycles); >> + tlr1 = xilinx_timer_tlr_cycles(priv, tcsr1, duty_cycles); >> + regmap_write(priv->map, TLR0, tlr0); >> + regmap_write(priv->map, TLR1, tlr1); >> + >> + enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1); >> + if (state->enabled) { >> + /* >> + * If the PWM is already running, then the counters will be >> + * reloaded at the end of the current cycle. >> + */ > > If state->enabled is false, $enabled isn't used, so you can move the > assignment into the if body and also limit the scope of $enabled.
Ok.
>> + if (!enabled) { >> + /* Load TLR into TCR */ >> + regmap_write(priv->map, TCSR0, tcsr0 | TCSR_LOAD); >> + regmap_write(priv->map, TCSR1, tcsr1 | TCSR_LOAD); >> + /* Enable timers all at once with ENALL */ >> + tcsr0 = (TCSR_PWM_SET & ~TCSR_ENT) | (tcsr0 & TCSR_UDT); >> + tcsr1 = TCSR_PWM_SET | TCSR_ENALL | (tcsr1 & TCSR_UDT); >> + regmap_write(priv->map, TCSR0, tcsr0); >> + regmap_write(priv->map, TCSR1, tcsr1); >> + } >> + } else { >> + regmap_write(priv->map, TCSR0, 0); >> + regmap_write(priv->map, TCSR1, 0); >> + } >> + >> + return 0; >> +} >> + >> +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.
>> + state->duty_cycle = xilinx_timer_get_period(priv, tlr1, tcsr1); > > ditto for duty_cycle. > >> + state->enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1); >> + state->polarity = PWM_POLARITY_NORMAL; >> [...] >> +static int xilinx_timer_probe(struct platform_device *pdev) >> +{ >> + int ret; >> + struct device *dev = &pdev->dev; >> + struct device_node *np = dev->of_node; >> + struct xilinx_timer_priv *priv; >> + struct xilinx_pwm_device *pwm; >> + u32 pwm_cells, one_timer; >> + void __iomem *regs; >> + >> + ret = of_property_read_u32(np, "#pwm-cells", &pwm_cells); >> + if (ret == -EINVAL) >> + return -ENODEV; >> + else if (ret) >> + return dev_err_probe(dev, ret, "could not read #pwm-cells\n"); > > Please capitalize error messages.
Ok.
>> [...] >> + if (ret) { >> + clk_rate_exclusive_put(priv->clk); >> + clk_disable_unprepare(priv->clk); >> + return dev_err_probe(dev, ret, "could not register pwm chip\n"); > > s/pwm/PWM/
Ok.
Thanks for the review.
--Sean
| |