[PATCH 0/2] Use ATAGs for RK3588 For RAM Info
From: Chris Morgan macromorgan@hotmail.com
Use the ATAG info provided by the Rockchip binary TPL to identify RAM banks.
This is needed because there are specific addresses that should not be written to for all RK3588 based devices with >=16GB of RAM, writing to these addresses immediately results in a crash.
Chris Morgan (2): rockchip: rk3588: Add support for ATAG parsing rockchip: rk3588: Add Support for RAM Defines from ATAGs
arch/arm/include/asm/arch-rockchip/atags.h | 222 +++++++++++++++++++++ arch/arm/mach-rockchip/Makefile | 1 + arch/arm/mach-rockchip/atags.c | 99 +++++++++ arch/arm/mach-rockchip/sdram.c | 58 ++++++ 4 files changed, 380 insertions(+) create mode 100644 arch/arm/include/asm/arch-rockchip/atags.h create mode 100644 arch/arm/mach-rockchip/atags.c
From: Chris Morgan macromorgan@hotmail.com
Add support for parsing the ATAGs created by the Rockchip binary RAM init. This ATAG parsing code was taken from the Rockchip BSP U-Boot source and tested only on parsing the RAM specific ATAGs for the RK3588.
Signed-off-by: Chris Morgan macromorgan@hotmail.com --- arch/arm/include/asm/arch-rockchip/atags.h | 222 +++++++++++++++++++++ arch/arm/mach-rockchip/Makefile | 1 + arch/arm/mach-rockchip/atags.c | 99 +++++++++ 3 files changed, 322 insertions(+) create mode 100644 arch/arm/include/asm/arch-rockchip/atags.h create mode 100644 arch/arm/mach-rockchip/atags.c
diff --git a/arch/arm/include/asm/arch-rockchip/atags.h b/arch/arm/include/asm/arch-rockchip/atags.h new file mode 100644 index 0000000000..9bae66d7f8 --- /dev/null +++ b/arch/arm/include/asm/arch-rockchip/atags.h @@ -0,0 +1,222 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/* + * (C) Copyright 2018 Rockchip Electronics Co., Ltd + * + */ + +#ifndef __RK_ATAGS_H_ +#define __RK_ATAGS_H_ + +/* Tag magic */ +#define ATAG_CORE 0x54410001 +#define ATAG_NONE 0x00000000 + +#define ATAG_SERIAL 0x54410050 +#define ATAG_BOOTDEV 0x54410051 +#define ATAG_DDR_MEM 0x54410052 +#define ATAG_TOS_MEM 0x54410053 +#define ATAG_RAM_PARTITION 0x54410054 +#define ATAG_ATF_MEM 0x54410055 +#define ATAG_PUB_KEY 0x54410056 +#define ATAG_SOC_INFO 0x54410057 +#define ATAG_BOOT1_PARAM 0x54410058 +#define ATAG_PSTORE 0x54410059 +#define ATAG_FWVER 0x5441005a +#define ATAG_MAX 0x544100ff + +/* Tag size and offset */ +#define ATAGS_SIZE (0x2000) /* 8K */ +#define ATAGS_OFFSET (0x200000 - ATAGS_SIZE)/* [2M-8K, 2M] */ +#define ATAGS_PHYS_BASE (CFG_SYS_SDRAM_BASE + ATAGS_OFFSET) + +/* tag_fwver.ver[fwid][] */ +#define FWVER_LEN 36 + +enum fwid { + FW_DDR, + FW_SPL, + FW_ATF, + FW_TEE, + FW_MAX, +}; + +struct tag_serial { + u32 version; + u32 enable; + u64 addr; + u32 baudrate; + u32 m_mode; + u32 id; + u32 reserved[2]; + u32 hash; +} __packed; + +struct tag_bootdev { + u32 version; + u32 devtype; + u32 devnum; + u32 mode; + u32 sdupdate; + u32 reserved[6]; + u32 hash; +} __packed; + +struct tag_ddr_mem { + u32 count; + u32 version; + u64 bank[20]; + u32 flags; + u32 data[2]; + u32 hash; +} __packed; + +struct tag_tos_mem { + u32 version; + struct { + char name[8]; + u64 phy_addr; + u32 size; + u32 flags; + } tee_mem; + + struct { + char name[8]; + u64 phy_addr; + u32 size; + u32 flags; + } drm_mem; + + u64 reserved[7]; + u32 reserved1; + u32 hash; +} __packed; + +struct tag_atf_mem { + u32 version; + u64 phy_addr; + u32 size; + u32 flags; + u32 reserved[2]; + u32 hash; +} __packed; + +struct tag_pub_key { + u32 version; + u32 len; + u8 data[768]; /* u32 rsa_n[64], rsa_e[64], rsa_c[64] */ + u32 flag; + u32 reserved[5]; + u32 hash; +} __packed; + +struct tag_ram_partition { + u32 version; + u32 count; + u32 reserved[4]; + + struct { + char name[16]; + u64 start; + u64 size; + } part[6]; + + u32 reserved1[3]; + u32 hash; +} __packed; + +struct tag_soc_info { + u32 version; + u32 name; /* Hex: 0x3288, 0x3399... */ + u32 flags; + u32 reserved[6]; + u32 hash; +} __packed; + +struct tag_boot1p { + u32 version; + u32 param[8]; + u32 reserved[4]; + u32 hash; +} __packed; + +struct tag_pstore { + u32 version; + struct { + u32 addr; + u32 size; + } buf[16]; + u32 hash; +} __packed; + +struct tag_fwver { + u32 version; + char ver[8][FWVER_LEN]; + u32 hash; +} __packed; + +struct tag_core { + u32 flags; + u32 pagesize; + u32 rootdev; +} __packed; + +struct tag_header { + u32 size; /* bytes = size * 4 */ + u32 magic; +} __packed; + +/* Must be 4 bytes align */ +struct tag { + struct tag_header hdr; + union { + struct tag_core core; + struct tag_serial serial; + struct tag_bootdev bootdev; + struct tag_ddr_mem ddr_mem; + struct tag_tos_mem tos_mem; + struct tag_ram_partition ram_part; + struct tag_atf_mem atf_mem; + struct tag_pub_key pub_key; + struct tag_soc_info soc; + struct tag_boot1p boot1p; + struct tag_pstore pstore; + struct tag_fwver fwver; + } u; +} __aligned(4); + +#define tag_next(t) ((struct tag *)((u32 *)(t) + (t)->hdr.size)) +#define tag_size(type) ((sizeof(struct tag_header) + sizeof(struct type)) >> 2) +#define for_each_tag(t, base) \ + for (t = base; t->hdr.size; t = tag_next(t)) + +/* + * atags_get_tag - get tag by tag magic + * + * @magic: tag magic, i.e. ATAG_SERIAL, ATAG_BOOTDEV, ... + * + * return: NULL on failed, otherwise return the tag that we want. + */ +struct tag *atags_get_tag(u32 magic); + +/* + * atags_is_available - check if atags is available, used for second or + * later pre-loaders. + * + * return: 0 is not available, otherwise available. + */ +int atags_is_available(void); + +/* + * atags_overflow - check if atags is overflow + * + * return: 0 if not overflow, otherwise overflow. + */ +int atags_overflow(struct tag *t); + +/* + * atags_bad_magic - check if atags magic is invalid. + * + * return: 1 if invalid, otherwise valid. + */ +int atags_bad_magic(u32 magic); +#endif diff --git a/arch/arm/mach-rockchip/Makefile b/arch/arm/mach-rockchip/Makefile index 1dc92066bb..4165cbe99f 100644 --- a/arch/arm/mach-rockchip/Makefile +++ b/arch/arm/mach-rockchip/Makefile @@ -28,6 +28,7 @@ endif
ifeq ($(CONFIG_TPL_BUILD),) obj-$(CONFIG_DISPLAY_CPUINFO) += cpu-info.o +obj-$(CONFIG_ROCKCHIP_RK3588) += atags.o endif
obj-$(CONFIG_$(SPL_TPL_)RAM) += sdram.o diff --git a/arch/arm/mach-rockchip/atags.c b/arch/arm/mach-rockchip/atags.c new file mode 100644 index 0000000000..9daa2f2fc0 --- /dev/null +++ b/arch/arm/mach-rockchip/atags.c @@ -0,0 +1,99 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * (C) Copyright 2018 Rockchip Electronics Co., Ltd. + * + */ + +#include <config.h> +#include <asm/io.h> +#include <asm/arch-rockchip/atags.h> + +#define HASH_LEN sizeof(u32) + +static u32 js_hash(void *buf, u32 len) +{ + u32 i, hash = 0x47C6A7E6; + char *data = buf; + + if (!buf || !len) + return hash; + + for (i = 0; i < len; i++) + hash ^= ((hash << 5) + data[i] + (hash >> 2)); + + return hash; +} + +int atags_bad_magic(u32 magic) +{ + bool bad; + + bad = ((magic != ATAG_CORE) && + (magic != ATAG_NONE) && + (magic < ATAG_SERIAL || magic > ATAG_MAX)); + if (bad) + printf("Magic(%x) is not support\n", magic); + + return bad; +} + +static int atags_size_overflow(struct tag *t, u32 tag_size) +{ + return (unsigned long)t + (tag_size << 2) - ATAGS_PHYS_BASE > ATAGS_SIZE; +} + +int atags_overflow(struct tag *t) +{ + bool overflow; + + overflow = atags_size_overflow(t, 0) || + atags_size_overflow(t, t->hdr.size); + if (overflow) + printf("Tag is overflow\n"); + + return overflow; +} + +int atags_is_available(void) +{ + struct tag *t = (struct tag *)ATAGS_PHYS_BASE; + + return (t->hdr.magic == ATAG_CORE); +} + +struct tag *atags_get_tag(u32 magic) +{ + u32 *hash, calc_hash, size; + struct tag *t; + + if (!atags_is_available()) + return NULL; + + for_each_tag(t, (struct tag *)ATAGS_PHYS_BASE) { + if (atags_overflow(t)) + return NULL; + + if (atags_bad_magic(t->hdr.magic)) + return NULL; + + if (t->hdr.magic != magic) + continue; + + size = t->hdr.size; + hash = (u32 *)((ulong)t + (size << 2) - HASH_LEN); + if (!*hash) { + debug("No hash, magic(%x)\n", magic); + return t; + } + calc_hash = js_hash(t, (size << 2) - HASH_LEN); + if (calc_hash == *hash) { + debug("Hash okay, magic(%x)\n", magic); + return t; + } + debug("Hash bad, magic(%x), orgHash=%x, nowHash=%x\n", + magic, *hash, calc_hash); + return NULL; + } + + return NULL; +}
Hi Chris,
On 3/26/24 21:49, Chris Morgan wrote:
From: Chris Morgan macromorgan@hotmail.com
Add support for parsing the ATAGs created by the Rockchip binary RAM init. This ATAG parsing code was taken from the Rockchip BSP U-Boot source and tested only on parsing the RAM specific ATAGs for the RK3588.
Can you tell us from which commit (and maybe branch/tag in the event they rename/rebase/delete branches) exactly so we can check if they fix stuff downstream every now and then?
Signed-off-by: Chris Morgan macromorgan@hotmail.com
arch/arm/include/asm/arch-rockchip/atags.h | 222 +++++++++++++++++++++ arch/arm/mach-rockchip/Makefile | 1 + arch/arm/mach-rockchip/atags.c | 99 +++++++++ 3 files changed, 322 insertions(+) create mode 100644 arch/arm/include/asm/arch-rockchip/atags.h create mode 100644 arch/arm/mach-rockchip/atags.c
diff --git a/arch/arm/include/asm/arch-rockchip/atags.h b/arch/arm/include/asm/arch-rockchip/atags.h new file mode 100644 index 0000000000..9bae66d7f8 --- /dev/null +++ b/arch/arm/include/asm/arch-rockchip/atags.h @@ -0,0 +1,222 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*
- (C) Copyright 2018 Rockchip Electronics Co., Ltd
- */
+#ifndef __RK_ATAGS_H_ +#define __RK_ATAGS_H_
+/* Tag magic */ +#define ATAG_CORE 0x54410001 +#define ATAG_NONE 0x00000000
+#define ATAG_SERIAL 0x54410050 +#define ATAG_BOOTDEV 0x54410051 +#define ATAG_DDR_MEM 0x54410052 +#define ATAG_TOS_MEM 0x54410053 +#define ATAG_RAM_PARTITION 0x54410054 +#define ATAG_ATF_MEM 0x54410055 +#define ATAG_PUB_KEY 0x54410056 +#define ATAG_SOC_INFO 0x54410057 +#define ATAG_BOOT1_PARAM 0x54410058 +#define ATAG_PSTORE 0x54410059 +#define ATAG_FWVER 0x5441005a +#define ATAG_MAX 0x544100ff
+/* Tag size and offset */ +#define ATAGS_SIZE (0x2000) /* 8K */ +#define ATAGS_OFFSET (0x200000 - ATAGS_SIZE)/* [2M-8K, 2M] */ +#define ATAGS_PHYS_BASE (CFG_SYS_SDRAM_BASE + ATAGS_OFFSET)
+/* tag_fwver.ver[fwid][] */ +#define FWVER_LEN 36
+enum fwid {
- FW_DDR,
- FW_SPL,
- FW_ATF,
- FW_TEE,
- FW_MAX,
+};
+struct tag_serial {
- u32 version;
- u32 enable;
- u64 addr;
- u32 baudrate;
- u32 m_mode;
- u32 id;
- u32 reserved[2];
- u32 hash;
+} __packed;
+struct tag_bootdev {
- u32 version;
- u32 devtype;
- u32 devnum;
- u32 mode;
- u32 sdupdate;
- u32 reserved[6];
- u32 hash;
+} __packed;
+struct tag_ddr_mem {
- u32 count;
- u32 version;
- u64 bank[20];
- u32 flags;
- u32 data[2];
- u32 hash;
+} __packed;
+struct tag_tos_mem {
- u32 version;
- struct {
char name[8];u64 phy_addr;u32 size;u32 flags;- } tee_mem;
- struct {
char name[8];u64 phy_addr;u32 size;u32 flags;- } drm_mem;
- u64 reserved[7];
- u32 reserved1;
- u32 hash;
+} __packed;
+struct tag_atf_mem {
- u32 version;
- u64 phy_addr;
- u32 size;
- u32 flags;
- u32 reserved[2];
- u32 hash;
+} __packed;
+struct tag_pub_key {
- u32 version;
- u32 len;
- u8 data[768]; /* u32 rsa_n[64], rsa_e[64], rsa_c[64] */
- u32 flag;
- u32 reserved[5];
- u32 hash;
+} __packed;
+struct tag_ram_partition {
- u32 version;
- u32 count;
- u32 reserved[4];
- struct {
char name[16];u64 start;u64 size;- } part[6];
- u32 reserved1[3];
- u32 hash;
+} __packed;
+struct tag_soc_info {
- u32 version;
- u32 name; /* Hex: 0x3288, 0x3399... */
- u32 flags;
- u32 reserved[6];
- u32 hash;
+} __packed;
+struct tag_boot1p {
- u32 version;
- u32 param[8];
- u32 reserved[4];
- u32 hash;
+} __packed;
+struct tag_pstore {
- u32 version;
- struct {
u32 addr;u32 size;- } buf[16];
- u32 hash;
+} __packed;
+struct tag_fwver {
- u32 version;
- char ver[8][FWVER_LEN];
- u32 hash;
+} __packed;
+struct tag_core {
- u32 flags;
- u32 pagesize;
- u32 rootdev;
+} __packed;
+struct tag_header {
- u32 size; /* bytes = size * 4 */
- u32 magic;
+} __packed;
+/* Must be 4 bytes align */ +struct tag {
- struct tag_header hdr;
- union {
struct tag_core core;struct tag_serial serial;struct tag_bootdev bootdev;struct tag_ddr_mem ddr_mem;struct tag_tos_mem tos_mem;struct tag_ram_partition ram_part;struct tag_atf_mem atf_mem;struct tag_pub_key pub_key;struct tag_soc_info soc;struct tag_boot1p boot1p;struct tag_pstore pstore;struct tag_fwver fwver;- } u;
+} __aligned(4);
+#define tag_next(t) ((struct tag *)((u32 *)(t) + (t)->hdr.size)) +#define tag_size(type) ((sizeof(struct tag_header) + sizeof(struct type)) >> 2) +#define for_each_tag(t, base) \
- for (t = base; t->hdr.size; t = tag_next(t))
+/*
- atags_get_tag - get tag by tag magic
- @magic: tag magic, i.e. ATAG_SERIAL, ATAG_BOOTDEV, ...
- return: NULL on failed, otherwise return the tag that we want.
- */
+struct tag *atags_get_tag(u32 magic);
+/*
- atags_is_available - check if atags is available, used for second or
later pre-loaders.
- return: 0 is not available, otherwise available.
- */
+int atags_is_available(void);
+/*
- atags_overflow - check if atags is overflow
This is not really useful to the user to know what this is doing.
I could suggest this rewording:
""" check if the tag t is a valid atag: entirely contained in the ATAGS physical address space [ATAGS_PHYS_BASE; ATAGS_PHYS_BASE + ATAGS_SIZE] """
- return: 0 if not overflow, otherwise overflow.
- */
+int atags_overflow(struct tag *t);
+/*
- atags_bad_magic - check if atags magic is invalid.
- return: 1 if invalid, otherwise valid.
- */
+int atags_bad_magic(u32 magic); +#endif diff --git a/arch/arm/mach-rockchip/Makefile b/arch/arm/mach-rockchip/Makefile index 1dc92066bb..4165cbe99f 100644 --- a/arch/arm/mach-rockchip/Makefile +++ b/arch/arm/mach-rockchip/Makefile @@ -28,6 +28,7 @@ endif
ifeq ($(CONFIG_TPL_BUILD),) obj-$(CONFIG_DISPLAY_CPUINFO) += cpu-info.o +obj-$(CONFIG_ROCKCHIP_RK3588) += atags.o
Please define a new symbol instead, you can "imply" it for RK35xx boards in a follow-up patch then.
Now to come to think of it... Maybe we should make ROCKCHIP_EXTERNAL_TPL imply ROCKCHIP_ATAGS? That would be the most sensible thing to do I believe?
I think we should also probably make different symbols for SPL and U-Boot proper (if we want to support the later). SPL would be "required" for any platform with a blob from Rockchip as TPL but U-Boot proper support only makes sense for debugging (e.g. if there's an atags command in U-Boot to dump them).
endif
obj-$(CONFIG_$(SPL_TPL_)RAM) += sdram.o diff --git a/arch/arm/mach-rockchip/atags.c b/arch/arm/mach-rockchip/atags.c new file mode 100644 index 0000000000..9daa2f2fc0 --- /dev/null +++ b/arch/arm/mach-rockchip/atags.c @@ -0,0 +1,99 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*
- (C) Copyright 2018 Rockchip Electronics Co., Ltd.
- */
+#include <config.h>
If I remember correctly, Tom has been hunting those config.h includes down, so better remove it if everything works as well :)
+#include <asm/io.h> +#include <asm/arch-rockchip/atags.h>
+#define HASH_LEN sizeof(u32)
+static u32 js_hash(void *buf, u32 len) +{
- u32 i, hash = 0x47C6A7E6;
- char *data = buf;
- if (!buf || !len)
return hash;- for (i = 0; i < len; i++)
hash ^= ((hash << 5) + data[i] + (hash >> 2));- return hash;
+}
+int atags_bad_magic(u32 magic) +{
- bool bad;
- bad = ((magic != ATAG_CORE) &&
(magic != ATAG_NONE) &&(magic < ATAG_SERIAL || magic > ATAG_MAX));- if (bad)
printf("Magic(%x) is not support\n", magic);
""" switch (magic) { case ATAG_NONE: case ATAG_CORE: case ATAG_SERIAL ... ATAG_MAX: return false; default: printf("Magic(%x) is not supported\n", magic); return true; } """
may be a tiny bit more readable but that's a matter of taste, so up to you.
Otherwise: - The error message has a typo: should be "not supported". - I would recommend to use 0x%x so that it highlights it is a hexadecimal value. - Use pr_err() or debug() instead of printf(), this applies to all printf in this patch. I **think**, debug() may be more warranted.
- return bad;
+}
+static int atags_size_overflow(struct tag *t, u32 tag_size) +{
- return (unsigned long)t + (tag_size << 2) - ATAGS_PHYS_BASE > ATAGS_SIZE;
""" tag_size << 2 """
may overflow itself, should we cast this into a u64 before the shift?
Similarly, unsigned long being 32b,
""" (unsigned long)t + (tag_size << 2) """
32b + 32b may overflow a 32b container.
Actually, maybe... we should be using phys_addr_t for this? It's an unsigned long long so 64b container for 64b archs and it represents what this is... a physical address?
Also, this can technically underflow as well, if (unsigned long)t + (tag_size << 2) is smaller than ATAGS_PHYS_BASE.
+}
+int atags_overflow(struct tag *t) +{
- bool overflow;
- overflow = atags_size_overflow(t, 0) ||
atags_size_overflow(t, t->hdr.size);
I don't really understand why we need to check for size 0? Do you have an idea what Rockchip wanted to do here?
- if (overflow)
printf("Tag is overflow\n");
- return overflow;
+}
+int atags_is_available(void) +{
- struct tag *t = (struct tag *)ATAGS_PHYS_BASE;
- return (t->hdr.magic == ATAG_CORE);
+}
+struct tag *atags_get_tag(u32 magic) +{
- u32 *hash, calc_hash, size;
- struct tag *t;
- if (!atags_is_available())
return NULL;- for_each_tag(t, (struct tag *)ATAGS_PHYS_BASE) {
if (atags_overflow(t))return NULL;if (atags_bad_magic(t->hdr.magic))return NULL;if (t->hdr.magic != magic)continue;size = t->hdr.size;hash = (u32 *)((ulong)t + (size << 2) - HASH_LEN);
Same issue of possible overflows as in atags_size_overflow().
if (!*hash) {
I assume this is "safe" because we check this is a "valid" physical address within the expected boundaries of ATAGS address space with `atags_overflow()`.
debug("No hash, magic(%x)\n", magic);return t;}calc_hash = js_hash(t, (size << 2) - HASH_LEN);
Ditto.
Cheers, Quentin
Hi Chris,
The ATAGS is used for passing parameter from bootloader to kernel at first, which has been replaced by DTB now for ARM platform.
And Rockchip using ATAGs for passing parameter like dram memory size/board uart in different boot process like DRAM init binary/ TPL/SPL to U-Boot since 2018.
And almost at the same time, Simon add bloblist for mainline U-Boot which for similar purpose.
So I'm not sure if this ATAGS should be accept in mainline U-Boot or not, even for rockchip platform only seems some kind of regression for this feature support.
Hi Simon and Tom,
Could you help to give some suggestion for this?
Thanks, - Kever On 2024/3/27 04:49, Chris Morgan wrote:
From: Chris Morgan macromorgan@hotmail.com
Add support for parsing the ATAGs created by the Rockchip binary RAM init. This ATAG parsing code was taken from the Rockchip BSP U-Boot source and tested only on parsing the RAM specific ATAGs for the RK3588.
Signed-off-by: Chris Morgan macromorgan@hotmail.com
arch/arm/include/asm/arch-rockchip/atags.h | 222 +++++++++++++++++++++ arch/arm/mach-rockchip/Makefile | 1 + arch/arm/mach-rockchip/atags.c | 99 +++++++++ 3 files changed, 322 insertions(+) create mode 100644 arch/arm/include/asm/arch-rockchip/atags.h create mode 100644 arch/arm/mach-rockchip/atags.c
diff --git a/arch/arm/include/asm/arch-rockchip/atags.h b/arch/arm/include/asm/arch-rockchip/atags.h new file mode 100644 index 0000000000..9bae66d7f8 --- /dev/null +++ b/arch/arm/include/asm/arch-rockchip/atags.h @@ -0,0 +1,222 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*
- (C) Copyright 2018 Rockchip Electronics Co., Ltd
- */
+#ifndef __RK_ATAGS_H_ +#define __RK_ATAGS_H_
+/* Tag magic */ +#define ATAG_CORE 0x54410001 +#define ATAG_NONE 0x00000000
+#define ATAG_SERIAL 0x54410050 +#define ATAG_BOOTDEV 0x54410051 +#define ATAG_DDR_MEM 0x54410052 +#define ATAG_TOS_MEM 0x54410053 +#define ATAG_RAM_PARTITION 0x54410054 +#define ATAG_ATF_MEM 0x54410055 +#define ATAG_PUB_KEY 0x54410056 +#define ATAG_SOC_INFO 0x54410057 +#define ATAG_BOOT1_PARAM 0x54410058 +#define ATAG_PSTORE 0x54410059 +#define ATAG_FWVER 0x5441005a +#define ATAG_MAX 0x544100ff
+/* Tag size and offset */ +#define ATAGS_SIZE (0x2000) /* 8K */ +#define ATAGS_OFFSET (0x200000 - ATAGS_SIZE)/* [2M-8K, 2M] */ +#define ATAGS_PHYS_BASE (CFG_SYS_SDRAM_BASE + ATAGS_OFFSET)
+/* tag_fwver.ver[fwid][] */ +#define FWVER_LEN 36
+enum fwid {
- FW_DDR,
- FW_SPL,
- FW_ATF,
- FW_TEE,
- FW_MAX,
+};
+struct tag_serial {
- u32 version;
- u32 enable;
- u64 addr;
- u32 baudrate;
- u32 m_mode;
- u32 id;
- u32 reserved[2];
- u32 hash;
+} __packed;
+struct tag_bootdev {
- u32 version;
- u32 devtype;
- u32 devnum;
- u32 mode;
- u32 sdupdate;
- u32 reserved[6];
- u32 hash;
+} __packed;
+struct tag_ddr_mem {
- u32 count;
- u32 version;
- u64 bank[20];
- u32 flags;
- u32 data[2];
- u32 hash;
+} __packed;
+struct tag_tos_mem {
- u32 version;
- struct {
char name[8];u64 phy_addr;u32 size;u32 flags;- } tee_mem;
- struct {
char name[8];u64 phy_addr;u32 size;u32 flags;- } drm_mem;
- u64 reserved[7];
- u32 reserved1;
- u32 hash;
+} __packed;
+struct tag_atf_mem {
- u32 version;
- u64 phy_addr;
- u32 size;
- u32 flags;
- u32 reserved[2];
- u32 hash;
+} __packed;
+struct tag_pub_key {
- u32 version;
- u32 len;
- u8 data[768]; /* u32 rsa_n[64], rsa_e[64], rsa_c[64] */
- u32 flag;
- u32 reserved[5];
- u32 hash;
+} __packed;
+struct tag_ram_partition {
- u32 version;
- u32 count;
- u32 reserved[4];
- struct {
char name[16];u64 start;u64 size;- } part[6];
- u32 reserved1[3];
- u32 hash;
+} __packed;
+struct tag_soc_info {
- u32 version;
- u32 name; /* Hex: 0x3288, 0x3399... */
- u32 flags;
- u32 reserved[6];
- u32 hash;
+} __packed;
+struct tag_boot1p {
- u32 version;
- u32 param[8];
- u32 reserved[4];
- u32 hash;
+} __packed;
+struct tag_pstore {
- u32 version;
- struct {
u32 addr;u32 size;- } buf[16];
- u32 hash;
+} __packed;
+struct tag_fwver {
- u32 version;
- char ver[8][FWVER_LEN];
- u32 hash;
+} __packed;
+struct tag_core {
- u32 flags;
- u32 pagesize;
- u32 rootdev;
+} __packed;
+struct tag_header {
- u32 size; /* bytes = size * 4 */
- u32 magic;
+} __packed;
+/* Must be 4 bytes align */ +struct tag {
- struct tag_header hdr;
- union {
struct tag_core core;struct tag_serial serial;struct tag_bootdev bootdev;struct tag_ddr_mem ddr_mem;struct tag_tos_mem tos_mem;struct tag_ram_partition ram_part;struct tag_atf_mem atf_mem;struct tag_pub_key pub_key;struct tag_soc_info soc;struct tag_boot1p boot1p;struct tag_pstore pstore;struct tag_fwver fwver;- } u;
+} __aligned(4);
+#define tag_next(t) ((struct tag *)((u32 *)(t) + (t)->hdr.size)) +#define tag_size(type) ((sizeof(struct tag_header) + sizeof(struct type)) >> 2) +#define for_each_tag(t, base) \
- for (t = base; t->hdr.size; t = tag_next(t))
+/*
- atags_get_tag - get tag by tag magic
- @magic: tag magic, i.e. ATAG_SERIAL, ATAG_BOOTDEV, ...
- return: NULL on failed, otherwise return the tag that we want.
- */
+struct tag *atags_get_tag(u32 magic);
+/*
- atags_is_available - check if atags is available, used for second or
later pre-loaders.
- return: 0 is not available, otherwise available.
- */
+int atags_is_available(void);
+/*
- atags_overflow - check if atags is overflow
- return: 0 if not overflow, otherwise overflow.
- */
+int atags_overflow(struct tag *t);
+/*
- atags_bad_magic - check if atags magic is invalid.
- return: 1 if invalid, otherwise valid.
- */
+int atags_bad_magic(u32 magic); +#endif diff --git a/arch/arm/mach-rockchip/Makefile b/arch/arm/mach-rockchip/Makefile index 1dc92066bb..4165cbe99f 100644 --- a/arch/arm/mach-rockchip/Makefile +++ b/arch/arm/mach-rockchip/Makefile @@ -28,6 +28,7 @@ endif
ifeq ($(CONFIG_TPL_BUILD),) obj-$(CONFIG_DISPLAY_CPUINFO) += cpu-info.o +obj-$(CONFIG_ROCKCHIP_RK3588) += atags.o endif
obj-$(CONFIG_$(SPL_TPL_)RAM) += sdram.o diff --git a/arch/arm/mach-rockchip/atags.c b/arch/arm/mach-rockchip/atags.c new file mode 100644 index 0000000000..9daa2f2fc0 --- /dev/null +++ b/arch/arm/mach-rockchip/atags.c @@ -0,0 +1,99 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*
- (C) Copyright 2018 Rockchip Electronics Co., Ltd.
- */
+#include <config.h> +#include <asm/io.h> +#include <asm/arch-rockchip/atags.h>
+#define HASH_LEN sizeof(u32)
+static u32 js_hash(void *buf, u32 len) +{
- u32 i, hash = 0x47C6A7E6;
- char *data = buf;
- if (!buf || !len)
return hash;- for (i = 0; i < len; i++)
hash ^= ((hash << 5) + data[i] + (hash >> 2));- return hash;
+}
+int atags_bad_magic(u32 magic) +{
- bool bad;
- bad = ((magic != ATAG_CORE) &&
(magic != ATAG_NONE) &&(magic < ATAG_SERIAL || magic > ATAG_MAX));- if (bad)
printf("Magic(%x) is not support\n", magic);- return bad;
+}
+static int atags_size_overflow(struct tag *t, u32 tag_size) +{
- return (unsigned long)t + (tag_size << 2) - ATAGS_PHYS_BASE > ATAGS_SIZE;
+}
+int atags_overflow(struct tag *t) +{
- bool overflow;
- overflow = atags_size_overflow(t, 0) ||
atags_size_overflow(t, t->hdr.size);- if (overflow)
printf("Tag is overflow\n");- return overflow;
+}
+int atags_is_available(void) +{
- struct tag *t = (struct tag *)ATAGS_PHYS_BASE;
- return (t->hdr.magic == ATAG_CORE);
+}
+struct tag *atags_get_tag(u32 magic) +{
- u32 *hash, calc_hash, size;
- struct tag *t;
- if (!atags_is_available())
return NULL;- for_each_tag(t, (struct tag *)ATAGS_PHYS_BASE) {
if (atags_overflow(t))return NULL;if (atags_bad_magic(t->hdr.magic))return NULL;if (t->hdr.magic != magic)continue;size = t->hdr.size;hash = (u32 *)((ulong)t + (size << 2) - HASH_LEN);if (!*hash) {debug("No hash, magic(%x)\n", magic);return t;}calc_hash = js_hash(t, (size << 2) - HASH_LEN);if (calc_hash == *hash) {debug("Hash okay, magic(%x)\n", magic);return t;}debug("Hash bad, magic(%x), orgHash=%x, nowHash=%x\n",magic, *hash, calc_hash);return NULL;- }
- return NULL;
+}
On Wed, Mar 27, 2024 at 06:32:06PM +0800, Kever Yang wrote:
Hi Chris,
The ATAGS is used for passing parameter from bootloader to kernel at first, which has been replaced by DTB now for ARM platform.
And Rockchip using ATAGs for passing parameter like dram memory size/board uart in different boot process like DRAM init binary/ TPL/SPL to U-Boot since 2018.
And almost at the same time, Simon add bloblist for mainline U-Boot which for similar purpose.
So I'm not sure if this ATAGS should be accept in mainline U-Boot or not, even for rockchip platform only seems some kind of regression for this feature support.
Hi Simon and Tom,
Could you help to give some suggestion for this?
This should be using bloblist, yes, as I understand it.
On Wed, Mar 27, 2024 at 06:32:06PM +0800, Kever Yang wrote:
Hi Chris,
The ATAGS is used for passing parameter from bootloader to kernel at first, which has been replaced by DTB now for ARM platform.
And Rockchip using ATAGs for passing parameter like dram memory size/board uart in different boot process like DRAM init binary/ TPL/SPL to U-Boot since 2018.
And almost at the same time, Simon add bloblist for mainline U-Boot which for similar purpose.
So I'm not sure if this ATAGS should be accept in mainline U-Boot or not, even for rockchip platform only seems some kind of regression for this feature support.
Hi Simon and Tom,
Could you help to give some suggestion for this?
I really meant to do this as an "RFC", so I apologize in advance for possibly causing more work in treating this as a full-fledged patch.
The problem I'm trying to solve is that I've got 2 boards, a Rock 5B as well as an Indiedroid Nova both with 16GB of RAM. I noticed that without the memory holes the Rock 5B defined in my Indiedroid it would also fail to boot. I've got 2 other boards as well with less than 16GB of RAM which seem to work fine without any holes (a 4GB Indiedroid Nova prototype and a GameForce Ace with 12GB of RAM).
The "RFC" part for which I'm really requesting guidance/comments is a question of "is it appropriate to use ATAGS" to get the RAM banks on this SoC, or is there a different way we should be doing it? If we can/should use the ATAGS, then I guess this can be pared down and refactored to just be RK3588 specific. If so, we can possibly add something like this to the RK3588 SoC specific code, guard it with an #ifdef ROCKCHIP_TPL to only call it when using the Rockchip specific RAM init (in the hopes that maybe one day we get our own RAM init), and then replace existing code for boards like the Rock 5B so that it no longer reserves these memory banks.
Thank you, Chris.
Thanks,
- Kever
On 2024/3/27 04:49, Chris Morgan wrote:
From: Chris Morgan macromorgan@hotmail.com
Add support for parsing the ATAGs created by the Rockchip binary RAM init. This ATAG parsing code was taken from the Rockchip BSP U-Boot source and tested only on parsing the RAM specific ATAGs for the RK3588.
Signed-off-by: Chris Morgan macromorgan@hotmail.com
arch/arm/include/asm/arch-rockchip/atags.h | 222 +++++++++++++++++++++ arch/arm/mach-rockchip/Makefile | 1 + arch/arm/mach-rockchip/atags.c | 99 +++++++++ 3 files changed, 322 insertions(+) create mode 100644 arch/arm/include/asm/arch-rockchip/atags.h create mode 100644 arch/arm/mach-rockchip/atags.c
diff --git a/arch/arm/include/asm/arch-rockchip/atags.h b/arch/arm/include/asm/arch-rockchip/atags.h new file mode 100644 index 0000000000..9bae66d7f8 --- /dev/null +++ b/arch/arm/include/asm/arch-rockchip/atags.h @@ -0,0 +1,222 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*
- (C) Copyright 2018 Rockchip Electronics Co., Ltd
- */
+#ifndef __RK_ATAGS_H_ +#define __RK_ATAGS_H_
+/* Tag magic */ +#define ATAG_CORE 0x54410001 +#define ATAG_NONE 0x00000000
+#define ATAG_SERIAL 0x54410050 +#define ATAG_BOOTDEV 0x54410051 +#define ATAG_DDR_MEM 0x54410052 +#define ATAG_TOS_MEM 0x54410053 +#define ATAG_RAM_PARTITION 0x54410054 +#define ATAG_ATF_MEM 0x54410055 +#define ATAG_PUB_KEY 0x54410056 +#define ATAG_SOC_INFO 0x54410057 +#define ATAG_BOOT1_PARAM 0x54410058 +#define ATAG_PSTORE 0x54410059 +#define ATAG_FWVER 0x5441005a +#define ATAG_MAX 0x544100ff
+/* Tag size and offset */ +#define ATAGS_SIZE (0x2000) /* 8K */ +#define ATAGS_OFFSET (0x200000 - ATAGS_SIZE)/* [2M-8K, 2M] */ +#define ATAGS_PHYS_BASE (CFG_SYS_SDRAM_BASE + ATAGS_OFFSET)
+/* tag_fwver.ver[fwid][] */ +#define FWVER_LEN 36
+enum fwid {
- FW_DDR,
- FW_SPL,
- FW_ATF,
- FW_TEE,
- FW_MAX,
+};
+struct tag_serial {
- u32 version;
- u32 enable;
- u64 addr;
- u32 baudrate;
- u32 m_mode;
- u32 id;
- u32 reserved[2];
- u32 hash;
+} __packed;
+struct tag_bootdev {
- u32 version;
- u32 devtype;
- u32 devnum;
- u32 mode;
- u32 sdupdate;
- u32 reserved[6];
- u32 hash;
+} __packed;
+struct tag_ddr_mem {
- u32 count;
- u32 version;
- u64 bank[20];
- u32 flags;
- u32 data[2];
- u32 hash;
+} __packed;
+struct tag_tos_mem {
- u32 version;
- struct {
char name[8];u64 phy_addr;u32 size;u32 flags;- } tee_mem;
- struct {
char name[8];u64 phy_addr;u32 size;u32 flags;- } drm_mem;
- u64 reserved[7];
- u32 reserved1;
- u32 hash;
+} __packed;
+struct tag_atf_mem {
- u32 version;
- u64 phy_addr;
- u32 size;
- u32 flags;
- u32 reserved[2];
- u32 hash;
+} __packed;
+struct tag_pub_key {
- u32 version;
- u32 len;
- u8 data[768]; /* u32 rsa_n[64], rsa_e[64], rsa_c[64] */
- u32 flag;
- u32 reserved[5];
- u32 hash;
+} __packed;
+struct tag_ram_partition {
- u32 version;
- u32 count;
- u32 reserved[4];
- struct {
char name[16];u64 start;u64 size;- } part[6];
- u32 reserved1[3];
- u32 hash;
+} __packed;
+struct tag_soc_info {
- u32 version;
- u32 name; /* Hex: 0x3288, 0x3399... */
- u32 flags;
- u32 reserved[6];
- u32 hash;
+} __packed;
+struct tag_boot1p {
- u32 version;
- u32 param[8];
- u32 reserved[4];
- u32 hash;
+} __packed;
+struct tag_pstore {
- u32 version;
- struct {
u32 addr;u32 size;- } buf[16];
- u32 hash;
+} __packed;
+struct tag_fwver {
- u32 version;
- char ver[8][FWVER_LEN];
- u32 hash;
+} __packed;
+struct tag_core {
- u32 flags;
- u32 pagesize;
- u32 rootdev;
+} __packed;
+struct tag_header {
- u32 size; /* bytes = size * 4 */
- u32 magic;
+} __packed;
+/* Must be 4 bytes align */ +struct tag {
- struct tag_header hdr;
- union {
struct tag_core core;struct tag_serial serial;struct tag_bootdev bootdev;struct tag_ddr_mem ddr_mem;struct tag_tos_mem tos_mem;struct tag_ram_partition ram_part;struct tag_atf_mem atf_mem;struct tag_pub_key pub_key;struct tag_soc_info soc;struct tag_boot1p boot1p;struct tag_pstore pstore;struct tag_fwver fwver;- } u;
+} __aligned(4);
+#define tag_next(t) ((struct tag *)((u32 *)(t) + (t)->hdr.size)) +#define tag_size(type) ((sizeof(struct tag_header) + sizeof(struct type)) >> 2) +#define for_each_tag(t, base) \
- for (t = base; t->hdr.size; t = tag_next(t))
+/*
- atags_get_tag - get tag by tag magic
- @magic: tag magic, i.e. ATAG_SERIAL, ATAG_BOOTDEV, ...
- return: NULL on failed, otherwise return the tag that we want.
- */
+struct tag *atags_get_tag(u32 magic);
+/*
- atags_is_available - check if atags is available, used for second or
later pre-loaders.
- return: 0 is not available, otherwise available.
- */
+int atags_is_available(void);
+/*
- atags_overflow - check if atags is overflow
- return: 0 if not overflow, otherwise overflow.
- */
+int atags_overflow(struct tag *t);
+/*
- atags_bad_magic - check if atags magic is invalid.
- return: 1 if invalid, otherwise valid.
- */
+int atags_bad_magic(u32 magic); +#endif diff --git a/arch/arm/mach-rockchip/Makefile b/arch/arm/mach-rockchip/Makefile index 1dc92066bb..4165cbe99f 100644 --- a/arch/arm/mach-rockchip/Makefile +++ b/arch/arm/mach-rockchip/Makefile @@ -28,6 +28,7 @@ endif ifeq ($(CONFIG_TPL_BUILD),) obj-$(CONFIG_DISPLAY_CPUINFO) += cpu-info.o +obj-$(CONFIG_ROCKCHIP_RK3588) += atags.o endif obj-$(CONFIG_$(SPL_TPL_)RAM) += sdram.o diff --git a/arch/arm/mach-rockchip/atags.c b/arch/arm/mach-rockchip/atags.c new file mode 100644 index 0000000000..9daa2f2fc0 --- /dev/null +++ b/arch/arm/mach-rockchip/atags.c @@ -0,0 +1,99 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*
- (C) Copyright 2018 Rockchip Electronics Co., Ltd.
- */
+#include <config.h> +#include <asm/io.h> +#include <asm/arch-rockchip/atags.h>
+#define HASH_LEN sizeof(u32)
+static u32 js_hash(void *buf, u32 len) +{
- u32 i, hash = 0x47C6A7E6;
- char *data = buf;
- if (!buf || !len)
return hash;- for (i = 0; i < len; i++)
hash ^= ((hash << 5) + data[i] + (hash >> 2));- return hash;
+}
+int atags_bad_magic(u32 magic) +{
- bool bad;
- bad = ((magic != ATAG_CORE) &&
(magic != ATAG_NONE) &&(magic < ATAG_SERIAL || magic > ATAG_MAX));- if (bad)
printf("Magic(%x) is not support\n", magic);- return bad;
+}
+static int atags_size_overflow(struct tag *t, u32 tag_size) +{
- return (unsigned long)t + (tag_size << 2) - ATAGS_PHYS_BASE > ATAGS_SIZE;
+}
+int atags_overflow(struct tag *t) +{
- bool overflow;
- overflow = atags_size_overflow(t, 0) ||
atags_size_overflow(t, t->hdr.size);- if (overflow)
printf("Tag is overflow\n");- return overflow;
+}
+int atags_is_available(void) +{
- struct tag *t = (struct tag *)ATAGS_PHYS_BASE;
- return (t->hdr.magic == ATAG_CORE);
+}
+struct tag *atags_get_tag(u32 magic) +{
- u32 *hash, calc_hash, size;
- struct tag *t;
- if (!atags_is_available())
return NULL;- for_each_tag(t, (struct tag *)ATAGS_PHYS_BASE) {
if (atags_overflow(t))return NULL;if (atags_bad_magic(t->hdr.magic))return NULL;if (t->hdr.magic != magic)continue;size = t->hdr.size;hash = (u32 *)((ulong)t + (size << 2) - HASH_LEN);if (!*hash) {debug("No hash, magic(%x)\n", magic);return t;}calc_hash = js_hash(t, (size << 2) - HASH_LEN);if (calc_hash == *hash) {debug("Hash okay, magic(%x)\n", magic);return t;}debug("Hash bad, magic(%x), orgHash=%x, nowHash=%x\n",magic, *hash, calc_hash);return NULL;- }
- return NULL;
+}
On 3/27/24 15:32, Chris Morgan wrote:
On Wed, Mar 27, 2024 at 06:32:06PM +0800, Kever Yang wrote:
Hi Chris,
The ATAGS is used for passing parameter from bootloader to kernel at first, which has been replaced by DTB now for ARM platform.
And Rockchip using ATAGs for passing parameter like dram memory size/board uart in different boot process like DRAM init binary/ TPL/SPL to U-Boot since 2018.
And almost at the same time, Simon add bloblist for mainline U-Boot which for similar purpose.
So I'm not sure if this ATAGS should be accept in mainline U-Boot or not, even for rockchip platform only seems some kind of regression for this feature support.
Hi Simon and Tom,
Could you help to give some suggestion for this?
I really meant to do this as an "RFC", so I apologize in advance for possibly causing more work in treating this as a full-fledged patch.
The problem I'm trying to solve is that I've got 2 boards, a Rock 5B as well as an Indiedroid Nova both with 16GB of RAM. I noticed that without the memory holes the Rock 5B defined in my Indiedroid it would also fail to boot. I've got 2 other boards as well with less than 16GB of RAM which seem to work fine without any holes (a 4GB Indiedroid Nova prototype and a GameForce Ace with 12GB of RAM).
Hi,
When I initially added these holes in the memory, I tried to ask Rockchip what are the holes for. I didn't get any answer, however the patches to reserve the holes were accepted. If we could get more information about why the holes are there, if that area is specific to something, or that it's fixed in a per-SoC basis, we could reserve it by hardcoding in the Linux DT, without the need for ATAGs. Without real information, we cannot be sure that for other variants of the SoC or some other bootrom configuration, the holes will not change/move.
Eugen
The "RFC" part for which I'm really requesting guidance/comments is a question of "is it appropriate to use ATAGS" to get the RAM banks on this SoC, or is there a different way we should be doing it? If we can/should use the ATAGS, then I guess this can be pared down and refactored to just be RK3588 specific. If so, we can possibly add something like this to the RK3588 SoC specific code, guard it with an #ifdef ROCKCHIP_TPL to only call it when using the Rockchip specific RAM init (in the hopes that maybe one day we get our own RAM init), and then replace existing code for boards like the Rock 5B so that it no longer reserves these memory banks.
Thank you, Chris.
Thanks,
- Kever
On 2024/3/27 04:49, Chris Morgan wrote:
From: Chris Morgan macromorgan@hotmail.com
Add support for parsing the ATAGs created by the Rockchip binary RAM init. This ATAG parsing code was taken from the Rockchip BSP U-Boot source and tested only on parsing the RAM specific ATAGs for the RK3588.
Signed-off-by: Chris Morgan macromorgan@hotmail.com
arch/arm/include/asm/arch-rockchip/atags.h | 222 +++++++++++++++++++++ arch/arm/mach-rockchip/Makefile | 1 + arch/arm/mach-rockchip/atags.c | 99 +++++++++ 3 files changed, 322 insertions(+) create mode 100644 arch/arm/include/asm/arch-rockchip/atags.h create mode 100644 arch/arm/mach-rockchip/atags.c
diff --git a/arch/arm/include/asm/arch-rockchip/atags.h b/arch/arm/include/asm/arch-rockchip/atags.h new file mode 100644 index 0000000000..9bae66d7f8 --- /dev/null +++ b/arch/arm/include/asm/arch-rockchip/atags.h @@ -0,0 +1,222 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*
- (C) Copyright 2018 Rockchip Electronics Co., Ltd
- */
+#ifndef __RK_ATAGS_H_ +#define __RK_ATAGS_H_
+/* Tag magic */ +#define ATAG_CORE 0x54410001 +#define ATAG_NONE 0x00000000
+#define ATAG_SERIAL 0x54410050 +#define ATAG_BOOTDEV 0x54410051 +#define ATAG_DDR_MEM 0x54410052 +#define ATAG_TOS_MEM 0x54410053 +#define ATAG_RAM_PARTITION 0x54410054 +#define ATAG_ATF_MEM 0x54410055 +#define ATAG_PUB_KEY 0x54410056 +#define ATAG_SOC_INFO 0x54410057 +#define ATAG_BOOT1_PARAM 0x54410058 +#define ATAG_PSTORE 0x54410059 +#define ATAG_FWVER 0x5441005a +#define ATAG_MAX 0x544100ff
+/* Tag size and offset */ +#define ATAGS_SIZE (0x2000) /* 8K */ +#define ATAGS_OFFSET (0x200000 - ATAGS_SIZE)/* [2M-8K, 2M] */ +#define ATAGS_PHYS_BASE (CFG_SYS_SDRAM_BASE + ATAGS_OFFSET)
+/* tag_fwver.ver[fwid][] */ +#define FWVER_LEN 36
+enum fwid {
- FW_DDR,
- FW_SPL,
- FW_ATF,
- FW_TEE,
- FW_MAX,
+};
+struct tag_serial {
- u32 version;
- u32 enable;
- u64 addr;
- u32 baudrate;
- u32 m_mode;
- u32 id;
- u32 reserved[2];
- u32 hash;
+} __packed;
+struct tag_bootdev {
- u32 version;
- u32 devtype;
- u32 devnum;
- u32 mode;
- u32 sdupdate;
- u32 reserved[6];
- u32 hash;
+} __packed;
+struct tag_ddr_mem {
- u32 count;
- u32 version;
- u64 bank[20];
- u32 flags;
- u32 data[2];
- u32 hash;
+} __packed;
+struct tag_tos_mem {
- u32 version;
- struct {
char name[8];u64 phy_addr;u32 size;u32 flags;- } tee_mem;
- struct {
char name[8];u64 phy_addr;u32 size;u32 flags;- } drm_mem;
- u64 reserved[7];
- u32 reserved1;
- u32 hash;
+} __packed;
+struct tag_atf_mem {
- u32 version;
- u64 phy_addr;
- u32 size;
- u32 flags;
- u32 reserved[2];
- u32 hash;
+} __packed;
+struct tag_pub_key {
- u32 version;
- u32 len;
- u8 data[768]; /* u32 rsa_n[64], rsa_e[64], rsa_c[64] */
- u32 flag;
- u32 reserved[5];
- u32 hash;
+} __packed;
+struct tag_ram_partition {
- u32 version;
- u32 count;
- u32 reserved[4];
- struct {
char name[16];u64 start;u64 size;- } part[6];
- u32 reserved1[3];
- u32 hash;
+} __packed;
+struct tag_soc_info {
- u32 version;
- u32 name; /* Hex: 0x3288, 0x3399... */
- u32 flags;
- u32 reserved[6];
- u32 hash;
+} __packed;
+struct tag_boot1p {
- u32 version;
- u32 param[8];
- u32 reserved[4];
- u32 hash;
+} __packed;
+struct tag_pstore {
- u32 version;
- struct {
u32 addr;u32 size;- } buf[16];
- u32 hash;
+} __packed;
+struct tag_fwver {
- u32 version;
- char ver[8][FWVER_LEN];
- u32 hash;
+} __packed;
+struct tag_core {
- u32 flags;
- u32 pagesize;
- u32 rootdev;
+} __packed;
+struct tag_header {
- u32 size; /* bytes = size * 4 */
- u32 magic;
+} __packed;
+/* Must be 4 bytes align */ +struct tag {
- struct tag_header hdr;
- union {
struct tag_core core;struct tag_serial serial;struct tag_bootdev bootdev;struct tag_ddr_mem ddr_mem;struct tag_tos_mem tos_mem;struct tag_ram_partition ram_part;struct tag_atf_mem atf_mem;struct tag_pub_key pub_key;struct tag_soc_info soc;struct tag_boot1p boot1p;struct tag_pstore pstore;struct tag_fwver fwver;- } u;
+} __aligned(4);
+#define tag_next(t) ((struct tag *)((u32 *)(t) + (t)->hdr.size)) +#define tag_size(type) ((sizeof(struct tag_header) + sizeof(struct type)) >> 2) +#define for_each_tag(t, base) \
- for (t = base; t->hdr.size; t = tag_next(t))
+/*
- atags_get_tag - get tag by tag magic
- @magic: tag magic, i.e. ATAG_SERIAL, ATAG_BOOTDEV, ...
- return: NULL on failed, otherwise return the tag that we want.
- */
+struct tag *atags_get_tag(u32 magic);
+/*
- atags_is_available - check if atags is available, used for second or
later pre-loaders.
- return: 0 is not available, otherwise available.
- */
+int atags_is_available(void);
+/*
- atags_overflow - check if atags is overflow
- return: 0 if not overflow, otherwise overflow.
- */
+int atags_overflow(struct tag *t);
+/*
- atags_bad_magic - check if atags magic is invalid.
- return: 1 if invalid, otherwise valid.
- */
+int atags_bad_magic(u32 magic); +#endif diff --git a/arch/arm/mach-rockchip/Makefile b/arch/arm/mach-rockchip/Makefile index 1dc92066bb..4165cbe99f 100644 --- a/arch/arm/mach-rockchip/Makefile +++ b/arch/arm/mach-rockchip/Makefile @@ -28,6 +28,7 @@ endif ifeq ($(CONFIG_TPL_BUILD),) obj-$(CONFIG_DISPLAY_CPUINFO) += cpu-info.o +obj-$(CONFIG_ROCKCHIP_RK3588) += atags.o endif obj-$(CONFIG_$(SPL_TPL_)RAM) += sdram.o diff --git a/arch/arm/mach-rockchip/atags.c b/arch/arm/mach-rockchip/atags.c new file mode 100644 index 0000000000..9daa2f2fc0 --- /dev/null +++ b/arch/arm/mach-rockchip/atags.c @@ -0,0 +1,99 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*
- (C) Copyright 2018 Rockchip Electronics Co., Ltd.
- */
+#include <config.h> +#include <asm/io.h> +#include <asm/arch-rockchip/atags.h>
+#define HASH_LEN sizeof(u32)
+static u32 js_hash(void *buf, u32 len) +{
- u32 i, hash = 0x47C6A7E6;
- char *data = buf;
- if (!buf || !len)
return hash;- for (i = 0; i < len; i++)
hash ^= ((hash << 5) + data[i] + (hash >> 2));- return hash;
+}
+int atags_bad_magic(u32 magic) +{
- bool bad;
- bad = ((magic != ATAG_CORE) &&
(magic != ATAG_NONE) &&(magic < ATAG_SERIAL || magic > ATAG_MAX));- if (bad)
printf("Magic(%x) is not support\n", magic);- return bad;
+}
+static int atags_size_overflow(struct tag *t, u32 tag_size) +{
- return (unsigned long)t + (tag_size << 2) - ATAGS_PHYS_BASE > ATAGS_SIZE;
+}
+int atags_overflow(struct tag *t) +{
- bool overflow;
- overflow = atags_size_overflow(t, 0) ||
atags_size_overflow(t, t->hdr.size);- if (overflow)
printf("Tag is overflow\n");- return overflow;
+}
+int atags_is_available(void) +{
- struct tag *t = (struct tag *)ATAGS_PHYS_BASE;
- return (t->hdr.magic == ATAG_CORE);
+}
+struct tag *atags_get_tag(u32 magic) +{
- u32 *hash, calc_hash, size;
- struct tag *t;
- if (!atags_is_available())
return NULL;- for_each_tag(t, (struct tag *)ATAGS_PHYS_BASE) {
if (atags_overflow(t))return NULL;if (atags_bad_magic(t->hdr.magic))return NULL;if (t->hdr.magic != magic)continue;size = t->hdr.size;hash = (u32 *)((ulong)t + (size << 2) - HASH_LEN);if (!*hash) {debug("No hash, magic(%x)\n", magic);return t;}calc_hash = js_hash(t, (size << 2) - HASH_LEN);if (calc_hash == *hash) {debug("Hash okay, magic(%x)\n", magic);return t;}debug("Hash bad, magic(%x), orgHash=%x, nowHash=%x\n",magic, *hash, calc_hash);return NULL;- }
- return NULL;
+}
On Wed, Mar 27, 2024 at 04:21:49PM +0200, Eugen Hristev wrote:
On 3/27/24 15:32, Chris Morgan wrote:
On Wed, Mar 27, 2024 at 06:32:06PM +0800, Kever Yang wrote:
Hi Chris,
The ATAGS is used for passing parameter from bootloader to kernel at first, which has been replaced by DTB now for ARM platform.
And Rockchip using ATAGs for passing parameter like dram memory size/board uart in different boot process like DRAM init binary/ TPL/SPL to U-Boot since 2018.
And almost at the same time, Simon add bloblist for mainline U-Boot which for similar purpose.
So I'm not sure if this ATAGS should be accept in mainline U-Boot or not, even for rockchip platform only seems some kind of regression for this feature support.
Hi Simon and Tom,
Could you help to give some suggestion for this?
I really meant to do this as an "RFC", so I apologize in advance for possibly causing more work in treating this as a full-fledged patch.
The problem I'm trying to solve is that I've got 2 boards, a Rock 5B as well as an Indiedroid Nova both with 16GB of RAM. I noticed that without the memory holes the Rock 5B defined in my Indiedroid it would also fail to boot. I've got 2 other boards as well with less than 16GB of RAM which seem to work fine without any holes (a 4GB Indiedroid Nova prototype and a GameForce Ace with 12GB of RAM).
Hi,
When I initially added these holes in the memory, I tried to ask Rockchip what are the holes for. I didn't get any answer, however the patches to reserve the holes were accepted. If we could get more information about why the holes are there, if that area is specific to something, or that it's fixed in a per-SoC basis, we could reserve it by hardcoding in the Linux DT, without the need for ATAGs. Without real information, we cannot be sure that for other variants of the SoC or some other bootrom configuration, the holes will not change/move.
Eugen
It's not *just* about the holes, but also making sure we use the full extent of the RAM. For example on my 4GB Indiedroid Nova the existing logic ignores about 256MB of RAM that's otherwise present. When we use the existing code the RAM map reported by Linux is as follows:
[ 0.000000] Early memory node ranges [ 0.000000] node 0: [mem 0x0000000000200000-0x00000000efffffff]
Whereas when we do ATAGS parsing to set the banks we get the following:
[ 0.000000] Early memory node ranges [ 0.000000] node 0: [mem 0x0000000000200000-0x00000000efffffff] [ 0.000000] node 0: [mem 0x00000001f0000000-0x00000001ffffffff]
So basically the issue is two-fold... one we need to create the memory holes when necessary, and two the current method of defining memory banks leaves some RAM unallocated. Parsing the ATAGS does both of these, I'm just curious if it's the RIGHT way or there's something else I'm missing...
Thank you, Chris.
The "RFC" part for which I'm really requesting guidance/comments is a question of "is it appropriate to use ATAGS" to get the RAM banks on this SoC, or is there a different way we should be doing it? If we can/should use the ATAGS, then I guess this can be pared down and refactored to just be RK3588 specific. If so, we can possibly add something like this to the RK3588 SoC specific code, guard it with an #ifdef ROCKCHIP_TPL to only call it when using the Rockchip specific RAM init (in the hopes that maybe one day we get our own RAM init), and then replace existing code for boards like the Rock 5B so that it no longer reserves these memory banks.
Thank you, Chris.
Thanks,
- Kever
On 2024/3/27 04:49, Chris Morgan wrote:
From: Chris Morgan macromorgan@hotmail.com
Add support for parsing the ATAGs created by the Rockchip binary RAM init. This ATAG parsing code was taken from the Rockchip BSP U-Boot source and tested only on parsing the RAM specific ATAGs for the RK3588.
Signed-off-by: Chris Morgan macromorgan@hotmail.com
arch/arm/include/asm/arch-rockchip/atags.h | 222 +++++++++++++++++++++ arch/arm/mach-rockchip/Makefile | 1 + arch/arm/mach-rockchip/atags.c | 99 +++++++++ 3 files changed, 322 insertions(+) create mode 100644 arch/arm/include/asm/arch-rockchip/atags.h create mode 100644 arch/arm/mach-rockchip/atags.c
diff --git a/arch/arm/include/asm/arch-rockchip/atags.h b/arch/arm/include/asm/arch-rockchip/atags.h new file mode 100644 index 0000000000..9bae66d7f8 --- /dev/null +++ b/arch/arm/include/asm/arch-rockchip/atags.h @@ -0,0 +1,222 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*
- (C) Copyright 2018 Rockchip Electronics Co., Ltd
- */
+#ifndef __RK_ATAGS_H_ +#define __RK_ATAGS_H_
+/* Tag magic */ +#define ATAG_CORE 0x54410001 +#define ATAG_NONE 0x00000000
+#define ATAG_SERIAL 0x54410050 +#define ATAG_BOOTDEV 0x54410051 +#define ATAG_DDR_MEM 0x54410052 +#define ATAG_TOS_MEM 0x54410053 +#define ATAG_RAM_PARTITION 0x54410054 +#define ATAG_ATF_MEM 0x54410055 +#define ATAG_PUB_KEY 0x54410056 +#define ATAG_SOC_INFO 0x54410057 +#define ATAG_BOOT1_PARAM 0x54410058 +#define ATAG_PSTORE 0x54410059 +#define ATAG_FWVER 0x5441005a +#define ATAG_MAX 0x544100ff
+/* Tag size and offset */ +#define ATAGS_SIZE (0x2000) /* 8K */ +#define ATAGS_OFFSET (0x200000 - ATAGS_SIZE)/* [2M-8K, 2M] */ +#define ATAGS_PHYS_BASE (CFG_SYS_SDRAM_BASE + ATAGS_OFFSET)
+/* tag_fwver.ver[fwid][] */ +#define FWVER_LEN 36
+enum fwid {
- FW_DDR,
- FW_SPL,
- FW_ATF,
- FW_TEE,
- FW_MAX,
+};
+struct tag_serial {
- u32 version;
- u32 enable;
- u64 addr;
- u32 baudrate;
- u32 m_mode;
- u32 id;
- u32 reserved[2];
- u32 hash;
+} __packed;
+struct tag_bootdev {
- u32 version;
- u32 devtype;
- u32 devnum;
- u32 mode;
- u32 sdupdate;
- u32 reserved[6];
- u32 hash;
+} __packed;
+struct tag_ddr_mem {
- u32 count;
- u32 version;
- u64 bank[20];
- u32 flags;
- u32 data[2];
- u32 hash;
+} __packed;
+struct tag_tos_mem {
- u32 version;
- struct {
char name[8];u64 phy_addr;u32 size;u32 flags;- } tee_mem;
- struct {
char name[8];u64 phy_addr;u32 size;u32 flags;- } drm_mem;
- u64 reserved[7];
- u32 reserved1;
- u32 hash;
+} __packed;
+struct tag_atf_mem {
- u32 version;
- u64 phy_addr;
- u32 size;
- u32 flags;
- u32 reserved[2];
- u32 hash;
+} __packed;
+struct tag_pub_key {
- u32 version;
- u32 len;
- u8 data[768]; /* u32 rsa_n[64], rsa_e[64], rsa_c[64] */
- u32 flag;
- u32 reserved[5];
- u32 hash;
+} __packed;
+struct tag_ram_partition {
- u32 version;
- u32 count;
- u32 reserved[4];
- struct {
char name[16];u64 start;u64 size;- } part[6];
- u32 reserved1[3];
- u32 hash;
+} __packed;
+struct tag_soc_info {
- u32 version;
- u32 name; /* Hex: 0x3288, 0x3399... */
- u32 flags;
- u32 reserved[6];
- u32 hash;
+} __packed;
+struct tag_boot1p {
- u32 version;
- u32 param[8];
- u32 reserved[4];
- u32 hash;
+} __packed;
+struct tag_pstore {
- u32 version;
- struct {
u32 addr;u32 size;- } buf[16];
- u32 hash;
+} __packed;
+struct tag_fwver {
- u32 version;
- char ver[8][FWVER_LEN];
- u32 hash;
+} __packed;
+struct tag_core {
- u32 flags;
- u32 pagesize;
- u32 rootdev;
+} __packed;
+struct tag_header {
- u32 size; /* bytes = size * 4 */
- u32 magic;
+} __packed;
+/* Must be 4 bytes align */ +struct tag {
- struct tag_header hdr;
- union {
struct tag_core core;struct tag_serial serial;struct tag_bootdev bootdev;struct tag_ddr_mem ddr_mem;struct tag_tos_mem tos_mem;struct tag_ram_partition ram_part;struct tag_atf_mem atf_mem;struct tag_pub_key pub_key;struct tag_soc_info soc;struct tag_boot1p boot1p;struct tag_pstore pstore;struct tag_fwver fwver;- } u;
+} __aligned(4);
+#define tag_next(t) ((struct tag *)((u32 *)(t) + (t)->hdr.size)) +#define tag_size(type) ((sizeof(struct tag_header) + sizeof(struct type)) >> 2) +#define for_each_tag(t, base) \
- for (t = base; t->hdr.size; t = tag_next(t))
+/*
- atags_get_tag - get tag by tag magic
- @magic: tag magic, i.e. ATAG_SERIAL, ATAG_BOOTDEV, ...
- return: NULL on failed, otherwise return the tag that we want.
- */
+struct tag *atags_get_tag(u32 magic);
+/*
- atags_is_available - check if atags is available, used for second or
later pre-loaders.
- return: 0 is not available, otherwise available.
- */
+int atags_is_available(void);
+/*
- atags_overflow - check if atags is overflow
- return: 0 if not overflow, otherwise overflow.
- */
+int atags_overflow(struct tag *t);
+/*
- atags_bad_magic - check if atags magic is invalid.
- return: 1 if invalid, otherwise valid.
- */
+int atags_bad_magic(u32 magic); +#endif diff --git a/arch/arm/mach-rockchip/Makefile b/arch/arm/mach-rockchip/Makefile index 1dc92066bb..4165cbe99f 100644 --- a/arch/arm/mach-rockchip/Makefile +++ b/arch/arm/mach-rockchip/Makefile @@ -28,6 +28,7 @@ endif ifeq ($(CONFIG_TPL_BUILD),) obj-$(CONFIG_DISPLAY_CPUINFO) += cpu-info.o +obj-$(CONFIG_ROCKCHIP_RK3588) += atags.o endif obj-$(CONFIG_$(SPL_TPL_)RAM) += sdram.o diff --git a/arch/arm/mach-rockchip/atags.c b/arch/arm/mach-rockchip/atags.c new file mode 100644 index 0000000000..9daa2f2fc0 --- /dev/null +++ b/arch/arm/mach-rockchip/atags.c @@ -0,0 +1,99 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*
- (C) Copyright 2018 Rockchip Electronics Co., Ltd.
- */
+#include <config.h> +#include <asm/io.h> +#include <asm/arch-rockchip/atags.h>
+#define HASH_LEN sizeof(u32)
+static u32 js_hash(void *buf, u32 len) +{
- u32 i, hash = 0x47C6A7E6;
- char *data = buf;
- if (!buf || !len)
return hash;- for (i = 0; i < len; i++)
hash ^= ((hash << 5) + data[i] + (hash >> 2));- return hash;
+}
+int atags_bad_magic(u32 magic) +{
- bool bad;
- bad = ((magic != ATAG_CORE) &&
(magic != ATAG_NONE) &&(magic < ATAG_SERIAL || magic > ATAG_MAX));- if (bad)
printf("Magic(%x) is not support\n", magic);- return bad;
+}
+static int atags_size_overflow(struct tag *t, u32 tag_size) +{
- return (unsigned long)t + (tag_size << 2) - ATAGS_PHYS_BASE > ATAGS_SIZE;
+}
+int atags_overflow(struct tag *t) +{
- bool overflow;
- overflow = atags_size_overflow(t, 0) ||
atags_size_overflow(t, t->hdr.size);- if (overflow)
printf("Tag is overflow\n");- return overflow;
+}
+int atags_is_available(void) +{
- struct tag *t = (struct tag *)ATAGS_PHYS_BASE;
- return (t->hdr.magic == ATAG_CORE);
+}
+struct tag *atags_get_tag(u32 magic) +{
- u32 *hash, calc_hash, size;
- struct tag *t;
- if (!atags_is_available())
return NULL;- for_each_tag(t, (struct tag *)ATAGS_PHYS_BASE) {
if (atags_overflow(t))return NULL;if (atags_bad_magic(t->hdr.magic))return NULL;if (t->hdr.magic != magic)continue;size = t->hdr.size;hash = (u32 *)((ulong)t + (size << 2) - HASH_LEN);if (!*hash) {debug("No hash, magic(%x)\n", magic);return t;}calc_hash = js_hash(t, (size << 2) - HASH_LEN);if (calc_hash == *hash) {debug("Hash okay, magic(%x)\n", magic);return t;}debug("Hash bad, magic(%x), orgHash=%x, nowHash=%x\n",magic, *hash, calc_hash);return NULL;- }
- return NULL;
+}
On Wed, Mar 27, 2024 at 10:03:11AM -0500, Chris Morgan wrote:
On Wed, Mar 27, 2024 at 04:21:49PM +0200, Eugen Hristev wrote:
On 3/27/24 15:32, Chris Morgan wrote:
On Wed, Mar 27, 2024 at 06:32:06PM +0800, Kever Yang wrote:
Hi Chris,
The ATAGS is used for passing parameter from bootloader to kernel at first, which has been replaced by DTB now for ARM platform.
And Rockchip using ATAGs for passing parameter like dram memory size/board uart in different boot process like DRAM init binary/ TPL/SPL to U-Boot since 2018.
And almost at the same time, Simon add bloblist for mainline U-Boot which for similar purpose.
So I'm not sure if this ATAGS should be accept in mainline U-Boot or not, even for rockchip platform only seems some kind of regression for this feature support.
Hi Simon and Tom,
Could you help to give some suggestion for this?
I really meant to do this as an "RFC", so I apologize in advance for possibly causing more work in treating this as a full-fledged patch.
The problem I'm trying to solve is that I've got 2 boards, a Rock 5B as well as an Indiedroid Nova both with 16GB of RAM. I noticed that without the memory holes the Rock 5B defined in my Indiedroid it would also fail to boot. I've got 2 other boards as well with less than 16GB of RAM which seem to work fine without any holes (a 4GB Indiedroid Nova prototype and a GameForce Ace with 12GB of RAM).
Hi,
When I initially added these holes in the memory, I tried to ask Rockchip what are the holes for. I didn't get any answer, however the patches to reserve the holes were accepted. If we could get more information about why the holes are there, if that area is specific to something, or that it's fixed in a per-SoC basis, we could reserve it by hardcoding in the Linux DT, without the need for ATAGs. Without real information, we cannot be sure that for other variants of the SoC or some other bootrom configuration, the holes will not change/move.
Eugen
It's not *just* about the holes, but also making sure we use the full extent of the RAM. For example on my 4GB Indiedroid Nova the existing logic ignores about 256MB of RAM that's otherwise present. When we use the existing code the RAM map reported by Linux is as follows:
[ 0.000000] Early memory node ranges [ 0.000000] node 0: [mem 0x0000000000200000-0x00000000efffffff]
Whereas when we do ATAGS parsing to set the banks we get the following:
[ 0.000000] Early memory node ranges [ 0.000000] node 0: [mem 0x0000000000200000-0x00000000efffffff] [ 0.000000] node 0: [mem 0x00000001f0000000-0x00000001ffffffff]
So basically the issue is two-fold... one we need to create the memory holes when necessary, and two the current method of defining memory banks leaves some RAM unallocated. Parsing the ATAGS does both of these, I'm just curious if it's the RIGHT way or there's something else I'm missing...
It's unfortunate that all of this is in the wild, from the Rockchip side. The best answer is that the platform should be passing all of this along via bloblist. I would hope that future platforms will do that. For now yes, I guess we need to constrain this ATAG parsing to rockchip as much as we can and use it.
On Thu, Mar 28, 2024 at 04:02:09PM -0400, Tom Rini wrote:
On Wed, Mar 27, 2024 at 10:03:11AM -0500, Chris Morgan wrote:
On Wed, Mar 27, 2024 at 04:21:49PM +0200, Eugen Hristev wrote:
On 3/27/24 15:32, Chris Morgan wrote:
On Wed, Mar 27, 2024 at 06:32:06PM +0800, Kever Yang wrote:
Hi Chris,
The ATAGS is used for passing parameter from bootloader to kernel at first, which has been replaced by DTB now for ARM platform.
And Rockchip using ATAGs for passing parameter like dram memory size/board uart in different boot process like DRAM init binary/ TPL/SPL to U-Boot since 2018.
And almost at the same time, Simon add bloblist for mainline U-Boot which for similar purpose.
So I'm not sure if this ATAGS should be accept in mainline U-Boot or not, even for rockchip platform only seems some kind of regression for this feature support.
Hi Simon and Tom,
Could you help to give some suggestion for this?
I really meant to do this as an "RFC", so I apologize in advance for possibly causing more work in treating this as a full-fledged patch.
The problem I'm trying to solve is that I've got 2 boards, a Rock 5B as well as an Indiedroid Nova both with 16GB of RAM. I noticed that without the memory holes the Rock 5B defined in my Indiedroid it would also fail to boot. I've got 2 other boards as well with less than 16GB of RAM which seem to work fine without any holes (a 4GB Indiedroid Nova prototype and a GameForce Ace with 12GB of RAM).
Hi,
When I initially added these holes in the memory, I tried to ask Rockchip what are the holes for. I didn't get any answer, however the patches to reserve the holes were accepted. If we could get more information about why the holes are there, if that area is specific to something, or that it's fixed in a per-SoC basis, we could reserve it by hardcoding in the Linux DT, without the need for ATAGs. Without real information, we cannot be sure that for other variants of the SoC or some other bootrom configuration, the holes will not change/move.
Eugen
It's not *just* about the holes, but also making sure we use the full extent of the RAM. For example on my 4GB Indiedroid Nova the existing logic ignores about 256MB of RAM that's otherwise present. When we use the existing code the RAM map reported by Linux is as follows:
[ 0.000000] Early memory node ranges [ 0.000000] node 0: [mem 0x0000000000200000-0x00000000efffffff]
Whereas when we do ATAGS parsing to set the banks we get the following:
[ 0.000000] Early memory node ranges [ 0.000000] node 0: [mem 0x0000000000200000-0x00000000efffffff] [ 0.000000] node 0: [mem 0x00000001f0000000-0x00000001ffffffff]
So basically the issue is two-fold... one we need to create the memory holes when necessary, and two the current method of defining memory banks leaves some RAM unallocated. Parsing the ATAGS does both of these, I'm just curious if it's the RIGHT way or there's something else I'm missing...
It's unfortunate that all of this is in the wild, from the Rockchip side. The best answer is that the platform should be passing all of this along via bloblist. I would hope that future platforms will do that. For now yes, I guess we need to constrain this ATAG parsing to rockchip as much as we can and use it.
Any thought/benefit of trying to force the ATAGS into a bloblist after the fact (instead of expecting it to be passed that way)? Otherwise yes I'll work on simplified ATAGS parsing for the memory info and constrain it to the rk3588 specific board code/wrap it around ROCKCHIP_TPL present.
Thank you, Chris
-- Tom
On Thu, Mar 28, 2024 at 04:58:53PM -0500, Chris Morgan wrote:
On Thu, Mar 28, 2024 at 04:02:09PM -0400, Tom Rini wrote:
On Wed, Mar 27, 2024 at 10:03:11AM -0500, Chris Morgan wrote:
On Wed, Mar 27, 2024 at 04:21:49PM +0200, Eugen Hristev wrote:
On 3/27/24 15:32, Chris Morgan wrote:
On Wed, Mar 27, 2024 at 06:32:06PM +0800, Kever Yang wrote:
Hi Chris,
The ATAGS is used for passing parameter from bootloader to kernel at first, which has been replaced by DTB now for ARM platform.
And Rockchip using ATAGs for passing parameter like dram memory size/board uart in different boot process like DRAM init binary/ TPL/SPL to U-Boot since 2018.
And almost at the same time, Simon add bloblist for mainline U-Boot which for similar purpose.
So I'm not sure if this ATAGS should be accept in mainline U-Boot or not, even for rockchip platform only seems some kind of regression for this feature support.
Hi Simon and Tom,
Could you help to give some suggestion for this?
I really meant to do this as an "RFC", so I apologize in advance for possibly causing more work in treating this as a full-fledged patch.
The problem I'm trying to solve is that I've got 2 boards, a Rock 5B as well as an Indiedroid Nova both with 16GB of RAM. I noticed that without the memory holes the Rock 5B defined in my Indiedroid it would also fail to boot. I've got 2 other boards as well with less than 16GB of RAM which seem to work fine without any holes (a 4GB Indiedroid Nova prototype and a GameForce Ace with 12GB of RAM).
Hi,
When I initially added these holes in the memory, I tried to ask Rockchip what are the holes for. I didn't get any answer, however the patches to reserve the holes were accepted. If we could get more information about why the holes are there, if that area is specific to something, or that it's fixed in a per-SoC basis, we could reserve it by hardcoding in the Linux DT, without the need for ATAGs. Without real information, we cannot be sure that for other variants of the SoC or some other bootrom configuration, the holes will not change/move.
Eugen
It's not *just* about the holes, but also making sure we use the full extent of the RAM. For example on my 4GB Indiedroid Nova the existing logic ignores about 256MB of RAM that's otherwise present. When we use the existing code the RAM map reported by Linux is as follows:
[ 0.000000] Early memory node ranges [ 0.000000] node 0: [mem 0x0000000000200000-0x00000000efffffff]
Whereas when we do ATAGS parsing to set the banks we get the following:
[ 0.000000] Early memory node ranges [ 0.000000] node 0: [mem 0x0000000000200000-0x00000000efffffff] [ 0.000000] node 0: [mem 0x00000001f0000000-0x00000001ffffffff]
So basically the issue is two-fold... one we need to create the memory holes when necessary, and two the current method of defining memory banks leaves some RAM unallocated. Parsing the ATAGS does both of these, I'm just curious if it's the RIGHT way or there's something else I'm missing...
It's unfortunate that all of this is in the wild, from the Rockchip side. The best answer is that the platform should be passing all of this along via bloblist. I would hope that future platforms will do that. For now yes, I guess we need to constrain this ATAG parsing to rockchip as much as we can and use it.
Any thought/benefit of trying to force the ATAGS into a bloblist after the fact (instead of expecting it to be passed that way)? Otherwise yes I'll work on simplified ATAGS parsing for the memory info and constrain it to the rk3588 specific board code/wrap it around ROCKCHIP_TPL present.
I don't think it's worth the code size to convert the ATAGS to bloblist at this time. In the future when we have rockchip platforms that pass bloblist natively at that point it might make sense to convert the ATAGS instead.
From: Chris Morgan macromorgan@hotmail.com
Add support for defining the usable RAM from ATAGs provided by the Rockchip binary TPL loader. This allows us to automatically account for necessary memory holes on RK3588 devices with 16GB of RAM or more, as well as ensure we can use the full amount of RAM available.
In the event we can't cleanly read the ATAG values from RAM or are not running an RK3588 board, simply fall back to the old method of detecting the RAM.
Tested on Indiedroid Nova with 4GB and 16GB of RAM.
Signed-off-by: Chris Morgan macromorgan@hotmail.com --- arch/arm/mach-rockchip/sdram.c | 58 ++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+)
diff --git a/arch/arm/mach-rockchip/sdram.c b/arch/arm/mach-rockchip/sdram.c index 0d9a0aef6f..58b78466b0 100644 --- a/arch/arm/mach-rockchip/sdram.c +++ b/arch/arm/mach-rockchip/sdram.c @@ -10,6 +10,7 @@ #include <ram.h> #include <asm/global_data.h> #include <asm/io.h> +#include <asm/arch-rockchip/atags.h> #include <asm/arch-rockchip/sdram.h> #include <dm/uclass-internal.h>
@@ -35,12 +36,69 @@ struct tos_parameter_t { s64 reserve[8]; };
+/* + * Read the ATAGs to identify all the memory banks. If we can't do it + * cleanly return 1 to note an unsuccessful attempt, otherwise return + * 0 for a successful attempt. + */ +int rockchip_atag_ram_banks(void) +{ + struct tag *t; + int bank_cnt; + size_t tmp; + + if (!CONFIG_IS_ENABLED(ARM64) && !CONFIG_IS_ENABLED(ROCKCHIP_RK3588)) + return 1; + + t = atags_get_tag(ATAG_DDR_MEM); + if (!t) + return 1; + + bank_cnt = t->u.ddr_mem.count; + + /* + * Check to make sure the first bank ends at 0xf0000000, if it + * does not fall back to the other methods of RAM bank + * detection. + */ + if (t->u.ddr_mem.bank[t->u.ddr_mem.count] != 0xf0000000) + return 1; + + /* + * Iterate over the RAM banks. If the start address of bank 0 + * is less than or equal to 0x200000, set it to 0x200000 to + * reserve room for A-TF. Make sure the size of bank 0 doesn't + * bleed into the address space for hardware (starting at + * 0xf0000000). Banks 1 and on can be defined as-is. + */ + for (int i = 0; i < (t->u.ddr_mem.count); i++) { + if (i == 0) { + if (t->u.ddr_mem.bank[i] <= 0x200000) + gd->bd->bi_dram[i].start = 0x200000; + else + gd->bd->bi_dram[i].start = t->u.ddr_mem.bank[i]; + tmp = gd->bd->bi_dram[i].start + t->u.ddr_mem.bank[(bank_cnt + i)]; + if (tmp > 0xf0000000) + gd->bd->bi_dram[i].size = 0xf0000000 - gd->bd->bi_dram[i].start; + else + gd->bd->bi_dram[i].size = t->u.ddr_mem.bank[(bank_cnt + i)]; + } else { + gd->bd->bi_dram[i].start = t->u.ddr_mem.bank[i]; + gd->bd->bi_dram[i].size = t->u.ddr_mem.bank[(bank_cnt + i)]; + } + }; + + return 0; +} + int dram_init_banksize(void) { size_t ram_top = (unsigned long)(gd->ram_size + CFG_SYS_SDRAM_BASE); size_t top = min((unsigned long)ram_top, (unsigned long)(gd->ram_top));
#ifdef CONFIG_ARM64 + if (!rockchip_atag_ram_banks()) + return 0; /* Reserve 0x200000 for ATF bl31 */ gd->bd->bi_dram[0].start = 0x200000; gd->bd->bi_dram[0].size = top - gd->bd->bi_dram[0].start;
Hi Chris,
On 3/26/24 21:49, Chris Morgan wrote:
From: Chris Morgan macromorgan@hotmail.com
Add support for defining the usable RAM from ATAGs provided by the Rockchip binary TPL loader. This allows us to automatically account for necessary memory holes on RK3588 devices with 16GB of RAM or more, as well as ensure we can use the full amount of RAM available.
In the event we can't cleanly read the ATAG values from RAM or are not running an RK3588 board, simply fall back to the old method of detecting the RAM.
Tested on Indiedroid Nova with 4GB and 16GB of RAM.
Signed-off-by: Chris Morgan macromorgan@hotmail.com
arch/arm/mach-rockchip/sdram.c | 58 ++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+)
diff --git a/arch/arm/mach-rockchip/sdram.c b/arch/arm/mach-rockchip/sdram.c index 0d9a0aef6f..58b78466b0 100644 --- a/arch/arm/mach-rockchip/sdram.c +++ b/arch/arm/mach-rockchip/sdram.c @@ -10,6 +10,7 @@ #include <ram.h> #include <asm/global_data.h> #include <asm/io.h> +#include <asm/arch-rockchip/atags.h> #include <asm/arch-rockchip/sdram.h> #include <dm/uclass-internal.h>
@@ -35,12 +36,69 @@ struct tos_parameter_t { s64 reserve[8]; };
+/*
- Read the ATAGs to identify all the memory banks. If we can't do it
- cleanly return 1 to note an unsuccessful attempt, otherwise return
- 0 for a successful attempt.
Return a valid error code instead? Negative if possible?
- */
+int rockchip_atag_ram_banks(void) +{
- struct tag *t;
- int bank_cnt;
- size_t tmp;
- if (!CONFIG_IS_ENABLED(ARM64) && !CONFIG_IS_ENABLED(ROCKCHIP_RK3588))
return 1;- t = atags_get_tag(ATAG_DDR_MEM);
I believe this will not compile for non RK3588 because this function is not defined.
There are a few ways to handle this: - always compile atags.c but have ifdefs there with inline functions returning -ENOTSUPP or something like that.
- if (!t)
return 1;
-ENOENT? -ENOXIO?
- bank_cnt = t->u.ddr_mem.count;
- /*
* Check to make sure the first bank ends at 0xf0000000, if it
Explain what 0xf0000000 represents here.
* does not fall back to the other methods of RAM bank* detection.
Do we really need the first bank to ends exactly at 0xf0000000? Or should we rather make sure that it doesn't go into that space? (so anything below that would be fine?). I assume this is the result of some experiments, what did you learn from those boards?
*/- if (t->u.ddr_mem.bank[t->u.ddr_mem.count] != 0xf0000000)
return 1;
-EINVAL?
- /*
* Iterate over the RAM banks. If the start address of bank 0* is less than or equal to 0x200000, set it to 0x200000 to* reserve room for A-TF. Make sure the size of bank 0 doesn't* bleed into the address space for hardware (starting at* 0xf0000000). Banks 1 and on can be defined as-is.*/- for (int i = 0; i < (t->u.ddr_mem.count); i++) {
This may result in out-of-bounds access. Indeed gd->bd->bi_dram is a build-time allocated array of size CONFIG_NR_DRAM_BANKS.
So I would recommend printing an error message if t->u.ddr_mem.count is bigger than CONFIG_NR_DRAM_BANKS. I assume we may want to still boot but with less RAM in that case? If we do, then only loop for the min between t->u.ddr_mem.count and CONFIG_NR_DRAM_BANKS.
if (i == 0) {if (t->u.ddr_mem.bank[i] <= 0x200000)gd->bd->bi_dram[i].start = 0x200000;elsegd->bd->bi_dram[i].start = t->u.ddr_mem.bank[i];tmp = gd->bd->bi_dram[i].start + t->u.ddr_mem.bank[(bank_cnt + i)];
This is incorrect, it may be offsetting up to 0x200000. If it's offset, you need to remove it from the address size.
""" if (t->u.ddr_mem.bank[i] <= 0x200000) tmp -= 0x200000; """
if (tmp > 0xf0000000)gd->bd->bi_dram[i].size = 0xf0000000 - gd->bd->bi_dram[i].start;
Shouldn't we do this check for all declared address spaces, not only the first one?
elsegd->bd->bi_dram[i].size = t->u.ddr_mem.bank[(bank_cnt + i)];
You may need to remove the 0x200000 offset here as well, e.g. with
"""
if (tmp > 0xf0000000) tmp = 0xf0000000;
gd->bd->bi_dram[i].size = tmp - gd->bd->bi_dram[i].start; """
Renaming tmp into end would probably also help figure out what it's supposed to contain.
} else {gd->bd->bi_dram[i].start = t->u.ddr_mem.bank[i];gd->bd->bi_dram[i].size = t->u.ddr_mem.bank[(bank_cnt + i)];}- };
You can make this for-loop logic a bit more readable (well, to me :) ) with:
""" /* Iterate over the RAM banks. */ /* If the start address of bank 0 * is less than or equal to 0x200000, set it to 0x200000 to * reserve room for A-TF. Make sure the size of bank 0 doesn't * bleed into the address space for hardware (starting at * 0xf0000000). */ if (t->u.ddr_mem.bank[0] <= 0x200000) gd->bd->bi_dram[0].start = 0x200000; else gd->bd->bi_dram[0].start = t->u.ddr_mem.bank[0];
tmp = gd->bd->bi_dram[0].start + t->u.ddr_mem.bank[bank_cnt];
if (tmp > 0xf0000000) gd->bd->bi_dram[0].size = 0xf0000000 - gd->bd->bi_dram[0].start; else gd->bd->bi_dram[0].size = t->u.ddr_mem.bank[bank_cnt];
for (int i = 1; i < (t->u.ddr_mem.count); i++) { gd->bd->bi_dram[i].start = t->u.ddr_mem.bank[i]; gd->bd->bi_dram[i].size = t->u.ddr_mem.bank[(bank_cnt + i)]; } """
Side question, do gd->bd->bi_dram have to store banks in a specific order (e.g. from lowest (0) to highest (0xfffffff) location in memory) or can it be random? If the former, is Rockchip guaranteeing us that the ATAGS will always be ordered properly or should we order them at runtime too?
Cheers, Quentin
Hi Chris and Quentin,
On 2024-03-27 11:51, Quentin Schulz wrote:
Hi Chris,
On 3/26/24 21:49, Chris Morgan wrote:
From: Chris Morgan macromorgan@hotmail.com
Add support for defining the usable RAM from ATAGs provided by the Rockchip binary TPL loader. This allows us to automatically account for necessary memory holes on RK3588 devices with 16GB of RAM or more, as well as ensure we can use the full amount of RAM available.
In the event we can't cleanly read the ATAG values from RAM or are not running an RK3588 board, simply fall back to the old method of detecting the RAM.
Tested on Indiedroid Nova with 4GB and 16GB of RAM.
Signed-off-by: Chris Morgan macromorgan@hotmail.com
arch/arm/mach-rockchip/sdram.c | 58 ++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+)
diff --git a/arch/arm/mach-rockchip/sdram.c b/arch/arm/mach-rockchip/sdram.c index 0d9a0aef6f..58b78466b0 100644 --- a/arch/arm/mach-rockchip/sdram.c +++ b/arch/arm/mach-rockchip/sdram.c @@ -10,6 +10,7 @@ #include <ram.h> #include <asm/global_data.h> #include <asm/io.h> +#include <asm/arch-rockchip/atags.h> #include <asm/arch-rockchip/sdram.h> #include <dm/uclass-internal.h>
@@ -35,12 +36,69 @@ struct tos_parameter_t { s64 reserve[8]; };
+/*
- Read the ATAGs to identify all the memory banks. If we can't do it
- cleanly return 1 to note an unsuccessful attempt, otherwise return
- 0 for a successful attempt.
Return a valid error code instead? Negative if possible?
- */
+int rockchip_atag_ram_banks(void) +{
- struct tag *t;
- int bank_cnt;
- size_t tmp;
- if (!CONFIG_IS_ENABLED(ARM64) && !CONFIG_IS_ENABLED(ROCKCHIP_RK3588))
CONFIG_IS_ENABLED should only be used if there is an xPL variant of the config symbol, IS_ENABLED should probably be used here. Also I would possible check for !IS_ENABLED(CONFIG_ROCKCHIP_EXTERNAL_TPL) instead.
return 1;- t = atags_get_tag(ATAG_DDR_MEM);
I believe this will not compile for non RK3588 because this function is not defined.
There are a few ways to handle this:
- always compile atags.c but have ifdefs there with inline functions
returning -ENOTSUPP or something like that.
- if (!t)
return 1;-ENOENT? -ENOXIO?
- bank_cnt = t->u.ddr_mem.count;
- /*
* Check to make sure the first bank ends at 0xf0000000, if itExplain what 0xf0000000 represents here.
You should use SDRAM_MAX_SIZE instead of 0xf0000000.
* does not fall back to the other methods of RAM bank* detection.Do we really need the first bank to ends exactly at 0xf0000000? Or should we rather make sure that it doesn't go into that space? (so anything below that would be fine?). I assume this is the result of some experiments, what did you learn from those boards?
The Rockchip TPL will report differently depending on model/build.
Some report regions that can be used as-is, other report actual memory. So we must handle the case where first bank is reported for 0 - SDRAM_MAX_SIZE and the other case when full memory, 0 - 8 GiB, is reported and skips the SDRAM_MAX_SIZE - 4GiB hw regs space.
Here are dumps of ddr_mem atag on RK3568/RK3588:
RK3588: https://gist.github.com/Kwiboo/1c020d37e3adbc9d0d79dc003d921977 RK3568: https://gist.github.com/Kwiboo/6d983693c79365b43c330eb3191cbace
count = number of banks bank[x] = start addr bank[x + count] = size
*/- if (t->u.ddr_mem.bank[t->u.ddr_mem.count] != 0xf0000000)
return 1;-EINVAL?
- /*
* Iterate over the RAM banks. If the start address of bank 0* is less than or equal to 0x200000, set it to 0x200000 to* reserve room for A-TF. Make sure the size of bank 0 doesn't* bleed into the address space for hardware (starting at* 0xf0000000). Banks 1 and on can be defined as-is.*/- for (int i = 0; i < (t->u.ddr_mem.count); i++) {
This may result in out-of-bounds access. Indeed gd->bd->bi_dram is a build-time allocated array of size CONFIG_NR_DRAM_BANKS.
So I would recommend printing an error message if t->u.ddr_mem.count is bigger than CONFIG_NR_DRAM_BANKS. I assume we may want to still boot but with less RAM in that case? If we do, then only loop for the min between t->u.ddr_mem.count and CONFIG_NR_DRAM_BANKS.
if (i == 0) {if (t->u.ddr_mem.bank[i] <= 0x200000)gd->bd->bi_dram[i].start = 0x200000;elsegd->bd->bi_dram[i].start = t->u.ddr_mem.bank[i];tmp = gd->bd->bi_dram[i].start + t->u.ddr_mem.bank[(bank_cnt + i)];This is incorrect, it may be offsetting up to 0x200000. If it's offset, you need to remove it from the address size.
""" if (t->u.ddr_mem.bank[i] <= 0x200000) tmp -= 0x200000; """
if (tmp > 0xf0000000)gd->bd->bi_dram[i].size = 0xf0000000 - gd->bd->bi_dram[i].start;Shouldn't we do this check for all declared address spaces, not only the first one?
elsegd->bd->bi_dram[i].size = t->u.ddr_mem.bank[(bank_cnt + i)];You may need to remove the 0x200000 offset here as well, e.g. with
"""
if (tmp > 0xf0000000) tmp = 0xf0000000;
gd->bd->bi_dram[i].size = tmp - gd->bd->bi_dram[i].start; """
Renaming tmp into end would probably also help figure out what it's supposed to contain.
} else {gd->bd->bi_dram[i].start = t->u.ddr_mem.bank[i];gd->bd->bi_dram[i].size = t->u.ddr_mem.bank[(bank_cnt + i)];}- };
You can make this for-loop logic a bit more readable (well, to me :) ) with:
""" /* Iterate over the RAM banks. */ /* If the start address of bank 0
- is less than or equal to 0x200000, set it to 0x200000 to
- reserve room for A-TF. Make sure the size of bank 0 doesn't
- bleed into the address space for hardware (starting at
- 0xf0000000).
*/ if (t->u.ddr_mem.bank[0] <= 0x200000) gd->bd->bi_dram[0].start = 0x200000; else gd->bd->bi_dram[0].start = t->u.ddr_mem.bank[0];
tmp = gd->bd->bi_dram[0].start + t->u.ddr_mem.bank[bank_cnt];
if (tmp > 0xf0000000) gd->bd->bi_dram[0].size = 0xf0000000 - gd->bd->bi_dram[0].start; else gd->bd->bi_dram[0].size = t->u.ddr_mem.bank[bank_cnt];
for (int i = 1; i < (t->u.ddr_mem.count); i++) { gd->bd->bi_dram[i].start = t->u.ddr_mem.bank[i]; gd->bd->bi_dram[i].size = t->u.ddr_mem.bank[(bank_cnt + i)]; } """
Side question, do gd->bd->bi_dram have to store banks in a specific order (e.g. from lowest (0) to highest (0xfffffff) location in memory) or can it be random? If the former, is Rockchip guaranteeing us that the ATAGS will always be ordered properly or should we order them at runtime too?
Based on dumps of multiple different versions and models, the banks have always been reported in correct order.
Regards, Jonas
Cheers, Quentin
participants (7)
-
Chris Morgan -
Chris Morgan -
Eugen Hristev -
Jonas Karlman -
Kever Yang -
Quentin Schulz -
Tom Rini