Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drivers: nrf: Rework UARTE shim for uart ASYNC API and add power management #13064

Merged
merged 7 commits into from
Mar 28, 2019
Merged

drivers: nrf: Rework UARTE shim for uart ASYNC API and add power management #13064

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ef33496
drivers: nrf: Rework UARTE shim for uart ASYNC API.
Dec 20, 2018
fd2f1d3
dts: nrf: Add DPPIC to device tree.
Feb 7, 2019
bafa4bd
boards: nrf: Enable TIMER1 by default in nrf9160_pca10090
Feb 26, 2019
587aa84
drivers: nrf: Enable usage of Async UART API for UARTE on nrf91
Feb 26, 2019
00f384f
tests: uart: Add chained write test case for UART async API
Feb 19, 2019
d61351a
tests: uart: Add nrf9160_pca10090 to UART async test
Feb 20, 2019
b1d41ba
drivers: nrf: Add power management to uarte shim
Mar 21, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CODEOWNERS
View file
Open in desktop
Validating CODEOWNERS rules …
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,7 @@
/tests/drivers/can/ @alexanderwachter
/tests/drivers/hwinfo/ @alexanderwachter
/tests/drivers/spi/ @tbursztyka
/tests/drivers/uart/uart_async_api/ @Mierunski
/tests/kernel/ @andrewboie @andyross @nashif
/tests/net/ @jukkar @tbursztyka @pfalcon
/tests/net/buf/ @jukkar @jhedberg @tbursztyka @pfalcon
Expand Down
4 changes: 4 additions & 0 deletions boards/arm/nrf9160_pca10090/nrf9160_pca10090_common.dts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,10 @@
status = "ok";
};

&timer1 {
Mierunski marked this conversation as resolved.
Show resolved Hide resolved
status = "ok";
};

&timer2 {
status = "ok";
};
Expand Down
128 changes: 128 additions & 0 deletions drivers/serial/Kconfig.nrfx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,13 @@ config UART_0_INTERRUPT_DRIVEN
help
This option enables UART interrupt support on port 0.

config UART_0_ASYNC
bool "Enable Asynchronous API support on port 0"
depends on UART_ASYNC_API
default y
help
This option enables UART Asynchronous API support on port 0.

config UART_0_NRF_PARITY_BIT
bool "Enable parity bit"
help
Expand All @@ -72,6 +79,20 @@ config UART_0_NRF_TX_BUFFER_SIZE
This value is limited by range of TXD.MAXCNT register for
particular SoC.

config UART_0_NRF_HW_ASYNC
bool "Use hardware RX byte counting"
depends on UART_0_NRF_UARTE
depends on UART_ASYNC_API
help
If default driver uses interrupts to count incoming bytes, it is possible
that with higher speeds and/or high cpu load some data can be lost.
It is recommended to use hardware byte counting in such scenarios.
Hardware RX byte counting requires timer instance and one PPI channel
Mierunski marked this conversation as resolved.
Show resolved Hide resolved

config UART_0_NRF_HW_ASYNC_TIMER
int "Timer instance"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what will be the default value? if 0 then it will enable TIMER0 instance and it may conflict with bluetooth as im afraid that it's using timer0 directly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no default value, user has to set it explicitly

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So as for now default value is n

Copy link
Member

@ioannisg ioannisg Mar 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand kconfig properly (@ulfalizer can help) this will be a bit undefined, i.e. it's not really a "not-set", cause the value is not boolean. So, it appears to me that the macro is still generated (Without a value though)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If value is not set then cmake will report an error and won't generate the project

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should give the developer the option to build this without menuconfig, and as far as I can understand Kconfig, this requires a default value for int- Kconfig symbols.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This value is taken into account only when developer selects hardware byte counting. When build with default configuration there are no issues with it. I don't want to select a default value here, as it should be explicitly set by developer if he decides to use hardware byte counting.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood, now, thanks. So, I suggest you do as @ulfalizer has done in several similar cases, i.e. remove the "depends on" and add the whole Kconfig definition in if UART_0_NRF_HW_ASYNC ... endif #UART_0_NRF_HW_ASYNC

Copy link
Collaborator

@ulfalizer ulfalizer Mar 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some notes on int defaults on the best practices page.

Usually, it's a good idea to give int symbols defaults, because they don't default to zero (for semi-messy reasons). If no default is given, then the prj.conf (or board defconfig files, etc.) must set the symbol. Otherwise, you'll get a .config that generates warnings when read back in, and that probably breaks the menuconfig, because warnings are turned into errors in scripts/kconfig/kconfig.py (there's an issue for it).

(Super nit: Trying to standardize making it # UART_0_NRF_HW_ASYNC instead of #UART_0_NRF_HW_ASYNC too btw. It's more common.)

Copy link
Collaborator

@ulfalizer ulfalizer Mar 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re. if vs depends on, depends on is cleaner if there's just a single symbol with the dependency.

If there's many symbols with the same dependencies in a row, if is cleaner.

It's a bit subjective when you have just two symbols or the like. It's good to be aware that if FOO is just a shorthand for adding depends on FOO to all the stuff within at least.

People tend to read a bunch of things into Kconfig that aren't there (probably because the official docs aren't great). :)

depends on UART_0_NRF_HW_ASYNC

endif # UART_0_NRF_UART || UART_0_NRF_UARTE

# ----------------- port 1 -----------------
Expand All @@ -94,6 +115,13 @@ config UART_1_INTERRUPT_DRIVEN
help
This option enables UART interrupt support on port 1.

config UART_1_ASYNC
bool "Enable Asynchronous API support on port 1"
depends on UART_ASYNC_API
default y
help
This option enables UART Asynchronous API support on port 1.

config UART_1_NRF_PARITY_BIT
bool "Enable parity bit"
help
Expand All @@ -107,13 +135,27 @@ config UART_1_NRF_FLOW_CONTROL

config UART_1_NRF_TX_BUFFER_SIZE
int "Size of RAM buffer"
depends on UART_INTERRUPT_DRIVEN
range 1 65535
default 32
help
Size of the transmit buffer for API function: fifo_fill.
This value is limited by range of TXD.MAXCNT register for
particular SoC.

config UART_1_NRF_HW_ASYNC
bool "Use hardware RX byte counting"
depends on UART_1_ASYNC
help
If default driver uses interrupts to count incoming bytes, it is possible
that with higher speeds and/or high cpu load some data can be lost.
It is recommended to use hardware byte counting in such scenarios.
Hardware RX byte counting requires timer instance and one PPI channel

config UART_1_NRF_HW_ASYNC_TIMER
int "Timer instance"
depends on UART_1_NRF_HW_ASYNC

endif # UART_1_NRF_UARTE

# ----------------- port 2 -----------------
Expand All @@ -136,6 +178,13 @@ config UART_2_INTERRUPT_DRIVEN
help
This option enables UART interrupt support on port 2.

config UART_2_ASYNC
bool "Enable Asynchronous API support on port 2"
depends on UART_ASYNC_API
default y
help
This option enables UART Asynchronous API support on port 2.

config UART_2_NRF_PARITY_BIT
bool "Enable parity bit"
help
Expand All @@ -156,6 +205,19 @@ config UART_2_NRF_TX_BUFFER_SIZE
This value is limited by range of TXD.MAXCNT register for
particular SoC.

config UART_2_NRF_HW_ASYNC
bool "Use hardware RX byte counting"
depends on UART_2_ASYNC
help
If default driver uses interrupts to count incoming bytes, it is possible
that with higher speeds and/or high cpu load some data can be lost.
It is recommended to use hardware byte counting in such scenarios.
Hardware RX byte counting requires timer instance and one PPI channel

config UART_2_NRF_HW_ASYNC_TIMER
int "Timer instance"
depends on UART_2_NRF_HW_ASYNC

endif # UART_2_NRF_UARTE

# ----------------- port 3 -----------------
Expand All @@ -178,6 +240,13 @@ config UART_3_INTERRUPT_DRIVEN
help
This option enables UART interrupt support on port 3.

config UART_3_ASYNC
bool "Enable Asynchronous API support on port 3"
depends on UART_ASYNC_API
default y
help
This option enables UART Asynchronous API support on port 3.

config UART_3_NRF_PARITY_BIT
bool "Enable parity bit"
help
Expand All @@ -198,8 +267,67 @@ config UART_3_NRF_TX_BUFFER_SIZE
This value is limited by range of TXD.MAXCNT register for
particular SoC.

config UART_3_NRF_HW_ASYNC
bool "Use hardware RX byte counting"
depends on UART_3_ASYNC
help
If default driver uses interrupts to count incoming bytes, it is possible
that with higher speeds and/or high cpu load some data can be lost.
It is recommended to use hardware byte counting in such scenarios.
Hardware RX byte counting requires timer instance and one PPI channel

config UART_3_NRF_HW_ASYNC_TIMER
int "Timer instance"
depends on UART_3_NRF_HW_ASYNC

endif # UART_3_NRF_UARTE


config NRFX_TIMER0
default y
Mierunski marked this conversation as resolved.
Show resolved Hide resolved
depends on UART_0_NRF_HW_ASYNC_TIMER = 0 || UART_1_NRF_HW_ASYNC_TIMER = 0 || \
UART_3_NRF_HW_ASYNC_TIMER = 0 || UART_2_NRF_HW_ASYNC_TIMER = 0

config NRFX_TIMER1
default y
Mierunski marked this conversation as resolved.
Show resolved Hide resolved
depends on UART_0_NRF_HW_ASYNC_TIMER = 1 || UART_1_NRF_HW_ASYNC_TIMER = 1 || \
UART_3_NRF_HW_ASYNC_TIMER = 1 || UART_2_NRF_HW_ASYNC_TIMER = 1

config NRFX_TIMER2
default y
Mierunski marked this conversation as resolved.
Show resolved Hide resolved
depends on UART_0_NRF_HW_ASYNC_TIMER = 2 || UART_1_NRF_HW_ASYNC_TIMER = 2 || \
UART_3_NRF_HW_ASYNC_TIMER = 2 || UART_2_NRF_HW_ASYNC_TIMER = 2

config NRFX_TIMER3
default y
depends on UART_0_NRF_HW_ASYNC_TIMER = 3 || UART_1_NRF_HW_ASYNC_TIMER = 3 || \
UART_3_NRF_HW_ASYNC_TIMER = 3 || UART_2_NRF_HW_ASYNC_TIMER = 3

config NRFX_TIMER4
default y
depends on UART_0_NRF_HW_ASYNC_TIMER = 4 || UART_1_NRF_HW_ASYNC_TIMER = 4 || \
UART_3_NRF_HW_ASYNC_TIMER = 4 || UART_2_NRF_HW_ASYNC_TIMER = 4


if UART_0_NRF_HW_ASYNC || UART_1_NRF_HW_ASYNC || UART_2_NRF_HW_ASYNC || UART_3_NRF_HW_ASYNC

config NRFX_TIMER
default y

config NRFX_PPI
depends on HAS_HW_NRF_PPI
default y

config NRFX_DPPI
depends on HAS_HW_NRF_DPPIC
default y

config UARTE_NRF_HW_ASYNC
bool
default y

endif

config NRF_UART_PERIPHERAL
bool

Expand Down
Loading