drivers: crypto: Add initial support for rpi_pico sha256 accelerator #85036

soburi · 2025-02-03T01:49:02Z

Add basic support for RaspberryPi Pico's SHA256 hardware accelerator.

soburi · 2025-02-14T22:22:08Z

Could you take a look if you have a bit of a while?

valeriosetti

The PR looks OK to me. I cannot test it locally because I don't have a board at hand, but at least the test that was extended builds correctly. I only left one minor non-blocking question/comment.

drivers/crypto/crypto_rpi_pico_sha256.c

ThreeEights · 2025-02-17T16:40:23Z

drivers/crypto/CMakeLists.txt

+zephyr_library_sources_ifdef(CONFIG_CRYPTO_SMARTBOND            crypto_smartbond.c)
+zephyr_library_sources_ifdef(CONFIG_CRYPTO_STM32                crypto_stm32.c)
+zephyr_library_sources_ifdef(CONFIG_CRYPTO_TINYCRYPT_SHIM       crypto_tc_shim.c)
+# zephyr-keep-sorted-stop


Sorting - good idea!

nit: I'd prefer it if the sorting is one commit, and the addition is a separate commit. The end result is the same, but it's easier to to see the two orthogonal things going on.

valeriosetti

Thanks for addressing previous comment. However reviewing the PR again I found a couple of new ones that I missed before. Sorry

drivers/crypto/crypto_rpi_pico_sha256.c

soburi · 2025-03-17T20:04:28Z

@ceolin @ThreeEights @ajf58

Could you take a look at it?

ajf58

The change looks functionally good, but I am concerned about the outright copy and paste of BSD code, then relabeling it as Apache 2.

I feel like someone else needs to comment on this.

ajf58 · 2025-03-17T21:23:15Z

drivers/crypto/CMakeLists.txt

+zephyr_library_sources_ifdef(CONFIG_CRYPTO_SMARTBOND            crypto_smartbond.c)
+zephyr_library_sources_ifdef(CONFIG_CRYPTO_STM32                crypto_stm32.c)
+zephyr_library_sources_ifdef(CONFIG_CRYPTO_TINYCRYPT_SHIM       crypto_tc_shim.c)
+# zephyr-keep-sorted-stop


nit: I'd prefer it if the sorting is one commit, and the addition is a separate commit. The end result is the same, but it's easier to to see the two orthogonal things going on.

ajf58 · 2025-03-17T21:53:37Z

drivers/crypto/crypto_rpi_pico_sha256.c

+LOG_MODULE_REGISTER(sha_rpi_pico_sha256, CONFIG_CRYPTO_LOG_LEVEL);
+
+/*
+ * This is based on the pico_sha256 implementation in the Pico-SDK.


Original code is a https://github.com/raspberrypi/pico-sdk/blob/2.1.0/src/rp2_common/pico_sha256/sha256.c , it's 3-Clause BSD (BSD-3-Clause).

I feel like this file has lifted-and-shifted too much for someone to ignore (many lines of code are the same, just formatted to meet the Zephyr Project's coding style).

I'd be happy if this file had a BSD-3-Clause on it. Zephyr must be compatible with this, we link against BSD-3-Clause all over the place. It's not easy t osee examples of that, however.

Alternatively, we could modify the version of the source code in the hal_rpi_pico. This is something I prefer to avoid, but that feels better at ensuring the software licence is upheld.

The codes derived from pico-sdk were separated into crypto_rpi_pico_sha256.h, and the license changed to BSD-3.

nit: I'd prefer it if the sorting is one commit, and the addition is a separate commit. The end result is the same, but it's easier to to see the two orthogonal things going on.

Addressed.

Add `zephyr-keep-sorted` and reorder file entries alphabetically. Signed-off-by: TOKITA Hiroshi <[email protected]>

Add `hardware_sha256/include` to `zephyr_include_directories`. Use the header only. Signed-off-by: TOKITA Hiroshi <[email protected]>

Add basic support for RaspberryPi Pico's SHA256 hardware accelerator. Signed-off-by: TOKITA Hiroshi <[email protected]>

Added sha256 accelerator to rpi_pico. Signed-off-by: TOKITA Hiroshi <[email protected]>

Add rpi_pico2 to the crypto_hash test target. Signed-off-by: TOKITA Hiroshi <[email protected]>

soburi · 2025-03-19T03:56:28Z

This PR contains BSD-3 derived code. (drivers/crypto/crypto_rpi_pico_sha256.h)
I added the label because I wanted to get TSC approval.

ajf58 · 2025-03-19T19:44:30Z

This PR contains BSD-3 derived code. (drivers/crypto/crypto_rpi_pico_sha256.h) I added the label because I wanted to get TSC approval.

I think, based on the conversation at #81183 (comment) that this won't be a viable solution.

My suggestion is

Change (extend) https://github.com/zephyrproject-rtos/hal_rpi_pico/blob/zephyr/src/rp2_common/pico_sha256/sha256.c in a PR. This already is BSD-3-Clause, nothing would change there.
Update the west manifest for Zephyr to bring in the new HAL.
Make use of the new code from here.

I know that this may feel contradictory to an earlier stance I have of "don't change the Pico SDK unless you have to." I feel like this is one of those times.

That said, I also hope we can do this in a way that makes wiring up the DMA engine to the SHA256 accelerator easy: it's a prime example of where one wants to offload this whilst the CPUs do something more useful.

soburi · 2025-03-20T01:48:06Z

@ajf58

That said, I also hope we can do this in a way that makes wiring up the DMA engine to the SHA256 accelerator easy: it's a prime example of where one wants to offload this whilst the CPUs do something more useful.

DMA processing must be based on Zephyr's API, and I have determined that this part cannot be used. This is the main reason why I forked this file.

ajf58 · 2025-03-22T08:54:40Z

@ajf58

That said, I also hope we can do this in a way that makes wiring up the DMA engine to the SHA256 accelerator easy: it's a prime example of where one wants to offload this whilst the CPUs do something more useful.

DMA processing must be based on Zephyr's API, and I have determined that this part cannot be used. This is the main reason why I forked this file.

Yes, I understand what problem you found. The approach you took was to breach the terms of the licence, which is not cool. "fork"ing a file is not a thing.

My suggestion is written out above as 3 steps. If you don't understand the steps as written, please ask specific clarifying questions.

soburi · 2025-03-22T11:15:03Z

@ajf58

Yes, I understand what problem you found. The approach you took was to breach the terms of the licence, which is not cool. "fork"ing a file is not a thing.

I've never heard of the BSD 3 clause license code can't forking.

ajf58 · 2025-03-22T12:51:33Z

@ajf58

Yes, I understand what problem you found. The approach you took was to breach the terms of the licence, which is not cool. "fork"ing a file is not a thing.

I've never heard of the BSD 3 clause license code can't forking.

Your original approach was to:

Remove the original copyright notice. Copyright (c) 2024 Raspberry Pi (Trading) Ltd.
Add your own name as the Copyright holder
Reissue the code under the Apache 2.0 license

This, to me, is a concerning behaviour. It should not require other people to check/monitor/flag that you are breaching other licence terms, you should do this yourself.

After I flagged this you have subsequently corrected this by putting back the original copyright notice, and license. From a technical perspective I don't like how this has been achieved, but that is not the concern I am raising here.

soburi · 2025-03-22T15:27:11Z

@ajf58

Your original approach was to:
1. Remove the original copyright notice. [Copyright (c) 2024 Raspberry Pi (Trading) Ltd.](https://github.com/raspberrypi/pico-sdk/blob/2.1.0/src/rp2_common/pico_sha256/sha256.c#L2)

2. Add your own name as the Copyright holder

3. Reissue the code under the Apache 2.0 license
This, to me, is a concerning behaviour. It should not require other people to check/monitor/flag that you are breaching other licence terms, you should do this yourself.

After I flagged this you have subsequently corrected this by putting back the original copyright notice, and license. From a technical perspective I don't like how this has been achieved, but that is not the concern I am raising here.

This is exactly as you pointed out.
I had mentioned that I was using it, but I had not properly addressed the license.
Thank you for pointing that out.

soburi marked this pull request as draft February 3, 2025 01:49

zephyrbot added area: Crypto / RNG platform: Raspberry Pi Pico Raspberry Pi Pico (RPi Pico) labels Feb 3, 2025

zephyrbot requested review from ceolin, ThreeEights and yonsch February 3, 2025 01:49

zephyrbot assigned ceolin Feb 3, 2025

soburi mentioned this pull request Feb 3, 2025

TaskList for Supporting RaspberryPi Pico peripherals #53810

Open

52 tasks

soburi force-pushed the rpi_pico_sha256 branch from 0d6e6b8 to a2b3bb7 Compare February 3, 2025 02:32

soburi added the DNM This PR should not be merged (Do Not Merge) label Feb 3, 2025

soburi marked this pull request as ready for review February 3, 2025 04:02

valeriosetti self-requested a review February 10, 2025 08:31

soburi force-pushed the rpi_pico_sha256 branch from a2b3bb7 to 19c04b7 Compare February 11, 2025 20:31

soburi removed the DNM This PR should not be merged (Do Not Merge) label Feb 11, 2025

valeriosetti reviewed Feb 17, 2025

View reviewed changes

drivers/crypto/crypto_rpi_pico_sha256.c Outdated Show resolved Hide resolved

ThreeEights reviewed Feb 17, 2025

View reviewed changes

valeriosetti mentioned this pull request Feb 19, 2025

Add crypto support to TI cc23x0 SoC #84523

Merged

soburi force-pushed the rpi_pico_sha256 branch from 19c04b7 to 3f8ef55 Compare March 8, 2025 01:59

zephyrbot requested a review from ajf58 March 8, 2025 02:00

soburi requested review from valeriosetti and ThreeEights March 8, 2025 03:34

valeriosetti requested changes Mar 10, 2025

View reviewed changes

drivers/crypto/crypto_rpi_pico_sha256.c Outdated Show resolved Hide resolved

drivers/crypto/crypto_rpi_pico_sha256.c Outdated Show resolved Hide resolved

soburi force-pushed the rpi_pico_sha256 branch 2 times, most recently from a6aeaeb to f2ca146 Compare March 10, 2025 22:42

soburi requested a review from valeriosetti March 11, 2025 05:36

soburi force-pushed the rpi_pico_sha256 branch from f2ca146 to ee650de Compare March 14, 2025 13:15

valeriosetti previously approved these changes Mar 14, 2025

View reviewed changes

ajf58 reviewed Mar 17, 2025

View reviewed changes

soburi added 5 commits March 18, 2025 07:56

drivers: crypto: sort file entries

5063e49

Add `zephyr-keep-sorted` and reorder file entries alphabetically. Signed-off-by: TOKITA Hiroshi <[email protected]>

modules: hal_rpi_pico: Add hardware_sha256 to include directories

c491aa4

Add `hardware_sha256/include` to `zephyr_include_directories`. Use the header only. Signed-off-by: TOKITA Hiroshi <[email protected]>

drivers: crypto: Add initial support for rpi_pico sha256 accelerator

94970f2

Add basic support for RaspberryPi Pico's SHA256 hardware accelerator. Signed-off-by: TOKITA Hiroshi <[email protected]>

boards: raspberrypi: rpi_pico2: Add sha256 accelerator

f7a50c1

Added sha256 accelerator to rpi_pico. Signed-off-by: TOKITA Hiroshi <[email protected]>

tests: crypto: crypto_hash: Add rpi_pico2 to test target

90f02be

Add rpi_pico2 to the crypto_hash test target. Signed-off-by: TOKITA Hiroshi <[email protected]>

soburi dismissed valeriosetti’s stale review via 90f02be March 17, 2025 23:00

soburi force-pushed the rpi_pico_sha256 branch from ee650de to 90f02be Compare March 17, 2025 23:00

soburi added the TSC Topics that need TSC discussion label Mar 19, 2025

github-project-automation bot added this to TSC Attention Needed Mar 19, 2025

github-project-automation bot moved this to Todo in TSC Attention Needed Mar 19, 2025

soburi marked this pull request as draft March 22, 2025 15:27

soburi removed the TSC Topics that need TSC discussion label Mar 22, 2025

nashif added the Licensing The PR has licensing issues => licensing expert to review label Apr 16, 2025

nashif moved this from Todo to In Progress in TSC Attention Needed Apr 24, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: crypto: Add initial support for rpi_pico sha256 accelerator #85036

drivers: crypto: Add initial support for rpi_pico sha256 accelerator #85036

soburi commented Feb 3, 2025 •

edited

Loading

soburi commented Feb 14, 2025

valeriosetti left a comment

ThreeEights Feb 17, 2025

ajf58 Mar 17, 2025

valeriosetti left a comment

soburi commented Mar 17, 2025

ajf58 left a comment

ajf58 Mar 17, 2025

ajf58 Mar 17, 2025

soburi Mar 17, 2025

soburi Mar 17, 2025

soburi commented Mar 19, 2025

ajf58 commented Mar 19, 2025 •

edited

Loading

soburi commented Mar 20, 2025

ajf58 commented Mar 22, 2025

soburi commented Mar 22, 2025

ajf58 commented Mar 22, 2025

soburi commented Mar 22, 2025

drivers: crypto: Add initial support for rpi_pico sha256 accelerator #85036

Are you sure you want to change the base?

drivers: crypto: Add initial support for rpi_pico sha256 accelerator #85036

Conversation

soburi commented Feb 3, 2025 • edited Loading

soburi commented Feb 14, 2025

valeriosetti left a comment

Choose a reason for hiding this comment

ThreeEights Feb 17, 2025

Choose a reason for hiding this comment

ajf58 Mar 17, 2025

Choose a reason for hiding this comment

valeriosetti left a comment

Choose a reason for hiding this comment

soburi commented Mar 17, 2025

ajf58 left a comment

Choose a reason for hiding this comment

ajf58 Mar 17, 2025

Choose a reason for hiding this comment

ajf58 Mar 17, 2025

Choose a reason for hiding this comment

soburi Mar 17, 2025

Choose a reason for hiding this comment

soburi Mar 17, 2025

Choose a reason for hiding this comment

soburi commented Mar 19, 2025

ajf58 commented Mar 19, 2025 • edited Loading

soburi commented Mar 20, 2025

ajf58 commented Mar 22, 2025

soburi commented Mar 22, 2025

ajf58 commented Mar 22, 2025

soburi commented Mar 22, 2025

soburi commented Feb 3, 2025 •

edited

Loading

ajf58 commented Mar 19, 2025 •

edited

Loading