Re: [U-Boot] [PATCH 1/1] ARMV7: Add support For Logic OMAP35x/DM37x modules

15 Dec 2011

Hi Peter,
In addition to Tom's comments, several comments below:
On 12/15/11 00:47, Peter Barada wrote:
...
From: Peter Barada peterb@logicpd.com
This patch adds basic support for OMAP35x/DM37x SOM LV/Torpedo
reference boards. It assumes U-boot is loaded to SDRAM with the
help of another small bootloader (x-load) running from SRAM.
Signed-off-by: Peter Barada peter.barada@logicpd.com
board/logicpd/omap3som/Makefile     |   48 +++
 board/logicpd/omap3som/config.mk    |   33 ++
 board/logicpd/omap3som/omap3logic.c |  566 +++++++++++++++++++++++++++++++++++
 board/logicpd/omap3som/omap3logic.h |   35 +++
 boards.cfg                          |    1 +
 include/configs/omap3_logic.h       |  356 ++++++++++++++++++++++
 6 files changed, 1039 insertions(+), 0 deletions(-)
 create mode 100644 board/logicpd/omap3som/Makefile
 create mode 100644 board/logicpd/omap3som/config.mk
 create mode 100644 board/logicpd/omap3som/omap3logic.c
 create mode 100644 board/logicpd/omap3som/omap3logic.h
 create mode 100644 include/configs/omap3_logic.h

diff --git a/board/logicpd/omap3som/Makefile b/board/logicpd/omap3som/Makefile
new file mode 100644
index 0000000..ef0409f
--- /dev/null
+++ b/board/logicpd/omap3som/Makefile
@@ -0,0 +1,48 @@
+#
+# (C) Copyright 2000, 2001, 2002
+# Wolfgang Denk, DENX Software Engineering, wd@denx.de.
+#
+# See file CREDITS for list of people who contributed to this
+# project.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation; either version 2 of
+# the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+# MA 02111-1307 USA
+#



+include $(TOPDIR)/config.mk



+LIB	= $(obj)lib$(BOARD).o



+COBJS-y	:= omap3logic.o



+COBJS	:= $(sort $(COBJS-y))
+SRCS	:= $(COBJS:.o=.c)
+OBJS	:= $(addprefix $(obj),$(COBJS))



+$(LIB):	$(obj).depend $(OBJS)

$(call cmd_link_o_target, $(OBJS))


+clean:

rm -f $(OBJS)


+distclean:	clean

rm -f $(LIB) core *.bak $(obj).depend

clean and distclean are obsolete in this directory level, please remove.
...



+#########################################################################



+# defines $(obj).depend target
+include $(SRCTREE)/rules.mk



+sinclude $(obj).depend
diff --git a/board/logicpd/omap3som/config.mk b/board/logicpd/omap3som/config.mk
new file mode 100644
index 0000000..897b252
--- /dev/null
+++ b/board/logicpd/omap3som/config.mk
@@ -0,0 +1,33 @@
+#
+# (C) Copyright 2006 - 2008
+# Texas Instruments, <www.ti.com>
+#
+# EVM uses OMAP3 (ARM-CortexA8) cpu
+# see http://www.ti.com/ for more information on Texas Instruments
+#
+# See file CREDITS for list of people who contributed to this
+# project.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation; either version 2 of
+# the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+# MA 02111-1307 USA
+#
+# Physical Address:
+# 8000'0000 (bank0)
+# A000/0000 (bank1)
+# Linux-Kernel is expected to be at 8000'8000, entry 8000'8000
+# (mem base + reserved)



+# For use with external or internal boots.
+CONFIG_SYS_TEXT_BASE = 0x80400000
As Tom already said, this should be in the board config file
and this file should be removed completely.
...
diff --git a/board/logicpd/omap3som/omap3logic.c b/board/logicpd/omap3som/omap3logic.c
new file mode 100644
index 0000000..5c6e896
--- /dev/null
+++ b/board/logicpd/omap3som/omap3logic.c
@@ -0,0 +1,566 @@
+/*


(C) Copyright 2011



Logic Product Development <www.logicpd.com>







Author :



Peter Barada peter.barada@logicpd.com







Derived from Beagle Board and 3430 SDP code by



Richard Woodruff r-woodruff2@ti.com



Syed Mohammed Khasim khasim@ti.com







See file CREDITS for list of people who contributed to this



project.







This program is free software; you can redistribute it and/or



modify it under the terms of the GNU General Public License as



published by the Free Software Foundation; either version 2 of



the License, or (at your option) any later version.







This program is distributed in the hope that it will be useful,



but WITHOUT ANY WARRANTY; without even the implied warranty of



MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the



GNU General Public License for more details.







You should have received a copy of the GNU General Public License



along with this program; if not, write to the Free Software



Foundation, Inc., 59 Temple Place, Suite 330, Boston,



MA 02111-1307 USA



I vote for removing the postal address,
because it is subject to change and you will not follow it,
but it is not a blocker.
[...]
...
+/*


Routine: logic_identify



Description: Detect if we are running on a Logic or Torpedo.



         This can be done by GPIO_189. If its low after driving it high,





         then its a SOM LV, else Torpedo.




*/

+unsigned int logic_identify(void)
+{

unsigned int val = 0;
u32 cpu_family = get_cpu_family();

You only use this once, so IMO can be inlined.
...

int i;

MUX_LOGIC_HSUSB0_DATA5_GPIO_MUX();

if (!gpio_request(189, "")) {

This does not look good... can it be:
if (gpio_request(...) == 0)
and also please provide a label with a description
instead of an empty one.
...


gpio_direction_output(189, 0);


gpio_set_value(189, 1);



/* Let it soak for a bit */


for (i = 0; i < 0x100; ++i)


	asm("nop");



gpio_direction_input(189);


val = gpio_get_value(189);


gpio_free(189);



printf("Board:");


if (cpu_family == CPU_OMAP36XX) {


	printf(" DM37xx");


	if (val) {


		printf(" Torpedo\n");


		val = MACH_TYPE_DM3730_TORPEDO;


	} else {


		printf(" SOM LV\n");


		val = MACH_TYPE_DM3730_SOM_LV;


	}


} else {


	printf(" OMAP35xx");


	if (val) {


		printf(" Torpedo\n");


		val = MACH_TYPE_OMAP3_TORPEDO;


	} else {


		printf(" SOM LV\n");


		val = MACH_TYPE_OMAP3530_LV_SOM;


	}


}


}

MUX_LOGIC_HSUSB0_DATA5_DATA5();

return val;

+}
The whole function looks like checkboard(), isn't it?
I think it should be then.
[...]
...
+/*


Routine: board_init



Description: Early hardware init.


*/

+int board_init(void)
+{

gpmc_init(); /* in SRAM or SDRAM, finish GPMC */

/* board id for Linux */
gd->bd->bi_arch_number = logic_identify();

This also can be moved to checkboard().
...


/* boot param addr */
gd->bd->bi_boot_params = (OMAP34XX_SDRC_CS0 + 0x100);


return 0;

+}
[...]
...
+/*


Routine: misc_init_r



Description: Init ethernet (done here so udelay works)


*/

+int misc_init_r(void)
+{
+#if defined(CONFIG_CMD_NET)

setup_net_chip();

+#endif
Can this be done in board_eth_init()?
So the net init code will be close to each other?
...


dieid_num_r();

return 0;

+}





Three empty lines?
There should be only one line between functions.
...
+int board_eth_init(bd_t *bis)
+{

int rc = 0;

+#ifdef CONFIG_SMC911X

rc = smc911x_initialize(0, CONFIG_SMC911X_BASE);

+#endif

return rc;

+}
board_eth_init() has a weak implementation, so
I think it would be much nicer:
#ifdef CONFIG_SMC911X
int board_eth_init(bd_t *bis)
{
    setup_net_chip();
return smc911x_initialize(0, CONFIG_SMC911X_BASE);
}
#endif
...





Again three empty lines?
...
+/*


IEN  - Input Enable



IDIS - Input Disable



PTD  - Pull type Down



PTU  - Pull type Up



DIS  - Pull type selection is inactive



EN   - Pull type selection is active



M0   - Mode 0



The commented string gives the final mux configuration for that pin


*/


+/*


Routine: set_muxconf_regs



Description: Setting up the configuration Mux registers specific to the



hardware. Many pins need to be moved from protect to primary





mode.




*/

+void set_muxconf_regs(void)
+{

/*SDRC*/

Alignment...
[...]
...

/*GPMC*/

same here, comments must be aligned to code.
[...]
...

/*DSS*/

ditto
[...]
...

/*CAMERA*/

ditto
[...]
...

/*Audio Interface */

ditto
...

MUX_VAL(CP(MCBSP2_FSX),		(IEN  | PTD | EN  | M7)); /*safe mode */
MUX_VAL(CP(MCBSP2_CLKX),	(IEN  | PTD | EN  | M7)); /*safe mode */
MUX_VAL(CP(MCBSP2_DR),		(IEN  | PTD | EN  | M7)); /*safe mode */
MUX_VAL(CP(MCBSP2_DX),		(IEN  | PTD | EN  | M7)); /*safe mode */

/*Expansion card  */

ditto
...

MUX_VAL(CP(MMC1_CLK),		(IDIS | PTU | EN  | M0));
MUX_VAL(CP(MMC1_CMD),		(IEN  | PTU | EN  | M0));
MUX_VAL(CP(MMC1_DAT0),		(IEN  | PTU | EN  | M0));
MUX_VAL(CP(MMC1_DAT1),		(IEN  | PTU | EN  | M0));
MUX_VAL(CP(MMC1_DAT2),		(IEN  | PTU | EN  | M0));
MUX_VAL(CP(MMC1_DAT3),		(IEN  | PTU | EN  | M0));
MUX_VAL(CP(MMC1_DAT4),		(IEN  | PTD | EN  | M7)); /*safe mode*/
MUX_VAL(CP(MMC1_DAT5),		(IEN  | PTD | EN  | M7)); /*safe mode*/
MUX_VAL(CP(MMC1_DAT6),		(IEN  | PTD | EN  | M7)); /*safe mode*/
MUX_VAL(CP(MMC1_DAT7),		(IEN  | PTD | EN  | M7)); /*safe mode*/
/*Wireless LAN */

ditto
...

MUX_VAL(CP(MMC2_CLK),		(IEN  | PTD | EN  | M7)); /*safe mode*/
MUX_VAL(CP(MMC2_CMD),		(IEN  | PTD | EN  | M7)); /*safe mode*/
MUX_VAL(CP(MMC2_DAT0),		(IEN  | PTD | EN  | M7)); /*safe mode*/
MUX_VAL(CP(MMC2_DAT1),		(IEN  | PTD | EN  | M7)); /*safe mode*/
MUX_VAL(CP(MMC2_DAT2),		(IEN  | PTD | EN  | M7)); /*safe mode*/
MUX_VAL(CP(MMC2_DAT3),		(IEN  | PTD | EN  | M7)); /*safe mode*/
MUX_VAL(CP(MMC2_DAT4),		(IEN  | PTD | EN  | M7)); /*safe mode*/
MUX_VAL(CP(MMC2_DAT5),		(IEN  | PTD | EN  | M7)); /*safe mode*/
MUX_VAL(CP(MMC2_DAT6),		(IEN  | PTD | EN  | M7)); /*safe mode*/
MUX_VAL(CP(MMC2_DAT7),		(IEN  | PTD | EN  | M7)); /*safe mode*/
/*Bluetooth*/

ditto
...

MUX_VAL(CP(MCBSP3_DX),		(IEN  | PTD | EN  | M7)); /*safe mode*/
MUX_VAL(CP(MCBSP3_DR),		(IEN  | PTD | EN  | M7)); /*safe mode*/
MUX_VAL(CP(MCBSP3_CLKX),	(IEN  | PTD | EN  | M7)); /*safe mode*/
MUX_VAL(CP(MCBSP3_FSX),		(IEN  | PTD | EN  | M7)); /*safe mode*/
MUX_VAL(CP(UART2_CTS),		(IEN  | PTD | EN  | M7)); /*safe mode*/
MUX_VAL(CP(UART2_RTS),		(IEN  | PTD | EN  | M7)); /*safe mode*/
MUX_VAL(CP(UART2_TX),		(IEN  | PTD | EN  | M7)); /*safe mode*/
MUX_VAL(CP(UART2_RX),		(IEN  | PTD | EN  | M7)); /*safe mode*/
/*Modem Interface */

ditto
...

MUX_VAL(CP(UART1_TX),		(IDIS | PTD | DIS | M0));
MUX_VAL(CP(UART1_RTS),		(IDIS | PTD | DIS | M0));
MUX_VAL(CP(UART1_CTS),		(IEN  | PTU | DIS | M0));
MUX_VAL(CP(UART1_RX),		(IEN  | PTD | DIS | M0));
MUX_VAL(CP(MCBSP4_CLKX),	(IEN  | PTD | EN  | M7)); /*safe mode*/
MUX_VAL(CP(MCBSP4_DR),		(IEN  | PTD | EN  | M7)); /*safe mode*/
MUX_VAL(CP(MCBSP4_DX),		(IDIS | PTD | EN  | M7)); /*safe mode*/
MUX_VAL(CP(MCBSP4_FSX),		(IDIS | PTD | EN  | M7)); /*safe mode*/

MUX_VAL(CP(MCBSP1_CLKR),	(IEN  | PTD | EN  | M7)); /*safe mode*/
MUX_VAL(CP(MCBSP1_FSR),		(IEN  | PTD | EN  | M7)); /*safe mode*/
MUX_VAL(CP(MCBSP1_DX),		(IEN  | PTD | EN  | M7)); /*safe mode*/
MUX_VAL(CP(MCBSP1_DR),		(IEN  | PTD | EN  | M7)); /*safe mode*/
MUX_VAL(CP(MCBSP_CLKS),		(IEN  | PTD | EN  | M7)); /*safe mode*/
MUX_VAL(CP(MCBSP1_FSX),		(IEN  | PTD | EN  | M7)); /*safe mode*/
MUX_VAL(CP(MCBSP1_CLKX),	(IEN  | PTD | EN  | M7)); /*safe mode*/
/*Serial Interface*/

ditto
[...]
...

/*Control and debug */

ditto
[...]
...

/*Die to Die */

ditto
[...]
...
+}
I think this function is *much better* than the mux config done .h file.
Good job!
-- 
Regards,
Igor.

    

Re: [U-Boot] [PATCH 1/1] ARMV7: Add support For Logic OMAP35x/DM37x modules

Igor Grinberg

Signed-off-by: Peter Barada peter.barada@logicpd.com