modules: hal_silabs: Add Silabs Gecko HAL for EFR32BG22 SoCs #13

sateeshkotapati · 2021-05-13T15:26:01Z

Origin: Silicon Labs Gecko SDK
URL:
https://www.silabs.com/products/development-tools/software/simplicity-studio
Version: 3.1.2
Purpose: Add support for Silicon Labs EFR32BG22 SoCs
License: Zlib
Maintained-by: External

Signed-off-by: Sateesh Kotapati [email protected]

sateeshkotapati · 2021-05-13T15:28:40Z

Support added for Silabs' EFR32BG22 SoCs, which I would like to contribute to zephyr

galak · 2021-05-13T15:58:41Z

Please submit an associated PR on the Zephyr repo that adds SoC and board support that utilizes this.

chrta

Thank you for your contribution, i just mark this as "Request changes" to block merging. I will do a proper review once the complete code is available, that adds support for this soc to zephyr.

It seems that the board initialization is now used from the service folder. This should be done in a consistent manner for all Silabs EXX32 SoCs (imho).
The README file https://github.com/zephyrproject-rtos/hal_silabs/blob/master/gecko/README should also be adapted to reflect what folders/files need to be copied.

sateeshkotapati · 2021-05-17T14:49:31Z

I missed gecko/README file in my first commit in this PR. That is updated now.
Board initialization is kept exclusive for EFR32BG22 SoCs and is not extended to all EXX32 devices because validation effort is huge for all the devices.

chrta

Thank you for the updates. I am ok with keeping the currently supported EXX32 boards as they are now, i agree that it would be a lot of effort. My main points in the comments:

It seems that my sdk version 3.1.2 i downloaded differs slightly from the code in this pr. This should be fixed/documented.
Changes applied to the sdk should be done in a separate commit outlining the reasoning for this, e.g. adding a file sl_board_control_config.h. There should be a documentation why this is necessary and how the file was created etc.
The newly added files in the service folder should be compiled if the SOC selects them and should not depend the SOC define itself. For this new CONFIG_-defines should be added.

chrta · 2021-05-18T10:24:13Z

gecko/CMakeLists.txt

+zephyr_sources_ifdef(CONFIG_SOC_SERIES_EFR32BG22   service/power_manager/src/sl_power_manager_hal_s2.c)
+zephyr_sources_ifdef(CONFIG_SOC_SERIES_EFR32BG22   service/hfxo_manager/src/sl_hfxo_manager.c)
+zephyr_sources_ifdef(CONFIG_SOC_SERIES_EFR32BG22   service/hfxo_manager/src/sl_hfxo_manager_hal_s2.c)
+zephyr_sources_ifdef(CONFIG_SOC_SERIES_EFR32BG22   common/src/sl_slist.c)


These implementations should not be compiled in because CONFIG_SOC_SERIES_EFR32BG22 is defined. I suppose that the implementation would also work for other Silabs SoCs, e.g. the EFM32PG22.

It should be done in a similar way as it is done for the emlib drivers. The soc in zephyr should select which service feature it supports, by defining e.g. CONFIG_SOC_GECKO_SERVICE_POWER_MANAGER and this should be used here.

Thank you for the review and detailed comments. I have incorporated all the review comments.

chrta · 2021-05-18T10:45:25Z

gecko/CMakeLists.txt

 zephyr_sources_ifdef(CONFIG_SOC_GECKO_RTC          emlib/src/em_rtc.c)
 zephyr_sources_ifdef(CONFIG_SOC_GECKO_RTCC         emlib/src/em_rtcc.c)
+zephyr_sources_ifdef(CONFIG_SOC_GECKO_RTCC         service/sleeptimer/src/sl_sleeptimer_hal_rtcc.c)
+zephyr_sources_ifdef(CONFIG_SOC_GECKO_RTCC         service/sleeptimer/src/sl_sleeptimer.c)


I thing these two c files should also only be compiled if a new define is set, that explicitly enables sleeptimer support. Maybe you can also have a look at zephyrproject-rtos/zephyr#29201 and #9 how it was done there.

I have incorporated this review comment.

chrta · 2021-05-18T10:46:56Z

gecko/service/sleeptimer/test/main.c

@@ -0,0 +1,2441 @@
+#include <stdio.h>


IMHO, if these tests are not compiled/used here, the complete test folders should be removed. This comment also applies to the test folders for the other services.

All test directories which are not used are removed.

chrta · 2021-05-18T11:43:24Z

gecko/board/inc/sl_board_control_config.h

@@ -0,0 +1,66 @@
+#ifndef SL_BOARD_CONTROL_CONFIG_H


Where does this file come from? It does not seem to be present in the downloaded sdk. Is this file required?

This file is copied from hardware\board\config directory. This is required as this header file is included in sl_board_init.c file.

Can you document this in the README file -> what to do if a new board is added

How does this work if a second board should be supported in a similar way that needs a different config?

chrta · 2021-05-18T11:45:49Z

gecko/board/src/sl_board_init.c

@@ -0,0 +1,98 @@
+/***************************************************************************//**


This file is a little bit different to the one that is in my installed sdk (version 3.1.2). Are these changes required?

chrta · 2021-05-18T11:49:08Z

gecko/common/inc/sl_status.h

@@ -0,0 +1,425 @@
+/*******************************************************************************


This file (and some files in the src folder) seem to be slightly different to the files in the sdk 3.1.2 i downloaded. Are these changes necessary?

These are necessary as these are used in power_manager files. Updated the files.

chrta · 2021-05-18T11:54:30Z

gecko/Device/SiliconLabs/EFR32BG22/Include/efr32bg22c112f352gm40.h

+ *        for EFR32BG22C112F352GM40
+ ******************************************************************************
+ * # License
+ * <b>Copyright 2020 Silicon Laboratories, Inc. www.silabs.com</b>


The copyright date of all files in the Device folder is different and some defines differ (compared to my installed sdk 3.1.2), e.g. _SILICON_LABS_EFR32_2G4HZ_HP_PA_MAX_OUTPUT_DBM vs _SILICON_LABS_EFR32_2G4HZ_LP_PA_MAX_OUTPUT_DBM

It seems that the code in this pr is not copied from 3.1.2, but an older version, or multiple variants exist of the sdk version 3.1.2.

chrta

Thank you for the changes. My open issues:

Please document in the README file how the file sl_board_control_config.h is generated, if a new board should be added.
How does it work if a second board is added that needs a different sl_board_control_config.h
Please squash the commits. I would only expect 2 commits (or maybe only one). One adding the device etc to this repository and a second commit that contains the necessary manual changes/additions.

chrta

Thank you for the changes, especially the addition to the README file. The only open issue/question i have is:

How does it work if a second board is added that needs a different configuration, i.e. sl_board_control_config.h. I think currently it does not work, but imho it should work.

I would prefer if the generated files would be in different subfolders containing the board name in the path and the build system takes care that the correct files are compiled and inclusion paths are set correctly.

For this one board the current approach might work. But once a second board is added it does not work without manually modifying the generated files in this repository. Imho this is bad.

sateeshkotapati · 2021-05-27T08:20:38Z

Thank you for the changes, especially the addition to the README file. The only open issue/question i have is:

How does it work if a second board is added that needs a different configuration, i.e. sl_board_control_config.h. I think currently it does not work, but imho it should work.

I would prefer if the generated files would be in different subfolders containing the board name in the path and the build system takes care that the correct files are compiled and inclusion paths are set correctly.

For this one board the current approach might work. But once a second board is added it does not work without manually modifying the generated files in this repository. Imho this is bad.

I have added the configuration files for all the silabs devices under gecko/board/config. sl_board_control_config.h will be chosen based on the board name that is given in zephyr\boards\arm\efr32_radio\Kconfig.defconfig

chrta · 2021-05-27T09:25:05Z

I have added the configuration files for all the silabs devices under gecko/board/config. sl_board_control_config.h will be chosen based on the board name that is given in zephyr\boards\arm\efr32_radio\Kconfig.defconfig

Oh yes, sorry i missed that. I noticed that you modified the board name in https://github.com/zephyrproject-rtos/zephyr/pull/35361/files#diff-e63781934d96e8af747cf94696a9ba7d1f946e4c0113aed1969212d8c4075e35R11 so it does not include the prefix efr32_radio_ like all the other boards do. I would prefer if It would have the prefix and stripping it is handled in gecko/CMakeLists.txt in a similar way as the cmake variable SILABS_GECKO_DEVICE is created. E.g. including this:

diff --git a/gecko/CMakeLists.txt b/gecko/CMakeLists.txt
index 731c93c..66f99a3 100644
--- a/gecko/CMakeLists.txt
+++ b/gecko/CMakeLists.txt
@@ -9,6 +9,15 @@
 # respectively.
 string(TOUPPER ${CONFIG_SOC_SERIES} SILABS_GECKO_DEVICE)
 
+# Remove any prefix from the board, e.g. efr32_radio_
+string(FIND ${CONFIG_BOARD} "brd" SILABS_GECKO_BRD_INDEX)
+
+if (${SILABS_GECKO_BRD_INDEX} LESS 0)
+  set(SILABS_GECKO_BOARD ${CONFIG_BOARD})
+else()
+  string(SUBSTRING ${CONFIG_BOARD} ${SILABS_GECKO_BRD_INDEX} -1 SILABS_GECKO_BOARD)
+endif()
+
 set(SILABS_GECKO_PART_NUMBER ${CONFIG_SOC_PART_NUMBER})
 
 zephyr_include_directories(
@@ -28,7 +37,7 @@ zephyr_include_directories(
   service/device_init/config/s2
   service/device_init/config/s2/sdid205
   board/inc
-  board/config/${CONFIG_BOARD}
+  board/config/${SILABS_GECKO_BOARD}
   )
 
 # The gecko SDK uses the cpu name to include the matching device header.

This way CONFIG_BOARD for the new boards can include prefixes.
I think for other boards where the config folder name is e.g. gecko/board/config/brd4100a_brd4001a this should be handled in this repository (hal_silabs) by modifying gecko/CMakeLists.txt and not by modifying CONFIG_BOARD in the main zephyr repository.

chrta

Thank you, lgtm

galak · 2021-06-10T23:50:26Z

gecko/CMakeLists.txt

 set(SILABS_GECKO_PART_NUMBER ${CONFIG_SOC_PART_NUMBER})

+if (${CONFIG_BOARD} STREQUAL "efr32bg_sltb010a")
+  set(SILABS_GECKO_BOARD "brd4184b")


What is SILABS_GECKO_BOARD used for?

Sorry for updating late. I missed the notification.
It is used to get board specific configuration from directory "zephyrproject\modules\hal\silabs\gecko\board\config"

What are you using from board/config/*?

galak · 2021-06-24T12:04:00Z

Can you split out the code import from any changes/additions as different commits. So the CMakeLists.txt would be a separate commit for example.

galak

Would prefer not to import the boards/configs. I'm also unsure about the services/. Can we limit the initial import / support to Device/SiliconLabs/EFR32BG22?

chrta

I remove my approval until discussion is resolved. Also a lot changed since my last review.

galak · 2021-07-20T19:15:56Z

Noticed another issue with some of the board/ files is they are not licensed under a license that would be acceptable to the Zephyr project.

https://docs.zephyrproject.org/latest/contribute/external.html#software-license

Added support for the BRD4184B board, as part of efr32_radio. Support for the EFR32BG22 SoC was added in main Zephyr repo to achieve this. For supporting EFR32BG22, also need to merge the PR below. hal_silabs modules PR is zephyrproject-rtos/hal_silabs#13

sateeshkotapati · 2022-02-14T08:53:52Z

Please review this PR, Thanks

sateeshkotapati · 2022-04-20T04:44:53Z

Hi Zephyr community,
Please re-review this PR. I have incorporated all the previous review comments.
Our goal to add more Silabs devices is blocked on this PR.
Thank you and appreciate your time.

Regards,
Sateesh

Origin: Silicon Labs Gecko SDK URL: https://www.silabs.com/products/development-tools/software/simplicity-studio Version: 3.1.2 Purpose: board configuration files are added. License: Zlib Maintained-by: External Signed-off-by: Sateesh Kotapati <[email protected]>

kgugala · 2023-07-27T13:07:22Z

this PR is deprecated. The changes have been merged in the followup PRs. Closing this one

galak requested review from chrta and mnkp May 13, 2021 15:58

chrta suggested changes May 14, 2021

View reviewed changes

sateeshkotapati mentioned this pull request May 17, 2021

Zephyr main efr32bg22 support zephyrproject-rtos/zephyr#35361

Closed

carlescufi requested a review from chrta May 17, 2021 15:28

chrta suggested changes May 18, 2021

View reviewed changes

chrta suggested changes May 26, 2021

View reviewed changes

sateeshkotapati force-pushed the silabs_efr32bg22_support branch 2 times, most recently from a95421e to f83b232 Compare May 26, 2021 10:52

chrta suggested changes May 27, 2021

View reviewed changes

sateeshkotapati force-pushed the silabs_efr32bg22_support branch from f83b232 to 4b91d4d Compare May 27, 2021 09:44

chrta approved these changes May 27, 2021

View reviewed changes

galak reviewed Jun 10, 2021

View reviewed changes

sateeshkotapati force-pushed the silabs_efr32bg22_support branch from 5cddaaa to 55c1530 Compare June 14, 2021 07:55

galak requested changes Jun 24, 2021

View reviewed changes

sateeshkotapati force-pushed the silabs_efr32bg22_support branch from a6209cb to 57051c1 Compare July 13, 2021 09:46

chrta reviewed Jul 14, 2021

View reviewed changes

chrta self-requested a review July 14, 2021 08:57

chrta mentioned this pull request Feb 24, 2022

Request for support to maintaining hal_silabs zephyrproject-rtos/zephyr#43139

Closed

sateeshkotapati force-pushed the silabs_efr32bg22_support branch from 18683ca to a58d307 Compare April 14, 2022 06:44

sateeshkotapati requested a review from galak April 14, 2022 06:48

sateeshkotapati force-pushed the silabs_efr32bg22_support branch from a58d307 to 03b096c Compare July 20, 2022 07:25

lpawelcz mentioned this pull request Sep 19, 2022

modules: hal_silabs: Add Silabs Gecko HAL for EFR32BG22C224F512IM40 SoC #19

Merged

fkokosinski mentioned this pull request Sep 29, 2022

MAINTAINERS: update hal_silabs maintainers zephyrproject-rtos/zephyr#50755

Merged

kgugala closed this Jul 27, 2023

		@@ -0,0 +1,98 @@
		/*************************************************************************//

		@@ -0,0 +1,425 @@
		/*******************************************************************************

modules: hal_silabs: Add Silabs Gecko HAL for EFR32BG22 SoCs #13

modules: hal_silabs: Add Silabs Gecko HAL for EFR32BG22 SoCs #13

Uh oh!

Conversation

sateeshkotapati commented May 13, 2021

Uh oh!

sateeshkotapati commented May 13, 2021

Uh oh!

galak commented May 13, 2021

Uh oh!

chrta left a comment

Choose a reason for hiding this comment

Uh oh!

sateeshkotapati commented May 17, 2021

Uh oh!

chrta left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrta left a comment

Choose a reason for hiding this comment

Uh oh!

chrta left a comment

Choose a reason for hiding this comment

Uh oh!

sateeshkotapati commented May 27, 2021

Uh oh!

chrta commented May 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrta left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

galak commented Jun 24, 2021

Uh oh!

galak left a comment

Choose a reason for hiding this comment

Uh oh!

chrta left a comment

Choose a reason for hiding this comment

Uh oh!

galak commented Jul 20, 2021

Uh oh!

sateeshkotapati commented Feb 14, 2022

chrta left a comment •

edited

Loading

chrta commented May 27, 2021 •

edited

Loading