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

[Quentin Schulz]

Hi Chris,

On 3/26/24 21:49, Chris Morgan wrote:
> From: Chris Morgan <macromorgan at 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 at 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