Re: [PATCH v2 1/2] dmaengine: avalon: Intel Avalon-MM DMA Interface for PCIe

[Alexander Gordeev]

On Tue, Oct 15, 2019 at 04:03:21PM +0530, Vinod Koul wrote:
> what kind of device is this? I dont think we want these and the ones
> coming below as part of kernel kconfig!

Yes, I have already been pointed out on this and will put those as
kernel module parameters in the next version. The device is an IP
block for Intel FPGAs.

> > +static int start_dma_xfer(struct avalon_dma_hw *hw,
> > +			  struct avalon_dma_desc *desc)
> > +{
> > +	size_t ctrl_off;
> > +	struct __dma_desc_table *__table;
> > +	struct dma_desc_table *table;
> > +	u32 rc_src_hi, rc_src_lo;
> > +	u32 ep_dst_lo, ep_dst_hi;
> > +	int last_id, *__last_id;
> > +	int nr_descs;
> > +
> > +	if (desc->direction == DMA_TO_DEVICE) {
> > +		__table = &hw->dma_desc_table_rd;
> > +
> > +		ctrl_off = AVALON_DMA_RD_CTRL_OFFSET;
> > +
> > +		ep_dst_hi = AVALON_DMA_RD_EP_DST_HI;
> > +		ep_dst_lo = AVALON_DMA_RD_EP_DST_LO;
> > +
> > +		__last_id = &hw->h2d_last_id;
> > +	} else if (desc->direction == DMA_FROM_DEVICE) {
> > +		__table = &hw->dma_desc_table_wr;
> > +
> > +		ctrl_off = AVALON_DMA_WR_CTRL_OFFSET;
> > +
> > +		ep_dst_hi = AVALON_DMA_WR_EP_DST_HI;
> > +		ep_dst_lo = AVALON_DMA_WR_EP_DST_LO;
> > +
> > +		__last_id = &hw->d2h_last_id;
> > +	} else {
> > +		BUG();
> > +	}
> > +
> > +	table = __table->cpu_addr;
> > +	memset(&table->flags, 0, sizeof(table->flags));
> > +
> > +	nr_descs = setup_dma_descs(table->descs, desc);
> > +	if (WARN_ON(nr_descs < 1))
> > +		return nr_descs;

(1) Here it may fail.

> > +	last_id = nr_descs - 1;
> > +	*__last_id = last_id;
> > +
> > +	rc_src_hi = __table->dma_addr >> 32;
> > +	rc_src_lo = (u32)__table->dma_addr;
> > +
> > +	start_xfer(hw->regs, ctrl_off,
> > +		   rc_src_hi, rc_src_lo,
> > +		   ep_dst_hi, ep_dst_lo,
> > +		   last_id);
> > +
> > +	return 0;
> 
> why have a return when you always return 0?

Please see (1) above.

> > +static irqreturn_t avalon_dma_interrupt(int irq, void *dev_id)
> > +{
> > +	struct avalon_dma *adma = (struct avalon_dma *)dev_id;
> > +	struct avalon_dma_chan *chan = &adma->chan;
> > +	struct avalon_dma_hw *hw = &chan->hw;
> > +	spinlock_t *lock = &chan->vchan.lock;
> > +	u32 *rd_flags = hw->dma_desc_table_rd.cpu_addr->flags;
> > +	u32 *wr_flags = hw->dma_desc_table_wr.cpu_addr->flags;

(2)

> > +	struct avalon_dma_desc *desc;
> > +	struct virt_dma_desc *vdesc;
> > +	bool rd_done;
> > +	bool wr_done;
> > +
> > +	spin_lock(lock);
> > +
> > +	rd_done = (hw->h2d_last_id < 0);
> > +	wr_done = (hw->d2h_last_id < 0);

(3)

> > +
> > +	if (rd_done && wr_done) {
> > +		spin_unlock(lock);
> > +		return IRQ_NONE;
> > +	}
> > +
> > +	do {
> > +		if (!rd_done && rd_flags[hw->h2d_last_id])
> > +			rd_done = true;
> > +
> > +		if (!wr_done && wr_flags[hw->d2h_last_id])
> > +			wr_done = true;
> > +	} while (!rd_done || !wr_done);
> 
> Can you explain this logic. I dont like busy loop in isr and who updates

Here is the comment in the next version of the patch:

The Intel documentation claims "The Descriptor Controller
writes a 1 to the done bit of the status DWORD to indicate
successful completion. The Descriptor Controller also sends
an MSI interrupt for the final descriptor. After receiving
this MSI, host software can poll the done bit to determine
status."

The above could be read like MSI interrupt might be delivered
before the corresponding done bit is set. But in reality it
does not happen at all (or happens really rare). So put here
the done bit polling, just in case.

> rd_done and rd_flags etc..?

rd_flags/wr_flags are updated by the hardware (2) and mapped in as
coherent DMA memory.

rd_done/wr_done are just local variables (3)

> > +static int __init avalon_pci_probe(struct pci_dev *pci_dev,
> > +				   const struct pci_device_id *id)
> > +{
> > +	void *adma;
> > +	void __iomem *regs;
> > +	int ret;
> > +
> > +	ret = pci_enable_device(pci_dev);
> > +	if (ret)
> > +		goto enable_err;
> > +
> > +	ret = pci_request_regions(pci_dev, DRIVER_NAME);
> > +	if (ret)
> > +		goto reg_err;
> > +
> > +	regs = pci_ioremap_bar(pci_dev, PCI_BAR);
> 
> This is a pci device, so we should really be using the PCI way of
> setting and querying the resources. Doing this thru kernel config
> options is not correct!

I assume you are referring to PCI_BAR in this particular case, right?
If so, this is an excerpt from the documentation that I read like a
BAR the DMA controller is mapped to needs to be configurable:

"When the DMA Descriptor Controller is externally instantiated, these
registers are accessed through a BAR. The offsets must be added to the
base address for the read controller. When the Descriptor Controller
is internally instantiated these registers are accessed through BAR0."

Thank you, Vinod!

> -- 
> ~Vinod