scripts/dts: globals: parse gpio-map nodes #15861

erwango · 2019-05-03T14:48:15Z

gpio-map is defined in dts v0.3 as "nexus node".
It allows to describe a pin connector so it can be referenced
through phandles and hence used in expansion device nodes
(typically implemented through overlays).

This change implements gpio controller resolution through these maps.
Few assumptions were taken in order to simplify the implementation.
These assumptions enforce some limitations to the use of gpio-map
but my understanding is that this should still allow to cover most
use cases.

Assumptions:
-gpio-size is the same for all gpio-controllers referenced in a map
-optional properties gpio-map-mask and gpio-map-pass-thru are
supposed to be omitted
The understanding of this last assumption is that flags provided in
the expansion device node will overwrite the connector flags

Fixes #15637

Signed-off-by: Erwan Gouriou [email protected]

erwango · 2019-05-03T14:57:20Z

@mike-scott, I had a try on a (former) modified version of your PR and it worked. Can you have a try when you have some time?

EDIT: Here is a branch making it working on top of #14057. Remaining detail is the change of a _0 to _1. Otherwise it, at least, compiles. I haven't been able to test though.

ulfalizer · 2019-05-06T11:57:36Z

scripts/dts/extract/globals.py

Unused import

Oversight, txs!

ulfalizer · 2019-05-06T12:00:13Z

scripts/dts/extract/globals.py

Using .get('gpio-map') instead of ['gpio-map'] here isn't safer, because if raw_map ever ends up as None, then len(raw_map) below will crash.

Ok, so using ['gpio-map'], then.
This should not end up as None since function is called only if gpio-map in reduced....

ulfalizer · 2019-05-06T12:00:38Z

scripts/dts/extract/globals.py

Ditto here, re. .get() vs. [].

ulfalizer · 2019-05-06T12:01:12Z

scripts/dts/extract/globals.py

Ditto here, re. .get() vs. [].

ulfalizer · 2019-05-06T12:04:59Z

scripts/dts/extract/globals.py

A bare try: ... except: is usually a bad idea, because it will hide stuff like references to undefined Python variables as well (those throw exceptions).

Catch Exception instead, or (better) do a more explicit check for whatever this is checking, without using exceptions.

Yeah, so actually this is the result of a late refactoring and this not chacking anything good anymore. Let me rework that.
EDIT: Actually it was catching stuff, but edited wrt your review

ulfalizer · 2019-05-06T12:05:19Z

scripts/dts/extract/globals.py

Ditto here.

erwango · 2019-05-06T12:45:33Z

@ulfalizer, thanks for the review. Updated.

ulfalizer

Do you think the function could have some x_to_y()-like name btw, so you can tell from the name what it maps to what? From the current name, you can only tell that it's doing some kind of mapping somehow related to GPIO.

ulfalizer · 2019-05-06T13:13:28Z

scripts/dts/extract/globals.py

Does the : mean that cell_parent is a gpio-map? Might be clearer to write it out in that case ("cell_parent, which is a gpio-map").

"From the object provided" is a bit confusing since the function takes two arguments.

ulfalizer · 2019-05-06T13:15:06Z

scripts/dts/extract/globals.py

These two are only used at the end of the function. Might be clearer to move them there, right before where they're used.

Then the documentation for the function ends up at the top too.

ulfalizer · 2019-05-06T13:17:42Z

scripts/dts/extract/globals.py

Maybe gpio_map_lookup() could return None or something instead if the controller isn't found. Reading this quickly, you wonder if the IndexError is there to guard the elem[1] lookup or the like.

Exceptions for control flow often gets confusing.

So I reworked a bit, I'm not finding that much better in terms of exception, but seems more coherent in the dt object type handling. Let me know.

mike-scott · 2019-05-06T16:01:00Z

@mike-scott, I had a try on a (former) modified version of your PR and it worked. Can you have a try when you have some time?

EDIT: Here is a branch making it working on top of #14057. Remaining detail is the change of a _0 to _1. Otherwise it, at least, compiles. I haven't been able to test though.

Absolutely. Thank you for working on this. I'll respond back once I kick the tires a bit and look through this.

mike-scott · 2019-05-06T16:03:30Z

@erwango Actually, I don't see the branch based on the shield changes? Did I miss a link?

erwango · 2019-05-06T16:05:30Z

@mike-scott, you didn't miss anything, I omited the paste operation. Here it is: https://github.com/erwango/zephyr/tree/test_14057_gpio_map

EDIT: It does not contain the last changes of current PR, but this should not change the global behavior.

ulfalizer · 2019-05-06T15:54:12Z

scripts/dts/extract/globals.py

Could reflow this paragraph so that the lines are about the same length (with e.g. gq in Vim).

ulfalizer · 2019-05-06T16:03:15Z

scripts/dts/extract/globals.py

Accidental period in the middle of the sentence?

ulfalizer · 2019-05-06T16:05:32Z

scripts/dts/extract/globals.py

Can't immediately tell which of the [gpio_index], [child_specifier_size], and [controller_phandle] lookups are meant to be able to generate the IndexError under "normal" operation of the program here.

Better to do something like this, and avoid try/except completely:

if gpio_index not in map_array: return None ...

That makes the intention clear, and means that errors in the code won't be eaten by the except IndexError:.

ulfalizer · 2019-05-06T16:07:31Z

scripts/dts/extract/globals.py

Should be is not None, not is not 'None'. Just if not gpio_controller: should work though.

Something like this:

if not gpio_controller: raise Exception("GPIO controller not found") cell_parent = gpio_controller

ulfalizer · 2019-05-06T16:07:53Z

scripts/dts/extract/globals.py

Ditto here.

mike-scott · 2019-05-07T06:46:15Z

@erwango I tested this tonight and noticed that the values for the PINs was the index of the gpio-map used (example is for Sara R4 modem -- see header pins 12 and 11):

		mdm-reset-gpios = <&arduino_header 12 0>;
		mdm-power-gpios = <&arduino_header 11 0>;

Example:

#define DT_NXP_KINETIS_UART_4006D000_UBLOX_SARA_R4_4006D000_SARA_R4_MDM_POWER_GPIOS_CONTROLLER		"GPIO_0"
#define DT_NXP_KINETIS_UART_4006D000_UBLOX_SARA_R4_4006D000_SARA_R4_MDM_POWER_GPIOS_FLAGS		0
#define DT_NXP_KINETIS_UART_4006D000_UBLOX_SARA_R4_4006D000_SARA_R4_MDM_POWER_GPIOS_PIN			11
#define DT_NXP_KINETIS_UART_4006D000_UBLOX_SARA_R4_4006D000_SARA_R4_MDM_RESET_GPIOS_CONTROLLER		"GPIO_2"
#define DT_NXP_KINETIS_UART_4006D000_UBLOX_SARA_R4_4006D000_SARA_R4_MDM_RESET_GPIOS_FLAGS		0
#define DT_NXP_KINETIS_UART_4006D000_UBLOX_SARA_R4_4006D000_SARA_R4_MDM_RESET_GPIOS_PIN			12

To fix this, I used this hacky code which updates the prop_values based on the content of the gpio-map:

diff --git a/scripts/dts/extract/globals.py b/scripts/dts/extract/globals.py
index 8f1097a803..a835e2fa89 100644
--- a/scripts/dts/extract/globals.py
+++ b/scripts/dts/extract/globals.py
@@ -331,7 +331,7 @@ def build_cell_array(prop_array):
 
     return ret_array
 
-def gpio_map_lookup(cell_parent, gpio_index):
+def gpio_map_lookup(cell_parent, gpio_index, prop_values):
     index = 0
     map_array = []
 
@@ -366,6 +366,15 @@ def gpio_map_lookup(cell_parent, gpio_index):
         map_array.append(raw_map[index:index+array_cell_size])
         index += array_cell_size
 
+    # if prop_values, then update the # of parent_specifier_size
+    if prop_values != None:
+        index = 0
+        while index < parent_specifier_size:
+            # assign prop_value skipping controller phandle
+            prop_values[index + 1] = map_array[gpio_index][child_specifier_size + 1 + index]
+            index += 1
+        print("AFTER ", prop_values)
+
     # Return the gpio controller phandle matching the provided index
     return phandles[map_array[gpio_index][child_specifier_size]]
 
@@ -393,7 +403,7 @@ def extract_controller(node_path, prop, prop_values, index,
             # parent is a 'gpio nexus node' (ie has gpio-map)
             try:
                 # controller is to be found in the map, using elem[1] as index
-                cell_parent = gpio_map_lookup(cell_parent, elem[1])
+                cell_parent = gpio_map_lookup(cell_parent, elem[1], prop_values)
             except IndexError:
                 raise Exception("GPIO index not available in gpio-map")
 
@@ -459,7 +469,7 @@ def extract_cells(node_path, prop, prop_values, names, index,
             # parent is a 'gpio nexus node' (ie has gpio-map)
             try:
                 # controller is to be found in the map, using elem[1] as index
-                cell_parent = gpio_map_lookup(cell_parent, elem[1])
+                cell_parent = gpio_map_lookup(cell_parent, elem[1], None)
             except IndexError:
                  raise Exception("GPIO index not available in gpio-map")

And ended up with:

#define DT_NXP_KINETIS_UART_4006D000_UBLOX_SARA_R4_4006D000_SARA_R4_MDM_POWER_GPIOS_CONTROLLER		"GPIO_0"
#define DT_NXP_KINETIS_UART_4006D000_UBLOX_SARA_R4_4006D000_SARA_R4_MDM_POWER_GPIOS_FLAGS		0
#define DT_NXP_KINETIS_UART_4006D000_UBLOX_SARA_R4_4006D000_SARA_R4_MDM_POWER_GPIOS_PIN			2
#define DT_NXP_KINETIS_UART_4006D000_UBLOX_SARA_R4_4006D000_SARA_R4_MDM_RESET_GPIOS_CONTROLLER		"GPIO_2"
#define DT_NXP_KINETIS_UART_4006D000_UBLOX_SARA_R4_4006D000_SARA_R4_MDM_RESET_GPIOS_FLAGS		0
#define DT_NXP_KINETIS_UART_4006D000_UBLOX_SARA_R4_4006D000_SARA_R4_MDM_RESET_GPIOS_PIN			2

erwango · 2019-05-07T06:49:45Z

@Hashcode, Txs for the info, it worked initially, but it seems I've not tested the refactoring enough. Let me rework that.

erwango · 2019-05-07T09:58:44Z

@ulfalizer, here is the last batch, which has the merit of fixing bug reported by @Hashcode (so slightly more complex treatment) and I hope fixing your comments (hence somewhat more readable).

ulfalizer · 2019-05-07T10:14:29Z

scripts/dts/extract/globals.py

Call it _ instead of unused. That's common in Python, and pylint knows not to flag it.

ok, thanks for the hint.

ulfalizer · 2019-05-07T10:19:25Z

scripts/dts/extract/globals.py

Is "v0.3" a typo? Can't find that version online.

It's available here: https://github.com/devicetree-org/devicetree-specification/blob/4b1dac80eaca45b4babf5299452a951008a5d864/source/devicetree-basics.rst#nexus-nodes-and-specifier-mapping.
"Soon to be released" according to maintainer.

EDIT: I omitted to mention that this is already supported by zephyr dtc.

Sure it'll be called v0.3?

Current one is v0.2, so I guess this is a reasonable assumption (https://www.devicetree.org/)

Bad idea to refer to a v0.3 version that's not out and not sure to be called v0.3 I think. Could say something like "in the upcoming (presumably v0.3) Device Tree specification" instead.

Small typo: devide -> device

ulfalizer · 2019-05-07T10:28:58Z

scripts/dts/extract/globals.py

More of a comment, but instead of "aim of this function...", it might be better to say what the function does.

"This function takes a foo and a bar and returns a (baz, qaz) tuple. If foo can't be found, this other value is returned instead", etc.

ulfalizer · 2019-05-07T10:29:04Z

scripts/dts/extract/globals.py

I don't get this part. How is child_specifier_size ("The number of 32-bit cells required to specify this component", according to the DT spec) related to phandles?

gpio-map is encoded in the following way:

gpio-map = <0 0 &soc_gpio1 1 0>, <1 0 &soc_gpio2 4 0>, <2 0 &soc_gpio1 3 0>, <3 0 &soc_gpio2 2 0>;

Taking the first line:
0 0: Child specifier (pin number, pin flag)
&soc_gpio2: GPIO controller phandle
1 0: Parent specifier (pin number, pin flag)

At this point we're looking for GPIO controller phandle which is the element number 'child_specifier_size+1' in the line.

About gpio-map pin flags (does not relates directly to your question, but this might be required for the full comprehension of the change):
Assuming optional params gpio-map-mask and gpio-map-pass-thru are omitted, child and parent specifier pin flags are not taken into account and pin flag used is the one provided in calling node (typically used in shield overlay file), which is the behavior we expect, at least in current shield use case. This is "Assumption 2".

AFAICS, this is looking at #gpio-cells though. It's saying "the number of GPIO cells + 1 is a phandle". Seems weird to me.

Sorry if I'm missing something.

So:
0 0: Child specifier (pin number, pin flag) -> length provided by current node's #gpio-cell
&soc_gpio2: GPIO controller phandle
1 0: Parent specifier (pin number, pin flag) -> length provided by GPIO controller's #gpio-cell

You're never indexing into gpio-map though. You're taking #gpio-cells + 1 and directly treating that as a phandle.

Indeed, Txs!

ulfalizer · 2019-05-07T12:08:56Z

scripts/dts/extract/globals.py

"Now that the length of each entry in 'gpio-map' is known..." maybe.

ulfalizer · 2019-05-07T12:10:19Z

scripts/dts/extract/globals.py

Do you think this could be called gpio_map_entries or the like?

ulfalizer · 2019-05-07T12:10:41Z

scripts/dts/extract/globals.py

Do you think this could be called gpio_map?

ulfalizer · 2019-05-07T12:12:03Z

scripts/dts/extract/globals.py

...then this would read as

gpio_map_entries.append(gpio_map[index:index+array_cell_size])

ulfalizer · 2019-05-07T12:19:56Z

scripts/dts/extract/globals.py

Is this looking up the right element?

Note that it's not comparing gpio_index against item[0] for each item in map_array, unlike the code above it.

Could maybe do something like this:

for entry in gpio_map_entries: if entry[0] == gpio_index: parent_controller_phandle = entry[child_specifier_size] ... return phandles[...] # gpio_index does not appear in gpio-map return None, None

Right, I kinda assumed first cell of each raw would be same as raw index.
This is indeed a shortcut, as it could be designed otherwise.

ulfalizer · 2019-05-07T13:29:06Z

scripts/dts/extract/globals.py

Could get rid of gpio_map_entries at this point, and do this:

for i in range(0, len(gpio_map), array_cell_size): entry = gpio_map[i:i+array_cell_size] if entry[0] == gpio_index: ... return ... # gpio_index did not match any entry in gpio-map return None, None

Done, and reworked comments

ulfalizer · 2019-05-07T13:59:51Z

scripts/dts/extract/globals.py

ulfalizer · 2019-05-07T14:01:12Z

scripts/dts/extract/globals.py

Forgot to update this comment.

Indeed, fixed

ulfalizer · 2019-05-07T14:02:23Z

scripts/dts/extract/globals.py

Still says "raws". Could say "the size of each entry in 'gpio-map'" or the like.

Ohh... maybe 'raws' is a typo for 'rows'. I kept reading it as something "raw".

"Before parsing, we need to know the row size in 'gpio-map'", maybe.

Indeed, fixed

gpio-map is a property of "nexus node", defined in dts v0.3. It allows to describe a pin connector so it can be referenced through phandles and hence used in expansion device nodes like a shield header (typically implemented through overlays). This change implements gpio controller resolution through these maps. Few assumptions were taken in order to simplify the implementation. These assumptions bring some limitations to the use of gpio-map but my understanding is that this should still allow to cover most use cases. Assumptions: -gpio-size is the same for all gpio-controllers referenced in a map -optional properties gpio-map-mask and gpio-map-pass-thru are supposed to be omitted The understanding of this last assumption is that flags provided in the expansion device node will overwrite the connector flags. In a latter stage, when need happen, these limitations can be revisited to unlock fully fledged gpio-map usage. Fixes zephyrproject-rtos#15637 Signed-off-by: Erwan Gouriou <[email protected]>

ulfalizer · 2019-05-07T14:11:07Z

scripts/dts/extract/globals.py

+    gpio_map = reduced[cell_parent]['props']['gpio-map']
+
+    # Before parsing, we need to know 'gpio-map' row size
+    # gpio-map raws are encoded as follows:


ulfalizer

Code nits fixed (except one raws -> rows typo).

Not an expert on how this is actually used, so someone else also ought to check.

Thanks for putting up with the nitting.

ulfalizer · 2019-05-07T14:14:36Z

scripts/dts/extract/globals.py


    return ret_array

+def child_to_parent_unmap(cell_parent, gpio_index):


I felt child_to_parent was not enough specific.
map_extract?

gpio_map_index_to_controller or something maybe (wonder if _index_ could be _id_ too).

Maybe cell_parent could just be called node_path, since it's not important to the function itself that it's a parent of something that you're passing in.

Just random ideas though.

gpio_map_child_to_parent ?

erwango · 2019-05-07T14:24:42Z

Thanks for putting up with the nitting.

Thanks for the in deep review

erwango · 2019-05-07T14:26:48Z

@mike-scott, updated https://github.com/erwango/zephyr/tree/test_14057_gpio_map with latest changes

mike-scott · 2019-05-07T14:40:37Z

@mike-scott, updated https://github.com/erwango/zephyr/tree/test_14057_gpio_map with latest changes

Thanks! I'll try again in a bit.

mike-scott · 2019-05-07T17:17:08Z

@erwango nice job, newest change fixes the pin assignment.

mike-scott · 2019-05-07T17:21:14Z

@MaureenHelm Can you take a look as I believe MCR20A and possibly other NXP boards have the potential to be used as shields including GPIO pin header assignments.

MaureenHelm · 2019-05-07T18:55:26Z

@MaureenHelm Can you take a look as I believe MCR20A and possibly other NXP boards have the potential to be used as shields including GPIO pin header assignments.

Yes, MCR20A needs this feature to be converted to a shield (#7462). There is also an NXP sensor shield (#10198). I haven't studied this PR in detail yet, but at first glance it looks great.

mike-scott

LGTM

erwango added area: Devicetree DNM This PR should not be merged (Do Not Merge) area: Shields Shields (add-on boards) labels May 3, 2019

erwango requested review from galak, mike-scott and ulfalizer May 3, 2019 14:48

erwango force-pushed the dev_gpio_map branch from c40dcce to 48028e6 Compare May 6, 2019 11:19

ulfalizer suggested changes May 6, 2019

View reviewed changes

erwango force-pushed the dev_gpio_map branch from 48028e6 to 953a6d2 Compare May 6, 2019 12:45

erwango requested a review from ulfalizer May 6, 2019 12:46

erwango removed the DNM This PR should not be merged (Do Not Merge) label May 6, 2019

ulfalizer suggested changes May 6, 2019

View reviewed changes

erwango force-pushed the dev_gpio_map branch from 953a6d2 to 80713c9 Compare May 6, 2019 15:28

erwango requested a review from ulfalizer May 6, 2019 15:29

ulfalizer suggested changes May 6, 2019

View reviewed changes

erwango force-pushed the dev_gpio_map branch from 80713c9 to 4779843 Compare May 7, 2019 09:56

erwango requested a review from ulfalizer May 7, 2019 09:58

ulfalizer suggested changes May 7, 2019

View reviewed changes

erwango force-pushed the dev_gpio_map branch from 4779843 to d4ed02d Compare May 7, 2019 12:02

erwango requested a review from ulfalizer May 7, 2019 12:03

ulfalizer suggested changes May 7, 2019

View reviewed changes

erwango force-pushed the dev_gpio_map branch from d4ed02d to 65592ca Compare May 7, 2019 13:19

ulfalizer reviewed May 7, 2019

View reviewed changes

erwango force-pushed the dev_gpio_map branch from 65592ca to 5b3a904 Compare May 7, 2019 13:41

erwango requested a review from ulfalizer May 7, 2019 13:42

ulfalizer suggested changes May 7, 2019

View reviewed changes

erwango force-pushed the dev_gpio_map branch from 5b3a904 to 00391dd Compare May 7, 2019 14:08

erwango requested a review from ulfalizer May 7, 2019 14:08

ulfalizer reviewed May 7, 2019

View reviewed changes

ulfalizer approved these changes May 7, 2019

View reviewed changes

ulfalizer reviewed May 7, 2019

View reviewed changes

mike-scott approved these changes May 7, 2019

View reviewed changes

nashif merged commit 1e314df into zephyrproject-rtos:master May 8, 2019

erwango mentioned this pull request May 23, 2019

Standard devicetree connectors for boards #15598

Closed

erwango deleted the dev_gpio_map branch September 3, 2019 12:04


		return ret_array

		def child_to_parent_unmap(cell_parent, gpio_index):

scripts/dts: globals: parse gpio-map nodes #15861

scripts/dts: globals: parse gpio-map nodes #15861

Uh oh!

Conversation

erwango commented May 3, 2019

Uh oh!

erwango commented May 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ulfalizer May 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erwango May 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erwango commented May 6, 2019

Uh oh!

ulfalizer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erwango May 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mike-scott commented May 6, 2019

Uh oh!

mike-scott commented May 6, 2019

Uh oh!

erwango commented May 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ulfalizer May 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mike-scott commented May 7, 2019

Uh oh!

erwango commented May 7, 2019

Uh oh!

erwango commented May 7, 2019

Uh oh!

Choose a reason for hiding this comment

erwango commented May 3, 2019 •

edited

Loading

ulfalizer May 6, 2019 •

edited

Loading

erwango May 6, 2019 •

edited

Loading

erwango May 6, 2019 •

edited

Loading

erwango commented May 6, 2019 •

edited

Loading

ulfalizer May 6, 2019 •

edited

Loading

erwango May 7, 2019 •

edited

Loading