Re: [PATCH v4 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.
> Each descriptor also carries a status word, and AXI MCDMA carries an
> AXI4-Stream sideband word holding TID, TDEST and TUSER that clients may
> need. Return a pointer to the status word so clients can read the status,
> the sideband and the APP fields together. The exact index layout is
> documented at the function.
> 
> Take the pointer from the End-Of-Frame descriptor, where the hardware
> writes these fields. For AXI DMA the pointer now starts at the status word
> of the EOF descriptor instead of the APP fields of the first descriptor,
> and the payload grows from 20 to 24 bytes. No in-tree consumer is affected,
> since axienet reads the RX frame length from result->residue rather than
> the APP fields.
> 
> Read xlnx,axistream-connected for MCDMA as well, and attach metadata_ops
> in xilinx_mcdma_prep_slave_sg() when an AXI Stream interface is present,
> so MCDMA clients use the metadata API the same way as AXI DMA clients.
> 
> 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>

Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
Thanks!

> ---
> Changes in V4:
>   - Restructured xilinx_dma_get_metadata_ptr(): AXIDMA is now the
>     fall-through path instead of a separate branch guarded by
>     WARN_ON_ONCE()/ERR_PTR().
>   - Rewrote the kernel-doc as an index table covering AXI DMA, MCDMA S2MM
>     and MCDMA MM2S, and documented that the pointer and payload length are
>     the same for both MCDMA directions.
>   - Added an inline comment explaining the union aliasing.
>   - Condensed the commit message.
> 
> 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 | 49 ++++++++++++++++++++++++++++-----
>   1 file changed, 42 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index 1b5b00f08c5f..2be95f0ba3ea 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -651,18 +651,49 @@ 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 hardware writes the status, sideband and APP fields into the last
> + * (End-Of-Frame) descriptor. These words are contiguous, so a client reads
> + * them by index from the returned pointer:
> + *
> + *   AXI DMA:          [0] status,        [1..] app
> + *   AXI MCDMA (S2MM): [0] status,        [1] sideband (TID/TDEST/TUSER), [2..] app
> + *   AXI MCDMA (MM2S): [0] ctrl sideband, [1] status,                     [2..] app
> + *
> + * For MCDMA the pointer and payload length are the same in both directions
> + * because the union members overlay the same descriptor words.
> + *
> + * Return: Pointer to the first metadata word.
>    */
>   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);
>   
> -	*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;
> +		/*
> +		 * The union members overlay the same words, so one pointer and
> +		 * length cover both directions (see the layout above).
> +		 */
> +		*max_len = *payload_len = sizeof(seg->hw.s2mm_status) +
> +					  sizeof(seg->hw.s2mm_sideband_status) +
> +					  sizeof(seg->hw.app);
> +		return &seg->hw.s2mm_status;
> +	}
> +
> +	/* Only AXIDMA and MCDMA attach metadata_ops, so this is AXIDMA. */
> +	struct xilinx_axidma_tx_segment *seg =
> +		list_last_entry(&desc->segments,
> +				struct xilinx_axidma_tx_segment, node);
> +
> +	*max_len = *payload_len = sizeof(seg->hw.status) +
> +				  sizeof(seg->hw.app);
> +	return &seg->hw.status;
>   }
>   
>   static struct dma_descriptor_metadata_ops xilinx_dma_metadata_ops = {
> @@ -2639,6 +2670,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 +3321,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");
>   	}