Re: changes to OHCI code...

[Sebastien R. <Seb...@sy...>]

Hi James,

Thanks a lot for your remarks and patches. You should really cross-post
to the linux1394-devel list as well, some people might be interested.
I comment your comments below:

>>>>> "James" == James Goodwin <jamesg@Filanet.com> writes:

James> One general issue I have for you (which I have no good fix for yet) is
James> the bh handling of received async packets. Sometimes packets are missed
James> on receive (the bh is not run). I think we need to loop in our bh to
James> handle all async packets received, instead of only trying to handle one
James> received packet in each bh run. My temp hack workaround for my sbp2
James> stuff right now is to run the dma_rcv_bh directly from the interrupt
James> handler (but this is bad!).

I have also noticed that sometimes the bottom halves are queued and not
executed... for the moment I am in the dark. As for looping in the bh,
it does that already... basically it reads the buffer until it is empty.
Yes calling the bh directly can be a temporary fix... I could do that,
but I am afraid it could be bad for some slow computer...

James> 	// // We need to add delays after the soft reset, setting LPS, and
James> enabling our link. This fixes the self-id // reception problem if we
James> initialize with no devices attached (at least for my board). Probably
James> don't need the // delay after the soft reset, since are polling for
James> reset completion status, but the other two places need a // delay. Yes,
James> delays like these are evil, but at least they only happen at driver
James> initialization!  //

I have added delays in the initialization loop. But I am not sure about
the delay after enabling the link... Since enabling the link triggers a
bus reset, I have moved it at the end of the initialization routine, and
I think the card should be ready immediatly after that to handle the
bus reset... What do you think ?

James> =================== In ohci_irq_handler:

James> 	// // In many cases we will receive a selfidcomplete interrupt while
James> the bus is still bouncing around a bit, // especially due to hot-plugs
James> of devices. One way to deal with this is to spin until the node id
James> becomes // valid. Not the nicest thing to do in the world, and we
James> should time-out if we never get a valid nodeid.  // But, it quickly
James> deals with the case where we hot-plug a new device in and the bus
James> bounces around a // bit.  //

If you are looping in the irq_handler until the node id is valid, isn't
there any chance to miss self-id packets (or can the loop be a bit long
for an irq handler ?).

James> =================== In handle_selfid:

James> 	// // In many cases with current OHCI hardware, we get the selfIDError
James> bit set on hot-plugs of devices. Quick fix // for this is to generate
James> another bus reset in response to seeing this (which clears things
James> up). This is what // Microsoft's OHCI implementation does as well.  //

Ok. I have added the fix with a timeout counter.

James> =================== In alloc_dma_rcv_ctx and alloc_dma_trm_ctx:

d-> task.sync = 0;
d-> task.next = NULL;

It is always good to initialize all the fields... 

James> =================== In ohci_irq_handler:

James> // Turn on phys dma reception. We
James> should probably manage the filtering somehow, instead // of blindly
James> turning it on.  
James>       reg_write(ohci,OHCI1394_PhyReqFilterHiSet, 0xffffffff); 
James>       reg_write(ohci,OHCI1394_PhyReqFilterLoSet, 0xffffffff);
James>       reg_write(ohci,OHCI1394_PhyUpperBound, 0xffff0000);

Ok. Fix included.

James> =================== In dma_rcv_bh:

James> We need to handle write requests that are
James> received to our middle address space (posted writes).  // In this case,
James> the hardware generates an ack_complete... but, if we pass the packet up
James> to ieee1394, // it will try and send a response (which it shouldn't),
James> because it assumes we returned ack_pending.

James>                                 		// // Handle case of posted
James> writes. If we receive an ack_complete, // we should not send a
James> response. Fake out upper layers by turning the // packet into a
James> broadcast packet... we should really modify the core stack // to accept
James> an ack received argument and figure out whether to reply.  
James> if (((d->spb[length/4-1]>>16)&0x1f) == 0x11) {
                d-> spb[0] |= (ALL_NODES<<16);
James  }

Yes we should see that with Andreas. I also would like the subsystem to
handle the single phy packet that is received once the bus reset is
completed and that indicate the precise moment in the FIFO after which
the new bus topology is in operation.

-- 
Sebastien Rougeaux
RSISE, The Australian National University