Hi Russell, On Mon, Oct 12, 2009 at 3:23 PM, Russell King - ARM Linux <li...@arm.linux.org.uk> wrote: > On Fri, Sep 04, 2009 at 12:04:08AM +0530, S, Venkatraman wrote: >> +static int dma_caps0_status; >> + >> +struct omap_dma_list_config_params { >> + int chid; > > This seems to be unused. > >> + int num_elem; > > This might want to be unsigned. > >> + struct omap_dma_sglist_node *sghead; >> + int sgheadphy; > > This should be dma_addr_t. > >> + int pausenode; > > Should this also be unsigned? (If so, the 'pauseafter' parameter should > be as well. > >> +}; > > >> +int omap_request_dma_sglist(int dev_id, const char *dev_name, >> + void (*callback) (int channel_id, u16 ch_status, void *data), >> + int *listid, int nelem, struct omap_dma_sglist_node **elems) >> +{ >> + struct omap_dma_list_config_params *lcfg; >> + struct omap_dma_sglist_node *desc; >> + int dma_lch; >> + int rc, i; >> + >> + if (unlikely((dma_caps0_status & DMA_CAPS_SGLIST_SUPPORT) == 0)) { >> + printk(KERN_ERR "omap DMA: sglist feature not supported\n"); >> + return -EPERM; >> + } >> + if (unlikely(nelem <= 2)) { >> + printk(KERN_ERR "omap DMA: Need >2 elements in the list\n"); >> + return -EINVAL; >> + } > > I'm not sure these unlikely()'s help for two reasons: > 1. gcc probably has already worked that out (later versions have become > quite a bit better at this.) > 2. since we're calling allocation functions, this isn't a hot path, so > the few cycles the above _may_ save really isn't worth it > >> + rc = omap_request_dma(dev_id, dev_name, >> + callback, NULL, &dma_lch); >> + if (rc < 0) { >> + printk(KERN_ERR "omap_dma_list: Request failed %d\n", rc); > > You use a prefix of "omap DMA" above, but "omap_dma_list" here - it would > be nice to be consistent. > >> + return rc; >> + } >> + *listid = dma_lch; >> + dma_chan[dma_lch].state = DMA_CH_NOTSTARTED; >> + lcfg = kmalloc(sizeof(*lcfg), GFP_KERNEL); >> + if (NULL == lcfg) >> + goto error1; >> + dma_chan[dma_lch].list_config = lcfg; >> + >> + lcfg->num_elem = nelem; >> + >> + *elems = desc = lcfg->sghead = dma_alloc_coherent(NULL, >> + sizeof(*desc), &(lcfg->sgheadphy), 0); >> + if (NULL == desc) >> + goto error1; >> + >> + for (i = 1; i < nelem; i++) { >> + desc->next = dma_alloc_coherent(NULL, >> + sizeof(*(desc->next)), &(desc->next_desc_add_ptr), >> 0); >> + if (NULL == desc->next) >> + goto error1; >> + desc = desc->next; >> + desc->next = NULL; >> + } > > The smallest allocation unit available from dma_alloc_coherent() is one > page. You really don't want to allocate one page per descriptor. > Instead, you want to do this: > > lcfg->sghead = dma_alloc_coherent(/*some device*/, > sizeof(*desc) * nelem, &lcfg->sgheadphy, 0); > if (!lcfg->sghead) > goto error1; > > *elems = desc = lcfg->sghead; > desc_dma = lcfg->sgheadphy; > for (i = 1; i < nelem; i++, desc++) { > desc->next = desc + 1; > desc->next_desc_add_ptr = desc_dma + sizeof(*desc); > } > > This might also mean that you can get rid of the desc->next walking > and just use a plain for loop. > > Also, using a struct device for the device actually doing the DMA would > be a Good Thing - this becomes much more relevent when we want to be > able to allocate coherent DMA pages from highmem (since passing NULL > means we have to allocate from the low DMA region.) > >> + desc->next_desc_add_ptr = OMAP_DMA_INVALID_DESCRIPTOR_POINTER; >> + >> + dma_write(0, CCDN(dma_lch)); /* Reset List index numbering */ >> + /* Initialize frame and element counters to invalid values */ >> + dma_write(OMAP_DMA_INVALID_FRAME_COUNT, CCFN(dma_lch)); >> + dma_write(OMAP_DMA_INVALID_ELEM_COUNT, CCEN(dma_lch)); >> + return 0; >> + >> +error1: >> + omap_release_dma_sglist(dma_lch); >> + return -ENOMEM; >> + >> +} >> +EXPORT_SYMBOL(omap_request_dma_sglist); >> + >> +/* The client can choose to not preconfigure the DMA registers >> + * In fast mode,the DMA controller uses the first element in the list to >> + * program the registers first, and then starts the transfer >> + */ >> + >> +int omap_set_dma_sglist_params(const int listid, >> + struct omap_dma_sglist_node *sghead, >> + struct omap_dma_channel_params *chparams) >> +{ >> + struct omap_dma_list_config_params *lcfg; >> + struct omap_dma_sglist_node *sgitcurr, *sgitprev; >> + int l = DMA_LIST_CDP_LISTMODE; /* Enable Linked list mode in CDP */ >> + >> + lcfg = dma_chan[listid].list_config; >> + if (lcfg->sghead != sghead) { >> + printk(KERN_ERR "Unknown config pointer passed\n"); >> + return -EPERM; >> + } > > If sghead can't be anything different from lcfg->sghead, what is the > purpose of passing sghead into this function? > >> + >> + if (NULL == chparams) >> + l |= DMA_LIST_CDP_FASTMODE; >> + else >> + omap_set_dma_params(listid, chparams); >> + /* The client can set the dma params and still use fast mode >> + * by using the set fast mode api >> + */ >> + dma_write(l, CDP(listid)); >> + >> + sgitprev = sghead; >> + sgitcurr = sgitprev->next; >> + >> + while (sgitcurr != NULL) { >> + switch (sgitcurr->desc_type) { >> + case OMAP_DMA_SGLIST_DESCRIPTOR_TYPE1: >> + omap_dma_list_set_ntype(sgitprev, 1); >> + break; >> + >> + case OMAP_DMA_SGLIST_DESCRIPTOR_TYPE2a: >> + /* intentional no break */ >> + case OMAP_DMA_SGLIST_DESCRIPTOR_TYPE2b: >> + omap_dma_list_set_ntype(sgitprev, 2); >> + break; >> + >> + case OMAP_DMA_SGLIST_DESCRIPTOR_TYPE3a: >> + /* intentional no break */ >> + case OMAP_DMA_SGLIST_DESCRIPTOR_TYPE3b: >> + omap_dma_list_set_ntype(sgitprev, 3); >> + break; >> + default: >> + return -EINVAL; >> + } >> + if (sgitcurr->flags & OMAP_DMA_LIST_SRC_VALID) >> + sgitprev->num_of_elem |= DMA_LIST_DESC_SRC_VALID; >> + if (sgitcurr->flags & OMAP_DMA_LIST_DST_VALID) >> + sgitprev->num_of_elem |= DMA_LIST_DESC_DST_VALID; >> + if (sgitcurr->flags & OMAP_DMA_LIST_NOTIFY_BLOCK_END) >> + sgitprev->num_of_elem |= DMA_LIST_DESC_BLK_END; >> + >> + sgitprev = sgitcurr; >> + sgitcurr = sgitcurr->next; >> + } > > This becomes: > for (sgprev = lcfg->sghead; > sgcurr = sgprev + 1, sgcurr < lcfg->sghead + lcfg->num_elem; > sgprev = sgcurr) { > switch (sgcurr->desc_type) { > ... > } > } > >> + >> + return 0; >> +} >> +EXPORT_SYMBOL(omap_set_dma_sglist_params); >> + >> +int omap_start_dma_sglist_transfers(const int listid, const int pauseafter) > > 'const' for non-pointer function parameters doesn't have a benefit for > code generation - the only thing it does is issue a warning if you write > to them. Are they serving a special purpose here? > >> +{ >> + struct omap_dma_list_config_params *lcfg; >> + struct omap_dma_sglist_node *sgn; >> + unsigned int l, type_id; >> + >> + lcfg = dma_chan[listid].list_config; >> + lcfg->pausenode = 0; >> + sgn = lcfg->sghead; >> + if (pauseafter > 0 && pauseafter <= lcfg->num_elem) { >> + for (l = 0; l < pauseafter; l++) >> + sgn = sgn->next; > > Are you sure this code is correct? Two problems: > > 1. If 'pauseafter' is 1, sgn is the second entry in the list, not the > first, so you can't pause after the first descriptor. > > 2. If 'pauseafter' is 'nelem' then sgn will be NULL, resulting in an oops. > >> + sgn->next_desc_add_ptr |= DMA_LIST_DESC_PAUSE; > > Assuming you have an out-by-one error above, this becomes: > sgn[pauseafter].next_desc_add_ptr |= DMA_LIST_DESC_PAUSE; > >> + lcfg->pausenode = pauseafter; >> + } >> + >> + /* Program the head descriptor's properties into CDP */ >> + switch (lcfg->sghead->desc_type) { >> + case OMAP_DMA_SGLIST_DESCRIPTOR_TYPE1: >> + type_id = DMA_LIST_CDP_TYPE1; >> + break; >> + case OMAP_DMA_SGLIST_DESCRIPTOR_TYPE2a: >> + case OMAP_DMA_SGLIST_DESCRIPTOR_TYPE2b: >> + type_id = DMA_LIST_CDP_TYPE2; >> + break; >> + case OMAP_DMA_SGLIST_DESCRIPTOR_TYPE3a: >> + case OMAP_DMA_SGLIST_DESCRIPTOR_TYPE3b: >> + type_id = DMA_LIST_CDP_TYPE3; >> + break; >> + default: >> + return -EINVAL; >> + } >> + >> + l = dma_read(CDP(listid)); >> + l |= type_id; >> + if (lcfg->sghead->flags & OMAP_DMA_LIST_SRC_VALID) >> + l |= DMA_LIST_CDP_SRC_VALID; >> + if (lcfg->sghead->flags & OMAP_DMA_LIST_DST_VALID) >> + l |= DMA_LIST_CDP_DST_VALID; >> + >> + dma_write(l, CDP(listid)); >> + >> + dma_write((lcfg->sgheadphy), CNDP(listid)); >> + printk(KERN_DEBUG "Start list transfer for list %x\n", >> + lcfg->sgheadphy); >> + omap_start_dma(listid); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL(omap_start_dma_sglist_transfers); >> + >> +int omap_resume_dma_sglist_transfers(const int listid, const int pauseafter) > > Same comment as above wrt 'const' parameters. > >> +{ >> + struct omap_dma_list_config_params *lcfg; >> + struct omap_dma_sglist_node *sgn; >> + int l; >> + >> + lcfg = dma_chan[listid].list_config; >> + sgn = lcfg->sghead; >> + /* Clear the previous pause, if any */ >> + lcfg->pausenode = 0; >> + >> + if (pauseafter > 0 && pauseafter <= lcfg->num_elem) { >> + for (l = 0; l < pauseafter; l++) >> + sgn = sgn->next; >> + sgn->next_desc_add_ptr |= DMA_LIST_DESC_PAUSE; > > Same comments as per omap_start_dma_sglist_transfers(). > >> + lcfg->pausenode = pauseafter; >> + } > > Incidentally, shouldn't this be an inline function or something like that, > since this seems to be a duplication of what's happening in > omap_start_dma_sglist_transfers() ? > >> + >> + /* Clear pause bit in CDP */ >> + l = dma_read(CDP(listid)); >> + printk(KERN_DEBUG "Resuming after pause: CDP=%x\n", l); >> + l &= ~(DMA_LIST_CDP_PAUSEMODE); >> + dma_write(l, CDP(listid)); >> + omap_start_dma(listid); >> + return 0; >> +} >> +EXPORT_SYMBOL(omap_resume_dma_sglist_transfers); >> + >> +int omap_release_dma_sglist(const int listid) >> +{ >> + struct omap_dma_list_config_params *lcfg; >> + struct omap_dma_sglist_node *sgn, *sgn2; >> + >> + lcfg = dma_chan[listid].list_config; >> + sgn = lcfg->sghead; >> + >> + if (NULL != sgn) { >> + sgn = sgn->next; >> + do { >> + sgn2 = sgn->next; >> + dma_free_coherent(NULL, sizeof(*sgn), sgn, >> + sgn->next_desc_add_ptr); >> + sgn = sgn2; >> + } while (sgn2 != NULL); >> + >> + dma_free_coherent(NULL, sizeof(*(lcfg->sghead)), >> + lcfg->sghead, lcfg->sgheadphy); > > As a result of the way these are allocated, this can become the much > simpler: > > dma_free_coherent(/*some device */, > sizeof(*lcfg->sghead) * lcfg->num_elem, > lcfg->sghead, lcfg->sgheadphy); > >> + } >> + if (NULL != dma_chan[listid].list_config) >> + kfree(dma_chan[listid].list_config); >> + dma_chan[listid].list_config = NULL; >> + omap_free_dma(listid); >> + return 0; >> +} >> +EXPORT_SYMBOL(omap_release_dma_sglist); >> + >> +int omap_get_completed_sglist_nodes(const int listid) >> +{ >> + int list_count; >> + >> + list_count = dma_read(CCDN(listid)); >> + return list_count & 0xffff; /* only 16 LSB bits are valid */ >> +} >> +EXPORT_SYMBOL(omap_get_completed_sglist_nodes); >> + >> +int omap_dma_sglist_is_paused(const int listid) >> +{ >> + int list_state; >> + >> + list_state = dma_read(CDP(listid)); >> + return (list_state & DMA_LIST_CDP_PAUSEMODE) ? 1 : 0; >> +} >> +EXPORT_SYMBOL(omap_dma_sglist_is_paused); >> + >> +void omap_dma_set_sglist_fastmode(const int listid, const int fastmode) >> +{ >> + int l = dma_read(CDP(listid)); >> + >> + if (fastmode) >> + l |= DMA_LIST_CDP_FASTMODE; >> + else >> + l &= ~(DMA_LIST_CDP_FASTMODE); >> + dma_write(l, CDP(listid)); >> +} >> +EXPORT_SYMBOL(omap_dma_set_sglist_fastmode); >> + >> >> #ifdef CONFIG_ARCH_OMAP1 >> >> @@ -2367,6 +2668,7 @@ static int __init omap_init_dma(void) >> kfree(dma_chan); >> return -ENOMEM; >> } >> + dma_caps0_status = dma_read(CAPS_0); >> } >> >> if (cpu_is_omap15xx()) { >> @@ -2418,6 +2720,7 @@ static int __init omap_init_dma(void) >> omap_clear_dma(ch); >> dma_chan[ch].dev_id = -1; >> dma_chan[ch].next_lch = -1; >> + dma_chan[ch].list_config = NULL; >> >> if (ch >= 6 && enable_1510_mode) >> continue; >> diff --git a/arch/arm/plat-omap/include/mach/dma.h >> b/arch/arm/plat-omap/include/mach/dma.h >> index 72f680b..ca2970d 100644 >> --- a/arch/arm/plat-omap/include/mach/dma.h >> +++ b/arch/arm/plat-omap/include/mach/dma.h >> @@ -112,8 +112,12 @@ >> #define OMAP1_DMA_COLOR_U(n) (0x40 * (n) + 0x22) >> #define OMAP1_DMA_CCR2(n) (0x40 * (n) + 0x24) >> #define OMAP1_DMA_LCH_CTRL(n) (0x40 * (n) + 0x2a) /* not on >> 15xx */ >> +#define OMAP1_DMA_COLOR(n) 0 >> #define OMAP1_DMA_CCEN(n) 0 >> #define OMAP1_DMA_CCFN(n) 0 >> +#define OMAP1_DMA_CDP(n) 0 >> +#define OMAP1_DMA_CNDP(n) 0 >> +#define OMAP1_DMA_CCDN(n) 0 >> >> /* Channel specific registers only on omap2 */ >> #define OMAP_DMA4_CSSA(n) (0x60 * (n) + 0x9c) >> @@ -132,6 +136,8 @@ >> #define OMAP1_DMA_IRQSTATUS_L0 0 >> #define OMAP1_DMA_IRQENABLE_L0 0 >> #define OMAP1_DMA_OCP_SYSCONFIG 0 >> +#define OMAP1_DMA_CAPS_0 0 >> + >> #define OMAP_DMA4_HW_ID 0 >> #define OMAP_DMA4_CAPS_0_L 0 >> #define OMAP_DMA4_CAPS_0_U 0 >> @@ -576,6 +582,83 @@ struct omap_dma_channel_params { >> #endif >> }; >> >> +struct omap_dma_sglist_type1_params { >> + u32 src_addr; >> + u32 dst_addr; >> + u16 cfn_fn; >> + u16 cicr; >> + u16 dst_elem_idx; >> + u16 src_elem_idx; >> + u32 dst_frame_idx_or_pkt_size; >> + u32 src_frame_idx_or_pkt_size; >> + u32 color; >> + u32 csdp; >> + u32 clnk_ctrl; >> + u32 ccr; >> +}; >> + >> +struct omap_dma_sglist_type2a_params { >> + u32 src_addr; >> + u32 dst_addr; >> + u16 cfn_fn; >> + u16 cicr; >> + u16 dst_elem_idx; >> + u16 src_elem_idx; >> + u32 dst_frame_idx_or_pkt_size; >> + u32 src_frame_idx_or_pkt_size; >> +}; >> + >> +struct omap_dma_sglist_type2b_params { >> + u32 src_or_dest_addr; >> + u16 cfn_fn; >> + u16 cicr; >> + u16 dst_elem_idx; >> + u16 src_elem_idx; >> + u32 dst_frame_idx_or_pkt_size; >> + u32 src_frame_idx_or_pkt_size; >> +}; >> + >> +struct omap_dma_sglist_type3a_params { >> + u32 src_addr; >> + u32 dst_addr; >> +}; >> + >> +struct omap_dma_sglist_type3b_params { >> + u32 src_or_dest_addr; >> +}; >> + >> +enum omap_dma_sglist_descriptor_select { >> + OMAP_DMA_SGLIST_DESCRIPTOR_TYPE1, >> + OMAP_DMA_SGLIST_DESCRIPTOR_TYPE2a, >> + OMAP_DMA_SGLIST_DESCRIPTOR_TYPE2b, >> + OMAP_DMA_SGLIST_DESCRIPTOR_TYPE3a, >> + OMAP_DMA_SGLIST_DESCRIPTOR_TYPE3b, >> +}; >> + >> +union omap_dma_sglist_node_type{ >> + struct omap_dma_sglist_type1_params t1; >> + struct omap_dma_sglist_type2a_params t2a; >> + struct omap_dma_sglist_type2b_params t2b; >> + struct omap_dma_sglist_type3a_params t3a; >> + struct omap_dma_sglist_type3b_params t3b; >> +}; >> + >> +struct omap_dma_sglist_node { >> + >> + /* Common elements for all descriptors */ >> + u32 next_desc_add_ptr; > > For correct typing, this should be: > > dma_addr_t next_desc_add_ptr; > > DMA addresses have type 'dma_addr_t' not u32. > >> + u32 num_of_elem; >> + /* Type specific elements */ >> + union omap_dma_sglist_node_type sg_node; >> + /* Control fields */ >> + int flags; > > Maybe unsigned? > >> + /* Fields that can be set in flags variable */ >> + #define OMAP_DMA_LIST_SRC_VALID (1) >> + #define OMAP_DMA_LIST_DST_VALID (2) >> + #define OMAP_DMA_LIST_NOTIFY_BLOCK_END (4) > > And if you're only using a small number of flags, do you need 32 bits for > them? Maybe something a little smaller like unsigned short? > >> + u32 desc_type; > > Isn't this enum omap_dma_sglist_descriptor_select ? If you correctly > type this, the compiler can check that you're handling all cases in a > switch statement automatically, which means if you add to the enum, > it'll tell you the place you missed. > >> + struct omap_dma_sglist_node *next; >> +}; >> >> extern void omap_set_dma_priority(int lch, int dst_port, int priority); >> extern int omap_request_dma(int dev_id, const char *dev_name, >> @@ -655,6 +738,59 @@ extern int omap_modify_dma_chain_params(int chain_id, >> struct omap_dma_channel_params >> params); >> extern int omap_dma_chain_status(int chain_id); >> #endif >> +/* omap_request_dma_sglist: >> + * Request to setup a DMA channel to transfer in linked list mode of nelem >> + * elements. The memory for the list will be allocated and returned in >> + * elems structure >> + */ >> +extern int omap_request_dma_sglist(int dev_id, const char *dev_name, >> + void (*callback) (int channel_id, u16 ch_status, void *data), >> + int *listid, int nelem, struct omap_dma_sglist_node **elems); >> +/* omap_set_dma_sglist_params >> + * Provide the configuration parameters for the sglist channel >> + * sghead should contain a fully populated list of nelems >> + * which completely describe the transfer. chparams, if not NULL, will >> + * set the appropriate parameters directly into the DMA register. >> + * If chparams is NULL, fastmode will be enabled automatically >> + */ >> +extern int omap_set_dma_sglist_params(const int listid, >> + struct omap_dma_sglist_node *sghead, >> + struct omap_dma_channel_params >> *chparams); >> +/* omap_start_dma_sglist_transfers >> + * Starts the linked list based DMA transfer for the specified listid >> + * If no pause is required, -1 is to be set in pauseafter. >> + * Else, the transfer will suspend after pauseafter elements. >> + */ >> +extern int omap_start_dma_sglist_transfers(const int listid, >> + const int pauseafter); >> +/* omap_resume_dma_sglist_transfers >> + * Resumes the previously paused transfer. >> + * Can be again set to pause at pauseafter node of the linked list >> + * The index is absolute (from the head of the list) >> + */ >> +extern int omap_resume_dma_sglist_transfers(const int listid, >> + const int pauseafter); >> +/* omap_release_dma_sglist >> + * Releases the list based DMA channel and the associated list descriptors >> + */ >> +extern int omap_release_dma_sglist(const int listid); >> +/* omap_get_completed_sglist_nodes >> + * Returns the number of completed elements in the linked list >> + * The value is transient if the API is invoked for an ongoing transfer >> + */ >> +int omap_get_completed_sglist_nodes(const int listid); >> +/* omap_dma_sglist_is_paused >> + * Returns non zero if the linked list is currently in pause state >> + */ >> +int omap_dma_sglist_is_paused(const int listid); >> +/* omap_dma_set_sglist_fastmode >> + * Set or clear the fastmode status of the transfer >> + * In fastmode, DMA register settings are updated from the first element >> + * of the linked list, before initiating the tranfer. >> + * In non-fastmode, the first element is used only after completing the >> + * transfer as already configured in the registers >> + */ >> +void omap_dma_set_sglist_fastmode(const int listid, const int fastmode); >> >> /* LCD DMA functions */ >> extern int omap_request_lcd_dma(void (*callback)(u16 status, void *data), >> ---
Apologies for a long break in this thread. Thanks for your detailed review. I have posted a patch with all your comments taken in. Best regards, Venkat. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html