| Message ID | 1358039458-31960-6-git-send-email-tomasz.figa@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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). 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? [...] Thanks, Mark. [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2012-November/131362.html
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. Lorenzo
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 >
Hi, On Wednesday 16 of January 2013 10:59:57 Dave Martin wrote: > 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". OK. So the conclusion is that I should change the cpus node to following: cpus { cpu { compatible = "arm,arm1176jzf-s", "arm,arm1176"; }; }; Am I right? Best regards, Tomasz
Tomasz Figa wrote: [...] > > 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". > > OK. So the conclusion is that I should change the cpus node to following: > > cpus { > cpu { cpu@0 { ? > compatible = "arm,arm1176jzf-s", "arm,arm1176"; > }; > }; > > Am I right? > I think so :-) - Kukjin
On Fri, Jan 25, 2013 at 07:15:48PM +0000, Kukjin Kim wrote: > Tomasz Figa wrote: > > [...] > > > > 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". > > > > OK. So the conclusion is that I should change the cpus node to following: > > > > cpus { > > cpu { > > cpu@0 { ? As the CPU has no MPIDR, and thus doesn't have a reg property, there's no reason to have a unit address. Just "cpu" should be fine. > > > compatible = "arm,arm1176jzf-s", "arm,arm1176"; > > }; > > }; > > > > Am I right? > > > I think so :-) > > - Kukjin > As Dave pointed out, the "jzf-s" portion might be superfluous. It's worth having the "arm,arm1176", though. Thanks, Mark.
On Mon, Jan 28, 2013 at 09:02:40AM +0000, Mark Rutland wrote: > On Fri, Jan 25, 2013 at 07:15:48PM +0000, Kukjin Kim wrote: > > Tomasz Figa wrote: > > > > [...] > > > > > > 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". > > > > > > OK. So the conclusion is that I should change the cpus node to following: > > > > > > cpus { > > > cpu { > > > > cpu@0 { ? > > As the CPU has no MPIDR, and thus doesn't have a reg property, there's no reason > to have a unit address. Just "cpu" should be fine. > > > > > > compatible = "arm,arm1176jzf-s", "arm,arm1176"; > > > }; > > > }; > > > > > > Am I right? > > > > > I think so :-) > > > > - Kukjin > > > > As Dave pointed out, the "jzf-s" portion might be superfluous. It's worth > having the "arm,arm1176", though. I think we should always have a fully-qualified part name as the first compatible entry, following general devicetree policy. It's better to have this and not need it than to discover later that we want it and incompatibly backport it in. Before deciding, do we follow a consistent policy for other CPUs, such as arm1136? The issues there are similar: the only software-visible difference between arm1136j-s and arm1136jf-s should be the absence/presence of VFP, and it looks like the code in vfpmodule.c should be able to probe it. Cheers ---Dave
diff --git a/arch/arm/boot/dts/s3c6400.dtsi b/arch/arm/boot/dts/s3c6400.dtsi new file mode 100644 index 0000000..76106b8 --- /dev/null +++ b/arch/arm/boot/dts/s3c6400.dtsi @@ -0,0 +1,33 @@ +/* + * Samsung's S3C6400 SoC device tree source + * + * Copyright (c) 2013 Tomasz Figa <tomasz.figa@gmail.com> + * + * Samsung's S3C6400 SoC device nodes are listed in this file. S3C6400 + * based board files can include this file and provide values for board specfic + * bindings. + * + * Note: This file does not include device nodes for all the controllers in + * S3C6400 SoC. As device tree coverage for S3C6400 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/ "s3c64xx.dtsi" + +/ { + compatible = "samsung,s3c6400"; + + vic0: interrupt-controller@71200000 { + interrupt-mask = <0xfffffe1f>; + wakeup-mask = <0x00200004>; + }; + + vic1: interrupt-controller@71300000 { + interrupt-mask = <0xffffffff>; + wakeup-mask = <0x53020000>; + }; +}; diff --git a/arch/arm/boot/dts/s3c6410.dtsi b/arch/arm/boot/dts/s3c6410.dtsi new file mode 100644 index 0000000..4a74df9 --- /dev/null +++ b/arch/arm/boot/dts/s3c6410.dtsi @@ -0,0 +1,33 @@ +/* + * Samsung's S3C6410 SoC device tree source + * + * Copyright (c) 2013 Tomasz Figa <tomasz.figa@gmail.com> + * + * Samsung's S3C6410 SoC device nodes are listed in this file. S3C6410 + * based board files can include this file and provide values for board specfic + * bindings. + * + * Note: This file does not include device nodes for all the controllers in + * S3C6410 SoC. As device tree coverage for S3C6410 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/ "s3c64xx.dtsi" + +/ { + compatible = "samsung,s3c6410"; + + vic0: interrupt-controller@71200000 { + interrupt-mask = <0xffffff7f>; + wakeup-mask = <0x00200004>; + }; + + vic1: interrupt-controller@71300000 { + interrupt-mask = <0xffffffff>; + wakeup-mask = <0x53020000>; + }; +}; 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"; + }; + }; + + vic0: interrupt-controller@71200000 { + compatible = "arm,pl192-vic"; + interrupt-controller; + reg = <0x71200000 0x1000>; + #interrupt-cells = <1>; + }; + + vic1: interrupt-controller@71300000 { + compatible = "arm,pl192-vic"; + interrupt-controller; + reg = <0x71300000 0x1000>; + #interrupt-cells = <1>; + }; + + serial@7f005000 { + compatible = "samsung,s3c6400-uart"; + reg = <0x7f005000 0x100>; + interrupt-parent = <&vic1>; + interrupts = <5 0>; + status = "disabled"; + }; + + serial@7f005400 { + compatible = "samsung,s3c6400-uart"; + reg = <0x7f005400 0x100>; + interrupt-parent = <&vic1>; + interrupts = <6 0>; + status = "disabled"; + }; + + serial@7f005800 { + compatible = "samsung,s3c6400-uart"; + reg = <0x7f005800 0x100>; + interrupt-parent = <&vic1>; + interrupts = <7 0>; + status = "disabled"; + }; + + serial@7f005c00 { + compatible = "samsung,s3c6400-uart"; + reg = <0x7f005c00 0x100>; + interrupt-parent = <&vic1>; + interrupts = <8 0>; + status = "disabled"; + }; + + sdhci@7c200000 { + compatible = "samsung,s3c6410-sdhci"; + reg = <0x7c200000 0x100>; + interrupt-parent = <&vic1>; + interrupts = <24 0>; + status = "disabled"; + }; + + sdhci@7c300000 { + compatible = "samsung,s3c6410-sdhci"; + reg = <0x7c300000 0x100>; + interrupt-parent = <&vic1>; + interrupts = <25 0>; + status = "disabled"; + }; + + sdhci@7c400000 { + compatible = "samsung,s3c6410-sdhci"; + reg = <0x7c400000 0x100>; + interrupt-parent = <&vic1>; + interrupts = <17 0>; + status = "disabled"; + }; +};
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> --- arch/arm/boot/dts/s3c6400.dtsi | 33 ++++++++++++++ arch/arm/boot/dts/s3c6410.dtsi | 33 ++++++++++++++ arch/arm/boot/dts/s3c64xx.dtsi | 97 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 163 insertions(+) create mode 100644 arch/arm/boot/dts/s3c6400.dtsi create mode 100644 arch/arm/boot/dts/s3c6410.dtsi create mode 100644 arch/arm/boot/dts/s3c64xx.dtsi