Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[E404X] Determine flash part at runtime #2456

Merged
merged 22 commits into from
Jun 16, 2022

Conversation

scott-brust
Copy link
Member

@scott-brust scott-brust commented May 24, 2022

Problem

The ESOMX, B404X, B524X, T404X and other SKUs will need to support multiple different flash chips in order to avoid dependencies on a single supplier. Historically we have handled hardware variations using OTP, but we cannot use that as we will not know the flash variant at boot. The exflash driver will need to discover what flash part is attached, and update the driver to communicate with it accordingly.

Solution

This PR reads the flash Device ID from the part, then modifies the driver to account for any differences/quirks of that part. Right now the following flash parts are supported
MX25R6435FZNIL0
GD25WQ64EQFG -- as of yet untested

Steps to Test

For ESOMX / other boron/NRF52840 platforms, this PR should be backwards compatible with the current 8 and 4mb MXIC parts (MX25R6435FZNIL0 and MX25L3233F)

Example App

Any, tinker is fine

References

See AVL Components List in sheets


Completeness

  • User is totes amazing for contributing!
  • Contributor has signed CLA (Info here)
  • Problem and Solution clearly stated
  • Run unit/integration/application tests on device
  • Added documentation
  • Added to CHANGELOG.md after merging (add links to docs and issues)

@scott-brust scott-brust marked this pull request as draft May 24, 2022 22:23
@scott-brust scott-brust requested a review from XuGuohui May 25, 2022 16:38
@scott-brust
Copy link
Member Author

@XuGuohui Here is where I am with the GD25 flash part support. Feel free to use this PR if it is helpful to bring up the part. I wont have hardware to test on for a few more days

@scott-brust scott-brust requested a review from keeramis May 31, 2022 15:24
@scott-brust scott-brust force-pushed the feature/sc-103088/flash-detection branch from 594fe78 to ff769cd Compare May 31, 2022 23:30
@scott-brust scott-brust changed the base branch from feature/sc-101084/e404x-platform to develop May 31, 2022 23:30
@YutingYou YutingYou force-pushed the feature/sc-103088/flash-detection branch 2 times, most recently from 7ed5d02 to f68614d Compare June 2, 2022 10:08
@technobly technobly added this to the 4.0.0 milestone Jun 6, 2022
@YutingYou YutingYou requested a review from avtolstoy June 7, 2022 05:40
Copy link
Member

@XuGuohui XuGuohui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to also remove HAL_PLATFORM_FLASH_MX25L3233F for Boron and the ESOM after rebasing on top of develop.

hal/src/nRF52840/exflash_hal.c Outdated Show resolved Hide resolved
hal/src/nRF52840/exflash_hal.c Outdated Show resolved Hide resolved
hal/src/nRF52840/exflash_hal.c Outdated Show resolved Hide resolved
hal/src/nRF52840/exflash_hal.c Outdated Show resolved Hide resolved
hal/src/nRF52840/exflash_hal.c Outdated Show resolved Hide resolved
hal/src/nRF52840/exflash_hal.c Outdated Show resolved Hide resolved
hal/src/nRF52840/exflash_hal.c Outdated Show resolved Hide resolved
hal/src/nRF52840/exflash_hal.c Outdated Show resolved Hide resolved
hal/src/nRF52840/exflash_hal.c Outdated Show resolved Hide resolved
hal/src/nRF52840/exflash_hal.c Outdated Show resolved Hide resolved
@YutingYou
Copy link
Contributor

To get rid of goto and use RAII lock in the driver, I change the c source file to cpp but keep most of the original driver. The OTP read/write passed the test. I'll continue to run an EEPROM test which is based on the file system.

@XuGuohui @avtolstoy Could you please help re-review the driver to make sure it is implemented as we discussed yesterday?

Copy link
Member

@XuGuohui XuGuohui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, we need to remove HAL_PLATFORM_FLASH_MX25L3233F for Boron and ESOMX after rebasing on top of develop.

hal/src/nRF52840/exflash_hal.cpp Outdated Show resolved Hide resolved
hal/src/nRF52840/exflash_hal.cpp Outdated Show resolved Hide resolved
hal/src/nRF52840/exflash_hal.cpp Outdated Show resolved Hide resolved
hal/src/nRF52840/exflash_hal.cpp Show resolved Hide resolved
hal/src/nRF52840/exflash_hal.cpp Outdated Show resolved Hide resolved
hal/src/nRF52840/exflash_hal.cpp Outdated Show resolved Hide resolved
hal/src/nRF52840/exflash_hal_params.h Outdated Show resolved Hide resolved
hal/src/nRF52840/exflash_hal_params.h Outdated Show resolved Hide resolved
@YutingYou YutingYou marked this pull request as ready for review June 8, 2022 15:31
@scott-brust
Copy link
Member Author

Pushed a fix to wake GD25 from sleep mode. Seems to work ok.
I still havent tested

  1. HAL_QSPI_CMD_GD25_PGMERS_SUSPEND
  2. HAL_QSPI_CMD_GD25_SEC_ERASE To see if this even works
  3. HAL_EXFLASH_COMMAND_LOCK_ENTIRE_OTP Is also still unimplemented for GD25, not sure if this is high priority though

hal/src/nRF52840/exflash_hal.cpp Outdated Show resolved Hide resolved
hal/src/nRF52840/exflash_hal.cpp Outdated Show resolved Hide resolved
hal/src/nRF52840/exflash_hal_params.h Outdated Show resolved Hide resolved
hal/inc/exflash_hal.h Outdated Show resolved Hide resolved
hal/src/nRF52840/exflash_hal.cpp Outdated Show resolved Hide resolved
hal/src/nRF52840/exflash_hal.cpp Show resolved Hide resolved
hal/src/nRF52840/exflash_hal.cpp Outdated Show resolved Hide resolved
hal/src/nRF52840/exflash_hal.cpp Outdated Show resolved Hide resolved
hal/src/nRF52840/exflash_hal.cpp Outdated Show resolved Hide resolved
hal/src/nRF52840/exflash_hal.cpp Outdated Show resolved Hide resolved
hal/src/nRF52840/exflash_hal.cpp Outdated Show resolved Hide resolved
hal/src/nRF52840/exflash_hal_params.h Outdated Show resolved Hide resolved
hal/src/nRF52840/exflash_hal_params.h Outdated Show resolved Hide resolved
@scott-brust
Copy link
Member Author

Tested latest changes with boron + GD25 flash, read/write OTP works, sleep/wake works.
Will test with boron +MXIC flash as well

@YutingYou
Copy link
Contributor

The wiring/filesystem test passed on E404X-GD25 and E404X-MXIC, also ran particle flash --serial bootloader.bin on Boron to see whether there's an issue in driver update, it worked fine.

hal/src/nRF52840/exflash_hal_params.h Outdated Show resolved Hide resolved
hal/src/nRF52840/exflash_hal.cpp Outdated Show resolved Hide resolved
hal/src/nRF52840/exflash_hal.cpp Outdated Show resolved Hide resolved
hal/src/nRF52840/exflash_hal.cpp Outdated Show resolved Hide resolved
hal/src/nRF52840/exflash_hal.cpp Show resolved Hide resolved
@scott-brust
Copy link
Member Author

Tested both MXIC and GD25 flash parts with Sleep, OTP read/write, and new HAL_EXFLASH_COMMAND_GET_OTP_SIZE.

Had to tweak the init again, hal_exflash_special_command returns success even if the flash part is asleep and does not return a non-null chip id (I suppose the NRF driver cannot distinguish if a custom instruction failed or legitimately returned all 00 in response).

@avtolstoy
Copy link
Member

avtolstoy commented Jun 14, 2022

@scott-brust One additional thing to test is sleep: the flash should correctly be put into suspended/low power mode and correctly re-initialized on wake-up (STOP or ULP modes).

@scott-brust
Copy link
Member Author

scott-brust commented Jun 14, 2022

@scott-brust One additional thing to test is sleep: the flash should correctly be put into suspended/low power mode and correctly re-initialized on wake-up (STOP or ULP modes).

Ok, I tested sleep using hal_exflash_sleep(), which is what appears to be used in sleep_hal.cpp, so I'm assuming that is good enough. I can see the command succeeding over the wire, but haven't measured any kind of current consumption change. The part is sleeping though, it does not respond to ID commands until we issue wake (in the case of the GD25 part)

@avtolstoy
Copy link
Member

@scott-brust This is a pretty major change affecting not only new devices with GD25 part, but all other Gen 3 platforms as well, so let's do a proper test with STOP/ULP modes. I'd suggest going through wiring/sleep20 suite on Boron/BSoM/Argon (any one), B5 SoM/Tracker (any one) and devices with new GD25 flash.

@scott-brust
Copy link
Member Author

@scott-brust This is a pretty major change affecting not only new devices with GD25 part, but all other Gen 3 platforms as well, so let's do a proper test with STOP/ULP modes. I'd suggest going through wiring/sleep20 suite on Boron/BSoM/Argon (any one), B5 SoM/Tracker (any one) and devices with new GD25 flash.

Good point, ill run wiring/sleep20 on both my boards locally. If that looks ok ill merge and we can run on wider fleet of devices on HIL

@avtolstoy
Copy link
Member

@scott-brust Note that sleep tests are manual and not runnable in automated fashion.

@scott-brust scott-brust merged commit f0cba93 into develop Jun 16, 2022
@scott-brust scott-brust deleted the feature/sc-103088/flash-detection branch June 16, 2022 21:34
@scott-brust
Copy link
Member Author

Tested the following permutations with this branch

  • BORON404X - flash-detect branch bootloader + OS + app
    • GD25 Sleep20: All passed except final cellular test
    • MXIC Sleep20: All passed except final cellular test
  • Argon (wake from GPIO)
    • Flash detect bootloader + 3.3.0 System part
      • HIBERNATE, ULTRA_LOW_POWER,STOP : OK
    • Flash detect bootloader + 2.3.0 System part:
      • HIBERNATE, ULTRA_LOW_POWER,STOP : OK
  • Tracker (Wake from duration)
    • Flash detect bootloader + 3.3.0 System part
      • HIBERNATE, ULTRA_LOW_POWER,STOP : OK
    • Flash detect bootloader + 2.3.0 System part:
      • HIBERNATE, ULTRA_LOW_POWER,STOP : OK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants