Hello Grant, Grant Likely wrote: > Thanks for the patch. Comments below. > > g. > > On Mon, Sep 14, 2009 at 2:05 AM, Heiko Schocher <h...@denx.de> wrote: >> - serial Console on PSC1 >> - 64MB SDRAM >> - MTD CFI Flash >> - Ethernet FEC >> - I2C with PCF8563 and Temp. Sensor ADM9240 >> - IDE support >> >> Signed-off-by: Heiko Schocher <h...@denx.de> >> --- >> - based on: >> git://git.kernel.org/pub/scm/linux/kernel/git/benh/powerpc.git next >> >> - checked with: >> >> $ ./scripts/checkpatch.pl >> 0001-mpc5200-support-for-the-MAN-mpc5200-based-board-uc1.patch >> total: 0 errors, 0 warnings, 1622 lines checked >> >> 0001-mpc5200-support-for-the-MAN-mpc5200-based-board-uc1.patch has no >> obvious style problems and is ready for submission. >> $ >> >> arch/powerpc/boot/dts/uc101.dts | 312 ++++++ >> arch/powerpc/configs/52xx/uc101_defconfig | 1303 >> ++++++++++++++++++++++++++ > > I generally don't like board specific defconfigs unless there is a > really compelling reason why it should be in the kernel tree. Please > add the stuff you need (as modules!) to mpc5200_defconfig.
OK, thanks for spotting this. I take a look how this works. > g. > >> diff --git a/arch/powerpc/boot/dts/uc101.dts >> b/arch/powerpc/boot/dts/uc101.dts >> new file mode 100644 >> index 0000000..28e1c90 >> --- /dev/null >> +++ b/arch/powerpc/boot/dts/uc101.dts >> @@ -0,0 +1,312 @@ >> +/* >> + * uc101 board Device Tree Source >> + * >> + * Copyright (C) 2009 DENX Software Engineering GmbH >> + * Heiko Schocher <h...@denx.de> >> + * >> + * This program is free software; you can redistribute it and/or modify it >> + * under the terms of the GNU General Public License as published by the >> + * Free Software Foundation; either version 2 of the License, or (at your >> + * option) any later version. >> + */ >> + >> +/dts-v1/; >> + >> +/ { >> + model = "man,uc101"; >> + compatible = "man,uc101"; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + interrupt-parent = <&mpc5200_pic>; >> + >> + cpus { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + PowerPC,5...@0 { >> + device_type = "cpu"; >> + reg = <0>; >> + d-cache-line-size = <32>; >> + i-cache-line-size = <32>; >> + d-cache-size = <0x4000>; // L1, 16K >> + i-cache-size = <0x4000>; // L1, 16K >> + timebase-frequency = <0>; // from bootloader >> + bus-frequency = <0>; // from bootloader >> + clock-frequency = <0>; // from bootloader >> + }; >> + }; >> + >> + memory { >> + device_type = "memory"; >> + reg = <0x00000000 0x04000000>; // 64MB >> + }; >> + >> + soc5...@f0000000 { >> + #address-cells = <1>; >> + #size-cells = <1>; >> + compatible = "fsl,mpc5200-immr"; >> + ranges = <0 0xf0000000 0x0000c000>; >> + reg = <0xf0000000 0x00000100>; >> + bus-frequency = <0>; // from bootloader >> + system-frequency = <0>; // from bootloader >> + >> + c...@200 { >> + compatible = "fsl,mpc5200-cdm"; >> + reg = <0x200 0x38>; >> + }; >> + >> + mpc5200_pic: interrupt-control...@500 { >> + // 5200 interrupts are encoded into two levels; >> + interrupt-controller; >> + #interrupt-cells = <3>; >> + compatible = "fsl,mpc5200-pic"; >> + reg = <0x500 0x80>; >> + interrupts = <0 0 3>; >> + }; >> + >> + gpt1: ti...@610 { // General Purpose Timer 1 in GPIO >> mode >> + compatible = >> "fsl,mpc5200b-gpt-gpio","fsl,mpc5200-gpt-gpio"; >> + reg = <0x610 0x10>; >> + interrupts = <1 10 0>; >> + gpio-controller; >> + }; >> + >> + gpt2: ti...@620 { // General Purpose Timer 2 in GPIO >> mode >> + compatible = >> "fsl,mpc5200b-gpt-gpio","fsl,mpc5200-gpt-gpio"; >> + reg = <0x620 0x10>; >> + interrupts = <1 11 0>; >> + gpio-controller; >> + }; >> + >> + gpt3: ti...@630 { // General Purpose Timer 3 in GPIO >> mode >> + compatible = >> "fsl,mpc5200b-gpt-gpio","fsl,mpc5200-gpt-gpio"; >> + reg = <0x630 0x10>; >> + interrupts = <1 12 0>; >> + gpio-controller; >> + #gpio-cells = <2>; >> + }; >> + >> + gpio_simple: g...@b00 { >> + compatible = "fsl,mpc5200-gpio"; >> + reg = <0xb00 0x40>; >> + interrupts = <1 7 0>; >> + gpio-controller; >> + #gpio-cells = <2>; >> + }; >> + >> + gpio_wkup: g...@c00 { >> + compatible = "fsl,mpc5200-gpio-wkup"; >> + reg = <0xc00 0x40>; >> + interrupts = <1 8 0 0 3 0>; >> + gpio-controller; >> + #gpio-cells = <2>; >> + }; >> + >> + dma-control...@1200 { >> + device_type = "dma-controller"; >> + compatible = "fsl,mpc5200-bestcomm"; >> + reg = <0x1200 0x80>; >> + interrupts = <3 0 0 3 1 0 3 2 0 3 3 0 >> + 3 4 0 3 5 0 3 6 0 3 7 0 >> + 3 8 0 3 9 0 3 10 0 3 11 0 >> + 3 12 0 3 13 0 3 14 0 3 15 0>; >> + }; >> + >> + x...@1f00 { >> + compatible = "fsl,mpc5200-xlb"; >> + reg = <0x1f00 0x100>; >> + }; >> + >> + ser...@2000 { // PSC1 >> + compatible = "fsl,mpc5200-psc-uart"; >> + reg = <0x2000 0x100>; >> + interrupts = <2 1 0>; >> + }; >> + >> + ser...@2200 { // PSC2 >> + compatible = "fsl,mpc5200-psc-uart"; >> + reg = <0x2200 0x100>; >> + interrupts = <2 2 0>; >> + }; >> + >> + ser...@2c00 { // PSC6 >> + compatible = "fsl,mpc5200-psc-uart"; >> + reg = <0x2c00 0x100>; >> + interrupts = <2 6 0>; >> + }; >> + >> + ether...@3000 { >> + compatible = "fsl,mpc5200-fec"; >> + reg = <0x3000 0x400>; >> + local-mac-address = [ 00 00 00 00 00 00 ]; >> + interrupts = <2 5 0>; >> + phy-handle = <&phy0>; >> + }; >> + >> + m...@3000 { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + compatible = "fsl,mpc5200-mdio"; >> + reg = <0x3000 0x400>; // fec range, since we >> need to setup fec interrupts >> + interrupts = <2 5 0>; // these are for "mii >> command finished", not link changes & co. >> + >> + phy0: ethernet-...@0 { >> + reg = <0>; > > For completeness, you should have a compatible property for the PHY > here in the form "<vendor>,<part number>" Ok, fix this. >> + }; >> + }; >> + >> + a...@3a00 { >> + compatible = "fsl,mpc5200-ata"; >> + reg = <0x3a00 0x100>; >> + interrupts = <2 7 0>; >> + }; >> + >> + i...@3d40 { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + compatible = "fsl,mpc5200-i2c","fsl-i2c"; >> + reg = <0x3d40 0x40>; >> + interrupts = <2 16 0>; >> + fsl5200-clocking; > > I believe fsl5200-clocking is no longer required. There is a patch > pending which removes this property from the other .dts files. Ok, fix this. >> + >> + hw...@2c { >> + compatible = "ad,adm9240"; >> + reg = <0x2c>; >> + }; >> + r...@51 { >> + compatible = "rtc,pcf8563"; >> + reg = <0x51>; >> + }; >> + }; >> + >> + s...@8000 { >> + compatible = "fsl,mpc5200-sram"; >> + reg = <0x8000 0x4000>; >> + }; >> + >> + }; >> + >> + localbus { >> + compatible = "fsl,mpc5200-lpb","simple-bus"; >> + #address-cells = <2>; >> + #size-cells = <1>; >> + ranges = <0 0 0xff800000 0x00800000 >> + 1 0 0x80000000 0x00800000 >> + 3 0 0x80000000 0x00800000>; >> + >> + fl...@0,0 { >> + compatible = "cfi-flash"; >> + reg = <0 0 0x00800000>; >> + bank-width = <2>; >> + device-width = <2>; >> + #size-cells = <1>; >> + #address-cells = <1>; >> + partit...@0 { >> + label = "DTS"; >> + reg = <0x0 0x00100000>; >> + }; >> + partit...@100000 { >> + label = "Kernel"; >> + reg = <0x100000 0x00200000>; >> + }; >> + partit...@300000 { >> + label = "RootFS"; >> + reg = <0x00300000 0x00200000>; >> + }; >> + >> + partit...@500000 { >> + label = "user"; >> + reg = <0x00500000 0x00200000>; >> + }; >> + partit...@700000 { >> + label = "U-Boot"; >> + reg = <0x00700000 0x00040000>; >> + }; >> + partit...@740000 { >> + label = "Env"; >> + reg = <0x00740000 0x00010000>; >> + }; >> + partit...@750000 { >> + label = "red. Env"; >> + reg = <0x00750000 0x00010000>; >> + }; >> + partit...@760000 { >> + label = "reserve"; >> + reg = <0x00760000 0x000a0000>; >> + }; >> + }; >> + >> + s...@1,0 { >> + compatible = "sram"; >> + reg = <1 0x100000 0x100000>; >> + }; >> + >> + gpio-controller-...@3,0 { >> + #gpio-cells = <2>; >> + compatible = "manroland,mucmc52-aux-gpio"; > > Here I see "manroland", but the top level model and compatible > properties use "man". Which should it be (I suspect "manroland" is a > better choice). You are right, I change this too "manroland". >> + reg = <3 0x00600100 0x1>; >> + gpio-controller; >> + }; >> + gpio-controller-...@3,0 { > > These names should be: > gpio-control...@3,600100 > gpio-control...@3,600104 > gpio-control...@3,600201 > > etc. OK, fix this. >> + #gpio-cells = <2>; >> + compatible = "manroland,mucmc52-aux-gpio"; >> + reg = <3 0x00600104 0x1>; >> + gpio-controller; >> + }; >> + gpio-controller-...@3,0 { >> + #gpio-cells = <2>; >> + compatible = "manroland,mucmc52-aux-gpio"; >> + reg = <3 0x00600200 0x1>; >> + gpio-controller; >> + }; >> + gpio-controller-...@3,0 { >> + #gpio-cells = <2>; >> + compatible = "manroland,mucmc52-aux-gpio"; >> + reg = <3 0x00600201 0x1>; >> + gpio-controller; >> + }; >> + gpio-controller-...@3,0 { >> + #gpio-cells = <2>; >> + compatible = "manroland,mucmc52-aux-gpio"; >> + reg = <3 0x00600202 0x1>; >> + gpio-controller; >> + }; >> + gpio-controller-...@3,0 { >> + #gpio-cells = <2>; >> + compatible = "manroland,mucmc52-aux-gpio"; >> + reg = <3 0x00600203 0x1>; >> + gpio-controller; >> + }; >> + gpio-controller-...@3,0 { >> + #gpio-cells = <2>; >> + compatible = "manroland,mucmc52-aux-gpio"; >> + reg = <3 0x00600204 0x1>; >> + gpio-controller; >> + }; >> + gpio-controller-...@3,0 { >> + #gpio-cells = <2>; >> + compatible = "manroland,mucmc52-aux-gpio"; >> + reg = <3 0x00600206 0x1>; >> + gpio-controller; >> + }; >> + gpio-controller-...@3,0 { >> + #gpio-cells = <2>; >> + compatible = "manroland,mucmc52-aux-gpio"; >> + reg = <3 0x00600207 0x1>; >> + gpio-controller; >> + }; >> + gpio-controller-...@3,0 { >> + #gpio-cells = <2>; >> + compatible = "manroland,mucmc52-aux-gpio"; >> + reg = <3 0x0060020f 0x1>; >> + gpio-controller; >> + }; >> + >> + disp...@3,0 { > > similarly, this should be disp...@3,600000 Ok. >> + device_type = "pdsp-display"; > > Drop device_type Ok. >> + compatible = "pdsp-display"; > > No vendor prefix? I take a look at this. >> + reg = <3 0x00600000 0x1000>; >> + }; >> + }; >> + >> +}; Thanks for reviewing this patch bye Heiko -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev