[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 blobThere 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 successError 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 blobThere 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 successError return values do not match+ * -FDT_ERR_BADNCELLS, if the node has a badly formatted or invalid[1] https://github.com/devicetree-org/devicetree-specification/blob/master/source/devicetree-basics.rst+ * #address-cells property + */ +int fdt_size_cells_or_parent(const void *fdt, int nodeoffset); + #endif_______________________________________________ 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |