On Fri, Mar 23, 2012 at 08:13:30PM +0800, Alex He wrote:
> Re-define XHCI_LEGACY_DISABLE_SMI and used it in right way. All SMI enable
> bits will be cleared to zero and flag bits 29:31 are also cleared to zero.
>
> Signed-off-by: Alex He <alex.he@xxxxxxx>
> ---
> drivers/usb/host/pci-quirks.c | 3 ++-
> drivers/usb/host/xhci-ext-caps.h | 4 ++--
> 2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
> index 7732d69..a1a1cb1 100644
> --- a/drivers/usb/host/pci-quirks.c
> +++ b/drivers/usb/host/pci-quirks.c
> @@ -826,7 +826,8 @@ static void __devinit quirk_usb_handoff_xhci(struct pci_dev *pdev)
> }
>
> /* Disable any BIOS SMIs */
> - writel(XHCI_LEGACY_DISABLE_SMI,
> + val = readl(base + ext_cap_offset + XHCI_LEGACY_CONTROL_OFFSET);
> + writel(val & XHCI_LEGACY_DISABLE_SMI,
> base + ext_cap_offset + XHCI_LEGACY_CONTROL_OFFSET);
>
> if (usb_is_intel_switchable_xhci(pdev))
> diff --git a/drivers/usb/host/xhci-ext-caps.h b/drivers/usb/host/xhci-ext-caps.h
> index c7f3312..582e17f 100644
> --- a/drivers/usb/host/xhci-ext-caps.h
> +++ b/drivers/usb/host/xhci-ext-caps.h
> @@ -62,8 +62,8 @@
> /* USB Legacy Support Control and Status Register - section 7.1.2 */
> /* Add this offset, plus the value of xECP in HCCPARAMS to the base address */
> #define XHCI_LEGACY_CONTROL_OFFSET (0x04)
> -/* bits 1:2, 5:12, and 17:19 need to be preserved; bits 21:28 should be zero */
> -#define XHCI_LEGACY_DISABLE_SMI ((0x3 << 1) + (0xff << 5) + (0x7 << 17))
> +/* bits 0, 4, 13:15 should be zero; bits 29:31 are RW1C */
> +#define XHCI_LEGACY_DISABLE_SMI (~((0x1<<0) + (0x1<<4) + (0x7<<13)))
Oh, this makes my brain hurt. Reverse masks are a pain to understand.
Please base the mask on the bits you want to preserve to keep the style
the same as the other masks in the code. I know the header file is not
always consistent, but that is the style I want.
You changed the comment to say that bits 29:31 are RW1C. Those bits are
status bits to indicate events to the BIOS, so it should have no effect
to write a zero to those bits. However, the mask you've defined will
write a one to those bits when they are set. Is that really what you
wanted to do?
Walking through this, the new XHCI_LEGACY_DISABLE_SMI mask is
(~((0x1<<0) + (0x1<<4) + (0x7<<13)))
which is this in boolean:
~(0000 0000 0000 0000 1110 0000 0001 0001)
which is this:
1111 1111 1111 1111 0001 1111 1110 1110
bit 31 0
You're ANDing the value read from the register with this mask. Any bit
that's zero in the mask is going to be changed to zero, and any bit
that's one is going to be preserved. So this mask will preserve bits
1:3, 5:12, and 16:31.
As I said, since bits 29:31 are RW1C, preserving those bits means you'll
be writing a one and clearing the event if any of the 29:31 bits are
set. If that's really what you mean to do, I would rather that be
explicit in the code, like this:
/* bits 1:3, 5:12, and 17:19 need to be preserved; bits 21:28 should be zero */
#define XHCI_LEGACY_DISABLE_SMI ((0x7 << 1) + (0xff << 5) + (0x7 << 17))
#define XHCI_LEGACY_SMI_EVENTS (0x7 << 29)
Please note the spaces around the << here. The line will be long
anyway, so keep the spaces intact to make the line humanly readable.
val = readl(base + ext_cap_offset + XHCI_LEGACY_CONTROL_OFFSET);
/* Mask off (turn off) any enabled SMIs */
val &= XHCI_LEGACY_SMI_EVENTS;
/* Clear any pending SMI events, RW1C */
val |= XHCI_LEGACY_SMI_EVENTS;
writel(val, base + ext_cap_offset + XHCI_LEGACY_CONTROL_OFFSET);
This assumes, of course, that you actually want to clear those SMI event
bits. I think you should, but I want you to look over that section as
well and tell me what you think.
Sarah Sharp
--
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
