Re: [U-Boot] [PATCH] mx6sx: Add initial support for Samtec VIN|ING 2000 board

On 11/22/2016 03:48 PM, Christoph Fritz wrote:

Commit message is missing

[...]

diff --git a/board/samtec/vining2000/imximage.cfg b/board/samtec/vining2000/imximage.cfg new file mode 100644 index 0000000..4133dda --- /dev/null +++ b/board/samtec/vining2000/imximage.cfg @@ -0,0 +1,132 @@ +/*

• • Copyright (C) 2016 samtec automotive software & electronics gmbh
• • SPDX-License-Identifier: GPL-2.0+
• */

+#define __ASSEMBLY__ +#include <config.h>

Is this needed at all ?

+/* image version */

+IMAGE_VERSION 2

[...]

diff --git a/board/samtec/vining2000/vining2000.c b/board/samtec/vining2000/vining2000.c new file mode 100644 index 0000000..e73e57b --- /dev/null +++ b/board/samtec/vining2000/vining2000.c

[...]

+static iomux_v3_cfg_t const uart1_pads[] = {

• MX6_PAD_GPIO1_IO04__UART1_TX | MUX_PAD_CTRL(UART_PAD_CTRL),
• MX6_PAD_GPIO1_IO05__UART1_RX | MUX_PAD_CTRL(UART_PAD_CTRL),

+};

+static iomux_v3_cfg_t const usdhc2_pads[] = {

• MX6_PAD_SD2_CLK__USDHC2_CLK | MUX_PAD_CTRL(0x10059),

Use macros for this MUX_PAD_CTRL() argument, fix globally.

• MX6_PAD_SD2_CMD__USDHC2_CMD | MUX_PAD_CTRL(0x17059),
• MX6_PAD_SD2_DATA0__USDHC2_DATA0 | MUX_PAD_CTRL(0x17059),
• MX6_PAD_SD2_DATA1__USDHC2_DATA1 | MUX_PAD_CTRL(0x17059),
• MX6_PAD_SD2_DATA2__USDHC2_DATA2 | MUX_PAD_CTRL(0x17059),
• MX6_PAD_SD2_DATA3__USDHC2_DATA3 | MUX_PAD_CTRL(0x17059),
• MX6_PAD_LCD1_VSYNC__GPIO3_IO_28 | MUX_PAD_CTRL(0x80000000),

+};

[...]

+int board_eth_init(bd_t *bis) +{

• struct iomuxc *iomuxc_regs = (struct iomuxc *)IOMUXC_BASE_ADDR;
• int reg, ret;
• unsigned char eth1addr[6];
• imx_iomux_v3_setup_multiple_pads(fec1_pads, ARRAY_SIZE(fec1_pads));
• gpio_direction_output(IMX_GPIO_NR(5, 9) , 0);
• reg = readl(&iomuxc_regs->gpr[1]);
• /* Generate phy reference clock via pin IOMUX ENET_REF_CLK1/2 by erasing

• * ENET1/2_TX_CLK_DIR gpr1[14:13], so that reference clock is driven by
  

• * ref_enetpll0/1.
  

• */
  

Multiline comment has this format: /* * foo * bar */

• reg &= ~(IOMUX_GPR1_FEC1_CLOCK_MUX2_SEL_MASK |

• 	IOMUX_GPR1_FEC2_CLOCK_MUX2_SEL_MASK);
  

• /* Enable ENET1/2_TX_CLK output driver. */
• reg |= IOMUX_GPR1_FEC1_CLOCK_MUX1_SEL_MASK |

• 	IOMUX_GPR1_FEC2_CLOCK_MUX1_SEL_MASK;
  

• writel(reg, &iomuxc_regs->gpr[1]);

clrsetbits_le32() here.

• ret = enable_fec_anatop_clock(0, ENET_50MHZ);
• if (ret)

• printf("FEC anatop MXC: %s:failed (%0x)\n",  __func__, ret);
  

• gpio_set_value(IMX_GPIO_NR(5, 9), 1);
• mdelay(5);
• gpio_direction_output(IMX_GPIO_NR(5, 9) , 0);
• mdelay(5);
• gpio_set_value(IMX_GPIO_NR(5, 9), 1);
• mdelay(5);

What's this random GPIO poking for ?

• ret = fecmxc_initialize_multi(bis, 0, CONFIG_FEC_MXC_PHYADDR,

• 			IMX_FEC_BASE);
  

• if (ret)

• printf("FEC MXC: %s:failed\n", __func__);
  

• /* just to get secound mac address */
• imx_get_mac_from_fuse(1, eth1addr);
• if (is_valid_ethaddr(eth1addr))

• if (!getenv("eth1addr"))
  

Wrap this into single conditional -- if (foo() && !bar)

• 	eth_setenv_enetaddr("eth1addr", eth1addr);
  

• return ret;

+}

+#define PC MUX_PAD_CTRL(I2C_PAD_CTRL) +/* I2C1 for PMIC */ +static struct i2c_pads_info i2c_pad_info1 = {

• .scl = {

• .i2c_mode = MX6_PAD_GPIO1_IO00__I2C1_SCL | PC,
  

• .gpio_mode = MX6_PAD_GPIO1_IO00__GPIO1_IO_0 | PC,
  

• .gp = IMX_GPIO_NR(1, 0),
  

• },
• .sda = {

• .i2c_mode = MX6_PAD_GPIO1_IO01__I2C1_SDA | PC,
  

• .gpio_mode = MX6_PAD_GPIO1_IO01__GPIO1_IO_1 | PC,
  

• .gp = IMX_GPIO_NR(1, 1),
  

• },

+};

+struct pmic *pfuze_init(unsigned char i2cbus)

static struct ?

+{

• struct pmic *p;
• int ret;
• unsigned int reg;
• ret = power_pfuze100_init(i2cbus);
• if (ret)

• return NULL;
  

• p = pmic_get("PFUZE100");
• ret = pmic_probe(p);
• if (ret)

• return NULL;
  

• pmic_reg_read(p, PFUZE100_DEVICEID, &reg);
• printf("PMIC: PFUZE100 ID=0x%02x\n", reg);
• /* Set SW1AB stanby volage to 0.975V */
• pmic_reg_read(p, PFUZE100_SW1ABSTBY, &reg);
• reg &= ~SW1x_STBY_MASK;
• reg |= SW1x_0_975V;
• pmic_reg_write(p, PFUZE100_SW1ABSTBY, reg);
• /* Set SW1AB/VDDARM step ramp up time from 16us to 4us/25mV */
• pmic_reg_read(p, PFUZE100_SW1ABCONF, &reg);
• reg &= ~SW1xCONF_DVSSPEED_MASK;
• reg |= SW1xCONF_DVSSPEED_4US;
• pmic_reg_write(p, PFUZE100_SW1ABCONF, reg);
• /* Set SW1C standby voltage to 0.975V */
• pmic_reg_read(p, PFUZE100_SW1CSTBY, &reg);
• reg &= ~SW1x_STBY_MASK;
• reg |= SW1x_0_975V;
• pmic_reg_write(p, PFUZE100_SW1CSTBY, reg);
• /* Set SW1C/VDDSOC step ramp up time from 16us to 4us/25mV */
• pmic_reg_read(p, PFUZE100_SW1CCONF, &reg);
• reg &= ~SW1xCONF_DVSSPEED_MASK;
• reg |= SW1xCONF_DVSSPEED_4US;
• pmic_reg_write(p, PFUZE100_SW1CCONF, reg);
• return p;

+}

+int pfuze_mode_init(struct pmic *p, u32 mode)

static int ?

+{

• unsigned char offset, i, switch_num;
• u32 id;
• int ret;
• pmic_reg_read(p, PFUZE100_DEVICEID, &id);
• id = id & 0xf;
• if (id == 0) {

• switch_num = 6;
  

• offset = PFUZE100_SW1CMODE;
  

• } else if (id == 1) {

• switch_num = 4;
  

• offset = PFUZE100_SW2MODE;
  

• } else {

• printf("Not supported, id=%d\n", id);
  

• return -EINVAL;
  

• }
• ret = pmic_reg_write(p, PFUZE100_SW1ABMODE, mode);

Why do you need to do this write ?

• if (ret < 0) {

• printf("Set SW1AB mode error!\n");
  

• return ret;
  

• }
• for (i = 0; i < switch_num - 1; i++) {

• ret = pmic_reg_write(p, offset + i * SWITCH_SIZE, mode);
  

• if (ret < 0) {
  

• 	printf("Set switch 0x%x mode error!\n",
  

• 	       offset + i * SWITCH_SIZE);
  

• 	return ret;
  

• }
  

• }
• return ret;

+}

+int power_init_board(void) +{

• struct pmic *p;
• int ret;
• p = pfuze_init(I2C_PMIC);
• if (!p)

• return -ENODEV;
  

• ret = pfuze_mode_init(p, APS_PFM);
• if (ret < 0)

• return ret;
  

• return 0;

+}

+#ifdef CONFIG_USB_EHCI_MX6 +#define USB_OTHERREGS_OFFSET 0x800 +#define UCTRL_PWR_POL (1 << 9)

+static iomux_v3_cfg_t const usb_otg_pads[] = {

• /* OGT1 */
• MX6_PAD_GPIO1_IO09__USB_OTG1_PWR | MUX_PAD_CTRL(NO_PAD_CTRL),
• MX6_PAD_GPIO1_IO10__ANATOP_OTG1_ID | MUX_PAD_CTRL(NO_PAD_CTRL),
• /* OTG2 */
• MX6_PAD_GPIO1_IO12__USB_OTG2_PWR | MUX_PAD_CTRL(NO_PAD_CTRL)

+};

+static void setup_usb(void) +{

• imx_iomux_v3_setup_multiple_pads(usb_otg_pads, ARRAY_SIZE(

• 					usb_otg_pads));
  

+}

+int board_usb_phy_mode(int port) +{

• if (port == 1)

• return USB_INIT_HOST;
  

• else

• return usb_phy_mode(port);
  

+}

+int board_ehci_hcd_init(int port) +{

• u32 *usbnc_usb_ctrl;
• if (port > 1)

• return -EINVAL;
  

• usbnc_usb_ctrl = (u32 *)(USB_BASE_ADDR + USB_OTHERREGS_OFFSET +

• 		port * 4);
  

drivers/usb/host/ehci-mx6.c usb_power_config() does that for you , please fix.

• /* Set Power polarity */
• setbits_le32(usbnc_usb_ctrl, UCTRL_PWR_POL);
• return 0;

+} +#endif

+int board_phy_config(struct phy_device *phydev) +{

• if (phydev->drv->config)

• phydev->drv->config(phydev);
  

Is this really needed?

• return 0;

+}

+int set_pwm_leds(void) +{ +#ifdef CONFIG_PWM_IMX

• imx_iomux_v3_setup_multiple_pads(pwm_led_pads, ARRAY_SIZE(

• 					pwm_led_pads));
  

• /* enable backlight PWM 2, green LED */
• if (pwm_init(1, 0, 0))

• goto error;
  

• /* duty cycle 200ns, period: 8000ns */
• if (pwm_config(1, 200, 8000))

• goto error;
  

• if (pwm_enable(1))

• goto error;
  

• /* enable backlight PWM 1, blue LED */
• if (pwm_init(0, 0, 0))

• goto error;
  

• /* duty cycle 200ns, period: 8000ns */
• if (pwm_config(0, 200, 8000))

• goto error;
  

• if (pwm_enable(0))

• goto error;
  

• /* enable backlight PWM 6, red LED */
• if (pwm_init(5, 0, 0))

• goto error;
  

• /* duty cycle 200ns, period: 8000ns */
• if (pwm_config(5, 200, 8000))

• goto error;
  

• if (pwm_enable(5))

• goto error;
  

+#else

• imx_iomux_v3_setup_multiple_pads(gpio_led_pads, ARRAY_SIZE(

• 					gpio_led_pads));
  

please don't split this in the middle of ARRAY_SIZE(, put thoe whole ARRAY_SIZE on new line and indent below the gpio_led_pads. Fix globally.

• gpio_direction_output(IMX_GPIO_NR(5, 14) , 1); /* green */
• gpio_direction_output(IMX_GPIO_NR(5, 20) , 1); /* red */
• gpio_direction_output(IMX_GPIO_NR(5, 15) , 1); /* blue */

+#endif

• return 0;

+error:

• printf("error LED\n");

This error message is useless, either reword it or remove it.

• return -1;

+}

+#define ADC1_HC0 (ADC1_BASE_ADDR + 0x00) +#define ADC1_HS (ADC1_BASE_ADDR + 0x08) +#define ADC1_R0 (ADC1_BASE_ADDR + 0x0c) +#define ADC1_CFG (ADC1_BASE_ADDR + 0x14) +#define ADC1_GC (ADC1_BASE_ADDR + 0x18) +#define MAX_ADC_CNT 4096

+int read_adc(void)

static !

+{

• u32 reg;
• unsigned int cnt;
• writel(0x308, ADC1_CFG);

Use macros instead of magic values.

• /* start auto calibration */
• reg = readl(ADC1_GC);
• reg |= (1 << 7);
• writel(reg, ADC1_GC);
• for (cnt = 0; readl(ADC1_GC) & (1 << 7) || cnt > MAX_ADC_CNT; cnt++)

Use BIT(n) instead of (1 << n), in a macro.

• udelay(1);
  

• if (cnt > MAX_ADC_CNT)

• goto adc_fail;
  

• /* start conversation */
• writel(0, ADC1_HC0);
• /* wait for conversation */
• for (cnt = 0; !readl(ADC1_HS) || cnt > MAX_ADC_CNT; cnt++)

• udelay(1);
  

• if (cnt > MAX_ADC_CNT)

• goto adc_fail;
  

• /* read result */
• reg = readl(ADC1_R0);
• return reg;

+adc_fail:

• printf("ADC failure\n");
• return -1;

+}

+#define VAL_UPPER 2498 +#define VAL_LOWER 1550

+int set_pin_state(void)

static ...

+{

• int val = read_adc();
• if (val >= VAL_UPPER)

• setenv("pin_state", "connected");
  

• else if (val < VAL_UPPER && val >= VAL_LOWER)

• setenv("pin_state", "open");
  

• else if (val < VAL_LOWER)

• setenv("pin_state", "button");
  

• return val;

+}

+int board_late_init(void) +{

• int ret;
• ret = set_pwm_leds();
• if (ret)

• return ret;
  

• set_pin_state();
• return ret;

+}

+int board_early_init_f(void) +{

• setup_iomux_uart();

+#ifdef CONFIG_USB_EHCI_MX6

• setup_usb();

Implement empty setup_usb() for case where CONFIG_USB_EHCI_MX6 is not set and remove this check here since setup_usb() will always be defined.

+#endif

• return 0;

+}

+static struct fsl_esdhc_cfg usdhc_cfg[2] = {

• {USDHC4_BASE_ADDR},
• {USDHC2_BASE_ADDR, 0, 4},

+};

+#define USDHC2_CD_GPIO IMX_GPIO_NR(3, 28)

+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 USDHC4_BASE_ADDR:

• ret = 1; /* Assume uSDHC4 is always present */
  

• break;
  

• case USDHC2_BASE_ADDR:

• ret = !gpio_get_value(USDHC2_CD_GPIO);
  

• break;
  

default: case is missing

• }
• return ret;

+}

+int board_mmc_init(bd_t *bis) +{

• int ret;
• /*

• * According to the board_mmc_init() the following map is done:
  

• * (U-Boot device node)    (Physical Port)
  

• * mmc0                    USDHC4
  

• * mmc1                    USDHC2
  

• */
  

• imx_iomux_v3_setup_multiple_pads(

• usdhc4_pads, ARRAY_SIZE(usdhc4_pads));
  

• usdhc_cfg[0].sdhc_clk = mxc_get_clock(MXC_ESDHC4_CLK);
• imx_iomux_v3_setup_multiple_pads(

• usdhc2_pads, ARRAY_SIZE(usdhc2_pads));
  

• gpio_direction_input(USDHC2_CD_GPIO);
• usdhc_cfg[1].sdhc_clk = mxc_get_clock(MXC_ESDHC2_CLK);
• ret = fsl_esdhc_initialize(bis, &usdhc_cfg[0]);
• ret |= fsl_esdhc_initialize(bis, &usdhc_cfg[1]);

Do not apply bitwise operations on signed integer values , just check one after the other and fail with a meaningful error message.

• if (ret) {

• printf("Warning: failed to initialize mmc dev\n");
  

• return ret;
  

• }
• return 0;

+}

[...]

diff --git a/include/configs/vining2000.h b/include/configs/vining2000.h new file mode 100644 index 0000000..a379ee2 --- /dev/null +++ b/include/configs/vining2000.h @@ -0,0 +1,171 @@ +/*

• • Copyright (C) 2016 samtec automotive software & electronics gmbh
• • Configuration settings for the Samtec VIN|ING 2000 board.
• • SPDX-License-Identifier: GPL-2.0+
• */

+#ifndef __CONFIG_H +#define __CONFIG_H

+#include "mx6_common.h"

+#ifdef CONFIG_SPL +#include "imx6_spl.h" +#endif

+/* Size of malloc() pool */ +#define CONFIG_SYS_MALLOC_LEN (3 * SZ_1M)

+#define CONFIG_BOARD_EARLY_INIT_F

+#define CONFIG_MXC_UART +#define CONFIG_MXC_UART_BASE UART1_BASE

Look at the distro_bootcmd.h

+#define CONFIG_EXTRA_ENV_SETTINGS \

• "script=boot.scr\0" \
• "image=zImage-vining2000\0" \
• "console=ttymxc0\0" \
• "fdt_high=0xffffffff\0" \
• "initrd_high=0xffffffff\0" \
• "initrd_addr=0x90000000\0" \
• "fdt_file=imx6sx-samtec-vining2000.dtb\0" \
• "fdt_addr=0x88000000\0" \
• "boot_fdt=try\0" \
• "loadaddr=0x80800000\0" \
• "ip_dyn=yes\0" \
• "mmcdev=0\0" \
• "mmcpart=1\0" \
• "mmcroot=/dev/mmcblk1p1 rootwait rw\0" \
• "emmcargs=setenv bootargs console=${console},${baudrate} " \

• "root=${mmcroot} fsckfix panic=3\0" \
  

• "loadbootscript=" \

• "fatload mmc ${mmcdev}:${mmcpart} ${loadaddr} ${script};\0" \
  

Drop this fatload stuff, just use 'load' command and autodetect FS. Avoid fat altogether if possible.

• "bootscript=echo Running bootscript from mmc ...; " \

• "source\0" \
  

• "loadimage=fatload mmc ${mmcdev}:${mmcpart} ${loadaddr} ${image}\0" \
• "loadfdt=fatload mmc ${mmcdev}:${mmcpart} ${fdt_addr} ${fdt_file}\0" \
• "emmcboot=echo Booting from emmc ...; " \

• "run emmcargs; mmc dev 0 0; mmc partconf 0 1 0x1 0x1; " \
  

• "fatload mmc 0:1 ${fdt_addr} oftree; " \
  

• "fatload mmc 0:1 ${loadaddr} zImage; " \
  

• "bootz ${loadaddr} - ${fdt_addr};\0" \
  

• "netargs=setenv bootargs console=${console},${baudrate} " \

• "root=/dev/nfs " \
  

• "ip=dhcp rw \0" \
• "netboot=echo Booting from net ...; " \

• "run netargs; " \
  

• "if test ${ip_dyn} = yes; then " \
  

• 	"setenv get_cmd dhcp; " \
  

• "else " \
  

• 	"setenv get_cmd tftp; " \
  

• "fi; " \
  

• "${get_cmd} ${image}; " \
  

• "if test ${boot_fdt} = yes || test ${boot_fdt} = try; then " \
  

• 	"if ${get_cmd} ${fdt_addr} ${fdt_file}; then " \
  

• 		"bootz ${loadaddr} - ${fdt_addr}; " \
  

• 	"else " \
  

• 		"if test ${boot_fdt} = try; then " \
  

• 			"bootz; " \
  

• 		"else " \
  

• 			"echo WARN: Cannot load the DT; " \
  

• 		"fi; " \
  

• 	"fi; " \
  

• "else " \
  

• 	"bootz; " \
  

• "fi;\0"
  

+#define CONFIG_BOOTCOMMAND \

•   "run emmcboot; run netboot;"
  

+/* Miscellaneous configurable options */ +#define CONFIG_SYS_MEMTEST_START 0x80000000

[...]