[Cbe-oss-dev] [PATCH 1/5 v2] usb: Fix PS3 EHCI suspend

[Geoff Levand]

Hi Alan,

On Mon, 2011-12-05 at 11:57 -0500, Alan Stern wrote:
> On Thu, 1 Dec 2011, Geoff Levand wrote:
> >  drivers/usb/host/ehci-hcd.c |   28 ++++++++++++++++++++++++++++
> >  1 files changed, 28 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> > index 46dccbf..4a8391c 100644
> > --- a/drivers/usb/host/ehci-hcd.c
> > +++ b/drivers/usb/host/ehci-hcd.c
> > @@ -48,6 +48,10 @@
> >  #include <asm/system.h>
> >  #include <asm/unaligned.h>
> >  
> > +#if defined(CONFIG_PPC_PS3)
> > +#include <asm/firmware.h>
> > +#endif
> 
> That's okay; you don't need the #if here -- there's nothing wrong with
> #include-ing the header unconditionally.


This is asm/firmware.h, and only powerpc has it.  Without the conditionals
the build will fail for non-powerpc.  I need this included for the
firmware_has_feature() test.


> > +
> >  /*-------------------------------------------------------------------------*/
> >  
> >  /*
> > @@ -236,6 +240,30 @@ static int handshake_on_error_set_halt(struct ehci_hcd *ehci, void __iomem *ptr,
> >  	int error;
> >  
> >  	error = handshake(ehci, ptr, mask, done, usec);
> > +
> > +	/*
> > +	 * The EHCI controller of the Cell Super Companion Chip used in the
> > +	 * PS3 will stop the root hub after all root hub ports are suspended.
> > +	 * When in this condition handshake will return -ETIMEDOUT.  The
> > +	 * STS_HLT bit will not be set, so inspection of the frame index is
> > +	 * used here to test for the condition.  If the condition is found
> > +	 * return success to allow the USB suspend to complete.
> > +	 */
> > +
> > +#if defined(CONFIG_USB_SUSPEND) && defined(CONFIG_PPC_PS3)
> > +	if (firmware_has_feature(FW_FEATURE_PS3_LV1) && error == -ETIMEDOUT) {
> > +#else
> > +	if (0) {
> > +#endif
> > +		unsigned int old_index = ehci_read_frame_index(ehci);
> > +
> > +		error = handshake(ehci, ptr, mask, done, usec);
> > +
> > +		if (error == -ETIMEDOUT
> > +			&& (ehci_read_frame_index(ehci) == old_index))
> > +			return 0;
> > +	}
> > +
> >  	if (error) {
> >  		ehci_halt(ehci);
> >  		ehci->rh_state = EHCI_RH_HALTED;
> > 
> 
> This is a little ugly, with the preprocessor stuff inside the function.  
> The generally preferred approach puts functions inside preprocessor 
> blocks instead of the other way around, like this:
> 
> #if defined(...)

OK, a new patch will follow.

-Geoff