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

[Alan Stern]

On Thu, 1 Dec 2011, Geoff Levand wrote:

> The EHCI USB 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 the ehci-hcd handshake routine will return
> -ETIMEDOUT and the USB runtime suspend sequence will fail.
> 
> The fix implemented here is to check the result of the handshake
> call done in handshake_on_error_set_halt() and if the condition
> is found return success to allow the USB suspend to complete.
> The STS_HLT bit will not be set, so inspection of the frame index
> is used to test for the condition.
> 
> Signed-off-by: Geoff Levand <geoff at infradead.org>
> ---
> Alan,
> 
> I did the logic so that there would be a minimal overhead
> when running on PS3, and should be optimized out when
> CONFIG_PPC_PS3=n.
> 
> -Geoff
> 
> v2: - Put work-around in handshake_on_error_set_halt();
> 
>  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.

> +
>  /*-------------------------------------------------------------------------*/
>  
>  /*
> @@ -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(...)

/*
 * The EHCI controller of the Cell ...
 */
static int handshake_for_broken_root_hub(...)
{
	...
}

#else

static inline int handshake_for_broken_root_hub(...)
{ return -ETIMEDOUT; }

#endif


static int handshake_on_error_set_halt(...)
{
 	int error;
 
 	error = handshake(ehci, ptr, mask, done, usec);
	if (error == -ETIMEDOUT)
		error = handshake_for_broken_root_hub(ehci, ptr, mask, 
				done, usec);
	...
}

Alan Stern