From mboxrd@z Thu Jan 1 00:00:00 1970 From: J. William Campbell Date: Fri, 15 Jul 2011 11:08:39 -0700 Subject: [U-Boot] [PATCH v1 (WIP) 00/16] [Timer]API Rewrite In-Reply-To: <20110715071734.4364916F76F1@gemini.denx.de> References: <1309261269-4363-1-git-send-email-graeme.russ@gmail.com> <20110711215637.6BC6A1579E14@gemini.denx.de> <4E1B88EE.9040104@gmail.com> <4E1BAED7.3070009@gmail.com> <4E1C5A8F.50908@comcast.net> <4E1C718F.2020203@emk-elektronik.de> <4E1CE7ED.7060500@gmail.com> <4E1CF2E0.1030702@comcast.net> <20110714194122.68CD31593511@gemini.denx.de> <4E1F8127.8030008@comcast.net> <20110715071734.4364916F76F1@gemini.denx.de> Message-ID: <4E208227.6010903@comcast.net> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 7/15/2011 12:17 AM, Wolfgang Denk wrote: > Dear "J. William Campbell", > > In message<4E1F8127.8030008@comcast.net> you wrote: >> I am pretty sure that is long enough for someone to notice. . I would be >> interested in seeing an example of such code as you refer to. Could you >> point me to one, because it seems to me that the only reason to code >> such a delay is that for some reason the user didn't want to keep >> looking at the termination condition so often. I think that that > arch/powerpc/cpu/mpc512x/i2c.c: > > 80 while (timeout--&& (status& I2C_BB)) { > ... > 88 udelay (1000); > 89 status = mpc_reg_in (®s->msr); > 90 } > > 103 while (timeout--&& !(*status& I2C_IF)) { > 104 udelay (1000); > 105 *status = mpc_reg_in (®s->msr); > 106 } > arch/powerpc/cpu/mpc512x/pci.c: > > 87 /* We need to wait at least a 1sec based on PCI specs */ > 88 for (i = 0; i< 1000; i++) > 89 udelay(1000); > > arch/powerpc/cpu/mpc5xx/spi.c: > > 258 for (i = 0; i< 1000; i++) { > ... > 265 udelay(1000); > 266 } > > 390 for (tm=0; tm<1000; ++tm) { > ... > 394 udelay (1000); > 395 } > > arch/powerpc/cpu/mpc5xxx/i2c.c: > > 102 while (timeout--&& (status& I2C_BB)) { > ... > 111 udelay(15); > 112 status = mpc_reg_in(®s->msr); > 113 } > > 125 while (timeout--&& !(*status& I2C_IF)) { > 126 udelay(15); > 127 *status = mpc_reg_in(®s->msr); > 128 } > > And just that you don't think this is in a single CPU only: > > arch/powerpc/cpu/mpc8260/pci.c: > > 343 for (i = 0; i< 1000; ++i) > ... > 345 udelay (1000); > > Or in board code: > > board/altera/common/cfide.c: > > 29 /* wait 500 ms for power to stabilize */ > 30 for (i = 0; i< 500; i++) > 31 udelay(1000); > > board/amcc/bamboo/bamboo.c: > > 1019 for (i=0; i<500; i++) > 1020 udelay(1000); > > or common code: > > common/cmd_fdc.c: > > 213 while((read_fdc_reg(FDC_SRA)&0x80)==0) { > 214 timeout--; > 215 udelay(10); > 216 if(timeout==0) /* timeout occured */ > 217 return FALSE; > 218 } > > 228 while((read_fdc_reg(FDC_MSR)&0xC0)!=0xC0) { > 229 /* direction out and ready */ > 230 udelay(10); > 231 timeout--; > 232 if(timeout==0) /* timeout occured */ > 233 return -1; > 234 } > etc.etc. > > 375 for(val=0;val<255;val++) > 376 udelay(500); /* wait some time to start motor */ > > 418 for(i=0;i<100;i++) > 419 udelay(500); /* wait 500usec for fifo overrun */ > > 600 for(i=0; i<255; i++) /* then we wait some time */ > 601 udelay(500); > > > common/usb.c: > > 93 inline void wait_ms(unsigned long ms) > 94 { > 95 while (ms--> 0) > 96 udelay(1000); > 97 } > > etc. etc. Note this last example which "spreads" the effect in a not > so nice way. > > Note that there are even places where udelay() is part of the protocol > timing implementation - like in the soft-I2C and soft-SPI drivers. > Hi All, Thanks for the pointers Wolfgang. From your examples, it seems we are talking exclusively about udelay. As I said in my previous post, on NIOS2, udelay needs to be mechanized on NIOS2 by a timed loop, not by using the same mechanism as the timer, as there is no way one can approximate microsecond resolution by the hardware timer. It is my opinion that there are many CPUs on which the udelay implementation must not be/should not be based on the same timer code as the get_time() operation, because in many cases the get_time() operation requires interrupts to be functional. As you have also pointed out, udelay needs to be available "early", before interrupts are available. Therefore, the udelay and get_timer must use the hardware at least somewhat differently. The must also not interfere with each other, which on some existing boards they did. If the I2C protocol must be available before interrupts are available, then udelay must be used. In the above examples, there are some loops in i2c and spi that appear to be waiting a full second. I assume they are using udelay because the get_timer feature is not yet available to them. I also assume that the example in common/usb.c uses udelay to wait for millisecond sized values for the same reason, i.e. that it may run before get_time() is available. However, if you examine the timer code on of the existing CPUs, you will find that udelay is NOT available early ( before interrupts are enabled) on quite a few of them. Therefore, none of the above code would work on these CPUs at present either. These CPUs must not have I2C or spi busses on them that are accessed "early". >> equivalent operation can be produced by a pretty simple re-coding of >> the loop. In any case, NIOS does not have the problem at present, so the >> suggested new work-around would be no worse than the present situation. > I think it is not correct to state that "NIOS does not have the > problem at present" - it does, only this is not a blatant problem at > the moment. Which in turn might result from the fact that all > presently supported NIOS boards has any support enabled for I2C or > SPI or USB or MMC or watchdogs or ... you name it. > > The first user who adds any of the currently unused (and thus > untested) standard features that work just fine on all other > architectures might run into issue... True, although I expect you will find the statement "on all the other architectures" to be false. Many other architectures, yes, all, no. These other architectures just don't have spi or I2C yet, or if they do, they don't use it "early". > >> It is also true that the hardware timer cannot be used in a >> reasonable version of udelay, as most of the desired delays may be very >> short relative to the timebase. A calibrated timing loop would be the >> best approach to the udelay problem. > Just try using Soft-I2C on a NIOS board and you will immediately > realize how crucial a working version of udelay() is. Agreed. The definition of "working" would need to include that the routine is available "early" and that it is accurate to within say +/-5% (other numbers welcome) of the specified delay, unless the delay is "small" relative to the CPU speed (not an issue on most CPUs, but is on some). On these existing NIOS2 processors, that will require a udelay based on a timing loop, as there is no microsecond level timer available. The timing loop can be calibrated using the 10 ms timer, so it can be accurate enough. It may need re-calibration if icache is turned on, but this is also doable. > >> In general, that is true. There may be a few cases where a delay of less >> than the resolution is essential to make something work. There are > These are not just "a few cases". There are tons of it, all over the > place - from protocol implementations like soft-I2C or soft-I2C to > timing requirements for RAM initializations etc. etc. And there are > enough places that mandate that the timing is at least in the > requested oder, and not off by factors of tens or hundrets or worse. > >> probably lots of other cases where we can easily remove the restriction >> on the resolution. We cannot fix the first kind, no matter what we do, >> to work on a lower resolution timer. The second kind we can and probably >> should fix, because they are coded in an overly-restrictive manner. In >> any case, we don't absolutely have to fix them until somebody decides to >> use the code on a CPU with a low resolution timer. Practically speaking, >> the suggested solution will therefore work on all extant cases. > It will work by chance, at best. See above - any attempt to enable > one of the currently not yet used standard features in U-Boot may > break your system. I don't consider this a sound base - to me it > feels like building on qicksand. In a perfect world, I would agree with you. Unfortunately (or maybe inevitably), large software projects are iterative. There are quite a few u-boot versions in the current code that do not meet all the requirements that exits for a fully compliant udelay. There were also quite a few board ports whose timers did not meet the implicit requirements to be fully "compliant". Full compliance is required for "plug and play" with u-boot code that has been written assuming this full compliance. Quite a few people have just made a port that works for them without worrying about this compliance. One could argue that compliance was not well enough documented, or that non-compliant boards should not have been accepted. All perhaps true (or not, because u-boot was still feeling its way forward), but we are where we are. We can't retro-actively change the NIOS2 boards that are out there already. However, we can require that if any of the boards are to be upgraded to include I2C or SPI devices, they must be supplied with a compliant udelay, either by a timed loop or increasing the hardware timer resolution. The get_timer design is sound for all existing designs if the resolution compensation is added. It preserves all working boards as working . If you modify the configuration of existing boards that currently are non-compliant in some way, they may quit working. That is true now, and it will be true if the change to include the resolution is made. If udelay doesn't have compliant resolution and accuracy, newly enabled code may fail. If you aren't using such code, you may not be motivated to fix the problem. It still should be fixed, but often will not be until it breaks something. So I think we are building on solid, backwards compatible ground. Yes, non-compliant boards need to be "fixed", but we need a way to move forward with minimum pain to the current users. Best Regards, Bill Campbell > > > Best regards, > > Wolfgang Denk >