Hi Albert, On Fri, Feb 3, 2012 at 2:06 PM, Albert ARIBAUD <albert.u.b...@aribaud.net> wrote: > 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? >
It would be better if you NAKed the specific patch, but anyway, I understood what you meant. > >> 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. I have had a look at this and I don't believe that I can. I need to call it from C and so it needs to conform to the C calling standard. I will send a new series showing what I mean. > > >> 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. Yes I don't propose to do this now. I believe that the first 100 instructions may well need to have differences between the CPUs. I believe we need a common file but that each CPU will need the ability to override this, using the common file as needed. Are you saying that you want a start.S in arch/arm/cpu and each arch/arch/cpu/*/ ? I would prefer there was only one file named start.S. > > >> 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. > I am happy to take these sorts of things on over time, but really I want to get the existing efforts in first. > >>> 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. Yes, and I would love to change the function signature of board_init_r() to remove the parameters, and add a generic 'change sp and jump' function to each architecture. Not planning to do so though. For now, this code needs to be specific to this task - only able to call board_init_r() with the parameters it needs and in the right order. > > >> 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 We can't agree on everything...anyway that discussion is not relevant to this series at present. > > >>> 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. OK and I hope that file is not arch/arm/cpu/start.S which would introduce confusion I think. > > >>>> 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? About 800 bytes for me: text data bss dec hex filename 177653 3932 218240 399825 619d1 u-boot 178429 3932 218240 400601 61cd9 u-boot > > >>>> 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? Since I didn't hear anything I have elected to patch this up a different way - basically splitting out memcpy() and memset() from string.c. See what you think of the new series. It introduces no breakages now, at least. > > Amicalement, > -- > Albert. Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
