* [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