Message-ID: <Y73/oUwuOwQFR0NZ@spud>
Date:   Wed, 11 Jan 2023 00:15:29 +0000
From:   Conor Dooley <conor@...nel.org>
To:     Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
Cc:     Thierry Reding <thierry.reding@...il.com>,
        Conor Dooley <conor.dooley@...rochip.com>,
        Daire McNamara <daire.mcnamara@...rochip.com>,
        linux-kernel@...r.kernel.org, linux-pwm@...r.kernel.org,
        linux-riscv@...ts.infradead.org
Subject: Re: [PATCH v13 1/2] pwm: add microchip soft ip corePWM driver

On Tue, Jan 10, 2023 at 11:48:05PM +0100, Uwe Kleine-König wrote:
> On Wed, Dec 21, 2022 at 11:29:12AM +0000, Conor Dooley wrote:
> > From: Conor Dooley <conor.dooley@...rochip.com>

> > +		delay_us = DIV_ROUND_UP_ULL(remaining_ns, NSEC_PER_USEC);
> > +		if ((delay_us / 1000) > MAX_UDELAY_MS)
> > +			msleep(delay_us / 1000 + 1);
> 
> Is this better than
> 
> 	msleep(DIV_ROUND_UP(delay_us, 1000);
> 
> ? Also I wonder about your usage of MAX_UDELAY_MS. This is about

I probably started hacking on the example you gave and didn't notice
the U. What I have here is ~what you suggested last time.

> udelay() but you're using usleep_range()?
> 
> > +		else
> > +			usleep_range(delay_us, delay_us * 2);
> 
> I wonder if there isn't a function that implements something like
> 
> 	wait_until(mchp_core_pwm->update_timestamp);
> 
> which would be a bit nicer than doing this by hand. Maybe fsleep()?

That'd be fsleep(delay_us), but does at least clean up some of the
messing.

> > +static void mchp_core_pwm_apply_duty(struct pwm_chip *chip, struct pwm_device *pwm,
> > +				     const struct pwm_state *state, u64 duty_steps,
> > +				     u8 period_steps)
> > +{
> > +	struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip);
> > +	u8 posedge, negedge;
> > +	u8 period_steps_val = PREG_TO_VAL(period_steps);
> > +
> > +	/*
> > +	 * Setting posedge == negedge doesn't yield a constant output,
> > +	 * so that's an unsuitable setting to model duty_steps = 0.
> > +	 * In that case set the unwanted edge to a value that never
> > +	 * triggers.
> > +	 */
> > +	if (state->polarity == PWM_POLARITY_INVERSED) {
> > +		negedge = !duty_steps ? period_steps_val : 0u;
> 
> IMHO
> 
> 		negedge = duty_steps ? 0 : period_steps_val;
> 
> is a bit easier to parse.
> 
> > +		posedge = duty_steps;
> > +	} else {
> > +		posedge = !duty_steps ? period_steps_val : 0u;
> > +		negedge = duty_steps;
> > +	}
> 
> The following code is equivalent:
> 
> 	u8 first_edge = 0, second_edge = duty_steps;
> 
> 	/*
> 	 * Setting posedge == negedge doesn't yield a constant output,
> 	 * so that's an unsuitable setting to model duty_steps = 0.
> 	 * In that case set the unwanted edge to a value that never
> 	 * triggers.
> 	 */
> 	if (duty_steps == 0)
> 		first_edge = period_steps_val;
> 
> 	if (state->polarity == PWM_POLARITY_INVERSED) {
> 		negedge = first_edge;
> 		posedge = second_edge;
> 	} else {
> 		posedge = first_edge;
> 		negedge = second_edge;
> 	}
> 
> I'm not sure if it's easier to understand. What do you think?

Despite having used them, I dislike ternary statements.

> > +	writel_relaxed(posedge, mchp_core_pwm->base + MCHPCOREPWM_POSEDGE(pwm->hwpwm));
> > +	writel_relaxed(negedge, mchp_core_pwm->base + MCHPCOREPWM_NEGEDGE(pwm->hwpwm));
> > +}
> > +
> > +static void mchp_core_pwm_calc_period(const struct pwm_state *state, unsigned long clk_rate,
> > +				      u16 *prescale, u8 *period_steps)
> > +{
> > +	u64 tmp;
> > +
> > +	/*
> > +	 * Calculate the period cycles and prescale values.
> > +	 * The registers are each 8 bits wide & multiplied to compute the period
> > +	 * using the formula:
> > +	 * (clock_period) * (prescale + 1) * (period_steps + 1)
> > +	 * so the maximum period that can be generated is 0x10000 times the
> > +	 * period of the input clock.
> > +	 * However, due to the design of the "hardware", it is not possible to
> > +	 * attain a 100% duty cycle if the full range of period_steps is used.
> > +	 * Therefore period_steps is restricted to 0xFE and the maximum multiple
> > +	 * of the clock period attainable is 0xFF00.
> > +	 */
> > +	tmp = mul_u64_u64_div_u64(state->period, clk_rate, NSEC_PER_SEC);
> > +
> > +	/*
> > +	 * The hardware adds one to the register value, so decrement by one to
> > +	 * account for the offset
> > +	 */
> > +	if (tmp >= MCHPCOREPWM_PERIOD_MAX) {
> > +		*prescale = MCHPCOREPWM_PRESCALE_MAX - 1;
> > +		*period_steps = MCHPCOREPWM_PERIOD_STEPS_MAX - 1;
> > +
> > +		return;
> > +	}
> > +
> > +	*prescale = div_u64(tmp, MCHPCOREPWM_PERIOD_STEPS_MAX);
> > +	/* PREG_TO_VAL() can produce a value larger than UINT8_MAX */
> > +	*period_steps = div_u64(tmp, PREG_TO_VAL(*prescale)) - 1;
> 
> This looks wrong, but I didn't think long about that. Did we discuss
> this already and/or are you sure this is correct?

We did discuss it previously AFAICT;
https://lore.kernel.org/linux-pwm/896d73ac-05af-8673-8379-29011800be83@microchip.com/

In that version of the code, prescale_val meant the mathematical value
used for calculations & "prescale" was the value written into the
register.

Now, we have ditched prescale_val and operate directly with what gets
written into the register.

I ran a test case through the calculation, and it seemed to work out?

> (We have:
> 	          (prescale + 1) * (period_steps + 1)
> 	period = ------------------------------------
> 	                       clk_rate
> 
> You calculate
> 	            period * clk_rate
> 	prescale = -------------------
> 	           NSEC_PER_SEC * 0xff

Say period = 2000 ns, clk_rate = 62.5 Mhz, giving a register value for
prescale of 0.49019... 

> 	                     period * clk_rate
> 	period_steps = ----------------------------- - 1
> 	               NSEC_PER_SEC * (prescale + 1)

Same numbers, but we use the PREG_TO_VAL() macro so the mathematical value
is 1.49019.

     2000 * 62.5E6
--------------------- - 1 = 82.88360....
  1E9 * (0.49016 + 1)

> 
> assuming exact arithmetic putting these into the above equation we get:
> 
> 
>     period * clk_rate                period * clk_rate
>   (------------------- + 1) * (-----------------------------) / clk_rate
>    NSEC_PER_SEC * 0xff         NSEC_PER_SEC * (prescale + 1)
> 
> and then substituting prescale this doesn't resolve to period, does it?
> Correct me if I'm wrong.)

(0.49016 + 1) * (82.88360 + 1)      124.99...
------------------------------ = ------------- = 0.00000199999
            62.5E6                  62.5E6

And then accounting for that fact that 2000 was really 2000E-9,
we arrive back where we started, give or take some rounding?

Doing that with integer maths works out more cleanly since 0.49016
becomes 0.
   2000 * 62.5E6
------------------ - 1 = 124
  1E9 * (0 + 1)

(0 + 1) * (124 + 1)      125
------------------- = --------- = 0.000002
      62.5E6            62.5E6

Unfortunately, I don't think I am seeing what you're seeing.

>     period * clk_rate                period * clk_rate
>   (------------------- + 1) * (-----------------------------) / clk_rate
>    NSEC_PER_SEC * 0xff         NSEC_PER_SEC * (prescale + 1)
                          ^
It may be this + 1, which I don't seem to have accounted for in my quick
run through a calculation?

*prescale = div_u64(tmp, MCHPCOREPWM_PERIOD_STEPS_MAX);

*period_steps = div_u64(tmp, PREG_TO_VAL(*prescale)) - 1;

The code does not add a 1 when it calculates prescale, only when it uses
the result to calculate period_steps, since prescale & period_steps are
the register values, not the "mathematical" ones.

Hopefully I've not gone and made a fool of myself...

> > +static inline void mchp_core_pwm_apply_period(struct mchp_core_pwm_chip *mchp_core_pwm,
> > +					      u8 prescale, u8 period_steps)
> > +{
> > +	writel_relaxed(prescale, mchp_core_pwm->base + MCHPCOREPWM_PRESCALE);
> > +	writel_relaxed(period_steps, mchp_core_pwm->base + MCHPCOREPWM_PERIOD);
> > +}
> 
> There is only one caller for this two-line function. I suggest to unroll it?

Sure.

> > +	ret = devm_pwmchip_add(&pdev->dev, &mchp_core_pwm->chip);
> > +	if (ret < 0)
> > +		return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
> > +
> > +	/*
> > +	 * Enabled synchronous update for channels with shadow registers
> > +	 * enabled. For channels without shadow registers, this has no effect
> > +	 * at all so is unconditionally enabled.
> > +	 */
> > +	writel_relaxed(1U, mchp_core_pwm->base + MCHPCOREPWM_SYNC_UPD);
> > +	mchp_core_pwm->update_timestamp = ktime_get();
> 
> This needs to be done before devm_pwmchip_add().

Makes sense, woops. I think I've revised this to the point that my
blinkers have turned on & I'll wait a while before resubmitting in order
to hopefully reset that.

Perhaps I need to watch a lecture on how to write a PWM driver since I
am clearly no good at it, given the 15 revisions. Do you know of any?

Thanks,
Conor.


Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists