linuxppc-dev.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
* [RFC][USB] powerpc: Workaround for the PPC440EPX USBH_23 errata
@ 2008-08-26 22:23 Vitaly Bordug
  2008-08-27 14:36 ` Alan Stern
  0 siblings, 1 reply; 18+ messages in thread
From: Vitaly Bordug @ 2008-08-26 22:23 UTC (permalink / raw)
  Cc: Stefan Roese, linuxppc-dev, linux-usb, Mark Miesfeld

A published errata for ppc440epx states, that when running Linux with both
EHCI and OHCI modules loaded, the EHCI module experiences a fatal error 
when a high-speed device is connected to the USB2.0, and functions normally
if OHCI module is not loaded. 

Quote from original descriprion:

The 440EPx USB 2.0 Host controller is an EHCI compliant controller.  In USB
2.0 Host controllers, each EHCI controller has one or more companion
controllers, which may be OHCI or UHCI.  An USB 2.0 Host controller will
contain one or more ports.  For each port, only one of the controllers is
connected at any one time. In the 440EPx, there is only one OHCI companion controller, 
and only one USB 2.0 Host port.
All ports on an USB 2.0 controller default to the companion controller.  If
you load only an ohci driver, it will have control of the ports and any
deviceplugged in will operate, although high speed devices will be forced to
operate at full speed.  When an ehci driver is loaded, it explicitly takes control
of the ports.  If there is a device connected, and / or every time there is a
new device connected, the ehci driver determines if the device is high speed or
not.  If it is high speed, the driver retains control of the port.  If it
is not, the driver explicitly gives the companion controller control of the
port.

The is a software workaround that uses 
Initial version of the software workaround was posted to linux-usb-devel:

http://www.mail-archive.com/linux-usb-devel@lists.sourceforge.net/msg54019.html

and later available from amcc.com:
http://www.amcc.com/Embedded/Downloads/download.html?cat=1&family=15&ins=2

The patch below is generally based on the latter, but reworked to
powerpc/of_device USB drivers, and uses a few devicetree inquiries to get
rid of all the hardcoded defines and CONFIG_* stuff, in favor to
defining specific quirk. The latter required to add more accurate
description into compatible field of USB node for 'sequioia' board. 

Signed-off-by: Vitaly Bordug <vitb@kernel.crashing.org>
Signed-off-by: Stefan Roese <sr@denx.de>
---

 arch/powerpc/boot/dts/sequoia.dts |    2 +-
 drivers/usb/host/ehci-hub.c       |   10 +++++++-
 drivers/usb/host/ehci-ppc-of.c    |   48 +++++++++++++++++++++++++++++++++++--
 drivers/usb/host/ehci.h           |   24 +++++++++++++++++++
 drivers/usb/host/ohci-ppc-of.c    |   27 ++++++++++++++++++++-
 5 files changed, 106 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/boot/dts/sequoia.dts b/arch/powerpc/boot/dts/sequoia.dts
index 53d322f..45a43a6 100644
--- a/arch/powerpc/boot/dts/sequoia.dts
+++ b/arch/powerpc/boot/dts/sequoia.dts
@@ -132,7 +132,7 @@
 		};
 
 		USB1: usb@e0000400 {
-			compatible = "ohci-be";
+			compatible = "ibm,usb-ohci-440epx", "ohci-be";
 			reg = <0 e0000400 60>;
 			interrupt-parent = <&UIC0>;
 			interrupts = <15 8>;
diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
index 740835b..290b34d 100644
--- a/drivers/usb/host/ehci-hub.c
+++ b/drivers/usb/host/ehci-hub.c
@@ -396,6 +396,7 @@ static inline void remove_companion_file(struct ehci_hcd *ehci)
 
 /*-------------------------------------------------------------------------*/
 
+
 static int check_reset_complete (
 	struct ehci_hcd	*ehci,
 	int		index,
@@ -424,8 +425,15 @@ static int check_reset_complete (
 		port_status &= ~PORT_RWC_BITS;
 		ehci_writel(ehci, port_status, status_reg);
 
-	} else
+		/* ensure 440EPX ohci controller state is operational */
+		if (ehci->has_amcc_usb23)
+			set_ohci_hcfs(ehci, 1);
+	} else {
 		ehci_dbg (ehci, "port %d high speed\n", index + 1);
+		/* ensure 440EPx ohci controller state is suspended */
+		if (ehci->has_amcc_usb23)
+			set_ohci_hcfs(ehci, 0);
+	}
 
 	return port_status;
 }
diff --git a/drivers/usb/host/ehci-ppc-of.c b/drivers/usb/host/ehci-ppc-of.c
index b018dee..c3b265d 100644
--- a/drivers/usb/host/ehci-ppc-of.c
+++ b/drivers/usb/host/ehci-ppc-of.c
@@ -107,16 +107,17 @@ ehci_hcd_ppc_of_probe(struct of_device *op, const struct of_device_id *match)
 {
 	struct device_node *dn = op->node;
 	struct usb_hcd *hcd;
-	struct ehci_hcd	*ehci;
+	struct ehci_hcd	*ehci = NULL;
 	struct resource res;
 	int irq;
 	int rv;
 
+	struct device_node *np;
+
 	if (usb_disabled())
 		return -ENODEV;
 
 	dev_dbg(&op->dev, "initializing PPC-OF USB Controller\n");
-
 	rv = of_address_to_resource(dn, 0, &res);
 	if (rv)
 		return rv;
@@ -125,6 +126,7 @@ ehci_hcd_ppc_of_probe(struct of_device *op, const struct of_device_id *match)
 	if (!hcd)
 		return -ENOMEM;
 
+
 	hcd->rsrc_start = res.start;
 	hcd->rsrc_len = res.end - res.start + 1;
 
@@ -172,6 +174,21 @@ ehci_hcd_ppc_of_probe(struct of_device *op, const struct of_device_id *match)
 				rv ? "NOT ": "");
 	}
 
+	np = of_find_compatible_node(NULL, NULL, "ibm,usb-ohci-440epx");
+	if (np != NULL) {
+	/* claim we really affected by usb23 erratum */
+		ehci->has_amcc_usb23 = 1;
+		if (!of_address_to_resource(np, 0, &res))
+			ehci->ohci_hcctrl_reg = ioremap(res.start +
+					OHCI_HCCTRL_OFFSET, OHCI_HCCTRL_LEN);
+		else
+			pr_debug(__FILE__ ": no ohci offset in fdt\n");
+		if (!ehci->ohci_hcctrl_reg) {
+			pr_debug(__FILE__ ": ioremap for ohci hcctrl failed\n");
+			return -ENOMEM;
+		}
+	}
+
 	rv = usb_add_hcd(hcd, irq, 0);
 	if (rv == 0)
 		return 0;
@@ -182,6 +199,9 @@ err_ioremap:
 err_irq:
 	release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
 err_rmr:
+
+	if (ehci->has_amcc_usb23)
+		iounmap(ehci->ohci_hcctrl_reg);
 	usb_put_hcd(hcd);
 
 	return rv;
@@ -191,6 +211,11 @@ err_rmr:
 static int ehci_hcd_ppc_of_remove(struct of_device *op)
 {
 	struct usb_hcd *hcd = dev_get_drvdata(&op->dev);
+	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
+
+	struct device_node *np;
+	struct resource res;
+
 	dev_set_drvdata(&op->dev, NULL);
 
 	dev_dbg(&op->dev, "stopping PPC-OF USB Controller\n");
@@ -201,6 +226,25 @@ static int ehci_hcd_ppc_of_remove(struct of_device *op)
 	irq_dispose_mapping(hcd->irq);
 	release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
 
+	/* use request_mem_region to test if the ohci driver is loaded.  if so
+	 * ensure the ohci core is operational.
+	 */
+	if (ehci->has_amcc_usb23) {
+		np = of_find_compatible_node(NULL, NULL, "ibm,usb-ohci-440epx");
+		if (np != NULL) {
+			if (!of_address_to_resource(np, 0, &res))
+				if (!request_mem_region(res.start,
+							    0x4, hcd_name))
+					set_ohci_hcfs(ehci, 1);
+				else
+					release_mem_region(res.start, 0x4);
+			else
+				pr_debug(__FILE__ ": no ohci offset in fdt\n");
+			of_node_put(np);
+		}
+
+		iounmap(ehci->ohci_hcctrl_reg);
+	}
 	usb_put_hcd(hcd);
 
 	return 0;
diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
index 69ebb09..a4426d1 100644
--- a/drivers/usb/host/ehci.h
+++ b/drivers/usb/host/ehci.h
@@ -118,6 +118,16 @@ struct ehci_hcd {			/* one per controller */
 	unsigned		has_fsl_port_bug:1; /* FreeScale */
 	unsigned		big_endian_mmio:1;
 	unsigned		big_endian_desc:1;
+	unsigned		has_amcc_usb23:1;
+
+	/* required for usb32 quirk */
+	#define OHCI_CTRL_HCFS          (3 << 6)
+	#define OHCI_USB_OPER           (2 << 6)
+	#define OHCI_USB_SUSPEND        (3 << 6)
+
+	#define OHCI_HCCTRL_OFFSET      0x4
+	#define OHCI_HCCTRL_LEN         0x4
+	__hc32			*ohci_hcctrl_reg;
 
 	u8			sbrn;		/* packed release number */
 
@@ -809,6 +819,20 @@ static inline u32 hc32_to_cpup (const struct ehci_hcd *ehci, const __hc32 *x)
 		le32_to_cpup((__force __le32 *)x);
 }
 
+static inline void set_ohci_hcfs(struct ehci_hcd *ehci, int operational)
+{
+	u32 hc_control;
+
+	hc_control = (readl_be(ehci->ohci_hcctrl_reg) & ~OHCI_CTRL_HCFS);
+	if (operational)
+		hc_control |= OHCI_USB_OPER;
+	else
+		hc_control |= OHCI_USB_SUSPEND;
+
+	writel_be(hc_control, ehci->ohci_hcctrl_reg);
+	(void) readl_be(ehci->ohci_hcctrl_reg);
+}
+
 /*-------------------------------------------------------------------------*/
 
 #ifndef DEBUG
diff --git a/drivers/usb/host/ohci-ppc-of.c b/drivers/usb/host/ohci-ppc-of.c
index a672527..72c49bf 100644
--- a/drivers/usb/host/ohci-ppc-of.c
+++ b/drivers/usb/host/ohci-ppc-of.c
@@ -80,7 +80,6 @@ static const struct hc_driver ohci_ppc_of_hc_driver = {
 	.start_port_reset =	ohci_start_port_reset,
 };
 
-
 static int __devinit
 ohci_hcd_ppc_of_probe(struct of_device *op, const struct of_device_id *match)
 {
@@ -92,6 +91,7 @@ ohci_hcd_ppc_of_probe(struct of_device *op, const struct of_device_id *match)
 
 	int rv;
 	int is_bigendian;
+	struct device_node *np;
 
 	if (usb_disabled())
 		return -ENODEV;
@@ -148,6 +148,31 @@ ohci_hcd_ppc_of_probe(struct of_device *op, const struct of_device_id *match)
 	if (rv == 0)
 		return 0;
 
+	/* by now, 440epx is known to show usb_23 erratum */
+	np = of_find_compatible_node(NULL, NULL, "ibm,usb-ehci-440epx");
+
+	/* Work around - At this point ohci_run has executed, the
+	* controller is running, everything, the root ports, etc., is
+	* set up.  If the ehci driver is loaded, put the ohci core in
+	* the suspended state.  The ehci driver will bring it out of
+	* suspended state when / if a non-high speed USB device is
+	* attached to the USB Host port.  If the ehci driver is not
+	* loaded, do nothing. request_mem_region is used to test if
+	* the ehci driver is loaded.
+	*/
+	if (np !=  NULL) {
+		ohci->flags |= OHCI_QUIRK_AMCC_USB23;
+		if (!of_address_to_resource(np, 0, &res))
+			if (!request_mem_region(res.start, 0x4, hcd_name)) {
+				writel_be((readl_be(&ohci->regs->control) |
+				    OHCI_USB_SUSPEND), &ohci->regs->control);
+				    (void) readl_be(&ohci->regs->control);
+			} else  {
+				release_mem_region(res.start, 0x4);
+			}
+		else
+		    pr_debug(__FILE__ ": cannot get ehci offset from fdt\n");
+	}
 	iounmap(hcd->regs);
 err_ioremap:
 	irq_dispose_mapping(irq);

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [RFC][USB] powerpc: Workaround for the PPC440EPX USBH_23 errata
  2008-08-26 22:23 [RFC][USB] powerpc: Workaround for the PPC440EPX USBH_23 errata Vitaly Bordug
@ 2008-08-27 14:36 ` Alan Stern
  2008-08-28 19:32   ` Vitaly Bordug
  0 siblings, 1 reply; 18+ messages in thread
From: Alan Stern @ 2008-08-27 14:36 UTC (permalink / raw)
  To: Vitaly Bordug; +Cc: Stefan Roese, linuxppc-dev, linux-usb, Mark Miesfeld

On Wed, 27 Aug 2008, Vitaly Bordug wrote:

> A published errata for ppc440epx states, that when running Linux with both
> EHCI and OHCI modules loaded, the EHCI module experiences a fatal error 
> when a high-speed device is connected to the USB2.0, and functions normally
> if OHCI module is not loaded. 
> 
> Quote from original descriprion:
> 
> The 440EPx USB 2.0 Host controller is an EHCI compliant controller.  In USB
> 2.0 Host controllers, each EHCI controller has one or more companion
> controllers, which may be OHCI or UHCI.  An USB 2.0 Host controller will
> contain one or more ports.  For each port, only one of the controllers is
> connected at any one time. In the 440EPx, there is only one OHCI companion controller, 
> and only one USB 2.0 Host port.
> All ports on an USB 2.0 controller default to the companion controller.  If
> you load only an ohci driver, it will have control of the ports and any
> deviceplugged in will operate, although high speed devices will be forced to
> operate at full speed.  When an ehci driver is loaded, it explicitly takes control
> of the ports.  If there is a device connected, and / or every time there is a
> new device connected, the ehci driver determines if the device is high speed or
> not.  If it is high speed, the driver retains control of the port.  If it
> is not, the driver explicitly gives the companion controller control of the
> port.

This doesn't explain why the fatal error occurs.

> The is a software workaround that uses 
> Initial version of the software workaround was posted to linux-usb-devel:
> 
> http://www.mail-archive.com/linux-usb-devel@lists.sourceforge.net/msg54019.html
> 
> and later available from amcc.com:
> http://www.amcc.com/Embedded/Downloads/download.html?cat=1&family=15&ins=2
> 
> The patch below is generally based on the latter, but reworked to
> powerpc/of_device USB drivers, and uses a few devicetree inquiries to get
> rid of all the hardcoded defines and CONFIG_* stuff, in favor to
> defining specific quirk. The latter required to add more accurate
> description into compatible field of USB node for 'sequioia' board. 

I have some doubts about parts of this patch.

> --- a/drivers/usb/host/ehci-ppc-of.c
> +++ b/drivers/usb/host/ehci-ppc-of.c
> @@ -107,16 +107,17 @@ ehci_hcd_ppc_of_probe(struct of_device *op, const struct of_device_id *match)
>  {
>  	struct device_node *dn = op->node;
>  	struct usb_hcd *hcd;
> -	struct ehci_hcd	*ehci;
> +	struct ehci_hcd	*ehci = NULL;
>  	struct resource res;
>  	int irq;
>  	int rv;
>  
> +	struct device_node *np;
> +
>  	if (usb_disabled())
>  		return -ENODEV;
>  
>  	dev_dbg(&op->dev, "initializing PPC-OF USB Controller\n");
> -
>  	rv = of_address_to_resource(dn, 0, &res);
>  	if (rv)
>  		return rv;
> @@ -125,6 +126,7 @@ ehci_hcd_ppc_of_probe(struct of_device *op, const struct of_device_id *match)
>  	if (!hcd)
>  		return -ENOMEM;
>  
> +

Is there any reason for these apparently gratuitous whitespace changes?

>  	hcd->rsrc_start = res.start;
>  	hcd->rsrc_len = res.end - res.start + 1;
>  
> @@ -172,6 +174,21 @@ ehci_hcd_ppc_of_probe(struct of_device *op, const struct of_device_id *match)
>  				rv ? "NOT ": "");
>  	}
>  
> +	np = of_find_compatible_node(NULL, NULL, "ibm,usb-ohci-440epx");
> +	if (np != NULL) {
> +	/* claim we really affected by usb23 erratum */
> +		ehci->has_amcc_usb23 = 1;
> +		if (!of_address_to_resource(np, 0, &res))
> +			ehci->ohci_hcctrl_reg = ioremap(res.start +
> +					OHCI_HCCTRL_OFFSET, OHCI_HCCTRL_LEN);
> +		else
> +			pr_debug(__FILE__ ": no ohci offset in fdt\n");
> +		if (!ehci->ohci_hcctrl_reg) {
> +			pr_debug(__FILE__ ": ioremap for ohci hcctrl failed\n");
> +			return -ENOMEM;

This is a memory leak.

> --- a/drivers/usb/host/ehci.h
> +++ b/drivers/usb/host/ehci.h
> @@ -809,6 +819,20 @@ static inline u32 hc32_to_cpup (const struct ehci_hcd *ehci, const __hc32 *x)
>  		le32_to_cpup((__force __le32 *)x);
>  }
>  
> +static inline void set_ohci_hcfs(struct ehci_hcd *ehci, int operational)
> +{
> +	u32 hc_control;
> +
> +	hc_control = (readl_be(ehci->ohci_hcctrl_reg) & ~OHCI_CTRL_HCFS);
> +	if (operational)
> +		hc_control |= OHCI_USB_OPER;
> +	else
> +		hc_control |= OHCI_USB_SUSPEND;
> +
> +	writel_be(hc_control, ehci->ohci_hcctrl_reg);
> +	(void) readl_be(ehci->ohci_hcctrl_reg);
> +}
> +

This should be protected by a preprocessor test so that the code 
doesn't get compiled on architectures that don't need it.

> --- a/drivers/usb/host/ohci-ppc-of.c
> +++ b/drivers/usb/host/ohci-ppc-of.c
> @@ -148,6 +148,31 @@ ohci_hcd_ppc_of_probe(struct of_device *op, const struct of_device_id *match)
>  	if (rv == 0)
>  		return 0;
>  
> +	/* by now, 440epx is known to show usb_23 erratum */
> +	np = of_find_compatible_node(NULL, NULL, "ibm,usb-ehci-440epx");
> +
> +	/* Work around - At this point ohci_run has executed, the
> +	* controller is running, everything, the root ports, etc., is
> +	* set up.  If the ehci driver is loaded, put the ohci core in
> +	* the suspended state.  The ehci driver will bring it out of
> +	* suspended state when / if a non-high speed USB device is
> +	* attached to the USB Host port.  If the ehci driver is not
> +	* loaded, do nothing. request_mem_region is used to test if
> +	* the ehci driver is loaded.
> +	*/
> +	if (np !=  NULL) {
> +		ohci->flags |= OHCI_QUIRK_AMCC_USB23;
> +		if (!of_address_to_resource(np, 0, &res))
> +			if (!request_mem_region(res.start, 0x4, hcd_name)) {
> +				writel_be((readl_be(&ohci->regs->control) |
> +				    OHCI_USB_SUSPEND), &ohci->regs->control);
> +				    (void) readl_be(&ohci->regs->control);

Wrong indentation level.

> +			} else  {
> +				release_mem_region(res.start, 0x4);
> +			}
> +		else
> +		    pr_debug(__FILE__ ": cannot get ehci offset from fdt\n");

Wrong indentation amount.

> +	}
>  	iounmap(hcd->regs);
>  err_ioremap:
>  	irq_dispose_mapping(irq);

I doubt this will interact properly with usbcore and the rest of
ohci-hcd.  They do not expect the root hub to be suspended unless they
know about it.

Alan Stern

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC][USB] powerpc: Workaround for the PPC440EPX USBH_23 errata
  2008-08-27 14:36 ` Alan Stern
@ 2008-08-28 19:32   ` Vitaly Bordug
  2008-08-28 20:13     ` Alan Stern
  0 siblings, 1 reply; 18+ messages in thread
From: Vitaly Bordug @ 2008-08-28 19:32 UTC (permalink / raw)
  To: Alan Stern; +Cc: Stefan Roese, linuxppc-dev, linux-usb, Mark Miesfeld

В Wed, 27 Aug 2008 10:36:20 -0400 (EDT)
Alan Stern <stern@rowland.harvard.edu> пишет:

> On Wed, 27 Aug 2008, Vitaly Bordug wrote:
> 
> > A published errata for ppc440epx states, that when running Linux
> > with both EHCI and OHCI modules loaded, the EHCI module experiences
> > a fatal error when a high-speed device is connected to the USB2.0,
> > and functions normally if OHCI module is not loaded. 
> > 
> > Quote from original descriprion:
> > 
> > The 440EPx USB 2.0 Host controller is an EHCI compliant
> > controller.  In USB 2.0 Host controllers, each EHCI controller has
> > one or more companion controllers, which may be OHCI or UHCI.  An
> > USB 2.0 Host controller will contain one or more ports.  For each
> > port, only one of the controllers is connected at any one time. In
> > the 440EPx, there is only one OHCI companion controller, and only
> > one USB 2.0 Host port. All ports on an USB 2.0 controller default
> > to the companion controller.  If you load only an ohci driver, it
> > will have control of the ports and any deviceplugged in will
> > operate, although high speed devices will be forced to operate at
> > full speed.  When an ehci driver is loaded, it explicitly takes
> > control of the ports.  If there is a device connected, and / or
> > every time there is a new device connected, the ehci driver
> > determines if the device is high speed or not.  If it is high
> > speed, the driver retains control of the port.  If it is not, the
> > driver explicitly gives the companion controller control of the
> > port.
> 
> This doesn't explain why the fatal error occurs.
> 
On certain 44x set of SoCs, only one controller is able to function,
e.g. technically they are mutually exclusive.

There used to be recommendation to use only hi-speed or full-speed
devices with specific conditions, when respective module was unloaded.
Later, it was observed that ohci suspend is enough to keep things
going, and it was turned into workaround, as explained below.

> > The is a software workaround that uses 
> > Initial version of the software workaround was posted to
> > linux-usb-devel:
> > 
> > http://www.mail-archive.com/linux-usb-devel@lists.sourceforge.net/msg54019.html
> > 
> > and later available from amcc.com:
> > http://www.amcc.com/Embedded/Downloads/download.html?cat=1&family=15&ins=2
> > 
> > The patch below is generally based on the latter, but reworked to
> > powerpc/of_device USB drivers, and uses a few devicetree inquiries
> > to get rid of all the hardcoded defines and CONFIG_* stuff, in
> > favor to defining specific quirk. The latter required to add more
> > accurate description into compatible field of USB node for
> > 'sequioia' board. 
> 
> I have some doubts about parts of this patch.
What stands about noted gotchas, I agree and will fix them, thanks for
looking through the patch. I agree this doesn't look pleasant, but
unfortunately is the only way to say use USB keyboard, and hi-speed
memory stick at the same time.


> 
> > --- a/drivers/usb/host/ehci-ppc-of.c
> > +++ b/drivers/usb/host/ehci-ppc-of.c
> > @@ -107,16 +107,17 @@ ehci_hcd_ppc_of_probe(struct of_device *op,
> > const struct of_device_id *match) {
> >  	struct device_node *dn = op->node;
> >  	struct usb_hcd *hcd;
> > -	struct ehci_hcd	*ehci;
> > +	struct ehci_hcd	*ehci = NULL;
> >  	struct resource res;
> >  	int irq;
> >  	int rv;
> >  
> > +	struct device_node *np;
> > +
> >  	if (usb_disabled())
> >  		return -ENODEV;
> >  
> >  	dev_dbg(&op->dev, "initializing PPC-OF USB Controller\n");
> > -
> >  	rv = of_address_to_resource(dn, 0, &res);
> >  	if (rv)
> >  		return rv;
> > @@ -125,6 +126,7 @@ ehci_hcd_ppc_of_probe(struct of_device *op,
> > const struct of_device_id *match) if (!hcd)
> >  		return -ENOMEM;
> >  
> > +
> 
> Is there any reason for these apparently gratuitous whitespace
> changes?
> 
> >  	hcd->rsrc_start = res.start;
> >  	hcd->rsrc_len = res.end - res.start + 1;
> >  
> > @@ -172,6 +174,21 @@ ehci_hcd_ppc_of_probe(struct of_device *op,
> > const struct of_device_id *match) rv ? "NOT ": "");
> >  	}
> >  
> > +	np = of_find_compatible_node(NULL, NULL,
> > "ibm,usb-ohci-440epx");
> > +	if (np != NULL) {
> > +	/* claim we really affected by usb23 erratum */
> > +		ehci->has_amcc_usb23 = 1;
> > +		if (!of_address_to_resource(np, 0, &res))
> > +			ehci->ohci_hcctrl_reg = ioremap(res.start +
> > +					OHCI_HCCTRL_OFFSET,
> > OHCI_HCCTRL_LEN);
> > +		else
> > +			pr_debug(__FILE__ ": no ohci offset in
> > fdt\n");
> > +		if (!ehci->ohci_hcctrl_reg) {
> > +			pr_debug(__FILE__ ": ioremap for ohci
> > hcctrl failed\n");
> > +			return -ENOMEM;
> 
> This is a memory leak.
> 
> > --- a/drivers/usb/host/ehci.h
> > +++ b/drivers/usb/host/ehci.h
> > @@ -809,6 +819,20 @@ static inline u32 hc32_to_cpup (const struct
> > ehci_hcd *ehci, const __hc32 *x) le32_to_cpup((__force __le32 *)x);
> >  }
> >  
> > +static inline void set_ohci_hcfs(struct ehci_hcd *ehci, int
> > operational) +{
> > +	u32 hc_control;
> > +
> > +	hc_control = (readl_be(ehci->ohci_hcctrl_reg) &
> > ~OHCI_CTRL_HCFS);
> > +	if (operational)
> > +		hc_control |= OHCI_USB_OPER;
> > +	else
> > +		hc_control |= OHCI_USB_SUSPEND;
> > +
> > +	writel_be(hc_control, ehci->ohci_hcctrl_reg);
> > +	(void) readl_be(ehci->ohci_hcctrl_reg);
> > +}
> > +
> 
> This should be protected by a preprocessor test so that the code 
> doesn't get compiled on architectures that don't need it.

The whole thing is supposed to be used only on some powerpc platforms,
so I'll move or #ifdef the upper.

> 
> > --- a/drivers/usb/host/ohci-ppc-of.c
> > +++ b/drivers/usb/host/ohci-ppc-of.c
> > @@ -148,6 +148,31 @@ ohci_hcd_ppc_of_probe(struct of_device *op,
> > const struct of_device_id *match) if (rv == 0)
> >  		return 0;
> >  
> > +	/* by now, 440epx is known to show usb_23 erratum */
> > +	np = of_find_compatible_node(NULL, NULL,
> > "ibm,usb-ehci-440epx"); +
> > +	/* Work around - At this point ohci_run has executed, the
> > +	* controller is running, everything, the root ports, etc.,
> > is
> > +	* set up.  If the ehci driver is loaded, put the ohci core
> > in
> > +	* the suspended state.  The ehci driver will bring it out
> > of
> > +	* suspended state when / if a non-high speed USB device is
> > +	* attached to the USB Host port.  If the ehci driver is not
> > +	* loaded, do nothing. request_mem_region is used to test if
> > +	* the ehci driver is loaded.
> > +	*/
> > +	if (np !=  NULL) {
> > +		ohci->flags |= OHCI_QUIRK_AMCC_USB23;
> > +		if (!of_address_to_resource(np, 0, &res))
> > +			if (!request_mem_region(res.start, 0x4,
> > hcd_name)) {
> > +
> > writel_be((readl_be(&ohci->regs->control) |
> > +				    OHCI_USB_SUSPEND),
> > &ohci->regs->control);
> > +				    (void)
> > readl_be(&ohci->regs->control);
> 
> Wrong indentation level.
> 
> > +			} else  {
> > +				release_mem_region(res.start, 0x4);
> > +			}
> > +		else
> > +		    pr_debug(__FILE__ ": cannot get ehci offset
> > from fdt\n");
> 
> Wrong indentation amount.
> 
> > +	}
> >  	iounmap(hcd->regs);
> >  err_ioremap:
> >  	irq_dispose_mapping(irq);
> 
> I doubt this will interact properly with usbcore and the rest of
> ohci-hcd.  They do not expect the root hub to be suspended unless they
> know about it.

I need to reemphasize, that upper is not "normal", but unfortunately
the only way to have both full and hi-speed usb live in peace with
44xEPX family. Quirks are not going to be executed on other chip
anyway, and on chip in question, it was tested and works stable enough.

I can add an explicit warning, that workaround is being utilised, to
make things clear if it will be considered appropriate.

Thanks,

-Vitaly

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC][USB] powerpc: Workaround for the PPC440EPX USBH_23 errata
  2008-08-28 19:32   ` Vitaly Bordug
@ 2008-08-28 20:13     ` Alan Stern
  2008-08-28 21:11       ` Steven A. Falco
  0 siblings, 1 reply; 18+ messages in thread
From: Alan Stern @ 2008-08-28 20:13 UTC (permalink / raw)
  To: Vitaly Bordug; +Cc: Stefan Roese, linuxppc-dev, linux-usb, Mark Miesfeld

On Thu, 28 Aug 2008, Vitaly Bordug wrote:

> > This doesn't explain why the fatal error occurs.
> > 
> On certain 44x set of SoCs, only one controller is able to function,
> e.g. technically they are mutually exclusive.
> 
> There used to be recommendation to use only hi-speed or full-speed
> devices with specific conditions, when respective module was unloaded.
> Later, it was observed that ohci suspend is enough to keep things
> going, and it was turned into workaround, as explained below.

Okay, good.  _This_ is what you should have put in the patch 
description, instead of all that other stuff.

> > I have some doubts about parts of this patch.
> What stands about noted gotchas, I agree and will fix them, thanks for
> looking through the patch. I agree this doesn't look pleasant, but
> unfortunately is the only way to say use USB keyboard, and hi-speed
> memory stick at the same time.

Your original post mentioned that the 440EPx has only one USB 2.0 host
port.  Then how can you use a keyboard and memory stick at the same
time?  You'd have to plug them into a hub -- in which case only one
controller would be needed, the one driving the hub.  The patch would 
be unnecessary.

> > I doubt this will interact properly with usbcore and the rest of
> > ohci-hcd.  They do not expect the root hub to be suspended unless they
> > know about it.
> 
> I need to reemphasize, that upper is not "normal", but unfortunately
> the only way to have both full and hi-speed usb live in peace with
> 44xEPX family. Quirks are not going to be executed on other chip
> anyway, and on chip in question, it was tested and works stable enough.
> 
> I can add an explicit warning, that workaround is being utilised, to
> make things clear if it will be considered appropriate.

Do these systems support CONFIG_PM?  If they do then your patch
shouldn't be needed, because then ohci-hcd will automatically suspend
the OHCI root hub 1 second after the last full/low-speed device is
unplugged.  You could reduce that 1 second value if you wanted to 
decrease the probability of conflicts.

Alan Stern

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC][USB] powerpc: Workaround for the PPC440EPX USBH_23 errata
  2008-08-28 20:13     ` Alan Stern
@ 2008-08-28 21:11       ` Steven A. Falco
  2008-08-28 21:33         ` Alan Stern
  2008-08-29 17:56         ` Matthias Fuchs
  0 siblings, 2 replies; 18+ messages in thread
From: Steven A. Falco @ 2008-08-28 21:11 UTC (permalink / raw)
  To: Alan Stern; +Cc: Stefan Roese, linux-usb, Mark Miesfeld, linuxppc-dev

Alan Stern wrote:
> Your original post mentioned that the 440EPx has only one USB 2.0 host
> port.  Then how can you use a keyboard and memory stick at the same
> time?  You'd have to plug them into a hub -- in which case only one
> controller would be needed, the one driving the hub.  The patch would 
> be unnecessary.
I have one of these processors on a Sequoia board.  What happens is that
if you build the kernel with both EHCI and OHCI support, then plug in
a modern USB memory stick, it initially tries EHCI, the driver fails, and
the whole thing falls back to OHCI.  So you wind up running at 12 Mbps.
The only way to make high speed work is to turn off the OHCI driver, and
then you cannot support slow devices with that kernel.

So, hile you cannot plug two devices in at one time, you can plug in
different speed devices at different times, and my understanding is that
this patch will let that work seamlessly.
> 
>>> I doubt this will interact properly with usbcore and the rest of
>>> ohci-hcd.  They do not expect the root hub to be suspended unless they
>>> know about it.
>> I need to reemphasize, that upper is not "normal", but unfortunately
>> the only way to have both full and hi-speed usb live in peace with
>> 44xEPX family. Quirks are not going to be executed on other chip
>> anyway, and on chip in question, it was tested and works stable enough.
>>
>> I can add an explicit warning, that workaround is being utilised, to
>> make things clear if it will be considered appropriate.
> 
> Do these systems support CONFIG_PM?  If they do then your patch
> shouldn't be needed, because then ohci-hcd will automatically suspend
> the OHCI root hub 1 second after the last full/low-speed device is
> unplugged.  You could reduce that 1 second value if you wanted to 
> decrease the probability of conflicts.
> 
> Alan Stern
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC][USB] powerpc: Workaround for the PPC440EPX USBH_23 errata
  2008-08-28 21:11       ` Steven A. Falco
@ 2008-08-28 21:33         ` Alan Stern
  2008-08-28 22:43           ` Benjamin Herrenschmidt
  2008-08-29 13:25           ` Steven A. Falco
  2008-08-29 17:56         ` Matthias Fuchs
  1 sibling, 2 replies; 18+ messages in thread
From: Alan Stern @ 2008-08-28 21:33 UTC (permalink / raw)
  To: Steven A. Falco; +Cc: Stefan Roese, linux-usb, Mark Miesfeld, linuxppc-dev

On Thu, 28 Aug 2008, Steven A. Falco wrote:

> Alan Stern wrote:
> > Your original post mentioned that the 440EPx has only one USB 2.0 host
> > port.  Then how can you use a keyboard and memory stick at the same
> > time?  You'd have to plug them into a hub -- in which case only one
> > controller would be needed, the one driving the hub.  The patch would 
> > be unnecessary.
> I have one of these processors on a Sequoia board.  What happens is that
> if you build the kernel with both EHCI and OHCI support, then plug in
> a modern USB memory stick, it initially tries EHCI, the driver fails, and
> the whole thing falls back to OHCI.  So you wind up running at 12 Mbps.
> The only way to make high speed work is to turn off the OHCI driver, and
> then you cannot support slow devices with that kernel.
> 
> So, hile you cannot plug two devices in at one time, you can plug in
> different speed devices at different times, and my understanding is that
> this patch will let that work seamlessly.

Is there some reason why it doesn't work already?  All the patch does
is suspend the OHCI root hub when you plug in the memory stick -- but
the root hub should already be suspended.

Unless the memory stick is already plugged in when the kernel boots.  
In which case the root hub won't be suspended -- it won't suspend until 
1 second after ohci-hcd initializes the controller.  Is that the 
scenario you're worried about?

Alan Stern

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC][USB] powerpc: Workaround for the PPC440EPX USBH_23 errata
  2008-08-28 21:33         ` Alan Stern
@ 2008-08-28 22:43           ` Benjamin Herrenschmidt
  2008-08-29 15:26             ` Alan Stern
  2008-08-29 13:25           ` Steven A. Falco
  1 sibling, 1 reply; 18+ messages in thread
From: Benjamin Herrenschmidt @ 2008-08-28 22:43 UTC (permalink / raw)
  To: Alan Stern; +Cc: linux-usb, linuxppc-dev, Mark Miesfeld, Stefan Roese

On Thu, 2008-08-28 at 17:33 -0400, Alan Stern wrote:
> Is there some reason why it doesn't work already?  All the patch does
> is suspend the OHCI root hub when you plug in the memory stick -- but
> the root hub should already be suspended.
> 
> Unless the memory stick is already plugged in when the kernel boots.  
> In which case the root hub won't be suspended -- it won't suspend
> until 
> 1 second after ohci-hcd initializes the controller.  Is that the 
> scenario you're worried about?

I suppose some embedded platforms don't use CONFIG_PM, is this still a
requirement for autosuspend ? Or do that happen always on an empty port
nowadays ?

Cheers,
Ben.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC][USB] powerpc: Workaround for the PPC440EPX USBH_23 errata
  2008-08-28 21:33         ` Alan Stern
  2008-08-28 22:43           ` Benjamin Herrenschmidt
@ 2008-08-29 13:25           ` Steven A. Falco
  1 sibling, 0 replies; 18+ messages in thread
From: Steven A. Falco @ 2008-08-29 13:25 UTC (permalink / raw)
  To: Alan Stern; +Cc: Stefan Roese, linux-usb, Mark Miesfeld, linuxppc-dev

Alan Stern wrote:
> On Thu, 28 Aug 2008, Steven A. Falco wrote:
> 
>> Alan Stern wrote:
>>> Your original post mentioned that the 440EPx has only one USB 2.0 host
>>> port.  Then how can you use a keyboard and memory stick at the same
>>> time?  You'd have to plug them into a hub -- in which case only one
>>> controller would be needed, the one driving the hub.  The patch would 
>>> be unnecessary.
>> I have one of these processors on a Sequoia board.  What happens is that
>> if you build the kernel with both EHCI and OHCI support, then plug in
>> a modern USB memory stick, it initially tries EHCI, the driver fails, and
>> the whole thing falls back to OHCI.  So you wind up running at 12 Mbps.
>> The only way to make high speed work is to turn off the OHCI driver, and
>> then you cannot support slow devices with that kernel.
>>
>> So, hile you cannot plug two devices in at one time, you can plug in
>> different speed devices at different times, and my understanding is that
>> this patch will let that work seamlessly.
> 
> Is there some reason why it doesn't work already?  All the patch does
> is suspend the OHCI root hub when you plug in the memory stick -- but
> the root hub should already be suspended.
> 
> Unless the memory stick is already plugged in when the kernel boots.  
> In which case the root hub won't be suspended -- it won't suspend until 
> 1 second after ohci-hcd initializes the controller.  Is that the 
> scenario you're worried about?
Not sure about other users, but in my case, yes.  I was using a USB stick
as the root filesystem.  When the kernel first came up, it would try the
EHCI driver, which would fail.  Then the drive would be recognized by the
OHCI driver, but would only run at full (12 Mbps) speed rather than high
speed.

I've since gone to a directly attached CF card on the processor external
bus, and I'm not using USB anymore.  But I think the patch is still
desirable to work around the processor bug.

	Steve

> 
> Alan Stern
> 
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC][USB] powerpc: Workaround for the PPC440EPX USBH_23 errata
  2008-08-28 22:43           ` Benjamin Herrenschmidt
@ 2008-08-29 15:26             ` Alan Stern
  2008-08-29 18:20               ` Vitaly Bordug
  0 siblings, 1 reply; 18+ messages in thread
From: Alan Stern @ 2008-08-29 15:26 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: linux-usb, linuxppc-dev, Mark Miesfeld, Stefan Roese

On Fri, 29 Aug 2008, Benjamin Herrenschmidt wrote:

> I suppose some embedded platforms don't use CONFIG_PM, is this still a
> requirement for autosuspend ? Or do that happen always on an empty port
> nowadays ?

ohci-hcd doesn't automatically suspend the root hub if CONFIG_PM isn't
set.  However it isn't necessary to set CONFIG_SLEEP,
CONFIG_HIBERNATION, or CONFIG_USB_SUSPEND.

Alan Stern

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC][USB] powerpc: Workaround for the PPC440EPX USBH_23 errata
  2008-08-28 21:11       ` Steven A. Falco
  2008-08-28 21:33         ` Alan Stern
@ 2008-08-29 17:56         ` Matthias Fuchs
  1 sibling, 0 replies; 18+ messages in thread
From: Matthias Fuchs @ 2008-08-29 17:56 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: Stefan Roese, Mark Miesfeld, Alan Stern

Hi,

please see my comment below. It might be helpful for some of you.

On Thursday 28 August 2008 23:11:51 Steven A. Falco wrote:
> Alan Stern wrote:
> > Your original post mentioned that the 440EPx has only one USB 2.0 host
> > port.  Then how can you use a keyboard and memory stick at the same
> > time?  You'd have to plug them into a hub -- in which case only one
> > controller would be needed, the one driving the hub.  The patch would
> > be unnecessary.
>
> I have one of these processors on a Sequoia board.  What happens is that
> if you build the kernel with both EHCI and OHCI support, then plug in
> a modern USB memory stick, it initially tries EHCI, the driver fails, and
> the whole thing falls back to OHCI.  So you wind up running at 12 Mbps.
> The only way to make high speed work is to turn off the OHCI driver, and
> then you cannot support slow devices with that kernel.
There is a simple trick to get all types (full, low ang high speed) of  
devices work: use a USB hub with transaction translators. We are doing so on 
a custom design based on the 440EPx. So we only build the kernel with EHCI 
support and there was no need for this ugly hack.
>
> So, hile you cannot plug two devices in at one time, you can plug in
> different speed devices at different times, and my understanding is that
> this patch will let that work seamlessly.
That's true.

Matthias

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC][USB] powerpc: Workaround for the PPC440EPX USBH_23 errata
  2008-08-29 15:26             ` Alan Stern
@ 2008-08-29 18:20               ` Vitaly Bordug
  2008-08-29 21:30                 ` Alan Stern
  0 siblings, 1 reply; 18+ messages in thread
From: Vitaly Bordug @ 2008-08-29 18:20 UTC (permalink / raw)
  To: Alan Stern; +Cc: linux-usb, linuxppc-dev, Mark Miesfeld, Stefan Roese

В Fri, 29 Aug 2008 11:26:07 -0400 (EDT)
Alan Stern <stern@rowland.harvard.edu> пишет:

> On Fri, 29 Aug 2008, Benjamin Herrenschmidt wrote:
> 
> > I suppose some embedded platforms don't use CONFIG_PM, is this
> > still a requirement for autosuspend ? Or do that happen always on
> > an empty port nowadays ?
> 
> ohci-hcd doesn't automatically suspend the root hub if CONFIG_PM isn't
> set.  However it isn't necessary to set CONFIG_SLEEP,
> CONFIG_HIBERNATION, or CONFIG_USB_SUSPEND.

That is good to know. 

But even assuming PM set, common use-case of
embedded systems to have stuff on USB bus that is never plugged off;
and in case of compiled-in ehci and ohci there is just 

Initializing USB Mass Storage driver...
usb 1-1: new high speed USB device using ppc-of-ehci and address 2
usb 1-1: device descriptor read/all, error -110
hub 1-0:1.0: unable to enumerate USB device on port 1

(not touching PM here to be clear)

even 1-second delay would be enough for root hub to be hosed... So, is
the patch (with all the issues addressed) acceptable for mainline?

Thanks, 
-Vitaly

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC][USB] powerpc: Workaround for the PPC440EPX USBH_23 errata
  2008-08-29 18:20               ` Vitaly Bordug
@ 2008-08-29 21:30                 ` Alan Stern
  2008-09-04 20:48                   ` Vitaly Bordug
  0 siblings, 1 reply; 18+ messages in thread
From: Alan Stern @ 2008-08-29 21:30 UTC (permalink / raw)
  To: Vitaly Bordug; +Cc: USB list, linuxppc-dev, Mark Miesfeld, Stefan Roese

On Fri, 29 Aug 2008, Vitaly Bordug wrote:

> But even assuming PM set, common use-case of
> embedded systems to have stuff on USB bus that is never plugged off;
> and in case of compiled-in ehci and ohci there is just 
> 
> Initializing USB Mass Storage driver...
> usb 1-1: new high speed USB device using ppc-of-ehci and address 2
> usb 1-1: device descriptor read/all, error -110
> hub 1-0:1.0: unable to enumerate USB device on port 1
> 
> (not touching PM here to be clear)
> 
> even 1-second delay would be enough for root hub to be hosed... So, is
> the patch (with all the issues addressed) acceptable for mainline?

Try doing this instead.  First, make sure CONFIG_PM is set!  :-)
Then replace your patch with something that adds a 2-second delay to 
the end of ehci_start() when running on a 440EPx system.  That should 
give the OHCI controller time to suspend before the EHCI root hub is 
registered.

A little more tweaking will be needed to handle system sleeps.  But 
this should be a good start.

What to do when CONFIG_PM is off is a separate matter.  Let's not worry 
about it for now -- especially since, as Matthias suggested, you can 
use a USB 2.0 hub.

Alan Stern

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC][USB] powerpc: Workaround for the PPC440EPX USBH_23 errata
  2008-08-29 21:30                 ` Alan Stern
@ 2008-09-04 20:48                   ` Vitaly Bordug
  2008-09-04 21:40                     ` Alan Stern
  0 siblings, 1 reply; 18+ messages in thread
From: Vitaly Bordug @ 2008-09-04 20:48 UTC (permalink / raw)
  To: Alan Stern; +Cc: USB list, Mark, linuxppc-dev, Miesfeld, Stefan Roese

В Fri, 29 Aug 2008 17:30:57 -0400 (EDT)
Alan Stern <stern@rowland.harvard.edu> пишет:

> On Fri, 29 Aug 2008, Vitaly Bordug wrote:
> 
> > But even assuming PM set, common use-case of
> > embedded systems to have stuff on USB bus that is never plugged off;
> > and in case of compiled-in ehci and ohci there is just 
> > 
> > Initializing USB Mass Storage driver...
> > usb 1-1: new high speed USB device using ppc-of-ehci and address 2
> > usb 1-1: device descriptor read/all, error -110
> > hub 1-0:1.0: unable to enumerate USB device on port 1
> > 
> > (not touching PM here to be clear)
> > 
> > even 1-second delay would be enough for root hub to be hosed... So,
> > is the patch (with all the issues addressed) acceptable for
> > mainline?
> 
> Try doing this instead.  First, make sure CONFIG_PM is set!  :-)
> Then replace your patch with something that adds a 2-second delay to 
> the end of ehci_start() when running on a 440EPx system.  That should 
> give the OHCI controller time to suspend before the EHCI root hub is 
> registered.
> 
I've started looking this way. Sorry, but this approach is a little bit
fragile, and unreliable.

First, it just did not work out in case of usb2 hub with memory stick
and keybord plugged - OHCI did not suspend, ehci is hosed and we're
happily using hi-speed device on full-speed:

ppc-of-ehci e0000300.ehci: OF EHCI
ppc-of-ehci e0000300.ehci: new USB bus registered, assigned bus number 1
ppc-of-ehci e0000300.ehci: irq 32, io mem 0xe0000300
ppc-of-ehci e0000300.ehci: USB 2.0 started, EHCI 1.00, driver 10 Dec
2004 usb usb1: configuration #1 chosen from 1 choice
hub 1-0:1.0: USB hub found
hub 1-0:1.0: 1 port detected
usb usb1: New USB device found, idVendor=1d6b, idProduct=0002
usb usb1: New USB device strings: Mfr=3, Product=2, SerialNumber=1
usb usb1: Product: OF EHCI
usb usb1: Manufacturer: Linux 2.6.26-00016-g8914f6a-dirty ehci_hcd
usb usb1: SerialNumber: PPC-OF USB
ppc-of-ohci e0000400.usb: OF OHCI
ppc-of-ohci e0000400.usb: new USB bus registered, assigned bus number 2
ppc-of-ohci e0000400.usb: irq 33, io mem 0xe0000400
usb usb2: configuration #1 chosen from 1 choice
hub 2-0:1.0: USB hub found
hub 2-0:1.0: 1 port detected
usb usb2: New USB device found, idVendor=1d6b, idProduct=0001
usb usb2: New USB device strings: Mfr=3, Product=2, SerialNumber=1
usb usb2: Product: OF OHCI
usb usb2: Manufacturer: Linux 2.6.26-00016-g8914f6a-dirty ohci_hcd
usb usb2: SerialNumber: PPC-OF USB
Initializing USB Mass Storage driver...
usb 1-1: new high speed USB device using ppc-of-ehci and address 2
usb 1-1: device not accepting address 2, error -110
usb 1-1: new high speed USB device using ppc-of-ehci and address 3
usb 1-1: device not accepting address 3, error -110
usb 1-1: new high speed USB device using ppc-of-ehci and address 4
usb 1-1: device not accepting address 4, error -110
usb 1-1: new high speed USB device using ppc-of-ehci and address 5
usb 1-1: device not accepting address 5, error -110
hub 1-0:1.0: unable to enumerate USB device on port 1
usb 2-1: new full speed USB device using ppc-of-ohci and address 2

Secondly, we may *not* rely on the fact, that OHCI will always have the
same suspend policy. Even kicking the code up to the shape when it will
automagically suspend in proper timing to get the HW issue around, we
cannot be sure that it will persist along kernel lifecycle, and won't
require concerned people to kick suspend timings back to the working
state subsequently each rc release.

Thirdly, PM is disabled by Kconfig explicitly in case of 44x. Reasoning
is not clear at the moment, but I believe that isn't there just in case.


> A little more tweaking will be needed to handle system sleeps.  But 
> this should be a good start.

Getting 44x into proper PM land is good direction, but right now there
is a situation for certain platform when USB trimmed down to full-speed
mode in evaluation design and many inheritances. The only way to have
all the benefits, was maintaining internally some version of AMCC
workaorund
> 
> What to do when CONFIG_PM is off is a separate matter.  Let's not
> worry about it for now -- especially since, as Matthias suggested,
> you can use a USB 2.0 hub.

Not every hub will work (none of available did so far), and it is often
not an option for embedded device without rewiring.

As this touches powerpc stuff only, are there any objections to let
powerpc peolple consider if approach suggested earlier is applicable or
not?

Thanks,

-Vitaly

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC][USB] powerpc: Workaround for the PPC440EPX USBH_23 errata
  2008-09-04 20:48                   ` Vitaly Bordug
@ 2008-09-04 21:40                     ` Alan Stern
  2008-09-05  1:35                       ` Benjamin Herrenschmidt
  2008-09-05 13:01                       ` Vitaly Bordug
  0 siblings, 2 replies; 18+ messages in thread
From: Alan Stern @ 2008-09-04 21:40 UTC (permalink / raw)
  To: Vitaly Bordug; +Cc: USB list, linuxppc-dev, Mark Miesfeld, Stefan Roese

On Fri, 5 Sep 2008, Vitaly Bordug wrote:

> I've started looking this way. Sorry, but this approach is a little bit
> fragile, and unreliable.
> 
> First, it just did not work out in case of usb2 hub with memory stick
> and keybord plugged - OHCI did not suspend, ehci is hosed and we're
> happily using hi-speed device on full-speed:

> Secondly, we may *not* rely on the fact, that OHCI will always have the
> same suspend policy. Even kicking the code up to the shape when it will
> automagically suspend in proper timing to get the HW issue around, we
> cannot be sure that it will persist along kernel lifecycle, and won't
> require concerned people to kick suspend timings back to the working
> state subsequently each rc release.
> 
> Thirdly, PM is disabled by Kconfig explicitly in case of 44x. Reasoning
> is not clear at the moment, but I believe that isn't there just in case.

I assume that's the reason the suggested approach failed.

> > What to do when CONFIG_PM is off is a separate matter.  Let's not
> > worry about it for now -- especially since, as Matthias suggested,
> > you can use a USB 2.0 hub.
> 
> Not every hub will work (none of available did so far), and it is often
> not an option for embedded device without rewiring.

It's odd that your hubs don't work.  What's wrong with them?

> As this touches powerpc stuff only, are there any objections to let
> powerpc peolple consider if approach suggested earlier is applicable or
> not?

I don't mind doing that, provided the changes are cleaned up so that 
they don't affect people who aren't building kernels for 44x systems.

Alan Stern

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC][USB] powerpc: Workaround for the PPC440EPX USBH_23 errata
  2008-09-04 21:40                     ` Alan Stern
@ 2008-09-05  1:35                       ` Benjamin Herrenschmidt
  2008-09-05 13:01                       ` Vitaly Bordug
  1 sibling, 0 replies; 18+ messages in thread
From: Benjamin Herrenschmidt @ 2008-09-05  1:35 UTC (permalink / raw)
  To: Alan Stern; +Cc: USB list, linuxppc-dev, Mark Miesfeld, Stefan Roese


> I assume that's the reason the suggested approach failed.

I think (hope) that Vitaly actually fixed that before trying it :-)

> I don't mind doing that, provided the changes are cleaned up so that 
> they don't affect people who aren't building kernels for 44x systems.

I also find the CONFIG_PM approach not terribly reliable... too timing
sensitive, and it forces boards to add CONFIG_PM which is not
necessarily the nicest thing to do in embedded space.

Sounds better to me to clean up the initial patch to ensure it's not
going to introduce bloat for other platforms and use that, ie just
consider it as yet another HW errata.

Cheers,
Ben.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC][USB] powerpc: Workaround for the PPC440EPX USBH_23 errata
  2008-09-04 21:40                     ` Alan Stern
  2008-09-05  1:35                       ` Benjamin Herrenschmidt
@ 2008-09-05 13:01                       ` Vitaly Bordug
  2008-09-05 15:17                         ` Alan Stern
  1 sibling, 1 reply; 18+ messages in thread
From: Vitaly Bordug @ 2008-09-05 13:01 UTC (permalink / raw)
  To: Alan Stern; +Cc: USB list, Mark, linuxppc-dev, Miesfeld, Stefan Roese

В Thu, 4 Sep 2008 17:40:49 -0400 (EDT)
Alan Stern <stern@rowland.harvard.edu> пишет:

> On Fri, 5 Sep 2008, Vitaly Bordug wrote:
> 
> > I've started looking this way. Sorry, but this approach is a little
> > bit fragile, and unreliable.
> > 
> > First, it just did not work out in case of usb2 hub with memory
> > stick and keybord plugged - OHCI did not suspend, ehci is hosed and
> > we're happily using hi-speed device on full-speed:
> 
> > Secondly, we may *not* rely on the fact, that OHCI will always have
> > the same suspend policy. Even kicking the code up to the shape when
> > it will automagically suspend in proper timing to get the HW issue
> > around, we cannot be sure that it will persist along kernel
> > lifecycle, and won't require concerned people to kick suspend
> > timings back to the working state subsequently each rc release.
> > 
> > Thirdly, PM is disabled by Kconfig explicitly in case of 44x.
> > Reasoning is not clear at the moment, but I believe that isn't
> > there just in case.
> 
> I assume that's the reason the suggested approach failed.

Oh geez. I may miss some small point, but dunno what made me appear
*that* dense. The kernel was properly rebuilt with Kconfig fixed and
_PM set.
> 
> > > What to do when CONFIG_PM is off is a separate matter.  Let's not
> > > worry about it for now -- especially since, as Matthias suggested,
> > > you can use a USB 2.0 hub.
> > 
> > Not every hub will work (none of available did so far), and it is
> > often not an option for embedded device without rewiring.
> 
> It's odd that your hubs don't work.  What's wrong with them?
> 

well, they do not have transaction translators then. Nothing really
wrong
> > As this touches powerpc stuff only, are there any objections to let
> > powerpc peolple consider if approach suggested earlier is
> > applicable or not?
> 
> I don't mind doing that, provided the changes are cleaned up so that 
> they don't affect people who aren't building kernels for 44x systems.
OK, thanks for review and comments...

--
Sincerely,

Vitaly

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC][USB] powerpc: Workaround for the PPC440EPX USBH_23 errata
  2008-09-05 13:01                       ` Vitaly Bordug
@ 2008-09-05 15:17                         ` Alan Stern
  2008-09-06  9:02                           ` Vitaly Bordug
  0 siblings, 1 reply; 18+ messages in thread
From: Alan Stern @ 2008-09-05 15:17 UTC (permalink / raw)
  To: Vitaly Bordug; +Cc: USB list, linuxppc-dev, Mark Miesfeld, Stefan Roese

On Fri, 5 Sep 2008, Vitaly Bordug wrote:

> > > Not every hub will work (none of available did so far), and it is
> > > often not an option for embedded device without rewiring.
> > 
> > It's odd that your hubs don't work.  What's wrong with them?
> > 
> 
> well, they do not have transaction translators then. Nothing really
> wrong

No.  If the hubs run at high speed then they must have transaction 
translators; it's required by the USB 2.0 specification.

Did you try using them with ohci-hcd not loaded?

Alan Stern

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC][USB] powerpc: Workaround for the PPC440EPX USBH_23 errata
  2008-09-05 15:17                         ` Alan Stern
@ 2008-09-06  9:02                           ` Vitaly Bordug
  0 siblings, 0 replies; 18+ messages in thread
From: Vitaly Bordug @ 2008-09-06  9:02 UTC (permalink / raw)
  To: Alan Stern; +Cc: USB list, Mark, linuxppc-dev, Miesfeld, Stefan Roese

В Fri, 5 Sep 2008 11:17:03 -0400 (EDT)
Alan Stern <stern@rowland.harvard.edu> пишет:

> On Fri, 5 Sep 2008, Vitaly Bordug wrote:
> 
> > > > Not every hub will work (none of available did so far), and it
> > > > is often not an option for embedded device without rewiring.
> > > 
> > > It's odd that your hubs don't work.  What's wrong with them?
> > > 
> > 
> > well, they do not have transaction translators then. Nothing really
> > wrong
> 
> No.  If the hubs run at high speed then they must have transaction 
> translators; it's required by the USB 2.0 specification.
> 
> Did you try using them with ohci-hcd not loaded?

yes, not with every unit though. That seems to be usb hub issue then.


Anyway, the patch is targeted more to
the embedded platforms, that cannot afford work the issue around with
extra devices.

> 
> Alan Stern

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2008-09-06  9:05 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-08-26 22:23 [RFC][USB] powerpc: Workaround for the PPC440EPX USBH_23 errata Vitaly Bordug
2008-08-27 14:36 ` Alan Stern
2008-08-28 19:32   ` Vitaly Bordug
2008-08-28 20:13     ` Alan Stern
2008-08-28 21:11       ` Steven A. Falco
2008-08-28 21:33         ` Alan Stern
2008-08-28 22:43           ` Benjamin Herrenschmidt
2008-08-29 15:26             ` Alan Stern
2008-08-29 18:20               ` Vitaly Bordug
2008-08-29 21:30                 ` Alan Stern
2008-09-04 20:48                   ` Vitaly Bordug
2008-09-04 21:40                     ` Alan Stern
2008-09-05  1:35                       ` Benjamin Herrenschmidt
2008-09-05 13:01                       ` Vitaly Bordug
2008-09-05 15:17                         ` Alan Stern
2008-09-06  9:02                           ` Vitaly Bordug
2008-08-29 13:25           ` Steven A. Falco
2008-08-29 17:56         ` Matthias Fuchs

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).