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@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@0", "cpu@100", "cpu@200", "cpu@300","cpu@400", "cpu@500", "cpu@600", "cpu@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@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