[PATCH v4 0/5] Add i.MX8MM support

[PATCH v4 0/5] Add i.MX8MM support

[Bryan O'Donoghue]

V4:
- Change the RELAX fix to drop subtraction of -1 for all users - Leonard
- Expand register definition from the 60 documented OTP registers to the
  entire 256 registers putatively in the address space*
- Add Reviewed-by as indicated - Leonard
- Added Suggested-by where it made sense - Bryan

* Dumping the expanded address space shows that there are indeed OTP values
  present that can be read back from registers that are not formally
  documented for i.MX8MM eg.

Bank 20
        0x55000801
        0x00014d14
        0xd503201f
        0x55000801
Bank 21
        0x00014d20
        0xd503201f
        0x00000000
        0x00000000

V3:
- Fix commit log for the expanding the ADDR field i.MX6 uses seven not four
  bits, which is why the existing define says 0x7F not 0x0F - bod

V2:
- Rebased to linux-next/master to align with i.8MQ work
- Two patches dropped as a result of rebase
- Added patch to expand OCOTP_CTRL_ADDR to 8 bits for all users - Leonard
- Makes sure nregs = 60 not 64 for i.MX8MM
- Tested imx8mm-evk, imx7s-warp7

V1:
This set adds support for the i.MX8MM.

When adding support for this processor there are two interesting gotchas to
watch for.

#1 We current do not preserve the WAIT field for i.MX6 and since we are
   reusing the i.MX6 set_timing() values, this would also affect i.MX8.
   On the face of it, it appears to be an inocuous error with no real side
   effects.

#2 Secondly the i.MX8MM will calculate a zero value for the RELAX bit-field
   when programming up OTP fuses.
   This is fine for programming the fuses but, it introduces a strange
   failure state with reloading the shadow registers subsequent to blowing
   an OTP fuse.
   The second important patch here then is ensuring the RELAX field is
   non-zero to avoid the failure state.


Bryan O'Donoghue (5):
  nvmem: imx-ocotp: Elongate OCOTP_CTRL ADDR field to eight bits
  nvmem: imx-ocotp: Ensure WAIT bits are preserved when setting timing
  nvmem: imx-ocotp: Ensure the RELAX field is non-zero
  nvmem: imx-ocotp: Add i.MX8MM support
  dt-bindings: imx-ocotp: Add i.MX8MM compatible

 .../devicetree/bindings/nvmem/imx-ocotp.txt        |  1 +
 drivers/nvmem/imx-ocotp.c                          | 14 +++++++++++---
 2 files changed, 12 insertions(+), 3 deletions(-)

-- 
2.20.1


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

[PATCH v4 1/5] nvmem: imx-ocotp: Elongate OCOTP_CTRL ADDR field to eight bits

[Bryan O'Donoghue]

i.MX6 defines OCOTP_CTRLn:ADDR as seven bit address-field with a one bit
RSVD0 field, i.MX7 defines OCOTP_CTRLn:ADDR as a four bit address-field
with a four bit RSVD0 field.

i.MX8 defines the OCOTP_CTRLn:ADDR bit-field as a full range eight bits.

i.MX6 and i.MX7 should return zero for their respective RSVD0 bits and
ignore a write-back of zero where i.MX8 will make use of the full range.

This patch expands the bit-field definition for all users to eight bits,
which is safe due to RSVD0 being a no-op for the i.MX6 and i.MX7.

Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>
Reviewed-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/nvmem/imx-ocotp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvmem/imx-ocotp.c b/drivers/nvmem/imx-ocotp.c
index 4cf7b61e4bf5..6600c4ddeb51 100644
--- a/drivers/nvmem/imx-ocotp.c
+++ b/drivers/nvmem/imx-ocotp.c
@@ -45,7 +45,7 @@
 #define IMX_OCOTP_ADDR_DATA2		0x0040
 #define IMX_OCOTP_ADDR_DATA3		0x0050
 
-#define IMX_OCOTP_BM_CTRL_ADDR		0x0000007F
+#define IMX_OCOTP_BM_CTRL_ADDR		0x000000FF
 #define IMX_OCOTP_BM_CTRL_BUSY		0x00000100
 #define IMX_OCOTP_BM_CTRL_ERROR		0x00000200
 #define IMX_OCOTP_BM_CTRL_REL_SHADOWS	0x00000400
-- 
2.20.1


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

[PATCH v4 2/5] nvmem: imx-ocotp: Ensure WAIT bits are preserved when setting timing

[Bryan O'Donoghue]

The i.MX6 and i.MX8 both have a bit-field spanning bits 27:22 called the
WAIT field.

The WAIT field according to the documentation for both parts "specifies
time interval between auto read and write access in one time program. It is
given in number of ipg_clk periods."

This patch ensures that the relevant field is read and written back to the
timing register.

Fixes: 0642bac7da42 ("nvmem: imx-ocotp: add write support")

Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>
Reviewed-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/nvmem/imx-ocotp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/nvmem/imx-ocotp.c b/drivers/nvmem/imx-ocotp.c
index 6600c4ddeb51..85a7d0da3abb 100644
--- a/drivers/nvmem/imx-ocotp.c
+++ b/drivers/nvmem/imx-ocotp.c
@@ -189,7 +189,8 @@ static void imx_ocotp_set_imx6_timing(struct ocotp_priv *priv)
 	strobe_prog = clk_rate / (1000000000 / 10000) + 2 * (DEF_RELAX + 1) - 1;
 	strobe_read = clk_rate / (1000000000 / 40) + 2 * (DEF_RELAX + 1) - 1;
 
-	timing = strobe_prog & 0x00000FFF;
+	timing = readl(priv->base + IMX_OCOTP_ADDR_TIMING) & 0x0FC00000;
+	timing |= strobe_prog & 0x00000FFF;
 	timing |= (relax       << 12) & 0x0000F000;
 	timing |= (strobe_read << 16) & 0x003F0000;
 
-- 
2.20.1


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

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

[Bryan O'Donoghue]

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")

Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>
Suggested-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/nvmem/imx-ocotp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvmem/imx-ocotp.c b/drivers/nvmem/imx-ocotp.c
index 85a7d0da3abb..2fa45d1d17eb 100644
--- 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)
 	 */
 	clk_rate = clk_get_rate(priv->clk);
 
-	relax = clk_rate / (1000000000 / DEF_RELAX) - 1;
+	relax = clk_rate / (1000000000 / DEF_RELAX);
 	strobe_prog = clk_rate / (1000000000 / 10000) + 2 * (DEF_RELAX + 1) - 1;
 	strobe_read = clk_rate / (1000000000 / 40) + 2 * (DEF_RELAX + 1) - 1;
 
-- 
2.20.1


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

[PATCH v4 4/5] nvmem: imx-ocotp: Add i.MX8MM support

[Bryan O'Donoghue]

This patch adds support to burn the fuses on the i.MX8MM.
https://www.nxp.com/webapp/Download?colCode=IMX8MMRM

The i.MX8MM is similar to i.MX6 processors in terms of addressing and clock
setup.

The documentation specifies 60 discreet OTP registers but, the fusemap
address space encompasses up to 256 registers. We map the entire putative
256 OTP registers.

Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>
---
 drivers/nvmem/imx-ocotp.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/nvmem/imx-ocotp.c b/drivers/nvmem/imx-ocotp.c
index 2fa45d1d17eb..cb056765551b 100644
--- a/drivers/nvmem/imx-ocotp.c
+++ b/drivers/nvmem/imx-ocotp.c
@@ -451,6 +451,12 @@ static const struct ocotp_params imx8mq_params = {
 	.set_timing = imx_ocotp_set_imx7_timing,
 };
 
+static const struct ocotp_params imx8mm_params = {
+	.nregs = 256,
+	.bank_address_words = 0,
+	.set_timing = imx_ocotp_set_imx6_timing,
+};
+
 static const struct of_device_id imx_ocotp_dt_ids[] = {
 	{ .compatible = "fsl,imx6q-ocotp",  .data = &imx6q_params },
 	{ .compatible = "fsl,imx6sl-ocotp", .data = &imx6sl_params },
@@ -461,6 +467,7 @@ static const struct of_device_id imx_ocotp_dt_ids[] = {
 	{ .compatible = "fsl,imx6sll-ocotp", .data = &imx6sll_params },
 	{ .compatible = "fsl,imx7ulp-ocotp", .data = &imx7ulp_params },
 	{ .compatible = "fsl,imx8mq-ocotp", .data = &imx8mq_params },
+	{ .compatible = "fsl,imx8mm-ocotp", .data = &imx8mm_params },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, imx_ocotp_dt_ids);
-- 
2.20.1


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

[PATCH v4 5/5] dt-bindings: imx-ocotp: Add i.MX8MM compatible

[Bryan O'Donoghue]

Add compatible for i.MX8MM as per arch/arm64/boot/dts/freescale/imx8mm.dtsi

Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>
Cc: Rob Herring <robh@kernel.org>
Reviewed-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 Documentation/devicetree/bindings/nvmem/imx-ocotp.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/nvmem/imx-ocotp.txt b/Documentation/devicetree/bindings/nvmem/imx-ocotp.txt
index 68f7d6fdd140..96ffd06d2ca8 100644
--- a/Documentation/devicetree/bindings/nvmem/imx-ocotp.txt
+++ b/Documentation/devicetree/bindings/nvmem/imx-ocotp.txt
@@ -15,6 +15,7 @@ Required properties:
 	"fsl,imx6sll-ocotp" (i.MX6SLL),
 	"fsl,imx7ulp-ocotp" (i.MX7ULP),
 	"fsl,imx8mq-ocotp" (i.MX8MQ),
+	"fsl,imx8mm-ocotp" (i.MX8MM),
 	followed by "syscon".
 - #address-cells : Should be 1
 - #size-cells : Should be 1
-- 
2.20.1


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

Re: [PATCH v4 4/5] nvmem: imx-ocotp: Add i.MX8MM support

[Leonard Crestez]

On 4/22/2019 6:05 PM, Bryan O'Donoghue wrote:
> This patch adds support to burn the fuses on the i.MX8MM.
>
> The i.MX8MM is similar to i.MX6 processors in terms of addressing and clock
> setup.
> 
> The documentation specifies 60 discreet OTP registers but, the fusemap
> address space encompasses up to 256 registers. We map the entire putative
> 256 OTP registers.
> 
> Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>

Reviewed-by: Leonard Crestez <leonard.crestez@nxp.com>

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

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

[Leonard Crestez]

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

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

[Bryan O'Donoghue]

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

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

[Leonard Crestez]

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

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

[Bryan O'Donoghue]

On 23/04/2019 15:42, Leonard Crestez wrote:
>> 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".

Exactly. The block only understands clocks, not absolute time values.

If you increase your ipg_clock to 133 Mhz the initial 17 nanosecond 
value breaks again or if you reduce your ipg_clk to 6.6 Mhz, again that 
value breaks.

But if you specify the right _ratio_ between RELAX and STROBE/READ 
cycles - you can push your clock in any direction you like.

Because the block 'ticks over' so to speak, on clock edges.


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