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

[Daniel Ritz]

On Friday 12 March 2004 21:32, Pavel Roskin wrote:
> 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

looks good.

> [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.

doesn't look so good. fixed. the fallback-to-pci code needs to be outside
of the serial stuff (anyway, to handle the setting for parallel non working
ISA interrupts right). 

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

yeah. done.

> 
> 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.  

yes. i made it also printk() a warning if request_irq() fails.

> 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. 

i do mean it. and i added a comment.

> The second "if (pci)" is almost certainly wrong because
> we only need success.

well, it's only request_irq() that could fail. but it worked before. you are of
course right. fixed.

> 
> "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.)
> 

done.

> 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.

errm....it has a comment on top of the whole block talking about MFUNC3 and
IRQSER. i did the constant thing anyway.

> 
> 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.

leftover when i had the code in ti113x.h. done.
> 
> > 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 :-)
> 

yes, 0 please. also try with INTB/INTRTIE code enabled. test if both slots work.

> 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?

don't trust it, trust the probe.

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

default should be both of the relevant bits set, so we have the nice fallback
as it currently is. what could also be done is:
	if it is initially parallel pci interrupts and they don't work even when
	setting mfunc0, then try the all-serial thing. but i feel not too safe
	about it.

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

don't know...why not?

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

yes! we want CSC interrupts working. yenta expects PCI CSC interrupts.

> I won't even look at IRQTIE issues at this point. 

please. i mean it's important that both slots are working.

> 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.

don't damage your hardware it's not worth it :)

> 
> -- 
> Regards,
> Pavel Roskin
> 

new patch:
http://ritz.dnsalias.org/linux/pcmcia-ti-routing-6.patch

rgds
-daniel