zephyriot / zephyr-issues

0 stars 0 forks source link

SPI API Update #797

Closed nashif closed 7 years ago

nashif commented 8 years ago

Reported by Tomasz Bursztyka:

Current SPI API, defined in include/spi.h, has many known limitations which does not help to enable optimized usage of memory, cpu, ease of use for the developer etc...

The idea is to fix all these limitations: zero-copy r/w buffers, asymmetric buffer handling, cleaner and easier API etc....

I'll send an RFC on the dev ML soon.

(Imported from Jira ZEP-852)

nashif commented 7 years ago

by Qiu Peiyang:

nashif commented 7 years ago

by Qiu Peiyang:

nashif commented 8 years ago

by Tomasz Bursztyka:

RFC sent on ml.

nashif commented 7 years ago

by Mark Linkmeyer:

Tomasz Bursztyka , what's the status of this story? Is it on track to be implemented and merged before the 1.6 merge window closes on Nov 9th?

nashif commented 7 years ago

by Tomasz Bursztyka:

This will have to wait 1.7, at best. QMSI has to get updated too.

nashif commented 7 years ago

by M R Rosen:

One this currently lacking from the SPI API is the ability to manually control the CS line. Some external devices (ex: SD cards) have long transactions that require CS to be held low for long periods of time, and it doesnt make sense to use a single transfer function call for the entire transaction. Thus, having the ability to have manual CS would expand the number of devices the SPI API can access without the need for external GPIO CS. We have already implemented this feature for Zephyr v1.5.0; we can share if I learn how to...

nashif commented 7 years ago

by Baohong Liu:

M R Rosen Do you mean separating the enable/disable of CS from the current API and adding a separate API for chip select? This may be useful for developers or "makers", but, not for production, I think.

nashif commented 7 years ago

by M R Rosen:

Baohong Liu What I mean is extending the current SPI API to allow manual control of the CS, which means something like spi_cs_assert/spi_cs_deassert, allowing people to interface with more devices. The reason is there are plenty of external SPI devices that have certain rules around CS that some SPI controllers do not comply with, forcing users to use GPIO in the place of automatic CS. The current driver model lets the driver author dictate how the CS operates; giving users of the driver no guarantees on how CS behaves. For many SPI devices, this is fine. For many others, CS have to manually controlled via GPIO; which might lead most users to rely on GPIO due to the undefined behavior of the CS. My suggestion is to expand the SPI API to allow both kinds of devices to be used via one API instead of using 2 (SPI + GPIO) to achieve the same task. So, really it comes down to how we want to interface with these devices, in development or in production, it doesnt matter; its usually a limitation of the external SPI device being accessed.

nashif commented 7 years ago

by Tomasz Bursztyka:

Could you give some example of devices requiring specific behavior of CS?

At least in my case I have one (but it's not enough to push for a generalization yet): cc2520 power management. Directly tweaking the CS line is needed to easily switch from one power mode to another (among other things).

My principal concern about exposing such CS access functions, is that it should be very well documented so people don't start to use it all the time, even when it's completely unnecessary. (and it is useless most of the time afaik)

nashif commented 7 years ago

by M R Rosen:

Tomasz Bursztyka We ran into the lack of control over CS when trying to access SD cards over SPI (many smaller SoCs dont have dedicated SD/MMC/SDIO controllers). I am also aware that many others on my team have had trouble handling CS when the driver/controller decides when to assert/deassert the CS; this can be fixed by either mandating CS be asserted during an entire call to any of the SPI functions (read/write/transfer) or by handing over control to the caller. Im more for the former and agree with you that this functionality might be used when it isnt needed (which is probably most of the time), but it still doesnt encompass everything, meaning interfacing with some devices requires manual control over the CS (which can be done via GPIO, but it would be nice if you didnt have to get two devices just to do a single task). I feel like you can make it clear in your documentation the difference between automatic and manual CS modes, but note that in both cases, the behavior does need to be well defined (ie, not just left upto the driver/controllers preference), or youll have everyone using GPIO for CS instead.

nashif commented 7 years ago

by Tomasz Bursztyka:

Ok looks to me you have got the same issue as we had on DW controllers (plus you got hit by current API limitation that does not let you running multiple transactions in one go): on these ones as soon as spi frequency is above 125Khz we get a very flacky CS line and thus transactions are unreliable. In that case, that required to fully hand over the CS line control over a dedicated GPIO line. Current hack to do so is not generic and not flexible but at least: it avoided to expose the CS line control to the caller (see drivers/spi/spi_dw.c or drivers/spi/spi_qmsi.c). Inside the driver, it will do the right CS assertion/deassertion through gpio. It's transparent to the user. I have actually a patch somewhere in on of my local branches to propose a more generic way to let SPI drivers controlling the CS line over a GPIO pin, still without exposing anything to the user.

Because for that kind of issue, I really don't want to expose CS function to the caller. I don't want to see this: spi_cs_assert() spi_write() spi_cs_deassert()

etc...

I already had a discussion on the ML about that, Zephyr being a preemptive OS the reason is all about interrupts and transaction owner. With the pseudo-code above there is nothing that prevents the system to interrupt the caller right after the spi_cs_assert(), schedule another task, which in turn will also start a transaction on the same spi controller and get the time to deassert CS. Once back to the first caller: the spi_write() won't work.

It also makes the API counter intuitive and ugly, exposing very low level SPI features to users that just want to read and/or write and don't want to mess up with gruesome hardware details. If CS line has to be exposed it would be only for very specific case (like my cc2520 one for instance), and not for common transactions.

nashif commented 7 years ago

by M R Rosen:

Tomasz Bursztyka If just having spi_write/read/transceiver was enough to deal with all devices, I was agree that would be the way to go. However, having already encountered situations where the CS lines needs to be controlled manually, it seems like this isnt enough. But I take your point that direct API for CS seems very low level and while I would point out that since we are dealing with a SPI API, not some generic interface API, having CS references I dont feel like is that bad; though at the same time I agree that it doesnt keep with the higher level idea that is the source of all these issues. I would suggest that SPI has, by the way it itself and most device using it interpret the spec (what little of it there is), a notion of a transaction. That is, SPI has the concept of a group of reads and writes all conducted when CS is low. Setting CS low begins a transaction and setting it high again finishes the transaction. Even if it is incomplete, setting CS high ends the transaction so setting it low again will start a whole new transaction (as we are both familiar with from that issue with how the DW controllers handle their automatic CS). This idea of a transaction in SPI is high level enough that I feel like it should be incorporated if you want to avoid specific functions for handling the CS. Arduino has the same notion in their SPI class, though it doesnt handle the CS for you. We could go that route though it doesnt full remove the issue of spi transactions taking multiple function calls unless you make convenient functions which did the transaction begin end inside them like spi_write vs spi_transaction_write, I dont really like adding that kind of complexity either, but again, it seems a rather inperfect situation that isnt totally unusual.

While I also agree the psuedocode you gave isnt the prettiest, it is necessary to avoid even uglier code from appearing where those spi_cs_assert/dessert are replaced by gpio_write in the devices that cant rely on the CS; which isnt fixing the problem at all. True many people could avoid doing that, but if the automatic CS isnt working for them, theyd have no option but to resort to the even uglier GPIO solution. Secondly, youre suggesting the spi_transceive is reentrant, which seems to be true for the qmsi and dw drivers, but not the k64 and intel drivers. If the scenario youre trying to prevent is a common one, you will need to be 100% sure all spi drivers implemented in Zephyr are indeed reentrant which I didnt realize was a mandate for any device driver at present.

It would be great if write/read/tranceive were enough, but as we already know of two situations where they arent without doing a survey of SPI devices, I feel like that merits considering

nashif commented 7 years ago

by Tomasz Bursztyka:

Reentrancy is missign on some drivers because patches were not done for them.

As I said: using gpio line as CS can be hidden in the spi drivers. We are already doing it for DW and QMSI drivers. So that's a non-issue here.

I am not entirely against exposing some CS control in public API, but only for very specific cases like devices that can be woken up by asserting CS for n milli-seconds etc... (no transaction is done). Things like that.

nashif commented 7 years ago

by M R Rosen:

Tomasz Bursztyka The fact that the driver uses GPIO under the hood I think is an acceptable solution as the SPI driver writer has an idea of what SoC they are supporting and thus the functionality of that SoC and whether they can replace the CS with GPIO or not. The question is whether we want higher level drivers like those for the SPI devices themselves to be making GPIO to control the CS as you had to do for the CC2250. I also agree, these are pretty specific cases, but they arent unique, thus we should endeavor to support them. I feel like just granting so kind of manual control of CS is the way to go. And as I originally suggested, this can be a configurable settings as part of the SPI config; it we can leave the current functionality as is for an automatic mode in configuration with this special control only in manual mode. The suggestion being if automatic CS mode doesnt work for your device, you can use manual mode to explicitly set transactions start and end points.

nashif commented 7 years ago

by Mark Linkmeyer:

Andrei said this story should be pushed out of 1.7, so I'm deferring it to 1.8.

nashif commented 7 years ago

by Qiu Peiyang:

Any update for this API?

nashif commented 7 years ago

by Tomasz Bursztyka:

Yes, it will land for 1.8

nashif commented 7 years ago

by Qiu Peiyang:

Can you please give a summary about what these APIs would be like in the future? So we can prepare the case for test now.

nashif commented 7 years ago

by Tomasz Bursztyka:

Can you look into devel's ML? I sent a while ago an RFC on what it would look like. Subject was "[RFC] SPI API update"

archive link: https://lists.zephyrproject.org/pipermail/zephyr-devel/ Look in September 2016

nashif commented 7 years ago

by M R Rosen:

Tomasz Bursztyka

I think you mentioned that you had this discussion on the mailing list before but this was before I joined the list so didnt get a chance to review it. I have now and Im seeing the same thing come up with your initial point #5. Thats exactly what we need for the most part. The whole DW CS being messed up aside, weve already found reasons to provide CS control to the user (or more importantly, the driver implementer) and as such need something like your point #5 in place to handle these cases (if you enable the multiple transactions, or more correctly, multiple calls to get and send data as I argued above that a transaction in SPI is typically CS negedge followed by some data moving ended by CS posedge, you get CS control for free mostly). I know its not an easy thing to solve, but to make a generic SPI API, we need some way of doing it. And we cant use GPIO or we will wind up with drivers that are suppose to be generic for things like the CC2250 using GPIO for the CS themselves, making code look like this:

gpio_set_pin(CS_PIN, 0); spi_transceive(...); gpio_set_pin(CS_PIN, 1);

Which I feel is way worse than a SPI-only solution (even if the driver uses GPIO under the hood, the SPI can implement its CS however it wants and the board has to make it work like you suggested in the mail thread).

Just want to make sure that this is at least thought about in the new API.

nashif commented 7 years ago

by Mark Linkmeyer:

Hi Tomasz Bursztyka , I see where you mention above that this story will land in 1.8. Just a reminder that per the TSC-approved Release Criteria, the goal is to have all high-priority stories in Code Review by the M2 milestone. That's next week. Will you have this story ready for Code Review by next week? I ask because I notice it's currently still in the To Do state waiting to be worked on. Thx.

nashif commented 7 years ago

by Michał Kruszewski:

Is dual/quad SPI going to be supported in the new SPI api or will it require new, separate api file?

nashif commented 7 years ago

by Tomasz Bursztyka:

Mark Linkmeyer As code-review is the only requested status, API itself can be in code-review by then.

Michał Kruszewski You mean 2-4 master controllers or 2-4 slave lines on one master controller? (both are already supported in current API btw)

M R Rosen Exposing CS control is out of question. First some controllers don't let any possibility to control their CS logic, and second by-passing it through GPIO is not something we can legitimate as a solution (DW controllers on quark se are easy because their CS lines are most of the time shared with a gpio one, so it was a "natural" hack to come with). I know some slaves are annoying with CS timing, but we will have to solve it another way: having a delay in spi_config that would be used before/after actual transaction processing, internally, something like that if possible. About the multiple-transaction itself, do you have any use-case? I don't get it when you say "Thats exactly what we need for the most part.". To me it seems rather rare use-case where you really need to hold the CS line after each transaction. It brings a lot of issues with many callers handling: the thread which got such transaction, should be the only one to continue on another transaction, blocking any other callers on that spi dev as long as it is not fully done.

nashif commented 7 years ago

by Michał Kruszewski:

Tomasz Bursztyka What I mean is: [ldual/quad spi|https://electronics.stackexchange.com/questions/28792/what-is-dual-quad-i-o ] . For example some of nRF chips have quad SPI peripheral and I wonder if it needs some extra API or its functionality will be included in this patch?

nashif commented 7 years ago

by Tomasz Bursztyka:

Michał Kruszewski As far as I can tell, this should not need anything from API. This seems to be very particular in both sides: master and slave. I see it more like a driver issue (both spi controller and the slave driver, whatever it is), nothing that needs to be handled/configured at runtime.

nashif commented 7 years ago

by M R Rosen:

Tomasz Bursztyka

To your first point, I dont see this as a problem. I agree that explicit control of CS isnt the norm so most of the time, youd let the driver handle it (hopefully it does it in a good way). If the driver doesnt support CS control because the control doesnt support it and it cant reasonably be done with knowledge of the SoCs the driver is supporting, then it can just error out if you try to use it. I dont know all of the design goals of Zephyr, but I dont think the APIs should just cater to the minimal set of available functionality in all the SoCs it supports. An example of this at present is the flash_write_protect_set function, in the API but not supported by MCUX driver so it just errors out if you use it.

To your second point, I agree, GPIO CS isnt a good solution and isnt aways going to work. Quark can do it because they know their SoC. But it shouldnt be the way we go about doing it all the time.

I guess what I hoped to get out of this is a supported, clean solution to what hacks we had to do to get SPI SD/MMC cards to work in Zephyr. If there isnt any interest in supporting these devices (or other devices that take alot of liberty with the SPI protocol), then this API revision need not care to address the things that these devices do thats fairly out of the box. Just for an example, SD cards want you to run the SPI clock for ~74 cycles without CS asserted. Without CS control, we CANNOT implement a generic MMCSD SPI driver for use on multiple SoCs without using GPIO for CS (true, some SoCs wont support this, but the MMCSD SPI driver can just see that the SPI doesnt have CS control ability and error out). I dont like it at all, but thats the way it goes. Im wondering if you have any suggestions then, if I want to write a MMCSD SPI driver that can be generic enough for Zephyr, how would I do it in the new SPI API? Thats really my concern and wanted to make it into the discussion since the API was being revised.

On a completely different note, please say you are adding the ability to chose if a given transfer is done via polling, interrupt or DMA (either via config or as an argument to the transceive function itself); completely different but it would be nice too!

nashif commented 7 years ago

by Mark Linkmeyer:

Hi Tomasz Bursztyka , if I'm understanding your reply correctly, you're saying that only the API portion of this story can be in code-review by this week (for the M2 milestone), but any other development work that's needed to fully implement this story will not be in code review this week. If that's the case, then by when do you expect the rest of this story to be implemented and in code review? In other words, when will the full story be in code review? Also, the description of this story is rather ambiguous. Based on what you plan to do, if you can make it more clearly state what will be done in 1.8, that would be good -- especially since QA needs to test it and the description is used to generate release notes. Thx.

nashif commented 7 years ago

by Tomasz Bursztyka:

Mark Linkmeyer Sorry, I can't be more precise than it's going to be done for 1.8. For QA, what I posted for them is the only thing I have for now. That being said, I am not going to replace at once the old API by the new one, for the good reason I cannot alone change all the SPI drivers (and I am not entirely sure some ext/hal enables some of the changes nicely so some drivers will be a pain to update). So we could have legacy API being the default, and the new one optional. Less stress on the changes, and QA will have time to fully prepare for 1.9 for instance. How does that sound?

M R Rosen There will never be polling mode. Zephyr is a fully preemptive OS so no polling. Drivers itself are using interrupts only. Then on top, there will be synchronous API calls (as for now), new asynchronous ones (via recently added k_poll_event object). I am not sure DMA will be part of the 1.8 update, the existing DMA API itself does not provide any scatter-gather DMA enabled buffer helpers. For your sd/mmc use case I think we understood each-other, I'll come up with something.

nashif commented 7 years ago

by M R Rosen:

Tomasz Bursztyka

There are some rare cases where you might want a polling mode for the same reason uart has a polling mode (unless that's being removed as well), mostly for spi to uart bridges and messaging from isr context (there as work around but just saying). As for DMA, the current DMA API seems to be very quark se centric and I would expect (and have so far seen) that most HALs will handle using DMA with the given controller for you (whether it's centralized DMA as in quark se or distributed like the easyDMA in things like nrf52) so why reinvent the wheel and reimplement that stuff in the Zephyr driver instead of just using the dma versions of the spi functions from the HAL? I don't know if it makes sense to treat dma as it's own devices even if that provides more flexibility. I'm just thinking that even we want to look like a better, thread-aware version of QMSI, we'd have polling, interrupt and dma versions of transceive.

nashif commented 7 years ago

by Tomasz Bursztyka:

1st stage of the API is done (overall API change & buffer management): https://github.com/tbursztyka/zephyr/tree/spi_update

It's still developing thus why I did not make any PR yet.

nashif commented 7 years ago

by Tomasz Bursztyka:

Hum, the PR does not appear automatically:

https://github.com/zephyrproject-rtos/zephyr/pull/145

nashif commented 7 years ago

by Michał Kruszewski:

Recently I have come across flash memory using SPI protocol where to save time and power consumption interrupt could be generated to inform the host CPU that programming or erasing memory has been finished. However this interrupt was generated on SPI MISO pin, what makes it impossible to handle without releasing SPI peripheral on most MPU. Tomasz Bursztyka do you think we could add some mechanism that would simplify implementing drivers for IC that includes such feature? Here is some description of active interrupt: http://www.adestotech.com/wp-content/uploads/Adesto-Serial-Flash-Active-Interrupt_ADAPP_005_3_16.pdf

nashif commented 7 years ago

by Tomasz Bursztyka:

Michał Kruszewski I quickly went through the pdf. It is a very specific use case indeed. If I understood well, it's only for writing-only type of command (re-program or erase), not reading obviously.

As it's only writing, reading interrupts would be masked (that would not make sense not to do that). Which means that, right after finishing to write the command, it would need to stay up and running and enable the reading interrupt again maybe? The "active interrupt" on MISO line would generate an RX interrupt I guess. It's hard to tell without testing in real.

But yes, to manage this config would need to have a dedicated bit for it.

However, the easiest solution, and without involving specific SPI API bits, would be for the dedicated spi flash driver for that chip to handle it through a gpio interrupt by itself. But it would mean that hardware is properly wired for it: either 'Y' the MISO line (slave output -> 2 inputs: master miso, and the relevant gpio line). Of course it's better if the miso line can be muxed with a gpio pin, then no specific wiring. And the gpio interrupt handler would be enabled by the spi flash driver relevantly.

What do you think? to me it seems possible to avoid involving SPI API specific bits.

nashif commented 7 years ago

by Michał Kruszewski:

Tomasz Bursztyka In my opinion 'Y' MISO is funny and bad idea. What we need is just some API function for releasing MISO pin. I have played with such flash chip and nRF SoC and all I had to do after sending Active Status Interrupt command was to release MISO pin from SPI peripheral cause when MISO register was configured I was not able use GPIO peripheral with that pin.

Instead of using SPI API specific bits I would just add one function for releasing MISO pin and another one for setting it back again after the interrupt has come.

nashif commented 7 years ago

by Tomasz Bursztyka:

I agree that the 'Y' looks awkwark, but you had the use case that your miso line could be pin-muxed to be gpio line. What if it's not possible?

Anyway, nack on these 2 functions, it's really not generic at all. On the controller I work against for instance, there is no such thing as "releasing MISO", it's just pin-muxed with a gpio pin. And that is really not something to do in SPI side, but on the spi flash slave driver.

Or do we talk about the same thing: "MISO register configured" == pinmuxer configured to route MISO signel and not the GPIO?

If so, then my solution of handling in fully in the spi flash driver would just work I think.

nashif commented 7 years ago

by Michał Kruszewski:

What I mean is that on some MCUs pin drivers (I mean hardware drivers inside IC) do not allow for multiple peripherals to use pin simultaneously. To implement higher lever spi flash driver there must be some functionality that would allow for 'deallocating' pin from SPI peripheral and allocating it for a while to GPIO peripheral.

nashif commented 7 years ago

by Tomasz Bursztyka:

Michał Kruszewski Yes so we are definitely talking about the same thing, this mechanism is pin-muxing :) Most, if not all, SoC do it. And Zephyr exposes the feature through a generic API in include/pinmux.h

nashif commented 7 years ago

by Tomasz Bursztyka:

M R Rosen Michał Kruszewski Could you guys review the patch-set in github? https://github.com/zephyrproject-rtos/zephyr/pull/145

nashif commented 7 years ago

by Michał Kruszewski:

Tomasz Bursztyka How urgent is it?

nashif commented 7 years ago

by Tomasz Bursztyka:

today/tomorrow would be really nice

nashif commented 7 years ago

by Michał Kruszewski:

Tomasz Bursztyka I can promise to do it till the end of thursday (18.05.2017).

nashif commented 7 years ago

by M R Rosen:

Tomasz Bursztyka Changes to my email resulted in me missing the notification on this. I will review today and provide feedback on the github pull request; thanks!

nashif commented 7 years ago

by Qiu Peiyang:

I have one question about this update. It looks like you only update SPI DW driver. So for other SPI drivers, like SPI QMSI driver, they are not compatible with these new APIs. Will you add these supports later ?

In file included from /home/ma/latest/zephyr/drivers/spi/spi_qmsi.c:13:0: /home/ma/latest/zephyr/drivers/spi/spi_qmsi.c: In function 'spi_qmsi_configure': /home/ma/latest/zephyr/drivers/spi/spi_qmsi.c:81:44: error: 'struct spi_config' has no member named 'config' cfg->frame_size = SPI_WORD_SIZE_GET(config->config) - 1; ^ /home/ma/latest/zephyr/include/spi.h:89:5: note: in definition of macro 'SPI_WORD_SIZE_GET' (((operation) & SPI_WORD_SIZE_MASK) >> SPI_WORD_SIZE_SHIFT) ^ /home/ma/latest/zephyr/drivers/spi/spi_qmsi.c:82:34: warning: implicit declaration of function 'SPI_MODE' [-Wimplicit-function-declaration] cfg->bus_mode = config_to_bmode(SPI_MODE(config->config)); ^ /home/ma/latest/zephyr/drivers/spi/spi_qmsi.c:82:49: error: 'struct spi_config' has no member named 'config' cfg->bus_mode = config_to_bmode(SPI_MODE(config->config)); ^ /home/ma/latest/zephyr/drivers/spi/spi_qmsi.c:86:37: error: 'struct spi_config' has no member named 'config' context->loopback = SPI_MODE(config->config) & SPI_MODE_LOOP; ^ /home/ma/latest/zephyr/drivers/spi/spi_qmsi.c:87:27: error: 'struct spi_config' has no member named 'max_sys_freq' cfg->clk_divider = config->max_sys_freq; ^ /home/ma/latest/zephyr/drivers/spi/spi_qmsi.c: At top level: /home/ma/latest/zephyr/drivers/spi/spi_qmsi.c:213:2: error: unknown field 'configure' specified in initializer .configure = spi_qmsi_configure, ^ /home/ma/latest/zephyr/drivers/spi/spi_qmsi.c:213:15: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types] .configure = spi_qmsi_configure, ^ /home/ma/latest/zephyr/drivers/spi/spi_qmsi.c:213:15: note: (near initialization for 'spi_qmsi_api.transceive') /home/ma/latest/zephyr/drivers/spi/spi_qmsi.c:214:2: error: unknown field 'slave_select' specified in initializer .slave_select = spi_qmsi_slave_select, ^ /home/ma/latest/zephyr/drivers/spi/spi_qmsi.c:214:18: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types] .slave_select = spi_qmsi_slave_select, ^ /home/ma/latest/zephyr/drivers/spi/spi_qmsi.c:214:18: note: (near initialization for 'spi_qmsi_api.release') /home/ma/latest/zephyr/drivers/spi/spi_qmsi.c:215:16: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types] .transceive = spi_qmsi_transceive, ^ /home/ma/latest/zephyr/drivers/spi/spi_qmsi.c:215:16: note: (near initialization for 'spi_qmsi_api.transceive') make[4]: [drivers/spi/spi_qmsi.o] Error 1 make[3]: [drivers/spi] Error 2 make[2]: [drivers] Error 2 make[2]: Leaving directory `/home/ma/latest/zephyr/samples/hello_world/outdir/quark_se_c1000_devboard' make[1]: [sub-make] Error 2 make[1]: Leaving directory `/home/ma/latest/zephyr' make: *** [all] Error 2

nashif commented 7 years ago

by Qiu Peiyang:

I am testing new APIs using sample app zephyr/samples/drivers/spi, but get weird log. Looks like this sample application keeps running and dumping too much log in a loop, but there is no loop in this sample. Finally, it outputs an error "spi_complete_loop: Buffer contents are different' and then stops.

Attach the log. [^minicom.cap]

nashif commented 7 years ago

by Qiu Peiyang:

Can anyone give a feedback ?

nashif commented 7 years ago

by Tomasz Bursztyka:

Qiu Peiyang Did you hard-looped miso/mosi lines? The amount of debug output is due to spi debugging being enabled. Abouth QMSI: I am not going to add any support myself as long as QMSI won't expose a nicer API to do so.

nashif commented 7 years ago

by Qiu Peiyang:

Tomasz Bursztyka Yes, I loop miso/mosi, so it can get a lot of 'Passed'. But it also gets error '[general] [ERR] spi_complete_loop: Buffer contents are different 0123456789abcdef vs ™š°±1²2³'.

nashif commented 7 years ago

by Sharron LIU:

two opens.

SPI QMSI driver is not updated with new APIs. Tomasz Bursztyka if you need a new Jira story tracking this?

with DW driver, still observed error "zephyr/samples/drivers/spi". Qiu Peiyang please sync up with developer if need a new Jira bug tracking this?

nashif commented 7 years ago

by Tomasz Bursztyka:

Qiu Peiyang Sharron LIU I have 4 different Mount Atlas boards here, and they all pass. Can you check if it fails in the slow config or the fast one? (something tells me the fast one will be the failing one). Can you get some logs as well and post them here?

Sharron LIU It's up to QMSI driver maintainer to follow up. But first, QMSI API itself would need to be reworked so the shim driver adaptation would be easy to do.

nashif commented 7 years ago

by Qiu Peiyang:

I post a log days ago and you can see it in 'Attachments'. I understand if CONFIG_SYS_LOG_SPI_LEVEL is enabled, too much debug log will be dumped, but it looks like there is a while statement that makes this application keep running in a loop.

You can see it in the log that this case keeps running again and again, and 'spi_dw_init' appears many times.