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.

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@libero.it>
> Tested-by: Dario Binacchi <dariobin@libero.it>
> Reviewed-by: Simon Glass <sjg@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@8000/dev@1,100";
> fdt-dummy2 = "/translation-test@8000/dev@2,200";
> fdt-dummy3 = "/translation-test@8000/noxlatebus@3,300/dev@42";
> + fdt-dummy4 = "/translation-test@8000/xlatebus@4,400/devs/dev@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@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@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@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@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@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);
>