From mboxrd@z Thu Jan 1 00:00:00 1970 From: Graeme Russ Date: Sat, 03 Dec 2011 08:22:45 +1100 Subject: [U-Boot] [PATCH 3/4] x86: Force the lib_sysinfo structure to be in the .data section In-Reply-To: References: <1322633275-10542-1-git-send-email-gabeblack@chromium.org> <1322633275-10542-4-git-send-email-gabeblack@chromium.org> <4ED93EAC.7040904@gmail.com> Message-ID: <4ED941A5.7060603@gmail.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Gabe, On 03/12/11 08:16, Gabe Black wrote: > > > On Fri, Dec 2, 2011 at 1:10 PM, Graeme Russ > wrote: > > Hi Gabe, > > On 30/11/11 17:07, Gabe Black wrote: > > Otherwise it ends up in the .bss section. U-boot assumes that it doesn't > > need to copy it over during relocation, and instead fills that whole > > section with zeroes. If we really were booting from ROM that would be > > appropriate, but we need some information from the coreboot tables > (memory > > size) before then and have to fill that structure before relocation. We > > skirt u-boot's assumption by putting this in .data where it assumes there > > is still read only but non-zero data. > > > > Signed-off-by: Gabe Black > > > --- > > arch/x86/cpu/coreboot/sysinfo.c | 8 +++++++- > > 1 files changed, 7 insertions(+), 1 deletions(-) > > > > diff --git a/arch/x86/cpu/coreboot/sysinfo.c > b/arch/x86/cpu/coreboot/sysinfo.c > > index 464f8a1..e74fe0a 100644 > > --- a/arch/x86/cpu/coreboot/sysinfo.c > > +++ b/arch/x86/cpu/coreboot/sysinfo.c > > @@ -30,4 +30,10 @@ > > > > #include > > > > -struct sysinfo_t lib_sysinfo; > > +/* > > + * This needs to be in the .data section so that it's copied over during > > + * relocation. By default it's put in the .bss section which is > simply filled > > + * with zeroes when transitioning from "ROM", which is really RAM, > to other > > + * RAM. > > + */ > > +struct sysinfo_t lib_sysinfo __attribute__((section(".data"))); > > I think this can be logically folded into the first patch > > Regards, > > Graeme > > > I would rather not do that since the first patch is, modulo checkpatch and > build fixes, just importing that code from coreboot's libpayload. This > change is making an important, non-obvious change which makes it work > properly within u-boot. These are two different things, and one way or the > other this one is important enough to be singled out as its own change and > not lost in the midst of the other. I agree that is a logical position to take, however I think that the fact that you introduce a commit which is known to be 'broken' is the overriding factor - Include to fact that the modification is made in the commit message and the comment like: /* * Note: U-Boot needs this to be in the .data section so.... * ... */ Also, can you supply any specific commit id(s) for the originating source - We typically try to include as much canonical information to identify the source to a particular commit Regards, Graeme