2010-12-22 14:29:03

by Anoop P.A

Subject: [PATCH V2 2/2] MSP onchip root hub over current quirk.

From: Anoop P A <[email protected]>

Adding chip specific code under quirk.

Signed-off-by: Anoop P A <[email protected]>
---
drivers/usb/core/hub.c | 45 ++++++++++++++++++++++++++++++++++++++-----
drivers/usb/core/quirks.c | 3 ++
include/linux/usb/quirks.h | 3 ++
3 files changed, 45 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 27115b4..4bff994 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -3377,12 +3377,45 @@ static void hub_events(void)
}

if (portchange & USB_PORT_STAT_C_OVERCURRENT) {
- dev_err (hub_dev,
- "over-current change on port %d\n",
- i);
- clear_port_feature(hdev, i,
- USB_PORT_FEAT_C_OVER_CURRENT);
- hub_power_on(hub, true);
+ usb_detect_quirks(hdev);
+ if (hdev->quirks & USB_QUIRK_MSP_OVERCURRENT) {
+ /* clear OCC bit */
+ clear_port_feature(hdev, i,
+ USB_PORT_FEAT_C_OVER_CURRENT);
+
+ /* This step is required to toggle the
+ * PP bit to 0 and 1 (by hub_power_on)
+ * in order the CSC bit to be
+ * transitioned properly for device
+ * hotplug
+ */
+ /* clear PP bit */
+ clear_port_feature(hdev, i,
+ USB_PORT_FEAT_POWER);
+
+ /* resume power */
+ hub_power_on(hub, true);
+
+ /* delay 100 usec */
+ udelay(100);
+
+ /* read OCA bit */
+ if (portstatus &
+ (1<<USB_PORT_FEAT_OVER_CURRENT)) {
+ /* declare overcurrent */
+ dev_err(hub_dev,
+ "over-current change \
+ on port %d\n", i);
+ }
+ } else {
+ dev_err(hub_dev,
+ "over-current change \
+ on port %d\n", i);
+ clear_port_feature(hdev, i,
+ USB_PORT_FEAT_C_OVER_CURRENT);
+ hub_power_on(hub, true);
+ }
+
}

if (portchange & USB_PORT_STAT_C_RESET) {
diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index 25719da..59843b9 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -88,6 +88,9 @@ static const struct usb_device_id usb_quirk_list[] = {
/* INTEL VALUE SSD */
{ USB_DEVICE(0x8086, 0xf1a5), .driver_info = USB_QUIRK_RESET_RESUME },

+ /* PMC MSP over current quirk */
+ { USB_DEVICE(0x1d6b, 0x0002), .driver_info = USB_QUIRK_MSP_OVERCURRENT },
+
{ } /* terminating entry must be last */
};

diff --git a/include/linux/usb/quirks.h b/include/linux/usb/quirks.h
index 3e93de7..97ab168 100644
--- a/include/linux/usb/quirks.h
+++ b/include/linux/usb/quirks.h
@@ -30,4 +30,7 @@
descriptor */
#define USB_QUIRK_DELAY_INIT 0x00000040

+/*MSP SoC onchip EHCI overcurrent issue */
+#define USB_QUIRK_MSP_OVERCURRENT 0x00000080
+
#endif /* __LINUX_USB_QUIRKS_H */
--
1.7.0.4

2010-12-23 02:18:23

by Alan Stern

Subject: Re: [PATCH V2 2/2] MSP onchip root hub over current quirk.

On Wed, 22 Dec 2010, Anoop P.A wrote:

> From: Anoop P A <[email protected]>
>
> Adding chip specific code under quirk.

NAK. See below.

> Signed-off-by: Anoop P A <[email protected]>
> ---
> drivers/usb/core/hub.c | 45 ++++++++++++++++++++++++++++++++++++++-----
> drivers/usb/core/quirks.c | 3 ++
> include/linux/usb/quirks.h | 3 ++
> 3 files changed, 45 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 27115b4..4bff994 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -3377,12 +3377,45 @@ static void hub_events(void)
> }
>
> if (portchange & USB_PORT_STAT_C_OVERCURRENT) {
> - dev_err (hub_dev,
> - "over-current change on port %d\n",
> - i);
> - clear_port_feature(hdev, i,
> - USB_PORT_FEAT_C_OVER_CURRENT);
> - hub_power_on(hub, true);
> + usb_detect_quirks(hdev);

This line is wrong. usb_detect_quirks() gets called only once per
device, when the device is initialized. Besides, you probably want to
use a hub-specific flag for this rather than a device-specific flag.

> + if (hdev->quirks & USB_QUIRK_MSP_OVERCURRENT) {

Also, it would be better to put this code in a separate subroutine
instead of indenting it so far.

> + /* clear OCC bit */
> + clear_port_feature(hdev, i,
> + USB_PORT_FEAT_C_OVER_CURRENT);
> +
> + /* This step is required to toggle the
> + * PP bit to 0 and 1 (by hub_power_on)
> + * in order the CSC bit to be
> + * transitioned properly for device
> + * hotplug
> + */
> + /* clear PP bit */
> + clear_port_feature(hdev, i,
> + USB_PORT_FEAT_POWER);
> +
> + /* resume power */
> + hub_power_on(hub, true);
> +
> + /* delay 100 usec */
> + udelay(100);
> +
> + /* read OCA bit */
> + if (portstatus &
> + (1<<USB_PORT_FEAT_OVER_CURRENT)) {
> + /* declare overcurrent */
> + dev_err(hub_dev,
> + "over-current change \
> + on port %d\n", i);
> + }
> + } else {
> + dev_err(hub_dev,
> + "over-current change \
> + on port %d\n", i);
> + clear_port_feature(hdev, i,
> + USB_PORT_FEAT_C_OVER_CURRENT);
> + hub_power_on(hub, true);
> + }
> +
> }
>
> if (portchange & USB_PORT_STAT_C_RESET) {
> diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
> index 25719da..59843b9 100644
> --- a/drivers/usb/core/quirks.c
> +++ b/drivers/usb/core/quirks.c
> @@ -88,6 +88,9 @@ static const struct usb_device_id usb_quirk_list[] = {
> /* INTEL VALUE SSD */
> { USB_DEVICE(0x8086, 0xf1a5), .driver_info = USB_QUIRK_RESET_RESUME },
>
> + /* PMC MSP over current quirk */
> + { USB_DEVICE(0x1d6b, 0x0002), .driver_info = USB_QUIRK_MSP_OVERCURRENT },
> +

This implementation is completely wrong. It applies to all USB-2.0
root hubs in Linux, not just the PMC MSP.

> { } /* terminating entry must be last */
> };
>
> diff --git a/include/linux/usb/quirks.h b/include/linux/usb/quirks.h
> index 3e93de7..97ab168 100644
> --- a/include/linux/usb/quirks.h
> +++ b/include/linux/usb/quirks.h
> @@ -30,4 +30,7 @@
> descriptor */
> #define USB_QUIRK_DELAY_INIT 0x00000040
>
> +/*MSP SoC onchip EHCI overcurrent issue */
> +#define USB_QUIRK_MSP_OVERCURRENT 0x00000080
> +
> #endif /* __LINUX_USB_QUIRKS_H */
>

2010-12-23 09:21:00

by Anoop P.A

Subject: Re: [PATCH V2 2/2] MSP onchip root hub over current quirk.

On Wed, 2010-12-22 at 21:18 -0500, Alan Stern wrote:
> On Wed, 22 Dec 2010, Anoop P.A wrote:
>
> > From: Anoop P A <[email protected]>
> >
> > Adding chip specific code under quirk.
>
> NAK. See below.
>
> > Signed-off-by: Anoop P A <[email protected]>
> > ---
> > drivers/usb/core/hub.c | 45 ++++++++++++++++++++++++++++++++++++++-----
> > drivers/usb/core/quirks.c | 3 ++
> > include/linux/usb/quirks.h | 3 ++
> > 3 files changed, 45 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > index 27115b4..4bff994 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -3377,12 +3377,45 @@ static void hub_events(void)
> > }
> >
> > if (portchange & USB_PORT_STAT_C_OVERCURRENT) {
> > - dev_err (hub_dev,
> > - "over-current change on port %d\n",
> > - i);
> > - clear_port_feature(hdev, i,
> > - USB_PORT_FEAT_C_OVER_CURRENT);
> > - hub_power_on(hub, true);
> > + usb_detect_quirks(hdev);
>
> This line is wrong. usb_detect_quirks() gets called only once per
> device, when the device is initialized. Besides, you probably want to
> use a hub-specific flag for this rather than a device-specific flag.

Can you point me to an example for the recommended way of doing the
hack. I don't have much exposure to USB subsystem.

>
> > + if (hdev->quirks & USB_QUIRK_MSP_OVERCURRENT) {
>
> Also, it would be better to put this code in a separate subroutine
> instead of indenting it so far.
>
> > + /* clear OCC bit */
> > + clear_port_feature(hdev, i,
> > + USB_PORT_FEAT_C_OVER_CURRENT);
> > +
> > + /* This step is required to toggle the
> > + * PP bit to 0 and 1 (by hub_power_on)
> > + * in order the CSC bit to be
> > + * transitioned properly for device
> > + * hotplug
> > + */
> > + /* clear PP bit */
> > + clear_port_feature(hdev, i,
> > + USB_PORT_FEAT_POWER);
> > +
> > + /* resume power */
> > + hub_power_on(hub, true);
> > +
> > + /* delay 100 usec */
> > + udelay(100);
> > +
> > + /* read OCA bit */
> > + if (portstatus &
> > + (1<<USB_PORT_FEAT_OVER_CURRENT)) {
> > + /* declare overcurrent */
> > + dev_err(hub_dev,
> > + "over-current change \
> > + on port %d\n", i);
> > + }
> > + } else {
> > + dev_err(hub_dev,
> > + "over-current change \
> > + on port %d\n", i);
> > + clear_port_feature(hdev, i,
> > + USB_PORT_FEAT_C_OVER_CURRENT);
> > + hub_power_on(hub, true);
> > + }
> > +
> > }
> >
> > if (portchange & USB_PORT_STAT_C_RESET) {
> > diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
> > index 25719da..59843b9 100644
> > --- a/drivers/usb/core/quirks.c
> > +++ b/drivers/usb/core/quirks.c
> > @@ -88,6 +88,9 @@ static const struct usb_device_id usb_quirk_list[] = {
> > /* INTEL VALUE SSD */
> > { USB_DEVICE(0x8086, 0xf1a5), .driver_info = USB_QUIRK_RESET_RESUME },
> >
> > + /* PMC MSP over current quirk */
> > + { USB_DEVICE(0x1d6b, 0x0002), .driver_info = USB_QUIRK_MSP_OVERCURRENT },
> > +
>
> This implementation is completely wrong. It applies to all USB-2.0
> root hubs in Linux, not just the PMC MSP.
>
> > { } /* terminating entry must be last */
> > };
> >
> > diff --git a/include/linux/usb/quirks.h b/include/linux/usb/quirks.h
> > index 3e93de7..97ab168 100644
> > --- a/include/linux/usb/quirks.h
> > +++ b/include/linux/usb/quirks.h
> > @@ -30,4 +30,7 @@
> > descriptor */
> > #define USB_QUIRK_DELAY_INIT 0x00000040
> >
> > +/*MSP SoC onchip EHCI overcurrent issue */
> > +#define USB_QUIRK_MSP_OVERCURRENT 0x00000080
> > +
> > #endif /* __LINUX_USB_QUIRKS_H */
> >
>
Thanks
Anoop

2010-12-23 16:08:39

by Alan Stern

Subject: Re: [PATCH V2 2/2] MSP onchip root hub over current quirk.

On Thu, 23 Dec 2010, Anoop P A wrote:

> > > + usb_detect_quirks(hdev);
> >
> > This line is wrong. usb_detect_quirks() gets called only once per
> > device, when the device is initialized. Besides, you probably want to
> > use a hub-specific flag for this rather than a device-specific flag.
>
> Can you point me to an example for the recommended way of doing the
> hack. I don't have much exposure to USB subsystem.

One example, suitable for PCI devices, can be found in
drivers/usb/host/ehci-pci.c:ehci_pci_setup().

However the best approach would be for you to avoid adding any
special-purpose code at all. Is it possible to handle
overcurrent-change events in a way that will work just as well for
normal hubs as for your MSP root hub?

Alan Stern