Re: [U-Boot] [PATCH 03/12] libfdt: Safer access to strings section
On Tue, Apr 17, 2018 at 12:03:19PM +0900, Masahiro Yamada wrote: > 2018-04-14 23:42 GMT+09:00 Warner Losh <i...@bsdimp.com>: > > On Fri, Apr 13, 2018 at 9:43 PM, David Gibson <da...@gibson.dropbear.id.au> > > wrote: > > > >> On Fri, Apr 13, 2018 at 12:53:19PM -0400, Tom Rini wrote: > >> > On Thu, Apr 12, 2018 at 02:39:19PM +1000, David Gibson wrote: > >> > > On Tue, Apr 10, 2018 at 10:42:45AM -0400, Simon Glass wrote: > >> > > > +U-Boot, Tom, Masahiro > >> > > > > >> > > > Hi David, > >> > > > > >> > > > On 10 April 2018 at 01:22, David Gibson <da...@gibson.dropbear.id.au> > >> wrote: > >> > > > > On Wed, Apr 04, 2018 at 01:21:10AM +0800, Simon Glass wrote: > >> > > > >> Hi David, > >> > > > >> > >> > > > >> On 3 April 2018 at 23:02, David Gibson < > >> da...@gibson.dropbear.id.au> wrote: > >> > > > >> > > >> > > > >> > On Fri, Mar 30, 2018 at 04:42:21PM +0800, Simon Glass wrote: > >> > > > >> > > Hi David, > >> > > > >> > > > >> > > > >> > > On 26 March 2018 at 07:25, David Gibson < > >> da...@gibson.dropbear.id.au> wrote: > >> > > > >> > > > fdt_string() is used to retrieve strings from a DT blob's > >> strings section. > >> > > > >> > > > It's rarely used directly, but is widely used internally. > >> > > > >> > > > > >> > > > >> > > > However, it doesn't do any bounds checking, which means in > >> the case of a > >> > > > >> > > > corrupted blob it could access bad memory, which libfdt is > >> supposed to > >> > > > >> > > > avoid. > >> > > > >> > > > > >> > > > >> > > > This write a safe alternative to fdt_string, > >> fdt_get_string(). It checks > >> > > > >> > > > both that the given offset is within the string section and > >> that the string > >> > > > >> > > > it points to is properly \0 terminated within the section. > >> It also returns > >> > > > >> > > > the string's length as a convenience (since it needs to > >> determine to do the > >> > > > >> > > > checks anyway). > >> > > > >> > > > > >> > > > >> > > > fdt_string() is rewritten in terms of fdt_get_string() for > >> compatibility. > >> > > > >> > > > > >> > > > >> > > > Most of the diff here is actually testing infrastructure. > >> > > > >> > > > > >> > > > >> > > > Signed-off-by: David Gibson <da...@gibson.dropbear.id.au> > >> > > > >> > > > --- > >> > > > >> > > > libfdt/fdt_ro.c | 61 > >> +++-- > >> > > > >> > > > libfdt/libfdt.h | 18 ++- > >> > > > >> > > > libfdt/version.lds | 2 +- > >> > > > >> > > > tests/.gitignore | 1 + > >> > > > >> > > > tests/Makefile.tests | 2 +- > >> > > > >> > > > tests/run_tests.sh | 1 + > >> > > > >> > > > tests/testdata.h | 1 + > >> > > > >> > > > tests/testutils.c| 11 +-- > >> > > > >> > > > tests/trees.S| 26 > >> > > > >> > > > tests/truncated_string.c | 79 > >> > >> > > > >> > > > 10 files changed, 193 insertions(+), 9 deletions(-) > >> > > > >> > > > create mode 100644 tests/truncated_string.c > >> > > > >> > > > >> > > > >> > > Similar code-size quesiton here. It looks like a lot of > >> checking code. > >> > > > >> > > Can we have an option to remove it? > >> > > > >> > > >> > > > >> > Again, I'm disinclined without a
Re: [U-Boot] [PATCH 03/12] libfdt: Safer access to strings section
On Sat, Apr 14, 2018 at 08:42:23AM -0600, Warner Losh wrote: > On Fri, Apr 13, 2018 at 9:43 PM, David Gibson <da...@gibson.dropbear.id.au> > wrote: > > > On Fri, Apr 13, 2018 at 12:53:19PM -0400, Tom Rini wrote: > > > On Thu, Apr 12, 2018 at 02:39:19PM +1000, David Gibson wrote: > > > > On Tue, Apr 10, 2018 at 10:42:45AM -0400, Simon Glass wrote: > > > > > +U-Boot, Tom, Masahiro > > > > > > > > > > Hi David, > > > > > > > > > > On 10 April 2018 at 01:22, David Gibson <da...@gibson.dropbear.id.au> > > wrote: > > > > > > On Wed, Apr 04, 2018 at 01:21:10AM +0800, Simon Glass wrote: > > > > > >> Hi David, > > > > > >> > > > > > >> On 3 April 2018 at 23:02, David Gibson < > > da...@gibson.dropbear.id.au> wrote: > > > > > >> > > > > > > >> > On Fri, Mar 30, 2018 at 04:42:21PM +0800, Simon Glass wrote: > > > > > >> > > Hi David, > > > > > >> > > > > > > > >> > > On 26 March 2018 at 07:25, David Gibson < > > da...@gibson.dropbear.id.au> wrote: > > > > > >> > > > fdt_string() is used to retrieve strings from a DT blob's > > strings section. > > > > > >> > > > It's rarely used directly, but is widely used internally. > > > > > >> > > > > > > > > >> > > > However, it doesn't do any bounds checking, which means in > > the case of a > > > > > >> > > > corrupted blob it could access bad memory, which libfdt is > > supposed to > > > > > >> > > > avoid. > > > > > >> > > > > > > > > >> > > > This write a safe alternative to fdt_string, > > fdt_get_string(). It checks > > > > > >> > > > both that the given offset is within the string section and > > that the string > > > > > >> > > > it points to is properly \0 terminated within the section. > > It also returns > > > > > >> > > > the string's length as a convenience (since it needs to > > determine to do the > > > > > >> > > > checks anyway). > > > > > >> > > > > > > > > >> > > > fdt_string() is rewritten in terms of fdt_get_string() for > > compatibility. > > > > > >> > > > > > > > > >> > > > Most of the diff here is actually testing infrastructure. > > > > > >> > > > > > > > > >> > > > Signed-off-by: David Gibson <da...@gibson.dropbear.id.au> > > > > > >> > > > --- > > > > > >> > > > libfdt/fdt_ro.c | 61 > > +++-- > > > > > >> > > > libfdt/libfdt.h | 18 ++- > > > > > >> > > > libfdt/version.lds | 2 +- > > > > > >> > > > tests/.gitignore | 1 + > > > > > >> > > > tests/Makefile.tests | 2 +- > > > > > >> > > > tests/run_tests.sh | 1 + > > > > > >> > > > tests/testdata.h | 1 + > > > > > >> > > > tests/testutils.c| 11 +-- > > > > > >> > > > tests/trees.S| 26 > > > > > >> > > > tests/truncated_string.c | 79 > > > > > > > >> > > > 10 files changed, 193 insertions(+), 9 deletions(-) > > > > > >> > > > create mode 100644 tests/truncated_string.c > > > > > >> > > > > > > > >> > > Similar code-size quesiton here. It looks like a lot of > > checking code. > > > > > >> > > Can we have an option to remove it? > > > > > >> > > > > > > >> > Again, I'm disinclined without a concrete example of a > > problem. Fwiw > > > > > >> > the code size change is +276 bytes on my setup. > > > > > >> > > > > > >> That might not sound like a lot, but the overhead of DT in U-Boot > > is > > > > > >> about 3KB, so this adds nearly 10%. >
Re: [U-Boot] [PATCH 03/12] libfdt: Safer access to strings section
On Fri, Apr 13, 2018 at 12:53:19PM -0400, Tom Rini wrote: > On Thu, Apr 12, 2018 at 02:39:19PM +1000, David Gibson wrote: > > On Tue, Apr 10, 2018 at 10:42:45AM -0400, Simon Glass wrote: > > > +U-Boot, Tom, Masahiro > > > > > > Hi David, > > > > > > On 10 April 2018 at 01:22, David Gibson <da...@gibson.dropbear.id.au> > > > wrote: > > > > On Wed, Apr 04, 2018 at 01:21:10AM +0800, Simon Glass wrote: > > > >> Hi David, > > > >> > > > >> On 3 April 2018 at 23:02, David Gibson <da...@gibson.dropbear.id.au> > > > >> wrote: > > > >> > > > > >> > On Fri, Mar 30, 2018 at 04:42:21PM +0800, Simon Glass wrote: > > > >> > > Hi David, > > > >> > > > > > >> > > On 26 March 2018 at 07:25, David Gibson > > > >> > > <da...@gibson.dropbear.id.au> wrote: > > > >> > > > fdt_string() is used to retrieve strings from a DT blob's > > > >> > > > strings section. > > > >> > > > It's rarely used directly, but is widely used internally. > > > >> > > > > > > >> > > > However, it doesn't do any bounds checking, which means in the > > > >> > > > case of a > > > >> > > > corrupted blob it could access bad memory, which libfdt is > > > >> > > > supposed to > > > >> > > > avoid. > > > >> > > > > > > >> > > > This write a safe alternative to fdt_string, fdt_get_string(). > > > >> > > > It checks > > > >> > > > both that the given offset is within the string section and that > > > >> > > > the string > > > >> > > > it points to is properly \0 terminated within the section. It > > > >> > > > also returns > > > >> > > > the string's length as a convenience (since it needs to > > > >> > > > determine to do the > > > >> > > > checks anyway). > > > >> > > > > > > >> > > > fdt_string() is rewritten in terms of fdt_get_string() for > > > >> > > > compatibility. > > > >> > > > > > > >> > > > Most of the diff here is actually testing infrastructure. > > > >> > > > > > > >> > > > Signed-off-by: David Gibson <da...@gibson.dropbear.id.au> > > > >> > > > --- > > > >> > > > libfdt/fdt_ro.c | 61 > > > >> > > > +++-- > > > >> > > > libfdt/libfdt.h | 18 ++- > > > >> > > > libfdt/version.lds | 2 +- > > > >> > > > tests/.gitignore | 1 + > > > >> > > > tests/Makefile.tests | 2 +- > > > >> > > > tests/run_tests.sh | 1 + > > > >> > > > tests/testdata.h | 1 + > > > >> > > > tests/testutils.c| 11 +-- > > > >> > > > tests/trees.S| 26 > > > >> > > > tests/truncated_string.c | 79 > > > >> > > > > > > >> > > > 10 files changed, 193 insertions(+), 9 deletions(-) > > > >> > > > create mode 100644 tests/truncated_string.c > > > >> > > > > > >> > > Similar code-size quesiton here. It looks like a lot of checking > > > >> > > code. > > > >> > > Can we have an option to remove it? > > > >> > > > > >> > Again, I'm disinclined without a concrete example of a problem. Fwiw > > > >> > the code size change is +276 bytes on my setup. > > > >> > > > >> That might not sound like a lot, but the overhead of DT in U-Boot is > > > >> about 3KB, so this adds nearly 10%. > > > > > > > > Hm. And how much is it compared to the whole U-Boot blob? > > > > > > > >> The specific problem is that when U-Boot SPL gets too big boards don't > > > >> boot. Because we take the upstream libfdt this will affect U-Boot. > > > >> > > > >> Do you have any thoughts on how we c
Re: [U-Boot] [PATCH 03/12] libfdt: Safer access to strings section
On Thu, Apr 12, 2018 at 03:00:17PM -0400, Tom Rini wrote: > On Thu, Apr 12, 2018 at 02:01:05PM +1000, David Gibson wrote: > > On Tue, Apr 10, 2018 at 06:36:06PM -0400, Tom Rini wrote: > > > On Tue, Apr 10, 2018 at 10:42:45AM -0400, Simon Glass wrote: > > > > +U-Boot, Tom, Masahiro > > > > > > > > Hi David, > > > > > > > > On 10 April 2018 at 01:22, David Gibson <da...@gibson.dropbear.id.au> > > > > wrote: > > > > > On Wed, Apr 04, 2018 at 01:21:10AM +0800, Simon Glass wrote: > > > > >> Hi David, > > > > >> > > > > >> On 3 April 2018 at 23:02, David Gibson <da...@gibson.dropbear.id.au> > > > > >> wrote: > > > > >> > > > > > >> > On Fri, Mar 30, 2018 at 04:42:21PM +0800, Simon Glass wrote: > > > > >> > > Hi David, > > > > >> > > > > > > >> > > On 26 March 2018 at 07:25, David Gibson > > > > >> > > <da...@gibson.dropbear.id.au> wrote: > > > > >> > > > fdt_string() is used to retrieve strings from a DT blob's > > > > >> > > > strings section. > > > > >> > > > It's rarely used directly, but is widely used internally. > > > > >> > > > > > > > >> > > > However, it doesn't do any bounds checking, which means in the > > > > >> > > > case of a > > > > >> > > > corrupted blob it could access bad memory, which libfdt is > > > > >> > > > supposed to > > > > >> > > > avoid. > > > > >> > > > > > > > >> > > > This write a safe alternative to fdt_string, fdt_get_string(). > > > > >> > > > It checks > > > > >> > > > both that the given offset is within the string section and > > > > >> > > > that the string > > > > >> > > > it points to is properly \0 terminated within the section. It > > > > >> > > > also returns > > > > >> > > > the string's length as a convenience (since it needs to > > > > >> > > > determine to do the > > > > >> > > > checks anyway). > > > > >> > > > > > > > >> > > > fdt_string() is rewritten in terms of fdt_get_string() for > > > > >> > > > compatibility. > > > > >> > > > > > > > >> > > > Most of the diff here is actually testing infrastructure. > > > > >> > > > > > > > >> > > > Signed-off-by: David Gibson <da...@gibson.dropbear.id.au> > > > > >> > > > --- > > > > >> > > > libfdt/fdt_ro.c | 61 > > > > >> > > > +++-- > > > > >> > > > libfdt/libfdt.h | 18 ++- > > > > >> > > > libfdt/version.lds | 2 +- > > > > >> > > > tests/.gitignore | 1 + > > > > >> > > > tests/Makefile.tests | 2 +- > > > > >> > > > tests/run_tests.sh | 1 + > > > > >> > > > tests/testdata.h | 1 + > > > > >> > > > tests/testutils.c| 11 +-- > > > > >> > > > tests/trees.S| 26 > > > > >> > > > tests/truncated_string.c | 79 > > > > >> > > > > > > > >> > > > 10 files changed, 193 insertions(+), 9 deletions(-) > > > > >> > > > create mode 100644 tests/truncated_string.c > > > > >> > > > > > > >> > > Similar code-size quesiton here. It looks like a lot of checking > > > > >> > > code. > > > > >> > > Can we have an option to remove it? > > > > >> > > > > > >> > Again, I'm disinclined without a concrete example of a problem. > > > > >> > Fwiw > > > > >> > the code size change is +276 bytes on my setup. > > > > >> > > > > >> That might not sound like a lot, but the overhead of DT in U-Boot is >
Re: [U-Boot] [PATCH 03/12] libfdt: Safer access to strings section
On Tue, Apr 10, 2018 at 10:42:45AM -0400, Simon Glass wrote: > +U-Boot, Tom, Masahiro > > Hi David, > > On 10 April 2018 at 01:22, David Gibson <da...@gibson.dropbear.id.au> wrote: > > On Wed, Apr 04, 2018 at 01:21:10AM +0800, Simon Glass wrote: > >> Hi David, > >> > >> On 3 April 2018 at 23:02, David Gibson <da...@gibson.dropbear.id.au> wrote: > >> > > >> > On Fri, Mar 30, 2018 at 04:42:21PM +0800, Simon Glass wrote: > >> > > Hi David, > >> > > > >> > > On 26 March 2018 at 07:25, David Gibson <da...@gibson.dropbear.id.au> > >> > > wrote: > >> > > > fdt_string() is used to retrieve strings from a DT blob's strings > >> > > > section. > >> > > > It's rarely used directly, but is widely used internally. > >> > > > > >> > > > However, it doesn't do any bounds checking, which means in the case > >> > > > of a > >> > > > corrupted blob it could access bad memory, which libfdt is supposed > >> > > > to > >> > > > avoid. > >> > > > > >> > > > This write a safe alternative to fdt_string, fdt_get_string(). It > >> > > > checks > >> > > > both that the given offset is within the string section and that the > >> > > > string > >> > > > it points to is properly \0 terminated within the section. It also > >> > > > returns > >> > > > the string's length as a convenience (since it needs to determine to > >> > > > do the > >> > > > checks anyway). > >> > > > > >> > > > fdt_string() is rewritten in terms of fdt_get_string() for > >> > > > compatibility. > >> > > > > >> > > > Most of the diff here is actually testing infrastructure. > >> > > > > >> > > > Signed-off-by: David Gibson <da...@gibson.dropbear.id.au> > >> > > > --- > >> > > > libfdt/fdt_ro.c | 61 +++-- > >> > > > libfdt/libfdt.h | 18 ++- > >> > > > libfdt/version.lds | 2 +- > >> > > > tests/.gitignore | 1 + > >> > > > tests/Makefile.tests | 2 +- > >> > > > tests/run_tests.sh | 1 + > >> > > > tests/testdata.h | 1 + > >> > > > tests/testutils.c| 11 +-- > >> > > > tests/trees.S| 26 > >> > > > tests/truncated_string.c | 79 > >> > > > > >> > > > 10 files changed, 193 insertions(+), 9 deletions(-) > >> > > > create mode 100644 tests/truncated_string.c > >> > > > >> > > Similar code-size quesiton here. It looks like a lot of checking code. > >> > > Can we have an option to remove it? > >> > > >> > Again, I'm disinclined without a concrete example of a problem. Fwiw > >> > the code size change is +276 bytes on my setup. > >> > >> That might not sound like a lot, but the overhead of DT in U-Boot is > >> about 3KB, so this adds nearly 10%. > > > > Hm. And how much is it compared to the whole U-Boot blob? > > > >> The specific problem is that when U-Boot SPL gets too big boards don't > >> boot. Because we take the upstream libfdt this will affect U-Boot. > >> > >> Do you have any thoughts on how we could avoid this size increase? > > > > So, again, I'm very disinclined to prioritize size over memory safety > > without a *concrete* example. i.e. "We hit this specific problem with > > size on this specific board that we were really using" rather than > > just "it might be a problem". > > > > IMO, thinking of it in terms of the "increase" is the wrong way > > arond. If size is really a problem for you, you want to consider how > > you can reduce it in any way, not just rolling back the most recent > > changes. The most obvious one to me would be to try > > -ffunction-sections to exclude any functions that aren't actually used > > by u-boot (if this is helpful and the compiler's an issue, I'd be > > willing to consider splitting up libfdt into a bunch more C files). > > Actually U-Boot does use that option. Believe me, a lo
Re: [U-Boot] [PATCH 03/12] libfdt: Safer access to strings section
On Tue, Apr 10, 2018 at 06:36:06PM -0400, Tom Rini wrote: > On Tue, Apr 10, 2018 at 10:42:45AM -0400, Simon Glass wrote: > > +U-Boot, Tom, Masahiro > > > > Hi David, > > > > On 10 April 2018 at 01:22, David Gibson <da...@gibson.dropbear.id.au> wrote: > > > On Wed, Apr 04, 2018 at 01:21:10AM +0800, Simon Glass wrote: > > >> Hi David, > > >> > > >> On 3 April 2018 at 23:02, David Gibson <da...@gibson.dropbear.id.au> > > >> wrote: > > >> > > > >> > On Fri, Mar 30, 2018 at 04:42:21PM +0800, Simon Glass wrote: > > >> > > Hi David, > > >> > > > > >> > > On 26 March 2018 at 07:25, David Gibson > > >> > > <da...@gibson.dropbear.id.au> wrote: > > >> > > > fdt_string() is used to retrieve strings from a DT blob's strings > > >> > > > section. > > >> > > > It's rarely used directly, but is widely used internally. > > >> > > > > > >> > > > However, it doesn't do any bounds checking, which means in the > > >> > > > case of a > > >> > > > corrupted blob it could access bad memory, which libfdt is > > >> > > > supposed to > > >> > > > avoid. > > >> > > > > > >> > > > This write a safe alternative to fdt_string, fdt_get_string(). It > > >> > > > checks > > >> > > > both that the given offset is within the string section and that > > >> > > > the string > > >> > > > it points to is properly \0 terminated within the section. It > > >> > > > also returns > > >> > > > the string's length as a convenience (since it needs to determine > > >> > > > to do the > > >> > > > checks anyway). > > >> > > > > > >> > > > fdt_string() is rewritten in terms of fdt_get_string() for > > >> > > > compatibility. > > >> > > > > > >> > > > Most of the diff here is actually testing infrastructure. > > >> > > > > > >> > > > Signed-off-by: David Gibson <da...@gibson.dropbear.id.au> > > >> > > > --- > > >> > > > libfdt/fdt_ro.c | 61 > > >> > > > +++-- > > >> > > > libfdt/libfdt.h | 18 ++- > > >> > > > libfdt/version.lds | 2 +- > > >> > > > tests/.gitignore | 1 + > > >> > > > tests/Makefile.tests | 2 +- > > >> > > > tests/run_tests.sh | 1 + > > >> > > > tests/testdata.h | 1 + > > >> > > > tests/testutils.c| 11 +-- > > >> > > > tests/trees.S| 26 > > >> > > > tests/truncated_string.c | 79 > > >> > > > > > >> > > > 10 files changed, 193 insertions(+), 9 deletions(-) > > >> > > > create mode 100644 tests/truncated_string.c > > >> > > > > >> > > Similar code-size quesiton here. It looks like a lot of checking > > >> > > code. > > >> > > Can we have an option to remove it? > > >> > > > >> > Again, I'm disinclined without a concrete example of a problem. Fwiw > > >> > the code size change is +276 bytes on my setup. > > >> > > >> That might not sound like a lot, but the overhead of DT in U-Boot is > > >> about 3KB, so this adds nearly 10%. > > > > > > Hm. And how much is it compared to the whole U-Boot blob? > > > > > >> The specific problem is that when U-Boot SPL gets too big boards don't > > >> boot. Because we take the upstream libfdt this will affect U-Boot. > > >> > > >> Do you have any thoughts on how we could avoid this size increase? > > > > > > So, again, I'm very disinclined to prioritize size over memory safety > > > without a *concrete* example. i.e. "We hit this specific problem with > > > size on this specific board that we were really using" rather than > > > just "it might be a problem". > > I'm either failing in my Google-fu or is there not an easy way to grab > the patches from patchwork/similar? But, if you shoot me the series > off-list, I can tell you how much U-Boot targets grow here (we can use > the same script as the kernel to re-sync sources back in, so I can give > you a before/after). But as Simon notes, we have a number of platforms > that need to use (parts of) libfdt and stick to ~30KiB or less in total, > sometimes including some memory for stack/etc and we've long been using > -ffunction-sections/etc (the latest round of trying to use LTO has me > thinking maybe we can see if that's a valid option finally, but that's > an aside). Thanks! We don't have a patchwork for these lists AFAIK, but you can get my draft branch from: https://github.com/dgibson/dtc/tree/safety -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v4 09/13] libfdt: Add fdt_getprop_namelen_w
On Mon, Jul 11, 2016 at 09:12:27AM +0200, Maxime Ripard wrote:
> On Wed, Jul 06, 2016 at 11:22:48AM +1000, David Gibson wrote:
> > On Tue, Jul 05, 2016 at 10:26:42AM +0200, Maxime Ripard wrote:
> > > Add a function to retrieve a writeable property only by the first
> > > characters of its name.
> > >
> > > Signed-off-by: Maxime Ripard <maxime.rip...@free-electrons.com>
> >
> > This shouldn't be exported, so it should go into libfdt_internal.h.
> >
> > > ---
> > > include/libfdt.h | 7 +++
> > > 1 file changed, 7 insertions(+)
> > >
> > > diff --git a/include/libfdt.h b/include/libfdt.h
> > > index f13b01f08f71..a55d2d0d8c7b 100644
> > > --- a/include/libfdt.h
> > > +++ b/include/libfdt.h
> > > @@ -619,6 +619,13 @@ const void *fdt_getprop_by_offset(const void *fdt,
> > > int offset,
> > > */
> > > const void *fdt_getprop_namelen(const void *fdt, int nodeoffset,
> > > const char *name, int namelen, int *lenp);
> > > +static inline void *fdt_getprop_namelen_w(void *fdt, int nodeoffset,
> > > + const char *name, int namelen,
> > > + int *lenp)
> > > +{
> > > + return (void *)(uintptr_t)fdt_getprop_namelen(fdt, nodeoffset, name,
> > > + namelen, lenp);
> >
> > uintptr_t ??
>
> This is defined in the exact same way than fdt_getprop_w. Should I
> change that as well?
Good point. I wonder why I did that in the first place. I suspect it
was to stop sparse whinging about the removed const. It's ok for now,
we can clean both up later if we get a better idea.
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 09/13] libfdt: Add fdt_getprop_namelen_w
On Tue, Jul 05, 2016 at 10:26:42AM +0200, Maxime Ripard wrote:
> Add a function to retrieve a writeable property only by the first
> characters of its name.
>
> Signed-off-by: Maxime Ripard <maxime.rip...@free-electrons.com>
This shouldn't be exported, so it should go into libfdt_internal.h.
> ---
> include/libfdt.h | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/include/libfdt.h b/include/libfdt.h
> index f13b01f08f71..a55d2d0d8c7b 100644
> --- a/include/libfdt.h
> +++ b/include/libfdt.h
> @@ -619,6 +619,13 @@ const void *fdt_getprop_by_offset(const void *fdt, int
> offset,
> */
> const void *fdt_getprop_namelen(const void *fdt, int nodeoffset,
> const char *name, int namelen, int *lenp);
> +static inline void *fdt_getprop_namelen_w(void *fdt, int nodeoffset,
> + const char *name, int namelen,
> + int *lenp)
> +{
> + return (void *)(uintptr_t)fdt_getprop_namelen(fdt, nodeoffset, name,
> + namelen, lenp);
uintptr_t ??
> +}
>
> /**
> * fdt_getprop - retrieve the value of a given property
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 07/13] libfdt: Fix separator spelling
On Tue, Jul 05, 2016 at 10:26:40AM +0200, Maxime Ripard wrote:
> The function fdt_path_next_seperator had an obvious mispell. Fix it.
>
> Signed-off-by: Maxime Ripard <maxime.rip...@free-electrons.com>
Huh.. this entire function appears not to be in upstream libfdt.
> ---
> lib/libfdt/fdt_ro.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/lib/libfdt/fdt_ro.c b/lib/libfdt/fdt_ro.c
> index 503150ef1dc5..6b737b211d2e 100644
> --- a/lib/libfdt/fdt_ro.c
> +++ b/lib/libfdt/fdt_ro.c
> @@ -140,12 +140,12 @@ int fdt_subnode_offset(const void *fdt, int
> parentoffset,
> }
>
> /*
> - * Find the next of path seperator, note we need to search for both '/' and
> ':'
> + * Find the next of path separator, note we need to search for both '/' and
> ':'
> * and then take the first one so that we do the right thing for e.g.
> * "foo/bar:option" and "bar:option/otheroption", both of which happen, so
> * first searching for either ':' or '/' does not work.
> */
> -static const char *fdt_path_next_seperator(const char *path)
> +static const char *fdt_path_next_separator(const char *path)
> {
> const char *sep1 = strchr(path, '/');
> const char *sep2 = strchr(path, ':');
> @@ -168,7 +168,7 @@ int fdt_path_offset(const void *fdt, const char *path)
>
> /* see if we have an alias */
> if (*path != '/') {
> - const char *q = fdt_path_next_seperator(path);
> + const char *q = fdt_path_next_separator(path);
>
> if (!q)
> q = end;
> @@ -188,7 +188,7 @@ int fdt_path_offset(const void *fdt, const char *path)
> p++;
> if (*p == '\0' || *p == ':')
> return offset;
> - q = fdt_path_next_seperator(p);
> + q = fdt_path_next_separator(p);
> if (!q)
> q = end;
>
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 06/13] libfdt: Add max phandle retrieval function
On Tue, Jul 05, 2016 at 10:26:39AM +0200, Maxime Ripard wrote:
> Add a function to retrieve the highest phandle in a given device tree.
>
> Signed-off-by: Maxime Ripard <maxime.rip...@free-electrons.com>
> Reviewed-by: Stefan Agner <ste...@agner.ch>
> Acked-by: Simon Glass <s...@chromium.org>
> ---
> include/libfdt.h| 13 +
> lib/libfdt/fdt_ro.c | 26 ++
> 2 files changed, 39 insertions(+)
>
> diff --git a/include/libfdt.h b/include/libfdt.h
> index fbbe58ceb3f1..4643be5adf39 100644
> --- a/include/libfdt.h
> +++ b/include/libfdt.h
> @@ -283,6 +283,19 @@ int fdt_move(const void *fdt, void *buf, int bufsize);
> */
> const char *fdt_string(const void *fdt, int stroffset);
>
> +/**
> + * fdt_get_max_phandle - retrieves the highest phandle in a tree
> + * @fdt: pointer to the device tree blob
> + *
> + * fdt_get_max_phandle retrieves the highest phandle in the given
> + * device tree
> + *
> + * returns:
> + * the highest phandle on success
> + * 0, if an error occurred
This will also return 0 if there are no phandles in the entire tree,
which isn't exactly an error.
> + */
> +uint32_t fdt_get_max_phandle(const void *fdt);
> +
> /**
> * fdt_num_mem_rsv - retrieve the number of memory reserve map entries
> * @fdt: pointer to the device tree blob
> diff --git a/lib/libfdt/fdt_ro.c b/lib/libfdt/fdt_ro.c
> index 12214c2dc2b5..503150ef1dc5 100644
> --- a/lib/libfdt/fdt_ro.c
> +++ b/lib/libfdt/fdt_ro.c
> @@ -47,6 +47,32 @@ static int _fdt_string_eq(const void *fdt, int stroffset,
> return (strnlen(p, len + 1) == len) && (memcmp(p, s, len) == 0);
> }
>
> +uint32_t fdt_get_max_phandle(const void *fdt)
> +{
> + uint32_t max_phandle = 0;
> + int offset;
> +
> + for (offset = fdt_next_node(fdt, -1, NULL);;
> + offset = fdt_next_node(fdt, offset, NULL)) {
> + uint32_t phandle;
> +
> + if (offset == -FDT_ERR_NOTFOUND)
> + return max_phandle;
> +
> + if (offset < 0)
> + return 0;
> +
> + phandle = fdt_get_phandle(fdt, offset);
> + if (phandle == (uint32_t)-1)
> + return 0;
It might be worth pointing out that *any* -1 phandle in the tree
counts as an error. Since -1 phandles are sometimes used as a
placeholder, ignoring them might be a better option.
> + if (phandle > max_phandle)
> + max_phandle = phandle;
> + }
> +
> + return 0;
> +}
> +
> int fdt_get_mem_rsv(const void *fdt, int n, uint64_t *address, uint64_t
> *size)
> {
> FDT_CHECK_HEADER(fdt);
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 08/13] libfdt: Add fdt_path_offset_namelen
On Tue, Jul 05, 2016 at 10:26:41AM +0200, Maxime Ripard wrote:
> Add a namelen variant of fdt_path_offset to retrieve the node offset using
> only a fixed number of characters.
>
> Reviewed-by: Simon Glass <s...@chromium.org>
> Signed-off-by: Maxime Ripard <maxime.rip...@free-electrons.com>
> ---
> include/libfdt.h| 16 +++-
> lib/libfdt/fdt_ro.c | 18 ++
> 2 files changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/include/libfdt.h b/include/libfdt.h
> index 4643be5adf39..f13b01f08f71 100644
> --- a/include/libfdt.h
> +++ b/include/libfdt.h
> @@ -365,6 +365,17 @@ int fdt_subnode_offset_namelen(const void *fdt, int
> parentoffset,
> */
> int fdt_subnode_offset(const void *fdt, int parentoffset, const char *name);
>
> +/**
> + * fdt_path_offset_namelen - find a tree node based on substring
> + * @fdt: pointer to the device tree blob
> + * @path: full path of the node to locate
> + * @namelen: number of characters of name to consider
> + *
> + * Identical to fdt_path_offset(), but only examine the first
> + * namelen characters of path for matching the node path.
> + */
> +int fdt_path_offset_namelen(const void *fdt, const char *path, int namelen);
> +
> /**
> * fdt_path_offset - find a tree node by its full path
> * @fdt: pointer to the device tree blob
> @@ -387,7 +398,10 @@ int fdt_subnode_offset(const void *fdt, int
> parentoffset, const char *name);
> * -FDT_ERR_BADSTRUCTURE,
> * -FDT_ERR_TRUNCATED, standard meanings.
> */
> -int fdt_path_offset(const void *fdt, const char *path);
> +static inline int fdt_path_offset(const void *fdt, const char *path)
> +{
> + return fdt_path_offset_namelen(fdt, path, strlen(path));
> +}
>
> /**
> * fdt_get_name - retrieve the name of a given node
> diff --git a/lib/libfdt/fdt_ro.c b/lib/libfdt/fdt_ro.c
> index 6b737b211d2e..9cc98db6e2bf 100644
> --- a/lib/libfdt/fdt_ro.c
> +++ b/lib/libfdt/fdt_ro.c
> @@ -145,10 +145,10 @@ int fdt_subnode_offset(const void *fdt, int
> parentoffset,
> * "foo/bar:option" and "bar:option/otheroption", both of which happen, so
> * first searching for either ':' or '/' does not work.
> */
> -static const char *fdt_path_next_separator(const char *path)
> +static const char *fdt_path_next_separator(const char *path, int len)
> {
> - const char *sep1 = strchr(path, '/');
> - const char *sep2 = strchr(path, ':');
> + const void *sep1 = memchr(path, '/', len);
> + const void *sep2 = memchr(path, ':', len);
>
> if (sep1 && sep2)
> return (sep1 < sep2) ? sep1 : sep2;
> @@ -158,9 +158,9 @@ static const char *fdt_path_next_separator(const char
> *path)
> return sep2;
> }
>
> -int fdt_path_offset(const void *fdt, const char *path)
> +int fdt_path_offset_namelen(const void *fdt, const char *path, int namelen)
> {
> - const char *end = path + strlen(path);
> + const char *end = path + namelen;
> const char *p = path;
> int offset = 0;
>
> @@ -168,7 +168,7 @@ int fdt_path_offset(const void *fdt, const char *path)
>
> /* see if we have an alias */
> if (*path != '/') {
> - const char *q = fdt_path_next_separator(path);
> + const char *q = fdt_path_next_separator(path, namelen);
>
> if (!q)
> q = end;
> @@ -181,14 +181,16 @@ int fdt_path_offset(const void *fdt, const char *path)
> p = q;
> }
>
> - while (*p) {
> + while (*p && (p < end)) {
The *p test should now be redundant. If you do see a \0 within the
length specified by namelen, you should return FDT_ERR_BADPATH, rather
than just truncating at the \0.
... and I just realised that path_offset_namelen() is already
upstream, so I should fix that there too.
So what the hell is this downstream fdt_path_next_separator() nonsense for?
> const char *q;
>
> while (*p == '/')
> p++;
> +
> if (*p == '\0' || *p == ':')
> return offset;
> - q = fdt_path_next_separator(p);
> +
> + q = fdt_path_next_separator(p, end - p);
> if (!q)
> q = end;
>
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 10/12] libfdt: Add overlay application function
On Wed, Jun 29, 2016 at 07:34:54PM -0700, Frank Rowand wrote:
> On 06/27/16 20:12, David Gibson wrote:
> > On Mon, Jun 27, 2016 at 01:40:00PM +0200, Maxime Ripard wrote:
> >> Hi David,
> >>
> >> On Mon, Jun 27, 2016 at 03:26:07PM +1000, David Gibson wrote:
> >>>> +static uint32_t overlay_get_target_phandle(const void *fdto, int
> >>>> fragment)
> >>>> +{
> >>>> +const uint32_t *val;
> >>>> +int len;
> >>>> +
> >>>> +val = fdt_getprop(fdto, fragment, "target", );
> >>>> +if (!val || (*val == 0x) || (len != sizeof(*val)))
> >>>> +return 0;
> >>>
> >>> This doesn't distinguish between a missing property (which may
> >>> indicate a valid overlay using a target-path or some other method)
> >>> and a badly formatted 'target' property, which is definitely an error
> >>> in the overlay.
> >>>
> >>> I think those should be treated differently.
> >>
> >> AFAIK, phandles can have any 32 bits values but 0x. In order
> >> to cover the two cases, we would need to have some error code, but
> >> that doesn't really work with returning a uint32_t.
> >
> > Actually phandles can have any value except 0x *or* 0. So you
> > can use 0 for "couldn't find" and -1 for "badly formatted".
>
> < snip >
>
> Hi David,
>
> I would like to capture this for the specification.
>
> It seems like I could say that a value of 0 in the FDT is not allowed.
>
> Then thinking of what Pantelis is doing with overlays, it seems like a
> value of 0x is allowed in the FDT, but it means not a valid
> phandle, so do not try to de-reference it.
>
> Does that sound good?
That should be ok. Basically both 0 and -1 are invalid phandle
values, so it's up to us if we want to assign them specific "error"
meanings.
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 10/12] libfdt: Add overlay application function
On Mon, Jun 27, 2016 at 01:40:00PM +0200, Maxime Ripard wrote:
> Hi David,
>
> On Mon, Jun 27, 2016 at 03:26:07PM +1000, David Gibson wrote:
> > > +static uint32_t overlay_get_target_phandle(const void *fdto, int
> > > fragment)
> > > +{
> > > + const uint32_t *val;
> > > + int len;
> > > +
> > > + val = fdt_getprop(fdto, fragment, "target", );
> > > + if (!val || (*val == 0x) || (len != sizeof(*val)))
> > > + return 0;
> >
> > This doesn't distinguish between a missing property (which may
> > indicate a valid overlay using a target-path or some other method)
> > and a badly formatted 'target' property, which is definitely an error
> > in the overlay.
> >
> > I think those should be treated differently.
>
> AFAIK, phandles can have any 32 bits values but 0x. In order
> to cover the two cases, we would need to have some error code, but
> that doesn't really work with returning a uint32_t.
Actually phandles can have any value except 0x *or* 0. So you
can use 0 for "couldn't find" and -1 for "badly formatted".
> Or maybe we can simply remove all the checks but the missing property,
> and let fdt_node_offset_by_phandle deal with the improper values?
>
> >
> > > + return fdt32_to_cpu(*val);
> > > +}
> > > +
> > > +static int overlay_get_target(const void *fdt, const void *fdto,
> > > + int fragment)
> > > +{
> > > + uint32_t phandle;
> > > + const char *path;
> > > +
> > > + /* Try first to do a phandle based lookup */
> > > + phandle = overlay_get_target_phandle(fdto, fragment);
> > > + if (phandle)
> > > + return fdt_node_offset_by_phandle(fdt, phandle);
> > > +
> > > + /* And then a path based lookup */
> > > + path = fdt_getprop(fdto, fragment, "target-path", NULL);
> > > + if (!path)
> > > + return -FDT_ERR_NOTFOUND;
> > > +
> > > + return fdt_path_offset(fdt, path);
> > > +}
> > > +
> > > +static int overlay_phandle_add_offset(void *fdt, int node,
> > > + const char *name, uint32_t delta)
> > > +{
> > > + const uint32_t *val;
> > > + uint32_t adj_val;
> > > + int len;
> > > +
> > > + val = fdt_getprop(fdt, node, name, );
> > > + if (!val)
> > > + return len;
> > > +
> > > + if (len != sizeof(*val))
> > > + return -FDT_ERR_BADSTRUCTURE;
> > > +
> > > + adj_val = fdt32_to_cpu(*val);
> > > + adj_val += delta;
> >
> > You should probably check for overflow here.
> >
> > > +
> > > + return fdt_setprop_inplace_u32(fdt, node, name, adj_val);
> > > +}
> > > +
> > > +static int overlay_adjust_node_phandles(void *fdto, int node,
> > > + uint32_t delta)
> > > +{
> > > + bool found = false;
> > > + int child;
> > > + int ret;
> > > +
> > > + ret = overlay_phandle_add_offset(fdto, node, "phandle", delta);
> > > + if (ret && ret != -FDT_ERR_NOTFOUND)
> > > + return ret;
> > > +
> > > + if (!ret)
> > > + found = true;
> > > +
> > > + ret = overlay_phandle_add_offset(fdto, node, "linux,phandle", delta);
> > > + if (ret && ret != -FDT_ERR_NOTFOUND)
> > > + return ret;
> >
> > I think the check for phandle vs. linux,phandle should be folded into
> > overlay_phandle_add_offset().
>
> I created overlay_phandle_add_offset to avoid duplicating the getprop,
> offset, setprop, pattern which I don't think is a good idea.
>
> And we'll have to have that kind of errors construct anyway to know if
> we modified any of the two, which is a success, or none, which is a
> failure.
Hm.. ok, you convinced me.
> > > + /*
> > > + * If neither phandle nor linux,phandle have been found return
> > > + * an error.
> > > + */
> > > + if (!found && !ret)
> > > + return ret;
> > > +
> > > + fdt_for_each_subnode(fdto, child, node)
> > > + overlay_adjust_node_phandles(fdto, child, delta);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int overlay_adjust_local_phandles(void *fdto, uint32_t delta)
> > > +{
> > > + /*
> > > + * Start adjusting the phandles from the overla
Re: [U-Boot] [PATCH v3 04/12] libfdt: Add new headers and defines
On Mon, Jun 27, 2016 at 09:25:27AM +0200, Maxime Ripard wrote: > Hi David, > > On Mon, Jun 27, 2016 at 01:39:06AM +1000, David Gibson wrote: > > On Fri, Jun 24, 2016 at 04:27:49PM +0200, Maxime Ripard wrote: > > > The libfdt overlay support introduces a bunch of new includes and > > > functions. > > > > > > Make sure we are able to build it by adding the needed glue. > > > > > > Signed-off-by: Maxime Ripard <maxime.rip...@free-electrons.com> > > > > Is this intended only for downstream u-boot only, or also for upstream > > libfdt? > > U-Boot only. > > > If it's u-boot only, you don't really need the #ifdef UBOOT. > > Unfortunately, libfdt_env.h is also included by some userspace tools > in U-Boot, which make use of strtoul while simple_strtoul is not > defined there (and we should rely on libc's anyway), leading to > link errors. Ah, ok. The ifdef makes sense then. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 07/12] libfdt: Add fdt_setprop_inplace_by_index
On Mon, Jun 27, 2016 at 11:16:52AM +0200, Maxime Ripard wrote:
> On Mon, Jun 27, 2016 at 01:45:11AM +1000, David Gibson wrote:
> > On Fri, Jun 24, 2016 at 04:27:52PM +0200, Maxime Ripard wrote:
> > > Add a function to modify inplace a property starting from a given index.
> > >
> > > This is especially useful when the property is an array of values, and you
> > > want to update one of them without changing the DT size.
> > >
> > > Signed-off-by: Maxime Ripard <maxime.rip...@free-electrons.com>
> > > ---
> > > include/libfdt.h | 34 +++---
> > > lib/libfdt/fdt_wip.c | 13 -
> > > 2 files changed, 39 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/include/libfdt.h b/include/libfdt.h
> > > index 4643be5adf39..2c8a42bcb667 100644
> > > --- a/include/libfdt.h
> > > +++ b/include/libfdt.h
> > > @@ -1032,6 +1032,27 @@ int fdt_size_cells(const void *fdt, int
> > > nodeoffset);
> > > /* Write-in-place functions */
> > > /**/
> > >
> > > +/**
> > > + * fdt_setprop_inplace_namelen_by_index - change a property's value,
> > > + *but not its size
> > > + * @fdt: pointer to the device tree blob
> > > + * @nodeoffset: offset of the node whose property to change
> > > + * @name: name of the property to change
> > > + * @namelen: number of characters of name to consider
> > > + * @index: index of the property to change in the array
> > > + * @val: pointer to data to replace the property value with
> > > + * @len: length of the property value
> > > + *
> > > + * Identical to fdt_setprop_inplace(), but modifies the given property
> > > + * starting from the given index, and using only the first characters
> > > + * of the name. It is useful when you want to manipulate only one value
> > > of
> > > + * an array and you have a string that doesn't end with \0.
> > > + */
> > > +int fdt_setprop_inplace_namelen_by_index(void *fdt, int nodeoffset,
> > > + const char *name, int namelen,
> > > + uint32_t index, const void *val,
> > > + int len);
> >
> > This looks like a good addition to upstream, but I don't like the
> > name. I'd suggest fdt_setprop_inplace_namelen_partial() because it
> > only overwrite part of the existing property value.
>
> Ack.
>
> > > +
> > > /**
> > > * fdt_setprop_inplace - change a property's value, but not its size
> > > * @fdt: pointer to the device tree blob
> > > @@ -1060,8 +1081,13 @@ int fdt_size_cells(const void *fdt, int
> > > nodeoffset);
> > > * -FDT_ERR_BADSTRUCTURE,
> > > * -FDT_ERR_TRUNCATED, standard meanings
> > > */
> > > -int fdt_setprop_inplace(void *fdt, int nodeoffset, const char *name,
> > > - const void *val, int len);
> > > +static inline int fdt_setprop_inplace(void *fdt, int nodeoffset,
> > > + const char *name, const void *val,
> > > + int len)
> > > +{
> > > + return fdt_setprop_inplace_namelen_by_index(fdt, nodeoffset, name,
> > > + strlen(name), 0, val, len);
> >
> > This effectively removes the error if len is not equal to the existing
> > property length, so that needs to be put back.
>
> So I'm not really sure what you want me to do.
>
> I can't check for the property length, since it would prevent that
> function from working, I can't check for the length + index, since we
> might update only a few bytes in the middle, and we want to keep that
> error case.
>
> Or should I just forgive about merging the two functions and just
> duplicate the two?
No, what I'm suggesting is that fdt_setprop_inplace(), whether or not
it is implemented using fdt_setprop_inplace_partial() *also* checks
that the existing property length exactly matches the replacement
length, and fails otherwise.
Doing this without too much code duplication might involve a private
internal function, or maybe it's just simpler to duplicate. But those
are the semantics they should have.
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 12/12] tests: Introduce DT overlay tests
On Mon, Jun 27, 2016 at 08:30:28AM +0200, Maxime Ripard wrote: > Hi David, > > On Mon, Jun 27, 2016 at 03:26:58PM +1000, David Gibson wrote: > > On Fri, Jun 24, 2016 at 04:27:57PM +0200, Maxime Ripard wrote: > > > This adds a bunch of unit tests for the "fdt apply" command. > > > > > > They've all been run successfully in the sandbox. However, as you still > > > require an out-of-tree dtc with overlay support, this is disabled by > > > default. > > > > > > Acked-by: Simon Glass <s...@chromium.org> > > > Acked-by: Pantelis Antoniou <pantelis.anton...@konsulko.com> > > > Signed-off-by: Maxime Ripard <maxime.rip...@free-electrons.com> > > > > These tests are geared for the u-boot tree, but for upstream > > application we really need testcases that fit within dtc/libfdt as > > well. It would be good if you can adapt some or all of these tests to > > work in that context. > > That's the plan ! :) Ok, great. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 12/12] tests: Introduce DT overlay tests
st-property;
> + };
> + };
> +};
> +
> +
> diff --git a/test/overlay/test-fdt-overlay.dts
> b/test/overlay/test-fdt-overlay.dts
> new file mode 100644
> index ..199aa5797ef4
> --- /dev/null
> +++ b/test/overlay/test-fdt-overlay.dts
> @@ -0,0 +1,88 @@
> +/*
> + * Copyright (c) 2016 NextThing Co
> + * Copyright (c) 2016 Free Electrons
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +/dts-v1/;
> +/plugin/;
> +
> +/ {
> + /* Test that we can change an int by another */
> + fragment@0 {
> + target = <>;
> +
> + __overlay__ {
> + test-int-property = <43>;
> + };
> + };
> +
> + /* Test that we can replace a string by a longer one */
> + fragment@1 {
> + target = <>;
> +
> + __overlay__ {
> + test-str-property = "foobar";
> + };
> + };
> +
> + /* Test that we add a new property */
> + fragment@2 {
> + target = <>;
> +
> + __overlay__ {
> + test-str-property-2 = "foobar2";
> + };
> + };
> +
> + /* Test that we add a new node (by phandle) */
> + fragment@3 {
> + target = <>;
> +
> + __overlay__ {
> + new-node {
> + new-property;
> + };
> + };
> + };
> +
> + /* Test that we add a new node (by path) */
> + fragment@4 {
> + target-path = "/";
> +
> + __overlay__ {
> + new-node {
> + new-property;
> + };
> + };
> + };
> +
> + fragment@5 {
> + target-path = "/";
> +
> + __overlay__ {
> + local: new-local-node {
> + new-property;
> + };
> + };
> + };
> +
> + fragment@6 {
> + target-path = "/";
> +
> + __overlay__ {
> + test-phandle = <>, <>;
> + };
> + };
> +
> + fragment@7 {
> + target = <>;
> +
> + __overlay__ {
> + sub-test-node {
> + new-sub-test-property;
> + };
> + };
> + };
> +};
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 10/12] libfdt: Add overlay application function
s_off;
> + int property;
> +
> + symbols_off = fdt_path_offset(dt, "/__symbols__");
> + fixups_off = fdt_path_offset(dto, "/__fixups__");
> +
> + fdt_for_each_property_offset(property, dto, fixups_off)
> + overlay_fixup_phandle(dt, dto, symbols_off, property);
> +
> + return 0;
> +}
> +
> +static int apply_overlay_node(void *dt, int target,
> + void *dto, int overlay)
I think 'overlay' should be 'fragment' here for consistency?
> +{
> + int property;
> + int node;
> +
> + fdt_for_each_property_offset(property, dto, overlay) {
> + const char *name;
> + const void *prop;
> + int prop_len;
> + int ret;
> +
> + prop = fdt_getprop_by_offset(dto, property, ,
> + _len);
> + if (!prop)
> + return -FDT_ERR_INTERNAL;
Actually, you probably should check the error code returned in
prop_len. Getting a NOTFOUND would indicate an internal error, but
you could also get BADSTRUCTURE or similar errors which would indicate
an error in input.
> +
> + ret = fdt_setprop(dt, target, name, prop, prop_len);
> + if (ret)
> + return ret;
> + }
> +
> + fdt_for_each_subnode(dto, node, overlay) {
> + const char *name = fdt_get_name(dto, node, NULL);
> + int nnode;
> + int ret;
> +
> + nnode = fdt_add_subnode(dt, target, name);
> + if (nnode == -FDT_ERR_EXISTS)
> + nnode = fdt_subnode_offset(dt, target, name);
> +
> + if (nnode < 0)
> + return nnode;
> +
> + ret = apply_overlay_node(dt, nnode, dto, node);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int overlay_merge(void *dt, void *dto)
> +{
> + int fragment;
> +
> + fdt_for_each_subnode(dto, fragment, 0) {
> + int overlay;
> + int target;
> + int ret;
> +
> + target = overlay_get_target(dt, dto, fragment);
> + if (target < 0)
> + continue;
> +
> + overlay = fdt_subnode_offset(dto, fragment, "__overlay__");
> + if (overlay < 0)
> + return overlay;
> +
> + ret = apply_overlay_node(dt, target, dto, overlay);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +int fdt_overlay_apply(void *fdt, void *fdto)
> +{
> + uint32_t delta = fdt_get_max_phandle(fdt) + 1;
> + int ret;
> +
> + FDT_CHECK_HEADER(fdt);
> + FDT_CHECK_HEADER(fdto);
> +
> + ret = overlay_adjust_local_phandles(fdto, delta);
> + if (ret)
> + goto err;
> +
> + ret = overlay_update_local_references(fdto, delta);
> + if (ret)
> + goto err;
> +
> + ret = overlay_fixup_phandles(fdt, fdto);
> + if (ret)
> + goto err;
> +
> + ret = overlay_merge(fdt, fdto);
> + if (!ret)
> + goto out;
> +
> +err:
This is a confusing use of gotos - this looks like it is in the exit
path for both success and failure cases, but it's not due to the
easy-to-miss goto out above.
> + /*
> + * The base device tree might have been damaged, erase its
> + * magic.
> + */
> + fdt_set_magic(fdt, ~0);
> +
> +out:
> + /*
> + * The overlay has been damaged, erase its magic.
> + */
> + fdt_set_magic(fdto, ~0);
> +
> + return ret;
> +}
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 00/12] cmd: fdt: Add device tree overlays support
On Fri, Jun 24, 2016 at 04:27:45PM +0200, Maxime Ripard wrote: > Hi, > > The device tree overlays are a great solution to the issue raised by > the bunch expandable boards we find everywhere these days, like the > Beaglebone, Raspberry Pi or CHIP. > > However, most of the time, the overlays are applied through a > mechanism involving the firmware request interface in Linux, that is > only fully functional once the userspace has been mounted and is > running. > > Some expansion boards might need to be enabled before that, because > they simply need to patch the DT early on, or need to be initialized > early in order to be fully functional, or because they provide access > to the root filesystem. > > In these cases, having the bootloader applying the overlay before > Linux starts seems like the easiest solution. > > This implementation doesn't provide all the Linux fancyness though, > there's no transactional application, which means that if the overlay > cannot be applied for a reason while you're still halfway through the > application, you're probably screwed. It also cannot remove an > overlay, but I don't think that it is currently a use-case. > > There's still a bunch of work to extend the libfdt unit tests to test > the new functions introduced, but these patches will be submitted > in a near future. Are you planning to send a new version of (the relevant portions of) these against upstream libfdt? > > Let me know what you think, > Maxime > > Changes from v2: > - Add Kconfig option for the libfdt overlay support > - Reworked the code to deal with Pantelis and David numerous > comments, among which: > * Remove the need for malloc in the overlay code, and added some > libfdt functions to do that > * Remove the DT magic in case of an error to not be able to use it > anymore > * Removed the fdt_ and _ function prefix for the static functions > * Plus the usual bunch of rework, error checking and optimizations. > > - Added new tests to deal with bugs reported by David (the overlay > was not applying when you add a subnode declared that was already > in the base device tree, and using a local phandle was only > working if the property storing it only had a length of 4). > > Changes from v1: > - Moved the overlay code to libfdt > - Added unit tests > - Refactored the code to reduce the amount of memory allocation > - No longer modify the overlay itself, but create a copy to operate > on instead. > - Removed the limitations on the fixups path, names and properties > length > - Fixed a few things here and there according to comments > > Maxime Ripard (12): > cmd: fdt: Narrow the check for fdt addr > scripts: Makefile.lib: Sanitize DTB names > vsprintf: Include stdarg for va_list > libfdt: Add new headers and defines > libfdt: Add iterator over properties > libfdt: Add max phandle retrieval function > libfdt: Add fdt_setprop_inplace_by_index > libfdt: Add fdt_path_offset_namelen > libfdt: Add fdt_getprop_namelen_w > libfdt: Add overlay application function > cmd: fdt: add fdt overlay application subcommand > tests: Introduce DT overlay tests > > Makefile | 1 + > cmd/fdt.c | 26 ++- > include/libfdt.h | 124 - > include/libfdt_env.h | 6 + > include/test/overlay.h| 16 ++ > include/test/suites.h | 1 + > include/vsprintf.h| 2 + > lib/Kconfig | 5 + > lib/libfdt/Makefile | 2 + > lib/libfdt/fdt_overlay.c | 381 > ++ > lib/libfdt/fdt_ro.c | 44 - > lib/libfdt/fdt_wip.c | 13 +- > scripts/Makefile.lib | 8 +- > test/Kconfig | 1 + > test/cmd_ut.c | 6 + > test/overlay/Kconfig | 11 ++ > test/overlay/Makefile | 15 ++ > test/overlay/cmd_ut_overlay.c | 243 > test/overlay/test-fdt-base.dts| 21 +++ > test/overlay/test-fdt-overlay.dts | 88 + > 20 files changed, 992 insertions(+), 22 deletions(-) > create mode 100644 include/test/overlay.h > create mode 100644 lib/libfdt/fdt_overlay.c > create mode 100644 test/overlay/Kconfig > create mode 100644 test/overlay/Makefile > create mode 100644 test/overlay/cmd_ut_overlay.c > create mode 100644 test/overlay/test-fdt-base.dts > create mode 100644 test/overlay/test-fdt-overlay.dts > -- David Gibson| I'll have my music baroque,
Re: [U-Boot] [PATCH v3 04/12] libfdt: Add new headers and defines
On Fri, Jun 24, 2016 at 04:27:49PM +0200, Maxime Ripard wrote: > The libfdt overlay support introduces a bunch of new includes and > functions. > > Make sure we are able to build it by adding the needed glue. > > Signed-off-by: Maxime Ripard <maxime.rip...@free-electrons.com> Is this intended only for downstream u-boot only, or also for upstream libfdt? If it's u-boot only, you don't really need the #ifdef UBOOT. If it's for upstream, then it shouldn't have u-boot specific stuff. In general the environment into which you're embedding libfdt (u-boot in this case) should provide libfdt_env.h. The one included in upstream libfdt is essentially just an example version designed to work with POSIX userspace. > --- > include/libfdt_env.h | 6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/include/libfdt_env.h b/include/libfdt_env.h > index 273b5d30f867..6c6845f76cf7 100644 > --- a/include/libfdt_env.h > +++ b/include/libfdt_env.h > @@ -23,6 +23,12 @@ typedef __be64 fdt64_t; > #define fdt64_to_cpu(x) be64_to_cpu(x) > #define cpu_to_fdt64(x) cpu_to_be64(x) > > +#ifdef __UBOOT__ > +#include > + > +#define strtoul(cp, endp, base) simple_strtoul(cp, endp, base) > +#endif > + > /* adding a ramdisk needs 0x44 bytes in version 2008.10 */ > #define FDT_RAMDISK_OVERHEAD 0x80 > -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 07/12] libfdt: Add fdt_setprop_inplace_by_index
On Fri, Jun 24, 2016 at 04:27:52PM +0200, Maxime Ripard wrote:
> Add a function to modify inplace a property starting from a given index.
>
> This is especially useful when the property is an array of values, and you
> want to update one of them without changing the DT size.
>
> Signed-off-by: Maxime Ripard <maxime.rip...@free-electrons.com>
> ---
> include/libfdt.h | 34 +++---
> lib/libfdt/fdt_wip.c | 13 -
> 2 files changed, 39 insertions(+), 8 deletions(-)
>
> diff --git a/include/libfdt.h b/include/libfdt.h
> index 4643be5adf39..2c8a42bcb667 100644
> --- a/include/libfdt.h
> +++ b/include/libfdt.h
> @@ -1032,6 +1032,27 @@ int fdt_size_cells(const void *fdt, int nodeoffset);
> /* Write-in-place functions */
> /**/
>
> +/**
> + * fdt_setprop_inplace_namelen_by_index - change a property's value,
> + *but not its size
> + * @fdt: pointer to the device tree blob
> + * @nodeoffset: offset of the node whose property to change
> + * @name: name of the property to change
> + * @namelen: number of characters of name to consider
> + * @index: index of the property to change in the array
> + * @val: pointer to data to replace the property value with
> + * @len: length of the property value
> + *
> + * Identical to fdt_setprop_inplace(), but modifies the given property
> + * starting from the given index, and using only the first characters
> + * of the name. It is useful when you want to manipulate only one value of
> + * an array and you have a string that doesn't end with \0.
> + */
> +int fdt_setprop_inplace_namelen_by_index(void *fdt, int nodeoffset,
> + const char *name, int namelen,
> + uint32_t index, const void *val,
> + int len);
This looks like a good addition to upstream, but I don't like the
name. I'd suggest fdt_setprop_inplace_namelen_partial() because it
only overwrite part of the existing property value.
> +
> /**
> * fdt_setprop_inplace - change a property's value, but not its size
> * @fdt: pointer to the device tree blob
> @@ -1060,8 +1081,13 @@ int fdt_size_cells(const void *fdt, int nodeoffset);
> * -FDT_ERR_BADSTRUCTURE,
> * -FDT_ERR_TRUNCATED, standard meanings
> */
> -int fdt_setprop_inplace(void *fdt, int nodeoffset, const char *name,
> - const void *val, int len);
> +static inline int fdt_setprop_inplace(void *fdt, int nodeoffset,
> + const char *name, const void *val,
> + int len)
> +{
> + return fdt_setprop_inplace_namelen_by_index(fdt, nodeoffset, name,
> + strlen(name), 0, val, len);
This effectively removes the error if len is not equal to the existing
property length, so that needs to be put back.
> +}
>
> /**
> * fdt_setprop_inplace_u32 - change the value of a 32-bit integer property
> @@ -1095,7 +1121,9 @@ static inline int fdt_setprop_inplace_u32(void *fdt,
> int nodeoffset,
> const char *name, uint32_t val)
> {
> fdt32_t tmp = cpu_to_fdt32(val);
> - return fdt_setprop_inplace(fdt, nodeoffset, name, , sizeof(tmp));
> + return fdt_setprop_inplace_namelen_by_index(fdt, nodeoffset,
> + name, strlen(name),
> + 0, , sizeof(tmp));
> }
>
> /**
> diff --git a/lib/libfdt/fdt_wip.c b/lib/libfdt/fdt_wip.c
> index 9fe988655fe3..737769fa59e2 100644
> --- a/lib/libfdt/fdt_wip.c
> +++ b/lib/libfdt/fdt_wip.c
> @@ -14,20 +14,23 @@
>
> #include "libfdt_internal.h"
>
> -int fdt_setprop_inplace(void *fdt, int nodeoffset, const char *name,
> - const void *val, int len)
> +int fdt_setprop_inplace_namelen_by_index(void *fdt, int nodeoffset,
> + const char *name, int namelen,
> + uint32_t index, const void *val,
> + int len)
> {
> void *propval;
> int proplen;
>
> - propval = fdt_getprop_w(fdt, nodeoffset, name, );
> + propval = fdt_getprop_namelen_w(fdt, nodeoffset, name, namelen,
> + );
> if (!propval)
> return proplen;
>
> - if (proplen != len)
> + if (proplen < (len + index))
> return -FDT_ERR_NOSPACE;
>
> - memcpy(propva
Re: [U-Boot] [PATCH v3 08/12] libfdt: Add fdt_path_offset_namelen
On Fri, Jun 24, 2016 at 04:27:53PM +0200, Maxime Ripard wrote:
> Add a namelen variant of fdt_path_offset to retrieve the node offset using
> only a fixed number of characters.
>
> Signed-off-by: Maxime Ripard <maxime.rip...@free-electrons.com>
Looks good for upstream.
> ---
> include/libfdt.h| 16 +++-
> lib/libfdt/fdt_ro.c | 18 ++
> 2 files changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/include/libfdt.h b/include/libfdt.h
> index 2c8a42bcb667..dbe8a0efca87 100644
> --- a/include/libfdt.h
> +++ b/include/libfdt.h
> @@ -365,6 +365,17 @@ int fdt_subnode_offset_namelen(const void *fdt, int
> parentoffset,
> */
> int fdt_subnode_offset(const void *fdt, int parentoffset, const char *name);
>
> +/**
> + * fdt_path_offset_namelen - find a tree node based on substring
> + * @fdt: pointer to the device tree blob
> + * @path: full path of the node to locate
> + * @namelen: number of characters of name to consider
> + *
> + * Identical to fdt_path_offset(), but only examine the first
> + * namelen characters of path for matching the node path.
> + */
> +int fdt_path_offset_namelen(const void *fdt, const char *path, int namelen);
> +
> /**
> * fdt_path_offset - find a tree node by its full path
> * @fdt: pointer to the device tree blob
> @@ -387,7 +398,10 @@ int fdt_subnode_offset(const void *fdt, int
> parentoffset, const char *name);
> * -FDT_ERR_BADSTRUCTURE,
> * -FDT_ERR_TRUNCATED, standard meanings.
> */
> -int fdt_path_offset(const void *fdt, const char *path);
> +static inline int fdt_path_offset(const void *fdt, const char *path)
> +{
> + return fdt_path_offset_namelen(fdt, path, strlen(path));
> +}
>
> /**
> * fdt_get_name - retrieve the name of a given node
> diff --git a/lib/libfdt/fdt_ro.c b/lib/libfdt/fdt_ro.c
> index 503150ef1dc5..05344d3eebfe 100644
> --- a/lib/libfdt/fdt_ro.c
> +++ b/lib/libfdt/fdt_ro.c
> @@ -145,10 +145,10 @@ int fdt_subnode_offset(const void *fdt, int
> parentoffset,
> * "foo/bar:option" and "bar:option/otheroption", both of which happen, so
> * first searching for either ':' or '/' does not work.
> */
> -static const char *fdt_path_next_seperator(const char *path)
> +static const char *fdt_path_next_seperator(const void *path, int len)
Not in scope for this patch, but I should fix that mispelling of 'separator'.
> {
> - const char *sep1 = strchr(path, '/');
> - const char *sep2 = strchr(path, ':');
> + const void *sep1 = memchr(path, '/', len);
> + const void *sep2 = memchr(path, ':', len);
>
> if (sep1 && sep2)
> return (sep1 < sep2) ? sep1 : sep2;
> @@ -158,9 +158,9 @@ static const char *fdt_path_next_seperator(const char
> *path)
> return sep2;
> }
>
> -int fdt_path_offset(const void *fdt, const char *path)
> +int fdt_path_offset_namelen(const void *fdt, const char *path, int namelen)
> {
> - const char *end = path + strlen(path);
> + const char *end = path + namelen;
> const char *p = path;
> int offset = 0;
>
> @@ -168,7 +168,7 @@ int fdt_path_offset(const void *fdt, const char *path)
>
> /* see if we have an alias */
> if (*path != '/') {
> - const char *q = fdt_path_next_seperator(path);
> + const char *q = fdt_path_next_seperator(path, namelen);
>
> if (!q)
> q = end;
> @@ -181,14 +181,16 @@ int fdt_path_offset(const void *fdt, const char *path)
> p = q;
> }
>
> - while (*p) {
> + while (*p && (p < end)) {
> const char *q;
>
> while (*p == '/')
> p++;
> +
> if (*p == '\0' || *p == ':')
> return offset;
> - q = fdt_path_next_seperator(p);
> +
> + q = fdt_path_next_seperator(p, end - p);
> if (!q)
> q = end;
>
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 09/12] libfdt: Add fdt_getprop_namelen_w
On Fri, Jun 24, 2016 at 04:27:54PM +0200, Maxime Ripard wrote:
> Add a function to retrieve a writeable property only by the first
> characters of its name.
>
> Signed-off-by: Maxime Ripard <maxime.rip...@free-electrons.com>
> ---
> include/libfdt.h | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/include/libfdt.h b/include/libfdt.h
> index dbe8a0efca87..b8758de3ae54 100644
> --- a/include/libfdt.h
> +++ b/include/libfdt.h
> @@ -619,6 +619,13 @@ const void *fdt_getprop_by_offset(const void *fdt, int
> offset,
> */
> const void *fdt_getprop_namelen(const void *fdt, int nodeoffset,
> const char *name, int namelen, int *lenp);
> +static inline void *fdt_getprop_namelen_w(void *fdt, int nodeoffset,
> + const char *name, int namelen,
> + int *lenp)
> +{
> + return (void *)(uintptr_t)fdt_getprop_namelen(fdt, nodeoffset, name,
> + namelen, lenp);
> +}
I believe you used this in your new setprop_inpace implementation. So
the series needs to be re-ordered to avoid breaking bisection.
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 7/9] libfdt: Add overlay application function
On Wed, Jun 15, 2016 at 12:34:00PM +0300, Pantelis Antoniou wrote:
> Hi David,
>
> > On Jun 15, 2016, at 06:14 , David Gibson <da...@gibson.dropbear.id.au>
> > wrote:
> >
> > On Tue, Jun 14, 2016 at 12:22:23PM +0300, Pantelis Antoniou wrote:
> >> Hi David,
> >>> On Jun 14, 2016, at 03:25 , David Gibson <da...@gibson.dropbear.id.au>
> >>> wrote:
> >>> On Fri, Jun 10, 2016 at 05:28:11PM +0300, Pantelis Antoniou wrote:
> > [snip]
> >>>>> +static int fdt_overlay_merge(void *dt, void *dto)
> >>>>> +{
> >>>>> + int root, fragment;
> >>>>> +
> >>>>> + root = fdt_path_offset(dto, "/");
> >>>>> + if (root < 0)
> >>>>> + return root;
> >>>>> +
> >>>>> + fdt_for_each_subnode(dto, fragment, root) {
> >>>>> + const char *name = fdt_get_name(dto, fragment, NULL);
> >>>>> + uint32_t target;
> >>>>> + int overlay;
> >>>>> + int ret;
> >>>>> +
> >>>>> + if (strncmp(name, "fragment", 8))
> >>>>> + continue;
> >>>>> +
> >>>>
> >>>> This is incorrect. The use of “fragment” is a convention only.
> >>>> The real test whether the node is an overlay fragment is that
> >>>> it contains a target property.
> >>>
> >>> Hmm.. I dislike that approach. First, it means that if new target
> >>> types are introduced in future, older code is likely to silently
> >>> ignore such fragments instead of realizing that it doesn't know how to
> >>> apply themm. Second, it raises weird issues if some node down within
> >>> a fragment also happens to have a property called "target”.
> >>
> >> Not really. If new targets are introduced then the fragment will be
> >> ignored.
> >
> > Yes.. and that's bad.
>
> That’s arguable.
!?! No, really, silent partial application is just horrible.
> >> We can have an argument about what is better to do (report an error or
> >> ignore a fragment) but what it comes down to is that that applicator
> >> does not know how to handle the new target method.
> >
> > Sure, let's have the argument. The overlay is constructed on the
> > assumption that all the pieces will be applied, or none of them. A
> > silent, partial application is an awful, awful failure mode. We
> > absolutely should report an error, and in order to do so we need to
> > know what are applicable fragments, whether or not we understand the
> > target designation (or any other meta-data the fragment has).
>
> This way also allows having nodes being something other than fragments.
>
> So instead of being painted into a corner (all subnodes that are not
> named ‘fragment@X’ are errors), we have flexibility in introducing
> nodes that contain configuration data for the loader.
There's no significant difference between the approaches from this
point of view. Metadata nodes are certainly possible (we already have
__symbols__ and __fixups__) but calling them something other than
fragment@ is no harder than leaving off the target property. In fact
even if it was workable, calling new metadata nodes fragment@ would be
stupidly confusing.
> > Given what's established so far, checking the name seems the obvious
> > way to do that.
> >
>
> Again, it’s arguable. Stricter checking against future-proofing.
>
> >> There is no issues with any target properties inside a fragment because
> >> the check is not made recursively.
> >
> > Ok, so the real test you're proposing is "at the top level AND has a
> > target property”.
>
> Yes
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 7/9] libfdt: Add overlay application function
On Tue, Jun 14, 2016 at 12:22:23PM +0300, Pantelis Antoniou wrote:
> Hi David,
> > On Jun 14, 2016, at 03:25 , David Gibson <da...@gibson.dropbear.id.au>
> > wrote:
> > On Fri, Jun 10, 2016 at 05:28:11PM +0300, Pantelis Antoniou wrote:
[snip]
> >>> +static int fdt_overlay_merge(void *dt, void *dto)
> >>> +{
> >>> + int root, fragment;
> >>> +
> >>> + root = fdt_path_offset(dto, "/");
> >>> + if (root < 0)
> >>> + return root;
> >>> +
> >>> + fdt_for_each_subnode(dto, fragment, root) {
> >>> + const char *name = fdt_get_name(dto, fragment, NULL);
> >>> + uint32_t target;
> >>> + int overlay;
> >>> + int ret;
> >>> +
> >>> + if (strncmp(name, "fragment", 8))
> >>> + continue;
> >>> +
> >>
> >> This is incorrect. The use of “fragment” is a convention only.
> >> The real test whether the node is an overlay fragment is that
> >> it contains a target property.
> >
> > Hmm.. I dislike that approach. First, it means that if new target
> > types are introduced in future, older code is likely to silently
> > ignore such fragments instead of realizing that it doesn't know how to
> > apply themm. Second, it raises weird issues if some node down within
> > a fragment also happens to have a property called "target”.
>
> Not really. If new targets are introduced then the fragment will be ignored.
Yes.. and that's bad.
> We can have an argument about what is better to do (report an error or
> ignore a fragment) but what it comes down to is that that applicator
> does not know how to handle the new target method.
Sure, let's have the argument. The overlay is constructed on the
assumption that all the pieces will be applied, or none of them. A
silent, partial application is an awful, awful failure mode. We
absolutely should report an error, and in order to do so we need to
know what are applicable fragments, whether or not we understand the
target designation (or any other meta-data the fragment has).
Given what's established so far, checking the name seems the obvious
way to do that.
> There is no issues with any target properties inside a fragment because
> the check is not made recursively.
Ok, so the real test you're proposing is "at the top level AND has a
target property".
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 7/9] libfdt: Add overlay application function
path, name, index, label);
> > +
> > + free(prop_cpy);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int fdt_overlay_fixup_phandles(void *dt, void *dto)
> > +{
> > + int fixups_off, symbols_off;
> > + int property;
> > +
> > + symbols_off = fdt_path_offset(dt, "/__symbols__");
> > + fixups_off = fdt_path_offset(dto, "/__fixups__");
> > +
> > + fdt_for_each_property(dto, fixups_off, property)
> > + fdt_overlay_fixup_phandle(dt, dto, symbols_off, property);
> > +
> > + return 0;
> > +}
> > +
> > +static int fdt_apply_overlay_node(void *dt, void *dto,
> > + int target, int overlay)
> > +{
> > + int property;
> > + int node;
> > +
> > + fdt_for_each_property(dto, overlay, property) {
> > + const char *name;
> > + const void *prop;
> > + int prop_len;
> > + int ret;
> > +
> > + prop = fdt_getprop_by_offset(dto, property, ,
> > +_len);
> > + if (!prop)
> > + return -FDT_ERR_INTERNAL;
> > +
> > + ret = fdt_setprop(dt, target, name, prop, prop_len);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + fdt_for_each_subnode(dto, node, overlay) {
> > + const char *name = fdt_get_name(dto, node, NULL);
> > + int nnode;
> > + int ret;
> > +
> > + nnode = fdt_add_subnode(dt, target, name);
> > + if (nnode < 0)
> > + return nnode;
> > +
> > + ret = fdt_apply_overlay_node(dt, dto, nnode, node);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int fdt_overlay_merge(void *dt, void *dto)
> > +{
> > + int root, fragment;
> > +
> > + root = fdt_path_offset(dto, "/");
> > + if (root < 0)
> > + return root;
> > +
> > + fdt_for_each_subnode(dto, fragment, root) {
> > + const char *name = fdt_get_name(dto, fragment, NULL);
> > + uint32_t target;
> > + int overlay;
> > + int ret;
> > +
> > + if (strncmp(name, "fragment", 8))
> > + continue;
> > +
>
> This is incorrect. The use of “fragment” is a convention only.
> The real test whether the node is an overlay fragment is that
> it contains a target property.
Hmm.. I dislike that approach. First, it means that if new target
types are introduced in future, older code is likely to silently
ignore such fragments instead of realizing that it doesn't know how to
apply themm. Second, it raises weird issues if some node down within
a fragment also happens to have a property called "target".
> > + target = fdt_overlay_get_target(dt, dto, fragment);
> > + if (target < 0)
> > + return target;
> > +
>
> So you could do ‘if (target < 0) continue;’ or handle a more complex error
> code.
>
> > + overlay = fdt_subnode_offset(dto, fragment, "__overlay__");
> > + if (overlay < 0)
> > + return overlay;
> > +
> > + ret = fdt_apply_overlay_node(dt, dto, target, overlay);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +int fdt_overlay_apply(void *fdt, void *fdto)
> > +{
> > + uint32_t delta = fdt_get_max_phandle(fdt) + 1;
> > + void *fdto_copy;
> > + int ret;
> > +
> > + FDT_CHECK_HEADER(fdt);
> > + FDT_CHECK_HEADER(fdto);
> > +
> > + /*
> > +* We're going to modify the overlay so that we can apply it.
> > +*
> > +* Make sure sure we don't destroy the original
> > +*/
> > + fdto_copy = malloc(fdt_totalsize(fdto));
> > + if (!fdto_copy)
> > + return -FDT_ERR_INTERNAL;
> > +
> > + ret = fdt_move(fdto, fdto_copy, fdt_totalsize(fdto));
> > + if (ret)
> > + goto out;
> > +
> > + ret = fdt_overlay_adjust_local_phandles(fdto_copy, delta);
> > + if (ret)
> > + goto out;
> > +
> > + ret = fdt_overlay_update_local_references(fdto_copy, delta);
> > + if (ret)
> > + goto out;
> > +
> > + ret = fdt_overlay_fixup_phandles(fdt, fdto_copy);
> > + if (ret)
> > + goto out;
> > +
> > + ret = fdt_overlay_merge(fdt, fdto_copy);
> > +
> > +out:
> > + free(fdto_copy);
> > + return ret;
> > +}
>
> I would caution against the liberal use of malloc in libfdt. We’re possibly
> running in a constrained
> environment; a custom extents based (non freeing) allocator should be better.
>
> We need some figures about memory consumption when this is enabled, and a
> CONFIG option to disable it.
>
> Regards
>
> — Pantelis
>
>
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 4/9] libfdt: Add new headers and defines
On Fri, Jun 10, 2016 at 05:03:36PM +0300, Pantelis Antoniou wrote: > Hi Maxime, > > > On May 27, 2016, at 12:13 , Maxime Ripard > > <maxime.rip...@free-electrons.com> wrote: > > > > The libfdt overlay support introduces a bunch of new includes and > > functions. > > > > Make sure we are able to build it by adding the needed glue. > > > > Signed-off-by: Maxime Ripard <maxime.rip...@free-electrons.com> > > --- > > include/libfdt_env.h | 7 +++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/include/libfdt_env.h b/include/libfdt_env.h > > index 273b5d30f867..2d2196031332 100644 > > --- a/include/libfdt_env.h > > +++ b/include/libfdt_env.h > > @@ -23,6 +23,13 @@ typedef __be64 fdt64_t; > > #define fdt64_to_cpu(x) be64_to_cpu(x) > > #define cpu_to_fdt64(x) cpu_to_be64(x) > > > > +#ifdef __UBOOT__ > > +#include "malloc.h" > > +#include "vsprintf.h" > > + > > +#define strtol(cp, endp, base) simple_strtol(cp, endp, base) > > +#endif > > + > > /* adding a ramdisk needs 0x44 bytes in version 2008.10 */ > > #define FDT_RAMDISK_OVERHEAD0x80 > > > > We need to figure out what the upstream libfdt/dtc maintainer’s take is on > this is. > For u-boot we’re fine and for now it’s OK. These were sent to the upstream dtc list as well. The concept is fine, but there are a number of problems in the implementation. I sent detailed review comments on the upstream versions, haven't seen a respin yet. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] fdt: Try to read #address-cells/size-cells from parent
On Wed, Mar 16, 2016 at 05:18:25PM +0100, Michal Simek wrote:
> Hi David,
>
> On 15.3.2016 01:27, David Gibson wrote:
> > On Mon, Mar 14, 2016 at 10:10:58PM +0100, Michal Simek wrote:
> >> On 13.3.2016 02:54, Simon Glass wrote:
> >>> Hi Michal,
> >>>
> >>> On 16 February 2016 at 09:10, Michal Simek <michal.si...@xilinx.com>
> >>> wrote:
> >>>> Hi Simon,
> >>>>
> >>>> On 16.2.2016 17:00, Simon Glass wrote:
> >>>>> Hi Michal,
> >>>>>
> >>>>> On 15 February 2016 at 02:58, Michal Simek <michal.si...@xilinx.com>
> >>>>> wrote:
> >>>>>> Hi Simon,
> >>>>>>
> >>>>>> On 10.2.2016 13:04, Michal Simek wrote:
> >>>>>>> Read #address-cells and #size-cells from parent if they are not
> >>>>>>> present in
> >>>>>>> current node.
> >>>>>>>
> >>>>>>> Signed-off-by: Michal Simek <michal.si...@xilinx.com>
> >>>>>>> ---
> >>>>>>>
> >>>>>>> I have code which read information about memory for zynqmp but memory
> >>>>>>> node most of the time doesn't contain #address/size-cells which are
> >>>>>>> present in parent node.
> >>>>>>> That's why let's try to read it from parent.
> >>>>>>>
> >>>>>>> Also I think that we shouldn't return 2 if property is not found
> >>>>>>> because
> >>>>>>> it has side effect on 32bit systems with #address/size-cells = <1>;
> >>>>>>>
> >>>>>>> ---
> >>>>>>> lib/libfdt/fdt_addresses.c | 18 ++
> >>>>>>> 1 file changed, 14 insertions(+), 4 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/lib/libfdt/fdt_addresses.c b/lib/libfdt/fdt_addresses.c
> >>>>>>> index 76054d98e5fd..b164d0988079 100644
> >>>>>>> --- a/lib/libfdt/fdt_addresses.c
> >>>>>>> +++ b/lib/libfdt/fdt_addresses.c
> >>>>>>> @@ -19,10 +19,15 @@ int fdt_address_cells(const void *fdt, int
> >>>>>>> nodeoffset)
> >>>>>>> const fdt32_t *ac;
> >>>>>>> int val;
> >>>>>>> int len;
> >>>>>>> + int parent;
> >>>>>>>
> >>>>>>> ac = fdt_getprop(fdt, nodeoffset, "#address-cells", );
> >>>>>>> - if (!ac)
> >>>>>>> - return 2;
> >>>>>>> + if (!ac) {
> >>>>>>> + parent = fdt_parent_offset(fdt, nodeoffset);
> >>>>>>> + ac = fdt_getprop(fdt, parent, "#address-cells", );
> >>>>>>> + if (!ac)
> >>>>>>> + return 2;
> >>>>>>> + }
> >>>>>>>
> >>>>>>> if (len != sizeof(*ac))
> >>>>>>> return -FDT_ERR_BADNCELLS;
> >>>>>>> @@ -39,10 +44,15 @@ int fdt_size_cells(const void *fdt, int
> >>>>>>> nodeoffset)
> >>>>>>> const fdt32_t *sc;
> >>>>>>> int val;
> >>>>>>> int len;
> >>>>>>> + int parent;
> >>>>>>>
> >>>>>>> sc = fdt_getprop(fdt, nodeoffset, "#size-cells", );
> >>>>>>> - if (!sc)
> >>>>>>> - return 2;
> >>>>>>> + if (!sc) {
> >>>>>>> + parent = fdt_parent_offset(fdt, nodeoffset);
> >>>>>>> + sc = fdt_getprop(fdt, parent, "#size-cells", );
> >>>>>>> + if (!sc)
> >>>>>>> + return 2;
> >>>>>>> + }
> >>>>>>>
> >>>>>>> if (len != sizeof(*sc))
> >>>>>>> return -FDT_ERR_BADNCELLS;
> >>>>>>>
> >>>>>>
> >>>>>> Simon: Any comment?
> >>>&
Re: [U-Boot] [PATCH] fdt: Try to read #address-cells/size-cells from parent
t; >> Code is in memory node I need to work with and asking for size-cells. > >> Current code returns 2 instead of error and the rest of code just works > >> with size = 2 which is incorrect for this setup. > >> > >> I have already changed size-cells = 2 in our repo because I need to > >> support for more than 4GB memory anyway but this should point to the > >> problem in that generic functions. > > > > I think this should go in a higher-level function. I very much doubt > > that this patch would be accepted upstream. > > > > Can you find the caller and make it call this function again (for the > > parent) when no nothing is found on the first call? Hopefully this > > caller will have access to the parent node and will not need to call > > fdt_parent_offset(). > > The funny part is that nothing is found means return 2. If this returns > something <0 then there is not a problem to try it with parents. I don't have the full context of this thread, so it's a bit hard to be sure, but this doesn't look right from what I can see. Two things to remember here: * #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. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 07/26] libfdt: Add a function to write a property placeholder
On Thu, Jan 28, 2016 at 09:39:27AM -0700, Simon Glass wrote:
> The existing function to add a new property to a tree being built requires
> that the entire contents of the new property be passed in. For some
> applications it is more convenient to be able to add the property contents
> later, perhaps by reading from a file. This avoids double-buffering of the
> contents.
>
> Add a new function to support this and adust the existing fdt_property() to
> use it.
>
> Signed-off-by: Simon Glass <s...@chromium.org>
So, obviously such a patch should really go towards upstream libfdt.
I'm happy enough with the concept, but I don't like the name.
I'd prefer fdt_property_reserve() - the idea being that it reserves
space for the property but doesn't fill it in.
> ---
>
> include/libfdt.h| 16
> lib/libfdt/fdt_sw.c | 16 ++--
> 2 files changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/include/libfdt.h b/include/libfdt.h
> index e48c21a..c3f37ee 100644
> --- a/include/libfdt.h
> +++ b/include/libfdt.h
> @@ -1181,6 +1181,22 @@ static inline int fdt_property_cell(void *fdt, const
> char *name, uint32_t val)
> {
> return fdt_property_u32(fdt, name, val);
> }
> +
> +/**
> + * fdt_property_val - add a new property and return a pointer to its value
> + *
> + * @fdt: pointer to the device tree blob
> + * @name: name of property to add
> + * @len: length of property value in bytes
> + * @valp: returns a pointer to where where the value should be placed
> + *
> + * returns:
> + * 0, on success
> + * -FDT_ERR_BADMAGIC,
> + * -FDT_ERR_NOSPACE, standard meanings
> + */
> +int fdt_property_val(void *fdt, const char *name, int len, void **valp);
> +
> #define fdt_property_string(fdt, name, str) \
> fdt_property(fdt, name, str, strlen(str)+1)
> int fdt_end_node(void *fdt);
> diff --git a/lib/libfdt/fdt_sw.c b/lib/libfdt/fdt_sw.c
> index 320a914..9c1df3d 100644
> --- a/lib/libfdt/fdt_sw.c
> +++ b/lib/libfdt/fdt_sw.c
> @@ -175,7 +175,7 @@ static int _fdt_find_add_string(void *fdt, const char *s)
> return offset;
> }
>
> -int fdt_property(void *fdt, const char *name, const void *val, int len)
> +int fdt_property_val(void *fdt, const char *name, int len, void **valp)
> {
> struct fdt_property *prop;
> int nameoff;
> @@ -193,7 +193,19 @@ int fdt_property(void *fdt, const char *name, const void
> *val, int len)
> prop->tag = cpu_to_fdt32(FDT_PROP);
> prop->nameoff = cpu_to_fdt32(nameoff);
> prop->len = cpu_to_fdt32(len);
> - memcpy(prop->data, val, len);
> + *valp = prop->data;
> + return 0;
> +}
> +
> +int fdt_property(void *fdt, const char *name, const void *val, int len)
> +{
> + void *ptr;
> + int ret;
> +
> + ret = fdt_property_val(fdt, name, len, );
> + if (ret)
> + return ret;
> + memcpy(ptr, val, len);
> return 0;
> }
>
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 07/26] libfdt: Add a function to write a property placeholder
On Fri, Jan 29, 2016 at 11:23:31AM -0700, Simon Glass wrote:
> Hi David,
>
> On 28 January 2016 at 22:29, David Gibson <da...@gibson.dropbear.id.au> wrote:
> > On Thu, Jan 28, 2016 at 09:39:27AM -0700, Simon Glass wrote:
> >> The existing function to add a new property to a tree being built requires
> >> that the entire contents of the new property be passed in. For some
> >> applications it is more convenient to be able to add the property contents
> >> later, perhaps by reading from a file. This avoids double-buffering of the
> >> contents.
> >>
> >> Add a new function to support this and adust the existing fdt_property() to
> >> use it.
> >>
> >> Signed-off-by: Simon Glass <s...@chromium.org>
> >
> > So, obviously such a patch should really go towards upstream libfdt.
> >
> > I'm happy enough with the concept, but I don't like the name.
> >
> > I'd prefer fdt_property_reserve() - the idea being that it reserves
> > space for the property but doesn't fill it in.
>
> Sounds good. I'll work up a patch. I tend to sync with upstream one a
> quarter or so.
Ok.
Actually, on further consideration, fdt_property_placeholder() is a
better name again.
>
> >
> >> ---
> >>
> >> include/libfdt.h| 16
> >> lib/libfdt/fdt_sw.c | 16 ++--
> >> 2 files changed, 30 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/include/libfdt.h b/include/libfdt.h
> >> index e48c21a..c3f37ee 100644
> >> --- a/include/libfdt.h
> >> +++ b/include/libfdt.h
> >> @@ -1181,6 +1181,22 @@ static inline int fdt_property_cell(void *fdt,
> >> const char *name, uint32_t val)
> >> {
> >> return fdt_property_u32(fdt, name, val);
> >> }
> >> +
> >> +/**
> >> + * fdt_property_val - add a new property and return a pointer to its value
> >> + *
> >> + * @fdt: pointer to the device tree blob
> >> + * @name: name of property to add
> >> + * @len: length of property value in bytes
> >> + * @valp: returns a pointer to where where the value should be placed
> >> + *
> >> + * returns:
> >> + * 0, on success
> >> + * -FDT_ERR_BADMAGIC,
> >> + * -FDT_ERR_NOSPACE, standard meanings
> >> + */
> >> +int fdt_property_val(void *fdt, const char *name, int len, void **valp);
> >> +
> >> #define fdt_property_string(fdt, name, str) \
> >> fdt_property(fdt, name, str, strlen(str)+1)
> >> int fdt_end_node(void *fdt);
> >> diff --git a/lib/libfdt/fdt_sw.c b/lib/libfdt/fdt_sw.c
> >> index 320a914..9c1df3d 100644
> >> --- a/lib/libfdt/fdt_sw.c
> >> +++ b/lib/libfdt/fdt_sw.c
> >> @@ -175,7 +175,7 @@ static int _fdt_find_add_string(void *fdt, const char
> >> *s)
> >> return offset;
> >> }
> >>
> >> -int fdt_property(void *fdt, const char *name, const void *val, int len)
> >> +int fdt_property_val(void *fdt, const char *name, int len, void **valp)
> >> {
> >> struct fdt_property *prop;
> >> int nameoff;
> >> @@ -193,7 +193,19 @@ int fdt_property(void *fdt, const char *name, const
> >> void *val, int len)
> >> prop->tag = cpu_to_fdt32(FDT_PROP);
> >> prop->nameoff = cpu_to_fdt32(nameoff);
> >> prop->len = cpu_to_fdt32(len);
> >> - memcpy(prop->data, val, len);
> >> + *valp = prop->data;
> >> + return 0;
> >> +}
> >> +
> >> +int fdt_property(void *fdt, const char *name, const void *val, int len)
> >> +{
> >> + void *ptr;
> >> + int ret;
> >> +
> >> + ret = fdt_property_val(fdt, name, len, );
> >> + if (ret)
> >> + return ret;
> >> + memcpy(ptr, val, len);
> >> return 0;
> >> }
> >>
>
> Regards,
> Simon
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/5] treewide: include libfdt_env.h before fdt.h
On Fri, Jan 18, 2013 at 06:59:46AM -0500, Gerald Van Baren wrote: On 01/17/2013 06:50 PM, David Gibson wrote: On Thu, Jan 17, 2013 at 01:32:45PM -0500, Jerry Van Baren wrote: Hi Scott, Kim, David, [snip] libfdt_env.h is where Kim typedef'ed fdt16_t, fdt32_t, fdt64_t I suspect the original intent was to have libfdt.h be the file that people #included. For whatever reason, most includes are (picking on fdt_ro.c arbitrarily): 51 #include libfdt_env.h 53 #include fdt.h 54 #include libfdt.h Since libfdt.h #includes fdt.h and libfdt_env.h, lines 51 and 53 (above) are redundant. It sorts out OK in dtc because libfdt_env.h includes stdint.h and defines fdt*_t, but it messes me up in u-boot where (currently) libfdt_env.h does *not* include stdint.h... Ok, so, the uboot libfdt_env.h should be fixed to define uintXX_t and fdtXX_t (either by including stdint or my other means). The purpose of libfdt_env.h is to define the things that libfdt requires, and those types are (now) such a requirement. I like the move, but have not done it (yet). I made a trial patch (see below) that uses libfdt.h as the interface and cleans out the (now redundant) other *fdt*.h includes. If this is in the right direction, I'll move the fdtXX_t typedefs and formally submit it. The test suite has one failure, but it fails with or without my changes. $ make check | grep FAIL fdtget-runtest.sh 77 121 66 111 97 114 100 78 97 109 101 0 77 121 66 111 97 114 100 70 97 109 105 108 121 78 97 109 101 0 label01.dts.fdtget.test.dtb / compatible: FAIL Results differ from expected *FAIL:1 ** TEST SUMMARY * Total testcases:1443 *PASS:1442 *FAIL:1 * Bad configuration:0 * Strange test result:0 ** Yeah, Jon and I are aware of that recently introduced bug. I've had other projects to attend to and haven't had a chance to fix it yet. It's basically a testsuite bug though, so it should be harmless. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: Digital signature ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/5] treewide: include libfdt_env.h before fdt.h
On Thu, Jan 17, 2013 at 01:32:45PM -0500, Jerry Van Baren wrote: Hi Scott, Kim, David, On 01/17/2013 12:54 PM, Kim Phillips wrote: On Wed, 16 Jan 2013 18:36:03 -0600 Scott Wood scottw...@freescale.com wrote: On 01/16/2013 05:59:04 PM, Kim Phillips wrote: and, if including libfdt.h which includes libfdt_env.h in the correct order, don't include fdt.h before libfdt.h. this is needed to get the fdt type definitions set from the project environment before fdt.h uses them. Signed-off-by: Kim Phillips kim.phill...@freescale.com Cc: Jerry Van Baren gvb.ub...@gmail.com Maybe fdt.h should include libfdt_env.h? Or just always use libfdt.h as the public header. Was just following along the same lines as the dtc commits 38ad79d3 dtc/tests: don't include fdt.h prior to libfdt.h and 20b866a7 dtc/fdtdump: include libfdt_env.h prior to fdt.h, acked by David G. I don't know why some only include fdt.h. devicetree-discuss/David: is there a prescribed way to go here? Change all fdt.h includers to just always include libfdt.h instead of libfdt_env.h prior to fdt.h? I started applying Kim's sparse patches to the u-boot source and ran into this issue pretty hard. In u-boot, there is an added complexity that the tools (host-based u-boot support tools) support flattened device trees, but explicitly include the u-boot version of libfdt declarations so they don't fall out of sync if the host has a non-compatible libfdt version. Having them out of sync would be a *horrible* situation to sort out - everything would build OK but nothing would work right, probably with no useful diagnostic information. This originated in 2008, so life may be better nowadays. Or maybe not. I would be in favor of explicitly including all the *fdt* headers in the sources. Alternately, Scott's suggestion of just including libfdt.h as the public header seems good, but I'm pretty sure it will mess me up with the explicit #including in the host-based tools build, leaving us with nasty work-arounds or a risk of hard to identify incompatible host vs. u-boot fdt versions. Who Includes Who fdt.h - no includes fdt_support.h - (u-boot only file) 29 #include fdt.h libfdt.h 54 #include libfdt_env.h 55 #include fdt.h libfdt_env.h - u-boot version is minimal, uses pre-existing macros for byte swapping - dtc version implements byte swapping, includes: 4 #include stddef.h 5 #include stdint.h 6 #include string.h libfdt_env.h is where Kim typedef'ed fdt16_t, fdt32_t, fdt64_t I suspect the original intent was to have libfdt.h be the file that people #included. For whatever reason, most includes are (picking on fdt_ro.c arbitrarily): 51 #include libfdt_env.h 53 #include fdt.h 54 #include libfdt.h Since libfdt.h #includes fdt.h and libfdt_env.h, lines 51 and 53 (above) are redundant. It sorts out OK in dtc because libfdt_env.h includes stdint.h and defines fdt*_t, but it messes me up in u-boot where (currently) libfdt_env.h does *not* include stdint.h... Ok, so, the uboot libfdt_env.h should be fixed to define uintXX_t and fdtXX_t (either by including stdint or my other means). The purpose of libfdt_env.h is to define the things that libfdt requires, and those types are (now) such a requirement. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: Digital signature ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/5] treewide: include libfdt_env.h before fdt.h
On Thu, Jan 17, 2013 at 11:54:56AM -0600, Kim Phillips wrote: On Wed, 16 Jan 2013 18:36:03 -0600 Scott Wood scottw...@freescale.com wrote: On 01/16/2013 05:59:04 PM, Kim Phillips wrote: and, if including libfdt.h which includes libfdt_env.h in the correct order, don't include fdt.h before libfdt.h. this is needed to get the fdt type definitions set from the project environment before fdt.h uses them. Signed-off-by: Kim Phillips kim.phill...@freescale.com Cc: Jerry Van Baren gvb.ub...@gmail.com Maybe fdt.h should include libfdt_env.h? Or just always use libfdt.h as the public header. Was just following along the same lines as the dtc commits 38ad79d3 dtc/tests: don't include fdt.h prior to libfdt.h and 20b866a7 dtc/fdtdump: include libfdt_env.h prior to fdt.h, acked by David G. I don't know why some only include fdt.h. devicetree-discuss/David: is there a prescribed way to go here? Change all fdt.h includers to just always include libfdt.h instead of libfdt_env.h prior to fdt.h? Yeah, I think just including libfdt.h instead of fdt.h is the way to go. The distinction is that fdt.h contains only passive declarations. That is, defines and structure/type declarations but no function prorotypes or other code. In particular that means that it is strictly about the FDT tree structure only, and not any of the libfdt specific entry points. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: Digital signature ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 3/4] dtc/libfdt: introduce fdt types for annotation by endian checkers
On Wed, Nov 28, 2012 at 05:33:01PM -0600, Kim Phillips wrote: Projects such as linux and u-boot run sparse on libfdt. libfdt contains the notion of endianness via usage of endian conversion functions such as fdt32_to_cpu. As such, in order to pass endian checks, libfdt has to annotate its fdt variables such that sparse can warn when mixing bitwise and regular integers. This patch adds these new fdtXX_t types and, ifdef __CHECKER__ (a symbol sparse defines), includes the bitwise annotation. Signed-off-by: Kim Phillips kim.phill...@freescale.com Much better, thanks Acked-by: David Gibson da...@gibson.dropbear.id.au -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 3/4] dtc/libfdt: introduce fdt types for annotation by endian checkers
On Wed, Nov 14, 2012 at 11:12:04PM -0600, Kim Phillips wrote:
On Thu, 15 Nov 2012 15:43:40 +1100
David Gibson da...@gibson.dropbear.id.au wrote:
On Wed, Nov 14, 2012 at 06:59:58PM -0600, Kim Phillips wrote:
+#define EXTRACT_BYTE(x, n) ((unsigned long long)((uint8_t *)x)[n])
+#define __SWAB16(x) ((EXTRACT_BYTE(x, 0) 8) | EXTRACT_BYTE(x, 1))
+#define __SWAB32(x) ((EXTRACT_BYTE(x, 0) 24) | (EXTRACT_BYTE(x, 1)
16) | \
+ (EXTRACT_BYTE(x, 2) 8) | EXTRACT_BYTE(x, 3))
+#define __SWAB64(x) ((EXTRACT_BYTE(x, 0) 56) | (EXTRACT_BYTE(x, 1)
48) | \
+ (EXTRACT_BYTE(x, 2) 40) | (EXTRACT_BYTE(x, 3) 32) | \
+ (EXTRACT_BYTE(x, 4) 24) | (EXTRACT_BYTE(x, 5) 16) | \
+ (EXTRACT_BYTE(x, 6) 8) | EXTRACT_BYTE(x, 7))
This is not right, or at least very misleading. swab usually refers
to an unconditional byteswap. But the macros above are nops on a
big-endian machine.
but they don't get called on a big endian system.
Yes, which means the nop-on-bigendian is double-implemented which is
also ugly.
This is the name
linux uses.
I'm not sure exactly which *swab* functions in Linux you're referring
to, but I'm pretty sure most of those are unconditional swaps.
If you want them renamed, please suggest names - I
can't read your mind.
Well, FDT_TO_CPU would do.
-static inline uint32_t fdt32_to_cpu(uint32_t x)
-{
- return (EXTRACT_BYTE(0) 24) | (EXTRACT_BYTE(1) 16) |
(EXTRACT_BYTE(2) 8) | EXTRACT_BYTE(3);
+/*
+ * determine host endianness:
+ * *__first_byte is 0x11 on big endian systems
+ * *__first_byte is 0x44 on little endian systems
+ */
+static const uint32_t __native = 0x11223344u;
+static const uint8_t *__first_byte = (const uint8_t *)__native;
+
+#define DEF_FDT_TO_CPU(bits) \
+static inline uint##bits##_t fdt##bits##_to_cpu(fdt##bits##_t x) \
+{ \
+ if (*__first_byte == 0x11) \
+ return (__force uint##bits##_t)x; \
+ else \
+ return (__force uint##bits##_t)__SWAB##bits(x); \
}
-#define cpu_to_fdt32(x) fdt32_to_cpu(x)
+DEF_FDT_TO_CPU(16)
+DEF_FDT_TO_CPU(32)
+DEF_FDT_TO_CPU(64)
In fact, I really don't see why you're rewriting the byteswapper
functions as part of this patch. The existing versions aren't very
nice, but if you want to rewrite those, please do it in a separate
patch.
This patchseries is about silencing sparse warnings in linux,
u-boot, and libfdt. Sparse is intelligent in that if you mismatch a
native type of value 0 to a bitwise restricted type, it won't call a
warning. The existing short-circuiting of the byteswapper
functions, i.e., defining cpu_to_fdt32(x) to fdt32_to_cpu(x) and
vice versa doesn't allow for correct attribution propagation.
Ah, right, yes that will have to go. You've also added the explicit
endianness test, though, which is a redundant change.
Therefore I chose to allow sparse to see the actual conversion. If
you have any other ideas on how to silence sparse in libfdt, let me
know.
Hrm. So you could either rename the macros, or just duplicate the
code in the to versions of the functions.
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 3/4] dtc/libfdt: introduce fdt types for annotation by endian checkers
On Wed, Nov 14, 2012 at 06:59:58PM -0600, Kim Phillips wrote:
Projects such as linux and u-boot run sparse on libfdt. libfdt
contains the notion of endianness via usage of endian conversion
functions such as fdt32_to_cpu. As such, in order to pass endian
checks, libfdt has to annotate its fdt variables such that sparse
can warn when mixing bitwise and regular integers. This patch adds
these new fdtXX_t types and, ifdef __CHECKER__ (a symbol sparse
defines), includes the bitwise annotation.
Signed-off-by: Kim Phillips kim.phill...@freescale.com
---
v2:
adds bitwise awareness: determine host endianness manually, and
annotate swabs with __force in fdtXX_to_cpu and cpu_to_fdtXX
conversion functions (the inline endian condition checks are
optimized out at compile time). This allows us to be able to check
libfdt bitwise conversions with sparse by building dtc with make
CC=cgcc. v2 also moves fdt32 definitions from fdt.h to libfdt_env.h
and changes fdt32 definitions to use __bitwise instead of __be32. No
separate _FDT_SPARSE was introduced because there's no need: using
__CHECKER__ directly is valid because it only occurs once, and in
libfdt_env.h.
In addition, the libfdt sparse fixes have been moved to a subsequent
patch.
v3: address comments from jdl:
o single set of fdt typedefs, since __bitwise is not defined if !CHECKER
o re-work EXTRACT_BYTE to take 'x' as a parameter, not a global
o SWAB function macros converted to take lowercase 'x' as a parameter,
not a global
libfdt/fdt.h| 42
libfdt/libfdt_env.h | 69
+
2 files changed, 75 insertions(+), 36 deletions(-)
diff --git a/libfdt/fdt.h b/libfdt/fdt.h
index 48ccfd9..45dd134 100644
--- a/libfdt/fdt.h
+++ b/libfdt/fdt.h
[snip]
diff --git a/libfdt/libfdt_env.h b/libfdt/libfdt_env.h
index 213d7fb..f0d97b9 100644
--- a/libfdt/libfdt_env.h
+++ b/libfdt/libfdt_env.h
@@ -5,25 +5,64 @@
#include stdint.h
#include string.h
-#define EXTRACT_BYTE(n) ((unsigned long long)((uint8_t *)x)[n])
-static inline uint16_t fdt16_to_cpu(uint16_t x)
-{
- return (EXTRACT_BYTE(0) 8) | EXTRACT_BYTE(1);
-}
-#define cpu_to_fdt16(x) fdt16_to_cpu(x)
+#ifdef __CHECKER__
+#define __force __attribute__((force))
+#define __bitwise __attribute__((bitwise))
+#else
+#define __force
+#define __bitwise
+#endif
+
+typedef uint16_t __bitwise fdt16_t;
+typedef uint32_t __bitwise fdt32_t;
+typedef uint64_t __bitwise fdt64_t;
I agree with Jon that the approach above is better than the earlier one.
+#define EXTRACT_BYTE(x, n) ((unsigned long long)((uint8_t *)x)[n])
+#define __SWAB16(x) ((EXTRACT_BYTE(x, 0) 8) | EXTRACT_BYTE(x, 1))
+#define __SWAB32(x) ((EXTRACT_BYTE(x, 0) 24) | (EXTRACT_BYTE(x, 1) 16)
| \
+ (EXTRACT_BYTE(x, 2) 8) | EXTRACT_BYTE(x, 3))
+#define __SWAB64(x) ((EXTRACT_BYTE(x, 0) 56) | (EXTRACT_BYTE(x, 1) 48)
| \
+ (EXTRACT_BYTE(x, 2) 40) | (EXTRACT_BYTE(x, 3) 32) | \
+ (EXTRACT_BYTE(x, 4) 24) | (EXTRACT_BYTE(x, 5) 16) | \
+ (EXTRACT_BYTE(x, 6) 8) | EXTRACT_BYTE(x, 7))
This is not right, or at least very misleading. swab usually refers
to an unconditional byteswap. But the macros above are nops on a
big-endian machine.
-static inline uint32_t fdt32_to_cpu(uint32_t x)
-{
- return (EXTRACT_BYTE(0) 24) | (EXTRACT_BYTE(1) 16) |
(EXTRACT_BYTE(2) 8) | EXTRACT_BYTE(3);
+/*
+ * determine host endianness:
+ * *__first_byte is 0x11 on big endian systems
+ * *__first_byte is 0x44 on little endian systems
+ */
+static const uint32_t __native = 0x11223344u;
+static const uint8_t *__first_byte = (const uint8_t *)__native;
+
+#define DEF_FDT_TO_CPU(bits) \
+static inline uint##bits##_t fdt##bits##_to_cpu(fdt##bits##_t x) \
+{ \
+ if (*__first_byte == 0x11) \
+ return (__force uint##bits##_t)x; \
+ else \
+ return (__force uint##bits##_t)__SWAB##bits(x); \
}
-#define cpu_to_fdt32(x) fdt32_to_cpu(x)
+DEF_FDT_TO_CPU(16)
+DEF_FDT_TO_CPU(32)
+DEF_FDT_TO_CPU(64)
In fact, I really don't see why you're rewriting the byteswapper
functions as part of this patch. The existing versions aren't very
nice, but if you want to rewrite those, please do it in a separate
patch.
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] libfdt: introduce fdt type annotation for use by endian checkers
On Tue, Oct 30, 2012 at 04:57:54PM -0500, Kim Phillips wrote: Projects such as linux and u-boot run sparse on libfdt. libfdt contains the notion of endianness via usage of endian conversion functions such as fdt32_to_cpu. As such, in order to pass endian checks, libfdt has to annotate its fdt variables as big endian. This patch does that ifdef __CHECKER__ (a symbol sparse defines), for two new fdt types: fdt32_t and fdt64_t, and subsequently silences warnings emitted by sparse when parsing libfdt. Signed-off-by: Kim Phillips kim.phill...@freescale.com --- note: wasn't sure whether to introduce the new fdt32 types, or just have libfdt use __be32 directly. I prefer having the fdt32 types, to match the cpu_to_fdt32() and so forth functions we already use. So I like this in principle, but a couple of nits. First, I'd really like to see an accompanying patch that adds targets to the dtc makefiles to run sparse over the sources. I couldn't really test this, because I couldn't figure out quite what options I needed to invoke sparse with to get it to work properly. And I'd also like to see the default libfdt_env.h updated to supply the necessary sparse stuff - including the necessary __force casts in its byteswap functions. [snip] diff --git a/libfdt/fdt.h b/libfdt/fdt.h index 48ccfd9..0d9c856 100644 --- a/libfdt/fdt.h +++ b/libfdt/fdt.h @@ -3,46 +3,54 @@ #ifndef __ASSEMBLY__ +#ifdef __CHECKER__ So, I'd prefer not to use __CHECKER__ directly here. I'd rather we defined a new specific symbol, that libfdt_env.h can set based on __CHECKER__ if it wants. Let's say _FDT_SPARSE The reason is because.. +typedef __be32 fdt32_t; +typedef __be64 fdt64_t; ..just running sparse does *not* immediately give you __be32 and __be64 types - those have to be defined in terms of __attribute__((bitwise)) and whatnot. Your uboot env probably does that already, but the default libfdt_env.h certainly doesn't. Effectively _FDT_SPARSE is sayint two things, first that we're compiling under sparse, but also that we have suitably defined endian types in the environment. Actually, given that libfdt_env.h is already required to provide the cpu_to_fdt32() and so forth macros, I think it's slightly neater to just require it to directly supply the fdt32_t etc. types when it defines _FDT_SPARSE as well, rather than defining them in terms of __beXX here then in terms of the attributes in the environment. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] Merging device trees at runtime for module-based systems
On Thu, Nov 01, 2012 at 10:24:06AM +0100, Daniel Mack wrote: On 01.11.2012 04:26, David Gibson wrote: On Fri, Oct 26, 2012 at 09:24:11AM +0200, Daniel Mack wrote: I would especially like to know where such a new functionality should live, which data types it should operate on and what would be an appropriate name for it. So.. the first thought I have reading the original mail in the thread is that it's arguable that you really want a more heavyweight firmware for this setup, that actively maintains a live device tree as OF does, rather than u-boot which is pretty oriented towards a close-to-static device setup. That's just a thought though, I'm not saying that at least some of this functionality doesn't belong in libfdt. So, my thought would be that stuff for manipulating big chunks of tree should go in a new .c file inside the libfdt tree. We already have del_node and nop_node of course, which can remove whole subtrees. I guess the big extra function you'd want would be something like: fdt_graft(void *fdt, int offset, void *subtree); Which would graft the tree blob give by subtree into the master tree (fdt) at node 'offset'. Actually that might need to take a name for the top-level of the subtree to take in the new tree too. I called the function fdt_overlay, but I guess the implementation is similar to what you thought of. I pushed it here, see the topmost 3 commits: https://github.com/zonque/dtc/commits/overlay Interesting. So, it seems to me that fdt_graft() and fdt_overlay() are different operations - both could be potentially useful. fdt_graft() would attach a new subtree somewhere within the master tree, with the assumption that the root of the subtree would become a new node in the resulting tree. Overwriting an existing subtree with a new one would be an error for a graft. fdt_overlay, as you've implemented, can either add new nodes or modify existing ones by replacing or adding new properties. So, some notes on the actual implementation: The in-place modification of the given path (which should really be const char *) in your fdt_add_subnode_r() is nasty, nasty, nasty. And it's unnecessary because you can use the existing fdt_add_subnode_namelen() to work with subsections of the path without needing to either have a temporary buffer or do in-place modification. ...except, I don't think you actually need fdt_add_subnoode_r() for your overlay implementation in any case. AFAICT in your fdt_overlay() implementation you're only adding nodes from the second tree if they contain properties (the fdt_add_subnode_r() call is under the FDT_PROP case). I'm not sure if that was a deliberate policy decision - if so I really can't see a reason for it. If instead you *always* add subnodes when they exist in the second tree, you'll be doing your add nodes from the FDT_BEGIN_NODE tag case. And you always get BEGIN_NODE tags for parents before subnodes, so you can naturally add your new subnode path component by component without having to walk down the path again in fdt_add_subnode_r(). As an added bonus you no longer need pathbuf and it's arbitrary size limit. Hrm.. wait... I guess you need a stack so you can handle FDT_END_NODE correctly. I suspect a recursive solution (effectively using the machine stack) would still take less (machine) stack space than pathbuf. Especially if pathbuf was increased up to PATH_MAX, which is my usual rule of thumb when I can't avoid an arbitrary buffer size. On a tangent, note that fdt_graft() as defined above, unlike fdt_overlay() would allow a considerably optimized implementation. Instead of doing lots of individual inserts into the tree (and therefore a lot of memmove()s), you could do one big _fdt_splice(), copy in the grafted tree's structure block, then run through it correcting property name offsets. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] Merging device trees at runtime for module-based systems
On Wed, Oct 31, 2012 at 10:36:08PM -0600, Stephen Warren wrote: On 10/31/2012 05:56 PM, Mitch Bradley wrote: On 10/31/2012 1:00 PM, Daniel Mack wrote: cc devicetree-discuss. Here's a reference to the full thread: http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/145221/ On 26.10.2012 20:39, Stephen Warren wrote: On 10/24/2012 03:47 AM, Daniel Mack wrote: Hi, a project I'm involved in uses a module/baseboard combo, and components on either board are described in DT. I'm currently using separate dts files which build upon each other with include statements, which works fine for development. In production though, we will certainly have running changes (and hence different versions) over the lifetime of the product for both the baseboard and the module, and the hardware has support for identifying the versions of both sides at runtime. ... I start to believe that the cleanest solution to this would be to have full DTC functionality in U-Boot and compile the tree ... which is exactly the way that Open Firmware does it, since the invention of the device tree. The model is that the boot firmware, which needs to know the system configuration to do its job anyway, exports that configuration via the device tree. Doesn't OF generate the DT from internal data structures (although I don't know where those come from...), Well.. in OF the device tree *is* a core live data structure. It will be constructed as devices are probed, firmware level device drivers are attached to it and it might be modified by client interface operations. Traditionally there is no tree in the flattened format, only the live tree which clients will query with OF calls. Some recent OF implementations do use the flat tree in some ways - we've had some systems where a flattened tree is provided by an early boot to the full OF; it acts as a skeleton that is then extended in OF's live tree. We've also had some OF implementations that for compatiblity with kernels that expect a flattened tree flatten their internal tree structure into the FDT format as the last thing before entering the OS. whereas what Daniel mentions above is more like the bootloader having access to a bunch of .dts fragments, selecting the appropriate subset of those to use, parsing them into an internal data structure (i.e. running dtc), and then generating a DTB from it. The overall result is that the bootloader causes a DTB to be generated at run-time, so at that level it's the same, but the implementation seems pretty different. How OF constructs its internal tree can vary a lot with the OF implementation, and I'm not terribly clear on what you had in mind, so I don't think either side is really sufficiently well defined to really say if they're similar or not. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] Merging device trees at runtime for module-based systems
On Fri, Oct 26, 2012 at 09:24:11AM +0200, Daniel Mack wrote: On 26.10.2012 02:53, David Gibson wrote: On Thu, Oct 25, 2012 at 10:46:32PM +0200, Wolfgang Denk wrote: Dear Daniel, In message 50893633.6070...@gmail.com you wrote: Overwrites must be addressed in the first place. The most common example is that a more generic part (the module tree) registers all details about a peripheral up-front but then sets its status to 'disabled'. That way, the more specific part (the base board tree) can overwrite this property to 'okay' at wish to enable it and not care for the pre-defined details. This is also how we do things in our device-trees. Agreed. I definitely can see the benefit of such a feature and would be happy if you could go forward and implement it. Ok then. I guess this should be something that can eventually be merged back into libfdt? I can't speak for the FDT custodian, but I think this makes a lot of sense. As a rule I'm happy to see more functionality for libfdt. I've only seen bits and pieces of this thread, though, so I'd need to see a summary of what exactly is being proposed. That's strange, as I copied you from the very first posting. Anyway, here's the archive: Oh I probably got them somewhere in my mail. But it's only recently that I really noticed - I get a fair bit of mail. http://lists.denx.de/pipermail/u-boot/2012-October/138227.html I would especially like to know where such a new functionality should live, which data types it should operate on and what would be an appropriate name for it. So.. the first thought I have reading the original mail in the thread is that it's arguable that you really want a more heavyweight firmware for this setup, that actively maintains a live device tree as OF does, rather than u-boot which is pretty oriented towards a close-to-static device setup. That's just a thought though, I'm not saying that at least some of this functionality doesn't belong in libfdt. So, my thought would be that stuff for manipulating big chunks of tree should go in a new .c file inside the libfdt tree. We already have del_node and nop_node of course, which can remove whole subtrees. I guess the big extra function you'd want would be something like: fdt_graft(void *fdt, int offset, void *subtree); Which would graft the tree blob give by subtree into the master tree (fdt) at node 'offset'. Actually that might need to take a name for the top-level of the subtree to take in the new tree too. Things get trickier when you consider what might need to be tweaked in the subtree to make it fit into the master tree. If it requires widespread alterations through the subtree that's going to get really ugly and I think you would be better off with a firmware with a fuller handling of a live device tree. But I think that can probably be avoided with proper design of the bindings. To get that to work you'll need to make sure you use some sort of local addressing within the subtree. Then it should only be necessary to insert/alter a ranges property at the top level of the subtree (or possibly its parent) to map that correctly into the global address space. Likewise interrupts within the subtree probably shouldn't address an external interrupt controller but rather the root of the tree. You can then insert an interrupt-map property which will wire those into the global interrupt tree. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 05/14] fdt: Export fdtdec_find_alias_node() function
On Thu, Oct 25, 2012 at 07:31:02PM -0700, Simon Glass wrote: This function is useful outside fdtdec, so export it. Hrm. fdt_path_offset() in libfdt itself will already look up aliases if given a path that doesn't start with '/'. So it's not clear why you need this function at all. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] Merging device trees at runtime for module-based systems
On Thu, Oct 25, 2012 at 10:46:32PM +0200, Wolfgang Denk wrote: Dear Daniel, In message 50893633.6070...@gmail.com you wrote: Overwrites must be addressed in the first place. The most common example is that a more generic part (the module tree) registers all details about a peripheral up-front but then sets its status to 'disabled'. That way, the more specific part (the base board tree) can overwrite this property to 'okay' at wish to enable it and not care for the pre-defined details. This is also how we do things in our device-trees. Agreed. I definitely can see the benefit of such a feature and would be happy if you could go forward and implement it. Ok then. I guess this should be something that can eventually be merged back into libfdt? I can't speak for the FDT custodian, but I think this makes a lot of sense. As a rule I'm happy to see more functionality for libfdt. I've only seen bits and pieces of this thread, though, so I'd need to see a summary of what exactly is being proposed. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 00/32] Initial sparse fix series
tags we can locate. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 00/32] Initial sparse fix series
On Thu, Oct 18, 2012 at 05:30:22PM -0500, Kim Phillips wrote: On Thu, 18 Oct 2012 23:11:12 +1100 David Gibson da...@gibson.dropbear.id.au wrote: On Wed, Oct 17, 2012 at 08:19:23PM -0400, Jerry Van Baren wrote: Hi David, Jon, Kim Phillips created a series of patches to change variable declarations that are big endian to be __be32/__be64. Since the device tree is defined to be big endian, he created a patch to mark the appropriate libfdt entities as __be*. So, in general I approve the idea of having endian annotations. Obviously we do need to make sure it doesn't break things. cool. On 10/16/2012 08:28 PM, Kim Phillips wrote: This 32-patch series only begins to address making u-boot source more 'sparseable,' or sparse-clean, ultimately to catch type, address space, and endianness mismatches and generally improve code quality. E.g., in this initial dose whose main purpose is to reduce the output volume to workable levels, a couple of endianness bugs are found and fixed in of_bus_default_translate() and fdt_get_base_address(). See [PATCH 14/32] common/fdt_support.c: sparse fixes. Upside: This is very good for identifying endian errors early. Downside: It could break/complicate non-linux uses of libfdt. This shouldn't be an inherent problem - we can always have the default behaviour be that be32 etc. are #defined to be uint32_t, and we only turn on real annotations when we have the right setup. It does complicate things a bit, but I think it should be manageable. I'd much prefer to see this done against the upstream dtc/libfdt, of course, rather than the u-boot copy. understood. What are your thoughts on this quest? [snip] Note that there are two libfdt dependencies: [snipped #1, u-boot-fdt dependency, NBD] 2. potential upstream dtc change dependencies, due to having to attribute base device tree header types to __be32 in include/libfdt. See patch 19/32 include/fdt.h: sparse fixes. It is unknown whether such changes would be welcome to dtc (but there's a way to find out :). [snip] Build-tested in both endians :). Please help test. Thanks, Kim Below is the actual patch for reference (it was in a separate email). The impact in terms of changed lines is pretty small. I'm not sure how this impacts non-linux / non-gcc systems since the sparse checker comes from a linux background and uses gcc extensions. Possibly this could be handled a definition: #ifndef _LINUX_TYPES_H typedef uint32_t __be32 typedef uint64_t __be64 #endif Yes, something along those lines would be appropriate, although I think that condition isn't right. right, we don't want all uint32_t's to be one endian, we just want fdt32 types to have big endian annotation when being checked by a checker such as sparse. Right. include/fdt.h | 33 + include/fdt_support.h | 2 ++ include/libfdt.h | 4 ++-- lib/libfdt/fdt.c | 2 +- lib/libfdt/fdt_ro.c | 2 +- lib/libfdt/fdt_rw.c | 4 ++-- lib/libfdt/fdt_sw.c | 4 ++-- lib/libfdt/fdt_wip.c | 2 +- 8 files changed, 28 insertions(+), 25 deletions(-) diff --git a/include/fdt.h b/include/fdt.h index c51212e..1b7f044 100644 --- a/include/fdt.h +++ b/include/fdt.h @@ -2,40 +2,41 @@ #define _FDT_H #ifndef __ASSEMBLY__ +#include asm/byteorder.h This change, however, is not acceptable. libfdt is supposed to compile in a wide range of environments (such as bootloaders and firmwares) which may be very different from a typical Unix userland environment. As such all the headers are built to have minimal dependencies on external libraries. The byteorder headers are, alas, horribly non-portable even amongst otherwise similar Unix systems, so right out for libfdt. In fact, the way this is supposed to work is that the *only* external header directly included by the fdt headers is libfdt_env.h. It is supplied by the surrounding build environment and is responsible for providing the minimal things that libfdt does require. Roughly speaking that's: stdint like types, some byteswap functions and a some string.h prototypes - exactly what's required should be better documented than it currently is. libfdt_env.h can do this either by including appropriate pre-existing headers from the environment, or by directly defining the required things, whichever makes sense. The libfdt_env.h which is shipped with libfdt is essentially just an example, for use in the environment of POSIX like userspace. Anyway, I think the right way to handle this is to decide on a name like __FDT_ANNOTATIONS__ or something. this is named __CHECKER__ in linux and u-boot. Ifdef __CHECKER__, then the code
Re: [U-Boot] Subject: [PATCH] libfdt: Add fdt functionality for more intuitive fdt handling
On Mon, May 07, 2012 at 02:56:54PM +0200, Peter Feuerer wrote:
libfdt: Add fdt functionality for more intuitive fdt handling
Hrm, more intuitive fdt handling is a bit of a stretch. It makes
certain common operations simpler.
New functions:
fdt_read - retrieve the value of a property by full path
fdt_write - create or change a property with full path and create subnodes if
needed
I don't love these names (too vague). fdt_{get,set}prop_global()
perhaps.
fdt_create_path - create subnode path with parents
Signed-off-by: Peter Feuerer peter.feue...@sysgo.com
CC: David Gibson da...@gibson.dropbear.id.au
CC: Gerald Van Baren g...@unssw.com
---
include/libfdt.h | 93
lib/libfdt/fdt_ro.c | 30 +
lib/libfdt/fdt_rw.c | 97
++
lib/libfdt/libfdt_internal.h |4 ++
4 files changed, 224 insertions(+), 0 deletions(-)
You're patching the core libfdt code, so you should make a patch
against upstream libfdt, rather than just the u-boot embedded copy.
diff --git a/include/libfdt.h b/include/libfdt.h
index de82ed5..822ab18 100644
--- a/include/libfdt.h
+++ b/include/libfdt.h
@@ -548,6 +548,37 @@ static inline void *fdt_getprop_w(void *fdt, int
nodeoffset,
}
/**
+ * fdt_read - retrieve the value of a property by full path
+ * @fdt: pointer to the device tree blob
+ * @path: string containing the absolute path of the property
+ * @name: name of the property
+ * @lenp: pointer to an integer variable (will be overwritten) or NULL
+ *
+ * fdt_getprop() retrieves a pointer to the value of the property
This doesn't appear to have been updated for the new function.
+ * named 'name' of the node at offset nodeoffset (this will be a
+ * pointer to within the device blob itself, not a copy of the value).
+ * If lenp is non-NULL, the length of the property value is also
+ * returned, in the integer pointed to by lenp.
+ *
+ * returns:
+ * pointer to the property's value
+ * if lenp is non-NULL, *lenp contains the length of the property
+ * value (=0)
+ * NULL, on error
+ * if lenp is non-NULL, *lenp contains an error code (0):
+ * -FDT_ERR_NOTFOUND, node does not have named property
+ * -FDT_ERR_BADOFFSET, nodeoffset did not point to FDT_BEGIN_NODE
tag
+ * -FDT_ERR_BADPATH,
+ * -FDT_ERR_BADMAGIC,
+ * -FDT_ERR_BADVERSION,
+ * -FDT_ERR_BADSTATE,
+ * -FDT_ERR_BADSTRUCTURE,
+ * -FDT_ERR_TRUNCATED, standard meanings
+ */
+const void *fdt_read(const void *fdt, const char *path,
+ const char *name, int *lenp);
+
+/**
* fdt_get_phandle - retrieve the phandle of a given node
* @fdt: pointer to the device tree blob
* @nodeoffset: structure block offset of the node
@@ -1066,6 +1097,38 @@ int fdt_set_name(void *fdt, int nodeoffset, const char
*name);
*/
int fdt_setprop(void *fdt, int nodeoffset, const char *name,
const void *val, int len);
+/**
+ * fdt_write - create or change a property with full path and create
+ * all subnodes to the property if needed
+ * @fdt: pointer to the device tree blob
+ * @path: string containing the absolute path of the property
+ * @name: name of the property to change
+ * @val: pointer to data to set the property value to
+ * @len: length of the property value
+ *
+ * fdt_write() sets the value of the named property in the given
+ * node to the given value and length, creating the property if it
+ * does not already exist. Additionally it creates all parent nodes if
+ * not yet existing.
+ *
+ * This function may insert or delete data from the blob, and will
+ * therefore change the offsets of some existing nodes.
+ *
+ * returns:
+ * 0, on success
+ * -FDT_ERR_NOSPACE, there is insufficient free space in the blob to
+ * contain the new property value
+ * -FDT_ERR_BADOFFSET, nodeoffset did not point to FDT_BEGIN_NODE tag
+ * -FDT_ERR_BADLAYOUT,
+ * -FDT_ERR_BADMAGIC,
+ * -FDT_ERR_BADVERSION,
+ * -FDT_ERR_BADSTATE,
+ * -FDT_ERR_BADSTRUCTURE,
+ * -FDT_ERR_BADLAYOUT,
+ * -FDT_ERR_TRUNCATED, standard meanings
+ */
+int fdt_write(void *fdt, const char *path, const char *name,
+ const void *val, int len);
/**
* fdt_setprop_cell - set a property to a single cell value
@@ -1204,6 +1267,36 @@ int fdt_add_subnode_namelen(void *fdt, int
parentoffset,
int fdt_add_subnode(void *fdt, int parentoffset, const char *name);
/**
+ * fdt_create_path - create subnode path with parents
+ * @fdt: pointer to the device tree blob
+ * @path: absolute path of subnodes to create
+ *
+ * fdt_create_path() creates absolute subnode path with all needed
+ * parents, if they don't exist.
+ *
+ * This function will insert data into the blob, and will therefore
+ * change
Re: [U-Boot] [PATCH 01/14] fdt: Tidy up a few fdtdec problems
On Mon, Nov 28, 2011 at 11:33:22AM -0700, Stephen Warren wrote:
On 11/23/2011 08:54 PM, Simon Glass wrote:
This fixes three trivial issues in fdtdec.c:
1. fdtdec_get_is_enabled() doesn't really need a default value
2. The fdt must be word-aligned, since otherwise it will fail on ARM
3. The compat_names[] array is missing its first element
diff --git a/lib/fdtdec.c b/lib/fdtdec.c
...
#define COMPAT(id, name) name
static const char * const compat_names[COMPAT_COUNT] = {
+ COMPAT(UNKNOWN, none),
};
Could you educate me on why that change is necessary? Maybe explain this
in the commit description?
-int fdtdec_get_is_enabled(const void *blob, int node, int default_val)
+int fdtdec_get_is_enabled(const void *blob, int node)
{
const char *cell;
cell = fdt_getprop(blob, node, status, NULL);
if (cell)
return 0 == strcmp(cell, ok);
- return default_val;
+ return 1;
}
Not that this patch changes this, but isn't okay also a legal
enabled value, and perhaps even the recommended value? See
http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-July/006389.html.
Yes. okay, not ok is the standard value for the status property.
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 02/14] fdt: Add functions to access phandles, arrays and bools
On Mon, Nov 28, 2011 at 11:41:32AM -0700, Stephen Warren wrote: On 11/23/2011 08:54 PM, Simon Glass wrote: Add a function to lookup a property which is a phandle in a node, and another to read a fixed-length integer array from an fdt property. Also add a function to read boolean properties. Signed-off-by: Simon Glass s...@chromium.org Looking at the U-Boot custodians web page, you need to send the core DT changes (well, probably anything DT related) to Jerry Van Baren. +/** + * Look up a property in a node and return its contents in an integer + * array of given length. The property must have at least enough data for + * the array (4*count bytes). It may have more, but this will be ignored. + * + * @param blob FDT blob + * @param node node to examine + * @param prop_namename of property to find + * @param arrayarray to fill with data + * @param countnumber of array elements + * @return 0 if ok, or -FDT_ERR_NOTFOUND if the property is not found, + * or -FDT_ERR_BADLAYOUT if not enough data + */ +int fdtdec_get_int_array(const void *blob, int node, const char *prop_name, + int *array, int count); The kernel's equivalent of this function retrieves an array of U32s. Is one version more correct than the other? Using u32 is a better idea. The property formats are all defined in terms of fixed width elements, so using a vague width type like int to interact with it is a bad idea. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] Adding new partition in uboot
On Sun, Mar 21, 2010 at 06:13:44PM -0400, Sagar Heroorkar wrote:
Hi David,
I was exploring the ways to add partition into the blob dynamically.
I followd the following steps.
1) say we have 5 partitions. Flash size is 128mb
norfl...@0,0{
1--
2--
3
5
partit...@f8 {
label = u_booot;
reg = 0xf8 0x6;
};
2) i am trying to add 6th partition dynamically in uboot.
I used the the nodeoffset of norfl...@0,0 which is parent offset wher i
want to create 6th partiton. I passed this parent offset to
ret = fdt_add_subnode(blob,nodeoffset,
partit...@680);
You should check ret for errors ( 0) before continuing.
nodeoffset = ret;
ptr[0] = 0x680;
ptr[1] = 0x80;
How is ptr declared?
offset = 0x680;
regs[0] += size_delta;
How is regs[0] initialized?
memcpy(regs, ptr, plen);
Where did plen come from?
ret = fdt_setprop(blob, nodeoffset, reg, regs, plen);
You should check for errors here too.
I am printing the the blob later. i see that node is created but the last
partition ie u-boot @ f8 is going away.
the blob size is 8000 byes which we built using the
dtc -S 8000 -R 7 -I dts -O dtb -o file.dtb file.dts
Let me know if we need to increase size of the blob or something which i m
missing.
This is going to help a lot if you can reply.
For this case where there's a flash partition that's sometimes there
and sometimes not, it might be simpler to put all the partitions,
including the not-always-present ones in the dts. Then you can use
fdt_nop_subnode() to remove the extra one on systems where it's not
present.
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] dtc/u-boot tool naming, ftdump, mkimage
On Wed, Jan 14, 2009 at 02:19:05PM -0500, Josh Boyer wrote: On Wed, Jan 14, 2009 at 02:11:22PM -0500, Jerry Van Baren wrote: Hi Matt, Matt Sealey wrote: Josh Boyer wrote: On Wed, Jan 14, 2009 at 11:57:42AM -0600, Matt Sealey wrote: I just noticed (was told by an affiliate) that the DTC compiler tools shares a tool name ftdump with the Freetype project. I was doing a lazy packaging effort to get a few tools around so we can all be running the same version and build some custom kernel RPMs, and this came up. Wouldn't a better name be fdtdump, do you think? It's usually not a fun idea to conflict with tools already on the system. A user may run ftdump and get the Freetype tool because of a simple mistake in paths or so. With regards to U-Boot proper, I would say the same is true of mkimage which conflicts with jigdo. In essence, while U-Boot is usually installed into a user's home directory (~/bin etc.) simple path mixups, going to root shell, using another box etc. means you may have multiple same-named tools on a system for various work, which do different things. Mkimage and ftdump are different audiences (OK, the same people but the chairs are arranged differently ;-). [snip] IMHO, the ftdump issue needs to be addressed at the DTC level (Jon Loeliger and David Gibson). U-Boot (and the linux kernel) is being driven by DTC. David has said before that ftdump is sort of a pointless debugging tool. Or at least that is what the Debian bug report referrenced as the reason from removing it from the Debian dtc package. Yeah, I wouldn't suggest packaging ftdump. It's potentially useful for debugging dtc itself, but if you want to dump a device tree for real, you can use dtc -I dtb -O dts. Maybe we should just remove it entirely if it's not really going to be maintained long-term. Hmm, perhaps. We should definitely remove it from the make install target. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] dtc/u-boot tool naming, ftdump, mkimage
On Wed, Jan 14, 2009 at 06:34:34PM -0600, Matt Sealey wrote: Josh Boyer wrote: David has said before that ftdump is sort of a pointless debugging tool. Or at least that is what the Debian bug report referrenced as the reason from removing it from the Debian dtc package. As long as you have the original device tree source code to hand. I tend to find that the guy who sent me the device tree and kernel for me to test, has gone to bed and will not be back for 8 to 10 hours. Rather than hang around, on the event that I think it's a device tree problem I tend to ftdump to see what's going on. Like I said, dtc -I dtb -O dts is the non-hacky way to do this. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] libfdt: Add function to explicitly expand aliases
On Fri, Oct 03, 2008 at 03:09:06PM -0500, Jon Loeliger wrote: Petri Lehtinen wrote: On Thu, Oct 02, 2008 at 07:05:53PM -0400, Jerry Van Baren wrote: [snip] diff --git a/include/libfdt.h b/include/libfdt.h index 5492a53..7cad68c 100644 --- a/include/libfdt.h +++ b/include/libfdt.h @@ -459,6 +459,32 @@ static inline void *fdt_getprop_w(void *fdt, int nodeoffset, uint32_t fdt_get_phandle(const void *fdt, int nodeoffset); /** + * fdt_get_namelen - get alias based on substring ===^^^=== + * @fdt: pointer to the device tree blob + * @name: name of the alias th look up + * @namelen: number of characters of name to consider + * + * Identical to fdt_get_alias(), but only examine the first namelen + * characters of name for matching the alias name. + */ +const char *fdt_get_alias_namelen(const void *fdt, + const char *name, int namelen); + This should be fdt_get_alias_namelen, right? It does look that way This patch has already been applied upstream, so correcting patches on top of this are welcome. Crap, sorry. I'll send a fixup patch if none of you gets to it first. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] libfdt: Add function to explicitly expand aliases
On Fri, Oct 03, 2008 at 03:09:06PM -0500, Jon Loeliger wrote: Petri Lehtinen wrote: On Thu, Oct 02, 2008 at 07:05:53PM -0400, Jerry Van Baren wrote: [snip] diff --git a/include/libfdt.h b/include/libfdt.h index 5492a53..7cad68c 100644 --- a/include/libfdt.h +++ b/include/libfdt.h @@ -459,6 +459,32 @@ static inline void *fdt_getprop_w(void *fdt, int nodeoffset, uint32_t fdt_get_phandle(const void *fdt, int nodeoffset); /** + * fdt_get_namelen - get alias based on substring ===^^^=== + * @fdt: pointer to the device tree blob + * @name: name of the alias th look up + * @namelen: number of characters of name to consider + * + * Identical to fdt_get_alias(), but only examine the first namelen + * characters of name for matching the alias name. + */ +const char *fdt_get_alias_namelen(const void *fdt, + const char *name, int namelen); + This should be fdt_get_alias_namelen, right? It does look that way This patch has already been applied upstream, so correcting patches on top of this are welcome. libfdt: Fix error in documentation for fdt_get_alias_namelen() Oops, screwed up the function name in the documenting comment for this function. Trivial correction in this patch. Signed-off-by: David Gibson [EMAIL PROTECTED] Index: dtc/libfdt/libfdt.h === --- dtc.orig/libfdt/libfdt.h2008-10-04 14:46:50.0 +1000 +++ dtc/libfdt/libfdt.h 2008-10-04 14:46:56.0 +1000 @@ -459,7 +459,7 @@ static inline void *fdt_getprop_w(void * uint32_t fdt_get_phandle(const void *fdt, int nodeoffset); /** - * fdt_get_namelen - get alias based on substring + * fdt_get_alias_namelen - get alias based on substring * @fdt: pointer to the device tree blob * @name: name of the alias th look up * @namelen: number of characters of name to consider -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 0/7] libfdt: Update to resync with dtc/libfdt
On Wed, Aug 20, 2008 at 09:45:04PM -0400, Jerry Van Baren wrote:
[EMAIL PROTECTED] wrote:
The following changesets resynchronize u-boot with the master libfdt.
Best regards,
gvb
First results using aliases with David's libfdt improvements...
These are the aliases:
= fdt p /aliases
aliases {
ethernet0 = /[EMAIL PROTECTED]/[EMAIL PROTECTED];
ethernet1 = /[EMAIL PROTECTED]/[EMAIL PROTECTED];
serial0 = /[EMAIL PROTECTED]/[EMAIL PROTECTED];
serial1 = /[EMAIL PROTECTED]/[EMAIL PROTECTED];
pci0 = /[EMAIL PROTECTED];
};
Dereference an alias by not using the '/' prefix per OF conventions:
= fdt print ethernet0
[EMAIL PROTECTED] {
device_type = network;
compatible = ucc_geth;
cell-index = 0x1;
reg = 0x2000 0x200;
interrupts = 0x20;
interrupt-parent = 0x2;
local-mac-address = [00 00 00 00 00 00];
rx-clock-name = none;
tx-clock-name = clk9;
phy-handle = 0x3;
phy-connection-type = rgmii-id;
pio-handle = 0x4;
};
Whooo-h!
Dereference the ethernet0 alias and print a property:
= fdt print ethernet0/phy-connection-type
libfdt fdt_path_offset() returned FDT_ERR_NOTFOUND
Uh... didn't I talk you out of this broken path-to-property stuff way
back when?
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
