> -----Original Message----- > From: svenkatr@xxxxxxxxx [mailto:svenkatr@xxxxxxxxx] On Behalf Of Venkatraman S > Sent: Wednesday, May 05, 2010 5:20 PM > To: Shilimkar, Santosh > Cc: linux-omap@xxxxxxxxxxxxxxx; linux-mmc@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; > Chikkature Rajashekar, Madhusudhan; Adrian Hunter; Tony Lindgren > Subject: Re: [PATCH v8 1/2] sDMA: descriptor autoloading feature > > [Long sections have been trimmed to the context of the discussion] > On Wed, May 5, 2010 at 3:02 PM, Shilimkar, Santosh > <santosh.shilimkar@xxxxxx> wrote: > >> -----Original Message----- > >> +static int dma_sglist_set_phy_params(struct omap_dma_sglist_node *sghead, > >> + dma_addr_t phyaddr, int nelem) > >> +{ > >> + struct omap_dma_sglist_node *sgcurr, *sgprev; > >> + dma_addr_t elem_paddr = phyaddr; > >> + > >> + for (sgprev = sghead; > >> + sgprev < sghead + nelem; > >> + sgprev++) { > >> + > >> + sgcurr = sgprev + 1; > >> + sgprev->next = sgcurr; > >> + elem_paddr += (int)sizeof(*sgcurr); > >> + sgprev->next_desc_add_ptr = elem_paddr; > >> + > >> + switch (sgcurr->desc_type) { > >> + case OMAP_DMA_SGLIST_DESCRIPTOR_TYPE1: > >> + omap_dma_list_set_ntype(sgprev, 1); > >> + break; > >> + > >> + case OMAP_DMA_SGLIST_DESCRIPTOR_TYPE2a: > >> + /* intentional no break */ > >> + case OMAP_DMA_SGLIST_DESCRIPTOR_TYPE2b: > >> + omap_dma_list_set_ntype(sgprev, 2); > >> + break; > >> + > >> + case OMAP_DMA_SGLIST_DESCRIPTOR_TYPE3a: > >> + /* intentional no break */ > >> + case OMAP_DMA_SGLIST_DESCRIPTOR_TYPE3b: > >> + omap_dma_list_set_ntype(sgprev, 3); > >> + break; > >> + > >> + default: > >> + return -EINVAL; > >> + > >> + } > > Are we supporting all the descriptor types. I think only type2a is > > supported. In that case please add FIXME, or WARN message here. > > From DMA perspective, all are supported - no restrictions. Only I have > not figured > out how to use type 2b and type 3b descriptors. It's not the fault of > DMA driver or > specification :-) It's actually upto the client to select the right type. OK. Then the question which I wanted to ask. For TX, 2b should have been better choice than 2a isn't it? > > >> + > >> + lcfg->sghead = sgparams; > >> + lcfg->num_elem = nelem; > >> + lcfg->sgheadphy = padd; > >> + lcfg->pausenode = -1; > >> + > >> + > >> + if (NULL == chparams) > > Minute point really. Better readability "ch_params" > OK > > >> + dma_write(l, CDP(lch)); > >> + dma_write((lcfg->sgheadphy), CNDP(lch)); > >> + /** > >> + * Barrier needed as writes to the > >> + * descriptor memory needs to be flushed > >> + * before it's used by DMA controller > >> + */ > > Little bit of re-wording if you can. > > Also you don't wanted the double ** > > /* > > * Memory barrier is needed because data may still be > > * in the write buffer. The barrier drains write buffers and > > * ensures that DMA sees correct descriptors > > */ > OK > > >> + wmb(); > >> + omap_start_dma(lch); > >> + > >> + /* Maintain the pause state in descriptor */ > >> + omap_set_dma_sglist_pausebit(lcfg, lcfg->pausenode, 0); > >> + omap_set_dma_sglist_pausebit(lcfg, pauseafter, 1); > >> + > >> + /** > >> + * Barrier needed as writes to the > >> + * descriptor memory needs to be flushed > >> + * before it's used by DMA controller > >> + */ > > Description change if possible > OK > > >> + wmb(); > >> + > >> + /* Errata i557 - pausebit should be cleared in no standby mode */ > > This should have been > > I couldn't understand this comment. This should have been a separate patch since it's an errata. > > >> + sys_cf = dma_read(OCP_SYSCONFIG); > >> + l = sys_cf; > >> + /* Middle mode reg set no Standby */ > >> + l &= ~(BIT(12) | BIT(13)); > >> + dma_write(l, OCP_SYSCONFIG); -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
