On Tue, Jan 28, 2014 at 2:20 AM, Stefano Babic sbabic@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@denx.de =====================================================================