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

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:

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

...

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

};