dmaengine : xilinx_dma two issues

[Lars-Peter Clausen]

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 at xilinx.com> wrote:
>>>>> -----Original Message-----
>>>>> From: Paul Thomas <pthomas8589 at gmail.com>
>>>>> Sent: Friday, January 8, 2021 9:27 PM
>>>>> To: Radhey Shyam Pandey <radheys at xilinx.com>
>>>>> Cc: Dan Williams <dan.j.williams at intel.com>; Vinod Koul
>>>>> <vkoul at kernel.org>; Michal Simek <michals at xilinx.com>; Matthew Murrian
>>>>> <matthew.murrian at goctsi.com>; Romain Perier
>>>>> <romain.perier at gmail.com>; Krzysztof Kozlowski <krzk at kernel.org>; Marc
>>>>> Ferland <ferlandm at amotus.ca>; Sebastian von Ohr
>>>>> <vonohr at smaract.com>; dmaengine at vger.kernel.org; Linux ARM <linux-
>>>>> arm-kernel at lists.infradead.org>; linux-kernel <linux-
>>>>> kernel at vger.kernel.org>; dave.jiang at intel.com; Shravya Kumbham
>>>>> <shravyak at xilinx.com>; git <git at xilinx.com>
>>>>> Subject: Re: dmaengine : xilinx_dma two issues
>>>>>
>>>>> Hi All,
>>>>>
>>>>> On Fri, Jan 8, 2021 at 2:13 AM Radhey Shyam Pandey <radheys at xilinx.com>
>>>>> wrote:
>>>>>>> -----Original Message-----
>>>>>>> From: Radhey Shyam Pandey
>>>>>>> Sent: Monday, January 4, 2021 10:50 AM
>>>>>>> To: Paul Thomas <pthomas8589 at gmail.com>; Dan Williams
>>>>>>> <dan.j.williams at intel.com>; Vinod Koul <vkoul at kernel.org>; Michal
>>>>>>> Simek <michals at xilinx.com>; Matthew Murrian
>>>>>>> <matthew.murrian at goctsi.com>; Romain Perier
>>>>>>> <romain.perier at gmail.com>; Krzysztof Kozlowski <krzk at kernel.org>;
>>>>>>> Marc Ferland <ferlandm at amotus.ca>; Sebastian von Ohr
>>>>>>> <vonohr at smaract.com>; dmaengine at vger.kernel.org; Linux ARM <linux-
>>>>>>> arm-kernel at lists.infradead.org>; linux-kernel <linux-
>>>>>>> kernel at vger.kernel.org>; Shravya Kumbham <shravyak at xilinx.com>; git
>>>>>>> <git at xilinx.com>
>>>>>>> Subject: RE: dmaengine : xilinx_dma two issues
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Paul Thomas <pthomas8589 at gmail.com>
>>>>>>>> Sent: Monday, December 28, 2020 10:14 AM
>>>>>>>> To: Dan Williams <dan.j.williams at intel.com>; Vinod Koul
>>>>>>>> <vkoul at kernel.org>; Michal Simek <michals at xilinx.com>; Radhey
>>>>>>>> Shyam Pandey <radheys at xilinx.com>; Matthew Murrian
>>>>>>>> <matthew.murrian at goctsi.com>; Romain Perier
>>>>>>> <romain.perier at gmail.com>;
>>>>>>>> Krzysztof Kozlowski <krzk at kernel.org>; Marc Ferland
>>>>>>>> <ferlandm at amotus.ca>; Sebastian von Ohr <vonohr at smaract.com>;
>>>>>>>> dmaengine at vger.kernel.org; Linux ARM <linux-
>>>>>>>> arm-kernel at lists.infradead.org>; linux-kernel <linux-
>>>>>>>> kernel at 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.

> 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.

- Lars