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

[Tim Harvey]

On Tue, Jan 28, 2014 at 2:20 AM, Stefano Babic <sbabic at denx.de> wrote:
> Hi Tim,
>
> On 28/01/2014 00:36, Tim Harvey wrote:
>
<snip>
>>>> 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,

I should not have used the word 'header'.

I _am_ talking about the Image Vector Table (IVT) and Device
Configuration Data (DCD) data structures that the IMX6 BOOT ROM needs
to boot and which are present in u-boot.imx.  The kobs-ng (at least
for IMX6) needs u-boot.imx because it contains these structures built
from imximage and it must flash them onto the boot media.

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

Can you re-read the README instructions?  Other than the bad link I
feel they are very clear.  I think perhaps your thinking that I was in
error specifying that kobs-ng needing u-boot.imx added to some
confusion?

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

I was not referring to making the code a part of uboot sources but
adding the functionality that it provides such that one could use
uboot to update itself on an IMX NAND device.  I'm actually surprised
nobody has done this before for IMX in general as its a bit
inconvenient to boot to a linux based OS in order to run kobs-ng to
flash the bootloader.  Regardless, this would be functionality added
later.

It appears the only other mainlined IMX6 bootloader to support NAND is
the Titanium and there is no README for it at all.  If there were, I
would expect it to say pretty much the same thing that my proposed
Ventana README states.  There was a comment by Fabio on original
titanium patch to include a README explaining how to flash and boot
from NAND but it apparently didn't make it in:
http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/158436/focus=158441

I've added Stefan Roese and Fabio to the cc to hear their thoughts.

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

No - the only communication channel to the GSC is the i2c bus.  The
only way it has to respond is to ACK or fail to ACK (aka NAK) within
the i2c bit time (aka NAK).  The device does not and can not support
i2c clock stretching.  The nature of GSC is such that the worst case
failure when talking at 100kbps will be at most 2 times in a row (due
to the latency of the timer tick) which is why I have a retry of 3.

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

no - this is a 'slave' device on the i2c bus that is too busy to
respond in the allotted time, not the controller.  This isn't unheard
of.  Some i2c based EEPROM's need a certain delay after writing for
example, before they are ready again for reading.

I 'can' as you say call the common i2c_read and wrap that around 3
retries but I don't any better solution to the other i2c read/writes
to the GSC such as reading the hwmon, RTC, or firmware registers with
up to 3 retries so it really doesn't shorten the code any.  Note that
I am using common i2c_read.  I don't really see any benefit to using
the common eeprom_read over the common i2c_read as I know I'm not
wrapping page boundaries.

Would you rather see me implement retries down at the i2c_core level
or at the i2c driver level?

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

fair enough - I have updated the wiki page to make this clear.

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

once I realized I could #define CONFIG_DISPLAY_BOARDINFO_LATE to make
checkconfig() run after relocation I no longer needed to have EEPROM
read prior to relocation and that removed the need for this structure
to be in the data segement.  I still have the structure so that eeprom
can be read just once (as its used in several places) but it no longer
needs to be in the data segment so the attribute will be removed in
the next patch version.

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

sure... but understand the only 'extra' code here is the
implementation of gsc_i2c_read and gsc_i2c_write which wrap the common
i2c_read/i2c_write functions around a retry of 3.   If I were to use
common i2c_read or eeprom_read I'm only substituting the call to
gsc_i2c_read.

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

Yes - I could, but then I wouldn't have a solution for the calls that
read the other device registers that the GSC emulates (which are at
different slave addresses).

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

Its both displaying the value and evaluating the value for pass, fail
(high) and fail (low).  As those return codes are not used, I'll
remove the return value.

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

got it - moving checkboard() to late init resolved this.

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

agreed - will be fixed

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

agreed - I will move most of this into user commands.

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

I'll for sure take a stab at this - I've been wanting to re-define
that structure using macros for a while anyway.

Thanks,

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