Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Received-SPF: Pass (protection.outlook.com: domain of xilinx.com designates
 149.199.62.198 as permitted sender) receiver=protection.outlook.com;
 client-ip=149.199.62.198; helo=xsj-pvapexch01.xlnx.xilinx.com;
Envelope-to: git@xilinx.com,
 michal.simek@xilinx.com,
 dave.jiang@intel.com,
 linux-kernel@vger.kernel.org,
 linux-arm-kernel@lists.infradead.org,
 dmaengine@vger.kernel.org,
 vonohr@smaract.com,
 ferlandm@amotus.ca,
 krzk@kernel.org,
 romain.perier@gmail.com,
 matthew.murrian@goctsi.com,
 vkoul@kernel.org,
 dan.j.williams@intel.com,
 pthomas8589@gmail.com,
 lars@metafoo.de
Subject: Re: dmaengine : xilinx_dma two issues
To:     Lars-Peter Clausen <lars@metafoo.de>,
        Michal Simek <michal.simek@xilinx.com>,
        Paul Thomas <pthomas8589@gmail.com>,
        "Radhey Shyam Pandey" <radheys@xilinx.com>
CC:     Dan Williams <dan.j.williams@intel.com>,
        Vinod Koul <vkoul@kernel.org>,
        Matthew Murrian <matthew.murrian@goctsi.com>,
        Romain Perier <romain.perier@gmail.com>,
        Krzysztof Kozlowski <krzk@kernel.org>,
        "Marc Ferland" <ferlandm@amotus.ca>,
        Sebastian von Ohr <vonohr@smaract.com>,
        "dmaengine@vger.kernel.org" <dmaengine@vger.kernel.org>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        "dave.jiang@intel.com" <dave.jiang@intel.com>,
        Shravya Kumbham <shravyak@xilinx.com>, git <git@xilinx.com>
References: <CAD56B7dpewwJVttuk4GAcAsx62HP=vPF9jmxTFNG3Z9RP4u9zA@mail.gmail.com>
 <BY5PR02MB652004976C500CD4EA763047C7D20@BY5PR02MB6520.namprd02.prod.outlook.com>
 <BY5PR02MB6520C9083F072E6907534497C7AE0@BY5PR02MB6520.namprd02.prod.outlook.com>
 <CAD56B7f9D5HnN-rx2QRi4z4HA-bM1=oVpUv6XY35HxBQkAaXmQ@mail.gmail.com>
 <BY5PR02MB6520112ACAD71BD7339BAE89C7AE0@BY5PR02MB6520.namprd02.prod.outlook.com>
 <CAD56B7eUrNYFnV8dhmRE-2RdAA+dix-dYGHAewDutF6B849b0g@mail.gmail.com>
 <d153eb8c-bc55-37b5-2b22-a4f6c6263d38@metafoo.de>
 <8c0dff8b-eac8-f1d9-0ca9-a901a438d6e9@xilinx.com>
 <70a1f7eb-9d32-1e9a-764b-781082292ab3@metafoo.de>
From:   Michal Simek <michal.simek@xilinx.com>
Message-ID: <a6378182-cda5-fbfc-38cf-620cc08ff124@xilinx.com>
Date:   Mon, 11 Jan 2021 17:07:52 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.6.0
MIME-Version: 1.0
In-Reply-To: <70a1f7eb-9d32-1e9a-764b-781082292ab3@metafoo.de>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 8bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Jan 2021 16:08:22.0948
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: c11141e7-f364-4941-9795-08d8b64b1e6f
X-MS-Exchange-CrossTenant-Id: 657af505-d5df-48d0-8300-c31994686c5c
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=657af505-d5df-48d0-8300-c31994686c5c;Ip=[149.199.62.198];Helo=[xsj-pvapexch01.xlnx.xilinx.com]
X-MS-Exchange-CrossTenant-AuthSource: BL2NAM02FT054.eop-nam02.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN8PR02MB5953
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 11. 01. 21 16:33, Lars-Peter Clausen wrote:
> On 1/11/21 10:32 AM, Michal Simek wrote:
>> Hi Lars,
>>
>> On 10. 01. 21 16:43, Lars-Peter Clausen wrote:
>>> On 1/10/21 4:16 PM, Paul Thomas wrote:
>>>> On Fri, Jan 8, 2021 at 1:36 PM Radhey Shyam Pandey
>>>> <radheys@xilinx.com> wrote:
>>>>>> -----Original Message-----
>>>>>> From: Paul Thomas <pthomas8589@gmail.com>
>>>>>> Sent: Friday, January 8, 2021 9:27 PM
>>>>>> To: Radhey Shyam Pandey <radheys@xilinx.com>
>>>>>> Cc: Dan Williams <dan.j.williams@intel.com>; Vinod Koul
>>>>>> <vkoul@kernel.org>; Michal Simek <michals@xilinx.com>; Matthew
>>>>>> Murrian
>>>>>> <matthew.murrian@goctsi.com>; Romain Perier
>>>>>> <romain.perier@gmail.com>; Krzysztof Kozlowski <krzk@kernel.org>;
>>>>>> Marc
>>>>>> Ferland <ferlandm@amotus.ca>; Sebastian von Ohr
>>>>>> <vonohr@smaract.com>; dmaengine@vger.kernel.org; Linux ARM <linux-
>>>>>> arm-kernel@lists.infradead.org>; linux-kernel <linux-
>>>>>> kernel@vger.kernel.org>; dave.jiang@intel.com; Shravya Kumbham
>>>>>> <shravyak@xilinx.com>; git <git@xilinx.com>
>>>>>> Subject: Re: dmaengine : xilinx_dma two issues
>>>>>>
>>>>>> Hi All,
>>>>>>
>>>>>> On Fri, Jan 8, 2021 at 2:13 AM Radhey Shyam Pandey
>>>>>> <radheys@xilinx.com>
>>>>>> wrote:
>>>>>>>> -----Original Message-----
>>>>>>>> From: Radhey Shyam Pandey
>>>>>>>> Sent: Monday, January 4, 2021 10:50 AM
>>>>>>>> To: Paul Thomas <pthomas8589@gmail.com>; Dan Williams
>>>>>>>> <dan.j.williams@intel.com>; Vinod Koul <vkoul@kernel.org>; Michal
>>>>>>>> Simek <michals@xilinx.com>; Matthew Murrian
>>>>>>>> <matthew.murrian@goctsi.com>; Romain Perier
>>>>>>>> <romain.perier@gmail.com>; Krzysztof Kozlowski <krzk@kernel.org>;
>>>>>>>> Marc Ferland <ferlandm@amotus.ca>; Sebastian von Ohr
>>>>>>>> <vonohr@smaract.com>; dmaengine@vger.kernel.org; Linux ARM <linux-
>>>>>>>> arm-kernel@lists.infradead.org>; linux-kernel <linux-
>>>>>>>> kernel@vger.kernel.org>; Shravya Kumbham <shravyak@xilinx.com>; git
>>>>>>>> <git@xilinx.com>
>>>>>>>> Subject: RE: dmaengine : xilinx_dma two issues
>>>>>>>>
>>>>>>>>> -----Original Message-----
>>>>>>>>> From: Paul Thomas <pthomas8589@gmail.com>
>>>>>>>>> Sent: Monday, December 28, 2020 10:14 AM
>>>>>>>>> To: Dan Williams <dan.j.williams@intel.com>; Vinod Koul
>>>>>>>>> <vkoul@kernel.org>; Michal Simek <michals@xilinx.com>; Radhey
>>>>>>>>> Shyam Pandey <radheys@xilinx.com>; Matthew Murrian
>>>>>>>>> <matthew.murrian@goctsi.com>; Romain Perier
>>>>>>>> <romain.perier@gmail.com>;
>>>>>>>>> Krzysztof Kozlowski <krzk@kernel.org>; Marc Ferland
>>>>>>>>> <ferlandm@amotus.ca>; Sebastian von Ohr <vonohr@smaract.com>;
>>>>>>>>> dmaengine@vger.kernel.org; Linux ARM <linux-
>>>>>>>>> arm-kernel@lists.infradead.org>; linux-kernel <linux-
>>>>>>>>> kernel@vger.kernel.org>
>>>>>>>>> Subject: dmaengine : xilinx_dma two issues
>>>>>>>>>
>>>>>>>>> Hello,
>>>>>>>>>
>>>>>>>>> I'm trying to get the 5.10 kernel up and running for our system,
>>>>>>>>> and I'm running into a couple of issues with xilinx_dma.
>>>>>>>> + (Xilinx mailing list)
>>>>>>>>
>>>>>>>> Thanks for bringing the issues to our notice. Replies inline.
>>>>>>>>
>>>>>>>>> First, commit 14ccf0aab46e 'dmaengine: xilinx_dma: In dma channel
>>>>>>>>> probe fix node order dependency' breaks our usage. Before this
>>>>>>>>> commit a
>>>>>>>> call to:
>>>>>>>>> dma_request_chan(&indio_dev->dev, "axi_dma_0"); returns fine, but
>>>>>>>>> after that commit it returns -19. The reason for this seems to be
>>>>>>>>> that the only channel that is setup is channel 1 (chan->id is 1 in
>>>>>>>> xilinx_dma_chan_probe()).
>>>>>>>>> However in
>>>>>>>>> of_dma_xilinx_xlate() chan_id is gets set to 0 (int chan_id =
>>>>>>>>> dma_spec-
>>>>>>>>>> args[0];), which causes the:
>>>>>>>>> !xdev->chan[chan_id]
>>>>>>>>> test to fail in of_dma_xilinx_xlate()
>>>>>>>> What is the channel number passed in dmaclient DT?
>>>>>> Is this a question for me?
>>>>> Yes, please also share the dmaclient DT client node. Need to see
>>>>> channel number passed to dmas property. Something like below-
>>>>>
>>>>> dmas = <& axi_dma_0 1>
>>>>> dma-names = "axi_dma_0"
>>>> OK, I think I need to revisit this and clean it up some. Currently In
>>>> the driver (a custom iio adc driver) it is hard coded:
>>>> dma_request_chan(&indio_dev->dev, "axi_dma_0");
>>>>
>>>> However, the DT also has the entries (currently unused by the driver):
>>>>           dmas = <&axi_dma_0 0>;
>>>>           dma-names = "axi_dma_0";
>>>>
>>>> I'll go back and clean up our driver to do something like
>>>> adi-axi-adc.c does:
>>>>
>>>>           if (!device_property_present(dev, "dmas"))
>>>>                   return 0;
>>>>
>>>>           if (device_property_read_string(dev, "dma-names", &dma_name))
>>>>                   dma_name = "axi_dma_0";
>>>>
>>>> Should the dmas node get used by the driver? I see the second argument
>>>> is: '0' for write/tx and '1' for read/rx channel. So I should be
>>>> setting this to 1 like this?
>>>>           dmas = <&axi_dma_0 1>;
>>>>           dma-names = "axi_dma_0";
>>>>
>>>> But where does that field get used?
>>> This got broken in "dmaengine: xilinx_dma: In dma channel probe fix node
>>> order dependency"
>>> <https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=14ccf0aab46e1888e2f45b6e995c621c70b32651>.
>>>
>>> Before if there was only one channel that channel was always at index 0.
>>> Regardless of whether the channel was RX or TX. But after that change
>>> the RX channel is always at offset 1, regardless of whether the DMA has
>>> one or two channels. This is a breakage in ABI.
>>>
>>> If you have the choice I'd recommend to not use the Xilinx DMA, it gets
>>> broken pretty much every other release.
>> I expect that you are talking about Xilinx releases and I hope that this
>> has changed over times when most of changes are upstreamed already. The
>> patch above you are referencing has been applied by Vinod and he is
>> checking patches a lot. If there is a problem and any breakage it needs
>> to be fixed. And bugs happen all the time and we have a way how to work
>> with it.
> 
> I don't know if it has gotten better. When I upgrade to a new release
> what takes up most of the time is figuring out why the Xilinx DMA
> doesn't work anymore. Its been like this for years.

Are you saying that upstreaming this driver doesn't improve his quality?
But I would expect when you figured this out you have sent patches to
fix it.


>> If you see there any issue please report them and let's fix them and
>> continue on this topic from technical point of view.
>> In connection to this problem what are you suggesting? Just revert this
>> patch or fix ordering differently? Would be good to provide your
>> suggestion and fix it.
> 
> Reverting would re-introduce the issue the patch was supposed to fix.
> 
> The would have been to use index 0 for the channel if there is only one
> channel. If there are two channels use 0 for TX and 1 for RX.
> 
> The problem is that the change has been around for a while and restoring
> the previous behavior will break users that are expecting the new
> behavior. It is a bit of a catch-22.

Ok. It means we need to find a way how to fix it and don't break
existing users.
I expect there shouldn't be hard to detect if there is only one channel.
If there is just use index 0.

Thanks,
Michal