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.

Sign up for GitHub

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

Jump to bottom

Add ondie and software ECC code to SPI NAND block device driver #258

Closed
wants to merge 1 commit into from
Closed

Add ondie and software ECC code to SPI NAND block device driver #258

wants to merge 1 commit into from

Conversation

daniel-0723
Copy link

@daniel-0723 daniel-0723 commented Mar 7, 2024 •
edited
Loading

Summary of changes

Add ondie and software ECC code to SPI NAND block device driver for using ondie ECC function of SPI NAND Flash like Macronix Flash MX31LF4GE4BC or external ECC.

And the BCH implementation for software ECC is created by Macronix.

Impact of changes

Migration actions required

Documentation

Pull request type 

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[x] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results 

[] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[x] Tests / results supplied as part of this PR
 <html>

This driver is tested on DISCO_L4R9I. The flash on this board is MX31LF4GE4BC. You need to replace it with MX31LF4GE4BC.

mbedgt: test case summary: 19 passes, 0 failures
mbedgt: all tests finished!
mbedgt: shuffle seed: 0.2715403632
mbedgt: test suite report:

his driver is tested on DISCO_L4R9I. The flash on this board is MX31LF4GE4BC. You need to replace it with MX31LF4GE4BC.

mbedgt: test case summary: 19 passes, 0 failures
mbedgt: all tests finished!
mbedgt: shuffle seed: 0.2715403632
mbedgt: test suite report:


targetplatform_nametest suiteresultelapsed_time (sec)copy_method
DISCO_L4R9I-GCC_ARMDISCO_L4R9Istorage-blockdevice-tests-tests-blockdevice-general_block_deviceOK39.01default
mbedgt: test suite results: 1 OK
mbedgt: test case report:
targetplatform_nametest suitetest casepassedfailed
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------
DISCO_L4R9I-GCC_ARMDISCO_L4R9Istorage-blockdevice-tests-tests-blockdevice-general_block_deviceDEFAULT Testing get type functionality10
DISCO_L4R9I-GCC_ARMDISCO_L4R9Istorage-blockdevice-tests-tests-blockdevice-general_block_deviceFLASHIAP Testing BlockDevice erase functionality10
DISCO_L4R9I-GCC_ARMDISCO_L4R9Istorage-blockdevice-tests-tests-blockdevice-general_block_deviceFLASHIAP Testing Deinit block device10
DISCO_L4R9I-GCC_ARMDISCO_L4R9Istorage-blockdevice-tests-tests-blockdevice-general_block_deviceFLASHIAP Testing Init block device10
DISCO_L4R9I-GCC_ARMDISCO_L4R9Istorage-blockdevice-tests-tests-blockdevice-general_block_deviceFLASHIAP Testing contiguous erase, write and read10
DISCO_L4R9I-GCC_ARMDISCO_L4R9Istorage-blockdevice-tests-tests-blockdevice-general_block_deviceFLASHIAP Testing multi threads erase program read10
DISCO_L4R9I-GCC_ARMDISCO_L4R9Istorage-blockdevice-tests-tests-blockdevice-general_block_deviceFLASHIAP Testing program read small data sizes10
DISCO_L4R9I-GCC_ARMDISCO_L4R9Istorage-blockdevice-tests-tests-blockdevice-general_block_deviceFLASHIAP Testing read write random blocks10
DISCO_L4R9I-GCC_ARMDISCO_L4R9Istorage-blockdevice-tests-tests-blockdevice-general_block_deviceFLASHIAP Testing unaligned erase blocks10
DISCO_L4R9I-GCC_ARMDISCO_L4R9Istorage-blockdevice-tests-tests-blockdevice-general_block_deviceFLASHIAP Testing write -> deinit -> init -> read10
DISCO_L4R9I-GCC_ARMDISCO_L4R9Istorage-blockdevice-tests-tests-blockdevice-general_block_deviceSPINAND Testing BlockDevice erase functionality10
DISCO_L4R9I-GCC_ARMDISCO_L4R9Istorage-blockdevice-tests-tests-blockdevice-general_block_deviceSPINAND Testing Deinit block device10
DISCO_L4R9I-GCC_ARMDISCO_L4R9Istorage-blockdevice-tests-tests-blockdevice-general_block_deviceSPINAND Testing Init block device10
DISCO_L4R9I-GCC_ARMDISCO_L4R9Istorage-blockdevice-tests-tests-blockdevice-general_block_deviceSPINAND Testing contiguous erase, write and read10
DISCO_L4R9I-GCC_ARMDISCO_L4R9Istorage-blockdevice-tests-tests-blockdevice-general_block_deviceSPINAND Testing multi threads erase program read10
DISCO_L4R9I-GCC_ARMDISCO_L4R9Istorage-blockdevice-tests-tests-blockdevice-general_block_deviceSPINAND Testing program read small data sizes10
DISCO_L4R9I-GCC_ARMDISCO_L4R9Istorage-blockdevice-tests-tests-blockdevice-general_block_deviceSPINAND Testing read write random blocks10
DISCO_L4R9I-GCC_ARMDISCO_L4R9Istorage-blockdevice-tests-tests-blockdevice-general_block_deviceSPINAND Testing unaligned erase blocks10
DISCO_L4R9I-GCC_ARMDISCO_L4R9Istorage-blockdevice-tests-tests-blockdevice-general_block_deviceSPINAND Testing write -> deinit -> init -> read10
mbedgt: test case results: 19 OK
mbedgt: completed in 41.02 sec

@multiplemonomials
Copy link
Collaborator

multiplemonomials commented Mar 14, 2024 •
edited
Loading

Hi! Thank you for the contribution!

I do have some questions which I mentioned on the PR from ARMmbed/mbed-os.

  • Do you have any test data to show that it works? It would be really great if you could run some sort of stress test on your block device to prove that it works. You might be able to adapt the I2C EEPROM block device test suite for this
  • Where did the code in bch.c come from? Could you leave a code comment indicating the original source and any docs that it has?
  • Would you be amenable to splitting this code up into 3 classes? I was thinking, we could keep the existing SPINANDBlockDevice mostly as is, but add new subclasses with your functionality -- maybe OnDieECCNANDBlockDevice (for the on-die ECC) and SwECCNANDBlockDevice (for the software ECC one). That way, compatibility of the original class would be guaranteed for anyone still using it.
  • Could you provide a list of part numbers that this code is designed to work with? If this is only used with one specific part, rather than a whole range of them (like SPIFBlockDevice) then it may be better as a community library rather than a part of Mbed.

@multiplemonomials
Copy link
Collaborator

Hi, as this PR seems inactive I am going to close it for now. If you find time to answer the above questions then please do reopen this PR and we will review.

@daniel-0723
Copy link
Author

daniel-0723 commented Jun 12, 2024 •
edited
Loading

Hi, please reopen this PR. @multiplemonomials

  1. We tested the SPINANDBlockDevice driver based on SPI NAND Block Device Testcase. You can refer to the testing data in the table of "Test results" section.
  2. The BCH implementation is created by Macronix.
  3. SPINANDBlockDevice is compatible with SoftwareECC device and OnDieECC device by reading ONFI table in init process.
  4. For the part numbers, please refer to the page Serial NAND Flash.
  5. By the way, we submit similar pull request in ZephyrOS PR.

@multiplemonomials
Copy link
Collaborator

Hi @daniel-0723, after consideration I decided to move the entire SPI NAND block device to a library outside of the Mbed source tree. Repo has been created here: https://github.com/mbed-ce-libraries-examples/MacronixFlash and I have invited you for full access, so you can update this driver as needed and add other collaborators.

Regarding the SPINAND driver, I would like to ask, which devices does it support? After a bit of research, it seems like it supports MX31 and MX35 QSPI NAND flashes, but not NAND flashes from other companies. Is that correct? If not, could you update the README of that repo to indicate what devices it does support?

Thank you for submitting this code, I'm sure that people will find it super useful!
Jamie

@daniel-0723
Copy link
Author

Hi @multiplemonomials, Mbed has merged the SPINANDBlockDevice driver which we submitted in 2021. Also, we submitted similar PR in Zephyr.

it seems like it supports MX31 and MX35 QSPI NAND flashes, but not NAND flashes from other companies. Is that correct?

This driver is compatible for Generic SPI NAND Flash not only for Macronix parts.

@multiplemonomials
Copy link
Collaborator

Yeah but, ARM Mbed and Mbed CE have a bit different criteria for what can & can't be merged. If we were to merge this into Mbed CE, us Mbed maintainers would be on the hook to support it. I don't want to commit to that if we don't have to as our resources are quite limited (as in, we don't have a supported NAND flash to test with!). Which is why I think the best solution is an officially endorsed but 3rd party library, able to be updated by you guys at will. This way, people can easily use your driver, but it's clear that it's maintained separately from Mbed.

Also, I did read through the driver a bit. It looks like, to read the ONFI table, it uses a Macronix specific sequence rather than the sequence in the ONFI standard. That's why I came to the conclusion that it's Macronix only.

@daniel-0723
Copy link
Author

Hi @multiplemonomials, if you don't have SPI NAND Flash to test with, we can provide you some custom testing boards whose orignal Flash is replaced with our Macronix SPI NAND Flash such as STM32L562QE-DK. Also, we are interested in participating Mbed-CE Flash driver maintaince. We can assit your Flash driver testing and provide testing data since you don't have enough resources to maintain it. As for the ONFI table, that's a parameter page in Flash to provide Flash information for user.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@daniel-0723 @multiplemonomials @daniel-723