| Message ID | 20190726091339.24420-2-matthias.bgg@kernel.org |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | [U-Boot,1/2] libfdt: fdt_address_cells() and fdt_size_cells() | expand |
Hi all, On 26/07/2019 11:13, matthias.bgg@kernel.org wrote: > From: Matthias Brugger <mbrugger@suse.com> > > According to the device tree specification, the default value for > was not present. > > This patch also makes fdt_address_cells() and fdt_size_cells() conform > to the behaviour documented in libfdt.h. The defaults are only returned > if fdt_getprop() returns -FDT_ERR_NOTFOUND, otherwise the actual error > is returned. > > This is based on upstream commit: > aa7254d ("libfdt: return correct value if #size-cells property is not present") > but misses the test case part, as we don't implement them in u-boot. > > Signed-off-by: Matthias Brugger <mbrugger@suse.com> After running these two patches through the CI [1] I realized that three test are failing: test/py sandbox test/py sandbox with clang test/py sandbox_flattree All three fail dm_test_fdt_translation() in the case "No translation for busses with #size-cells == 0" [2]. Can anybody with more insight in the test infrastructure and the sandbox architecture help me to identify if this is a) a bug in the sandbox b) a bug in our test c) a bug in my patch I write this because I'm pretty sure that it is not option c), as we just stick to the specs here. Regards, Matthias [1] https://travis-ci.org/mbgg/u-boot/builds/565955218 [2] https://github.com/u-boot/u-boot/blob/master/test/dm/test-fdt.c#L511 > --- > scripts/dtc/libfdt/fdt_addresses.c | 16 +++++++++++++--- > scripts/dtc/libfdt/libfdt.h | 2 +- > 2 files changed, 14 insertions(+), 4 deletions(-) > > diff --git a/scripts/dtc/libfdt/fdt_addresses.c b/scripts/dtc/libfdt/fdt_addresses.c > index 49537b578d..f13a87dfa0 100644 > --- a/scripts/dtc/libfdt/fdt_addresses.c > +++ b/scripts/dtc/libfdt/fdt_addresses.c > @@ -64,7 +64,7 @@ static int fdt_cells(const void *fdt, int nodeoffset, const char *name) > > c = fdt_getprop(fdt, nodeoffset, name, &len); > if (!c) > - return 2; > + return len; > > if (len != sizeof(*c)) > return -FDT_ERR_BADNCELLS; > @@ -78,10 +78,20 @@ static int fdt_cells(const void *fdt, int nodeoffset, const char *name) > > int fdt_address_cells(const void *fdt, int nodeoffset) > { > - return fdt_cells(fdt, nodeoffset, "#address-cells"); > + int val; > + > + val = fdt_cells(fdt, nodeoffset, "#address-cells"); > + if (val == -FDT_ERR_NOTFOUND) > + return 2; > + return val; > } > > int fdt_size_cells(const void *fdt, int nodeoffset) > { > - return fdt_cells(fdt, nodeoffset, "#size-cells"); > + int val; > + > + val = fdt_cells(fdt, nodeoffset, "#size-cells"); > + if (val == -FDT_ERR_NOTFOUND) > + return 1; > + return val; > } > diff --git a/scripts/dtc/libfdt/libfdt.h b/scripts/dtc/libfdt/libfdt.h > index 66f01fec53..5c778b115b 100644 > --- a/scripts/dtc/libfdt/libfdt.h > +++ b/scripts/dtc/libfdt/libfdt.h > @@ -1109,7 +1109,7 @@ int fdt_address_cells(const void *fdt, int nodeoffset); > * > * returns: > * 0 <= n < FDT_MAX_NCELLS, on success > - * 2, if the node has no #size-cells property > + * 1, if the node has no #size-cells property > * -FDT_ERR_BADNCELLS, if the node has a badly formatted or invalid > * #size-cells property > * -FDT_ERR_BADMAGIC, >
+Stephen Warren Hi Matthias, On Thu, 1 Aug 2019 at 05:42, Matthias Brugger <matthias.bgg@gmail.com> wrote: > > Hi all, > > On 26/07/2019 11:13, matthias.bgg@kernel.org wrote: > > From: Matthias Brugger <mbrugger@suse.com> > > > > According to the device tree specification, the default value for > > was not present. > > > > This patch also makes fdt_address_cells() and fdt_size_cells() conform > > to the behaviour documented in libfdt.h. The defaults are only returned > > if fdt_getprop() returns -FDT_ERR_NOTFOUND, otherwise the actual error > > is returned. > > > > This is based on upstream commit: > > aa7254d ("libfdt: return correct value if #size-cells property is not present") > > but misses the test case part, as we don't implement them in u-boot. > > > > Signed-off-by: Matthias Brugger <mbrugger@suse.com> > > After running these two patches through the CI [1] I realized that three test > are failing: > test/py sandbox > test/py sandbox with clang > test/py sandbox_flattree > > All three fail dm_test_fdt_translation() in the case "No translation for busses > with #size-cells == 0" [2]. > > Can anybody with more insight in the test infrastructure and the sandbox > architecture help me to identify if this is > a) a bug in the sandbox > b) a bug in our test > c) a bug in my patch > > I write this because I'm pretty sure that it is not option c), as we just stick > to the specs here. Can you check the test and see? It might well be that the test is wrong. I hope we don't have tet code relying on this. Regards, SImon
Hi all, On 13/08/2019 11:34, Simon Glass wrote: > +Stephen Warren > > Hi Matthias, > > On Thu, 1 Aug 2019 at 05:42, Matthias Brugger <matthias.bgg@gmail.com> wrote: >> >> Hi all, >> >> On 26/07/2019 11:13, matthias.bgg@kernel.org wrote: >>> From: Matthias Brugger <mbrugger@suse.com> >>> >>> According to the device tree specification, the default value for >>> was not present. >>> >>> This patch also makes fdt_address_cells() and fdt_size_cells() conform >>> to the behaviour documented in libfdt.h. The defaults are only returned >>> if fdt_getprop() returns -FDT_ERR_NOTFOUND, otherwise the actual error >>> is returned. >>> >>> This is based on upstream commit: >>> aa7254d ("libfdt: return correct value if #size-cells property is not present") >>> but misses the test case part, as we don't implement them in u-boot. >>> >>> Signed-off-by: Matthias Brugger <mbrugger@suse.com> >> >> After running these two patches through the CI [1] I realized that three test >> are failing: >> test/py sandbox >> test/py sandbox with clang >> test/py sandbox_flattree >> >> All three fail dm_test_fdt_translation() in the case "No translation for busses >> with #size-cells == 0" [2]. >> >> Can anybody with more insight in the test infrastructure and the sandbox >> architecture help me to identify if this is >> a) a bug in the sandbox >> b) a bug in our test >> c) a bug in my patch >> >> I write this because I'm pretty sure that it is not option c), as we just stick >> to the specs here. > > Can you check the test and see? It might well be that the test is wrong. > > I hope we don't have tet code relying on this. > I think I found the error. I missed a commit in libftd which fixes the issue. I'll send a v2 soon. Thanks, Matthias
diff --git a/scripts/dtc/libfdt/fdt_addresses.c b/scripts/dtc/libfdt/fdt_addresses.c index 49537b578d..f13a87dfa0 100644 --- a/scripts/dtc/libfdt/fdt_addresses.c +++ b/scripts/dtc/libfdt/fdt_addresses.c @@ -64,7 +64,7 @@ static int fdt_cells(const void *fdt, int nodeoffset, const char *name) c = fdt_getprop(fdt, nodeoffset, name, &len); if (!c) - return 2; + return len; if (len != sizeof(*c)) return -FDT_ERR_BADNCELLS; @@ -78,10 +78,20 @@ static int fdt_cells(const void *fdt, int nodeoffset, const char *name) int fdt_address_cells(const void *fdt, int nodeoffset) { - return fdt_cells(fdt, nodeoffset, "#address-cells"); + int val; + + val = fdt_cells(fdt, nodeoffset, "#address-cells"); + if (val == -FDT_ERR_NOTFOUND) + return 2; + return val; } int fdt_size_cells(const void *fdt, int nodeoffset) { - return fdt_cells(fdt, nodeoffset, "#size-cells"); + int val; + + val = fdt_cells(fdt, nodeoffset, "#size-cells"); + if (val == -FDT_ERR_NOTFOUND) + return 1; + return val; } diff --git a/scripts/dtc/libfdt/libfdt.h b/scripts/dtc/libfdt/libfdt.h index 66f01fec53..5c778b115b 100644 --- a/scripts/dtc/libfdt/libfdt.h +++ b/scripts/dtc/libfdt/libfdt.h @@ -1109,7 +1109,7 @@ int fdt_address_cells(const void *fdt, int nodeoffset); * * returns: * 0 <= n < FDT_MAX_NCELLS, on success - * 2, if the node has no #size-cells property + * 1, if the node has no #size-cells property * -FDT_ERR_BADNCELLS, if the node has a badly formatted or invalid * #size-cells property * -FDT_ERR_BADMAGIC,