[U-Boot] [PATCH v4 8/8] mx53: Add Board support for GE PPD

[Fabio Estevam]

On Fri, Nov 3, 2017 at 8:10 AM, Martyn Welch <martyn at welchs.me.uk> wrote:
> From: Peter Senna Tschudin <peter.senna at collabora.com>
>
> Create board support for GE PPD, based on mx53loco.
>
> Use mx53ppd_defconfig make target to configure for this board.
>
> Signed-off-by: Peter Senna Tschudin <peter.senna at collabora.com>
> Signed-off-by: Ian Ray <ian.ray at ge.com>
> Signed-off-by: Nandor Han <nandor.han at ge.com>
> Signed-off-by: Martyn Welch <martyn.welch at collabora.co.uk>
> Cc: Stefano Babic <sbabic at denx.de>

Please make sure to make checkpatch happy for all the series.

Currently we have the following warning/errors for this patch:

total: 5 errors, 22 warnings, 25 checks, 1128 lines checked

> +u32 get_board_rev(void)
> +{
> +       struct iim_regs *iim = (struct iim_regs *)IMX_IIM_BASE;
> +       struct fuse_bank *bank = &iim->bank[0];
> +       struct fuse_bank0_regs *fuse =
> +               (struct fuse_bank0_regs *)bank->fuse_regs;
> +
> +       int rev = readl(&fuse->gp[6]);

Here you read the value from the fuse.


> +
> +       rev = 0;

but here you overwrite it.

> +static void clock_1GHz(void)

Maybe you can make it static int instead

> +{
> +       int ret;
> +       u32 ref_clk = MXC_HCLK;
> +       /*
> +        * After increasing voltage to 1.25V, we can switch
> +        * CPU clock to 1GHz and DDR to 400MHz safely
> +        */
> +       ret = mxc_set_clock(ref_clk, 1000, MXC_ARM_CLK);
> +       if (ret)
> +               printf("CPU:   Switch CPU clock to 1GHZ failed\n");

and propagate the error.


> +
> +       ret = mxc_set_clock(ref_clk, 400, MXC_PERIPH_CLK);
> +       ret |= mxc_set_clock(ref_clk, 400, MXC_DDR_CLK);
> +       if (ret)
> +               printf("CPU:   Switch DDR clock to 400MHz failed\n");

and here too.

> +}
> +
> +void ppd_gpio_init(void)
> +{
> +       int i;
> +       imx_iomux_v3_setup_multiple_pads(ppd_pads, ARRAY_SIZE(ppd_pads));
> +       for (i = 0; i < ARRAY_SIZE(ppd_gpios); ++i) {
> +               gpio_direction_output(ppd_gpios[i].gpio, ppd_gpios[i].value);
> +       }

No need for the {

> + * Extracts MAC and product information from the VPD.
> + */
> +static int vpd_callback(
> +       void *userdata,
> +       uint8_t id,
> +       uint8_t version,
> +       uint8_t type,
> +       size_t size,
> +       uint8_t const *data)

Better put all parameters into a single line if possible.


> +static void process_vpd(struct vpd_cache *vpd)
> +{
> +       int fec_index = -1;
> +
> +       switch (vpd->product_id)
> +       {
> +       case VPD_PRODUCT_PPD:
> +               fec_index = 0;
> +               break;
> +       }

Does it really make sense to have a switch for only one possible value?

> +
> +       if (fec_index >= 0 && (vpd->has & VPD_HAS_MAC1)) {
> +               eth_env_set_enetaddr("ethaddr", vpd->mac1);
> +       }


No need to have the {

> +static int read_vpd(uint eeprom_bus)
> +{
> +       struct vpd_cache vpd;
> +       int res;
> +       int size = 1024;
> +       uint8_t *data;
> +       unsigned int current_i2c_bus = i2c_get_bus_num();
> +
> +       res = i2c_set_bus_num(eeprom_bus);
> +       if (res < 0)
> +               return res;
> +
> +       data = (uint8_t *)malloc(size);

cast not needed.

> +       if (!data)
> +               return -ENOMEM;
> +
> +       res = i2c_read(VPD_EEPROM_ADDR, 0,
> +                       VPD_EEPROM_ADDR_LEN, data, size);
> +
> +       if (res == 0) {
> +               memset(&vpd, 0, sizeof(vpd));
> +               vpd_reader(size, data, &vpd, vpd_callback);
> +               process_vpd(&vpd);
> +       }
> +
> +       free(data);
> +
> +       i2c_set_bus_num(current_i2c_bus);
> +       return res;
> +}
> +
> +void check_time(void)

static


> +int board_init(void)
> +{
> +       gd->bd->bi_boot_params = PHYS_SDRAM_1 + 0x100;
> +
> +       mxc_set_sata_internal_clock();
> +       setup_iomux_i2c();
> +
> +       setup_i2c(1, CONFIG_SYS_I2C_SPEED, 0x7f, &i2c_pad_info1);
> +
> +       return 0;
> +}
> +
> +int misc_init_r(void)
> +{
> +       const char *cause;
> +
> +       /* We care about WDOG only, treating everything else as
> +        * a power-on-reset.
> +        */
> +       if (get_imx_reset_cause() & 0x0010) {
> +               cause = "WDOG";
> +       } else {
> +               cause = "POR";
> +       }


No need for the {

> +void lcd_enable(void)

static