Skip to content

Add crypto support to TI cc23x0 SoC #84523

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

Merged

Conversation

jpanisbl
Copy link
Contributor

@jpanisbl jpanisbl commented Jan 24, 2025

This series adds crypto support to TI cc23x0 SoC.

Datasheet: https://www.ti.com/lit/ds/symlink/cc2340r5.pdf

@zephyrbot
Copy link
Collaborator

zephyrbot commented Jan 24, 2025

The following west manifest projects have changed revision in this Pull Request:

Name Old Revision New Revision Diff

All manifest checks OK

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@jpanisbl jpanisbl force-pushed the baylibre/upstream-cc23x0-crypto branch from ad39107 to da7684f Compare February 4, 2025 14:06
@jpanisbl jpanisbl marked this pull request as ready for review February 4, 2025 16:23
@zephyrbot zephyrbot added area: Crypto / RNG platform: TI SimpleLink Texas Instruments SimpleLink MCU labels Feb 4, 2025
@valeriosetti valeriosetti self-requested a review February 10, 2025 08:30
@kartben kartben added this to the v4.2.0 milestone Feb 17, 2025
Copy link
Collaborator

@valeriosetti valeriosetti left a comment

Choose a reason for hiding this comment

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

The overall implementation looks good to me. I only left few suggestions for improvements (some of which are also optional)

@valeriosetti
Copy link
Collaborator

valeriosetti commented Feb 19, 2025

One last suggestion: it could be nice to extend zephyr/samples/drivers/crypto/ sample to add support for your driver (as done for example in #85036). This allows everybody using the same board that you are using to run this sample and validate the driver.

@jpanisbl jpanisbl force-pushed the baylibre/upstream-cc23x0-crypto branch from da7684f to f68a815 Compare February 19, 2025 17:02
@zephyrbot zephyrbot added the area: Samples Samples label Feb 19, 2025
@zephyrbot zephyrbot requested review from kartben and nashif February 19, 2025 17:03
valeriosetti
valeriosetti previously approved these changes Feb 19, 2025
Copy link
Collaborator

@valeriosetti valeriosetti left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my comments. LGTM :)

ceolin
ceolin previously approved these changes Feb 21, 2025
Copy link
Member

@ceolin ceolin left a comment

Choose a reason for hiding this comment

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

I have only one concern about the semaphore usage across crypto operations because I don't know how the hw operates.

From the driver interface perspective it looks good.

}

/* Load key */
AESWriteKEY(ctx->key.bit_stream);
Copy link
Member

Choose a reason for hiding this comment

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

I am wondering if you shouldn't protect this call with the semaphore. What happens if another thread changes the key while there is an operation happening ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your time Flavio.
You're right, that's something that must be addressed. That's also the case for the whole encrypt/decrypt operation (not only for the key).

Copy link
Contributor Author

@jpanisbl jpanisbl Feb 21, 2025

Choose a reason for hiding this comment

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

Also, I read more carefully the documentation about "mutexes VS semaphores". and it seems to me that I should replace the existing mutex (currently used to protect the session) with a semaphore. The mutex I'm using to protect the session may not do the trick, since it can be locked twice by the same thread. I will double-check with @valeriosetti .
IOW, there will probably be 2 semaphores: 1 for the session, and 1 for the access to HW (similarly to crypto_stm32.c).

Copy link
Collaborator

Choose a reason for hiding this comment

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

After some internal discussion we agreed to:

  • remove the session semaphore/mutex. Rationale: we don´t care if multiple thread start multiple sessions; we only care not to use AES HW at the same time.
  • change device_sem to be a mutex and set it to protect any access to the AES HW (mutex over semaphore in this case because mutexes are thread-aware).
  • keep the aes_done of course for HW synchronization.

This is the same as crypto_si32.c and crypto_stm32.c.

@jpanisbl jpanisbl dismissed stale reviews from ceolin and valeriosetti via 3d850a4 February 21, 2025 10:34
@jpanisbl jpanisbl force-pushed the baylibre/upstream-cc23x0-crypto branch from f68a815 to 3d850a4 Compare February 21, 2025 10:34
@jpanisbl
Copy link
Contributor Author

jpanisbl commented Feb 21, 2025

As a complement, using the sample that is part of this PR:

*** Booting Zephyr OS build 0e205aafcff5 ***
I: Cipher Sample
I: ECB Mode
D: AES operation completed
I: Output length (encryption): 16
I: ECB mode ENCRYPT - Match
E: ECB decryption not supported by the hardware
I: CBC Mode
E: Unsupported mode
I: CTR Mode
D: AES operation completed
D: AES operation completed
D: AES operation completed
D: AES operation completed
I: Output length (encryption): 64
I: CTR mode ENCRYPT - Match
D: AES operation completed
D: AES operation completed
D: AES operation completed
D: AES operation completed
I: Output length (decryption): 64
I: CTR mode DECRYPT - Match
I: CCM Mode
D: Compute CMAC
D: AES operation completed (block 0)
D: AES operation completed (block 1)
D: AES operation completed (data)
D: AES operation completed (data)
D: Encrypt data
D: AES operation completed
D: AES operation completed
D: Encrypt tag
D: AES operation completed
I: Output length (encryption): 23
I: CCM mode ENCRYPT - Match
D: Decrypt data
D: AES operation completed
D: AES operation completed
D: Compute CMAC
D: AES operation completed (block 0)
D: AES operation completed (block 1)
D: AES operation completed (data)
D: AES operation completed (data)
D: Encrypt tag
D: AES operation completed
D: Check tag
I: Output length (decryption): 23
I: CCM mode DECRYPT - Match
I: GCM Mode
E: Unsupported mode

@jpanisbl jpanisbl force-pushed the baylibre/upstream-cc23x0-crypto branch from 3d850a4 to 9048839 Compare February 21, 2025 14:34
Add support for AES module to cc23x0 SoC. The driver supports the
following modes:
- ECB encryption only (since decryption is not supported by the HW)
- CTR
- CCM

Signed-off-by: Stoyan Bogdanov <[email protected]>
Signed-off-by: Julien Panis <[email protected]>
Add support for AES module to cc23x0 SoC.

Signed-off-by: Stoyan Bogdanov <[email protected]>
Signed-off-by: Julien Panis <[email protected]>
Enable AES module.

Signed-off-by: Stoyan Bogdanov <[email protected]>
Signed-off-by: Julien Panis <[email protected]>
For the CCM mode, increase slightly the output buffer size because
the cc23x0 directly fills this buffer with 16-byte blocks.
This prevents from doing unnecessary memcpy that would have an
impact on the driver's performance.

Signed-off-by: Julien Panis <[email protected]>
@jpanisbl jpanisbl force-pushed the baylibre/upstream-cc23x0-crypto branch from 9048839 to 0e205aa Compare February 21, 2025 16:03
Copy link
Member

@ceolin ceolin left a comment

Choose a reason for hiding this comment

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

Thanks guys !

@carlescufi carlescufi merged commit 52ceb6a into zephyrproject-rtos:main Mar 7, 2025
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants