Patchwork [U-Boot] fdt_support: Use CONFIG_NR_DRAM_BANKS if defined

login
register
mail settings
Submitter Doug Anderson
Date April 30, 2013, 8:22 p.m.
Message ID <1367353321-21204-1-git-send-email-dianders@chromium.org>
Download mbox | patch
Permalink /patch/240687/
State Accepted
Delegated to: Tom Rini
Headers show

Comments

Doug Anderson - April 30, 2013, 8:22 p.m.
It appears that there are some cases where we have more than 4 banks
of memory.  Use CONFIG_NR_DRAM_BANKS if it's defined to handle this.
This will take up a little extra stack space (64 bytes extra if we go
up to 8 banks), but that seems OK.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
Note: nothing in-tree has 8 banks defined yet, but some configs have
it defined that are not in tree yet.

 common/fdt_support.c | 4 ++++
 1 file changed, 4 insertions(+)
Tom Rini - April 30, 2013, 8:35 p.m.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/30/2013 04:22 PM, Doug Anderson wrote:
> It appears that there are some cases where we have more than 4 
> banks of memory.  Use CONFIG_NR_DRAM_BANKS if it's defined to 
> handle this. This will take up a little extra stack space (64 bytes
> extra if we go up to 8 banks), but that seems OK.
> 
> Signed-off-by: Doug Anderson <dianders@chromium.org> --- Note: 
> nothing in-tree has 8 banks defined yet, but some configs have it 
> defined that are not in tree yet.

And I guess having this knowledge correct for the kernel is useful in
other contexts like when we want to power down some banks of memory
but not others?  I mean, there's "lots" of platforms that lie and say
1 bank since we require contiguous mapping.  Thanks!

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJRgCsOAAoJENk4IS6UOR1WXAMP/iE4gC9vI/R1dwchMrfkwVQC
hEM2WqO1/ABmP8u85e8+wT6omTw0BIsrGjoyDQlZuNYrCsh+Gem34yvUPArSIGQN
C+oVnNSBCa5R5Iaa86ZjT8vo5U7gSxdN2CPUcuyKCop9CC8RZCHNv52o8SnRZALQ
Fmd/pdKftOrw6McetNM+sb6UJKxEa+ZBLi5UeWZ1gMS33E0EkhOCx89yDj5auwxp
toSfwTivuqZ9MEiDGOVTlx3LGLk3RZBQFFzBC8aWeWGLw7oyjMO09yp9YDi5rXZ9
9yqOQCUIkEvmv8ZrtMhOQGZOihMvNBt8q4aBF611PSUyDrRb93qZ53RIxZkiI/5F
n+0HKreuIBa3UNTr2UVyrUl0mfc3JovTsUvTrw6TmLJllsXVsJg57Drzbcw1WSV4
MG7wDyPwmZ6jknAkS8BlpHkchFlLXW78r8oTrPeg2i4AF01/cTefp0RUG0IXpZ8N
4kiMm72Su/v64zOpKNqZQ6PINDBAaBWrCVFdFckErHQTWyp+x8XRhe5YpQuFDxdY
EYtfz4bimXZhMIYf/NVsZUQ7NiuUxJ+3Eg0SWB291ITk/RB7LGU57Gt3a0Qz/0Hj
frikRuhR6Go83qaB88UeEUz4TUqnz+m8SB1+Fzb9zTK1FPQaBGc55hD65Nxr5nqd
dKpkxnd5sD6suhBS+axy
=DSap
-----END PGP SIGNATURE-----
Doug Anderson - April 30, 2013, 8:49 p.m.
Tom,

On Tue, Apr 30, 2013 at 1:35 PM, Tom Rini <trini@ti.com> wrote:
> And I guess having this knowledge correct for the kernel is useful in
> other contexts like when we want to power down some banks of memory
> but not others?  I mean, there's "lots" of platforms that lie and say
> 1 bank since we require contiguous mapping.  Thanks!

Thanks for the review!

At the moment I'm _not_ convinced that there's a good reason to
specify 8 banks.  We appear to have lied and said 1 bank on
exynos5250-snow (ARM Chromebook) and I don't know of any bad side
effects.

The code I'm looking at right now indicates 8 banks.  We need to track
down why someone did that but it doesn't seem totally crazy to allow
specifying the proper number of banks so I figured I'd send this patch
up.

If you prefer, we can leave this patch hanging until we actually track
down if specifying 8 banks was really needed.

-Doug
Tom Rini - April 30, 2013, 9:14 p.m.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/30/2013 04:49 PM, Doug Anderson wrote:
> Tom,
> 
> On Tue, Apr 30, 2013 at 1:35 PM, Tom Rini <trini@ti.com> wrote:
>> And I guess having this knowledge correct for the kernel is
>> useful in other contexts like when we want to power down some
>> banks of memory but not others?  I mean, there's "lots" of
>> platforms that lie and say 1 bank since we require contiguous
>> mapping.  Thanks!
> 
> Thanks for the review!
> 
> At the moment I'm _not_ convinced that there's a good reason to 
> specify 8 banks.  We appear to have lied and said 1 bank on 
> exynos5250-snow (ARM Chromebook) and I don't know of any bad side 
> effects.
> 
> The code I'm looking at right now indicates 8 banks.  We need to
> track down why someone did that but it doesn't seem totally crazy
> to allow specifying the proper number of banks so I figured I'd
> send this patch up.
> 
> If you prefer, we can leave this patch hanging until we actually
> track down if specifying 8 banks was really needed.

Yes please, lets hold.  Thanks!

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJRgDQoAAoJENk4IS6UOR1WztkP+QEs7IvExh9Dq0AHrj81wQ9Q
Ml29BZGsdJ5mLIt6jhJ7HSr310cu3FODgbVuNt01Aj0Q2X+C1mCRYqhoIDwfcSUJ
EWVhUaphlmiBd2OrMH+3HPUwQ+kFfjt5LNFuXwRei0tgz+sy6NTQ+QZFuZ9FiBJD
UKtavOsvd3XipdklU5UEGoBj6OJxU6hBOyehZ3Cckwgfeg0L/1uV07Vd8kSFFc5e
xoWXN7O+QkdlNkWeruxPF7uq1MeM2VusCuvGWK4srrED+WSAKFhqsi7t3N66iNny
lXDhYPtuSr5HF5xua4kwWdbM/GneVd5m0p979TvIwvwhM1bMr00mfIoH9HEjzNF6
Bvq0wcCwIEZLwBFNNpn9X9zIzwXIgUKbMqjHQXiuizY8LROdXXnkg53k9o2pDO5+
uGO8cKZMXJYEU4zW+wbSlI/Cz7WoylsXhSBPfF5gkRSIxKtYmcS/iQn/nKMgebVO
TaGx76/r8xOvA5WY+wCs7HMEJip5UU00rG7MvjokwxOSUf/2rVHiDWl0MEAlh7M4
4KAMzb61P/fUiXrZv5K9Z6sgPmGynjItKnw0UigTWKG6DvRy0HuOlF//O8qAuWKH
+eyjg2F24pS9cGRMni3M9cUBH1W6secIpZkqs3goxeNVZyfb29kswolymfbcU4GC
zXmnz8gBTLDKGtTzLlXC
=s42z
-----END PGP SIGNATURE-----
Vadim Bendebury - May 15, 2013, 3:58 p.m.
On Tue, Apr 30, 2013 at 2:14 PM, Tom Rini <trini@ti.com> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 04/30/2013 04:49 PM, Doug Anderson wrote:
>> Tom,
>>
>> On Tue, Apr 30, 2013 at 1:35 PM, Tom Rini <trini@ti.com> wrote:
>>> And I guess having this knowledge correct for the kernel is
>>> useful in other contexts like when we want to power down some
>>> banks of memory but not others?  I mean, there's "lots" of
>>> platforms that lie and say 1 bank since we require contiguous
>>> mapping.  Thanks!
>>
>> Thanks for the review!
>>
>> At the moment I'm _not_ convinced that there's a good reason to
>> specify 8 banks.  We appear to have lied and said 1 bank on
>> exynos5250-snow (ARM Chromebook) and I don't know of any bad side
>> effects.
>>
>> The code I'm looking at right now indicates 8 banks.  We need to
>> track down why someone did that but it doesn't seem totally crazy
>> to allow specifying the proper number of banks so I figured I'd
>> send this patch up.
>>
>> If you prefer, we can leave this patch hanging until we actually
>> track down if specifying 8 banks was really needed.
>
> Yes please, lets hold.  Thanks!
>

I looked into this a bit more, what happens on this particular target
(Exynos5420 with 4GB DRAM onboard) is that out of 4GB of memory only
3.5GB is usable, as the lower .5 GB of address range is taken by the
architecture, and the address bus width is 32 bits.

U-boot code makes several assumptions:
 - bank size is a power of 2
 - bank base is aligned with bank size
 - all bank sizes are the same

with this in mind, the only way to describe our memory situation is to
define 7 banks, .5GB each, the lowest one starting at 0x20000000
(.5GB).

This is not a big deal for u-boot (maybe very marginally inefficient
when determining the actual memory size). Is this a big deal for
kernel? I mean it is easy to squash these seven memory banks into one
when filling out the memory node of the device tree, the question is
is it even necessary?

cheers --vb

> - --
> Tom
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.11 (GNU/Linux)
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
>
> iQIcBAEBAgAGBQJRgDQoAAoJENk4IS6UOR1WztkP+QEs7IvExh9Dq0AHrj81wQ9Q
> Ml29BZGsdJ5mLIt6jhJ7HSr310cu3FODgbVuNt01Aj0Q2X+C1mCRYqhoIDwfcSUJ
> EWVhUaphlmiBd2OrMH+3HPUwQ+kFfjt5LNFuXwRei0tgz+sy6NTQ+QZFuZ9FiBJD
> UKtavOsvd3XipdklU5UEGoBj6OJxU6hBOyehZ3Cckwgfeg0L/1uV07Vd8kSFFc5e
> xoWXN7O+QkdlNkWeruxPF7uq1MeM2VusCuvGWK4srrED+WSAKFhqsi7t3N66iNny
> lXDhYPtuSr5HF5xua4kwWdbM/GneVd5m0p979TvIwvwhM1bMr00mfIoH9HEjzNF6
> Bvq0wcCwIEZLwBFNNpn9X9zIzwXIgUKbMqjHQXiuizY8LROdXXnkg53k9o2pDO5+
> uGO8cKZMXJYEU4zW+wbSlI/Cz7WoylsXhSBPfF5gkRSIxKtYmcS/iQn/nKMgebVO
> TaGx76/r8xOvA5WY+wCs7HMEJip5UU00rG7MvjokwxOSUf/2rVHiDWl0MEAlh7M4
> 4KAMzb61P/fUiXrZv5K9Z6sgPmGynjItKnw0UigTWKG6DvRy0HuOlF//O8qAuWKH
> +eyjg2F24pS9cGRMni3M9cUBH1W6secIpZkqs3goxeNVZyfb29kswolymfbcU4GC
> zXmnz8gBTLDKGtTzLlXC
> =s42z
> -----END PGP SIGNATURE-----
Tom Rini - May 15, 2013, 4:46 p.m.
On Wed, May 15, 2013 at 08:58:03AM -0700, Vadim Bendebury wrote:

> On Tue, Apr 30, 2013 at 2:14 PM, Tom Rini <trini@ti.com> wrote:
> > -----BEGIN PGP SIGNED MESSAGE-----
> > Hash: SHA1
> >
> > On 04/30/2013 04:49 PM, Doug Anderson wrote:
> >> Tom,
> >>
> >> On Tue, Apr 30, 2013 at 1:35 PM, Tom Rini <trini@ti.com> wrote:
> >>> And I guess having this knowledge correct for the kernel is
> >>> useful in other contexts like when we want to power down some
> >>> banks of memory but not others?  I mean, there's "lots" of
> >>> platforms that lie and say 1 bank since we require contiguous
> >>> mapping.  Thanks!
> >>
> >> Thanks for the review!
> >>
> >> At the moment I'm _not_ convinced that there's a good reason to
> >> specify 8 banks.  We appear to have lied and said 1 bank on
> >> exynos5250-snow (ARM Chromebook) and I don't know of any bad side
> >> effects.
> >>
> >> The code I'm looking at right now indicates 8 banks.  We need to
> >> track down why someone did that but it doesn't seem totally crazy
> >> to allow specifying the proper number of banks so I figured I'd
> >> send this patch up.
> >>
> >> If you prefer, we can leave this patch hanging until we actually
> >> track down if specifying 8 banks was really needed.
> >
> > Yes please, lets hold.  Thanks!
> >
> 
> I looked into this a bit more, what happens on this particular target
> (Exynos5420 with 4GB DRAM onboard) is that out of 4GB of memory only
> 3.5GB is usable, as the lower .5 GB of address range is taken by the
> architecture, and the address bus width is 32 bits.
> 
> U-boot code makes several assumptions:
>  - bank size is a power of 2
>  - bank base is aligned with bank size
>  - all bank sizes are the same
> 
> with this in mind, the only way to describe our memory situation is to
> define 7 banks, .5GB each, the lowest one starting at 0x20000000
> (.5GB).
> 
> This is not a big deal for u-boot (maybe very marginally inefficient
> when determining the actual memory size). Is this a big deal for
> kernel? I mean it is easy to squash these seven memory banks into one
> when filling out the memory node of the device tree, the question is
> is it even necessary?

OK, this would be the second case of needing to describe the memory in
the DT in a way that conflicts with how we dynamically do the node.
Lets go and try again at a patch that lets boards opt-in to "do not
override the memory property, it was already correct and you broke it".
Doug Anderson - May 15, 2013, 4:51 p.m.
Vadim,

On Wed, May 15, 2013 at 8:58 AM, Vadim Bendebury <vbendeb@chromium.org> wrote:
> This is not a big deal for u-boot (maybe very marginally inefficient
> when determining the actual memory size). Is this a big deal for
> kernel? I mean it is easy to squash these seven memory banks into one
> when filling out the memory node of the device tree, the question is
> is it even necessary?

I think the kernel can go either way.  It can handle 1 big bank or 7
banks.  The parts that were broken in the past were:
* U-boot would refuse to tell the kernel about more than 4 banks
(that's what my patch fixed).
* The kernel choked if it was told about a bogus 8th bank that started
at 0 and was 0 bytes big.

What about if we just take my patch to support more than 4 banks
(Vadim now has good justification for needing it)?  ...and then we'll
fix our U-Boot not to tell the kernel about a bogus 8th bank (that was
just a bug in our config file).
Doug Anderson - May 17, 2013, 4:26 p.m.
Tom,

On Wed, May 15, 2013 at 9:51 AM, Doug Anderson <dianders@chromium.org> wrote:
> Vadim,
>
> On Wed, May 15, 2013 at 8:58 AM, Vadim Bendebury <vbendeb@chromium.org> wrote:
>> This is not a big deal for u-boot (maybe very marginally inefficient
>> when determining the actual memory size). Is this a big deal for
>> kernel? I mean it is easy to squash these seven memory banks into one
>> when filling out the memory node of the device tree, the question is
>> is it even necessary?
>
> I think the kernel can go either way.  It can handle 1 big bank or 7
> banks.  The parts that were broken in the past were:
> * U-boot would refuse to tell the kernel about more than 4 banks
> (that's what my patch fixed).
> * The kernel choked if it was told about a bogus 8th bank that started
> at 0 and was 0 bytes big.
>
> What about if we just take my patch to support more than 4 banks
> (Vadim now has good justification for needing it)?  ...and then we'll
> fix our U-Boot not to tell the kernel about a bogus 8th bank (that was
> just a bug in our config file).

Do you think it would be OK to apply my patch now given Vadim's
justification of why we need 7 banks in U-Boot.  AKA: we need 7 banks
so banks are a power of 2 and all the same size (which U-Boot
assumes).

...or would you prefer not to have it and come up with some other solution?

Thanks!

-Doug
Tom Rini - May 17, 2013, 4:40 p.m.
On Fri, May 17, 2013 at 09:26:19AM -0700, Doug Anderson wrote:

> Tom,
> 
> On Wed, May 15, 2013 at 9:51 AM, Doug Anderson <dianders@chromium.org> wrote:
> > Vadim,
> >
> > On Wed, May 15, 2013 at 8:58 AM, Vadim Bendebury <vbendeb@chromium.org> wrote:
> >> This is not a big deal for u-boot (maybe very marginally inefficient
> >> when determining the actual memory size). Is this a big deal for
> >> kernel? I mean it is easy to squash these seven memory banks into one
> >> when filling out the memory node of the device tree, the question is
> >> is it even necessary?
> >
> > I think the kernel can go either way.  It can handle 1 big bank or 7
> > banks.  The parts that were broken in the past were:
> > * U-boot would refuse to tell the kernel about more than 4 banks
> > (that's what my patch fixed).
> > * The kernel choked if it was told about a bogus 8th bank that started
> > at 0 and was 0 bytes big.
> >
> > What about if we just take my patch to support more than 4 banks
> > (Vadim now has good justification for needing it)?  ...and then we'll
> > fix our U-Boot not to tell the kernel about a bogus 8th bank (that was
> > just a bug in our config file).
> 
> Do you think it would be OK to apply my patch now given Vadim's
> justification of why we need 7 banks in U-Boot.  AKA: we need 7 banks
> so banks are a power of 2 and all the same size (which U-Boot
> assumes).
> 
> ...or would you prefer not to have it and come up with some other solution?

I think my email must have been lost in the shuffle, see
http://patchwork.ozlabs.org/patch/240687/

So yes, I've got another fix in mind that should solve this and some
other problems.
Doug Anderson - May 17, 2013, 4:48 p.m.
Tom,

On Fri, May 17, 2013 at 9:40 AM, Tom Rini <trini@ti.com> wrote:
> I think my email must have been lost in the shuffle, see
> http://patchwork.ozlabs.org/patch/240687/
>
> So yes, I've got another fix in mind that should solve this and some
> other problems.

Saw your reply but don't completely understand it.  I think we'd still
like U-Boot to populate the memory property if possible and it sounds
like your patch would prevent that.  One reason is that we'd like to
be able to handle different memory configurations (one of which is
3.5G) with a single binary and have U-Boot just tell the kernel how
much space there is.

-Doug
Tom Rini - May 17, 2013, 4:52 p.m.
On Fri, May 17, 2013 at 09:48:13AM -0700, Doug Anderson wrote:
> Tom,
> 
> On Fri, May 17, 2013 at 9:40 AM, Tom Rini <trini@ti.com> wrote:
> > I think my email must have been lost in the shuffle, see
> > http://patchwork.ozlabs.org/patch/240687/
> >
> > So yes, I've got another fix in mind that should solve this and some
> > other problems.
> 
> Saw your reply but don't completely understand it.  I think we'd still
> like U-Boot to populate the memory property if possible and it sounds
> like your patch would prevent that.  One reason is that we'd like to
> be able to handle different memory configurations (one of which is
> 3.5G) with a single binary and have U-Boot just tell the kernel how
> much space there is.

So, I've been assuming that these are different platforms that you
already append different device trees on, so that you use the same
binary still and just different DTs.  Is that not the case here?
Doug Anderson - May 17, 2013, 4:59 p.m.
Tom,

On Fri, May 17, 2013 at 9:52 AM, Tom Rini <trini@ti.com> wrote:
>> Saw your reply but don't completely understand it.  I think we'd still
>> like U-Boot to populate the memory property if possible and it sounds
>> like your patch would prevent that.  One reason is that we'd like to
>> be able to handle different memory configurations (one of which is
>> 3.5G) with a single binary and have U-Boot just tell the kernel how
>> much space there is.
>
> So, I've been assuming that these are different platforms that you
> already append different device trees on, so that you use the same
> binary still and just different DTs.  Is that not the case here?

Current thought is that we'll even share a device tree between the two
boards since differences between the two are very minimal.  Sorta like
how you can buy a Galaxy Nexus with 8G or 16G.  They're the same
device (as far as I know) just with a different eMMC part stuffed on.

-Doug
Tom Rini - May 17, 2013, 6:05 p.m.
On Fri, May 17, 2013 at 09:59:23AM -0700, Doug Anderson wrote:
> Tom,
> 
> On Fri, May 17, 2013 at 9:52 AM, Tom Rini <trini@ti.com> wrote:
> >> Saw your reply but don't completely understand it.  I think we'd still
> >> like U-Boot to populate the memory property if possible and it sounds
> >> like your patch would prevent that.  One reason is that we'd like to
> >> be able to handle different memory configurations (one of which is
> >> 3.5G) with a single binary and have U-Boot just tell the kernel how
> >> much space there is.
> >
> > So, I've been assuming that these are different platforms that you
> > already append different device trees on, so that you use the same
> > binary still and just different DTs.  Is that not the case here?
> 
> Current thought is that we'll even share a device tree between the two
> boards since differences between the two are very minimal.  Sorta like
> how you can buy a Galaxy Nexus with 8G or 16G.  They're the same
> device (as far as I know) just with a different eMMC part stuffed on.

OK.  I'll pick up this patch then, thanks!
Jerry Van Baren - May 17, 2013, 6:13 p.m.
On 05/17/2013 02:05 PM, Tom Rini wrote:
> On Fri, May 17, 2013 at 09:59:23AM -0700, Doug Anderson wrote:
>> Tom,
>>
>> On Fri, May 17, 2013 at 9:52 AM, Tom Rini <trini@ti.com> wrote:
>>>> Saw your reply but don't completely understand it.  I think we'd still
>>>> like U-Boot to populate the memory property if possible and it sounds
>>>> like your patch would prevent that.  One reason is that we'd like to
>>>> be able to handle different memory configurations (one of which is
>>>> 3.5G) with a single binary and have U-Boot just tell the kernel how
>>>> much space there is.
>>>
>>> So, I've been assuming that these are different platforms that you
>>> already append different device trees on, so that you use the same
>>> binary still and just different DTs.  Is that not the case here?
>>
>> Current thought is that we'll even share a device tree between the two
>> boards since differences between the two are very minimal.  Sorta like
>> how you can buy a Galaxy Nexus with 8G or 16G.  They're the same
>> device (as far as I know) just with a different eMMC part stuffed on.
>
> OK.  I'll pick up this patch then, thanks!

FWIIW...

Acked-by: Gerald Van Baren <vanbaren@cideas.com>

Thanks,
gvb
Tom Rini - May 22, 2013, 2:59 p.m.
On Tue, Apr 30, 2013 at 10:22:00AM -0000, Doug Anderson wrote:

> It appears that there are some cases where we have more than 4 banks
> of memory.  Use CONFIG_NR_DRAM_BANKS if it's defined to handle this.
> This will take up a little extra stack space (64 bytes extra if we go
> up to 8 banks), but that seems OK.
> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>

Applied to u-boot/master, thanks!

Patch

diff --git a/common/fdt_support.c b/common/fdt_support.c
index 812acb4..416100e 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -387,7 +387,11 @@  static void write_cell(u8 *addr, u64 val, int size)
 	}
 }
 
+#ifdef CONFIG_NR_DRAM_BANKS
+#define MEMORY_BANKS_MAX CONFIG_NR_DRAM_BANKS
+#else
 #define MEMORY_BANKS_MAX 4
+#endif
 int fdt_fixup_memory_banks(void *blob, u64 start[], u64 size[], int banks)
 {
 	int err, nodeoffset;