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?
> + partition@400000 {
> + /* 58.75MB for JFFS2 based Root file System */
> + reg = <0x00400000 0x03ac0000>;
> + label = "NOR Root File System";
> + };
Don't specify 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".
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?
> 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.
Is there a strong need to support both 32 and 36 bit in the first place?
> +/* ************************************************************************
> + *
> + * 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.
> + 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).
> + 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).
> +
> + /* 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?
-Scott
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
