Re: [PATCH 1/2] rockchip: rk3588: Add support for ATAG parsing

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