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

[Sergei Shtylyov <sshtylyov@mvista.com>]

Hello.

On 02-02-2012 15:49, 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.

    We can do:

void musb_platform_read_ints(u8 *usb, u8 *tx, u8 *tx);

    That's what I thought first about but then got lazy. :-)

WBR, Sergei