[U-Boot] [PATCH 1/3] mxs_ocotp: prevent error path from returning success

[U-Boot] [PATCH 1/3] mxs_ocotp: prevent error path from returning success

[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 <hector.palacios@digi.com>
---
 drivers/misc/mxs_ocotp.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/misc/mxs_ocotp.c b/drivers/misc/mxs_ocotp.c
index 545d3ebf520e..09002814f2f0 100644
--- a/drivers/misc/mxs_ocotp.c
+++ b/drivers/misc/mxs_ocotp.c
@@ -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;
 }

[U-Boot] [PATCH 2/3] mxs_ocotp: check for errors from the OTP controller after writing

[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 <hector.palacios@digi.com>
---
 drivers/misc/mxs_ocotp.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/misc/mxs_ocotp.c b/drivers/misc/mxs_ocotp.c
index 09002814f2f0..1659ee6a5eec 100644
--- a/drivers/misc/mxs_ocotp.c
+++ b/drivers/misc/mxs_ocotp.c
@@ -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))

[U-Boot] [PATCH 2/3] mxs_ocotp: check for errors from the OTP controller after writing

[Fabio Estevam]

Hi Hector,

On Fri, Nov 21, 2014 at 2:54 PM, Hector Palacios
<hector.palacios@digi.com> wrote:
> 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 <hector.palacios@digi.com>
> ---
>  drivers/misc/mxs_ocotp.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/misc/mxs_ocotp.c b/drivers/misc/mxs_ocotp.c
> index 09002814f2f0..1659ee6a5eec 100644
> --- a/drivers/misc/mxs_ocotp.c
> +++ b/drivers/misc/mxs_ocotp.c
> @@ -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:

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;
       }

[U-Boot] [PATCH 2/3] mxs_ocotp: check for errors from the OTP controller after writing

[Hector Palacios]

Hi Fabio,

On 11/21/2014 06:10 PM, Fabio Estevam wrote:
> Hi Hector,
> 
> On Fri, Nov 21, 2014 at 2:54 PM, Hector Palacios
> <hector.palacios@digi.com> wrote:
>> 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 <hector.palacios@digi.com>
>> ---
>>  drivers/misc/mxs_ocotp.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/misc/mxs_ocotp.c b/drivers/misc/mxs_ocotp.c
>> index 09002814f2f0..1659ee6a5eec 100644
>> --- a/drivers/misc/mxs_ocotp.c
>> +++ b/drivers/misc/mxs_ocotp.c
>> @@ -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:
> 
> 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;
>        }
> 

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.

[U-Boot] [PATCH 2/3] mxs_ocotp: check for errors from the OTP controller after writing

[Fabio Estevam]

On Fri, Nov 21, 2014 at 4:04 PM, Hector Palacios
<hector.palacios@digi.com> wrote:

>> 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;
>>        }
>>
>
> 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 :-)

[U-Boot] [PATCH 2/3] mxs_ocotp: check for errors from the OTP controller after writing

[Fabio Estevam]

On Sat, Nov 22, 2014 at 5:29 PM, Fabio Estevam <festevam@gmail.com> wrote:
> On Fri, Nov 21, 2014 at 4:04 PM, Hector Palacios
> <hector.palacios@digi.com> wrote:
>
>>> 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;
>>>        }
>>>
>>
>> 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.

[U-Boot] [PATCH 2/3] mxs_ocotp: check for errors from the OTP controller after writing

[Fabio Estevam]

On Fri, Nov 21, 2014 at 2:54 PM, Hector Palacios
<hector.palacios@digi.com> wrote:
> 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 <hector.palacios@digi.com>

Reviewed-by: Fabio Estevam <fabio.estevam@freescale.com>

[U-Boot] [PATCH 2/3] mxs_ocotp: check for errors from the OTP controller after writing

[Stefano Babic]

On 21/11/2014 17:54, Hector Palacios wrote:
> 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 <hector.palacios@digi.com>
> ---
>  drivers/misc/mxs_ocotp.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/misc/mxs_ocotp.c b/drivers/misc/mxs_ocotp.c
> index 09002814f2f0..1659ee6a5eec 100644
> --- a/drivers/misc/mxs_ocotp.c
> +++ b/drivers/misc/mxs_ocotp.c
> @@ -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))
> 

Applied to u-boot-imx, thanks !

Best regards,
Stefano Babic

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

[U-Boot] [PATCH 3/3] mxs_ocotp: clear the error flag before initiating write operation

[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 <hector.palacios@digi.com>
---
 drivers/misc/mxs_ocotp.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/misc/mxs_ocotp.c b/drivers/misc/mxs_ocotp.c
index 1659ee6a5eec..6f0a1d3e6da8 100644
--- a/drivers/misc/mxs_ocotp.c
+++ b/drivers/misc/mxs_ocotp.c
@@ -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) {

[U-Boot] [PATCH 3/3] mxs_ocotp: clear the error flag before initiating write operation

[Stefano Babic]

On 21/11/2014 17:54, Hector Palacios wrote:
> 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 <hector.palacios@digi.com>
> ---
>  drivers/misc/mxs_ocotp.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/misc/mxs_ocotp.c b/drivers/misc/mxs_ocotp.c
> index 1659ee6a5eec..6f0a1d3e6da8 100644
> --- a/drivers/misc/mxs_ocotp.c
> +++ b/drivers/misc/mxs_ocotp.c
> @@ -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

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

[U-Boot] [PATCH 1/3] mxs_ocotp: prevent error path from returning success

[Stefano Babic]

On 21/11/2014 17:54, Hector Palacios wrote:
> 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 <hector.palacios@digi.com>
> ---
>  drivers/misc/mxs_ocotp.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/misc/mxs_ocotp.c b/drivers/misc/mxs_ocotp.c
> index 545d3ebf520e..09002814f2f0 100644
> --- a/drivers/misc/mxs_ocotp.c
> +++ b/drivers/misc/mxs_ocotp.c
> @@ -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;
>  }
> 
Applied to u-boot-imx, thanks !

Best regards,
Stefano Babic


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