[U-Boot] [PATCH] ventana: Add Gateworks Ventana family support

[Stefano Babic]

Hi Tim,

On 28/01/2014 00:36, Tim Harvey wrote:

> 
> 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.

ok, I see. Clock for NAND is not enabled in nitrogen's file. Well, then
we should have only clocks.cfg for ventana.

>>> 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?

I think there is a misunderstanding due to the usage of "headers". What
do you mean with headers here ? As you talk about BOOT ROM, the only
header that the ROM understands is the i.MX image header, that is the
description of the image itself with the DCD and all that stuff. When
you say that kobs-ng needs "headers", it seems you are talking about .h
files,

As far as I know, kobs-ng is a flasher - a utility to install u-boot on
the target. It is not the only method to install the binary. I think you
should rework the text making the statements more clearer.

> 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.

If as I think kobs-ng is only a flasher, it does not take part to the
build of U-Boot binaries. IMHO it should be not part of U-Boot sources,
but maybe there are some features that can be interesting.

>>> + * 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 ;)

As there is no guarantee that works, but it is only statistics, it looks
more a workaround as a solution.

> 
> Our GSC can occasionally be 'busy' and NAK (every 1Hz it does some
> internal processing).

ok - but is there no way to understand that the component is busy ?

I mean, in most hardware solutions (there is plenty of them in
Freescale's SOCs), software must poll a bit (a pin, whatever...) to
check if it can access the component. Nothing here ?

>  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

I read the doc - however, there is no mention about this important detail !

>>
>>> +
>>> +/*
>>> + * 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.

If it is at the moment dead code, please remove. Dead code that passes a
review will only confuse people.

>>> + * 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 -

Right, this is what I said with "payload". You have two functions:
- read from hardware
- check and interprete the result

The function that chjeck the data calls the function to read the data.

> inside read_eeprom I verify
> checksum and some other sanity checks.

As I said, you can check outside this function.

>  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.

Right. But again, if this is not strictly necessary, it is always better
to reuse common code. You can also check 3 times the result of
read_eeprom() to implement your retry-on-NAK, without implementing your
own version.

> 
>>
>>> +#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.

ok - then this is only a function that outputs details about hardwrae. Then:
- function must be declared static
- why should the function return a int if it is not checked at all ?

>>> +#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. 

This is correct. udelay() at this point is not yet working.
board_early_init_f() is thought to set up hardware that is required
before relocation. At this point, generally clocks and pinmuxes setup
take place. The rest should be postponed to a later point, that is,
board_init, or even board_late_init() (if you activate it) or misc_init().


> 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.

You do not need HDMI before relocation.


>> 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.

You are mixing two things. During the boot process, it is better to
print only what is really needed to speed up the process. The user has
maybe no access to the console, and he wants that the system reacts very
soon - with kernel, application, everything.

If a user is operating at U-Boot console, he has all the time he wants.
Some output can be displayed with an additional command.

See "clocks" command for i.MXes - clock configuration is printed only on
demand.


>>
>> 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?).

I need to buy a new keyboard - "s" key is not working well. It is not
"maks", but "masks". Yes, accessing bit with and and or logical
operation. And using the provided helper functions (clrsetbits_).

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
=====================================================================