Hi Chris,
On 3/26/24 21:49, Chris Morgan wrote:
From: Chris Morgan <macromor...@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 <macromor...@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
