On 17/11/2016 23:24, Måns Rullgård wrote: > Mason <slash.tmp@xxxxxxx> writes: > >> On 17/11/2016 23:01, Måns Rullgård wrote: >> >>> Mason writes: >>> >>>> On 17/11/2016 17:10, Måns Rullgård wrote: >>>> >>>>> Mason writes: >>>>> >>>>>> On 17/11/2016 14:05, Måns Rullgård wrote: >>>>>> >>>>>>> I don't see why the sbox needs to be configured first. It's just a >>>>>>> crossbar switch, and until something starts actually issuing requests, >>>>>>> it shouldn't matter how it is connected. >>>>>> >>>>>> If the NAND controller starts sending to an uninitialized SBOX, >>>>>> then (IIUC) some bits are lost forever (black-holed). >>>>> >>>>> That's expected. Don't do that. >>>>> >>>>>> So "NAND, SBOX, MBUS" cannot work. >>>>>> >>>>>> "SBOX, MBUS, NAND" should work, but it doesn't. >>>>>> >>>>>> "SBOX, NAND, MBUS" works. >>>>>> >>>>>> Perhaps there is a better solution than making a channel exclusive >>>>>> to a client. Do you have a suggestion? >>>>> >>>>> First figure out exactly what the hardware requires. How did the >>>>> hardware designers expect it to be used? >>>> >>>> That is precisely the problem :-( >>>> >>>> I sat down with a HW engineer, and gave him the trace of MMIO >>>> writes issued from the NAND and DMA drivers. >>>> >>>> He ran that in a simulation, and reported that it should work. >>>> The reality is that it does not. Perhaps the actual timing is >>>> important (some kind of race condition). >>> >>> How does it "not work"? >> >> Reading works as expected. Writing fails like this: >> >> The DMA IRQ triggers as expected, but then software is >> supposed to wait for the NAND controller to signal >> completion by polling the idle bit. This bit is never >> set, it remains stuck at 0. >> >> But when I do the SBOX setup at init, and do the NAND >> before the MBUS setup, the bit is set as expected. > > What if you do the NAND controller setup before you start the dma > transaction, i.e. NAND, SBOX, MBUS, for writes and the opposite (SBOX, > MBUS, NAND) for reads? Configuring the receiving end first is usually > safe and makes sense. I found the problem! To recap, the issue was that 10-30% of writes failed (randomly), as described above. If I add udelay(10); at the start of tangox_dma_pchan_detach() then writes no longer fail. The problem seems to be that the switch-box connection is torn down too soon. As far as I can tell, your code tears down the sbox connection as soon as the IRQ arrives, right? However, the interrupt request only signals that all the data in RAM have been pushed to the memory bus; they have not necessarily all reached the device yet. So tearing down the sbox (sometimes) discards the last few bytes in the transfer. I discussed this issue with the resident MBUS/SBOX expert. He says we should tear down the sbox connection only once the device signals it is ready for the next operation. So my request to hook dma_request_chan() was misguided, I would need a different hook/call-back into the DMA driver. Is there a symmetric function to dma_async_issue_pending? Something like dma_async_cleanup_last_transfer? https://www.kernel.org/doc/Documentation/dmaengine/client.txt dmaengine_terminate_async() ? dmaengine_synchronize() ? /** * dmaengine_synchronize() - Synchronize DMA channel termination * @chan: The channel to synchronize * * Synchronizes to the DMA channel termination to the current context. When this * function returns it is guaranteed that all transfers for previously issued * descriptors have stopped and and it is safe to free the memory assoicated * with them. Furthermore it is guaranteed that all complete callback functions * for a previously submitted descriptor have finished running and it is safe to * free resources accessed from within the complete callbacks. * * The behavior of this function is undefined if dma_async_issue_pending() has * been called between dmaengine_terminate_async() and this function. * * This function must only be called from non-atomic context and must not be * called from within a complete callback of a descriptor submitted on the same * channel. */ static inline void dmaengine_synchronize(struct dma_chan *chan) { might_sleep(); if (chan->device->device_synchronize) chan->device->device_synchronize(chan); } Could the sbox tear-down be implemented in chan->device->device_synchronize? Is that the right place for such an operation? Regards. -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
