On Wed, Feb 05, 2014 at 04:47:04PM +0100, Hannes Petermaier wrote: > Adds support for Bernecker & Rainer Industrieelektronik GmbH KWB > Motherboard, using TI's AM3352 SoC. > > Most of code is derived from TI's AM335x_EVM > > Signed-off-by: Hannes Petermaier <oe5...@oevsv.at> > Cc: tr...@ti.com > --- > board/BuR/bur_kwb/Makefile | 16 ++ > board/BuR/bur_kwb/am335xScreen.c | 548 > ++++++++++++++++++++++++++++++++++++++ > board/BuR/bur_kwb/am335xScreen.h | 45 ++++
Minor nit, this should be am335x_screen.[ch]
> +ifeq ($(CONFIG_SPL_BUILD),y)
> +obj-y := mux.o
> +endif
> +ifeq ($(CONFIG_SYS_SCREEN),y)
> +obj-y += am335xScreen.o
> +endif
> +obj-y += board.o
obj-$(CONFIG_SPL_BUILD) +=
obj-$(CONFIG_SYS_SCREEN) +=
> diff --git a/board/BuR/bur_kwb/am335xScreen.c
> b/board/BuR/bur_kwb/am335xScreen.c
[snip]
> +#if defined(CONFIG_SYS_SCREEN) && !defined(CONFIG_SPL_BUILD)
You shouldn't need this. It'll be discarded if unused.
> +
> +#define DEBUG
> +#ifdef DEBUG
> +#define DBG(...) printf(__VA_ARGS__)
> +#else
> +#define DBG(...)
> +#endif /* DEBUG */
We provide debug(...) already.
> +#define SCREEN_HMAX 1366
> +#define SCREEN_VMAX 768
For consistency, please '#define<space>' globally, thanks.
> +static const unsigned char char_tab2[257*8] = {
A comment about what this is please.
> +static void setPix(struct screen_typ *pscr,
> + unsigned int x, unsigned int y, unsigned int color)
> +{
> + unsigned int addroffset = 0;
> +
> + /* braces not for function, only for better reading by human :-) */
> + addroffset = y * (pscr->resx*4) + (x*4);
> + *(unsigned int *)(pscr->pfb+addroffset) = (color & 0x00FFFFFF);
> +}
> +static void outch(struct screen_typ *pscr,
Newline after functions, fix globally. Also set_pix(), etc, also fix
globally.
> +++ b/board/BuR/bur_kwb/board.c
[snip]
> + if (i2c_probe(TPS65217_CHIP_PM)) {
> + printf("PMIC (0x%02x) not found! skip further initalization.\n",
> + TPS65217_CHIP_PM);
You should be able to do this with puts() since it's all constants.
> + /*
> + * Increase USB current limit to 1300mA or 1800mA and set
> + * the MPU voltage controller as needed.
> + */
> + if (dpll_mpu_opp100.m == MPUPLL_M_1000) {
> + usb_cur_lim = TPS65217_USB_INPUT_CUR_LIMIT_1800MA;
> + mpu_vdd = TPS65217_DCDC_VOLT_SEL_1325MV;
> + } else {
> + usb_cur_lim = TPS65217_USB_INPUT_CUR_LIMIT_1300MA;
> + mpu_vdd = TPS65217_DCDC_VOLT_SEL_1275MV;
> + }
Since you've got a tps65217 (like beaglebone) you can drop the tps65250
include earlier in the file.
> +++ b/board/BuR/bur_kwb/u-boot.lds
Since you don't support NOR, you should be able to use the generic
linker script.
> +++ b/include/configs/bur_kwb.h
[snip]
> +#include <config_cmd_default.h>
> +/*
> -------------------------------------------------------------------------*/
> +/* -- undefinig unused features --
> */
> +/* prior defined from config_defaults.h */
> +#undef CONFIG_BOOTM_LINUX
> +#undef CONFIG_BOOTM_NETBSD
> +#undef CONFIG_BOOTM_PLAN9
> +#undef CONFIG_BOOTM_RTEMS
> +#undef CONFIG_GZIP
> +#undef CONFIG_ZLIB
Space not tab please.
> +#define CONFIG_SYS_NO_FLASH
Duplicate.
> +/* MMC/SD IP block */
> +#if defined(CONFIG_EMMC_BOOT)
So, CONFIG_EMMC_BOOT is used in am335x_evm.h since we have to support
building for a few different variants. In this case it looks like you
can just drop setting EMMC_BOOT in boards.cfg and then simplify a lot of
the logic in the config file too.
> + #define CONFIG_MMC
> + #define CONFIG_GENERIC_MMC
> + #define CONFIG_OMAP_HSMMC
> + #define CONFIG_CMD_MMC
> + #define CONFIG_SUPPORT_EMMC_BOOT
So, are you making use of 'mmc open' / 'mmc close' on this platform to
work with the eMMC boot partitions today?
> +/*
> + * Our platforms make use of SPL to initalize the hardware (primarily
> + * memory) enough for full U-Boot to be loaded. We also support Falcon
> + * Mode so that the Linux kernel can be booted directly from SPL
> + * instead, if desired. We make use of the general SPL framework found
> + * under common/spl/. Given our generally common memory map, we set a
> + * number of related defaults and sizes here.
> + */
> +#define CONFIG_SPL
> +#define CONFIG_SPL_FRAMEWORK
> +#undef CONFIG_SPL_OS_BOOT
Just don't set CONFIG_SPL_OS_BOOT, and drop the function from board.c
that you've got too.
> +#define CONFIG_SYS_NS16550_COM1 0x44e09000 /* Base EVM has
> UART0 */
> +#define CONFIG_SYS_NS16550_COM2 0x48022000 /* UART1 */
> +#define CONFIG_SYS_NS16550_COM3 0x48024000 /* UART2 */
> +#define CONFIG_SYS_NS16550_COM4 0x481a6000 /* UART3 */
> +#define CONFIG_SYS_NS16550_COM5 0x481a8000 /* UART4 */
> +#define CONFIG_SYS_NS16550_COM6 0x481aa000 /* UART5 */
Do you use / care about all uarts?
And it looks like there's other parts of the config file that could be
cleaned up a bit too. Perhaps you can leverage
include/configs/ti_armv7_common.h ? If not, I'd really like to hear
about the shortcomings there as one of the goals with it is to make
adding new boards easier and have a smaller code footprint.
Thanks!
--
Tom
signature.asc
Description: Digital signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
