New subject: [PATCH 0/3] system device tree initial set of bindings

12 Dec 2019

      On 12/9/19 8:25 PM, Stefano Stabellini via System-dt wrote:
...
On Mon, 2 Dec 2019, Rob Herring wrote:
...
On Thu, Nov 07, 2019 at 02:13:18PM -0800, Stefano Stabellini wrote:
...
Hi all,
This small series introduces a few key concepts needed to make system
device tree a reality:

cpus,cluster
indirect-bus
address-map

Hi,
On ST SoCs changing of domain not only impact memory and interrupt 
mapping but also properties like status, clocks or pinctrl. If I compare 
our TF-A, U-Boot, kernel or Optee device trees, I would say that 
generate them require to be able to overwrite, delete or add properties 
and nodes. It is close to apply overlays.
If I understand well we are going in a direction that may require to 
define new concepts like address-map for each properties that we would 
like to change per domain, right ? Does that sound achievable if we have 
to apply that logic to all properties and node ?
Would not it be better to describe theses changes in an /overlay (or 
/chosen) node and let a tool apply them and generate the devicetrees per 
domain ?
Regards,
Benjamin
...
...
...
We discussed a few times making address-map a property that can be used
by any bus masters, including a PCI RC for instance. Thus, I tried not
to tie address-map with cpus,cluster. Let me know if you would like to
make address-map a cpus,cluster specific property instead.
Each patch introduces references to other sections introduced by the
other patches. In other words, the patch series doesn't bisect properly,
but I don't know if that is important for devicetree spec changes.
It's a single feature, so a single patch is fine.
Even better. Are you OK with squashing it on commit, or do you prefer I
collapse all patches in this series into a single one? I think that
keeping it as a patch series for now should make it easier to read.
...
...
Nothing related to interrupts is introduced because the existing
bindings seem to be sufficient for the task.
I have also attached a complete example where the properties are used,
and also interrupts are handled.
In the future, send examples inline. I'm more interested in the examples
than spec text at this point. While a real example is good, it would
also be nice to cut it down to only the interesting parts.
OK
...
...
Cheers,
Stefano
/dts-v1/;
/ {
   compatible = "xlnx,versal-vc-p-a2197-00-revA", "xlnx,versal-vc-p-a2197-00", "xlnx,versal-vc-p-a2197", "xlnx,versal";
   #address-cells = <0x2>;
   #size-cells = <0x2>;
   model = "Xilinx Versal A2197 Processor board revA";
cpus {
   	#address-cells = <0x1>;
   	#size-cells = <0x0>;
   	compatible = "cpus,cluster";
cpu@0 {
	compatible = "arm,cortex-a72", "arm,armv8";
	device_type = "cpu";
	enable-method = "psci";
	operating-points-v2 = <0x1>;
	reg = <0x0>;
	cpu-idle-states = <0x2>;
	clocks = <0x3 0x4d>;
};

cpu@1 {
	compatible = "arm,cortex-a72", "arm,armv8";
	device_type = "cpu";
	enable-method = "psci";
	operating-points-v2 = <0x1>;
	reg = <0x1>;
	cpu-idle-states = <0x2>;
};

idle-states {
	entry-method = "psci";

	cpu-sleep-0 {
		compatible = "arm,idle-state";
		arm,psci-suspend-param = <0x40000000>;
		local-timer-stop;
		entry-latency-us = <0x12c>;
		exit-latency-us = <0x258>;
		min-residency-us = <0x2710>;
		phandle = <0x2>;
	};
};

};
cpus_r5: cpus_r5 {
The node naming here bothers me as it violates the practice of node
names being generic. While yes, that's pretty nit-picky, it seems like
bad design to abandon that from the start. The way around this is
unit-addresses (and therefore reg). A unit-address would be also be
helpful I think for additional things such as the domain configuration.
However, it creates 2 problems. First, what's the numbering? cpus@0,
cpus@1, etc.? Just making up 0-N is not ideal (though acceptable).
Second, there's only 1 number space (and cell sizes) at any given level
and the root level already has one. You could do something like this:
cpus {
   cpus-cluster@0 {
     reg = <0>;
     cpu@0 {};
   };
   cpus-cluster@1 {
     reg = <1>;
     cpu@0 {};
   };
}
While that breaks the 'works on current OS's', it's a trivial change to
support.
Either way is OK for me. However, my suggestion would be not to break
'works on current OS's' for this issue. I would rather follow your
suggestion of using 0-N as numbering although I realize it is not ideal:
cpus {
       /* default cpus stuff */
   };

   cpus-cluster@0 {
       /* R5 stuff */
   };
...
We have to consider the same issue for other top-level nodes: chosen,
aliases, reserved-memory.
...
#address-cells = <0x1>;
#size-cells = <0x0>;
compatible = "cpus,cluster";

#ranges-size-cells = <0x1>;
#ranges-address-cells = <0x1>;

address-map = <0xf1000000 &amba 0xf1000000 0xeb00000>,
              <0xf9000000 &amba_rpu 0xf9000000 0x10000>,
              <0x0 &memory@00000000 0x0 0x80000000>,

Pretty sure that doesn't compile...
I don't think this scales very well. The 'default' cpus without an
'address-map' can access everything, but the R5 (and every other
cluster) can only access what's listed. If 90% of the address map is the
same, we should only have to describe the translations for the 10% that
are different.
I like that way of thinking because it could simplify things. We could
say that a cpus cluster gets all the root node mappings by default? The
only thing left would be indirect-bus nodes which don't map to the root
node automatically. In this example, only amba_rpu which is consistent
with the address above. It would become for the R5 cluster:
address-map = <0xf9000000 &amba_rpu 0xf9000000 0x10000>,
              <0x0 &tcm 0xFFE90000 0x10000>;

However, the problem is that amba_apu has overlapping addresses with
amba_rpu, so amba_apu would have to become an indirect-bus too, and that
would break 'works on current OS's'. In fact, the cpus node would have
to become:
cpus {
     address-map = <0xf9000000 &amba_apu 0xf9000000 0x10000>;
 [...]

};
which wouldn't work with current OSes. I am fine with this but I wanted
to provide all information :-)
...
...
              <0x0 &tcm 0xFFE90000 0x10000>;

cpu@0 {
	compatible = "arm,cortex-r5";
	device_type = "cpu";
	reg = <0x0>;
};

cpu@1 {
	compatible = "arm,cortex-r5";
	device_type = "cpu";
	reg = <0x1>;
};

};
cpu_opp_table {
   	compatible = "operating-points-v2";
   	opp-shared;
   	phandle = <0x1>;
opp00 {
	opp-hz = <0x0 0x47868bf4>;
	opp-microvolt = <0xf4240>;
	clock-latency-ns = <0x7a120>;
};

opp01 {
	opp-hz = <0x0 0x23c345fa>;
	opp-microvolt = <0xf4240>;
	clock-latency-ns = <0x7a120>;
};

opp02 {
	opp-hz = <0x0 0x17d783fc>;
	opp-microvolt = <0xf4240>;
	clock-latency-ns = <0x7a120>;
};

opp03 {
	opp-hz = <0x0 0x11e1a2fd>;
	opp-microvolt = <0xf4240>;
	clock-latency-ns = <0x7a120>;
};

};
dcc {
   	compatible = "arm,dcc";
   	status = "okay";
   };
fpga {
   	compatible = "fpga-region";
   	fpga-mgr = <0x4>;
   	#address-cells = <0x2>;
   	#size-cells = <0x2>;
   };
versal_fpga {
   	compatible = "xlnx,versal-fpga";
   	phandle = <0x4>;
   };
amba_apu: amba_apu {
This should have an address given 'address-map' defines one.
bus@f100000
(Nit: Don't use '_' in node names. dtc will tell you this).
Actually 'address-map' does not define an address for this bus:
address-map is only specified at cpus_r5 which cannot access amba_apu.
Maybe you are thinking of amba (accessible by both clusters) or amba_rpu
(accessible by cpus_r5 only)? More on this below.
amba_apu gets a mapping into the parent address space because it is a
"simple-bus". The lowest of the addresses of the devices under amba_apu
is 0xf9000000, so I guess it would be:
bus@f9000000
If that is correct, and if we want to use the same node naming scheme
for both amba_apu and amba_rpu, then we would have a "duplication"
issue. The problem is that actually the base address is the same for
both amba_apu (only accessible by cpus) and amba_rpu (only accessible by
cpus_r5). They would both end up being called "bus@f9000000".
One suggestion is to rename amba_apu, which is a simple-bus, to
bus@f9000000, and to rename amba_rpu, which is an indirect-bus, to
indirect-bus@0 because indirect-map is not automatically mapped to the
parent address space. There is no root node mapping by default.
Finally we could rename amba to bus@f1000000 as you suggested:
amba_apu: bus@f9000000 {
       /* apu only bus */
   };
amba_rpu: indirect-bus@0 {
       /* rpu only bus */
   };
amba: bus@f1000000 {
       /* everything else */
   };
If we want to change amba_apu to also be an indirect-bus (see discussion
above) then they would be named indirect-bus@0 and indirect-bus@1.
...
...
compatible = "simple-bus";
#address-cells = <0x2>;
#size-cells = <0x2>;
ranges;

gic_a72: interrupt-controller@f9000000 {
	compatible = "arm,gic-v3";
	#interrupt-cells = <0x3>;
	#address-cells = <0x2>;
	#size-cells = <0x2>;
	ranges;
	reg = <0x0 0xf9000000 0x0 0x80000 0x0 0xf9080000 0x0 0x80000>;
	interrupt-controller;
	interrupt-parent = <&gic_a72>;
	interrupts = <0x1 0x9 0x4>;
	num_cpus = <0x2>;
	num_interrupts = <0x60>;
	phandle = <0x5>;

	gic-its@f9020000 {
		compatible = "arm,gic-v3-its";
		msi-controller;
		msi-cells = <0x1>;
		reg = <0x0 0xf9020000 0x0 0x20000>;
		phandle = <0x1b>;
	};
};

iommu: smmu@fd800000 {
    compatible = "arm,mmu-500";
    status = "okay";
    reg = <0x0 0xfd800000 0x0 0x40000>;
    stream-match-mask = <0x7c00>;
    #iommu-cells = <0x1>;
    #global-interrupts = <0x1>;
    interrupt-parent = <&gic_a72>;
    interrupts = <0x0 0x6b 0x4 0x0 0x6b 0x4 0x0 0x6b 0x4 0x0 0x6b 0x4 0x0 0x6b 0x4 0x0 0x6b 0x4 0x0 0x6b 0x4 0x0 0x6b 0x4 0x0 0x6b 0x4 0x0 0x6b 0x4 0x0 0x6b 0x4 0x0 0x6b 0x4 0x0 0x6b 0x4 0x0 0x6b 0x4 0x0 0x6b 0x4 0x0 0x6b 0x4 0x0 0x6b 0x4 0x0 0x6b 0x4 0x0 0x6b 0x4 0x0 0x6b 0x4 0x0 0x6b 0x4 0x0 0x6b 0x4 0x0 0x6b 0x4 0x0 0x6b 0x4 0x0 0x6b 0x4 0x0 0x6b 0x4 0x0 0x6b 0x4 0x0 0x6b 0x4 0x0 0x6b 0x4 0x0 0x6b 0x4 0x0 0x6b 0x4 0x0 0x6b 0x4 0x0 0x6b 0x4>;
};

timer {
     compatible = "arm,armv8-timer";
     interrupt-parent = <&gic_a72>;
     interrupts = <0x1 0xd 0x4 0x1 0xe 0x4 0x1 0xb 0x4 0x1 0xa 0x4>;
};

};
amba_rpu: amba_rpu {
   	compatible = "indirect-bus";
   	#address-cells = <0x2>;
   	#size-cells = <0x2>;
gic_r5: interrupt-controller@f9000000 {
	compatible = "arm,pl390";
	#interrupt-cells = <3>;
	interrupt-controller;
	reg = <0x0 0xf9000000 0x0 0x1000 0x0 0xf9000000 0x0 0x100>;
};

};
amba: amba {
   	compatible = "simple-bus";
   	#address-cells = <0x2>;
   	#size-cells = <0x2>;
   	ranges;
/* Proxy Interrupt Controller */
imux: interrupt-multiplex {
	compatible = "interrupt-multiplex";
	#address-cells = <0x0>;
	#interrupt-cells = <3>;
	/* copy all attributes from child to parent */
	interrupt-map-pass-thru = <0xffffffff 0xffffffff 0xffffffff>;
	/* mask all child bits to always match the first 0x0 entries */
	interrupt-map-mask = <0x0 0x0 0x0>;
	/* 1:1 mapping of all interrupts to gic_a72 and gic_r5 */
	/* child address cells, child interrupt cells, parent, parent interrupt cells */
	interrupt-map = <0x0 0x0 0x0 &gic_a72 0x0 0x0 0x0>,
			<0x0 0x0 0x0 &gic_r5 0x0 0x0 0x0>;

Note that there's no need for this node. Just put these properties in
the parent node. and remove 'interupt-parent' in all the child nodes.
Sounds good.
...
...
};

can@ff060000 {
	compatible = "xlnx,canfd-2.0";
	status = "okay";
	reg = <0x0 0xff060000 0x0 0x6000>;
	interrupt-parent = <&imux>;
	interrupts = <0x0 0x14 0x1>;
	clock-names = "can_clk", "s_axi_aclk";
	rx-fifo-depth = <0x40>;
	tx-mailbox-count = <0x20>;
	clocks = <0x6 0x3 0x52>;
	power-domains = <0x7 0x1822401f>;
};

[...]
...
tcm: tcm@ffe90000 {

How does this work being a child of this bus and in the address-map?
address-map is specified in terms of remapping from the address space of
the root node to the address space of the cpus cluster. In this case,
TCM has address 0xffe90000 in the address space of the root node (amba
is a simple-bus and its addresses get automatically mapped to the
parent, the root node.) Then, TCM gets 1:1 mapped into the address space
of cpus_r5 by address-map.
In short:

TCM is 0xffe90000 under /amba
TCM is 0xffe90000 under the root node (simple-bus mapping)
TCM is 0xffe90000 under cpus_r5 (address-map mapping)

...
...
	compatible = "mmio-sram";
	reg = <0x0 0xffe90000 0x0 0x10000>;
};

};

Re: [System-dt] [PATCH 0/3] system device tree initial set of bindings