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 14:42:29 +0000	[thread overview]
Message-ID: <AM0PR04MB64347666742FE924AB9C3617EE230@AM0PR04MB6434.eurprd04.prod.outlook.com> (raw)
In-Reply-To: a4a8d5f0-3c05-be80-fd15-8c4984337152@nexus-software.ie

On 4/23/2019 5:07 PM, Bryan O'Donoghue wrote:
> 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:

>> 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.

Having nanosecond constants in the code and converting them to ipg 
clocks is the right approach. It's just that linux driver mixed the units.

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

It makes sense for physical hardware requirements really to be measured 
in seconds. It's just the OCOTP block can only measure time based on the 
bus clock input so it wants the driver to convert "real time" to "ipg 
clock periods".

Supporting arbitrary ipg clock rates seems reasonable for a driver in a 
general purpose OS.

--
Regards,
Leonard

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