| Message ID | 20200918174532.7847-1-andre.przywara@arm.com |
|---|---|
| State | Superseded |
| Delegated to: | Stefan Roese |
| Headers | show |
| Series | cfi_flash: Fix devicetree address determination | expand |
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 0x96000044 > elr: 00000000000211dc lr : 00000000000211b0 (reloc) > elr: 000000007ff5e1dc lr : 000000007ff5e1b0 > x0 : 00000000000000f0 x1 : 000000007ff5e1d8 > x2 : 000000007edfbc48 x3 : 0000000000000000 > x4 : 0000000000000000 x5 : 00000000000000f0 > x6 : 000000007edfbc2c x7 : 0000000000000000 > x8 : 000000007ffd8d70 x9 : 000000000000000c > x10: 0400000000000003 x11: 0000000000000055 > ^^^^^^^^^^^^^^^^ > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > 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
Hi Stefan, On Mon, 21 Sep 2020 at 07:28, Stefan Roese <sr@denx.de> 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 0x96000044 > > elr: 00000000000211dc lr : 00000000000211b0 (reloc) > > elr: 000000007ff5e1dc lr : 000000007ff5e1b0 > > x0 : 00000000000000f0 x1 : 000000007ff5e1d8 > > x2 : 000000007edfbc48 x3 : 0000000000000000 > > x4 : 0000000000000000 x5 : 00000000000000f0 > > x6 : 000000007edfbc2c x7 : 0000000000000000 > > x8 : 000000007ffd8d70 x9 : 000000000000000c > > x10: 0400000000000003 x11: 0000000000000055 > > ^^^^^^^^^^^^^^^^ > > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > --- > > 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
Hi Simon, On 22.09.20 15:51, Simon Glass wrote: > Hi Stefan, > > On Mon, 21 Sep 2020 at 07:28, Stefan Roese <sr@denx.de> 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 0x96000044 >>> elr: 00000000000211dc lr : 00000000000211b0 (reloc) >>> elr: 000000007ff5e1dc lr : 000000007ff5e1b0 >>> x0 : 00000000000000f0 x1 : 000000007ff5e1d8 >>> x2 : 000000007edfbc48 x3 : 0000000000000000 >>> x4 : 0000000000000000 x5 : 00000000000000f0 >>> x6 : 000000007edfbc2c x7 : 0000000000000000 >>> x8 : 000000007ffd8d70 x9 : 000000000000000c >>> x10: 0400000000000003 x11: 0000000000000055 >>> ^^^^^^^^^^^^^^^^ >>> >>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >>> --- >>> 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
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 <sr@denx.de> 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 0x96000044 >>>> elr: 00000000000211dc lr : 00000000000211b0 (reloc) >>>> elr: 000000007ff5e1dc lr : 000000007ff5e1b0 >>>> x0 : 00000000000000f0 x1 : 000000007ff5e1d8 >>>> x2 : 000000007edfbc48 x3 : 0000000000000000 >>>> x4 : 0000000000000000 x5 : 00000000000000f0 >>>> x6 : 000000007edfbc2c x7 : 0000000000000000 >>>> x8 : 000000007ffd8d70 x9 : 000000000000000c >>>> x10: 0400000000000003 x11: 0000000000000055 >>>> ^^^^^^^^^^^^^^^^ >>>> >>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >>>> --- >>>> 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_cells()") recently fixed that particular issue (I missed that one when doing the bisect earlier). However I still consider this patch here useful, since it removes code duplication (and the original bug gives a good rationale for that!). So I will repost this one here, but leave it up to you whether to merge it or not. Also: this function was the only user of dev_read_{addr,size}_cells(). Shall we consequently remove them? They have this somewhat surprising feature of querying the parent now, which prevents them from being used when someone want to determine the current #a-c and #s-c applicable for subnodes, for instance. > 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. That sounds useful, but can this be done easily? Cheers, Andre
Hi André, On Wed, 23 Sep 2020 at 09:39, André Przywara <andre.przywara@arm.com> 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 <sr@denx.de> 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 0x96000044 > >>>> elr: 00000000000211dc lr : 00000000000211b0 (reloc) > >>>> elr: 000000007ff5e1dc lr : 000000007ff5e1b0 > >>>> x0 : 00000000000000f0 x1 : 000000007ff5e1d8 > >>>> x2 : 000000007edfbc48 x3 : 0000000000000000 > >>>> x4 : 0000000000000000 x5 : 00000000000000f0 > >>>> x6 : 000000007edfbc2c x7 : 0000000000000000 > >>>> x8 : 000000007ffd8d70 x9 : 000000000000000c > >>>> x10: 0400000000000003 x11: 0000000000000055 > >>>> ^^^^^^^^^^^^^^^^ > >>>> > >>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> > >>>> --- > >>>> 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_cells()") > recently fixed that particular issue (I missed that one when doing the > bisect earlier). > > However I still consider this patch here useful, since it removes code > duplication (and the original bug gives a good rationale for that!). > So I will repost this one here, but leave it up to you whether to merge > it or not. > > Also: this function was the only user of dev_read_{addr,size}_cells(). > Shall we consequently remove them? They have this somewhat surprising > feature of querying the parent now, which prevents them from being used > when someone want to determine the current #a-c and #s-c applicable for > subnodes, for instance. Yes I agree that is surprising. If we don't think they will be useful in future I think removing them is OK. > > > 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. > That sounds useful, but can this be done easily? We could add a check at the start of each function, perhaps. Regards, Simon
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;
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 0x96000044 elr: 00000000000211dc lr : 00000000000211b0 (reloc) elr: 000000007ff5e1dc lr : 000000007ff5e1b0 x0 : 00000000000000f0 x1 : 000000007ff5e1d8 x2 : 000000007edfbc48 x3 : 0000000000000000 x4 : 0000000000000000 x5 : 00000000000000f0 x6 : 000000007edfbc2c x7 : 0000000000000000 x8 : 000000007ffd8d70 x9 : 000000000000000c x10: 0400000000000003 x11: 0000000000000055 ^^^^^^^^^^^^^^^^ Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- drivers/mtd/cfi_flash.c | 25 +++++++------------------ 1 file changed, 7 insertions(+), 18 deletions(-)