Thread: Re: changes to OHCI code...
Brought to you by:
aeb,
bencollins
From: Sebastien R. <Seb...@sy...> - 2000-04-22 08:27:34
|
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 |
From: James G. <jamesg@Filanet.com> - 2000-04-25 05:45:15
|
Hi Sebastien, Thanks for the response! When I get a free moment, will further investigate some of these issues. Comments below. Cheers! --James >-----Original Message----- >From: Sebastien Rougeaux [mailto:Seb...@sy...] >Sent: Saturday, April 22, 2000 1:21 AM >To: jamesg@Filanet.com >Cc: lin...@li... >Subject: Re: changes to OHCI code... > >>>>>> "James" == James Goodwin <jamesg@Filanet.com> writes: > >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 ? I'm not sure that enabling the link should cause a bus reset. Hopefully not... since higher levels call reset_host_bus right after init (would result in two quick bus resets). Either way, since hardware interrupts are not enabled until the very end of ohci_initialize it should be safe. =) > >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 ?). I think it is a little ugly to loop in the irq_handler until the node id is valid, since it can potentially be many ms in the isr. Would probably be better to move all the self-id code, as well as the majority of the rest of the irq_handler code to a bh. But, this change to loop on node id valid has caused no problems in my testing (hot-plugging with up to eight devices attached). Also, we shouldn't worry about losing self-ids packets. If we wait until the node id is valid, we will always get the last set of self-ids (which has been dma-ed into the self-id buffer)... the last set is all we really care about anyway (who cares about sets of stale self-ids). Again, I've done a decent bit of hot-plug testing with this change... > >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. Yes, I think if we can report up to the higher level driver an "ack sent" argument, it can determine whether it needs to send a response packet. > >-- >Sebastien Rougeaux >RSISE, The Australian National University |