Re: [PATCH DNM v4 6/6] arm64: dts: qcom: kaanapali: Add MPAM MSC nodes for the L2 caches

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] arm64: dts: qcom: kaanapali: undocumented qcom,kaanapali-l2-cache compatible
- [Medium] arm64: dts: qcom: kaanapali: invalid root L2 cache MMIO nodes and compatibles
--

commit b790aa2c6d3cc2130d835b988b9078c67a5328af
Author: Yin Li <yin.li@oss.qualcomm.com>

arm64: dts: qcom: kaanapali: Add MPAM MSC nodes for the L2 caches

This commit moves the L2 cache nodes l2_0 and l2_1 from the CPU nodes to the
root node in arch/arm64/boot/dts/qcom/kaanapali.dtsi and adds disabled child
arm,mpam-msc nodes for each L2 cache.

> diff --git a/arch/arm64/boot/dts/qcom/kaanapali.dtsi b/arch/arm64/boot/dts/qcom/kaanapali.dtsi
> index e48a179d07220..10aee29a1ba8a 100644
> --- a/arch/arm64/boot/dts/qcom/kaanapali.dtsi
> +++ b/arch/arm64/boot/dts/qcom/kaanapali.dtsi

[ ... ]

> @@ -532,6 +520,46 @@ soccp_smp2p_in: slave-kernel {
>  		};
>  	};
>  
> +	l2_0: cache@19870000 {
> +		compatible = "qcom,kaanapali-l2-cache", "cache";
> +		cache-level = <2>;
> +		cache-unified;
> +		cache-size = <0xc00000>;
> +		reg = <0x0 0x19870000 0x0 0x2000>;
> +		ranges = <0x0 0x0 0x19870000 0x2000>;
> +
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +
> +		msc@0 {
> +			compatible = "arm,mpam-msc";
> +			reg = <0x0 0x2000>;
> +			arm,not-ready-us = <1>;
> +			status = "disabled";
> +		};
> +
> +	};

[Severity: Medium]
Does adding compatible = "qcom,kaanapali-l2-cache", "cache" to l2_0 and l2_1
in arch/arm64/boot/dts/qcom/kaanapali.dtsi cause dtbs_check and checkpatch.pl
regressions for kaanapali-mtp.dtb and kaanapali-qrd.dtb?

The compatible string "qcom,kaanapali-l2-cache" does not appear to be
documented in Documentation/devicetree/bindings/.

Without a binding schema for "qcom,kaanapali-l2-cache" (and using the node
name cache@... rather than cache-controller@... as shown in
Documentation/devicetree/bindings/arm/arm,mpam-msc.yaml), dtbs_check flags
"qcom,kaanapali-l2-cache" as undocumented and validates l2_0 and l2_1 only
against the generic "cache" schema, which rejects the unit-address on
cache@19870000 and cache@1a870000 as well as the reg, ranges, #address-cells,
#size-cells, and msc@0 properties.

Should a DT binding schema be added for "qcom,kaanapali-l2-cache" (or an
existing documented cache-controller binding be used) before using it in
arch/arm64/boot/dts/qcom/kaanapali.dtsi?

> +
> +	l2_1: cache@1a870000 {
> +		compatible = "qcom,kaanapali-l2-cache", "cache";
> +		cache-level = <2>;
> +		cache-unified;
> +		cache-size = <0xc00000>;
> +		reg = <0x0 0x1a870000 0x0 0x2000>;
> +		ranges = <0x0 0x0 0x1a870000 0x2000>;
> +
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +
> +		msc@0 {
> +			compatible = "arm,mpam-msc";
> +			reg = <0x0 0x2000>;
> +			arm,not-ready-us = <1>;
> +			status = "disabled";
> +		};
> +
> +	};
> +
>  	soc: soc@0 {
>  		compatible = "simple-bus";

[Severity: Medium]
Is it intentional to place the MMIO-mapped l2_0 (cache@19870000) and l2_1
(cache@1a870000) nodes directly under the root node rather than under soc@0,
and to assign the firmware-restricted 0x2000-byte MSC MMIO regions directly to
the enabled parent cache nodes via reg?

On Kaanapali, other MMIO devices in the 0x0..0x10_00000000 address range live
under soc@0:

arch/arm64/boot/dts/qcom/kaanapali.dtsi:
	soc: soc@0 {
		compatible = "simple-bus";

		#address-cells = <2>;
		#size-cells = <2>;
		dma-ranges = <0x0 0x0 0x0 0x0 0x10 0x0>;
		ranges = <0x0 0x0 0x0 0x0 0x10 0x0>;

Because l2_0 and l2_1 are placed directly under the root node and remain
enabled while only their child msc@0 nodes are disabled,
of_platform_default_populate() -> of_platform_device_create_pdata() will
unconditionally instantiate platform devices (19870000.cache and
1a870000.cache) claiming the restricted MSC MMIO resources on every boot.

In addition, the commit message states:

"Each MSC is nested directly under its L2 cache node so that the cache node
is the MSC's parent device and its CPU affinity is derived from that cache."

and:

"This DTS is therefore only an example for other platform DT authors and is
not intended to be merged."

However, in drivers/resctrl/mpam_devices.c:mpam_dt_create_foundling_msc():

			if (!of_platform_device_create(child, NULL, NULL))
				pr_err("Failed to create MSC device for %pOF\n", child);

NULL is passed as the parent struct device, so the L2 cache node is only the
parent OF device_node rather than the MSC platform device's parent device.

Should the cache nodes omit duplicating the child MSC reg window on the
parent node (or remain under /cpus or /soc@0 as appropriate), and should this
patch be marked with an RFC prefix if it is not intended to be merged?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028-mpam-resctrl-dt-knp-support-v4-0-bdda1e445cba@oss.qualcomm.com?part=6