Re: [PATCH v3 3/5] nvmem: imx-ocotp: Ensure the RELAX field is non-zero

From: Leonard Crestez <leonard.crestez@nxp.com>
To: Bryan O'Donoghue <pure.logic@nexus-software.ie>,
	"l.stach@pengutronix.de" <l.stach@pengutronix.de>,
	Peng Fan <peng.fan@nxp.com>
Cc: Aisheng Dong <aisheng.dong@nxp.com>,
	Abel Vesa <abel.vesa@nxp.com>, Anson Huang <anson.huang@nxp.com>,
	"srinivas.kandagatla@linaro.org" <srinivas.kandagatla@linaro.org>,
	dl-linux-imx <linux-imx@nxp.com>,
	"kernel@pengutronix.de" <kernel@pengutronix.de>,
	Fabio Estevam <fabio.estevam@nxp.com>,
	"shawnguo@kernel.org" <shawnguo@kernel.org>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v3 3/5] nvmem: imx-ocotp: Ensure the RELAX field is non-zero
Date: Mon, 22 Apr 2019 10:28:03 +0000	[thread overview]
Message-ID: <VI1PR04MB553397CF98ACF2D8BA089452EE220@VI1PR04MB5533.eurprd04.prod.outlook.com> (raw)
In-Reply-To: 20190419171927.24269-4-pure.logic@nexus-software.ie

On 4/19/2019 8:19 PM, Bryan O'Donoghue wrote:
> The RELAX field of the OCOTP block quote "specifies the time to add to all
> default timing parameters other than the Tpgm and Trd. It is given in
> number of ipg_clk periods".
> 
> On the i.MX8MM the calculation for the RELAX value is turning out to be
> zero which is not a problem for programming OTP values but, does
> subsequently mess up reloading the OTP shadow registers.
> 
> This patch ensures the RELAX field is at least one ipg_clk cycle, which
> seems like a pretty obvious floor to place on a value such as this.
> 
> Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>
> ---
>   drivers/nvmem/imx-ocotp.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/nvmem/imx-ocotp.c b/drivers/nvmem/imx-ocotp.c
> index 85a7d0da3abb..5b625d61e433 100644
> --- a/drivers/nvmem/imx-ocotp.c
> +++ b/drivers/nvmem/imx-ocotp.c
> @@ -186,6 +186,8 @@ static void imx_ocotp_set_imx6_timing(struct ocotp_priv *priv)
>   	clk_rate = clk_get_rate(priv->clk);
>   
>   	relax = clk_rate / (1000000000 / DEF_RELAX) - 1;
> +	if (!relax)
> +		relax = 1;

Math here hurts my head. Why is there a -1 above?

How about "relax = DIV_ROUND_UP(clk_rate, 1000000000 / DEF_RELAX);"? 
Rounding up seems safe and it should guarantee that the result is >= 1. 
As far as I understand a value which is slightly larger here shouldn't hurt.

Looking at uboot there DEF_RELAX is moved to the other side but that 
risks overflow at very high ipg rates.

Other chips have similar 66Mhz ipg rates; my guess is that this bug 
affects other chips as well.

--
Regards,
Leonard

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel