arch: arm: nrf: add hardware description for nrf kconfig files #8363

jakub-uC · 2018-06-13T13:18:26Z

Simplify Kconfig.nrfx for serial to use HAS_HW_NRF_UART0.

Signed-off-by: Jakub Rzeszutko [email protected]

jakub-uC · 2018-06-13T13:31:30Z

Purpose of this PR is to allow to display and configure in Kconfig only these drivers that are available for particular SoC.

codecov-io · 2018-06-13T13:45:44Z

Codecov Report

Merging #8363 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #8363   +/-   ##
=======================================
  Coverage   64.61%   64.61%           
=======================================
  Files         421      421           
  Lines       40296    40296           
  Branches     6803     6803           
=======================================
  Hits        26037    26037           
  Misses      11126    11126           
  Partials     3133     3133

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5bfc7ff...06e7ed5. Read the comment docs.

anangl · 2018-06-13T14:18:17Z

@jarz-nordic I think such explanation (why all these config entries are introduced) should be contained in the commit message. And the change in Kconfig.nrfx for serial driver should be mentioned as an example of usage of these entries, or moved to a separate commit.

anangl · 2018-06-13T14:20:42Z

arch/arm/soc/nordic_nrf/Kconfig.peripherals

@@ -0,0 +1,208 @@
+# Kconfig.peripherals - Nordic Semiconductor nRFx MCU line
+#
+# Copyright (c) 2018-2018 Nordic Semiconductor ASA


One 2018 will be sufficient.

anangl · 2018-06-13T14:21:38Z

arch/arm/soc/nordic_nrf/Kconfig.peripherals

@@ -0,0 +1,208 @@
+# Kconfig.peripherals - Nordic Semiconductor nRFx MCU line


This description does not explain much.

anangl · 2018-06-13T14:24:10Z

arch/arm/soc/nordic_nrf/nrf52/Kconfig.series

@@ -8,15 +8,13 @@
 config  SOC_SERIES_NRF52X
 	bool "Nordic Semiconductor nRF52 series MCU"
 	select CPU_CORTEX_M4
-	select CPU_HAS_FPU


This change together with the XIP one should be extracted to a separate commit with an explanation in the commit message why these selects have been moved.

Now there are 3 logical (I hope) commits.

ulfalizer · 2018-06-13T15:02:46Z

arch/arm/soc/nordic_nrf/Kconfig

@@ -15,5 +15,7 @@ config SOC_FAMILY
 	string
 	default "nordic_nrf"

+gsource "arch/arm/soc/nordic_nrf/Kconfig.peripherals"


A plain source would do here. gsource works like an optional include (like -include in make) when you don't have any wildcards in the filename.

ioannisg · 2018-06-14T07:22:37Z

arch/arm/soc/nordic_nrf/nrf52/Kconfig.soc


 config SOC_NRF52840
 	depends on SOC_SERIES_NRF52X
 	bool
+	select CPU_HAS_FPU
+	select XIP


Why XIP is not selected in nRF52832?

nRF52832 does not have QSPI hence does not support XIP

fixed
I was told that this is different XIP. I've created one more configuration parameter:
HAS_HW_NRF_XIP.

ulfalizer

gsource nit fixed.

nordic-krch · 2018-06-14T11:34:53Z

arch/arm/soc/nordic_nrf/Kconfig.peripherals

+config HAS_HW_NRF_UARTE1
+	bool
+
+config HAS_HW_NRF_USB


should be USBD

nordic-krch · 2018-06-14T11:36:45Z

arch/arm/soc/nordic_nrf/nrf51/Kconfig.soc

@@ -11,11 +11,71 @@ depends on SOC_SERIES_NRF51X

 config SOC_NRF51822_QFAA
 	bool "NRF51822_QFAA"
+	select HAS_HW_NRF_CCM


GPIOTE, ADC missing

nordic-krch · 2018-06-14T11:36:56Z

arch/arm/soc/nordic_nrf/nrf51/Kconfig.soc


 config SOC_NRF51822_QFAB
 	bool "NRF51822_QFAB"
+	select HAS_HW_NRF_CCM


GPIOTE, ADC missing

ioannisg · 2018-06-18T07:24:37Z

arch/arm/soc/nordic_nrf/Kconfig.peripherals

+config HAS_HW_NRF_WDT
+	bool
+
+config HAS_HW_NRF_XIP


As far as I understand, this has been added to describe the QSPI peripheral that exists in nrf52840, which provides the capability to execute-in-place from external flash.

My concern is that all the above Kconfig definitions have names that directly map to the peripheral names of nrf52840 (as defined in the nRF product specification), except for this one. To be consistent, I think we should rename this to something like HAS_HW_NRF_QSPI.

ioannisg · 2018-06-18T07:26:02Z

arch/arm/soc/nordic_nrf/nrf52/Kconfig.series

 	select CPU_HAS_MPU
 	select SOC_FAMILY_NRF
 	select NRF_RTC_TIMER
 	select CLOCK_CONTROL
 	select CLOCK_CONTROL_NRF5
 	select SYS_POWER_LOW_POWER_STATE_SUPPORTED
 	select SYS_POWER_DEEP_SLEEP_SUPPORTED
-	select XIP


just for the sake of simplicity, could you revert this change (i.e. add and remove XIP in different lines)?

ioannisg

Some minor requested changes

erwango · 2018-06-18T07:46:20Z

This is duplicate from information available in device tree.
So one will have to populate this, maintain SoC dtsi file and make sure that they are aligned (else I guess there will be some issues in build system)..
Not sure this will make things more simple.

jakub-uC · 2018-06-18T07:53:59Z

Hi,
Yes, in some way this is duplicate information with DTS but it was done for the reason.
This is our interim solution to let Kconfig know what peripherals are available to display and configure only for available hardware.

There is an ogoing discussion how to solve it in future:
#7302
but until that time we want to use proposed solution.
Our approach is similar to proposals that are made, so we will need no much effort to adopt.

jakub-uC · 2018-06-18T08:16:33Z

Fixed changes requested by @ioannisg

anangl

Looks good.
But I think it would be more adequate to start the titles of the first two commits with "arch: nrf:" instead of "drivers: all:".

erwango · 2018-06-18T08:35:01Z

@jarz-nordic

This is our interim solution to let Kconfig ...

Thanks for explanation (it would have been useful to detail in initial PR description, but ok).

ioannisg · 2018-06-18T10:16:44Z

Looks good.
One last thing: "arch: nrf:" is wrong; we need "arch: arm: nrf: ..." instead.

jakub-uC · 2018-06-18T10:29:40Z

I've changed prefix to: arch: arm: nrf:

Created NRF5x peripheral list that can be used to describe each NRF5x SoC. Basing on this description Kconfig file can display and allow to configure only these drivers that are available for particular SoC. Signed-off-by: Jakub Rzeszutko <[email protected]>

Configuration parameter SOC_SERIES_NRF52X is common for all NRF5X SoCs. Due to that it cannot select: CPU_HAS_FPU because not all Nordic microcontrollers supports that. Selection of this parameter was moved to configuration of each SoC in Kconfig.soc file. Signed-off-by: Jakub Rzeszutko <[email protected]>

Condition: depends on ((SOC_SERIES_NRF52X || SOC_SERIES_NRF51X) && (!SOC_NRF52810)) for displaing configuration for UART0 peripheral has been replaced with: depends on HAS_HW_NRF_UART0. Signed-off-by: Jakub Rzeszutko <[email protected]>

carlescufi · 2018-06-18T12:22:34Z

@erwango @tbursztyka @galak This tries to address the lack of DTS data availability decribed here by listing the hardware support in Kconfig in parallel to DTS. I personally think it's a compromise until #7302 has been addressed properly, but I want to ask your opinion as well.
EDIT: Once this is addressed by #7302 then we can remove those Kconfig symbols and select statements to clean this up, but right now there is no real alternative to this.

erwango · 2018-06-18T12:54:53Z

My personal point of view is that Kconfig symbols to describe HW are duplicate information vs what should be extracted from dts. #7302 might eventually output to generation of some symbols that could then be used in Kconfig space.
Problem is that we're not even aligned on what output of #7302 should be.
I'm not sure everybody aligns on the need to be able to amend dts configuration by menuconfig.

SebastianBoe · 2018-06-18T13:36:11Z

This PR has the purpose of

allow to display and configure in Kconfig only these drivers that are available for particular SoC.

It is reasonable to expect this from the configuration system. Duplicating information between DTS and Kconfig seems reasonable to me until #7302 is resolved.

carlescufi · 2018-06-18T17:32:42Z

@MaureenHelm Does the NXP hardware support use a similar approach to this?

carlescufi · 2018-06-18T17:45:15Z

@jarz-nordic could we make some of those common to all drivers instead of them being Nordic-specific?
We could define the Kconfig items in a common place and then the rest of the hardware vendors could use those as well until we have a consistent solution using DT

MaureenHelm · 2018-06-18T17:56:08Z

Does the NXP hardware support use a similar approach to this?

I've defined some HAS_* configs in ext/hal/nxp/mcux/Kconfig to set if a given peripheral is present in an SoC, and make the corresponding driver depend on that config. It's inconsistent though, because not every type of NXP peripheral has one of these configs, and thus there are cases that a user could incorrectly enable a driver for peripheral that doesn't exist in the SoC. I tried to reconcile this in #4317 but it was rejected on the grounds that I was introducing too many new configs.

carlescufi · 2018-06-18T18:06:53Z

@MaureenHelm thanks for the input. I spoke to @nashif and I think we need to reconsider this. It might take a while until we get a proper way to obtain DT info from Kconfig, so in the meantime we could have a set of generic peripheral Kconfig options (eg. HAS_HW_ADC) which would be shared by all hardware implementations, including NXP and Nordic. I suggest we put all of those in a centralized place instead of having them spread around the drivers/ folder.
So something like dts/common/Kconfig.peripherals where we define all of those. This file would be removed once we reach an agreement on how to properly fix this.

MaureenHelm · 2018-06-18T18:44:38Z

There are a few cases in the Kinetis family where we have multiple variants of a given peripheral type (e.g., SPI, DSPI, LPSPI), so it isn't sufficient to define a generic peripheral Kconfig option like HAS_HW_SPI

nashif · 2018-06-18T19:03:06Z

so it isn't sufficient to define a generic peripheral Kconfig option like HAS_HW_SPI

It should be ok to define HAS_HW_DSPI and HAS_HW_LPSPI which can be used by other vendors if they support them. I know some of those would be vendor specific and only one vendor might have them, but at least in this case it is consistent and safe for future support.

What I dislike is having those (many) HAS_HW_XXX defined for every vendor individually.

nashif · 2018-06-18T19:05:13Z

So something like dts/common/Kconfig.peripherals

I would not put this in dts/ and make it appear as a DTS thing, DTS does nothing with those configs, at some point when move this decision to DTS we can make the change and remove them all hopefully.

Waiting to hear from the participants of the meeting last week where DTS in general was discussed @erwango , @galak , @tbursztyka

jakub-uC · 2018-06-19T05:52:34Z

What is wrong about having such file by each vendor individually?

In my opinion one file with peripheral list containing all vendors and all possible peripheral variants will make developers work more difficult.

@carlescufi : generic list of peripherals like HAS_HW_ADC is not enough for us. Some SoCs have ADC other SAADC only, some have SPI other SPIM or both. The best example is with nrf52840.
It has possibility to configure 2 serial ports at time: UART0 and UARTE1 or UARTE0 and UARTE1. It does not have UART1.

nordic-krch · 2018-06-19T05:56:25Z

I'm not sure if this all should be in common kconfig with the list of all possible options when in many cases names may be unique to vendor. It will generate long list and when vendor will be adding new peripheral it will require change to common file.

Let say that there is LPUART which is low power UART, but other vendor has LEUART which is low energy uart. Should it use LPUART or create LEUART in common list. If LPUART is used it may confuse somebody because there will be kconfigs like:

config LEUART0_ENABLE
depends on LPUART0

Additionally, we might want to not only put peripherals there but also specific features in those peripherals. Example: SPI in device X has max speed 4M and in device Y has 8M. I want to limit menu for those instances of SPI which do not support 8M. That i can to by creating flag like HAS_HW_SPI3_8M. Such flag do not belong to generic, common file for sure.

Summarizing, I think that each vendor should has own list of peripherals/features since those will be used only in Kconfig specific to that vendor and nowhere else.

carlescufi · 2018-06-19T18:02:42Z

Merging but keeping track of this in #8481

jakub-uC requested review from galak and MaureenHelm as code owners June 13, 2018 13:18

carlescufi requested a review from erwango June 13, 2018 13:19

anangl requested changes Jun 13, 2018

View reviewed changes

ulfalizer suggested changes Jun 13, 2018

View reviewed changes

jakub-uC force-pushed the nrf_hw_description_in_kconfig branch from 6a885b7 to e2625be Compare June 14, 2018 07:17

ioannisg requested changes Jun 14, 2018

View reviewed changes

ulfalizer approved these changes Jun 14, 2018

View reviewed changes

jakub-uC force-pushed the nrf_hw_description_in_kconfig branch from e2625be to 73edfa3 Compare June 14, 2018 08:15

jakub-uC mentioned this pull request Jun 14, 2018

drivers: serial: Initial support for the UARTE peripheral on the nRF52. #5257

Closed

jakub-uC changed the title ~~drivers: all: add hardware description for nrf kconfig files~~ drivers: all nrf: add hardware description for nrf kconfig files Jun 14, 2018

nordic-krch reviewed Jun 14, 2018

View reviewed changes

jakub-uC force-pushed the nrf_hw_description_in_kconfig branch 2 times, most recently from 5be0909 to 1e77d3a Compare June 14, 2018 12:44

ioannisg reviewed Jun 18, 2018

View reviewed changes

ioannisg requested changes Jun 18, 2018

View reviewed changes

ioannisg added the platform: nRF Nordic nRFx label Jun 18, 2018

jakub-uC force-pushed the nrf_hw_description_in_kconfig branch 2 times, most recently from 56c1f5b to f128104 Compare June 18, 2018 08:14

anangl reviewed Jun 18, 2018

View reviewed changes

jakub-uC force-pushed the nrf_hw_description_in_kconfig branch from 897f73e to aefba9f Compare June 18, 2018 10:28

jakub-uC changed the title ~~arch: nrf: add hardware description for nrf kconfig files~~ arch: arm: nrf: add hardware description for nrf kconfig files Jun 18, 2018

Jakub Rzeszutko added 3 commits June 18, 2018 13:09

jakub-uC force-pushed the nrf_hw_description_in_kconfig branch from 66c5bc0 to 06e7ed5 Compare June 18, 2018 11:11

ioannisg approved these changes Jun 18, 2018

View reviewed changes

anangl mentioned this pull request Jun 18, 2018

arch: arm: nrf: Use SystemInit() from MDK in SoC initialization #8447

Merged

carlescufi approved these changes Jun 18, 2018

View reviewed changes

carlescufi requested review from SebastianBoe and tbursztyka June 18, 2018 12:21

nashif approved these changes Jun 19, 2018

View reviewed changes

carlescufi mentioned this pull request Jun 19, 2018

Remove the Kconfig helper options for nRF ICs once DT can replace them #8481

Closed

carlescufi merged commit fab8246 into zephyrproject-rtos:master Jun 19, 2018

jakub-uC deleted the nrf_hw_description_in_kconfig branch January 30, 2019 14:58

		@@ -0,0 +1,208 @@
		# Kconfig.peripherals - Nordic Semiconductor nRFx MCU line

arch: arm: nrf: add hardware description for nrf kconfig files #8363

arch: arm: nrf: add hardware description for nrf kconfig files #8363

Conversation

jakub-uC commented Jun 13, 2018

jakub-uC commented Jun 13, 2018

codecov-io commented Jun 13, 2018 • edited Loading

Codecov Report

anangl commented Jun 13, 2018

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

anangl Jun 13, 2018 • 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

ulfalizer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nordic-krch Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nordic-krch Jun 14, 2018 • 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

ioannisg left a comment

Choose a reason for hiding this comment

erwango commented Jun 18, 2018

jakub-uC commented Jun 18, 2018 • edited Loading

jakub-uC commented Jun 18, 2018

anangl left a comment

Choose a reason for hiding this comment

erwango commented Jun 18, 2018 • edited Loading

ioannisg commented Jun 18, 2018

jakub-uC commented Jun 18, 2018

carlescufi commented Jun 18, 2018 • edited Loading

erwango commented Jun 18, 2018

SebastianBoe commented Jun 18, 2018

carlescufi commented Jun 18, 2018

carlescufi commented Jun 18, 2018 • edited Loading

MaureenHelm commented Jun 18, 2018

carlescufi commented Jun 18, 2018 • edited Loading

MaureenHelm commented Jun 18, 2018

nashif commented Jun 18, 2018

nashif commented Jun 18, 2018

jakub-uC commented Jun 19, 2018

nordic-krch commented Jun 19, 2018 • edited Loading

carlescufi commented Jun 19, 2018

codecov-io commented Jun 13, 2018 •

edited

Loading

anangl Jun 13, 2018 •

edited

Loading

nordic-krch Jun 14, 2018 •

edited

Loading

nordic-krch Jun 14, 2018 •

edited

Loading

jakub-uC commented Jun 18, 2018 •

edited

Loading

erwango commented Jun 18, 2018 •

edited

Loading

carlescufi commented Jun 18, 2018 •

edited

Loading

carlescufi commented Jun 18, 2018 •

edited

Loading

carlescufi commented Jun 18, 2018 •

edited

Loading

nordic-krch commented Jun 19, 2018 •

edited

Loading