[PATCH 16/31] fdt: translate address if #size-cells = <0>
Simon Glass
sjg at chromium.org
Sat Aug 29 23:20:36 CEST 2020
Hi Dario,
On Tue, 25 Aug 2020 at 03:25, Dario Binacchi <dariobin at libero.it> wrote:
>
> The __of_translate_address routine translates an address from the
> device tree into a CPU physical address. A note in the description of
> the routine explains that the crossing of any level with
there is something missing here. Do you have a # at the start of the line?
> since inherited from IBM. This does not happen for Texas Instruments, or
> at least for the beaglebone device tree. Without this patch, in fact,
> the translation into physical addresses of the registers contained in the
> am33xx-clocks.dtsi nodes would not be possible. They all have a parent
> with #size-cells = <0>.
>
> The CONFIG_OF_TRANSLATE_ZERO_SIZE_CELLS symbol makes translation
> possible even in the case of crossing levels with #size-cells = <0>.
>
> The patch acts conservatively on address translation, except for
> removing a check within the of_translate_one function in the
> drivers/core/of_addr.c file:
>
> +
> ranges = of_get_property(parent, rprop, &rlen);
> - if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
> - debug("no ranges; cannot translate\n");
> - return 1;
> - }
> if (ranges == NULL || rlen == 0) {
> offset = of_read_number(addr, na);
> memset(addr, 0, pna * 4);
> debug("empty ranges; 1:1 translation\n");
>
> There are two reasons:
> 1 The function of_empty_ranges_quirk always returns false, invalidating
> the following if statement in case of null ranges. Therefore one of
> the two checks is useless.
>
> 2 The implementation of the of_translate_one function found in the
> common/fdt_support.c file has removed this check while keeping the one
> about the 1:1 translation.
>
> The patch adds a test and modifies a check for the correctness of an
> address in the case of enabling translation also for zero size cells.
> The added test checks translations of addresses generated by nodes of
> a device tree similar to those you can find in the files am33xx.dtsi
> and am33xx-clocks.dtsi for which the patch was created.
>
> The patch was also tested on a beaglebone black board. The addresses
> generated for the registers of the loaded drivers are those specified
> by the AM335x reference manual.
>
> Signed-off-by: Dario Binacchi <dariobin at libero.it>
> Tested-by: Dario Binacchi <dariobin at libero.it>
> ---
>
> arch/sandbox/dts/test.dts | 21 +++++++++++
> common/fdt_support.c | 10 ++++--
> drivers/core/Kconfig | 12 +++++++
> drivers/core/fdtaddr.c | 2 +-
> drivers/core/of_addr.c | 14 +++-----
> drivers/core/ofnode.c | 11 ++++--
> test/dm/test-fdt.c | 73 +++++++++++++++++++++++++++++++++++++--
> 7 files changed, 127 insertions(+), 16 deletions(-)
>
> diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
> index 1d8956abbe..7f5d8f3aeb 100644
> --- a/arch/sandbox/dts/test.dts
> +++ b/arch/sandbox/dts/test.dts
> @@ -39,6 +39,7 @@
> fdt-dummy1 = "/translation-test at 8000/dev at 1,100";
> fdt-dummy2 = "/translation-test at 8000/dev at 2,200";
> fdt-dummy3 = "/translation-test at 8000/noxlatebus at 3,300/dev at 42";
> + fdt-dummy4 = "/translation-test at 8000/xlatebus at 4,400/devs/dev at 19";
> usb0 = &usb_0;
> usb1 = &usb_1;
> usb2 = &usb_2;
> @@ -977,6 +978,7 @@
> 1 0x100 0x9000 0x1000
> 2 0x200 0xA000 0x1000
> 3 0x300 0xB000 0x1000
> + 4 0x400 0xC000 0x1000
> >;
>
> dma-ranges = <0 0x000 0x10000000 0x1000
> @@ -1013,6 +1015,25 @@
> reg = <0x42>;
> };
> };
> +
> + xlatebus at 4,400 {
> + compatible = "sandbox,zero-size-cells-bus";
> + reg = <4 0x400 0x1000>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges = <0 4 0x400 0x1000>;
> +
> + devs {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + dev at 19 {
> + compatible = "denx,u-boot-fdt-dummy";
> + reg = <0x19>;
> + };
> + };
> + };
> +
> };
>
> osd {
> diff --git a/common/fdt_support.c b/common/fdt_support.c
> index a565b470f8..402401e073 100644
> --- a/common/fdt_support.c
> +++ b/common/fdt_support.c
> @@ -1001,8 +1001,14 @@ void fdt_del_node_and_alias(void *blob, const char *alias)
> /* Max address size we deal with */
> #define OF_MAX_ADDR_CELLS 4
> #define OF_BAD_ADDR FDT_ADDR_T_NONE
> -#define OF_CHECK_COUNTS(na, ns) ((na) > 0 && (na) <= OF_MAX_ADDR_CELLS && \
> - (ns) > 0)
> +#define OF_CHECK_ADDR_COUNT(na) ((na) > 0 && (na) <= OF_MAX_ADDR_CELLS)
> +#if defined(CONFIG_OF_TRANSLATE_ZERO_SIZE_CELLS)
> +#define OF_CHECK_SIZE_COUNT(ns) ((ns) >= 0)
> +#else
> +#define OF_CHECK_SIZE_COUNT(ns) ((ns) > 0)
> +#endif
> +#define OF_CHECK_COUNTS(na, ns) (OF_CHECK_ADDR_COUNT(na) && \
> + OF_CHECK_SIZE_COUNT(ns))
>
> /* Debug utility */
> #ifdef DEBUG
> diff --git a/drivers/core/Kconfig b/drivers/core/Kconfig
> index 00d1d80dc3..2a683ae404 100644
> --- a/drivers/core/Kconfig
> +++ b/drivers/core/Kconfig
> @@ -217,6 +217,18 @@ config OF_TRANSLATE
> used for the address translation. This function is faster and
> smaller in size than fdt_translate_address().
>
> +config OF_TRANSLATE_ZERO_SIZE_CELLS
> + bool "Enable translation for zero size cells"
> + depends on OF_TRANSLATE
> + default n
> + help
> + The routine used to translate an FDT address into a physical CPU
> + address was developed by IBM. It considers that crossing any level
> + with #size-cells = <0> makes translation impossible, even if it is
> + not the way it was specified.
> + Enabling this option makes translation possible even in the case
> + of crossing levels with #size-cells = <0>.
> +
> config SPL_OF_TRANSLATE
> bool "Translate addresses using fdt_translate_address in SPL"
> depends on SPL_DM && SPL_OF_CONTROL
> diff --git a/drivers/core/fdtaddr.c b/drivers/core/fdtaddr.c
> index 8b48aa5bc5..5a9f08aa44 100644
> --- a/drivers/core/fdtaddr.c
> +++ b/drivers/core/fdtaddr.c
> @@ -49,7 +49,7 @@ fdt_addr_t devfdt_get_addr_index(const struct udevice *dev, int index)
>
> reg += index * (na + ns);
>
> - if (ns) {
> + if (IS_ENABLED(CONFIG_OF_TRANSLATE_ZERO_SIZE_CELLS) || ns) {
Here we should check a flag, perhaps in a new gd->dm_flags value, to
determine the behaviour. The problem with using a CONFIG here is that
you cannot test it without recompiling. Also the code-size difference
is negligible.
So you can use the CONFIG to set the flag, perhaps in dm_init(), and
add a helper to change the setting, which can be used in tests.
> /*
> * Use the full-fledged translate function for complex
> * bus setups.
> diff --git a/drivers/core/of_addr.c b/drivers/core/of_addr.c
> index ca34d84922..ddac5f9a2b 100644
> --- a/drivers/core/of_addr.c
> +++ b/drivers/core/of_addr.c
> @@ -18,7 +18,11 @@
> /* Max address size we deal with */
> #define OF_MAX_ADDR_CELLS 4
> #define OF_CHECK_ADDR_COUNT(na) ((na) > 0 && (na) <= OF_MAX_ADDR_CELLS)
> +#if defined(CONFIG_OF_TRANSLATE_ZERO_SIZE_CELLS)
> +#define OF_CHECK_COUNTS(na, ns) (OF_CHECK_ADDR_COUNT(na) && (ns) >= 0)
> +#else
> #define OF_CHECK_COUNTS(na, ns) (OF_CHECK_ADDR_COUNT(na) && (ns) > 0)
This could be changed to have just one #define by bringing the CONFIG
into the expression.
> +#endif
>
> static struct of_bus *of_match_bus(struct device_node *np);
>
> @@ -162,11 +166,6 @@ const __be32 *of_get_address(const struct device_node *dev, int index,
> }
> EXPORT_SYMBOL(of_get_address);
>
> -static int of_empty_ranges_quirk(const struct device_node *np)
> -{
> - return false;
> -}
> -
> static int of_translate_one(const struct device_node *parent,
> struct of_bus *bus, struct of_bus *pbus,
> __be32 *addr, int na, int ns, int pna,
> @@ -193,11 +192,8 @@ static int of_translate_one(const struct device_node *parent,
> * As far as we know, this damage only exists on Apple machines, so
> * This code is only enabled on powerpc. --gcl
> */
> +
> ranges = of_get_property(parent, rprop, &rlen);
> - if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
> - debug("no ranges; cannot translate\n");
> - return 1;
> - }
> if (ranges == NULL || rlen == 0) {
> offset = of_read_number(addr, na);
> memset(addr, 0, pna * 4);
> diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c
> index d02d8d33fe..b5744e86c3 100644
> --- a/drivers/core/ofnode.c
> +++ b/drivers/core/ofnode.c
> @@ -304,7 +304,10 @@ fdt_addr_t ofnode_get_addr_size_index(ofnode node, int index, fdt_size_t *size)
>
> ns = of_n_size_cells(ofnode_to_np(node));
>
> - if (IS_ENABLED(CONFIG_OF_TRANSLATE) && ns > 0) {
> + if (IS_ENABLED(CONFIG_OF_TRANSLATE) &&
> + (ns > 0 ||
> + (IS_ENABLED(CONFIG_OF_TRANSLATE_ZERO_SIZE_CELLS) &&
> + ns >= 0))) {
> return of_translate_address(ofnode_to_np(node), prop_val);
> } else {
> na = of_n_addr_cells(ofnode_to_np(node));
> @@ -655,8 +658,12 @@ fdt_addr_t ofnode_get_addr_size(ofnode node, const char *property,
> ns = of_n_size_cells(np);
> *sizep = of_read_number(prop + na, ns);
>
> - if (CONFIG_IS_ENABLED(OF_TRANSLATE) && ns > 0)
> + if (CONFIG_IS_ENABLED(OF_TRANSLATE) &&
> + (ns > 0 ||
> + (IS_ENABLED(CONFIG_OF_TRANSLATE_ZERO_SIZE_CELLS) &&
> + ns >= 0))) {
> return of_translate_address(np, prop);
> + }
> else
> return of_read_number(prop, na);
> } else {
> diff --git a/test/dm/test-fdt.c b/test/dm/test-fdt.c
> index 04802deb7f..30a7a933dd 100644
> --- a/test/dm/test-fdt.c
> +++ b/test/dm/test-fdt.c
> @@ -581,6 +581,64 @@ U_BOOT_DRIVER(fdt_dummy_drv) = {
> .id = UCLASS_TEST_DUMMY,
> };
>
> +static int zero_size_cells_bus_bind(struct udevice *dev)
> +{
> + ofnode child;
> + int err;
> +
> + ofnode_for_each_subnode(child, dev_ofnode(dev)) {
> + if (ofnode_get_property(child, "compatible", NULL))
> + continue;
> +
> + err = device_bind_driver_to_node(dev,
> + "zero_size_cells_bus_child_drv",
> + "zero_size_cells_bus_child",
> + child, NULL);
> + if (err) {
> + dev_err(dev, "%s: failed to bind %s\n", __func__,
> + ofnode_get_name(child));
> + return err;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static const struct udevice_id zero_size_cells_bus_ids[] = {
> + { .compatible = "sandbox,zero-size-cells-bus" },
> + { }
> +};
> +
> +U_BOOT_DRIVER(zero_size_cells_bus) = {
> + .name = "zero_size_cells_bus_drv",
> + .id = UCLASS_TEST_DUMMY,
> + .of_match = zero_size_cells_bus_ids,
> + .bind = zero_size_cells_bus_bind,
> +};
> +
> +static int zero_size_cells_bus_child_bind(struct udevice *dev)
> +{
> + ofnode child;
> + int err;
> +
> + ofnode_for_each_subnode(child, dev_ofnode(dev)) {
> + err = lists_bind_fdt(dev, child, NULL, false);
> + if (err) {
> + dev_err(dev, "%s: lists_bind_fdt, err=%d\n",
> + __func__, err);
> + return err;
> + }
> + }
> +
> + return 0;
> +}
> +
> +U_BOOT_DRIVER(zero_size_cells_bus_child_drv) = {
> + .name = "zero_size_cells_bus_child_drv",
> + .id = UCLASS_TEST_DUMMY,
> + .bind = zero_size_cells_bus_child_bind,
> +};
> +
> static int dm_test_fdt_translation(struct unit_test_state *uts)
> {
> struct udevice *dev;
> @@ -599,10 +657,21 @@ static int dm_test_fdt_translation(struct unit_test_state *uts)
> ut_asserteq_str("dev at 2,200", dev->name);
> ut_asserteq(0xA000, dev_read_addr(dev));
>
> - /* No translation for busses with #size-cells == 0 */
> ut_assertok(uclass_find_device_by_seq(UCLASS_TEST_DUMMY, 3, true, &dev));
> ut_asserteq_str("dev at 42", dev->name);
> - ut_asserteq(0x42, dev_read_addr(dev));
> +
> + if (!IS_ENABLED(CONFIG_OF_TRANSLATE_ZERO_SIZE_CELLS)) {
Here, adjust the flag so you can run both tests one after the other.
> + /* No translation for busses with #size-cells == 0 */
> + ut_asserteq(0x42, dev_read_addr(dev));
> + } else {
> + /* Translation for busses with #size-cells == 0 */
> + ut_asserteq(0x8042, dev_read_addr(dev));
> +
> + ut_assertok(uclass_find_device_by_seq(UCLASS_TEST_DUMMY, 4, true,
> + &dev));
> + ut_asserteq_str("dev at 19", dev->name);
> + ut_asserteq(0xC019, dev_read_addr(dev));
lower-case hex please
> + }
>
> /* dma address translation */
> ut_assertok(uclass_find_device_by_seq(UCLASS_TEST_DUMMY, 0, true, &dev));
> --
> 2.17.1
>
Regards,
Simon
More information about the U-Boot
mailing list