Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 4 Mar 2021 08:42:03 +0900
From:   William Breathitt Gray <vilhelm.gray@gmail.com>
To:     Fabrice Gasnier <fabrice.gasnier@foss.st.com>
Cc:     jic23@kernel.org, alexandre.torgue@foss.st.com,
        mcoquelin.stm32@gmail.com, olivier.moysan@foss.st.com,
        linux-iio@vger.kernel.org,
        linux-stm32@st-md-mailman.stormreply.com,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] counter: stm32-timer-cnt: fix ceiling miss-alignment
 with reload register
Message-ID: <YEAeyyJ+GH10ep7S@shinobu>
References: <1614793789-10346-1-git-send-email-fabrice.gasnier@foss.st.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="raztPjiYvdctBTkF"
Content-Disposition: inline
In-Reply-To: <1614793789-10346-1-git-send-email-fabrice.gasnier@foss.st.com>
Precedence: bulk


--raztPjiYvdctBTkF
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Mar 03, 2021 at 06:49:49PM +0100, Fabrice Gasnier wrote:
> Ceiling value may be miss-aligned with what's actually configured into the
> ARR register. This is seen after probe as currently the ARR value is zero,
> whereas ceiling value is set to the maximum. So:
> - reading ceiling reports zero
> - in case the counter gets enabled without any prior configuration,
>   it won't count.
> - in case the function gets set by the user 1st, (priv->ceiling) is used.
>=20
> Fix it by getting rid of the cached "priv->ceiling" variable. Rather use
> the ARR register value directly by using regmap read or write when needed.
> There should be no drawback on performance as priv->ceiling isn't used in
> performance critical path.
> There's also no point in writing ARR while setting function (sms), so
> it can be safely removed.
>=20
> Fixes: ad29937e206f ("counter: Add STM32 Timer quadrature encoder")
>=20
> Suggested-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

Acked-by: William Breathitt Gray <vilhelm.gray@gmail.com>

> ---
> Note: this applies on top of:
> - "counter: stm32-timer-cnt: fix ceiling write max value"

Note, if your patch requires prerequisite patches, please provide the
`git patch-id --stable` patch ID for it; this helps make sure the
patches are applied in the correct order. You can have `git
format-patch` generate this automatically for you by using the `--base`
option:
https://git-scm.com/docs/git-format-patch#_base_tree_information

William Breathitt Gray

> ---
>  drivers/counter/stm32-timer-cnt.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
>=20
> diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-ti=
mer-cnt.c
> index 2295be3..75bc401 100644
> --- a/drivers/counter/stm32-timer-cnt.c
> +++ b/drivers/counter/stm32-timer-cnt.c
> @@ -31,7 +31,6 @@ struct stm32_timer_cnt {
>  	struct counter_device counter;
>  	struct regmap *regmap;
>  	struct clk *clk;
> -	u32 ceiling;
>  	u32 max_arr;
>  	bool enabled;
>  	struct stm32_timer_regs bak;
> @@ -75,8 +74,10 @@ static int stm32_count_write(struct counter_device *co=
unter,
>  			     const unsigned long val)
>  {
>  	struct stm32_timer_cnt *const priv =3D counter->priv;
> +	u32 ceiling;
> =20
> -	if (val > priv->ceiling)
> +	regmap_read(priv->regmap, TIM_ARR, &ceiling);
> +	if (val > ceiling)
>  		return -EINVAL;
> =20
>  	return regmap_write(priv->regmap, TIM_CNT, val);
> @@ -138,10 +139,6 @@ static int stm32_count_function_set(struct counter_d=
evice *counter,
> =20
>  	regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN, 0);
> =20
> -	/* TIMx_ARR register shouldn't be buffered (ARPE=3D0) */
> -	regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_ARPE, 0);
> -	regmap_write(priv->regmap, TIM_ARR, priv->ceiling);
> -
>  	regmap_update_bits(priv->regmap, TIM_SMCR, TIM_SMCR_SMS, sms);
> =20
>  	/* Make sure that registers are updated */
> @@ -199,7 +196,6 @@ static ssize_t stm32_count_ceiling_write(struct count=
er_device *counter,
>  	regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_ARPE, 0);
>  	regmap_write(priv->regmap, TIM_ARR, ceiling);
> =20
> -	priv->ceiling =3D ceiling;
>  	return len;
>  }
> =20
> @@ -374,7 +370,6 @@ static int stm32_timer_cnt_probe(struct platform_devi=
ce *pdev)
> =20
>  	priv->regmap =3D ddata->regmap;
>  	priv->clk =3D ddata->clk;
> -	priv->ceiling =3D ddata->max_arr;
>  	priv->max_arr =3D ddata->max_arr;
> =20
>  	priv->counter.name =3D dev_name(dev);
> --=20
> 2.7.4
>=20

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

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

iQIzBAABCgAdFiEEk5I4PDJ2w1cDf/bghvpINdm7VJIFAmBAHsEACgkQhvpINdm7
VJI4JRAAg2Y+kgxmuZE/9tSHqFDyqpU5z9TnfxJDAHoWYVt8MfIys2u8zR3ZkbXj
ipa30R9WdJYB4btIdhNKKalhJGO47qYhPE7jw0f4WhRgyVkSIfuYRaXFHbQ7a4+P
OwqKTLDsrSDz6RL17JKkLb7Ks5ROrVHnl9OYAjhRupTY2WMAEza2gcPhcKOwsSo6
9eL6p0rB9/NMUhBi+DRLe/tkwxZH33nm0Re8wI27Ujx59R3rP3ZPzk0vgBpX1upd
JXRKbnsIjxj47waBbDAQx2DQp14NgHNEdPxNgJlzmQMB6bRdSw1xVhR93Z0w9rVK
TGLFiMe2RBvwHB0MuOm75dsOwvlSbg/eLiK3LUS7DDzg8GPmpkPOX387ONmj6EOw
QiI0JaMTLYodBXiD2gwQf0h8m1k3/tlXcnL8/4+8nMNt8TDvxuP1XdGajhEOhMOE
mNeb+SHFZ3D5NPh9/Ce3coLWJrNYcE+UwIhp/BGyfm+gQPuUbDG1lh3a7voQsa9I
jGgzo5tBFWSrUfLzGrV+kqTNMM987H9/ykMkydimcqFTWcj0z7Xy3XEEEarPOg5r
Cu0UvU4+CbIvccsXtLYWs6kjJQuaemL5eX7Oqh9qQunN07o5xbVFLCwH7+g/0AcH
BgC31bYF6EdEBeknuEtMugdRXnKvQYfJVfX8xmNdtDv2+dDVKWo=
=IqtO
-----END PGP SIGNATURE-----

--raztPjiYvdctBTkF--