| 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 | expand |
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; > +} >
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; >> +} >> >
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
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
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; +}
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(+)