[PATCH v2 1/4] dmaengine: xilinx_dma: populate dma caps properly

[vinod.koul@intel.com (Vinod Koul)]

On Tue, Jan 09, 2018 at 07:36:11AM +0000, Appana Durga Kedareswara Rao wrote:
> Hi,
> 
> 	Thanks for the review... 
> 
> >On Tue, Jan 09, 2018 at 04:48:10AM +0000, Appana Durga Kedareswara Rao
> >wrote:
> >> Hi,
> >>
> >> >On Mon, Jan 08, 2018 at 05:25:01PM +0000, Appana Durga Kedareswara
> >> >Rao
> >> >wrote:
> >> >> Hi,
> >> >>
> >> >> <Snip>
> >> >> >> >> +	xdev->common.dst_addr_widths = BIT(addr_width / 8);
> >> >> >> >> +	xdev->common.src_addr_widths = BIT(addr_width / 8);
> >> >> >> >
> >> >> >> >Do you not support trf of 1byte, 2 bytes, or 4 bytes wide transfers?
> >> >> >> >What is value of addr_width here typically? Usually controllers
> >> >> >> >can support different widths and this is a surprise that you
> >> >> >> >support only one value
> >> >> >>
> >> >> >> Controller supports address width of 32 and 64.
> >> >> >
> >> >> >Then this should have both 32 and 64 values here
> >> >>
> >> >> Address width is configurable parameter at the h/w level.
> >> >> Since this IP is a soft IP user can create a design with either
> >> >> 32-bit or 64-bit address configuration.
> >> >
> >> >and not both right?
> >>
> >> Yes not both at the same time...
> >> Axi dma controller can be configured for either 32-bit or 64-bit address...
> >
> >So my suspicion was correct.  I would suggest you to read up on the
> >documentation again. The src/dst_addr_widths has _nothing_ to do with 32/64
> >bit addresses used.
> >
> >It is the capability of the dma controller to do transfers with data width as 8bits,
> >16 bits, so on. iKey is "data width" and not address type.
> >This typically translates to DMA FIFO configuration of the controller!
> 
> Thanks for the detailed explanation... 

Welcome but I don't understand why you xilinx folks cant wrap your replies,
it is *very* hard to read on screens with 80chars

> I have gone through the spec again controller does supports 1 byte, 2
> byte, 4 byte up to 128 byte transfers.

I think you are talking about length and NOT width

> In order to do variable length transfers user needs to drive a valid value
> to the tkeep strobe signal at the h/w level.

bingo

> And user needs to configure the below parameters c_m_axis_mm2s_tdata_width
> or c_m_axis_s2mm_tdata_width With desired configuration at the h/w level.
> Controller supports data width of 8, 16, 32, 64, 128, 256, 512 and 1,024
> bits (i.e. c_m_axis_mm2s_tdata_width/ c_m_axis_s2mm_tdata_width parameters
> range)

holy cow, did you read that right. and 1024 bit width, how wide is your FIFO

I think you are off the rails here, please get your length and width
concepts right, they are NOT same and refer to different things and cannot be
used interchangably

> 
> At the s/w level currently we are getting c_m_axis_mm2s_tdata_width/ c_m_axis_s2mm_tdata_width
> Configuration as xlnx,datawidth property in the device-tree.
> 
> So proper values for the src/dst_addr width fields should be, datawidth property in bytes.
> Please correct me if I am wrong... 
> 
> Changes looks like below...
> Here width is in bytes based on the h/w configuration... 
> 
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -2411,6 +2411,8 @@ static int xilinx_dma_chan_probe(struct xilinx_dma_device *xdev,
>                 chan->direction = DMA_MEM_TO_DEV;
>                 chan->id = chan_id;
>                 chan->tdest = chan_id;
> +               xdev->common.directions = BIT(DMA_MEM_TO_DEV);
> +               xdev->common.src_addr_widths = BIT(width);
>  
>                 chan->ctrl_offset = XILINX_DMA_MM2S_CTRL_OFFSET;
>                 if (xdev->dma_config->dmatype == XDMA_TYPE_VDMA) {
> @@ -2428,6 +2430,8 @@ static int xilinx_dma_chan_probe(struct xilinx_dma_device *xdev,
>                 chan->direction = DMA_DEV_TO_MEM;
>                 chan->id = chan_id;
>                 chan->tdest = chan_id - xdev->nr_channels;
> +               xdev->common.directions |= BIT(DMA_DEV_TO_MEM);
> +               xdev->common.dst_addr_widths = BIT(width);
>  
>                 chan->ctrl_offset = XILINX_DMA_S2MM_CTRL_OFFSET;
>                 if (xdev->dma_config->dmatype == XDMA_TYPE_VDMA) {
> 
> 
> Regards,
> Kedar.
> 
> >
> >--
> >~Vinod
> --
> To unsubscribe from this list: send the line "unsubscribe dmaengine" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
~Vinod