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/a...

...

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

+}