From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?Q?Andr=c3=a9_Przywara?= Date: Wed, 23 Sep 2020 16:38:58 +0100 Subject: [PATCH] cfi_flash: Fix devicetree address determination In-Reply-To: <689f4c44-a996-daa2-0bc6-db78de085114@denx.de> References: <20200918174532.7847-1-andre.przywara@arm.com> <689f4c44-a996-daa2-0bc6-db78de085114@denx.de> Message-ID: <206f9065-e8e9-0053-5038-a7b80dace1be@arm.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 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 >>>> --- >>>> ?? 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