| Message ID | 1423058539-26403-22-git-send-email-parth.dixit@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi Parth, On 04/02/2015 14:02, parth.dixit@linaro.org wrote: > From: Naresh Bhat <naresh.bhat@linaro.org> > > Create a memory node for DOM0. I'm not convince it's necessary to have a separate patch for this. I would squash it the #20. > Signed-off-by: Naresh Bhat <naresh.bhat@linaro.org> > --- > xen/arch/arm/domain_build.c | 48 +++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 48 insertions(+) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index bb7f043..30bebe5 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -1155,6 +1155,50 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo, > return res; > } > > +static int make_memory_node_acpi(const struct domain *d, > + void *fdt, > + int addr_cells, > + int size_cells, > + const struct kernel_info *kinfo) > +{ > + int res, i; > + int reg_size = addr_cells + size_cells; > + int nr_cells = reg_size*kinfo->mem.nr_banks; > + __be32 reg[nr_cells]; > + __be32 *cells; > + > + DPRINT("Create memory node (reg size %d, nr cells %d)\n", reg_size, nr_cells); > + > + /* ePAPR 3.4 */ > + res = fdt_begin_node(fdt, "memory"); > + if ( res ) > + return res; > + > + res = fdt_property_string(fdt, "device_type", "memory"); > + if ( res ) > + return res; > + > + cells = ®[0]; > + for ( i = 0 ; i < kinfo->mem.nr_banks; i++ ) > + { > + u64 start = kinfo->mem.bank[i].start; > + u64 size = kinfo->mem.bank[i].size; > + > + DPRINT(" Bank %d: %#"PRIx64"->%#"PRIx64"\n", > + i, start, start + size); > + > + dt_set_range(&cells, fdt, start, size); > + } > + > + res = fdt_property(fdt, "reg", reg, sizeof(reg)); > + if ( res ) > + return res; > + > + res = fdt_end_node(fdt); > + > + return res; > +} > + Why did you duplicate make_memory_node rather than slightly modify the function to support both ACPI and DT version? > /* > * Prepare a minimal DTB for DOM0 which contains > * bootargs, memory information, > @@ -1196,6 +1240,10 @@ static int prepare_dtb_acpi(struct domain *d, struct kernel_info *kinfo) > if ( ret ) > return ret; > > + ret = make_memory_node_acpi(d, kinfo->fdt, 2, 1, kinfo); I forget to mention it on the previous patch. Please add 2 defines for the value 2/1. > + if ( ret ) > + goto err; > + > ret = fdt_end_node(kinfo->fdt); > if ( ret < 0 ) > goto err; > Regards,
Hi Stefano, On 06/02/2015 00:01, Stefano Stabellini wrote: > On Wed, 4 Feb 2015, parth.dixit@linaro.org wrote: >> From: Naresh Bhat <naresh.bhat@linaro.org> >> >> Create a memory node for DOM0. >> >> Signed-off-by: Naresh Bhat <naresh.bhat@linaro.org> >> --- >> xen/arch/arm/domain_build.c | 48 +++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 48 insertions(+) >> >> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >> index bb7f043..30bebe5 100644 >> --- a/xen/arch/arm/domain_build.c >> +++ b/xen/arch/arm/domain_build.c >> @@ -1155,6 +1155,50 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo, >> return res; >> } >> >> +static int make_memory_node_acpi(const struct domain *d, >> + void *fdt, >> + int addr_cells, >> + int size_cells, >> + const struct kernel_info *kinfo) >> +{ >> + int res, i; >> + int reg_size = addr_cells + size_cells; >> + int nr_cells = reg_size*kinfo->mem.nr_banks; >> + __be32 reg[nr_cells]; >> + __be32 *cells; >> + >> + DPRINT("Create memory node (reg size %d, nr cells %d)\n", reg_size, nr_cells); >> + >> + /* ePAPR 3.4 */ >> + res = fdt_begin_node(fdt, "memory"); >> + if ( res ) >> + return res; >> + >> + res = fdt_property_string(fdt, "device_type", "memory"); >> + if ( res ) >> + return res; >> + >> + cells = ®[0]; >> + for ( i = 0 ; i < kinfo->mem.nr_banks; i++ ) >> + { >> + u64 start = kinfo->mem.bank[i].start; >> + u64 size = kinfo->mem.bank[i].size; >> + >> + DPRINT(" Bank %d: %#"PRIx64"->%#"PRIx64"\n", >> + i, start, start + size); >> + >> + dt_set_range(&cells, fdt, start, size); >> + } >> + >> + res = fdt_property(fdt, "reg", reg, sizeof(reg)); >> + if ( res ) >> + return res; >> + >> + res = fdt_end_node(fdt); >> + >> + return res; >> +} > > What's the difference with make_memory_node? Couldn't you just use that > instead? AFAICS, the only difference is the way we get the number of address/size cells. I agree that we should extend make_memory_node to get those number of cells in parameter rather than duplicating the function. Regards,
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index bb7f043..30bebe5 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -1155,6 +1155,50 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo, return res; } +static int make_memory_node_acpi(const struct domain *d, + void *fdt, + int addr_cells, + int size_cells, + const struct kernel_info *kinfo) +{ + int res, i; + int reg_size = addr_cells + size_cells; + int nr_cells = reg_size*kinfo->mem.nr_banks; + __be32 reg[nr_cells]; + __be32 *cells; + + DPRINT("Create memory node (reg size %d, nr cells %d)\n", reg_size, nr_cells); + + /* ePAPR 3.4 */ + res = fdt_begin_node(fdt, "memory"); + if ( res ) + return res; + + res = fdt_property_string(fdt, "device_type", "memory"); + if ( res ) + return res; + + cells = ®[0]; + for ( i = 0 ; i < kinfo->mem.nr_banks; i++ ) + { + u64 start = kinfo->mem.bank[i].start; + u64 size = kinfo->mem.bank[i].size; + + DPRINT(" Bank %d: %#"PRIx64"->%#"PRIx64"\n", + i, start, start + size); + + dt_set_range(&cells, fdt, start, size); + } + + res = fdt_property(fdt, "reg", reg, sizeof(reg)); + if ( res ) + return res; + + res = fdt_end_node(fdt); + + return res; +} + /* * Prepare a minimal DTB for DOM0 which contains * bootargs, memory information, @@ -1196,6 +1240,10 @@ static int prepare_dtb_acpi(struct domain *d, struct kernel_info *kinfo) if ( ret ) return ret; + ret = make_memory_node_acpi(d, kinfo->fdt, 2, 1, kinfo); + if ( ret ) + goto err; + ret = fdt_end_node(kinfo->fdt); if ( ret < 0 ) goto err;