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

[boojin.kim@samsung.com (Boojin Kim)]

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 <boojin.kim@samsung.com> wrote:
>
> >> > -       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.

>
> >>
> >> > +               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
> > 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
> > address as following.
> >
> > #define S3C2412_IISTXD                  (0x10)
> > #define S3C2412_IISRXD                  (0x14)
> >
> > These definitions should be referred to DMA machine code to set fifo
> address
> > 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.

>
> > And, SLAVE_CONFIG command exist to pass slave configuration data from
> client
> > 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.

>
> >
> > Second, 'burst_sz' isn't fixed value. Namely, Client driver changes
> 'burst_sz'
> > 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.