RE: [PATCH] powerpc/usb: Fix 440EPx USBH_3 USBH_5 EHCI errata

2009-03-09 Thread - Reyneke 

 Please provide a valid email address here.

New email client helpfully stripped out the address...

 called in a context where you can't iomap ? (ie with a spinlock held).

That is indeed the case. Keeping ehci generic is a valid point, but because of 
the above I don't think adding a hook is going to work. I'll take a look at 
the *platform_private option and re-post - this would also allow for better 
runtime checking, as you suggested.

 Cheers
  Jan




--
 On Fri, 2009-03-06 at 11:30 +, - Reyneke wrote:
 Patch applies to 440EPx devices in USB EHCI host mode (USB 2.0).

From the 440EPx errata:

 USBH_3: Host hangs after underrun or overrun occurs
 USBH_5: EHCI0_INSNREGxx registers are reset by a Soft or Light Host 
 Controller Reset

 Workround for USBH_3 is to enable Break Memory Transfer (BMT) in INSNREG3. 
 But the controller is reset after this fix is applied, and thus the current 
 workround is lost. The following short patch ensures INSNREG3 is correctly 
 set after reset.

 Signed-off-by: Jan Reyneke

 Please provide a valid email address here.

 ---


 A few issues here. First, it would be preferable to have this in the
 ehci-ppc-of.c file. If you can't stick that in such a place that it will
 be called after ehci_reset, then maybe you can add a reset hook so
 that ehci-ppc-of.c gets to wrap the real ehci_reset().

 Also, while the ifdef CONFIG_440EPX is good to prevent building the code
 on machines that don't need it, it's also not enough. We allow building
 kernels that support multiple boards and SoC's within the same major CPU
 family and thus you -also- need runtime detection. Either using a quirk
 (I think the USB drivers have quirk flags) or just always doing the
 of_device_is_compatible() thingy which is yet another reason for finding
 a way to move that up into ehci-ppc-of.c

 That would also avoid some duplication...



 So if you manage to move the quirk here, you can thus re-use the
 existing code, or is the reset always called in a context where you
 can't iomap ? (ie with a spinlock held).

 In any case, I don't like adding a specific field to the generic ehci
 structure like that. If that's what it takes, add a void
 *platform_private to it, and use -that- to stick a host specific data
 structure, but for something not performance sensitive such as a reset,
 if you can get away with always mapping/unmapping, it's probably better.

 Cheers,
 Ben.



_
View your Twitter and Flickr updates from one place – Learn more!
http://clk.atdmt.com/UKM/go/137984870/direct/01/
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH] powerpc/usb: Fix 440EPx USBH_3 USBH_5 EHCI errata

2009-03-07 Thread Benjamin Herrenschmidt 
On Fri, 2009-03-06 at 11:30 +, - Reyneke wrote:
 Patch applies to 440EPx devices in USB EHCI host mode (USB 2.0).
 
 From the 440EPx errata:
 
 USBH_3: Host hangs after underrun or overrun occurs
 USBH_5: EHCI0_INSNREGxx registers are reset by a Soft or Light Host 
 Controller Reset
 
 Workround for USBH_3 is to enable Break Memory Transfer (BMT) in INSNREG3. 
 But the controller is reset after this fix is applied, and thus the current 
 workround is lost. The following short patch ensures INSNREG3 is correctly 
 set after reset.

 Signed-off-by: Jan Reyneke 

Please provide a valid email address here.

 ---
  ehci-hcd.c|7 +++
  ehci-ppc-of.c |   30 +++---
  ehci.h|5 +
  3 files changed, 31 insertions(+), 11 deletions(-)
 
 diff -uprN a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
 --- a/drivers/usb/host/ehci.h2009-03-04 01:05:22.0 +
 +++ b/drivers/usb/host/ehci.h2009-03-06 10:52:53.0 +
 @@ -137,6 +137,11 @@ struct ehci_hcd {/* one per controlle
  
  u8sbrn;/* packed release number */
  
 +#if defined(CONFIG_440EPX)
 +#define PPC440EPX_EHCI0_INSREG_BMT(0x1  0)
 +__iomem u32 *insn_regs;/* INSNREGx device memory/io */
 +#endif

I don't think you need the above based on what you do later on...
(see comments below)

  /* irq statistics */
  #ifdef EHCI_STATS
  struct ehci_statsstats;
 diff -uprN a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
 --- a/drivers/usb/host/ehci-hcd.c2009-03-04 01:05:22.0 +
 +++ b/drivers/usb/host/ehci-hcd.c2009-03-06 10:54:36.0 +
 @@ -217,6 +217,13 @@ static int ehci_reset (struct ehci_hcd *
  if (ehci_is_TDI(ehci))
  tdi_reset (ehci);
  
 +#if defined(CONFIG_440EPX)
 +/* USBH_5: INSN values are lost on reset - redo USBH_3.
 +   See also ppc44x_enable_bmt.*/
 +if (ehci-insn_regs)
 +out_be32(ehci-insn_regs + 3, PPC440EPX_EHCI0_INSREG_BMT);
 +#endif

A few issues here. First, it would be preferable to have this in the
ehci-ppc-of.c file. If you can't stick that in such a place that it will
be called after ehci_reset, then maybe you can add a reset hook so
that ehci-ppc-of.c gets to wrap the real ehci_reset().

Also, while the ifdef CONFIG_440EPX is good to prevent building the code
on machines that don't need it, it's also not enough. We allow building
kernels that support multiple boards and SoC's within the same major CPU
family and thus you -also- need runtime detection. Either using a quirk
(I think the USB drivers have quirk flags) or just always doing the
of_device_is_compatible() thingy which is yet another reason for finding
a way to move that up into ehci-ppc-of.c

That would also avoid some duplication...

  /*
 - * 440EPx Errata USBH_3
 - * Fix: Enable Break Memory Transfer (BMT) in INSNREG3
 - */
 -#define PPC440EPX_EHCI0_INSREG_BMT(0x1  0)
 + * 440EPx Errata USBH_3  USBH_5
 + * Fix: Enable Break Memory Transfer (BMT) in INSNREG3. Also cache
 + * the registers so we can redo the USBH_3 fix on future resets */
  static int __devinit
 -ppc44x_enable_bmt(struct device_node *dn)
 +ppc44x_enable_bmt(struct device_node *dn, struct ehci_hcd* ehci)
  {
 -__iomem u32 *insreg_virt;
  
 -insreg_virt = of_iomap(dn, 1);
 -if (!insreg_virt)
 +#if defined(CONFIG_440EPX)
 +
 +ehci-insn_regs = of_iomap(dn, 1);
 +if (!ehci-insn_regs)
  return  -EINVAL;
  
 -out_be32(insreg_virt + 3, PPC440EPX_EHCI0_INSREG_BMT);
 +out_be32(ehci-insn_regs + 3, PPC440EPX_EHCI0_INSREG_BMT);
  
 -iounmap(insreg_virt);
 +#endif
  return 0;
 +
  }

So if you manage to move the quirk here, you can thus re-use the
existing code, or is the reset always called in a context where you
can't iomap ? (ie with a spinlock held).

In any case, I don't like adding a specific field to the generic ehci
structure like that. If that's what it takes, add a void
*platform_private to it, and use -that- to stick a host specific data
structure, but for something not performance sensitive such as a reset,
if you can get away with always mapping/unmapping, it's probably better.
 
Cheers,
Ben.


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

[PATCH] powerpc/usb: Fix 440EPx USBH_3 USBH_5 EHCI errata

2009-03-06 Thread - Reyneke 

Patch applies to 440EPx devices in USB EHCI host mode (USB 2.0).

From the 440EPx errata:

USBH_3: Host hangs after underrun or overrun occurs
USBH_5: EHCI0_INSNREGxx registers are reset by a Soft or Light Host Controller 
Reset

Workround for USBH_3 is to enable Break Memory Transfer (BMT) in INSNREG3. But 
the controller is reset after this fix is applied, and thus the current 
workround is lost. The following short patch ensures INSNREG3 is correctly set 
after reset.


Signed-off-by: Jan Reyneke 
---
 ehci-hcd.c|7 +++
 ehci-ppc-of.c |   30 +++---
 ehci.h|5 +
 3 files changed, 31 insertions(+), 11 deletions(-)

diff -uprN a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
--- a/drivers/usb/host/ehci.h2009-03-04 01:05:22.0 +
+++ b/drivers/usb/host/ehci.h2009-03-06 10:52:53.0 +
@@ -137,6 +137,11 @@ struct ehci_hcd {/* one per controlle
 
 u8sbrn;/* packed release number */
 
+#if defined(CONFIG_440EPX)
+#define PPC440EPX_EHCI0_INSREG_BMT(0x1  0)
+__iomem u32 *insn_regs;/* INSNREGx device memory/io */
+#endif
+
 /* irq statistics */
 #ifdef EHCI_STATS
 struct ehci_statsstats;
diff -uprN a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
--- a/drivers/usb/host/ehci-hcd.c2009-03-04 01:05:22.0 +
+++ b/drivers/usb/host/ehci-hcd.c2009-03-06 10:54:36.0 +
@@ -217,6 +217,13 @@ static int ehci_reset (struct ehci_hcd *
 if (ehci_is_TDI(ehci))
 tdi_reset (ehci);
 
+#if defined(CONFIG_440EPX)
+/* USBH_5: INSN values are lost on reset - redo USBH_3.
+   See also ppc44x_enable_bmt.*/
+if (ehci-insn_regs)
+out_be32(ehci-insn_regs + 3, PPC440EPX_EHCI0_INSREG_BMT);
+#endif
+
 return retval;
 }
 
diff -uprN a/drivers/usb/host/ehci-ppc-of.c b/drivers/usb/host/ehci-ppc-of.c
--- a/drivers/usb/host/ehci-ppc-of.c2009-03-04 01:05:22.0 +
+++ b/drivers/usb/host/ehci-ppc-of.c2009-03-06 10:56:08.0 +
@@ -82,23 +82,24 @@ static const struct hc_driver ehci_ppc_o
 
 
 /*
- * 440EPx Errata USBH_3
- * Fix: Enable Break Memory Transfer (BMT) in INSNREG3
- */
-#define PPC440EPX_EHCI0_INSREG_BMT(0x1  0)
+ * 440EPx Errata USBH_3  USBH_5
+ * Fix: Enable Break Memory Transfer (BMT) in INSNREG3. Also cache
+ * the registers so we can redo the USBH_3 fix on future resets */
 static int __devinit
-ppc44x_enable_bmt(struct device_node *dn)
+ppc44x_enable_bmt(struct device_node *dn, struct ehci_hcd* ehci)
 {
-__iomem u32 *insreg_virt;
 
-insreg_virt = of_iomap(dn, 1);
-if (!insreg_virt)
+#if defined(CONFIG_440EPX)
+
+ehci-insn_regs = of_iomap(dn, 1);
+if (!ehci-insn_regs)
 return  -EINVAL;
 
-out_be32(insreg_virt + 3, PPC440EPX_EHCI0_INSREG_BMT);
+out_be32(ehci-insn_regs + 3, PPC440EPX_EHCI0_INSREG_BMT);
 
-iounmap(insreg_virt);
+#endif
 return 0;
+
 }
 
 
@@ -183,7 +184,7 @@ ehci_hcd_ppc_of_probe(struct of_device *
 ehci-hcs_params = ehci_readl(ehci, ehci-caps-hcs_params);
 
 if (of_device_is_compatible(dn, ibm,usb-ehci-440epx)) {
-rv = ppc44x_enable_bmt(dn);
+rv = ppc44x_enable_bmt(dn, ehci);
 ehci_dbg(ehci, Break Memory Transfer (BMT) is %senabled!\n,
 rv ? NOT : );
 }
@@ -221,6 +222,13 @@ static int ehci_hcd_ppc_of_remove(struct
 
 usb_remove_hcd(hcd);
 
+#if defined(CONFIG_440EPX)
+if (ehci-insn_regs) {
+iounmap(ehci-insn_regs);
+ehci-insn_regs = 0;
+}
+#endif
+
 iounmap(hcd-regs);
 irq_dispose_mapping(hcd-irq);
 release_mem_region(hcd-rsrc_start, hcd-rsrc_len);



_
View your Twitter and Flickr updates from one place – Learn more!
http://clk.atdmt.com/UKM/go/137984870/direct/01/
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev