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

[Draft] os/se/ameba: Optimize AmebaSmart SE APIs access #6612

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ziyik
Copy link
Contributor

@ziyik ziyik commented Jan 2, 2025

  • Remove Flash Lock Unlock for all SE APIs
  • Call Flash Lock Unlock for necessary APIs only

- Remove Flash Lock Unlock for all SE APIs
- Call Flash Lock Unlock for necessary APIs only
@sunghan-chang
Copy link
Contributor

@ziyik With this PR, could you let us know elapsed time of each function in before and after?

@ziyik
Copy link
Contributor Author

ziyik commented Jan 3, 2025

@ziyik With this PR, could you let us know elapsed time of each function in before and after?

This PR is created to check if it will improve the I2S noise issue, since it's not helping I would like to abandon it.
Is this okay?

@sunghan-chang
Copy link
Contributor

@ziyik This could be not enough to resolve the issue. But in my opinion, this is necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hello @ziyik

Isn't it necessary to call flash lock/unlock in key manager APIs?

ameba_hal_set_key
ameba_hal_get_key
ameba_hal_remove_key
ameba_hal_generate_key

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These APIs will modify or operate with the RAM key, and all Flash keys will be loaded in the RAM area during init.
These APIs do not interact with Flash.

Copy link
Contributor

Choose a reason for hiding this comment

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

and all Flash keys will be loaded in the RAM area during init
-> Do you mean se_initailize while booting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, during init, the factory key will be loaded into the RAM slot.

@ziyik
Copy link
Contributor Author

ziyik commented Jan 7, 2025

@ziyik With this PR, could you let us know elapsed time of each function in before and after?

I think you are concerned about the time needed for FLASH_Write_Lock(); and FLASH_Write_Unlock();.
I have tried to measure the time needed for both APIs, It causes less than a systick.

Below are the code used to measure:
tickbefore_sleep = SYSTIMER_TickGet();
FLASH_Write_Lock();
FLASH_Write_Unlock();
tickafter_sleep = SYSTIMER_TickGet();

printf("\n tickbefore_sleep = %d \n", tickbefore_sleep);
printf("\n tickafter_sleep = %d \n", tickafter_sleep);
printf("\n tick different = %d \n", tickafter_sleep - tickbefore_sleep);
printf("\n Time in us = %d \n", (((tickafter_sleep - tickbefore_sleep) * 1000 * 1000)/ (32 * 1024)));

Below is the print log:
tickbefore_sleep = 1032175

tickafter_sleep = 1032175

tick different = 0

Time in us = 0

I execute the same code multiple times, most of the time is consumes 0 ticks, and only a few time it consumes 1 tick, which is like 30us.

@zhongnuo-tang
Copy link
Contributor

is PR, could you let us know elapsed time of each function in before and after?

Hi, I don’t think this aligns with what Mr. Chang was asking. Instead of measuring the time taken for FLASH_Write_Lock() and FLASH_Write_Unlock(), the focus should be on measuring the execution time of each Secure Execution (SE) API.

The main concern is how long each SE API takes, as optimizing and removing unnecessary FLASH_Write_Lock() calls will help reduce the time IRQs are disabled, leading to better performance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants