-
Notifications
You must be signed in to change notification settings - Fork 8.2k
drivers: flash: Introduce BFLB flash driver. #96680
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
base: main
Are you sure you want to change the base?
Conversation
|
The following west manifest projects have changed revision in this Pull Request:
✅ All manifest checks OK Note: This message is automatically posted and updated by the Manifest GitHub Action. |
cd0455e to
e821c9d
Compare
5db0053 to
2a4c5d9
Compare
|
@de-nordic PTAL? Thanks. |
drivers/flash/flash_bflb.c
Outdated
| if (length == 0) { | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In all cases, lack of action should not generate an error. This means the length check should go above range check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| if (offset < 0) { | ||
| LOG_WRN("0x%lx: before start of flash", (long)offset); | ||
| return -EINVAL; | ||
| } | ||
| if ((offset + len) > TOTAL_SIZE) { | ||
| LOG_WRN("0x%lx: ends past the end of flash", (long)offset); | ||
| return -EINVAL; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to separate checks, imho.
The check should be:
offset < 0 || len > TOTAL_SIZE || ((TOTAL_SIZE - offset) < len)
above avoids integer overflows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separated errors for different problems however, are useful. Done
51322a4 to
5e98d2d
Compare
drivers/flash/flash_bflb.c
Outdated
| return ret; | ||
| } | ||
|
|
||
| if (len == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Len check above range valid check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, counted 3, forgetting there's 2 different read... Done
5e98d2d to
0a04e32
Compare
|
@de-nordic PTAL |
drivers/flash/flash_bflb.c
Outdated
| static bool flash_bflb_is_in_xip(void *func) | ||
| { | ||
| if ((uint32_t)func > BFLB_XIP_BASE && (uint32_t)func < BFLB_XIP_END) { | ||
| LOG_ERR("function at %d is in XIP and will crash the device", (uint32_t)func); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably want to use %p instead of %d and avoid casting the func.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
drivers/flash/flash_bflb.c
Outdated
|
|
||
| #if defined(CONFIG_SOC_SERIES_BL70X) || defined(CONFIG_SOC_SERIES_BL60X) | ||
|
|
||
| static void flash_bflb_l1c_wrap(bool yes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not my driver, but yes for a bool variable is not descriptive of the purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's the same as 'writing?' yes/no, l1c_wrap? yes/no
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
|
||
| static void flash_bflb_l1c_wrap(bool yes) | ||
| { | ||
| /* Do nothing on Bl61x: no L1C */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You will probably get here unused variable warning from static analysis, so it would be good to have ARG_UNUSED(yes) here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
drivers/flash/flash_bflb.c
Outdated
| if (data_mode == 0) { | ||
| return 0; | ||
| } else if (data_mode == 1) { | ||
| return 1; | ||
| } else if (data_mode == 2) { | ||
| return 2; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just return data_mode here.
Shouldn't be here some asserts, for validation builds, to check if only values covered by ifs are processed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| tmp &= ~SF_CTRL_SF_IF_0_DAT_EN_MSK; | ||
| } | ||
|
|
||
| /* writing ? */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not know. Are we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
drivers/flash/flash_bflb.c
Outdated
| erase_cmd.rw = 0; | ||
| erase_cmd.addr_size = 3; | ||
|
|
||
| for (uint32_t i = start / ERASE_SIZE; i < ((len / ERASE_SIZE) + start / ERASE_SIZE); i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On some CPUs it is better avoid division when possible, and this loop will have at least two of these every run, if fail to optimize. Consider pre calculating loop-independent ending condition before defining loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only runs on CPUs that are fine with that and all have FPUs as well, will add the precalculations however.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
drivers/flash/flash_bflb.c
Outdated
| ret = flash_bflb_restore_xip_state(data); | ||
| irq_unlock(locker); | ||
|
|
||
| return ret; | ||
|
|
||
| exit_here: | ||
| flash_bflb_restore_xip_state(data); | ||
| irq_unlock(locker); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as flash_bflb_write
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
drivers/flash/flash_bflb.c
Outdated
| /* from SDK because the zephyr CRC are no good for the bflb flash crc, this is supposedly a | ||
| * implementation of ZIP crc32 | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No good in what respect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a different algorithm / gives different results
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| /* copy cfg reg to memory as cfg will not be in it and inaccessible */ | ||
| data->reg_copy = cfg->reg; | ||
|
|
||
| /* get flash config using xip access */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
? Some leftover?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it is designating all the code under it until the unlock
drivers/flash/flash_bflb.c
Outdated
| irq_unlock(locker); | ||
| return ret; | ||
| } | ||
| /* TODO: AES flash support goes here */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ever gonna do that TODO thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's for a future feature update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
0a04e32 to
2cf8917
Compare
Introduce Bouffalolab Flash Controller with support for bank 1 Signed-off-by: Camille BAUD <[email protected]>
Enable the Flash Controller Signed-off-by: Camille BAUD <[email protected]>
Enable the flash controller. Relocation is required. Signed-off-by: Camille BAUD <[email protected]>
Adds the flash to supported for testing Signed-off-by: Camille BAUD <[email protected]>
2cf8917 to
562f09f
Compare
|
GREAT again hitting some global changes that aren't related to my PR! |
|
Please retry analysis of this Pull-Request directly on SonarQube Cloud |
Introduce flash driver for 1 bank of flash (all of them on bl60x and bl70x, half of them on bl61x)
Caveat: no bank2 support (feature of bl61x)
Not sure how to handle the flash naming and devices as they are either separate die flash or external flash but need the controller to work (and so be soc-nv-flash).