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