search

RfidResearchGroup / proxmark3

Iceman Fork - Proxmark3
http://www.icedev.se
GNU General Public License v3.0
3.86k stars 1.02k forks source link

SPI Flashmem read/write speeds #248

Open iceman1001 opened 5 years ago

iceman1001 commented 5 years ago

SPI Flashmem read / write speeds to get rid of those pesky bitflips that happend when getting data from SPI.

See PR https://github.com/RfidResearchGroup/proxmark3/pull/244

doegox commented 5 years ago

From @cjbrigato:

I'm on something as a result of your test but still can you ensure the test has been made in both connected and disconnected mode with btaddon as battery with bt in both case? If the result are the very same, then the can you DO this :

If you got time, and I would be please to have your result. In fact your commit DID solve the writing at 24mhz in a larger scale then with former settings. But it seems that actually reading back with the same settings without a full reset after data have been written at 24mhz with your settings is actually where the problem lies. Considering how the client can operate its then normal that we had very low odd to face this case.

I think you can do a very quick standalone mode that just write a static 1kb from pages 5-8 back to 1-4 or even an hardcoded 1k after wiping the 4 first pages then read it back and compare to make the issue trigger.

About the delays these are quite well documented in the chip manual so safe and still low timeouts constants can be defined. Consider hardware capping values in case of lacking the info (like the 0.8ms/page states hardware cap is for write operations, and it tells quite a lot about what is an efficient and still a safe timeout)

Sorry for awfull formatting and typos. Quite the war here

doegox commented 5 years ago

Thanks @cjbrigato, I'll test it this evening BTW mem read had several issues (silent limit to 8k, silent wrapping at 64k, using #dbg) so I removed it. Now you can use mem dump p for reading in console. mem dump does what mem save and mem read were doing.

cjbrigato commented 5 years ago

Yes but mem dump if I'm correct in my assertion would not show the issue. I think we are hitting something very simple here and that the offset showed may either not include a bit flip but just an offset or if it include a bit flip it is would just be as a consequence of the very specific and always same delay we are missing. Maybe even a simple case of a busy kind of state we did not took as a casebof error and started reading some void before actually receiving the data.

cjbrigato commented 5 years ago

The other thing That may happen maybe that another value in initialization may have to be modified refarding the delay before reading. I had thought time configuring them with former settings with the same kind of behavior and right now I can recall all the issue there was when writing then reading in such case. Still double resetting and reiniting should prevent hitting the bug most of the time... But this definitely burry Fast reads possibility.

cjbrigato commented 5 years ago

Yes my memory came back to me from the abyss of frustration and self denying and it was a very long trip, so I'll be graceful to avoid emotional detail here and jump right through the revelation of my number 1 suspect : DLYBCT Look at this extract :

uint32_t dlybct = 0;
    uint8_t ncpha = 1;
    uint8_t cpol = 0;
    if (baudrate > FLASH_MINFAST) {
        baudrate = FLASH_FASTBAUD;
        //csaat = 0;
        dlybct = 1500;
        ncpha = 0;
        cpol = 0;
    }

    AT91C_BASE_SPI->SPI_CSR[2] =
        SPI_DLYBCT(dlybct, MCK) | // Delay between Consecutive Transfers (32 MCK periods)
        SPI_DLYBS(0, MCK)      | // Delay Beforce SPCK CLock
        SPI_SCBR(baudrate, MCK) | // SPI Baudrate Selection
        AT91C_SPI_BITS_8      | // Bits per Transfer (8 bits)

As you see, the there was a time where a dlybct of 0 was perfect. We used the true weird spi mode 0 AT invented, we wrote at 24mhz as long as delay where enforced beforehand (not voluntarily, but that was why it worked for me) I or 48mhz if we wanted it to actually work, we would we read a 24mhz and everyone was happy and fine. Then one day I tried to implement fast reads. I and then the doors of hell opened as a punition for not being satisfied enough with what was actually fine, but satisfying and indeed enough. The beast took a very vicious form, as he mimicked a configuration constant which was always meant to and has always been 0, as is the mode 0 the chip sacred document always stated. This was legend, legend of the DLYBCTS, as the dlybct was reputated able to break the whole equilibrium of the spi communication by having consequences on the Dlybs, and which should always be a (0,mck) in any mode case, no matter what these mode3 aliens where saying about their case.

More seriously you can see there that whenbyou Start operating either not in true mode 0 or when you need to break the normal delay rules (which was the case with how fast read works) you have to correct the dlybct, and with consequence on the dlybs during the context, you can then may have to sync back at some point. This is what stopped me making fast read working, and this the last relicates of these very dark moment that are seen in this condition with a dlybct = 1500. What it did was that either writing and reading would work and Fast read just show offseted and bitfliped results, or another config would do a correct read and fast read yielding same result (being correct or not result they were the very same) but never the same couple if you would write the same another time... Etc. I fear that your new setting may have an impact on the dlybct at some point, and it even smells like this need a 2ns multiple... But (as the at described behavior we talked about former thread).

In any way, I'd be glad to be proven wrong quite soon, because I made an unbreakable promess to my wife that I will never mess with the DLYBCTS again.

Edit : BTW @doegox if you need to work with custom timeouts check the checkbusy function : it is always safe to split by WaitUS(WINBOND_WRITE_DELAY); (well, you can when spi configuration is ensured stable, but which we are right now doubting about). Check chip doc for your wanted multiples of this value and (hardcapings and given values for each config, everything is precisely written as annexe in documentation)

iceman1001 commented 5 years ago

Right now I am confused. At which speeds does READ / WRITE work ? @doegox says it works. @cjbrigato says special case doesn't work.

Is it only timing 24/48 Mhz or is it DLYBCTS ?

@doegox tests shows read / write works.

I do agree that the firmware should deal with right settings for READ/WRITE (if needed) seamless to user who programs.

doegox commented 5 years ago

@iceman1001 My tests show read/write from client works, but from client means we introduce some communication delays. So before claiming it works, I've to make some more tests in standalone (connected & disconnected) with some random data/offsets/sizes with consecutive erase/write/reads. I'll write a specific standalone mode for mem tests as suggested by @cjbrigato.

iceman1001 commented 5 years ago

aha, client -> device works. but device -> device doesnt. ok. Now I am back on track

doegox commented 5 years ago

device -> device doesnt.

Well, to be proven. I tried yesterday successfully hf_colin standalone mode while disconnected and on battery and with a regular rdv4 while connected. But @cjbrigato described weirder cases to try (btaddon fpc_usart_dev but disable smart card and support)

iceman1001 commented 5 years ago

disable smart card could maybe influence the timer setup or init missing?

cjbrigato commented 5 years ago

Well right now i'm not saying that device->device doesn't work. I'm even sayin it works.

What I said is that what may not be working would actually be device READING-back in standalone (because client implies reset spi and delays by design) what device had Written at 24Mhz by Itself just before (so not using mem save as any proof). What it means then is that my set spi 48Mhz / then back to 24Mhz in standalone is just what forced a full featured Reset/reconfig, enough to avoid the read-back bug.

So if i'm right, right now everything works. But not reading back what was written just before in standalone without Doing the whole initialisation phase again (which is then defeating the purpose of half of the function i've written in flashmem. *.Cont(*args) functions are made to make possible custom and subtile Flash operation with the strict minimum of writing/wiping/reg reading/initing phases, just as the CheckBusy() which may template custom timeout function for special case with nearly minimal delays and no magic numbers (again, we could actually do this by taking back caps and numbers from the doc so make clever bounds to timeouts).

The facts that

are _not g_ood news. Believe me if just a spi speed setting can throw a bad diagnostic (speed not a problem since @doegox commit, it was decoy here), then playing with DLYBCT in such context require strong monitoring of your own mind.

iceman1001 commented 5 years ago

so with spiffs and all , is this an issue still? or shall we make a note of it in the upcoming documentation over spiffs? (close?)

iceman1001 commented 5 years ago

maybe that checkbusy is the cause of this aswell?

cjbrigato commented 5 years ago

As discussed with @doegox, I'll rewrite the flash mem driver in the same way spiffs has been (idem potence and safetyness) so if any one may still have custom needs which needs low level handling of flash, then he will find an api as coherent.