[U-Boot] [PATCH] fdt_support: Use CONFIG_NR_DRAM_BANKS if defined
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(+)
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;
-----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
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
-----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
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-----
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".
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).
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
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.
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
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?
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
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!
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
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!
participants (4)
-
Doug Anderson -
Jerry Van Baren -
Tom Rini -
Vadim Bendebury