[U-Boot,1/2] arm: mx6: add support for TBS2910 Matrix ARM miniPC

Add initial support for TBS2910 Matrix ARM miniPC.
Support includes MMC, Ethernet, UARTs, HDMI, USB, SATA, PCI, I2C, RTC.

Cc: Stefano Babic <sbabic@denx.de>
Signed-off-by: Soeren Moch <smoch@web.de>
---
 arch/arm/Kconfig            |   4 +
 board/tbs/tbs2910/Kconfig   |  23 +++
 board/tbs/tbs2910/Makefile  |   9 +
 board/tbs/tbs2910/tbs2910.c | 420 ++++++++++++++++++++++++++++++++++++++++++++
 configs/tbs2910_defconfig   |   3 +
 include/configs/tbs2910.h   | 238 +++++++++++++++++++++++++
 6 files changed, 697 insertions(+)
 create mode 100644 board/tbs/tbs2910/Kconfig
 create mode 100644 board/tbs/tbs2910/Makefile
 create mode 100644 board/tbs/tbs2910/tbs2910.c
 create mode 100644 configs/tbs2910_defconfig
 create mode 100644 include/configs/tbs2910.h

Hi Soeren,

On 18/10/2014 13:13, Soeren Moch wrote:
> Add initial support for TBS2910 Matrix ARM miniPC.
> Support includes MMC, Ethernet, UARTs, HDMI, USB, SATA, PCI, I2C, RTC.
> 
> Cc: Stefano Babic <sbabic@denx.de>
> Signed-off-by: Soeren Moch <smoch@web.de>
> ---

Just a couple of small questions:


>  arch/arm/Kconfig            |   4 +
>  board/tbs/tbs2910/Kconfig   |  23 +++
>  board/tbs/tbs2910/Makefile  |   9 +
>  board/tbs/tbs2910/tbs2910.c | 420 ++++++++++++++++++++++++++++++++++++++++++++
>  configs/tbs2910_defconfig   |   3 +
>  include/configs/tbs2910.h   | 238 +++++++++++++++++++++++++
>  6 files changed, 697 insertions(+)
>  create mode 100644 board/tbs/tbs2910/Kconfig
>  create mode 100644 board/tbs/tbs2910/Makefile
>  create mode 100644 board/tbs/tbs2910/tbs2910.c
>  create mode 100644 configs/tbs2910_defconfig
>  create mode 100644 include/configs/tbs2910.h
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 72558b8..cb72381 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -414,6 +414,9 @@ config TARGET_GW_VENTANA
>  config TARGET_HUMMINGBOARD
>  	bool "Support hummingboard"
>  
> +config TARGET_TBS2910
> +	bool "Support tbs2910"
> +
>  config TARGET_TQMA6
>  	bool "TQ Systems TQMa6 board"
>  
> @@ -682,6 +685,7 @@ source "board/sunxi/Kconfig"
>  source "board/syteco/jadecpu/Kconfig"
>  source "board/syteco/zmx25/Kconfig"
>  source "board/taskit/stamp9g20/Kconfig"
> +source "board/tbs/tbs2910/Kconfig"
>  source "board/ti/am335x/Kconfig"
>  source "board/ti/am43xx/Kconfig"
>  source "board/ti/ti814x/Kconfig"
> diff --git a/board/tbs/tbs2910/Kconfig b/board/tbs/tbs2910/Kconfig
> new file mode 100644
> index 0000000..c514e24
> --- /dev/null
> +++ b/board/tbs/tbs2910/Kconfig
> @@ -0,0 +1,23 @@
> +if TARGET_TBS2910
> +
> +config SYS_CPU
> +	string
> +	default "armv7"
> +
> +config SYS_BOARD
> +	string
> +	default "tbs2910"
> +
> +config SYS_VENDOR
> +	string
> +	default "tbs"
> +
> +config SYS_SOC
> +	string
> +	default "mx6"
> +
> +config SYS_CONFIG_NAME
> +	string
> +	default "tbs2910"
> +
> +endif
> diff --git a/board/tbs/tbs2910/Makefile b/board/tbs/tbs2910/Makefile
> new file mode 100644
> index 0000000..bf5f22b
> --- /dev/null
> +++ b/board/tbs/tbs2910/Makefile
> @@ -0,0 +1,9 @@
> +#
> +# Copyright (C) 2007, Guennadi Liakhovetski <lg@denx.de>
> +#
> +# (C) Copyright 2011 Freescale Semiconductor, Inc.
> +#
> +# SPDX-License-Identifier:	GPL-2.0+
> +#
> +
> +obj-y  := tbs2910.o
> diff --git a/board/tbs/tbs2910/tbs2910.c b/board/tbs/tbs2910/tbs2910.c
> new file mode 100644
> index 0000000..34cfd84
> --- /dev/null
> +++ b/board/tbs/tbs2910/tbs2910.c
> @@ -0,0 +1,420 @@
> +/*
> + * Copyright (C) 2012 Freescale Semiconductor, Inc.
> + * Author: Fabio Estevam <fabio.estevam@freescale.com>
> + *
> + * Copyright (C) 2014 Soeren Moch <smoch@web.de>
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +#include <asm/arch/clock.h>
> +#include <asm/arch/imx-regs.h>
> +#include <asm/arch/iomux.h>
> +#include <asm/arch/mx6-pins.h>
> +#include <asm/errno.h>
> +#include <asm/gpio.h>
> +#include <asm/imx-common/mxc_i2c.h>
> +#include <asm/imx-common/iomux-v3.h>
> +#include <asm/imx-common/sata.h>
> +#include <asm/imx-common/boot_mode.h>
> +#include <asm/imx-common/video.h>
> +#include <mmc.h>
> +#include <fsl_esdhc.h>
> +#include <miiphy.h>
> +#include <netdev.h>
> +#include <asm/arch/mxc_hdmi.h>
> +#include <asm/arch/crm_regs.h>
> +#include <asm/io.h>
> +#include <asm/arch/sys_proto.h>
> +#include <i2c.h>
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +#define WEAK_PULLUP	(PAD_CTL_PUS_47K_UP |			\
> +	PAD_CTL_SPEED_LOW | PAD_CTL_DSE_40ohm | PAD_CTL_HYS |	\
> +	PAD_CTL_SRE_SLOW)
> +
> +#define UART_PAD_CTRL  (PAD_CTL_PUS_100K_UP |			\
> +	PAD_CTL_SPEED_MED | PAD_CTL_DSE_40ohm |			\
> +	PAD_CTL_SRE_FAST  | PAD_CTL_HYS)
> +
> +#define USDHC_PAD_CTRL (PAD_CTL_PUS_47K_UP |			\
> +	PAD_CTL_SPEED_LOW | PAD_CTL_DSE_80ohm |			\
> +	PAD_CTL_SRE_FAST  | PAD_CTL_HYS)
> +
> +#define ENET_PAD_CTRL  (PAD_CTL_PUS_100K_UP |			\
> +	PAD_CTL_SPEED_MED | PAD_CTL_DSE_40ohm | PAD_CTL_HYS)
> +
> +#define I2C_PAD_CTRL  (PAD_CTL_PUS_100K_UP |			\
> +	PAD_CTL_SPEED_MED | PAD_CTL_DSE_40ohm | PAD_CTL_HYS |	\
> +	PAD_CTL_ODE | PAD_CTL_SRE_FAST)
> +
> +#define I2C_PAD MUX_PAD_CTRL(I2C_PAD_CTRL)
> +
> +#ifdef CONFIG_SYS_I2C
> +/* I2C1, SGTL5000 */
> +static struct i2c_pads_info i2c_pad_info0 = {
> +	.scl = {
> +		.i2c_mode = MX6_PAD_CSI0_DAT9__I2C1_SCL | I2C_PAD,
> +		.gpio_mode = MX6_PAD_CSI0_DAT9__GPIO5_IO27 | I2C_PAD,
> +		.gp = IMX_GPIO_NR(5, 27)
> +	},
> +	.sda = {
> +		.i2c_mode = MX6_PAD_CSI0_DAT8__I2C1_SDA | I2C_PAD,
> +		.gpio_mode = MX6_PAD_CSI0_DAT8__GPIO5_IO26 | I2C_PAD,
> +		.gp = IMX_GPIO_NR(5, 26)
> +	}
> +};
> +
> +/* I2C2 HDMI */
> +static struct i2c_pads_info i2c_pad_info1 = {
> +	.scl = {
> +		.i2c_mode = MX6_PAD_KEY_COL3__I2C2_SCL | I2C_PAD,
> +		.gpio_mode = MX6_PAD_KEY_COL3__GPIO4_IO12 | I2C_PAD,
> +		.gp = IMX_GPIO_NR(4, 12)
> +	},
> +	.sda = {
> +		.i2c_mode = MX6_PAD_KEY_ROW3__I2C2_SDA | I2C_PAD,
> +		.gpio_mode = MX6_PAD_KEY_ROW3__GPIO4_IO13 | I2C_PAD,
> +		.gp = IMX_GPIO_NR(4, 13)
> +	}
> +};
> +
> +/* I2C3, CON11, DS1307, PCIe_SMB */
> +static struct i2c_pads_info i2c_pad_info2 = {
> +	.scl = {
> +		.i2c_mode = MX6_PAD_GPIO_3__I2C3_SCL | I2C_PAD,
> +		.gpio_mode = MX6_PAD_GPIO_3__GPIO1_IO03 | I2C_PAD,
> +		.gp = IMX_GPIO_NR(1, 3)
> +	},
> +	.sda = {
> +		.i2c_mode = MX6_PAD_GPIO_6__I2C3_SDA | I2C_PAD,
> +		.gpio_mode = MX6_PAD_GPIO_6__GPIO1_IO06 | I2C_PAD,
> +		.gp = IMX_GPIO_NR(1, 6)
> +	}
> +};
> +#endif /* CONFIG_SYS_I2C */
> +
> +static iomux_v3_cfg_t const uart1_pads[] = {
> +	MX6_PAD_CSI0_DAT10__UART1_TX_DATA | MUX_PAD_CTRL(UART_PAD_CTRL),
> +	MX6_PAD_CSI0_DAT11__UART1_RX_DATA | MUX_PAD_CTRL(UART_PAD_CTRL),
> +};
> +
> +static iomux_v3_cfg_t const uart2_pads[] = {
> +	MX6_PAD_EIM_D26__UART2_TX_DATA | MUX_PAD_CTRL(UART_PAD_CTRL),
> +	MX6_PAD_EIM_D27__UART2_RX_DATA | MUX_PAD_CTRL(UART_PAD_CTRL),
> +};
> +
> +static iomux_v3_cfg_t const enet_pads[] = {
> +	MX6_PAD_ENET_MDIO__ENET_MDIO		| MUX_PAD_CTRL(ENET_PAD_CTRL),
> +	MX6_PAD_ENET_MDC__ENET_MDC		| MUX_PAD_CTRL(ENET_PAD_CTRL),
> +	MX6_PAD_RGMII_TXC__RGMII_TXC		| MUX_PAD_CTRL(ENET_PAD_CTRL),
> +	MX6_PAD_RGMII_TD0__RGMII_TD0		| MUX_PAD_CTRL(ENET_PAD_CTRL),
> +	MX6_PAD_RGMII_TD1__RGMII_TD1		| MUX_PAD_CTRL(ENET_PAD_CTRL),
> +	MX6_PAD_RGMII_TD2__RGMII_TD2		| MUX_PAD_CTRL(ENET_PAD_CTRL),
> +	MX6_PAD_RGMII_TD3__RGMII_TD3		| MUX_PAD_CTRL(ENET_PAD_CTRL),
> +	MX6_PAD_RGMII_TX_CTL__RGMII_TX_CTL	| MUX_PAD_CTRL(ENET_PAD_CTRL),
> +	MX6_PAD_ENET_REF_CLK__ENET_TX_CLK	| MUX_PAD_CTRL(ENET_PAD_CTRL),
> +	MX6_PAD_RGMII_RXC__RGMII_RXC		| MUX_PAD_CTRL(ENET_PAD_CTRL),
> +	MX6_PAD_RGMII_RD0__RGMII_RD0		| MUX_PAD_CTRL(ENET_PAD_CTRL),
> +	MX6_PAD_RGMII_RD1__RGMII_RD1		| MUX_PAD_CTRL(ENET_PAD_CTRL),
> +	MX6_PAD_RGMII_RD2__RGMII_RD2		| MUX_PAD_CTRL(ENET_PAD_CTRL),
> +	MX6_PAD_RGMII_RD3__RGMII_RD3		| MUX_PAD_CTRL(ENET_PAD_CTRL),
> +	MX6_PAD_RGMII_RX_CTL__RGMII_RX_CTL	| MUX_PAD_CTRL(ENET_PAD_CTRL),
> +	/* AR8035 PHY Reset */
> +	MX6_PAD_ENET_CRS_DV__GPIO1_IO25		| MUX_PAD_CTRL(NO_PAD_CTRL),
> +};
> +
> +static iomux_v3_cfg_t const pcie_pads[] = {
> +	MX6_PAD_KEY_COL4__GPIO4_IO14 | MUX_PAD_CTRL(WEAK_PULLUP),/* W_DISABLE# */
> +	MX6_PAD_GPIO_17__GPIO7_IO12 | MUX_PAD_CTRL(NO_PAD_CTRL), /* PERST#     */
> +};
> +
> +int dram_init(void)
> +{
> +  gd->ram_size = 2048ul * 1024 * 1024;
> +
> +	return 0;
> +}
> +
> +static void setup_iomux_enet(void)
> +{
> +	imx_iomux_v3_setup_multiple_pads(enet_pads, ARRAY_SIZE(enet_pads));
> +
> +	/* Reset AR8035 PHY */
> +	gpio_direction_output(IMX_GPIO_NR(1, 25) , 0);
> +	udelay(500);
> +	gpio_set_value(IMX_GPIO_NR(1, 25), 1);
> +}
> +
> +static void setup_pcie(void)
> +{
> +	imx_iomux_v3_setup_multiple_pads(pcie_pads, ARRAY_SIZE(pcie_pads));
> +}
> +
> +static void setup_iomux_uart(void)
> +{
> +	imx_iomux_v3_setup_multiple_pads(uart1_pads, ARRAY_SIZE(uart1_pads));
> +	imx_iomux_v3_setup_multiple_pads(uart2_pads, ARRAY_SIZE(uart2_pads));
> +}
> +
> +#ifdef CONFIG_FSL_ESDHC
> +static iomux_v3_cfg_t const usdhc2_pads[] = {
> +	MX6_PAD_SD2_CLK__SD2_CLK     | MUX_PAD_CTRL(USDHC_PAD_CTRL),
> +	MX6_PAD_SD2_CMD__SD2_CMD     | MUX_PAD_CTRL(USDHC_PAD_CTRL),
> +	MX6_PAD_SD2_DAT0__SD2_DATA0  | MUX_PAD_CTRL(USDHC_PAD_CTRL),
> +	MX6_PAD_SD2_DAT1__SD2_DATA1  | MUX_PAD_CTRL(USDHC_PAD_CTRL),
> +	MX6_PAD_SD2_DAT2__SD2_DATA2  | MUX_PAD_CTRL(USDHC_PAD_CTRL),
> +	MX6_PAD_SD2_DAT3__SD2_DATA3  | MUX_PAD_CTRL(USDHC_PAD_CTRL),
> +	MX6_PAD_NANDF_D2__GPIO2_IO02 | MUX_PAD_CTRL(NO_PAD_CTRL), /* CD */
> +};
> +
> +static iomux_v3_cfg_t const usdhc3_pads[] = {
> +	MX6_PAD_SD3_CLK__SD3_CLK     | MUX_PAD_CTRL(USDHC_PAD_CTRL),
> +	MX6_PAD_SD3_CMD__SD3_CMD     | MUX_PAD_CTRL(USDHC_PAD_CTRL),
> +	MX6_PAD_SD3_DAT0__SD3_DATA0  | MUX_PAD_CTRL(USDHC_PAD_CTRL),
> +	MX6_PAD_SD3_DAT1__SD3_DATA1  | MUX_PAD_CTRL(USDHC_PAD_CTRL),
> +	MX6_PAD_SD3_DAT2__SD3_DATA2  | MUX_PAD_CTRL(USDHC_PAD_CTRL),
> +	MX6_PAD_SD3_DAT3__SD3_DATA3  | MUX_PAD_CTRL(USDHC_PAD_CTRL),
> +	MX6_PAD_NANDF_D0__GPIO2_IO00 | MUX_PAD_CTRL(NO_PAD_CTRL), /* CD */
> +};
> +
> +static iomux_v3_cfg_t const usdhc4_pads[] = {
> +	MX6_PAD_SD4_CLK__SD4_CLK    | MUX_PAD_CTRL(USDHC_PAD_CTRL),
> +	MX6_PAD_SD4_CMD__SD4_CMD    | MUX_PAD_CTRL(USDHC_PAD_CTRL),
> +	MX6_PAD_SD4_DAT0__SD4_DATA0 | MUX_PAD_CTRL(USDHC_PAD_CTRL),
> +	MX6_PAD_SD4_DAT1__SD4_DATA1 | MUX_PAD_CTRL(USDHC_PAD_CTRL),
> +	MX6_PAD_SD4_DAT2__SD4_DATA2 | MUX_PAD_CTRL(USDHC_PAD_CTRL),
> +	MX6_PAD_SD4_DAT3__SD4_DATA3 | MUX_PAD_CTRL(USDHC_PAD_CTRL),
> +	MX6_PAD_SD4_DAT4__SD4_DATA4 | MUX_PAD_CTRL(USDHC_PAD_CTRL),
> +	MX6_PAD_SD4_DAT5__SD4_DATA5 | MUX_PAD_CTRL(USDHC_PAD_CTRL),
> +	MX6_PAD_SD4_DAT6__SD4_DATA6 | MUX_PAD_CTRL(USDHC_PAD_CTRL),
> +	MX6_PAD_SD4_DAT7__SD4_DATA7 | MUX_PAD_CTRL(USDHC_PAD_CTRL),
> +};
> +
> +static struct fsl_esdhc_cfg usdhc_cfg[3] = {
> +	{USDHC2_BASE_ADDR},
> +	{USDHC3_BASE_ADDR},
> +	{USDHC4_BASE_ADDR},
> +};
> +
> +#define USDHC2_CD_GPIO	IMX_GPIO_NR(2, 2)
> +#define USDHC3_CD_GPIO	IMX_GPIO_NR(2, 0)
> +
> +int board_mmc_getcd(struct mmc *mmc)
> +{
> +	struct fsl_esdhc_cfg *cfg = (struct fsl_esdhc_cfg *)mmc->priv;
> +	int ret = 0;
> +
> +	switch (cfg->esdhc_base) {
> +	case USDHC2_BASE_ADDR:
> +		ret = !gpio_get_value(USDHC2_CD_GPIO);
> +		break;
> +	case USDHC3_BASE_ADDR:
> +		ret = !gpio_get_value(USDHC3_CD_GPIO);
> +		break;
> +	case USDHC4_BASE_ADDR:
> +		ret = 1; /* eMMC/uSDHC4 is always present */
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +int board_mmc_init(bd_t *bis)
> +{
> +	s32 status = 0;
> +	int i;
> +
> +	/*
> +	 * According to the board_mmc_init() the following map is done:
> +	 * (U-boot device node)    (Physical Port)
> +	 * mmc0                    SD2
> +	 * mmc1                    SD3
> +	 * mmc2                    eMMC
> +	 */
> +	for (i = 0; i < CONFIG_SYS_FSL_USDHC_NUM; i++) {
> +		switch (i) {
> +		case 0:
> +			imx_iomux_v3_setup_multiple_pads(
> +				usdhc2_pads, ARRAY_SIZE(usdhc2_pads));
> +			gpio_direction_input(USDHC2_CD_GPIO);
> +			usdhc_cfg[0].sdhc_clk = mxc_get_clock(MXC_ESDHC2_CLK);
> +			break;
> +		case 1:
> +			imx_iomux_v3_setup_multiple_pads(
> +				usdhc3_pads, ARRAY_SIZE(usdhc3_pads));
> +			gpio_direction_input(USDHC3_CD_GPIO);
> +			usdhc_cfg[1].sdhc_clk = mxc_get_clock(MXC_ESDHC3_CLK);
> +			break;
> +		case 2:
> +			imx_iomux_v3_setup_multiple_pads(
> +				usdhc4_pads, ARRAY_SIZE(usdhc4_pads));
> +			usdhc_cfg[2].sdhc_clk = mxc_get_clock(MXC_ESDHC4_CLK);
> +			break;
> +		default:
> +			printf("Warning: you configured more USDHC controllers"
> +			       "(%d) then supported by the board (%d)\n",
> +			       i + 1, CONFIG_SYS_FSL_USDHC_NUM);
> +			return status;
> +		}
> +
> +		status |= fsl_esdhc_initialize(bis, &usdhc_cfg[i]);
> +	}
> +
> +	return status;
> +}
> +#endif /* CONFIG_FSL_ESDHC */
> +
> +static int mx6_rgmii_rework(struct phy_device *phydev)
> +{
> +	unsigned short val;
> +
> +	/* To enable AR8035 ouput a 125MHz clk from CLK_25M */
> +	phy_write(phydev, MDIO_DEVAD_NONE, 0xd, 0x7);
> +	phy_write(phydev, MDIO_DEVAD_NONE, 0xe, 0x8016);
> +	phy_write(phydev, MDIO_DEVAD_NONE, 0xd, 0x4007);
> +
> +	val = phy_read(phydev, MDIO_DEVAD_NONE, 0xe);
> +	val &= 0xffe3;
> +	val |= 0x18;
> +	phy_write(phydev, MDIO_DEVAD_NONE, 0xe, val);
> +
> +	/* introduce tx clock delay */
> +	phy_write(phydev, MDIO_DEVAD_NONE, 0x1d, 0x5);
> +	val = phy_read(phydev, MDIO_DEVAD_NONE, 0x1e);
> +	val |= 0x0100;
> +	phy_write(phydev, MDIO_DEVAD_NONE, 0x1e, val);
> +
> +	return 0;

I miss the point - which is the difference with ar8035_config() in
drivers/net/phy/atheros.c and why cnnot you reuse it ? Can we avoid to
duplicate the code ?

> +}
> +
> +int board_phy_config(struct phy_device *phydev)
> +{
> +	mx6_rgmii_rework(phydev);
> +
> +	if (phydev->drv->config)
> +		phydev->drv->config(phydev);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_VIDEO_IPUV3
> +static void do_enable_hdmi(struct display_info_t const *dev)
> +{
> +	imx_enable_hdmi_phy();
> +}
> +
> +struct display_info_t const displays[] = {{
> +	.bus	= -1,
> +	.addr	= 0,
> +	.pixfmt	= IPU_PIX_FMT_RGB24,
> +	.detect	= detect_hdmi,
> +	.enable	= do_enable_hdmi,
> +	.mode	= {
> +		.name           = "HDMI",
> +		.refresh        = 60,
> +		.xres           = 1024,
> +		.yres           = 768,
> +		.pixclock       = 15385,
> +		.left_margin    = 220,
> +		.right_margin   = 40,
> +		.upper_margin   = 21,
> +		.lower_margin   = 7,
> +		.hsync_len      = 60,
> +		.vsync_len      = 10,
> +		.sync           = FB_SYNC_EXT,
> +		.vmode          = FB_VMODE_NONINTERLACED
> +} } };
> +size_t display_count = ARRAY_SIZE(displays);
> +
> +static void setup_display(void)
> +{
> +	struct mxc_ccm_reg *mxc_ccm = (struct mxc_ccm_reg *)CCM_BASE_ADDR;
> +	int reg;
> +
> +	enable_ipu_clock();
> +	imx_setup_hdmi();
> +
> +	/* Turn on LDB0, LDB1, IPU,IPU DI0 clocks */
> +	reg = readl(&mxc_ccm->CCGR3);
> +	reg |=  MXC_CCM_CCGR3_LDB_DI0_MASK | MXC_CCM_CCGR3_LDB_DI1_MASK;
> +	writel(reg, &mxc_ccm->CCGR3);
> +
> +	/* set LDB0, LDB1 clk select to 011/011 */
> +	reg = readl(&mxc_ccm->cs2cdr);
> +	reg &= ~(MXC_CCM_CS2CDR_LDB_DI0_CLK_SEL_MASK
> +		 | MXC_CCM_CS2CDR_LDB_DI1_CLK_SEL_MASK);
> +	reg |= (3 << MXC_CCM_CS2CDR_LDB_DI0_CLK_SEL_OFFSET)
> +	      | (3 << MXC_CCM_CS2CDR_LDB_DI1_CLK_SEL_OFFSET);
> +	writel(reg, &mxc_ccm->cs2cdr);
> +
> +	reg = readl(&mxc_ccm->cscmr2);
> +	reg |= MXC_CCM_CSCMR2_LDB_DI0_IPU_DIV | MXC_CCM_CSCMR2_LDB_DI1_IPU_DIV;
> +	writel(reg, &mxc_ccm->cscmr2);
> +
> +	reg = readl(&mxc_ccm->chsccdr);
> +	reg |= (CHSCCDR_CLK_SEL_LDB_DI0
> +		<< MXC_CCM_CHSCCDR_IPU1_DI0_CLK_SEL_OFFSET);
> +	reg |= (CHSCCDR_CLK_SEL_LDB_DI0
> +		<< MXC_CCM_CHSCCDR_IPU1_DI1_CLK_SEL_OFFSET);
> +	writel(reg, &mxc_ccm->chsccdr);
> +}

This is also copied from sabresd. Can we factorize in some way ?

> +#endif /* CONFIG_VIDEO_IPUV3 */
> +
> +int board_eth_init(bd_t *bis)
> +{
> +	setup_iomux_enet();
> +	setup_pcie();
> +
> +	return cpu_eth_init(bis);
> +}
> +
> +int board_early_init_f(void)
> +{
> +	setup_iomux_uart();
> +#ifdef CONFIG_VIDEO_IPUV3
> +	setup_display();

I do not understand why setup_display() should be called at this point.
Generally, board_early_init_f() is called to setup iomux for peripherals
needed before relocation, as uart, letting the rest of the setup in
board_init(). Why do you need here ?

> +#endif
> +	return 0;
> +}
> +
> +int board_init(void)
> +{
> +	/* address of boot parameters */
> +	gd->bd->bi_boot_params = CONFIG_SYS_SDRAM_BASE + 0x100;
> +
> +#ifdef CONFIG_SYS_I2C
> +	setup_i2c(0, CONFIG_SYS_I2C_SPEED, 0x7f, &i2c_pad_info0);
> +	setup_i2c(1, CONFIG_SYS_I2C_SPEED, 0x7f, &i2c_pad_info1);
> +	setup_i2c(2, CONFIG_SYS_I2C_SPEED, 0x7f, &i2c_pad_info2);
> +#endif
> +#ifdef CONFIG_DWC_AHSATA
> +	setup_sata();
> +#endif
> +	return 0;
> +}
> +
> +#ifdef CONFIG_CMD_BMODE
> +static const struct boot_mode board_boot_modes[] = {
> +	/* 4 bit bus width */
> +	{"sd2",	 MAKE_CFGVAL(0x40, 0x28, 0x00, 0x00)},
> +	{"sd3",	 MAKE_CFGVAL(0x40, 0x30, 0x00, 0x00)},
> +	/* 8 bit bus width */
> +	{"emmc", MAKE_CFGVAL(0x40, 0x38, 0x00, 0x00)},
> +	{NULL,	 0},
> +};
> +#endif
> +
> +int board_late_init(void)
> +{
> +#ifdef CONFIG_CMD_BMODE
> +	add_board_boot_modes(board_boot_modes);
> +#endif
> +	return 0;
> +}
> +
> +int checkboard(void)
> +{
> +	puts("Board: TBS Matrix ARM mini PC\n");
> +	return 0;
> +}
> diff --git a/configs/tbs2910_defconfig b/configs/tbs2910_defconfig
> new file mode 100644
> index 0000000..602d691
> --- /dev/null
> +++ b/configs/tbs2910_defconfig
> @@ -0,0 +1,3 @@
> +CONFIG_SYS_EXTRA_OPTIONS="IMX_CONFIG=board/boundary/nitrogen6x/nitrogen6q2g.cfg,MX6Q"

Be aware that changes in the ddr-setup in that file will have a
consequence on your board.

> index 0000000..364f5b2
> --- /dev/null
> +++ b/include/configs/tbs2910.h
> @@ -0,0 +1,238 @@
> +/*
> + * Copyright (C) 2013 Freescale Semiconductor, Inc
> + * Copyright (C) 2014 Soeren Moch <smoch@web.de>
> + *
> + * Configuration settings for the TBS2910 MatrixARM board (Version 2.1).
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +#ifndef __TBS2910_CONFIG_H
> +#define __TBS2910_CONFIG_H
> +
> +#include "mx6_common.h"
> +#include <asm/arch/imx-regs.h>
> +#include <asm/imx-common/gpio.h>
> +
> +/* General configuration */
> +#define CONFIG_MX6
> +#define CONFIG_DISPLAY_CPUINFO
> +#define CONFIG_DISPLAY_BOARDINFO_LATE
> +#define CONFIG_SYS_THUMB_BUILD
> +
> +#define CONFIG_MACH_TYPE		3980
> +
> +#define CONFIG_CMDLINE_TAG
> +#define CONFIG_SETUP_MEMORY_TAGS
> +#define CONFIG_INITRD_TAG
> +#define CONFIG_REVISION_TAG
> +#define CONFIG_SYS_GENERIC_BOARD
> +
> +#define CONFIG_BOARD_EARLY_INIT_F
> +#define CONFIG_BOARD_LATE_INIT
> +#define CONFIG_MXC_GPIO
> +#define CONFIG_CMD_GPIO
> +
> +#define CONFIG_SYS_LONGHELP
> +#define CONFIG_SYS_HUSH_PARSER
> +#define CONFIG_SYS_PROMPT		"Matrix U-Boot> "
> +#define CONFIG_BOOTDELAY		3
> +#define CONFIG_AUTO_COMPLETE
> +#define CONFIG_CMDLINE_EDITING
> +#define CONFIG_SYS_MAXARGS		16
> +#define CONFIG_SYS_CBSIZE		1024
> +#define CONFIG_SYS_PBSIZE		(CONFIG_SYS_CBSIZE + sizeof(CONFIG_SYS_PROMPT) + 16)
> +#define CONFIG_SYS_HZ			1000
> +
> +/* Physical Memory Map */
> +#define CONFIG_NR_DRAM_BANKS		1
> +#define CONFIG_SYS_SDRAM_BASE		MMDC0_ARB_BASE_ADDR
> +
> +#define CONFIG_SYS_INIT_RAM_ADDR	IRAM_BASE_ADDR
> +#define CONFIG_SYS_INIT_RAM_SIZE	IRAM_SIZE
> +#define CONFIG_SYS_INIT_SP_OFFSET \
> +	(CONFIG_SYS_INIT_RAM_SIZE - GENERATED_GBL_DATA_SIZE)
> +#define CONFIG_SYS_INIT_SP_ADDR \
> +	(CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_INIT_SP_OFFSET)
> +
> +#define CONFIG_SYS_MALLOC_LEN		(128 * 1024 * 1024)
> +
> +#define CONFIG_SYS_MEMTEST_START	CONFIG_SYS_SDRAM_BASE
> +#define CONFIG_SYS_MEMTEST_END		(CONFIG_SYS_MEMTEST_START + 500 * 1024 * 1024)
> +
> +#define CONFIG_SYS_TEXT_BASE		0x80000000
> +#define CONFIG_SYS_BOOTMAPSZ		0x6C000000
> +#define CONFIG_SYS_LOAD_ADDR		0x10800000
> +
> +/* Serial console */
> +#define CONFIG_MXC_UART
> +#define CONFIG_MXC_UART_BASE		UART1_BASE /* select UART1/UART2 */
> +#define CONFIG_BAUDRATE			115200
> +
> +#define CONFIG_SYS_CONSOLE_IS_IN_ENV
> +#define CONFIG_CONSOLE_MUX
> +#define CONFIG_CONS_INDEX		1
> +
> +/* *** Command definition *** */
> +#include <config_cmd_default.h>
> +
> +#undef CONFIG_CMD_IMLS
> +
> +#define CONFIG_CMD_BMODE
> +#define CONFIG_CMD_SETEXPR
> +#define CONFIG_CMD_MEMTEST
> +#define CONFIG_CMD_TIME
> +
> +/* Filesystems / image support */
> +#define CONFIG_CMD_EXT4
> +#define CONFIG_CMD_FAT
> +#define CONFIG_DOS_PARTITION
> +#define CONFIG_EFI_PARTITION
> +#define CONFIG_CMD_FS_GENERIC
> +
> +#define CONFIG_OF_LIBFDT
> +#define CONFIG_CMD_BOOTZ
> +#define CONFIG_SUPPORT_RAW_INITRD
> +#define CONFIG_FIT
> +
> +/* MMC */
> +#define CONFIG_FSL_ESDHC
> +#define CONFIG_FSL_USDHC
> +#define CONFIG_SYS_FSL_USDHC_NUM	3
> +#define CONFIG_SYS_FSL_ESDHC_ADDR	USDHC4_BASE_ADDR
> +
> +#define CONFIG_MMC
> +#define CONFIG_CMD_MMC
> +#define CONFIG_GENERIC_MMC
> +#define CONFIG_BOUNCE_BUFFER
> +
> +/* Ethernet */
> +#define CONFIG_FEC_MXC
> +#define CONFIG_CMD_PING
> +#define CONFIG_CMD_DHCP
> +#define CONFIG_CMD_MII
> +#define CONFIG_CMD_NET
> +#define CONFIG_FEC_MXC
> +#define CONFIG_MII
> +#define IMX_FEC_BASE			ENET_BASE_ADDR
> +#define CONFIG_FEC_XCV_TYPE		RGMII
> +#define CONFIG_ETHPRIME			"FEC"
> +#define CONFIG_FEC_MXC_PHYADDR		4	
> +#define CONFIG_PHYLIB
> +#define CONFIG_PHY_ATHEROS
> +
> +/* Framebuffer */
> +#define CONFIG_VIDEO
> +#ifdef CONFIG_VIDEO
> +#define CONFIG_VIDEO_IPUV3
> +#define CONFIG_IPUV3_CLK		260000000
> +#define CONFIG_CFB_CONSOLE
> +#define CONFIG_CFB_CONSOLE_ANSI
> +#define CONFIG_VIDEO_SW_CURSOR
> +#define CONFIG_VGA_AS_SINGLE_DEVICE
> +#define CONFIG_VIDEO_BMP_RLE8
> +#define CONFIG_IMX_HDMI
> +#define CONFIG_IMX_VIDEO_SKIP
> +#define CONFIG_CMD_HDMIDETECT
> +#endif
> +
> +/* PCI */
> +#define CONFIG_CMD_PCI
> +#ifdef CONFIG_CMD_PCI
> +#define CONFIG_PCI
> +#define CONFIG_PCI_PNP
> +#define CONFIG_PCI_SCAN_SHOW
> +#define CONFIG_PCIE_IMX
> +#define CONFIG_PCIE_IMX_PERST_GPIO	IMX_GPIO_NR(7, 12)
> +#endif
> +
> +/* SATA */
> +#define CONFIG_CMD_SATA
> +#ifdef CONFIG_CMD_SATA
> +#define CONFIG_DWC_AHSATA
> +#define CONFIG_SYS_SATA_MAX_DEVICE	1
> +#define CONFIG_DWC_AHSATA_PORT_ID	0
> +#define CONFIG_DWC_AHSATA_BASE_ADDR	SATA_ARB_BASE_ADDR
> +#define CONFIG_LBA48
> +#define CONFIG_LIBATA
> +#endif
> +
> +/* USB */
> +#define CONFIG_CMD_USB
> +#ifdef CONFIG_CMD_USB
> +#define CONFIG_USB_EHCI
> +#define CONFIG_USB_EHCI_MX6
> +#define CONFIG_USB_MAX_CONTROLLER_COUNT 2
> +#define CONFIG_MXC_USB_PORTSC		(PORT_PTS_UTMI | PORT_PTS_PTW)
> +#define CONFIG_USB_STORAGE
> +#define CONFIG_USB_KEYBOARD
> +#ifdef CONFIG_USB_KEYBOARD
> +#define CONFIG_SYS_USB_EVENT_POLL
> +#define CONFIG_SYS_STDIO_DEREGISTER
> +#define CONFIG_PREBOOT "if hdmidet; then usb start; fi"
> +#endif /* CONFIG_USB_KEYBOARD */
> +#endif /* CONFIG_CMD_USB      */
> +
> +/* RTC */
> +#define CONFIG_CMD_DATE
> +#ifdef CONFIG_CMD_DATE
> +#define CONFIG_CMD_I2C
> +#define CONFIG_RTC_DS1307
> +#define CONFIG_SYS_RTC_BUS_NUM		2
> +#endif
> +
> +/* I2C */
> +#define CONFIG_CMD_I2C
> +#ifdef CONFIG_CMD_I2C
> +#define CONFIG_SYS_I2C
> +#define CONFIG_SYS_I2C_MXC
> +#define CONFIG_SYS_I2C_SPEED		100000
> +#define CONFIG_I2C_EDID
> +#endif
> +
> +/* Fuses */
> +#define CONFIG_CMD_FUSE
> +#ifdef CONFIG_CMD_FUSE
> +#define CONFIG_MXC_OCOTP
> +#endif
> +
> +#ifndef CONFIG_SYS_DCACHE_OFF
> +#define CONFIG_CMD_CACHE
> +#endif
> +
> +/* Flash and environment organization */
> +#define CONFIG_SYS_NO_FLASH
> +
> +#define CONFIG_ENV_IS_IN_MMC
> +#define CONFIG_SYS_MMC_ENV_DEV		2
> +#define CONFIG_SYS_MMC_ENV_PART		1
> +#define CONFIG_ENV_SIZE			(8 * 1024)
> +#define CONFIG_ENV_OFFSET		(384 * 1024)
> +#define CONFIG_ENV_OVERWRITE
> +
> +#define CONFIG_EXTRA_ENV_SETTINGS \
> +        "bootargs_mmc1=console=ttymxc0,115200 di0_primary console=tty1\0" \
> +        "bootargs_mmc2=video=mxcfb0:dev=hdmi,1920x1080M@60 video=mxcfb1:off video=mxcfb2:off fbmem=28M\0" \
> +        "bootargs_mmc3=root=/dev/mmcblk0p1 rootwait consoleblank=0 quiet\0" \
> +        "bootargs_mmc=setenv bootargs ${bootargs_mmc1} ${bootargs_mmc2} ${bootargs_mmc3}\0" \
> +        "bootargs_upd=setenv bootargs console=ttymxc0,115200 rdinit=/sbin/init enable_wait_mode=off\0" \
> +        "bootcmd_mmc=run bootargs_mmc; mmc dev 2; mmc read 0x10800000 0x800 0x4000; bootm\0" \
> +        "bootcmd_up1="	"load mmc 1 0x10800000 uImage\0" \
> +        "bootcmd_up2="	"load mmc 1 0x10d00000 uramdisk.img; " \
> +			"run bootargs_upd; " \
> +			"bootm 0x10800000 0x10d00000\0" \
> +        "fan=gpio set 92\0" \
> +        "console=ttymxc0\0" \
> +        "stdin=serial,usbkbd\0" \
> +        "stdout=serial,vga\0" \
> +        "stderr=serial,vga\0"
> +
> +#define CONFIG_BOOTCOMMAND \
> +	"mmc rescan; " \
> +	"if run bootcmd_up1; then " \
> +		"run bootcmd_up2; " \
> +	"else " \
> +		"run bootcmd_mmc; " \
> +	"fi"
> +
> +#endif			       /* __TBS2910_CONFIG_H * */
> 

Best regards,
Stefano Babic

Hi Stefano,

Thanks for your review.

>> +static int mx6_rgmii_rework(struct phy_device *phydev)
>> +{
>> +	unsigned short val;
>> +
>> +	/* To enable AR8035 ouput a 125MHz clk from CLK_25M */
>> +	phy_write(phydev, MDIO_DEVAD_NONE, 0xd, 0x7);
>> +	phy_write(phydev, MDIO_DEVAD_NONE, 0xe, 0x8016);
>> +	phy_write(phydev, MDIO_DEVAD_NONE, 0xd, 0x4007);
>> +
>> +	val = phy_read(phydev, MDIO_DEVAD_NONE, 0xe);
>> +	val &= 0xffe3;
>> +	val |= 0x18;
>> +	phy_write(phydev, MDIO_DEVAD_NONE, 0xe, val);
>> +
>> +	/* introduce tx clock delay */
>> +	phy_write(phydev, MDIO_DEVAD_NONE, 0x1d, 0x5);
>> +	val = phy_read(phydev, MDIO_DEVAD_NONE, 0x1e);
>> +	val |= 0x0100;
>> +	phy_write(phydev, MDIO_DEVAD_NONE, 0x1e, val);
>> +
>> +	return 0;
>
> I miss the point - which is the difference with ar8035_config() in
> drivers/net/phy/atheros.c and why cnnot you reuse it ? Can we avoid to
> duplicate the code ?

OK, I will remove this code.

>> +static void setup_display(void)
>> +{
>> +	struct mxc_ccm_reg *mxc_ccm = (struct mxc_ccm_reg *)CCM_BASE_ADDR;
>> +	int reg;
>> +
>> +	enable_ipu_clock();
>> +	imx_setup_hdmi();
>> +
>> +	/* Turn on LDB0, LDB1, IPU,IPU DI0 clocks */
>> +	reg = readl(&mxc_ccm->CCGR3);
>> +	reg |=  MXC_CCM_CCGR3_LDB_DI0_MASK | MXC_CCM_CCGR3_LDB_DI1_MASK;
>> +	writel(reg, &mxc_ccm->CCGR3);
>> +
>> +	/* set LDB0, LDB1 clk select to 011/011 */
>> +	reg = readl(&mxc_ccm->cs2cdr);
>> +	reg &= ~(MXC_CCM_CS2CDR_LDB_DI0_CLK_SEL_MASK
>> +		 | MXC_CCM_CS2CDR_LDB_DI1_CLK_SEL_MASK);
>> +	reg |= (3 << MXC_CCM_CS2CDR_LDB_DI0_CLK_SEL_OFFSET)
>> +	      | (3 << MXC_CCM_CS2CDR_LDB_DI1_CLK_SEL_OFFSET);
>> +	writel(reg, &mxc_ccm->cs2cdr);
>> +
>> +	reg = readl(&mxc_ccm->cscmr2);
>> +	reg |= MXC_CCM_CSCMR2_LDB_DI0_IPU_DIV | MXC_CCM_CSCMR2_LDB_DI1_IPU_DIV;
>> +	writel(reg, &mxc_ccm->cscmr2);
>> +
>> +	reg = readl(&mxc_ccm->chsccdr);
>> +	reg |= (CHSCCDR_CLK_SEL_LDB_DI0
>> +		<< MXC_CCM_CHSCCDR_IPU1_DI0_CLK_SEL_OFFSET);
>> +	reg |= (CHSCCDR_CLK_SEL_LDB_DI0
>> +		<< MXC_CCM_CHSCCDR_IPU1_DI1_CLK_SEL_OFFSET);
>> +	writel(reg, &mxc_ccm->chsccdr);
>> +}
>
> This is also copied from sabresd. Can we factorize in some way ?

I can, and probably should, simplify this code. But in fact this code
is wrong, and in the same way for many imx6q boards (e.g. sabresd,
wandboard, nitrogen6x,...).

This code configures the external video clock (LDB_DIx clock). This
clock is hardcoded to have 65MHz in drivers/video/ipu_common.c:ldb_clk.
But in fact the clock rate is configured to 75.42MHz (528MHz/7) on all
boards. So the display does not show 1024x768@60Hz as configured, but
something similar to 1024x768@70Hz (not VESA compliant), which much
monitors can handle, but on other monitors there are problems.
(one out of tree monitors works for me).

My intention was to get this initial support for tbs2910 merged with the
(wrong) code from sabresd used as template, and later to discuss how to
cleanup this code.

Do you prefer a simplified version of this code for the initial patch?
Or should we aim for a fixed video clock in the first place?

>> +#endif /* CONFIG_VIDEO_IPUV3 */
>> +
>> +int board_eth_init(bd_t *bis)
>> +{
>> +	setup_iomux_enet();
>> +	setup_pcie();
>> +
>> +	return cpu_eth_init(bis);
>> +}
>> +
>> +int board_early_init_f(void)
>> +{
>> +	setup_iomux_uart();
>> +#ifdef CONFIG_VIDEO_IPUV3
>> +	setup_display();
>
> I do not understand why setup_display() should be called at this point.
> Generally, board_early_init_f() is called to setup iomux for peripherals
> needed before relocation, as uart, letting the rest of the setup in
> board_init(). Why do you need here ?

In fact this was also copied from sabresd/wandboard/nitrogen6x.
My assumption was, that the clocks must be configured before the
ipu is initialized.

>> diff --git a/configs/tbs2910_defconfig b/configs/tbs2910_defconfig
>> new file mode 100644
>> index 0000000..602d691
>> --- /dev/null
>> +++ b/configs/tbs2910_defconfig
>> @@ -0,0 +1,3 @@
>> +CONFIG_SYS_EXTRA_OPTIONS="IMX_CONFIG=board/boundary/nitrogen6x/nitrogen6q2g.cfg,MX6Q"
>
> Be aware that changes in the ddr-setup in that file will have a
> consequence on your board.

Yes, I'm aware of that. But nitrogen6x is very well maintained. So I see no need to
duplicate this config and reuse it in the same way as wandboard does.

Best regards,
Soeren Moch

Hi Soeren,

On 21/10/2014 19:54, Soeren Moch wrote:
>>
>> This is also copied from sabresd. Can we factorize in some way ?
> 
> I can, and probably should, simplify this code. But in fact this code
> is wrong, and in the same way for many imx6q boards (e.g. sabresd,
> wandboard, nitrogen6x,...).

That makes things only worse. Duplicating the code, we have an
additional board with bad / wrong code.

> 
> This code configures the external video clock (LDB_DIx clock). This
> clock is hardcoded to have 65MHz in drivers/video/ipu_common.c:ldb_clk.
> But in fact the clock rate is configured to 75.42MHz (528MHz/7) on all
> boards. So the display does not show 1024x768@60Hz as configured, but
> something similar to 1024x768@70Hz (not VESA compliant), which much
> monitors can handle, but on other monitors there are problems.
> (one out of tree monitors works for me).

Ok, understood.

> 
> My intention was to get this initial support for tbs2910 merged with the
> (wrong) code from sabresd used as template, and later to discuss how to
> cleanup this code.
> 
> Do you prefer a simplified version of this code for the initial patch?

Maybe this is the best approach: you can at the beginning drop support
for video, an let your board be merged without wrong code. Then we can
discuss about fixing the wrong clock as shared code, letting that all
boards take advantage for that.

> 
>>> +#endif /* CONFIG_VIDEO_IPUV3 */
>>> +
>>> +int board_eth_init(bd_t *bis)
>>> +{
>>> +    setup_iomux_enet();
>>> +    setup_pcie();
>>> +
>>> +    return cpu_eth_init(bis);
>>> +}
>>> +
>>> +int board_early_init_f(void)
>>> +{
>>> +    setup_iomux_uart();
>>> +#ifdef CONFIG_VIDEO_IPUV3
>>> +    setup_display();
>>
>> I do not understand why setup_display() should be called at this point.
>> Generally, board_early_init_f() is called to setup iomux for peripherals
>> needed before relocation, as uart, letting the rest of the setup in
>> board_init(). Why do you need here ?
> 
> In fact this was also copied from sabresd/wandboard/nitrogen6x.
> My assumption was, that the clocks must be configured before the
> ipu is initialized.

That is correct, but ipu is initialized by video_init() after
board_init() is called. Generally, board_early_init() is responsible for
setup some initial peripherals, for example the iomux for uart or for
RAM controller. The common initialization is then put into board_init().
I am expecting that you have no issues by moving setup_display() in the
board_init() function.

> 
>>> diff --git a/configs/tbs2910_defconfig b/configs/tbs2910_defconfig
>>> new file mode 100644
>>> index 0000000..602d691
>>> --- /dev/null
>>> +++ b/configs/tbs2910_defconfig
>>> @@ -0,0 +1,3 @@
>>> +CONFIG_SYS_EXTRA_OPTIONS="IMX_CONFIG=board/boundary/nitrogen6x/nitrogen6q2g.cfg,MX6Q"
>>>
>>
>> Be aware that changes in the ddr-setup in that file will have a
>> consequence on your board.
> 
> Yes, I'm aware of that. But nitrogen6x is very well maintained. So I see
> no need to
> duplicate this config and reuse it in the same way as wandboard does.

ok, that is enough, then it is fine with me.

Best regards,
Stefano Babic

Hi Stefano!

On 10/22/14 10:36, Stefano Babic wrote:
> Hi Soeren,
>
> On 21/10/2014 19:54, Soeren Moch wrote:
>>>
>>> This is also copied from sabresd. Can we factorize in some way ?
>>
>> I can, and probably should, simplify this code. But in fact this code
>> is wrong, and in the same way for many imx6q boards (e.g. sabresd,
>> wandboard, nitrogen6x,...).
>
> That makes things only worse. Duplicating the code, we have an
> additional board with bad / wrong code.
>
>>
>> This code configures the external video clock (LDB_DIx clock). This
>> clock is hardcoded to have 65MHz in drivers/video/ipu_common.c:ldb_clk.
>> But in fact the clock rate is configured to 75.42MHz (528MHz/7) on all
>> boards. So the display does not show 1024x768@60Hz as configured, but
>> something similar to 1024x768@70Hz (not VESA compliant), which much
>> monitors can handle, but on other monitors there are problems.
>> (one out of tree monitors works for me).
>
> Ok, understood.
>
>>
>> My intention was to get this initial support for tbs2910 merged with the
>> (wrong) code from sabresd used as template, and later to discuss how to
>> cleanup this code.
>>
>> Do you prefer a simplified version of this code for the initial patch?
>
> Maybe this is the best approach: you can at the beginning drop support
> for video, an let your board be merged without wrong code. Then we can
> discuss about fixing the wrong clock as shared code, letting that all
> boards take advantage for that.

U-Boot without video support is useless for me. So I will try to
setup the video clock correctly in a new version of this patch.

>>
>>>> +#endif /* CONFIG_VIDEO_IPUV3 */
>>>> +
>>>> +int board_eth_init(bd_t *bis)
>>>> +{
>>>> +    setup_iomux_enet();
>>>> +    setup_pcie();
>>>> +
>>>> +    return cpu_eth_init(bis);
>>>> +}
>>>> +
>>>> +int board_early_init_f(void)
>>>> +{
>>>> +    setup_iomux_uart();
>>>> +#ifdef CONFIG_VIDEO_IPUV3
>>>> +    setup_display();
>>>
>>> I do not understand why setup_display() should be called at this point.
>>> Generally, board_early_init_f() is called to setup iomux for peripherals
>>> needed before relocation, as uart, letting the rest of the setup in
>>> board_init(). Why do you need here ?
>>
>> In fact this was also copied from sabresd/wandboard/nitrogen6x.
>> My assumption was, that the clocks must be configured before the
>> ipu is initialized.
>
> That is correct, but ipu is initialized by video_init() after
> board_init() is called. Generally, board_early_init() is responsible for
> setup some initial peripherals, for example the iomux for uart or for
> RAM controller. The common initialization is then put into board_init().
> I am expecting that you have no issues by moving setup_display() in the
> board_init() function.
>

You are right here, there is no problem with setup_display() in board_init().

So I wonder why all imx6q boards have setup_display() in board_early_init(),
you even committed such code for mx6qsabreauto yesterday.

Best regards,
Soeren Moch

Hi Soeren,

On 22/10/2014 14:16, Soeren Moch wrote:

> U-Boot without video support is useless for me. So I will try to
> setup the video clock correctly in a new version of this patch.

ok, fine with me.

> You are right here, there is no problem with setup_display() in
> board_init().
> 
> So I wonder why all imx6q boards have setup_display() in
> board_early_init(),
> you even committed such code for mx6qsabreauto yesterday.

Well, after each review a code reveals new aspects.

Anyway, this is not a big issue, but setting the display is not one
thing that must be done very early.

Best regards,
Stefano Babic