Re: [PATCH 5/6] ARM: dts: Add basic dts include files for Samsung S3C64xx SoCs

[Dave Martin <dave.martin@linaro.org>]

On Mon, Jan 14, 2013 at 03:05:32PM +0000, Lorenzo Pieralisi wrote:
> On Mon, Jan 14, 2013 at 02:48:41PM +0000, Mark Rutland wrote:
> > Hello,
> > 
> > This all looks good. I just have a couple of comments about the cpus node.
> > 
> > On Sun, Jan 13, 2013 at 01:10:57AM +0000, Tomasz Figa wrote:
> > > This patch adds basic device tree definitions for Samsung S3C64xx SoCs.
> > > 
> > > Since all the SoCs in the series are very similar, the files are created
> > > hierarchically - one file for the whole series and then separate files
> > > for particular SoCs including the common one.
> > > 
> > > Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com>
> > 
> > [...]
> > 
> > > diff --git a/arch/arm/boot/dts/s3c64xx.dtsi b/arch/arm/boot/dts/s3c64xx.dtsi
> > > new file mode 100644
> > > index 0000000..55d6e08
> > > --- /dev/null
> > > +++ b/arch/arm/boot/dts/s3c64xx.dtsi
> > > @@ -0,0 +1,97 @@
> > > +/*
> > > + * Samsung's S3C64xx SoC series common device tree source
> > > + *
> > > + * Copyright (c) 2013 Tomasz Figa <tomasz.figa@gmail.com>
> > > + *
> > > + * Samsung's S3C64xx SoC series device nodes are listed in this file.
> > > + * Particular SoCs from S3C64xx series can include this file and provide
> > > + * values for SoCs specfic bindings.
> > > + *
> > > + * Note: This file does not include device nodes for all the controllers in
> > > + * S3C64xx SoCs. As device tree coverage for S3C64xx increases, additional
> > > + * nodes can be added to this file.
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License version 2 as
> > > + * published by the Free Software Foundation.
> > > + */
> > > +
> > > +/include/ "skeleton.dtsi"
> > > +
> > > +/ {
> > > +	cpus {
> > > +		cpu@0 {
> > > +			compatible = "arm,arm1176jzf-s";
> > > +		};
> > > +	};
> > 
> > You can drop the unit address from the cpu node - it's meant to be there to
> > differentiate multiple nodes (and is supposed to match the reg property, which
> > the 1176jzf-s can't have, as it doesn't have an MPIDR).
> 
> Well, this is a point that I should consider since the kernel docs I wrote are
> misleading, they require the reg property that can not be there on UP.
> True, MPIDR does not exist in this case, but I have to document this in the
> bindings since it is unclear.
> 
> > 
> > Also, "arm,arm1176jzf-s" isn't listed in the binding doc. There was a question
> > about how to maintain this list [1], but I can't seem to find a conclusion, if
> > any were reached.  It might be worth appending "arm,arm1176" to the compatible
> > list for the cpu node in case we want to enable something via dt for all 1176
> > variations.
> > 
> > Dave, Lorenzo, any thoughts?
> 
> Eh, frankly I do not know how to handle this. Either we add a compatible
> string to the bindings anytime a DT gets merged in the kernel but how
> to maintain it, it has to be defined. Happy to hear some feedback on
> this.

Well, the number of CPU types does not grow rapidly.  It will be much
less than one per SoC -- so keeping the list up to date shouldn't be
that much effort.

For ARM1176JZF-S, it could make sense for the comatible list to be

	"arm,arm1176jzf-s", "arm,arm1176"

...since the differences between 1176 variants are software probeable
(i.e., whether there is an FPU or not).  AFAIK the J, Z apply to all
ARM1176, and the -S (synthesisable RTL) is nothing to do with software.
The kernel probably only really needs to know "arm,arm1176".

Cheers
---Dave

> 
> Lorenzo
>