microbuilder / LPC810_CodeBase

Open source code base for the ARM Cortex M0+ LPC810 family from NXP
85 stars 43 forks source link

SystemCoreClock wastes a lot of space #2

Closed cpldcpu closed 11 years ago

cpldcpu commented 11 years ago

I tried to find out why the codebase is still rather large, given its limited functionality. One of the apparent space-wasters I came across is the handling of the system clock rate. Since it is stored as a variable, all clock-specific functions have to be calculated in run time. On a device with al program space as small as the LPC810 it seems unlikely that people do need the functionality.

When removing SystemCoreClockUpdate (void) from the codebase and setting the clock rate to a fixed value, more than 200 bytes of codespace are freed. I would suggest to introduce a global define for the clockrate. There seem to be some helper functions in the CMSIS defines to allow this, but I don't want to mess around with them in case there is an existing solution.

microbuilder commented 11 years ago

If you're willing to submit a pull request, I'd be happy to integrate it, and I agree the CMSIS startup code is one of the biggest wastes of space. I haven't looked seriously into alternatives, though, since the goal here was just to provide a simple example project, but there is room for improvement on the total code size, starting with limiting calls to the core CMSIS functions.

cpldcpu commented 11 years ago

I'd be willing to do it, but right now I am a bit unsure of how to do it without "hacking" it in. A clean way would be to derive the clockrate from the clock configuration. However, currently the clock configuration does not seem to be included. How is it handled usually? I am relatively new to LPC.

microbuilder commented 11 years ago

The clock configuration is handled in the CMSIS startup code, which is the problem with size here. It's convenient to use the CMSIS functions and startup code since they are common to every other project/MCU and it gets the config out of the way for people new to the chip, but on such a small device even a couple hundred bytes of overhead for these helper functions is significant.

cpldcpu commented 11 years ago

Ok, found it! One question: Why is this in system_lpc8xx.c? If it could be moved to the header-file, __SYSTEM_CLOCK could be used as a global clock constant.

#define CLOCK_SETUP           1
#define SYSOSCCTRL_Val        0x00000000              // Reset: 0x000
#define WDTOSCCTRL_Val        0x00000000              // Reset: 0x000
#define SYSPLLCTRL_Val        0x00000041              // Reset: 0x000

... and all the other defines for the clock.

cpldcpu commented 11 years ago

Oh, sorry. I noticed that the original CMSIS comes like that. How strange - this does not look like very professional code. Also the copy&paste clock register description seems somewhat out of place.

Ok, so I just may change it around.

microbuilder commented 11 years ago

I've never been very fond of this approach with the CMSIS code either. I hate setting up my clock this way, and I resisted it strongly with the LPC1343 and LPC1114 (the first Cortex chips I seriously used), and I literally spent a month creating my own system header files by hand from the UM (https://github.com/microbuilder/LPC1343CodeBase/blob/master/lpc134x.h). But it's tough to swim against the stream all the time and I've just kind of accepted that CMSIS is here to stay, and that there's good and bad about it.

I agree it's not the most elegant way to handle things, but that's the way the code comes it seems to be what people are used to in the Cortex M space.

That said ... if you want to put together an alternative, I'd be happy to integrate it and perhaps chose between the CMSIS and custom implementation with a compilation flag or something similar just to cover both needs (small or 'standard')? But in principle, I think we should avoid touching the canned CMSIS and vendor files and implement something code compatible in a separate smaller file. 4KB is an extremely tight fit, and this sort of ham-fisted approach doesn't work well to get the most out of these capable little chips.

cpldcpu commented 11 years ago

I tried :) See pull request.