[U-Boot] [PATCH] add initial support for bluegiga apx4devkit

[Veli-Pekka Peltola]

Hi Stefano,

Thanks for your review. Please see my comments below.

On 12/14/2011 03:20 PM, Stefano Babic wrote:
> On 13/12/2011 15:54, Veli-Pekka Peltola wrote:
>> Add initial support for Bluegiga APX4 CoM and development kit.
>>
>> Signed-off-by: Veli-Pekka Peltola<veli-pekka.peltola at bluegiga.com>
>> Cc: Stefano Babic<sbabic at denx.de>
>> ---
>
> Hi Veli-Pekka,
>
>>   board/bluegiga/apx4devkit/Makefile     |   45 +++++++
>>   board/bluegiga/apx4devkit/apx4devkit.c |  204 ++++++++++++++++++++++++++++++++
>>   boards.cfg                             |    1 +
>>   include/configs/apx4devkit.h           |  176 +++++++++++++++++++++++++++
>
> You should also update the MAINTAINERS file, adding your name as board
> maintainer.

Will be added to v2.

>>   4 files changed, 426 insertions(+), 0 deletions(-)
>>   create mode 100644 board/bluegiga/apx4devkit/Makefile
>>   create mode 100644 board/bluegiga/apx4devkit/apx4devkit.c
>>   create mode 100644 include/configs/apx4devkit.h
>>
>
> your board configuration file does not set any SPL constant. I cannot
> understand how it works. Only u-boot.bin can be built, and this is only
> a part of the system. What about the first boot loader (SPL) ?

Currently we are using a different strategy to load U-Boot from NAND 
than M28EVK for example. We have very small U-Boot placed to first 
sector in NAND to load this bigger version of U-Boot.

But now I understand that our way is far away from optimal so this will 
be reworked in v2.

> Please add also in the commit message which components (Ethernet, MMC,
> I2C, SPI,...) you support with your patch.

Will be added to v2 commit message.

>> +DECLARE_GLOBAL_DATA_PTR;
>> +
>> +#define MUX_CONFIG_GENERIC	(MXS_PAD_3V3 | MXS_PAD_8MA | MXS_PAD_PULLUP)
>> +#define MUX_CONFIG_GPMI	(MXS_PAD_3V3 | MXS_PAD_4MA | MXS_PAD_NOPULL)
>> +#define MUX_CONFIG_GPMI_12MA	(MXS_PAD_3V3 | MXS_PAD_12MA | MXS_PAD_NOPULL)
>> +
>> +const iomux_cfg_t iomux_setup[] = {
>> +	/* GPMI NAND */
>> +	MX28_PAD_GPMI_D00__GPMI_D0 | MUX_CONFIG_GPMI,
>> +	MX28_PAD_GPMI_D01__GPMI_D1 | MUX_CONFIG_GPMI,
>> +	MX28_PAD_GPMI_D02__GPMI_D2 | MUX_CONFIG_GPMI,
>> +	MX28_PAD_GPMI_D03__GPMI_D3 | MUX_CONFIG_GPMI,
>> +	MX28_PAD_GPMI_D04__GPMI_D4 | MUX_CONFIG_GPMI,
>> +	MX28_PAD_GPMI_D05__GPMI_D5 | MUX_CONFIG_GPMI,
>> +	MX28_PAD_GPMI_D06__GPMI_D6 | MUX_CONFIG_GPMI,
>> +	MX28_PAD_GPMI_D07__GPMI_D7 | MUX_CONFIG_GPMI,
>> +	MX28_PAD_GPMI_ALE__GPMI_ALE | MUX_CONFIG_GPMI,
>> +	MX28_PAD_GPMI_CLE__GPMI_CLE | MUX_CONFIG_GPMI,
>> +	MX28_PAD_GPMI_CE0N__GPMI_CE0N | MUX_CONFIG_GPMI,
>> +	MX28_PAD_GPMI_RDY0__GPMI_READY0 | MUX_CONFIG_GPMI,
>> +	MX28_PAD_GPMI_RDN__GPMI_RDN | MUX_CONFIG_GPMI_12MA,
>> +	MX28_PAD_GPMI_WRN__GPMI_WRN | MUX_CONFIG_GPMI_12MA,
>> +	MX28_PAD_GPMI_RESETN__GPMI_RESETN | MUX_CONFIG_GPMI_12MA,
>> +
>> +	/* MMC0 */
>> +	MX28_PAD_SSP0_DATA0__SSP0_D0 | MUX_CONFIG_GENERIC,
>> +	MX28_PAD_SSP0_DATA1__SSP0_D1 | MUX_CONFIG_GENERIC,
>> +	MX28_PAD_SSP0_DATA2__SSP0_D2 | MUX_CONFIG_GENERIC,
>> +	MX28_PAD_SSP0_DATA3__SSP0_D3 | MUX_CONFIG_GENERIC,
>> +	MX28_PAD_SSP0_DATA4__SSP0_D4 | MUX_CONFIG_GENERIC,
>> +	MX28_PAD_SSP0_DATA5__SSP0_D5 | MUX_CONFIG_GENERIC,
>> +	MX28_PAD_SSP0_DATA6__SSP0_D6 | MUX_CONFIG_GENERIC,
>> +	MX28_PAD_SSP0_DATA7__SSP0_D7 | MUX_CONFIG_GENERIC,
>> +	MX28_PAD_SSP0_CMD__SSP0_CMD | MUX_CONFIG_GENERIC,
>> +	MX28_PAD_SSP0_DETECT__SSP0_CARD_DETECT |
>> +		(MXS_PAD_3V3 | MXS_PAD_8MA | MXS_PAD_NOPULL),
>> +	MX28_PAD_SSP0_SCK__SSP0_SCK |
>> +		(MXS_PAD_3V3 | MXS_PAD_12MA | MXS_PAD_NOPULL),
>> +
>> +	/* FEC Ethernet */
>> +	MX28_PAD_ENET0_MDC__ENET0_MDC | MUX_CONFIG_GENERIC,
>> +	MX28_PAD_ENET0_MDIO__ENET0_MDIO | MUX_CONFIG_GENERIC,
>> +	MX28_PAD_ENET0_RX_EN__ENET0_RX_EN | MUX_CONFIG_GENERIC,
>> +	MX28_PAD_ENET0_TX_EN__ENET0_TX_EN | MUX_CONFIG_GENERIC,
>> +	MX28_PAD_ENET0_RXD0__ENET0_RXD0 | MUX_CONFIG_GENERIC,
>> +	MX28_PAD_ENET0_RXD1__ENET0_RXD1 | MUX_CONFIG_GENERIC,
>> +	MX28_PAD_ENET0_TXD0__ENET0_TXD0 | MUX_CONFIG_GENERIC,
>> +	MX28_PAD_ENET0_TXD1__ENET0_TXD1 | MUX_CONFIG_GENERIC,
>> +	MX28_PAD_ENET_CLK__CLKCTRL_ENET | MUX_CONFIG_GENERIC,
>> +};
>> +
>> +/*
>> + * Functions
>> + */
>> +int board_early_init_f(void)
>> +{
>> +	/* Setup only pads needed by U-Boot that are not initialized before */
>> +	mxs_iomux_setup_multiple_pads(iomux_setup, ARRAY_SIZE(iomux_setup));
>
> Mmmhh..die Pins are setup in the SPL code, checks in
> arch/arm/cpu/arm926ejs/mx28/spl_boot.c.
>
> You are adding a lot of pins that are maybe duplicated, are'nt they ?

After we switch to SPL, these will be moved/removed.

>> +#ifdef CONFIG_CMD_NET
>> +
>> +#define MII_PHY_CTRL2	0x1f
>> +
>> +int fecmxc_mii_postcall(int phy)
>> +{
>> +	miiphy_write("FEC", 0, MII_PHY_CTRL2, 0x8180);
>> +	return 0;
>> +}
>
> So you have only one ethernet interface - add also this info in your
> commit message.

Ok. I will also add a comment here why this is needed.

>> +void imx_get_mac_from_fuse(char *mac)
>> +{
>> +	memset(mac, 0, 6);
>> +}
>
> This is wrong, and surely you do not get the address from fuses.

What is correct way to get ethernet mac from environment instead of 
fuses? FEC driver seems to call imx_get_mac_from_fuse always. Should it 
be changed to use a new configuration option? Or should we try to read 
mac from fuses which are zeros and then fall back to environment?

>> +#ifdef CONFIG_SERIAL_TAG
>> +#define MXS_OCOTP_MAX_TIMEOUT	1000000
>> +void get_board_serial(struct tag_serialnr *serialnr)
>> +{
>> +	struct mx28_ocotp_regs *ocotp_regs =
>> +		(struct mx28_ocotp_regs *)MXS_OCOTP_BASE;
>> +
>> +	serialnr->high = 0;
>> +	serialnr->low = 0;
>> +
>> +	writel(OCOTP_CTRL_RD_BANK_OPEN,&ocotp_regs->hw_ocotp_ctrl_set);
>> +
>> +	if (mx28_wait_mask_clr(&ocotp_regs->hw_ocotp_ctrl_reg, OCOTP_CTRL_BUSY,
>> +				MXS_OCOTP_MAX_TIMEOUT)) {
>> +		printf("MXS: Can't get serial number from OCOTP\n");
>> +		return;
>> +	}
>
> I think we can factorize some code with m28evk, and putting code to
> access OCOTP in a common directory, such as arch/arm/cpu/arm926ejs/mx28.

Sounds like a good idea. I will do that.

>> +#ifdef CONFIG_REVISION_TAG
>> +u32 get_board_rev(void)
>> +{
>> +	if (getenv("revision#") != NULL)
>> +		return simple_strtoul(getenv("revision#"), NULL, 10);
>> +	return 0;
>> +}
>
> Do you get the revision from an environment and not from hardware ?

Yes, that's the plan. In the history we have used revision number to 
include information about integrated modem, wifi and so on.

> There is a big chance to have the wrong revision number...

Yes but even with empty environment the board will still boot up.

>> +/* Environment is in NAND */
>> +#define CONFIG_ENV_IS_IN_NAND
>> +#define CONFIG_ENV_SECT_SIZE		(128 * 1024)
>> +#define CONFIG_ENV_SIZE		(128 * 1024)
>> +#define CONFIG_ENV_SIZE_REDUND		CONFIG_ENV_SIZE
>> +#define CONFIG_ENV_OFFSET		0x120000
>> +#define CONFIG_ENV_OFFSET_REDUND	\
>> +		(CONFIG_ENV_OFFSET + CONFIG_ENV_SECT_SIZE)
>
> You set the environment in NAND, but you reserve only one sector for
> each of the two copies. If the single sector becomes bad, you lose the
> redundancy. Should you not have more as one sector for the environment ?

Statistically this has not been a problem so far. But I see the risk and 
I will increase environment size a little bit.

> Best regards,
> Stefano Babic

--
Veli-Pekka Peltola