On Wed, Jun 06, 2007 at 02:52:38PM +0800, tiansm@xxxxxxxxxx wrote:
So I've taken the whole patch set and combined it into just two separate
patches for the -queue tree. I combined them because splitting a
patchset into just new and modified files isn't terribly useful way.
Patches should rather be split in a logic way such as "add suport for
new feature x", "cleanup foobar frobnication state engine" or "fix bug y".
A few comments still:
arch/mips/pci/fixup-lm2e.c:
> +int __init pcibios_map_irq(struct pci_dev *dev, u8 slot, u8 pin)
> +{
> + unsigned int val;
> + if (PCI_SLOT(dev->devfn) == 4) { /* wireless card(notebook) */
> + dev->irq = BONITO_IRQ_BASE + 26;
> + return dev->irq;
> + } else if (PCI_SLOT(dev->devfn) == 5) { /* via686b */
> + switch (PCI_FUNC(dev->devfn)) {
> + case 2:
> + dev->irq = 10;
> + break;
> + case 3:
> + dev->irq = 11;
> + break;
> + case 5:
> + dev->irq = 9;
> + break;
> + }
> + return dev->irq;
> + } else if (PCI_SLOT(dev->devfn) == 6) { /* radeon 7000 */
> + dev->irq = BONITO_IRQ_BASE + 27;
> + return dev->irq;
> + } else if (PCI_SLOT(dev->devfn) == 7) { /* 8139 */
> + dev->irq = BONITO_IRQ_BASE + 26;
> + return dev->irq;
> + } else if (PCI_SLOT(dev->devfn) == 8) { /* nec usb */
> + switch (PCI_FUNC(dev->devfn)) {
> + case 0:
> + dev->irq = BONITO_IRQ_BASE + 26;
> + break;
> + case 1:
> + dev->irq = BONITO_IRQ_BASE + 27;
> + break;
> + case 2:
> + dev->irq = BONITO_IRQ_BASE + 28;
> + break;
> + }
> + pci_read_config_dword(dev, 0xe0, &val);
> + pci_write_config_dword(dev, 0xe0, (val & ~7) | 0x4);
> + pci_write_config_dword(dev, 0xe4, 1 << 5);
> + return dev->irq;
> + } else
> + return 0;
> +}
The purpose of pcibios_map_irq() is to map PCI slot numbers to host system
interrupt numbers. PCI-to-PCI bridge may also need to be taken in
consideration, that's why the function also receives a pin number but a
generic standard compliant PCI device only ever uses INTA.
Things not to do:
o modify the pci_dev structure pointed to by the function's dev argument.
So I changed the first argument to const struct pci_dev * to make that
sort of things a bit harder in the future.
o doing any kind of other initialization. That sort of stuff should go
elsewhere such as into a DECLARE_PCI_FIXUP_* call.
o The generic MIPS PCI code calls pcibios_map_irq() in order when
initializing dev->irq, so you can't refer to that variable because
either it's unset or your changes would be overwritten right away.
o To figure our the interrupt numbers you probably want to look at just
the slot and pin arguments; PCI_{FUNC,SLOT} like any other dereferencing
of dev look suspiciously wrong in this function - pcibios_map_irq
can normally be implemented as just an array lookup, see fixup-malta.c
or for a slightly more complicated example supporting multiple rather
different systems fixup-sni.c.
Can you send a patch to fix this, please? Thanks :-)
> +static void __init loongson2e_686b_func5_fixup(struct pci_dev *pdev)
> +{
> + printk(KERN_INFO"ac97 interrupt = 9\n");
General comment - your kernel patches are fairly talkative - probably a
bit too much.
> +static void __init loongson2e_fixup_pcimap(struct pci_dev *pdev)
[...]
> +DECLARE_PCI_FIXUP_HEADER(PCI_ANY_ID, PCI_ANY_ID, loongson2e_fixup_pcimap);
This one doesn't initialize or fixup any PCI device at all so I think
this shold not be implemented as a PCI fixup. I suggest doing that
before calling register_pci_controller().
> diff --git a/arch/mips/pci/ops-lm2e.c b/arch/mips/pci/ops-lm2e.c
This file could probable be combined with ops-bonito64.c. After all it's
banging the essentially same hardware.
> diff --git a/include/asm-mips/mach-lemote/bonito.h b/include/asm-mips/mach-lemote/bonito.h
> new file mode 100644
> index 0000000..83f7ac3
> --- /dev/null
> +++ b/include/asm-mips/mach-lemote/bonito.h
> @@ -0,0 +1,381 @@
> +/*
> + * Based on Algorithmics header
> + */
And the original (C) header said:
* Bonito Register Map
*
* This file is the original bonito.h from Algorithmics with minor changes
* to fit into linux.
*
* Copyright (c) 1999 Algorithmics Ltd
*
* Carsten Langgaard, carstenl@xxxxxxxx
* Copyright (C) 2001 MIPS Technologies, Inc. All rights reserved.
*
* Algorithmics gives permission for anyone to use and modify this file
* without any obligation or license condition except that you retain
* this copyright message in any source redistribution in whole or part.
So I did you a favor and put the (C) notice back into place.
Anyway, it seems the Fulong is based on a slightly older Bonito64 version
and both Malta and Fulong can be made to share that header file easily.
A patch to resolve those issues would be appreciated. Note that now
that I've put the patches into the -queue tree I'd appreciate a patch
relative to that tree which lives at:
git://git.linux-mips.org/pub/scm/linux-queue.git
If you want to clone this tree and alrady have a copy of the normal
Linux/MIPS git tree on your disk you can speedup the clone by a few orders
of magnitude by using the --reference option like this:
git clone --reference ~/src/linux/linux-mips \
git://git.linux-mips.org/pub/scm/linux-queue.git
It will bring down the clone process to something on the order of a
few seconds.
Ralf
