fdt_support: Use VLA instead of MEMORY_BANKS_MAX

PATCH Superseded Ramon Fried 3 participants 11 messages Aug 12, 2018 – Aug 13, 2018

lore

Share & tools

lore

Ramon Fried

author

2018-08-12 20:37 UTC #0 lore raw

PATCH 1/1 fdt_support: Use VLA instead of MEMORY_BANKS_MAX

From: Ramon Fried <[email protected]>

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 <[email protected]>

---
 common/fdt_support.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

common/fdt_support.c +2 -7

index 34d2bd5..e898236 100644
@@ -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) {

Signature · 1 line

2.7.4

Peter Robinson

2018-08-13 07:20 UTC #1 lore raw

PATCH 1/1 fdt_support: Use VLA instead of MEMORY_BANKS_MAX

Quoted 5 lines · Ramon Fried

From: Ramon Fried <[email protected]>
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/

Quoted 41 lines

Fixes 2a1f4f1758b5 ("Revert "fdt_support: Use CONFIG_NR_DRAM_BANKS if

defined"")

Signed-off-by Ramon Fried <[email protected]>

---
 common/fdt_support.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

common/fdt_support.c +2 -8

index 34d2bd5..e898236 100644
@@ -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 at lists.denx.de
https://lists.denx.de/listinfo/u-boot

Tom Rini

2018-08-13 14:52 UTC #2 lore raw

PATCH 1/1 fdt_support: Use VLA instead of MEMORY_BANKS_MAX

Quoted 9 lines · Peter Robinson

Quoted 5 lines · Ramon Fried
From: Ramon Fried <[email protected]>
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.

Quoted 44 lines

[1] https://lwn.net/Articles/749064/

Quoted 41 lines

Fixes 2a1f4f1758b5 ("Revert "fdt_support: Use CONFIG_NR_DRAM_BANKS if

defined"")

Signed-off-by Ramon Fried <[email protected]>

---
 common/fdt_support.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

common/fdt_support.c +2 -8

index 34d2bd5..e898236 100644
@@ -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 at lists.denx.de
https://lists.denx.de/listinfo/u-boot

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180813/49254a37/attachment.sig>

Ramon Fried

author

2018-08-13 18:54 UTC #3 lore raw

PATCH 1/1 fdt_support: Use VLA instead of MEMORY_BANKS_MAX

Quoted 10 lines · Tom Rini

Quoted 1 line · Peter Robinson
On Sun, Aug 12, 2018 at 9:37 PM, Ramon Fried <[email protected]>
Quoted 6 lines · wrote:
Quoted 5 lines
From: Ramon Fried <[email protected]>
Instead of relaying on user to configure MEMORY_BANKS_MAX correctly, use VLA (variable length array) to accommodate the required banks.

Quoted 8 lines

Quoted 2 lines
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

Quoted 56 lines

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.
Quoted 11 lines
[1] https://lwn.net/Articles/749064/
Quoted 8 lines
Fixes 2a1f4f1758b5 ("Revert "fdt_support: Use CONFIG_NR_DRAM_BANKS if
defined"")
Signed-off-by Ramon Fried <[email protected]>
---
 common/fdt_support.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)
Quoted 5 lines
common/fdt_support.c static int fdt_pack_reg(const void *fdt, void
Quoted 5 lines
common/fdt_support.c +0 -0
index 34d2bd5..e898236 100644
@@ -409,19 +409,14 @@ static int fdt_pack_reg(const void *fdt, void
*buf, u64 *address, u64 *size,
Quoted 5 lines
common/fdt_support.c static int fdt_pack_reg(const void *fdt, void *buf, u64 *address, u64 *size,
Quoted 5 lines
common/fdt_support.c +0 -1
        return p - (char *)buf;
 }
 
-#define MEMORY_BANKS_MAX 4
 int fdt_fixup_memory_banks(void *blob, u64 start[], u64 size[], int
banks)
Quoted 4 lines
common/fdt_support.c static int fdt_pack_reg(const void *fdt, void *buf, u64 *address, u64 *size,
Quoted 4 lines
common/fdt_support.c +0 -1
 {
        int err, nodeoffset;
        int len, i;
-       u8 tmp[MEMORY_BANKS_MAX * 16]; /* Up to 64-bit address +
64-bit size */
Quoted 15 lines
Quoted 15 lines
common/fdt_support.c +2 -6
+       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
Quoted 4 lines
Quoted 4 lines
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
https://lists.denx.de/listinfo/u-boot
--
Tom

Tom Rini

2018-08-13 16:08 UTC #4 lore raw

PATCH 1/1 fdt_support: Use VLA instead of MEMORY_BANKS_MAX

Quoted 24 lines · Ramon Fried

Quoted 10 lines · Tom Rini
Quoted 1 line · Peter Robinson
On Sun, Aug 12, 2018 at 9:37 PM, Ramon Fried <[email protected]>
Quoted 6 lines · wrote:
Quoted 5 lines
From: Ramon Fried <[email protected]>
Instead of relaying on user to configure MEMORY_BANKS_MAX correctly, use VLA (variable length array) to accommodate the required banks.
Quoted 8 lines
Quoted 2 lines
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.

Quoted 5 lines

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!

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180813/9099d8b3/attachment.sig>

Ramon Fried

author

2018-08-13 16:14 UTC #5 lore raw

PATCH 1/1 fdt_support: Use VLA instead of MEMORY_BANKS_MAX

Quoted 28 lines · Tom Rini

Quoted 5 lines · Ramon Fried
Quoted 3 lines · Tom Rini
Quoted 1 line · Peter Robinson
On Sun, Aug 12, 2018 at 9:37 PM, Ramon Fried
<[email protected]>
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines · wrote:
Quoted 5 lines
From: Ramon Fried <[email protected]>
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
Quoted 6 lines
Quoted 6 lines
Quoted 1 line
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
Quoted 5 lines
Quoted 1 line
The whole purpose of my patch was to enable to fixup more banks than defined in
CONFIG_NR_DRAM_BANKS.

Quoted 12 lines

Quoted 5 lines
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?

Signature · 1 line

Sent from my Android device with K-9 Mail. Please excuse my brevity.

Tom Rini

2018-08-13 16:15 UTC #6 lore raw

PATCH 1/1 fdt_support: Use VLA instead of MEMORY_BANKS_MAX

Quoted 41 lines · Ramon Fried

Quoted 28 lines · Tom Rini
Quoted 5 lines · Ramon Fried
Quoted 3 lines · Tom Rini
Quoted 1 line · Peter Robinson
On Sun, Aug 12, 2018 at 9:37 PM, Ramon Fried
<[email protected]>
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines · wrote:
Quoted 5 lines
From: Ramon Fried <[email protected]>
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
Quoted 6 lines
Quoted 6 lines
Quoted 1 line
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
Quoted 5 lines
Quoted 1 line
The whole purpose of my patch was to enable to fixup more banks than defined in
CONFIG_NR_DRAM_BANKS.
Quoted 12 lines
Quoted 5 lines
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!

Quoted 2 lines

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?

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180813/3f121c67/attachment.sig>

Ramon Fried

author

2018-08-13 16:22 UTC #7 lore raw

PATCH 1/1 fdt_support: Use VLA instead of MEMORY_BANKS_MAX

Quoted 48 lines · Tom Rini

Quoted 1 line · Ramon Fried
On August 13, 2018 7:08:22 PM GMT+03:00, Tom Rini
Quoted 2 lines · <[email protected]> wrote:
Quoted 2 lines · Ramon Fried
Quoted 1 line
On Mon, Aug 13, 2018 at 5:52 PM Tom Rini <[email protected]>
Quoted 10 lines · wrote:
Quoted 10 lines
Quoted 4 lines
Quoted 4 lines · Peter Robinson
Quoted 2 lines
On Sun, Aug 12, 2018 at 9:37 PM, Ramon Fried
<[email protected]>
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines · wrote:
Quoted 4 lines
From: Ramon Fried <[email protected]>
Instead of relaying on user to configure MEMORY_BANKS_MAX correctly, use VLA (variable length array) to accommodate
the
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
required banks.
With the kernel actively removing VLAs [1] does it make sense
for
Quoted 6 lines
Quoted 6 lines
us
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
to use them?
Agreed.
Also, why is the answer NOT to go back to the way things were
with
Quoted 4 lines
Quoted 4 lines
Quoted 1 line
Quoted 1 line
5e5745465c94 and increase CONFIG_NR_DRAM_BANKS when needed? It
seems
Quoted 2 lines
Quoted 1 line
The whole purpose of my patch was to enable to fixup more banks
than
Quoted 12 lines
Quoted 12 lines
Quoted 8 lines
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.
Quoted 1 line
Quoted 1 line
This may also mean that certain platforms need to bump their default
up

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
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.

Signature · 1 line

Sent from my Android device with K-9 Mail. Please excuse my brevity.

Ramon Fried

author

2018-08-13 19:55 UTC #8 lore raw

PATCH 1/1 fdt_support: Use VLA instead of MEMORY_BANKS_MAX

Quoted 55 lines · Ramon Fried

Quoted 53 lines · Tom Rini
Quoted 1 line · Ramon Fried
On August 13, 2018 7:08:22 PM GMT+03:00, Tom Rini
Quoted 2 lines · <[email protected]> wrote:
Quoted 2 lines · Ramon Fried
Quoted 1 line
On Mon, Aug 13, 2018 at 5:52 PM Tom Rini <[email protected]>
Quoted 10 lines · wrote:
Quoted 10 lines
Quoted 4 lines
Quoted 4 lines · Peter Robinson
Quoted 2 lines
On Sun, Aug 12, 2018 at 9:37 PM, Ramon Fried
<[email protected]>
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines · wrote:
Quoted 4 lines
From: Ramon Fried <[email protected]>
Instead of relaying on user to configure MEMORY_BANKS_MAX correctly, use VLA (variable length array) to accommodate
the
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
required banks.
With the kernel actively removing VLAs [1] does it make sense
for
Quoted 6 lines
Quoted 6 lines
us
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
to use them?
Agreed.
Also, why is the answer NOT to go back to the way things were
with
Quoted 4 lines
Quoted 4 lines
Quoted 1 line
Quoted 1 line
5e5745465c94 and increase CONFIG_NR_DRAM_BANKS when needed? It
seems
Quoted 2 lines
Quoted 1 line
The whole purpose of my patch was to enable to fixup more banks
than
Quoted 12 lines
Quoted 12 lines
Quoted 8 lines
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.
Quoted 1 line
Quoted 1 line
This may also mean that certain platforms need to bump their default
up
Quoted 4 lines
Quoted 2 lines
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?

Quoted 7 lines

Quoted 3 lines
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

Quoted 2 lines

Sent from my Android device with K-9 Mail. Please excuse my brevity.

Tom Rini

2018-08-13 16:59 UTC #9 lore raw

PATCH 1/1 fdt_support: Use VLA instead of MEMORY_BANKS_MAX

Quoted 63 lines · Ramon Fried

Quoted 61 lines · Ramon Fried
Quoted 48 lines · Tom Rini
Quoted 1 line · Ramon Fried
On August 13, 2018 7:08:22 PM GMT+03:00, Tom Rini
Quoted 2 lines · <[email protected]> wrote:
Quoted 2 lines · Ramon Fried
Quoted 1 line
On Mon, Aug 13, 2018 at 5:52 PM Tom Rini <[email protected]>
Quoted 10 lines · wrote:
Quoted 10 lines
Quoted 4 lines
Quoted 4 lines · Peter Robinson
Quoted 2 lines
On Sun, Aug 12, 2018 at 9:37 PM, Ramon Fried
<[email protected]>
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines · wrote:
Quoted 4 lines
From: Ramon Fried <[email protected]>
Instead of relaying on user to configure MEMORY_BANKS_MAX correctly, use VLA (variable length array) to accommodate
the
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
required banks.
With the kernel actively removing VLAs [1] does it make sense
for
Quoted 6 lines
Quoted 6 lines
us
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
to use them?
Agreed.
Also, why is the answer NOT to go back to the way things were
with
Quoted 4 lines
Quoted 4 lines
Quoted 1 line
Quoted 1 line
5e5745465c94 and increase CONFIG_NR_DRAM_BANKS when needed? It
seems
Quoted 2 lines
Quoted 1 line
The whole purpose of my patch was to enable to fixup more banks
than
Quoted 12 lines
Quoted 12 lines
Quoted 8 lines
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.
Quoted 1 line
Quoted 1 line
This may also mean that certain platforms need to bump their default
up
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
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.

Quoted 6 lines

Quoted 1 line
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!

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180813/0400d4d1/attachment.sig>

Ramon Fried

author

2018-08-13 17:19 UTC #10 lore raw

PATCH 1/1 fdt_support: Use VLA instead of MEMORY_BANKS_MAX

Quoted 86 lines · Tom Rini

Quoted 1 line · Ramon Fried
On Mon, Aug 13, 2018 at 7:22 PM Ramon Fried <[email protected]>
Quoted 2 lines · wrote:
Quoted 2 lines
On August 13, 2018 7:15:14 PM GMT+03:00, Tom Rini
<[email protected]>
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines · wrote:
Quoted 1 line · Ramon Fried
On August 13, 2018 7:08:22 PM GMT+03:00, Tom Rini
Quoted 2 lines · <[email protected]> wrote:
Quoted 2 lines · Ramon Fried
Quoted 1 line
On Mon, Aug 13, 2018 at 5:52 PM Tom Rini <[email protected]>
Quoted 2 lines · wrote:
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
On Mon, Aug 13, 2018 at 08:20:03AM +0100, Peter Robinson
Quoted 7 lines · wrote:
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
On Sun, Aug 12, 2018 at 9:37 PM, Ramon Fried
<[email protected]>
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines · wrote:
Quoted 3 lines
From: Ramon Fried <[email protected]>
Instead of relaying on user to configure
MEMORY_BANKS_MAX
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
correctly, use VLA (variable length array) to
accommodate
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
the
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
required banks.
With the kernel actively removing VLAs [1] does it make
sense
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
for
Quoted 6 lines
Quoted 6 lines
us
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
to use them?
Agreed.
Also, why is the answer NOT to go back to the way things
were
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
with
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
5e5745465c94 and increase CONFIG_NR_DRAM_BANKS when needed?
It
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
seems
Quoted 2 lines
Quoted 1 line
The whole purpose of my patch was to enable to fixup more
banks
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
than
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
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.
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
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
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
be.
Quoted 1 line
Quoted 1 line
This may also mean that certain platforms need to bump their
default
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
up
Quoted 1 line
Quoted 1 line
in order to support the hardware you're using that shows this
issue.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 1 line
Thanks!
I'm confused. To which hardware you're referring to? Do you
still
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
think we should revert my patch?
Yes, I think we should bring the code back to the way it was for a
long
Quoted 11 lines
Quoted 6 lines
Quoted 2 lines
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

Quoted 6 lines

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

Signature · 1 line

Sent from my Android device with K-9 Mail. Please excuse my brevity.