On 10/09/18 18:14, AKASHI, Takahiro wrote:

Frank, Rob,

On Tue, Oct 09, 2018 at 10:47:15AM -0700, Frank Rowand wrote:

quoted

On 10/08/18 17:37, AKASHI, Takahiro wrote:

quoted

On Fri, Oct 05, 2018 at 08:23:57AM -0500, Rob Herring wrote:

quoted

On Thu, Oct 4, 2018 at 10:07 PM AKASHI, Takahiro
[off-list ref] wrote:

quoted

Rob,

# I haven't replied to this comment yet.

On Fri, Sep 28, 2018 at 08:44:42AM -0500, Rob Herring wrote:

quoted

+David Gibson

On Fri, Sep 28, 2018 at 1:48 AM AKASHI Takahiro
[off-list ref] wrote:

quoted

These functions will be used later to handle kexec-specific properties
in arm64's kexec_file implementation.

Signed-off-by: AKASHI Takahiro <redacted>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <redacted>
Cc: devicetree at vger.kernel.org
---
 drivers/of/fdt.c       | 56 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_fdt.h |  4 +++
 2 files changed, 60 insertions(+)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 800ad252cf9c..c65c31562ccb 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c

@@ -25,6 +25,7 @@
 #include <linux/debugfs.h>
 #include <linux/serial_core.h>
 #include <linux/sysfs.h>
+#include <linux/types.h>

 #include <asm/setup.h>  /* for COMMAND_LINE_SIZE */
 #include <asm/page.h>

@@ -1323,3 +1324,58 @@ late_initcall(of_fdt_raw_init);
 #endif

 #endif /* CONFIG_OF_EARLY_FLATTREE */
+
+#define FDT_ALIGN(x, a)        (((x) + (a) - 1) & ~((a) - 1))
+#define FDT_TAGALIGN(x)        (FDT_ALIGN((x), FDT_TAGSIZE))
+
+int fdt_prop_len(const char *prop_name, int len)
+{
+       return (strlen(prop_name) + 1) +
+               sizeof(struct fdt_property) +
+               FDT_TAGALIGN(len);

Looks like you are using this to calculate how much space you need to
allocate in addition to the current DTB for a couple of new or
replaced properties. I'm not sure that this calculation is completely
accurate. And it is strange there doesn't seem to be any libfdt
function for this already. It would be simpler to just add some fixed
additional amount.

Maybe David G has comments on this?

quoted

+}
+

The rest of this should go in drivers/of/fdt_address.c. Ultimately, it
should go into libfdt, but I'm fine with having it in the kernel for
now.

I'd like to have this function in the kernel for now.

quoted

quoted

+static void fill_property(void *buf, u64 val64, int cells)
+{
+       __be32 val32;
+
+       while (cells) {
+               val32 = cpu_to_fdt32((val64 >> (32 * (--cells))) & U32_MAX);
+               memcpy(buf, &val32, sizeof(val32));
+               buf += sizeof(val32);

This is kind of hard to read. I would copy u-boot's fdt_pack_reg function.

Are you sure?
I originally implemented this function in a similar way that fdt_pack_reg()
was, but, you suggested, in your past comment[1], that we'd be better to
have of_read_number()-like implementation.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-May/579118.html

Yeah, you're right. Plus, I'm not sure the u-boot one would work for
unaligned accesses with armv5 and earlier h/w.

My only comment then is I think you can drop the U32_MAX masking.

Okay, then I will leave this function, yet renaming it to
cpu64_to_fdt_cells() after Frank's comment.

I have second guessed myself and do not like the name I suggested
because what the function really does is either cpu32 to be32 or
cpu64 to be64.

Okay.

quoted

I agree with Rob that readability is important here.  Instead of
having a fill_property() function, how about having inline code,
something like (untested even for thinkos):

	prop = buf;

	if (addr_cells == 1) {
		*(__be32 *)prop = cpu32_to_be32(addr);
		prop += 4;
	} else {
		*(__be64 *)prop = cpu64_to_be64(addr);
		prop += 8;
	}

	if (size_cells == 1)
		*(__be32 *)prop = cpu32_to_be32(size);
	else
		*(__be64 *)prop = cpu64_to_be64(size);

You might want to also give Rob a chance to bike shed on this
suggestion.

Basically, I don't care either way, but what Rob suggested
is that some architecture(s) might not handle correctly
unaligned memory access here.

Good point about unaligned memory access.  That rules out my
suggestion above.  But if using the fill_property() implementation,
either come up with a name that better describes what the function
does or add a function header comment that gives the reader a
clue as to what the function accomplishes.

-Frank

I just want to stay tuned with Rob.

Thanks,
-Takahiro Akashi

quoted

-Frank

quoted

Thanks,
-Takahiro Akashi

quoted

Rob