lkml.org 
[lkml]   [2021]   [Aug]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v5 3/3] pwm: Add support for Xilinx AXI Timer
Date


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

\
 
 \ /
  Last update: 2021-08-17 01:52    [W:0.809 / U:0.012 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site