[PATCH v2 1/3] rockchip: Add initial RK3582 support

[Quentin Schulz]

Hi Jonas,

On 8/11/25 6:36 PM, Jonas Karlman wrote:
> Hi Quentin,
> 
> On 8/11/2025 2:11 PM, Quentin Schulz wrote:
>> Hi Jonas,
>>
>> On 8/4/25 8:16 PM, Jonas Karlman wrote:
[...]
>>> +		/* policy: always fail one rkvdec core on rk3583 */
>>> +		if (!(ip_state[1] & (FAIL_RKVDEC0 | FAIL_RKVDEC1)))
>>
>> You could want to only check FAIL_RKVDEC0 as FAIL_RKVDEC1 is a bit (and
>> not a bitfield so we don't need to set the whole bitfield in the event
>> it's only partially set like for CPU clusters). No strong opinion.
> 
> Main reason for this is to keep all policy logic more consistent.
> 

Fair enough.

>>
>>> +			ip_state[1] |= FAIL_RKVDEC1;
>>> +	}
>>> +
>>> +	/* policy: always fail one rkvenc core on rk3582/rk3583 */
>>> +	if (!(ip_state[2] & (FAIL_RKVENC0 | FAIL_RKVENC1)))
>>> +		ip_state[2] |= FAIL_RKVENC1;
>>> +
>>> +	log_debug("ip-state: %02x %02x %02x (policy)\n",
>>> +		  ip_state[0], ip_state[1], ip_state[2]);
>>> +
>>> +	/* cpu cluster1: ip_state[0]: bit4~5 */
>>> +	if ((ip_state[0] & FAIL_CPU_CLUSTER1) == FAIL_CPU_CLUSTER1) {
>>
>> No need for the == FAIL_CPU_CLUSTER1 check as you'll have set the whole
>> bitfield earlier even if only one is set in the original ip_state from
>> the register. No strong opinion.
> 
> Main reason for keeping this is that if you comment out the policy part
> above, you could end up with a single core removed, so this extra check

No you cannot at this point in the code :)

Earlier you do:

+	/* policy: fail entire big core cluster when one or more core is bad */
+	if (ip_state[0] & FAIL_CPU_CLUSTER1)
+		ip_state[0] |= FAIL_CPU_CLUSTER1;

This means that all bits in FAIL_CPU_CLUSTER1 bitfield are set if one is 
set.

This means that if you check

+ if (ip_state[0] & FAIL_CPU_CLUSTER1)

later on in the code, necessarily if it matches it means all CPUs are 
disabled in the cluster.

The only benefit is if you are anticipating removing this policy in the 
future where then the cpu-map shouldn't be removed if not all cores 
aren't failed. As it is today, it is not a necessary check (but I guess 
it doesn't hurt, so keep it if you feel more comfortable with it, this 
is really just me nitpicking).

[...]
>>> +	parent = fdt_path_offset(blob, "/cpus");
>>> +	if (parent < 0) {
>>> +		log_err("Could not find /cpus, parent=%d\n", parent);
>>> +		return parent;
>>> +	}
>>> +
>>> +	/* 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_err("Could not find %s, node=%d\n",
>>> +				cpu_node_names[i], node);
>>> +			return node;
>>> +		}
>>> +	}
>>
>> I would move this closer to the renaming/removing of cpu-map. No strong
>> opinion though.
> 
> Sounds good, I think this is just leftover from thinking we should
> continue to load Linux in case of an error, and just trying to fail as
> much as possible before there is a chance of an early return.
> 
>>
>> I'm wondering why you aren't using a direct path like for other
>> controllers, e.g. fdt_status_fail_by_pathf(blob, cpu_node_names[i]) by
>> simply prefixing each node_name with /cpus/ ?
> 
> Good question, I think it may initially have been that I updated the
> ppi-partitions affinity using phandles to the cpu nodes. However, that
> part was removed before sending this out, will rework this for a v4.
> 

Then you would be matching the same workflow as other removals earlier 
in the code and won't have to mind about the return-early scenario you 
were concerned about just above.

>>
>>> +
>>> +	node = fdt_path_offset(blob, "/");
>>> +	if (node < 0) {
>>> +		log_err("Could not find /, node=%d\n", node);
>>> +		return node;
>>> +	}
>>> +
>>> +	snprintf(soc_comp, sizeof(soc_comp), "rockchip,rk35%x", cpu_code[1]);
>>> +
>>> +	for (i = 0, comp_len = 0;
>>> +	     (comp = fdt_stringlist_get(blob, node, "compatible", i, &len));
>>> +	     i++) {
>>> +		/* stop at soc compatible */
>>> +		if (!strcmp(comp, soc_comp) ||
>>> +		    !strcmp(comp, "rockchip,rk3588s") ||
>>> +		    !strcmp(comp, "rockchip,rk3588"))
>>> +			break;
>>> +
>>> +		log_debug("compatible[%d]: %s\n", i, comp);
>>> +		comp_len += len + 1;
>>> +	}
>>> +
>>
>> If it happens that none of the expected strings are found in the
>> compatible property, we should probably return early.
> 
> If none of them is found this logic will just try to append the two
> compatibles at the end of any existing compatible. Should be safe?
> 

Mmmmmm, you're right, it should be fine I think, but maybe we don't want 
to add rockchip,rk3588s if the DT only has e.g. myvendor,rk3582-myboard?

> The four main scenarios I try to anticipate is:
> - rk3582 boards: <board>, rockchip,rk3582, rockchip,rk3588s (do nothing)

This you aren't doing at the moment, maybe we should return early if 
soc_comp is found in the compatible list?

> - rk3588s boards: <board>, rockchip,rk3588s (inject)
> - rk3588-generic: rockchip,rk3588 (replace)
> - other: <board>, <wrong soc> (append)
> 
>>
>>> +	/* truncate to only include board compatible */
>>> +	fdt_setprop_placeholder(blob, node, "compatible", comp_len, &data);
>>> +
>>> +	/* append soc compatible */
>>> +	fdt_appendprop_string(blob, node, "compatible", soc_comp);
>>> +	fdt_appendprop_string(blob, node, "compatible", "rockchip,rk3588s");
>>
>> I would rather insert rk3582/rk3583 before the rk3588/rk3588s and keep
>> the rest untouched instead of truncating to rockchip,rk3588/rk3588s and
>> then adding rockchip,rk3588s back only.
> 
> That is what this is trying to do without having to add too complex code.
> 
> We can only replace the full nil separated string array, and adding
> logic to re-construct a single working string became too complex. We
> already had helper functions that could be used even if they are a
> little bit inefficient by causing three fdt prop value replace.
> 
> The truncating changes the string length to where first (or none) soc
> compatible is found to then insert correct soc compatible.
> 

Yes, but if downstream decides to do

rockchip,rk3582, rockchip,rk3588s, <board>

We are screwed :) as we'll return only the first two. Does the user not 
following the spec deserve to have their DT broken? Maybe /me shrugs

>> I wouldn't want my device tree
>> to be modified too much and we don't know what we'll get from downstream
>> )I'm not necessarily talking about Rockchip's vendor kernel here) DTBs
>> for example, so truncating doesn't seem to be the best plan? What do you
>> think?
> 
> We could possible make this very simple by just appending soc compatible
> when it is missing and not care about the compatible order.
> 
> My intention was to ensure that we always end up with:
> 
>    <board>, [<...>, *] rockchip,rk3582, rockchip,rk3588s
> 
> With a simplified append it may instead end up as:
> 
>    <board>, [<...>, *] rockchip,rk3588s, rockchip,rk3582
> 
> Does the order matter or should I just drop all this and replace with
> something simple like:
> 
>    ret = fdt_stringlist_search(blob, node, "compatible", soc_comp);
>    if (ret == -FDT_ERR_NOTFOUND)
>    	fdt_appendprop_string(blob, node, "compatible", soc_comp);
> 


Nope, this doesn't follow the DT spec then:

"""
  The property
value consists of a concatenated list of null terminated strings, from 
most specific to most general.
"""

though this is said for the "compatible" property of a device node. For 
the root node the following is said:

"""
Specifies a list of platform architectures with
which this platform is compatible. This prop-
erty can be used by operating systems in select-
ing platform specific code. The recommended
form of the property value is:
"manufacturer,model"
"""

c.f. 
https://github.com/devicetree-org/devicetree-specification/releases/download/v0.4/devicetree-specification-v0.4.pdf

Cheers,
Quentin