| Message ID | 1311158773-30314-3-git-send-email-boojin.kim@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jul 20, 2011 at 4:16 PM, Boojin Kim <boojin.kim@samsung.com> wrote: > Signed-off-by: Boojin Kim <boojin.kim@samsung.com> > Signed-off-by: Kukjin Kim <kgene.kim@samsung.com> > --- > drivers/dma/pl330.c | 53 +++++++++++++++++++++++++++++++++++++------------- > 1 files changed, 39 insertions(+), 14 deletions(-) > > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c > index 586ab39..880f010 100644 > --- a/drivers/dma/pl330.c > +++ b/drivers/dma/pl330.c > @@ -256,25 +256,50 @@ static int pl330_alloc_chan_resources(struct dma_chan *chan) > static int pl330_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd, unsigned long arg) > { > struct dma_pl330_chan *pch = to_pchan(chan); > - struct dma_pl330_desc *desc; > + struct dma_pl330_desc *desc, *_dt; > unsigned long flags; > + struct dma_pl330_dmac *pdmac = pch->dmac; > + struct dma_slave_config *slave_config; > + struct dma_pl330_peri *peri; > + LIST_HEAD(list); > > - /* Only supports DMA_TERMINATE_ALL */ > - if (cmd != DMA_TERMINATE_ALL) > - return -ENXIO; > - > - spin_lock_irqsave(&pch->lock, flags); > - > - /* FLUSH the PL330 Channel thread */ > - pl330_chan_ctrl(pch->pl330_chid, PL330_OP_FLUSH); > + switch (cmd) { > + case DMA_TERMINATE_ALL: > + spin_lock_irqsave(&pch->lock, flags); > > - /* Mark all desc done */ > - list_for_each_entry(desc, &pch->work_list, node) > - desc->status = DONE; > + /* FLUSH the PL330 Channel thread */ > + pl330_chan_ctrl(pch->pl330_chid, PL330_OP_FLUSH); > > - spin_unlock_irqrestore(&pch->lock, flags); > + /* Mark all desc done */ > + list_for_each_entry_safe(desc, _dt, &pch->work_list , node) { > + desc->status = DONE; > + pch->completed = desc->txd.cookie; > + list_move_tail(&desc->node, &list); > + } > > - 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. > + 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. <will review remaining patches soon, gotta go now>
Jassi Brar wrote: > Sent: Thursday, July 21, 2011 4:18 AM > To: Boojin Kim > Cc: linux-arm-kernel@lists.infradead.org; linux-samsung-soc@vger.kernel.org; > Vinod Koul; Dan Williams; Kukjin Kim > Subject: Re: [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command > > On Wed, Jul 20, 2011 at 4:16 PM, Boojin Kim <boojin.kim@samsung.com> wrote: > > Signed-off-by: Boojin Kim <boojin.kim@samsung.com> > > Signed-off-by: Kukjin Kim <kgene.kim@samsung.com> > > --- > > drivers/dma/pl330.c | 53 > > +++++++++++++++++++++++++++++++++++++---------- > --- > > 1 files changed, 39 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c > > index 586ab39..880f010 100644 > > --- a/drivers/dma/pl330.c > > +++ b/drivers/dma/pl330.c > > @@ -256,25 +256,50 @@ static int pl330_alloc_chan_resources(struct > > dma_chan > *chan) > > static int pl330_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd, > unsigned long arg) > > { > > struct dma_pl330_chan *pch = to_pchan(chan); > > - struct dma_pl330_desc *desc; > > + struct dma_pl330_desc *desc, *_dt; > > unsigned long flags; > > + struct dma_pl330_dmac *pdmac = pch->dmac; > > + struct dma_slave_config *slave_config; > > + struct dma_pl330_peri *peri; > > + LIST_HEAD(list); > > > > - /* Only supports DMA_TERMINATE_ALL */ > > - if (cmd != DMA_TERMINATE_ALL) > > - return -ENXIO; > > - > > - spin_lock_irqsave(&pch->lock, flags); > > - > > - /* FLUSH the PL330 Channel thread */ > > - pl330_chan_ctrl(pch->pl330_chid, PL330_OP_FLUSH); > > + switch (cmd) { > > + case DMA_TERMINATE_ALL: > > + spin_lock_irqsave(&pch->lock, flags); > > > > - /* Mark all desc done */ > > - list_for_each_entry(desc, &pch->work_list, node) > > - desc->status = DONE; > > + /* FLUSH the PL330 Channel thread */ > > + pl330_chan_ctrl(pch->pl330_chid, PL330_OP_FLUSH); > > > > - spin_unlock_irqrestore(&pch->lock, flags); > > + /* Mark all desc done */ > > + list_for_each_entry_safe(desc, _dt, &pch->work_list , > > node) > { > > + desc->status = DONE; > > + pch->completed = desc->txd.cookie; > > + list_move_tail(&desc->node, &list); > > + } > > > > - 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. > > > > + 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. 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. 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. > > <will review remaining patches soon, gotta go now>
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. >> >> > + 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. > 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 > > 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.
Jassi Brar wrote: > Sent: Thursday, July 21, 2011 2:00 PM > To: Boojin Kim > Cc: linux-arm-kernel@lists.infradead.org; linux-samsung-soc@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.
On Thu, Jul 21, 2011 at 12:04 PM, Boojin Kim <boojin.kim@samsung.com> wrote: > Jassi Brar wrote: >> Sent: Thursday, July 21, 2011 2:00 PM >> To: Boojin Kim >> Cc: linux-arm-kernel@lists.infradead.org; linux-samsung-soc@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. > NAK.
On Thu, Jul 21, 2011 at 12:47:49AM +0530, Jassi Brar wrote: > On Wed, Jul 20, 2011 at 4:16 PM, Boojin Kim <boojin.kim@samsung.com> wrote: > > + 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. That's utter crap, and isn't what the DMA engine API is about. The above looks correctly implemented. Slave DMA engine users are supposed to supply the device DMA register address via this DMA_SLAVE_CONFIG call. Doing this via platform data for the DMA device is braindead.
On Thu, Jul 21, 2011 at 1:41 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Thu, Jul 21, 2011 at 12:47:49AM +0530, Jassi Brar wrote: >> On Wed, Jul 20, 2011 at 4:16 PM, Boojin Kim <boojin.kim@samsung.com> wrote: >> > + 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. > > That's utter crap, and isn't what the DMA engine API is about. > > The above looks correctly implemented. Slave DMA engine users are > supposed to supply the device DMA register address via this > DMA_SLAVE_CONFIG call. Doing this via platform data for the DMA > device is braindead. Rather than have 32 client drivers expect fifo_address from the platform and then provide to the DMAC, IMHO it is better for a single driver(DMAC) to get 32 addresses from the platform. Esp when the DMAC driver already expect similar h/w parameter -- *direction*. I don't understand why is 'fifo_address' a property much different than 'direction' of the channel ? If a channel is flexible enough to change it's 'fifo_address', most probably it could also change direction of transfers. So, why do we want to take seriously 'fifo_address' provided by the client drivers and not the 'direction' ?
On Thu, Jul 21, 2011 at 11:14 AM, Jassi Brar <jassisinghbrar@gmail.com> wrote: > On Thu, Jul 21, 2011 at 1:41 PM, Russell King - ARM Linux > <linux@arm.linux.org.uk> wrote: >> On Thu, Jul 21, 2011 at 12:47:49AM +0530, Jassi Brar wrote: >>> PL330 has fixed channels to peripherals. >>> So FIFO addresses(burst_sz too?) should already be set via platform data. >>> Client drivers shouldn't bother. >> >> That's utter crap, and isn't what the DMA engine API is about. >> >> The above looks correctly implemented. Slave DMA engine users are >> supposed to supply the device DMA register address via this >> DMA_SLAVE_CONFIG call. Doing this via platform data for the DMA >> device is braindead. > > Rather than have 32 client drivers expect fifo_address from the > platform and then > provide to the DMAC, IMHO it is better for a single driver(DMAC) to > get 32 addresses > from the platform. My idea (when I implemented it) is that the device driver knows the physical and virtual base and thus the address where DMA data is destined. It knows all other registers, it remaps them, noone else should be bothered with containing the knowledge of adress ranges for the specific hardware block. Passing this through platform data requires the machine to keep track not only of the base adress of the peripheral (as is generally contained in the machine or device tree) but also the offset of specific registers in that memory region, which is too much. Usually this means the offsets are defined in files like <mach/perfoo.h> which means they can't be pushed down into drivers/foo/foo.c and creates unnecessary bindings and de-encapsulation of the driver. We want to get rid of such stuff. (I think?) Therefore I introduced this to confine the different registers into the driver itself and ask the DMA engine to transfer to a specific address. > Esp when the DMAC driver already expect similar h/w > parameter -- *direction*. > > I don't understand why is 'fifo_address' a property much different > than 'direction' of the > channel ? struct dma_slave_config { enum dma_data_direction direction; dma_addr_t src_addr; dma_addr_t dst_addr; enum dma_slave_buswidth src_addr_width; enum dma_slave_buswidth dst_addr_width; u32 src_maxburst; u32 dst_maxburst; }; We do support changing direction as well as src and dst address (i.e. FIFO endpoint) at runtime. Check drivers/mmc/host/mmci.c for an example of how we switch a single channel from TX to RX in runtime using this one property. However it has been noted by Russell that the direction member is unnecessary since the device_prep_slave_sg() function in the dmaengine vtable already takes a direction argument like this: struct dma_async_tx_descriptor *(*device_prep_slave_sg)( struct dma_chan *chan, struct scatterlist *sgl, unsigned int sg_len, enum dma_data_direction direction, unsigned long flags); Therefore the direction setting needs to go from either the config struct or the device_prep_slave_sg() call, as right now the channel is being told about the direction twice which is not elegant and confusing. So you even have two ways of changing direction at runtime... Yours, Linus Walleij
On Thu, Jul 21, 2011 at 02:44:28PM +0530, Jassi Brar wrote: > On Thu, Jul 21, 2011 at 1:41 PM, Russell King - ARM Linux > <linux@arm.linux.org.uk> wrote: > > On Thu, Jul 21, 2011 at 12:47:49AM +0530, Jassi Brar wrote: > >> On Wed, Jul 20, 2011 at 4:16 PM, Boojin Kim <boojin.kim@samsung.com> wrote: > >> > + 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. > > > > That's utter crap, and isn't what the DMA engine API is about. > > > > The above looks correctly implemented. Slave DMA engine users are > > supposed to supply the device DMA register address via this > > DMA_SLAVE_CONFIG call. Doing this via platform data for the DMA > > device is braindead. > > Rather than have 32 client drivers expect fifo_address from the > platform and then > provide to the DMAC, IMHO it is better for a single driver(DMAC) to > get 32 addresses > from the platform. > Esp when the DMAC driver already expect similar h/w parameter -- *direction*. Conversely, when a platform doesn't know where the FIFOs are because they're located inside the actual devices, and the device driver does, then it makes much more sense for the device driver to provide the DMA engine with the bus address of that. Does your hardware have a hardware block from the device itself containing all the systems FIFOs ? > I don't understand why is 'fifo_address' a property much different > than 'direction' of the channel ? Some channels can do memory-to-peripheral and peripheral-to-memory transfers. Some peripherals provide a single set of DMA request/ack lines to perform this function. Some peripherals have different addresses for the TX and RX FIFOs within the device. > If a channel is flexible enough to change it's 'fifo_address', most > probably it could also change direction of transfers. There are DMA engines and setups where that's true. > So, why do we want to take seriously 'fifo_address' provided by the > client drivers and not the 'direction' ? The direction in the DMA slave config call I believe to be an annoying overdesign which shouldn't be there - the DMA slave config call should configure the DMA channel for the peripherals characteristics. The actual channel direction should be setup at preparation time along with the DMA buffer mapping etc.
On Thu, Jul 21, 2011 at 3:53 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Thu, Jul 21, 2011 at 11:14 AM, Jassi Brar <jassisinghbrar@gmail.com> wrote: >> On Thu, Jul 21, 2011 at 1:41 PM, Russell King - ARM Linux >> <linux@arm.linux.org.uk> wrote: >>> On Thu, Jul 21, 2011 at 12:47:49AM +0530, Jassi Brar wrote: >>>> PL330 has fixed channels to peripherals. >>>> So FIFO addresses(burst_sz too?) should already be set via platform data. >>>> Client drivers shouldn't bother. >>> >>> That's utter crap, and isn't what the DMA engine API is about. >>> >>> The above looks correctly implemented. Slave DMA engine users are >>> supposed to supply the device DMA register address via this >>> DMA_SLAVE_CONFIG call. Doing this via platform data for the DMA >>> device is braindead. >> >> Rather than have 32 client drivers expect fifo_address from the >> platform and then >> provide to the DMAC, IMHO it is better for a single driver(DMAC) to >> get 32 addresses >> from the platform. > > My idea (when I implemented it) is that the device driver knows the > physical and virtual base and thus the address where DMA data is > destined. It knows all other registers, it remaps them, noone else should > be bothered with containing the knowledge of adress ranges for the > specific hardware block. > > Passing this through platform data requires the machine to keep track > not only of the base adress of the peripheral (as is generally contained > in the machine or device tree) but also the offset of specific registers > in that memory region, which is too much. > > Usually this means the offsets are defined in files like <mach/perfoo.h> > which means they can't be pushed down into drivers/foo/foo.c and > creates unnecessary bindings and de-encapsulation of the driver. > We want to get rid of such stuff. (I think?) > > Therefore I introduced this to confine the different registers into > the driver itself and ask the DMA engine to transfer to a specific > address. While that does make sense, it makes mandatory the ritual of calling DMA_SLAVE_CONFIG mandatory for most of the cases. And I think the effort to set fifo_addr for peripherals is overrated. > >> Esp when the DMAC driver already expect similar h/w >> parameter -- *direction*. >> >> I don't understand why is 'fifo_address' a property much different >> than 'direction' of the >> channel ? > > struct dma_slave_config { > enum dma_data_direction direction; > dma_addr_t src_addr; > dma_addr_t dst_addr; > enum dma_slave_buswidth src_addr_width; > enum dma_slave_buswidth dst_addr_width; > u32 src_maxburst; > u32 dst_maxburst; > }; > > We do support changing direction as well as src and dst address > (i.e. FIFO endpoint) at runtime. > > Check drivers/mmc/host/mmci.c for an example of how we switch > a single channel from TX to RX in runtime using this one property. > > However it has been noted by Russell that the direction member > is unnecessary since the device_prep_slave_sg() function in the > dmaengine vtable already takes a direction argument like this: > > struct dma_async_tx_descriptor *(*device_prep_slave_sg)( > struct dma_chan *chan, struct scatterlist *sgl, > unsigned int sg_len, enum dma_data_direction direction, > unsigned long flags); > > Therefore the direction setting needs to go from either the config > struct or the device_prep_slave_sg() call, as right now the channel > is being told about the direction twice which is not elegant and > confusing. > > So you even have two ways of changing direction at runtime... Of course there are ways to set the direction... but whatever we do it can't really be changed from what h/w can only do. And that is my point. Let clients not bother trying to 'configure' what is the only supported option by h/w. And I don't suggest dropping the DMA_SLAVE_CONFIG callback - just keep it for rarer situations when we have configurable channels. And I assumed that was your original idea too. ----------------------- * @DMA_SLAVE_CONFIG: this command is only implemented by DMA controllers * that need to runtime reconfigure the slave channels (as opposed to passing * configuration data in statically from the platform). An additional * argument of struct dma_slave_config must be passed in with this * command. ----------------------- Regards, -j > > Yours, > Linus Walleij >
On Thu, Jul 21, 2011 at 4:59 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: >> >> PL330 has fixed channels to peripherals. >> >> So FIFO addresses(burst_sz too?) should already be set via platform data. >> >> Client drivers shouldn't bother. >> > >> > That's utter crap, and isn't what the DMA engine API is about. >> > >> > The above looks correctly implemented. Slave DMA engine users are >> > supposed to supply the device DMA register address via this >> > DMA_SLAVE_CONFIG call. Doing this via platform data for the DMA >> > device is braindead. >> >> Rather than have 32 client drivers expect fifo_address from the >> platform and then >> provide to the DMAC, IMHO it is better for a single driver(DMAC) to >> get 32 addresses >> from the platform. >> Esp when the DMAC driver already expect similar h/w parameter -- *direction*. > > Conversely, when a platform doesn't know where the FIFOs are because > they're located inside the actual devices, and the device driver does, > then it makes much more sense for the device driver to provide the > DMA engine with the bus address of that. Yes, that is a case to consider. Perhaps we already do something like that while setting i2c addresses of attached devices in board files... and similarly it might be possible for the developer to know what the fifo address is going to be after the device is up and running esp when it is interfaced with an internal component like DMA. > > Does your hardware have a hardware block from the device itself containing > all the systems FIFOs ? I am not sure what you ask, but let me say what I know. In this case atleast, all PL330 DMA channels have fixed source/destination address on the device side. So it's not like developer doesn't know fifo_addr here. >> I don't understand why is 'fifo_address' a property much different >> than 'direction' of the channel ? > > Some channels can do memory-to-peripheral and peripheral-to-memory > transfers. Some peripherals provide a single set of DMA request/ack > lines to perform this function. Some peripherals have different > addresses for the TX and RX FIFOs within the device. Likewise we had something like that for I2S IP of S3C2440(or A0 ?) a single fifo address shared by TX and RX channel. Depending if it's full/half duplex capable we can have both or one 'virtual' channel active at a time. > >> If a channel is flexible enough to change it's 'fifo_address', most >> probably it could also change direction of transfers. > > There are DMA engines and setups where that's true. Of course, and I think this 'runtime' configuration should be done only for such cases. thnx
On Thu, Jul 21, 2011 at 08:42:40PM +0530, Jassi Brar wrote: > On Thu, Jul 21, 2011 at 4:59 PM, Russell King - ARM Linux > <linux@arm.linux.org.uk> wrote: > > Does your hardware have a hardware block from the device itself containing > > all the systems FIFOs ? > I am not sure what you ask, but let me say what I know. > In this case atleast, all PL330 DMA channels have fixed source/destination > address on the device side. So it's not like developer doesn't know > fifo_addr here. Even so, your approach is _conceptually_ wrong. Think about it. You declare your devices giving the bus address where they're located. So, lets say for argument that your UART is located at 0x10001000. Your UART driver knows that the FIFO register is at offset 0x20 from the base address. Your platform data provides the UART driver with a DMA match function and data specific for that match function. This data encodes the specific DMA channel. Now, why should you have to encode into the DMA drivers platform data that DMA channel X has its FIFO at 0x10001020? Not only do you have to declare the base address of the UART but also you have to know the offset into the UART. Why not just let the UART driver - which knows that the base address is 0x10001000, and the FIFO is at offset 0x20 above that - tell the DMA driver that's where the FIFO is located? Finally, your specific SoC may have a PL330, which may have FIFOs tied to specific DMA request signals. That's an _implementation_ _detail_. That doesn't mean that's always going to be the case. I've heard that ARM Ltd may start using the PL330 on their evaluation boards. They have a habbit of muxing several DMA request signals to several different peripherals. Will I need to rewrite all the Samsung users of PL330 when that happens because they've decided they don't like the DMA engine API and have gone off and done their own thing instead? So no, encoding the FIFO addresses into platform data for the DMA controller is brain dead and totally the wrong thing to do. And when you come to moving stuff over to DT, you'll see that's even more true there. So don't make the mistake now. Do things sensibly and follow the DMA engine API. Arrange for all your drivers to call DMA_SLAVE_CONFIG with the address of the FIFO.
On Thu, Jul 21, 2011 at 07:58:03PM +0530, Jassi Brar wrote: > While that does make sense, it makes mandatory the ritual of calling > DMA_SLAVE_CONFIG mandatory for most of the cases. > And I think the effort to set fifo_addr for peripherals is overrated. You have to do that anyway. You're dealing with DMA engine drivers which you don't know the implementation details of. Think about it for a moment, please. The point of the DMA engine abstraction is to make DMA users independent of the DMA engine actually being used. That means all DMA engine drivers and all DMA engine users have to provide a basic set of information to allow them to interact with each other. Encoding random bits of information into inappropriate places like FIFO addresses into DMA engine platform data just means you might was well stay with your private API, because that driver may never be able to work with another DMA engine implementation. So please, either do the job properly, or don't waste peoples time with a half hearted mess of inappropriate platform data.
On Thu, Jul 21, 2011 at 8:53 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Thu, Jul 21, 2011 at 08:42:40PM +0530, Jassi Brar wrote: >> On Thu, Jul 21, 2011 at 4:59 PM, Russell King - ARM Linux >> <linux@arm.linux.org.uk> wrote: >> > Does your hardware have a hardware block from the device itself containing >> > all the systems FIFOs ? >> I am not sure what you ask, but let me say what I know. >> In this case atleast, all PL330 DMA channels have fixed source/destination >> address on the device side. So it's not like developer doesn't know >> fifo_addr here. > > Even so, your approach is _conceptually_ wrong. Think about it. > > You declare your devices giving the bus address where they're located. > So, lets say for argument that your UART is located at 0x10001000. > > Your UART driver knows that the FIFO register is at offset 0x20 from > the base address. Your platform data provides the UART driver with a > DMA match function and data specific for that match function. This > data encodes the specific DMA channel. > > Now, why should you have to encode into the DMA drivers platform data > that DMA channel X has its FIFO at 0x10001020? Not only do you have > to declare the base address of the UART but also you have to know the > offset into the UART. > > Why not just let the UART driver - which knows that the base address > is 0x10001000, and the FIFO is at offset 0x20 above that - tell the > DMA driver that's where the FIFO is located? Yes I understand, the idea was to avoid optional DMA_SLAVE_CONFIG call. Apparently we give different weightage to the pros and cons. Anyways, I accept your opinion. Though you might want to consider changing the DMA_SLAVE_CONFIG API from optional to mandatory for Slave capable DMACs. Otherwise I don't see common client drivers working over different DMACs. Thanks.
On Thu, Jul 21, 2011 at 09:26:22PM +0530, Jassi Brar wrote: > Though you might want to consider changing the DMA_SLAVE_CONFIG API from > optional to mandatory for Slave capable DMACs. Otherwise I don't see common > client drivers working over different DMACs. Yes, I think we will have to, otherwise we can't ensure we have cross- compatibility between different DMA engine implementations. We already have platforms where we have peripheral drivers making use of several different DMA engine implementations, so nailing this down now, before the number of DMA engine implementations increases still further is definitely something that needs doing.
On Wed, Jul 20, 2011 at 4:16 PM, Boojin Kim <boojin.kim@samsung.com> wrote: > Signed-off-by: Boojin Kim <boojin.kim@samsung.com> > Signed-off-by: Kukjin Kim <kgene.kim@samsung.com> > --- > drivers/dma/pl330.c | 53 +++++++++++++++++++++++++++++++++++++------------- > 1 files changed, 39 insertions(+), 14 deletions(-) > > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c > index 586ab39..880f010 100644 > --- a/drivers/dma/pl330.c > +++ b/drivers/dma/pl330.c > @@ -256,25 +256,50 @@ static int pl330_alloc_chan_resources(struct dma_chan *chan) > static int pl330_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd, unsigned long arg) > { > struct dma_pl330_chan *pch = to_pchan(chan); > - struct dma_pl330_desc *desc; > + struct dma_pl330_desc *desc, *_dt; > unsigned long flags; > + struct dma_pl330_dmac *pdmac = pch->dmac; > + struct dma_slave_config *slave_config; > + struct dma_pl330_peri *peri; > + LIST_HEAD(list); > > - /* Only supports DMA_TERMINATE_ALL */ > - if (cmd != DMA_TERMINATE_ALL) > - return -ENXIO; > - > - spin_lock_irqsave(&pch->lock, flags); > - > - /* FLUSH the PL330 Channel thread */ > - pl330_chan_ctrl(pch->pl330_chid, PL330_OP_FLUSH); > + switch (cmd) { > + case DMA_TERMINATE_ALL: > + spin_lock_irqsave(&pch->lock, flags); > > - /* Mark all desc done */ > - list_for_each_entry(desc, &pch->work_list, node) > - desc->status = DONE; > + /* FLUSH the PL330 Channel thread */ > + pl330_chan_ctrl(pch->pl330_chid, PL330_OP_FLUSH); > > - spin_unlock_irqrestore(&pch->lock, flags); > + /* Mark all desc done */ > + list_for_each_entry_safe(desc, _dt, &pch->work_list , node) { > + desc->status = DONE; > + pch->completed = desc->txd.cookie; > + list_move_tail(&desc->node, &list); > + } > > - pl330_tasklet((unsigned long) pch); > + list_splice_tail_init(&list, &pdmac->desc_pool); > + spin_unlock_irqrestore(&pch->lock, flags); > + break; > + case DMA_SLAVE_CONFIG: > + slave_config = (struct dma_slave_config *)arg; > + peri = pch->chan.private; > + > + 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); > + } > + break; As discussed yesterday, DMA_SLAVE_CONFIG is assumed to be madatory, so please dismantle the 'struct dma_pl330_peri', move fifo_addr, burst_sz and rqtype to 'struct dma_pl330_chan' and poison them - that will force clients to do DMA_SLAVE_CONFIG. Move 'peri_id' to 'struct dma_pl330_platdata' And protect the changes to channel parameters with lock.
On Thu, Jul 21, 2011 at 4:28 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote: > [Me] >> Therefore I introduced this to confine the different registers into >> the driver itself and ask the DMA engine to transfer to a specific >> address. > > While that does make sense, it makes mandatory the ritual of calling > DMA_SLAVE_CONFIG mandatory for most of the cases. > And I think the effort to set fifo_addr for peripherals is overrated. Oh well. I beg to differ, maybe I'm poisoned by stuff like this: http://en.wikipedia.org/wiki/Encapsulation_%28object-oriented_programming%29 so let's say we agree to disagree on this then. > Of course there are ways to set the direction... but whatever we do > it can't really be changed from what h/w can only do. > And that is my point. Let clients not bother trying to 'configure' what is > the only supported option by h/w. The interface is generic, and as is demonstrated in the U300 COH901318 and also I think ARM RealView and Versatile reference designs, the DMA channel for the MMCI block is bidirectional, so you really change the direction of the channel at runtime, it's not like I'm making it up and introducing that possibility just for fun :-D So the generic case is that you can request a direction for the channel, and if the hardware doesn't support that, then NAK any tries to set it to a direction which is illegal. Yes, some abstraction but generalization does have a price, and I think it's worth it. Yours, Linus Walleij
Jassi Brar wrote: > Sent: Friday, July 22, 2011 2:42 PM > To: Boojin Kim > Cc: linux-arm-kernel@lists.infradead.org; linux-samsung-soc@vger.kernel.org; > Vinod Koul; Dan Williams; Kukjin Kim > Subject: Re: [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command > > On Wed, Jul 20, 2011 at 4:16 PM, Boojin Kim <boojin.kim@samsung.com> wrote: > > Signed-off-by: Boojin Kim <boojin.kim@samsung.com> > > Signed-off-by: Kukjin Kim <kgene.kim@samsung.com> > > --- > > drivers/dma/pl330.c | 53 > > +++++++++++++++++++++++++++++++++++++---------- > --- > > 1 files changed, 39 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c > > index 586ab39..880f010 100644 > > --- a/drivers/dma/pl330.c > > +++ b/drivers/dma/pl330.c > > @@ -256,25 +256,50 @@ static int pl330_alloc_chan_resources(struct > > dma_chan > *chan) > > static int pl330_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd, > unsigned long arg) > > { > > struct dma_pl330_chan *pch = to_pchan(chan); > > - struct dma_pl330_desc *desc; > > + struct dma_pl330_desc *desc, *_dt; > > unsigned long flags; > > + struct dma_pl330_dmac *pdmac = pch->dmac; > > + struct dma_slave_config *slave_config; > > + struct dma_pl330_peri *peri; > > + LIST_HEAD(list); > > > > - /* Only supports DMA_TERMINATE_ALL */ > > - if (cmd != DMA_TERMINATE_ALL) > > - return -ENXIO; > > - > > - spin_lock_irqsave(&pch->lock, flags); > > - > > - /* FLUSH the PL330 Channel thread */ > > - pl330_chan_ctrl(pch->pl330_chid, PL330_OP_FLUSH); > > + switch (cmd) { > > + case DMA_TERMINATE_ALL: > > + spin_lock_irqsave(&pch->lock, flags); > > > > - /* Mark all desc done */ > > - list_for_each_entry(desc, &pch->work_list, node) > > - desc->status = DONE; > > + /* FLUSH the PL330 Channel thread */ > > + pl330_chan_ctrl(pch->pl330_chid, PL330_OP_FLUSH); > > > > - spin_unlock_irqrestore(&pch->lock, flags); > > + /* Mark all desc done */ > > + list_for_each_entry_safe(desc, _dt, &pch->work_list , > > node) > { > > + desc->status = DONE; > > + pch->completed = desc->txd.cookie; > > + list_move_tail(&desc->node, &list); > > + } > > > > - pl330_tasklet((unsigned long) pch); > > + list_splice_tail_init(&list, &pdmac->desc_pool); > > + spin_unlock_irqrestore(&pch->lock, flags); > > + break; > > + case DMA_SLAVE_CONFIG: > > + slave_config = (struct dma_slave_config *)arg; > > + peri = pch->chan.private; > > + > > + 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); > > + } > > + break; > As discussed yesterday, DMA_SLAVE_CONFIG is assumed to be madatory, > so please dismantle the 'struct dma_pl330_peri', move fifo_addr, > burst_sz and rqtype > to 'struct dma_pl330_chan' and poison them - that will force clients > to do DMA_SLAVE_CONFIG. Move 'peri_id' to 'struct dma_pl330_platdata' > And protect the changes to channel parameters with lock. It's fine that you understand my implementation. I'm agree to remove unnecessary structure. But, this item should bring the change of all DMA machine code and doesn't has deeply relationship with the purpose of this patch series (samsung dma usage). So, I'd like to make it to another patch and submit it. How about my opinion ?
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c index 586ab39..880f010 100644 --- a/drivers/dma/pl330.c +++ b/drivers/dma/pl330.c @@ -256,25 +256,50 @@ static int pl330_alloc_chan_resources(struct dma_chan *chan) static int pl330_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd, unsigned long arg) { struct dma_pl330_chan *pch = to_pchan(chan); - struct dma_pl330_desc *desc; + struct dma_pl330_desc *desc, *_dt; unsigned long flags; + struct dma_pl330_dmac *pdmac = pch->dmac; + struct dma_slave_config *slave_config; + struct dma_pl330_peri *peri; + LIST_HEAD(list); - /* Only supports DMA_TERMINATE_ALL */ - if (cmd != DMA_TERMINATE_ALL) - return -ENXIO; - - spin_lock_irqsave(&pch->lock, flags); - - /* FLUSH the PL330 Channel thread */ - pl330_chan_ctrl(pch->pl330_chid, PL330_OP_FLUSH); + switch (cmd) { + case DMA_TERMINATE_ALL: + spin_lock_irqsave(&pch->lock, flags); - /* Mark all desc done */ - list_for_each_entry(desc, &pch->work_list, node) - desc->status = DONE; + /* FLUSH the PL330 Channel thread */ + pl330_chan_ctrl(pch->pl330_chid, PL330_OP_FLUSH); - spin_unlock_irqrestore(&pch->lock, flags); + /* Mark all desc done */ + list_for_each_entry_safe(desc, _dt, &pch->work_list , node) { + desc->status = DONE; + pch->completed = desc->txd.cookie; + list_move_tail(&desc->node, &list); + } - pl330_tasklet((unsigned long) pch); + list_splice_tail_init(&list, &pdmac->desc_pool); + spin_unlock_irqrestore(&pch->lock, flags); + break; + case DMA_SLAVE_CONFIG: + slave_config = (struct dma_slave_config *)arg; + peri = pch->chan.private; + + 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); + } + break; + default: + dev_err(pch->dmac->pif.dev, "Not supported command.\n"); + return -ENXIO; + } return 0; }