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

Mon Aug 11 19:45:33 CEST 2025

Hi Quentin,

On 8/11/2025 6:58 PM, Quentin Schulz wrote:
> 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).

The main reason is that end-users may want to remove the policy and just
disable the cores that are marked as bad and not the entire cluster.

E.g. for my personal builds I will be able to just remove or comment out
the policy part above, or if there is a policy change in the future the
removal part will handle such cases without having to be also changed.

On my RK3582 boards there is typically only one single bad core in cpu,
vdec or venc. To get most out of the SoC I may want to skip the policy
to run all working cores without too much code modification ;-)

> 
> [...]
>>>> +	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?

Yes, will fix this for v4.

> 
>> - 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 guess we could relax the logic to return early if rockchip,rk3582
already exists and only truncate and append if rockchip,rk3588s or
rockchip,rk3588 is found last, will try something like that for v4.

> 
>>> 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:

Agree, and I think that the "radxa,e52c", "rockchip,rk3582",
"rockchip,rk3588s" already defined the proper order we should use.

Regards,
Jonas

> 
> """
>   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