fdt: fix address cell count checking in fdt_translate_address()

PATCH New Przemyslaw Marczak 5 participants 14 messages Oct 28, 2015 – Jan 12, 2016

lore

Share & tools

lore

Przemyslaw Marczak

author

2015-10-28 14:37 UTC #0 lore raw

PATCH 1/1 fdt: fix address cell count checking in fdt_translate_address()

Commit: dm: core: Enable optional use of fdt_translate_address()

Enables use of this function as default, but after this it's not possible to get dev address for the case in which: '#size-cells == 0'

This causes errors when getting address for some GPIOs, for which the '#size-cells' is set to 0.

Example error:
'__of_translate_address: Bad cell count for gpx0'

Allowing for that case by modifying the macro 'OF_CHECK_COUNTS', (called from )__of_translate_address(), fixes the issue.

Now, this macro doesn't check, that '#size-cells' is greater than 0.

This is possible from the specification point of view, but I'm not sure that it doesn't introduce a regression for other configs.

Please test and share the results.

Tested-on: Odroid U3, Odroid X2, Odroid XU3, Sandbox.

Signed-off-by Przemyslaw Marczak <[email protected]>

Cc Masahiro Yamada <[email protected]>

Cc Lukasz Majewski <[email protected]>

Cc Jaehoon Chung <[email protected]>

Cc Stefan Roese <[email protected]>

Cc Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

Cc Marek Vasut <[email protected]>

---
 common/fdt_support.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

common/fdt_support.c +3 -4

index f86365e..5f808cc 100644
@@ -946,8 +946,7 @@ void fdt_del_node_and_alias(void *blob, const char *alias)
 /* Max address size we deal with */
 #define OF_MAX_ADDR_CELLS	4
 #define OF_BAD_ADDR	((u64)-1)
-#define OF_CHECK_COUNTS(na, ns)	((na) > 0 && (na) <= OF_MAX_ADDR_CELLS && \
-			(ns) > 0)
+#define OF_CHECK_COUNTS(na)	((na) > 0 && (na) <= OF_MAX_ADDR_CELLS)
 
 /* Debug utility */
 #ifdef DEBUG

@@ -1115,7 +1114,7 @@ static u64 __of_translate_address(void *blob, int node_offset, const fdt32_t *in
 
 	/* Cound address cells & copy address locally */
 	bus->count_cells(blob, parent, &na, &ns);
-	if (!OF_CHECK_COUNTS(na, ns)) {
+	if (!OF_CHECK_COUNTS(na)) {
 		printf("%s: Bad cell count for %s\n", __FUNCTION__,
 		       fdt_get_name(blob, node_offset, NULL));
 		goto bail;

@@ -1142,7 +1141,7 @@ static u64 __of_translate_address(void *blob, int node_offset, const fdt32_t *in
 		/* Get new parent bus and counts */
 		pbus = &of_busses[0];
 		pbus->count_cells(blob, parent, &pna, &pns);
-		if (!OF_CHECK_COUNTS(pna, pns)) {
+		if (!OF_CHECK_COUNTS(pna)) {
 			printf("%s: Bad cell count for %s\n", __FUNCTION__,
 				fdt_get_name(blob, node_offset, NULL));
 			break;

Signature · 1 line

1.9.1

Jaehoon Chung

2015-10-29 07:28 UTC #1 lore raw

PATCH 1/1 fdt: fix address cell count checking in fdt_translate_address()

Hi, Przemyslaw.

Quoted 14 lines · Przemyslaw Marczak

Commit: dm: core: Enable optional use of fdt_translate_address()
Enables use of this function as default, but after this it's not possible to get dev address for the case in which: '#size-cells == 0'
This causes errors when getting address for some GPIOs, for which the '#size-cells' is set to 0.
Example error:
'__of_translate_address: Bad cell count for gpx0'
Allowing for that case by modifying the macro 'OF_CHECK_COUNTS', (called from )__of_translate_address(), fixes the issue.

Quoted 8 lines

Now, this macro doesn't check, that '#size-cells' is greater than 0.
This is possible from the specification point of view, but I'm not sure that it doesn't introduce a regression for other configs.
Please test and share the results.
Tested-on: Odroid U3, Odroid X2, Odroid XU3, Sandbox.

I have also tested with exynos boards.
As you mentioned, it seems that needs to test for other SoC.

Tested-by Jaehoon Chung <[email protected]>

Best Regards,
Jaehoon Chung

Quoted 46 lines

Signed-off-by Przemyslaw Marczak <[email protected]>

Cc Masahiro Yamada <[email protected]>

Cc Lukasz Majewski <[email protected]>

Cc Jaehoon Chung <[email protected]>

Cc Stefan Roese <[email protected]>

Cc Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

Cc Marek Vasut <[email protected]>

---
 common/fdt_support.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

common/fdt_support.c +3 -4

index f86365e..5f808cc 100644
@@ -946,8 +946,7 @@ void fdt_del_node_and_alias(void *blob, const char *alias)
 /* Max address size we deal with */
 #define OF_MAX_ADDR_CELLS	4
 #define OF_BAD_ADDR	((u64)-1)
-#define OF_CHECK_COUNTS(na, ns)	((na) > 0 && (na) <= OF_MAX_ADDR_CELLS && \
-			(ns) > 0)
+#define OF_CHECK_COUNTS(na)	((na) > 0 && (na) <= OF_MAX_ADDR_CELLS)
 
 /* Debug utility */
 #ifdef DEBUG

@@ -1115,7 +1114,7 @@ static u64 __of_translate_address(void *blob, int node_offset, const fdt32_t *in
 
 	/* Cound address cells & copy address locally */
 	bus->count_cells(blob, parent, &na, &ns);
-	if (!OF_CHECK_COUNTS(na, ns)) {
+	if (!OF_CHECK_COUNTS(na)) {
 		printf("%s: Bad cell count for %s\n", __FUNCTION__,
 		       fdt_get_name(blob, node_offset, NULL));
 		goto bail;

@@ -1142,7 +1141,7 @@ static u64 __of_translate_address(void *blob, int node_offset, const fdt32_t *in
 		/* Get new parent bus and counts */
 		pbus = &of_busses[0];
 		pbus->count_cells(blob, parent, &pna, &pns);
-		if (!OF_CHECK_COUNTS(pna, pns)) {
+		if (!OF_CHECK_COUNTS(pna)) {
 			printf("%s: Bad cell count for %s\n", __FUNCTION__,
 				fdt_get_name(blob, node_offset, NULL));
 			break;

Lukasz Majewski

2015-10-29 17:12 UTC #2 lore raw

PATCH 1/1 fdt: fix address cell count checking in fdt_translate_address()

Hi Jaehoon,

Quoted 75 lines

Hi, Przemyslaw.
Quoted 15 lines · Przemyslaw Marczak
Commit: dm: core: Enable optional use of fdt_translate_address()
Enables use of this function as default, but after this it's not possible to get dev address for the case in which: '#size-cells == 0'
This causes errors when getting address for some GPIOs, for which the '#size-cells' is set to 0.
Example error:
'__of_translate_address: Bad cell count for gpx0'
Allowing for that case by modifying the macro 'OF_CHECK_COUNTS', (called from )__of_translate_address(), fixes the issue.
Quoted 8 lines
Now, this macro doesn't check, that '#size-cells' is greater than 0.
This is possible from the specification point of view, but I'm not sure that it doesn't introduce a regression for other configs.
Please test and share the results.
Tested-on: Odroid U3, Odroid X2, Odroid XU3, Sandbox.
I have also tested with exynos boards.
As you mentioned, it seems that needs to test for other SoC.
Tested-by Jaehoon Chung <[email protected]>
Best Regards,
Jaehoon Chung
Quoted 39 lines
Signed-off-by Przemyslaw Marczak <[email protected]>
Cc Masahiro Yamada <[email protected]>
Cc Lukasz Majewski <[email protected]>
Cc Jaehoon Chung <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Simon Glass <[email protected]>
Cc Bin Meng <[email protected]>
Cc Marek Vasut <[email protected]>
---
 common/fdt_support.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
common/fdt_support.c +0 -0
index f86365e..5f808cc 100644
@@ -946,8 +946,7 @@ void fdt_del_node_and_alias(void *blob, const
char *alias) /* Max address size we deal with */
#define OF_MAX_ADDR_CELLS 4
#define OF_BAD_ADDR ((u64)-1)
-#define OF_CHECK_COUNTS(na, ns) ((na) > 0 && (na) <=
OF_MAX_ADDR_CELLS && \
common/fdt_support.c +1 -1
-			(ns) > 0)
+#define OF_CHECK_COUNTS(na)	((na) > 0 && (na) <=
OF_MAX_ADDR_CELLS)
/* Debug utility */
#ifdef DEBUG
common/fdt_support.c +0 -0
@@ -1115,7 +1114,7 @@ static u64 __of_translate_address(void *blob,
int node_offset, const fdt32_t *in
common/fdt_support.c +1 -1
 	/* Cound address cells & copy address locally */
 	bus->count_cells(blob, parent, &na, &ns);
-	if (!OF_CHECK_COUNTS(na, ns)) {
+	if (!OF_CHECK_COUNTS(na)) {
 		printf("%s: Bad cell count for %s\n", __FUNCTION__,
 		       fdt_get_name(blob, node_offset, NULL));
 		goto bail;
@@ -1142,7 +1141,7 @@ static u64 __of_translate_address(void *blob,
int node_offset, const fdt32_t *in /* Get new parent bus and counts

Quoted 9 lines

Quoted 8 lines
*/ pbus = &of_busses[0];
common/fdt_support.c +1 -1
 		pbus->count_cells(blob, parent, &pna, &pns);
-		if (!OF_CHECK_COUNTS(pna, pns)) {
+		if (!OF_CHECK_COUNTS(pna)) {
 			printf("%s: Bad cell count for %s\n",
__FUNCTION__, fdt_get_name(blob, node_offset, NULL));
break;

Tested-by Lukasz Majewski <[email protected]>

Test HW: Exynos 4210 - Trats board.

Signature · 5 lines

Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Simon Glass

2015-10-29 17:15 UTC #3 lore raw

PATCH 1/1 fdt: fix address cell count checking in fdt_translate_address()

In reply to Przemyslaw Marczak

Hi Stefan,

Quoted 50 lines · Przemyslaw Marczak

Commit: dm: core: Enable optional use of fdt_translate_address()
Enables use of this function as default, but after this it's not possible to get dev address for the case in which: '#size-cells == 0'
This causes errors when getting address for some GPIOs, for which the '#size-cells' is set to 0.
Example error:
'__of_translate_address: Bad cell count for gpx0'
Allowing for that case by modifying the macro 'OF_CHECK_COUNTS', (called from )__of_translate_address(), fixes the issue.
Now, this macro doesn't check, that '#size-cells' is greater than 0.
This is possible from the specification point of view, but I'm not sure that it doesn't introduce a regression for other configs.
Please test and share the results.
Tested-on: Odroid U3, Odroid X2, Odroid XU3, Sandbox.
Signed-off-by Przemyslaw Marczak <[email protected]>
Cc Masahiro Yamada <[email protected]>
Cc Lukasz Majewski <[email protected]>
Cc Jaehoon Chung <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Simon Glass <[email protected]>
Cc Bin Meng <[email protected]>
Cc Marek Vasut <[email protected]>
---
 common/fdt_support.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
common/fdt_support.c +1 -2
index f86365e..5f808cc 100644
@@ -946,8 +946,7 @@ void fdt_del_node_and_alias(void *blob, const char *alias)
 /* Max address size we deal with */
 #define OF_MAX_ADDR_CELLS      4
 #define OF_BAD_ADDR    ((u64)-1)
-#define OF_CHECK_COUNTS(na, ns)        ((na) > 0 && (na) <= OF_MAX_ADDR_CELLS && \
-                       (ns) > 0)
+#define OF_CHECK_COUNTS(na)    ((na) > 0 && (na) <= OF_MAX_ADDR_CELLS)
 
 /* Debug utility */
 #ifdef DEBUG

common/fdt_support.c static u64 __of_translate_address(void *blob, int node_offset, const fdt32_t *in · line 1117

Quoted 5 lines

common/fdt_support.c +1 -1

@@ -1115,7 +1114,7 @@ static u64 __of_translate_address(void *blob, int node_offset, const fdt32_t *in
 
        /* Cound address cells & copy address locally */
        bus->count_cells(blob, parent, &na, &ns);
-       if (!OF_CHECK_COUNTS(na, ns)) {
+       if (!OF_CHECK_COUNTS(na)) {

This seems to conflict with the comment at the top of this function:

* Note: We consider that crossing any level with #size-cells == 0 to mean
* that translation is impossible (that is we are not dealing with a value
* that can be mapped to a cpu physical address). This is not really specified
* that way, but this is traditionally the way IBM at least do things

What should we do here?

Quoted 13 lines

common/fdt_support.c +1 -2

                printf("%s: Bad cell count for %s\n", __FUNCTION__,
                       fdt_get_name(blob, node_offset, NULL));
                goto bail;

@@ -1142,7 +1141,7 @@ static u64 __of_translate_address(void *blob, int node_offset, const fdt32_t *in
                /* Get new parent bus and counts */
                pbus = &of_busses[0];
                pbus->count_cells(blob, parent, &pna, &pns);
-               if (!OF_CHECK_COUNTS(pna, pns)) {
+               if (!OF_CHECK_COUNTS(pna)) {
                        printf("%s: Bad cell count for %s\n", __FUNCTION__,
                                fdt_get_name(blob, node_offset, NULL));
                        break;
--

Quoted 2 lines

1.9.1

Regards,
Simon

Przemyslaw Marczak

author

2015-11-03 09:57 UTC #4 lore raw

PATCH 1/1 fdt: fix address cell count checking in fdt_translate_address()

Hello All,

Quoted 61 lines · Simon Glass

Hi Stefan,
Quoted 50 lines · Przemyslaw Marczak
Commit: dm: core: Enable optional use of fdt_translate_address()
Enables use of this function as default, but after this it's not possible to get dev address for the case in which: '#size-cells == 0'
This causes errors when getting address for some GPIOs, for which the '#size-cells' is set to 0.
Example error:
'__of_translate_address: Bad cell count for gpx0'
Allowing for that case by modifying the macro 'OF_CHECK_COUNTS', (called from )__of_translate_address(), fixes the issue.
Now, this macro doesn't check, that '#size-cells' is greater than 0.
This is possible from the specification point of view, but I'm not sure that it doesn't introduce a regression for other configs.
Please test and share the results.
Tested-on: Odroid U3, Odroid X2, Odroid XU3, Sandbox.
Signed-off-by Przemyslaw Marczak <[email protected]>
Cc Masahiro Yamada <[email protected]>
Cc Lukasz Majewski <[email protected]>
Cc Jaehoon Chung <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Simon Glass <[email protected]>
Cc Bin Meng <[email protected]>
Cc Marek Vasut <[email protected]>
---
  common/fdt_support.c | 7 +++----
  1 file changed, 3 insertions(+), 4 deletions(-)
common/fdt_support.c +1 -2
index f86365e..5f808cc 100644
@@ -946,8 +946,7 @@ void fdt_del_node_and_alias(void *blob, const char *alias)
  /* Max address size we deal with */
  #define OF_MAX_ADDR_CELLS      4
  #define OF_BAD_ADDR    ((u64)-1)
-#define OF_CHECK_COUNTS(na, ns)        ((na) > 0 && (na) <= OF_MAX_ADDR_CELLS && \
-                       (ns) > 0)
+#define OF_CHECK_COUNTS(na)    ((na) > 0 && (na) <= OF_MAX_ADDR_CELLS)
 
  /* Debug utility */
  #ifdef DEBUG
common/fdt_support.c static u64 __of_translate_address(void *blob, int node_offset, const fdt32_t *in · line 1117
Quoted 5 lines
common/fdt_support.c +1 -1
@@ -1115,7 +1114,7 @@ static u64 __of_translate_address(void *blob, int node_offset, const fdt32_t *in
 
         /* Cound address cells & copy address locally */
         bus->count_cells(blob, parent, &na, &ns);
-       if (!OF_CHECK_COUNTS(na, ns)) {
+       if (!OF_CHECK_COUNTS(na)) {
This seems to conflict with the comment at the top of this function:

Quoted 7 lines

* Note: We consider that crossing any level with #size-cells == 0 to mean
* that translation is impossible (that is we are not dealing with a value
* that can be mapped to a cpu physical address). This is not really specified
* that way, but this is traditionally the way IBM at least do things
What should we do here?

Is that commit acceptable? I would like send V2 with removing the above comment.

Best regards,

Signature · 4 lines

Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

Quoted 19 lines

Quoted 13 lines

common/fdt_support.c +1 -2

                 printf("%s: Bad cell count for %s\n", __FUNCTION__,
                        fdt_get_name(blob, node_offset, NULL));
                 goto bail;

@@ -1142,7 +1141,7 @@ static u64 __of_translate_address(void *blob, int node_offset, const fdt32_t *in
                 /* Get new parent bus and counts */
                 pbus = &of_busses[0];
                 pbus->count_cells(blob, parent, &pna, &pns);
-               if (!OF_CHECK_COUNTS(pna, pns)) {
+               if (!OF_CHECK_COUNTS(pna)) {
                         printf("%s: Bad cell count for %s\n", __FUNCTION__,
                                 fdt_get_name(blob, node_offset, NULL));
                         break;
--

Quoted 2 lines

1.9.1

Regards,
Simon

Simon Glass

2015-11-06 03:16 UTC #5 lore raw

PATCH 1/1 fdt: fix address cell count checking in fdt_translate_address()

Hi,

Quoted 80 lines · Przemyslaw Marczak

Hello All,
Quoted 76 lines · Simon Glass
Hi Stefan,
Quoted 57 lines · Przemyslaw Marczak
Commit: dm: core: Enable optional use of fdt_translate_address()
Enables use of this function as default, but after this it's not possible to get dev address for the case in which: '#size-cells == 0'
This causes errors when getting address for some GPIOs, for which the '#size-cells' is set to 0.
Example error:
'__of_translate_address: Bad cell count for gpx0'
Allowing for that case by modifying the macro 'OF_CHECK_COUNTS', (called from )__of_translate_address(), fixes the issue.
Now, this macro doesn't check, that '#size-cells' is greater than 0.
This is possible from the specification point of view, but I'm not sure that it doesn't introduce a regression for other configs.
Please test and share the results.
Tested-on: Odroid U3, Odroid X2, Odroid XU3, Sandbox.
Signed-off-by Przemyslaw Marczak <[email protected]>
Cc Masahiro Yamada <[email protected]>
Cc Lukasz Majewski <[email protected]>
Cc Jaehoon Chung <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Simon Glass <[email protected]>
Cc Bin Meng <[email protected]>
Cc Marek Vasut <[email protected]>
---
  common/fdt_support.c | 7 +++----
  1 file changed, 3 insertions(+), 4 deletions(-)
common/fdt_support.c +1 -2
index f86365e..5f808cc 100644
@@ -946,8 +946,7 @@ void fdt_del_node_and_alias(void *blob, const char
 *alias)
  /* Max address size we deal with */
  #define OF_MAX_ADDR_CELLS      4
  #define OF_BAD_ADDR    ((u64)-1)
-#define OF_CHECK_COUNTS(na, ns)        ((na) > 0 && (na) <=
 OF_MAX_ADDR_CELLS && \
-                       (ns) > 0)
+#define OF_CHECK_COUNTS(na)    ((na) > 0 && (na) <= OF_MAX_ADDR_CELLS)
 
  /* Debug utility */
  #ifdef DEBUG
common/fdt_support.c +0 -0
@@ -1115,7 +1114,7 @@ static u64 __of_translate_address(void *blob, int
node_offset, const fdt32_t *in
         /* Cound address cells & copy address locally */
         bus->count_cells(blob, parent, &na, &ns);
common/fdt_support.c static u64 __of_translate_address(void *blob, int node_offset, const fdt32_t *in · line 1117
Quoted 2 lines
common/fdt_support.c +1 -1
-       if (!OF_CHECK_COUNTS(na, ns)) {
+       if (!OF_CHECK_COUNTS(na)) {
This seems to conflict with the comment at the top of this function:
* Note: We consider that crossing any level with #size-cells == 0 to
mean
* that translation is impossible (that is we are not dealing with a
value
* that can be mapped to a cpu physical address). This is not really
specified
* that way, but this is traditionally the way IBM at least do things

Quoted 5 lines

Quoted 2 lines
What should we do here?
Is that commit acceptable? I would like send V2 with removing the above comment.

That's what I am worried about. Presumably the comment is accurate today and this check has some value. I was hoping Stefan might know.

Quoted 31 lines

Best regards,
--
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

Quoted 21 lines

Quoted 12 lines

                 printf("%s: Bad cell count for %s\n", __FUNCTION__,
                        fdt_get_name(blob, node_offset, NULL));
                 goto bail;

common/fdt_support.c +0 -0

@@ -1142,7 +1141,7 @@ static u64 __of_translate_address(void *blob, int

node_offset, const fdt32_t *in

                 /* Get new parent bus and counts */
                 pbus = &of_busses[0];
                 pbus->count_cells(blob, parent, &pna, &pns);

common/fdt_support.c +1 -1

-               if (!OF_CHECK_COUNTS(pna, pns)) {
+               if (!OF_CHECK_COUNTS(pna)) {
                         printf("%s: Bad cell count for %s\n",

__FUNCTION__,

Quoted 5 lines

                                 fdt_get_name(blob, node_offset, NULL));
                         break;

--
1.9.1

Regards,
Simon

Stefan Roese

2015-11-06 06:47 UTC #6 lore raw

PATCH 1/1 fdt: fix address cell count checking in fdt_translate_address()

Quoted 83 lines · Simon Glass

Hi,
Quoted 80 lines · Przemyslaw Marczak
Hello All,
Quoted 76 lines · Simon Glass
Hi Stefan,
Quoted 57 lines · Przemyslaw Marczak
Commit: dm: core: Enable optional use of fdt_translate_address()
Enables use of this function as default, but after this it's not possible to get dev address for the case in which: '#size-cells == 0'
This causes errors when getting address for some GPIOs, for which the '#size-cells' is set to 0.
Example error:
'__of_translate_address: Bad cell count for gpx0'
Allowing for that case by modifying the macro 'OF_CHECK_COUNTS', (called from )__of_translate_address(), fixes the issue.
Now, this macro doesn't check, that '#size-cells' is greater than 0.
This is possible from the specification point of view, but I'm not sure that it doesn't introduce a regression for other configs.
Please test and share the results.
Tested-on: Odroid U3, Odroid X2, Odroid XU3, Sandbox.
Signed-off-by Przemyslaw Marczak <[email protected]>
Cc Masahiro Yamada <[email protected]>
Cc Lukasz Majewski <[email protected]>
Cc Jaehoon Chung <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Simon Glass <[email protected]>
Cc Bin Meng <[email protected]>
Cc Marek Vasut <[email protected]>
---
   common/fdt_support.c | 7 +++----
   1 file changed, 3 insertions(+), 4 deletions(-)
common/fdt_support.c +1 -2
index f86365e..5f808cc 100644
@@ -946,8 +946,7 @@ void fdt_del_node_and_alias(void *blob, const char
 *alias)
   /* Max address size we deal with */
   #define OF_MAX_ADDR_CELLS      4
   #define OF_BAD_ADDR    ((u64)-1)
-#define OF_CHECK_COUNTS(na, ns)        ((na) > 0 && (na) <=
 OF_MAX_ADDR_CELLS && \
-                       (ns) > 0)
+#define OF_CHECK_COUNTS(na)    ((na) > 0 && (na) <= OF_MAX_ADDR_CELLS)
 
   /* Debug utility */
   #ifdef DEBUG
common/fdt_support.c +0 -0
@@ -1115,7 +1114,7 @@ static u64 __of_translate_address(void *blob, int
node_offset, const fdt32_t *in
          /* Cound address cells & copy address locally */
          bus->count_cells(blob, parent, &na, &ns);
common/fdt_support.c static u64 __of_translate_address(void *blob, int node_offset, const fdt32_t *in · line 1117
Quoted 2 lines
common/fdt_support.c +1 -1
-       if (!OF_CHECK_COUNTS(na, ns)) {
+       if (!OF_CHECK_COUNTS(na)) {
This seems to conflict with the comment at the top of this function:
* Note: We consider that crossing any level with #size-cells == 0 to
mean
* that translation is impossible (that is we are not dealing with a
value
* that can be mapped to a cpu physical address). This is not really
specified
* that way, but this is traditionally the way IBM at least do things

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
What should we do here?
Is that commit acceptable? I would like send V2 with removing the above comment.
That's what I am worried about. Presumably the comment is accurate today and this check has some value. I was hoping Stefan might know.

Unfortunately no. I just stumbled over this problem with the translation of the "complex" ranges on the MVEBU platform. And noticed that we already have this functionality to translate the addresses the "right way".

I'm wondering how this problem with those GPIOs is handled in the kernel? I assume that it is working correctly there, right? Przemyslaw, could you perhaps check this and see, why its working there? And change / fix it in U-Boot accordingly?

Thanks,
Stefan

Simon Glass

2016-01-05 00:59 UTC #7 lore raw

PATCH 1/1 fdt: fix address cell count checking in fdt_translate_address()

Hi Przemyslaw,

Quoted 106 lines · Stefan Roese

Quoted 92 lines · Simon Glass
Hi,
Quoted 87 lines · Przemyslaw Marczak
Hello All,
Quoted 81 lines · Simon Glass
Hi Stefan,
Quoted 58 lines · Przemyslaw Marczak
Commit: dm: core: Enable optional use of fdt_translate_address()
Enables use of this function as default, but after this it's not possible to get dev address for the case in which: '#size-cells == 0'
This causes errors when getting address for some GPIOs, for which the '#size-cells' is set to 0.
Example error:
'__of_translate_address: Bad cell count for gpx0'
Allowing for that case by modifying the macro 'OF_CHECK_COUNTS', (called from )__of_translate_address(), fixes the issue.
Now, this macro doesn't check, that '#size-cells' is greater than 0.
This is possible from the specification point of view, but I'm not sure that it doesn't introduce a regression for other configs.
Please test and share the results.
Tested-on: Odroid U3, Odroid X2, Odroid XU3, Sandbox.
Signed-off-by Przemyslaw Marczak <[email protected]>
Cc Masahiro Yamada <[email protected]>
Cc Lukasz Majewski <[email protected]>
Cc Jaehoon Chung <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Simon Glass <[email protected]>
Cc Bin Meng <[email protected]>
Cc Marek Vasut <[email protected]>
---
   common/fdt_support.c | 7 +++----
   1 file changed, 3 insertions(+), 4 deletions(-)
common/fdt_support.c +1 -2
index f86365e..5f808cc 100644
@@ -946,8 +946,7 @@ void fdt_del_node_and_alias(void *blob, const char
 *alias)
   /* Max address size we deal with */
   #define OF_MAX_ADDR_CELLS      4
   #define OF_BAD_ADDR    ((u64)-1)
-#define OF_CHECK_COUNTS(na, ns)        ((na) > 0 && (na) <=
 OF_MAX_ADDR_CELLS && \
-                       (ns) > 0)
+#define OF_CHECK_COUNTS(na)    ((na) > 0 && (na) <= OF_MAX_ADDR_CELLS)
 
   /* Debug utility */
   #ifdef DEBUG
common/fdt_support.c +0 -0
@@ -1115,7 +1114,7 @@ static u64 __of_translate_address(void *blob, int
node_offset, const fdt32_t *in
          /* Cound address cells & copy address locally */
          bus->count_cells(blob, parent, &na, &ns);
common/fdt_support.c static u64 __of_translate_address(void *blob, int node_offset, const fdt32_t *in · line 1117
Quoted 2 lines
common/fdt_support.c +1 -1
-       if (!OF_CHECK_COUNTS(na, ns)) {
+       if (!OF_CHECK_COUNTS(na)) {
This seems to conflict with the comment at the top of this function:
* Note: We consider that crossing any level with #size-cells == 0 to
mean
* that translation is impossible (that is we are not dealing with a
value
* that can be mapped to a cpu physical address). This is not really
specified
* that way, but this is traditionally the way IBM at least do things
What should we do here?
Quoted 6 lines
Quoted 2 lines
Is that commit acceptable? I would like send V2 with removing the above comment.
That's what I am worried about. Presumably the comment is accurate today and this check has some value. I was hoping Stefan might know.
Unfortunately no. I just stumbled over this problem with the translation of the "complex" ranges on the MVEBU platform. And noticed that we already have this functionality to translate the addresses the "right way".

Quoted 4 lines

I'm wondering how this problem with those GPIOs is handled in the kernel? I assume that it is working correctly there, right? Przemyslaw, could you perhaps check this and see, why its working there? And change / fix it in U-Boot accordingly?

Let's pick up this patch for now as a bug-fix. We can deal with this problem after the release.

Regards,
Simon

Simon Glass

2016-01-07 19:24 UTC #8 lore raw

PATCH 1/1 fdt: fix address cell count checking in fdt_translate_address()

+Stephen

Quoted 109 lines · Simon Glass

Hi Przemyslaw,
Quoted 106 lines · Stefan Roese
Quoted 92 lines · Simon Glass
Hi,
Quoted 87 lines · Przemyslaw Marczak
Hello All,
Quoted 81 lines · Simon Glass
Hi Stefan,
Quoted 58 lines · Przemyslaw Marczak
Commit: dm: core: Enable optional use of fdt_translate_address()
Enables use of this function as default, but after this it's not possible to get dev address for the case in which: '#size-cells == 0'
This causes errors when getting address for some GPIOs, for which the '#size-cells' is set to 0.
Example error:
'__of_translate_address: Bad cell count for gpx0'
Allowing for that case by modifying the macro 'OF_CHECK_COUNTS', (called from )__of_translate_address(), fixes the issue.
Now, this macro doesn't check, that '#size-cells' is greater than 0.
This is possible from the specification point of view, but I'm not sure that it doesn't introduce a regression for other configs.
Please test and share the results.
Tested-on: Odroid U3, Odroid X2, Odroid XU3, Sandbox.
Signed-off-by Przemyslaw Marczak <[email protected]>
Cc Masahiro Yamada <[email protected]>
Cc Lukasz Majewski <[email protected]>
Cc Jaehoon Chung <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Simon Glass <[email protected]>
Cc Bin Meng <[email protected]>
Cc Marek Vasut <[email protected]>
---
   common/fdt_support.c | 7 +++----
   1 file changed, 3 insertions(+), 4 deletions(-)
common/fdt_support.c +1 -2
index f86365e..5f808cc 100644
@@ -946,8 +946,7 @@ void fdt_del_node_and_alias(void *blob, const char
 *alias)
   /* Max address size we deal with */
   #define OF_MAX_ADDR_CELLS      4
   #define OF_BAD_ADDR    ((u64)-1)
-#define OF_CHECK_COUNTS(na, ns)        ((na) > 0 && (na) <=
 OF_MAX_ADDR_CELLS && \
-                       (ns) > 0)
+#define OF_CHECK_COUNTS(na)    ((na) > 0 && (na) <= OF_MAX_ADDR_CELLS)
 
   /* Debug utility */
   #ifdef DEBUG
common/fdt_support.c +0 -0
@@ -1115,7 +1114,7 @@ static u64 __of_translate_address(void *blob, int
node_offset, const fdt32_t *in
          /* Cound address cells & copy address locally */
          bus->count_cells(blob, parent, &na, &ns);
common/fdt_support.c static u64 __of_translate_address(void *blob, int node_offset, const fdt32_t *in · line 1117
Quoted 2 lines
common/fdt_support.c +1 -1
-       if (!OF_CHECK_COUNTS(na, ns)) {
+       if (!OF_CHECK_COUNTS(na)) {
This seems to conflict with the comment at the top of this function:
* Note: We consider that crossing any level with #size-cells == 0 to
mean
* that translation is impossible (that is we are not dealing with a
value
* that can be mapped to a cpu physical address). This is not really
specified
* that way, but this is traditionally the way IBM at least do things
What should we do here?
Quoted 6 lines
Quoted 2 lines
Is that commit acceptable? I would like send V2 with removing the above comment.
That's what I am worried about. Presumably the comment is accurate today and this check has some value. I was hoping Stefan might know.
Unfortunately no. I just stumbled over this problem with the translation of the "complex" ranges on the MVEBU platform. And noticed that we already have this functionality to translate the addresses the "right way".

Quoted 7 lines

Quoted 4 lines
I'm wondering how this problem with those GPIOs is handled in the kernel? I assume that it is working correctly there, right? Przemyslaw, could you perhaps check this and see, why its working there? And change / fix it in U-Boot accordingly?
Let's pick up this patch for now as a bug-fix. We can deal with this problem after the release.

Applied to u-boot-dm/master.

I'll post a revert after the release. It seems like you and Stephen are making good progress.

- Simon

Przemyslaw Marczak

author

2016-01-08 12:01 UTC #9 lore raw

PATCH 1/1 fdt: fix address cell count checking in fdt_translate_address()

Hello Simon,

Quoted 120 lines · Simon Glass

+Stephen
Quoted 109 lines · Simon Glass
Hi Przemyslaw,
Quoted 106 lines · Stefan Roese
Quoted 92 lines · Simon Glass
Hi,
Quoted 87 lines · Przemyslaw Marczak
Hello All,
Quoted 81 lines · Simon Glass
Hi Stefan,
Quoted 58 lines · Przemyslaw Marczak
Commit: dm: core: Enable optional use of fdt_translate_address()
Enables use of this function as default, but after this it's not possible to get dev address for the case in which: '#size-cells == 0'
This causes errors when getting address for some GPIOs, for which the '#size-cells' is set to 0.
Example error:
'__of_translate_address: Bad cell count for gpx0'
Allowing for that case by modifying the macro 'OF_CHECK_COUNTS', (called from )__of_translate_address(), fixes the issue.
Now, this macro doesn't check, that '#size-cells' is greater than 0.
This is possible from the specification point of view, but I'm not sure that it doesn't introduce a regression for other configs.
Please test and share the results.
Tested-on: Odroid U3, Odroid X2, Odroid XU3, Sandbox.
Signed-off-by Przemyslaw Marczak <[email protected]>
Cc Masahiro Yamada <[email protected]>
Cc Lukasz Majewski <[email protected]>
Cc Jaehoon Chung <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Simon Glass <[email protected]>
Cc Bin Meng <[email protected]>
Cc Marek Vasut <[email protected]>
---
    common/fdt_support.c | 7 +++----
    1 file changed, 3 insertions(+), 4 deletions(-)
common/fdt_support.c +1 -2
index f86365e..5f808cc 100644
@@ -946,8 +946,7 @@ void fdt_del_node_and_alias(void *blob, const char
 *alias)
    /* Max address size we deal with */
    #define OF_MAX_ADDR_CELLS      4
    #define OF_BAD_ADDR    ((u64)-1)
-#define OF_CHECK_COUNTS(na, ns)        ((na) > 0 && (na) <=
 OF_MAX_ADDR_CELLS && \
-                       (ns) > 0)
+#define OF_CHECK_COUNTS(na)    ((na) > 0 && (na) <= OF_MAX_ADDR_CELLS)
 
    /* Debug utility */
    #ifdef DEBUG
common/fdt_support.c +0 -0
@@ -1115,7 +1114,7 @@ static u64 __of_translate_address(void *blob, int
node_offset, const fdt32_t *in
           /* Cound address cells & copy address locally */
           bus->count_cells(blob, parent, &na, &ns);
common/fdt_support.c
Quoted 2 lines
common/fdt_support.c +1 -1
-       if (!OF_CHECK_COUNTS(na, ns)) {
+       if (!OF_CHECK_COUNTS(na)) {
This seems to conflict with the comment at the top of this function:
* Note: We consider that crossing any level with #size-cells == 0 to
mean
* that translation is impossible (that is we are not dealing with a
value
* that can be mapped to a cpu physical address). This is not really
specified
* that way, but this is traditionally the way IBM at least do things
What should we do here?
Quoted 6 lines
Quoted 2 lines
Is that commit acceptable? I would like send V2 with removing the above comment.
That's what I am worried about. Presumably the comment is accurate today and this check has some value. I was hoping Stefan might know.
Unfortunately no. I just stumbled over this problem with the translation of the "complex" ranges on the MVEBU platform. And noticed that we already have this functionality to translate the addresses the "right way".
Quoted 7 lines
Quoted 4 lines
I'm wondering how this problem with those GPIOs is handled in the kernel? I assume that it is working correctly there, right? Przemyslaw, could you perhaps check this and see, why its working there? And change / fix it in U-Boot accordingly?
Let's pick up this patch for now as a bug-fix. We can deal with this problem after the release.

Quoted 8 lines

Applied to u-boot-dm/master.
I'll post a revert after the release. It seems like you and Stephen are making good progress.
- Simon

Why so fast with this one?

I think, that more proper for a temporary fix is my latest patch with #size-cells count checking only if ranges found in the parent node.

I will continue the discussion with Stephen.

Best regards,

Signature · 4 lines

Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

Simon Glass

2016-01-11 16:59 UTC #10 lore raw

PATCH 1/1 fdt: fix address cell count checking in fdt_translate_address()

Hi Przemyslaw,

Quoted 156 lines · Przemyslaw Marczak

Hello Simon,
Quoted 143 lines · Simon Glass
+Stephen
Quoted 129 lines · Simon Glass
Hi Przemyslaw,
Quoted 125 lines · Stefan Roese
Quoted 107 lines · Simon Glass
Hi,
Quoted 101 lines · Przemyslaw Marczak
Hello All,
Quoted 94 lines · Simon Glass
Hi Stefan,
Quoted 64 lines · Przemyslaw Marczak
Commit: dm: core: Enable optional use of fdt_translate_address()
Enables use of this function as default, but after this it's not possible to get dev address for the case in which: '#size-cells == 0'
This causes errors when getting address for some GPIOs, for which the '#size-cells' is set to 0.
Example error:
'__of_translate_address: Bad cell count for gpx0'
Allowing for that case by modifying the macro 'OF_CHECK_COUNTS', (called from )__of_translate_address(), fixes the issue.
Now, this macro doesn't check, that '#size-cells' is greater than 0.
This is possible from the specification point of view, but I'm not sure
that it doesn't introduce a regression for other configs.
Please test and share the results.
Tested-on: Odroid U3, Odroid X2, Odroid XU3, Sandbox.
Signed-off-by Przemyslaw Marczak <[email protected]>
Cc Masahiro Yamada <[email protected]>
Cc Lukasz Majewski <[email protected]>
Cc Jaehoon Chung <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Simon Glass <[email protected]>
Cc Bin Meng <[email protected]>
Cc Marek Vasut <[email protected]>
---
    common/fdt_support.c | 7 +++----
    1 file changed, 3 insertions(+), 4 deletions(-)
common/fdt_support.c +0 -0
index f86365e..5f808cc 100644
@@ -946,8 +946,7 @@ void fdt_del_node_and_alias(void *blob, const
char
*alias)
    /* Max address size we deal with */
    #define OF_MAX_ADDR_CELLS      4
    #define OF_BAD_ADDR    ((u64)-1)
-#define OF_CHECK_COUNTS(na, ns) ((na) > 0 && (na) <=
OF_MAX_ADDR_CELLS && \
common/fdt_support.c +1 -1
-                       (ns) > 0)
+#define OF_CHECK_COUNTS(na)    ((na) > 0 && (na) <=
OF_MAX_ADDR_CELLS)
    /* Debug utility */
    #ifdef DEBUG
common/fdt_support.c +0 -0
@@ -1115,7 +1114,7 @@ static u64 __of_translate_address(void *blob,
int
node_offset, const fdt32_t *in
           /* Cound address cells & copy address locally */
           bus->count_cells(blob, parent, &na, &ns);
common/fdt_support.c
Quoted 2 lines
common/fdt_support.c +1 -1
-       if (!OF_CHECK_COUNTS(na, ns)) {
+       if (!OF_CHECK_COUNTS(na)) {
This seems to conflict with the comment at the top of this function:
* Note: We consider that crossing any level with #size-cells == 0
to
mean
* that translation is impossible (that is we are not dealing with
a
value
* that can be mapped to a cpu physical address). This is not
really
specified
* that way, but this is traditionally the way IBM at least do
things
What should we do here?
Quoted 8 lines
Quoted 3 lines
Is that commit acceptable? I would like send V2 with removing the above
comment.
That's what I am worried about. Presumably the comment is accurate today and this check has some value. I was hoping Stefan might know.
Unfortunately no. I just stumbled over this problem with the translation of the "complex" ranges on the MVEBU platform. And noticed that we already have this functionality to translate the addresses the "right way".
Quoted 8 lines
Quoted 4 lines
I'm wondering how this problem with those GPIOs is handled in the kernel? I assume that it is working correctly there, right? Przemyslaw, could you perhaps check this and see, why its working there? And change / fix it in U-Boot accordingly?
Let's pick up this patch for now as a bug-fix. We can deal with this problem after the release.
Quoted 8 lines
Applied to u-boot-dm/master.
I'll post a revert after the release. It seems like you and Stephen are making good progress.
- Simon

Quoted 6 lines

Why so fast with this one?
I think, that more proper for a temporary fix is my latest patch with #size-cells count checking only if ranges found in the parent node.
I will continue the discussion with Stephen.

The release is scheduled for today, so we had to do something to fix the breakage.

Once you have a full solution figured out we can revert this patch and apply what you come up with.

Regards,
Simon

Przemyslaw Marczak

author

2016-01-12 10:49 UTC #11 lore raw

PATCH 1/1 fdt: fix address cell count checking in fdt_translate_address()

Hello Simon,

Quoted 166 lines · Simon Glass

Hi Przemyslaw,
Quoted 156 lines · Przemyslaw Marczak
Hello Simon,
Quoted 143 lines · Simon Glass
+Stephen
Quoted 129 lines · Simon Glass
Hi Przemyslaw,
Quoted 125 lines · Stefan Roese
Quoted 107 lines · Simon Glass
Hi,
Quoted 101 lines · Przemyslaw Marczak
Hello All,
Quoted 94 lines · Simon Glass
Hi Stefan,
Quoted 64 lines · Przemyslaw Marczak
Commit: dm: core: Enable optional use of fdt_translate_address()
Enables use of this function as default, but after this it's not possible to get dev address for the case in which: '#size-cells == 0'
This causes errors when getting address for some GPIOs, for which the '#size-cells' is set to 0.
Example error:
'__of_translate_address: Bad cell count for gpx0'
Allowing for that case by modifying the macro 'OF_CHECK_COUNTS', (called from )__of_translate_address(), fixes the issue.
Now, this macro doesn't check, that '#size-cells' is greater than 0.
This is possible from the specification point of view, but I'm not sure
that it doesn't introduce a regression for other configs.
Please test and share the results.
Tested-on: Odroid U3, Odroid X2, Odroid XU3, Sandbox.
Signed-off-by Przemyslaw Marczak <[email protected]>
Cc Masahiro Yamada <[email protected]>
Cc Lukasz Majewski <[email protected]>
Cc Jaehoon Chung <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Simon Glass <[email protected]>
Cc Bin Meng <[email protected]>
Cc Marek Vasut <[email protected]>
---
     common/fdt_support.c | 7 +++----
     1 file changed, 3 insertions(+), 4 deletions(-)
common/fdt_support.c +0 -0
index f86365e..5f808cc 100644
@@ -946,8 +946,7 @@ void fdt_del_node_and_alias(void *blob, const
char
*alias)
     /* Max address size we deal with */
     #define OF_MAX_ADDR_CELLS      4
     #define OF_BAD_ADDR    ((u64)-1)
-#define OF_CHECK_COUNTS(na, ns) ((na) > 0 && (na) <=
OF_MAX_ADDR_CELLS && \
common/fdt_support.c +1 -1
-                       (ns) > 0)
+#define OF_CHECK_COUNTS(na)    ((na) > 0 && (na) <=
OF_MAX_ADDR_CELLS)
     /* Debug utility */
     #ifdef DEBUG
common/fdt_support.c +0 -0
@@ -1115,7 +1114,7 @@ static u64 __of_translate_address(void *blob,
int
node_offset, const fdt32_t *in
            /* Cound address cells & copy address locally */
            bus->count_cells(blob, parent, &na, &ns);
common/fdt_support.c
Quoted 2 lines
common/fdt_support.c +1 -1
-       if (!OF_CHECK_COUNTS(na, ns)) {
+       if (!OF_CHECK_COUNTS(na)) {
This seems to conflict with the comment at the top of this function:
* Note: We consider that crossing any level with #size-cells == 0
to
mean
* that translation is impossible (that is we are not dealing with
a
value
* that can be mapped to a cpu physical address). This is not
really
specified
* that way, but this is traditionally the way IBM at least do
things
What should we do here?
Quoted 8 lines
Quoted 3 lines
Is that commit acceptable? I would like send V2 with removing the above
comment.
That's what I am worried about. Presumably the comment is accurate today and this check has some value. I was hoping Stefan might know.
Unfortunately no. I just stumbled over this problem with the translation of the "complex" ranges on the MVEBU platform. And noticed that we already have this functionality to translate the addresses the "right way".
Quoted 8 lines
Quoted 4 lines
I'm wondering how this problem with those GPIOs is handled in the kernel? I assume that it is working correctly there, right? Przemyslaw, could you perhaps check this and see, why its working there? And change / fix it in U-Boot accordingly?
Let's pick up this patch for now as a bug-fix. We can deal with this problem after the release.
Quoted 8 lines
Applied to u-boot-dm/master.
I'll post a revert after the release. It seems like you and Stephen are making good progress.
- Simon
Quoted 6 lines
Why so fast with this one?
I think, that more proper for a temporary fix is my latest patch with #size-cells count checking only if ranges found in the parent node.
I will continue the discussion with Stephen.

Quoted 10 lines

The release is scheduled for today, so we had to do something to fix the breakage.
Once you have a full solution figured out we can revert this patch and apply what you come up with.
Regards,
Simon

Ok. It's hard to convince Stephen to accept such change, so I will send a patch with another solution - just bring back fdtdec_get_addr() for Exynos GPIO driver. And will revert this one within the patchset.

Best regards,

Signature · 4 lines

Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

Simon Glass

2016-01-12 13:59 UTC #12 lore raw

PATCH 1/1 fdt: fix address cell count checking in fdt_translate_address()

Hi Przemyslaw,

Quoted 198 lines · Przemyslaw Marczak

Hello Simon,
Quoted 194 lines · Simon Glass
Hi Przemyslaw,
Quoted 175 lines · Przemyslaw Marczak
Hello Simon,
Quoted 161 lines · Simon Glass
+Stephen
Quoted 151 lines · Simon Glass
Hi Przemyslaw,
Quoted 139 lines · Stefan Roese
Quoted 125 lines · Simon Glass
Hi,
Quoted 107 lines · Przemyslaw Marczak
Hello All,
Quoted 100 lines · Simon Glass
Hi Stefan,
Quoted 67 lines · Przemyslaw Marczak
Commit: dm: core: Enable optional use of fdt_translate_address()
Enables use of this function as default, but after this it's not possible to get dev address for the case in which: '#size-cells == 0'
This causes errors when getting address for some GPIOs, for which the '#size-cells' is set to 0.
Example error:
'__of_translate_address: Bad cell count for gpx0'
Allowing for that case by modifying the macro 'OF_CHECK_COUNTS', (called from )__of_translate_address(), fixes the issue.
Now, this macro doesn't check, that '#size-cells' is greater than 0.
This is possible from the specification point of view, but I'm not sure
that it doesn't introduce a regression for other configs.
Please test and share the results.
Tested-on: Odroid U3, Odroid X2, Odroid XU3, Sandbox.
Signed-off-by Przemyslaw Marczak <[email protected]>
Cc Masahiro Yamada <[email protected]>
Cc Lukasz Majewski <[email protected]>
Cc Jaehoon Chung <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Simon Glass <[email protected]>
Cc Bin Meng <[email protected]>
Cc Marek Vasut <[email protected]>
---
     common/fdt_support.c | 7 +++----
     1 file changed, 3 insertions(+), 4 deletions(-)
common/fdt_support.c +0 -0
index f86365e..5f808cc 100644
@@ -946,8 +946,7 @@ void fdt_del_node_and_alias(void *blob, const
char
*alias)
     /* Max address size we deal with */
     #define OF_MAX_ADDR_CELLS      4
     #define OF_BAD_ADDR    ((u64)-1)
-#define OF_CHECK_COUNTS(na, ns) ((na) > 0 && (na) <=
OF_MAX_ADDR_CELLS && \
common/fdt_support.c +1 -1
-                       (ns) > 0)
+#define OF_CHECK_COUNTS(na)    ((na) > 0 && (na) <=
OF_MAX_ADDR_CELLS)
     /* Debug utility */
     #ifdef DEBUG
common/fdt_support.c +0 -0
@@ -1115,7 +1114,7 @@ static u64 __of_translate_address(void
 *blob,
int
node_offset, const fdt32_t *in
            /* Cound address cells & copy address locally */
            bus->count_cells(blob, parent, &na, &ns);
common/fdt_support.c
Quoted 2 lines
common/fdt_support.c +1 -1
-       if (!OF_CHECK_COUNTS(na, ns)) {
+       if (!OF_CHECK_COUNTS(na)) {
This seems to conflict with the comment at the top of this function:
* Note: We consider that crossing any level with #size-cells
== 0
to
mean
* that translation is impossible (that is we are not dealing
with
a
value
* that can be mapped to a cpu physical address). This is not
really
specified
* that way, but this is traditionally the way IBM at least do
things
Quoted 6 lines
Quoted 2 lines
What should we do here?
Is that commit acceptable? I would like send V2 with removing the above
comment.
Quoted 2 lines
That's what I am worried about. Presumably the comment is accurate today and this check has some value. I was hoping Stefan might know.
Unfortunately no. I just stumbled over this problem with the translation of the "complex" ranges on the MVEBU platform. And noticed that we already have this functionality to translate the addresses the "right way".
Quoted 4 lines
I'm wondering how this problem with those GPIOs is handled in the kernel? I assume that it is working correctly there, right? Przemyslaw, could you perhaps check this and see, why its working there? And change / fix it in U-Boot accordingly?
Quoted 2 lines
Let's pick up this patch for now as a bug-fix. We can deal with this problem after the release.
Quoted 8 lines
Applied to u-boot-dm/master.
I'll post a revert after the release. It seems like you and Stephen are making good progress.
- Simon
Quoted 6 lines
Why so fast with this one?
I think, that more proper for a temporary fix is my latest patch with #size-cells count checking only if ranges found in the parent node.
I will continue the discussion with Stephen.
The release is scheduled for today, so we had to do something to fix the breakage.
Once you have a full solution figured out we can revert this patch and apply what you come up with.

Quoted 8 lines

Quoted 4 lines
Regards,
Simon
Ok. It's hard to convince Stephen to accept such change, so I will send a patch with another solution - just bring back fdtdec_get_addr() for Exynos GPIO driver. And will revert this one within the patchset.

Please hold off on that. I'll accept your other patch but let's see if Stephen wants to write something first. Using fdtdec_get_addr() doesn't make sense although I fully understand your frustration. Let's give it a week.

Regards,
Simon

Przemyslaw Marczak

author

2016-01-12 14:25 UTC #13 lore raw

PATCH 1/1 fdt: fix address cell count checking in fdt_translate_address()

Hello Simon,

Quoted 210 lines · Simon Glass

Hi Przemyslaw,
Quoted 198 lines · Przemyslaw Marczak
Hello Simon,
Quoted 194 lines · Simon Glass
Hi Przemyslaw,
Quoted 175 lines · Przemyslaw Marczak
Hello Simon,
Quoted 161 lines · Simon Glass
+Stephen
Quoted 151 lines · Simon Glass
Hi Przemyslaw,
Quoted 139 lines · Stefan Roese
Quoted 125 lines · Simon Glass
Hi,
Quoted 107 lines · Przemyslaw Marczak
Hello All,
Quoted 100 lines · Simon Glass
Hi Stefan,
Quoted 67 lines · Przemyslaw Marczak
Commit: dm: core: Enable optional use of fdt_translate_address()
Enables use of this function as default, but after this it's not possible to get dev address for the case in which: '#size-cells == 0'
This causes errors when getting address for some GPIOs, for which the '#size-cells' is set to 0.
Example error:
'__of_translate_address: Bad cell count for gpx0'
Allowing for that case by modifying the macro 'OF_CHECK_COUNTS', (called from )__of_translate_address(), fixes the issue.
Now, this macro doesn't check, that '#size-cells' is greater than 0.
This is possible from the specification point of view, but I'm not sure
that it doesn't introduce a regression for other configs.
Please test and share the results.
Tested-on: Odroid U3, Odroid X2, Odroid XU3, Sandbox.
Signed-off-by Przemyslaw Marczak <[email protected]>
Cc Masahiro Yamada <[email protected]>
Cc Lukasz Majewski <[email protected]>
Cc Jaehoon Chung <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Simon Glass <[email protected]>
Cc Bin Meng <[email protected]>
Cc Marek Vasut <[email protected]>
---
      common/fdt_support.c | 7 +++----
      1 file changed, 3 insertions(+), 4 deletions(-)
common/fdt_support.c +0 -0
index f86365e..5f808cc 100644
@@ -946,8 +946,7 @@ void fdt_del_node_and_alias(void *blob, const
char
*alias)
      /* Max address size we deal with */
      #define OF_MAX_ADDR_CELLS      4
      #define OF_BAD_ADDR    ((u64)-1)
-#define OF_CHECK_COUNTS(na, ns) ((na) > 0 && (na) <=
OF_MAX_ADDR_CELLS && \
common/fdt_support.c +1 -1
-                       (ns) > 0)
+#define OF_CHECK_COUNTS(na)    ((na) > 0 && (na) <=
OF_MAX_ADDR_CELLS)
      /* Debug utility */
      #ifdef DEBUG
common/fdt_support.c +0 -0
@@ -1115,7 +1114,7 @@ static u64 __of_translate_address(void
 *blob,
int
node_offset, const fdt32_t *in
             /* Cound address cells & copy address locally */
             bus->count_cells(blob, parent, &na, &ns);
common/fdt_support.c
Quoted 2 lines
common/fdt_support.c +1 -1
-       if (!OF_CHECK_COUNTS(na, ns)) {
+       if (!OF_CHECK_COUNTS(na)) {
This seems to conflict with the comment at the top of this function:
* Note: We consider that crossing any level with #size-cells
== 0
to
mean
* that translation is impossible (that is we are not dealing
with
a
value
* that can be mapped to a cpu physical address). This is not
really
specified
* that way, but this is traditionally the way IBM at least do
things
Quoted 6 lines
Quoted 2 lines
What should we do here?
Is that commit acceptable? I would like send V2 with removing the above
comment.
Quoted 2 lines
That's what I am worried about. Presumably the comment is accurate today and this check has some value. I was hoping Stefan might know.
Unfortunately no. I just stumbled over this problem with the translation of the "complex" ranges on the MVEBU platform. And noticed that we already have this functionality to translate the addresses the "right way".
Quoted 4 lines
I'm wondering how this problem with those GPIOs is handled in the kernel? I assume that it is working correctly there, right? Przemyslaw, could you perhaps check this and see, why its working there? And change / fix it in U-Boot accordingly?
Quoted 2 lines
Let's pick up this patch for now as a bug-fix. We can deal with this problem after the release.
Quoted 8 lines
Applied to u-boot-dm/master.
I'll post a revert after the release. It seems like you and Stephen are making good progress.
- Simon
Quoted 6 lines
Why so fast with this one?
I think, that more proper for a temporary fix is my latest patch with #size-cells count checking only if ranges found in the parent node.
I will continue the discussion with Stephen.
The release is scheduled for today, so we had to do something to fix the breakage.
Once you have a full solution figured out we can revert this patch and apply what you come up with.
Quoted 8 lines
Quoted 4 lines
Regards,
Simon
Ok. It's hard to convince Stephen to accept such change, so I will send a patch with another solution - just bring back fdtdec_get_addr() for Exynos GPIO driver. And will revert this one within the patchset.

Quoted 9 lines

Please hold off on that. I'll accept your other patch but let's see if Stephen wants to write something first. Using fdtdec_get_addr() doesn't make sense although I fully understand your frustration. Let's give it a week.
Regards,
Simon

Ah, too late:)

I have prepared patch, it's simple and doesn't touch the code - only device-tree. And this would be a proper approach, according to Stephen's comments.

Best regards,

Signature · 4 lines

Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com