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@xxxxxxxxxxxxx>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
