| Message ID | 1297514825-10345-3-git-send-email-thomas.abraham@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Sat, Feb 12, 2011 at 06:17:00PM +0530, Thomas Abraham wrote: > This patch adds a basic dts file for Samsung's SMDKV310 board which > is based on the s5pv310 machine. > > Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org> Structure looks good. Some comments below, but it is getting close. > --- > arch/arm/mach-s5pv310/mach-smdkv310.dts | 78 +++++++++++++++++++++++++++++++ > 1 files changed, 78 insertions(+), 0 deletions(-) > create mode 100755 arch/arm/mach-s5pv310/mach-smdkv310.dts > > diff --git a/arch/arm/mach-s5pv310/mach-smdkv310.dts b/arch/arm/mach-s5pv310/mach-smdkv310.dts > new file mode 100755 > index 0000000..75aa76d > --- /dev/null > +++ b/arch/arm/mach-s5pv310/mach-smdkv310.dts > @@ -0,0 +1,78 @@ > +/dts-v1/; > + > +/ { > + model = "smdkv310"; You can be verbose and human-friendly here. ie. "Samsung SMDKv310 eval board" > + compatible = "samsung,s5pv310","samsung,smdkv310"; The specific board should come first (most compatible) followed by the value for the soc (less specific). > + #address-cells = <1>; > + #size-cells = <1>; > + > + cpus { #address-cells = <1>; #size-cells = <0>; > + cpu@0{ > + compatible = "arm,cortex-a9"; reg = <0>; > + }; > + > + cpu@1 { > + compatible = "arm,cortex-a9"; reg = <1>; > + }; > + }; > + > + memory { > + device_type = "memory"; > + reg = <0x40000000 0x08000000>; > + }; > + > + chosen { > + bootargs = "root=/dev/ram0 rw ramdisk=8192 initrd=0x41000000,8M console=ttySAC1,115200 init=/linuxrc"; initrd address should be passed via the linux,initrd-start and linux,initrd-end properties. U-Boot already knows how to do this so it shouldn't be encoded in the kernel parameters list. > + console-defaults = <0x3c5 0x3 0x111>; Drop console-defaults. The configuration should be encoded in the uart's device tree node. > + xtal-frequency = <24000000>; It looks like this is misplaced. If this is the primary frequency for the system, then it should go either in the root node, the soc node, or the cpu nodes. > + }; > + > + soc { > + #address-cells = <1>; > + #size-cells = <1>; > + interrupt-parent = <&GIC>; > + compatible = "samsung,s5pv310-soc","simple-bus"; > + ranges; > + > + GIC:gic@0x10500000 { > + #interrupt-cells = <1>; > + interrupt-controller; > + reg = <0x10500000 0x1000>; > + compatible = "samsung,s5pv310-gic","arm,gic"; > + }; > + > + watchdog@0x10060000 { > + reg = <0x10060000 0x400>; > + interrupts = <552>; This of course will need to be fixed up when we get the irq infrastructure properly handling cascaded irq controllers. It is fine for the time being though. > + compatible = "samsung,s5pv310-wdt","samsung,s3c2410-wdt"; > + }; > + > + serial@0x13800000 { > + reg = <0x13800000 0x100>; > + interrupts = <16 18 17>; > + reg-defaults = <0x3c5 0x3 0x111>; reg-defaults doesn't sound like a very good binding. It's better when the properties reflect what is trying to be configured instead of a set of magic values. (That said, sometimes magic values are appropriate, but even then it needs to be well documented so that mere-mortals have a fighting chance of understanding and manipulating it). > + compatible = "samsung,s5pv310-uart","samsung,s3c2410-uart"; > + }; > + > + serial@0x13810000 { > + reg = <0x13810000 0x100>; > + interrupts = <20 22 21>; > + reg-defaults = <0x3c5 0x3 0x111>; > + compatible = "samsung,s5pv310-uart","samsung,s3c2410-uart"; > + }; > + > + serial@0x13820000 { > + reg = <0x13820000 0x100>; > + interrupts = <24 26 25>; > + reg-defaults = <0x3c5 0x3 0x111>; > + compatible = "samsung,s5pv310-uart","samsung,s3c2410-uart"; > + }; > + > + serial@0x13830000 { > + reg = <0x13830000 0x100>; > + interrupts = <28 30 29>; > + reg-defaults = <0x3c5 0x3 0x111>; > + compatible = "samsung,s5pv310-uart","samsung,s3c2410-uart"; > + }; > + }; > +}; > -- > 1.6.6.rc2 >
Hi Grant, On 17 February 2011 06:04, Grant Likely <grant.likely@secretlab.ca> wrote: > On Sat, Feb 12, 2011 at 06:17:00PM +0530, Thomas Abraham wrote: >> This patch adds a basic dts file for Samsung's SMDKV310 board which >> is based on the s5pv310 machine. >> >> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org> > > Structure looks good. Some comments below, but it is getting close. > >> --- >> arch/arm/mach-s5pv310/mach-smdkv310.dts | 78 +++++++++++++++++++++++++++++++ >> 1 files changed, 78 insertions(+), 0 deletions(-) >> create mode 100755 arch/arm/mach-s5pv310/mach-smdkv310.dts >> >> diff --git a/arch/arm/mach-s5pv310/mach-smdkv310.dts b/arch/arm/mach-s5pv310/mach-smdkv310.dts >> new file mode 100755 >> index 0000000..75aa76d >> --- /dev/null >> +++ b/arch/arm/mach-s5pv310/mach-smdkv310.dts >> @@ -0,0 +1,78 @@ >> +/dts-v1/; >> + >> +/ { >> + model = "smdkv310"; <snip> >> + >> + chosen { >> + bootargs = "root=/dev/ram0 rw ramdisk=8192 initrd=0x41000000,8M console=ttySAC1,115200 init=/linuxrc"; > > initrd address should be passed via the linux,initrd-start and > linux,initrd-end properties. U-Boot already knows how to do this so > it shouldn't be encoded in the kernel parameters list. Ok. "linux,initrd-start" and "linux,initrd-end" properties will be added in chosen node and initrd parameters from bootargs removed as below. chosen { bootargs = "root=/dev/ram0 console=ttySAC1,115200 init=/linuxrc"; linux,initrd-start=<0x41000000>; linux,initrd-end=<0x40800000>; } ; > >> + console-defaults = <0x3c5 0x3 0x111>; > > Drop console-defaults. The configuration should be encoded in the > uart's device tree node. The stage at which console_init is called, the code that initializes the uart module for console operation does not have access to the uart device tree node. Hence, the console-default register values are put into the chosen node which is easier to access. If this not the right approach, maybe the uart device tree node will have to be looked up by using "for_each_compatible_node" or similar. Would you suggest using for_each_compatible_node in this case. > >> + xtal-frequency = <24000000>; > > It looks like this is misplaced. If this is the primary frequency for > the system, then it should go either in the root node, the soc node, > or the cpu nodes. This is the frequency of the crystal clock from which all the SoC internal clocks are derived. The crystal clock frequency depends on the board. So it was put into the chosen node. Since this is board specific, this is added in the chosen node. > >> + }; >> + <snip> >> + >> + serial@0x13800000 { >> + reg = <0x13800000 0x100>; >> + interrupts = <16 18 17>; >> + reg-defaults = <0x3c5 0x3 0x111>; > > reg-defaults doesn't sound like a very good binding. It's better when > the properties reflect what is trying to be configured instead of a > set of magic values. (That said, sometimes magic values are > appropriate, but even then it needs to be well documented so that > mere-mortals have a fighting chance of understanding and manipulating it). > Ok. This will be changed to be more informative. >> + compatible = "samsung,s5pv310-uart","samsung,s3c2410-uart"; >> + }; <snip> Thanks, Thomas.
Hi Thomas, replies below... On Thu, Feb 17, 2011 at 6:55 AM, Thomas Abraham <thomas.abraham@linaro.org> wrote: > Hi Grant, > > On 17 February 2011 06:04, Grant Likely <grant.likely@secretlab.ca> wrote: >> On Sat, Feb 12, 2011 at 06:17:00PM +0530, Thomas Abraham wrote: >>> This patch adds a basic dts file for Samsung's SMDKV310 board which >>> is based on the s5pv310 machine. >>> >>> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org> >> >> Structure looks good. Some comments below, but it is getting close. >> >>> --- >>> arch/arm/mach-s5pv310/mach-smdkv310.dts | 78 +++++++++++++++++++++++++++++++ >>> 1 files changed, 78 insertions(+), 0 deletions(-) >>> create mode 100755 arch/arm/mach-s5pv310/mach-smdkv310.dts >>> >>> diff --git a/arch/arm/mach-s5pv310/mach-smdkv310.dts b/arch/arm/mach-s5pv310/mach-smdkv310.dts >>> new file mode 100755 >>> index 0000000..75aa76d >>> --- /dev/null >>> +++ b/arch/arm/mach-s5pv310/mach-smdkv310.dts >>> @@ -0,0 +1,78 @@ >>> +/dts-v1/; >>> + >>> +/ { >>> + model = "smdkv310"; > > <snip> > >>> + >>> + chosen { >>> + bootargs = "root=/dev/ram0 rw ramdisk=8192 initrd=0x41000000,8M console=ttySAC1,115200 init=/linuxrc"; >> >> initrd address should be passed via the linux,initrd-start and >> linux,initrd-end properties. U-Boot already knows how to do this so >> it shouldn't be encoded in the kernel parameters list. > > Ok. "linux,initrd-start" and "linux,initrd-end" properties will be > added in chosen node and initrd parameters from bootargs removed as > below. > > chosen { > bootargs = "root=/dev/ram0 console=ttySAC1,115200 init=/linuxrc"; > linux,initrd-start=<0x41000000>; > linux,initrd-end=<0x40800000>; > } ; If you're using u-boot, you shouldn't need to add these properties at all. U-Boot will add them for you when you use the command "bootm <kernel-addr> <initrd-addr> <dtb-addr>" >> >>> + console-defaults = <0x3c5 0x3 0x111>; >> >> Drop console-defaults. The configuration should be encoded in the >> uart's device tree node. > > The stage at which console_init is called, the code that initializes > the uart module for console operation does not have access to the uart > device tree node. Hence, the console-default register values are put > into the chosen node which is easier to access. If this not the right > approach, maybe the uart device tree node will have to be looked up by > using "for_each_compatible_node" or similar. Would you suggest using > for_each_compatible_node in this case. Yes, you need to look up the node. It actually isn't very hard to do. for_each_compatible_node() is a fine approach. > >> >>> + xtal-frequency = <24000000>; >> >> It looks like this is misplaced. If this is the primary frequency for >> the system, then it should go either in the root node, the soc node, >> or the cpu nodes. > > This is the frequency of the crystal clock from which all the SoC > internal clocks are derived. The crystal clock frequency depends on > the board. So it was put into the chosen node. Since this is board > specific, this is added in the chosen node. The chosen node is for reflecting software configuration (kernel parameters, handle to console node, initrd, etc), not hardware configuration. System clock definitely falls in the category of hardware configuration. The SoC or the root node would be an okay place to put it. The chosen node is definitely the wrong place. g.
diff --git a/arch/arm/mach-s5pv310/mach-smdkv310.dts b/arch/arm/mach-s5pv310/mach-smdkv310.dts new file mode 100755 index 0000000..75aa76d --- /dev/null +++ b/arch/arm/mach-s5pv310/mach-smdkv310.dts @@ -0,0 +1,78 @@ +/dts-v1/; + +/ { + model = "smdkv310"; + compatible = "samsung,s5pv310","samsung,smdkv310"; + #address-cells = <1>; + #size-cells = <1>; + + cpus { + cpu@0{ + compatible = "arm,cortex-a9"; + }; + + cpu@1 { + compatible = "arm,cortex-a9"; + }; + }; + + memory { + device_type = "memory"; + reg = <0x40000000 0x08000000>; + }; + + chosen { + bootargs = "root=/dev/ram0 rw ramdisk=8192 initrd=0x41000000,8M console=ttySAC1,115200 init=/linuxrc"; + console-defaults = <0x3c5 0x3 0x111>; + xtal-frequency = <24000000>; + }; + + soc { + #address-cells = <1>; + #size-cells = <1>; + interrupt-parent = <&GIC>; + compatible = "samsung,s5pv310-soc","simple-bus"; + ranges; + + GIC:gic@0x10500000 { + #interrupt-cells = <1>; + interrupt-controller; + reg = <0x10500000 0x1000>; + compatible = "samsung,s5pv310-gic","arm,gic"; + }; + + watchdog@0x10060000 { + reg = <0x10060000 0x400>; + interrupts = <552>; + compatible = "samsung,s5pv310-wdt","samsung,s3c2410-wdt"; + }; + + serial@0x13800000 { + reg = <0x13800000 0x100>; + interrupts = <16 18 17>; + reg-defaults = <0x3c5 0x3 0x111>; + compatible = "samsung,s5pv310-uart","samsung,s3c2410-uart"; + }; + + serial@0x13810000 { + reg = <0x13810000 0x100>; + interrupts = <20 22 21>; + reg-defaults = <0x3c5 0x3 0x111>; + compatible = "samsung,s5pv310-uart","samsung,s3c2410-uart"; + }; + + serial@0x13820000 { + reg = <0x13820000 0x100>; + interrupts = <24 26 25>; + reg-defaults = <0x3c5 0x3 0x111>; + compatible = "samsung,s5pv310-uart","samsung,s3c2410-uart"; + }; + + serial@0x13830000 { + reg = <0x13830000 0x100>; + interrupts = <28 30 29>; + reg-defaults = <0x3c5 0x3 0x111>; + compatible = "samsung,s5pv310-uart","samsung,s3c2410-uart"; + }; + }; +};
This patch adds a basic dts file for Samsung's SMDKV310 board which is based on the s5pv310 machine. Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org> --- arch/arm/mach-s5pv310/mach-smdkv310.dts | 78 +++++++++++++++++++++++++++++++ 1 files changed, 78 insertions(+), 0 deletions(-) create mode 100755 arch/arm/mach-s5pv310/mach-smdkv310.dts