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

added support for Micron MT29F nand flash (IEC-110) #327

Merged

Conversation

UnTraDe
Copy link
Contributor

@UnTraDe UnTraDe commented May 16, 2024

Checklist

  • Component contains License
  • Component contains README.md
  • Component contains idf_component.yml file with url field defined
  • Component was added to upload job
  • Component was added to build job
  • Optional: Component contains unit tests
  • CI passing

Change description

Add support for the Micron MT29F nand chip, tested specifically on MT29F4G01ABAFDWB

@CLAassistant
Copy link

CLAassistant commented May 16, 2024

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot changed the title added support for Micron MT29F nand flash added support for Micron MT29F nand flash (IEC-110) May 16, 2024
@UnTraDe
Copy link
Contributor Author

UnTraDe commented Jun 10, 2024

Hi, I don't really understand what the CI is failing at

@igrr
Copy link
Member

igrr commented Jun 10, 2024

@UnTraDe the failing clang-tidy job was fixed on the master branch, if your rebase your branch it should get fixed here as well.

Pre-commit check has noticed a minor formatting issue. If you install pre-commit hooks (pip install pre-commit && pre-commit install) then next time you make a commit in this repo, it will be automatically formatted. See https://pre-commit.com/ for more information about this tool.

(Alternatively, you can check the failing pre-commit job log and manually apply formatting changes as shown there.)

@igrr
Copy link
Member

igrr commented Jun 19, 2024

Just a formatting issue left

diff --git a/spi_nand_flash/src/nand.c b/spi_nand_flash/src/nand.c
index 61975a4..4791980 100644
--- a/spi_nand_flash/src/nand.c
+++ b/spi_nand_flash/src/nand.c
@@ -136,13 +136,13 @@ static esp_err_t spi_nand_micron_init(spi_nand_flash_device_t *dev)
         .command = CMD_READ_ID,
         .dummy_bits = 16,
         .miso_len = 1,
-        .miso_data = &device_id};
+        .miso_data = &device_id
+    };
     spi_nand_execute_transaction(dev->config.device_handle, &t);
     dev->read_page_delay_us = 25;
     dev->erase_block_delay_us = 10000;
     dev->program_page_delay_us = 600;
-    switch (device_id)
-    {
+    switch (device_id) {
     case MICRON_DI_34:
         dev->dhara_nand.num_blocks = 2048;
         dev->dhara_nand.log2_ppb = 6;        // 64 pages per block

@UnTraDe
Copy link
Contributor Author

UnTraDe commented Jun 19, 2024

I've fixed the formatting :)

@igrr
Copy link
Member

igrr commented Jun 19, 2024

@UnTraDe I'm sorry, I forgot one thing: please increase the version number in


to 0.2.0. Then your changes will be automatically released once the PR is merged.

@UnTraDe
Copy link
Contributor Author

UnTraDe commented Jun 19, 2024

P.S
This flash is relatively big (4Gb or 512 MB) with the following configuration:
image
So basically it has 2048*64 = 131072 addressable logical sectors, but the spi_nand_flash API accepts sector ID as uint16_t, so the entire flash cannot be utilized fully:

esp_err_t spi_nand_flash_read_sector(spi_nand_flash_device_t *handle, uint8_t *buffer, uint16_t sector_id);

It seems that the dhara library API uses dhara_sector_t which is defined as:

typedef uint32_t dhara_sector_t;

So it might be an easy change to just use uint32_t instead?

@igrr
Copy link
Member

igrr commented Jun 19, 2024

So it might be an easy change to just use uint32_t instead?

I see no downside to it... @RathiSonika WDYT?

@RathiSonika
Copy link
Collaborator

Yes, there shouldn't be an issue. In fact, it should be uint32_t. Thank you for pointing that out.

@UnTraDe
Copy link
Contributor Author

UnTraDe commented Jun 23, 2024

Should I make it part of this PR or open a separate one?

@UnTraDe
Copy link
Contributor Author

UnTraDe commented Jul 9, 2024

Is there anything else blocking this PR? 🤔

@igrr
Copy link
Member

igrr commented Jul 10, 2024

@UnTraDe Sorry for the late reply, I was on leave. You can either add the uint32_t change here or in a separate PR, up to you. If the current PR without uint32_t change is already useable on this flash chip, I guess we can release it first.

If you could clean up the commit history (git rebase -i to combine commits) before we merge the PR, it would be much appreciated.

@UnTraDe UnTraDe force-pushed the spi_nand_flash/support_for_micron_MT29F branch 2 times, most recently from bfb385b to 6b8aa6d Compare July 10, 2024 16:48
@UnTraDe
Copy link
Contributor Author

UnTraDe commented Jul 10, 2024

@igrr
I think it should be in a separate PR, the current version is usable, you just can't refer to the upper sectors.
I tried to rebase on master, but since I have merge in this branch it kind of messed up the PR, adding commits from other people in-line. What should I do? (I've reverted the rebase for now)

@UnTraDe
Copy link
Contributor Author

UnTraDe commented Jul 23, 2024

@igrr Sorry for bumping again, currently our project relies on cloning the fork locally, and specifying it's path relative to the Cargo.toml which causes a bit of a mess. Should I open a new branch and redo these changes to make the commit history a bit more straightforwad?
After that I'll open a new PR for the uint32_t change

@igrr igrr force-pushed the spi_nand_flash/support_for_micron_MT29F branch from 0b05d1c to 2f4efeb Compare July 23, 2024 10:47
@igrr
Copy link
Member

igrr commented Jul 23, 2024

@UnTraDe I've pushed one additional fix for CI to pass and cleaned up commit history. I'll merge it as soon as the pipeline passes.

@igrr igrr enabled auto-merge July 23, 2024 10:53
@igrr igrr merged commit 2da4e71 into espressif:master Jul 23, 2024
61 checks passed
@UnTraDe UnTraDe deleted the spi_nand_flash/support_for_micron_MT29F branch July 25, 2024 15:39
@UnTraDe UnTraDe mentioned this pull request Jul 28, 2024
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants