mxs_ocotp: prevent error path from returning success

PATCH New Hector Palacios 3 participants 11 messages Nov 21, 2014 – Dec 1, 2014

Subsystems (2)

Arm i.MX and Layerscape Char and misc drivers

lore

Share & tools

lore

Hector Palacios

author

2014-11-21 16:54 UTC #0 lore raw

PATCH 1/3 mxs_ocotp: prevent error path from returning success

The code may goto 'fail' upon error with 'ret' variable set to an error code, but this variable was being overwritten by a final preparation function to restore the HCLK, so success was (in general) returned even after an error was hit previously.

With this change, the function may now return success even if the final preparation function fails, but it's probably enough to print a message because (if successful) the real programming of the fuses has already completed.

Signed-off-by Hector Palacios <[email protected]>

---
 drivers/misc/mxs_ocotp.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

drivers/misc/mxs_ocotp.c +1 -4

index 545d3ebf520e..09002814f2f0 100644
@@ -223,11 +223,8 @@ static int mxs_ocotp_write_fuse(uint32_t addr, uint32_t mask)
 
 fail:
 	mxs_ocotp_scale_vddio(0, &vddio_val);
-	ret = mxs_ocotp_scale_hclk(0, &hclk_val);
-	if (ret) {
+	if (mxs_ocotp_scale_hclk(0, &hclk_val))
 		puts("Failed scaling up the HCLK!\n");
-		return ret;
-	}
 
 	return ret;
 }

Hector Palacios

author

2014-11-21 16:54 UTC #1 lore raw

PATCH 2/3 mxs_ocotp: check for errors from the OTP controller after writing

The write operation may fail when trying to write to a locked area. In this case the ERROR bit is set in the CTRL register. Check for that condition and return an error.

Signed-off-by Hector Palacios <[email protected]>

---
 drivers/misc/mxs_ocotp.c | 7 +++++++
 1 file changed, 7 insertions(+)

drivers/misc/mxs_ocotp.c +7 -0

index 09002814f2f0..1659ee6a5eec 100644
@@ -221,6 +221,13 @@ static int mxs_ocotp_write_fuse(uint32_t addr, uint32_t mask)
 		goto fail;
 	}
 
+	/* Check for errors */
+	if (readl(&ocotp_regs->hw_ocotp_ctrl) & OCOTP_CTRL_ERROR) {
+		puts("Failed writing fuses!\n");
+		ret = -EPERM;
+		goto fail;
+	}
+
 fail:
 	mxs_ocotp_scale_vddio(0, &vddio_val);
 	if (mxs_ocotp_scale_hclk(0, &hclk_val))

Fabio Estevam

reviewer

2014-11-21 17:10 UTC #2 lore raw

PATCH 2/3 mxs_ocotp: check for errors from the OTP controller after writing

Hi Hector,

Quoted 15 lines · Hector Palacios

The write operation may fail when trying to write to a locked area. In this case the ERROR bit is set in the CTRL register. Check for that condition and return an error.
Signed-off-by Hector Palacios <[email protected]>
---
 drivers/misc/mxs_ocotp.c | 7 +++++++
 1 file changed, 7 insertions(+)
drivers/misc/mxs_ocotp.c +0 -0
index 09002814f2f0..1659ee6a5eec 100644
@@ -221,6 +221,13 @@ static int mxs_ocotp_write_fuse(uint32_t addr, uint32_t mask)
                goto fail;

drivers/misc/mxs_ocotp.c static int mxs_ocotp_write_fuse(uint32_t addr, uint32_t mask) · line 231

Quoted 10 lines

drivers/misc/mxs_ocotp.c +7 -0

        }
 
+       /* Check for errors */
+       if (readl(&ocotp_regs->hw_ocotp_ctrl) & OCOTP_CTRL_ERROR) {
+               puts("Failed writing fuses!\n");
+               ret = -EPERM;
+               goto fail;
+       }
+
 fail:

What about doing this instead?

       /* Check for errors */
       ret = readl(&ocotp_regs->hw_ocotp_ctrl) & OCOTP_CTRL_ERROR);
       if (ret) {
               printfs("Failed writing the fuses: %d\n", ret);
               goto fail;
       }

Hector Palacios

author

2014-11-21 18:04 UTC #3 lore raw

PATCH 2/3 mxs_ocotp: check for errors from the OTP controller after writing

Hi Fabio,

Quoted 32 lines · Fabio Estevam

Hi Hector,
Quoted 15 lines · Hector Palacios
The write operation may fail when trying to write to a locked area. In this case the ERROR bit is set in the CTRL register. Check for that condition and return an error.
Signed-off-by Hector Palacios <[email protected]>
---
 drivers/misc/mxs_ocotp.c | 7 +++++++
 1 file changed, 7 insertions(+)
drivers/misc/mxs_ocotp.c +0 -0
index 09002814f2f0..1659ee6a5eec 100644
@@ -221,6 +221,13 @@ static int mxs_ocotp_write_fuse(uint32_t addr, uint32_t mask)
                goto fail;
drivers/misc/mxs_ocotp.c static int mxs_ocotp_write_fuse(uint32_t addr, uint32_t mask) · line 231
Quoted 10 lines
drivers/misc/mxs_ocotp.c +7 -0
        }
 
+       /* Check for errors */
+       if (readl(&ocotp_regs->hw_ocotp_ctrl) & OCOTP_CTRL_ERROR) {
+               puts("Failed writing fuses!\n");
+               ret = -EPERM;
+               goto fail;
+       }
+
 fail:
What about doing this instead?

Quoted 7 lines

       /* Check for errors */
       ret = readl(&ocotp_regs->hw_ocotp_ctrl) & OCOTP_CTRL_ERROR);
       if (ret) {
               printfs("Failed writing the fuses: %d\n", ret);
               goto fail;
       }

Although the code looks nicer you are not returning a meaningful error code, just a mask value (and yes, I know the error code does not get anywhere, but still). --
Hector Palacios

PS. Sorry about the resending of the message but the mailing list server kept rejecting it.

Fabio Estevam

reviewer

2014-11-22 19:29 UTC #4 lore raw

PATCH 2/3 mxs_ocotp: check for errors from the OTP controller after writing

Quoted 10 lines · Hector Palacios

Quoted 9 lines

What about doing this instead?

       /* Check for errors */
       ret = readl(&ocotp_regs->hw_ocotp_ctrl) & OCOTP_CTRL_ERROR);
       if (ret) {
               printfs("Failed writing the fuses: %d\n", ret);
               goto fail;
       }

Quoted 2 lines

Although the code looks nicer you are not returning a meaningful error code, just a mask value (and yes, I know the error code does not get anywhere, but still).

I am just returning the real error code, not a 'fake' one :-)

Fabio Estevam

reviewer

2014-11-22 19:31 UTC #5 lore raw

PATCH 2/3 mxs_ocotp: check for errors from the OTP controller after writing

Quoted 13 lines · Fabio Estevam

Quoted 10 lines · Hector Palacios

Quoted 9 lines

What about doing this instead?

       /* Check for errors */
       ret = readl(&ocotp_regs->hw_ocotp_ctrl) & OCOTP_CTRL_ERROR);
       if (ret) {
               printfs("Failed writing the fuses: %d\n", ret);
               goto fail;
       }

Quoted 4 lines

Quoted 2 lines
Although the code looks nicer you are not returning a meaningful error code, just a mask value (and yes, I know the error code does not get anywhere, but still).
I am just returning the real error code, not a 'fake' one :-)

Actually your code is correct and I misread it. Sorry about that.

Fabio Estevam

reviewer

2014-11-22 19:31 UTC #6 lore raw

PATCH 2/3 mxs_ocotp: check for errors from the OTP controller after writing

In reply to Hector Palacios

Quoted 5 lines · Hector Palacios

The write operation may fail when trying to write to a locked area. In this case the ERROR bit is set in the CTRL register. Check for that condition and return an error.
Signed-off-by Hector Palacios <[email protected]>

Reviewed-by Fabio Estevam <[email protected]>

Stefano Babic

2014-12-01 09:35 UTC #7 lore raw

PATCH 2/3 mxs_ocotp: check for errors from the OTP controller after writing

In reply to Hector Palacios

Quoted 18 lines · Hector Palacios

The write operation may fail when trying to write to a locked area. In this case the ERROR bit is set in the CTRL register. Check for that condition and return an error.
Signed-off-by Hector Palacios <[email protected]>
---
 drivers/misc/mxs_ocotp.c | 7 +++++++
 1 file changed, 7 insertions(+)
drivers/misc/mxs_ocotp.c +0 -0
index 09002814f2f0..1659ee6a5eec 100644
@@ -221,6 +221,13 @@ static int mxs_ocotp_write_fuse(uint32_t addr, uint32_t mask)
 		goto fail;
 	}
 

drivers/misc/mxs_ocotp.c static int mxs_ocotp_write_fuse(uint32_t addr, uint32_t mask) · line 233

Quoted 10 lines

drivers/misc/mxs_ocotp.c +7 -0

+	/* Check for errors */
+	if (readl(&ocotp_regs->hw_ocotp_ctrl) & OCOTP_CTRL_ERROR) {
+		puts("Failed writing fuses!\n");
+		ret = -EPERM;
+		goto fail;
+	}
+
 fail:
 	mxs_ocotp_scale_vddio(0, &vddio_val);
 	if (mxs_ocotp_scale_hclk(0, &hclk_val))

Applied to u-boot-imx, thanks !

Best regards,
Stefano Babic

Signature · 5 lines

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

Hector Palacios

author

2014-11-21 16:54 UTC #8 lore raw

PATCH 3/3 mxs_ocotp: clear the error flag before initiating write operation

In reply to Hector Palacios

A previous operation may have set the error flag, which must be cleared before a new write operation can be issued.

Signed-off-by Hector Palacios <[email protected]>

---
 drivers/misc/mxs_ocotp.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/misc/mxs_ocotp.c +2 -0

index 1659ee6a5eec..6f0a1d3e6da8 100644
@@ -187,6 +187,8 @@ static int mxs_ocotp_write_fuse(uint32_t addr, uint32_t mask)
 	uint32_t hclk_val, vddio_val;
 	int ret;
 
+	mxs_ocotp_clear_error();
+
 	/* Make sure the banks are closed for reading. */
 	ret = mxs_ocotp_read_bank_open(0);
 	if (ret) {

Stefano Babic

2014-12-01 09:35 UTC #9 lore raw

PATCH 3/3 mxs_ocotp: clear the error flag before initiating write operation

Quoted 14 lines · Hector Palacios

A previous operation may have set the error flag, which must be cleared before a new write operation can be issued.
Signed-off-by Hector Palacios <[email protected]>
---
 drivers/misc/mxs_ocotp.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/misc/mxs_ocotp.c +0 -0
index 1659ee6a5eec..6f0a1d3e6da8 100644

drivers/misc/mxs_ocotp.c static int mxs_ocotp_write_fuse(uint32_t addr, uint32_t mask) · line 194

Quoted 8 lines

drivers/misc/mxs_ocotp.c +2 -0

@@ -187,6 +187,8 @@ static int mxs_ocotp_write_fuse(uint32_t addr, uint32_t mask)
 	uint32_t hclk_val, vddio_val;
 	int ret;
 
+	mxs_ocotp_clear_error();
+
 	/* Make sure the banks are closed for reading. */
 	ret = mxs_ocotp_read_bank_open(0);
 	if (ret) {

Applied to u-boot-imx, thanks !

Best regards,
Stefano Babic

Signature · 5 lines

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

Stefano Babic

2014-12-01 09:35 UTC #10 lore raw

PATCH 1/3 mxs_ocotp: prevent error path from returning success

In reply to Hector Palacios

Quoted 23 lines · Hector Palacios

The code may goto 'fail' upon error with 'ret' variable set to an error code, but this variable was being overwritten by a final preparation function to restore the HCLK, so success was (in general) returned even after an error was hit previously.
With this change, the function may now return success even if the final preparation function fails, but it's probably enough to print a message because (if successful) the real programming of the fuses has already completed.
Signed-off-by Hector Palacios <[email protected]>
---
 drivers/misc/mxs_ocotp.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)
drivers/misc/mxs_ocotp.c +0 -0
index 545d3ebf520e..09002814f2f0 100644
@@ -223,11 +223,8 @@ static int mxs_ocotp_write_fuse(uint32_t addr, uint32_t mask)
 
 fail:

drivers/misc/mxs_ocotp.c static int mxs_ocotp_write_fuse(uint32_t addr, uint32_t mask) · line 230

Quoted 10 lines

drivers/misc/mxs_ocotp.c +1 -4

 	mxs_ocotp_scale_vddio(0, &vddio_val);
-	ret = mxs_ocotp_scale_hclk(0, &hclk_val);
-	if (ret) {
+	if (mxs_ocotp_scale_hclk(0, &hclk_val))
 		puts("Failed scaling up the HCLK!\n");
-		return ret;
-	}
 
 	return ret;
 }

Applied to u-boot-imx, thanks !

Best regards,
Stefano Babic

Signature · 5 lines

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================