dmaengine Archive on lore.kernel.org
 help / color / Atom feed
From: Jon Hunter <jonathanh@nvidia.com>
To: Dmitry Osipenko <digetx@gmail.com>,
	Peter Ujfalusi <peter.ujfalusi@ti.com>,
	Sameer Pujar <spujar@nvidia.com>, Vinod Koul <vkoul@kernel.org>
Cc: <dan.j.williams@intel.com>, <tiwai@suse.com>,
	<dmaengine@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<sharadg@nvidia.com>, <rlokhande@nvidia.com>,
	<dramesh@nvidia.com>, <mkumard@nvidia.com>,
	linux-tegra <linux-tegra@vger.kernel.org>
Subject: Re: [PATCH] [RFC] dmaengine: add fifo_size member
Date: Mon, 10 Jun 2019 09:01:25 +0100
Message-ID: <4526f63f-8e77-334d-7656-ae1c7bc57d3b@nvidia.com> (raw)
In-Reply-To: <68e72598-8d53-115c-14a2-9d3042165aff@gmail.com>


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

  reply index

Thread overview: 60+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-30 11:30 Sameer Pujar
2019-04-30 11:30 ` [PATCH] " Sameer Pujar
2019-05-02  6:04 ` Vinod Koul
2019-05-02  6:04   ` [PATCH] " Vinod Koul
2019-05-02 10:53   ` Sameer Pujar
2019-05-02 12:25     ` Vinod Koul
2019-05-02 13:29       ` Sameer Pujar
2019-05-03 19:10         ` Peter Ujfalusi
2019-05-04 10:23         ` Vinod Koul
2019-05-06 13:04           ` Sameer Pujar
2019-05-06 15:50             ` Vinod Koul
2019-06-06  3:49               ` Sameer Pujar
2019-06-06  6:00                 ` Peter Ujfalusi
2019-06-06  6:41                   ` Sameer Pujar
2019-06-06  7:14                     ` Jon Hunter
2019-06-06 10:22                       ` Peter Ujfalusi
2019-06-06 10:49                         ` Jon Hunter
2019-06-06 11:54                           ` Peter Ujfalusi
2019-06-06 12:37                             ` Jon Hunter
2019-06-06 13:45                               ` Dmitry Osipenko
2019-06-06 13:55                                 ` Dmitry Osipenko
2019-06-06 14:26                                   ` Jon Hunter
2019-06-06 14:36                                     ` Jon Hunter
2019-06-06 14:36                                     ` Dmitry Osipenko
2019-06-06 14:47                                       ` Jon Hunter
2019-06-06 14:25                                 ` Jon Hunter
2019-06-06 15:18                                   ` Dmitry Osipenko
2019-06-06 16:32                                     ` Jon Hunter
2019-06-06 16:44                                       ` Dmitry Osipenko
2019-06-06 16:53                                         ` Jon Hunter
2019-06-06 17:25                                           ` Dmitry Osipenko
2019-06-06 17:56                                             ` Dmitry Osipenko
2019-06-07  9:24                                             ` Jon Hunter
2019-06-07  5:50                               ` Peter Ujfalusi
2019-06-07  9:18                                 ` Jon Hunter
2019-06-07 10:27                                   ` Jon Hunter
2019-06-07 12:17                                     ` Peter Ujfalusi
2019-06-07 12:58                                       ` Jon Hunter
2019-06-07 13:35                                         ` Peter Ujfalusi
2019-06-07 20:53                                           ` Dmitry Osipenko
2019-06-10  8:01                                             ` Jon Hunter [this message]
2019-06-10  7:59                                           ` Jon Hunter
2019-06-13  4:43                 ` Vinod Koul
2019-06-17  7:07                   ` Sameer Pujar
2019-06-18  4:33                     ` Vinod Koul
2019-06-20 10:29                       ` Sameer Pujar
2019-06-24  6:26                         ` Vinod Koul
2019-06-25  2:57                           ` Sameer Pujar
2019-07-05  6:15                             ` Sameer Pujar
2019-07-15 15:42                               ` Sameer Pujar
2019-07-19  5:04                               ` Vinod Koul
2019-07-23  5:54                                 ` Sameer Pujar
2019-07-29  6:10                                   ` Vinod Koul
2019-07-31  9:48                                     ` Jon Hunter
2019-07-31 15:16                                       ` Vinod Koul
2019-08-02  8:51                                         ` Jon Hunter
2019-08-08 12:38                                           ` Vinod Koul
2019-08-19 15:56                                             ` Jon Hunter
2019-08-20 11:05                                               ` Vinod Koul
2019-09-16  9:02                                                 ` Sameer Pujar

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4526f63f-8e77-334d-7656-ae1c7bc57d3b@nvidia.com \
    --to=jonathanh@nvidia.com \
    --cc=dan.j.williams@intel.com \
    --cc=digetx@gmail.com \
    --cc=dmaengine@vger.kernel.org \
    --cc=dramesh@nvidia.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-tegra@vger.kernel.org \
    --cc=mkumard@nvidia.com \
    --cc=peter.ujfalusi@ti.com \
    --cc=rlokhande@nvidia.com \
    --cc=sharadg@nvidia.com \
    --cc=spujar@nvidia.com \
    --cc=tiwai@suse.com \
    --cc=vkoul@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

dmaengine Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dmaengine/0 dmaengine/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dmaengine dmaengine/ https://lore.kernel.org/dmaengine \
		dmaengine@vger.kernel.org dmaengine@archiver.kernel.org
	public-inbox-index dmaengine


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.dmaengine


AGPL code for this site: git clone https://public-inbox.org/ public-inbox