espressif / idf-extra-components

Additional components for ESP-IDF, maintained by Espressif
149 stars 89 forks source link

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

Closed UnTraDe closed 2 months ago

UnTraDe commented 4 months ago

Checklist

Change description

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

CLAassistant commented 4 months ago

CLA assistant check
All committers have signed the CLA.

UnTraDe commented 3 months ago

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

igrr commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

I've fixed the formatting :)

igrr commented 3 months ago

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

UnTraDe commented 3 months ago

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?

igrr commented 3 months ago

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

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

RathiSonika commented 3 months ago

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

UnTraDe commented 3 months ago

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

UnTraDe commented 2 months ago

Is there anything else blocking this PR? 🤔

igrr commented 2 months ago

@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 commented 2 months ago

@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 commented 2 months ago

@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 commented 2 months ago

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