[U-Boot] [PATCH] fdt_support: Use VLA instead of MEMORY_BANKS_MAX
From: Ramon Fried ramon.fried@intel.com
Instead of relaying on user to configure MEMORY_BANKS_MAX correctly, use VLA (variable length array) to accommodate the required banks.
Fixes: 2a1f4f1758b5 ("Revert "fdt_support: Use CONFIG_NR_DRAM_BANKS if defined"")
Signed-off-by: Ramon Fried ramon.fried@intel.com --- common/fdt_support.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/common/fdt_support.c b/common/fdt_support.c index 34d2bd5..e898236 100644 --- a/common/fdt_support.c +++ b/common/fdt_support.c @@ -409,19 +409,14 @@ static int fdt_pack_reg(const void *fdt, void *buf, u64 *address, u64 *size, return p - (char *)buf; }
-#define MEMORY_BANKS_MAX 4 int fdt_fixup_memory_banks(void *blob, u64 start[], u64 size[], int banks) { int err, nodeoffset; int len, i; - u8 tmp[MEMORY_BANKS_MAX * 16]; /* Up to 64-bit address + 64-bit size */ + u8 tmp[banks * 16]; /* Up to 64-bit address + 64-bit size */
- if (banks > MEMORY_BANKS_MAX) { - printf("%s: num banks %d exceeds hardcoded limit %d." - " Recompile with higher MEMORY_BANKS_MAX?\n", - __FUNCTION__, banks, MEMORY_BANKS_MAX); + if (!banks) return -1; - }
err = fdt_check_header(blob); if (err < 0) {
On Sun, Aug 12, 2018 at 9:37 PM, Ramon Fried ramon.fried@gmail.com wrote:
From: Ramon Fried ramon.fried@intel.com
Instead of relaying on user to configure MEMORY_BANKS_MAX correctly, use VLA (variable length array) to accommodate the required banks.
With the kernel actively removing VLAs [1] does it make sense for us to use them?
[1] https://lwn.net/Articles/749064/
Fixes: 2a1f4f1758b5 ("Revert "fdt_support: Use CONFIG_NR_DRAM_BANKS if defined"")
Signed-off-by: Ramon Fried ramon.fried@intel.com
common/fdt_support.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/common/fdt_support.c b/common/fdt_support.c index 34d2bd5..e898236 100644 --- a/common/fdt_support.c +++ b/common/fdt_support.c @@ -409,19 +409,14 @@ static int fdt_pack_reg(const void *fdt, void *buf, u64 *address, u64 *size, return p - (char *)buf; }
-#define MEMORY_BANKS_MAX 4 int fdt_fixup_memory_banks(void *blob, u64 start[], u64 size[], int banks) { int err, nodeoffset; int len, i;
u8 tmp[MEMORY_BANKS_MAX * 16]; /* Up to 64-bit address + 64-bit size */
u8 tmp[banks * 16]; /* Up to 64-bit address + 64-bit size */
if (banks > MEMORY_BANKS_MAX) {printf("%s: num banks %d exceeds hardcoded limit %d."" Recompile with higher MEMORY_BANKS_MAX?\n",__FUNCTION__, banks, MEMORY_BANKS_MAX);
if (!banks) return -1;
} err = fdt_check_header(blob); if (err < 0) {-- 2.7.4
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
On Mon, Aug 13, 2018 at 08:20:03AM +0100, Peter Robinson wrote:
On Sun, Aug 12, 2018 at 9:37 PM, Ramon Fried ramon.fried@gmail.com wrote:
From: Ramon Fried ramon.fried@intel.com
Instead of relaying on user to configure MEMORY_BANKS_MAX correctly, use VLA (variable length array) to accommodate the required banks.
With the kernel actively removing VLAs [1] does it make sense for us to use them?
Agreed.
Also, why is the answer NOT to go back to the way things were with 5e5745465c94 and increase CONFIG_NR_DRAM_BANKS when needed? It seems like the problem may be platforms setting this to 1 when really it should be higher _even_if_ the memory space in question is contiguous. Yes, you've always been able to say we have 1 bank of N physical chips and it's been OK, but now I guess we're finally hitting the point where that doesn't work.
[1] https://lwn.net/Articles/749064/
Fixes: 2a1f4f1758b5 ("Revert "fdt_support: Use CONFIG_NR_DRAM_BANKS if defined"")
Signed-off-by: Ramon Fried ramon.fried@intel.com
common/fdt_support.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/common/fdt_support.c b/common/fdt_support.c index 34d2bd5..e898236 100644 --- a/common/fdt_support.c +++ b/common/fdt_support.c @@ -409,19 +409,14 @@ static int fdt_pack_reg(const void *fdt, void *buf, u64 *address, u64 *size, return p - (char *)buf; }
-#define MEMORY_BANKS_MAX 4 int fdt_fixup_memory_banks(void *blob, u64 start[], u64 size[], int banks) { int err, nodeoffset; int len, i;
u8 tmp[MEMORY_BANKS_MAX * 16]; /* Up to 64-bit address + 64-bit size */
u8 tmp[banks * 16]; /* Up to 64-bit address + 64-bit size */
if (banks > MEMORY_BANKS_MAX) {printf("%s: num banks %d exceeds hardcoded limit %d."" Recompile with higher MEMORY_BANKS_MAX?\n",__FUNCTION__, banks, MEMORY_BANKS_MAX);
if (!banks) return -1;
} err = fdt_check_header(blob); if (err < 0) {-- 2.7.4
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
On Mon, Aug 13, 2018 at 5:52 PM Tom Rini trini@konsulko.com wrote:
On Mon, Aug 13, 2018 at 08:20:03AM +0100, Peter Robinson wrote:
On Sun, Aug 12, 2018 at 9:37 PM, Ramon Fried ramon.fried@gmail.com
wrote:
From: Ramon Fried ramon.fried@intel.com
Instead of relaying on user to configure MEMORY_BANKS_MAX correctly, use VLA (variable length array) to accommodate the required banks.
With the kernel actively removing VLAs [1] does it make sense for us to use them?
Agreed.
Also, why is the answer NOT to go back to the way things were with 5e5745465c94 and increase CONFIG_NR_DRAM_BANKS when needed? It seems
The whole purpose of my patch was to enable to fixup more banks than defined in CONFIG_NR_DRAM_BANKS.
Another option would be to add +#ifndef MEMORY_BANKS_MAX #define MEMORY_BANKS_MAX 4 +#endif and let the use alter the value in include/configs if necessary.
Thanks, Ramon.
like the problem may be platforms setting this to 1 when really it
should be higher _even_if_ the memory space in question is contiguous. Yes, you've always been able to say we have 1 bank of N physical chips and it's been OK, but now I guess we're finally hitting the point where that doesn't work.
[1] https://lwn.net/Articles/749064/
Fixes: 2a1f4f1758b5 ("Revert "fdt_support: Use CONFIG_NR_DRAM_BANKS if defined"")
Signed-off-by: Ramon Fried ramon.fried@intel.com
common/fdt_support.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/common/fdt_support.c b/common/fdt_support.c index 34d2bd5..e898236 100644 --- a/common/fdt_support.c +++ b/common/fdt_support.c @@ -409,19 +409,14 @@ static int fdt_pack_reg(const void *fdt, void
*buf, u64 *address, u64 *size,
return p - (char *)buf;}
-#define MEMORY_BANKS_MAX 4 int fdt_fixup_memory_banks(void *blob, u64 start[], u64 size[], int
banks)
{ int err, nodeoffset; int len, i;
u8 tmp[MEMORY_BANKS_MAX * 16]; /* Up to 64-bit address +64-bit size */
u8 tmp[banks * 16]; /* Up to 64-bit address + 64-bit size */
if (banks > MEMORY_BANKS_MAX) {printf("%s: num banks %d exceeds hardcoded limit %d."" Recompile with higher MEMORY_BANKS_MAX?\n",__FUNCTION__, banks, MEMORY_BANKS_MAX);
if (!banks) return -1;
} err = fdt_check_header(blob); if (err < 0) {-- 2.7.4
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
-- Tom
On Mon, Aug 13, 2018 at 09:54:30PM +0300, Ramon Fried wrote:
On Mon, Aug 13, 2018 at 5:52 PM Tom Rini trini@konsulko.com wrote:
On Mon, Aug 13, 2018 at 08:20:03AM +0100, Peter Robinson wrote:
On Sun, Aug 12, 2018 at 9:37 PM, Ramon Fried ramon.fried@gmail.com
wrote:
From: Ramon Fried ramon.fried@intel.com
Instead of relaying on user to configure MEMORY_BANKS_MAX correctly, use VLA (variable length array) to accommodate the required banks.
With the kernel actively removing VLAs [1] does it make sense for us to use them?
Agreed.
Also, why is the answer NOT to go back to the way things were with 5e5745465c94 and increase CONFIG_NR_DRAM_BANKS when needed? It seems
The whole purpose of my patch was to enable to fixup more banks than defined in CONFIG_NR_DRAM_BANKS.
Another option would be to add +#ifndef MEMORY_BANKS_MAX #define MEMORY_BANKS_MAX 4 +#endif and let the use alter the value in include/configs if necessary.
I think for our purposes it's best to say that, as the code was written, if we need more banks to be configured at build time, they should be. This may also mean that certain platforms need to bump their default up in order to support the hardware you're using that shows this issue. Thanks!
On August 13, 2018 7:08:22 PM GMT+03:00, Tom Rini trini@konsulko.com wrote:
On Mon, Aug 13, 2018 at 09:54:30PM +0300, Ramon Fried wrote:
On Mon, Aug 13, 2018 at 5:52 PM Tom Rini trini@konsulko.com wrote:
On Mon, Aug 13, 2018 at 08:20:03AM +0100, Peter Robinson wrote:
On Sun, Aug 12, 2018 at 9:37 PM, Ramon Fried
wrote:
From: Ramon Fried ramon.fried@intel.com
Instead of relaying on user to configure MEMORY_BANKS_MAX correctly, use VLA (variable length array) to accommodate the required banks.
With the kernel actively removing VLAs [1] does it make sense for
us
to use them?
Agreed.
Also, why is the answer NOT to go back to the way things were with 5e5745465c94 and increase CONFIG_NR_DRAM_BANKS when needed? It
seems
The whole purpose of my patch was to enable to fixup more banks than defined in CONFIG_NR_DRAM_BANKS.
Another option would be to add +#ifndef MEMORY_BANKS_MAX #define MEMORY_BANKS_MAX 4 +#endif and let the use alter the value in include/configs if necessary.
I think for our purposes it's best to say that, as the code was written, if we need more banks to be configured at build time, they should be. This may also mean that certain platforms need to bump their default up in order to support the hardware you're using that shows this issue. Thanks!
I'm confused. To which hardware you're referring to? Do you still think we should revert my patch?
On Mon, Aug 13, 2018 at 07:14:00PM +0300, Ramon Fried wrote:
On August 13, 2018 7:08:22 PM GMT+03:00, Tom Rini trini@konsulko.com wrote:
On Mon, Aug 13, 2018 at 09:54:30PM +0300, Ramon Fried wrote:
On Mon, Aug 13, 2018 at 5:52 PM Tom Rini trini@konsulko.com wrote:
On Mon, Aug 13, 2018 at 08:20:03AM +0100, Peter Robinson wrote:
On Sun, Aug 12, 2018 at 9:37 PM, Ramon Fried
wrote:
From: Ramon Fried ramon.fried@intel.com
Instead of relaying on user to configure MEMORY_BANKS_MAX correctly, use VLA (variable length array) to accommodate the required banks.
With the kernel actively removing VLAs [1] does it make sense for
us
to use them?
Agreed.
Also, why is the answer NOT to go back to the way things were with 5e5745465c94 and increase CONFIG_NR_DRAM_BANKS when needed? It
seems
The whole purpose of my patch was to enable to fixup more banks than defined in CONFIG_NR_DRAM_BANKS.
Another option would be to add +#ifndef MEMORY_BANKS_MAX #define MEMORY_BANKS_MAX 4 +#endif and let the use alter the value in include/configs if necessary.
I think for our purposes it's best to say that, as the code was written, if we need more banks to be configured at build time, they should be. This may also mean that certain platforms need to bump their default up in order to support the hardware you're using that shows this issue. Thanks!
I'm confused. To which hardware you're referring to? Do you still think we should revert my patch?
Yes, I think we should bring the code back to the way it was for a long while. And I assume there was a specific piece of hardware that triggered this round of changes?
On August 13, 2018 7:15:14 PM GMT+03:00, Tom Rini trini@konsulko.com wrote:
On Mon, Aug 13, 2018 at 07:14:00PM +0300, Ramon Fried wrote:
On August 13, 2018 7:08:22 PM GMT+03:00, Tom Rini
trini@konsulko.com wrote:
On Mon, Aug 13, 2018 at 09:54:30PM +0300, Ramon Fried wrote:
On Mon, Aug 13, 2018 at 5:52 PM Tom Rini trini@konsulko.com
wrote:
On Mon, Aug 13, 2018 at 08:20:03AM +0100, Peter Robinson wrote:
On Sun, Aug 12, 2018 at 9:37 PM, Ramon Fried
wrote:
> From: Ramon Fried ramon.fried@intel.com > > Instead of relaying on user to configure MEMORY_BANKS_MAX > correctly, use VLA (variable length array) to accommodate
the
> required banks.
With the kernel actively removing VLAs [1] does it make sense
for
us
to use them?
Agreed.
Also, why is the answer NOT to go back to the way things were
with
5e5745465c94 and increase CONFIG_NR_DRAM_BANKS when needed? It
seems
The whole purpose of my patch was to enable to fixup more banks
than
defined in CONFIG_NR_DRAM_BANKS.
Another option would be to add +#ifndef MEMORY_BANKS_MAX #define MEMORY_BANKS_MAX 4 +#endif and let the use alter the value in include/configs if necessary.
I think for our purposes it's best to say that, as the code was written, if we need more banks to be configured at build time, they should
be.
This may also mean that certain platforms need to bump their default
up
in order to support the hardware you're using that shows this issue. Thanks!
I'm confused. To which hardware you're referring to? Do you still think we should revert my patch?
Yes, I think we should bring the code back to the way it was for a long while. And I assume there was a specific piece of hardware that triggered this round of changes?
Yes. Dragonboards. I can implement this fixup function in the snapdragon arch folder.
On Mon, Aug 13, 2018 at 7:22 PM Ramon Fried ramon.fried@gmail.com wrote:
On August 13, 2018 7:15:14 PM GMT+03:00, Tom Rini trini@konsulko.com wrote:
On Mon, Aug 13, 2018 at 07:14:00PM +0300, Ramon Fried wrote:
On August 13, 2018 7:08:22 PM GMT+03:00, Tom Rini
trini@konsulko.com wrote:
On Mon, Aug 13, 2018 at 09:54:30PM +0300, Ramon Fried wrote:
On Mon, Aug 13, 2018 at 5:52 PM Tom Rini trini@konsulko.com
wrote:
On Mon, Aug 13, 2018 at 08:20:03AM +0100, Peter Robinson wrote:
> On Sun, Aug 12, 2018 at 9:37 PM, Ramon Fried
wrote: > > From: Ramon Fried ramon.fried@intel.com > > > > Instead of relaying on user to configure MEMORY_BANKS_MAX > > correctly, use VLA (variable length array) to accommodate
the
> > required banks. > > With the kernel actively removing VLAs [1] does it make sense
for
us
> to use them?
Agreed.
Also, why is the answer NOT to go back to the way things were
with
5e5745465c94 and increase CONFIG_NR_DRAM_BANKS when needed? It
seems
The whole purpose of my patch was to enable to fixup more banks
than
defined in CONFIG_NR_DRAM_BANKS.
Another option would be to add +#ifndef MEMORY_BANKS_MAX #define MEMORY_BANKS_MAX 4 +#endif and let the use alter the value in include/configs if necessary.
I think for our purposes it's best to say that, as the code was written, if we need more banks to be configured at build time, they should
be.
This may also mean that certain platforms need to bump their default
up
in order to support the hardware you're using that shows this issue. Thanks!
I'm confused. To which hardware you're referring to? Do you still think we should revert my patch?
Yes, I think we should bring the code back to the way it was for a long while. And I assume there was a specific piece of hardware that triggered this round of changes?
Yes. Dragonboards. I can implement this fixup function in the snapdragon arch folder.
Tom, a last effort to reduce code duplication. is this acceptable ?
#if CONFIG_NR_DRAM_BANKS > 4 #define MEMORY_BANKS_MAX CONFIG_NR_DRAM_BANKS #else #define MEMORY_BANKS_MAX 4 #endif
On Mon, Aug 13, 2018 at 10:55:03PM +0300, Ramon Fried wrote:
On Mon, Aug 13, 2018 at 7:22 PM Ramon Fried ramon.fried@gmail.com wrote:
On August 13, 2018 7:15:14 PM GMT+03:00, Tom Rini trini@konsulko.com wrote:
On Mon, Aug 13, 2018 at 07:14:00PM +0300, Ramon Fried wrote:
On August 13, 2018 7:08:22 PM GMT+03:00, Tom Rini
trini@konsulko.com wrote:
On Mon, Aug 13, 2018 at 09:54:30PM +0300, Ramon Fried wrote:
On Mon, Aug 13, 2018 at 5:52 PM Tom Rini trini@konsulko.com
wrote:
> On Mon, Aug 13, 2018 at 08:20:03AM +0100, Peter Robinson wrote: > > > On Sun, Aug 12, 2018 at 9:37 PM, Ramon Fried
> wrote: > > > From: Ramon Fried ramon.fried@intel.com > > > > > > Instead of relaying on user to configure MEMORY_BANKS_MAX > > > correctly, use VLA (variable length array) to accommodate
the
> > > required banks. > > > > With the kernel actively removing VLAs [1] does it make sense
for
us
> > to use them? > > Agreed. > > Also, why is the answer NOT to go back to the way things were
with
> 5e5745465c94 and increase CONFIG_NR_DRAM_BANKS when needed? It
seems
> The whole purpose of my patch was to enable to fixup more banks
than
defined in CONFIG_NR_DRAM_BANKS.
Another option would be to add +#ifndef MEMORY_BANKS_MAX #define MEMORY_BANKS_MAX 4 +#endif and let the use alter the value in include/configs if necessary.
I think for our purposes it's best to say that, as the code was written, if we need more banks to be configured at build time, they should
be.
This may also mean that certain platforms need to bump their default
up
in order to support the hardware you're using that shows this issue. Thanks!
I'm confused. To which hardware you're referring to? Do you still think we should revert my patch?
Yes, I think we should bring the code back to the way it was for a long while. And I assume there was a specific piece of hardware that triggered this round of changes?
Yes. Dragonboards. I can implement this fixup function in the snapdragon arch folder.
Tom, a last effort to reduce code duplication. is this acceptable ?
#if CONFIG_NR_DRAM_BANKS > 4 #define MEMORY_BANKS_MAX CONFIG_NR_DRAM_BANKS #else #define MEMORY_BANKS_MAX 4 #endif
If you've got some time, can you add CONFIG_NR_DRAM_BANKS to Kconfig and set the default to 4 ? I'll take care of re-running moveconfig.py if there's conflicts. This could probably go in the top-level Kconfig file near the malloc options. Thanks!
On August 13, 2018 7:59:05 PM GMT+03:00, Tom Rini trini@konsulko.com wrote:
On Mon, Aug 13, 2018 at 10:55:03PM +0300, Ramon Fried wrote:
On Mon, Aug 13, 2018 at 7:22 PM Ramon Fried ramon.fried@gmail.com
wrote:
On August 13, 2018 7:15:14 PM GMT+03:00, Tom Rini
wrote:
On Mon, Aug 13, 2018 at 07:14:00PM +0300, Ramon Fried wrote:
On August 13, 2018 7:08:22 PM GMT+03:00, Tom Rini
trini@konsulko.com wrote:
On Mon, Aug 13, 2018 at 09:54:30PM +0300, Ramon Fried wrote: > On Mon, Aug 13, 2018 at 5:52 PM Tom Rini trini@konsulko.com
wrote:
> > > On Mon, Aug 13, 2018 at 08:20:03AM +0100, Peter Robinson
wrote:
> > > > > On Sun, Aug 12, 2018 at 9:37 PM, Ramon Fried ramon.fried@gmail.com > > wrote: > > > > From: Ramon Fried ramon.fried@intel.com > > > > > > > > Instead of relaying on user to configure
MEMORY_BANKS_MAX
> > > > correctly, use VLA (variable length array) to
accommodate
the
> > > > required banks. > > > > > > With the kernel actively removing VLAs [1] does it make
sense
for
us > > > to use them? > > > > Agreed. > > > > Also, why is the answer NOT to go back to the way things
were
with
> > 5e5745465c94 and increase CONFIG_NR_DRAM_BANKS when needed?
It
seems > > > The whole purpose of my patch was to enable to fixup more
banks
than
> defined in > CONFIG_NR_DRAM_BANKS. > > Another option would be to add > +#ifndef MEMORY_BANKS_MAX > #define MEMORY_BANKS_MAX 4 > +#endif > and let the use alter the value in include/configs if
necessary.
I think for our purposes it's best to say that, as the code was written, if we need more banks to be configured at build time, they
should
be.
This may also mean that certain platforms need to bump their
default
up
in order to support the hardware you're using that shows this
issue.
Thanks!
I'm confused. To which hardware you're referring to? Do you
still
think we should revert my patch?
Yes, I think we should bring the code back to the way it was for a
long
while. And I assume there was a specific piece of hardware that triggered this round of changes?
Yes. Dragonboards. I can implement this fixup function in the snapdragon arch folder.
Tom, a last effort to reduce code duplication. is this acceptable ?
#if CONFIG_NR_DRAM_BANKS > 4 #define MEMORY_BANKS_MAX CONFIG_NR_DRAM_BANKS #else #define MEMORY_BANKS_MAX 4 #endif
If you've got some time, can you add CONFIG_NR_DRAM_BANKS to Kconfig and set the default to 4 ? I'll take care of re-running moveconfig.py if there's conflicts. This could probably go in the top-level Kconfig file near the malloc options. Thanks!
Sure. Np
participants (3)
-
Peter Robinson -
Ramon Fried -
Tom Rini