sunxi: support fuse cmd to read/write fuse

Message ID 1517066427-19157-1-git-send-email-jun.nie@linaro.org
State New
Headers show
Series
  • sunxi: support fuse cmd to read/write fuse
Related show

Commit Message

Jun Nie Jan. 27, 2018, 3:20 p.m.
Support fuse cmd to read/write fuse. Power supply for fuse
should be ready, name is VDD_EFUSE in some schematic.

Signed-off-by: Jun Nie <jun.nie@linaro.org>
---
 arch/arm/mach-sunxi/cpu_info.c | 60 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

Comments

Andre Przywara Jan. 27, 2018, 5:45 p.m. | #1
On 27/01/18 15:20, Jun Nie wrote:
> Support fuse cmd to read/write fuse. Power supply for fuse
> should be ready, name is VDD_EFUSE in some schematic.

Mmh, in general I am not sure it is a good idea to expose this so easily
to the user. I guess a clueless user can easily brick his board by
typing something at the "fuse write" command. I understand that one has
to manually enable the fuse command first to allow access, but this is
still quite a high risk, especially since a lot of the fuses are not
documented.
Would love to hear opinions from others about that topic.

Also I would have hoped for a bit more documentation.
How do those banks/words from the write command map to the fuses, for
instance? What fuses are available and useful? What are the implications
of writing the secure boot fuse, for instance?

And do we know how access to the fuses is affected by the exception
level / mode we are in? My understanding is that SID access is secure
only, which would make it inaccessible for the 64-bit boards where
U-Boot runs in EL2. But then again at least the A64 does not seem to
care about this (unless the secure boot fuse is set).

More inline ...

> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
>  arch/arm/mach-sunxi/cpu_info.c | 60 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
> 
> diff --git a/arch/arm/mach-sunxi/cpu_info.c b/arch/arm/mach-sunxi/cpu_info.c
> index 25a5ec2..30bc2bf 100644
> --- a/arch/arm/mach-sunxi/cpu_info.c
> +++ b/arch/arm/mach-sunxi/cpu_info.c
> @@ -133,6 +133,30 @@ uint32_t sun8i_efuse_read(uint32_t offset)
>  	reg_val = readl(SUNXI_SIDC_BASE + SIDC_RDKEY);
>  	return reg_val;
>  }
> +
> +uint32_t sun8i_efuse_write(u32 offset, u32 val)
> +{
> +	u32 reg_val;
> +
> +	writel(val, SUNXI_SIDC_BASE + SIDC_RDKEY);
> +
> +	reg_val = readl(SUNXI_SIDC_BASE + SIDC_PRCTL);
> +	reg_val &= ~(((0x1ff) << 16) | 0x3);
> +	reg_val |= (offset << 16);

Would be good to put names to those magic values.
I understand this is from the code as I sent you ;-), but still ...

> +	writel(reg_val, SUNXI_SIDC_BASE + SIDC_PRCTL);
> +
> +	reg_val &= ~(((0xff) << 8) | 0x3);
> +	reg_val |= (SIDC_OP_LOCK << 8) | 0x1;
> +	writel(reg_val, SUNXI_SIDC_BASE + SIDC_PRCTL);
> +
> +	while (readl(SUNXI_SIDC_BASE + SIDC_PRCTL) & 0x1)
> +		;

shall we have a timeout or limited retries here?

> +
> +	reg_val &= ~(((0x1ff) << 16) | ((0xff) << 8) | 0x3);
> +	writel(reg_val, SUNXI_SIDC_BASE + SIDC_PRCTL);
> +
> +	return 0;
> +}
>  #endif
>  
>  int sunxi_get_sid(unsigned int *sid)
> @@ -164,3 +188,39 @@ int sunxi_get_sid(unsigned int *sid)
>  	return -ENODEV;
>  #endif
>  }
> +
> +int fuse_read(u32 bank, u32 word, u32 *sid)
> +{
> +#ifdef CONFIG_MACH_SUN8I_H3
> +	*sid = sun8i_efuse_read(word);
> +#elif defined SUNXI_SID_BASE
> +	*sid = readl((ulong)SUNXI_SID_BASE + word);
> +#else
> +	return -ENODEV;
> +#endif
> +	return 0;
> +}
> +
> +int fuse_prog(u32 bank, u32 word, u32 val)
> +{

I would feel better if we have the write access protected by a separate
Kconfig symbol. So without this being defined either nothing happens or
the user gets a warning.

> +#ifdef CONFIG_MACH_SUN8I_H3

I guess this applies to more than the H3, namely the A64 and A83T,
possibly also H5 and others?

> +	return sun8i_efuse_write(word, val);
> +#elif defined SUNXI_SID_BASE
> +	writel(val, (ulong)SUNXI_SID_BASE + word);

Are you sure that works? If I read [1] correctly, you always have to use
a special algorithm to burn a fuse, a simple MMIO write access would not
suffice.
The algorithm seems to be different for older SoCs (pre-H3).

> +#else
> +	return -ENODEV;
> +#endif
> +	return 0;
> +}
> +
> +int fuse_sense(u32 bank, u32 word, u32 *val)
> +{
> +	/* We do not support sensing :-( */

Isn't sense the only thing we actually implement? At least this is my
understanding from reading doc/README.fuse.

Cheers,
Andre.


> +	return -EINVAL;
> +}
> +
> +int fuse_override(u32 bank, u32 word, u32 val)
> +{
> +	/* We do not support overriding :-( */
> +	return -EINVAL;
> +}
>
Jun Nie Jan. 29, 2018, 7:28 a.m. | #2
2018-01-28 1:45 GMT+08:00 André Przywara <andre.przywara@arm.com>:
> On 27/01/18 15:20, Jun Nie wrote:
>> Support fuse cmd to read/write fuse. Power supply for fuse
>> should be ready, name is VDD_EFUSE in some schematic.
>
> Mmh, in general I am not sure it is a good idea to expose this so easily
> to the user. I guess a clueless user can easily brick his board by
> typing something at the "fuse write" command. I understand that one has
> to manually enable the fuse command first to allow access, but this is
> still quite a high risk, especially since a lot of the fuses are not
> documented.
> Would love to hear opinions from others about that topic.

Yes, it is more or less risky for user that do not have much knowledge on fuse.
Let's see what other developers think.
>
> Also I would have hoped for a bit more documentation.
> How do those banks/words from the write command map to the fuses, for
> instance? What fuses are available and useful? What are the implications
> of writing the secure boot fuse, for instance?
>
> And do we know how access to the fuses is affected by the exception
> level / mode we are in? My understanding is that SID access is secure
> only, which would make it inaccessible for the 64-bit boards where
> U-Boot runs in EL2. But then again at least the A64 does not seem to
> care about this (unless the secure boot fuse is set).
>
> More inline ...
>
>> Signed-off-by: Jun Nie <jun.nie@linaro.org>
>> ---
>>  arch/arm/mach-sunxi/cpu_info.c | 60 ++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 60 insertions(+)
>>
>> diff --git a/arch/arm/mach-sunxi/cpu_info.c b/arch/arm/mach-sunxi/cpu_info.c
>> index 25a5ec2..30bc2bf 100644
>> --- a/arch/arm/mach-sunxi/cpu_info.c
>> +++ b/arch/arm/mach-sunxi/cpu_info.c
>> @@ -133,6 +133,30 @@ uint32_t sun8i_efuse_read(uint32_t offset)
>>       reg_val = readl(SUNXI_SIDC_BASE + SIDC_RDKEY);
>>       return reg_val;
>>  }
>> +
>> +uint32_t sun8i_efuse_write(u32 offset, u32 val)
>> +{
>> +     u32 reg_val;
>> +
>> +     writel(val, SUNXI_SIDC_BASE + SIDC_RDKEY);
>> +
>> +     reg_val = readl(SUNXI_SIDC_BASE + SIDC_PRCTL);
>> +     reg_val &= ~(((0x1ff) << 16) | 0x3);
>> +     reg_val |= (offset << 16);
>
> Would be good to put names to those magic values.
> I understand this is from the code as I sent you ;-), but still ...

The code is from sid_program_key() in this file, but almost identical
with yours. Macro is better than magic number, I can change all these
magic number in next version patch if the patch is needed.
https://github.com/allwinner-zh/bootloader/blob/master/u-boot-2011.09/arch/arm/cpu/armv7/sun8iw6/efuse.c

>
>> +     writel(reg_val, SUNXI_SIDC_BASE + SIDC_PRCTL);
>> +
>> +     reg_val &= ~(((0xff) << 8) | 0x3);
>> +     reg_val |= (SIDC_OP_LOCK << 8) | 0x1;
>> +     writel(reg_val, SUNXI_SIDC_BASE + SIDC_PRCTL);
>> +
>> +     while (readl(SUNXI_SIDC_BASE + SIDC_PRCTL) & 0x1)
>> +             ;
>
> shall we have a timeout or limited retries here?

Yeah.
>
>> +
>> +     reg_val &= ~(((0x1ff) << 16) | ((0xff) << 8) | 0x3);
>> +     writel(reg_val, SUNXI_SIDC_BASE + SIDC_PRCTL);
>> +
>> +     return 0;
>> +}
>>  #endif
>>
>>  int sunxi_get_sid(unsigned int *sid)
>> @@ -164,3 +188,39 @@ int sunxi_get_sid(unsigned int *sid)
>>       return -ENODEV;
>>  #endif
>>  }
>> +
>> +int fuse_read(u32 bank, u32 word, u32 *sid)
>> +{
>> +#ifdef CONFIG_MACH_SUN8I_H3
>> +     *sid = sun8i_efuse_read(word);
>> +#elif defined SUNXI_SID_BASE
>> +     *sid = readl((ulong)SUNXI_SID_BASE + word);
>> +#else
>> +     return -ENODEV;
>> +#endif
>> +     return 0;
>> +}
>> +
>> +int fuse_prog(u32 bank, u32 word, u32 val)
>> +{
>
> I would feel better if we have the write access protected by a separate
> Kconfig symbol. So without this being defined either nothing happens or
> the user gets a warning.

Good idea.
>
>> +#ifdef CONFIG_MACH_SUN8I_H3
>
> I guess this applies to more than the H3, namely the A64 and A83T,
> possibly also H5 and others?

I do not have that much information. So this depends on more developer's
input.
>
>> +     return sun8i_efuse_write(word, val);
>> +#elif defined SUNXI_SID_BASE
>> +     writel(val, (ulong)SUNXI_SID_BASE + word);
>
> Are you sure that works? If I read [1] correctly, you always have to use
> a special algorithm to burn a fuse, a simple MMIO write access would not
> suffice.
> The algorithm seems to be different for older SoCs (pre-H3).

This also depends on other's input. I did not test this due to lack of hardware.
>
>> +#else
>> +     return -ENODEV;
>> +#endif
>> +     return 0;
>> +}
>> +
>> +int fuse_sense(u32 bank, u32 word, u32 *val)
>> +{
>> +     /* We do not support sensing :-( */
>
> Isn't sense the only thing we actually implement? At least this is my
> understanding from reading doc/README.fuse.

Thanks for pointing the doc. I had thought it means accessibility detection.
>
> Cheers,
> Andre.
>
>
>> +     return -EINVAL;
>> +}
>> +
>> +int fuse_override(u32 bank, u32 word, u32 val)
>> +{
>> +     /* We do not support overriding :-( */
>> +     return -EINVAL;
>> +}
>>
>
Calvin Johnson Jan. 29, 2018, 7:41 a.m. | #3
Hi,

> -----Original Message-----

> From: U-Boot [mailto:u-boot-bounces@lists.denx.de] On Behalf Of Jun Nie

> Sent: Monday, January 29, 2018 12:59 PM

> To: André Przywara <andre.przywara@arm.com>

> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Icenowy Zheng

> <icenowy@aosc.xyz>; 370719159@qq.com

> Subject: Re: [U-Boot] [PATCH] sunxi: support fuse cmd to read/write fuse


[snip]

> >> +int fuse_prog(u32 bank, u32 word, u32 val)

> >> +{

> >

> > I would feel better if we have the write access protected by a separate

> > Kconfig symbol. So without this being defined either nothing happens or

> > the user gets a warning.

> 

> Good idea.


Yes, fuse programming should be disabled by default and should be enabled 
only by user who really knows what he is doing.

Also, it will be good to get a confirmation from the user whether user really wants
to program the fuse.

Regards
Calvin
Maxime Ripard Jan. 29, 2018, 10:51 a.m. | #4
On Sat, Jan 27, 2018 at 05:45:45PM +0000, André Przywara wrote:
> On 27/01/18 15:20, Jun Nie wrote:

> > Support fuse cmd to read/write fuse. Power supply for fuse

> > should be ready, name is VDD_EFUSE in some schematic.

> 

> Mmh, in general I am not sure it is a good idea to expose this so easily

> to the user. I guess a clueless user can easily brick his board by

> typing something at the "fuse write" command. I understand that one has

> to manually enable the fuse command first to allow access, but this is

> still quite a high risk, especially since a lot of the fuses are not

> documented.

> Would love to hear opinions from others about that topic.


I agree.

(and Jagan and I should have been in Cc of that patch)

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Patch

diff --git a/arch/arm/mach-sunxi/cpu_info.c b/arch/arm/mach-sunxi/cpu_info.c
index 25a5ec2..30bc2bf 100644
--- a/arch/arm/mach-sunxi/cpu_info.c
+++ b/arch/arm/mach-sunxi/cpu_info.c
@@ -133,6 +133,30 @@  uint32_t sun8i_efuse_read(uint32_t offset)
 	reg_val = readl(SUNXI_SIDC_BASE + SIDC_RDKEY);
 	return reg_val;
 }
+
+uint32_t sun8i_efuse_write(u32 offset, u32 val)
+{
+	u32 reg_val;
+
+	writel(val, SUNXI_SIDC_BASE + SIDC_RDKEY);
+
+	reg_val = readl(SUNXI_SIDC_BASE + SIDC_PRCTL);
+	reg_val &= ~(((0x1ff) << 16) | 0x3);
+	reg_val |= (offset << 16);
+	writel(reg_val, SUNXI_SIDC_BASE + SIDC_PRCTL);
+
+	reg_val &= ~(((0xff) << 8) | 0x3);
+	reg_val |= (SIDC_OP_LOCK << 8) | 0x1;
+	writel(reg_val, SUNXI_SIDC_BASE + SIDC_PRCTL);
+
+	while (readl(SUNXI_SIDC_BASE + SIDC_PRCTL) & 0x1)
+		;
+
+	reg_val &= ~(((0x1ff) << 16) | ((0xff) << 8) | 0x3);
+	writel(reg_val, SUNXI_SIDC_BASE + SIDC_PRCTL);
+
+	return 0;
+}
 #endif
 
 int sunxi_get_sid(unsigned int *sid)
@@ -164,3 +188,39 @@  int sunxi_get_sid(unsigned int *sid)
 	return -ENODEV;
 #endif
 }
+
+int fuse_read(u32 bank, u32 word, u32 *sid)
+{
+#ifdef CONFIG_MACH_SUN8I_H3
+	*sid = sun8i_efuse_read(word);
+#elif defined SUNXI_SID_BASE
+	*sid = readl((ulong)SUNXI_SID_BASE + word);
+#else
+	return -ENODEV;
+#endif
+	return 0;
+}
+
+int fuse_prog(u32 bank, u32 word, u32 val)
+{
+#ifdef CONFIG_MACH_SUN8I_H3
+	return sun8i_efuse_write(word, val);
+#elif defined SUNXI_SID_BASE
+	writel(val, (ulong)SUNXI_SID_BASE + word);
+#else
+	return -ENODEV;
+#endif
+	return 0;
+}
+
+int fuse_sense(u32 bank, u32 word, u32 *val)
+{
+	/* We do not support sensing :-( */
+	return -EINVAL;
+}
+
+int fuse_override(u32 bank, u32 word, u32 val)
+{
+	/* We do not support overriding :-( */
+	return -EINVAL;
+}