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

From: Bryan O'Donoghue <pure.logic@nexus-software.ie>
To: Leonard Crestez <leonard.crestez@nxp.com>, 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 15:07:22 +0100	[thread overview]
Message-ID: <a4a8d5f0-3c05-be80-fd15-8c4984337152@nexus-software.ie> (raw)
In-Reply-To: <AM0PR04MB6434F2814AF55D476844AE40EE230@AM0PR04MB6434.eurprd04.prod.outlook.com>

On 23/04/2019 09:48, Leonard Crestez wrote:
> 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.

The original sin here is specification of the RELAX, STROBE_READ and 
STROBE_PROG fields in terms of absolute time values.

The u-boot code and therefore the Linux code specifies timings in terms 
of absolute nanoseconds but, the documentation specifies the values in 
terms of ipg clocks.

The error we have here, and also in u-boot is specifying 17 nanoseconds 
for relax or 37 nanoseconds of strobe_read and then trying to bludgeon 
those values via an ipg_clk division into the HW_OCOTP_TIMING bit-fields.

All fine and dandy if your ipg_clk is always the same on each SoC but, 
arseways if your ipg_clk changes between SoC versions.. as it invariably 
will.

So.. if we are going to change the ratios, we should get rid of these 
silly nanosecond declarations and instead express the values clocks, 
like the documentation does.

As an example:

#define RELAX_IPG_CLOCKS		2
#define STROBE_PROG_CLOCKS	800
#define STROBE_READ_CLOCKS	40

17 nanoseconds is literally the defintion of a magic number which 
_ought_ to have been defined in cycles from the beginnign

ie. @ 66 Mhz one cycle is aprox 15 nanoseconds

So OK, I'll have a go a rewriting this ...

---
bod

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