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