RE: [PATCH 2/4] dmaengine: vdma: Add support for mulit-channel dma mode

Hi Vinod,

On Tue, Jun 21, 2016 at 04:02:05PM +0000, Appana Durga Kedareswara Rao
wrote:

quoted

Hi Vinod,

	Thanks for the review...

quoted

On Fri, Jun 10, 2016 at 02:42:32PM +0530, Kedareswara rao Appana wrote:

quoted

This patch adds support for AXI DMA multi-channel dma mode
Multichannel mode enables DMA to connect to multiple masters And
slaves on the streaming side.
In Multichannel mode AXI DMA supports 2D transfers.

Funny formatting!

Will fix in next version...

quoted

Can you elobrate what you meant by Multichannel mode? This patch
seems to do two things, one is to add interleaved dma support and
something else. Can you explain the latter part?

AXI DMA has two Stream interfaces (Memory to Stream MM2S and Stream to
Memory S2MM)

what is a stream in this context?

Stream means I/O transfer (Memory to I/O and I/O to Memory).
Sorry if I confused you.

quoted

In Multi-Channel dma mode each stream interface can be configured up to 16

channels.

quoted

In Multi-channel DMA mode IP supports only interleaved transfers (2-D

transfers).

quoted

quoted

quoted

 /**
+ * struct xilinx_mcdma_config - DMA Multi channel configuration
+structure
+ * @tdest: Channel to operate on
+ * @tid:   Channel configuration
+ * @tuser: Tuser configuration
+ * @ax_user: ax_user value
+ * @ax_cache: ax_cache value
+ */
+struct xilinx_mcdma_config {
+	u8 tdest;
+	u8 tid;
+	u8 tuser;
+	u8 ax_user;
+	u8 ax_cache;

can you describe these in details, what do these do, what are the
values to be programmed?

As said above In Multi-Channel Mode each Stream interface can be
Configured up to 16 channels each channel is differentiated based on the tdest

and tid values.

Then why are you not registering 16 channels for this? That should give you
channel to operate on!

The number of channels are configurable.
We are registering number of Channels that h/w configured for.

Will fix in the next version. Will remove this config.
And based on the channel type will configure the h/w.

quoted

tdest:
TDEST provides routing information for the data stream.

pls elobrate

Need to configure this with the channel number that
We would like to transfer data.

quoted

TDEST values are static for the entire packet.

what do these mean

We shouldn't modify this value when a transfer is going on...
Providing it in the config option user may not aware of all this possibilities.
Will remove this config and will handle this in the driver itself.

quoted

tid:
Provides a stream identifier. TID values are static for entire packet.
TID values provided in the TX descriptor field are presented on TID
signals of the streaming side.

what is this used for?

Currently I am not using this parameter will remove this config.
But usually it is used as a packet identifier.

quoted

tuser:
Sideband signals used for user-defined information.
TUSER values are static for entire packet. TUSER values provided in
the TX Descriptor field are presented on TUSER signals of streaming side.

what is this used for?

Used for user-defined information.
Currently I am not using it will remove this in the next version...

quoted

ax_user:
Sideband signals used for user-defined information ARUSER values and
their interpretations are user-defined

How is it used by client?

Currently I am not using it will remove this in the next version...

quoted

ax_cache:
Cache type this signal provides additional information about the
cacheable Characteristics of the transfer.

and what are those...

It is a 4-bit filed.
Bit 0: Bufferable
Bit 1: Modifiable
Bit 2: Non-cacheable
Bit 3: Normal

Currently I am not using it will remove this in the next version...

quoted

In the above parameters tdest and tid are mandatory to differentiate
b/w different channels Other parameters are optional please let me
know if you don't want me to Include other optional parameters will remove in

the next version.

dmaengine provides channel as an argument so it is quite odd that you need to
differentiate b/w different channels. This only makes me worry that things are
not standard and fishy

Will fix in the next version will handle
Based on the channel type only.

Regards,
Kedar.

--
~Vinod