[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.