[U-Boot] [PATCH] fdt: fix address cell count checking in fdt_translate_address()

[U-Boot] [PATCH] fdt: fix address cell count checking in fdt_translate_address()

[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 <p.marczak@samsung.com>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Lukasz Majewski <l.majewski@samsung.com>
Cc: Jaehoon Chung <jh80.chung@samsung.com>
Cc: Stefan Roese <sr@denx.de>
Cc: Simon Glass <sjg@chromium.org>
Cc: Bin Meng <bmeng.cn@gmail.com>
Cc: Marek Vasut <marex@denx.de>
---
 common/fdt_support.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/common/fdt_support.c b/common/fdt_support.c
index f86365e..5f808cc 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -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;
-- 
1.9.1

[U-Boot] [PATCH] fdt: fix address cell count checking in fdt_translate_address()

[Jaehoon Chung]

Hi, Przemyslaw.


On 10/28/2015 11:37 PM, Przemyslaw Marczak wrote:
> 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.

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

Tested-by: Jaehoon Chung <jh80.chung@samsung.com>

Best Regards,
Jaehoon Chung

> 
> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> Cc: Lukasz Majewski <l.majewski@samsung.com>
> Cc: Jaehoon Chung <jh80.chung@samsung.com>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Bin Meng <bmeng.cn@gmail.com>
> Cc: Marek Vasut <marex@denx.de>
> ---
>  common/fdt_support.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/common/fdt_support.c b/common/fdt_support.c
> index f86365e..5f808cc 100644
> --- a/common/fdt_support.c
> +++ b/common/fdt_support.c
> @@ -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;
> 

[U-Boot] [PATCH] fdt: fix address cell count checking in fdt_translate_address()

[Lukasz Majewski]

Hi Jaehoon,

> Hi, Przemyslaw.
> 
> 
> On 10/28/2015 11:37 PM, Przemyslaw Marczak wrote:
> > 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.
> 
> I have also tested with exynos boards.
> As you mentioned, it seems that needs to test for other SoC.
> 
> Tested-by: Jaehoon Chung <jh80.chung@samsung.com>
> 
> Best Regards,
> Jaehoon Chung
> 
> > 
> > Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
> > Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> > Cc: Lukasz Majewski <l.majewski@samsung.com>
> > Cc: Jaehoon Chung <jh80.chung@samsung.com>
> > Cc: Stefan Roese <sr@denx.de>
> > Cc: Simon Glass <sjg@chromium.org>
> > Cc: Bin Meng <bmeng.cn@gmail.com>
> > Cc: Marek Vasut <marex@denx.de>
> > ---
> >  common/fdt_support.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/common/fdt_support.c b/common/fdt_support.c
> > index f86365e..5f808cc 100644
> > --- a/common/fdt_support.c
> > +++ b/common/fdt_support.c
> > @@ -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;
> > 
> 

Tested-by: Lukasz Majewski <l.majewski@samsung.com>

Test HW: Exynos 4210 - Trats board.

-- 
Best regards,

Lukasz Majewski

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

[U-Boot] [PATCH] fdt: fix address cell count checking in fdt_translate_address()

[Simon Glass]

Hi Stefan,

On 28 October 2015 at 08:37, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
> 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 <p.marczak@samsung.com>
> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> Cc: Lukasz Majewski <l.majewski@samsung.com>
> Cc: Jaehoon Chung <jh80.chung@samsung.com>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Bin Meng <bmeng.cn@gmail.com>
> Cc: Marek Vasut <marex@denx.de>
> ---
>  common/fdt_support.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/common/fdt_support.c b/common/fdt_support.c
> index f86365e..5f808cc 100644
> --- a/common/fdt_support.c
> +++ b/common/fdt_support.c
> @@ -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)) {

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?

>                 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;
> --
> 1.9.1
>

Regards,
Simon

[U-Boot] [PATCH] fdt: fix address cell count checking in fdt_translate_address()

[Przemyslaw Marczak]

Hello All,

On 10/29/2015 06:15 PM, Simon Glass wrote:
> Hi Stefan,
>
> On 28 October 2015 at 08:37, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
>> 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 <p.marczak@samsung.com>
>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>> Cc: Lukasz Majewski <l.majewski@samsung.com>
>> Cc: Jaehoon Chung <jh80.chung@samsung.com>
>> Cc: Stefan Roese <sr@denx.de>
>> Cc: Simon Glass <sjg@chromium.org>
>> Cc: Bin Meng <bmeng.cn@gmail.com>
>> Cc: Marek Vasut <marex@denx.de>
>> ---
>>   common/fdt_support.c | 7 +++----
>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/common/fdt_support.c b/common/fdt_support.c
>> index f86365e..5f808cc 100644
>> --- a/common/fdt_support.c
>> +++ b/common/fdt_support.c
>> @@ -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)) {
>
> 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?
>

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

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

>>                  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;
>> --
>> 1.9.1
>>
>
> Regards,
> Simon
>

[U-Boot] [PATCH] fdt: fix address cell count checking in fdt_translate_address()

[Simon Glass]

Hi,

On 3 November 2015 at 02:57, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
> Hello All,
>
>
> On 10/29/2015 06:15 PM, Simon Glass wrote:
>>
>> Hi Stefan,
>>
>> On 28 October 2015 at 08:37, Przemyslaw Marczak <p.marczak@samsung.com>
>> wrote:
>>>
>>> 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 <p.marczak@samsung.com>
>>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>>> Cc: Lukasz Majewski <l.majewski@samsung.com>
>>> Cc: Jaehoon Chung <jh80.chung@samsung.com>
>>> Cc: Stefan Roese <sr@denx.de>
>>> Cc: Simon Glass <sjg@chromium.org>
>>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>> Cc: Marek Vasut <marex@denx.de>
>>> ---
>>>   common/fdt_support.c | 7 +++----
>>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/common/fdt_support.c b/common/fdt_support.c
>>> index f86365e..5f808cc 100644
>>> --- a/common/fdt_support.c
>>> +++ b/common/fdt_support.c
>>> @@ -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)) {
>>
>>
>> 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?
>>
>
> 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.

>
> Best regards,
> --
> Przemyslaw Marczak
> Samsung R&D Institute Poland
> Samsung Electronics
> p.marczak at samsung.com
>
>
>>>                  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;
>>> --
>>> 1.9.1
>>>
>>
>> Regards,
>> Simon
>>
>

Regards,
Simon

[U-Boot] [PATCH] fdt: fix address cell count checking in fdt_translate_address()

[Stefan Roese]

On 06.11.2015 04:16, Simon Glass wrote:
> Hi,
>
> On 3 November 2015 at 02:57, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
>> Hello All,
>>
>>
>> On 10/29/2015 06:15 PM, Simon Glass wrote:
>>>
>>> Hi Stefan,
>>>
>>> On 28 October 2015 at 08:37, Przemyslaw Marczak <p.marczak@samsung.com>
>>> wrote:
>>>>
>>>> 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 <p.marczak@samsung.com>
>>>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>>>> Cc: Lukasz Majewski <l.majewski@samsung.com>
>>>> Cc: Jaehoon Chung <jh80.chung@samsung.com>
>>>> Cc: Stefan Roese <sr@denx.de>
>>>> Cc: Simon Glass <sjg@chromium.org>
>>>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>>> Cc: Marek Vasut <marex@denx.de>
>>>> ---
>>>>    common/fdt_support.c | 7 +++----
>>>>    1 file changed, 3 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/common/fdt_support.c b/common/fdt_support.c
>>>> index f86365e..5f808cc 100644
>>>> --- a/common/fdt_support.c
>>>> +++ b/common/fdt_support.c
>>>> @@ -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)) {
>>>
>>>
>>> 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?
>>>
>>
>> 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

[U-Boot] [PATCH] fdt: fix address cell count checking in fdt_translate_address()

[Simon Glass]

Hi Przemyslaw,

On 5 November 2015 at 23:47, Stefan Roese <sr@denx.de> wrote:
> On 06.11.2015 04:16, Simon Glass wrote:
>>
>> Hi,
>>
>> On 3 November 2015 at 02:57, Przemyslaw Marczak <p.marczak@samsung.com>
>> wrote:
>>>
>>> Hello All,
>>>
>>>
>>> On 10/29/2015 06:15 PM, Simon Glass wrote:
>>>>
>>>>
>>>> Hi Stefan,
>>>>
>>>> On 28 October 2015 at 08:37, Przemyslaw Marczak <p.marczak@samsung.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> 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 <p.marczak@samsung.com>
>>>>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>>>>> Cc: Lukasz Majewski <l.majewski@samsung.com>
>>>>> Cc: Jaehoon Chung <jh80.chung@samsung.com>
>>>>> Cc: Stefan Roese <sr@denx.de>
>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>>>> Cc: Marek Vasut <marex@denx.de>
>>>>> ---
>>>>>    common/fdt_support.c | 7 +++----
>>>>>    1 file changed, 3 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/common/fdt_support.c b/common/fdt_support.c
>>>>> index f86365e..5f808cc 100644
>>>>> --- a/common/fdt_support.c
>>>>> +++ b/common/fdt_support.c
>>>>> @@ -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)) {
>>>>
>>>>
>>>>
>>>> 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?
>>>>
>>>
>>> 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?

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

Regards,
Simon

[U-Boot] [PATCH] fdt: fix address cell count checking in fdt_translate_address()

[Simon Glass]

+Stephen

On 4 January 2016 at 17:59, Simon Glass <sjg@chromium.org> wrote:
> Hi Przemyslaw,
>
> On 5 November 2015 at 23:47, Stefan Roese <sr@denx.de> wrote:
>> On 06.11.2015 04:16, Simon Glass wrote:
>>>
>>> Hi,
>>>
>>> On 3 November 2015 at 02:57, Przemyslaw Marczak <p.marczak@samsung.com>
>>> wrote:
>>>>
>>>> Hello All,
>>>>
>>>>
>>>> On 10/29/2015 06:15 PM, Simon Glass wrote:
>>>>>
>>>>>
>>>>> Hi Stefan,
>>>>>
>>>>> On 28 October 2015 at 08:37, Przemyslaw Marczak <p.marczak@samsung.com>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>> 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 <p.marczak@samsung.com>
>>>>>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>>>>>> Cc: Lukasz Majewski <l.majewski@samsung.com>
>>>>>> Cc: Jaehoon Chung <jh80.chung@samsung.com>
>>>>>> Cc: Stefan Roese <sr@denx.de>
>>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>>>>> Cc: Marek Vasut <marex@denx.de>
>>>>>> ---
>>>>>>    common/fdt_support.c | 7 +++----
>>>>>>    1 file changed, 3 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/common/fdt_support.c b/common/fdt_support.c
>>>>>> index f86365e..5f808cc 100644
>>>>>> --- a/common/fdt_support.c
>>>>>> +++ b/common/fdt_support.c
>>>>>> @@ -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)) {
>>>>>
>>>>>
>>>>>
>>>>> 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?
>>>>>
>>>>
>>>> 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?
>
> 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

[U-Boot] [PATCH] fdt: fix address cell count checking in fdt_translate_address()

[Przemyslaw Marczak]

Hello Simon,

On 01/07/2016 08:24 PM, Simon Glass wrote:
> +Stephen
>
> On 4 January 2016 at 17:59, Simon Glass <sjg@chromium.org> wrote:
>> Hi Przemyslaw,
>>
>> On 5 November 2015 at 23:47, Stefan Roese <sr@denx.de> wrote:
>>> On 06.11.2015 04:16, Simon Glass wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 3 November 2015 at 02:57, Przemyslaw Marczak <p.marczak@samsung.com>
>>>> wrote:
>>>>>
>>>>> Hello All,
>>>>>
>>>>>
>>>>> On 10/29/2015 06:15 PM, Simon Glass wrote:
>>>>>>
>>>>>>
>>>>>> Hi Stefan,
>>>>>>
>>>>>> On 28 October 2015 at 08:37, Przemyslaw Marczak <p.marczak@samsung.com>
>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>> 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 <p.marczak@samsung.com>
>>>>>>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>>>>>>> Cc: Lukasz Majewski <l.majewski@samsung.com>
>>>>>>> Cc: Jaehoon Chung <jh80.chung@samsung.com>
>>>>>>> Cc: Stefan Roese <sr@denx.de>
>>>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>>>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>>>>>> Cc: Marek Vasut <marex@denx.de>
>>>>>>> ---
>>>>>>>     common/fdt_support.c | 7 +++----
>>>>>>>     1 file changed, 3 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/common/fdt_support.c b/common/fdt_support.c
>>>>>>> index f86365e..5f808cc 100644
>>>>>>> --- a/common/fdt_support.c
>>>>>>> +++ b/common/fdt_support.c
>>>>>>> @@ -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)) {
>>>>>>
>>>>>>
>>>>>>
>>>>>> 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?
>>>>>>
>>>>>
>>>>> 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?
>>
>> 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
>
>

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,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

[U-Boot] [PATCH] fdt: fix address cell count checking in fdt_translate_address()

[Simon Glass]

Hi Przemyslaw,

On 8 January 2016 at 05:01, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
> Hello Simon,
>
>
> On 01/07/2016 08:24 PM, Simon Glass wrote:
>>
>> +Stephen
>>
>> On 4 January 2016 at 17:59, Simon Glass <sjg@chromium.org> wrote:
>>>
>>> Hi Przemyslaw,
>>>
>>> On 5 November 2015 at 23:47, Stefan Roese <sr@denx.de> wrote:
>>>>
>>>> On 06.11.2015 04:16, Simon Glass wrote:
>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>> On 3 November 2015 at 02:57, Przemyslaw Marczak <p.marczak@samsung.com>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>> Hello All,
>>>>>>
>>>>>>
>>>>>> On 10/29/2015 06:15 PM, Simon Glass wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Hi Stefan,
>>>>>>>
>>>>>>> On 28 October 2015 at 08:37, Przemyslaw Marczak
>>>>>>> <p.marczak@samsung.com>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> 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 <p.marczak@samsung.com>
>>>>>>>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>>>>>>>> Cc: Lukasz Majewski <l.majewski@samsung.com>
>>>>>>>> Cc: Jaehoon Chung <jh80.chung@samsung.com>
>>>>>>>> Cc: Stefan Roese <sr@denx.de>
>>>>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>>>>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>>>>>>> Cc: Marek Vasut <marex@denx.de>
>>>>>>>> ---
>>>>>>>>     common/fdt_support.c | 7 +++----
>>>>>>>>     1 file changed, 3 insertions(+), 4 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/common/fdt_support.c b/common/fdt_support.c
>>>>>>>> index f86365e..5f808cc 100644
>>>>>>>> --- a/common/fdt_support.c
>>>>>>>> +++ b/common/fdt_support.c
>>>>>>>> @@ -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)) {
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> 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?
>>>>>>>
>>>>>>
>>>>>> 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?
>>>
>>>
>>> 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
>>
>>
>
> 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

[U-Boot] [PATCH] fdt: fix address cell count checking in fdt_translate_address()

[Przemyslaw Marczak]

Hello Simon,

On 01/11/2016 05:59 PM, Simon Glass wrote:
> Hi Przemyslaw,
>
> On 8 January 2016 at 05:01, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
>> Hello Simon,
>>
>>
>> On 01/07/2016 08:24 PM, Simon Glass wrote:
>>>
>>> +Stephen
>>>
>>> On 4 January 2016 at 17:59, Simon Glass <sjg@chromium.org> wrote:
>>>>
>>>> Hi Przemyslaw,
>>>>
>>>> On 5 November 2015 at 23:47, Stefan Roese <sr@denx.de> wrote:
>>>>>
>>>>> On 06.11.2015 04:16, Simon Glass wrote:
>>>>>>
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On 3 November 2015 at 02:57, Przemyslaw Marczak <p.marczak@samsung.com>
>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>> Hello All,
>>>>>>>
>>>>>>>
>>>>>>> On 10/29/2015 06:15 PM, Simon Glass wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Hi Stefan,
>>>>>>>>
>>>>>>>> On 28 October 2015 at 08:37, Przemyslaw Marczak
>>>>>>>> <p.marczak@samsung.com>
>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 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 <p.marczak@samsung.com>
>>>>>>>>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>>>>>>>>> Cc: Lukasz Majewski <l.majewski@samsung.com>
>>>>>>>>> Cc: Jaehoon Chung <jh80.chung@samsung.com>
>>>>>>>>> Cc: Stefan Roese <sr@denx.de>
>>>>>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>>>>>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>>>>>>>> Cc: Marek Vasut <marex@denx.de>
>>>>>>>>> ---
>>>>>>>>>      common/fdt_support.c | 7 +++----
>>>>>>>>>      1 file changed, 3 insertions(+), 4 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/common/fdt_support.c b/common/fdt_support.c
>>>>>>>>> index f86365e..5f808cc 100644
>>>>>>>>> --- a/common/fdt_support.c
>>>>>>>>> +++ b/common/fdt_support.c
>>>>>>>>> @@ -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)) {
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> 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?
>>>>>>>>
>>>>>>>
>>>>>>> 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?
>>>>
>>>>
>>>> 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
>>>
>>>
>>
>> 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
>
>

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,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

[U-Boot] [PATCH] fdt: fix address cell count checking in fdt_translate_address()

[Simon Glass]

Hi Przemyslaw,

On 12 January 2016 at 03:49, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
> Hello Simon,
>
>
> On 01/11/2016 05:59 PM, Simon Glass wrote:
>>
>> Hi Przemyslaw,
>>
>> On 8 January 2016 at 05:01, Przemyslaw Marczak <p.marczak@samsung.com>
>> wrote:
>>>
>>> Hello Simon,
>>>
>>>
>>> On 01/07/2016 08:24 PM, Simon Glass wrote:
>>>>
>>>>
>>>> +Stephen
>>>>
>>>> On 4 January 2016 at 17:59, Simon Glass <sjg@chromium.org> wrote:
>>>>>
>>>>>
>>>>> Hi Przemyslaw,
>>>>>
>>>>> On 5 November 2015 at 23:47, Stefan Roese <sr@denx.de> wrote:
>>>>>>
>>>>>>
>>>>>> On 06.11.2015 04:16, Simon Glass wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 3 November 2015 at 02:57, Przemyslaw Marczak
>>>>>>> <p.marczak@samsung.com>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Hello All,
>>>>>>>>
>>>>>>>>
>>>>>>>> On 10/29/2015 06:15 PM, Simon Glass wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Hi Stefan,
>>>>>>>>>
>>>>>>>>> On 28 October 2015 at 08:37, Przemyslaw Marczak
>>>>>>>>> <p.marczak@samsung.com>
>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> 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 <p.marczak@samsung.com>
>>>>>>>>>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>>>>>>>>>> Cc: Lukasz Majewski <l.majewski@samsung.com>
>>>>>>>>>> Cc: Jaehoon Chung <jh80.chung@samsung.com>
>>>>>>>>>> Cc: Stefan Roese <sr@denx.de>
>>>>>>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>>>>>>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>>>>>>>>> Cc: Marek Vasut <marex@denx.de>
>>>>>>>>>> ---
>>>>>>>>>>      common/fdt_support.c | 7 +++----
>>>>>>>>>>      1 file changed, 3 insertions(+), 4 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/common/fdt_support.c b/common/fdt_support.c
>>>>>>>>>> index f86365e..5f808cc 100644
>>>>>>>>>> --- a/common/fdt_support.c
>>>>>>>>>> +++ b/common/fdt_support.c
>>>>>>>>>> @@ -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)) {
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 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?
>>>>>>>>>
>>>>>>>>
>>>>>>>> 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?
>>>>>
>>>>>
>>>>>
>>>>> 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
>>>>
>>>>
>>>
>>> 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
>>
>>
>
> 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

[U-Boot] [PATCH] fdt: fix address cell count checking in fdt_translate_address()

[Przemyslaw Marczak]

Hello Simon,

On 01/12/2016 02:59 PM, Simon Glass wrote:
> Hi Przemyslaw,
>
> On 12 January 2016 at 03:49, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
>> Hello Simon,
>>
>>
>> On 01/11/2016 05:59 PM, Simon Glass wrote:
>>>
>>> Hi Przemyslaw,
>>>
>>> On 8 January 2016 at 05:01, Przemyslaw Marczak <p.marczak@samsung.com>
>>> wrote:
>>>>
>>>> Hello Simon,
>>>>
>>>>
>>>> On 01/07/2016 08:24 PM, Simon Glass wrote:
>>>>>
>>>>>
>>>>> +Stephen
>>>>>
>>>>> On 4 January 2016 at 17:59, Simon Glass <sjg@chromium.org> wrote:
>>>>>>
>>>>>>
>>>>>> Hi Przemyslaw,
>>>>>>
>>>>>> On 5 November 2015 at 23:47, Stefan Roese <sr@denx.de> wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 06.11.2015 04:16, Simon Glass wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 3 November 2015 at 02:57, Przemyslaw Marczak
>>>>>>>> <p.marczak@samsung.com>
>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Hello All,
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 10/29/2015 06:15 PM, Simon Glass wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Hi Stefan,
>>>>>>>>>>
>>>>>>>>>> On 28 October 2015 at 08:37, Przemyslaw Marczak
>>>>>>>>>> <p.marczak@samsung.com>
>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> 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 <p.marczak@samsung.com>
>>>>>>>>>>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>>>>>>>>>>> Cc: Lukasz Majewski <l.majewski@samsung.com>
>>>>>>>>>>> Cc: Jaehoon Chung <jh80.chung@samsung.com>
>>>>>>>>>>> Cc: Stefan Roese <sr@denx.de>
>>>>>>>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>>>>>>>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>>>>>>>>>> Cc: Marek Vasut <marex@denx.de>
>>>>>>>>>>> ---
>>>>>>>>>>>       common/fdt_support.c | 7 +++----
>>>>>>>>>>>       1 file changed, 3 insertions(+), 4 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/common/fdt_support.c b/common/fdt_support.c
>>>>>>>>>>> index f86365e..5f808cc 100644
>>>>>>>>>>> --- a/common/fdt_support.c
>>>>>>>>>>> +++ b/common/fdt_support.c
>>>>>>>>>>> @@ -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)) {
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> 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?
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 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?
>>>>>>
>>>>>>
>>>>>>
>>>>>> 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
>>>>>
>>>>>
>>>>
>>>> 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
>>>
>>>
>>
>> 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
>
>

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,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com