On Fri, 5 Dec 2014, Arseny Solokha wrote:
> From: Arseny Solokha <asolokha@xxxxxxxxxx>
>
> Commit 8dccddbc2368 ("OHCI: final fix for NVIDIA problems (I hope)")
> introduced into 3.1.9 broke boot on e.g. Freescale P2020DS development
> board. The code path that was previously specific to NVIDIA controllers
> had then become taken for all chips.
>
> However, the M5237 installed on the board wedges solid when accessing
> its base+OHCI_FMINTERVAL register, making it impossible to boot any
> kernel newer than 3.1.8 on this particular and apparently other similar
> machines.
>
> Don't readl() and writel() base+OHCI_FMINTERVAL on PCI ID 10b9:5237.
I seem to recall seeing something like this before, but I can't
remember where or when. At any rate, there's nothing like it in the
current code...
> The patch is suitable for the -next tree as well as all maintained
> kernels up to 3.2 inclusive.
>
> Signed-off-by: Arseny Solokha <asolokha@xxxxxxxxxx>
> ---
> drivers/usb/host/pci-quirks.c | 14 +++++++++++---
> include/linux/pci_ids.h | 1 +
> 2 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
> index 2f3aceb..3b29478 100644
> --- a/drivers/usb/host/pci-quirks.c
> +++ b/drivers/usb/host/pci-quirks.c
> @@ -571,7 +571,7 @@ static void quirk_usb_handoff_ohci(struct pci_dev *pdev)
> {
> void __iomem *base;
> u32 control;
> - u32 fminterval;
> + u32 fminterval = 0;
> int cnt;
>
> if (!mmio_resource_enabled(pdev, 0))
> @@ -619,7 +619,13 @@ static void quirk_usb_handoff_ohci(struct pci_dev *pdev)
> }
>
> /* software reset of the controller, preserving HcFmInterval */
> - fminterval = readl(base + OHCI_FMINTERVAL);
> + if (!(pdev->vendor == PCI_VENDOR_ID_AL &&
> + pdev->device == PCI_DEVICE_ID_AL_M5237)) {
> + /* ULi M5237 OHCI controller (10b9:5237) locks the whole system
> + * when accessing the OHCI_FMINTERVAL offset.
> + */
You don't need to specify the vendor and device IDs in the comment.
That's what #defines are for.
Also, the accepted format for multi-line comments is:
/*
* Blah, blah, blah...
* Blah, blah, blah...
*/
> + fminterval = readl(base + OHCI_FMINTERVAL);
> + }
> writel(OHCI_HCR, base + OHCI_CMDSTATUS);
>
> /* reset requires max 10 us delay */
> @@ -628,7 +634,9 @@ static void quirk_usb_handoff_ohci(struct pci_dev *pdev)
> break;
> udelay(1);
> }
> - writel(fminterval, base + OHCI_FMINTERVAL);
> + if (!(pdev->vendor == PCI_VENDOR_ID_AL &&
> + pdev->device == PCI_DEVICE_ID_AL_M5237))
> + writel(fminterval, base + OHCI_FMINTERVAL);
This is very ugly, with these repeated tests. You should set a
no_fminterval flag at the start of the function and then use it instead
of doing the same tests over again.
> /* Now the controller is safely in SUSPEND and nothing can wake it up */
> iounmap(base);
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index 1fa99a3..266fc5c 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -1107,6 +1107,7 @@
> #define PCI_DEVICE_ID_AL_M5219 0x5219
> #define PCI_DEVICE_ID_AL_M5228 0x5228
> #define PCI_DEVICE_ID_AL_M5229 0x5229
> +#define PCI_DEVICE_ID_AL_M5237 0x5237
> #define PCI_DEVICE_ID_AL_M5451 0x5451
> #define PCI_DEVICE_ID_AL_M7101 0x7101
There's no reason to add an ID to pci_ids.h if it's only going to be
used in one source file. Put the ID in the source file instead.
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
