Re: [PATCH V3 4/4] dmaengine: xilinx_dma: Extend metadata handling for AXI DMA and MCDMA

["Pandey, Radhey Shyam" <radheys@amd.com>]

> From: Suraj Gupta <suraj.gupta2@amd.com>
> 
> xilinx_dma_get_metadata_ptr() exposed only the descriptor APP fields. Both
> AXI DMA and AXI MCDMA descriptors carry a status word with the transfer
> status, and AXI MCDMA additionally carries an AXI4-Stream sideband status
> word holding TID, TDEST and TUSER that clients may need. Extend the
> metadata handling to expose these.
> 
> The returned pointer now starts at the descriptor status word for both AXI
> DMA and AXI MCDMA. As the descriptor words are contiguous, the client sees
> the status at index 0, followed for AXI MCDMA by the sideband status
> (TID/TDEST/TUSER) at index 1 and the APP fields, or for AXI DMA by the APP
> fields directly. The payload length is derived from the field sizes.
> 
> This changes the get_metadata_ptr() contract for AXI DMA. The pointer now
> starts at the status word of the last (EOF) descriptor instead of the APP
> fields of the first, and the payload grows from 20 to 24 bytes. A client
> reading app[0] now reads the status word. No in-tree consumer is affected,
> as the axienet driver reads the RX frame length from result->residue
> rather than the APP fields. Reading the EOF descriptor is also correct, as
> the hardware writes the status and APP fields there.
> 
> The index 0 and 1 layout described above is for the AXI MCDMA receive
> (S2MM) direction, which is where metadata is consumed. On the transmit
> (MM2S) direction the same descriptor words hold different fields.
> 
> The probe logic is extended to read xlnx,axistream-connected for MCDMA, and
> xilinx_mcdma_prep_slave_sg() attaches metadata_ops when an AXI Stream
> interface is present, so MCDMA clients can use the metadata API in the same
> way as AXI DMA clients.

Nit - make the commit description precise.>
> Signed-off-by: Suraj Gupta <suraj.gupta2@amd.com>
> Co-developed-by: Srinivas Neeli <srinivas.neeli@amd.com>
> Signed-off-by: Srinivas Neeli <srinivas.neeli@amd.com>
> ---
> Changes in V3:
>   - Renamed subject to include "AXI DMA and MCDMA" (was "AXI MCDMA" only).
>   - Complete rewrite of commit message and implementation.
>   - Metadata pointer now returns status field at index 0 instead of APP
>     fields, exposing status and sideband information to clients.
>   - Changed from list_first_entry to list_last_entry to return the EOF
>     descriptor where hardware writes status and APP fields.
>   - Added explicit handling for both AXIDMA and MCDMA types with proper
>     payload length calculation.
>   - Added WARN_ON_ONCE for unsupported DMA types.
>   - Removed the 'chan' field from struct xilinx_dma_tx_descriptor (was
>     added in V2) as it's no longer needed; channel is obtained from
>     tx->chan instead.
>   - Dropped V2 patches 4/5 (dt-bindings xlnx,include-stscntrl-strm) and
>     5/5 (xferred_bytes support) as the approach changed to use residue.
> 
> Changes in V2:
>   - Added support for MCDMA metadata handling alongside AXIDMA.
>   - Added 'chan' field to struct xilinx_dma_tx_descriptor.
> ---
>   drivers/dma/xilinx/xilinx_dma.c | 48 ++++++++++++++++++++++++++++-----
>   1 file changed, 41 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index 1b5b00f08c5f..f5c4e0ca2cc4 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -651,18 +651,48 @@ static inline void xilinx_aximcdma_buf(struct xilinx_dma_chan *chan,
>    * @tx: async transaction descriptor
>    * @payload_len: metadata payload length
>    * @max_len: metadata max length
> - * Return: The app field pointer.
> + *
> + * The returned pointer starts at the descriptor status word for both AXI DMA
> + * and AXI MCDMA. As the descriptor words are contiguous, the client sees the
> + * status at index 0, followed for AXI MCDMA by the sideband status
> + * (TID/TDEST/TUSER) at index 1 and the APP fields from index 2, or for AXI DMA
> + * by the APP fields from index 1. These fields are populated by the hardware on
> + * the End-Of-Frame descriptor, so the pointer is taken from there.
> + *
> + * Return: Pointer to the descriptor status field.
>    */
>   static void *xilinx_dma_get_metadata_ptr(struct dma_async_tx_descriptor *tx,
>   					 size_t *payload_len, size_t *max_len)
>   {
>   	struct xilinx_dma_tx_descriptor *desc = to_dma_tx_descriptor(tx);
> -	struct xilinx_axidma_tx_segment *seg;
> +	struct xilinx_dma_chan *chan = to_xilinx_chan(tx->chan);
> +
> +	if (chan->xdev->dma_config->dmatype == XDMA_TYPE_AXIMCDMA) {
> +		struct xilinx_aximcdma_tx_segment *seg =
> +			list_last_entry(&desc->segments,
> +					struct xilinx_aximcdma_tx_segment, node);
> +

This func is common for MCDMA s2mm and mm2s channel. Please make it 
generic to address both.

> +		/* [0] = status, [1] = sideband (TID/TDEST/TUSER), [2..] = app */
> +		*max_len = *payload_len = sizeof(seg->hw.s2mm_status) +
> +					 sizeof(seg->hw.s2mm_sideband_status) +
> +					 sizeof(seg->hw.app);
> +		return &seg->hw.s2mm_status;
> +	}
> +
> +	if (chan->xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA) {
> +		struct xilinx_axidma_tx_segment *seg =
> +			list_last_entry(&desc->segments,
> +					struct xilinx_axidma_tx_segment, node);
> +
> +		/* [0] = status, [1..] = app */
> +		*max_len = *payload_len = sizeof(seg->hw.status) +
> +					 sizeof(seg->hw.app);
> +		return &seg->hw.status;
> +	}
>   
> -	*max_len = *payload_len = sizeof(u32) * XILINX_DMA_NUM_APP_WORDS;
> -	seg = list_first_entry(&desc->segments,
> -			       struct xilinx_axidma_tx_segment, node);
> -	return seg->hw.app;
> +	/* Only AXIDMA and MCDMA attach metadata_ops today. */
> +	WARN_ON_ONCE(1);
> +	return ERR_PTR(-EINVAL);

We are registering metadata ops for MCDMA and AXI DMA so when is this 
error condition going to happen?

>   }
>   
>   static struct dma_descriptor_metadata_ops xilinx_dma_metadata_ops = {
> @@ -2639,6 +2669,9 @@ xilinx_mcdma_prep_slave_sg(struct dma_chan *dchan, struct scatterlist *sgl,
>   		segment->hw.control |= XILINX_MCDMA_BD_EOP;
>   	}
>   
> +	if (chan->xdev->has_axistream_connected)
> +		desc->async_tx.metadata_ops = &xilinx_dma_metadata_ops;
> +
>   	return &desc->async_tx;
>   
>   error:
> @@ -3287,7 +3320,8 @@ static int xilinx_dma_probe(struct platform_device *pdev)
>   
>   	dma_set_max_seg_size(xdev->dev, xdev->max_buffer_len);
>   
> -	if (xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA) {
> +	if (xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA ||
> +	    xdev->dma_config->dmatype == XDMA_TYPE_AXIMCDMA) {
>   		xdev->has_axistream_connected =
>   			of_property_read_bool(node, "xlnx,axistream-connected");
>   	}