Message-ID: <48c7dbdc-04a2-42de-964f-fd86cf070797@spud>
Date:   Wed, 22 Mar 2023 22:49:40 +0000
From:   Conor Dooley <conor@...nel.org>
To:     Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
Cc:     Conor Dooley <conor.dooley@...rochip.com>,
        Thierry Reding <thierry.reding@...il.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 v14 1/2] pwm: add microchip soft ip corePWM driver

On Wed, Mar 22, 2023 at 11:55:36AM +0100, Uwe Kleine-König wrote:
> On Mon, Mar 06, 2023 at 09:48:58AM +0000, Conor Dooley wrote:
> > Add a driver that supports the Microchip FPGA "soft" PWM IP core.

> > +static void mchp_core_pwm_calc_period(const struct pwm_state *state, unsigned long clk_rate,
> > +				      u16 *prescale, u16 *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:
> > +	 *           (prescale + 1) * (period_steps + 1)
> > +	 * period = -------------------------------------
> > +	 *                      clk_rate
> > +	 * 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;
> > +	}
> > +
> > +	/*
> > +	 * The optimal value for prescale can be calculated using the maximum
> > +	 * permitted value of period_steps, 0xff.
> 
> I had to think about that one for a while. The maximal value for
> (period_steps + 1) is 0xff with the reasoning above?! That's also what
> the code uses.

I've become really disenfranchised with these register/variable names.
I feel like just changing them to disconnect the variables used for
calculation from the register names a little, so that the "is there a +1
needed here or not" stuff becomes a little clearer.
It always makes sense to be when I am in an "I respun the patch today"
mode, but by the time we're in the review stage I get muddled.
God forbid I have to look at this in 10 years time.

That said, there is a bit of a mistake here. The comment two above says
"Therefore period_steps is restricted to 0xFE" when I'm capping things
off. Some inaccuracies have probably snuck in during the various
respins, and I think the comment above is "correct" but misleading, as
it muddies the waters about variable versus register names.

> Also as the comment is written here, it's wrong (or misleading)
> depending on the semantic of "optimal". If you want to achive
> 
> 	(prescale + 1) * (period_steps + 1) <= 64009
> 
> you should pick prescale == period_steps == 252 to get optimally near
> 64009.
> However the idea is to pick a set of values with period_steps being big
> to allow a finegrained selection for the duty cycle, right?

Correct. I'll update the comments with an explanation as to what the
objective is, rather than just referring to it as "optimal".

> Consider
> 
> 	clk_rate = 1000000
> 	period = 64009000
> 
> then your code gives:
> 
>               period * clk_rate
> 	tmp = ----------------- = 64009
>                 NSEC_PER_SEC
> 
> and so *prescale = 251 and *period_steps = 253. 
> 
> Wasn't the intention to pick *prescale = 250 and then
> *period_steps = 255?
> 
> Depending on your semantics of "optimal", either (252, 252) or (250,
> 255) is better than (251, 253). I think that means you shouldn't ignore
> the -1?
> 
> One thing I think is strange is that with clk_rate = 1000001 and your
> algorithm we get:
> 
> requested period = 1274998 ns -> real period = 1269998.73000127  (prescale = 4, period_steps = 253)
> requested period = 1274999 ns -> real period = 1271998.728001272 (prescale = 5, period_steps = 211)
> 
> while 1271998.728001272 would be a better match for a request with
> period = 1274998 than 1269998.73000127.
> 
> I spend too much time to think about that now. I'm unsure if this is
> because the -1 is missing, or if there is a bug in the idea to pick a
> small prescale to allow a big period_steps value (in combination with
> the request to pick the biggest possible period).
> 
> Hmm, maybe you understand that better than me? I'll have to think about
> it.

I'll have to think about it too, I'll clear a space among the todo-lists
on my whiteboard tomorrow or Friday and get back to you...

> 
> > +	 *
> > +	 *             period * clk_rate
> > +	 * prescale = ------------------- - 1
> > +	 *            NSEC_PER_SEC * 0xff
> > +	 *
> > +	 * However, we are purely interested in the integer upper bound of this
> > +	 * calculation, so ignore the subtraction & rely on the truncation done
> > +	 * by the division.
> > +	 *
> > +	 *  period * clk_rate
> > +	 * ------------------- was precomputed as `tmp`
> > +	 *    NSEC_PER_SEC
> > +	 *
> > +	 * period_steps is then computed using the result:
> > +	 *                      period * clk_rate
> > +	 * period_steps = ----------------------------- - 1
> > +	 *                NSEC_PER_SEC * (prescale + 1)
> > +	 */
> > +	*prescale = div_u64(tmp, MCHPCOREPWM_PERIOD_STEPS_MAX);
> > +	*period_steps = div_u64(tmp, PREG_TO_VAL(*prescale)) - 1;
> > +}
> > +
> > [..]
> > +static int mchp_core_pwm_probe(struct platform_device *pdev)
> > +{
> > +	struct mchp_core_pwm_chip *mchp_core_pwm;
> > +	struct resource *regs;
> > +
> > +	mchp_core_pwm = devm_kzalloc(&pdev->dev, sizeof(*mchp_core_pwm), GFP_KERNEL);
> > +	if (!mchp_core_pwm)
> > +		return -ENOMEM;
> > +
> > +	mchp_core_pwm->base = devm_platform_get_and_ioremap_resource(pdev, 0, &regs);
> > +	if (IS_ERR(mchp_core_pwm->base))
> > +		return PTR_ERR(mchp_core_pwm->base);
> > +
> > +	mchp_core_pwm->clk = devm_clk_get_enabled(&pdev->dev, NULL);
> > +	if (IS_ERR(mchp_core_pwm->clk))
> > +		return dev_err_probe(&pdev->dev, PTR_ERR(mchp_core_pwm->clk),
> > +				     "failed to get PWM clock\n");
> > +
> > +	if (of_property_read_u32(pdev->dev.of_node, "microchip,sync-update-mask",
> > +				 &mchp_core_pwm->sync_update_mask))
> > +		mchp_core_pwm->sync_update_mask = 0;
> > +
> > +	mutex_init(&mchp_core_pwm->lock);
> > +
> > +	mchp_core_pwm->chip.dev = &pdev->dev;
> > +	mchp_core_pwm->chip.ops = &mchp_core_pwm_ops;
> > +	mchp_core_pwm->chip.npwm = 16;
> > +
> > +	mchp_core_pwm->channel_enabled = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_EN(0));
> > +	mchp_core_pwm->channel_enabled |=
> > +		readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_EN(1)) << 8;
> > +
> > +	writel_relaxed(1U, mchp_core_pwm->base + MCHPCOREPWM_SYNC_UPD);
> 
> This one is just for the case where there is an unapplied configuration
> in the registers, right?

No, this is me realising that I had a misconception about how that
register works. You write the bit once, and it enables the mode for
channels that have been configured that way at synthesis-time, rather
than how I somehow thought it worked which was as a "flush" from the
shadow registers into the "real" ones.

> 
> > +	mchp_core_pwm->update_timestamp = ktime_get();
> > +
> > +	return devm_pwmchip_add(&pdev->dev, &mchp_core_pwm->chip);
> 
> An error message if devm_pwmchip_add() fails would be nice.

Sure, can do!

Thanks,
Conor.

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

Powered by blists - more mailing lists