On Mon, Nov 27, 2023 at 7:31 PM Jonas Karlman <jo...@kwiboo.se> wrote: > > Hi Quentin, > > On 2023-11-27 19:42, Quentin Schulz wrote: > > Hi Jonas, > > > > ________________________________________ > > From: U-Boot <u-boot-boun...@lists.denx.de> on behalf of Jonas Karlman > > <jo...@kwiboo.se> > > Sent: 26 November 2023 20:46 > > To: Kever Yang; Simon Glass; Philipp Tomsich > > Cc: Jonas Karlman; u-boot@lists.denx.de > > Subject: [PATCH] rockchip: rk3328: Set efuse auto mode and timing control > > > > Reading from efuse return zero when mainline TF-A is used. > > > > => dump_efuse > > 00000000: 00 00 00 00 .... > > 00000004: 00 00 00 00 .... > > 00000008: 00 00 00 00 .... > > 0000000c: 00 00 00 00 .... > > 00000010: 00 00 00 00 .... > > 00000014: 00 00 00 00 .... > > 00000018: 00 00 00 00 .... > > 0000001c: 00 00 00 00 .... > > > > However, when vendor TF-A blobs is used reading from efuse works. > > > > Change to use auto mode, enable finish and auto access err interrupts > > and set timing control using same values that vendor TF-A blob use to > > fix this. > > > > With this efuse can be read when either of mainline TF-A or vendor blob > > is used. > > > > => dump_efuse > > 00000000: 52 4b 33 82 RK3. > > 00000004: 00 fe 21 55 ..!U > > 00000008: 52 4b 57 34 RKW4 > > 0000000c: 35 30 32 39 5029 > > 00000010: 00 00 00 00 .... > > 00000014: 08 25 0c 0f .%.. > > 00000018: 02 0d 08 00 .... > > 0000001c: 00 00 f0 00 .... > > > > Signed-off-by: Jonas Karlman <jo...@kwiboo.se> > > --- > > arch/arm/mach-rockchip/rk3328/rk3328.c | 34 ++++++++++++++++++++++++++ > > 1 file changed, 34 insertions(+) > > > > diff --git a/arch/arm/mach-rockchip/rk3328/rk3328.c > > b/arch/arm/mach-rockchip/rk3328/rk3328.c > > index de17b8868273..edb22a58fc67 100644 > > --- a/arch/arm/mach-rockchip/rk3328/rk3328.c > > +++ b/arch/arm/mach-rockchip/rk3328/rk3328.c > > @@ -19,6 +19,22 @@ DECLARE_GLOBAL_DATA_PTR; > > #define GRF_BASE 0xFF100000 > > #define UART2_BASE 0xFF130000 > > #define FW_DDR_CON_REG 0xFF7C0040 > > +#define EFUSE_NS_BASE 0xFF260000 > > + > > +#define EFUSE_MOD 0x0000 > > +#define EFUSE_INT_CON 0x0014 > > +#define EFUSE_T_CSB_P 0x0028 > > +#define EFUSE_T_PGENB_P 0x002C > > +#define EFUSE_T_LOAD_P 0x0030 > > +#define EFUSE_T_ADDR_P 0x0034 > > +#define EFUSE_T_STROBE_P 0x0038 > > +#define EFUSE_T_CSB_R 0x003C > > +#define EFUSE_T_PGENB_R 0x0040 > > +#define EFUSE_T_LOAD_R 0x0044 > > +#define EFUSE_T_ADDR_R 0x0048 > > +#define EFUSE_T_STROBE_R 0x004C > > + > > +#define EFUSE_TIMING(s, l) (((s) << 16) | (l)) > > > > const char * const boot_devices[BROM_LAST_BOOTSOURCE + 1] = { > > [BROM_BOOTSOURCE_EMMC] = "/mmc@ff520000", > > @@ -54,6 +70,24 @@ int arch_cpu_init(void) > > > > /* Disable the ddr secure region setting to make it non-secure */ > > rk_setreg(FW_DDR_CON_REG, 0x200); > > + > > + /* Use efuse auto mode */ > > + writel(0x46, EFUSE_NS_BASE + EFUSE_MOD); > > + > > + /* Enable efuse finish and auto access err interrupt */ > > + writel(0x07, EFUSE_NS_BASE + EFUSE_INT_CON); > > + > > + /* Set efuse timing control */ > > + writel(EFUSE_TIMING(1, 241), EFUSE_NS_BASE + EFUSE_T_CSB_P); > > + writel(EFUSE_TIMING(1, 241), EFUSE_NS_BASE + EFUSE_T_PGENB_P); > > + writel(EFUSE_TIMING(1, 241), EFUSE_NS_BASE + EFUSE_T_LOAD_P); > > + writel(EFUSE_TIMING(1, 241), EFUSE_NS_BASE + EFUSE_T_ADDR_P); > > + writel(EFUSE_TIMING(2, 240), EFUSE_NS_BASE + EFUSE_T_STROBE_P); > > + writel(EFUSE_TIMING(1, 4), EFUSE_NS_BASE + EFUSE_T_CSB_R); > > + writel(EFUSE_TIMING(1, 4), EFUSE_NS_BASE + EFUSE_T_PGENB_R); > > + writel(EFUSE_TIMING(1, 4), EFUSE_NS_BASE + EFUSE_T_LOAD_R); > > + writel(EFUSE_TIMING(1, 4), EFUSE_NS_BASE + EFUSE_T_ADDR_R); > > + writel(EFUSE_TIMING(2, 3), EFUSE_NS_BASE + EFUSE_T_STROBE_R); > > > > Shouldn't this be done in the efuse driver instead? > > I was considering the same and was unsure if these values would need to > be initialized at EL3 in SPL or could be initialized at EL1 in U-Boot > proper (in the efuse driver). > > Did not have time to investigate further and decided to just do it at > EL3 same as vendor TF-A, so code ended up here.
Is there a reason not to do it in the upstream TF-A like the vendor one does? I'm sure I've seen patches around that do this when I've dug into it in the past. Is there reasons where TF-A would need access to the information the efuses provide, I'm guessing things like serial numbers etc, given the vendor TF-A does this I suspect that's where the patch should go rather than U-Boot. Peter