[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,

On 4/10/19 7:43 AM, Jia He wrote:
Hi Sharan

After I dig more on it, I would like to rework this patch 4/9.

Please see the comments from libfdt author [1]

"

   * #address-cells and #size-cells describe the format of addresses
     for children of this node, not this node itself.  So if you're
     looking to parse 'reg' for this node, you *always* need to look at
     the parent, not just as a fallback.

   * #address-cells and #size-cells are *not* inherited.  If they're
     missing in a node, then the format for its children's addresses is
     2 cell addresses and 2 cell sizes, it is *not* correct to look at
     the next parent up for these properties.
"

So, seems I should

parent = fdt_parent_offset(fdt, nodeoffset);
naddr = fdt_address_cells(parent, nodeoffset);
nsize = fdt_size_cells(parent, nodeoffset);

I agree, we could do this way as well.

My suggestion was based on the previous patch where we implement interrupt-cells. For interpreting the interrupt property, we read interrupt-cell of the parent within the function fdt_interrupt_cells. In this function we pass the node offset of the child. We could try and keep the APIs consistent for similar type of functions.

[1] https://lists.denx.de/pipermail/u-boot/2016-March/248354.html

On 2019/4/9 1:29, Sharan Santhanam wrote:
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

--

---
Cheers,
Justin (Jia He)


_______________________________________________
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®.