Re: RFC: usb: musb: Changes proposed for adding CPPI4.1 DMA

[Felipe Balbi]

On Thu, Feb 02, 2012 at 04:57:16AM +0000, Gupta, Ajay Kumar wrote:
> Hi,
> > On 31-01-2012 8:41, Gupta, Ajay Kumar wrote:
> > 
> > >>> As a next step to dma-engine based cppi4.1 driver implementation
> > >>> this RFC has the overview of changes in the musb driver.
> > >>> RFC on CPPI slave driver changes will follow next.
> > 
> > >>> Overview of changes in the musb driver
> > >>> ======================================
> > 
> > >>> 1)Add a dma-engine.c file in the drivers/usb/musb folder
> > >>> 2)This file will host the current musb dma APIs and translates them
> > to
> > >>>     dmaengine APIs.
> > >>> 3)This will help to keep the changes in drivers/usb/musb/musb* files
> > >>>     minimal and also to retain compatibility other DMA (Mentor etc.)
> > >>>     drivers which are yet to be moved to drivers/dma
> > >>> 4)drivers/usb/musb/dma-engine.c, will wrap the dmaengine APIs to
> > >>>     make existing musb APIs compatible.
> > >>> 5)drivers/usb/musb/dma-engine.c file will implement the filter
> > >>>     functions and also implement .dma_controller_create (allocates
> > >>>     &   provides "dma_controller" object) and .dma_controller_delete
> > >>> 6)CPPI4.1 DMA specific queue and buffer management will be internal
> > >>>     to slave CPPI DMA driver implementation.
> > 
> > >>      You mean drivers/dma/ driver?
> > 
> > > yes.
> > 
> > >> I think you are forgotting that CPPI 4.1 MUSB
> > >> has some registers controlling DMA/interrupts beside those of CPPI
> > 4.1
> > >> controller and MUSB core itself. How do they fit in your scheme?
> > 
> > > We have been discussing on how to handle these in slave driver and
> > 
> >     These certainly cannot be handled in the slave driver because the
> > registers are different for every controller implementation and, the
> > main thing, they don't belong to CPPI 4.1 as such.
> 
> Felipe suggested to use device tree for differences in register maps
> among different platforms.
> 
> I do see issues in reading wrapper interrupt status register and then
> calling musb_interrupt() [defined inside musb_core.c] from slave driver.

I have been thinking about that lately. In the end of the day, I want to
remove direct dependencies between musb_core and glue. So what I was
thinking about goes like so:

Glue layer basically has to prepare musb->int_usb, musb->int_tx and
musb->int_rx for musb. Maybe handle some glue specific stuff and so on,
but the IRQ line still belongs to MUSB.

So the idea would be to add something like:

musb_platform_read_intrusb()
musb_platform_read_intrtx()
musb_platform_read_intrrx()

those would default to basic:

musb_readb(musb->mregs, MUSB_INTRUSB);
musb_readw(musb->mregs, MUSB_INTRTX);
musb_readw(musb->mregs, MUSB_INTRRX);

if platform ops aren't passed. So, it would look something like:

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index 72a424d..ba0bcc2 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -1488,9 +1488,9 @@ static irqreturn_t generic_interrupt(int irq, void *__hci)
 
 	spin_lock_irqsave(&musb->lock, flags);
 
-	musb->int_usb = musb_readb(musb->mregs, MUSB_INTRUSB);
-	musb->int_tx = musb_readw(musb->mregs, MUSB_INTRTX);
-	musb->int_rx = musb_readw(musb->mregs, MUSB_INTRRX);
+	musb->int_usb = musb_platform_read_intusb(musb->controller);
+	musb->int_tx = musb_platform_read_inttx(musb->controller);
+	musb->int_rx = musb_platform_read_intrx(musb->controller);
 
 	if (musb->int_usb || musb->int_tx || musb->int_rx)
 		retval = musb_interrupt(musb);

those would make sure to prepare the cached IRQ status registers for
MUSB core.

Keep in mind that this is only necessary because on
DaVinci/OMAP-L13x/AM35x devices you guys have decided to make the
wrapper read the IRQ status register from MUSB address space. And
because those are clear-on-read, we're screwed.

Oh well, this is the best I could come up with. Any problems you guys
see ?

-- 
balbi

Attachment: signature.asc
Description: Digital signature