modules: hal: nordic: Add missing mappings of peripheral base addresses #18937

anangl · 2019-09-05T14:01:02Z

For quite a few peripherals that are currently supported by nrfx HALs
or drivers there are no definitions of corresponding CMSIS-Core
peripheral accessing symbols that would provide their base addresses
in the proper domain (secure or non-secure), accordingly to the build
target. This commits adds devicetree nodes for these peripherals and
updates the revision of the hal_nordic module so that it can provide
the missing symbol definitions basing on data from the devicetree.

Requires zephyrproject-rtos/hal_nordic#3 to go in first.

ulfalizer · 2019-09-05T14:16:09Z

@anangl
Might want to get this in via the topic-dts branch and change

inherits:
    !include base.yaml

to

include: base.yaml

and

category: optional/required

to

required: false/true

That'll save some later changes, though the stuff on the topic-dts branch is mostly backwards-compatible.

ioannisg

It's more like "dts: nordic: nrf9160: " It seems to be a PR for DTS updates for nRF9160

anangl · 2019-09-05T15:27:23Z

I forgot about the aliases for non-secure part. As usual...
Fixed.

anangl · 2019-09-05T15:30:18Z

@ioannisg

It's more like "dts: nordic: nrf9160: " It seems to be a PR for DTS updates for nRF9160

The actual purpose of these changes in dts is to provide data for these mappings of base addresses. That's why I used such title. But indeed, it seems that it will be clearer to use two commits - one with the dts changes and one with the update of west manifest. What you think?

ioannisg · 2019-09-05T15:31:37Z

@ioannisg

It's more like "dts: nordic: nrf9160: " It seems to be a PR for DTS updates for nRF9160

The actual purpose of these changes in dts is to provide data for these mappings of base addresses. That's why I used such title. But indeed, it seems that it will be clearer to use two commits - one with the dts changes and one with the update of west manifest. What you think?

Yeap, that seems better

pabigot

This looks fine as a cleanup, though I second @ulfalizer's recommendation to wait until material currently staged in topic-dts is merged. Also waiting until west.yml references a SHA1 before approving.

One thing I don't understand, which this PR makes very obvious, is why there aliases (e.g. i2c-0) for every node label (i2c0) (not the same as the node label property value "I2C_0"). There are a very small number of situations where an alias is still necessary, primarily for cross-target common resources like sw0 and led1, which probably should be handled a different way.

In all of Zephyr master there is one driver, three samples, and four tests that reference generated alias symbols.

It's not in scope for this PR, but we should move towards getting rid of those.

anangl · 2019-09-09T05:15:50Z

Rebased on master.

anangl · 2019-09-09T08:26:31Z

Updated west.yml with the proper revision of the hal_nordic module as zephyrproject-rtos/hal_nordic#3 has been merged.

galak · 2019-09-09T14:29:46Z

We use aliases to get macros generated from dts without base addresses in their names. We use such macros in drivers and also in nrfx_config_nrf9160.h. The thing is that the DT_INST_* macros cannot be used for these purposes as the index contained in the macro name does not necessarily match the index of the hardware instance it refers to. For instance DT_INST_0_NORDIC_NRF_I2C_BASE_ADDRESS may not refer to NRF_TWIM0 while DT_NORDIC_NRF_I2C_I2C_0_BASE_ADDRESS, which is generated for the i2c-0 alias, always refers to this particular hardware instance.

What's the issue with the _INST_ defines? I understand that INST_0 doesn't necessary refer to the same HW instance always, but when do you need to know which specific hw instance it is?

pabigot · 2019-09-09T14:31:13Z

@galak The hardware address for the instance is being taken from the devicetree information.

galak · 2019-09-09T15:51:35Z

@galak The hardware address for the instance is being taken from the devicetree information.

what do you mean by the hardware address? If it's taken from the DT info, then why do we care about needing to know the specific instance from the HW perspective?

The reason DT_INST_ can't be used for various drivers on most SoCs is related to pinmux info. Once we're able to generate pinmux info for a given SoC that should free up the driver to utilize DT_INST_ defines. So I'm trying to understand in the nordic case what prevents to the use of DT_INST_ defines today.

pabigot · 2019-09-09T17:41:29Z

This isn't my area, but: what ~~@nvlsianpu~~ sorry, @anangl was referencing above is that Nordic HAL module in its Zephyr fork uses the devicetree content to determine the hardware addresses to use for symbols like PWM0 which are supposed to correspond to the first instance of the PWM peripheral, rather than hard-coding a constant that might end up inconsistent with what's in the devicetree.

If Zephyr's identification of that instance is not the same as the one selected by other systems that Zephyr applications must coordinate with (such as a softdevice blob) things will go very badly wrong. Because DT_INST_0_FOO is not always the vendor-defined FOO0 you can't use the generated symbols that way. You could use DT_ALIAS_FOO_0 as now, or when supported avoid an alias and use DT_LABEL_FOO0 which is fixed to identify the node with a particular address.

galak · 2019-09-09T19:14:39Z

This isn't my area, but: what ~~@nvlsianpu~~ sorry, @anangl was referencing above is that Nordic HAL module in its Zephyr fork uses the devicetree content to determine the hardware addresses to use for symbols like PWM0 which are supposed to correspond to the first instance of the PWM peripheral, rather than hard-coding a constant that might end up inconsistent with what's in the devicetree.

If Zephyr's identification of that instance is not the same as the one selected by other systems that Zephyr applications must coordinate with (such as a softdevice blob) things will go very badly wrong. Because DT_INST_0_FOO is not always the vendor-defined FOO0 you can't use the generated symbols that way. You could use DT_ALIAS_FOO_0 as now, or when supported avoid an alias and use DT_LABEL_FOO0 which is fixed to identify the node with a particular address.

@anangl where is this mapping happing between the DT_ALIAS define and nordic internals?

pabigot · 2019-09-09T19:23:51Z

https://github.com/zephyrproject-rtos/hal_nordic/blob/master/nrfx_config_nrf9160.h

pabigot · 2019-09-09T19:26:37Z

Or maybe not, as I don't see it there. I know it was somewhere.... I'm going to stop with this discussion because it's clearly outside my ken, but I still want #18986 to get some love regardless.

anangl · 2019-09-10T07:28:43Z

@galak

@anangl where is this mapping happing between the DT_ALIAS define and nordic internals?

These mappings do not use DT_ALIAS macros but the older ones generated for aliases: DT_<compatible>_<alias_name>_*. And they are in the file pointed out by @pabigot. Here is one example from the revision of this file currently used by master (this PR updates the hal_nordic module to the revision that adds more such mappings).

anangl · 2019-09-10T11:41:45Z

Rebased on master and in result skipped the commit modifying west.yml as the needed revision update already went in through #18895.

ulfalizer

Some nits re. new binding stuff.

ulfalizer · 2019-09-10T12:59:58Z

dts/bindings/arm/nordic,nrf-egu.yaml

+
+properties:
+    compatible:
+      constraint: "nordic,nrf-egu"


Could be replaced with a

compatible: "nordic,nrf-egu"

at the top level now. That avoids a deprecation warning too.

ulfalizer · 2019-09-10T13:00:16Z

dts/bindings/arm/nordic,nrf-kmu.yaml

+
+properties:
+    compatible:
+      constraint: "nordic,nrf-kmu"


Ditto here.

ulfalizer · 2019-09-10T13:00:56Z

dts/bindings/arm/nordic,nrf-egu.yaml

+# Copyright (c) 2019 Nordic Semiconductor ASA
+#
+# SPDX-License-Identifier: Apache-2.0
+#


Could shorten the header a bit to

# Copyright (c) 2019 Nordic Semiconductor ASA # SPDX-License-Identifier: Apache-2.0

I changed it to that in a bunch of other files.

ulfalizer · 2019-09-10T13:01:09Z

dts/bindings/arm/nordic,nrf-kmu.yaml

+# Copyright (c) 2019 Nordic Semiconductor ASA
+#
+# SPDX-License-Identifier: Apache-2.0
+#


Ditto here re. header.

ulfalizer · 2019-09-10T13:01:37Z

dts/bindings/audio/nordic,nrf-pdm.yaml

+
+properties:
+    compatible:
+      constraint: "nordic,nrf-pdm"


Ditto here re. compatible.

ulfalizer · 2019-09-10T13:02:34Z

dts/bindings/i2s/nordic,nrf-i2s.yaml

+
+properties:
+    compatible:
+      constraint: "nordic,nrf-i2s"


Ditto here re. compatible.

ulfalizer · 2019-09-10T13:02:47Z

dts/bindings/power/nordic,nrf-regulators.yaml

+# Copyright (c) 2019 Nordic Semiconductor ASA
+#
+# SPDX-License-Identifier: Apache-2.0
+#


Ditto here re. header.

ulfalizer · 2019-09-10T13:02:58Z

dts/bindings/power/nordic,nrf-regulators.yaml

+
+properties:
+    compatible:
+      constraint: "nordic,nrf-regulators"


Ditto here re. compatible.

ulfalizer · 2019-09-10T13:03:08Z

dts/bindings/power/nordic,nrf-vmc.yaml

+# Copyright (c) 2019 Nordic Semiconductor ASA
+#
+# SPDX-License-Identifier: Apache-2.0
+#


Ditto here re. header.

ulfalizer · 2019-09-10T13:03:18Z

dts/bindings/power/nordic,nrf-vmc.yaml

+
+properties:
+    compatible:
+      constraint: "nordic,nrf-vmc"


Ditto here re. compatible.

ulfalizer · 2019-09-10T13:06:09Z

edtlib prints deprecation warnings that say what needs to be changed btw.

For quite a few peripherals that are currently supported by nrfx HALs or drivers there are no definitions of corresponding CMSIS-Core peripheral accessing symbols that would provide their base addresses in the proper domain (secure or non-secure), accordingly to the build target. This commits adds devicetree nodes for these peripherals so that their base addresses can be used in definitions of the accessing symbols mentioned above. Signed-off-by: Andrzej Głąbek <andrzej.glabek@nordicsemi.no>

anangl · 2019-09-10T15:47:47Z

@ulfalizer I addressed your comments.

ulfalizer

Thanks

anangl added area: Devicetree platform: nRF Nordic nRFx Blocked Blocked by another PR or issue DNM This PR should not be merged (Do Not Merge) area: Modules labels Sep 5, 2019

anangl requested review from ioannisg, carlescufi, galak and Mierunski September 5, 2019 14:01

carlescufi requested review from pabigot and ulfalizer September 5, 2019 14:05

ioannisg reviewed Sep 5, 2019

View reviewed changes

anangl force-pushed the add_missing_nrf9160_peripheral_symbols branch from 8f87b63 to 73e6a7e Compare September 5, 2019 15:26

pabigot reviewed Sep 5, 2019

View reviewed changes

anangl force-pushed the add_missing_nrf9160_peripheral_symbols branch from 73e6a7e to 8df60b8 Compare September 6, 2019 10:50

anangl requested review from alexanderwachter, avisconti, dbkinder and erwango as code owners September 6, 2019 10:50

anangl requested a review from a user September 6, 2019 10:50

anangl requested review from kgugala, MaureenHelm, nategraff-sifive and pgielda as code owners September 6, 2019 10:50

anangl changed the base branch from master to topic-dts September 6, 2019 10:50

anangl removed the request for review from avisconti September 6, 2019 10:51

anangl removed Blocked Blocked by another PR or issue DNM This PR should not be merged (Do Not Merge) labels Sep 9, 2019

anangl force-pushed the add_missing_nrf9160_peripheral_symbols branch from 287c170 to 2037a73 Compare September 9, 2019 08:22

pabigot approved these changes Sep 9, 2019

View reviewed changes

ioannisg requested a review from galak September 9, 2019 11:40

ioannisg self-assigned this Sep 9, 2019

ioannisg added this to the v2.1.0 milestone Sep 9, 2019

anangl force-pushed the add_missing_nrf9160_peripheral_symbols branch from 2037a73 to 24f4e97 Compare September 10, 2019 11:39

ioannisg approved these changes Sep 10, 2019

View reviewed changes

carlescufi approved these changes Sep 10, 2019

View reviewed changes

ulfalizer suggested changes Sep 10, 2019

View reviewed changes

anangl force-pushed the add_missing_nrf9160_peripheral_symbols branch from 24f4e97 to 5528b44 Compare September 10, 2019 15:46

ulfalizer approved these changes Sep 10, 2019

View reviewed changes

carlescufi merged commit a19356d into zephyrproject-rtos:master Sep 10, 2019

anangl deleted the add_missing_nrf9160_peripheral_symbols branch September 11, 2019 05:36

pabigot mentioned this pull request Sep 19, 2019

devicetree: fixed non-alias reference to specific nodes #19285

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

modules: hal: nordic: Add missing mappings of peripheral base addresses #18937

modules: hal: nordic: Add missing mappings of peripheral base addresses #18937

anangl commented Sep 5, 2019

ulfalizer commented Sep 5, 2019 •

edited

Loading

ioannisg left a comment

anangl commented Sep 5, 2019

anangl commented Sep 5, 2019

ioannisg commented Sep 5, 2019

pabigot left a comment

anangl commented Sep 9, 2019

anangl commented Sep 9, 2019

galak commented Sep 9, 2019

pabigot commented Sep 9, 2019

galak commented Sep 9, 2019

pabigot commented Sep 9, 2019 •

edited

Loading

galak commented Sep 9, 2019

pabigot commented Sep 9, 2019

pabigot commented Sep 9, 2019

anangl commented Sep 10, 2019 •

edited

Loading

anangl commented Sep 10, 2019

ulfalizer left a comment

ulfalizer Sep 10, 2019

ulfalizer Sep 10, 2019

ulfalizer Sep 10, 2019

ulfalizer Sep 10, 2019

ulfalizer Sep 10, 2019

ulfalizer Sep 10, 2019

ulfalizer Sep 10, 2019

ulfalizer Sep 10, 2019

ulfalizer Sep 10, 2019

ulfalizer Sep 10, 2019

ulfalizer commented Sep 10, 2019

anangl commented Sep 10, 2019

ulfalizer left a comment

modules: hal: nordic: Add missing mappings of peripheral base addresses #18937

modules: hal: nordic: Add missing mappings of peripheral base addresses #18937

Conversation

anangl commented Sep 5, 2019

ulfalizer commented Sep 5, 2019 • edited Loading

ioannisg left a comment

Choose a reason for hiding this comment

anangl commented Sep 5, 2019

anangl commented Sep 5, 2019

ioannisg commented Sep 5, 2019

pabigot left a comment

Choose a reason for hiding this comment

anangl commented Sep 9, 2019

anangl commented Sep 9, 2019

galak commented Sep 9, 2019

pabigot commented Sep 9, 2019

galak commented Sep 9, 2019

pabigot commented Sep 9, 2019 • edited Loading

galak commented Sep 9, 2019

pabigot commented Sep 9, 2019

pabigot commented Sep 9, 2019

anangl commented Sep 10, 2019 • edited Loading

anangl commented Sep 10, 2019

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

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

ulfalizer commented Sep 10, 2019

anangl commented Sep 10, 2019

ulfalizer left a comment

Choose a reason for hiding this comment

ulfalizer commented Sep 5, 2019 •

edited

Loading

pabigot commented Sep 9, 2019 •

edited

Loading

anangl commented Sep 10, 2019 •

edited

Loading