diff mbox series

[V4,02/10] xen/arm: introduce new helper device_tree_get_meminfo

Message ID 20210728102758.3269446-3-penny.zheng@arm.com (mailing list archive)
State Superseded
Headers show
Series Domain on Static Allocation | expand

Commit Message

Penny Zheng July 28, 2021, 10:27 a.m. UTC
A few functions iterate over the device tree property to get memory info,
like "reg" or "xen,static-mem", so this commit creates a new helper
device_tree_get_meminfo to extract the
common codes.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
 xen/arch/arm/bootfdt.c | 104 +++++++++++++++++------------------------
 1 file changed, 42 insertions(+), 62 deletions(-)

Comments

Julien Grall Aug. 11, 2021, 1:35 p.m. UTC | #1
Hi Penny,

On 28/07/2021 11:27, Penny Zheng wrote:
> A few functions iterate over the device tree property to get memory info,
> like "reg" or "xen,static-mem", so this commit creates a new helper
> device_tree_get_meminfo to extract the
> common codes.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>

Ah, this is where you did the consolidation. Sorry, I didn't notice this 
patch.

In general, we are avoiding to introduce code and then rework it in the 
same series. Instead, the rework is done first and then the function is 
used.

So can you move this patch first?

Cheers,
Penny Zheng Aug. 16, 2021, 5:27 a.m. UTC | #2
Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Wednesday, August 11, 2021 9:35 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH V4 02/10] xen/arm: introduce new helper
> device_tree_get_meminfo
> 
> Hi Penny,
> 
> On 28/07/2021 11:27, Penny Zheng wrote:
> > A few functions iterate over the device tree property to get memory
> > info, like "reg" or "xen,static-mem", so this commit creates a new
> > helper device_tree_get_meminfo to extract the common codes.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> 
> Ah, this is where you did the consolidation. Sorry, I didn't notice this patch.
> 
> In general, we are avoiding to introduce code and then rework it in the same
> series. Instead, the rework is done first and then the function is used.
> 
> So can you move this patch first?
> 

Thx for the explanation. I'll reorganize. ;)

> Cheers,
> 
> --

Cheers,

Penny


> Julien Grall
diff mbox series

Patch

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index d2714446e1..04210684c9 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -63,6 +63,44 @@  void __init device_tree_get_reg(const __be32 **cell, u32 address_cells,
     *size = dt_next_cell(size_cells, cell);
 }
 
+static int __init device_tree_get_meminfo(const void *fdt, int node,
+                                          const char *prop_name,
+                                          u32 address_cells, u32 size_cells,
+                                          void *data)
+{
+    const struct fdt_property *prop;
+    unsigned int i, banks;
+    const __be32 *cell;
+    u32 reg_cells = address_cells + size_cells;
+    paddr_t start, size;
+    struct meminfo *mem = data;
+
+    prop = fdt_get_property(fdt, node, prop_name, NULL);
+    if ( !prop )
+        return -ENOENT;
+
+    cell = (const __be32 *)prop->data;
+    banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
+
+    for ( i = 0; i < banks && mem->nr_banks < NR_MEM_BANKS; i++ )
+    {
+        device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
+        /* Some DT may describe empty bank, ignore them */
+        if ( !size )
+            continue;
+        mem->bank[mem->nr_banks].start = start;
+        mem->bank[mem->nr_banks].size = size;
+        mem->nr_banks++;
+    }
+
+    if ( i < banks )
+    {
+        printk("Warning: Max number of supported memory regions reached.\n");
+        return -ENOSPC;
+    }
+    return 0;
+}
+
 u32 __init device_tree_get_u32(const void *fdt, int node,
                                const char *prop_name, u32 dflt)
 {
@@ -139,14 +177,6 @@  static int __init process_memory_node(const void *fdt, int node,
                                       u32 address_cells, u32 size_cells,
                                       void *data)
 {
-    const struct fdt_property *prop;
-    int i;
-    int banks;
-    const __be32 *cell;
-    paddr_t start, size;
-    u32 reg_cells = address_cells + size_cells;
-    struct meminfo *mem = data;
-
     if ( address_cells < 1 || size_cells < 1 )
     {
         printk("fdt: node `%s': invalid #address-cells or #size-cells",
@@ -154,27 +184,7 @@  static int __init process_memory_node(const void *fdt, int node,
         return -EINVAL;
     }
 
-    prop = fdt_get_property(fdt, node, "reg", NULL);
-    if ( !prop )
-        return -ENOENT;
-
-    cell = (const __be32 *)prop->data;
-    banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
-
-    for ( i = 0; i < banks && mem->nr_banks < NR_MEM_BANKS; i++ )
-    {
-        device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
-        /* Some DT may describe empty bank, ignore them */
-        if ( !size )
-            continue;
-        mem->bank[mem->nr_banks].start = start;
-        mem->bank[mem->nr_banks].size = size;
-        mem->nr_banks++;
-    }
-
-    if ( i < banks )
-        return -ENOSPC;
-    return 0;
+    return device_tree_get_meminfo(fdt, node, "reg", address_cells, size_cells, data);
 }
 
 static int __init process_reserved_memory_node(const void *fdt, int node,
@@ -195,13 +205,7 @@  static int __init process_reserved_memory_node(const void *fdt, int node,
 
 static int __init process_static_memory(const void *fdt, int node, void *data)
 {
-    int i = 0, banks;
-    const __be32 *cell;
-    paddr_t start, size;
-    u32 address_cells, size_cells, reg_cells;
-    struct meminfo *mem = data;
-    const struct fdt_property *prop;
-
+    u32 address_cells, size_cells;
 
     address_cells = device_tree_get_u32(fdt, node,
                                         "#xen,static-mem-address-cells", 0);
@@ -213,33 +217,9 @@  static int __init process_static_memory(const void *fdt, int node, void *data)
                  "\"#xen,static-mem-address-cell\".\n");
          return -EINVAL;
     }
-    reg_cells = address_cells + size_cells;
-
-    prop = fdt_get_property(fdt, node, "xen,static-mem", NULL);
-    /*
-     * Static memory shall belong to a specific domain, that is,
-     * its node `domUx` has compatible string "xen,domain".
-     */
-    if ( fdt_node_check_compatible(fdt, node, "xen,domain") != 0 )
-    {
-        printk("xen,static-mem property can only be located under /domUx node.\n");
-        return -EINVAL;
-    }
-
-    cell = (const __be32 *)prop->data;
-    banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
-
-    for ( ; i < banks && mem->nr_banks < NR_MEM_BANKS; i++ )
-    {
-        device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
-        mem->bank[mem->nr_banks].start = start;
-        mem->bank[mem->nr_banks].size = size;
-        mem->nr_banks++;
-    }
 
-    if ( i < banks )
-        return -ENOSPC;
-    return 0;
+    return device_tree_get_meminfo(fdt, node, "xen,static-mem", address_cells,
+                                   size_cells, data);
 }
 
 static int __init process_reserved_memory(const void *fdt, int node,