Hi Scott, See the reply inline.
> -----Original Message----- > From: Wood Scott-B07421 > Sent: Wednesday, September 25, 2013 7:22 AM > To: Xie Xiaobo-R63061 > Cc: linuxppc-dev@lists.ozlabs.org; Johnston Michael-R49610 > Subject: Re: [PATCH V4 3/3] powerpc/85xx: Add TWR-P1025 board support > > On Tue, 2013-09-24 at 18:48 +0800, Xie Xiaobo wrote: > > + partition@80000 { > > + /* 3.5 MB for Linux Kernel Image */ > > + reg = <0x00080000 0x00380000>; > > + label = "NOR Linux Kernel Image"; > > + }; > > Is this enough? I will enlarge it to 6MB. > > > + partition@400000 { > > + /* 58.75MB for JFFS2 based Root file System */ > > + reg = <0x00400000 0x03ac0000>; > > + label = "NOR Root File System"; > > + }; > > Don't specify jffs2. OK, I will remove "jffs2" > > > + /* CS2 for Display */ > > + ssd1289@2,0 { > > + #address-cells = <1>; > > + #size-cells = <1>; > > + compatible = "ssd1289"; > > + reg = <0x2 0x0000 0x0002 > > + 0x2 0x0002 0x0002>; > > + }; > > Node names should be generic. What does ssd1289 do? If this is actually > the display device, then it should be called "display@2,0". OK. The ssd1289 is a LCD controller. > > How about a vendor prefix on that compatible? Why #address-cells/#size- > cells despite no child nodes? Where is a binding that says what each of > those two reg resources mean? I will add the vendor prefix. I review the ssd1289 driver, and the #address-cells/#size-cells were un-used. I will remove them. > > > diff --git a/arch/powerpc/boot/dts/p1025twr_32b.dts > > b/arch/powerpc/boot/dts/p1025twr_32b.dts > > new file mode 100644 > > index 0000000..ccb173f > > --- /dev/null > > +++ b/arch/powerpc/boot/dts/p1025twr_32b.dts > > @@ -0,0 +1,135 @@ > > +/* > > + * P1025 TWR Device Tree Source (32-bit address map) > > + * > > + * Copyright 2013 Freescale Semiconductor Inc. > > + * > > + * Redistribution and use in source and binary forms, with or without > > + * modification, are permitted provided that the following conditions > are met: > > + * * Redistributions of source code must retain the above > copyright > > + * notice, this list of conditions and the following disclaimer. > > + * * Redistributions in binary form must reproduce the above > copyright > > + * notice, this list of conditions and the following disclaimer > in the > > + * documentation and/or other materials provided with the > distribution. > > + * * Neither the name of Freescale Semiconductor nor the > > + * names of its contributors may be used to endorse or promote > products > > + * derived from this software without specific prior written > permission. > > + * > > + * > > + * ALTERNATIVELY, this software may be distributed under the terms of > > +the > > + * GNU General Public License ("GPL") as published by the Free > > +Software > > + * Foundation, either version 2 of that License or (at your option) > > +any > > + * later version. > > + * > > + * THIS SOFTWARE IS PROVIDED BY Freescale Semiconductor ``AS IS'' AND > > +ANY > > + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE > > +IMPLIED > > + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE > > +ARE > > + * DISCLAIMED. IN NO EVENT SHALL Freescale Semiconductor BE LIABLE > > +FOR ANY > > + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL > > +DAMAGES > > + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR > > +SERVICES; > > + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER > > +CAUSED AND > > + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, > > +OR TORT > > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE > > +USE OF THIS > > + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > > + */ > > + > > +/include/ "fsl/p1021si-pre.dtsi" > > +/ { > > + model = "fsl,P1025"; > > + compatible = "fsl,TWR-P1025"; > > + > > + memory { > > + device_type = "memory"; > > + }; > > + > > + lbc: localbus@ffe05000 { > > + reg = <0 0xffe05000 0 0x1000>; > > + > > + /* NOR Flash and SSD1289 */ > > + ranges = <0x0 0x0 0x0 0xec000000 0x04000000 > > + 0x2 0x0 0x0 0xe0000000 0x00020000>; > > + }; > > + > > + soc: soc@ffe00000 { > > + ranges = <0x0 0x0 0xffe00000 0x100000>; > > + }; > > + > > + pci0: pcie@ffe09000 { > > + ranges = <0x2000000 0x0 0xa0000000 0 0xa0000000 0x0 > 0x20000000 > > + 0x1000000 0x0 0x00000000 0 0xffc10000 0x0 0x10000>; > > + reg = <0 0xffe09000 0 0x1000>; > > + pcie@0 { > > + ranges = <0x2000000 0x0 0xa0000000 > > + 0x2000000 0x0 0xa0000000 > > + 0x0 0x20000000 > > + > > + 0x1000000 0x0 0x0 > > + 0x1000000 0x0 0x0 > > + 0x0 0x100000>; > > + }; > > + }; > > + > > + pci1: pcie@ffe0a000 { > > + reg = <0 0xffe0a000 0 0x1000>; > > + ranges = <0x2000000 0x0 0x80000000 0 0x80000000 0x0 > 0x20000000 > > + 0x1000000 0x0 0x00000000 0 0xffc00000 0x0 0x10000>; > > + pcie@0 { > > + ranges = <0x2000000 0x0 0x80000000 > > + 0x2000000 0x0 0x80000000 > > + 0x0 0x20000000 > > + > > + 0x1000000 0x0 0x0 > > + 0x1000000 0x0 0x0 > > + 0x0 0x100000>; > > + }; > > + }; > > + > > + qe: qe@ffe80000 { > > + ranges = <0x0 0x0 0xffe80000 0x40000>; > > + reg = <0 0xffe80000 0 0x480>; > > + brg-frequency = <0>; > > + bus-frequency = <0>; > > + status = "disabled"; /* no firmware loaded */ > > + > > + enet3: ucc@2000 { > > + device_type = "network"; > > + compatible = "ucc_geth"; > > + rx-clock-name = "clk12"; > > + tx-clock-name = "clk9"; > > + pio-handle = <&pio1>; > > + phy-handle = <&qe_phy0>; > > + phy-connection-type = "mii"; > > + }; > > + > > + mdio@2120 { > > + qe_phy0: ethernet-phy@18 { > > + interrupt-parent = <&mpic>; > > + interrupts = <4 1 0 0>; > > + reg = <0x18>; > > + device_type = "ethernet-phy"; > > + }; > > + qe_phy1: ethernet-phy@19 { > > + interrupt-parent = <&mpic>; > > + interrupts = <5 1 0 0>; > > + reg = <0x19>; > > + device_type = "ethernet-phy"; > > + }; > > + tbi-phy@11 { > > + reg = <0x11>; > > + device_type = "tbi-phy"; > > + }; > > + }; > > + > > + enet4: ucc@2400 { > > + device_type = "network"; > > + compatible = "ucc_geth"; > > + rx-clock-name = "none"; > > + tx-clock-name = "clk13"; > > + pio-handle = <&pio2>; > > + phy-handle = <&qe_phy1>; > > + phy-connection-type = "rmii"; > > + }; > > + }; > > +}; > > Don't duplicate all this just for 32/36 bit. Use a dtsi for (e.g.) the > contents of the QE node. I will remove QE node to dtsi file. > > Is there a strong need to support both 32 and 36 bit in the first place? Don't have strong need to support 36 bit for TWR. Does it mean that I can name the file "p1025twr.dts" instead of "p1025twr_32b.dts"? > > > +/* > > +********************************************************************* > > +*** > > + * > > + * Setup the architecture > > + * > > + */ > > +static void __init twr_p1025_setup_arch(void) { #ifdef > > +CONFIG_QUICC_ENGINE > > + struct device_node *np; > > +#endif > > + > > + if (ppc_md.progress) > > + ppc_md.progress("twr_p1025_setup_arch()", 0); > > + > > + mpc85xx_smp_init(); > > + > > + fsl_pci_assign_primary(); > > + > > +#ifdef CONFIG_QUICC_ENGINE > > + mpc85xx_qe_init(); > > + > > +#if defined(CONFIG_UCC_GETH) || defined(CONFIG_SERIAL_QE) > > + if (machine_is(twr_p1025)) { > > + struct ccsr_guts __iomem *guts; > > + > > + np = of_find_node_by_name(NULL, "global-utilities"); > > Look for it by compatible. OK. > > > + if (np) { > > + guts = of_iomap(np, 0); > > + if (!guts) > > + pr_err("twr_p1025: could not map" > > + "global utilities register\n"); > > Don't linewrap printed string constants (this is an exception to the 80- > column rule). OK, I will fix it. > > > + else { > > + /* P1025 has pins muxed for QE and other functions. To > > + * enable QE UEC mode, we need to set bit QE0 for UCC1 > > + * in Eth mode, QE0 and QE3 for UCC5 in Eth mode, QE9 > > + * and QE12 for QE MII management signals in PMUXCR > > + * register. > > + */ > > + > > + printk(KERN_INFO "P1025 pinmux configured for QE\n"); > > Bad indentation, and use pr_info() (or better, just remove it; it's > implied by the absence of the error on the other branch of the if). Ok, I will remove it. > > > + > > + /* Set QE mux bits in PMUXCR */ > > + setbits32(&guts->pmuxcr, MPC85xx_PMUXCR_QE(0) | > > + MPC85xx_PMUXCR_QE(3) | > > + MPC85xx_PMUXCR_QE(9) | > > + MPC85xx_PMUXCR_QE(12)); > > + iounmap(guts); > > + > > +#if defined(CONFIG_SERIAL_QE) > > + /* On P1025TWR board, the UCC7 acted as UART port. > > + * However, The UCC7's CTS pin is low level in default, > > + * it will impact the transmission in full duplex > > + * communication. So disable the Flow control pin PA18. > > + * The UCC7 UART just can use RXD and TXD pins. > > + */ > > + par_io_config_pin(0, 18, 0, 0, 0, 0); #endif > > Any reason not to do this unconditionally? This is a board issue, the code already have a condition - defined SERIAL_QE, and I will add a condition "if (machine_is(twr_p1025))". > > -Scott > _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev