Message ID | 20200825092124.4284-17-dariobin@libero.it |
---|---|
State | Changes Requested |
Delegated to: | Lokesh Vutla |
Headers | show |
Series | Add DM support for omap PWM backlight | expand |
Hi Dario, On Tue, 25 Aug 2020 at 03:25, Dario Binacchi <dariobin@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@libero.it> > Tested-by: Dario Binacchi <dariobin@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@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; > @@ -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@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 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@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); > - 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@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
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@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; @@ -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@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 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) { /* * 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) +#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@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); - ut_asserteq(0x42, dev_read_addr(dev)); + + if (!IS_ENABLED(CONFIG_OF_TRANSLATE_ZERO_SIZE_CELLS)) { + /* 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@19", dev->name); + ut_asserteq(0xC019, dev_read_addr(dev)); + } /* dma address translation */ ut_assertok(uclass_find_device_by_seq(UCLASS_TEST_DUMMY, 0, true, &dev));