On 08.10.20 12:15, Heinrich Schuchardt wrote:
On 08.10.20 11:49, Stefan Roese wrote:
On 08.10.20 10:39, Heinrich Schuchardt wrote:
On 08.10.20 09:08, Stefan Roese wrote:
On 24.09.20 01:22, 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 (-arm64), which was crashing due to decoding
a wrong flash base 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.przyw...@arm.com>
Applied to u-boot-cfi-flash/master
Thanks,
Stefan
---
Changes v1 .. v2:
- Use live tree compatible function
- Drop unneeded size variable
drivers/mtd/cfi_flash.c | 24 ++++++------------------
1 file changed, 6 insertions(+), 18 deletions(-)
diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
index b7289ba5394..9e3a652f445 100644
--- a/drivers/mtd/cfi_flash.c
+++ b/drivers/mtd/cfi_flash.c
@@ -2468,29 +2468,17 @@ 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;
+ 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 = dev_read_addr_index(dev, idx);
Why don't you additionally read the size here to populate
flash_info[].size?
fdt_size_t size;
addr = dev_read_addr_size_index(dev, idx, &size);
It was not done before this either. IIRC, then the size is auto detected
by querying the flash chip.
Do you see any issue without this? Feel free to send a follow up patch
is something needs to get fixed / tuned.
Yes, there is a function flash_get_size().
I am not worried about this special patch but about the logic of the
driver as a whole.
Why does function flash_get_size() consider CONFIG_SYS_FLASH_BANKS_SIZES
but not the size information from the DTB?
All this CFI code is pretty ancient. Many years before devicetree was
introduced to U-Boot. IIRC, there were many boards that could be
equipped with different CFI flash chips (different sizes). That's were
this flash_get_size() etc comes from.
Do we need
CONFIG_SYS_FLASH_BANKS_SIZES and weak function cfi_flash_bank_size() at all?
I agree that this code could benefit from some "cleanup", removing
some of the old legacy stuff. Perhaps I will find some time to tackle
this in the near future. But if someone beats me here, I would not
be too disappointed. ;)
Thanks,
Stefan
