Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 12 Jan 2023 15:10:09 +0000
From:   Conor Dooley <conor.dooley@microchip.com>
To:     Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= 
        <u.kleine-koenig@pengutronix.de>
CC:     Conor Dooley <conor@kernel.org>,
        Thierry Reding <thierry.reding@gmail.com>,
        Daire McNamara <daire.mcnamara@microchip.com>,
        <linux-kernel@vger.kernel.org>, <linux-pwm@vger.kernel.org>,
        <linux-riscv@lists.infradead.org>
Subject: Re: [PATCH v13 1/2] pwm: add microchip soft ip corePWM driver
Message-ID: <Y8Ai0aF5A+O43kjZ@wendy>
References: <20221221112912.147210-1-conor@kernel.org>
 <20221221112912.147210-2-conor@kernel.org>
 <20230110224805.3pqxd3yv4wyci2zj@pengutronix.de>
 <20230112120141.fpjwhtjuaxrwqt5m@pengutronix.de>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="fVFk6pos5f7gI6TJ"
Content-Disposition: inline
In-Reply-To: <20230112120141.fpjwhtjuaxrwqt5m@pengutronix.de>
Precedence: bulk

--fVFk6pos5f7gI6TJ
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Thu, Jan 12, 2023 at 01:01:41PM +0100, Uwe Kleine-K=F6nig wrote:
> Hello,
>=20
> On Tue, Jan 10, 2023 at 11:48:05PM +0100, Uwe Kleine-K=F6nig wrote:
> > Hello Conor,
> >=20
> > On Wed, Dec 21, 2022 at 11:29:12AM +0000, Conor Dooley wrote:
> > > +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 p=
eriod
> > > +	 * 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 us=
ed.
> > > +	 * Therefore period_steps is restricted to 0xFE and the maximum mul=
tiple
> > > +	 * of the clock period attainable is 0xFF00.
> > > +	 */
> > > +	tmp =3D 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 >=3D MCHPCOREPWM_PERIOD_MAX) {
> > > +		*prescale =3D MCHPCOREPWM_PRESCALE_MAX - 1;
> > > +		*period_steps =3D MCHPCOREPWM_PERIOD_STEPS_MAX - 1;
> > > +
> > > +		return;
> > > +	}
> > > +
> > > +	*prescale =3D div_u64(tmp, MCHPCOREPWM_PERIOD_STEPS_MAX);
> > > +	/* PREG_TO_VAL() can produce a value larger than UINT8_MAX */
> > > +	*period_steps =3D div_u64(tmp, PREG_TO_VAL(*prescale)) - 1;
> >=20
> > This looks wrong, but I didn't think long about that. Did we discuss
> > this already and/or are you sure this is correct?
> >=20
> > (We have:
> > 	          (prescale + 1) * (period_steps + 1)
> > 	period =3D ------------------------------------
> > 	                       clk_rate
> >=20
>=20
> We want prescale small such that period_steps can be big to give
> fine-grained control over the available duty_cycles. period_steps is a
> 8-bit value < 0xff, so we get:
>=20
>                     period * clk_rate  =20
> 	prescale =3D ------------------- - 1
>                    NSEC_PER_SEC * 0xff
>=20
> which in code then reads:
>=20
> 	*prescale =3D div_u64(tmp, MCHPCOREPWM_PERIOD_STEPS_MAX)
> 	if (*prescale)
> 		*prescale -=3D 1;
>=20
>=20
> > You calculate
> > 	            period * clk_rate
> > 	prescale =3D -------------------
> > 	           NSEC_PER_SEC * 0xff
> >=20
> > 	                     period * clk_rate
> > 	period_steps =3D ----------------------------- - 1
> > 	               NSEC_PER_SEC * (prescale + 1)
>=20
> The formula for period_steps is right.

I stood in front of the whiteboard for a bit to puzzle it all out and
have come to the realisation that you are right. I was implicitly
converting in my head from "mathematical" values to register values &
therefore not subtracting. I must've hyperfocused on the underflow when
I adjusted your suggestion back in v5 or w/e it was.

I must also have got rather unlucky that the configurations I picked to
check whether the calculations worked out, happened to. I suppose as
well, the way in which it was mis-calculating also avoids the PWM_DEBUG
complaints too.

Perhaps I'll insert your nice formulae in the next version in comments,
so they're there for next time.

Thanks again & sorry for consuming so much of your time on this,
Conor.


--fVFk6pos5f7gI6TJ
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQRh246EGq/8RLhDjO14tDGHoIJi0gUCY8Ai0QAKCRB4tDGHoIJi
0sQnAQDsop1eRPZcbZgK9DgYp3wSCMO28qbboB6YdQAdQMzbTQD/VPhbn4dHc/6C
yTsaXuUHVY4G3F0m8oPsFcTOlB0TeQ8=
=VQbM
-----END PGP SIGNATURE-----

--fVFk6pos5f7gI6TJ--