[v2,05/10] dma/ioat: add start and stop functions

  Add start, stop and recover functions for IOAT devices.

Signed-off-by: Conor Walsh <conor.walsh@intel.com>
Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 doc/guides/dmadevs/ioat.rst    |  3 ++
 drivers/dma/ioat/ioat_dmadev.c | 87 ++++++++++++++++++++++++++++++++++
 2 files changed, 90 insertions(+)
  

On 03/09/2021 12:17, Conor Walsh wrote:
> Add start, stop and recover functions for IOAT devices.
>
> Signed-off-by: Conor Walsh<conor.walsh@intel.com>
> Signed-off-by: Bruce Richardson<bruce.richardson@intel.com>
> ---
>   doc/guides/dmadevs/ioat.rst    |  3 ++
>   drivers/dma/ioat/ioat_dmadev.c | 87 ++++++++++++++++++++++++++++++++++
>   2 files changed, 90 insertions(+)
>
> diff --git a/doc/guides/dmadevs/ioat.rst b/doc/guides/dmadevs/ioat.rst
> index b6d88fe966..f7742642b5 100644
> --- a/doc/guides/dmadevs/ioat.rst
> +++ b/doc/guides/dmadevs/ioat.rst
> @@ -86,3 +86,6 @@ The following code shows how the device is configured in ``test_dmadev.c``:
>      :start-after: Setup of the dmadev device. 8<
>      :end-before: >8 End of setup of the dmadev device.
>      :dedent: 1
> +
> +Once configured, the device can then be made ready for use by calling the
> +``rte_dmadev_start()`` API.
> diff --git a/drivers/dma/ioat/ioat_dmadev.c b/drivers/dma/ioat/ioat_dmadev.c
> index 94f9139e0d..9f9feecd49 100644
> --- a/drivers/dma/ioat/ioat_dmadev.c
> +++ b/drivers/dma/ioat/ioat_dmadev.c
> @@ -73,6 +73,91 @@ ioat_vchan_setup(struct rte_dmadev *dev, uint16_t vchan __rte_unused,
>   	return 0;
>   }
>   
> +/* Recover IOAT device. */
> +static inline int
> +__ioat_recover(struct ioat_dmadev *ioat)
> +{
> +	uint32_t chanerr, retry = 0;
> +	uint16_t mask = ioat->qcfg.nb_desc - 1;
> +
> +	/* Clear any channel errors. Reading and writing to chanerr does this. */
> +	chanerr = ioat->regs->chanerr;
> +	ioat->regs->chanerr = chanerr;
> +
> +	/* Reset Channel. */
> +	ioat->regs->chancmd = IOAT_CHANCMD_RESET;
> +
> +	/* Write new chain address to trigger state change. */
> +	ioat->regs->chainaddr = ioat->desc_ring[(ioat->next_read - 1) & mask].next;
> +	/* Ensure channel control and status addr are correct. */
> +	ioat->regs->chanctrl = IOAT_CHANCTRL_ANY_ERR_ABORT_EN |
> +			IOAT_CHANCTRL_ERR_COMPLETION_EN;
> +	ioat->regs->chancmp = ioat->status_addr;
> +
> +	/* Allow HW time to move to the ARMED state. */
> +	do {
> +		rte_pause();
> +		retry++;
> +	} while (ioat->regs->chansts != IOAT_CHANSTS_ARMED && retry < 200);
> +
> +	/* Exit as failure if device is still HALTED. */
> +	if (ioat->regs->chansts != IOAT_CHANSTS_ARMED)
> +		return -1;
> +
> +	/* Store next write as offset as recover will move HW and SW ring out of sync. */
> +	ioat->offset = ioat->next_read;
> +
> +	/* Prime status register with previous address. */
> +	ioat->status = ioat->desc_ring[(ioat->next_read - 2) & mask].next;
> +
> +	return 0;
> +}
> +
> +/* Start a configured device. */
> +static int
> +ioat_dev_start(struct rte_dmadev *dev)
> +{
> +	struct ioat_dmadev *ioat = dev->dev_private;
> +
> +	if (ioat->qcfg.nb_desc == 0 || ioat->desc_ring == NULL)
> +		return -EBUSY;
> +
> +	/* Inform hardware of where the descriptor ring is. */
> +	ioat->regs->chainaddr = ioat->ring_addr;
> +	/* Inform hardware of where to write the status/completions. */
> +	ioat->regs->chancmp = ioat->status_addr;
> +
> +	/* Prime the status register to be set to the last element. */
> +	ioat->status = ioat->ring_addr + ((ioat->qcfg.nb_desc - 1) * DESC_SZ);
> +
> +	printf("IOAT.status: %s [%#lx]\n",
> +			chansts_readable[ioat->status & IOAT_CHANSTS_STATUS],
> +			ioat->status);
> +
> +	if ((ioat->regs->chansts & IOAT_CHANSTS_STATUS) == IOAT_CHANSTS_HALTED) {
> +		IOAT_PMD_WARN("Device HALTED on start, attempting to recover\n");
> +		if (__ioat_recover(ioat) != 0) {
> +			IOAT_PMD_ERR("Device couldn't be recovered");
> +			return -1;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +/* Stop a configured device. */
> +static int
> +ioat_dev_stop(struct rte_dmadev *dev)
> +{
> +	struct ioat_dmadev *ioat = dev->dev_private;
> +
> +	ioat->regs->chancmd = IOAT_CHANCMD_SUSPEND;
> +	/* Allow the device time to suspend itself. */
> +	rte_delay_ms(1);
> +
> +	return 0;

It be more beneficial to check if the device has actually suspended 
before returning. Similar to recover, a timeout could be set by which 
the device is expected to be suspended. If the device is still not 
suspended by then, return error.

IMO this would be more useful to an application that always returning 0, 
since the device may still be active.


With the above addressed,

Reviewed-by: Kevin Laatz <kevin.laatz@intel.com>
  

<snip>

> > +/* Stop a configured device. */
> > +static int
> > +ioat_dev_stop(struct rte_dmadev *dev)
> > +{
> > +	struct ioat_dmadev *ioat = dev->dev_private;
> > +
> > +	ioat->regs->chancmd = IOAT_CHANCMD_SUSPEND;
> > +	/* Allow the device time to suspend itself. */
> > +	rte_delay_ms(1);
> > +
> > +	return 0;
> 
> It be more beneficial to check if the device has actually suspended
> before returning. Similar to recover, a timeout could be set by which
> the device is expected to be suspended. If the device is still not
> suspended by then, return error.
> 
> IMO this would be more useful to an application that always returning 0,
> since the device may still be active.
> 
> 
> With the above addressed,
> 
> Reviewed-by: Kevin Laatz <kevin.laatz@intel.com>

Thanks for the review Kevin, I agree this is a better solution than just delaying and returning 0.
I will implement this and include it as part of v3.
/Conor.