Re: [PATCH v7 09/12] dmaengine: ti: New driver for K3 UDMA

Vinod Koul
2019-12-23T11:26:29Z

On 23-12-19, 10:59, Peter Ujfalusi wrote:

> >> +static void udma_reset_counters(struct udma_chan *uc)
> >> +{
> >> + u32 val;
> >> +
> >> + if (uc->tchan) {
> >> + val = udma_tchanrt_read(uc->tchan, UDMA_TCHAN_RT_BCNT_REG);
> >> + udma_tchanrt_write(uc->tchan, UDMA_TCHAN_RT_BCNT_REG, val);
> >
> > so you read back from UDMA_TCHAN_RT_BCNT_REG and write same value to
> > it??
>
> Yes, that's correct. This is how we can reset it. The counter is
> decremented with the value you have written to the register.

aha, with so many read+write back I would have added a helper.. Not a
big deal though can be updated later

> >> +static struct udma_desc *udma_alloc_tr_desc(struct udma_chan *uc,
> >> + size_t tr_size, int tr_count,
> >> + enum dma_transfer_direction dir)
> >> +{
> >> + struct udma_hwdesc *hwdesc;
> >> + struct cppi5_desc_hdr_t *tr_desc;
> >> + struct udma_desc *d;
> >> + u32 reload_count = 0;
> >> + u32 ring_id;
> >> +
> >> + switch (tr_size) {
> >> + case 16:
> >> + case 32:
> >> + case 64:
> >> + case 128:
> >> + break;
> >> + default:
> >> + dev_err(uc->ud->dev, "Unsupported TR size of %zu\n", tr_size);
> >> + return NULL;
> >> + }
> >> +
> >> + /* We have only one descriptor containing multiple TRs */
> >> + d = kzalloc(sizeof(*d) + sizeof(d->hwdesc[0]), GFP_ATOMIC);
> >
> > this is invoked from prep_ so should use GFP_NOWAIT, we dont use
> > GFP_ATOMIC :)
>
> Ok. btw: EDMA and sDMA driver is using GFP_ATOMIC :o

heh, we made sure to document this bit :)

> >> +static int udma_configure_statictr(struct udma_chan *uc, struct udma_desc *d,
> >> + enum dma_slave_buswidth dev_width,
> >> + u16 elcnt)
> >> +{
> >> + if (uc->ep_type != PSIL_EP_PDMA_XY)
> >> + return 0;
> >> +
> >> + /* Bus width translates to the element size (ES) */
> >> + switch (dev_width) {
> >> + case DMA_SLAVE_BUSWIDTH_1_BYTE:
> >> + d->static_tr.elsize = 0;
> >> + break;
> >> + case DMA_SLAVE_BUSWIDTH_2_BYTES:
> >> + d->static_tr.elsize = 1;
> >> + break;
> >> + case DMA_SLAVE_BUSWIDTH_3_BYTES:
> >> + d->static_tr.elsize = 2;
> >> + break;
> >> + case DMA_SLAVE_BUSWIDTH_4_BYTES:
> >> + d->static_tr.elsize = 3;
> >> + break;
> >> + case DMA_SLAVE_BUSWIDTH_8_BYTES:
> >> + d->static_tr.elsize = 4;
> >
> > seems like ffs(dev_width) to me?
>
> Not really:
> ffs(DMA_SLAVE_BUSWIDTH_1_BYTE) = 1
> ffs(DMA_SLAVE_BUSWIDTH_2_BYTES) = 2
> ffs(DMA_SLAVE_BUSWIDTH_3_BYTES) = 1

I missed this!

> ffs(DMA_SLAVE_BUSWIDTH_4_BYTES) = 3
> ffs(DMA_SLAVE_BUSWIDTH_8_BYTES) = 4

Otherwise you are ffs() - 1

--
~Vinod