[U-Boot] [PATCH 2/2] iMX5: EfikaMX: Work-in-progress board support

[Stefano Babic <sbabic@denx.de>]

On 01/12/2011 11:46 PM, Marek Vasut wrote:
> Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> ---
>  board/efikamx/Makefile     |   52 ++++
>  board/efikamx/config.mk    |   25 ++
>  board/efikamx/efikamx.c    |  668 ++++++++++++++++++++++++++++++++++++++++++++
>  board/efikamx/imximage.cfg |  124 ++++++++
>  boards.cfg                 |    1 +
>  include/configs/efikamx.h  |  233 +++++++++++++++
>  6 files changed, 1103 insertions(+), 0 deletions(-)
>  create mode 100644 board/efikamx/Makefile
>  create mode 100644 board/efikamx/config.mk
>  create mode 100644 board/efikamx/efikamx.c
>  create mode 100644 board/efikamx/imximage.cfg
>  create mode 100644 include/configs/efikamx.h

Could you describe better which is the current status for this porting
and define what is already supported ? I mean, "work-in-progress" tells
me nothing about which peripherals are working and which not. Could you
add to your commit-id a short description, adding on which hardware
u-boot is running (on the development board ? on efika smarttotp ? on
efiga smartbook ? on all of them ?), and which peripherals are currently
supported.

You should add your name to the MAINTAINERS file, too.

> diff --git a/board/efikamx/efikamx.c b/board/efikamx/efikamx.c
> new file mode 100644
> index 0000000..10cf111
> +/******************************************************************************
> + * Shared variables / local defines
> + ******************************************************************************/
> +/* LED */
> +#define	EFIKAMX_LED_BLUE	0x1
> +#define	EFIKAMX_LED_GREEN	0x2
> +#define	EFIKAMX_LED_RED		0x4
> +
> +void efikamx_toggle_led(uint32_t mask);
> +
> +/* Board revisions */
> +#define	EFIKAMX_BOARD_REV_11	0x1
> +#define	EFIKAMX_BOARD_REV_12	0x2
> +#define	EFIKAMX_BOARD_REV_13	0x3
> +#define	EFIKAMX_BOARD_REV_14	0x4
> +
> +/******************************************************************************
> + * Board identification
> + ******************************************************************************/
> +static u32 board_rev;
> +
> +u32 get_efika_rev(void)
> +{
> +	u32 rev = 0;
> +	/*
> +	 * Retrieve board ID:
> +	 * 	rev1.1: 1,1,1
             ^---spaces instead of tabs, please fix globally

> +	 * 	rev1.2: 1,1,0
> +	 * 	rev1.3: 1,0,1
> +	 * 	rev1.4: 1,0,0
> +	 */
> +	mxc_request_iomux(MX51_PIN_NANDF_CS0, IOMUX_CONFIG_GPIO);
> +	mxc_gpio_direction(IOMUX_TO_GPIO(MX51_PIN_NANDF_CS0), MXC_GPIO_DIRECTION_OUT);

Line too long, please fix globally.

> +	mxc_request_iomux(MX51_PIN_NANDF_RB3, IOMUX_CONFIG_GPIO);
> +	mxc_iomux_set_pad(MX51_PIN_NANDF_RB3, PAD_CTL_100K_PU);
> +	mxc_gpio_direction(IOMUX_TO_GPIO(MX51_PIN_NANDF_RB3), MXC_GPIO_DIRECTION_IN);
> +	rev |= (!!mxc_gpio_get(IOMUX_TO_GPIO(MX51_PIN_NANDF_RB3))) << 2;

Is it ok to leave the NAND pins configured as GPIO, or should they set
back for the NFC controller ?

> +/******************************************************************************
> + * SPI configuration
> + ******************************************************************************/
> +#ifdef CONFIG_MXC_SPI

Is there a reason why CONFIG_MXC_SPI is not set ? As I understand, it is
required to set the PMIC to make things working. If CONFIG_MXC_SPI,
something wrong happens. I would prefer you drop the #ifdef, and instead
of that you add a check at the beginning of the file reporting a
compiler error if CONFIG_MXC_SPI is not set.

> +static void power_init(void)
> +{
> +	unsigned int val;
> +	unsigned int reg;
> +	struct mxc_ccm_reg *mxc_ccm = (struct mxc_ccm_reg *)MXC_CCM_BASE;
> +
> +	/* Write needed to Power Gate 2 register */
> +	val = pmic_reg_read(REG_POWER_MISC);
> +	val &= ~PWGT2SPIEN;
> +	pmic_reg_write(REG_POWER_MISC, val);
> +
> +	/* Externally powered */
> +	val = pmic_reg_read(REG_CHARGE);
> +	val |= ICHRG0 | ICHRG1 | ICHRG2 | ICHRG3 | CHGAUTOB;
> +	pmic_reg_write(REG_CHARGE, val);
> +
> +	/* power up the system first */
> +	pmic_reg_write(REG_POWER_MISC, PWUP);
> +
> +	/* NOTE: if (is_soc_rev(CHIP_REV_2_0) >= 0) */
> +
> +	/* Set core voltage to 1.1V */
> +	val = pmic_reg_read(REG_SW_0);
> +	val = (val & (~0x1F)) | 0x14;

Replace fix constants with defines, adding them to mc13892.h if they are
missing.

> +	pmic_reg_write(REG_SW_0, val);
> +
> +	/* Setup VCC (SW2) to 1.25 */
> +	val = pmic_reg_read(REG_SW_1);
> +	val = (val & (~0x1F)) | 0x1A;

Ditto

> +	/* Green LED */
> +	mxc_request_iomux(MX51_PIN_CSI1_VSYNC, IOMUX_CONFIG_ALT3);
> +	mxc_gpio_direction(IOMUX_TO_GPIO(MX51_PIN_CSI1_VSYNC), MXC_GPIO_DIRECTION_OUT);

Line to long

> +void efikamx_toggle_led(uint32_t mask)
> +{
> +	mxc_gpio_set(IOMUX_TO_GPIO(MX51_PIN_CSI1_D9), mask & EFIKAMX_LED_BLUE); 
    ^
    |--- trailing whitespaces

> +
> +	switch (__raw_readl(SRC_BASE_ADDR + 0x8)) {

We have a structure for this register,please use struct src from imx_regs.h

> +# Boot Device : one of
> +# spi, sd (the board has no nand neither onenand)
> +
> +# FIX XXX

Why fix ?


> diff --git a/include/configs/efikamx.h b/include/configs/efikamx.h
> new file mode 100644
> index 0000000..cb29bcd
> --- /dev/null
> +++ b/include/configs/efikamx.h
> @@ -0,0 +1,233 @@
> +/*
> + * Copyright (C) 2007, Guennadi Liakhovetski <lg@denx.de>
> + *
> + * (C) Copyright 2009 Freescale Semiconductor, Inc.
> + *
> + * Configuration settings for the MX51EVK Board
> + *
> + * 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
> + */
> +
> +#ifndef __CONFIG_H
> +#define __CONFIG_H
> +
> +#include <asm/arch/imx-regs.h>
> +#include <config_cmd_default.h>
> +
> +/*
> + * High Level Board Configuration Options
> + */
> +/* An i.MX51 CPU */
> +#define CONFIG_MX51

Move this define before including imx-regs.h. Patches for mx53evk are
currently merged in u-boot-imx, and the imx-regs.h file has an #ifdef
CONFIG_MX51 switch to define the different offsets for the two processors.

> +/*
> + * SPI Interface
> + */
> +#ifdef	CONFIG_CMD_SPI
> +
> +#define CONFIG_HARD_SPI			1
> +#define CONFIG_MXC_SPI			1
> +#define CONFIG_DEFAULT_SPI_BUS		1
> +#define CONFIG_DEFAULT_SPI_MODE		(SPI_MODE_0 | SPI_CS_HIGH)
> +
> +/* SPI FLASH */
> +#ifdef	CONFIG_CMD_SF
> +#define CONFIG_FSL_SF

I see, I let this useless CONFIG_ when I developped the vision2 board.
However, CONFIG_FSL_SF is not used at all in u-boot. It was a test I
made using a SPI flash driver from Freescale's code. You should drop it,
and I have to do the same for the vision2.

> +
> +#define CONFIG_SPI_FLASH
> +#define CONFIG_SPI_FLASH_SST
> +#define CONFIG_SPI_FLASH_CS		(1 | 121 << 8)

Is it possible that the efika use exactly the same gpio (GPIO4_25) as
the vision2 board, or it is only a cut&paste error ?

> +#define	__io

I think we have already discussed abot this define. Should it not move
to another file, such as imx-regs.h ? Or is there a better solution ?

Best regards
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================