[U-Boot] [RFC v3 03/15] dma: add bcm6348-iudma support

[Grygorii Strashko]

Hi 

I'd appreciated if you can clarify few points below.

On 02/21/2018 10:10 AM, Álvaro Fernández Rojas wrote:
> BCM6348 IUDMA controller is present on multiple BMIPS (BCM63xx) SoCs.
> 
> Signed-off-by: Álvaro Fernández Rojas <noltari at gmail.com>
> ---
>   v3: no changes
>   v2: Fix dma rx burst config and select DMA_CHANNELS.
> 
>   drivers/dma/Kconfig         |   9 +
>   drivers/dma/Makefile        |   1 +
>   drivers/dma/bcm6348-iudma.c | 505 ++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 515 insertions(+)
>   create mode 100644 drivers/dma/bcm6348-iudma.c
>

[...]
 
>
> +static int bcm6348_iudma_enable(struct dma *dma)
> +{
> +	struct bcm6348_iudma_priv *priv = dev_get_priv(dma->dev);
> +	struct bcm6348_chan_priv *ch_priv = priv->ch_priv[dma->id];
> +	struct bcm6348_dma_desc *dma_desc;
> +	uint8_t i;
> +
> +	/* init dma rings */
> +	dma_desc = ch_priv->dma_ring;
> +	for (i = 0; i < ch_priv->dma_ring_size; i++) {
> +		if (bcm6348_iudma_chan_is_rx(dma->id)) {
> +			dma_desc->status = DMAD_ST_OWN_MASK;
> +			dma_desc->length = PKTSIZE_ALIGN;
> +			dma_desc->address = virt_to_phys(net_rx_packets[i]);

You are filling RX queue/ring with buffers defined in Net core.
Does it mean that this DMA driver will not be usable for other purposes, as
Net can be compiled out?

Wouldn't it be reasonable to have some sort of .receive_prepare() callback in
DMA dma_ops, so DMA user can control which buffers to push in RX DMA channel?
And it also can be used in eth_ops.free_pkt() callback (see below).

> +		} else {
> +			dma_desc->status = 0;
> +			dma_desc->length = 0;
> +			dma_desc->address = 0;
> +		}
> +
> +		if (i == ch_priv->dma_ring_size - 1)
> +			dma_desc->status |= DMAD_ST_WRAP_MASK;
> +
> +		if (bcm6348_iudma_chan_is_rx(dma->id))
> +			writel_be(1,
> +				  priv->base + DMA_FLOWC_ALLOC_REG(dma->id));
> +
> +		dma_desc++;
> +	}
> +
> +	/* init to first descriptor */
> +	ch_priv->desc_id = 0;
> +
> +	/* force cache writeback */
> +	flush_dcache_range((ulong)ch_priv->dma_ring,
> +		ALIGN_END_ADDR(struct bcm6348_dma_desc, ch_priv->dma_ring,
> +			       ch_priv->dma_ring_size));
> +
> +	/* clear sram */
> +	writel_be(0, priv->sram + DMAS_STATE_DATA_REG(dma->id));
> +	writel_be(0, priv->sram + DMAS_DESC_LEN_STATUS_REG(dma->id));
> +	writel_be(0, priv->sram + DMAS_DESC_BASE_BUFPTR_REG(dma->id));
> +
> +	/* set dma ring start */
> +	writel_be(virt_to_phys(ch_priv->dma_ring),
> +		  priv->sram + DMAS_RSTART_REG(dma->id));
> +
> +	/* set flow control */
> +	if (bcm6348_iudma_chan_is_rx(dma->id)) {
> +		u32 val;
> +
> +		setbits_be32(priv->base + DMA_CFG_REG,
> +			     DMA_CFG_FLOWC_ENABLE(dma->id));
> +
> +		val = ch_priv->dma_ring_size / 3;
> +		writel_be(val, priv->base + DMA_FLOWC_THR_LO_REG(dma->id));
> +
> +		val = (ch_priv->dma_ring_size * 2) / 3;
> +		writel_be(val, priv->base + DMA_FLOWC_THR_HI_REG(dma->id));
> +	}
> +
> +	/* set dma max burst */
> +	writel_be(ch_priv->dma_ring_size,
> +		  priv->chan + DMAC_BURST_REG(dma->id));
> +
> +	/* clear interrupts */
> +	writel_be(DMAC_IR_DONE_MASK, priv->chan + DMAC_IR_ST_REG(dma->id));
> +	writel_be(0, priv->chan + DMAC_IR_EN_REG(dma->id));
> +
> +	setbits_be32(priv->chan + DMAC_CFG_REG(dma->id), DMAC_CFG_ENABLE_MASK);
> +
> +	return 0;
> +}

[...]

> +
> +static int bcm6348_iudma_receive(struct dma *dma, void **dst)
> +{
> +	struct bcm6348_iudma_priv *priv = dev_get_priv(dma->dev);
> +	struct bcm6348_chan_priv *ch_priv = priv->ch_priv[dma->id];
> +	struct bcm6348_dma_desc *dma_desc;
> +	void __iomem *dma_buff;
> +	uint16_t status;
> +	int ret;
> +
> +	/* get dma ring descriptor address */
> +	dma_desc = ch_priv->dma_ring;
> +	dma_desc += ch_priv->desc_id;
> +
> +	/* invalidate cache data */
> +	invalidate_dcache_range((ulong)dma_desc,
> +		ALIGN_END_ADDR(struct bcm6348_dma_desc, dma_desc, 1));
> +
> +	/* check dma own */
> +	if (dma_desc->status & DMAD_ST_OWN_MASK)
> +		return 0;
> +
> +	/* check dma end */
> +	if (!(dma_desc->status & DMAD_ST_EOP_MASK))
> +		return -EINVAL;
> +
> +	/* get dma buff descriptor address */
> +	dma_buff = phys_to_virt(dma_desc->address);
> +
> +	/* invalidate cache data */
> +	invalidate_dcache_range((ulong)dma_buff,
> +				(ulong)(dma_buff + PKTSIZE_ALIGN));
> +
> +	/* get dma data */
> +	*dst = dma_buff;
> +	ret = dma_desc->length;
> +
> +	/* reinit dma descriptor */
> +	status = dma_desc->status & DMAD_ST_WRAP_MASK;
> +	status |= DMAD_ST_OWN_MASK;
> +
> +	dma_desc->length = PKTSIZE_ALIGN;
> +	dma_desc->status = status;
> +
> +	/* flush cache */
> +	flush_dcache_range((ulong)dma_desc,
> +		ALIGN_END_ADDR(struct bcm6348_dma_desc, dma_desc, 1));

Could you clarify pls, if you do return dma_desc to RX ring here or not?

if yes, wouldn't it cause potential problem on Net RX path 
		ret = eth_get_ops(current)->recv(current, flags, &packet);
^^ (1) here buffer will be received from DMA ( and pushed back to RX ring ?? )

		flags = 0;
		if (ret > 0)
			net_process_received_packet(packet, ret);
^^ (2) here it will be passed in Net stack

		if (ret >= 0 && eth_get_ops(current)->free_pkt)
			eth_get_ops(current)->free_pkt(current, packet, ret);
^^ at this point it should be safe to return buffer in DMA RX ring.

		if (ret <= 0)
			break;

Can DMA overwrite packet after point (1) while packet is still processed (2)?

> +
> +	/* set flow control buffer alloc */
> +	writel_be(1, priv->base + DMA_FLOWC_ALLOC_REG(dma->id));
> +
> +	/* enable dma */
> +	setbits_be32(priv->chan + DMAC_CFG_REG(dma->id), DMAC_CFG_ENABLE_MASK);
> +
> +	/* set interrupt */
> +	writel_be(DMAC_IR_DONE_MASK, priv->chan + DMAC_IR_EN_REG(dma->id));
> +
> +	/* increment dma descriptor */
> +	ch_priv->desc_id = (ch_priv->desc_id + 1) % ch_priv->dma_ring_size;
> +
> +	return ret - 4;
> +}
> +



-- 
regards,
-grygorii