Re: [PATCH 3/7] arm64: dts: ti: k3-am65: Switch to "ti,j721e-system-controller" compatible

[Andrew Davis <afd@ti.com>]

On 4/24/23 9:49 AM, Nishanth Menon wrote:
> Switch scm-conf to "ti,j721e-system-controller" compatible to be more
> specific.
> 
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
>   arch/arm64/boot/dts/ti/k3-am65-main.dtsi | 2 +-
>   arch/arm64/boot/dts/ti/k3-am65-mcu.dtsi  | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
> index 227573773b26..40fa631f2f3d 100644
> --- a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
> @@ -475,7 +475,7 @@ sdhci1: mmc@4fa0000 {
>   	};
>   
>   	scm_conf: scm-conf@100000 {
> -		compatible = "syscon", "simple-mfd";
> +		compatible = "ti,j721e-system-controller", "syscon", "simple-mfd";
>   		reg = <0 0x00100000 0 0x1c000>;
>   		#address-cells = <1>;
>   		#size-cells = <1>;
> diff --git a/arch/arm64/boot/dts/ti/k3-am65-mcu.dtsi b/arch/arm64/boot/dts/ti/k3-am65-mcu.dtsi
> index 5dfa31840e9c..566dc584d3f3 100644
> --- a/arch/arm64/boot/dts/ti/k3-am65-mcu.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-am65-mcu.dtsi
> @@ -7,7 +7,7 @@
>   
>   &cbass_mcu {
>   	mcu_conf: scm-conf@40f00000 {
> -		compatible = "syscon", "simple-mfd";
> +		compatible = "ti,j721e-system-controller", "syscon", "simple-mfd";

This node is not a "j721e-system-controller". Only the one in main could be
said to be one, but even it is different enough that this is not correct IMHO.
It almost seems like you are using "ti,j721e-system-controller" as a workaround
for the restriction on raw "syscon", "simple-mfd" nodes. And just replacing all
instance of those with something that avoids the warning.

What we should do here is turn both of these nodes into "simple-bus". The sub-nodes
themselves would describe what they are. This is the normal DT way vs having
all our device nodes pointing into one big "syscon" node with various offsets (which
makes it hard to see all users of a node and near impossible to work out the real
memory map in these "system-controller" nodes).

Worse, if the parent is a "syscon" then the whole memory region gets one big regmap
over it, and any child nodes that also build a regmap for their smaller sub-range
leads to having two regmaps pointing to the same memory area. This breaks some
assumptions around atomic access and reg caching.

Taking a quick look I see some of our sub-node drivers expecting the parent to
always be a syscon, others turn themselves into a syscon, and others still do the
normal "reg" mapping. What a mess..

To unwind this I'd suggest we do this:

  * Add support for these sub-node drivers to use the normal "reg" property by
    default if available, falling back to expecting the parent to be a syscon only
    for backwards compatibility.

  * Add "reg" properties to the sub-nodes.

  * Remove "syscon" from our system-controller nodes and instead use "simple-bus".
    Which more accurately describes what these regions are, and prevents issues like
    having a regmap over gaps (as these system-controller have gaps in between the
    sub device memory regions)

We would still have to add simple compatibles for the efuse and pcie mode/id regions,
but that is much more correct than hiding them in the device's node like done in patch
1/7 of this series.

Andrew

>   		reg = <0x0 0x40f00000 0x0 0x20000>;
>   		#address-cells = <1>;
>   		#size-cells = <1>;