On Sat, Dec 10, 2011 at 2:42 AM, Graeme Russ <graeme.r...@gmail.com> wrote: > Hi Vadim, > > Oops, dropped the ML... > > On Dec 10, 2011 9:20 PM, "Graeme Russ" <graeme.r...@gmail.com> wrote: >> >> Hi Vadim, >> >> On Dec 10, 2011 12:14 PM, "Graeme Russ" <graeme.r...@gmail.com> wrote: >> > >> > Hi Vadim, >> > >> > On Dec 10, 2011 12:08 PM, "Vadim Bendebury" <vben...@chromium.org> >> > wrote: >> > > >> > > On Fri, Dec 9, 2011 at 3:57 PM, Graeme Russ <graeme.r...@gmail.com> >> > > wrote: >> > > > >> > > > Hi Vadim, >> > > > >> > > > On Dec 10, 2011 10:31 AM, "Vadim Bendebury" <vben...@chromium.org> >> > > > wrote: >> > > > > >> > > > > There have been a couple of unused variable cases, causing >> > > > > compilation >> > > > > warnings when building the eNET target. >> > > > > >> > > > > While the board/eNET/eNET.c:last_stage_init() case seems a >> > > > > leftover >> > > > > from a quick edit, the >> > > > > arch/x86/cpu/sc520/sc520_timer.c:sc520_udelay() >> > > > > seems to actually require accessing the registers discarding the >> > > > > values. >> > > > >> > > > Thanks for picking this up, I've been a bit preoccupie lately. >> > > > >> > > > I'll need to look a bit more closely, but there should be no need >> > > > for such trickery... >> > > > >> > > > Regards, >> > > > >> > > > Graeme >> > > > >> > > > >> > > >> > > Hi Graeme, thank you for a quick response. >> > > >> > > Do you mean that there is no need to read the registers before the >> > > actual udelay functionality or do you have another way of pacifying >> > > the compiler in mind? >> > >> > On closer inspection, I can't think of a better way >> > >> > Acked-by: Graeme Russ <graeme.r...@gmail.com> >> >> Sorry, bit I just had a closer look at this and after reading the >> datasheet I've come to the realization that the udelay function is broken - >> not badly, but it can timeout up to 1ms early. >> >> The read of swtmrmilli reads the current value of the millisecond timer, >> latches the value of the internal microsecond timer and resets the >> millisecond timer to zero >> >> The read of swtmrmicro reads the latched microsecond value >> >> The code assumes that reading swtmrmicro resets the microsecond counter, >> which it does not. So if the internal microsecond timer is, say, 900 then >> udelay(500) for example will return without any delay at all >> >> I need to fix this, but cannot do so any time soon... >> >> I have no objection to the compiler warning fix, but is there any point in >> applying a cosmetic fix to broken code? >> > > I'm not near my dev PC so the formatting is all wrong, but this should fix > the bug and the compiler warning: > > void sc520_udelay(unsigned long usec) > { > int m; > long u; > long start_us; > > /* Reset millisecond counter */ > m = readw(&sc520_mmcr->swtmrmilli); > m = 0; >
Looks reasonable, not much difference from the original submission IMO. The important thing is that there should be no compilation warnings allowed, as if there are a few 'legitimate' ones, it becomes easy to let the illegitimate ones to slip through :P cheers, /vb > /* Read initial microsecond count */ > start_us = readw(&sc520_mmcr->swtmrmicro); > > do { > > > m += readw(&sc520_mmcr->swtmrmilli); > u = readw(&sc520_mmcr->swtmrmicro) + (m * 1000); > } while ((u - start_us) < usec); > } > > Regards, > > Graeme _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot