Re: dmaengine : xilinx_dma two issues

[Michal Simek <michal.simek@xilinx.com>]

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