From mboxrd@z Thu Jan  1 00:00:00 1970
From: Dirk Behme <dirk.behme@googlemail.com>
Date: Mon, 10 Nov 2008 20:28:14 +0100
Subject: [U-Boot] [PATCH 11/13 v5] ARM: OMAP3: Add EVM board
In-Reply-To: <4917eef4.02a1660a.35ee.72f8SMTPIN_ADDED@mx.google.com>
References: <490df3be.1438560a.2e49.3fb7@mx.google.com>
	<490df3da.1438560a.2e2d.31f5@mx.google.com>
	<20081109150321.GF25307@game.jcrosoft.org>
	<4917eef4.02a1660a.35ee.72f8SMTPIN_ADDED@mx.google.com>
Message-ID: <49188B4E.1010809@googlemail.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Dear Wolfgang,

Wolfgang Denk wrote:
> Dear Jean-Christophe PLAGNIOL-VILLARD,
> 
> In message <20081109150321.GF25307@game.jcrosoft.org> you wrote:
> 
>>>+	i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE);
>>>+#endif
>>>+
>>>+#if defined(CONFIG_CMD_NET)
>>>+	setup_net_chip();
>>>+#endif
>>>+
>>>+	return 0;
>>>+}
>>>+
>>>+/******************************************************************************
>>>+ * 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)
>>>+{
>>>+	MUX_EVM();
>>>+}
>>>+
>>>+/******************************************************************************
>>>+ * Routine: setup_net_chip
>>>+ * Description: Setting up the configuration GPMC registers specific to the
>>>+ *              Ethernet hardware. Pin Muxing for the SMC9118 is initialized
>>>+ *              here.
>>>+ *****************************************************************************/
>>>+static int setup_net_chip(void)
>>>+{
>>
>>in this function please add some blank line to make the code more readable
>>
>>>+	int i = 0;
>>>+
>>>+	/* Configure GPMC registers */
>>>+	(*(volatile int *)(OMAP34XX_GPMC_BASE + 0x0150)) = 0x00001000;
>>>+	(*(volatile int *)(OMAP34XX_GPMC_BASE + 0x0154)) = 0x001e1e01;
>>>+	(*(volatile int *)(OMAP34XX_GPMC_BASE + 0x0158)) = 0x00080300;
>>>+	(*(volatile int *)(OMAP34XX_GPMC_BASE + 0x015C)) = 0x1c091c09;
>>>+	(*(volatile int *)(OMAP34XX_GPMC_BASE + 0x0160)) = 0x04181f1f;
>>>+	(*(volatile int *)(OMAP34XX_GPMC_BASE + 0x0164)) = 0x00000FCF;
>>>+	(*(volatile int *)(OMAP34XX_GPMC_BASE + 0x0168)) = 0x00000f6c;
>>>+
>>>+	/* Configure PIN MUX registers */
>>>+	/* Enable GPMC Pin Mux Registers */
>>>+	/* Enable GPMC_CLK Pin in CONTROL_PADCONF_gpmc_ncs7 register */
>>>+	(*(volatile int *)(OMAP34XX_CTRL_BASE + 0xBC)) |= 0x00180000;
>>>+	/* Enable CS5 Pin in CONTROL_PADCONF_gpmc_ncs5 register */
>>>+	(*(volatile int *)(OMAP34XX_CTRL_BASE + 0xB8)) |= 0x00000018;
>>>+	(*(volatile int *)(OMAP34XX_CTRL_BASE + 0xB8)) &= 0xFFFFFFF8;
>>>+	/* Enable offmode for nwe in CONTROL_PADCONF_GPMC_NWE register */
>>>+	(*(volatile int *)(OMAP34XX_CTRL_BASE + 0xC4)) |= 0x00000F00;
>>>+	/* En off mode for noe and ale in CONTROL_PADCONF_GPMC_NADV_ALE reg */
>>>+	(*(volatile int *)(OMAP34XX_CTRL_BASE + 0xC0)) |= 0x0E000E00;
>>>+	/* Enable gpmc_nbe0_cle in CONTROL_PADCONF_GPMC_NWE register */
>>>+	(*(volatile int *)(OMAP34XX_CTRL_BASE + 0xC4)) |= 0x00180000;
>>>+
>>>+	/* Enable gpmc_nbe1 in CONTROL_PADCONF_GPMC_NBE1 register and
>>>+	configuring the mux mode to 0 */
>>>+	(*(volatile int *)(OMAP34XX_CTRL_BASE + 0xC8)) |= 0x00000018;
>>>+	(*(volatile int *)(OMAP34XX_CTRL_BASE + 0xC8)) &= 0xFFFFFFF8;
>>>+	/* Enable d15 in CONTROL_PADCONF_GPMC_D15 register */
>>>+	(*(volatile int *)(OMAP34XX_CTRL_BASE + 0xAC)) |= 0x00000018;
>>>+	/* Enable d14 - d13 in CONTROL_PADCONF_GPMC_D13 register */
>>>+	(*(volatile int *)(OMAP34XX_CTRL_BASE + 0xA8)) |= 0x00180018;
>>>+	/* Enable d12 - d11 in CONTROL_PADCONF_GPMC_D11 register */
>>>+	(*(volatile int *)(OMAP34XX_CTRL_BASE + 0xA4)) |= 0x00180018;
>>>+	/* Enable d10 - d9 in CONTROL_PADCONF_GPMC_D9 register */
>>>+	(*(volatile int *)(OMAP34XX_CTRL_BASE + 0xA0)) |= 0x00180018;
>>>+	/* Enable d8 - d7 in CONTROL_PADCONF_GPMC_D7 register */
>>>+	(*(volatile int *)(OMAP34XX_CTRL_BASE + 0x9C)) |= 0x00180018;
>>>+	/* Enable d6 - d5 in CONTROL_PADCONF_GPMC_D5 register */
>>>+	(*(volatile int *)(OMAP34XX_CTRL_BASE + 0x98)) |= 0x00180018;
> 
> 
> And please use accessor functions / macros to access the registers.
> Access through volatile pointers is considered a Bad Thing (TM).

Sorry for not mentioning this earlier, but you don't have to comment on

a) readx/writex accessor functions / macros
b) hardcoded values vs. speaking macros

These are known issues of OMAP3 patch set I will fix step by step. 
Once we send the "final" OMAP3 patch set to the mailing list, if I 
missed something, then you are welcome to comment on it, though ;)

Thanks for your review and best regards

Dirk