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

[Vinod Koul]

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