Re: [PATCH] cfi_flash: Fix devicetree address determination
Hi André, On Wed, 23 Sep 2020 at 09:39, André Przywara wrote: > > On 23/09/2020 06:26, Stefan Roese wrote: > > Hi Simon, > > > > On 22.09.20 15:51, Simon Glass wrote: > >> Hi Stefan, > >> > >> On Mon, 21 Sep 2020 at 07:28, Stefan Roese wrote: > >>> > >>> Hi Andre, > >>> > >>> (added Simon) > >>> > >>> On 18.09.20 19:45, Andre Przywara wrote: > The cfi-flash driver uses an open-coded version of the generic > algorithm to decode and translate multiple frames of a "reg" property. > > This starts off the wrong foot by using the address-cells and > size-cells > properties of *this* very node, and not of the parent. This somewhat > happened to work back when we were using a wrong default size of 2, > but broke about a year ago with commit 0ba41ce1b781 ("libfdt: return > correct value if #size-cells property is not present"). > > Instead of fixing the reinvented wheel, just use the generic function > that does all of this properly. > > This fixes U-Boot on QEMU (outside of EL1), which was crashing due to > decoding a wrong start address: > DRAM: 1 GiB > Flash: "Synchronous Abort" handler, esr 0x9644 > elr: 000211dc lr : 000211b0 (reloc) > elr: 7ff5e1dc lr : 7ff5e1b0 > x0 : 00f0 x1 : 7ff5e1d8 > x2 : 7edfbc48 x3 : > x4 : x5 : 00f0 > x6 : 7edfbc2c x7 : > x8 : 7ffd8d70 x9 : 000c > x10: 0403 x11: 0055 > > > Signed-off-by: Andre Przywara > --- > drivers/mtd/cfi_flash.c | 25 +++-- > 1 file changed, 7 insertions(+), 18 deletions(-) > > diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c > index b7289ba5394..656ff326e17 100644 > --- a/drivers/mtd/cfi_flash.c > +++ b/drivers/mtd/cfi_flash.c > @@ -2468,29 +2468,18 @@ unsigned long flash_init(void) > #ifdef CONFIG_CFI_FLASH /* for driver model */ > static int cfi_flash_probe(struct udevice *dev) > { > - const fdt32_t *cell; > - int addrc, sizec; > - int len, idx; > + fdt_addr_t addr; > + fdt_size_t size; > + int idx; > > - addrc = dev_read_addr_cells(dev); > - sizec = dev_read_size_cells(dev); > - > - /* decode regs; there may be multiple reg tuples. */ > - cell = dev_read_prop(dev, "reg", &len); > - if (!cell) > - return -ENOENT; > - idx = 0; > - len /= sizeof(fdt32_t); > - while (idx < len) { > - phys_addr_t addr; > - > - addr = dev_translate_address(dev, cell + idx); > + for (idx = 0; idx < CFI_MAX_FLASH_BANKS; idx++) { > + addr = devfdt_get_addr_size_index(dev, idx, &size); > + if (addr == FDT_ADDR_T_NONE) > + break; > > flash_info[cfi_flash_num_flash_banks].dev = dev; > flash_info[cfi_flash_num_flash_banks].base = addr; > cfi_flash_num_flash_banks++; > - > - idx += addrc + sizec; > } > gd->bd->bi_flashstart = flash_info[0].base; > > > >>> > >>> This fails on my Octeon MIPS64 platform "octeon_ebb7304". I did some > >>> debugging and found that here "of_offset" is a 64 bit value (type long) > >>> which gets truncated in dev_of_offset() to 32 bit (type int). > >>> > >>> This problem only arises when of_live_active() is set. Here, "of_offset" > >>> holds a pointer AFACT and truncating it to 32 bits breaks things. > >>> > >>> I'm wondering why this did not hit me earlier on this 64bit platform. > >>> Simon, do you have a quick idea how to solve this? > >> > >> Well I don't think ofnode should use long for of_offset, since int > >> should be enough. > >> > >> ofnode_to_offset() converts an ofnode to a DT offset but only if it is > >> not using livetree. With livetree there are no offsets so this is not > >> going to work. If you define OF_CHECKS you will see that. > > > > This does not work right now. I'll send a patch fixing compiling with > > OF_CHECK enabled shortly. > > > >> Note that an ofnode can either hold a pointer or an offset. There are > >> detailed comments on ofnode_union to explain how it is supposed to > >> work. > > > > Right. Thanks for all the detailed infos in the header. The main issue > > seems to be, that this CFI patch uses a function from fdtaddr.c > > (devfdt_get_addr_size_index), which unconditionally uses dev_of_offset() > > without checking if livetree is enabled or not. This breaks on my > > 64 bit platform (see below). > > > >> This patch looks correct to me, but perhaps there is something else > >> going on? > > > > Making th
Re: [PATCH] cfi_flash: Fix devicetree address determination
On 23/09/2020 06:26, Stefan Roese wrote: > Hi Simon, > > On 22.09.20 15:51, Simon Glass wrote: >> Hi Stefan, >> >> On Mon, 21 Sep 2020 at 07:28, Stefan Roese wrote: >>> >>> Hi Andre, >>> >>> (added Simon) >>> >>> On 18.09.20 19:45, Andre Przywara wrote: The cfi-flash driver uses an open-coded version of the generic algorithm to decode and translate multiple frames of a "reg" property. This starts off the wrong foot by using the address-cells and size-cells properties of *this* very node, and not of the parent. This somewhat happened to work back when we were using a wrong default size of 2, but broke about a year ago with commit 0ba41ce1b781 ("libfdt: return correct value if #size-cells property is not present"). Instead of fixing the reinvented wheel, just use the generic function that does all of this properly. This fixes U-Boot on QEMU (outside of EL1), which was crashing due to decoding a wrong start address: DRAM: 1 GiB Flash: "Synchronous Abort" handler, esr 0x9644 elr: 000211dc lr : 000211b0 (reloc) elr: 7ff5e1dc lr : 7ff5e1b0 x0 : 00f0 x1 : 7ff5e1d8 x2 : 7edfbc48 x3 : x4 : x5 : 00f0 x6 : 7edfbc2c x7 : x8 : 7ffd8d70 x9 : 000c x10: 0403 x11: 0055 Signed-off-by: Andre Przywara --- drivers/mtd/cfi_flash.c | 25 +++-- 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c index b7289ba5394..656ff326e17 100644 --- a/drivers/mtd/cfi_flash.c +++ b/drivers/mtd/cfi_flash.c @@ -2468,29 +2468,18 @@ unsigned long flash_init(void) #ifdef CONFIG_CFI_FLASH /* for driver model */ static int cfi_flash_probe(struct udevice *dev) { - const fdt32_t *cell; - int addrc, sizec; - int len, idx; + fdt_addr_t addr; + fdt_size_t size; + int idx; - addrc = dev_read_addr_cells(dev); - sizec = dev_read_size_cells(dev); - - /* decode regs; there may be multiple reg tuples. */ - cell = dev_read_prop(dev, "reg", &len); - if (!cell) - return -ENOENT; - idx = 0; - len /= sizeof(fdt32_t); - while (idx < len) { - phys_addr_t addr; - - addr = dev_translate_address(dev, cell + idx); + for (idx = 0; idx < CFI_MAX_FLASH_BANKS; idx++) { + addr = devfdt_get_addr_size_index(dev, idx, &size); + if (addr == FDT_ADDR_T_NONE) + break; flash_info[cfi_flash_num_flash_banks].dev = dev; flash_info[cfi_flash_num_flash_banks].base = addr; cfi_flash_num_flash_banks++; - - idx += addrc + sizec; } gd->bd->bi_flashstart = flash_info[0].base; >>> >>> This fails on my Octeon MIPS64 platform "octeon_ebb7304". I did some >>> debugging and found that here "of_offset" is a 64 bit value (type long) >>> which gets truncated in dev_of_offset() to 32 bit (type int). >>> >>> This problem only arises when of_live_active() is set. Here, "of_offset" >>> holds a pointer AFACT and truncating it to 32 bits breaks things. >>> >>> I'm wondering why this did not hit me earlier on this 64bit platform. >>> Simon, do you have a quick idea how to solve this? >> >> Well I don't think ofnode should use long for of_offset, since int >> should be enough. >> >> ofnode_to_offset() converts an ofnode to a DT offset but only if it is >> not using livetree. With livetree there are no offsets so this is not >> going to work. If you define OF_CHECKS you will see that. > > This does not work right now. I'll send a patch fixing compiling with > OF_CHECK enabled shortly. > >> Note that an ofnode can either hold a pointer or an offset. There are >> detailed comments on ofnode_union to explain how it is supposed to >> work. > > Right. Thanks for all the detailed infos in the header. The main issue > seems to be, that this CFI patch uses a function from fdtaddr.c > (devfdt_get_addr_size_index), which unconditionally uses dev_of_offset() > without checking if livetree is enabled or not. This breaks on my > 64 bit platform (see below). > >> This patch looks correct to me, but perhaps there is something else >> going on? > > Making this change below, works for me: > > - addr = devfdt_get_addr_size_index(dev, idx, &size); > + addr = dev_read_addr_index(dev, idx); Ouch, sorry for that! One thing I noticed: Technically this fix is no longer needed, since Heinrich's patch ae6b33dcc342 ("dm: fix ofnode_read_addr/size_cel
Re: [PATCH] cfi_flash: Fix devicetree address determination
Hi Simon, On 22.09.20 15:51, Simon Glass wrote: Hi Stefan, On Mon, 21 Sep 2020 at 07:28, Stefan Roese wrote: Hi Andre, (added Simon) On 18.09.20 19:45, Andre Przywara wrote: The cfi-flash driver uses an open-coded version of the generic algorithm to decode and translate multiple frames of a "reg" property. This starts off the wrong foot by using the address-cells and size-cells properties of *this* very node, and not of the parent. This somewhat happened to work back when we were using a wrong default size of 2, but broke about a year ago with commit 0ba41ce1b781 ("libfdt: return correct value if #size-cells property is not present"). Instead of fixing the reinvented wheel, just use the generic function that does all of this properly. This fixes U-Boot on QEMU (outside of EL1), which was crashing due to decoding a wrong start address: DRAM: 1 GiB Flash: "Synchronous Abort" handler, esr 0x9644 elr: 000211dc lr : 000211b0 (reloc) elr: 7ff5e1dc lr : 7ff5e1b0 x0 : 00f0 x1 : 7ff5e1d8 x2 : 7edfbc48 x3 : x4 : x5 : 00f0 x6 : 7edfbc2c x7 : x8 : 7ffd8d70 x9 : 000c x10: 0403 x11: 0055 Signed-off-by: Andre Przywara --- drivers/mtd/cfi_flash.c | 25 +++-- 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c index b7289ba5394..656ff326e17 100644 --- a/drivers/mtd/cfi_flash.c +++ b/drivers/mtd/cfi_flash.c @@ -2468,29 +2468,18 @@ unsigned long flash_init(void) #ifdef CONFIG_CFI_FLASH /* for driver model */ static int cfi_flash_probe(struct udevice *dev) { - const fdt32_t *cell; - int addrc, sizec; - int len, idx; + fdt_addr_t addr; + fdt_size_t size; + int idx; - addrc = dev_read_addr_cells(dev); - sizec = dev_read_size_cells(dev); - - /* decode regs; there may be multiple reg tuples. */ - cell = dev_read_prop(dev, "reg", &len); - if (!cell) - return -ENOENT; - idx = 0; - len /= sizeof(fdt32_t); - while (idx < len) { - phys_addr_t addr; - - addr = dev_translate_address(dev, cell + idx); + for (idx = 0; idx < CFI_MAX_FLASH_BANKS; idx++) { + addr = devfdt_get_addr_size_index(dev, idx, &size); + if (addr == FDT_ADDR_T_NONE) + break; flash_info[cfi_flash_num_flash_banks].dev = dev; flash_info[cfi_flash_num_flash_banks].base = addr; cfi_flash_num_flash_banks++; - - idx += addrc + sizec; } gd->bd->bi_flashstart = flash_info[0].base; This fails on my Octeon MIPS64 platform "octeon_ebb7304". I did some debugging and found that here "of_offset" is a 64 bit value (type long) which gets truncated in dev_of_offset() to 32 bit (type int). This problem only arises when of_live_active() is set. Here, "of_offset" holds a pointer AFACT and truncating it to 32 bits breaks things. I'm wondering why this did not hit me earlier on this 64bit platform. Simon, do you have a quick idea how to solve this? Well I don't think ofnode should use long for of_offset, since int should be enough. ofnode_to_offset() converts an ofnode to a DT offset but only if it is not using livetree. With livetree there are no offsets so this is not going to work. If you define OF_CHECKS you will see that. This does not work right now. I'll send a patch fixing compiling with OF_CHECK enabled shortly. Note that an ofnode can either hold a pointer or an offset. There are detailed comments on ofnode_union to explain how it is supposed to work. Right. Thanks for all the detailed infos in the header. The main issue seems to be, that this CFI patch uses a function from fdtaddr.c (devfdt_get_addr_size_index), which unconditionally uses dev_of_offset() without checking if livetree is enabled or not. This breaks on my 64 bit platform (see below). This patch looks correct to me, but perhaps there is something else going on? Making this change below, works for me: - addr = devfdt_get_addr_size_index(dev, idx, &size); + addr = dev_read_addr_index(dev, idx); Maybe we should make sure, that all functions from fdtaddr.c are not used with livetree active? To prevent similar issues using devfdt_foo() functions with livetree active. Thanks, Stefan
Re: [PATCH] cfi_flash: Fix devicetree address determination
Hi Stefan, On Mon, 21 Sep 2020 at 07:28, Stefan Roese wrote: > > Hi Andre, > > (added Simon) > > On 18.09.20 19:45, Andre Przywara wrote: > > The cfi-flash driver uses an open-coded version of the generic > > algorithm to decode and translate multiple frames of a "reg" property. > > > > This starts off the wrong foot by using the address-cells and size-cells > > properties of *this* very node, and not of the parent. This somewhat > > happened to work back when we were using a wrong default size of 2, > > but broke about a year ago with commit 0ba41ce1b781 ("libfdt: return > > correct value if #size-cells property is not present"). > > > > Instead of fixing the reinvented wheel, just use the generic function > > that does all of this properly. > > > > This fixes U-Boot on QEMU (outside of EL1), which was crashing due to > > decoding a wrong start address: > > DRAM: 1 GiB > > Flash: "Synchronous Abort" handler, esr 0x9644 > > elr: 000211dc lr : 000211b0 (reloc) > > elr: 7ff5e1dc lr : 7ff5e1b0 > > x0 : 00f0 x1 : 7ff5e1d8 > > x2 : 7edfbc48 x3 : > > x4 : x5 : 00f0 > > x6 : 7edfbc2c x7 : > > x8 : 7ffd8d70 x9 : 000c > > x10: 0403 x11: 0055 > > > > > > Signed-off-by: Andre Przywara > > --- > > drivers/mtd/cfi_flash.c | 25 +++-- > > 1 file changed, 7 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c > > index b7289ba5394..656ff326e17 100644 > > --- a/drivers/mtd/cfi_flash.c > > +++ b/drivers/mtd/cfi_flash.c > > @@ -2468,29 +2468,18 @@ unsigned long flash_init(void) > > #ifdef CONFIG_CFI_FLASH /* for driver model */ > > static int cfi_flash_probe(struct udevice *dev) > > { > > - const fdt32_t *cell; > > - int addrc, sizec; > > - int len, idx; > > + fdt_addr_t addr; > > + fdt_size_t size; > > + int idx; > > > > - addrc = dev_read_addr_cells(dev); > > - sizec = dev_read_size_cells(dev); > > - > > - /* decode regs; there may be multiple reg tuples. */ > > - cell = dev_read_prop(dev, "reg", &len); > > - if (!cell) > > - return -ENOENT; > > - idx = 0; > > - len /= sizeof(fdt32_t); > > - while (idx < len) { > > - phys_addr_t addr; > > - > > - addr = dev_translate_address(dev, cell + idx); > > + for (idx = 0; idx < CFI_MAX_FLASH_BANKS; idx++) { > > + addr = devfdt_get_addr_size_index(dev, idx, &size); > > + if (addr == FDT_ADDR_T_NONE) > > + break; > > > > flash_info[cfi_flash_num_flash_banks].dev = dev; > > flash_info[cfi_flash_num_flash_banks].base = addr; > > cfi_flash_num_flash_banks++; > > - > > - idx += addrc + sizec; > > } > > gd->bd->bi_flashstart = flash_info[0].base; > > > > > > This fails on my Octeon MIPS64 platform "octeon_ebb7304". I did some > debugging and found that here "of_offset" is a 64 bit value (type long) > which gets truncated in dev_of_offset() to 32 bit (type int). > > This problem only arises when of_live_active() is set. Here, "of_offset" > holds a pointer AFACT and truncating it to 32 bits breaks things. > > I'm wondering why this did not hit me earlier on this 64bit platform. > Simon, do you have a quick idea how to solve this? Well I don't think ofnode should use long for of_offset, since int should be enough. ofnode_to_offset() converts an ofnode to a DT offset but only if it is not using livetree. With livetree there are no offsets so this is not going to work. If you define OF_CHECKS you will see that. Note that an ofnode can either hold a pointer or an offset. There are detailed comments on ofnode_union to explain how it is supposed to work. This patch looks correct to me, but perhaps there is something else going on? Regards, Simon
Re: [PATCH] cfi_flash: Fix devicetree address determination
Hi Andre, (added Simon) On 18.09.20 19:45, Andre Przywara wrote: The cfi-flash driver uses an open-coded version of the generic algorithm to decode and translate multiple frames of a "reg" property. This starts off the wrong foot by using the address-cells and size-cells properties of *this* very node, and not of the parent. This somewhat happened to work back when we were using a wrong default size of 2, but broke about a year ago with commit 0ba41ce1b781 ("libfdt: return correct value if #size-cells property is not present"). Instead of fixing the reinvented wheel, just use the generic function that does all of this properly. This fixes U-Boot on QEMU (outside of EL1), which was crashing due to decoding a wrong start address: DRAM: 1 GiB Flash: "Synchronous Abort" handler, esr 0x9644 elr: 000211dc lr : 000211b0 (reloc) elr: 7ff5e1dc lr : 7ff5e1b0 x0 : 00f0 x1 : 7ff5e1d8 x2 : 7edfbc48 x3 : x4 : x5 : 00f0 x6 : 7edfbc2c x7 : x8 : 7ffd8d70 x9 : 000c x10: 0403 x11: 0055 Signed-off-by: Andre Przywara --- drivers/mtd/cfi_flash.c | 25 +++-- 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c index b7289ba5394..656ff326e17 100644 --- a/drivers/mtd/cfi_flash.c +++ b/drivers/mtd/cfi_flash.c @@ -2468,29 +2468,18 @@ unsigned long flash_init(void) #ifdef CONFIG_CFI_FLASH /* for driver model */ static int cfi_flash_probe(struct udevice *dev) { - const fdt32_t *cell; - int addrc, sizec; - int len, idx; + fdt_addr_t addr; + fdt_size_t size; + int idx; - addrc = dev_read_addr_cells(dev); - sizec = dev_read_size_cells(dev); - - /* decode regs; there may be multiple reg tuples. */ - cell = dev_read_prop(dev, "reg", &len); - if (!cell) - return -ENOENT; - idx = 0; - len /= sizeof(fdt32_t); - while (idx < len) { - phys_addr_t addr; - - addr = dev_translate_address(dev, cell + idx); + for (idx = 0; idx < CFI_MAX_FLASH_BANKS; idx++) { + addr = devfdt_get_addr_size_index(dev, idx, &size); + if (addr == FDT_ADDR_T_NONE) + break; flash_info[cfi_flash_num_flash_banks].dev = dev; flash_info[cfi_flash_num_flash_banks].base = addr; cfi_flash_num_flash_banks++; - - idx += addrc + sizec; } gd->bd->bi_flashstart = flash_info[0].base; This fails on my Octeon MIPS64 platform "octeon_ebb7304". I did some debugging and found that here "of_offset" is a 64 bit value (type long) which gets truncated in dev_of_offset() to 32 bit (type int). This problem only arises when of_live_active() is set. Here, "of_offset" holds a pointer AFACT and truncating it to 32 bits breaks things. I'm wondering why this did not hit me earlier on this 64bit platform. Simon, do you have a quick idea how to solve this? Thanks, Stefan
[PATCH] cfi_flash: Fix devicetree address determination
The cfi-flash driver uses an open-coded version of the generic algorithm to decode and translate multiple frames of a "reg" property. This starts off the wrong foot by using the address-cells and size-cells properties of *this* very node, and not of the parent. This somewhat happened to work back when we were using a wrong default size of 2, but broke about a year ago with commit 0ba41ce1b781 ("libfdt: return correct value if #size-cells property is not present"). Instead of fixing the reinvented wheel, just use the generic function that does all of this properly. This fixes U-Boot on QEMU (outside of EL1), which was crashing due to decoding a wrong start address: DRAM: 1 GiB Flash: "Synchronous Abort" handler, esr 0x9644 elr: 000211dc lr : 000211b0 (reloc) elr: 7ff5e1dc lr : 7ff5e1b0 x0 : 00f0 x1 : 7ff5e1d8 x2 : 7edfbc48 x3 : x4 : x5 : 00f0 x6 : 7edfbc2c x7 : x8 : 7ffd8d70 x9 : 000c x10: 0403 x11: 0055 Signed-off-by: Andre Przywara --- drivers/mtd/cfi_flash.c | 25 +++-- 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c index b7289ba5394..656ff326e17 100644 --- a/drivers/mtd/cfi_flash.c +++ b/drivers/mtd/cfi_flash.c @@ -2468,29 +2468,18 @@ unsigned long flash_init(void) #ifdef CONFIG_CFI_FLASH /* for driver model */ static int cfi_flash_probe(struct udevice *dev) { - const fdt32_t *cell; - int addrc, sizec; - int len, idx; + fdt_addr_t addr; + fdt_size_t size; + int idx; - addrc = dev_read_addr_cells(dev); - sizec = dev_read_size_cells(dev); - - /* decode regs; there may be multiple reg tuples. */ - cell = dev_read_prop(dev, "reg", &len); - if (!cell) - return -ENOENT; - idx = 0; - len /= sizeof(fdt32_t); - while (idx < len) { - phys_addr_t addr; - - addr = dev_translate_address(dev, cell + idx); + for (idx = 0; idx < CFI_MAX_FLASH_BANKS; idx++) { + addr = devfdt_get_addr_size_index(dev, idx, &size); + if (addr == FDT_ADDR_T_NONE) + break; flash_info[cfi_flash_num_flash_banks].dev = dev; flash_info[cfi_flash_num_flash_banks].base = addr; cfi_flash_num_flash_banks++; - - idx += addrc + sizec; } gd->bd->bi_flashstart = flash_info[0].base; -- 2.17.5