[U-Boot] [PATCH] ventana: Add Gateworks Ventana family support
Tim Harvey
tharvey at gateworks.com
Tue Jan 28 00:36:57 CET 2014
On Mon, Jan 27, 2014 at 6:39 AM, Stefano Babic <sbabic at denx.de> wrote:
> Hi Tim,
Hi Stefano - thanks for the review!
>
> please run script/checkpatch on your patch before next submit. The tool
> reports plenty of issues that must be solved for mainlining.
>
It looks like I missed some >80 char formatting issues regarding the
env vars in the config file. I'll fix those up.
> On 21/01/2014 19:41, Tim Harvey wrote:
>> Gateworks Ventana is a product family based on the i.MX6. This
>> patch adds support for all boards in the Ventana family. Where
>> possible, data from the boards EEPROM is used to determine various
>> details about the board at runtime.
>>
>> Signed-off-by: Tim Harvey <tharvey at gateworks.com>
>> ---
>> board/gateworks/gw_ventana/1066mhz_4x128mx16.cfg | 42 +
>> board/gateworks/gw_ventana/800mhz_2x128mx16.cfg | 42 +
>> board/gateworks/gw_ventana/800mhz_4x128mx16.cfg | 42 +
>
> These three files to set up the DDR are *identical* to the files with
> the same name in /board/boundary/nitrogen6x.
>
> A such as duplication is not allowed. There should be a good reason to
> have new files - for solidrun, different values are requested for DDR.
>
> Please consider to refer to supplied files instead of copying.
I will include the ddr config files from boundary with a
../../boundary/... where they do not differ. The clocks.cfg does
differ (see below) but I liked the way Boundary broke these files out
do want to re-use as much as possible.
>
>
>> diff --git a/board/gateworks/gw_ventana/README b/board/gateworks/gw_ventana/README
>> new file mode 100644
>> index 0000000..9de55ba
>> --- /dev/null
>> +++ b/board/gateworks/gw_ventana/README
>> @@ -0,0 +1,49 @@
>> +U-Boot for the Gateworks Ventana Product Family boards
>> +
>> +This file contains information for the port of U-Boot to the Gateworks
>> +Ventana Product family boards.
>> +
>> +1. Boot source, boot from NAND
>> +------------------------------
>> +
>> +The i.MX6 BOOT ROM expects some headers that provide details of NAND layout
>> +and bad block information (referred to as 'bootstreams') which are replicated
>> +multiple times in NAND.
>> The number of replications is configurable through
>> +board strapping options and eFUSE settings. The Freescale 'kobs-ng'
>> +application from the Freescale LTIB BSP, which runs under Linux, must be used
>> +to program the bootstream in order to setup the replicated headers correctly.
>
> The behavior is quite different as we have currently in mainline. With
> kobs-ng you flash u-boot.bin, while the result image for i.MXes in
> mailine is u-boot.imx (u-boot.bin with imx header).
I'm not familiar with the IMX family other than IMX6 but for IMX6
kobs-ng does use u-boot.imx and not u-boot.bin. kobs needs the
headers which are not part of u-boot.bin. Are you sure you are not
mistaken here? Can you point me to some references?
>
> This behavior is also different to the roadmap we have discussed some
> times ago for the i.MX6. We will move i.MX to use SPL, and the bad
> blocks handling, not managed by the BOOT ROM, will be done inside SPL.
Yes - I would also like to an SPL bootloader, but have not quite
gotten that working yet for NAND. I felt it was a good start to get
my current patchset mainlined as a starting point.
Regardless of what is loading the next level (u-boot.bin) the initial
flashing of NAND is still currently handled only by kobs-ng so I'm not
sure how this differs in this respect. I plan to look at adding the
functionality of kobs-ng to u-boot at some point.
>
>> +
>> +The Gateworks Ventana boards with NAND flash have been factory programmed
>> +such that their eFUSE settings expect 2 copies of the boostream (this is
>> +specified by providing kobs-ng with the --search_exponent=1 argument). Once in
>> +Linux with MTD support for the NAND on /dev/mtd0 you can program the boostream
>> +with:
>> +
>> +kobs-ng init -v -x --search_exponent=1 u-boot.imx
>> +
>> +This information is taken from
>> +
>> +https://trac.gateworks.com/wiki/ventana%3Abuild_uboot
>
> The link is broken - there is no permission with https, while http works.
Thanks for catching this. The correct link is
http://trac.gateworks.com/wiki/ventana/bootloader#BuildingfromSource
and I will fix it in the next patch version.
>
>> +
>> +2. Build
>> +--------
>> +
>> +There are several Gateworks Ventana boards that share a simliar design but
>> +vary based on CPU, Memory configuration, and subloaded devices. Although
>> +the subloaded devices are handled dynamically in the bootloader using
>> +factory configured EEPROM data to modify the device-tree, the CPU choice
>> +(IMX6Q vs IMX6DL) and memory configurations are currently compile-time
>> +options.
>
> We will have another advantage with SPL, that is we can have a single
> image working on different i.MX6 variants (Solo/Dual/Quad).
agreed.
I really wanted to get to SPL before posting but felt I have a good
starting point equivalent with the other IMX6 boards currently in
mainline. For SPL I need nand and i2c support (as we use i2c to
access the EEPROM which contains details about DDR configuration) and
currently both of those are creating some dependency issues that keep
the SPL from building for me. I haven't had time to dig into that
yet.
>
>> +
>> +The following Gateworks Ventana configurations exist:
>> + gwventanaq1gspi: MX6Q,1GB,SPI FLASH
>> + gwventanaq : MX6Q,512MB,NAND FLASH
>> + gwventanaq1g : MX6Q,1GB,NAND FLASH
>> + gwventanadl : MX6DL,512MB,NAND FLASH
>> + gwventanadl1g : MX6DL,1GB,NAND FLASH
>> +
>> +To build U-Boot for the MX6Q,1GB,NAND FLASH for example:
>> +
>> + make gwventanaq1g_config
>> + make u-boot.imx
>
> u-boot.imx should be the default target. But it seems weird you build
> u-boot.imx and then discard it (or am I wrong ?) to take u-boot.bin.
u-boot.imx is the default target. We don't discard it. I will change
the instructions to just 'make gwventanaq1g_config; make' to avoid
confusion.
>
>> +
>> diff --git a/board/gateworks/gw_ventana/clocks.cfg b/board/gateworks/gw_ventana/clocks.cfg
>> new file mode 100644
>> index 0000000..31790e7
>> --- /dev/null
>> +++ b/board/gateworks/gw_ventana/clocks.cfg
>> @@ -0,0 +1,41 @@
>> +/*
>> + * Copyright (C) 2013 Gateworks Corporation
>> + *
>
> This is the only change in the file. The original file has a Boundary
> Device Copyright. You cannot simply substitute the original Copyright
> with your own.
>
> clocks.cfg is then (apart some whitespaces) identical to clocks.cfg in
> nitrogen. Please do not duplicate files.
I agree that the original copyright should be there and I'll add it,
but there is an important difference between the two so I cannot use
the original. The Gateworks board uses NAND flash thus the CCM_CCGR4
register differs by a few bits.
- DATA 4, CCM_CCGR4, 0x00FFF300
+ DATA 4, CCM_CCGR4, 0xFFFFF300 /* enable NAND/GPMI/BCH clocks */
>
>
>> --- /dev/null
>> +++ b/board/gateworks/gw_ventana/ddr-setup.cfg
>> @@ -0,0 +1,96 @@
>> +/*
>> + * Copyright (C) 2013 Boundary Devices
>> + *
>> + * SPDX-License-Identifier: GPL-2.0+
>> + *
>> + * Device Configuration Data (DCD)
>> + *
>> + * Each entry must have the format:
>> + * Addr-type Address Value
>> + *
>> + * where:
>> + * Addr-type register length (1,2 or 4 bytes)
>> + * Address absolute address of the register
>> + * value value to be stored in the register
>> + */
>> +
>
> even ddr-setup.cfg is identical (but here Copyright is untouched).
> It seems we do not need any special .cfg file for the board.
just clocks.cfg at this point must differ.
>
<snip>
>> +
>> +int board_mmc_init(bd_t *bis)
>> +{
>> + s32 status = 0;
>> + u32 index = 0;
>> +
>> + usdhc_cfg[0].sdhc_clk = mxc_get_clock(MXC_ESDHC3_CLK);
>> + usdhc_cfg[0].max_bus_width = 4;
>> +
>> + for (index = 0; index < CONFIG_SYS_FSL_USDHC_NUM; ++index) {
>> + switch (index) {
>> + case 0:
>
> I know I am the first responsible for this code. But if
> CONFIG_SYS_FSL_USDHC_NUM = 1 (and this is not easy changable without
> modifying the board), why do we need a loop ?
Agreed - a loop is silly here, I'll remove it.
>
>> +/* Gateworks System Controller I2C access may NAK when busy - use
>
> Wrong multiline comment. It should be:
>
> /*
> * your comment
> * here
> */
will fix thanks. Someone else mentioned that to me and I have not
been able to find a style guide for multi-line comments. Looks like
opening and closing should contain no text.
>
>> + * retries.
>> + */
>> +int gsc_i2c_read(uchar chip, uint addr, int alen, uchar *buf, int len)
>> +{
>> + int retry = 3;
>> + int n = 0;
>> + int ret;
>> +
>> + while (n++ < retry) {
>> + ret = i2c_read(chip, addr, alen, buf, len);
>> + if (!ret)
>> + break;
>> + printf("%s: 0x%02x 0x%02x retry%d: %d\n", __func__, chip, addr,
>> + n, ret);
>> + if (ret != -ENODEV)
>> + break;
>> + mdelay(10);
>> + }
>
> Whcih is the benefit of trying three times ?
it provides a little more redundancy than 2 times, and a little less than 4 ;)
Our GSC can occasionally be 'busy' and NAK (every 1Hz it does some
internal processing). As there are several places where I perform an
i2c transaction to one of the GSC's slave devices (it emulates several
i2c devices) I wanted to consolidate where I had retry logic. There
is nothing magic about 3 tries necessarily but I have found that with
the speed of the SoC we never fail more than 2 successive reads due to
the periodic loading of the GSC.
If you want more info on the GSC you can read about it here:
http://trac.gateworks.com/wiki/gsc
>
>> +int gsc_i2c_write(uchar chip, uint addr, int alen, uchar *buf, int len)
>> +{
>> + int retry = 3;
>> + int n = 0;
>> + int ret;
>> +
>> + while (n++ < retry) {
>> + ret = i2c_write(chip, addr, alen, buf, len);
>> + if (!ret)
>> + break;
>> + printf("%s: 0x%02x 0x%02x retry%d: %d\n", __func__, chip, addr,
>> + n, ret);
>> + if (ret != -ENODEV)
>> + break;
>> + mdelay(10);
>> + }
>> + mdelay(1);
>> + return ret;
>> +}
>
> Same here. If first time fails, something happens. It makes no sense to
> try multiple times.
in our case I feel it does - see above.
>
>> +
>> +/*
>> + * For SPL boot some boards need i2c before SDRAM is initialized so force
>> + * variables to live in SRAM
>> + */
>
> Agree - but then I will see SPL...
>
>> +static struct ventana_board_info __attribute__((section(".data"))) ventana_info;
>
> Can you explain why do you need this ?
This was in preparation for SPL. I'm not clear if the struct needs
any special attributes at this time to be available to all the
functions that use it. Because currently the BOOTROM sets up DDR I'm
thinking perhaps not, but I seem to recall before I added the
attribute I ran into some issues.
>
>> +
>> +/* read ventana EEPROM and return structure or NULL on error
>
> Multiline comment
ack
>
>> + * should be called once, the first time eeprom data is needed
>> + */
>> +static void
>> +read_eeprom(void)
>> +{
>
> You have a I2C eprom. This is supported in U-Boot. Why do you not
> set CONFIG_SYS_I2C_EEPROM_ADDR in your config file and use general code ?
>
> It seems to me you can drop your own read/write_eeprom() function.
I considered this before (using common/cmd_eeprom.c), but the fact
that I want to be able to handle retries on NAK's would keep me from
using a generic read_eeprom feature. Also, that common support only
just replaces my gsc_i2c_read function - inside read_eeprom I verify
checksum and some other sanity checks. Grepping for
CONFIG_SYS_I2C_EEPROM_ADDR there are a lot of boards that implement
their own read_eeprom functionality for one reason or another.
>
>> + int i;
>> + int chksum;
>> + struct ventana_board_info *info = &ventana_info;
>> + unsigned char *buf = (unsigned char *)&ventana_info;
>> + int n = 0;
>> +
>> + memset(info, 0, sizeof(ventana_info));
>> +
>> + /* wait for bus and device exist - we will not boot w/o our EEPROM */
>> + while (1) {
>> + if (0 == i2c_set_bus_num(0) && 0 == i2c_probe(0x51))
>> + break;
>> + mdelay(1);
>> + n++;
>> + }
>> +
>> + /* read eeprom config section */
>> + if (gsc_i2c_read(0x51, 0x00, 1, buf, sizeof(ventana_info))) {
>> + printf("EEPROM: Failed to read EEPROM\n");
>> + info->model[0] = 0;
>> + return;
>> + }
>> +
>> + /* sanity checks */
>> + if (info->model[0] != 'G' || info->model[1] != 'W') {
>> + printf("EEPROM: Invalid Model in EEPROM\n");
>> + info->model[0] = 0;
>> + return;
>> + }
>
> You can do your checks on the pyload after reading from the storage and
> using common functions.
Agreed, if I could use the common functions. However here I am
reading from a device that requires some special handling (retry on
NAK).
>
>> +#ifdef CONFIG_CMD_GSC
>> +int read_hwmon(const char *name, uint reg, uint size, uint low, uint high)
>> +{
>> + unsigned char buf[3];
>> + uint ui;
>> + int ret;
>> +
>> + printf("%-8s:", name);
>> + memset(buf, 0, sizeof(buf));
>> + if (gsc_i2c_read(0x29, reg, 1, buf, size)) {
>> + printf("fRD\n");
>> + ret = -1;
>> + } else {
>> + ret = 0;
>> + ui = buf[0] | (buf[1]<<8) | (buf[2]<<16);
>> + if (ui == 0xffffff) {
>> + printf("fVL");
>> + } else if (ui < low) {
>> + printf("%d fLO", ui);
>> + ret = -2;
>> + } else if (ui > high) {
>> + printf("%d fHI", ui);
>> + ret = -3;
>> + } else {
>> + printf("%d", ui);
>> + }
>> + }
>> + printf("\n");
>> +
>> + return ret;
>> +}
>
> I need help - I have not understood. This functions prints only
> something - return values is discarded when function is called. Buf is
> filled, but can you better explain the purpose of the function.
The gsc command (below) is currently just showing the values from the
hardware monitor (temp and various voltage rails). The read_hwmon
function above is a helper function to display the value, or a failure
high, or failure low in a standard fashion.
>
>> +
>> +int do_gsc(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>> +{
>> + struct ventana_board_info *info = &ventana_info;
>> +
>> + i2c_set_bus_num(0);
>> + if ((strncasecmp((const char *)info->model, "GW51", 4) == 0)) {
>> + read_hwmon("Temp", 0x00, 2, 0, 9000);
>> + read_hwmon("VIN", 0x02, 3, 8000, 60000);
>> + read_hwmon("VDD_3P3", 0x05, 3, 3300*0.9, 3300*1.1);
>> + read_hwmon("VBATT", 0x08, 3, 2000*0.9, 3000*1.1);
>> + read_hwmon("VDD_CORE", 0x0e, 3, 1175*0.9, 1175*1.1);
>> + read_hwmon("VDD_SOC", 0x11, 3, 1175*0.9, 1175*1.1);
>> + read_hwmon("VDD_HIGH", 0x14, 3, 3000*0.9, 3000*1.1);
>> + read_hwmon("VDD_DDR", 0x17, 3, 1500*0.9, 1500*1.1);
>> + read_hwmon("VDD_5P0", 0x0b, 3, 5000*0.9, 5000*1.1);
>> + read_hwmon("VDD_2P5", 0x23, 3, 2500*0.9, 2500*1.1);
>> + read_hwmon("VDD_1P8", 0x1d, 3, 1800*0.9, 1800*1.1);
>> + } else if ((strncasecmp((const char *)info->model, "GW52", 4) == 0)) {
>> + read_hwmon("Temp", 0x00, 2, 0, 9000);
>> + read_hwmon("VIN", 0x02, 3, 8000, 60000);
>> + read_hwmon("VDD_3P3", 0x05, 3, 3300*0.9, 3300*1.1);
>> + read_hwmon("VBATT", 0x08, 3, 2000*0.9, 3000*1.1);
>> + read_hwmon("VDD_CORE", 0x0e, 3, 1175*0.9, 1175*1.1);
>> + read_hwmon("VDD_SOC", 0x11, 3, 1175*0.9, 1175*1.1);
>> + read_hwmon("VDD_HIGH", 0x14, 3, 3000*0.9, 3000*1.1);
>> + read_hwmon("VDD_DDR", 0x17, 3, 1500*0.9, 1500*1.1);
>> + read_hwmon("VDD_5P0", 0x0b, 3, 5000*0.9, 5000*1.1);
>> + read_hwmon("VDD_2P5", 0x23, 3, 2500*0.9, 2500*1.1);
>> + read_hwmon("VDD_1P8", 0x1d, 3, 1800*0.9, 1800*1.1);
>> + read_hwmon("VDD_1P0", 0x20, 3, 1000*0.9, 1000*1.1);
>> + } else if ((strncasecmp((const char *)info->model, "GW53", 4) == 0)) {
>> + read_hwmon("Temp", 0x00, 2, 0, 9000);
>> + read_hwmon("VIN", 0x02, 3, 8000, 60000);
>> + read_hwmon("VDD_3P3", 0x05, 3, 3300*0.9, 3300*1.1);
>> + read_hwmon("VBATT", 0x08, 3, 2000*0.9, 3000*1.1);
>> + read_hwmon("VDD_CORE", 0x0e, 3, 1175*0.9, 1175*1.1);
>> + read_hwmon("VDD_SOC", 0x11, 3, 1175*0.9, 1175*1.1);
>> + read_hwmon("VDD_HIGH", 0x14, 3, 3000*0.9, 3000*1.1);
>> + read_hwmon("VDD_DDR", 0x17, 3, 1500*0.9, 1500*1.1);
>> + read_hwmon("VDD_5P0", 0x0b, 3, 5000*0.9, 5000*1.1);
>> + read_hwmon("VDD_2P5", 0x23, 3, 2500*0.9, 2500*1.1);
>> + read_hwmon("VDD_1P8", 0x1d, 3, 1800*0.9, 1800*1.1);
>> + read_hwmon("VDD_1P0", 0x20, 3, 1000*0.9, 1000*1.1);
>> + } else if ((strncasecmp((const char *)info->model, "GW54", 4) == 0)) {
>> + read_hwmon("Temp", 0x00, 2, 0, 9000);
>> + read_hwmon("VIN", 0x02, 3, 8000, 60000);
>> + read_hwmon("VDD_3P3", 0x05, 3, 3300*0.9, 3300*1.1);
>> + read_hwmon("VBATT", 0x08, 3, 2000*0.9, 3000*1.1);
>> + read_hwmon("VDD_CORE", 0x0e, 3, 1375*0.9, 1375*1.1);
>> + read_hwmon("VDD_SOC", 0x11, 3, 1375*0.9, 1375*1.1);
>> + read_hwmon("VDD_HIGH", 0x14, 3, 3000*0.9, 3000*1.1);
>> + read_hwmon("VDD_DDR", 0x17, 3, 1500*0.9, 1500*1.1);
>> + read_hwmon("VDD_5P0", 0x0b, 3, 5000*0.9, 5000*1.1);
>> + read_hwmon("VDD_2P5", 0x23, 3, 2500*0.9, 2500*1.1);
>> + read_hwmon("VDD_1P8", 0x1d, 3, 1800*0.9, 1800*1.1);
>> + read_hwmon("VDD_1P0", 0x20, 3, 1000*0.9, 1000*1.1);
>> + }
>> + return 0;
>> +}
>
> I think we can do better. Most cases are duplicated. Please split into
> common calls (for example, read_hwmon("Temp", 0x00, 2, 0, 9000) is
> always called) and board-variations. The identification is done on a
> single byte (GW51-GW54): you can use a more readable switch..case
> instead of multiple strncmp()
Agreed that there is a lot of duplication here. I'll revise as the
only voltage rails not common across the 4 baseboards are VDD_SOC,
VDD_ARM, VDD_1P0.
I'm not sure I agree the switch-case is more readable although it
would be higher performance if that was something desirable. The 4
gateworks baseboards get customized into 'specials' and sometimes
these specials end up with special handling based on models which is
why I like to keep the comparisons done as strings, but I suppose I
could cross that bridge if we come to it later.
>
>> +
>> +
>> +/* get_mac from env string, with default
>> + */
>> +static void get_mac(char *envvar, unsigned char *def)
>> +{
>> + char str[20];
>> + char *env = getenv(envvar);
>> +
>> + if (!env) {
>> + sprintf(str, "%02X:%02X:%02X:%02X:%02X:%02X",
>> + def[0], def[1], def[2], def[3], def[4], def[5]);
>> + setenv(envvar, str);
>> + }
>> +}
>
> No idea why you need it. The FEC driver takes care of MAC address and
> set the ethaddr variable. This seems to me wrong.
Agreed - I think this is a hold-over from when I started u-boot
support early last year. I will remove.
>
<snip>
>> +
>> +int board_phy_config(struct phy_device *phydev)
>> +{
>> + unsigned short val;
>> +
>> + /* Marvel 88E1510 */
>> + if (phydev->phy_id == 0x1410dd1) {
>> + /* Errata 3.1 - PHY initialization */
>> + phy_write(phydev, MDIO_DEVAD_NONE, 22, 0x00ff);
>> + phy_write(phydev, MDIO_DEVAD_NONE, 17, 0x214b);
>> + phy_write(phydev, MDIO_DEVAD_NONE, 16, 0x2144);
>> + phy_write(phydev, MDIO_DEVAD_NONE, 17, 0x0c28);
>> + phy_write(phydev, MDIO_DEVAD_NONE, 16, 0x2146);
>> + phy_write(phydev, MDIO_DEVAD_NONE, 17, 0xb233);
>> + phy_write(phydev, MDIO_DEVAD_NONE, 16, 0x214d);
>> + phy_write(phydev, MDIO_DEVAD_NONE, 17, 0xcc0c);
>> + phy_write(phydev, MDIO_DEVAD_NONE, 16, 0x2159);
>> + phy_write(phydev, MDIO_DEVAD_NONE, 22, 0x00fb);
>> + phy_write(phydev, MDIO_DEVAD_NONE, 7, 0xc00d);
>> + phy_write(phydev, MDIO_DEVAD_NONE, 22, 0);
>> +
>
> This does not seem to me the best way to do. IMHO you should add support
> for Marvel 88E1510 in drivers/net/phy/, maybe as Alaska phy.
ok - I can do that.
>
> In board_phy_config() should contain only board specific initialization
> - an errata means that the same initialization is common to all boards
> having this chip.
>
>> + /* LED configuration (See datasheet section 2.26.4)
>> + * LED[0] (SPD:Amber) R16_3.3:0 to 0111: on-GbE link
>> + * LED[1] (LNK:Green) R16_3.7:4 to 0001: on-link, blink-activity
>> + */
>> + phy_write(phydev, MDIO_DEVAD_NONE, 22, 3);
>> + val = phy_read(phydev, MDIO_DEVAD_NONE, 16);
>> + val &= 0xff00;
>> + val |= 0x0017;
>> + phy_write(phydev, MDIO_DEVAD_NONE, 16, val);
>> + phy_write(phydev, MDIO_DEVAD_NONE, 22, 0);
>> + }
>
> This is also part of the phy driver.
I would think LED configuration should be a board specific config as
its up to the board vendor what they want their LED's to do.
>
<snip>
>> +
>> +static void setup_board_gpio(const char *model)
>> +{
>> + const char *s;
>> + char arg[10];
>> + size_t len;
>> + int i;
>> + enum {
>> + GW51xx,
>> + GW52xx,
>> + GW53xx,
>> + GW54xx,
>> + UNKNOWN,
>> + };
>> + struct dio_cfg {
>> + iomux_v3_cfg_t gpio_padmux;
>> + unsigned gpio_param;
>> + iomux_v3_cfg_t pwm_padmux;
>> + unsigned pwm_param;
>> + };
>> + int board_type = UNKNOWN;
>> + struct dio_cfg dio_cfg[] = {
>> + /* GW51xx */
>> + { MX6_PAD_SD1_DAT0__GPIO1_IO16, IMX_GPIO_NR(1, 16),
>> + 0, 0 },
>> + { MX6_PAD_SD1_DAT2__GPIO1_IO19, IMX_GPIO_NR(1, 19),
>> + MX6_PAD_SD1_DAT2__PWM2_OUT, 2 },
>> + { MX6_PAD_SD1_DAT1__GPIO1_IO17, IMX_GPIO_NR(1, 17),
>> + MX6_PAD_SD1_DAT1__PWM3_OUT, 3 },
>> + { MX6_PAD_SD1_CMD__GPIO1_IO18, IMX_GPIO_NR(1, 18),
>> + MX6_PAD_SD1_CMD__PWM4_OUT, 4 },
>> +
>> + /* GW52xx */
>> + { MX6_PAD_SD1_DAT0__GPIO1_IO16, IMX_GPIO_NR(1, 16),
>> + 0, 0 },
>> + { MX6_PAD_SD1_DAT2__GPIO1_IO19, IMX_GPIO_NR(1, 19),
>> + MX6_PAD_SD1_DAT2__PWM2_OUT, 2 },
>> + { MX6_PAD_SD1_DAT1__GPIO1_IO17, IMX_GPIO_NR(1, 17),
>> + MX6_PAD_SD1_DAT1__PWM3_OUT, 3 },
>> + { MX6_PAD_SD1_CLK__GPIO1_IO20, IMX_GPIO_NR(1, 20),
>> + 0, 0 },
>> +
>> + /* GW53xx */
>> + { MX6_PAD_SD1_DAT0__GPIO1_IO16, IMX_GPIO_NR(1, 16),
>> + 0, 0 },
>> + { MX6_PAD_SD1_DAT2__GPIO1_IO19, IMX_GPIO_NR(1, 19),
>> + MX6_PAD_SD1_DAT2__PWM2_OUT, 2 },
>> + { MX6_PAD_SD1_DAT1__GPIO1_IO17, IMX_GPIO_NR(1, 17),
>> + MX6_PAD_SD1_DAT1__PWM3_OUT, 3 },
>> + { MX6_PAD_SD1_CLK__GPIO1_IO20, IMX_GPIO_NR(1, 20),
>> + 0, 0 },
>> +
>> + /* GW54xx */
>> + { MX6_PAD_GPIO_9__GPIO1_IO09, IMX_GPIO_NR(1, 9),
>> + MX6_PAD_GPIO_9__PWM1_OUT, 1 },
>> + { MX6_PAD_SD1_DAT2__GPIO1_IO19, IMX_GPIO_NR(1, 19),
>> + MX6_PAD_SD1_DAT2__PWM2_OUT, 2 },
>> + { MX6_PAD_SD4_DAT1__GPIO2_IO09, IMX_GPIO_NR(2, 9),
>> + MX6_PAD_SD4_DAT1__PWM3_OUT, 3 },
>> + { MX6_PAD_SD4_DAT2__GPIO2_IO10, IMX_GPIO_NR(2, 10),
>> + MX6_PAD_SD4_DAT2__PWM4_OUT, 4 },
>> + };
>> +
>> + if (strncasecmp(model, "GW51", 4) == 0) {
>> + board_type = GW51xx;
>> +
>> + /* PANLEDG# (GRN off) */
>> + imx_iomux_v3_setup_pad(MX6_PAD_KEY_COL0__GPIO4_IO06 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_output(IMX_GPIO_NR(4, 6), 1);
>> +
>
> Again, this is not straightforward and difficult to read. Use tables for
> each board variation you want to add and split between common code and
> specific code.
Ok - I'll put all the pad config in a model specific array at least.
>
>> + /* PANLEDR# (RED off) */
>> + imx_iomux_v3_setup_pad(MX6_PAD_KEY_ROW0__GPIO4_IO07 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_output(IMX_GPIO_NR(4, 7), 1);
>> +
>> + /* GPS_SHDN */
>> + imx_iomux_v3_setup_pad(MX6_PAD_GPIO_2__GPIO1_IO02 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_output(IMX_GPIO_NR(1, 2), 1);
>> +
>> + /* Analog video codec power enable */
>> + imx_iomux_v3_setup_pad(MX6_PAD_CSI0_DATA_EN__GPIO5_IO20 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_output(IMX_GPIO_NR(5, 20), 1);
>> +
>> + /* Expansion IO0 - PWREN# */
>> + imx_iomux_v3_setup_pad(MX6_PAD_EIM_A19__GPIO2_IO19 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_output(IMX_GPIO_NR(2, 19), 0);
>> +
>> + /* Expansion IO1 - IRQ# */
>> + imx_iomux_v3_setup_pad(MX6_PAD_EIM_A20__GPIO2_IO18 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_input(IMX_GPIO_NR(2, 18));
>> + } /* end GW51xx */
>> +
>> + else if (strncasecmp(model, "GW52", 4) == 0) {
>> + board_type = GW52xx;
>> +
>> + /* PANLEDG# (GRN off) */
>> + imx_iomux_v3_setup_pad(MX6_PAD_KEY_COL0__GPIO4_IO06 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_output(IMX_GPIO_NR(4, 6), 1);
>> +
>> + /* PANLEDR# (RED off) */
>> + imx_iomux_v3_setup_pad(MX6_PAD_KEY_ROW0__GPIO4_IO07 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_output(IMX_GPIO_NR(4, 7), 1);
>> +
>> + /* MX6_LOCLED# (off) */
>> + imx_iomux_v3_setup_pad(MX6_PAD_KEY_ROW4__GPIO4_IO15 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_output(IMX_GPIO_NR(4, 15), 1);
>> +
>> + /* GPS_SHDN */
>> + imx_iomux_v3_setup_pad(MX6_PAD_ENET_RXD0__GPIO1_IO27 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_output(IMX_GPIO_NR(1, 27), 1);
>> +
>> + /* Expansion IO0 - PWREN# */
>> + imx_iomux_v3_setup_pad(MX6_PAD_EIM_A19__GPIO2_IO19 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_output(IMX_GPIO_NR(2, 19), 0);
>> +
>> + /* Expansion IO1 - IRQ# */
>> + imx_iomux_v3_setup_pad(MX6_PAD_EIM_A20__GPIO2_IO18 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_input(IMX_GPIO_NR(2, 18));
>> +
>> + /* MSATA Enable */
>> + imx_iomux_v3_setup_pad(MX6_PAD_SD4_DAT0__GPIO2_IO08 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + if (is_cpu_type(MXC_CPU_MX6Q)) {
>> + gpio_direction_output(IMX_GPIO_NR(2, 8),
>> + (hwconfig("msata")) ? 1 : 0);
>> + printf("MSATA: %s\n", (hwconfig("msata") ?
>> + "enabled" : "disabled"));
>> + } else {
>> + gpio_direction_output(IMX_GPIO_NR(2, 8), 0);
>> + }
>> +
>> + /* USBOTG Select (PCISKT or FrontPanel) */
>> + imx_iomux_v3_setup_pad(MX6_PAD_GPIO_2__GPIO1_IO02 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_output(IMX_GPIO_NR(1, 2), 0);
>> +
>> + /* Analog video codec power enable */
>> + imx_iomux_v3_setup_pad(MX6_PAD_EIM_D31__GPIO3_IO31 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_output(IMX_GPIO_NR(3, 31), 1);
>> +
>> + /* UART2_EN# */
>> + imx_iomux_v3_setup_pad(MX6_PAD_SD4_DAT3__GPIO2_IO11 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + printf("RS232: %s\n", (hwconfig("rs232")) ?
>> + "enabled" : "disabled");
>> + gpio_direction_output(IMX_GPIO_NR(2, 11),
>> + (hwconfig("rs232")) ? 0 : 1);
>> + /* TODO: flush UART RX FIFO after disable */
>> + } /* end GW52xx */
>> +
>> + else if (strncasecmp(model, "GW53", 4) == 0) {
>> + board_type = GW53xx;
>> +
>> + /* PANLEDG# (GRN off) */
>> + imx_iomux_v3_setup_pad(MX6_PAD_KEY_COL0__GPIO4_IO06 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_output(IMX_GPIO_NR(4, 6), 1);
>> +
>> + /* PANLEDR# (RED off) */
>> + imx_iomux_v3_setup_pad(MX6_PAD_KEY_ROW0__GPIO4_IO07 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_output(IMX_GPIO_NR(4, 7), 1);
>> +
>> + /* MX6_LOCLED# (off) */
>> + imx_iomux_v3_setup_pad(MX6_PAD_KEY_ROW4__GPIO4_IO15 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_output(IMX_GPIO_NR(4, 15), 1);
>> +
>> + /* GPS_SHDN */
>> + imx_iomux_v3_setup_pad(MX6_PAD_ENET_RXD0__GPIO1_IO27 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_output(IMX_GPIO_NR(1, 27), 1);
>> +
>> + /* Expansion IO0 - PWREN# */
>> + imx_iomux_v3_setup_pad(MX6_PAD_EIM_A19__GPIO2_IO19 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_output(IMX_GPIO_NR(2, 19), 0);
>> +
>> + /* Expansion IO1 - IRQ# */
>> + imx_iomux_v3_setup_pad(MX6_PAD_EIM_A20__GPIO2_IO18 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_input(IMX_GPIO_NR(2, 18));
>> +
>> + /* MSATA Enable */
>> + imx_iomux_v3_setup_pad(MX6_PAD_SD4_DAT0__GPIO2_IO08 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + if (is_cpu_type(MXC_CPU_MX6Q)) {
>> + gpio_direction_output(IMX_GPIO_NR(2, 8),
>> + (hwconfig("msata")) ? 1 : 0);
>> + printf("MSATA: %s\n", (hwconfig("msata") ?
>> + "enabled" : "disabled"));
>> + } else {
>> + gpio_direction_output(IMX_GPIO_NR(2, 8), 0);
>> + }
>> +
>> + /* Analog video codec power enable */
>> + imx_iomux_v3_setup_pad(MX6_PAD_EIM_D31__GPIO3_IO31 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_output(IMX_GPIO_NR(3, 31), 1);
>> +
>> + /* UART2_EN# */
>> + imx_iomux_v3_setup_pad(MX6_PAD_SD4_DAT3__GPIO2_IO11 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + printf("RS232: %s\n", (hwconfig("rs232")) ?
>> + "enabled" : "disabled");
>> + gpio_direction_output(IMX_GPIO_NR(2, 11),
>> + (hwconfig("rs232")) ? 0 : 1);
>> + /* TODO: flush UART RX FIFO after disable */
>> + } /* end GW53xx */
>> +
>> + else if (strncasecmp(model, "GW54", 4) == 0) {
>> + board_type = GW54xx;
>> + if (strncasecmp(model, "GW5400-A", 8) == 0) {
>> + /* PANLEDG# (GRN off) */
>> + imx_iomux_v3_setup_pad(MX6_PAD_KEY_COL0__GPIO4_IO06 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_output(IMX_GPIO_NR(4, 6), 1);
>> +
>> + /* PANLEDR# (RED off) */
>> + imx_iomux_v3_setup_pad(MX6_PAD_KEY_COL2__GPIO4_IO10 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_output(IMX_GPIO_NR(4, 10), 1);
>> +
>> + /* MX6_LOCLED# (off) */
>> + imx_iomux_v3_setup_pad(MX6_PAD_KEY_ROW4__GPIO4_IO15 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_output(IMX_GPIO_NR(4, 15), 1);
>> +
>> + /* MIPI DIO */
>> + imx_iomux_v3_setup_pad(MX6_PAD_SD1_DAT3__GPIO1_IO21 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> +
>> + /* RS485 Transmit Enable */
>> + imx_iomux_v3_setup_pad(MX6_PAD_EIM_D24__GPIO3_IO24 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_output(IMX_GPIO_NR(3, 24), 0);
>> +
>> + /* Expansion IO0 - PWREN# */
>> + imx_iomux_v3_setup_pad(MX6_PAD_KEY_ROW0__GPIO4_IO07 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_output(IMX_GPIO_NR(4, 7), 0);
>> +
>> + /* Expansion IO1 - IRQ# */
>> + imx_iomux_v3_setup_pad(MX6_PAD_KEY_ROW1__GPIO4_IO09 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_input(IMX_GPIO_NR(4, 9));
>> + }
>> +
>> + else if ((strncasecmp(model, "GW5400", 6) == 0) ||
>> + (strncasecmp(model, "GW5410", 6) == 0)
>> + ) {
>> + /* PANLEDG# (GRN off) */
>> + imx_iomux_v3_setup_pad(MX6_PAD_KEY_COL0__GPIO4_IO06 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_output(IMX_GPIO_NR(4, 6), 1);
>> +
>> + /* PANLEDR# (RED off) */
>> + imx_iomux_v3_setup_pad(MX6_PAD_KEY_ROW0__GPIO4_IO07 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_output(IMX_GPIO_NR(4, 7), 1);
>> +
>> + /* MX6_LOCLED# (off) */
>> + imx_iomux_v3_setup_pad(MX6_PAD_KEY_ROW4__GPIO4_IO15 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_output(IMX_GPIO_NR(4, 15), 1);
>> +
>> + /* RS485 Transmit Enable */
>> + imx_iomux_v3_setup_pad(MX6_PAD_SD3_DAT4__GPIO7_IO01 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_output(IMX_GPIO_NR(7, 1), 0);
>> +
>> + /* Expansion IO0 - PWREN# */
>> + imx_iomux_v3_setup_pad(MX6_PAD_EIM_A19__GPIO2_IO19 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_output(IMX_GPIO_NR(2, 19), 0);
>> +
>> + /* Expansion IO1 - IRQ# */
>> + imx_iomux_v3_setup_pad(MX6_PAD_EIM_A20__GPIO2_IO18 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_input(IMX_GPIO_NR(2, 18));
>> +
>> + /* MSATA Enable */
>> + imx_iomux_v3_setup_pad(MX6_PAD_SD4_DAT0__GPIO2_IO08 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + if (is_cpu_type(MXC_CPU_MX6Q)) {
>> + gpio_direction_output(IMX_GPIO_NR(2, 8),
>> + (hwconfig("msata")) ?
>> + 1 : 0);
>> + printf("MSATA: %s\n", (hwconfig("msata") ?
>> + "enabled" : "disabled"));
>> + } else {
>> + gpio_direction_output(IMX_GPIO_NR(2, 8), 0);
>> + }
>> +
>> + /* Analog video codec power enable */
>> + imx_iomux_v3_setup_pad(MX6_PAD_EIM_D31__GPIO3_IO31 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_output(IMX_GPIO_NR(3, 31), 1);
>> + }
>> +
>> + /* UART2_EN# */
>> + imx_iomux_v3_setup_pad(MX6_PAD_SD4_DAT3__GPIO2_IO11 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + printf("RS232: %s\n", (hwconfig("rs232")) ?
>> + "enabled" : "disabled");
>> + gpio_direction_output(IMX_GPIO_NR(2, 11),
>> + (hwconfig("rs232")) ? 0 : 1);
>> + /* TODO: flush UART RX FIFO after disable */
>> +
>> + /* DIOI2C_DIS# */
>> + imx_iomux_v3_setup_pad(MX6_PAD_GPIO_19__GPIO4_IO05 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_output(IMX_GPIO_NR(4, 5), 0);
>> + } /* end GW54xx */
>> +
>> + /* Configure DIO pinmux/padctl registers
>> + * see IMX6DQRM/IMX6SDLRM IOMUXC_SW_PAD_CTL_PAD_* register definitions
>> + */
>> + if (board_type > UNKNOWN)
>> + return;
>> + for (i = 0; i < 4; i++) {
>> + struct dio_cfg *cfg = &dio_cfg[(4*board_type)+i];
>> + unsigned ctrl = DIO_PAD_CTRL;
>> +
>> + sprintf(arg, "dio%d", i);
>> + if (hwconfig(arg)) {
>> + s = hwconfig_subarg(arg, "padctrl", &len);
>> + if (s)
>> + ctrl = simple_strtoul(s, NULL, 16) & 0x3ffff;
>> + if (hwconfig_subarg_cmp(arg, "mode", "gpio")) {
>> + printf("DIO%d: GPIO%d_IO%02d (gpio-%d)\n", i,
>> + (cfg->gpio_param/32)+1,
>> + cfg->gpio_param%32,
>> + cfg->gpio_param);
>> + imx_iomux_v3_setup_pad(cfg->gpio_padmux |
>> + MUX_PAD_CTRL(ctrl));
>> + gpio_direction_input(cfg->gpio_param);
>> + } else if (hwconfig_subarg_cmp("dio2", "mode", "pwm") &&
>> + cfg->pwm_padmux) {
>> + printf("DIO%d: pwm%d\n", i, cfg->pwm_param);
>> + imx_iomux_v3_setup_pad(cfg->pwm_padmux |
>> + MUX_PAD_CTRL(ctrl));
>> + }
>> + }
>> + }
>> +}
>> +
>> +static int setup_pcie(void)
>> +{
>> + struct ventana_board_info *info = &ventana_info;
>> +
>> + if ((strncasecmp((const char *)info->model, "GW51", 4) == 0)) {
>> + /* assert PCI_RST#
>> + * (will be released by OS when clock is valid) */
>> + imx_iomux_v3_setup_pad(MX6_PAD_GPIO_0__GPIO1_IO00 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_output(IMX_GPIO_NR(1, 0), 0);
>> + } else if ((strncasecmp((const char *)info->model, "GW52", 4) == 0)) {
>> + /* assert PCI_RST#
>> + * (will be released by OS when clock is valid) */
>> + imx_iomux_v3_setup_pad(MX6_PAD_ENET_TXD1__GPIO1_IO29 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_output(IMX_GPIO_NR(1, 29), 0);
>> + } else if ((strncasecmp((const char *)info->model, "GW53", 4) == 0)) {
>> + /* assert PCI_RST#
>> + * (will be released by OS when clock is valid) */
>> + imx_iomux_v3_setup_pad(MX6_PAD_ENET_TXD1__GPIO1_IO29 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_output(IMX_GPIO_NR(1, 29), 0);
>> + } else if ((strncasecmp((const char *)info->model, "GW54", 4) == 0)) {
>> + /* PCICK_SSON: disable spread-spectrum clock */
>> + imx_iomux_v3_setup_pad(MX6_PAD_SD1_CLK__GPIO1_IO20 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_output(IMX_GPIO_NR(1, 20), 0);
>> +
>> + /* assert PCI_RST#
>> + * (will be released by OS when clock is valid) */
>> + imx_iomux_v3_setup_pad(MX6_PAD_ENET_TXD1__GPIO1_IO29 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_output(IMX_GPIO_NR(1, 29), 0);
>> + }
>> +
>> + return 0;
>> +}
>
> Please take a look at Marek's later merged patches that add PCI-E
> support for i.MX6 to U-Boot, and rebase on top of it. I can suggest to
> use u-boot-imx as base for your patches, because patches are already merged.
I'm aware of Marek's patches but wasn't sure when they were scheduled
to go in. I will rebase against u-boot-imx and put the above in the
new imx6_pcie_toggle_reset function.
>
> Generally, I think you should change the behavior of selecting data on
> depend of the board. Instead of checkig in each function which is the
> board, you should add table containinig all required values, and choose
> one of them in only one function.
pcie reset, gpio default configuration (pinmux and direction/level),
and the hwmon registers are the only places that are model dependent.
I feel it is more readable to leave the model checks in place instead
of trying to create structures that have all of that in them.
>
>> +
>> +#if defined(CONFIG_VIDEO_IPUV3)
>> +
>> +static iomux_v3_cfg_t const backlight_pads[] = {
>> + /* Backlight on MIPI connector: J16 */
>> + MX6_PAD_SD2_CMD__GPIO1_IO11 | MUX_PAD_CTRL(NO_PAD_CTRL),
>> +
>> + /* Backlight CABEN on LVDS connector: J6 */
>> + MX6_PAD_SD2_CLK__GPIO1_IO10 | MUX_PAD_CTRL(NO_PAD_CTRL),
>> +};
>> +
>> +struct display_info_t {
>> + int bus;
>> + int addr;
>> + int pixfmt;
>> + int (*detect)(struct display_info_t const *dev);
>> + void (*enable)(struct display_info_t const *dev);
>> + struct fb_videomode mode;
>> +};
>> +
>> +
>> +static int detect_hdmi(struct display_info_t const *dev)
>> +{
>> + struct hdmi_regs *hdmi = (struct hdmi_regs *)HDMI_ARB_BASE_ADDR;
>> + return readb(&hdmi->phy_stat0) & HDMI_PHY_HPD;
>> +}
>> +
>> +static void do_enable_hdmi(struct display_info_t const *dev)
>> +{
>> + imx_enable_hdmi_phy();
>> +}
>> +
>> +static int detect_i2c(struct display_info_t const *dev)
>> +{
>> + return ((0 == i2c_set_bus_num(dev->bus)) &&
>> + (0 == i2c_probe(dev->addr)));
>> +}
>> +
>> +static void enable_lvds(struct display_info_t const *dev)
>> +{
>> + struct iomuxc *iomux = (struct iomuxc *)
>> + IOMUXC_BASE_ADDR;
>> +
>> + /* set CH0 data width to 24bit (IOMUXC_GPR2:5 0=18bit, 1=24bit) */
>> + u32 reg = readl(&iomux->gpr[2]);
>> + reg |= IOMUXC_GPR2_DATA_WIDTH_CH0_24BIT;
>> + writel(reg, &iomux->gpr[2]);
>> +
>> + /* Disable CABC:
>> + * when enabled this feature sets backlight automatically according
>> + * to content which may cause annoying unstable backlight issue
>> + */
>> + gpio_direction_output(IMX_GPIO_NR(1, 10), 0);
>> +
>> + /* Enable Backlight */
>> + imx_iomux_v3_setup_pad(MX6_PAD_SD1_CMD__GPIO1_IO18 |
>> + MUX_PAD_CTRL(NO_PAD_CTRL));
>> + gpio_direction_output(IMX_GPIO_NR(1, 18), 1);
>> +}
>> +
>> +static struct display_info_t const displays[] = {{
>> + /* HDMI Output */
>> + .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
>> +} }, {
>> + /* HannStar HSD100PXN1-A00 with egalx_ts controller
>> + * (aka Freescale MXC-LVDS1 10" 1024x768 60Hz LCD touchscreen)
>> + */
>> + .bus = 2,
>> + .addr = 0x4,
>> + .pixfmt = IPU_PIX_FMT_LVDS666,
>> + .detect = detect_i2c,
>> + .enable = enable_lvds,
>> + .mode = {
>> + .name = "Hannstar-XGA",
>> + .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
>> +} } };
>> +
>> +int board_video_skip(void)
>> +{
>> + int i;
>> + int ret;
>> + char const *panel = getenv("panel");
>> + if (!panel) {
>> + for (i = 0; i < ARRAY_SIZE(displays); i++) {
>> + struct display_info_t const *dev = displays+i;
>> + if (dev->detect(dev)) {
>> + panel = dev->mode.name;
>> + printf("auto-detected panel %s\n", panel);
>> + break;
>> + }
>> + }
>> + if (!panel) {
>> + panel = displays[0].mode.name;
>> + i = 0;
>> + printf("No panel detected: default to %s\n", panel);
>> + }
>> + } else {
>> + for (i = 0; i < ARRAY_SIZE(displays); i++) {
>> + if (!strcmp(panel, displays[i].mode.name))
>> + break;
>> + }
>> + }
>> + if (i < ARRAY_SIZE(displays)) {
>> + ret = ipuv3_fb_init(&displays[i].mode, 0,
>> + displays[i].pixfmt);
>> + if (!ret) {
>> + displays[i].enable(displays+i);
>> + printf("DISP: %s (%ux%u)\n",
>> + displays[i].mode.name,
>> + displays[i].mode.xres,
>> + displays[i].mode.yres);
>> + } else
>> + printf("LCD %s cannot be configured: %d\n",
>> + displays[i].mode.name, ret);
>> + } else {
>> + printf("unsupported panel %s\n", panel);
>> + ret = -EINVAL;
>> + }
>> + return (0 != ret);
>> +
> }
>
> Nak. This function is more or less copied from another board. There is
> still a patch doing this and my suggestion is to factorize the part in
> board_video_skip(), taht is not really strict bound to the board.
Yes, both mx6sabresd and nitrogen6x use this common code and I saw the
proposed patch to add it to wandboard as well with your response
asking for it to be made common. I'll check with Otavio to see if he
is working on something already. I agree some common support could
help here.
>
>> +
>> +static void setup_display(void)
>> +{
>> + struct mxc_ccm_reg *mxc_ccm = (struct mxc_ccm_reg *)CCM_BASE_ADDR;
>> + struct iomuxc *iomux = (struct iomuxc *)IOMUXC_BASE_ADDR;
>> + int reg;
>> +
>> + enable_ipu_clock();
>> + imx_setup_hdmi();
>> +
>> + /* Turn on LDB0,IPU,IPU DI0 clocks */
>> + reg = readl(&mxc_ccm->CCGR3);
>> + reg |= MXC_CCM_CCGR3_LDB_DI0_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;
>> + writel(reg, &mxc_ccm->cscmr2);
>> +
>> + reg = readl(&mxc_ccm->chsccdr);
>> + reg |= (CHSCCDR_CLK_SEL_LDB_DI0
>> + <<MXC_CCM_CHSCCDR_IPU1_DI0_CLK_SEL_OFFSET);
>> + writel(reg, &mxc_ccm->chsccdr);
>> +
>> + reg = IOMUXC_GPR2_BGREF_RRMODE_EXTERNAL_RES
>> + |IOMUXC_GPR2_DI1_VS_POLARITY_ACTIVE_HIGH
>> + |IOMUXC_GPR2_DI0_VS_POLARITY_ACTIVE_LOW
>> + |IOMUXC_GPR2_BIT_MAPPING_CH1_SPWG
>> + |IOMUXC_GPR2_DATA_WIDTH_CH1_18BIT
>> + |IOMUXC_GPR2_BIT_MAPPING_CH0_SPWG
>> + |IOMUXC_GPR2_DATA_WIDTH_CH0_18BIT
>> + |IOMUXC_GPR2_LVDS_CH1_MODE_DISABLED
>> + |IOMUXC_GPR2_LVDS_CH0_MODE_ENABLED_DI0;
>> + writel(reg, &iomux->gpr[2]);
>> +
>> + reg = readl(&iomux->gpr[3]);
>> + reg = (reg & ~IOMUXC_GPR3_LVDS0_MUX_CTL_MASK)
>> + | (IOMUXC_GPR3_MUX_SRC_IPU1_DI0
>> + <<IOMUXC_GPR3_LVDS0_MUX_CTL_OFFSET);
>> + writel(reg, &iomux->gpr[3]);
>> +
>> + /* backlights off until needed */
>> + imx_iomux_v3_setup_multiple_pads(backlight_pads,
>> + ARRAY_SIZE(backlight_pads));
>> + gpio_direction_input(IMX_GPIO_NR(1, 10)); /* LVDS */
>> + gpio_direction_input(IMX_GPIO_NR(1, 11)); /* MIPI */
>> +}
>> +#endif /* CONFIG_VIDEO_IPUV3 */
>> +
>> +static int setup_pmic_voltages(void)
>> +{
>> + int ret;
>> + unsigned char value, rev_id = 0;
>> +
>> + ret = i2c_set_bus_num(1);
>> + if (ret)
>> + return ret;
>> + if (!i2c_probe(0x8)) {
>
> Nak. We have a generic PMIX framework. Please use it.
agreed - I see that now and will revise.
>
>> +/*
>> + * Board Support
>> + */
>> +
>> +/*
>> + * Do not overwrite the console
>> + * Use always serial for U-Boot console
>> + */
>> +int overwrite_console(void)
>
> This is not required, please drop it.
will do
>
>> +{
>> + return 1;
>> +}
>> +
>> +
>> +/*
>> + * very early in the call chain - setup SoC peripherals
>> + * (NB: Can not printf from here)
>> + */
>> +int board_early_init_f(void)
>> +{
>> + setup_iomux_uart();
>> +#ifdef CONFIG_MXC_SPI
>> + setup_spi();
>
> board_early_init_f() is called before relocation and malloc() is not
> available. SPI uses malloc(), then you should move setup_spi to a later
> point.
ok - will move.
>
>> +#endif
>> + gpio_direction_output(IMX_GPIO_NR(3, 22), 0); /* OTG power off */
>> +
>> + /* Note this gets called again later,
>> + * but needed in case i2c bus is stuck */
>> + timer_init();
>
> timer_init() ? I think this is called by generic code.
it is, but as the common states its called later (at least it was when
I first wrote this) and causes one of the error handling functions to
hang when it tries to delay. The specific issue I was working around
here had to do with no console output from the bootloader if you had
for example an HDMI monitor plugged in with a bad EDID clk/data pin.
I'll check to see if this is still needed as this code was written and
tested some time ago.
>
>> + 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);
>> +#if defined(CONFIG_VIDEO_IPUV3)
>> + setup_display();
>> +#endif
>> +
>> + return 0;
>> +}
>> +
>> +#if defined(CONFIG_DISPLAY_BOARDINFO)
>> +/* Identify board and display banner/info
>> + */
>> +#define SRC_SBMR1 0x020d8004 /* holds BOOT_CFG1-BOOT_CFG4 from eFuse/pins */
>> +#define SRC_SBMR2 0x020d801c
>> +#define SRC_GPR9 0x020d8040 /* holds copy of BOOT_CFG1-BOOT_CFG4 acted on */
>> +#define SRC_GPR10 0x020d8044
>> +
>
> No access to internal register using absolute addresses. Use structures
> in imx_regs.h. that is struct src->sbmrX
ack
>
>> +/* this reads boot_cfg efuse/pins - does not reflect what actually booted
>> + */
>> +void show_boot_mode(uint boot_cfg)
>> +{
>> + switch ((boot_cfg & 0x000000ff) >> 4) {
>> + case 0x2:
>> + printf("SATA");
>> + break;
>> + case 0x3:
>> + printf("SPI NOR");
>> + break;
>> + case 0x4:
>> + case 0x5:
>> + /* BOOT_CFG2[3:4] is devno */
>> + printf(" SD%d", (boot_cfg & 0x00001800) >> 11);
>> + break;
>> + case 0x6:
>> + case 0x7:
>> + /* BOOT_CFG2[3:4] is devno */
>> + printf(" MMC%d", (boot_cfg & 0x00001800) >> 11);
>> + break;
>> + case 0x8 ... 0xf:
>> + printf("NAND");
>> + break;
>> + default:
>> + printf("Unknown");
>> + break;
>> + }
>> + printf(" 0x%08x\n", boot_cfg);
>> +}
>> +
>
> This does not belog here. Please move this code in a separate patch and
> make it available to all boards. Code should flow into
> ./arch/arm/imx-common. Please add a function getting an enumated value
> as return value, not only a function that output the result.
>
> Use puts() instead of printf() if you print a constant string.
>
> Instead of:
> printf("NAND");
>
> Use:
> puts("NAND");
>
ack
>> +
>> +int checkboard(void)
>> +{
>> + struct ventana_board_info *info = &ventana_info;
>> + uint src_sbmr2 = readl(SRC_SBMR2);
>> + uint src_gpr10 = readl(SRC_GPR10);
>> +
>> + /* check for valid i2c busses - if one was 'stuck' it did not get
>> + * initialized
>> + */
>> + if (i2c_set_bus_num(0))
>> + printf("invalid /dev/i2c-0\n");
>
> /dev has no meaning in U-Boot.
will fix
>
>> + if (i2c_set_bus_num(1))
>> + printf("invalid /dev/i2c-1\n");
>> + if (i2c_set_bus_num(2))
>> + printf("invalid /dev/i2c-2\n");
>> + read_eeprom();
>> + if (!(src_sbmr2 & 1<<4)) {
>> + /* can consider this 'manufacturing mode' if needed */
>> + printf("First boot - eFUSE not blown\n");
>> + }
>> +
>> + printf("APP_IMAGE: %s\n", (src_gpr10 & 1<<30) ?
>> + "Secondary" : "Primary");
>> + if (src_gpr10 & 1<<29)
>> + printf("NAND: bad blocks in application image\n");
>> +
>> +#if defined(CONFIG_ENV_IS_IN_SPI_FLASH)
>> + printf("Env: SPI FLASH\n");
>> +#elif defined(CONFIG_ENV_IS_IN_MMC)
>> + printf("Env: MMC\n");
>> +#elif defined(CONFIG_ENV_IS_IN_NAND)
>> + printf("Env: NAND FLASH\n");
>> +#endif
>> +
>> + /* SRC_SBMR1 reflects eFUSE/pin */
>> + printf("BOOT_CFG: ");
>> + show_boot_mode(readl(SRC_SBMR1));
>> + /* SRC_GPR9 reflects what was actually booted off of if not 0
>> + * (ie if bmode was used) */
>> + if (readl(SRC_GPR9)) {
>> + printf("BMODE: ");
>> + show_boot_mode(readl(SRC_GPR9));
>> + }
>> + printf("\n");
>> + printf("Gateworks Corporation Copyright 2014\n");
>> + if (info->model[0]) {
>> + printf("Model: %s\n", info->model);
>> + printf("MFGDate: %02x-%02x-%02x%02x\n",
>> + info->mfgdate[0], info->mfgdate[1],
>> + info->mfgdate[2], info->mfgdate[3]);
>> + printf("Serial:%d\n", info->serial);
>> + } else {
>> + printf("Invalid EEPROM - board will not function fully\n");
>> + }
>> +
>> + return 0;
>> +}
>> +#endif
>
> General remak: you print a lot of stuff by booting. This can slow down
> your boot process significantly adding several seconds to your start up,
> before the kernel is loaded.
agreed - perhaps there are some common defines I can use to quiet
things down by default or even better an env variable that the user
can set?
We've found that showing the boot device, and model in the bootloader
is extremely helpfull in supporting users but I suppose I could move
some of the other things like DIO/SATA/RS232 configuration (hwconfig
controlled) reporting to a user command.
>
>> +
>> +/* Set gd->ram_size
>> + */
>> +int dram_init(void)
>> +{
>> + struct ventana_board_info *info = &ventana_info;
>> +
>> + gd->ram_size = ((ulong)CONFIG_DDR_MB * 1024 * 1024);
>> + if (info->model[0] && info->sdram_size > 0 && info->sdram_size < 9) {
>> + int i = info->sdram_size;
>> + gd->ram_size = 32*1024*1024;
>> + while (--i)
>> + gd->ram_size *= 2;
>> + }
>
> Use get_ramsize to dynamically find the size of the installed RAM.
will do - looking at doc/README.memory-test it seems that
get_ram_size() should be very fast and can also catch hardware related
memory errors.
>
>> +/* initialize periperhals
>> + */
>> +int board_init(void)
>> +{
>> + int ret = 0;
>> + struct iomuxc_base_regs *const iomuxc_regs
>> + = (struct iomuxc_base_regs *)IOMUXC_BASE_ADDR;
>> +
>> +#ifdef CONFIG_CMD_NAND
>> + setup_gpmi_nand();
>> +#endif
>> +
>> + clrsetbits_le32(&iomuxc_regs->gpr[1],
>> + IOMUXC_GPR1_OTG_ID_MASK,
>> + IOMUXC_GPR1_OTG_ID_GPIO1);
>> +
>> + imx_iomux_v3_setup_multiple_pads(usb_pads, ARRAY_SIZE(usb_pads));
>> +
>> + /* address of linux boot parameters */
>> + gd->bd->bi_boot_params = PHYS_SDRAM + 0x100;
>> +
>> +#ifdef CONFIG_SYS_I2C_MXC
>> + ret = setup_pmic_voltages();
>> + if (ret)
>> + return -1;
>> +#endif
>> +
>> + setup_pcie();
>> +
>> +#ifdef CONFIG_CMD_SATA
>> + setup_sata();
>> +#endif
>> +
>> + if (!i2c_set_bus_num(0)) {
>> + unsigned char buf[4];
>> + if (!gsc_i2c_read(0x20, 14, 1, buf, 1)) {
>> + printf("GSC: v%d", buf[0]);
>> + if (!gsc_i2c_read(0x20, 10, 1, buf, 4)) {
>> + /* show firmware revision and CRC */
>> + printf(" 0x%04x", buf[2] | buf[3]<<8);
>> + /* show status register */
>
> I do not understand the code here. This should belong to checkboard().
it is displaying info about firmware revision and status of GSC - I
will move it to checkboard().
>
>> +/* late init
>> + */
>> +int misc_init_r(void)
>> +{
>> + /* set env vars based on board model from EEPROM */
>> + if (ventana_info.model[0]) {
>> + char str[sizeof(ventana_info.model)];
>> + char fdt[sizeof(ventana_info.model)+20];
>> + char *p;
>> + int i;
>> + const char *prefix = "";
>> +
>> + if (is_cpu_type(MXC_CPU_MX6Q))
>> + prefix = "imx6q";
>> + else if (is_cpu_type(MXC_CPU_MX6DL))
>> + prefix = "imx6dl";
>> +
>> + memset(str, 0, sizeof(str));
>> + for (i = 0; i < (sizeof(ventana_info.model)-1) &&
>> + ventana_info.model[i]; i++)
>> + str[i] = tolower(ventana_info.model[i]);
>> + if (!getenv("model"))
>> + setenv("model", str);
>> + if (!getenv("fdt_file")) {
>> + sprintf(fdt, "%s-%s.dtb", prefix, str);
>> + setenv("fdt_file", fdt);
>> + }
>> + p = strchr(str, '-');
>> + if (p) {
>> + *p++ = 0;
>> +
>> + setenv("model_base", str);
>> + if (!getenv("fdt_file1")) {
>> + sprintf(fdt, "%s-%s.dtb", prefix, str);
>> + setenv("fdt_file1", fdt);
>> + }
>> + str[4] = 'x';
>> + str[5] = 'x';
>> + str[6] = 0;
>> + if (!getenv("fdt_file2")) {
>> + sprintf(fdt, "%s-%s.dtb", prefix, str);
>> + setenv("fdt_file2", fdt);
>> + }
>> + }
>> + get_mac("ethaddr", ventana_info.mac0);
>> + get_mac("eth1addr", ventana_info.mac1);
>> + sprintf(str, "%6d", ventana_info.serial);
>> + setenv("serial#", str);
>> + setup_board_gpio(getenv("model"));
>> + }
>> +
>> + /* generate a random eth mac if no EEPROM (1st boot - mfg mode) */
>> + else {
>> + u32 ethaddr_low, ethaddr_high;
>> + char str[20];
>> +
>> + /* use Device Unique ID bits 0-64 from eFUSE
>> + * (OCOTP_CFG1/OCOTP_CFG2) */
>> + fuse_read(0, 1, ðaddr_low);
>> + fuse_read(0, 2, ðaddr_high);
>> +
>> + /*
>> + * setting the 2nd LSB in the most significant byte of
>> + * the address makes it a locally administered ethernet
>> + * address
>> + */
>> + ethaddr_high &= 0xfeff;
>> + ethaddr_high |= 0x0200;
>> + sprintf(str, "%02X:%02X:%02X:%02X:%02X:%02X",
>> + ethaddr_high >> 8, ethaddr_high & 0xff,
>> + ethaddr_low >> 24, (ethaddr_low >> 16) & 0xff,
>> + (ethaddr_low >> 8) & 0xff, ethaddr_low & 0xff);
>> + printf("### Setting random MAC address = \"%s\"\n", str);
>> + setenv("ethaddr", str);
>> + }
>
> Apart that this functionality should not be added to misc_init(), but to
> the corresponding entry point (board_eth_init for example), I do not
> think it is correct. If a random address can be generated, it should be
> part of the FEC driver. The FEC already reads from fuses, that makes the
> code here useless.
Agreed - I never noticed that was available in common code. I will remove it.
>
>> +
>> +#ifdef CONFIG_CMD_BMODE
>> + add_board_boot_modes(board_boot_modes);
>> +#endif
>> +
>> + /* disable GSC boot watchdog
>> + *
>> + * The Gateworks System Controller implements a boot
>> + * watchdog (always enabled) to cover things like ERR006282 which can
>> + * lead to random boot failures.
>> + */
>> + if (!i2c_set_bus_num(0)) {
>> + unsigned char val;
>> + if (!gsc_i2c_read(0x20, 1, 1, &val, 1)) {
>> + val |= 0x80;
>> + if (gsc_i2c_write(0x20, 1, 1, &val, 1))
>> + printf("Error: could not disable GSC Watchdog\n");
>> + } else {
>> + printf("Error: could not disable GSC Watchdog\n");
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +#if defined(CONFIG_OF_LIBFDT) && defined(CONFIG_OF_BOARD_SETUP)
>> +void ft_board_setup(void *blob, bd_t *bd)
>> +{
>> + struct ventana_board_info *info = &ventana_info;
>> + struct node_info nodes[] = {
>> + { "sst,w25q256", MTD_DEV_TYPE_NOR, }, /* SPI flash */
>> + { "fsl,imx6q-gpmi-nand", MTD_DEV_TYPE_NAND, }, /* NAND flash */
>> + };
>> + const char *model = getenv("model");
>> +
>> + if (getenv("fdt_noauto")) {
>> + printf(" Skiping ft_board_setup (fdt_noauto defined)\n");
>> + return;
>> + }
>> +
>> + /* MTD partitions
>> + * Update partition nodes using info from mtdparts env var
>> + */
>> + printf(" Updating MTD partitions...\n");
>> + fdt_fixup_mtdparts(blob, nodes, ARRAY_SIZE(nodes));
>> +
>> + if (!model) {
>> + printf("invalid board info: Leaving FDT fully enabled\n");
>> + return;
>> + }
>> + printf(" Adjusting FDT per EEPROM for %s...\n", model);
>> +
>> + /* Note that fdt_fixup_ethernet is called in arm/lib/bootm before this
>> + * which sets mac-address and local-mac-address properties of
>> + * ethernet<n> aliases to ethaddr...eth<n>addr env
>> + */
>> +
>> + /* board serial number */
>> + fdt_setprop(blob, 0, "system-serial", getenv("serial#"),
>> + strlen(getenv("serial#") + 1));
>> +
>> + /* board (model contains model from device-tree) */
>> + fdt_setprop(blob, 0, "board", info->model,
>> + strlen((const char *)info->model) + 1);
>> +
>> + /* Peripheral Config */
>> + if (!info->config_eth0)
>> + fdt_del_node_and_alias(blob, "ethernet0");
>> + if (!info->config_eth1)
>> + fdt_del_node_and_alias(blob, "ethernet1");
>> + if (!info->config_hdmi_out)
>> + fdt_del_node_and_alias(blob, "hdmi_out");
>> + if (!info->config_sata)
>> + fdt_del_node_and_alias(blob, "ahci0");
>> + if (!info->config_pcie)
>> + fdt_del_node_and_alias(blob, "pcie");
>> + if (!info->config_ssi0)
>> + fdt_del_node_and_alias(blob, "ssi0");
>> + if (!info->config_ssi1)
>> + fdt_del_node_and_alias(blob, "ssi1");
>> + if (!info->config_lcd)
>> + fdt_del_node_and_alias(blob, "lcd0");
>> + if (!info->config_lvds0)
>> + fdt_del_node_and_alias(blob, "lvds0");
>> + if (!info->config_lvds1)
>> + fdt_del_node_and_alias(blob, "lvds1");
>> + if (!info->config_usb0)
>> + fdt_del_node_and_alias(blob, "usb0");
>> + if (!info->config_usb1)
>> + fdt_del_node_and_alias(blob, "usb1");
>> + if (!info->config_sd0)
>> + fdt_del_node_and_alias(blob, "usdhc0");
>> + if (!info->config_sd1)
>> + fdt_del_node_and_alias(blob, "usdhc1");
>> + if (!info->config_sd2)
>> + fdt_del_node_and_alias(blob, "usdhc2");
>> + if (!info->config_sd3)
>> + fdt_del_node_and_alias(blob, "usdhc3");
>> + if (!info->config_uart0)
>> + fdt_del_node_and_alias(blob, "serial0");
>> + if (!info->config_uart1)
>> + fdt_del_node_and_alias(blob, "serial1");
>> + if (!info->config_uart2)
>> + fdt_del_node_and_alias(blob, "serial2");
>> + if (!info->config_uart3)
>> + fdt_del_node_and_alias(blob, "serial3");
>> + if (!info->config_uart4)
>> + fdt_del_node_and_alias(blob, "serial4");
>> + if (!info->config_ipu0)
>> + fdt_del_node_and_alias(blob, "ipu0");
>> + if (!info->config_ipu1)
>> + fdt_del_node_and_alias(blob, "ipu1");
>> + if (!info->config_flexcan)
>> + fdt_del_node_and_alias(blob, "can0");
>> + if (!info->config_mipi_dsi)
>> + fdt_del_node_and_alias(blob, "mipi_dsi");
>> + if (!info->config_mipi_csi)
>> + fdt_del_node_and_alias(blob, "mipi_csi");
>> + if (!info->config_tzasc0)
>> + fdt_del_node_and_alias(blob, "tzasc0");
>> + if (!info->config_tzasc1)
>> + fdt_del_node_and_alias(blob, "tzasc1");
>> + if (!info->config_i2c0)
>> + fdt_del_node_and_alias(blob, "i2c0");
>> + if (!info->config_i2c1)
>> + fdt_del_node_and_alias(blob, "i2c1");
>> + if (!info->config_i2c2)
>> + fdt_del_node_and_alias(blob, "i2c2");
>> + if (!info->config_vpu)
>> + fdt_del_node_and_alias(blob, "vpu");
>> + if (!info->config_csi0)
>> + fdt_del_node_and_alias(blob, "csi0");
>> + if (!info->config_csi1)
>> + fdt_del_node_and_alias(blob, "csi1");
>> + if (!info->config_caam)
>> + fdt_del_node_and_alias(blob, "caam");
>> + if (!info->config_espci0)
>> + fdt_del_node_and_alias(blob, "spi0");
>> + if (!info->config_espci1)
>> + fdt_del_node_and_alias(blob, "spi1");
>> + if (!info->config_espci2)
>> + fdt_del_node_and_alias(blob, "spi2");
>> + if (!info->config_espci3)
>> + fdt_del_node_and_alias(blob, "spi3");
>> + if (!info->config_espci4)
>> + fdt_del_node_and_alias(blob, "spi4");
>> + if (!info->config_espci5)
>> + fdt_del_node_and_alias(blob, "spi5");
>> + if (!info->config_hdmi_in)
>> + fdt_del_node_and_alias(blob, "hdmi_in");
>> + if (!info->config_vid_out)
>> + fdt_del_node_and_alias(blob, "cvbs_out");
>> + if (!info->config_vid_in)
>> + fdt_del_node_and_alias(blob, "cvbs_in");
>> + if (!info->config_nand)
>> + fdt_del_node_and_alias(blob, "nand");
>> + if (!info->config_gps)
>> + fdt_del_node_and_alias(blob, "pps");
>
> I find the way quite singular. It seems you have a full-blown .dts file
> and you want to drop the features that you do not find in eeprom. A more
> conventional way is to have a simpler or not fully configured dts and
> add only the feature you want.
Yes, this is what I am doing - removing nodes from a full-featured devicetree.
I feel it better represents the board to have the 'full featured'
version present in the upstream Linux kernel and to simply disable
nodes that are removed. Also, adding features instead would add a lot
of code here as the whole dts representation of that feature would be
necessary (bus connections etc) - it would get very messy and
duplicate much of what is already present in the mainline Linux device
tree's. The above represents everything possible based on the
baseboard schematics and eeprom bit configuration but does not mean
all the combinations the above can result in are order-able boards.
>
> dts has the keyword "disabled", that let you enable or disable a
> specific feature. Maybe you could exchange the "status" field in dts
> instead of adding/dropping nodes.
I could do this but I'm not sure what is more appropriate. I'll need
to inquire on the linux and device-tree mailing lists.
>
>> +}
>> +#endif /* defined(CONFIG_OF_FLAT_TREE) && defined(CONFIG_OF_BOARD_SETUP) */
>> +
>> diff --git a/board/gateworks/gw_ventana/gw_ventana.cfg b/board/gateworks/gw_ventana/gw_ventana.cfg
>> new file mode 100644
>> index 0000000..4e07528
>> --- /dev/null
>> +++ b/board/gateworks/gw_ventana/gw_ventana.cfg
>> @@ -0,0 +1,42 @@
>> +/*
>> + * Copyright (C) 2013 Gateworks Corporation
>> + *
>> + * SPDX-License-Identifier: GPL-2.0+
>> + *
>> + * Refer doc/README.imximage for more details about how-to configure
>> + * and create imximage boot image
>> + *
>> + * The syntax is taken as close as possible with the kwbimage
>> + */
>> +
>> +/* image version */
>> +IMAGE_VERSION 2
>> +
>> +/*
>> + * Boot Device : one of
>> + * spi, sd, nand, sata
>> + */
>> +#ifdef CONFIG_SPI_FLASH
>> +BOOT_FROM spi
>> +#else
>> +BOOT_FROM nand
>> +#endif
>
> Do you boot also from SPI ?
The original prototype board for the Ventana family did (we only
produced and shipped a handful of these but there are still some in
the wild). This is the config for gwventanaq1gspi config in
boards.cfg.
>
>> +
>> +#define __ASSEMBLY__
>> +#include <config.h>
>> +#include "asm/arch/mx6-ddr.h"
>> +#include "asm/arch/iomux.h"
>> +#include "asm/arch/crm_regs.h"
>> +
>> +/* Memory configuration (size is overridden via eeprom config) */
>> +#include "ddr-setup.cfg"
>> +#if defined(CONFIG_MX6Q) && CONFIG_DDR_MB == 1024
>> + #include "1066mhz_4x128mx16.cfg"
>> +#elif defined(CONFIG_MX6DL) && CONFIG_DDR_MB == 1024
>> + #include "800mhz_4x128mx16.cfg"
>> +#elif defined(CONFIG_MX6DL) && CONFIG_DDR_MB == 512
>> + #include "800mhz_2x128mx16.cfg"
>> +#else
>> + #error "Unsupported CPU/Memory configuration"
>> +#endif
>> +#include "clocks.cfg"
>> diff --git a/board/gateworks/gw_ventana/ventana_eeprom.h b/board/gateworks/gw_ventana/ventana_eeprom.h
>> new file mode 100644
>> index 0000000..ea6524a
>> --- /dev/null
>> +++ b/board/gateworks/gw_ventana/ventana_eeprom.h
>> @@ -0,0 +1,107 @@
>> +/*
>> + * ventana_eeprom.h - Gateworks Ventana EEPROM Configuration
>> + * v1.00
>> + */
>> +#ifndef _VENTANA_EEPROM_
>> +#define _VENTANA_EEPROM_
>> +
>> +struct ventana_board_info {
>> + u8 mac0[6]; /* 0x00: MAC1 */
>> + u8 mac1[6]; /* 0x06: MAC2 */
>> + u8 res0[12]; /* 0x0C: reserved */
>> + u32 serial; /* 0x18: Serial Number (read only) */
>> + u8 res1[4]; /* 0x1C: reserved */
>> + u8 mfgdate[4]; /* 0x20: MFG date (read only) */
>> + u8 res2[7]; /* 0x24 */
>> + /* sdram config */
>> + u8 sdram_size; /* 0x2B: enum (512,1024,2048) MB */
>> + u8 sdram_speed; /* 0x2C: enum (100,133,166,200,267,333,400) MHz */
>> + u8 sdram_width; /* 0x2D: enum (32,64) bit */
>> + /* cpu config */
>> + u8 cpu_speed; /* 0x2E: enum (800,1000,1200) MHz */
>> + u8 cpu_type; /* 0x2F: enum (imx6q,imx6d,imx6dl,imx6s) */
>> + u8 model[16]; /* 0x30: model string */
>> + /* FLASH config */
>> + u8 nand_flash_size; /* 0x40: enum (4,8,16,32,64,128) MB */
>> + u8 spi_flash_size; /* 0x41: enum (4,8,16,32,64,128) MB */
>> +
>> + /* Config1: SoC Peripherals */
>> + u8 config_eth0:1; /* 0: 0x42 */
>> + u8 config_eth1:1; /* 1 */
>> + u8 config_hdmi_out:1;/* 2 */
>> + u8 config_sata:1; /* 3 */
>> + u8 config_pcie:1; /* 4 */
>> + u8 config_ssi0:1; /* 5 */
>> + u8 config_ssi1:1; /* 6 */
>> + u8 config_lcd:1; /* 7 */
>
> Generally, using bitwise fields is deprecated in favour of using maks.
> This because there is no conventions how the compiler should assign (MSB
> or LSB) the single bits.
Can you point me to an example? I'm not sure what you mean by 'maks'
(macros defining bit positions perhaps?).
>
>> +
>> + u8 config_lvds0:1; /* 0: 0x43 */
>> + u8 config_lvds1:1; /* 1 */
>> + u8 config_usb0:1; /* 2 (USB EHCI) */
>> + u8 config_usb1:1; /* 3 (USB OTG) */
>> + u8 config_sd0:1; /* 4 */
>> + u8 config_sd1:1; /* 5 */
>> + u8 config_sd2:1; /* 6 */
>> + u8 config_sd3:1; /* 7 */
>> +
>> + u8 config_uart0:1; /* 0: 0x44 */
>> + u8 config_uart1:1; /* 1 */
>> + u8 config_uart2:1; /* 2 */
>> + u8 config_uart3:1; /* 3 */
>> + u8 config_uart4:1; /* 4 */
>> + u8 config_ipu0:1; /* 5 */
>> + u8 config_ipu1:1; /* 6 */
>> + u8 config_flexcan:1; /* 7 */
>> +
>> + u8 config_mipi_dsi:1;/* 0: 0x45 */
>> + u8 config_mipi_csi:1;/* 1 */
>> + u8 config_tzasc0:1; /* 2 */
>> + u8 config_tzasc1:1; /* 3 */
>> + u8 config_i2c0:1; /* 4 */
>> + u8 config_i2c1:1; /* 5 */
>> + u8 config_i2c2:1; /* 6 */
>> + u8 config_vpu:1; /* 7 */
>> +
>> + u8 config_csi0:1; /* 0: 0x46 */
>> + u8 config_csi1:1; /* 1 */
>> + u8 config_caam:1; /* 2 */
>> + u8 config_mezz:1; /* 3 */
>> + u8 config_res1:1; /* 4 */
>> + u8 config_res2:1; /* 5 */
>> + u8 config_res3:1; /* 6 */
>> + u8 config_res4:1; /* 7 */
>> +
>> + u8 config_espci0:1; /* 0: 0x47 */
>> + u8 config_espci1:1; /* 1 */
>> + u8 config_espci2:1; /* 2 */
>> + u8 config_espci3:1; /* 3 */
>> + u8 config_espci4:1; /* 4 */
>> + u8 config_espci5:1; /* 5 */
>> + u8 config_res5:1; /* 6 */
>> + u8 config_res6:1; /* 7 */
>> +
>> + /* Config2: Other Peripherals */
>> + u8 config_gps:1; /* 0: 0x48 */
>> + u8 config_spifl0:1; /* 1 */
>> + u8 config_spifl1:1; /* 2 */
>> + u8 config_gspbatt:1; /* 3 */
>> + u8 config_hdmi_in:1; /* 4 */
>> + u8 config_vid_out:1; /* 5 */
>> + u8 config_vid_in:1; /* 6 */
>> + u8 config_nand:1; /* 7 */
>> +
>> + u8 config_res8:1; /* 0: 0x49 */
>> + u8 config_res9:1; /* 1 */
>> + u8 config_res10:1; /* 2 */
>> + u8 config_res11:1; /* 3 */
>> + u8 config_res12:1; /* 4 */
>> + u8 config_res13:1; /* 5 */
>> + u8 config_res14:1; /* 6 */
>> + u8 config_res15:1; /* 7 */
>> +
>> + u8 res3[4]; /* 0x4A */
>> +
>> + u8 chksum[2]; /* 0x4E */
>> +};
>> +
>> +#endif
>> diff --git a/boards.cfg b/boards.cfg
>> index a8336cc..7784b3a 100644
>> --- a/boards.cfg
>> +++ b/boards.cfg
>> @@ -295,6 +295,7 @@ Active arm armv7 mx6 - udoo udoo_qua
>> Active arm armv7 mx6 - wandboard wandboard_dl wandboard:IMX_CONFIG=board/boundary/nitrogen6x/nitrogen6dl.cfg,MX6DL,DDR_MB=1024 Fabio Estevam <fabio.estevam at freescale.com>
>> Active arm armv7 mx6 - wandboard wandboard_quad wandboard:IMX_CONFIG=board/boundary/nitrogen6x/nitrogen6q2g.cfg,MX6Q,DDR_MB=2048 Fabio Estevam <fabio.estevam at freescale.com>
>> Active arm armv7 mx6 - wandboard wandboard_solo wandboard:IMX_CONFIG=board/boundary/nitrogen6x/nitrogen6s.cfg,MX6S,DDR_MB=512 Fabio Estevam <fabio.estevam at freescale.com>
>> +Active arm armv7 mx6 barco titanium titanium titanium:IMX_CONFIG=board/barco/titanium/imximage.cfg Stefan Roese <sr at denx.de>
>
> Why do you touch the titanium ?
it wasn't in alphabetical order (barco should be before boundary). I
will not touch it as that shouldn't be part of my patch.
>
>> Active arm armv7 mx6 boundary nitrogen6x mx6qsabrelite nitrogen6x:IMX_CONFIG=board/boundary/nitrogen6x/nitrogen6q.cfg,MX6Q,DDR_MB=1024,SABRELITE Eric Nelson <eric.nelson at boundarydevices.com>
>> Active arm armv7 mx6 boundary nitrogen6x nitrogen6dl nitrogen6x:IMX_CONFIG=board/boundary/nitrogen6x/nitrogen6dl.cfg,MX6DL,DDR_MB=1024 Eric Nelson <eric.nelson at boundarydevices.com>
>> Active arm armv7 mx6 boundary nitrogen6x nitrogen6dl2g nitrogen6x:IMX_CONFIG=board/boundary/nitrogen6x/nitrogen6dl2g.cfg,MX6DL,DDR_MB=2048 Eric Nelson <eric.nelson at boundarydevices.com>
>> @@ -308,7 +309,11 @@ Active arm armv7 mx6 freescale mx6qsabreauto
>> Active arm armv7 mx6 freescale mx6sabresd mx6dlsabresd mx6sabresd:IMX_CONFIG=board/boundary/nitrogen6x/nitrogen6dl.cfg,MX6DL Fabio Estevam <fabio.estevam at freescale.com>
>> Active arm armv7 mx6 freescale mx6sabresd mx6qsabresd mx6sabresd:IMX_CONFIG=board/freescale/imx/ddr/mx6q_4x_mt41j128.cfg,MX6Q Fabio Estevam <fabio.estevam at freescale.com>
>> Active arm armv7 mx6 freescale mx6slevk mx6slevk mx6slevk:IMX_CONFIG=board/freescale/mx6slevk/imximage.cfg,MX6SL Fabio Estevam <fabio.estevam at freescale.com>
>> -Active arm armv7 mx6 barco titanium titanium titanium:IMX_CONFIG=board/barco/titanium/imximage.cfg Stefan Roese <sr at denx.de>
>> +Active arm armv7 mx6 gateworks gw_ventana gwventanaq1gspi gw_ventana:IMX_CONFIG=board/gateworks/gw_ventana/gw_ventana.cfg,MX6Q,DDR_MB=1024,SPI_FLASH Tim Harvey <tharvey at gateworks.com>
>
> Please let the list sorted.
will do - I failed to sort the new additions with respect to each other.
Incidentally, I noticed that there are quite a few lines out of order
from what tools/reformat.py does.
Thanks again for the review! I hope to have a v2 ready in perhaps a week.
Tim
>
> Best regards,
> Stefano Babic
>
> --
> =====================================================================
> DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
> =====================================================================
More information about the U-Boot
mailing list