On Tue, Mar 27, 2012 at 10:53:07AM +0800, Alex He wrote: > 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 the xHCI has been handoffed to OS > and it is no use for BIOS any more. > > > > > 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. > > > Acctualy, I just want to disable all SMI and clear status bits[29:30] > but without any affect other bits of USBLEGCTLSTS. Ok, that's what I thought you were trying to do. You also need to save any preserved bits in the register, while still writing zero to the reserved bits. So you'll need to write zeroes to bits 28:21, since they are RsvdZ. You're not doing that with this patch (or my own psuedo code, since I just noticed the RsvdZ bits). > > 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) > Yes. It is the better way. > > > > 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 > that prerfectionism rule. The opinions on the 80-char rule differ from maintainer to maintainer, so maybe you've received conflicting advice? For me, in general, I want lines to be under 80 characters long. If you're going over 80 characters in a function, you really need to be refactoring that code into a new function. However, if there's no way to make a line shorter, or squeezing the line into 80 characters breaks the readability of the code, then it's fine to go over 80 chars. I would rather the code be readable than strictly follow the 80-char 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); > > > > 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 > Yes. The spec doesn't say these SMI event bits[29:31] need be cleared > during the handoff. But I have told my reason above. What do you think > about it? I am glad to resend v3 patch if we reach the agreement. Yes, I think we should clear the SMI event bits. So please resend your patch with the improved masking, and making sure to write zeroes to those RsvdZ bits. 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
