[U-Boot] [PATCH] fdt_support: Use VLA instead of MEMORY_BANKS_MAX

[U-Boot] [PATCH] fdt_support: Use VLA instead of MEMORY_BANKS_MAX

[Ramon Fried]

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) {
-- 
2.7.4

[U-Boot] [PATCH] fdt_support: Use VLA instead of MEMORY_BANKS_MAX

[Peter Robinson]

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 at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH] fdt_support: Use VLA instead of MEMORY_BANKS_MAX

[Tom Rini]

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 at lists.denx.de
> > https://lists.denx.de/listinfo/u-boot

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

[U-Boot] [PATCH] fdt_support: Use VLA instead of MEMORY_BANKS_MAX

[Tom Rini]

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!

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

[U-Boot] [PATCH] fdt_support: Use VLA instead of MEMORY_BANKS_MAX

[Ramon Fried]

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? 

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

[U-Boot] [PATCH] fdt_support: Use VLA instead of MEMORY_BANKS_MAX

[Tom Rini]

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?

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

[U-Boot] [PATCH] fdt_support: Use VLA instead of MEMORY_BANKS_MAX

[Ramon Fried]

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
>> ><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. 

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

[U-Boot] [PATCH] fdt_support: Use VLA instead of MEMORY_BANKS_MAX

[Tom Rini]

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
> > >> ><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!

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

[U-Boot] [PATCH] fdt_support: Use VLA instead of MEMORY_BANKS_MAX

[Ramon Fried]

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

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

[U-Boot] [PATCH] fdt_support: Use VLA instead of MEMORY_BANKS_MAX

[Ramon Fried]

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 at lists.denx.de
> > > https://lists.denx.de/listinfo/u-boot
>
> --
> Tom
>

[U-Boot] [PATCH] fdt_support: Use VLA instead of MEMORY_BANKS_MAX

[Ramon Fried]

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

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