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

[Felipe Balbi]

On Thu, Feb 02, 2012 at 03:12:44PM +0400, Sergei Shtylyov wrote:
> Hello.
> 
> On 02-02-2012 13:09, Felipe Balbi 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 ?
> 
>    On DaVinci/OMAP-L1x these 3 calls need to extract data from a
> single 32-bit register, so that doesn't seem a good idea to me. The

that's a problem on DaVinci/OMAP-L1x.

> current scheme seems OK to me. Or either implement a signle function
> to read all 3 interrupt masks...
> 
> musb_platform_read_ints()

I wanted to avoid glue layer having to access the musb pointer directly.
If I implement musb_platform_read_ints() I will have to pass musb as
argument, or agree on another way to return the values. Thanks, but no
thanks.

I want to remove direct access of musb from glue layer, and at some
point I will have to do it in order to fix a few problems we might still
have with modules, basically because glue layer could be trying to
access memory which was freed already.

-- 
balbi

Attachment: signature.asc
Description: Digital signature