[v6,9/9] ARM: vexpress: Add Device Tree for V2P-CA15 core tile (TC1 variant)

On Thu, 2012-01-19 at 17:00 +0000, David Vrabel wrote:
> > Ok, /include/ "skeleton.dtsi" is gone then :-)
> 
> The problem wasn't with including skeleton.dtsi.  With
> CONFIG_ARM_ATAG_DTB_COMPAT the zImage decompressor modifies the appended
> DTB using information from the ATAGs (see atags_to_fdt()).
> 
> If there's an ATAG giving the amount of RAM the DTB's "memory" node is
> replaced with a new one.  Since the vexpress DTBs don't have a "memory"
> node it's added and the DTB ends up with two nodes describing memory.

As it turned out it was just the "skeleton.dtsi" problem after all - I
mean the fact that there where two device_type="memory" nodes in the
tree.

The decompressor's setprop()
(arch/arm/boot/compressed/atags_to_fdt.c:12) uses libfdt's
fdt_setprop(), which correctly ignores the "@00000000" component of the
node name and sets the reg property as expected. So as long as there is
exactly one "memory[@address]" node in the tree,
CONFIG_ARM_ATAG_DTB_COMPAT is happy.

I will remove the /include/ from the dts files for VE (see below) in the
v3.3-rc1 based series.

Thanks for spotting this!

Paweł

On Wed, Jan 25, 2012 at 05:43:16PM +0000, Pawel Moll wrote:
> On Thu, 2012-01-19 at 17:00 +0000, David Vrabel wrote:
> > > Ok, /include/ "skeleton.dtsi" is gone then :-)
> > 
> > The problem wasn't with including skeleton.dtsi.  With
> > CONFIG_ARM_ATAG_DTB_COMPAT the zImage decompressor modifies the appended
> > DTB using information from the ATAGs (see atags_to_fdt()).
> > 
> > If there's an ATAG giving the amount of RAM the DTB's "memory" node is
> > replaced with a new one.  Since the vexpress DTBs don't have a "memory"
> > node it's added and the DTB ends up with two nodes describing memory.
> 
> As it turned out it was just the "skeleton.dtsi" problem after all - I
> mean the fact that there where two device_type="memory" nodes in the
> tree.
> 
> The decompressor's setprop()
> (arch/arm/boot/compressed/atags_to_fdt.c:12) uses libfdt's
> fdt_setprop(), which correctly ignores the "@00000000" component of the
> node name and sets the reg property as expected. So as long as there is
> exactly one "memory[@address]" node in the tree,
> CONFIG_ARM_ATAG_DTB_COMPAT is happy.
> 
> I will remove the /include/ from the dts files for VE (see below) in the
> v3.3-rc1 based series.
> 
> Thanks for spotting this!
> 
> Pawe??

This carries a significant risk of unintended fragmentation and buggy
maintenance.  This patch is a good example of the kind of change which
could easily go wrong.  (I'm not saying that it is wrong -- just using
it as an example.)

Since we will end up with a significantly large number of device trees
for vexpress, I can foresee that we'll end up with a highly reduncant
set of .dts{,i} files (each of which is often rather internally redundant
too).

Does anyone have a view on whether it's acceptable to generate device
tree sources from another form, instead of having them verbatim in the
kernel tree?  This could involve a preprocessor, or something more
heavyweight.

The dts /include/ mechanism solves this problem only for the simplest
of cases.

Cheers
---Dave

> 
> diff --git a/arch/arm/boot/dts/vexpress-v2p-ca15-tc1.dts b/arch/arm/boot/dts/vexpress-v2p-ca15-tc1.dts
> index 02cada5..2a690f2 100644
> --- a/arch/arm/boot/dts/vexpress-v2p-ca15-tc1.dts
> +++ b/arch/arm/boot/dts/vexpress-v2p-ca15-tc1.dts
> @@ -9,13 +9,15 @@
>  
>  /dts-v1/;
>  
> -/include/ "skeleton.dtsi"
> -
>  / {
>  	model = "V2P-CA15";
>  	arm,hbi = <0x237>;
>  	compatible = "arm,vexpress,v2p-ca15,tc1", "arm,vexpress,v2p-ca15", "arm,vexpress";
>  	interrupt-parent = <&gic>;
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +
> +	chosen { };
>  
>  	aliases {
>  		serial0 = &v2m_serial0;
> diff --git a/arch/arm/boot/dts/vexpress-v2p-ca5s.dts b/arch/arm/boot/dts/vexpress-v2p-ca5s.dts
> index da26a13..d4c5322 100644
> --- a/arch/arm/boot/dts/vexpress-v2p-ca5s.dts
> +++ b/arch/arm/boot/dts/vexpress-v2p-ca5s.dts
> @@ -9,13 +9,15 @@
>  
>  /dts-v1/;
>  
> -/include/ "skeleton.dtsi"
> -
>  / {
>  	model = "V2P-CA5s";
>  	arm,hbi = <0x225>;
>  	compatible = "arm,vexpress,v2p-ca5s", "arm,vexpress";
>  	interrupt-parent = <&gic>;
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +
> +	chosen { };
>  
>  	aliases {
>  		serial0 = &v2m_serial0;
> diff --git a/arch/arm/boot/dts/vexpress-v2p-ca9.dts b/arch/arm/boot/dts/vexpress-v2p-ca9.dts
> index 84542e7..5d90ce5 100644
> --- a/arch/arm/boot/dts/vexpress-v2p-ca9.dts
> +++ b/arch/arm/boot/dts/vexpress-v2p-ca9.dts
> @@ -9,13 +9,15 @@
>  
>  /dts-v1/;
>  
> -/include/ "skeleton.dtsi"
> -
>  / {
>  	model = "V2P-CA9";
>  	arm,hbi = <0x191>;
>  	compatible = "arm,vexpress,v2p-ca9", "arm,vexpress";
>  	interrupt-parent = <&gic>;
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +
> +	chosen { };
>  
>  	aliases {
>  		serial0 = &v2m_serial0;
> 
> 
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Mon, Jan 30, 2012 at 05:42:12PM +0000, Dave Martin wrote:
> On Wed, Jan 25, 2012 at 05:43:16PM +0000, Pawel Moll wrote:
> > On Thu, 2012-01-19 at 17:00 +0000, David Vrabel wrote:
> > > > Ok, /include/ "skeleton.dtsi" is gone then :-)
> > > 
> > > The problem wasn't with including skeleton.dtsi.  With
> > > CONFIG_ARM_ATAG_DTB_COMPAT the zImage decompressor modifies the appended
> > > DTB using information from the ATAGs (see atags_to_fdt()).
> > > 
> > > If there's an ATAG giving the amount of RAM the DTB's "memory" node is
> > > replaced with a new one.  Since the vexpress DTBs don't have a "memory"
> > > node it's added and the DTB ends up with two nodes describing memory.
> > 
> > As it turned out it was just the "skeleton.dtsi" problem after all - I
> > mean the fact that there where two device_type="memory" nodes in the
> > tree.
> > 
> > The decompressor's setprop()
> > (arch/arm/boot/compressed/atags_to_fdt.c:12) uses libfdt's
> > fdt_setprop(), which correctly ignores the "@00000000" component of the
> > node name and sets the reg property as expected. So as long as there is
> > exactly one "memory[@address]" node in the tree,
> > CONFIG_ARM_ATAG_DTB_COMPAT is happy.
> > 
> > I will remove the /include/ from the dts files for VE (see below) in the
> > v3.3-rc1 based series.
> > 
> > Thanks for spotting this!
> > 
> > Pawe??
> 
> This carries a significant risk of unintended fragmentation and buggy
> maintenance.  This patch is a good example of the kind of change which
> could easily go wrong.  (I'm not saying that it is wrong -- just using
> it as an example.)
> 
> Since we will end up with a significantly large number of device trees
> for vexpress, I can foresee that we'll end up with a highly reduncant
> set of .dts{,i} files (each of which is often rather internally redundant
> too).
> 
> Does anyone have a view on whether it's acceptable to generate device
> tree sources from another form, instead of having them verbatim in the
> kernel tree?  This could involve a preprocessor, or something more
> heavyweight.

Yes, the xilinx folks have been using a dts generator to create the
device tree that matches an FPGA design.  This works on ppc and
microblaze, and they'll do the same thing for their ARM FPGA SoC.

g.

On Mon, Jan 30, 2012 at 02:31:15PM -0700, Grant Likely wrote:
> On Mon, Jan 30, 2012 at 05:42:12PM +0000, Dave Martin wrote:
> > On Wed, Jan 25, 2012 at 05:43:16PM +0000, Pawel Moll wrote:
> > > On Thu, 2012-01-19 at 17:00 +0000, David Vrabel wrote:
> > > > > Ok, /include/ "skeleton.dtsi" is gone then :-)
> > > > 
> > > > The problem wasn't with including skeleton.dtsi.  With
> > > > CONFIG_ARM_ATAG_DTB_COMPAT the zImage decompressor modifies the appended
> > > > DTB using information from the ATAGs (see atags_to_fdt()).
> > > > 
> > > > If there's an ATAG giving the amount of RAM the DTB's "memory" node is
> > > > replaced with a new one.  Since the vexpress DTBs don't have a "memory"
> > > > node it's added and the DTB ends up with two nodes describing memory.
> > > 
> > > As it turned out it was just the "skeleton.dtsi" problem after all - I
> > > mean the fact that there where two device_type="memory" nodes in the
> > > tree.
> > > 
> > > The decompressor's setprop()
> > > (arch/arm/boot/compressed/atags_to_fdt.c:12) uses libfdt's
> > > fdt_setprop(), which correctly ignores the "@00000000" component of the
> > > node name and sets the reg property as expected. So as long as there is
> > > exactly one "memory[@address]" node in the tree,
> > > CONFIG_ARM_ATAG_DTB_COMPAT is happy.
> > > 
> > > I will remove the /include/ from the dts files for VE (see below) in the
> > > v3.3-rc1 based series.
> > > 
> > > Thanks for spotting this!
> > > 
> > > Pawe??
> > 
> > This carries a significant risk of unintended fragmentation and buggy
> > maintenance.  This patch is a good example of the kind of change which
> > could easily go wrong.  (I'm not saying that it is wrong -- just using
> > it as an example.)
> > 
> > Since we will end up with a significantly large number of device trees
> > for vexpress, I can foresee that we'll end up with a highly reduncant
> > set of .dts{,i} files (each of which is often rather internally redundant
> > too).
> > 
> > Does anyone have a view on whether it's acceptable to generate device
> > tree sources from another form, instead of having them verbatim in the
> > kernel tree?  This could involve a preprocessor, or something more
> > heavyweight.
> 
> Yes, the xilinx folks have been using a dts generator to create the
> device tree that matches an FPGA design.  This works on ppc and
> microblaze, and they'll do the same thing for their ARM FPGA SoC.

OK, well I guess it's good to know we have the option to consider such
techniques for vexpress in the future.

For now, I suggest we paste in the .dts files as-is, since the situation
is not too unmanageable for now, and we don't unnecessary feature creep
to hold up merging of the series.

Cheers
---Dave