RE: [PATCH v1] ppc44x:PHY fixup for USB on canyonlands board

2010-11-24 Thread Benjamin Herrenschmidt 
On Wed, 2010-11-24 at 10:25 +0530, Rupjyoti Sarmah wrote:
 
 So, do you suggest to move this macro to the canyonlands.c ? Or
 introduce
 canyonlands.h ?

Just move it to the .c file.

Cheers,
Ben.


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH v1] ppc44x:PHY fixup for USB on canyonlands board

2010-11-23 Thread Rupjyoti Sarmah 
This fix is a reset for USB PHY that requires some amount of time for power to 
be stable on Canyonlands.

Signed-off-by: Rupjyoti Sarmah rsar...@apm.com
---
 arch/powerpc/boot/dts/canyonlands.dts  |   13 +++
 arch/powerpc/platforms/44x/44x.h   |5 +
 arch/powerpc/platforms/44x/Makefile|1 +
 arch/powerpc/platforms/44x/canyonlands.c   |  122 
 arch/powerpc/platforms/44x/ppc44x_simple.c |1 -
 5 files changed, 141 insertions(+), 1 deletions(-)
 create mode 100644 arch/powerpc/platforms/44x/canyonlands.c

diff --git a/arch/powerpc/boot/dts/canyonlands.dts 
b/arch/powerpc/boot/dts/canyonlands.dts
index a303703..a9f7538 100644
--- a/arch/powerpc/boot/dts/canyonlands.dts
+++ b/arch/powerpc/boot/dts/canyonlands.dts
@@ -224,6 +224,13 @@
};
};
 
+   c...@2,0 {
+   #address-cells = 1;
+   #size-cells = 1;
+   compatible = apm,ppc460ex-bcsr;
+   reg = 2 0x0 0x9;
+   };
+
n...@3,0 {
compatible = ibm,ndfc;
reg = 0x0003 0x 
0x2000;
@@ -320,6 +327,12 @@
interrupts = 0x3 0x4;
};
 
+   GPIO0: g...@ef600b00 {
+   compatible = ibm,ppc4xx-gpio;
+   reg = 0xef600b00 0x0048;
+   gpio-controller;
+   };
+
ZMII0: emac-z...@ef600d00 {
compatible = ibm,zmii-460ex, ibm,zmii;
reg = 0xef600d00 0x000c;
diff --git a/arch/powerpc/platforms/44x/44x.h b/arch/powerpc/platforms/44x/44x.h
index dbc4d2b..bc2ab7a 100644
--- a/arch/powerpc/platforms/44x/44x.h
+++ b/arch/powerpc/platforms/44x/44x.h
@@ -4,4 +4,9 @@
 extern u8 as1_readb(volatile u8 __iomem  *addr);
 extern void as1_writeb(u8 data, volatile u8 __iomem *addr);
 
+#define BCSR_USB_EN0x11
+#define GPIO0_OSRH 0xC
+#define GPIO0_TSRH 0x14
+#define GPIO0_ISR1H0x34
+
 #endif /* __POWERPC_PLATFORMS_44X_44X_H */
diff --git a/arch/powerpc/platforms/44x/Makefile 
b/arch/powerpc/platforms/44x/Makefile
index 82ff326..6854e73 100644
--- a/arch/powerpc/platforms/44x/Makefile
+++ b/arch/powerpc/platforms/44x/Makefile
@@ -6,3 +6,4 @@ obj-$(CONFIG_WARP)  += warp.o
 obj-$(CONFIG_XILINX_VIRTEX_5_FXT) += virtex.o
 obj-$(CONFIG_XILINX_ML510) += virtex_ml510.o
 obj-$(CONFIG_ISS4xx)   += iss4xx.o
+obj-$(CONFIG_CANYONLANDS)+= canyonlands.o
diff --git a/arch/powerpc/platforms/44x/canyonlands.c 
b/arch/powerpc/platforms/44x/canyonlands.c
new file mode 100644
index 000..f13b62f
--- /dev/null
+++ b/arch/powerpc/platforms/44x/canyonlands.c
@@ -0,0 +1,122 @@
+/*
+ * This contain platform specific code for Canyonalnds board based on
+ * APM ppc44x series of processors.
+ *
+ * Copyright (c) 2010, Applied Micro Circuits Corporation
+ * Author: Rupjyoti Sarmah rsar...@apm.com
+ *
+ * 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

Re: [PATCH v1] ppc44x:PHY fixup for USB on canyonlands board

2010-11-23 Thread Stefan Roese 
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
 
 --- a/arch/powerpc/platforms/44x/44x.h
 +++ b/arch/powerpc/platforms/44x/44x.h
 @@ -4,4 +4,9 @@
  extern u8 as1_readb(volatile u8 __iomem  *addr);
  extern void as1_writeb(u8 data, volatile u8 __iomem *addr);
 
 +#define BCSR_USB_EN  0x11

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

 +#define GPIO0_OSRH   0xC
 +#define GPIO0_TSRH   0x14
 +#define GPIO0_ISR1H  0x34
 +
  #endif /* __POWERPC_PLATFORMS_44X_44X_H */
 diff --git a/arch/powerpc/platforms/44x/Makefile
 b/arch/powerpc/platforms/44x/Makefile index 82ff326..6854e73 100644
 --- a/arch/powerpc/platforms/44x/Makefile
 +++ b/arch/powerpc/platforms/44x/Makefile
 @@ -6,3 +6,4 @@ obj-$(CONFIG_WARP)+= warp.o
  obj-$(CONFIG_XILINX_VIRTEX_5_FXT) += virtex.o
  obj-$(CONFIG_XILINX_ML510) += virtex_ml510.o
  obj-$(CONFIG_ISS4xx) += iss4xx.o
 +obj-$(CONFIG_CANYONLANDS)+= canyonlands.o
 diff --git a/arch/powerpc/platforms/44x/canyonlands.c
 b/arch/powerpc/platforms/44x/canyonlands.c new file mode 100644
 index 000..f13b62f
 --- /dev/null
 +++ b/arch/powerpc/platforms/44x/canyonlands.c
 @@ -0,0 +1,122 @@
 +/*
 + * This contain platform specific code for Canyonalnds board based on
  ^^^
  Canyonlands

 + * APM ppc44x series of processors.
 + *
 + * Copyright (c) 2010, Applied Micro Circuits Corporation
 + * Author: Rupjyoti Sarmah rsar...@apm.com
 + *
 + * 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(10);
 +
 + clrbits8(bcsr[7], BCSR_USB_EN);
 + udelay(10);

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 |

Re: [PATCH v1] ppc44x:PHY fixup for USB on canyonlands board

2010-11-23 Thread Benjamin Herrenschmidt 

  +   setbits8(bcsr[7], BCSR_USB_EN);
  +   udelay(10);
  +
  +   clrbits8(bcsr[7], BCSR_USB_EN);
  +   udelay(10);
 
 Thats a total bootup delay of 200ms. Is this really needed?

In addition, so large delays should use msleep() if possible (depends
how early we are here).

Cheers,
Ben,

 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.

Seconded,

Cheers,
Ben.


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

RE: [PATCH v1] ppc44x:PHY fixup for USB on canyonlands board

2010-11-23 Thread Rupjyoti Sarmah 

 +#define BCSR_USB_EN 0x11
This define is wrong here. Its not common for all 44x platforms but
Canyonlands specific.

So, do you suggest to move this macro to the canyonlands.c ? Or introduce
canyonlands.h ?

Regards,
Rup
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v1] ppc44x:PHY fixup for USB on canyonlands board

2010-11-23 Thread Stefan Roese 
On Wednesday 24 November 2010 05:55:03 Rupjyoti Sarmah wrote:
  +#define BCSR_USB_EN   0x11
 
 This define is wrong here. Its not common for all 44x platforms but
 
 Canyonlands specific.
 
 So, do you suggest to move this macro to the canyonlands.c ? Or introduce
 canyonlands.h ?

canyonlands.c is fine with me.

Cheers,
Stefan
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev