| Message ID | 20200923232204.24466-1-andre.przywara@arm.com |
|---|---|
| State | Accepted |
| Commit | 492b9917c684bbfd42479e049c00b1e502f8b53f |
| Delegated to: | Stefan Roese |
| Headers | show |
| Series | [v2] cfi_flash: Fix devicetree address determination | expand |
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.przywara@arm.com> > --- > Changes v1 .. v2: > - Use live tree compatible function > - Drop unneeded size variable Reviewed-by: Stefan Roese <sr@denx.de> Thanks, Stefan > 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); > + 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; > > Viele Grüße, Stefan
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.przywara@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); > + 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; > > Viele Grüße, Stefan
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.przywara@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); Best regards Heinrich >> + 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; >> > > > Viele Grüße, > Stefan >
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.przywara@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. Thanks, Stefan
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.przywara@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? Do we need CONFIG_SYS_FLASH_BANKS_SIZES and weak function cfi_flash_bank_size() at all? Best regards Heinrich > > Thanks, > Stefan
On 08/10/2020 11:15, Heinrich Schuchardt wrote: Hi, > 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.przywara@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? Do we need > CONFIG_SYS_FLASH_BANKS_SIZES and weak function cfi_flash_bank_size() at all? That's a good point: given that this symbol is in config_whitelist.txt, it's probably some legacy from the dawn of time. Maybe for some hacks, as those lines in cfi_flash.c suggest: ============ max_size = cfi_flash_bank_size(banknum); if (max_size && info->size > max_size) { debug("[truncated from %ldMiB]", info->size >> 20); info->size = max_size; } ============ Maybe someone mounted a bigger flash chip than there was space in the MMIO frame? But I totally agree that this is very confusing and should either be removed entirely (preferred, but would need to be tested on those boards using it) or extended to cover DTBs as well. Cheers, Andre
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.przywara@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
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); + 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 (-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.przywara@arm.com> --- 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(-)