Re: [Minios-devel] [UNIKRAFT PATCHv4 4/9] plat/common: Introduce fdt_{address, size}_cells_or_parent helpers

[Sharan Santhanam <sharan.santhanam@xxxxxxxxx>]

Hello Jia He,

Please find the comments inline:

Thanks & Regards
Sharan

On 3/27/19 3:34 AM, Jia He wrote:
> In some cases, devices need to find the "#address-cells" "size-cells"
> properties recursively(node, parent, grandparent...).
> This patch introduces two helpers to simplify the codes
>
> Signed-off-by: Jia He <justin.he@xxxxxxx>
> ---
>   plat/common/fdt.c         | 57 +++++++++++++++++++++++++++++++++++++++
>   plat/common/include/fdt.h | 34 +++++++++++++++++++++++
>   2 files changed, 91 insertions(+)
>
> diff --git a/plat/common/fdt.c b/plat/common/fdt.c
> index 916a263..6feec0a 100644
> --- a/plat/common/fdt.c
> +++ b/plat/common/fdt.c
> @@ -99,3 +99,60 @@ int fdt_interrupt_cells(const void *fdt, int offset)
>   
>   	return val;
>   }

Would rename this function fdt_address_cell_get or fdt_address_cell?
> + 
> +int fdt_address_cells_or_parent(const void *fdt, int nodeoffset)
> +{
> +       int prop_len;
> +       const fdt32_t *prop;
> +
> +       do {
 The specification[1] states "The #address-cells
 property defines the number of <u32> cells used to encode the
 address field in a child node's reg property."

 In our case shouldn't the operation be fdt_parent_offset and
 then fdt_getprop?

> +               /* Find whether the property exists in this node */
> +               prop = fdt_getprop(fdt, nodeoffset,
> +                               "#address-cells", &prop_len);
> +               /* If not, try to find in parent node */
> +               nodeoffset = fdt_parent_offset(fdt, nodeoffset);
> +       } while (nodeoffset >= 0 && prop == NULL);
> +
> +       /* as per DT spec, set the address-cells to 2 if no such
> +        * property anywhere in this node or parent
> +        */
> +       if (prop == NULL)
> +               return 2;
> +
When would this condition happen? If prop is not null why would we have 
to check this
> +       if (nodeoffset < 0)
> +               return nodeoffset;
> +
> +       if (prop_len >= (int)sizeof(fdt32_t))
> +               return fdt32_to_cpu(prop[0]);
> +
> +       return -FDT_ERR_NOTFOUND;
> +}
> +
Would rename this function fdt_size_cell_get or fdt_size_cell?
> +int fdt_size_cells_or_parent(const void *fdt, int nodeoffset)
> +{
> +       int prop_len;
> +       const fdt32_t *prop;
> +
> +       do {
 The specification[1] states "The #size-cells
 property defines the number of <u32> cells used to encode the
 size field in a child node's reg property."

 In our case shouldn't the operation be fdt_parent_offset and
 then fdt_getprop?

> +               /* Find whether the property exists in this node */
> +               prop = fdt_getprop(fdt, nodeoffset,
> +                               "#size-cells", &prop_len);
> +                       break;
> +               /* If not, try to find in parent node */
> +               nodeoffset = fdt_parent_offset(fdt, nodeoffset);
> +       } while (nodeoffset >= 0 && prop == NULL);
> +
> +       /* as per DT spec, set the size-cells to 1 if no such
> +        * property anywhere in this node or parent
> +        */
> +       if (prop == NULL)
> +               return 1;
> +
When would this condition happen? If prop is not null why would we have 
to check this
> +       if (nodeoffset < 0)
> +               return nodeoffset;
> +
> +       if (prop_len >= (int)sizeof(fdt32_t))
> +               return fdt32_to_cpu(prop[0]);
> +
> +       return -FDT_ERR_NOTFOUND;
> +}
> diff --git a/plat/common/include/fdt.h b/plat/common/include/fdt.h
> index afc4753..969cd28 100644
> --- a/plat/common/include/fdt.h
> +++ b/plat/common/include/fdt.h
> @@ -76,4 +76,38 @@ int fdt_getprop_u32_by_offset(const void *fdt, int 
> nodeoffset,
>    */
>   int fdt_interrupt_cells(const void *fdt, int nodeoffset);
>   
> +/**
> + * fdt_address_cells_or_parent - retrieve cell size for a bus represented
> + * in the tree
> + * @fdt: pointer to the device tree blob
There is no prop parameter
> + * @prop: cell name of the property containing the string list
> + * @nodeoffset: offset of the node to find the address size for
> + *
> + * When the node or parent has a valid #address-cells property, returns
> + * its value.
> + *
> + * returns:
> + *     0 <= n < FDT_MAX_NCELLS, on success
Error return values do not match
> + *      -FDT_ERR_BADNCELLS, if the node has a badly formatted or invalid
> + *             #address-cells property
> + */
> +int fdt_address_cells_or_parent(const void *fdt, int nodeoffset);
> +
> +/**
> + * fdt_size_cells_or_parent - retrieve cell size for a bus represented
> + * in the tree
> + * @fdt: pointer to the device tree blob
There is no prop parameter
> + * @prop: cell name of the property containing the string list
> + * @nodeoffset: offset of the node to find the address size for
> + *
> + * When the node or parent has a valid #size-cells property, returns its
> + * value.
> + *
> + * returns:
> + *     0 <= n < FDT_MAX_NCELLS, on success
Error return values do not match
> + *      -FDT_ERR_BADNCELLS, if the node has a badly formatted or invalid
> + *             #address-cells property
> + */
> +int fdt_size_cells_or_parent(const void *fdt, int nodeoffset);
> +
>   #endif
[1] 
https://github.com/devicetree-org/devicetree-specification/blob/master/source/devicetree-basics.rst
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel