[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 at 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 at samsung.com>
> Cc: Masahiro Yamada <yamada.masahiro at socionext.com>
> Cc: Lukasz Majewski <l.majewski at samsung.com>
> Cc: Jaehoon Chung <jh80.chung at samsung.com>
> Cc: Stefan Roese <sr at denx.de>
> Cc: Simon Glass <sjg at chromium.org>
> Cc: Bin Meng <bmeng.cn at gmail.com>
> Cc: Marek Vasut <marex at 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