Updated address-map properties for Arm v8-M and similar

[mbolivar-nordic]

Problem statement

The existing address-map property in the CPU cluster binding does
not properly account for multiple possible cluster address maps
depending on runtime CPU state.

Example

The motivating example is an Arm v8-M CPU with TrustZone. SoCs with
these CPUs frequently have different secure- and non-secure addresses
for identical peripherals. That is, the CPU clusters in these SoCs
have address maps that vary based on the current execution level of
the CPU.

Proposal

1. Extend the "cpus,cluster" binding to allow properties matching the
   pattern address-map-* to allow for multiple address maps.

   This draws inspiration from the pinctrl binding,
   which allows users to select multiple pin configurations, one per
   desired state, using pinctrl-<N> properties. (In the address map
   situation, we are using multiple properties to describe hardware
   rather than configure it, but the mechanism is similar.)

2. Extend the hardware specific portions of the "openamp,domain-v1"
   binding to allow the domain to express which address map to use,
   either implicitly or explicitly.

Application to Arm v8-M

Here are a couple of options for how we could use this. Other ideas
welcome.

Option 1

With the following macros:

#define EXECUTION_LEVEL_SECURE (1U << 31)
#define EXECUTION_LEVEL_NONSECURE (0U << 31)

And the following example CPU cluster:

	cpu0: cpu0-cluster {
		compatible = "cpus,cluster";
		#address-cells = <1>;
		#size-cells = <0>;
		#ranges-address-cells = <1>;
		#ranges-size-cells = <1>;

		/* Non-secure address map. */
		address-map =
			...
			<0x40000000 &peripherals 0x0 0x10000000>;
		/* Secure address map. */
		address-map-secure =
                        ...,
			<0x50000000 &peripherals 0x0 0x10000000>;

		cpu@0 {
			#address-cells = <1>;
			#size-cells = <1>;
			device_type = "cpu";
			compatible = "arm,cortex-m33f";
			reg = <0>;  /* ... */
		};
	};

We could define the following domains:

	domains {
		domain_cpu0 {
			compatible = "openamp,domain-v1";
			cpus = <&cpu0 0x1 EXECUTION_LEVEL_SECURE>;
                        /* ... */
                };

		domain_cpu0_ns {
			compatible = "openamp,domain-v1";
			cpus = <&cpu0 0x1 EXECUTION_LEVEL_NONSECURE>;
                        /* ... */
                };
        };

With the definition in the binding for "arm,cortex-m33f" CPUs that the
secure execution level should use the address-map-secure property,
and analogously for the non-secure execution level, we could allow the
generated DTS files for a given domain to automatically apply the
correct address map in this situation.

Option 2

We could be more explicit by changing the above as follows:

	cpu0: cpu0-cluster {
		/* Non-secure address map. */
		address-map-0 =
			...
			<0x40000000 &peripherals 0x0 0x10000000>;
		/* Secure address map. */
		address-map-1 =
                        ...,
			<0x50000000 &peripherals 0x0 0x10000000>;

                address-map-names = "non-secure", "secure";
	};

And then extend the domain binding so we could, for example, do
something like this:

	domains {
		domain_cpu0 {
			compatible = "openamp,domain-v1";
			cpus = <&cpu0 0x1 EXECUTION_LEVEL_SECURE>;
                        address-map-name = "secure";
                };
        };

with the same effect.

This is more redundant and introduces room for error, however.

[sstabellini]

This topic was discussed and Option #1 was my initial proposal. The preference of Rob and others in the Device Tree community was to add an extra field to address-map instead.

	/* extending address-map with a secure/non-secure execution mode cell */
	address-map = <0x0 0xff110000 &amba 0xff110000 0x1000
				   0x1 0xc0110000 &amba 0xc0110000 0x1000>;

where the initial 0x0 and 0x1 mark whether the mapping is non-secure or secure.

[mbolivar-nordic]

That proposal doesn't seem to require a secure-reg, though, right? And I suppose we could still apply the "pick the right map based on the execution level" trick given the CPU compatible, right?

[mbolivar-nordic]

Either way it seems like we may want to lift the information about the CPU into the compatible of the cluster node; something like this:

	cpu0: cpu0-cluster {
		compatible = "cpus,cluster", "arm,v8-m-cluster"; /* ... */
        };

Since the address-map property or properties logically belong to the cluster, the binding for the cluster itself should be where the semantics of the property/ies should be specified, not the individual CPUs.

[sstabellini]

Yes, the proposal does not require secure-reg.

If you need to express different addresses for individual peripheral, it would require one of the two (not both):

1. one more cell in reg (#address-cells = <3>), e.g.: reg = <0x1 0x0 0xff000000 0x0 0x1000>

2. use ranges at the bus level:

   amba {
   compatible = "secure-bus";
   #address-cells = <0x1>;
   #size-cells = <0x1>;
   /* non-secure mapping /
   ranges = <0xff110000 0x0 0xff110000 0x1000
   / secure mapping */
   0xff110000 0x1 0xc0110000 0x1000>;

    timer0: timer@ff110000 {
    	compatible = "cdns,ttc";
    	status = "okay";
   
    	/* single address */
    	reg = <0xff110000 0x1000>
    };
   

   };

here we are using ranges under amba to map a single address to 2 different addresses for secure and non-secure.

But I think that we can avoid doing that if we rely only on address-map as in my other comment above.

[sstabellini]

I am OK with adding a cpu-specific compatible string to the cluster because, as you wrote, it makes sense logically. However, it also needs to be duplicated at the cpu level I think to be "compatible" with the device tree property. We cannot remove it from cpu because we are using the same definition of the cpu node as the regular device tree.

[mbolivar-nordic]

Just to clarify my question here, I'm asking if we can do something like this as an option 3:

	cpu0: cpu0-cluster {
		compatible = "cpus,cluster", "arm,cortex-m33f-cluster";
		#address-cells = <1>;
		#size-cells = <0>;
		#ranges-address-cells = <1>;
		#ranges-size-cells = <1>;

		address-map =
			<EXECUTION_LEVEL_NONSECURE 0x40000000 &peripherals 0x0 0x10000000>;
			<EXECUTION_LEVEL_SECURE 0x50000000 &peripherals 0x0 0x10000000>;

		cpu@0 {
			#address-cells = <1>;
			#size-cells = <1>;
			device_type = "cpu";
			compatible = "arm,cortex-m33f";
			reg = <0>;  /* ... */
		};
	};

	domains {
		foo {
			cpus = <&cpu0 0x1 EXECUTION_LEVEL_NONSECURE>;
		};
		bar {
			cpus = <&cpu0 0x1 EXECUTION_LEVEL_SECURE>;
		};
	};

And then domains foo and bar "automatically" will only get the correct elements from the address-map by matching against the execution level mask in a cluster-dependent way. That is, since the cluster has compatible "arm,cortex-m33f-cluster", we will specify the extra semantics in a binding for that compatible.

If I understand correctly, then my follow-up question is: don't we now need a new #ranges-flags-cells property in the CPU cluster, which specifies the number of cells needed to fit the new "flags" field in each address map entry?

[sstabellini]

Yes you understand correctly, but there is one difficulty here:

compatible = "cpus,cluster", "arm,cortex-m33f-cluster";
[...]
compatible = "arm,cortex-m33f";

I don't know if there is a precedence for defining a compatible string (arm,cortex-m33f-cluster) as an extension of an existing other compatible string ("arm,cortex-m33f" + "-cluster").

If I understand correctly, then my follow-up question is: don't we now need a new #ranges-
flags-cells property in the CPU cluster, which specifies the number of cells needed to fit the
new "flags" field in each address map entry?

No because the "flags" field in each address map entry would actually be considered the first cell of the address. So address cells go up by 1.

[mbolivar-nordic]

No because the "flags" field in each address map entry would actually be considered the first cell of the address. So address cells go up by 1.

@sstabellini I finally got around to implementing this and I have some follow ups.

It looks like I should be doing this in cpu0-cluster:

		#ranges-address-cells = <2>;

and then assuming that the first cell is always a flags cell.

That's OK for me, but:

• Won't that mean that "every system devicetree in the world" will need to add 1 to its #ranges-address-cells, to make room for flags? That seems like it will be really disruptive for you at AMD and for any other existing system DT users.
• What if we need more than 32 bits in the future? With the hard-coded assumption that we only have 1 flags cell, we seem to be stuck. It seems weird to build in future-proofing in other places (like #access-flags-cells), but not do it here.

Overall it seems a lot less disruptive and more future-proof to add a new #ranges-flags-cells -- what am I missing?

[sstabellini]

Hi Marti,
Actually I don't disagree with you here, but that was the feedback from Rob and others in the DT community.

For AMD this is not a problem because we never have different address maps for secure/non-secure accesses.