-
Notifications
You must be signed in to change notification settings - Fork 14
pico-sdk: adapt for the sha256 driver to Zephyr #13
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
Conversation
@ajf58 @ThreeEights @petejohanson @kartben Could you take a look if you have a time, please? |
ChangeLog.zephyr.md
Outdated
Need to take care to not break these changes when updating pico-sdk. | ||
|
||
## Patch List: | ||
- [#10] pico-sdk: Disabling sanity check the IRQ status |
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.
- [#10] pico-sdk: Disabling sanity check the IRQ status | |
- [#10] pico-sdk: Disabling DMA routines and sanity check the IRQ status |
Crypto subsystem does support DMA though, so if this feature becomes desired, then should it be implemented in the Zephyr driver part? Couldn't the existing HAL code be used for DMA operations?
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.
DMA operations must be performed via the Zephyr DMA driver, so the HAL code cannot be used. DMA support must be implemented separately.
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.
Reasonable, most of that code just calls into HAL DMA methods
ChangeLog.zephyr.md
Outdated
Need to take care to not break these changes when updating pico-sdk. | ||
|
||
## Patch List: | ||
- [#11] pico-sdk: Disabling sanity check the IRQ status |
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.
please refrain from using "sanity check" term https://docs.zephyrproject.org/latest/contribute/coding_guidelines/index.html#rule-a-2-inclusive-language
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.
Thank you for pointing it out.
The changes made during the rebase were not reflected, resulting in inconsistent content.
So, I changed the entire sentence.
src/rp2_common/pico_sha256/sha256.c
Outdated
#if defined(__ZEPHYR__) | ||
void pico_sha256_write_padding(pico_sha256_state_t *state) { | ||
write_padding(state); | ||
} | ||
#endif |
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.
The #if !defined(__ZEPHYR__)
is useful to keep merging new SDK releases. However, I don't think the inverse is required.
#if defined(__ZEPHYR__) | |
void pico_sha256_write_padding(pico_sha256_state_t *state) { | |
write_padding(state); | |
} | |
#endif | |
void pico_sha256_write_padding(pico_sha256_state_t *state) { | |
write_padding(state); | |
} |
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 true that no one else had done it this way, so I reflected it.
Comment out unused DMA-related code and add wrappers to expose internal functions. Signed-off-by: TOKITA Hiroshi <[email protected]>
Add a note about the sha256 zephyr adaptation. Signed-off-by: TOKITA Hiroshi <[email protected]>
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.
All makes sense, good to go if it builds
Comment out unused DMA-related code and add wrappers to expose
internal functions.