* [PATCH] cfi_flash: Fix devicetree address determination
@ 2020-09-18 17:45 Andre Przywara
2020-09-21 13:28 ` Stefan Roese
0 siblings, 1 reply; 6+ messages in thread
From: Andre Przywara @ 2020-09-18 17:45 UTC (permalink / raw)
To: u-boot
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;
--
2.17.5
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH] cfi_flash: Fix devicetree address determination 2020-09-18 17:45 [PATCH] cfi_flash: Fix devicetree address determination Andre Przywara @ 2020-09-21 13:28 ` Stefan Roese 2020-09-22 13:51 ` Simon Glass 0 siblings, 1 reply; 6+ messages in thread From: Stefan Roese @ 2020-09-21 13:28 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] cfi_flash: Fix devicetree address determination 2020-09-21 13:28 ` Stefan Roese @ 2020-09-22 13:51 ` Simon Glass 2020-09-23 5:26 ` Stefan Roese 0 siblings, 1 reply; 6+ messages in thread From: Simon Glass @ 2020-09-22 13:51 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] cfi_flash: Fix devicetree address determination 2020-09-22 13:51 ` Simon Glass @ 2020-09-23 5:26 ` Stefan Roese 2020-09-23 15:38 ` André Przywara 0 siblings, 1 reply; 6+ messages in thread From: Stefan Roese @ 2020-09-23 5:26 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] cfi_flash: Fix devicetree address determination 2020-09-23 5:26 ` Stefan Roese @ 2020-09-23 15:38 ` André Przywara 2020-09-24 16:08 ` Simon Glass 0 siblings, 1 reply; 6+ messages in thread From: André Przywara @ 2020-09-23 15:38 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] cfi_flash: Fix devicetree address determination 2020-09-23 15:38 ` André Przywara @ 2020-09-24 16:08 ` Simon Glass 0 siblings, 0 replies; 6+ messages in thread From: Simon Glass @ 2020-09-24 16:08 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-09-24 16:08 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-09-18 17:45 [PATCH] cfi_flash: Fix devicetree address determination Andre Przywara 2020-09-21 13:28 ` Stefan Roese 2020-09-22 13:51 ` Simon Glass 2020-09-23 5:26 ` Stefan Roese 2020-09-23 15:38 ` André Przywara 2020-09-24 16:08 ` Simon Glass
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox