Dear Hannes, In message <1391615224-26493-1-git-send-email-oe5...@oevsv.at> you 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 ++++ > board/BuR/bur_kwb/board.c | 522 ++++++++++++++++++++++++++++++++++++ > board/BuR/bur_kwb/board.h | 18 ++ > board/BuR/bur_kwb/mux.c | 213 +++++++++++++++ > board/BuR/bur_kwb/u-boot.lds | 101 +++++++
See previous comments. Repeating the "bur_" part in the board directory name is redundant; I recommend to drop that. > + * History > + * ======= > + * 17.12.2013 hpm created Please drop this. Please fix globally. > +#define DEBUG > +#ifdef DEBUG > +#define DBG(...) printf(__VA_ARGS__) > +#else > +#define DBG(...) > +#endif /* DEBUG */ We already have debug(); please use existing standard facilities. > + if ('\r' == *str) { ... > + } else if ('\n' == *str) { Please avoid this reversed notation style. Please fix globally. ... > +} > +int drawrecticle(unsigned int x, unsigned int y, > + unsigned int w, unsigned int h, unsigned int color) > +{ CodingStyle says: "In source files, separate functions with one blank line." Please fix globally. > + setPix(&screen, j, i, color); We do not allow CamelCase identifiers. Please fix globally. > +int prints(const char *fmt, ...) ... > +int printsxy(unsigned int x, unsigned int y, const char *fmt, ...) etc. BTW - are you not reinventing the wheel here? What about functions like video_drawstring(), video_putchar(), ... ? > index 0000000..5a1b15f > --- /dev/null > +++ b/board/BuR/bur_kwb/board.c > @@ -0,0 +1,522 @@ Pretty significant parts of this file appear to be identical to code in board/BuR/bur_tseries/board.c as added by your earlier patch. Please factor out such common code into a common/ directory so it can be shared across boards. > + * History > + * ======= > + * xx.10.2013 hpm - created, adopted from ti_am335x_evm > + * 06.12.2013 hpm - setting MPU_PLL to 600 MHz (as required for > + * industrial) > + * - implemented simple boot-mode detection > + * - reduced resetpulse for USB2SD controller > + * from 5ms to 1ms > + * 10.12.2013 hpm - switch OFF LCD-Screen within SPL-Stage > + * (due to invalid 3V3 pullup resistor) > + * - powerUP 3V3 via I2C-Resetcontroller within SPL-Stage > + * 16.12.2013 hpm - V1.10: changed bootaddr of VxWorks from > + * 0x82000000 to 0x80100000 > + * 24.12.2013 hpm - V1.11: added LCD-support > + * 30.01.2014 hpm - V2.00: ported to new U-Boot (2014) sources As mentioned before: please drop globally! > + printf("echo setting PMIC Register in the RTC.\n"); > + *(unsigned int *)0x44e3e098 = 0x1f010; > + printf("read back PMIC Register: 0x%08x\n", > + *(unsigned int *)0x44e3e098); > + > + year = *(unsigned int *)0x44e3e014; > + mon = *(unsigned int *)0x44e3e010; > + d = *(unsigned int *)0x44e3e00C; > + h = *(unsigned int *)0x44e3e008; > + min = *(unsigned int *)0x44e3e004; Please use proper I/O accessors to access hardware registers, and declare a proper C struct to describe the register layout. Please fix globally. > +static struct cpsw_slave_data cpsw_slaves[] = { > + { > + .slave_reg_ofs = 0x208, > + .sliver_reg_ofs = 0xd80, ... > + .slave_reg_ofs = 0x308, > + .sliver_reg_ofs = 0xdc0, Would it make sense to provide symboolic names for such magic numbers? And how about some comments to explain the actual settings? > diff --git a/board/BuR/bur_kwb/board.h b/board/BuR/bur_kwb/board.h > new file mode 100644 > index 0000000..def41aa > --- /dev/null > +++ b/board/BuR/bur_kwb/board.h > @@ -0,0 +1,18 @@ > +/* > + * board.h > + * > + * BUR-KWB boards information header > + * > + * Copyright (C) 2013, Bernecker & Rainer Industrieelektronik GmbH - > + * http://www.br-automation.com > + * > + * SPDX-License-Identifier: GPL-2.0+ > + */ > + > +#ifndef _BOARD_H_ > +#define _BOARD_H_ > + > +void enable_uart0_pin_mux(void); > +void enable_i2c0_pin_mux(void); > +void enable_board_pin_mux(void); This file appears to be identical to board/BuR/bur_tseries/board.h as added by your earlier patch. Please use a common header file instead. > diff --git a/board/BuR/bur_kwb/mux.c b/board/BuR/bur_kwb/mux.c > new file mode 100644 > index 0000000..da4b9f3 > --- /dev/null > +++ b/board/BuR/bur_kwb/mux.c > @@ -0,0 +1,213 @@ > +static struct module_pin_mux uart0_pin_mux[] = { > + /* UART0_CTS */ > + {OFFSET(uart0_ctsn), (MODE(7) | PULLUDEN | PULLUP_EN | RXACTIVE)}, > + /* UART0_RXD */ > + {OFFSET(uart0_rxd), (MODE(0) | PULLUDEN | PULLUP_EN | RXACTIVE)}, > + /* UART0_TXD */ > + {OFFSET(uart0_txd), (MODE(0) | PULLUDEN)}, > + {-1}, > +}; Again, there is lots of duplicated code. Please factor out common parts. Please fix globally. > diff --git a/board/BuR/bur_kwb/u-boot.lds b/board/BuR/bur_kwb/u-boot.lds > new file mode 100644 > index 0000000..021f869 > --- /dev/null > +++ b/board/BuR/bur_kwb/u-boot.lds Not needed? Drop?? > diff --git a/include/configs/bur_kwb.h b/include/configs/bur_kwb.h > new file mode 100644 > index 0000000..66048dd > --- /dev/null > +++ b/include/configs/bur_kwb.h Large duplicated sections again... Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de If the facts don't fit the theory, change the facts. -- Albert Einstein _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot