| Message ID | 1313744097-27289-5-git-send-email-boojin.kim@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Aug 19, 2011 at 2:24 PM, Boojin Kim <boojin.kim@samsung.com> wrote: > @@ -324,6 +362,9 @@ static void pl330_free_chan_resources(struct dma_chan *chan) > pl330_release_channel(pch->pl330_chid); > pch->pl330_chid = NULL; > > + if (pch->cyclic) > + list_splice_tail_init(&pch->work_list, &pch->dmac->desc_pool); 'cyclic' member is 'enum cyclic_mode', please observe the rule and compare it only against the enum values. > +static struct dma_async_tx_descriptor *pl330_prep_dma_cyclic( > + struct dma_chan *chan, dma_addr_t dma_addr, size_t len, > + size_t period_len, enum dma_data_direction direction) > +{ > + struct dma_pl330_desc *desc; > + struct dma_pl330_chan *pch = to_pchan(chan); > + dma_addr_t dst; > + dma_addr_t src; > + > + desc = pl330_get_desc(pch); > + if (!desc) { > + dev_err(pch->dmac->pif.dev, "%s:%d Unable to fetch desc\n", > + __func__, __LINE__); > + return NULL; > + } > + > + switch (direction) { > + case DMA_TO_DEVICE: > + desc->rqcfg.src_inc = 1; > + desc->rqcfg.dst_inc = 0; > + src = dma_addr; > + dst = pch->fifo_addr; > + break; > + case DMA_FROM_DEVICE: > + desc->rqcfg.src_inc = 0; > + desc->rqcfg.dst_inc = 1; > + src = pch->fifo_addr; > + dst = dma_addr; > + break; > + default: > + dev_err(pch->dmac->pif.dev, "%s:%d Invalid dma direction\n", > + __func__, __LINE__); > + return NULL; > + } > + > + desc->rqcfg.brst_size = pch->burst_sz; > + desc->rqcfg.brst_len = 1; > + > + if (!pch->cyclic) > + pch->cyclic = CYCLIC_PREP; The need for check here seems suspicious. Is it really needed? If not, please remove it.
Jassi Brar wrote: > > > @@ -324,6 +362,9 @@ static void pl330_free_chan_resources(struct > dma_chan *chan) > > pl330_release_channel(pch->pl330_chid); > > pch->pl330_chid = NULL; > > > > + if (pch->cyclic) > > + list_splice_tail_init(&pch->work_list, &pch->dmac- > >desc_pool); > 'cyclic' member is 'enum cyclic_mode', please observe the rule and > compare > it only against the enum values. I will address your comment. > > > > +static struct dma_async_tx_descriptor *pl330_prep_dma_cyclic( > > + struct dma_chan *chan, dma_addr_t dma_addr, size_t len, > > + size_t period_len, enum dma_data_direction direction) > > +{ > > + struct dma_pl330_desc *desc; > > + struct dma_pl330_chan *pch = to_pchan(chan); > > + dma_addr_t dst; > > + dma_addr_t src; > > + > > + desc = pl330_get_desc(pch); > > + if (!desc) { > > + dev_err(pch->dmac->pif.dev, "%s:%d Unable to fetch > desc\n", > > + __func__, __LINE__); > > + return NULL; > > + } > > + > > + switch (direction) { > > + case DMA_TO_DEVICE: > > + desc->rqcfg.src_inc = 1; > > + desc->rqcfg.dst_inc = 0; > > + src = dma_addr; > > + dst = pch->fifo_addr; > > + break; > > + case DMA_FROM_DEVICE: > > + desc->rqcfg.src_inc = 0; > > + desc->rqcfg.dst_inc = 1; > > + src = pch->fifo_addr; > > + dst = dma_addr; > > + break; > > + default: > > + dev_err(pch->dmac->pif.dev, "%s:%d Invalid dma > direction\n", > > + __func__, __LINE__); > > + return NULL; > > + } > > + > > + desc->rqcfg.brst_size = pch->burst_sz; > > + desc->rqcfg.brst_len = 1; > > + > > + if (!pch->cyclic) > > + pch->cyclic = CYCLIC_PREP; > The need for check here seems suspicious. > Is it really needed? If not, please remove it. It's required because Cyclic operation is passed from CYCLIC_PREP to CYCLIC_TRIGGER. Thanks Boojin
On Mon, Aug 22, 2011 at 5:33 PM, Boojin Kim <boojin.kim@samsung.com> wrote: >> >> > +static struct dma_async_tx_descriptor *pl330_prep_dma_cyclic( >> > + struct dma_chan *chan, dma_addr_t dma_addr, size_t len, >> > + size_t period_len, enum dma_data_direction direction) >> > +{ >> > + struct dma_pl330_desc *desc; >> > + struct dma_pl330_chan *pch = to_pchan(chan); >> > + dma_addr_t dst; >> > + dma_addr_t src; >> > + >> > + desc = pl330_get_desc(pch); >> > + if (!desc) { >> > + dev_err(pch->dmac->pif.dev, "%s:%d Unable to fetch >> desc\n", >> > + __func__, __LINE__); >> > + return NULL; >> > + } >> > + >> > + switch (direction) { >> > + case DMA_TO_DEVICE: >> > + desc->rqcfg.src_inc = 1; >> > + desc->rqcfg.dst_inc = 0; >> > + src = dma_addr; >> > + dst = pch->fifo_addr; >> > + break; >> > + case DMA_FROM_DEVICE: >> > + desc->rqcfg.src_inc = 0; >> > + desc->rqcfg.dst_inc = 1; >> > + src = pch->fifo_addr; >> > + dst = dma_addr; >> > + break; >> > + default: >> > + dev_err(pch->dmac->pif.dev, "%s:%d Invalid dma >> direction\n", >> > + __func__, __LINE__); >> > + return NULL; >> > + } >> > + >> > + desc->rqcfg.brst_size = pch->burst_sz; >> > + desc->rqcfg.brst_len = 1; >> > + >> > + if (!pch->cyclic) >> > + pch->cyclic = CYCLIC_PREP; >> The need for check here seems suspicious. >> Is it really needed? If not, please remove it. > It's required because Cyclic operation is passed from CYCLIC_PREP to > CYCLIC_TRIGGER. I don't see why you need to differentiate between PREP and TRIGGER in cyclic mode. I think, you should simply have 'bool cyclic' instead of enum.
Jassi Brar [mailto:jassisinghbrar@gmail.com] > Sent: Tuesday, August 23, 2011 2:42 PM > To: Boojin Kim > Cc: linux-arm-kernel@lists.infradead.org; linux-samsung- > soc@vger.kernel.org; Vinod Koul; Kukjin Kim; Dan Williams; Mark Brown; > Grant Likely; Russell King > Subject: Re: [PATCH v6 04/15] DMA: PL330: Add DMA_CYCLIC capability > > On Mon, Aug 22, 2011 at 5:33 PM, Boojin Kim <boojin.kim@samsung.com> > wrote: > >> > >> > +static struct dma_async_tx_descriptor *pl330_prep_dma_cyclic( > >> > + struct dma_chan *chan, dma_addr_t dma_addr, size_t > len, > >> > + size_t period_len, enum dma_data_direction > direction) > >> > +{ > >> > + struct dma_pl330_desc *desc; > >> > + struct dma_pl330_chan *pch = to_pchan(chan); > >> > + dma_addr_t dst; > >> > + dma_addr_t src; > >> > + > >> > + desc = pl330_get_desc(pch); > >> > + if (!desc) { > >> > + dev_err(pch->dmac->pif.dev, "%s:%d Unable to fetch > >> desc\n", > >> > + __func__, __LINE__); > >> > + return NULL; > >> > + } > >> > + > >> > + switch (direction) { > >> > + case DMA_TO_DEVICE: > >> > + desc->rqcfg.src_inc = 1; > >> > + desc->rqcfg.dst_inc = 0; > >> > + src = dma_addr; > >> > + dst = pch->fifo_addr; > >> > + break; > >> > + case DMA_FROM_DEVICE: > >> > + desc->rqcfg.src_inc = 0; > >> > + desc->rqcfg.dst_inc = 1; > >> > + src = pch->fifo_addr; > >> > + dst = dma_addr; > >> > + break; > >> > + default: > >> > + dev_err(pch->dmac->pif.dev, "%s:%d Invalid dma > >> direction\n", > >> > + __func__, __LINE__); > >> > + return NULL; > >> > + } > >> > + > >> > + desc->rqcfg.brst_size = pch->burst_sz; > >> > + desc->rqcfg.brst_len = 1; > >> > + > >> > + if (!pch->cyclic) > >> > + pch->cyclic = CYCLIC_PREP; > >> The need for check here seems suspicious. > >> Is it really needed? If not, please remove it. > > It's required because Cyclic operation is passed from CYCLIC_PREP to > > CYCLIC_TRIGGER. > I don't see why you need to differentiate between PREP and TRIGGER in > cyclic mode. I think, you should simply have 'bool cyclic' instead of > enum. On cyclic mode, Pl330_tasklet() operation is changed according to the value of 'cyclic'. In case of CYCLIC_PREP, Pl330_tasklet()operation is same with normal operation. In case of CYCLIC_TRIGGER, pl330_tasklet()reloads the done data into work_list. So, 'Cyclic' needs two status(CYCLIC_PREP and CYCLIC_TRIGGER) to distinguish the operation on pl330_tasklet(). The sequence is following. device_prep_dma_cyclic() -> set CYCLIC_PREP -> device_issue_pending() -> Pl330_tasklet() with CYCLIC_PREP -> set CYCLIC_TRIGGER -> PL330 IRQ -> Pl330_tasklet() with CYCLIC_TRIGGER. Thanks Boojin
On Tue, Aug 23, 2011 at 12:38 PM, Boojin Kim <boojin.kim@samsung.com> wrote: > Jassi Brar [mailto:jassisinghbrar@gmail.com] >> Sent: Tuesday, August 23, 2011 2:42 PM >> To: Boojin Kim >> Cc: linux-arm-kernel@lists.infradead.org; linux-samsung- >> soc@vger.kernel.org; Vinod Koul; Kukjin Kim; Dan Williams; Mark Brown; >> Grant Likely; Russell King >> Subject: Re: [PATCH v6 04/15] DMA: PL330: Add DMA_CYCLIC capability >> >> On Mon, Aug 22, 2011 at 5:33 PM, Boojin Kim <boojin.kim@samsung.com> >> wrote: >> >> >> >> > +static struct dma_async_tx_descriptor *pl330_prep_dma_cyclic( >> >> > + struct dma_chan *chan, dma_addr_t dma_addr, size_t >> len, >> >> > + size_t period_len, enum dma_data_direction >> direction) >> >> > +{ >> >> > + struct dma_pl330_desc *desc; >> >> > + struct dma_pl330_chan *pch = to_pchan(chan); >> >> > + dma_addr_t dst; >> >> > + dma_addr_t src; >> >> > + >> >> > + desc = pl330_get_desc(pch); >> >> > + if (!desc) { >> >> > + dev_err(pch->dmac->pif.dev, "%s:%d Unable to fetch >> >> desc\n", >> >> > + __func__, __LINE__); >> >> > + return NULL; >> >> > + } >> >> > + >> >> > + switch (direction) { >> >> > + case DMA_TO_DEVICE: >> >> > + desc->rqcfg.src_inc = 1; >> >> > + desc->rqcfg.dst_inc = 0; >> >> > + src = dma_addr; >> >> > + dst = pch->fifo_addr; >> >> > + break; >> >> > + case DMA_FROM_DEVICE: >> >> > + desc->rqcfg.src_inc = 0; >> >> > + desc->rqcfg.dst_inc = 1; >> >> > + src = pch->fifo_addr; >> >> > + dst = dma_addr; >> >> > + break; >> >> > + default: >> >> > + dev_err(pch->dmac->pif.dev, "%s:%d Invalid dma >> >> direction\n", >> >> > + __func__, __LINE__); >> >> > + return NULL; >> >> > + } >> >> > + >> >> > + desc->rqcfg.brst_size = pch->burst_sz; >> >> > + desc->rqcfg.brst_len = 1; >> >> > + >> >> > + if (!pch->cyclic) >> >> > + pch->cyclic = CYCLIC_PREP; >> >> The need for check here seems suspicious. >> >> Is it really needed? If not, please remove it. >> > It's required because Cyclic operation is passed from CYCLIC_PREP to >> > CYCLIC_TRIGGER. >> I don't see why you need to differentiate between PREP and TRIGGER in >> cyclic mode. I think, you should simply have 'bool cyclic' instead of >> enum. > On cyclic mode, Pl330_tasklet() operation is changed according to the value of > 'cyclic'. > In case of CYCLIC_PREP, Pl330_tasklet()operation is same with normal > operation. By 'normal' you mean non-cyclic, right? That doesn't seem correct to do. Once the desc has been marked cyclic by device_prep_dma_cyclic(), you shouldn't treat it like a non-cyclic anymore. > The sequence is following. > device_prep_dma_cyclic() -> set CYCLIC_PREP -> device_issue_pending() -> > Pl330_tasklet() with CYCLIC_PREP -> set CYCLIC_TRIGGER -> PL330 IRQ -> > Pl330_tasklet() with CYCLIC_TRIGGER. "device_issue_pending() ->Pl330_tasklet() with CYCLIC_PREP" is no different from "device_issue_pending() ->Pl330_tasklet() with CYCLIC_TRIGGER" because the list of 'done' xfers would be null yet.
Jassi Brar wrote: > On Tue, Aug 23, 2011 at 12:38 PM, Boojin Kim <boojin.kim@samsung.com> > wrote: > > Jassi Brar [mailto:jassisinghbrar@gmail.com] > >> Sent: Tuesday, August 23, 2011 2:42 PM > >> To: Boojin Kim > >> Cc: linux-arm-kernel@lists.infradead.org; linux-samsung- > >> soc@vger.kernel.org; Vinod Koul; Kukjin Kim; Dan Williams; Mark > Brown; > >> Grant Likely; Russell King > >> Subject: Re: [PATCH v6 04/15] DMA: PL330: Add DMA_CYCLIC capability > >> > >> On Mon, Aug 22, 2011 at 5:33 PM, Boojin Kim > <boojin.kim@samsung.com> > >> wrote: > >> >> > >> >> > +static struct dma_async_tx_descriptor *pl330_prep_dma_cyclic( > >> >> > + struct dma_chan *chan, dma_addr_t dma_addr, > size_t > >> len, > >> >> > + size_t period_len, enum dma_data_direction > >> direction) > >> >> > +{ > >> >> > + struct dma_pl330_desc *desc; > >> >> > + struct dma_pl330_chan *pch = to_pchan(chan); > >> >> > + dma_addr_t dst; > >> >> > + dma_addr_t src; > >> >> > + > >> >> > + desc = pl330_get_desc(pch); > >> >> > + if (!desc) { > >> >> > + dev_err(pch->dmac->pif.dev, "%s:%d Unable to > fetch > >> >> desc\n", > >> >> > + __func__, __LINE__); > >> >> > + return NULL; > >> >> > + } > >> >> > + > >> >> > + switch (direction) { > >> >> > + case DMA_TO_DEVICE: > >> >> > + desc->rqcfg.src_inc = 1; > >> >> > + desc->rqcfg.dst_inc = 0; > >> >> > + src = dma_addr; > >> >> > + dst = pch->fifo_addr; > >> >> > + break; > >> >> > + case DMA_FROM_DEVICE: > >> >> > + desc->rqcfg.src_inc = 0; > >> >> > + desc->rqcfg.dst_inc = 1; > >> >> > + src = pch->fifo_addr; > >> >> > + dst = dma_addr; > >> >> > + break; > >> >> > + default: > >> >> > + dev_err(pch->dmac->pif.dev, "%s:%d Invalid dma > >> >> direction\n", > >> >> > + __func__, __LINE__); > >> >> > + return NULL; > >> >> > + } > >> >> > + > >> >> > + desc->rqcfg.brst_size = pch->burst_sz; > >> >> > + desc->rqcfg.brst_len = 1; > >> >> > + > >> >> > + if (!pch->cyclic) > >> >> > + pch->cyclic = CYCLIC_PREP; > >> >> The need for check here seems suspicious. > >> >> Is it really needed? If not, please remove it. > >> > It's required because Cyclic operation is passed from CYCLIC_PREP > to > >> > CYCLIC_TRIGGER. > >> I don't see why you need to differentiate between PREP and TRIGGER > in > >> cyclic mode. I think, you should simply have 'bool cyclic' instead > of > >> enum. > > On cyclic mode, Pl330_tasklet() operation is changed according to > the value of > > 'cyclic'. > > In case of CYCLIC_PREP, Pl330_tasklet()operation is same with normal > > operation. > By 'normal' you mean non-cyclic, right? That doesn't seem correct to > do. > Once the desc has been marked cyclic by device_prep_dma_cyclic(), you > shouldn't treat it like a non-cyclic anymore. > > > > The sequence is following. > > device_prep_dma_cyclic() -> set CYCLIC_PREP -> device_issue_pending() > -> > > Pl330_tasklet() with CYCLIC_PREP -> set CYCLIC_TRIGGER -> PL330 IRQ > -> > > Pl330_tasklet() with CYCLIC_TRIGGER. > "device_issue_pending() ->Pl330_tasklet() with CYCLIC_PREP" is no > different > from "device_issue_pending() ->Pl330_tasklet() with CYCLIC_TRIGGER" > because the list of 'done' xfers would be null yet. Okay, You're right. CYCLIC_PREP is a redundancy status. I will address your comment. Thanks Boojin Kim
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c index 59943ec..3a0baac 100644 --- a/drivers/dma/pl330.c +++ b/drivers/dma/pl330.c @@ -43,6 +43,12 @@ enum desc_status { DONE, }; +enum cyclic_mode { + NO_CYCLIC, + CYCLIC_PREP, + CYCLIC_TRIGGER, +}; + struct dma_pl330_chan { /* Schedule desc completion */ struct tasklet_struct task; @@ -75,6 +81,9 @@ struct dma_pl330_chan { int burst_sz; /* the peripheral fifo width */ int burst_len; /* the number of burst */ dma_addr_t fifo_addr; + + /* for cyclic capability */ + enum cyclic_mode cyclic; }; struct dma_pl330_dmac { @@ -161,6 +170,31 @@ static inline void free_desc_list(struct list_head *list) spin_unlock_irqrestore(&pdmac->pool_lock, flags); } +static inline void handle_cyclic_desc_list(struct list_head *list) +{ + struct dma_pl330_desc *desc; + struct dma_pl330_chan *pch; + unsigned long flags; + + if (list_empty(list)) + return; + + list_for_each_entry(desc, list, node) { + dma_async_tx_callback callback; + + /* Change status to reload it */ + desc->status = PREP; + pch = desc->pchan; + callback = desc->txd.callback; + if (callback) + callback(desc->txd.callback_param); + } + + spin_lock_irqsave(&pch->lock, flags); + list_splice_tail_init(list, &pch->work_list); + spin_unlock_irqrestore(&pch->lock, flags); +} + static inline void fill_queue(struct dma_pl330_chan *pch) { struct dma_pl330_desc *desc; @@ -214,7 +248,10 @@ static void pl330_tasklet(unsigned long data) spin_unlock_irqrestore(&pch->lock, flags); - free_desc_list(&list); + if (pch->cyclic == CYCLIC_TRIGGER) + handle_cyclic_desc_list(&list); + else + free_desc_list(&list); } static void dma_pl330_rqcb(void *token, enum pl330_op_err err) @@ -245,6 +282,7 @@ static int pl330_alloc_chan_resources(struct dma_chan *chan) spin_lock_irqsave(&pch->lock, flags); pch->completed = chan->cookie = 1; + pch->cyclic = NO_CYCLIC; pch->pl330_chid = pl330_request_channel(&pdmac->pif); if (!pch->pl330_chid) { @@ -324,6 +362,9 @@ static void pl330_free_chan_resources(struct dma_chan *chan) pl330_release_channel(pch->pl330_chid); pch->pl330_chid = NULL; + if (pch->cyclic) + list_splice_tail_init(&pch->work_list, &pch->dmac->desc_pool); + spin_unlock_irqrestore(&pch->lock, flags); } @@ -347,7 +388,11 @@ pl330_tx_status(struct dma_chan *chan, dma_cookie_t cookie, static void pl330_issue_pending(struct dma_chan *chan) { - pl330_tasklet((unsigned long) to_pchan(chan)); + struct dma_pl330_chan *pch = to_pchan(chan); + pl330_tasklet((unsigned long) pch); + + if (pch->cyclic == CYCLIC_PREP) + pch->cyclic = CYCLIC_TRIGGER; } /* @@ -560,6 +605,52 @@ static inline int get_burst_len(struct dma_pl330_desc *desc, size_t len) return burst_len; } +static struct dma_async_tx_descriptor *pl330_prep_dma_cyclic( + struct dma_chan *chan, dma_addr_t dma_addr, size_t len, + size_t period_len, enum dma_data_direction direction) +{ + struct dma_pl330_desc *desc; + struct dma_pl330_chan *pch = to_pchan(chan); + dma_addr_t dst; + dma_addr_t src; + + desc = pl330_get_desc(pch); + if (!desc) { + dev_err(pch->dmac->pif.dev, "%s:%d Unable to fetch desc\n", + __func__, __LINE__); + return NULL; + } + + switch (direction) { + case DMA_TO_DEVICE: + desc->rqcfg.src_inc = 1; + desc->rqcfg.dst_inc = 0; + src = dma_addr; + dst = pch->fifo_addr; + break; + case DMA_FROM_DEVICE: + desc->rqcfg.src_inc = 0; + desc->rqcfg.dst_inc = 1; + src = pch->fifo_addr; + dst = dma_addr; + break; + default: + dev_err(pch->dmac->pif.dev, "%s:%d Invalid dma direction\n", + __func__, __LINE__); + return NULL; + } + + desc->rqcfg.brst_size = pch->burst_sz; + desc->rqcfg.brst_len = 1; + + if (!pch->cyclic) + pch->cyclic = CYCLIC_PREP; + + fill_px(&desc->px, dst, src, period_len); + + return &desc->txd; +} + static struct dma_async_tx_descriptor * pl330_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dst, dma_addr_t src, size_t len, unsigned long flags) @@ -791,6 +882,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id) case MEMTODEV: case DEVTOMEM: dma_cap_set(DMA_SLAVE, pd->cap_mask); + dma_cap_set(DMA_CYCLIC, pd->cap_mask); break; default: dev_err(&adev->dev, "DEVTODEV Not Supported\n"); @@ -819,6 +911,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id) pd->device_alloc_chan_resources = pl330_alloc_chan_resources; pd->device_free_chan_resources = pl330_free_chan_resources; pd->device_prep_dma_memcpy = pl330_prep_dma_memcpy; + pd->device_prep_dma_cyclic = pl330_prep_dma_cyclic; pd->device_tx_status = pl330_tx_status; pd->device_prep_slave_sg = pl330_prep_slave_sg; pd->device_control = pl330_control;