[PATCH 1/2] rockchip: Add initial RK3582 support

[Quentin Schulz]

Hi Jonas,

On 12/10/24 11:23 PM, Jonas Karlman wrote:
> The RK3582 SoC is a variant of the RK3588S with some IP blocks disabled.
> What blocks are disabled/non-working is indicated by ip-state in OTP.
> 
> This add initial support for RK3582 by using ft_system_setup() to mark
> any cpu and/or gpu node with status=fail as indicated by ip-state.
> 
> This apply same policy as vendor U-Boot for RK3582, i.e. two big cpu
> cores and the gpu is always failed/disabled.
> 
> Enable Kconfig option OF_SYSTEM_SETUP in board defconfig to make use of
> the required DT fixups for RK3582 board variants.
> 
> Signed-off-by: Jonas Karlman <jonas at kwiboo.se>
> ---
>   arch/arm/mach-rockchip/rk3588/rk3588.c | 128 +++++++++++++++++++++++++
>   1 file changed, 128 insertions(+)
> 
> diff --git a/arch/arm/mach-rockchip/rk3588/rk3588.c b/arch/arm/mach-rockchip/rk3588/rk3588.c
> index c1dce3ee3703..06e6318312b2 100644
> --- a/arch/arm/mach-rockchip/rk3588/rk3588.c
> +++ b/arch/arm/mach-rockchip/rk3588/rk3588.c
> @@ -7,6 +7,7 @@
>   #define LOG_CATEGORY LOGC_ARCH
>   
>   #include <dm.h>
> +#include <fdt_support.h>
>   #include <misc.h>
>   #include <spl.h>
>   #include <asm/armv8/mmu.h>
> @@ -185,6 +186,12 @@ int arch_cpu_init(void)
>   
>   #define RK3588_OTP_CPU_CODE_OFFSET		0x02
>   #define RK3588_OTP_SPECIFICATION_OFFSET		0x06
> +#define RK3588_OTP_IP_STATE_OFFSET		0x1d
> +
> +#define BAD_CPU_CLUSTER0		GENMASK(3, 0)
> +#define BAD_CPU_CLUSTER1		GENMASK(5, 4)
> +#define BAD_CPU_CLUSTER2		GENMASK(7, 6)
> +#define BAD_GPU				GENMASK(4, 1)
>   
>   int checkboard(void)
>   {
> @@ -230,3 +237,124 @@ int checkboard(void)
>   
>   	return 0;
>   }
> +
> +#ifdef CONFIG_OF_SYSTEM_SETUP
> +static int fdt_path_del_node(void *fdt, const char *path)
> +{
> +	int nodeoffset;
> +
> +	nodeoffset = fdt_path_offset(fdt, path);
> +	if (nodeoffset < 0)
> +		return nodeoffset;
> +
> +	return fdt_del_node(fdt, nodeoffset);
> +}
> +
> +static int fdt_path_set_name(void *fdt, const char *path, const char *name)
> +{
> +	int nodeoffset;
> +
> +	nodeoffset = fdt_path_offset(fdt, path);
> +	if (nodeoffset < 0)
> +		return nodeoffset;
> +
> +	return fdt_set_name(fdt, nodeoffset, name);
> +}
> +
> +int ft_system_setup(void *blob, struct bd_info *bd)
> +{
> +	static const char * const cpu_node_names[] = {
> +		"cpu at 0", "cpu at 100", "cpu at 200", "cpu at 300",
> +		"cpu at 400", "cpu at 500", "cpu at 600", "cpu at 700",
> +	};
> +	u8 cpu_code[2], ip_state[3];
> +	int parent, node, i, ret;
> +	struct udevice *dev;
> +
> +	if (!IS_ENABLED(CONFIG_ROCKCHIP_OTP) || !CONFIG_IS_ENABLED(MISC))
> +		return -ENOSYS;
> +
> +	ret = uclass_get_device_by_driver(UCLASS_MISC,
> +					  DM_DRIVER_GET(rockchip_otp), &dev);
> +	if (ret) {
> +		log_debug("Could not find otp device, ret=%d\n", ret);
> +		return ret;
> +	}
> +
> +	/* cpu-code: SoC model, e.g. 0x35 0x82 or 0x35 0x88 */
> +	ret = misc_read(dev, RK3588_OTP_CPU_CODE_OFFSET, cpu_code, 2);
> +	if (ret < 0) {
> +		log_debug("Could not read cpu-code, ret=%d\n", ret);
> +		return ret;
> +	}
> +
> +	log_debug("cpu-code: %02x %02x\n", cpu_code[0], cpu_code[1]);
> +
> +	/* skip on rk3588 */
> +	if (cpu_code[0] == 0x35 && cpu_code[1] == 0x88)
> +		return 0;
> +
> +	ret = misc_read(dev, RK3588_OTP_IP_STATE_OFFSET, &ip_state, 3);

Do you have information about what;s in the third byte? It's not used in 
this patch series for example, so wondering if it really makes sense to 
read it and dump it?

> +	if (ret < 0) {
> +		log_debug("Could not read ip-state, ret=%d\n", ret);
> +		return ret;
> +	}
> +
> +	log_debug("ip-state: %02x %02x %02x\n",
> +		  ip_state[0], ip_state[1], ip_state[2]);
> +
> +	if (cpu_code[0] == 0x35 && cpu_code[1] == 0x82) {
> +		/* policy: always disable gpu */
> +		ip_state[1] |= BAD_GPU;
> +
> +		/* policy: always disable one big core cluster */
> +		if (!(ip_state[0] & (BAD_CPU_CLUSTER1 | BAD_CPU_CLUSTER2)))
> +			ip_state[0] |= BAD_CPU_CLUSTER2;

That's a very interesting choice, wondering what's prompted this 
decision (which I assume comes from vendor tree :) ).

> +	}
> +
> +	if (ip_state[0] & BAD_CPU_CLUSTER1) {
> +		/* fail entire cluster when one or more core is bad */
> +		ip_state[0] |= BAD_CPU_CLUSTER1;
> +		fdt_path_del_node(blob, "/cpus/cpu-map/cluster1");
> +	}
> +
> +	if (ip_state[0] & BAD_CPU_CLUSTER2) {
> +		/* fail entire cluster when one or more core is bad */
> +		ip_state[0] |= BAD_CPU_CLUSTER2;
> +		fdt_path_del_node(blob, "/cpus/cpu-map/cluster2");
> +	} else if (ip_state[0] & BAD_CPU_CLUSTER1) {
> +		/* cluster nodes must be named in a continuous series */
> +		fdt_path_set_name(blob, "/cpus/cpu-map/cluster2", "cluster1");
> +	}
> +

Nitpick on readability (to me at least, matter of a taste :) )

Could suggest:

if (ip_state[0] & BAD_CPU_CLUSTER1) {
     /* cluster nodes must be named in a continuous series */
     fdt_path_set_name(blob, "/cpus/cpu-map/cluster2", "cluster1");

     if (!(ip_state[0] & BAD_CPU_CLUSTER2))
         /* cluster nodes must be named in a continuous series */
         fdt_path_set_name(blob, "/cpus/cpu-map/cluster2", "cluster1");
}

if (ip_state[0] & BAD_CPU_CLUSTER2) {
     /* fail entire cluster when one or more core is bad */
     ip_state[0] |= BAD_CPU_CLUSTER2;
     fdt_path_del_node(blob, "/cpus/cpu-map/cluster2");
}

For CPU clusters, it seems the bitmask matches how many cores there are 
in a cluster. Right now we are removing the whole cluster even if only 
one core in the cluster is broken. But I see later that we only disable 
the cores marked as bad. The dt-binding says clusters are for one or 
more cores, so maybe we are too eager here to remove a cluster even if 
not all cores in the cluster are broken? Do you know what are the 
possible side effects of removing a cluster but still having one of its 
cores enabled in DT?

> +	/* gpu: ip_state[1]: bit1~4 */
> +	if (ip_state[1] & BAD_GPU) {
> +		log_debug("fail gpu\n");
> +		fdt_status_fail_by_pathf(blob, "/gpu at fb000000");
> +	}
> +

Here I'm a bit perplexed. We don't define multiple cores in the GPU node 
on Linux kernel IIRC. I have not a single clue why the bitmask for the 
GPU is 3 bits, do you have any idea or information?

> +	parent = fdt_path_offset(blob, "/cpus");
> +	if (parent < 0) {
> +		log_debug("Could not find /cpus, parent=%d\n", parent);

Any reason for not making this an error message? Are we expecting this 
code path to be hit in "normal" behavior?

> +		return 0;
> +	}
> +
> +	/* cpu: ip_state[0]: bit0~7 */
> +	for (i = 0; i < 8; i++) {
> +		/* fail any bad cpu core */
> +		if (!(ip_state[0] & BIT(i)))
> +			continue;
> +
> +		node = fdt_subnode_offset(blob, parent, cpu_node_names[i]);
> +		if (node >= 0) {
> +			log_debug("fail cpu %s\n", cpu_node_names[i]);
> +			fdt_status_fail(blob, node);
> +		} else {
> +			log_debug("Could not find %s, node=%d\n",
> +				  cpu_node_names[i], node);

Any particular reason for making this a debug message rather than an 
error message?

Cheers,
Quentin