| Message ID | 1367353321-21204-1-git-send-email-dianders@chromium.org |
|---|---|
| State | Accepted |
| Delegated to: | Tom Rini |
| Headers | show |
-----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-----
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 -----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 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!
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;
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(+)