Messages in this thread | | | Subject | RE: Updated MSI Patches | Date | Wed, 1 Oct 2003 11:26:37 -0700 | From | "Nguyen, Tom L" <> |
| |
On Wednesday, October 01, 2003, Jeff Garzik wrote: >> +Q2. Will it work on all the Pentium processors (P3, P4, Xeon, >> +AMD processors)? In P3 IPI's are transmitted on the APIC local >> +bus and in P4 and Xeon they are transmitted on the system >> +bus. Are there any implications with this? >> + >> +A2. MSI support enables a PCI device sending an inbound >> +memory write (0xfeexxxxx as target address) on its PCI bus >> +directly to the FSB. Since the message address has a >> +redirection hint bit cleared, it should work. >> + >> +Q3. The target address 0xfeexxxxx will be translated by the >> +Host Bridge into an interrupt message. Are there any >> +limitations on the chipsets such as Intel 8xx, Intel e7xxx, >> +or VIA? >> + >> +A3. If these chipsets support an inbound memory write with >> +target address set as 0xfeexxxxx, as conformed to PCI >> +specification 2.3 or latest, then it should work.
>I would prefer a section in the documentation that spells out, in plain >English, exactly what hardware support is needed for MSI to work. i.e.
>"APIC required and must be enabled" >"Northbridge must support MSI transactions" >etc.
>Using that information, users and developers may know _exactly_ whether >they can use MSI or not. From "Q2/A2" and "Q3/A3" above, it is not >clear to me. Agree. Thanks!
>> +static struct hw_interrupt_type msix_irq_type = { >> + "PCI MSI-X", >> + startup_msi_irq_w_maskbit, >> + shutdown_msi_irq_w_maskbit, >> + enable_msi_irq_w_maskbit, >> + disable_msi_irq_w_maskbit, >> + act_msi_irq_w_maskbit, >> + end_msi_irq_w_maskbit, >> + set_msi_irq_affinity >> +}; >> + >> +/* >> + * Interrupt Type for MSI PCI/PCI-X/PCI-Express Devices, >> + * which implement the MSI Capability Structure with >> + * Mask-and-Pending Bits. >> + */ >> +static struct hw_interrupt_type msi_irq_w_maskbit_type = { >> + "PCI MSI", >> + startup_msi_irq_w_maskbit, >> + shutdown_msi_irq_w_maskbit, >> + enable_msi_irq_w_maskbit, >> + disable_msi_irq_w_maskbit, >> + act_msi_irq_w_maskbit, >> + end_msi_irq_w_maskbit, >> + set_msi_irq_affinity >> +}; >> + >> +/* >> + * Interrupt Type for MSI PCI/PCI-X/PCI-Express Devices, >> + * which implement the MSI Capability Structure without >> + * Mask-and-Pending Bits. >> + */ >> +static struct hw_interrupt_type msi_irq_wo_maskbit_type = { >> + "PCI MSI", >> + startup_msi_irq_wo_maskbit, >> + shutdown_msi_irq_wo_maskbit, >> + enable_msi_irq_wo_maskbit, >> + disable_msi_irq_wo_maskbit, >> + act_msi_irq_wo_maskbit, >> + end_msi_irq_wo_maskbit, >> + set_msi_irq_affinity >> +};
>Suggest converting these structure initializers to C99 style: > > .member_name = value,
Agree. Thanks!
>> +/** >> + * msix_capability_init: configure device's MSI-X capability structure >> + * argument: dev of struct pci_dev type >> + * >> + * description: called to configure the device's MSI-X capability structure >> + * with a single MSI. To request for additional MSI vectors, the device drivers >> + * are required to utilize the following supported APIs: >> + * 1) msi_alloc_vectors(...) for requesting one or more MSI >> + * 2) msi_free_vectors(...) for releasing one or more MSI back to PCI subsystem >> + *
>When added header comments, please use the official kernel style, so >that it may be picked up automatically by the kernel documentation >processing system (Documentation/DocBook/*). Specifically, > > /** > >begins a header (you did this), > > * my_function - description > >begins a function header (you need to use " - "), > > * @arg1: description... > * @arg2: description... > >describes the arguments.
>The rest is fine. Note that lines ending with a colon ("blah blah:\n") >are bolded, and considered paragraph leaders.
Agree. Thanks for this input!
>> @@ -564,6 +565,9 @@ >> /* Fix up broken headers */ >> pci_fixup_device(PCI_FIXUP_HEADER, dev); >> >> + /* Fix up broken MSI hardware */ >> + pci_fixup_device(PCI_FIXUP_MSI, dev); >> + >> /* >> * Add the device to our list of discovered devices >> * and the bus list for fixup functions, etc.
>Why does MSI need a separate list of fixups? You are right. Sorry, I forgot to remove these lines after testing first alternative "quirk" solution. Will remove these lines.
>> +struct msg_data { >> + __u32 vector : 8, >> + delivery_mode : 3, /* 000b: FIXED >> + 001b: lowest priority >> + 111b: ExtINT */ >> + reserved_1 : 3, >> + level : 1, /* 0: deassert, 1: assert */ >> + trigger : 1, /* 0: edge, 1: level */ >> + reserved_2 : 16; >> +} __attribute__ ((packed)); >> + >> +struct msg_address { >> + union { >> + struct { __u32 >> + reserved_1 : 2, >> + dest_mode : 1, /* 0: physical, >> + 1: logical */ >> + redirection_hint : 1, /* 0: dedicated CPU >> + 1: lowest priority */ >> + reserved_2 : 8, >> + dest_id : 8, /* Destination ID */ >> + header : 12; /* FEEH */ >> + }u; >> + __u32 value; >> + }lo_address; >> + __u32 hi_address; >> +} __attribute__ ((packed)); >> + >> +struct msi_desc { >> + struct { >> + __u32 type : 5, /* {0: unused, 5h:MSI, 11h:MSI-X} */ >> + maskbit : 1, /* mask-pending bit supported ? */ >> + reserved: 2, /* reserved */ >> + entry_nr: 8, /* specific enabled entry */ >> + default_vector: 8, /* default pre-assigned vector */ >> + current_cpu: 8; /* current destination cpu */ >> + }msi_attrib; >> + >> + struct { >> + __u32 head : 16, >> + tail : 16; >> + }link; >> + >> + unsigned long mask_base; >> + struct pci_dev *dev; >> +};
>These bitfields should be defined in big-endian and little-endian forms, >shouldn't they? grep for __LITTLE_ENDIAN_BITFIELD and >__BIG_ENDIAN_BITFIELD in include/*/*.h.
Thanks for your input. We'll make change appropriately.
Thanks, Long - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |