[PATCH v6 17/28] fdt: translate address if #size-cells = <0>

[Heinrich Schuchardt]

On 12/1/20 9:48 PM, dariobin at libero.it wrote:
> Dear Heinrich
>
>>
>> On 11/22/20 5:11 PM, Dario Binacchi 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
>> > #size-cells = <0> is to be considered an error not by specification but
>> > 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>.
>>
>> Dear Dario,
>>
>> Thanks for addressing the incorrect assumption in function
>> __of_translate_address().
>>
>> The chapter "2.3.6 reg" of "Devicetree Specification, Release v0.3"
>> has this sentence:
>>
>> "If the parent node specifies a value of 0 for #size-cells, the length
>> field in the value of reg shall be omitted."
>>
>> What I do not understand yet is why we should have a configuration
>> option for something that is explicitely required by the device-tree
>> specification.
>
> Because even though this is required by the device tree
> specification, the u-boot address translation code considered
> it an error.
> The patch behaves conservatively trying to fix the address
> translation for the beaglebone device tree, so the Kconfig
> symbol must be activated without, however, generating address
> translation errors for device trees that until now have never
> highlighted this problem and for which the crossing of any level
> with #size-cells = <0> was to be considered an error.

Is there any device tree in our repository that is throwing such an error?

Why should we in future create bogus errors?

I see no worth in the configuration switch. Please, remove it.

Best regards

Heinrich

>
> Best regards
>
> Dario
>
>> Best regards
>>
>> Heinrich
>>
>> >
>> > 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>
>> > Reviewed-by: Simon Glass <sjg at chromium.org>
>> >
>> > ---
>> >
>> > (no changes since v4)
>> >
>> > Changes in v4:
>> > - Add Sphinx documentation for dm_flags.
>> > - Convert GD_DM_FLG_* to enum.
>> > - Include device_compat.h header in test/dm/test-fdt.c for dev_xxx
>> macros.
>> >
>> > Changes in v3:
>> > - Comment dm_flags field in the global_data structure.
>> >
>> > Changes in v2:
>> > - Fix a missing line in the commit message.
>> > - Add dm_flags to global_data structure and GD_DM_FLG_SIZE_CELLS_0
>> macro
>> > to test without recompiling.
>> > - Update the OF_CHECK_COUNTS macro in order to have just one
>> > #define by bringing the GD_DM_FLG_SIZE_CELLS_0 into the expression.
>> > - Lower-case the 0xC019 hex number.
>> >
>> > arch/sandbox/dts/test.dts | 21 ++++++++++
>> > common/fdt_support.c | 6 ++-
>> > drivers/core/Kconfig | 12 ++++++
>> > drivers/core/fdtaddr.c | 2 +-
>> > drivers/core/of_addr.c | 14 ++-----
>> > drivers/core/ofnode.c | 7 +++-
>> > drivers/core/root.c | 3 ++
>> > include/asm-generic/global_data.h | 18 ++++++++
>> > test/dm/test-fdt.c | 69 ++++++++++++++++++++++++++++++-
>> > 9 files changed, 136 insertions(+), 16 deletions(-)
>> >
>> > diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
>> > index f3b766271d..637d79caf7 100644
>> > --- a/arch/sandbox/dts/test.dts
>> > +++ b/arch/sandbox/dts/test.dts
>> > @@ -41,6 +41,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;
>> > @@ -1061,6 +1062,7 @@
>> > 1 0x100 0x9000 0x1000
>> > 2 0x200 0xA000 0x1000
>> > 3 0x300 0xB000 0x1000
>> > + 4 0x400 0xC000 0x1000
>> > >;
>> >
>> > dma-ranges = <0 0x000 0x10000000 0x1000
>> > @@ -1097,6 +1099,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 5ae75df3c6..c817f994ed 100644
>> > --- a/common/fdt_support.c
>> > +++ b/common/fdt_support.c
>> > @@ -20,6 +20,8 @@
>> > #include <exports.h>
>> > #include <fdtdec.h>
>> >
>> > +DECLARE_GLOBAL_DATA_PTR;
>> > +
>> > /**
>> > * fdt_getprop_u32_default_node - Return a node's property or a default
>> > *
>> > @@ -996,8 +998,8 @@ 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_COUNTS(na, ns) (((na) > 0 && (na) <=
>> OF_MAX_ADDR_CELLS) && \
>> > + ((ns) > 0 || (gd->dm_flags & GD_DM_FLG_SIZE_CELLS_0)))
>> >
>> > /* Debug utility */
>> > #ifdef DEBUG
>> > diff --git a/drivers/core/Kconfig b/drivers/core/Kconfig
>> > index ffae6f9795..c85a685823 100644
>> > --- a/drivers/core/Kconfig
>> > +++ b/drivers/core/Kconfig
>> > @@ -231,6 +231,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..51a0093d65 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 (ns || (gd->dm_flags & GD_DM_FLG_SIZE_CELLS_0)) {
>> > /*
>> > * 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..a590533c8a 100644
>> > --- a/drivers/core/of_addr.c
>> > +++ b/drivers/core/of_addr.c
>> > @@ -18,7 +18,9 @@
>> > /* 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)
>> > -#define OF_CHECK_COUNTS(na, ns) (OF_CHECK_ADDR_COUNT(na) && (ns) > 0)
>> > +#define OF_CHECK_COUNTS(na, ns) (OF_CHECK_ADDR_COUNT(na) && \
>> > + ((ns) > 0 || \
>> > + (gd->dm_flags & GD_DM_FLG_SIZE_CELLS_0)))
>> >
>> > static struct of_bus *of_match_bus(struct device_node *np);
>> >
>> > @@ -162,11 +164,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 +190,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 a68076bf35..2d52eaccb0 100644
>> > --- a/drivers/core/ofnode.c
>> > +++ b/drivers/core/ofnode.c
>> > @@ -304,7 +304,8 @@ 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 || (gd->dm_flags & GD_DM_FLG_SIZE_CELLS_0))) {
>> > return of_translate_address(ofnode_to_np(node), prop_val);
>> > } else {
>> > na = of_n_addr_cells(ofnode_to_np(node));
>> > @@ -678,8 +679,10 @@ 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 || (gd->dm_flags & GD_DM_FLG_SIZE_CELLS_0))) {
>> > return of_translate_address(np, prop);
>> > + }
>> > else
>> > return of_read_number(prop, na);
>> > } else {
>> > diff --git a/drivers/core/root.c b/drivers/core/root.c
>> > index 5f10d7a39c..aac1911ecb 100644
>> > --- a/drivers/core/root.c
>> > +++ b/drivers/core/root.c
>> > @@ -132,6 +132,9 @@ int dm_init(bool of_live)
>> > {
>> > int ret;
>> >
>> > + if (IS_ENABLED(CONFIG_OF_TRANSLATE_ZERO_SIZE_CELLS))
>> > + gd->dm_flags |= GD_DM_FLG_SIZE_CELLS_0;
>> > +
>> > if (gd->dm_root) {
>> > dm_warn("Virtual root driver already exists!\n");
>> > return -EINVAL;
>> > diff --git a/include/asm-generic/global_data.h
>> b/include/asm-generic/global_data.h
>> > index 87d827d0f4..9beb8be0e8 100644
>> > --- a/include/asm-generic/global_data.h
>> > +++ b/include/asm-generic/global_data.h
>> > @@ -183,6 +183,12 @@ struct global_data {
>> > struct global_data *new_gd;
>> >
>> > #ifdef CONFIG_DM
>> > + /**
>> > + * @dm_flags: additional flags for Driver Model
>> > + *
>> > + * See &enum gd_dm_flags
>> > + */
>> > + unsigned long dm_flags;
>> > /**
>> > * @dm_root: root instance for Driver Model
>> > */
>> > @@ -549,6 +555,18 @@ enum gd_flags {
>> > GD_FLG_SMP_READY = 0x40000,
>> > };
>> >
>> > +/**
>> > + * enum gd_dm_flags - global data flags for Driver Model
>> > + *
>> > + * See field dm_flags of &struct global_data.
>> > + */
>> > +enum gd_dm_flags {
>> > + /**
>> > + * @GD_DM_FLG_SIZE_CELLS_0: Enable #size-cells=<0> translation
>> > + */
>> > + GD_DM_FLG_SIZE_CELLS_0 = 0x00001,
>> > +};
>> > +
>> > #endif /* __ASSEMBLY__ */
>> >
>> > #endif /* __ASM_GENERIC_GBL_DATA_H */
>> > diff --git a/test/dm/test-fdt.c b/test/dm/test-fdt.c
>> > index cc12419ea0..dd18160cbe 100644
>> > --- a/test/dm/test-fdt.c
>> > +++ b/test/dm/test-fdt.c
>> > @@ -5,6 +5,7 @@
>> >
>> > #include <common.h>
>> > #include <dm.h>
>> > +#include <dm/device_compat.h>
>> > #include <errno.h>
>> > #include <fdtdec.h>
>> > #include <log.h>
>> > @@ -581,6 +582,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,11 +658,19 @@ 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);
>> > + /* No translation for busses with #size-cells == 0 */
>> > ut_asserteq(0x42, dev_read_addr(dev));
>> >
>> > + /* Translation for busses with #size-cells == 0 */
>> > + gd->dm_flags |= GD_DM_FLG_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));
>> > + gd->dm_flags &= ~GD_DM_FLG_SIZE_CELLS_0;
>> > +
>> > /* dma address translation */
>> > ut_assertok(uclass_find_device_by_seq(UCLASS_TEST_DUMMY, 0, true,
>> &dev));
>> > dma_addr[0] = cpu_to_be32(0);
>> >