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

hw/drivers/flash/spiflash: Add option to ignore JEDEC ID #2858

Merged
merged 1 commit into from
Aug 3, 2022

Conversation

StarGate01
Copy link
Contributor

This PR introduces three new optional configuration parameters: SPIFLASH_IGNORE_MANUFACTURER, SPIFLASH_IGNORE_MEMORY_TYPE, and SPIFLASH_IGNORE_MEMORY_CAPACITY. (Default: 0).

These parameters selectively disable the whitelist checks of the SPI Flash JEDEC IDs.

Rationale

I am currently porting the InfiniTime OS (written for the PineTime smartwatch) to a popular Chinese smartwatch series called "P8". mynewt-core is used for the bootloader.

These P8 watches come in a whole zoo of variants, each being compatible with the firmware but containing sometimes a bit different chips due to the silicon shortage and general chip availability on the global market. This is usually no problem to handle.

However, the SPI driver of mynewt-core enforces a strict whitelist on the reported SPI Flash JEDEC ID. I see how this is useful in normal applications where the hardware configuration is known in detail and compatibility has to be ensured, however this leads to my bootloader refusing to work on prior unknown / new smartwatch variants, bricking them.

In the past, others and I have added definitions for SPI IDs (e.g. #2798 , #2582 ), but I can't keep up with all the models and I don't want to brick any more watches due to a purely whitelisting (not technical) issue.

I am open to suggestions, I think these optional configuration parameters would be quite useful to have.

@StarGate01
Copy link
Contributor Author

@kasjer Is this something you would consider merging? The feature would be opt-in.

@kasjer
Copy link
Contributor

kasjer commented Jul 21, 2022 via email

@kasjer
Copy link
Contributor

kasjer commented Jul 25, 2022

@StarGate01 maybe you could change your commit a little bit to be more consistent with mynewt style that requires opening brace { to be on the same line as if.
Since new syscfg values have default value 0 it can be safely used in plain if (not #if)
Instead of having:

#if !MYNEWT_VAL(SPIFLASH_IGNORE_MANUFACTURER)
        assert(manufacturer == supported_chips[0].fc_jedec_id.ji_manufacturer);
        if (manufacturer != supported_chips[0].fc_jedec_id.ji_manufacturer) {
            rc = -1;
            goto err;
        }
#endif

we would have:

        assert(!MYNEWT_VAL(SPIFLASH_IGNORE_MANUFACTURER) &&
               manufacturer == supported_chips[0].fc_jedec_id.ji_manufacturer);
        if (!MYNEWT_VAL(SPIFLASH_IGNORE_MANUFACTURER) &&
            manufacturer != supported_chips[0].fc_jedec_id.ji_manufacturer) {
            rc = -1;
            goto err;
        }

generate code would be the same and compiler would check syntax that otherwise would not be checked in some build depending on ignore flags and mynewt style would be preserved without additional pre-processor conditions added for style sake only.

You can amend your commit and force push.

@StarGate01
Copy link
Contributor Author

@kasjer Thank you for the style comments; I have applied your suggestions.

@apache-mynewt-bot
Copy link

Style check summary

No suggestions at this time!

@kasjer
Copy link
Contributor

kasjer commented Aug 3, 2022

@StarGate01 thanks for contributions

@kasjer kasjer merged commit db05f05 into apache:master Aug 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants