SIM3U support #57871

M1cha · 2023-05-14T07:39:52Z

I split up the commits to make this easier to review. If you think that this PR is too big I can create multiple PRs as well.

This depends on zephyrproject-rtos/hal_silabs#29
Overview: #55287

This bumps the hal_silabs in order to add initial support for sim3u SoCs. WIP, because the revision wasn't merged, yet. Signed-off-by: Michael Zimmermann <[email protected]>

zephyrbot · 2023-05-14T07:40:46Z

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

Name	Old Revision	New Revision	Diff
hal_silabs	zephyrproject-rtos/hal_silabs@`249c08f`	zephyrproject-rtos/hal_silabs@`785f4ed`	zephyrproject-rtos/[email protected]

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

M1cha · 2023-05-14T08:28:20Z

I don't understand the cause this error:

Error: drivers/spi/spi_si32.c:37:WARNING: Violation to rule 5.7 (Tag name should be unique) tag: spi_config
drivers/spi/spi_si32.c:171:WARNING: Violation to rule 5.7 (Tag name should be unique) tag: spi_config
drivers/spi/spi_si32.c:215:WARNING: Violation to rule 5.7 (Tag name should be unique) tag: spi_config

I'm not declaring a struct in any of those places and the variable names also don't shadow any other variables.

Includes the SoC, pinctrl, flash and devicetree. I had to include the flash driver that early because I couldn't make zephyr compile without flash driver nodes in the device tree. Signed-off-by: Michael Zimmermann <[email protected]>

this serves two main purposes: - change the CPU clock via devicetree nodes - provide the APB frequency to device drivers via the clock driver interface Theoretically this could also support choosing between the available clock sources, but right now we only support LPOSC0 going into PLL0, going into AHB. Turning the PLL back off is also not supported since the only current usecase is to set the PLL frequency, turn it on, and switch the AHB over to it. Signed-off-by: Michael Zimmermann <[email protected]>

This is just the driver for banks 0,1,2,3. Bank 4 will come via a separate commit since it needs a different driver. Signed-off-by: Michael Zimmermann <[email protected]>

The code looks very similar to the normal one but it uses different hardware registers and has additional features to configure the high drive functionality. Signed-off-by: Michael Zimmermann <[email protected]>

This supports polling and interrupt APIs, but not the async API. Signed-off-by: Michael Zimmermann <[email protected]>

This only supports master mode right now. Signed-off-by: Michael Zimmermann <[email protected]>

This is the radiomodule used in the current GARDENA smart gateways. - SPI si4467 transceiver - controls an RGB LED via high drive pins. It's expected to mirror the state of 3 low-drive pins coming from the Linux SoC. - UART is connected to the Linux SoC. Usually it's used for PPP, but it can also be used for debugging when PPP is not active. Signed-off-by: Michael Zimmermann <[email protected]>

cfriedt · 2023-05-14T22:54:35Z

drivers/gpio/gpio_si32_hd.c

+	/* Set pin function to GPIO.
+	 * This is the only supported mode for now.
+	 */
+	switch (pin) {


This entire switch statement looks like it should be reduced to 1 line of code. Is there really no function that takes the pin as a parameter?

There isn't because we only use the register-descriptions of the HAL so there's not much abstraction or simplification on top of what the registers look like. We also can't just calculate the offset within the PBHD4_PBFSEL register based on the pin number because pin5 is special and uses a different number of bits.

You could use the FOR_EACH macro from Zephyr to avoid this code duplication.

cfriedt · 2023-05-14T22:58:05Z

drivers/gpio/gpio_si32_hd.c

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


I would suggest using a DT property to differentiate between high and non-high drive support, potentially incorporating Zephyr GPIO flags in a better way to enable that functionality, and combining these two gpio drivers.

There should be several drivers that can be used for reference.

Additionally, I just wanted to make sure that there is no potential overlap with the gecko silabs gpio drivers.

@fkokosinski might know as silabs maintainer.

I would suggest using a DT property to differentiate between high and non-high drive support, potentially incorporating Zephyr GPIO flags in a better way to enable that functionality, and combining these two gpio drivers.

During development I thought about that but decided against it because the registers look completely different and have different data types, so it would end up in every function needing an if/else block. I didn't like that because it wouldn't save any lines of code, would make the code harder to follow and (less importantly) reduce performance since you have to do that check for every gpio API call.

fkokosinski · 2023-05-15T08:34:07Z

drivers/clock_control/Kconfig.si32

+config CLOCK_CONTROL_SI32_APB
+	bool "SI32 APB clock control"
+	default y
+	depends on DT_HAS_SILABS_SI32_APB_ENABLED


Please add help strings to these 3 symbols in this file.

fkokosinski · 2023-05-15T08:53:08Z

drivers/clock_control/clock_control_si32_ahb.c

+		return -ENODEV;
+	}
+
+	if (config->freq != 20000000) {


Why is this check needed? What happens if clock freq in the device config is 20000000?

fkokosinski · 2023-05-15T09:09:54Z

drivers/gpio/gpio_si32.c

+	GPIO_DEVICE_INIT(DT_NODELABEL(gpio##__suffix), __suffix,                                   \
+			 DT_REG_ADDR(DT_NODELABEL(gpio##__suffix)))
+
+#if DT_NODE_HAS_STATUS(DT_NODELABEL(gpio0), okay)


Maybe use DT_FOREACH_STATUS_OKAY for this GPIO_DEVICE_INIT_SI32 section?

fkokosinski · 2023-05-15T09:13:05Z

drivers/gpio/gpio_si32_hd.c

+	/* Set pin function to GPIO.
+	 * This is the only supported mode for now.
+	 */
+	switch (pin) {


You could use the FOR_EACH macro from Zephyr to avoid this code duplication.

fkokosinski · 2023-05-15T09:13:30Z

drivers/serial/Kconfig.si32

+	default y
+	depends on DT_HAS_SILABS_SI32_USART_ENABLED
+	select SERIAL_HAS_DRIVER
+	select SERIAL_SUPPORT_INTERRUPT


Add help string.

fkokosinski · 2023-05-15T09:15:17Z

drivers/spi/Kconfig.si32

+config SPI_SI32
+	bool "SI32 SPI controller driver"
+	default y
+	depends on DT_HAS_SILABS_SI32_SPI_ENABLED


Add missing help string.

fkokosinski · 2023-05-15T09:17:14Z

dts/bindings/gpio/silabs,si32-gpio.yaml

+      const: 2
+
+    disable-pullups:
+      required: false


Remove this line. Required is false by default.

fkokosinski · 2023-05-15T09:17:36Z

dts/bindings/gpio/silabs,si32-hd-gpio.yaml

+      const: 2
+
+    nchannel-current-limit:
+      required: false


Ditto in this file - remove required: false.

fkokosinski · 2023-05-15T09:19:05Z

include/zephyr/dt-bindings/gpio/silabs-si32-hd-gpio.h

+/** @endcond */
+
+/** Enable port bank current limit on this pin. */
+#define SI32_GPIO_DS_ENABLE_CURRENT_LIMIT (0x0U << SI32_HD_GPIO_DS_POS)


Is this intentionally always 0?

oops, nice catch

fkokosinski · 2023-05-15T09:23:11Z

west.yml

@@ -126,7 +126,7 @@ manifest:
      groups:
        - hal
    - name: hal_silabs
-      revision: 249c08f16f5ee9663c8b225b68faf8ec54a21e8e
+      revision: 785f4ed3aadb57d1b694be48c6f06fd5617df609


Wrong revision? I think you want to point to your PR in the hal_silabs repository with pull/<pr_num>/head here.

M1cha · 2023-05-15T09:30:46Z

include/zephyr/dt-bindings/gpio/silabs-si32-hd-gpio.h

+/**
+ * @name GPIO drive strength flags
+ *
+ * The drive strength flags are a Zephyr specific extension of the standard GPIO
+ * flags specified by the Linux GPIO binding. Only applicable for Espressif
+ * ESP32 SoCs.
+ *
+ * The interface supports two different drive strengths:
+ * `DFLT` - The lowest drive strength supported by the HW
+ * `ALT` - The highest drive strength supported by the HW
+ *
+ * @{
+ */
+/** @cond INTERNAL_HIDDEN */


This was obviously copy&pasted from esp32. I'll fix that.

de-nordic · 2023-05-15T15:43:16Z

drivers/flash/flash_si32.c

+
+static bool flash_si32_valid_range(off_t offset, uint32_t size, bool write)
+{
+	if ((offset > SOC_NV_FLASH_SIZE) || ((offset + size) > SOC_NV_FLASH_SIZE)) {


off_t can take negative value.

de-nordic · 2023-05-15T15:45:47Z

drivers/flash/flash_si32.c

+	if (!flash_si32_valid_range(offset, size, false)) {
+		return -EINVAL;
+	}
+
+	if (!size) {
+		return 0;
+	}


If size is zero then no work will be done anyway, why not check this before validating range?

de-nordic · 2023-05-15T15:47:04Z

drivers/flash/flash_si32.c

+	if (!size) {
+		return 0;
+	}


If size is zero then no work will be done anyway, why not check this before validating range?

de-nordic · 2023-05-15T15:50:18Z

drivers/flash/flash_si32.c

+	.erase_value = 0xff,
+};
+
+static bool flash_si32_valid_range(off_t offset, uint32_t size, bool write)


Instead of the bool write you could just pass op_alignment and have universal check for range validation, just a suggestion.

de-nordic · 2023-05-15T15:52:52Z

drivers/flash/flash_si32.c

+	if (!size) {
+		return 0;
+	}


Lots of validation happens before size == 0 decides that no work will be done.

henrikbrixandersen

I split up the commits to make this easier to review. If you think that this PR is too big I can create multiple PRs as well.

Thank you for submitting this. However, I would prefer seeing this PR split into multiple PRs. An initial PR for adding the board with UART and GPIO support, and separate PRs for the remaining peripherals. The rationale is, that these various drivers need to be reviewed by different groups of people, which is difficult with a PR of this size - the other is the size of the PR itself. Reviewing changes to 60+ files with 3k+ changes is way more difficult than reviewing smaller changes.

Please see https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html

henrikbrixandersen · 2023-05-19T13:10:23Z

Also, you lack documentation for the newly added board :)

github-actions · 2023-07-19T00:48:35Z

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.

M1cha · 2023-08-03T04:51:06Z

I missed the stale notification 🤷
However, I'll split up the PR like @henrikbrixandersen suggested anyway.

M1cha · 2023-10-21T06:46:28Z

Here's the new PR: #64209

It's still big because I was only able to remove the highdrive UART driver and the SPI driver. Everything else is actually needed just to get UART working.

This is cherry-picked from the PR already opened by Michael Zimmermann: zephyrproject-rtos#57871 Eventually, this commit should be updated with the new PR posted here: zephyrproject-rtos#64209 For this reason, do not merge substantial improvements over the original PR into this commit - it would (likely) be forgotten when switching to the new PR. Note: As requested by the reviewers upstream, the new PR is reduced in scope and therefore, the extracted parts need to be brought back manually. (cherry picked from commit a2ce87510b7ee119f3df179192ffb5ff083b6100) (squashed ab4505ff0d234c457aaa0d3ddc27833157d0d0ad)

WIP: west.yml: hal_silabs: update hal version

4c6f95c

This bumps the hal_silabs in order to add initial support for sim3u SoCs. WIP, because the revision wasn't merged, yet. Signed-off-by: Michael Zimmermann <[email protected]>

M1cha requested review from nashif, carlescufi, galak, MaureenHelm, tbursztyka, mnkp, de-nordic and ioannisg as code owners May 14, 2023 07:39

zephyrbot added manifest-hal_silabs area: Pinctrl area: Flash area: Devicetree Binding PR modifies or adds a Device Tree binding area: Clock Control platform: Silabs Silicon Labs area: UART Universal Asynchronous Receiver-Transmitter area: SPI SPI bus area: GPIO labels May 14, 2023

zephyrbot requested review from cfriedt, dcpleung, fkokosinski, henrikbrixandersen, nordic-krch, teburd and tgorochowik May 14, 2023 07:40

zephyrbot assigned fkokosinski May 14, 2023

zephyrbot added the manifest label May 14, 2023

M1cha mentioned this pull request May 14, 2023

sim3u167bgm SOC support #55287

Open

13 tasks

M1cha force-pushed the sim3u branch from 3d71c67 to 9c3f1d7 Compare May 14, 2023 08:23

M1cha added 7 commits May 14, 2023 10:50

Initial sim3u1xx support

abfef70

Includes the SoC, pinctrl, flash and devicetree. I had to include the flash driver that early because I couldn't make zephyr compile without flash driver nodes in the device tree. Signed-off-by: Michael Zimmermann <[email protected]>

sim3u: add gpio driver

28e7750

This is just the driver for banks 0,1,2,3. Bank 4 will come via a separate commit since it needs a different driver. Signed-off-by: Michael Zimmermann <[email protected]>

sim3u: add gpio high driver driver

25a8388

The code looks very similar to the normal one but it uses different hardware registers and has additional features to configure the high drive functionality. Signed-off-by: Michael Zimmermann <[email protected]>

sim3u: add usart driver

250fdcb

This supports polling and interrupt APIs, but not the async API. Signed-off-by: Michael Zimmermann <[email protected]>

sim3u: add SPI driver

7cef089

This only supports master mode right now. Signed-off-by: Michael Zimmermann <[email protected]>

M1cha force-pushed the sim3u branch from 9c3f1d7 to 4d6bcec Compare May 14, 2023 08:51

cfriedt requested changes May 14, 2023

View reviewed changes

fkokosinski requested changes May 15, 2023

View reviewed changes

M1cha commented May 15, 2023

View reviewed changes

de-nordic reviewed May 15, 2023

View reviewed changes

henrikbrixandersen requested changes May 19, 2023

View reviewed changes

github-actions bot added the Stale label Jul 19, 2023

github-actions bot closed this Aug 3, 2023

M1cha mentioned this pull request Oct 21, 2023

SIM3U: board + UART #64209

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SIM3U support #57871

SIM3U support #57871

M1cha commented May 14, 2023

zephyrbot commented May 14, 2023

M1cha commented May 14, 2023

cfriedt May 14, 2023

M1cha May 15, 2023

fkokosinski May 15, 2023

cfriedt May 14, 2023

M1cha May 15, 2023 •

edited

Loading

fkokosinski May 15, 2023

fkokosinski May 15, 2023

fkokosinski May 15, 2023

fkokosinski May 15, 2023

fkokosinski May 15, 2023

fkokosinski May 15, 2023

fkokosinski May 15, 2023

fkokosinski May 15, 2023

fkokosinski May 15, 2023

M1cha May 15, 2023

fkokosinski May 15, 2023

M1cha May 15, 2023

de-nordic May 15, 2023

de-nordic May 15, 2023

de-nordic May 15, 2023

de-nordic May 15, 2023

de-nordic May 15, 2023

henrikbrixandersen left a comment

henrikbrixandersen commented May 19, 2023

github-actions bot commented Jul 19, 2023

M1cha commented Aug 3, 2023

M1cha commented Oct 21, 2023 •

edited

Loading

SIM3U support #57871

SIM3U support #57871

Conversation

M1cha commented May 14, 2023

zephyrbot commented May 14, 2023

M1cha commented May 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M1cha May 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henrikbrixandersen left a comment

Choose a reason for hiding this comment

henrikbrixandersen commented May 19, 2023

github-actions bot commented Jul 19, 2023

M1cha commented Aug 3, 2023

M1cha commented Oct 21, 2023 • edited Loading

M1cha May 15, 2023 •

edited

Loading

M1cha commented Oct 21, 2023 •

edited

Loading