From mboxrd@z Thu Jan 1 00:00:00 1970 From: Albert ARIBAUD Date: Fri, 03 Feb 2012 23:06:31 +0100 Subject: [U-Boot] [PATCH v3 0/6] Introduce generic relocation feature In-Reply-To: References: <1324923878-8176-1-git-send-email-sjg@chromium.org> <4F1674AF.9050000@aribaud.net> Message-ID: <4F2C5A67.8040807@aribaud.net> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Simon, Le 18/01/2012 20:31, Simon Glass a ?crit : > [+TI maintainers, tx25 board maintainer] > > Hi Albert, >>> For ARM, a new arch/arm/lib/proc.S file is created, which holds generic >>> ARM assembler code (things that cannot be written in C and are common >>> functions used by all ARM CPUs). This helps reduce duplication. Interrupt >>> handling code and perhaps even some startup code can move there later. >>> >>> It may be useful for other architectures with a lot of different CPUs >>> to have a similar file. >> >> NAK for several reasons: > > I think you are NAKing the 'arm: Add processor function library' > patch out of the series: Yes. Wasn't that clear? > Create reloc.h and include it where needed > define CONFIG_SYS_SKIP_RELOC for all archs > Add generic relocation feature > arm: Add processor function library > arm: Move over to generic relocation > arm: Remove unused code in start.S > > Q1: Should I remove that patch and just repeat the code from there in > each start.S file? You should keep the code that jumps to board_init_r as it is. > The impact would be to remove most of the code in > the relocate_code() function in each start.S except for the last few > instructions (reproduced below). Yes. >> 1. The code that goes on proc.S is already in start.S, which already >> contains "things which cannot be written in C and are common functions used >> by all ARM CPUs". Granted, right now start.S is duplicated in multiple CPU >> directories; so the thing to do is to merge all ARM start.S files into a >> single one, rather than merging only one of its parts. > > Q2: What should this merged file be called and what directory should > it be in? This is the question I asked last time, and the lack of an > answer is the reason why I have been unable to make progress here. > Please advise your preference and I will sort it out. Sorry if I missed this question. Obviously the file will be called start.S exactly like all its merge ancestors, and it should reside in arch/arm/cpu. Note that I already said the merging of start.S should *not* be part of this patch set, and should be a patch set of its own. > Note I don't think I can do this in one series - there is far too much > variation between the files for me to take on that way. I need a new > file than I can bit I bit move things over into, allowing affected > parties to comment on each as I go. Note that I do not require that *you* do such a merge either, though help is always welcome of course. >> 2. There is no interest in moving this segment of code from all start.S >> files into a new proc.S file: there is no gain is code size obviously, and >> there is an increase in source file count. > > Just checking that you see that the code is removed from start.S, not > moved. The code in proc.S is: > > .globl proc_call_board_init_r > proc_call_board_init_r: > #ifndef CONFIG_SYS_ICACHE_OFF > mcr p15, 0, r0, c7, c5, 0 @ invalidate icache > mcr p15, 0, r0, c7, c10, 4 @ DSB > mcr p15, 0, r0, c7, c5, 4 @ ISB > #endif > mov sp, r3 > /* jump to it ... */ > mov pc, r2 > > which is taken from the end of each relocate_code() copy. Assuming we > are on the page, then ok. We are -- the code removed from start.S is relocation, which I quite agree with since you replace it with the C relocation code. >> 3. I consider that files should contain 'things' based on their common >> /functionality/, not on their similar /nature/, and "going about starting up >> U-Boot" is a functionality. > > The code here sets the stack pointer and calls a function. However I > agree this unlikely to be useful outside starting up. See Q2. Ditto. >> Note that eventually, having a single start.S will achieve the same effect >> as this 'proc.S' patch. > > At present start.S runs for a bit and then jumps to board_init_f(). At > the end of board_init_f() we jump back to start.S (relocate_code() > function) and from there to board_init_r(). Also start.S has exception > handling code. > > I think start.S should be thought of as in three parts: > - early CPU init (hardest to make common) > - relocation code (last 3 patches of this series) > - exception code > > The first one clearly belongs in start.S. I don't think the other two > do. They are not related to CPU start-up Exception code is indeed related to startup code in that they are vectored through the same ARM-defined exception vectors table. > The relocation code is only > there because of the need to call a function with a new stack pointer. No -- the relocation code is there *and* we need a switch of stack pointer, but relocation and stack switching are functionally unrelated. > Other than that it can be written in C. Apart from reset the exception > code isn't related to starting up the CPU - it is only there because > the it is a convenient assembler file (your reasoning suggests that > you would in fact want this in a new except.S file). I disagree. > So maybe we want start.S, reloc.S and except.S? No. I just agree that relocation can and should go out of start.S. The rest -- exception vectors table, startup until board_init_f(), pivot to board_init_r() and exception handlers -- stays in start.S >> Note also that this start.S commonalization should be a completely different >> patch set. > > Are you saying you want a later series which moves all the start.S > relocate_code() code from each cpu dir into a single place? If so, > please see Q2. I would like such a patch series, but I don't ask anyone for it. If I have time I might submit it myself. >>> Code size on my ARMv7 system increases by 54 bytes with generic >>> relocation. This overhead is mostly just literal pool access and setting >>> up to call the relocated U-Boot at the end. >>> >>> On my system, execution time increases from 10.8ms to 15.6ms due to the >>> less efficient C implementations of the copy and zero loops. If execution >>> time is of concern, you can define CONFIG_USE_ARCH_MEMSET and >>> CONFIG_USE_ARCH_MEMCPY to reduce it. For met this reduces relocation time >>> to 5.4ms, i.e. twice as fast as the old system. >> >> Side question: is there any reason not to define these CONFIG options >> systematically? > > Code size. What is the code size increase of using arch-specific mem ops? >>> One problem remains which causes mx31pdk to fail to build. It doesn't >>> have string.c in its SPL code and the architecture-specific versions of >>> memset()/memcpy() are too large. I propose to add a local change to >>> reloc.c that uses inline code for boards that use the old legacy SPL >>> framework. We can remove it later. This is not included in v2 but I am >>> interested in comments on this approach. An alternative would be just >>> to add simple memset()/memcpy() functions just for this board (and one >>> other affected MX31 board). >> >> I am not too fond of the "later removal" approach. In my experience, this >> invariably tends to the "old bloat in the code no one knows the reason of" >> situations. > > Me neither. The only board affected is the tx25. We could let the > maintainer fix it up, I suppose. I have no way of testing it. John: ping? > Regards, > Simon Amicalement, -- Albert.