idf-extra-components icon indicating copy to clipboard operation
idf-extra-components copied to clipboard

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

Open UnTraDe opened this issue 1 year ago • 1 comments

Checklist

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

Change description

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

UnTraDe avatar May 16 '24 10:05 UnTraDe

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar May 16 '24 10:05 CLAassistant

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

UnTraDe avatar Jun 10 '24 16:06 UnTraDe

@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 avatar Jun 10 '24 17:06 igrr

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

igrr avatar Jun 19 '24 03:06 igrr

I've fixed the formatting :)

UnTraDe avatar Jun 19 '24 08:06 UnTraDe

@UnTraDe I'm sorry, I forgot one thing: please increase the version number in https://github.com/espressif/idf-extra-components/blob/d9b31d9cc535c750c517bd700a86a1268072b69c/spi_nand_flash/idf_component.yml#L1 to 0.2.0. Then your changes will be automatically released once the PR is merged.

igrr avatar Jun 19 '24 09:06 igrr

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: https://github.com/espressif/idf-extra-components/blob/d9b31d9cc535c750c517bd700a86a1268072b69c/spi_nand_flash/include/spi_nand_flash.h#L50 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?

UnTraDe avatar Jun 19 '24 11:06 UnTraDe

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

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

igrr avatar Jun 19 '24 12:06 igrr

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

RathiSonika avatar Jun 19 '24 12:06 RathiSonika

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

UnTraDe avatar Jun 23 '24 07:06 UnTraDe

Is there anything else blocking this PR? 🤔

UnTraDe avatar Jul 09 '24 12:07 UnTraDe

@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.

igrr avatar Jul 10 '24 15:07 igrr

@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 avatar Jul 10 '24 16:07 UnTraDe

@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

UnTraDe avatar Jul 23 '24 08:07 UnTraDe

@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 avatar Jul 23 '24 10:07 igrr