On 07/06/2019 15.58, Jon Hunter wrote:
>> Imho if you can explain it without using 'HACK' in the sentences it
>> might be OK, but it does not feel right.
>
> I don't perceive this as a hack. Although from looking at the
> description of the src/dst_maxburst these are burst size with regard to
> the device, so maybe it is a stretch.
>
>> However since your ADMA and ADMIF is highly coupled and it does needs
>> special maxburst information (burst and allocated FIFO depth) I would
>> rather use src_maxburst/dst_maxburst alone for DEV_TO_MEM/MEM_TO_DEV:
>>
>> ADMA_BURST_SIZE(maxburst) ((maxburst) & 0xff)
>> ADMA_FIFO_SIZE(maxburst) (((maxburst) >> 8) & 0xffffff)
>>
>> So lower 1 byte is the burst value you want from ADMA
>> the other 3 bytes are the allocated FIFO size for the given ADMAIF channel.
>>
>> Sure, you need a header for this to make sure there is no
>> misunderstanding between the two sides.
>
> I don't like this because as I mentioned to Dmitry, the ADMA can perform
> memory-to-memory transfers where such encoding would not be applicable.
mem2mem does not really use dma_slave_config, it is for used by
is_slave_direction() == true type of transfers.
But true, if you use ADMA against anything other than ADMAIF then this
might be not right for non cyclic transfers.
> That does not align with the description in the
> include/linux/dmaengine.h either.
True.
>> Or pass the allocated FIFO size via maxburst and then the ADMA driver
>> will pick a 'good/safe' burst value for it.
>>
>> Or new member, but do you need two of them for src/dst? Probably
>> fifo_depth is better word for it, or allocated_fifo_depth.
>
> Right, so looking at the struct dma_slave_config we have ...
>
> u32 src_maxburst;
> u32 dst_maxburst;
> u32 src_port_window_size;
> u32 dst_port_window_size;
>
> Now if we could make these window sizes a union like the following this
> could work ...
>
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index 8fcdee1c0cf9..851251263527 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -360,8 +360,14 @@ struct dma_slave_config {
> enum dma_slave_buswidth dst_addr_width;
> u32 src_maxburst;
> u32 dst_maxburst;
> - u32 src_port_window_size;
> - u32 dst_port_window_size;
> + union {
> + u32 port_window_size;
> + u32 port_fifo_size;
> + } src;
> + union {
> + u32 port_window_size;
> + u32 port_fifo_size;
> + } dst;
What if in the future someone will have a setup where they would need both?
So not sure. Your problems are coming from a split DMA setup where the
two are highly coupled, but sits in a different place and need to be
configured as one device.
I think xilinx_dma is facing with similar issues and they have a custom
API to set parameters which does not fit or is peripheral specific:
include/linux/dma/xilinx_dma.h
Not sure if that is an acceptable solution.
- Péter
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
