[PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command

Jassi Brar wrote:

Sent: Thursday, July 21, 2011 2:00 PM
To: Boojin Kim
Cc: linux-arm-kernel at lists.infradead.org; linux-samsung-soc at vger.kernel.org;
Vinod Koul; Dan Williams; Kukjin Kim
Subject: Re: [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command

On Thu, Jul 21, 2011 at 9:43 AM, Boojin Kim [off-list ref] wrote:

quoted

quoted

quoted

-       pl330_tasklet((unsigned long) pch);
+               list_splice_tail_init(&list, &pdmac->desc_pool);
+               spin_unlock_irqrestore(&pch->lock, flags);
+               break;
+       case DMA_SLAVE_CONFIG:

Please protect this section too using spin_lock.

Why is spin_lock need here?
This code just sets configuration data into own channel structure.

It does seem unncessary, but the configuration that is set here is read
in other parts of the driver. However unlikely but there is theoretical
possibility of race here - one shouldn't count upon goodness of client
drivers.

Yes, Client driver doesn't afflict the configuration data again.
So, I think spin_lock isn't required here.

quoted

quoted

quoted

+               if (slave_config->direction == DMA_TO_DEVICE) {
+                       if (slave_config->dst_addr)
+                               peri->fifo_addr = 
slave_config->dst_addr;
+                       if (slave_config->dst_addr_width)
+                               peri->burst_sz = __ffs(slave_config-
dst_addr_width);
+               } else if (slave_config->direction == DMA_FROM_DEVICE) 
{
+                       if (slave_config->src_addr)
+                               peri->fifo_addr = 
slave_config->src_addr;
+                       if (slave_config->src_addr_width)
+                               peri->burst_sz = __ffs(slave_config-
src_addr_width);
+               }

PL330 has fixed channels to peripherals.
So FIFO addresses(burst_sz too?) should already be set via platform data.
Client drivers shouldn't bother.

First, DMA machine code should know the FIFO address of all client drivers

to

quoted

set via platform data.
In this case, Problem is that the definition of FIFO address is almost
declared to the header file of client driver.
For example, sound\soc\samsung\regs-i2s-v2.h only has the definition of

fifo

quoted

address as following.

#define S3C2412_IISTXD                  (0x10)
#define S3C2412_IISRXD                  (0x14)

These definitions should be referred to DMA machine code to set fifo

address

quoted

through platform data.
I think it's not good method.

Crap!
Client drivers don't conjure up the fifo address - if not hardcoded they
are gotten via platform_data already.

For it, DMA machine code should include all header files of client driver that 
has definition of FIFO address.
The number of header file would be more than 10. And I think it make the code 
a little complicated.

quoted

And, SLAVE_CONFIG command exist to pass slave configuration data from

client

quoted

driver to DMA driver.
So, I think that it's proper to pass fifo address through SLAVE_CONFIG
command.

If it's still not clear, read the para above definition of 'struct
dma_slave_config'
in include/linux/dmaengine.h

Other DMA client drivers in mainline code already use SLAVE_CONFIG command to 
pass fifo address as following.
Please refer to Sound/soc/imx/imx-pcm-dma/mx2.c, Driver/mmc/host/mmci.c, 
Drivers/mmc/host/mxcmmc.c and so on.

And, Other DMA drivers also support to SLAVE_CONFIG command for it.
Please refer to amba-pl08x.c, coh901318.c, ste_dma40.c and so on in 
'driver/dma' directory.
So, In my opinion, this is proper method to handle the client's fifo address.

quoted

Second, 'burst_sz' isn't fixed value. Namely, Client driver changes

'burst_sz'

quoted

according to its usecase.
For example, I2S driver sets 'burst_sz' to 4 in case of stereo
playback/recording. But in case of mono playback/recording, It changes
'burst_sz' to 2.
So, Client driver should use SLAVE_CONFIG command to set 'burst_sz' 
because
it's not fixed value.

Yeah yeah, ok, that's why I put that with a ? in bracket.
I just don't remember testing with fixed burst_sz with pl330.