On Wed, Jul 18, 2018 at 12:49:52PM +0200, Sebastian Huber wrote:
> Make the default cells compatible with Linux
> (OF_ROOT_NODE_ADDR_CELLS_DEFAULT and OF_ROOT_NODE_SIZE_CELLS_DEFAULT)
> which is one cell (except on SPARC, here the default address cells are
> two).
>
> In case the current node has no cells property, then query the parent,
> and so on. This is in line with the Linux implementation of
> of_n_addr_cells() and of_n_size_cells().
>
> Signed-off-by: Sebastian Huber <sebastian.huber@xxxxxxxxxxxxxxxxxx>
Nack. I can see why you want this, but the "default" implemented in
Linux is a horrible workaround for old broken firmwares, and trying to
replicate it in libfdt just makes things worse.
I do wonder if I should just remove the default entirely to further
encourage these to be always specified in modern trees.
> ---
> libfdt/fdt_addresses.c | 60 ++++++++++++++++++++++++-------------------
> libfdt/libfdt.h | 11 +++++---
> tests/.gitignore | 1 +
> tests/Makefile.tests | 1 +
> tests/addr_size_cells.c | 5 ++++
> tests/addr_size_cells2.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++
> tests/addresses.dts | 23 +++++++++++++++++
> tests/run_tests.sh | 2 ++
> 8 files changed, 139 insertions(+), 31 deletions(-)
> create mode 100644 tests/addr_size_cells2.c
>
> diff --git a/libfdt/fdt_addresses.c b/libfdt/fdt_addresses.c
> index eff4dbc..d3a5305 100644
> --- a/libfdt/fdt_addresses.c
> +++ b/libfdt/fdt_addresses.c
> @@ -1,6 +1,7 @@
> /*
> * libfdt - Flat Device Tree manipulation
> * Copyright (C) 2014 David Gibson <david@xxxxxxxxxxxxxxxxxxxxx>
> + * Copyright (C) 2018 embedded brains GmbH
> *
> * libfdt is dual licensed: you can use it either under the terms of
> * the GPL, or the BSD license, at your option.
> @@ -55,42 +56,47 @@
>
> #include "libfdt_internal.h"
>
> -int fdt_address_cells(const void *fdt, int nodeoffset)
> +static int fdt_cells(const void *fdt, int nodeoffset, const char *name,
> + int default_cells)
> {
> - const fdt32_t *ac;
> + const fdt32_t *c;
> int val;
> int len;
>
> - ac = fdt_getprop(fdt, nodeoffset, "#address-cells", &len);
> - if (!ac)
> - return 2;
> -
> - if (len != sizeof(*ac))
> - return -FDT_ERR_BADNCELLS;
> + do {
> + c = fdt_getprop(fdt, nodeoffset, name, &len);
>
> - val = fdt32_to_cpu(*ac);
> - if ((val <= 0) || (val > FDT_MAX_NCELLS))
> - return -FDT_ERR_BADNCELLS;
> + if (c) {
> + if (len != sizeof(*c))
> + return -FDT_ERR_BADNCELLS;
>
> - return val;
> -}
> + val = fdt32_to_cpu(*c);
> + if ((val <= 0) || (val > FDT_MAX_NCELLS))
> + return -FDT_ERR_BADNCELLS;
>
> -int fdt_size_cells(const void *fdt, int nodeoffset)
> -{
> - const fdt32_t *sc;
> - int val;
> - int len;
> + return val;
> + }
>
> - sc = fdt_getprop(fdt, nodeoffset, "#size-cells", &len);
> - if (!sc)
> - return 2;
> + nodeoffset = fdt_parent_offset(fdt, nodeoffset);
Using parent_offset here makes this roughly O(N^2) in the size of the
tree, which isn't nice. I guess it's only for the broken case where
these aren't specified locally.
But these properties were never supposed to be inherited, and while
Linux does that to handle those old broken trees, there are other
trees out that use the actual IEEE1275 specified defaults (what libfdt
implements now) and will be broken if you try to inherit.
> + } while (nodeoffset >= 0);
>
> - if (len != sizeof(*sc))
> - return -FDT_ERR_BADNCELLS;
> + return default_cells;
> +}
>
> - val = fdt32_to_cpu(*sc);
> - if ((val < 0) || (val > FDT_MAX_NCELLS))
> - return -FDT_ERR_BADNCELLS;
> +int fdt_address_cells(const void *fdt, int nodeoffset)
> +{
> + /* Use default value of Linux kernel (OF_ROOT_NODE_ADDR_CELLS_DEFAULT) */
> + int default_cells;
> +#ifdef __sparc__
Also, this is right out. Unlike the kernel libfdt doesn't know that
the tree it's operating on is intended for the same platform as it is
running on right now.
> + default_cells = 2;
> +#else
> + default_cells = 1;
> +#endif
> + return fdt_cells(fdt, nodeoffset, "#address-cells", default_cells);
> +}
>
> - return val;
> +int fdt_size_cells(const void *fdt, int nodeoffset)
> +{
> + /* Use default value of Linux kernel (OF_ROOT_NODE_SIZE_CELLS_DEFAULT) */
> + return fdt_cells(fdt, nodeoffset, "#size-cells", 1);
> }
> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> index 38ec313..cf80cb5 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -1120,11 +1120,13 @@ const char *fdt_stringlist_get(const void *fdt, int nodeoffset,
> * @fdt: pointer to the device tree blob
> * @nodeoffset: offset of the node to find the address size for
> *
> - * When the node has a valid #address-cells property, returns its value.
> + * When the node has a valid #address-cells property, returns its value. In
> + * case the node has no #address-cells property, then ask its parent and so on.
> *
> * returns:
> * 0 <= n < FDT_MAX_NCELLS, on success
> - * 2, if the node has no #address-cells property
> + * 1, if the node and all its parents have no #address-cells property (2
> + * on the SPARC architecture)
> * -FDT_ERR_BADNCELLS, if the node has a badly formatted or invalid
> * #address-cells property
> * -FDT_ERR_BADMAGIC,
> @@ -1141,11 +1143,12 @@ int fdt_address_cells(const void *fdt, int nodeoffset);
> * @fdt: pointer to the device tree blob
> * @nodeoffset: offset of the node to find the address range size for
> *
> - * When the node has a valid #size-cells property, returns its value.
> + * When the node has a valid #size-cells property, returns its value. In case
> + * the node has no #size-cells property, then ask its parent and so on.
> *
> * returns:
> * 0 <= n < FDT_MAX_NCELLS, on success
> - * 2, if the node has no #address-cells property
> + * 1, if the node and all its parents have no #size-cells property
> * -FDT_ERR_BADNCELLS, if the node has a badly formatted or invalid
> * #size-cells property
> * -FDT_ERR_BADMAGIC,
> diff --git a/tests/.gitignore b/tests/.gitignore
> index d423570..7a99f5a 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -4,6 +4,7 @@
> tmp.*
> /add_subnode_with_nops
> /addr_size_cells
> +/addr_size_cells2
> /appendprop[12]
> /asm_tree_dump
> /boot-cpuid
> diff --git a/tests/Makefile.tests b/tests/Makefile.tests
> index 2c2c4fd..e7cf6bc 100644
> --- a/tests/Makefile.tests
> +++ b/tests/Makefile.tests
> @@ -9,6 +9,7 @@ LIB_TESTS_L = get_mem_rsv \
> sized_cells \
> notfound \
> addr_size_cells \
> + addr_size_cells2 \
> stringlist \
> setprop_inplace nop_property nop_node \
> sw_tree1 sw_states \
> diff --git a/tests/addr_size_cells.c b/tests/addr_size_cells.c
> index 6090d93..2a487e6 100644
> --- a/tests/addr_size_cells.c
> +++ b/tests/addr_size_cells.c
> @@ -2,6 +2,7 @@
> * libfdt - Flat Device Tree manipulation
> * Testcase for #address-cells and #size-cells handling
> * Copyright (C) 2014 David Gibson, <david@xxxxxxxxxxxxxxxxxxxxx>
> + * Copyright (C) 2018 embedded brains GmbH
> *
> * This library is free software; you can redistribute it and/or
> * modify it under the terms of the GNU Lesser General Public License
> @@ -60,5 +61,9 @@ int main(int argc, char *argv[])
> check_node(fdt, "/", 2, 2);
> check_node(fdt, "/identity-bus@0", 2, 2);
> check_node(fdt, "/simple-bus@1000000", 2, 1);
> + check_node(fdt, "/c0", -FDT_ERR_BADNCELLS, -FDT_ERR_BADNCELLS);
> + check_node(fdt, "/c1", -FDT_ERR_BADNCELLS, -FDT_ERR_BADNCELLS);
> + check_node(fdt, "/c2", -FDT_ERR_BADNCELLS, -FDT_ERR_BADNCELLS);
> + check_node(fdt, "/c3/c4", 3, 4);
> PASS();
> }
> diff --git a/tests/addr_size_cells2.c b/tests/addr_size_cells2.c
> new file mode 100644
> index 0000000..c250866
> --- /dev/null
> +++ b/tests/addr_size_cells2.c
> @@ -0,0 +1,67 @@
> +/*
> + * libfdt - Flat Device Tree manipulation
> + * Testcase for #address-cells and #size-cells handling
> + * Copyright (C) 2014 David Gibson, <david@xxxxxxxxxxxxxxxxxxxxx>
> + * Copyright (C) 2018 embedded brains GmbH
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public License
> + * as published by the Free Software Foundation; either version 2.1 of
> + * the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <stdint.h>
> +
> +#include <libfdt.h>
> +
> +#include "tests.h"
> +#include "testdata.h"
> +
> +static void check_node(const void *fdt, const char *path, int ac, int sc)
> +{
> + int offset;
> + int xac, xsc;
> +
> + offset = fdt_path_offset(fdt, path);
> + if (offset < 0)
> + FAIL("Couldn't find path %s", path);
> +
> + xac = fdt_address_cells(fdt, offset);
> + xsc = fdt_size_cells(fdt, offset);
> +
> + if (xac != ac)
> + FAIL("Address cells for %s is %d instead of %d\n",
> + path, xac, ac);
> + if (xsc != sc)
> + FAIL("Size cells for %s is %d instead of %d\n",
> + path, xsc, sc);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> + void *fdt;
> +
> + if (argc != 2)
> + CONFIG("Usage: %s <dtb file>\n", argv[0]);
> +
> + test_init(argc, argv);
> + fdt = load_blob(argv[1]);
> +
> +#ifdef __sparc__
> + check_node(fdt, "/", 2, 1);
> +#else
> + check_node(fdt, "/", 1, 1);
> +#endif
> + PASS();
> +}
> diff --git a/tests/addresses.dts b/tests/addresses.dts
> index a2faaf5..fb6d389 100644
> --- a/tests/addresses.dts
> +++ b/tests/addresses.dts
> @@ -12,4 +12,27 @@
> #address-cells = <2>;
> #size-cells = <1>;
> };
> +
> + c0 {
> + #address-cells = <1 1>;
> + #size-cells = <2 2>;
> + };
> +
> + c1 {
> + #address-cells = <0x80000000>;
> + #size-cells = <0x80000001>;
> + };
> +
> + c2 {
> + #address-cells = <5>;
> + #size-cells = <6>;
> + };
> +
> + c3 {
> + #address-cells = <3>;
> + #size-cells = <4>;
> +
> + c4 {
> + };
> + };
> };
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index cf87066..7348c9c 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -336,6 +336,8 @@ libfdt_tests () {
>
> run_dtc_test -I dts -O dtb -o addresses.test.dtb addresses.dts
> run_test addr_size_cells addresses.test.dtb
> + run_dtc_test -I dts -O dtb -o addresses2.test.dtb empty.dts
> + run_test addr_size_cells2 addresses2.test.dtb
>
> run_dtc_test -I dts -O dtb -o stringlist.test.dtb stringlist.dts
> run_test stringlist stringlist.test.dtb
--
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
Attachment:
signature.asc
Description: PGP signature
