| Message ID | 20200617235131.21374-1-atish.patra@wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] riscv: Use correct version of fdtdec_get_addr_size | expand |
Hi Atish, On Thu, Jun 18, 2020 at 7:51 AM Atish Patra <atish.patra at wdc.com> wrote: > > fdtdec_get_addr_size uses a fixed value for address_cells & size_cells > which may not work correctly always. fdtdec_get_addr_size_no_parent > will automatically calculate the cell sizes from parent but not > optimized especially when parent node is already available with the > caller. > > Use fdtdec_get_addr_size_auto_parent that automatically calculate the > cell sizes and optimized for the given usecase. > > Signed-off-by: Atish Patra <atish.patra at wdc.com> > --- > arch/riscv/lib/fdt_fixup.c | 2 +- > lib/fdtdec.c | 3 ++- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/arch/riscv/lib/fdt_fixup.c b/arch/riscv/lib/fdt_fixup.c > index 6db48ad04a56..f2ec37b57b15 100644 > --- a/arch/riscv/lib/fdt_fixup.c > +++ b/arch/riscv/lib/fdt_fixup.c > @@ -44,7 +44,7 @@ int riscv_fdt_copy_resv_mem_node(const void *src, void *dst) > fdt_for_each_subnode(node, src, offset) { > name = fdt_get_name(src, node, NULL); > > - addr = fdtdec_get_addr_size_auto_noparent(src, node, > + addr = fdtdec_get_addr_size_auto_parent(src, offset, node, > "reg", 0, &size, > false); > if (addr == FDT_ADDR_T_NONE) { > diff --git a/lib/fdtdec.c b/lib/fdtdec.c > index 1f2b763acc31..b62eb142ccc3 100644 > --- a/lib/fdtdec.c > +++ b/lib/fdtdec.c > @@ -1296,7 +1296,8 @@ int fdtdec_add_reserved_memory(void *blob, const char *basename, > const char *name = fdt_get_name(blob, node, NULL); > phys_addr_t addr, size; > > - addr = fdtdec_get_addr_size(blob, node, "reg", &size); > + addr = fdtdec_get_addr_size_auto_parent(blob, parent, node, > + "reg", 0, &size, false); There is already a patch for this change (although slightly different, but fixed the same thing) http://patchwork.ozlabs.org/project/uboot/patch/1591767391-2669-2-git-send-email-bmeng.cn at gmail.com/ Please also send them in separate patches in the future, as one is RISC-V specific and the other one is for generic codes. Regards, Bin
On Wed, Jun 17, 2020 at 5:46 PM Bin Meng <bmeng.cn at gmail.com> wrote: > > Hi Atish, > > On Thu, Jun 18, 2020 at 7:51 AM Atish Patra <atish.patra at wdc.com> wrote: > > > > fdtdec_get_addr_size uses a fixed value for address_cells & size_cells > > which may not work correctly always. fdtdec_get_addr_size_no_parent > > will automatically calculate the cell sizes from parent but not > > optimized especially when parent node is already available with the > > caller. > > > > Use fdtdec_get_addr_size_auto_parent that automatically calculate the > > cell sizes and optimized for the given usecase. > > > > Signed-off-by: Atish Patra <atish.patra at wdc.com> > > --- > > arch/riscv/lib/fdt_fixup.c | 2 +- > > lib/fdtdec.c | 3 ++- > > 2 files changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/arch/riscv/lib/fdt_fixup.c b/arch/riscv/lib/fdt_fixup.c > > index 6db48ad04a56..f2ec37b57b15 100644 > > --- a/arch/riscv/lib/fdt_fixup.c > > +++ b/arch/riscv/lib/fdt_fixup.c > > @@ -44,7 +44,7 @@ int riscv_fdt_copy_resv_mem_node(const void *src, void *dst) > > fdt_for_each_subnode(node, src, offset) { > > name = fdt_get_name(src, node, NULL); > > > > - addr = fdtdec_get_addr_size_auto_noparent(src, node, > > + addr = fdtdec_get_addr_size_auto_parent(src, offset, node, > > "reg", 0, &size, > > false); > > if (addr == FDT_ADDR_T_NONE) { > > diff --git a/lib/fdtdec.c b/lib/fdtdec.c > > index 1f2b763acc31..b62eb142ccc3 100644 > > --- a/lib/fdtdec.c > > +++ b/lib/fdtdec.c > > @@ -1296,7 +1296,8 @@ int fdtdec_add_reserved_memory(void *blob, const char *basename, > > const char *name = fdt_get_name(blob, node, NULL); > > phys_addr_t addr, size; > > > > - addr = fdtdec_get_addr_size(blob, node, "reg", &size); > > + addr = fdtdec_get_addr_size_auto_parent(blob, parent, node, > > + "reg", 0, &size, false); > > There is already a patch for this change (although slightly different, > but fixed the same thing) > http://patchwork.ozlabs.org/project/uboot/patch/1591767391-2669-2-git-send-email-bmeng.cn at gmail.com/ > Ah I missed that. I used this version compared to fdtdec_get_addr_size_fixed because of the following comment in the header file. "You probably don't want to use this function directly except to parse non-standard properties, and never to parse the "reg" property. Instead, use one of the "auto" variants below, which automatically honor the #address-cells and #size-cells properties in the parent node." Do you want to update your patch or I can send v2 by splitting riscv & generic changes ? Please let me know your preference. > Please also send them in separate patches in the future, as one is > RISC-V specific and the other one is for generic codes. > > Regards, > Bin
Hi Atish, On Thu, Jun 18, 2020 at 3:04 PM Atish Patra <atishp at atishpatra.org> wrote: > > On Wed, Jun 17, 2020 at 5:46 PM Bin Meng <bmeng.cn at gmail.com> wrote: > > > > Hi Atish, > > > > On Thu, Jun 18, 2020 at 7:51 AM Atish Patra <atish.patra at wdc.com> wrote: > > > > > > fdtdec_get_addr_size uses a fixed value for address_cells & size_cells > > > which may not work correctly always. fdtdec_get_addr_size_no_parent > > > will automatically calculate the cell sizes from parent but not > > > optimized especially when parent node is already available with the > > > caller. > > > > > > Use fdtdec_get_addr_size_auto_parent that automatically calculate the > > > cell sizes and optimized for the given usecase. > > > > > > Signed-off-by: Atish Patra <atish.patra at wdc.com> > > > --- > > > arch/riscv/lib/fdt_fixup.c | 2 +- > > > lib/fdtdec.c | 3 ++- > > > 2 files changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/arch/riscv/lib/fdt_fixup.c b/arch/riscv/lib/fdt_fixup.c > > > index 6db48ad04a56..f2ec37b57b15 100644 > > > --- a/arch/riscv/lib/fdt_fixup.c > > > +++ b/arch/riscv/lib/fdt_fixup.c > > > @@ -44,7 +44,7 @@ int riscv_fdt_copy_resv_mem_node(const void *src, void *dst) > > > fdt_for_each_subnode(node, src, offset) { > > > name = fdt_get_name(src, node, NULL); > > > > > > - addr = fdtdec_get_addr_size_auto_noparent(src, node, > > > + addr = fdtdec_get_addr_size_auto_parent(src, offset, node, > > > "reg", 0, &size, > > > false); > > > if (addr == FDT_ADDR_T_NONE) { > > > diff --git a/lib/fdtdec.c b/lib/fdtdec.c > > > index 1f2b763acc31..b62eb142ccc3 100644 > > > --- a/lib/fdtdec.c > > > +++ b/lib/fdtdec.c > > > @@ -1296,7 +1296,8 @@ int fdtdec_add_reserved_memory(void *blob, const char *basename, > > > const char *name = fdt_get_name(blob, node, NULL); > > > phys_addr_t addr, size; > > > > > > - addr = fdtdec_get_addr_size(blob, node, "reg", &size); > > > + addr = fdtdec_get_addr_size_auto_parent(blob, parent, node, > > > + "reg", 0, &size, false); > > > > There is already a patch for this change (although slightly different, > > but fixed the same thing) > > http://patchwork.ozlabs.org/project/uboot/patch/1591767391-2669-2-git-send-email-bmeng.cn at gmail.com/ > > > > Ah I missed that. I used this version compared to > fdtdec_get_addr_size_fixed because of the following comment > in the header file. > > "You probably don't want to use this function directly except to parse > non-standard properties, and never to parse the "reg" property. Instead, > use one of the "auto" variants below, which automatically honor the > #address-cells and #size-cells properties in the parent node." > > Do you want to update your patch or I can send v2 by splitting riscv & > generic changes ? > Please let me know your preference. I intended to use fdtdec_get_addr_size_fixed() because na and ns are already known at this point. Calling fdtdec_get_addr_size_auto_parent() duplicates the efforts of getting values of na and ns, and is not necessary. > > > Please also send them in separate patches in the future, as one is > > RISC-V specific and the other one is for generic codes. Regards, Bin
On Thu, Jun 18, 2020 at 12:19 AM Bin Meng <bmeng.cn at gmail.com> wrote: > > Hi Atish, > > On Thu, Jun 18, 2020 at 3:04 PM Atish Patra <atishp at atishpatra.org> wrote: > > > > On Wed, Jun 17, 2020 at 5:46 PM Bin Meng <bmeng.cn at gmail.com> wrote: > > > > > > Hi Atish, > > > > > > On Thu, Jun 18, 2020 at 7:51 AM Atish Patra <atish.patra at wdc.com> wrote: > > > > > > > > fdtdec_get_addr_size uses a fixed value for address_cells & size_cells > > > > which may not work correctly always. fdtdec_get_addr_size_no_parent > > > > will automatically calculate the cell sizes from parent but not > > > > optimized especially when parent node is already available with the > > > > caller. > > > > > > > > Use fdtdec_get_addr_size_auto_parent that automatically calculate the > > > > cell sizes and optimized for the given usecase. > > > > > > > > Signed-off-by: Atish Patra <atish.patra at wdc.com> > > > > --- > > > > arch/riscv/lib/fdt_fixup.c | 2 +- > > > > lib/fdtdec.c | 3 ++- > > > > 2 files changed, 3 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/arch/riscv/lib/fdt_fixup.c b/arch/riscv/lib/fdt_fixup.c > > > > index 6db48ad04a56..f2ec37b57b15 100644 > > > > --- a/arch/riscv/lib/fdt_fixup.c > > > > +++ b/arch/riscv/lib/fdt_fixup.c > > > > @@ -44,7 +44,7 @@ int riscv_fdt_copy_resv_mem_node(const void *src, void *dst) > > > > fdt_for_each_subnode(node, src, offset) { > > > > name = fdt_get_name(src, node, NULL); > > > > > > > > - addr = fdtdec_get_addr_size_auto_noparent(src, node, > > > > + addr = fdtdec_get_addr_size_auto_parent(src, offset, node, > > > > "reg", 0, &size, > > > > false); > > > > if (addr == FDT_ADDR_T_NONE) { > > > > diff --git a/lib/fdtdec.c b/lib/fdtdec.c > > > > index 1f2b763acc31..b62eb142ccc3 100644 > > > > --- a/lib/fdtdec.c > > > > +++ b/lib/fdtdec.c > > > > @@ -1296,7 +1296,8 @@ int fdtdec_add_reserved_memory(void *blob, const char *basename, > > > > const char *name = fdt_get_name(blob, node, NULL); > > > > phys_addr_t addr, size; > > > > > > > > - addr = fdtdec_get_addr_size(blob, node, "reg", &size); > > > > + addr = fdtdec_get_addr_size_auto_parent(blob, parent, node, > > > > + "reg", 0, &size, false); > > > > > > There is already a patch for this change (although slightly different, > > > but fixed the same thing) > > > http://patchwork.ozlabs.org/project/uboot/patch/1591767391-2669-2-git-send-email-bmeng.cn at gmail.com/ > > > > > > > Ah I missed that. I used this version compared to > > fdtdec_get_addr_size_fixed because of the following comment > > in the header file. > > > > "You probably don't want to use this function directly except to parse > > non-standard properties, and never to parse the "reg" property. Instead, > > use one of the "auto" variants below, which automatically honor the > > #address-cells and #size-cells properties in the parent node." > > > > Do you want to update your patch or I can send v2 by splitting riscv & > > generic changes ? > > Please let me know your preference. > > I intended to use fdtdec_get_addr_size_fixed() because na and ns are > already known at this point. Calling > fdtdec_get_addr_size_auto_parent() duplicates the efforts of getting > values of na and ns, and is not necessary. > Yes. But the duplication effort is just reading two parameters. Anyways, it's not a big deal. I will drop the generic fix from my patch and resent after rebasing on top of your series. > > > > > Please also send them in separate patches in the future, as one is > > > RISC-V specific and the other one is for generic codes. > > Regards, > Bin
diff --git a/arch/riscv/lib/fdt_fixup.c b/arch/riscv/lib/fdt_fixup.c index 6db48ad04a56..f2ec37b57b15 100644 --- a/arch/riscv/lib/fdt_fixup.c +++ b/arch/riscv/lib/fdt_fixup.c @@ -44,7 +44,7 @@ int riscv_fdt_copy_resv_mem_node(const void *src, void *dst) fdt_for_each_subnode(node, src, offset) { name = fdt_get_name(src, node, NULL); - addr = fdtdec_get_addr_size_auto_noparent(src, node, + addr = fdtdec_get_addr_size_auto_parent(src, offset, node, "reg", 0, &size, false); if (addr == FDT_ADDR_T_NONE) { diff --git a/lib/fdtdec.c b/lib/fdtdec.c index 1f2b763acc31..b62eb142ccc3 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -1296,7 +1296,8 @@ int fdtdec_add_reserved_memory(void *blob, const char *basename, const char *name = fdt_get_name(blob, node, NULL); phys_addr_t addr, size; - addr = fdtdec_get_addr_size(blob, node, "reg", &size); + addr = fdtdec_get_addr_size_auto_parent(blob, parent, node, + "reg", 0, &size, false); if (addr == FDT_ADDR_T_NONE) { debug("failed to read address/size for %s\n", name); continue;
fdtdec_get_addr_size uses a fixed value for address_cells & size_cells which may not work correctly always. fdtdec_get_addr_size_no_parent will automatically calculate the cell sizes from parent but not optimized especially when parent node is already available with the caller. Use fdtdec_get_addr_size_auto_parent that automatically calculate the cell sizes and optimized for the given usecase. Signed-off-by: Atish Patra <atish.patra at wdc.com> --- arch/riscv/lib/fdt_fixup.c | 2 +- lib/fdtdec.c | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-)