linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* RFC: usb: musb: Changes proposed for adding CPPI4.1 DMA
@ 2012-01-25 15:22 Gupta, Ajay Kumar
  2012-01-26  9:02 ` Felipe Balbi
  2012-01-27 16:09 ` Sergei Shtylyov
  0 siblings, 2 replies; 14+ messages in thread
From: Gupta, Ajay Kumar @ 2012-01-25 15:22 UTC (permalink / raw)
  To: linux-usb; +Cc: linux-kernel, Pasupathy, Visuvanadan, Balbi, Felipe

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.
 
 
Brief on each DMA related API used in /drivers/usb/musb*
========================================================= 
1)
MUSB DMA API currently used: dma_controller_create()          
MUSB DMA API parameters currently used: 
		struct musb  *musb, 
		void __iomem *mregs

Proposal: This API will be implemented in the dma-engine.c and will
         allocate and populate dma_controller object.										   

2)
MUSB DMA API currently used: dma_controller_delete() 
MUSB DMA API parameters currently used: 
		struct dma_controller *controller

Proposal: This API will be implemented in the dma-engine.c and frees
         the dma_controller object

3)
MUSB DMA API currently used: dma_controller.start()
MUSB DMA API parameters currently used: 
		struct dma_controller *controller

Proposal: This will be an empty function. The current actions intended
          for start of HW DMA (as whole engine - not the specific channel)
	    will be implemented in cppi41 slave driver.

4)
MUSB DMA API currently used: dma_controller.stop()
MUSB DMA API parameters currently used: 
		struct dma_controller *controller

Proposal: This will be an empty function. The current actions intended
          for stop of HW DMA (as whole engine - not the specific channel)
	    will be implemented in cppi41 slave driver.

5)
MUSB DMA API currently used: dma_controller.chan_alloc()       
MUSB DMA API parameters currently used: 
				struct dma_controller *,	
				struct musb_hw_ep *,		  
				u8 is_tx                
Proposal								
* This function translates to the dma_request_channel API of dma-engine.
* The filter function that helps to acquire the channel is also part of
  this implementation.
* The dma_chan structure returned by dma-engine API is going to be 
  different from "dma_channel" structure. As the channel structure 
  does not carry any important information except status and
  associating DMA-HW channel structure, dma engine.c could still
  translate/emulate the similar (almost same) structure to musb* files.
* The endpoint and direction information is used in filter function.
* A challenge here is to implement a filter function that scales up 
  for more number of channels (64 channels at this point)
* Another challenge is the maintain the platform data on endpoints vs 
  channels (which change between SoCs)
* DMA engine APIs used for this purpose:
		Alloc function:
		Return type : struct dma_chan *
		Name : dma_request_channel()
		Arguments : dma_cap_mask_t *mask
		dma_filter_fn fn
		void *fn_param

		Filter function
		Return type : bool
		Name : *dma_filter_function
		Argument : struct dma_chan *chan 


6)		
MUSB DMA API currently used: dma_controller.chan_program() 
MUSB DMA API parameters currently used: 
				struct dma_channel * 
				u16 maxpacket
				u8 mode, 
				dma_addr_t dma_addr,
				u32 length  

Proposal:
* All the parameters except the maxpacket directly applies in dma-engine API							
* Max packet is used for Transparent (mode0) mode of DMA where, each
  burst of DMA programming will be of maxpacket size.
* For all generic DMA requests - SG structure of DMA engine API will 
  have only one entry
* DMA driver would require the "maxpacket" size, for deciding type of 
  DMA transfer. As the current API does not provide option, it can be 
  part of a private data to slave DMA driver through dma_chan structure.
  Alternatively, dmaengine's "DMA_SLAVE_CONFIG" control command also
  can be used for this purpose
* For ISO requests, each frame buffer is treated as an entry of SG structure.
  ISO programming will require some changes in musb_host.c as it currently 
  programs each frame buffer as a separate DMA request.
 
 
7)												
MUSB DMA API currently used: dma_controller.chan_release()
MUSB DMA API parameters currently used: 
		struct dma_channel	*channel

Proposal: Releases the channel - typically happens only during the rmmod 
        of the driver

8)
MUSB DMA API currently used: dma_controller.chan_abort()
MUSB DMA API parameters currently used: 
		struct dma_channel	*channel

Proposal: This translates into the control commands of DMA engine. We can use 
          "DMA_TERMINATE_ALL" control command for this purpose.


Regards,
Ajay


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: RFC: usb: musb: Changes proposed for adding CPPI4.1 DMA
  2012-01-25 15:22 RFC: usb: musb: Changes proposed for adding CPPI4.1 DMA Gupta, Ajay Kumar
@ 2012-01-26  9:02 ` Felipe Balbi
  2012-01-27 15:01   ` Gupta, Ajay Kumar
  2012-01-27 16:09 ` Sergei Shtylyov
  1 sibling, 1 reply; 14+ messages in thread
From: Felipe Balbi @ 2012-01-26  9:02 UTC (permalink / raw)
  To: Gupta, Ajay Kumar
  Cc: linux-usb, linux-kernel, Pasupathy, Visuvanadan, Balbi, Felipe

[-- Attachment #1: Type: text/plain, Size: 5723 bytes --]

Hi,

(please format your emails better next time)

On Wed, Jan 25, 2012 at 03:22:32PM +0000, 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.

looks good.

> Brief on each DMA related API used in /drivers/usb/musb*
> ========================================================= 
> 1)
> MUSB DMA API currently used: dma_controller_create()          
> MUSB DMA API parameters currently used: 
> 		struct musb  *musb, 
> 		void __iomem *mregs
> 
> Proposal: This API will be implemented in the dma-engine.c and will
> allocate and populate dma_controller object.

k

> 2)
> MUSB DMA API currently used: dma_controller_delete() 
> MUSB DMA API parameters currently used: 
> 		struct dma_controller *controller
> 
> Proposal: This API will be implemented in the dma-engine.c and frees
>          the dma_controller object

k

> 3)
> MUSB DMA API currently used: dma_controller.start()
> MUSB DMA API parameters currently used: 
> 		struct dma_controller *controller
> 
> Proposal: This will be an empty function. The current actions intended
>           for start of HW DMA (as whole engine - not the specific channel)
>	    will be implemented in cppi41 slave driver.

why don' you make this one actually enable the hw ? Maybe call
pm_runtime_get_sync(), enable the DMA, increase a usecount on the
controller and stuff like that ?

> 4)
> MUSB DMA API currently used: dma_controller.stop()
> MUSB DMA API parameters currently used: 
> 		struct dma_controller *controller
> 
> Proposal: This will be an empty function. The current actions intended
>           for stop of HW DMA (as whole engine - not the specific channel)
>	    will be implemented in cppi41 slave driver.

likewise, but reversing start ?

> 5)
> MUSB DMA API currently used: dma_controller.chan_alloc()       
> MUSB DMA API parameters currently used: 
> 				struct dma_controller *,	
> 				struct musb_hw_ep *,		  
> 				u8 is_tx                
> Proposal
> * This function translates to the dma_request_channel API of dma-engine.
> * The filter function that helps to acquire the channel is also part of
>   this implementation.
> * The dma_chan structure returned by dma-engine API is going to be 
>   different from "dma_channel" structure. As the channel structure 
>   does not carry any important information except status and
>   associating DMA-HW channel structure, dma engine.c could still
>   translate/emulate the similar (almost same) structure to musb* files.
> * The endpoint and direction information is used in filter function.
> * A challenge here is to implement a filter function that scales up 
>   for more number of channels (64 channels at this point)

to start, make it as simple as necessary. Implement other trickery
later.

> * Another challenge is the maintain the platform data on endpoints vs 
>   channels (which change between SoCs)

We need to move out of platform_data. While doing that, make it match on
DT attributes.

> 6)		
> MUSB DMA API currently used: dma_controller.chan_program() 
> MUSB DMA API parameters currently used: 
> 				struct dma_channel * 
> 				u16 maxpacket
> 				u8 mode, 
> 				dma_addr_t dma_addr,
> 				u32 length  
> 
> Proposal:
> * All the parameters except the maxpacket directly applies in dma-engine API
> * Max packet is used for Transparent (mode0) mode of DMA where, each
>   burst of DMA programming will be of maxpacket size.
> * For all generic DMA requests - SG structure of DMA engine API will 
>   have only one entry
> * DMA driver would require the "maxpacket" size, for deciding type of 
>   DMA transfer. As the current API does not provide option, it can be 
>   part of a private data to slave DMA driver through dma_chan structure.
>   Alternatively, dmaengine's "DMA_SLAVE_CONFIG" control command also
>   can be used for this purpose
> * For ISO requests, each frame buffer is treated as an entry of SG structure.
>   ISO programming will require some changes in musb_host.c as it currently 
>   programs each frame buffer as a separate DMA request.

k

> 7)
> MUSB DMA API currently used: dma_controller.chan_release()
> MUSB DMA API parameters currently used: 
> 		struct dma_channel	*channel
> 
> Proposal: Releases the channel - typically happens only during the rmmod 
>         of the driver

k

> 8)
> MUSB DMA API currently used: dma_controller.chan_abort()
> MUSB DMA API parameters currently used: 
> 		struct dma_channel	*channel
> 
> Proposal: This translates into the control commands of DMA engine. We can use 
>           "DMA_TERMINATE_ALL" control command for this purpose.

k

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: RFC: usb: musb: Changes proposed for adding CPPI4.1 DMA
  2012-01-26  9:02 ` Felipe Balbi
@ 2012-01-27 15:01   ` Gupta, Ajay Kumar
  0 siblings, 0 replies; 14+ messages in thread
From: Gupta, Ajay Kumar @ 2012-01-27 15:01 UTC (permalink / raw)
  To: Balbi, Felipe; +Cc: linux-usb, linux-kernel, Pasupathy, Visuvanadan

Hi,
> On Wed, Jan 25, 2012 at 03:22:32PM +0000, 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.
> 
> looks good.
> 
> > Brief on each DMA related API used in /drivers/usb/musb*
> > =========================================================
> > 1)
> > MUSB DMA API currently used: dma_controller_create()
> > MUSB DMA API parameters currently used:
> > 		struct musb  *musb,
> > 		void __iomem *mregs
> >
> > Proposal: This API will be implemented in the dma-engine.c and will
> > allocate and populate dma_controller object.
> 
> k
> 
> > 2)
> > MUSB DMA API currently used: dma_controller_delete() MUSB DMA API
> > parameters currently used:
> > 		struct dma_controller *controller
> >
> > Proposal: This API will be implemented in the dma-engine.c and frees
> >          the dma_controller object
> 
> k
> 
> > 3)
> > MUSB DMA API currently used: dma_controller.start() MUSB DMA API
> > parameters currently used:
> > 		struct dma_controller *controller
> >
> > Proposal: This will be an empty function. The current actions intended
> >           for start of HW DMA (as whole engine - not the specific
> channel)
> >	    will be implemented in cppi41 slave driver.
> 
> why don' you make this one actually enable the hw ? Maybe call
> pm_runtime_get_sync(), enable the DMA, increase a usecount on the
> controller and stuff like that ?

Sounds good, will look into this.
> 
> > 4)
> > MUSB DMA API currently used: dma_controller.stop() MUSB DMA API
> > parameters currently used:
> > 		struct dma_controller *controller
> >
> > Proposal: This will be an empty function. The current actions intended
> >           for stop of HW DMA (as whole engine - not the specific
> channel)
> >	    will be implemented in cppi41 slave driver.
> 
> likewise, but reversing start ?
> 
> > 5)
> > MUSB DMA API currently used: dma_controller.chan_alloc()
> > MUSB DMA API parameters currently used:
> > 				struct dma_controller *,
> > 				struct musb_hw_ep *,
> > 				u8 is_tx
> > Proposal
> > * This function translates to the dma_request_channel API of dma-
> engine.
> > * The filter function that helps to acquire the channel is also part
> of
> >   this implementation.
> > * The dma_chan structure returned by dma-engine API is going to be
> >   different from "dma_channel" structure. As the channel structure
> >   does not carry any important information except status and
> >   associating DMA-HW channel structure, dma engine.c could still
> >   translate/emulate the similar (almost same) structure to musb*
> files.
> > * The endpoint and direction information is used in filter function.
> > * A challenge here is to implement a filter function that scales up
> >   for more number of channels (64 channels at this point)
> 
> to start, make it as simple as necessary. Implement other trickery
> later.

Yes, ofcourse.
> 
> > * Another challenge is the maintain the platform data on endpoints vs
> >   channels (which change between SoCs)
> 
> We need to move out of platform_data. While doing that, make it match on
> DT attributes.

Ok fine.

Regards,
Ajay

> 
> > 6)
> > MUSB DMA API currently used: dma_controller.chan_program() MUSB DMA
> > API parameters currently used:
> > 				struct dma_channel *
> > 				u16 maxpacket
> > 				u8 mode,
> > 				dma_addr_t dma_addr,
> > 				u32 length
> >
> > Proposal:
> > * All the parameters except the maxpacket directly applies in
> > dma-engine API
> > * Max packet is used for Transparent (mode0) mode of DMA where, each
> >   burst of DMA programming will be of maxpacket size.
> > * For all generic DMA requests - SG structure of DMA engine API will
> >   have only one entry
> > * DMA driver would require the "maxpacket" size, for deciding type of
> >   DMA transfer. As the current API does not provide option, it can be
> >   part of a private data to slave DMA driver through dma_chan
> structure.
> >   Alternatively, dmaengine's "DMA_SLAVE_CONFIG" control command also
> >   can be used for this purpose
> > * For ISO requests, each frame buffer is treated as an entry of SG
> structure.
> >   ISO programming will require some changes in musb_host.c as it
> currently
> >   programs each frame buffer as a separate DMA request.
> 
> k
> 
> > 7)
> > MUSB DMA API currently used: dma_controller.chan_release() MUSB DMA
> > API parameters currently used:
> > 		struct dma_channel	*channel
> >
> > Proposal: Releases the channel - typically happens only during the
> rmmod
> >         of the driver
> 
> k
> 
> > 8)
> > MUSB DMA API currently used: dma_controller.chan_abort() MUSB DMA API
> > parameters currently used:
> > 		struct dma_channel	*channel
> >
> > Proposal: This translates into the control commands of DMA engine. We
> can use
> >           "DMA_TERMINATE_ALL" control command for this purpose.
> 
> k
> 
> --
> balbi

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: RFC: usb: musb: Changes proposed for adding CPPI4.1 DMA
  2012-01-25 15:22 RFC: usb: musb: Changes proposed for adding CPPI4.1 DMA Gupta, Ajay Kumar
  2012-01-26  9:02 ` Felipe Balbi
@ 2012-01-27 16:09 ` Sergei Shtylyov
  2012-01-31  4:41   ` Gupta, Ajay Kumar
  1 sibling, 1 reply; 14+ messages in thread
From: Sergei Shtylyov @ 2012-01-27 16:09 UTC (permalink / raw)
  To: Gupta, Ajay Kumar
  Cc: linux-usb, linux-kernel, Pasupathy, Visuvanadan, Balbi, Felipe

Hello.

On 01/25/2012 06:22 PM, 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? 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?

WBR, Sergei

^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: RFC: usb: musb: Changes proposed for adding CPPI4.1 DMA
  2012-01-27 16:09 ` Sergei Shtylyov
@ 2012-01-31  4:41   ` Gupta, Ajay Kumar
  2012-01-31 11:27     ` Sergei Shtylyov
  0 siblings, 1 reply; 14+ messages in thread
From: Gupta, Ajay Kumar @ 2012-01-31  4:41 UTC (permalink / raw)
  To: Sergei Shtylyov
  Cc: linux-usb, linux-kernel, Pasupathy, Visuvanadan, Balbi, Felipe

Hi Sergei,
> On 01/25/2012 06:22 PM, 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
would post our proposal in RFC for slave driver design. Do you have
any proposal?

Regards,
Ajay
> 
> WBR, Sergei

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: RFC: usb: musb: Changes proposed for adding CPPI4.1 DMA
  2012-01-31  4:41   ` Gupta, Ajay Kumar
@ 2012-01-31 11:27     ` Sergei Shtylyov
  2012-02-02  4:57       ` Gupta, Ajay Kumar
  0 siblings, 1 reply; 14+ messages in thread
From: Sergei Shtylyov @ 2012-01-31 11:27 UTC (permalink / raw)
  To: Gupta, Ajay Kumar
  Cc: linux-usb, linux-kernel, Pasupathy, Visuvanadan, Balbi, Felipe

Hello.

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.

> would post our proposal in RFC for slave driver design. Do you have
> any proposal?

    I think this will need hooks from dma-engine.c to the glue layers. I was 
going to implement such in my version of MUSB CPPI 4.1 driver (in order to 
also support AM35x) but lacked time.

> Regards,
> Ajay

WBR, Sergei

^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: RFC: usb: musb: Changes proposed for adding CPPI4.1 DMA
  2012-01-31 11:27     ` Sergei Shtylyov
@ 2012-02-02  4:57       ` Gupta, Ajay Kumar
  2012-02-02  9:09         ` Felipe Balbi
  2012-02-02 11:05         ` Sergei Shtylyov
  0 siblings, 2 replies; 14+ messages in thread
From: Gupta, Ajay Kumar @ 2012-02-02  4:57 UTC (permalink / raw)
  To: Sergei Shtylyov
  Cc: linux-usb, linux-kernel, Pasupathy, Visuvanadan, Balbi, Felipe

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.

> 
> > would post our proposal in RFC for slave driver design. Do you have
> > any proposal?
> 
>     I think this will need hooks from dma-engine.c to the glue layers. I
> was going to implement such in my version of MUSB CPPI 4.1 driver (in order
> to also support AM35x) but lacked time.

That would mean a change in current drivers/dma API.
 
Ajay
> 
> > Regards,
> > Ajay
> 
> WBR, Sergei

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: RFC: usb: musb: Changes proposed for adding CPPI4.1 DMA
  2012-02-02  4:57       ` Gupta, Ajay Kumar
@ 2012-02-02  9:09         ` Felipe Balbi
  2012-02-02 11:12           ` Sergei Shtylyov
  2012-02-02 11:05         ` Sergei Shtylyov
  1 sibling, 1 reply; 14+ messages in thread
From: Felipe Balbi @ 2012-02-02  9:09 UTC (permalink / raw)
  To: Gupta, Ajay Kumar
  Cc: Sergei Shtylyov, linux-usb, linux-kernel, Pasupathy, Visuvanadan,
	Balbi, Felipe

[-- Attachment #1: Type: text/plain, Size: 4045 bytes --]

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 #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: RFC: usb: musb: Changes proposed for adding CPPI4.1 DMA
  2012-02-02  4:57       ` Gupta, Ajay Kumar
  2012-02-02  9:09         ` Felipe Balbi
@ 2012-02-02 11:05         ` Sergei Shtylyov
  2012-02-03  8:42           ` Gupta, Ajay Kumar
  1 sibling, 1 reply; 14+ messages in thread
From: Sergei Shtylyov @ 2012-02-02 11:05 UTC (permalink / raw)
  To: Gupta, Ajay Kumar
  Cc: linux-usb, linux-kernel, Pasupathy, Visuvanadan, Balbi, Felipe

Hello.

On 02-02-2012 8:57, 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 don't see how the device tree would magically help here.

> I do see issues in reading wrapper interrupt status register and then
> calling musb_interrupt() [defined inside musb_core.c] from slave driver.

>>> would post our proposal in RFC for slave driver design. Do you have
>>> any proposal?

>>      I think this will need hooks from dma-engine.c to the glue layers. I
>> was going to implement such in my version of MUSB CPPI 4.1 driver (in order
>> to also support AM35x) but lacked time.

> That would mean a change in current drivers/dma API.

    No, it doesn't. I'm proposing hooks from dma-engine.c which is situated in 
drivers/usb/musb/ if I got you right. It just means that the calls to 
dma-engine.c as MUSB DMA driver won't map to the calls to the DMA slave driver 
as directly as you've presented before.

> Ajay

>>> Regards,
>>> Ajay

WBR, Sergei

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: RFC: usb: musb: Changes proposed for adding CPPI4.1 DMA
  2012-02-02  9:09         ` Felipe Balbi
@ 2012-02-02 11:12           ` Sergei Shtylyov
  2012-02-02 11:49             ` Felipe Balbi
  0 siblings, 1 reply; 14+ messages in thread
From: Sergei Shtylyov @ 2012-02-02 11:12 UTC (permalink / raw)
  To: balbi; +Cc: Gupta, Ajay Kumar, linux-usb, linux-kernel, Pasupathy, Visuvanadan

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 current scheme 
seems OK to me. Or either implement a signle function to read all 3 interrupt 
masks...

musb_platform_read_ints()

WBR, Sergei

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: RFC: usb: musb: Changes proposed for adding CPPI4.1 DMA
  2012-02-02 11:12           ` Sergei Shtylyov
@ 2012-02-02 11:49             ` Felipe Balbi
  2012-02-02 12:02               ` Sergei Shtylyov
  0 siblings, 1 reply; 14+ messages in thread
From: Felipe Balbi @ 2012-02-02 11:49 UTC (permalink / raw)
  To: Sergei Shtylyov
  Cc: balbi, Gupta, Ajay Kumar, linux-usb, linux-kernel, Pasupathy,
	Visuvanadan

[-- Attachment #1: Type: text/plain, Size: 4993 bytes --]

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 #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: RFC: usb: musb: Changes proposed for adding CPPI4.1 DMA
  2012-02-02 11:49             ` Felipe Balbi
@ 2012-02-02 12:02               ` Sergei Shtylyov
  2012-02-02 12:15                 ` Felipe Balbi
  0 siblings, 1 reply; 14+ messages in thread
From: Sergei Shtylyov @ 2012-02-02 12:02 UTC (permalink / raw)
  To: balbi; +Cc: Gupta, Ajay Kumar, linux-usb, linux-kernel, Pasupathy, Visuvanadan

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: RFC: usb: musb: Changes proposed for adding CPPI4.1 DMA
  2012-02-02 12:02               ` Sergei Shtylyov
@ 2012-02-02 12:15                 ` Felipe Balbi
  0 siblings, 0 replies; 14+ messages in thread
From: Felipe Balbi @ 2012-02-02 12:15 UTC (permalink / raw)
  To: Sergei Shtylyov
  Cc: balbi, Gupta, Ajay Kumar, linux-usb, linux-kernel, Pasupathy,
	Visuvanadan

[-- Attachment #1: Type: text/plain, Size: 566 bytes --]

Hi,

On Thu, Feb 02, 2012 at 04:02:21PM +0400, Sergei Shtylyov wrote:
> >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. :-)

hmm... yeah, that's doable. Looks nice, actually. I'd ack that

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: RFC: usb: musb: Changes proposed for adding CPPI4.1 DMA
  2012-02-02 11:05         ` Sergei Shtylyov
@ 2012-02-03  8:42           ` Gupta, Ajay Kumar
  0 siblings, 0 replies; 14+ messages in thread
From: Gupta, Ajay Kumar @ 2012-02-03  8:42 UTC (permalink / raw)
  To: Sergei Shtylyov
  Cc: linux-usb, linux-kernel, Pasupathy, Visuvanadan, Balbi, Felipe

Hi,
> On 02-02-2012 8:57, 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 don't see how the device tree would magically help here..

I don't know this as of now as need to explore DT and see if that would help.

> 
> > I do see issues in reading wrapper interrupt status register and then
> > calling musb_interrupt() [defined inside musb_core.c] from slave
> driver.
> 
> >>> would post our proposal in RFC for slave driver design. Do you have
> >>> any proposal?
> 
> >>      I think this will need hooks from dma-engine.c to the glue
> layers. I
> >> was going to implement such in my version of MUSB CPPI 4.1 driver (in
> order
> >> to also support AM35x) but lacked time.
> 
> > That would mean a change in current drivers/dma API.
> 
>     No, it doesn't. I'm proposing hooks from dma-engine.c which is
> situated in
> drivers/usb/musb/ if I got you right. It just means that the calls to
> dma-engine.c as MUSB DMA driver won't map to the calls to the DMA slave
> driver as directly as you've presented before.

got it but passing the interrupt status from slave driver will be difficult
on am18x as there is single irq line for musb in AM18x whereas ti81xx and
AM335x has two interrupt line one for CPPI DMA and one for each mentor controller.

Ajay
> 
> > Ajay
> 
> >>> Regards,
> >>> Ajay
> 
> WBR, Sergei

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2012-02-03  8:43 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-01-25 15:22 RFC: usb: musb: Changes proposed for adding CPPI4.1 DMA Gupta, Ajay Kumar
2012-01-26  9:02 ` Felipe Balbi
2012-01-27 15:01   ` Gupta, Ajay Kumar
2012-01-27 16:09 ` Sergei Shtylyov
2012-01-31  4:41   ` Gupta, Ajay Kumar
2012-01-31 11:27     ` Sergei Shtylyov
2012-02-02  4:57       ` Gupta, Ajay Kumar
2012-02-02  9:09         ` Felipe Balbi
2012-02-02 11:12           ` Sergei Shtylyov
2012-02-02 11:49             ` Felipe Balbi
2012-02-02 12:02               ` Sergei Shtylyov
2012-02-02 12:15                 ` Felipe Balbi
2012-02-02 11:05         ` Sergei Shtylyov
2012-02-03  8:42           ` Gupta, Ajay Kumar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).