Re: [PATCH v8 1/2] sDMA: descriptor autoloading feature

[Venkatraman S <svenkatr@ti.com>]

[Long sections have been trimmed to the context of the discussion]
On Wed, May 5, 2010 at 3:02 PM, Shilimkar, Santosh
<santosh.shilimkar@ti.com> 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.

>> +
>> +     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.

>> +     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);