[PATCH] yenta: irq-routing for TI bridges...again

[Pavel Roskin]

On Thu, 11 Mar 2004, Daniel Ritz wrote:

> > Kernel addresses should not be printed unless you expect the
> > subsequent stack dump and panic.  There are better ideas for the prefix,
>
> i know. it was more for debugging where more the uniqueness was relevant.
> the address is the easiest thing....

OK, the patch is much better now.  For now, it's the results for TI
PCI1410.

[root at lark root]# modprobe yenta_socket
Linux Kernel Card Services
  options:  [pci] [cardbus] [pm]
Yenta: CardBus bridge found at 0000:01:01.0 [0000:0000]
Yenta: Enabling burst memory read transactions
Yenta: Using CSCINT to route CSC interrupts to PCI
Yenta: Routing CardBus interrupts to PCI
Yenta TI: socket 0000:01:01.0, mfunc 0x00000000, devctl 0x66
Yenta TI: socket 0000:01:01.0 probing PCI interrupt failed, trying to fix
Yenta TI: socket 0000:01:01.0 falling back to parallel PCI interrupts
Yenta TI: socket 0000:01:01.0 parallel PCI interrupts ok
Yenta TI: socket 0000:01:01.0 probing ISA interrupts failed, trying to fix
Yenta TI: socket 0000:01:01.0 routing card interrupts to PCI
Yenta: ISA IRQ mask 0x0000, PCI irq 12
Socket status: 30000006
[root at lark root]# rmmod yenta_socket
[root at lark root]# modprobe yenta_socket
Yenta: CardBus bridge found at 0000:01:01.0 [0000:0000]
Yenta: Using CSCINT to route CSC interrupts to PCI
Yenta: Routing CardBus interrupts to PCI
Yenta TI: socket 0000:01:01.0, mfunc 0x00000002, devctl 0x60
Yenta TI: socket 0000:01:01.0 probing ISA interrupts failed, trying to fix
Yenta: ISA IRQ mask 0x0000, PCI irq 12
Socket status: 30000006
[root at lark root]#

I'm a bit concerned that the driver is "trying to fix" something the
second time when PCI interrupts should be working already.  See below.

Please don't call a variable "pci" - it's very unclear.  A longer name
like "pci_irq_status" would be better.

It's unclear what yenta_probe_cb_irq() returns.  I understand it would
return -1 for fatal failures, 0 for no interrupt and 1 if the PCI
interrupts work.  Then "if (pci)" would treat failures and successful
probes in the same way, which would need at least a comment, even if you
really mean it.  The second "if (pci)" is almost certainly wrong because
we only need success.

"switch" and "case" should have the same indentation.  (Sorry for picking
at your coding style again.  I would gladly avoid it if I wasn't afraid
that it might reduce chances of your code being applied to the kernel.)

Code like "mfunc = (mfunc & ~0xf000) | 0x1000" should have a comment
explaining what it does.  Ideally, numbers should be replaced with named
constants.  Only after reading the spec I realized that it sets MFUNC3 to
IRQSER.

It would be great if you add another member to "struct yenta_socket"
instead of using private[7].  The "private" array is meant for individual
socket types, while private[7] is only used in yenta_socket.c.

> attached the new patch. if you have other cleanups in mind: please as patch
> ontop as i'm lazy :)
>
> > what can be done better, then post it.  I'll be glad to test it on more
> > systems.
>
> thanx. you don't have a non-working dual-slot chip around, have you?
> i'd like to test if the INTB/INTRTIE stuff is working/necessary..

I have a working dual-slot card around (TI PCI1221), but I can make it
non-working if I write 0 or random data to mfunc and devctl :-)

And now we come to the need to define the scope of the problems we want to
address.

1) Do we want to deal with mfunc being random at startup or we only want
to deal with 0 and/or certain specific values?  Can we just ignore the
original value if we determine that it's incorrect?

2) Same question about INTMODE bits in devctl - do we want to deal with
all possible incorrect settings or only some of them?

3) Do we want to try to enable ISA interrupts if PCI interrupts are
working and ISA interrupts are not?

4) Do we want to try to enable PCI interrupts if ISA interrupts are
working and PCI interrupts are not?

I won't even look at IRQTIE issues at this point.  We need to define how
ambitious we are, and write code with clear understanding of what can and
what cannot be trusted.

I only have PCI cards with TI chipset, so my ability to test ISA
interrupts is very limited.  I may try connect the chip to an ISA slot,
but my experience with soldering is much less than with programming.

-- 
Regards,
Pavel Roskin