Re: [PATCH v4 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>,
	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>,
	"l.stach@pengutronix.de" <l.stach@pengutronix.de>
Subject: Re: [PATCH v4 3/5] nvmem: imx-ocotp: Ensure the RELAX field is non-zero
Date: Tue, 23 Apr 2019 08:48:51 +0000	[thread overview]
Message-ID: <AM0PR04MB6434F2814AF55D476844AE40EE230@AM0PR04MB6434.eurprd04.prod.outlook.com> (raw)
In-Reply-To: 20190422150500.11082-4-pure.logic@nexus-software.ie

On 4/22/2019 6:05 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 drops the -1 component of the RELAX field calculation. Since
> RELAX specifies a number of cycles to do nothing, adding one extra cycle is
> safe. The DEF_RELAX components of the timing calculation are unaffected.
> 
> Fixes: 0642bac7da42 ("nvmem: imx-ocotp: add write support")
> 
> diff --git a/drivers/nvmem/imx-ocotp.c b/drivers/nvmem/imx-ocotp.c
> @@ -185,7 +185,7 @@ static void imx_ocotp_set_imx6_timing(struct ocotp_priv *priv)
>   
> -	relax = clk_rate / (1000000000 / DEF_RELAX) - 1;
> +	relax = clk_rate / (1000000000 / DEF_RELAX);

On a closer look the lines below also look odd:

>   	strobe_prog = clk_rate / (1000000000 / 10000) + 2 * (DEF_RELAX + 1) - 1;
>   	strobe_read = clk_rate / (1000000000 / 40) + 2 * (DEF_RELAX + 1) - 1;

The DEF_RELAX constant is defined in ns but strobe_prog and strobe_read 
are in ipg cycles so this mixes units?! It might work if the result is a 
value "higher" than needed but it should be fixed.

My suggestion would be to check the uboot code, maybe copy it and and 
rerun your tests:

https://git.denx.de/?p=u-boot.git;a=blob;f=drivers/misc/mxc_ocotp.c;h=f84fe88db193583d8f17917b5b2dc2aec313e483;hb=HEAD#l280

Or just replace DEF_RELAX with relax.

--
Regards,
Leonard

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