Spi0command

[ChocolateFrogsNuts]

Adds a function to execute generic SPI commands on SPI0.
This needs to be handled carefully as SPI0 is used to access the flash chip - so
precautions need to be taken to ensure the code doing it is precached() before
accessing the SPI controller.

This is part 2 of the solution to #6559

[devyte]

@ChocolateFrogsNuts I see you take as arg an spiIfNum which can be only 0 or 1. I assume that is to select between normal spi and hspi. If that is correct, what is the meaning of the "0" in the function names? It confused me at first, I thought it indicated that the code was limited to only one of the two.

EDIT: I think I understand now: the internals allow choosing either of the two, but SPI0Command hardcodes the arg to 0. I assume you did this because you have only tested with bus 0?

[devyte]

One last thing: this is experimental, I think it makes sense to put the code in a namespace that reflects as much, e. g. namespace experimental. That also aligns with usage I intend to put in play elsewhere for code that isn't meant to be used by users, the main reason being that backwards compatibility is not guaranteed in minor/sub releases for these functions.

[ChocolateFrogsNuts]

@ChocolateFrogsNuts I see you take as arg an spiIfNum which can be only 0 or 1. I assume that is to select between normal spi and hspi. If that is correct, what is the meaning of the "0" in the function names? It confused me at first, I thought it indicated that the code was limited to only one of the two.

EDIT: I think I understand now: the internals allow choosing either of the two, but SPI0Command hardcodes the arg to 0. I assume you did this because you have only tested with bus 0?

Partly yes, it's only been tested with bus 0. Using the parameter also prevents the compiler from deciding the base address of the controller can be optimised as a constant. It was actually optimising the SPI register access back to being via constants loaded from flash (exactly what I was trying to avoid) until I made the parameter volatile (it worked out it was only ever set to 0 and optimised all the decision making and base+offset calculations away).

[ChocolateFrogsNuts]

I'll expand on the comments where possible over the next couple of days (some of the SPI registers don't actually have meaningful names/info beyond what I've called the variables)
Also I'll adopt the other changes as suggested unless I've given reasons not to above.

[devyte]

Nice work.

it's only been tested with bus 0

I don't suppose you can somehow test this with bus 1? It's ok if not, but in that case please add a comment above SPI0Command that bus 0 is hardcoded because bus 1 isn't tested.

I'm approving now. Please let me know once you're done with any additional comments that you can add (the more the merrier, remembering this stuff in the future is unlikely). I'll merge at that point.

[devyte]

BTW, doesn't this supersede #6628 ? If so, maybe close that one and keep this one?

[ChocolateFrogsNuts]

BTW, doesn't this supersede #6628 ? If so, maybe close that one and keep this one?

6628 is required by this, and this branch was created from the branch for 6628 - which is why that code is showing in this PR.
They are technically separate features hence the separate branch and PR - I had hoped 6628 might get merged before I created this PR.

Anyway, merging #6628 then this should work or just merge this - your choice.
After this is merged there will be one last small PR with the XMC flash chip specific code.

EDIT: oh and I'm done with the comments now I think.

[devyte]

About the xmc code, where do you intend to call it? I ask, because the features here are in the core, which isn't accessible from the bootloader, so if you intend to call from the bootloader, the code here has to be moved.

[ChocolateFrogsNuts]

About the xmc code, where do you intend to call it? I ask, because the features here are in the core, which isn't accessible from the bootloader, so if you intend to call from the bootloader, the code here has to be moved.

I'm intending to call it from the core, most likely from user_init() in core_esp8266_main.cpp
There are several other init functions called from there.

Do we even have a bootloader that runs when not using OTA updatable flash? Even if we do I'm thinking it needs to be kept to the bare minimum required to get the main firmware running.

[devyte]

Yes, eboot. On bootup it checks if a new image is present and if so it copies it from the empty area to the start of the flash. This happens before the app is launched, that's why I asked: there is flash access before app bootup.

[ChocolateFrogsNuts]

Had a look at eboot - At this stage I think it is not included in non-OTA firmwares... but will look further until I'm sure.

Also the issues with the XMC chips don't seem to show up during the initial firmware load from flash to the esp - possibly due to lower flash clock speed or some other factor such as WiFi not drawing power yet and/or no writes to flash.

Basically I see no reason to call it from the boot loader as early in the app is sufficient, and there is a possibility the bootloader is not guaranteed to be part of the bootup process (still looking into that)

[ChocolateFrogsNuts]

Further to above:
Yes, eboot is present in all firmware generated, regardless of whether it uses OTA updates.
However eboot does not make any changes to the flash access speeds - they are already configured by the ROM code based on the first 4 bytes stored in flash.

Thus it seems pointless to further complicate eboot by trying to move the spi0command/XMC specific code there as it would also make spi0command inaccessible to the app (ie it becomes less generically useful).

[devyte]

and/or no writes to flash

eboot copies the new image found in the empty space over to address 0x0. That is the part that worries me, because per your approach, the flash driver would still be at 75% during that read/write process. For correct flash access, I would expect either a slower flash clock or the driver at 100% during eboot.
Why do you think the special xmc settings are not needed for eboot but are needed for the app? What's different?

[ChocolateFrogsNuts]

The 75% drive conditions would exist for the actual load of eboot itself. The only way I can see to avoid that would be to set a slower flash speed initially, then upgrade it after the drive has been set.
Possibly the simplest thing to do there is leave eboot as-is, set the flash speed in the IDE to 20 or 26 mhz, and then have the XMC code bump up the flash speed after it sets the drive level.
I was hoping to have a solution that didn't require the user to set two different flash speeds as the problem doesn't seem to surface until power demands begin to increase. The flash chip seems to struggle for power in some way as it really shows up best when flash writes are added to the mix (eg SPIFFS).

EDIT: I've never seen it struggle if wifi RF is at reduced power and there are no flash writes happening - conditions which hold up until the app gets into running user code.

[devyte]

Does reading work at 75% with rf etc?
I'm thinking along the lines of wrapping the flash read/write perations to set drive level before/reset after.

[ChocolateFrogsNuts]

ahh yes I see your point for OTA updates now... eboot works differently to the espressif bootloader in that it copies the new code, rather than executing it at its new address.

OTA update is unlikely to involve a power cycle, just a soft reset. The drive output setting is maintained across this reset, so the flash chip will be running 100% drive during eboot.
If it's a cold boot - causing the flash chip to be at 75% drive - it won't be the result of an OTA update - ie there will be no requirement to copy a new firmware.

[ChocolateFrogsNuts]

Does reading work st 75% with rf etc?
I'm thinking along the lines of wrapping the flash read/write perations to set drive level before/reset after.

Reading seems to be 100% ok unless the RF is at full power or there are flash writes.
With RF at full power it's still fine 99.99% of the time...
Flash writes seem 100% ok when loading firmware over USB (I assume RF is off for that)
When you put RF and flash writes together, it was crashing every time. I assume it's because the flash chip doesn't have a lot of power stored internally, and the additional RF load reduces the voltage available to the flash chip, making it struggle to recharge it's internal caps fast enough when aiming for 75%. At 100% it maintains a higher voltage with the charge pump, meaning more energy available to cope with surges in demand. (there is at least one internal charge pump in the flash chip according to the XMC datasheet, and the drive setting affects the voltage it maintains)

[devyte]

Be very careful of the assumptions you're making there. Does the flash setting survive a hard reboot of the board via rst pin?

Off the top of my head, I can think of two scenarios where eboot could be faced with having to update after a hard pin reset: receiving the ota image and immediately doing a deepsleep, which on wakeup is a pulse to the reset pin, and some combinations of development with the esp connected to a laptop (e. g. direct flashing to the empty area), where esptool could reset via rst pin.
The first is more likely to be hit by the average user, the second is unlikely, but were it hit, it would be very hard to figure out what's wrong.

Also, consider a power outage or brownout etc after receiving the image. Unlikely, but wouldn't that potentially brick the esp?

[ChocolateFrogsNuts]

Be very careful of the assumptions you're making there. Does the flash setting survive a hard reboot of the board via rst pin?

It definitely survives the reset after a usb upload, with reset by esptool... can't remember if I also tested with the reset button on the board, but I will do that (though I'm sure it's connected to the same rst line used by esptool). As far as I know the only thing to reset the drive setting to default is loss of power to the flash chip.

Off the top of my head, I can think of two scenarios where eboot could be faced with having to update after a hard pin reset: receiving the ota image and immediately doing a deepsleep, which on wakeup is a pulse to the reset pin, and some combinations of development with the esp connected to a laptop (e. g. direct flashing to the empty area), where esptool could reset via rst pin.
The first is more likely to be hit by the average user, the second is unlikely, but were it hit, it would be very hard to figure out what's wrong.

Deepsleep and the direct flashing scenario should be the same as rst then - no loss of power to the flash chip, so 100% drive is still enabled.

Also, consider a power outage or brownout etc after receiving the image. Unlikely, but wouldn't that potentially brick the esp?

Potentially yes. At worst it would require a usb connection to either flash the new firmware directly and clear the eboot command from the RTC eeprom, or to have esptool enable 100% drive in the flash - the reset afterwards would then start eboot with 100% flash drive and copy the new firmware successfully.

So we are down to a power outage between writing the eboot command to RTC and erasing it after completing the copy being a problem. Possibly ensuring the flash speed is reduced during the eboot copy would be better (and perhaps something that could be applied as a safety measure for all chips?) It should only take a couple of lines of code in eboot....

[devyte]

So maybe something like this in eboot?

if(flashid == xmc && flashspeed >= 40mhz) 
{
   oldflashspeed = flashspeed;
   flashspeed = 26Mhz;
} 
... //do bin copy
flashspeed = oldflashspeed;

?

[ChocolateFrogsNuts]

So maybe something like this in eboot?

pretty much what I was thinking... it looks like it's just save/set/restore a single register.
Was thinking 20mhz so there's a good safety margin.... and possibly not checking for XMC - just do it for all so other random chips are covered - aside from OTA updates taking a few seconds longer to apply there should be no risk/cost to doing it on other chips, but that's up to you...

[devyte]

I understand your thinking, and I'm of two minds there. I think at the minimum the worst case time delta for the copy operation would need to measured (so a bin near 1MB) before deciding.
In any case, a big phat comment in the core code where you set the register is needed as reminder that eboot code needs to be updated as well if ever there is another chip brand added to the logic.

[ChocolateFrogsNuts]

I understand your thinking, and I'm of two minds there. I think at the minimum the worst case time delta for the copy operation would need to measured (so a bin near 1MB) before deciding.

Test code based on the code in eboot, copying 1MB from address 0 of flash to 3MB into a 4MB flash at 40MHz SPI = 13937ms.
Same code at 20MHz SPI = 16313ms - That means most of the time is spent waiting for the flash chip to store the data, not waiting for the SPI bus transfer.
Cost of being cautious... <2.4 seconds.

Got some test code temporarily switching the the SPI frequency while doing the coyp above, but there's a slight hitch... switching 80->40 works, 40->20 works, 26->20 works, but for some reason 80->20 does not (wdt timeout during the copy). Fortunately 80->26 works, so we can work with that...
(and yes I've looked at all the SPI0 register values to see if there's another one needs changing)

Worst bit is testing can be difficult - sometimes it works perfectly with 75% drive at 80MHz flash, other times not.... but so far it always works if either the flash speed is reduced below 40 mhz or the drive is 100%.

[devyte]

@ChocolateFrogsNuts I merged #6628 to get some exposure while we discuss this further. Could you please resolve the conflicts here?

[devyte]

Except for that enum, I think this can be merged as is. It is marked experimental, so we don't need to maintain compat, and that allows making changes in the future if needed. Once a final decision is made as to how to fully support xmc flash and similar, we can move it out of the namespace. In the meantime, it can help if anyone needs it.