On Mon, Mar 26, 2012 at 12:13:20PM -0700, Sarah Sharp wrote: > 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. OK. > > 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? Yes. I want to clear bits 29:31 since OS take away the xHCI after handoff. > > 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) Agree. > > Please note the spaces around the << here. The line will be long > anyway, so keep the spaces intact to make the line humanly readable. The 80-characters limit always puzzle me. You encourage me to break this perfectionism rule.:) > > 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); > It is the better way. > 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 The spec does not says that the SMI event bits should be clear during the handoff. But I have my reason above. What do you think about it? I will commit v3 patch if we reach agreement. Thanks, Alex -- 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
