Re: RFC: rewrite Documentation/pci.txt v3

[Grant Grundler <grundler@parisc-linux.org>]

Randy,
many thanks for the thorough review...lots of silly grammar things
that I should have but just didn't see.

On Mon, Oct 23, 2006 at 12:20:23PM -0700, Randy Dunlap wrote:
> On Sat, 21 Oct 2006 01:14:36 -0600 Grant Grundler wrote:
> 
> > 0. Structure of PCI drivers
> > ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > PCI drivers "discover" PCI devices in a system via pci_register_driver().
> > Actually, it's the other way around. The PCI generic code will notify the
> > driver at every the PCI device which match a "description" advertised
>          whenever ?

Yes. :)
I've rephrased that sentence to be a bit clearer:
	When the PCI generic code discovers a new device, the driver
	with a matching "description" will be notified.

Remember, this document is an introduction, not a complete reference.

> > by the driver. Details on this below.
> > 
> > 1. pci_register_driver() call
> > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > New-style drivers call pci_register_driver() during their
> > initialization with a pointer to a structure describing the driver
> > (struct pci_driver):
> 
> Could/would you add all of this to include/linux/pci.h as
> kernel-doc?

Hrm...that's a good idea. I could work on that as time permits
but I've got several other equally important things queued up.

This would also be a good "intern" project if a kernel newbie is
looking for something useful to do. I'd be happy to advise/review
results as needed.

> although that would require that _all_ struct fields be described.
> 
> > 	field name	Description
> > 	----------	------------------------------------------------------
> 
> > 			(Please see Documentation/power/pci.txt for descriptions
> > 			of PCI Power Management and the related functions)
> 
> end with .)

fixed 

> > The ID table is an array of struct pci_device_id ending with an all-zero entry.
>                                                   ^entries ?

fixed

> > Once added, the driver probe routine will be invoked for any unclaimed
> > PCI devices listed in it's (newly updated) pci_ids list.
> *                       its

fixed (*ugh* Jes, can I borrow some brown paper bags from you? ;)

> > 	o If the driver is not a hotplug driver then use only 
> > 	  __init/exit and __initdata/exitdata.
> 
> I'd prefer to see exit, exitdata, etc., spelled with leading __.

I agree. Fixed several of those in than section. I think I got them all.

> 
> >         o Pointers to functions marked as __devexit must be created using
> >           __devexit_p(function_name).  That will generate the function
> >           name or NULL if the __devexit function will be discarded.
> 
> > 3. Device Initialization Steps
> > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > As noted in introduction, most PCI drivers need the following steps
> *            ^the
> 
> > for device initialization:
> 
> > 3.1 Request MMIO/IOP resources
> > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > Memory (MMIO), and I/O port addresses numbers should NOT be read directly
> 
> not both addresses and numbers...

Correct...just addresses.

> > from the PCI device config space. Use the values in the pci_dev structure
> > as the PCI "bus address" might have been remapped to a "host physical"
> > address by the kernel.
> > 
> 
> > 3.2 Enable the PCI device
> > ~~~~~~~~~~~~~~~~~~~~~~~~~
> >    Before touching any device registers, you need to enable the PCI device
> s/you need/the driver needs/
> 
> > by calling pci_enable_device(). This will:
> > 	o wakes up the device if it was in suspended state,
> *         wake
> > 	o enable I/O and memory regions of the device,
> > 	o allocates an IRQ (if BIOS did not),
> *         allocate
> + s/,/./

thanks - all three fixed

> > NOTE: pci_enable_device() can fail! Check the return value.
> > 
> >    pci_set_master() will enable DMA by setting the bus master bit
> > in PCI_COMMAND register. It also fixes the latency timer value if
> *   ^the

fixed

> 
> > it's set to something bogus by the BIOS.
> > 
> 
> > 3.2 Set the DMA mask size
> > ~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> > Similarly, drivers must also "register" this capability if the device
> > can directly address "consistent" in System RAM above 4G physical address.
> * "consistent" <what> ?

ah sorry: consistent memory.

> > Drivers for PCI-X and PCIe compliant devices should also call
> > pci_set_consistent_dma_mask().
> 
> > 3.5 register IRQ handler
> > ~~~~~~~~~~~~~~~~~~~~~~~~
> > While calling request_irq() is the the last step describe here,
> > this is often just another intermediate step to initializing a device.
> > This step can often be deferred until the device is opened for use.
> > 
> > All interrupt handlers should be registered with IRQF_SHARED and use the
> > devid to map IRQs to devices (remember that all PCI interrupts are shared).
> * (excluding MSI)

good catch. I believe this is part of the original text that
predates all the MSI support.

New text now reads:
	All interrupt handlers for IRQ lines should be registered with
	IRQF_SHAREDand use the devid to map IRQs to devices (remember
	that all PCI IRQ linescan be shared).

> 
> > If your PCI device supports both, try to enable MSI-X first.
> > Only one can be enabled at a time.  Many architectures, chipsets,
> > or BIOSs do NOT support MSI or MSI-X and the call to pci_enable_msi/msix
> * BIOSes ?

I don't know. I'll use BIOSes.

> 
> > will fail. This is important to note since many drivers have
> > two (or more) interrupt handlers: one for MSI/MSI-X and another for IRQs.
> > They choose which handler to register with request_irq() based on the
> > return value from pci_enable_msi/msix().
> > 
> > There are (at least) two really good reasons for using MSI:
> > 1) MSI is an exclusive interrupt vector by definition.
> >    This means the interrupt handler doesn't have to verify that
> >    it's device caused the interrupt.
> *    its
> 
> > 4  PCI device shutdown
>   4.

Both fixed - thanks

> > ~~~~~~~~~~~~~~~~~~~~~~
> 
> > 4.1 Stop IRQs on the device
> > ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > How to do this is chip/device specific. If it's not done, it opens
> > the possibility of a "screaming interrupt" if (and only if)
> > the IRQ is shared with another device.
> > 
> > When the shared IRQ handler is "unhoooked", the remaining devices
> > using the same IIRQ still need the IRQ enabled. Thus if the "unhooked"
> 
>  IIRQ ?

IRQ - fixed.

> 
> > device asserts IRQ signal, the system wil respond assuming it was one
> > of the remaining devices asserted the IRQ line. Since none of the other
> > devices will handle the IRQ, the system will "hang" until it decides
> > the IRQ isn't going to get handled and masks the IRQ (100,000 iterations
> > later). Once the shared IRQ is masked, the remaining devices will
> > stop functioning properly. Not a nice situation.
> > 
> 
> > 4.4 release DMA buffers
> > ~~~~~~~~~~~~~~~~~~~~~~~
> > Once DMA is stopped, clean up streaming DMA first.
> > ie. unmap data buffers and return buffers to "upstream"
> 
>   i.e.
> 
> > owners if there is one.
> > 
> > 4.6 Disable device from responding to MMIO/IO Port addresses
> > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > This is just a call to pci_disable_device().
> > This is the symetric opposite of pci_enable_device().
> 
>               symmetric

thanks - I just mispelled it...I will run this through ispell before
submitting it again for review.

> 
> > Do not do anything with the device after calling pci_disable_device().
> 
> HTH.

yes - it did help.

many thanks,
grant