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
