On Thursday 13 January 2011 10:37:47 Stefano Babic wrote:
> On 01/12/2011 11:46 PM, Marek Vasut wrote:
> > Signed-off-by: Marek Vasut <marek.va...@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.
Will do, sry
>
> 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
Fixed in v2
>
> > + * 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.
DTTO
>
> > + 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 ?
I don't think there is even NAND used on efikamx. Though I might be mistaken.
>
> > +/***********************************************************************
> > ******* + * 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.
I added compile-time check. Fixed in v2
>
> > +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.
Is any sane datasheet for this thing available ? If so, can you link me to it
please?
>
> > + 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
Fixed in v2
>
> > +void efikamx_toggle_led(uint32_t mask)
> > +{
> > + mxc_gpio_set(IOMUX_TO_GPIO(MX51_PIN_CSI1_D9), mask & EFIKAMX_LED_BLUE);
>
> ^
>
> |--- trailing whitespaces
Fixed in v2
> >
> > +
> > + 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 ?
Ah, that's because I used SD card to boot it, it should officially boot from
SPI
nor. Fixed in v2
>
> > 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 <l...@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.
Fixed in v2
>
> > +/*
> > + * 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.
Fixed in v2
>
> > +
> > +#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 ?
seems to be the same.
>
> > +#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 ?
The thread where we discussed this got deaf. I tried asking further, but no
reply :-(
>
> Best regards
> Stefano Babic
Thanks for the review, it helped greatly.
Cheers
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
