MrK

@Gunjan Gupta the function calling callback assigned to struct dma_async_tx_descriptor is vchan_complete() and it runs in soft irq context. Yes I can say no single spi device driver should be allowed to mess with dma data and api which is being used by the spi controller driver. Do you see a difference between spi controller driver and spi device ? example: spi-sun6i.c and for example spidev.c, spi controller may or even may not use dma api - that depends on sun6i_spi_can_dma() - which I have mentioned already once upon and any data which comes from dma should not be passed over to a module requesting a transfer to be processed by spi controller. A transfer is described by struct spi_transfer and this is the only data to be exchanged between spi device (e.g. spidev) and spi controller. finally blocking spi controller's function like *_transfer_one(), e.g. sun6i_spi_transfer_one() handles the struct spi_transfer and the transfer is completed with success or error as soon as the *_transfer_one() returns. It is blocking which means it requires kernel thread or userspace thread/process executed in kernel context by means of the the ioctl(), while vchan_complete() runs soft irq context stays for that callbacks are not allowed to sleep, locking critical sections is more advanced topic. this is 1st reason why spi device should not be aware of dma operation of spi controller driver. 2nd reason is what the purpose for spi device being notified twice about completion of transfer request is ? if you don't get it why twice then here is hint: once by return from blocking *_transfer_one() and second time by call to callback from vchan_complete() you suggested. answer is: it is not required and mindless. Overall you exposed such a lack of knowledge of kernel internals like execution contexts, threads vs tasklets, principles of modular software design: api exported, api inherited, even C you know barely - scope of static functions, e.g. static int sun6i_spi_prepare_dma(struct sun6i_spi *sspi, struct spi_transfer *tfr) and execution scope of local (placed on stack) variables, I mean this: struct dma_async_tx_descriptor *rxdesc, *txdesc; from the function above so you posted such a stupid comment that it took me long time to make a decision what should I do and I considered following options: - close the account and not live any reply - live the reply and close account because I don't know how such incompetent person can do a review of someones code who is more proficient in C and kernel internals. okay last but least, Do you think you are the person who knows what does not need to be change and what requires further development ? you pretend to be "mr-i-know-everything" don't you ? if so, why did you want to incorporate patch with dma assigned wrong irq ? didn't you know right dma irq number for h616/618, did you? I didn't know and had to figure this out, that was complex process involving information that spi controller driver timeouts, source of h616 spi controller, h616 dma source, dynamic debugging, thesis that dma irq is not being fired proved with h616 manual and the dtsi. Quite complex process not to be solved by weak minds. 3rd - last, have you heard about dev_warn_ratelimited() ? you haven't likely. and indeed EPROBE_DEFER can cause *probe() to be called many times, no one knows at which frequency. the article you referred to doesn't state anything on frequency. there will be no PRs. @ag123 good luck. you have "brilliant" individuals. and I realized I waste my time over here. it is right time to use settings->delete account now.

it's my 10 PM. it was nice to talk to you. bye

@Stephen Graf yes, I am going to fork mainline and raising PRs. I posted patches right now just maybe for assessment .. so you think it is wrong then ignore them.

@Gunjan Gupta re #1 - consider that logs are created in sun6i_spi_probe() which is called once so that claim about "noise" is weird and it is good to know what the errno was at least this conclusion comes from engineering practice. #2 - no reason to keep it. description about this flag goes like this: was there callback assigned to txdesc ? is there anywhere txdesc->calback = my_doing_ping_function ? why do I need to prove that white is white ? #3. I see. No I do not have anything other with Allwinner than zero 3.

@Gunjan Gupta Honestly, these changes are not much required but the last one lets someone know if e.g. dma feature was enabled in dts properly. so maybe this extended logging has some value. regarding second patch, yes I changed the DMA_PREP_INTERRUPT flag to 0 for txdesc as it is meaningless because its (*callback) filed is not being set. it is NULL by default. Yes, the driver will work no worse without these small changes. I am going to design the support for chip select driven by hw master controller and this will be huge change to a few files. I wonder if I should keep it for myself if these relatively neutral changes are your concern.

Let's return for a moment to spi and dma topic. @pixdrift, @Gunjan Gupta, @Stephen Graf The issue was about dma irq number, the interrupt handler was not triggered thus vchans were not freed and that's why vchan_complete() tasklet was not able to do its job i.e. call registered callback, and in case of spi-sun6i, rx dma descriptor uses callback to release completion and move sun6i_spi_transfer_one() forward. when the callback was not called, the wait_for_completion_timeout(&sspi->dma_rx_done, timeout); used to return due to timeout only. I copied dma section from h6 dts to h616 and did not notice that the irq does not match dma vector number. I can say it works, spi flash is online, spi1 loop works, dma is enabled to both spi controllers. You are welcome to give a try. I attached two patches. 0001 is the same as previous, 0002 fixes dma irq number and this makes massive difference. 0002-orange-pi-zero3-dma-uses-right-interrupt-number.patch 0001-orangepi-zero-3-dts-restored-dma-instance-spi1-confi.patch

okay, here is some more detailed insight into spi and dma .. and the answer is here: if amount of data to transfer is less than size of spi fifo which is 64 bytes then driver falls back into cpu fill path. that's why 0xdeadbeef -> spi-pipe was false positive .. while this fails: root@orangepizero3:~# ./spi-test2.sh ./spi-test2.sh ++ spi-config -d /dev/spidev1.0 -q /dev/spidev1.0: mode=0, lsb=0, bits=8, speed=100000000, spiready=0 +++ ls -l ./spi-test2.sh +++ cut '-d ' -f5-5 ++ BS=138 ++ cat ./spi-test2.sh ++ spi-pipe -b 138 -n 1 -d /dev/spidev1.0 ++ hexdump -C SPI_IOC_MESSAGE: Connection timed out root@orangepizero3:~# dmesg | tail [ 13.387410] dwmac-sun8i 5020000.ethernet eth0: No Safety Features support found [ 13.387421] dwmac-sun8i 5020000.ethernet eth0: No MAC Management Counters available [ 13.387430] dwmac-sun8i 5020000.ethernet eth0: PTP not supported by HW [ 13.387754] dwmac-sun8i 5020000.ethernet eth0: configuring for phy/rgmii-rxid link mode [ 16.452945] dwmac-sun8i 5020000.ethernet eth0: Link is Up - 1Gbps/Full - flow control rx/tx [ 272.324653] spi_master spi1: RX DMA timeout [ 272.324685] spi_master spi1: spi1.0: timeout transferring 138 bytes@100000000Hz for 532(500)ms [ 272.324727] spidev spi1.0: SPI transfer failed: -110 [ 272.324756] spi_master spi1: failed to transfer one message from queue [ 272.324773] spi_master spi1: noqueue transfer failed Conclusion is only one: DMA support in spi-sun6i.c is broken (or damn broken) I will try to pin this ****** thing down and fix it.

sorry for bad news, spi dma looked too good to be true. just like teh wifi, have a look over here: https://github.com/orangepi-xunlong/firmware/issues/1

I reckon it is good idea to play with ioctl() on spi node by myself and try various even freaky configurations. will the spi-sun6i driver survive ?

so overall dma is not ready. spi1 seems to work because of spi-pipe pass and nothing else. I haven't tried anything else by now. spi0 working with real device fails when dmas are enabled.

@Gunjan Gupta I was about committing the change about spi0 dma and decided to double check it. Bad news are that spi0 reports rx dma timeout. So I can not do that. Strange is that dma works for spi1 when tested with spi-pipe it works. I added even a green jumper between miso-mosi to see if loop works and it does. Attached screenshots. root@orangepizero3:~# ./spi-test1.sh ++ spi-config -d /dev/spidev1.0 -q /dev/spidev1.0: mode=0, lsb=0, bits=8, speed=100000000, spiready=0 ++ printf '\xde\xad\xbe\xef' ++ spi-pipe -b4 -n 1 -d /dev/spidev1.0 ++ hexdump -C 00000000 de ad be ef |....| 00000004 root@orangepizero3:~# I tried to upload a screenshot of size of abt 3.3M and it did not work. Is there size limit applied ? Here is the diff: --- a/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi +++ b/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi @@ -625,6 +625,8 @@ spi0: spi@5010000 { status = "disabled"; #address-cells = <1>; #size-cells = <0>; + dmas = <&dma 22>, <&dma 22>; + dma-names = "tx", "rx"; I am sure of that dma channel is right. (double checked 616 user manual, dma section: 3.9) spi-pipe passes two buffers for rx and tx and I don't know how flash driver uses spi driver. for example it may request one way transfer e.g. rx dma after sending flash read block/page command this might be the difference. You are welcome to give a try by yourself and let's see if this is something which happens to me or something "almost there" like the wifi issue. wifi is longer story I found the same assert issue in firmware reported 2 years ago. ok, not this time. Let us know if adding "dmas=" property to spi0 works on your board. And regarding 2nd issue, yes I was sure dtbo was loaded by u-boot and fdt appy did not complain about anything. You may see that my overlay is 858 bytes long while your should be abt. 699 bytes. This because I added "dmas" property to overlay for the 1st time anyway this is not enough, dma channels will stay disabled just like I have explained. Of course I had to add instance of dma device to h616 dtsi otherwise fdt apply will fail and will not link overlay because of unknown reference. However I did not add "dmas=" to spi1 declaration in h616 dtsi. it was present to overlay only. Give a try and I can make a bet you will spi driver complaining that it can't allocate tx and rx dma in kernel log while you have enabled it already in overlay.

@pixdrift No problem. I reckon I can say I am proficient in hw to some extent and I do have still decent tools like the Hp logic analyzer which can bust or confirm some statement. The Hp16702 was top class analyzer in 2000's. It is versatile tool running HP-UX so telling almost everything about its features would make this reply very long so maybe not this time. I am glad you are positive about my contribution so I look forward to doing even more. @ag123 I started with the spi because it seems to be good choice for interfacing fpga to orange pi zero 3. It seems viable to me and I'm familiar with verilog and xilinx xc3/xc6 families a bit. At least I used to be so I may have some good reason to recall it. I will fork soon the armbian as you advised. And thank you. Looking forward. The patch I have posted here is composed of two parts 1. change to h616 dts - I followed sun50i-h6.dtsi 2. change to spi-sun6i.c - the change was result of my work on issue like why adding reference to dma in spi overlay is void and I figured out that of_dma_request_slave_channel() does not care about overlays. Only root dts matters so that's why dma_request_chan() failed and forwarded errno in pointer returned by the of_* function above. so this change is new but the last one. I am going to create more complex spi-sun6i patch in maybe 3 days or so (need to focus on things of certain prio right now) which will allow to switch CS management to spi controller on demand and prepare some proof-of-concept userspace test cases + nice screenshots from the LA (logic analyzer).

Thanks.

@Gunjan Gupta I see. I'm new to armbian so I do everything my own way. Promise I will do better over time.

@Stephen Graf Thank you. I found that no matter which userspace tool is being used I will not see 2x sck period of cs-sck setup and hold times (ref. H616 datasheet, section 5.10.3) due to spi-sun6i.c driver limitations. The spi controller according to H616 user manual, section 9.3 has more features than the existing driver is able to use. I want develop the driver further and preserve legacy modes while allowing these new extensions. An excerpt from spi-sun6i.c: I am sure "we do not want it all the time". In the mean time I re-enabled spi1-dma. It seems to work however I reckon that it is too early to be confident of the dma. I will run more advanced tests once these spi controller extensions not handled by now will be done. This is accomplished in attached patch. @pixdrift Should I fork you repo, commit my changes and create pull request or posting a patch to this forum is good enough ? I think the patch should be added to series.conf if any. Right now I compile kernel out-of-tree (source tree) which is faster than the whole build system and I don't need to use VM because the build system requires root neither I don't use ubuntu by default but other distribution. I noticed that pin-mux complains about some spi mis-configuration. I haven't updated my armbian yet since last time. example: [ 1.530864] sun6i-spi 5010000.spi: Error applying setting, reverse things back [ 1.531168] sun50i-h616-pinctrl 300b000.pinctrl: could not request pin 230 (PH6) from group PH6 on device 300b000.pinctrl [ 1.531178] sun6i-spi 5011000.spi: Error applying setting, reverse things back @ag123 re: 2 interfaces, yes there are two independent instances of spi controller, #0 and #1. #0 is able to work in quad mode while #1 can use single mosi/miso or double rate only. #0 is routed to onboard spi flash in case of e.g. orange pi zero 3. Everything is possible. It depends on board vendor only. For example spi #0 could be routed to connector also so some external device could share sck, miso, mosi with onboard spi flash but the CS. Section 9.3 of 616 user manual gives brief idea on how the spi controllers work and section 4.3 of 616 datasheet is about pin assignment. Looks like spi0 is available on PC only and spi1 on PH. 0001-orangepi-zero-3-dts-restored-dma-instance-spi1-confi.patch