Hi Rup,

On Tuesday 23 November 2010 13:56:53 Rupjyoti Sarmah wrote:

This fix is a reset for USB PHY that requires some amount of time for power
to be stable on Canyonlands.

Since this is version 2 of your patch, "[PATCH v2]" would have been a bit 
better in the subject line. Its also a good practice to summarize the changes 
between patch versions below the "---" line.

Please find a some further comments below.

<snip>
 

This define is wrong here. Its not common for all 44x platforms but 
Canyonlands specific.

                                              ^^^^^^^^^^^
                                              Canyonlands

+ * APM ppc44x series of processors.
+ *
+ * Copyright (c) 2010, Applied Micro Circuits Corporation
+ * Author: Rupjyoti Sarmah [off-list ref]
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ *
+ */
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <asm/pci-bridge.h>
+#include <asm/ppc4xx.h>
+#include <asm/udbg.h>
+#include <asm/uic.h>
+#include <linux/of_platform.h>
+#include "44x.h"
+
+static __initdata struct of_device_id ppc44x_of_bus[] = {
+	{ .compatible = "ibm,plb4", },
+	{ .compatible = "ibm,opb", },
+	{ .compatible = "ibm,ebc", },
+	{ .compatible = "simple-bus", },
+	{},
+};
+
+static int __init ppc44x_device_probe(void)
+{
+	of_platform_bus_probe(NULL, ppc44x_of_bus, NULL);
+
+	return 0;
+}
+machine_device_initcall(canyonlands, ppc44x_device_probe);
+
+/* Using this code only for the Canyonlands board.  */
+
+static int __init ppc44x_probe(void)
+{
+	unsigned long root = of_get_flat_dt_root();
+		if (of_flat_dt_is_compatible(root, "amcc,canyonlands")) {
+			ppc_pci_set_flags(PPC_PCI_REASSIGN_ALL_RSRC);
+			return 1;
+		}
+	return 0;
+}
+
+/* PHY fixup code on Canyonlands kit. */

PHY is a bit unspecific. One might think that this is an ethernet PHY (see 
remark below about better comments in the code)?

+
+static int __init ppc460ex_canyonlands_fixup(void)
+{
+	u8 __iomem *bcsr ;
+	void __iomem  *vaddr;

Double space before *vaddr.

+	struct device_node *np;
+	u32 val ;
+
+	np = of_find_compatible_node(NULL, NULL, "apm,ppc460ex-bcsr");
+	if (!np) {
+		printk(KERN_ERR "failed did not find apm, ppc460ex bcsr 

node\n");

+		return -ENODEV;
+	}
+
+	bcsr = of_iomap(np, 0);
+	of_node_put(np);
+
+	if (!bcsr) {
+		printk(KERN_CRIT "Could not remap bcsr\n");
+		return -ENODEV;
+	}
+
+	np = of_find_compatible_node(NULL, NULL, "ibm,ppc4xx-gpio");
+	vaddr = of_iomap(np, 0);
+	if (!vaddr) {
+		printk(KERN_CRIT "Could not get gpio node address\n");
+		return -ENODEV;
+	}
+
+	setbits8(&bcsr[7], BCSR_USB_EN);
+	udelay(100000);
+
+	clrbits8(&bcsr[7], BCSR_USB_EN);
+	udelay(100000);

Thats a total bootup delay of 200ms. Is this really needed?

+
+	/* configure gpio16 and gpio19 as alternate1 */
+
+	/* GPIO0_ISR1H for alternate 1 settings */
+	val = in_be32(vaddr + GPIO0_ISR1H);
+	out_be32((vaddr + GPIO0_ISR1H), val | 0x4200000);

setbits32 might be even simpler.

+	/* GPIO0_OSRH for alternate 1 settings */
+	val = in_be32(vaddr + GPIO0_OSRH);
+	out_be32((vaddr + GPIO0_OSRH), val | 0x42000000);

Same here.

+	/* GPIO0_TSRH for alternate 1 settings */
+	val = in_be32(vaddr + GPIO0_TSRH);
+	out_be32((vaddr + GPIO0_TSRH), val | 0x42000000);

And here.

And I suggest to add a few comments to the code explaining why exactly you are 
setting/clearing the bits in the BCSR and the GPIO registers.

Cheers,
Stefan