[U-Boot] [PATCH 1/1] arm: mx5: Add fuse supply gate enable in fsl_iim

[U-Boot] [PATCH 1/1] arm: mx5: Add fuse supply gate enable in fsl_iim

[Sergey Alyoshin]

Enable fuse supply gate before fuse programming and disable after.

Signed-off-by: Sergey Alyoshin <alyoshin.s@gmail.com>
Tested-by: Sergey Alyoshin <alyoshin.s@gmail.com>
---
 arch/arm/cpu/armv7/mx5/clock.c           |   12 ++++++++++++
 arch/arm/include/asm/arch-mx5/clock.h    |    1 +
 arch/arm/include/asm/arch-mx5/crm_regs.h |    3 +++
 drivers/misc/fsl_iim.c                   |   18 +++++++++++++++++-
 4 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/arch/arm/cpu/armv7/mx5/clock.c b/arch/arm/cpu/armv7/mx5/clock.c
index fb3b128..10c1e34 100644
--- a/arch/arm/cpu/armv7/mx5/clock.c
+++ b/arch/arm/cpu/armv7/mx5/clock.c
@@ -749,6 +749,18 @@ void enable_nfc_clk(unsigned char enable)
 		MXC_CCM_CCGR5_EMI_ENFC(cg));
 }
 
+#ifdef CONFIG_FSL_IIM
+void enable_efuse_prog_gate(unsigned char enable)
+{
+	if (enable)
+		setbits_le32(&mxc_ccm->cgpr,
+				MXC_CCM_CGPR_EFUSE_PROG_SUPPLY_GATE);
+	else
+		clrbits_le32(&mxc_ccm->cgpr,
+				MXC_CCM_CGPR_EFUSE_PROG_SUPPLY_GATE);
+}
+#endif
+
 /* Config main_bus_clock for periphs */
 static int config_periph_clk(u32 ref, u32 freq)
 {
diff --git a/arch/arm/include/asm/arch-mx5/clock.h b/arch/arm/include/asm/arch-mx5/clock.h
index 9ee79ae..5f3927a 100644
--- a/arch/arm/include/asm/arch-mx5/clock.h
+++ b/arch/arm/include/asm/arch-mx5/clock.h
@@ -53,5 +53,6 @@ void enable_usboh3_clk(bool enable);
 void mxc_set_sata_internal_clock(void);
 int enable_i2c_clk(unsigned char enable, unsigned i2c_num);
 void enable_nfc_clk(unsigned char enable);
+void enable_efuse_prog_gate(unsigned char enable);
 
 #endif /* __ASM_ARCH_CLOCK_H */
diff --git a/arch/arm/include/asm/arch-mx5/crm_regs.h b/arch/arm/include/asm/arch-mx5/crm_regs.h
index 392881c..efe57e0 100644
--- a/arch/arm/include/asm/arch-mx5/crm_regs.h
+++ b/arch/arm/include/asm/arch-mx5/crm_regs.h
@@ -305,6 +305,9 @@ struct mxc_ccm_reg {
 /* Define the bits in register CCDR */
 #define MXC_CCM_CCDR_IPU_HS_MASK			(0x1 << 17)
 
+/* Define the bits in register CGPR */
+#define MXC_CCM_CGPR_EFUSE_PROG_SUPPLY_GATE		(1 << 4)
+
 /* Define the bits in register CCGRx */
 #define MXC_CCM_CCGR_CG_MASK				0x3
 #define MXC_CCM_CCGR_CG_OFF				0x0
diff --git a/drivers/misc/fsl_iim.c b/drivers/misc/fsl_iim.c
index 44ae7b1..8529141 100644
--- a/drivers/misc/fsl_iim.c
+++ b/drivers/misc/fsl_iim.c
@@ -16,6 +16,9 @@
 #ifndef CONFIG_MPC512X
 #include <asm/arch/imx-regs.h>
 #endif
+#if defined(CONFIG_MX51) || defined(CONFIG_MX53)
+#include <asm/arch/clock.h>
+#endif
 
 /* FSL IIM-specific constants */
 #define STAT_BUSY		0x80
@@ -74,6 +77,15 @@
 #error Endianess is not defined: please fix to continue
 #endif
 
+#if defined(CONFIG_MX51) || defined(CONFIG_MX53)
+static inline void enable_fuse_prog(unsigned char enable)
+{
+	enable_efuse_prog_gate(enable);
+}
+#else
+static inline void enable_fuse_prog(unsigned char enable) {}
+#endif
+
 /* IIM control registers */
 struct fsl_iim {
 	u32 stat;
@@ -237,12 +249,16 @@ int fuse_prog(u32 bank, u32 word, u32 val)
 	if (ret)
 		return ret;
 
+	enable_fuse_prog(1);
 	for (bit = 0; val; bit++, val >>= 1)
 		if (val & 0x01) {
 			ret = prog_bit(regs, bank, word, bit);
-			if (ret)
+			if (ret) {
+				enable_fuse_prog(0);
 				return ret;
+			}
 		}
+	enable_fuse_prog(0);
 
 	return 0;
 }
-- 
1.7.10.4

[U-Boot] [PATCH 1/1] arm: mx5: Add fuse supply gate enable in fsl_iim

[Benoît Thébaudeau]

Dear Sergey Alyoshin,

On Thursday, December 12, 2013 5:46:21 PM, Sergey Alyoshin wrote:
> Enable fuse supply gate before fuse programming and disable after.
> 
> Signed-off-by: Sergey Alyoshin <alyoshin.s@gmail.com>
> Tested-by: Sergey Alyoshin <alyoshin.s@gmail.com>

Have you also tested without this patch first too? On which SoC?

My tests had shown that this is not required on i.MX51. It's probably the same
on i.MX53. I have not checked if i.MX25 and i.MX35 also have such a bit, but
they also work fine as is. This is not even done by Freescale:
http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/drivers/char/mxc_iim.c?h=imx_3.0.35_4.1.0

In the Reference Manual, nothing says in the IIM / fuse chapters that this bit
is needed for proper programming. It is just described in the CCM register map,
and nothing refers to it. It is perhaps only useful for test purposes. I'd vote
for letting this bit untouched if possible.

Fabio, do you have more information from Freescale about this bit?

Best regards,
Beno?t

[U-Boot] [PATCH 1/1] arm: mx5: Add fuse supply gate enable in fsl_iim

[Benoît Thébaudeau]

On Friday, December 13, 2013 1:52:19 AM, Beno?t Th?baudeau wrote:
> Dear Sergey Alyoshin,
> 
> On Thursday, December 12, 2013 5:46:21 PM, Sergey Alyoshin wrote:
> > Enable fuse supply gate before fuse programming and disable after.
> > 
> > Signed-off-by: Sergey Alyoshin <alyoshin.s@gmail.com>
> > Tested-by: Sergey Alyoshin <alyoshin.s@gmail.com>
> 
> Have you also tested without this patch first too? On which SoC?
> 
> My tests had shown that this is not required on i.MX51. It's probably the
> same
> on i.MX53. I have not checked if i.MX25 and i.MX35 also have such a bit, but
> they also work fine as is. This is not even done by Freescale:
> http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/drivers/char/mxc_iim.c?h=imx_3.0.35_4.1.0
> 
> In the Reference Manual, nothing says in the IIM / fuse chapters that this
> bit
> is needed for proper programming. It is just described in the CCM register
> map,
> and nothing refers to it. It is perhaps only useful for test purposes. I'd
> vote
> for letting this bit untouched if possible.
> 
> Fabio, do you have more information from Freescale about this bit?

Also note that the data sheets of the i.MX51 and i.MX53 both mention a dedicated
VDD_FUSE pin, with several notes saying that it should not be powered if
programming is not required in order to avoid inadvertently blowing fuses in
read mode. These notes would not be required if this bit gating this supply by
default really did exactly that. I think that it is rather an internal feature
for Freescale only, e.g. to blow Freescale bits like UID in die production.

Best regards,
Beno?t

[U-Boot] [PATCH 1/1] arm: mx5: Add fuse supply gate enable in fsl_iim

[Sergey Alyoshin]

Hello, Beno?t,

On Fri, Dec 13, 2013 at 4:52 AM, Beno?t Th?baudeau
<benoit.thebaudeau@advansee.com> wrote:
> On Thursday, December 12, 2013 5:46:21 PM, Sergey Alyoshin wrote:
>> Enable fuse supply gate before fuse programming and disable after.
>>
>> Signed-off-by: Sergey Alyoshin <alyoshin.s@gmail.com>
>> Tested-by: Sergey Alyoshin <alyoshin.s@gmail.com>
>
> Have you also tested without this patch first too? On which SoC?

I have tried to write MAC address in fuse (bank 1) on i.MX53 custom board
and 'fuse sense' show no change, reboot show no change either.

I have not tested this on i.MX51, but this register and bit is the same on
i.MX51 with exactly the same description.  With this patch I have
successfully written MAC addresses on several i.MX53 boards.

In Linux 2.6.35 from Freescale this bit is also set for fuse programming,
e.g. in arch/arm/mach-mx5/mx51_babbage.c and arch/arm/mach-mx5/mx53_loco.c
in mxc_iim_enable_fuse().

> My tests had shown that this is not required on i.MX51. It's probably the same
> on i.MX53. I have not checked if i.MX25 and i.MX35 also have such a bit, but
> they also work fine as is. This is not even done by Freescale:
> http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/drivers/char/mxc_iim.c?h=imx_3.0.35_4.1.0
>
> In the Reference Manual, nothing says in the IIM / fuse chapters that this bit
> is needed for proper programming. It is just described in the CCM register map,
> and nothing refers to it. It is perhaps only useful for test purposes. I'd vote
> for letting this bit untouched if possible.
>
> Fabio, do you have more information from Freescale about this bit?

[U-Boot] [PATCH 1/1] arm: mx5: Add fuse supply gate enable in fsl_iim

[Sergey Alyoshin]

On Fri, Dec 13, 2013 at 5:04 AM, Beno?t Th?baudeau
<benoit.thebaudeau@advansee.com> wrote:
> On Friday, December 13, 2013 1:52:19 AM, Beno?t Th?baudeau wrote:
>> Dear Sergey Alyoshin,
>>
>> On Thursday, December 12, 2013 5:46:21 PM, Sergey Alyoshin wrote:
>> > Enable fuse supply gate before fuse programming and disable after.
>> >
>> > Signed-off-by: Sergey Alyoshin <alyoshin.s@gmail.com>
>> > Tested-by: Sergey Alyoshin <alyoshin.s@gmail.com>
>>
>> Have you also tested without this patch first too? On which SoC?
>>
>> My tests had shown that this is not required on i.MX51. It's probably the
>> same
>> on i.MX53. I have not checked if i.MX25 and i.MX35 also have such a bit, but
>> they also work fine as is. This is not even done by Freescale:
>> http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/drivers/char/mxc_iim.c?h=imx_3.0.35_4.1.0
>>
>> In the Reference Manual, nothing says in the IIM / fuse chapters that this
>> bit
>> is needed for proper programming. It is just described in the CCM register
>> map,
>> and nothing refers to it. It is perhaps only useful for test purposes. I'd
>> vote
>> for letting this bit untouched if possible.
>>
>> Fabio, do you have more information from Freescale about this bit?
>
> Also note that the data sheets of the i.MX51 and i.MX53 both mention a dedicated
> VDD_FUSE pin, with several notes saying that it should not be powered if
> programming is not required in order to avoid inadvertently blowing fuses in
> read mode. These notes would not be required if this bit gating this supply by
> default really did exactly that. I think that it is rather an internal feature
> for Freescale only, e.g. to blow Freescale bits like UID in die production.

I apply 3.3V on VDD_FUSE pin and still without setting
EFUSE_PROG_SUPPLY_GATE bank 1 was not programmed on i.MX53.

[U-Boot] [PATCH 1/1] arm: mx5: Add fuse supply gate enable in fsl_iim

[Benoît Thébaudeau]

Dear Sergey Alyoshin,

On Thursday, December 12, 2013 5:46:21 PM, Sergey Alyoshin wrote:
> Enable fuse supply gate before fuse programming and disable after.
> 
> Signed-off-by: Sergey Alyoshin <alyoshin.s@gmail.com>
> Tested-by: Sergey Alyoshin <alyoshin.s@gmail.com>

You convinced me. ;)

> ---
>  arch/arm/cpu/armv7/mx5/clock.c           |   12 ++++++++++++
>  arch/arm/include/asm/arch-mx5/clock.h    |    1 +
>  arch/arm/include/asm/arch-mx5/crm_regs.h |    3 +++
>  drivers/misc/fsl_iim.c                   |   18 +++++++++++++++++-
>  4 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/cpu/armv7/mx5/clock.c b/arch/arm/cpu/armv7/mx5/clock.c
> index fb3b128..10c1e34 100644
> --- a/arch/arm/cpu/armv7/mx5/clock.c
> +++ b/arch/arm/cpu/armv7/mx5/clock.c
> @@ -749,6 +749,18 @@ void enable_nfc_clk(unsigned char enable)
>  		MXC_CCM_CCGR5_EMI_ENFC(cg));
>  }
>  
> +#ifdef CONFIG_FSL_IIM
> +void enable_efuse_prog_gate(unsigned char enable)

Perhaps enable_efuse_prog_supply would be a better naming.

And bool would be more appropriate than unsigned char.

> +{
> +	if (enable)
> +		setbits_le32(&mxc_ccm->cgpr,
> +				MXC_CCM_CGPR_EFUSE_PROG_SUPPLY_GATE);
> +	else
> +		clrbits_le32(&mxc_ccm->cgpr,
> +				MXC_CCM_CGPR_EFUSE_PROG_SUPPLY_GATE);
> +}
> +#endif
> +
>  /* Config main_bus_clock for periphs */
>  static int config_periph_clk(u32 ref, u32 freq)
>  {
> diff --git a/arch/arm/include/asm/arch-mx5/clock.h
> b/arch/arm/include/asm/arch-mx5/clock.h
> index 9ee79ae..5f3927a 100644
> --- a/arch/arm/include/asm/arch-mx5/clock.h
> +++ b/arch/arm/include/asm/arch-mx5/clock.h
> @@ -53,5 +53,6 @@ void enable_usboh3_clk(bool enable);
>  void mxc_set_sata_internal_clock(void);
>  int enable_i2c_clk(unsigned char enable, unsigned i2c_num);
>  void enable_nfc_clk(unsigned char enable);
> +void enable_efuse_prog_gate(unsigned char enable);

Ditto for the function declaration.

>  
>  #endif /* __ASM_ARCH_CLOCK_H */
> diff --git a/arch/arm/include/asm/arch-mx5/crm_regs.h
> b/arch/arm/include/asm/arch-mx5/crm_regs.h
> index 392881c..efe57e0 100644
> --- a/arch/arm/include/asm/arch-mx5/crm_regs.h
> +++ b/arch/arm/include/asm/arch-mx5/crm_regs.h
> @@ -305,6 +305,9 @@ struct mxc_ccm_reg {
>  /* Define the bits in register CCDR */
>  #define MXC_CCM_CCDR_IPU_HS_MASK			(0x1 << 17)
>  
> +/* Define the bits in register CGPR */
> +#define MXC_CCM_CGPR_EFUSE_PROG_SUPPLY_GATE		(1 << 4)
> +
>  /* Define the bits in register CCGRx */
>  #define MXC_CCM_CCGR_CG_MASK				0x3
>  #define MXC_CCM_CCGR_CG_OFF				0x0
> diff --git a/drivers/misc/fsl_iim.c b/drivers/misc/fsl_iim.c
> index 44ae7b1..8529141 100644
> --- a/drivers/misc/fsl_iim.c
> +++ b/drivers/misc/fsl_iim.c
> @@ -16,6 +16,9 @@
>  #ifndef CONFIG_MPC512X
>  #include <asm/arch/imx-regs.h>
>  #endif
> +#if defined(CONFIG_MX51) || defined(CONFIG_MX53)
> +#include <asm/arch/clock.h>
> +#endif
>  
>  /* FSL IIM-specific constants */
>  #define STAT_BUSY		0x80
> @@ -74,6 +77,15 @@
>  #error Endianess is not defined: please fix to continue
>  #endif
>  
> +#if defined(CONFIG_MX51) || defined(CONFIG_MX53)
> +static inline void enable_fuse_prog(unsigned char enable)
> +{
> +	enable_efuse_prog_gate(enable);
> +}
> +#else
> +static inline void enable_fuse_prog(unsigned char enable) {}
> +#endif
> +

Please drop the 'inline', and move this function after the struct fsl_iim type
definition, i.e. right before prepare_access().

Also, you don't have to redefine a function, so you could just do:
+#if !defined(CONFIG_MX51) && !defined(CONFIG_MX53)
+static void enable_efuse_prog_supply(bool enable) {}
+#endif
+

>  /* IIM control registers */
>  struct fsl_iim {
>  	u32 stat;
> @@ -237,12 +249,16 @@ int fuse_prog(u32 bank, u32 word, u32 val)
>  	if (ret)
>  		return ret;
>  
> +	enable_fuse_prog(1);
>  	for (bit = 0; val; bit++, val >>= 1)
>  		if (val & 0x01) {
>  			ret = prog_bit(regs, bank, word, bit);
> -			if (ret)
> +			if (ret) {
> +				enable_fuse_prog(0);
>  				return ret;
> +			}
>  		}
> +	enable_fuse_prog(0);
>  
>  	return 0;
>  }
> --
> 1.7.10.4

Best regards,
Beno?t

[U-Boot] [PATCH 1/1] arm: mx5: Add fuse supply gate enable in fsl_iim

[Benoît Thébaudeau]

On Friday, December 13, 2013 1:42:10 PM, Beno?t Th?baudeau wrote:
> On Thursday, December 12, 2013 5:46:21 PM, Sergey Alyoshin wrote:
> > +#if defined(CONFIG_MX51) || defined(CONFIG_MX53)
> > +static inline void enable_fuse_prog(unsigned char enable)
> > +{
> > +	enable_efuse_prog_gate(enable);
> > +}
> > +#else
> > +static inline void enable_fuse_prog(unsigned char enable) {}
> > +#endif
> > +
> 
> Please drop the 'inline', and move this function after the struct fsl_iim
> type
> definition, i.e. right before prepare_access().
> 
> Also, you don't have to redefine a function, so you could just do:
> +#if !defined(CONFIG_MX51) && !defined(CONFIG_MX53)
> +static void enable_efuse_prog_supply(bool enable) {}
> +#endif
> +

Or even better:
> +#if !defined(CONFIG_MX51) && !defined(CONFIG_MX53)
> +#define enable_efuse_prog_supply(enable)
> +#endif
> +

Best regards,
Beno?t