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

[driver] Adding SpiStack Flash #1054

Merged
merged 7 commits into from Aug 13, 2023

Conversation

rasmuskleist
Copy link
Contributor

@rasmuskleist rasmuskleist commented Aug 3, 2023

I have changed the implementation of the BdSpiFlash to use the W25M512JV instruction set as discussed in #890. In addition, I have added a wrapper that makes the stacked die seem like a contiguous piece of memory for homogenous storage devices. This was also discussed in #890, although my implementation is somewhat different.

Currently, the dieSelect function does more than I probably want it to, so I would like your opinion on the following alternative implementation.

  1. Set/reset ADP bit before device reset inside the initialize function. I believe that this should cause all dies to enter the desired addressing mode upon device reset. However, not all devices implement WSR3 command, but in those cases this will probably be a NOP. Alternatively, the ADP bit can be set only for devices larger than 16MB.
  2. Do not reset the device inside the initialize function and implement a seperate reset function. With the stacked dies a reset casues all dies to enter the addressing mode specified by the ADP bit. In the initialize function the device should then enter 4B addressing, and inside BdSpiStack, a loop should then initialize all dies. Here En4BAM is only called when the device size is larger than 16MB
  3. Enter/Exit 4B addressing on every die select depending on the device size as currently implemented. This does add some overhead on each dieSelect.
  4. Use 4B commands e.g. PP4BA for devices larger than 16MB. This makes it very explicit to the device that 4B addressing is being used. However, logic will have to be implemented in both the spiOperations function and the read, program and erase functions.
  5. Use extended address register for 4B addressing. This is only for backward compatability with devices prior to 2011, so I think it is not worth considering.

Finally, the current implementation waits for the BUSY bit to clear after issuing a command. Although I do find it nice that the operation finishes before returning, it does block the driver from issuing commands that can be done while busy. For example with the SpiStack concurrent operations can be performed. Therefore I like your opinion on waiting for the BUSY bit to clear in the program and erase function before issuing the command? Thereby I can select another die and double the troughput. However, I should add that the performance gain is probably very small in practice unless I change the BdSpiStack addressing to be modulo erase block. This could possibly double the throughput, but since I do not know my own throughput at the moment, this is possibly better suited for a later PR!

Copy link
Member

@salkinium salkinium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like your opinion on the following alternative implementation.

I don't have enough understanding of flash chips to make an informed decision. All alternatives read like gibberish to me… gotta pass that potato back to you 😛

I like your opinion on waiting for the BUSY bit to clear in the program and erase function before issuing the command?

Could you flip it? Do an operation, then continue doing things until you really must wait again? That's how we typically do it in other drivers, basically moves the last wait in the function to the front if that makes sense here.

@rasmuskleist
Copy link
Contributor Author

I'm no expert on flash chips either! For the moment I will leave the selectDie function as is, unless you have any comments to it. Afterall it is working and the overhead is probably not that significant, especially compared to the erase/program durations.

I have flipped the waiting as suggested, and will confirm that it works with my W25M512VJ flash chip tonight. Then I will convert it to a PR. I should add that flipping it will add some overhead for example in the read fuction, but I believe it is worth it since the driver only waits when it has to. This will also make the flow of the tests seem strange as it can enter the memory test while still busy erasing the chip. I can ignore this as it is only aesthetic or I can make the busy function public and wait the threads manually. What do you think?

@rasmuskleist rasmuskleist marked this pull request as ready for review August 7, 2023 16:37
@salkinium
Copy link
Member

Yeah, you should probably expose the wait method, otherwise it'll be confusing.
You may also explicitly wait for completion sonetimes like when rebooting or going to sleep.

@rasmuskleist
Copy link
Contributor Author

rasmuskleist commented Aug 11, 2023

I have made the isBusy and waitWhileBusy function available in BdSpiFlash and implemented similar functions in BdSpiStackFlash. Here isBusy returns true if any of the stacked dies is busy which i presume is the desired behaviour. I also noticed whilst doing some further testing of integrating BdSpiFlash with FatFs that the nesting level should be increased in order to use write.

Currently, I do not see any operation in the BdSpiFlash implementation that can wait for completion, which is not already waiting. After all, the intention is not to wait for erase and program to finish, so concurrent operations can take place inside BdSpiStackFlash.

Copy link
Member

@salkinium salkinium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice driver! Please squash and then I'll merge it.

@rasmuskleist
Copy link
Contributor Author

rasmuskleist commented Aug 12, 2023

Thanks! Should I squash more than this?

It occured to me as I was testing the driver with fibers, that it is problematic if a void functions returns a result and vice versa. I have now fixed this issue.

@salkinium salkinium merged commit ba23833 into modm-io:develop Aug 13, 2023
13 checks passed
@salkinium salkinium added this to the 2023q3 milestone Aug 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants