Extending SPIF BD SFDP support #20
Conversation
|
Hello: @geky, @bulislaw , @donatieng , @ARMmbed/mbed-os-maintainers , @dannybenor , @davidsaada - I'd appreciate your review of SPIF Block Device extended SFDP support |
|
Hey @offirko, quick question. How does this relate to qspif-blockdevice and ARMmbed/mbed-os#7774? |
|
@geky, AFAIU the planned steps by order are:
@dannybenor - please correct me if I'm wrong |
| bd_size_t SPIFBlockDevice::get_erase_size() const | ||
| { | ||
| // return minimal erase size supported by all regions (0 if none exists) | ||
| return _min_common_erase_size; |
There was a problem hiding this comment.
Maybe this should assert if there's no common erase size? Most of the existing code doesn't expect errors from this function.
There was a problem hiding this comment.
SFDP's JEDEC Standard specifies several examples where memory is divided into several sector regions - some of them support only 4KB erase, and some only 64KB erase - I think we shouldn't assert at such case:
There was a problem hiding this comment.
@geky Maybe this seems harsh, but shouldn't we deprecate this function (the one that doesn't accept the address as parameter)? We have this problem in other cases as well (FlashIAP for instance).
There was a problem hiding this comment.
What does LittleFS call then? LittleFS only support fixed-size blocks, but that's the majority of the storage devices (especially larger ones).
My thought was if the blockdevice-level get_erase_size gets called on a device without a common block size then it's an error (shouldn't use a block-based filesystem on a block device without a common block size).
I double checked with LittleFS and FAT and it looks like both will error if get_erase_size returns 0, so maybe returning zero is a good idea.
| /*********************************************/ | ||
| /************** Local Functions **************/ | ||
| /*********************************************/ | ||
| static unsigned int local_math_power(int base, int exp) |
There was a problem hiding this comment.
Why not use std::pow? Then we could potentially share code.
There was a problem hiding this comment.
I think std::pow operates on float/double and would have required casting (not too critical)..
Also, using this small local function made it possible for me to avoid including <math.h> or "mbed.h"
There was a problem hiding this comment.
Oh you're right, I missed that there is no int overload... It was a small thought anyways
|
Sorry, had to revert the namespace mbed. It broke compatibility, which was being relied on by CI: 662c4a6 Was this the only blockdevice being moved into the mbed namespace? @deepikabhavnani has some plans to move to correct namespacing, but this will probably need to be a separate set of work: |
|
@geky - thanks for reverting namespace ... missed that with all the namespace recent flipflops |
No description provided.