[riot-notifications] [RIOT-OS/RIOT] cpu/qn908x: Initial minimal support for NXP QN908x CPUs. (#13855)

benpicco notifications at github.com
Sun Nov 29 18:01:39 CET 2020


@benpicco commented on this pull request.

looks pretty good.
Please make a separate commit for adding the vendor files, that makes the review easier. 

> +void clocks_init(void)
+{
+    /* Set up clock selectors - Attach clocks to the peripheries */
+
+    /* Switch XTAL_CLK to 32M */
+    CLOCK_AttachClk(k32M_to_XTAL_CLK);
+    /* Switch 32K_CLK to XTAL32K */
+    CLOCK_AttachClk(kXTAL32K_to_32K_CLK);
+    /* Switch SYS_CLK to XTAL */
+    CLOCK_AttachClk(kXTAL_to_SYS_CLK);
+    /* Switch WDT_CLK to APB */
+    CLOCK_AttachClk(kAPB_to_WDT_CLK);
+
+    /* Set up dividers */
+
+    /* Set OSC32M_DIV divider to value 2 */
+    CLOCK_SetClkDiv(kCLOCK_DivOsc32mClk, 1U);
+    /* Set XTAL_DIV divider to value 2 */
+    CLOCK_SetClkDiv(kCLOCK_DivXtalClk, 1U);
+    /* Set AHB_DIV divider to value 2 */
+    CLOCK_SetClkDiv(kCLOCK_DivAhbClk, 1U);
+    /* Set FRG_MULT1 to value 0, Set FRG_DIV1 to value 255 */
+    CLOCK_SetClkDiv(kCLOCK_DivFrg1, 0U);
+    /* Set FRG_MULT0 to value 0, Set FRG_DIV0 to value 255 */
+    CLOCK_SetClkDiv(kCLOCK_DivFrg0, 0U);
+    /* Set APB_DIV divider to value 1 */
+    CLOCK_SetClkDiv(kCLOCK_DivApbClk, 0U);
+}

This should be done in `cpu/`, the board can set config defines. 

> +FEATURES_PROVIDED += periph_cpuid
+FEATURES_PROVIDED += periph_wdt periph_wdt_cb

Those are independent of the board can be provided by the CPU directly 

> +ifneq (,$(filter saul_default,$(USEMODULE)))
+  USEMODULE += saul_gpio
+endif

I'd keep that in the actual board's `Makefile.dep`

> +            uint32_t pin = bitarithm_lsb(status);
+            status ^= 1u << pin;

You can also use 

```C
status = bitarithm_test_and_clear(status, &pin);
```

which will do the same thing

> + *
+ * @{
+ *
+ * @file
+ * @brief       Common Pin MUX functions.
+ *
+ * @author      iosabi <iosabi at protonmail.com>
+ *
+ * @}
+ */
+
+#include "mux_common.h"
+
+#include "vendor/drivers/fsl_iocon.h"
+
+void mux_set_pin(gpio_t pin, uint32_t func)

`gpio_init_mux()` would be more conventional name for this function :) 
That can also live in `gpio.c`.

> +# General Public License v2.1. See the file LICENSE in the top level
+# directory for more details.
+#
+
+config BOARD
+    default "qn9080dk" if BOARD_QN9080DK
+
+config BOARD_QN9080DK
+    bool
+    default y
+    select BOARD_COMMON_QN9080
+    select CPU_MODEL_QN9080XHN
+
+    # Put defined MCU peripherals here (in alphabetical order)
+    select HAS_PERIPH_GPIO
+    select HAS_PERIPH_GPIO_IRQ

This can be moved to the cpu - the SoC will *always* have GPIOs and they might be used for internal functions 

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/13855#pullrequestreview-540496370
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20201129/b876b903/attachment.htm>


More information about the notifications mailing list