From: Heiko Schocher <hs@denx.de>
To: Grant Likely <grant.likely@secretlab.ca>
Cc: linuxppc-dev@ozlabs.org
Subject: Re: [PATCH] mpc5200: support for the MAN mpc5200 based board uc101
Date: Mon, 14 Sep 2009 16:12:20 +0200 [thread overview]
Message-ID: <4AAE4F44.7080300@denx.de> (raw)
In-Reply-To: <fa686aa40909140638r422db79aj7568f492163c5d96@mail.gmail.com>
Hello Grant,
Grant Likely wrote:
> Thanks for the patch. Comments below.
>
> g.
>
> On Mon, Sep 14, 2009 at 2:05 AM, Heiko Schocher <hs@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 <hs@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 <hs@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,5200@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
>> + };
>> +
>> + soc5200@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
>> +
>> + cdm@200 {
>> + compatible = "fsl,mpc5200-cdm";
>> + reg = <0x200 0x38>;
>> + };
>> +
>> + mpc5200_pic: interrupt-controller@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: timer@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: timer@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: timer@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: gpio@b00 {
>> + compatible = "fsl,mpc5200-gpio";
>> + reg = <0xb00 0x40>;
>> + interrupts = <1 7 0>;
>> + gpio-controller;
>> + #gpio-cells = <2>;
>> + };
>> +
>> + gpio_wkup: gpio@c00 {
>> + compatible = "fsl,mpc5200-gpio-wkup";
>> + reg = <0xc00 0x40>;
>> + interrupts = <1 8 0 0 3 0>;
>> + gpio-controller;
>> + #gpio-cells = <2>;
>> + };
>> +
>> + dma-controller@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>;
>> + };
>> +
>> + xlb@1f00 {
>> + compatible = "fsl,mpc5200-xlb";
>> + reg = <0x1f00 0x100>;
>> + };
>> +
>> + serial@2000 { // PSC1
>> + compatible = "fsl,mpc5200-psc-uart";
>> + reg = <0x2000 0x100>;
>> + interrupts = <2 1 0>;
>> + };
>> +
>> + serial@2200 { // PSC2
>> + compatible = "fsl,mpc5200-psc-uart";
>> + reg = <0x2200 0x100>;
>> + interrupts = <2 2 0>;
>> + };
>> +
>> + serial@2c00 { // PSC6
>> + compatible = "fsl,mpc5200-psc-uart";
>> + reg = <0x2c00 0x100>;
>> + interrupts = <2 6 0>;
>> + };
>> +
>> + ethernet@3000 {
>> + compatible = "fsl,mpc5200-fec";
>> + reg = <0x3000 0x400>;
>> + local-mac-address = [ 00 00 00 00 00 00 ];
>> + interrupts = <2 5 0>;
>> + phy-handle = <&phy0>;
>> + };
>> +
>> + mdio@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-phy@0 {
>> + reg = <0>;
>
> For completeness, you should have a compatible property for the PHY
> here in the form "<vendor>,<part number>"
Ok, fix this.
>> + };
>> + };
>> +
>> + ata@3a00 {
>> + compatible = "fsl,mpc5200-ata";
>> + reg = <0x3a00 0x100>;
>> + interrupts = <2 7 0>;
>> + };
>> +
>> + i2c@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.
>> +
>> + hwmon@2c {
>> + compatible = "ad,adm9240";
>> + reg = <0x2c>;
>> + };
>> + rtc@51 {
>> + compatible = "rtc,pcf8563";
>> + reg = <0x51>;
>> + };
>> + };
>> +
>> + sram@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>;
>> +
>> + flash@0,0 {
>> + compatible = "cfi-flash";
>> + reg = <0 0 0x00800000>;
>> + bank-width = <2>;
>> + device-width = <2>;
>> + #size-cells = <1>;
>> + #address-cells = <1>;
>> + partition@0 {
>> + label = "DTS";
>> + reg = <0x0 0x00100000>;
>> + };
>> + partition@100000 {
>> + label = "Kernel";
>> + reg = <0x100000 0x00200000>;
>> + };
>> + partition@300000 {
>> + label = "RootFS";
>> + reg = <0x00300000 0x00200000>;
>> + };
>> +
>> + partition@500000 {
>> + label = "user";
>> + reg = <0x00500000 0x00200000>;
>> + };
>> + partition@700000 {
>> + label = "U-Boot";
>> + reg = <0x00700000 0x00040000>;
>> + };
>> + partition@740000 {
>> + label = "Env";
>> + reg = <0x00740000 0x00010000>;
>> + };
>> + partition@750000 {
>> + label = "red. Env";
>> + reg = <0x00750000 0x00010000>;
>> + };
>> + partition@760000 {
>> + label = "reserve";
>> + reg = <0x00760000 0x000a0000>;
>> + };
>> + };
>> +
>> + sram@1,0 {
>> + compatible = "sram";
>> + reg = <1 0x100000 0x100000>;
>> + };
>> +
>> + gpio-controller-100@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-104@3,0 {
>
> These names should be:
> gpio-controller@3,600100
> gpio-controller@3,600104
> gpio-controller@3,600201
>
> etc.
OK, fix this.
>> + #gpio-cells = <2>;
>> + compatible = "manroland,mucmc52-aux-gpio";
>> + reg = <3 0x00600104 0x1>;
>> + gpio-controller;
>> + };
>> + gpio-controller-200@3,0 {
>> + #gpio-cells = <2>;
>> + compatible = "manroland,mucmc52-aux-gpio";
>> + reg = <3 0x00600200 0x1>;
>> + gpio-controller;
>> + };
>> + gpio-controller-201@3,0 {
>> + #gpio-cells = <2>;
>> + compatible = "manroland,mucmc52-aux-gpio";
>> + reg = <3 0x00600201 0x1>;
>> + gpio-controller;
>> + };
>> + gpio-controller-202@3,0 {
>> + #gpio-cells = <2>;
>> + compatible = "manroland,mucmc52-aux-gpio";
>> + reg = <3 0x00600202 0x1>;
>> + gpio-controller;
>> + };
>> + gpio-controller-203@3,0 {
>> + #gpio-cells = <2>;
>> + compatible = "manroland,mucmc52-aux-gpio";
>> + reg = <3 0x00600203 0x1>;
>> + gpio-controller;
>> + };
>> + gpio-controller-204@3,0 {
>> + #gpio-cells = <2>;
>> + compatible = "manroland,mucmc52-aux-gpio";
>> + reg = <3 0x00600204 0x1>;
>> + gpio-controller;
>> + };
>> + gpio-controller-206@3,0 {
>> + #gpio-cells = <2>;
>> + compatible = "manroland,mucmc52-aux-gpio";
>> + reg = <3 0x00600206 0x1>;
>> + gpio-controller;
>> + };
>> + gpio-controller-207@3,0 {
>> + #gpio-cells = <2>;
>> + compatible = "manroland,mucmc52-aux-gpio";
>> + reg = <3 0x00600207 0x1>;
>> + gpio-controller;
>> + };
>> + gpio-controller-20f@3,0 {
>> + #gpio-cells = <2>;
>> + compatible = "manroland,mucmc52-aux-gpio";
>> + reg = <3 0x0060020f 0x1>;
>> + gpio-controller;
>> + };
>> +
>> + display@3,0 {
>
> similarly, this should be display@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
next prev parent reply other threads:[~2009-09-14 13:57 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-09-14 8:05 [PATCH] mpc5200: support for the MAN mpc5200 based board uc101 Heiko Schocher
2009-09-14 13:38 ` Grant Likely
2009-09-14 14:12 ` Heiko Schocher [this message]
2009-09-14 19:16 ` Wolfgang Grandegger
2009-09-15 5:06 ` Heiko Schocher
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4AAE4F44.7080300@denx.de \
--to=hs@denx.de \
--cc=grant.likely@secretlab.ca \
--cc=linuxppc-dev@ozlabs.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.