Re: [PATCH] [RFC] dmaengine: add fifo_size member

[Jon Hunter]

On 07/06/2019 21:53, Dmitry Osipenko wrote:
> 07.06.2019 16:35, Peter Ujfalusi ÐÐÑÐÑ:
>>
>>
>> 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.
> 
> If there are no other drivers with the exactly same requirement, then
> the custom API is an a good variant given that there is a precedent
> already. It is always possible to convert to a common thing later on
> since that's all internal to kernel.
> 
> Jon / Sameer, you should check all the other drivers thoroughly to find
> anyone who is doing the same thing as you need in order to achieve
> something that is really common. I'm also wondering if it will be
> possible to make dma_slave_config more flexible in order to start
> accepting vendor specific properties in a somewhat common fashion, maybe
> Vinod and Dan already have some thoughts on it? Apparently there is
> already a need for the customization and people are just starting to
> invent their own thing, but maybe that's fine too. That's really up to
> subsys maintainer to decide in what direction to steer.

I am not a fan of having custom APIs, however, I would agree that
extending the dma_slave_config to allow a DMA specific structure to be
passed with additional configuration would be useful in this case as
well as the Xilinx case.

Cheers
Jon

-- 
nvpublic