[U-Boot] [PATCH v3 3/3] arm, davinci: add support for am1808 based enbw_cmc board

[Heiko Schocher <hs@denx.de>]

Hello Igor,

Igor Grinberg wrote:
> Hi Heiko,
> 
> On 11/28/11 12:44, Heiko Schocher wrote:
>> - booting from NOR Flash with direct boot method
>> - POST support
>> - LOGBUF support
>>
>> Signed-off-by: Heiko Schocher <hs@denx.de>
>> Cc: Paulraj Sandeep <s-paulraj@ti.com>
>> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
>> Cc: Igor Grinberg <grinberg@compulab.co.il>
>> Cc: Christian Riesch <christian.riesch@omicron.at>
>> ***
[...]
>> diff --git a/board/enbw/enbw_cmc/Makefile b/board/enbw/enbw_cmc/Makefile
>> new file mode 100644
>> index 0000000..bdba069
>> --- /dev/null
>> +++ b/board/enbw/enbw_cmc/Makefile
>> @@ -0,0 +1,51 @@
[...]
>> +clean:
>> +	rm -f $(SOBJS) $(OBJS)
>> +
>> +distclean:	clean
>> +	rm -f $(LIB) core *.bak *~ .depend
> 
> clean and distclean on that directory level should be gone.

fixed, thanks!

>> +
>> +#########################################################################
>> +# This is for $(obj).depend target
>> +include $(SRCTREE)/rules.mk
>> +
>> +sinclude $(obj).depend
>> +
>> +#########################################################################
>> diff --git a/board/enbw/enbw_cmc/enbw_cmc.c b/board/enbw/enbw_cmc/enbw_cmc.c
>> new file mode 100644
>> index 0000000..6333b3a
>> --- /dev/null
>> +++ b/board/enbw/enbw_cmc/enbw_cmc.c
>> @@ -0,0 +1,652 @@
>> +/*
>> + * (C) Copyright 2011
>> + * Heiko Schocher, DENX Software Engineering, hs at denx.de.
[...]
>> +
>> +#include <common.h>
>> +#include <command.h>
>> +#include <environment.h>
>> +#include <hwconfig.h>
>> +#include <i2c.h>
>> +#include <malloc.h>
>> +#include <miiphy.h>
>> +#include <mmc.h>
>> +#include <net.h>
>> +#include <netdev.h>
>> +#include <asm/arch/hardware.h>
>> +#include <asm/arch/emif_defs.h>
>> +#include <asm/arch/emac_defs.h>
>> +#include <asm/gpio.h>
>> +#include <asm/arch/gpio.h>
>> +#include <asm/arch/davinci_misc.h>
>> +#include <asm/arch/sdmmc_defs.h>
>> +#include <asm/arch/timer_defs.h>
>> +#include <asm/arch/pinmux_defs.h>
>> +#include <asm/io.h>
>> +#include <asm/arch/da850_lowlevel.h>
> 
> Can the above includes be made like:
> all #include <*.h> together
> all #include <asm/*.h> together
> all #include <asm/arch/*.h> together
> ?

Done.

> 
>> +
>> +DECLARE_GLOBAL_DATA_PTR;
>> +
[...]
>> +
>> +const int pinmuxes_size = ARRAY_SIZE(pinmuxes);
> 
> Where do you use this one? and why not just inline it?

This is introduced through patch (not in mainline yet):

[U-Boot,v3,07/15] arm, davinci: Remove duplication of pinmux configuration code
http://patchwork.ozlabs.org/patch/127680/

>> +
>> +struct gpio_config {
>> +	char name[GPIO_NAME_SIZE];
>> +	unsigned char bank;
>> +	unsigned char gpio;
>> +	unsigned char out;
>> +	unsigned char value;
>> +};
>> +
>> +static const struct gpio_config enbw_gpio_config[] = {
>> +	{ "RS485 enable", 8, 11, 1, 0 },
>> +	{ "RS485 iso", 8, 10, 1, 0 },
>> +	{ "W2HUT RS485 Rx ena", 8, 9, 1, 0 },
>> +	{ "W2HUT RS485 iso", 8, 8, 1, 0 },
>> +	{ "LAN reset", 7, 15, 1, 1 },
>> +	{ "ena 11V PLC", 7, 14, 1, 0 },
>> +	{ "ena 1.5V PLC", 7, 13, 1, 0 },
>> +	{ "disable VBUS", 7, 12, 1, 1 },
>> +	{ "PLC reset", 6, 13, 1, 1 },
>> +	{ "LCM RS", 6, 12, 1, 0 },
>> +	{ "LCM R/W", 6, 11, 1, 0 },
>> +	{ "PLC pairing", 6, 10, 1, 0 },
>> +	{ "PLC MDIO CLK", 6, 9, 1, 0 },
>> +	{ "HK218", 6, 8, 1, 0 },
>> +	{ "HK218 Rx", 6, 1, 1, 1 },
>> +	{ "TPM reset", 6, 0, 1, 1 },
>> +	{ "LCM E", 2, 2, 1, 1 },
>> +	{ "PV-IF RxD ena", 0, 15, 1, 0 },
>> +	{ "LED1", 1, 15, 1, 1 },
>> +	{ "LED2", 0, 1, 1, 1 },
>> +	{ "LED3", 0, 2, 1, 1 },
>> +	{ "LED4", 0, 3, 1, 1 },
>> +	{ "LED5", 0, 4, 1, 1 },
>> +	{ "LED6", 0, 5, 1, 0 },
>> +	{ "LED7", 0, 6, 1, 0 },
>> +	{ "LED8", 0, 14, 1, 0 },
>> +	{ "USER1", 0, 12, 0, 0 },
>> +	{ "USER2", 0, 13, 0, 0 },
>> +};
> 
> This will look much better if all values will be aligned -
> making columns.

Done.

> Also, IMO, this is usable platform wide.

Hmm.. this is board specific gpio pin setup ...

>> +
>> +#ifndef CONFIG_USE_IRQ
>> +void irq_init(void)
>> +{
>> +	/*
>> +	 * Mask all IRQs by clearing the global enable and setting
>> +	 * the enable clear for all the 90 interrupts.
>> +	 */
>> +
>> +	writel(0, &davinci_aintc_regs->ger);
>> +
>> +	writel(0, &davinci_aintc_regs->hier);
>> +
>> +	writel(0xffffffff, &davinci_aintc_regs->ecr1);
>> +	writel(0xffffffff, &davinci_aintc_regs->ecr2);
>> +	writel(0xffffffff, &davinci_aintc_regs->ecr3);
>> +}
>> +#endif
>> +
>> +void davinci_emac_mii_mode_sel(int mode_sel)
>> +{
>> +	int val;
>> +
>> +	val = readl(&davinci_syscfg_regs->cfgchip3);
>> +	if (mode_sel == 0)
>> +		val &= ~(1 << 8);
>> +	else
>> +		val |= (1 << 8);
>> +	writel(val, &davinci_syscfg_regs->cfgchip3);
>> +}
> 
> This one also can be used platform wide.

Ok, done. Move for this board/davinci/common/misc.c to
arch/arm/cpu/arm926ejs/davinci/misc.c in a seperate patch.

[...]
>> +int board_init(void)
>> +{
>> +	int i, ret;
>> +
>> +#ifndef CONFIG_USE_IRQ
>> +	irq_init();
>> +#endif
>> +	/* address of boot parameters, not used as booting with DTT */
>> +	gd->bd->bi_boot_params = 0;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(enbw_gpio_config); i++) {
>> +		int gpio = enbw_gpio_config[i].bank * 16 +
>> +			enbw_gpio_config[i].gpio;
>> +
>> +		ret = gpio_request(gpio, enbw_gpio_config[i].name);
>> +		if (ret)
>> +			printf("%s: Could not get %s gpio\n", __func__,
>> +				enbw_gpio_config[i].name);
>> +		else
> 
> instead of having that else and adding another level of indentation below
> you can just add continue;

Done.

>> +			if (enbw_gpio_config[i].out)
>> +				gpio_direction_output(gpio,
>> +					enbw_gpio_config[i].value);
>> +			else
>> +				gpio_direction_input(gpio);
>> +	}
>> +
>> +
>> +	/* setup the SUSPSRC for ARM to control emulation suspend */
>> +	clrbits_le32(&davinci_syscfg_regs->suspsrc,
>> +		(DAVINCI_SYSCFG_SUSPSRC_EMAC | DAVINCI_SYSCFG_SUSPSRC_I2C |
>> +		DAVINCI_SYSCFG_SUSPSRC_SPI1 | DAVINCI_SYSCFG_SUSPSRC_TIMER0 |
>> +		DAVINCI_SYSCFG_SUSPSRC_UART2));
>> +
>> +#ifdef CONFIG_DRIVER_TI_EMAC
>> +	davinci_emac_mii_mode_sel(0);
>> +#endif /* CONFIG_DRIVER_TI_EMAC */
> 
> Can't the above be called from board_eth_init()?
> Or is it too late?

Moved.

[...]
>> +U_BOOT_CMD(led, 3, 1, do_led,
>> +	"switch on/off board led",
>> +	"[name] [on/off]"
>> +);
> 
> Why can't you use the common/cmd_led.c?

We should add the possibilty to define
static const led_tbl_t led_commands board specific ... the
more or less fix table is not fitting my board needs.

[...]
>> +void board_gpio_init(void)
>> +{
>> +	struct davinci_gpio *gpio = davinci_gpio_bank01;
>> +	int i;
>> +
>> +	/*
>> +	 * Power on required peripherals
>> +	 * ARM does not have access by default to PSC0 and PSC1
>> +	 * assuming here that the DSP bootloader has set the IOPU
>> +	 * such that PSC access is available to ARM
>> +	 */
>> +	for (i = 0; i < ARRAY_SIZE(lpsc); i++)
>> +		lpsc_on(lpsc[i].lpsc_no);
>> +
>> +	/*
>> +	 * set LED (gpio Interface not usable here)
>> +	 * set LED pins to output and state 0
>> +	 */
>> +	clrbits_le32(&gpio->dir, 0x8000407e);
>> +	clrbits_le32(&gpio->out_data, 0x8000407e);
>> +	/* set LED 1 - 5 to state on */
>> +	setbits_le32(&gpio->out_data, 0x8000001e);
>> +
>> +	return;
> 
> no need for this return.

removed.

>> +}
> 
> This also looks like can be done platform wide and
> not for the specific board.

Yep, calling here da8xx_configure_lpsc_items().

[...]
>> +int board_mmc_init(bd_t *bis)
>> +{
>> +	int err;
>> +
>> +	mmc_sd1.input_clk = clk_get(DAVINCI_MMC_CLKID);
>> +	/* Add slot-0 to mmc subsystem */
>> +	err = davinci_mmc_init(bis, &mmc_sd1);
>> +
>> +	return err;
> 
> You can just return davinci_mmc_init(bis, &mmc_sd1);
> and remove the err variable.

Done.

> Also, is clk_get() never fails?

No.

[...]
>> diff --git a/include/configs/enbw_cmc.h b/include/configs/enbw_cmc.h
>> new file mode 100644
>> index 0000000..bd00f39
>> --- /dev/null
>> +++ b/include/configs/enbw_cmc.h
[...]
>> +#define CONFIG_ARM926EJS		/* arm926ejs CPU core */
>> +#define CONFIG_SOC_DA8XX		/* TI DA8xx SoC */
>> +#define CONFIG_SOC_DA850		/* TI DA850 SoC */
>> +#define CONFIG_SYS_CLK_FREQ		clk_get(DAVINCI_ARM_CLKID)
>> +#define CONFIG_SYS_OSCIN_FREQ		24000000
>> +#define CONFIG_SYS_TIMERBASE		DAVINCI_TIMER0_BASE
>> +#define CONFIG_SYS_HZ_CLOCK		clk_get(DAVINCI_AUXCLK_CLKID)
> 
> This and the one above it using clk_get() does not look good.
> Is this how all davinci boards define it?

Yes.

>> +/*
>> + * Flash & Environment
>> + */
>> +#ifdef CONFIG_USE_NAND
>> +#define CONFIG_NAND_DAVINCI
>> +#define	CONFIG_SYS_NAND_USE_FLASH_BBT
>> +#define CONFIG_SYS_NAND_4BIT_HW_ECC_OOBFIRST
>> +#define	CONFIG_SYS_NAND_PAGE_2K
> 
> Alignment?

fixed.

[...]

Thanks for your comments!

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany