Re: [Minios-devel] [UNIKRAFT early RFC PATCH 02/11] plat/common/ofw: Move fdt_reg_read_number to header file for static inline

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 21/06/2019 07:57, Jia He wrote:
> Signed-off-by: Jia He <justin.he@xxxxxxx>
> ---
>   plat/drivers/include/ofw/fdt.h | 17 ++++++++++++++++-
>   plat/drivers/ofw/fdt.c         | 17 +----------------
>   2 files changed, 17 insertions(+), 17 deletions(-)
>
> diff --git a/plat/drivers/include/ofw/fdt.h b/plat/drivers/include/ofw/fdt.h
> index d00614d..54ce011 100644
> --- a/plat/drivers/include/ofw/fdt.h
> +++ b/plat/drivers/include/ofw/fdt.h
> @@ -91,7 +91,7 @@ int fdt_interrupt_cells(const void *fdt, int nodeoffset);
>    *      -FDT_ERR_NOTFOUND, if the node doesn't have address property
>    *      -FDT_ERR_NOSPACE, if the node doesn't have address for index
>    */
> -int fdt_get_address(const void *fdt, int nodeoffset, int index,
> +int fdt_get_address(const void *fdt, int nodeoffset, uint32_t index,
I don't think this change belong to this patch. You probably want to fold this 
in the patch which introduced the function or (if merged) in move it in a 
separate patch.
>                         uint64_t *addr, uint64_t *size);
>   
>   /**
> @@ -133,4 +133,19 @@ int fdt_node_offset_by_compatible_list(const void *fdt, 
> int startoffset,
>   const void *fdt_get_interrupt(const void *fdt, int nodeoffset,
>                                 int index, int *size);
>   
> +static inline uint64_t fdt_reg_read_number(const fdt32_t *regs, uint32_t size)
AFAICT the patch introduce this function has not been merged. So it would be 
preferable if you fold this into this patch.
> +{
> +       uint64_t number = 0;
> +
> +       if (size >= 3 || size <= 0)
> +               return -FDT_ERR_BADNCELLS;
> +
> +       for (uint32_t i = 0; i < size; i++) {
> +               number <<= 32;
> +               number |= fdt32_to_cpu(*regs);
> +               regs++;
> +       }
> +
> +       return number;
> +}
>   #endif
> diff --git a/plat/drivers/ofw/fdt.c b/plat/drivers/ofw/fdt.c
> index 450cf58..ff4bce6 100644
> --- a/plat/drivers/ofw/fdt.c
> +++ b/plat/drivers/ofw/fdt.c
> @@ -35,6 +35,7 @@
>   #include <libfdt_env.h>
>   #include <fdt.h>
>   #include <libfdt.h>
> +#include <ofw/fdt.h>
Why do you suddenly need the include here? Didn't need this before so the 
compiler check the prototype match the declaration?
>   
>   #include <uk/print.h>
>   
> @@ -103,22 +104,6 @@ int fdt_interrupt_cells(const void *fdt, int offset)
>         return val;
>   }
>   
> -static uint64_t fdt_reg_read_number(const fdt32_t *regs, uint32_t size)
> -{
> -       uint64_t number = 0;
> -
> -       if (size >= 3 || size <= 0)
> -               return -FDT_ERR_BADNCELLS;
> -
> -       for (uint32_t i = 0; i < size; i++) {
> -               number <<= 32;
> -               number |= fdt32_to_cpu(*regs);
> -               regs++;
> -       }
> -
> -       return number;
> -}
> -
>   /* Default translator (generic bus) */
>   static void fdt_default_count_cells(const void *fdt, int parentoffset,
>                                                int *addrc, int *sizec)
Cheers,

--
Julien Grall

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel