zephyr icon indicating copy to clipboard operation
zephyr copied to clipboard

Introduction of Atmosic Platform

Open hzarnani opened this issue 1 year ago • 12 comments

This is Atmosic Technologies' first set of changes enabling the build of samples/hello_world for board ATMEVK-3330-QN-5.

More to come in future pull requests are additional boards in the same family of BLE SoC platforms (atm33evk) as well as similar content for two other families of platforms.

This change set is intentionally kept small to make it easy to review and get through the first level of issues such as setting up a new HAL for our platform within the zephyrproject-rtos space. The content can be found here: https://github.com/Atmosic/_hal_atmosic.

hzarnani avatar Apr 18 '24 02:04 hzarnani

Hello @hzarnani, and thank you very much for your first pull request to the Zephyr project! Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary. If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

github-actions[bot] avatar Apr 18 '24 02:04 github-actions[bot]

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff
hal_atmosic None c760881bad95fa31d8906508880fab676ba8d6f0 N/A

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

zephyrbot avatar Apr 18 '24 02:04 zephyrbot

Needs converting to hwmv2

Thanks for the feedback. Will do.

hzarnani avatar Apr 24 '24 01:04 hzarnani

Initial comments after having a very quick look at the hal repo:

  • You're adding a sample to the hal repo, why?
  • You're adding ramfunc and other linker files to the repo, why?
  • You have mbedtls and micro ecc in the repo, zephyr doesn't even use micro ecc, why is it there?
  • You are seemingly adding a bunch of random files depending upon configuration options, and hidden compiling with libraries e.g. mbedtls - this looks wrong
  • There are parts of the code doing things with bluetooth, does the device work with the zephyr bluetooth controller and host without binary blobs? If not, that should not be there
  • You're adding what looks to be hidden libraries e.g. "AT command support", what is this? Libraries for functionality should be in zephyr itself not a hal directory, we do not allow hidden hal libraries like that, hal is for hardware

nordicjm avatar Apr 25 '24 06:04 nordicjm

Initial comments after having a very quick look at the hal repo:

  • You're adding a sample to the hal repo, why?

Yes, the spe example is a minimal secure support application (much smaller than TF-M) to enable Zephyr applications to run in nonsecure mode.

  • You're adding ramfunc and other linker files to the repo, why?

Yes, these are necessary to support binary libraries in our self-hosted repositories.

  • You have mbedtls and micro ecc in the repo, zephyr doesn't even use micro ecc, why is it there?

These support mbedtls crypto acceleration on our platform through a combination of hardware and optimized software.

  • You are seemingly adding a bunch of random files depending upon configuration options, and hidden compiling with libraries e.g. mbedtls - this looks wrong

We have a significant amount of code supporting multiple platforms and environments in self-hosted repositories that aren't shown here. This is our first attempt to peel out a reasonable set of software to upstream to zephyrproject.org.

If the feedback here is that this first step should reduce hal_atmosic to absolutely nothing more than the minimal hardware peripherals necessary for hello_world and then grow it later, we can go that route.

  • There are parts of the code doing things with bluetooth, does the device work with the zephyr bluetooth controller and host without binary blobs? If not, that should not be there

Our platforms use the zephyr bluetooth host, but we have our own bluetooth controller, which has to be in binary form for a variety of reasons.

  • You're adding what looks to be hidden libraries e.g. "AT command support", what is this? Libraries for functionality should be in zephyr itself not a hal directory, we do not allow hidden hal libraries like that, hal is for hardware

We can move this to our self-hosted repos, if it is offensive.

dt-atmosic avatar Apr 26 '24 18:04 dt-atmosic

Fundamental problems:

  1. The HAL (hal_atmosic) uses Zephyr APIs

We should be able to migrate that content out to our self-hosted repos.

  1. The HAL contains CMake and Kconfig, those really belong in zephyr/modules
  2. No zephyr/modules/hal_atmosic folder

A little history here - we began our development back on Zephyr v0.6, and things have changed many times since then. Is there a definitive document that covers 2 and 3?

  1. The HAL contains too much stuff that has nothing to do with a HAL

We can migrate that content out to our self-hosted repos.

dt-atmosic avatar Apr 26 '24 18:04 dt-atmosic

Yes, these are necessary to support binary libraries in our self-hosted repositories.

Then the commit is not acceptable

nordicjm avatar Apr 29 '24 06:04 nordicjm

  1. The HAL contains CMake and Kconfig, those really belong in zephyr/modules
  2. No zephyr/modules/hal_atmosic folder

A little history here - we began our development back on Zephyr v0.6, and things have changed many times since then. Is there a definitive document that covers 2 and 3?

I would appreciate some clarity on both of these points.

There seem to be plenty of CMake counter examples: https://github.com/zephyrproject-rtos/hal_ambiq/blob/main/CMakeLists.txt https://github.com/zephyrproject-rtos/hal_nxp/blob/master/CMakeLists.txt https://github.com/zephyrproject-rtos/hal_renesas/blob/main/CMakeLists.txt https://github.com/zephyrproject-rtos/hal_nuvoton/blob/master/CMakeLists.txt https://github.com/zephyrproject-rtos/hal_xtensa/blob/master/CMakeLists.txt https://github.com/zephyrproject-rtos/hal_ti/blob/master/CMakeLists.txt https://github.com/zephyrproject-rtos/hal_stm32/blob/main/CMakeLists.txt https://github.com/zephyrproject-rtos/hal_silabs/blob/master/CMakeLists.txt https://github.com/zephyrproject-rtos/hal_st/blob/master/CMakeLists.txt https://github.com/zephyrproject-rtos/hal_microchip/blob/master/CMakeLists.txt https://github.com/zephyrproject-rtos/hal_infineon/blob/master/CMakeLists.txt https://github.com/zephyrproject-rtos/hal_atmel/blob/master/CMakeLists.txt https://github.com/zephyrproject-rtos/hal_hpmicro/blob/zephyr/CMakeLists.txt ...

With respect to our hal's modular Kconfig hierarchy, are you suggesting that we flatten that into a single Kconfig.<manufacturer> under zephyr/modules? Or perhaps migrate said hierarchy under zephyr/modules/hal_atmosic? Please help me understand why that is necessary/preferred vs. having Kconfig in the hal itself.

dt-atmosic avatar Apr 29 '24 23:04 dt-atmosic

Yes, these are necessary to support binary libraries in our self-hosted repositories.

Then the commit is not acceptable

They also support source code currently in hal_atmosic:

rev-2/include/armgcc/compiler.h:#define __UNINIT __attribute__((section(".uninit." AT)))
rev-5/include/armgcc/compiler.h:#define __UNINIT __attribute__((section(".uninit." AT)))
drivers/atm_bp_clock/atm_bp_clock.c:    __attribute__((section(".data_text"))) __STATIC
drivers/atm_bp_clock/atm_bp_clock.c:__attribute__((noinline, section(".data_text"))) void
drivers/atm_bp_clock/atm_bp_clock.c:__attribute__((noinline, section(".data_text"))) void

Nearly all of the code in hal_atmosic has a common origin and is used by several other environments. Read: supporting, but not specific to Zephyr.

dt-atmosic avatar Apr 30 '24 20:04 dt-atmosic

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

github-actions[bot] avatar Jun 30 '24 00:06 github-actions[bot]

Hi, when will the integration mature to be merged with the Zephyr official release? My team is evaluating Atmosic ATM330e to replace our previous MCU. It runs on Zephyr, and we also plan to use Zephyr for quick porting.

edantoni96 avatar Jul 02 '24 05:07 edantoni96

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

github-actions[bot] avatar Oct 20 '24 00:10 github-actions[bot]