| Message ID | CANy1buKFnx71Q7htiBq0pKvAwBeOZK3tiMaUOmXt+SHXctA+7A@mail.gmail.com |
|---|---|
| State | Not Applicable |
| Delegated to: | Graeme Russ |
| Headers | show |
Hi Vadim, On Dec 10, 2011 10:31 AM, "Vadim Bendebury" <vbendeb@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 > > The source code is being modified accordingly. > > TEST: > - the eNET target now builds cleanly > - examining disassembled sc520_timer.o shows that the registers are > still being accessed in the beginning of the function. > > Signed-off-by: Vadim Bendebury <vbendeb@chromium.org> > --- > > [Resend with the proper cc: header] > > arch/x86/cpu/sc520/sc520_timer.c | 1 + > board/eNET/eNET.c | 5 ----- > 2 files changed, 1 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/cpu/sc520/sc520_timer.c b/arch/x86/cpu/sc520/sc520_timer.c > index 495a694..a7bbe92 100644 > --- a/arch/x86/cpu/sc520/sc520_timer.c > +++ b/arch/x86/cpu/sc520/sc520_timer.c > @@ -87,4 +87,5 @@ void sc520_udelay(unsigned long usec) > m += readw(&sc520_mmcr->swtmrmilli); > u = readw(&sc520_mmcr->swtmrmicro) + (m * 1000); > } while (u < usec); > + (void) temp; > } > diff --git a/board/eNET/eNET.c b/board/eNET/eNET.c > index 429fe1b..2f26470 100644 > --- a/board/eNET/eNET.c > +++ b/board/eNET/eNET.c > @@ -178,11 +178,6 @@ void show_boot_progress(int val) > > int last_stage_init(void) > { > - int minor; > - int major; > - > - major = minor = 0; > - > outb(0x00, LED_LATCH_ADDRESS); > > register_timer_isr(enet_timer_isr); > -- > 1.7.3.1 > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot
On Fri, Dec 9, 2011 at 3:57 PM, Graeme Russ <graeme.russ@gmail.com> wrote: > > Hi Vadim, > > On Dec 10, 2011 10:31 AM, "Vadim Bendebury" <vbendeb@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? cheers, /v > > > > > The source code is being modified accordingly. > > > > TEST: > > - the eNET target now builds cleanly > > - examining disassembled sc520_timer.o shows that the registers are > > still being accessed in the beginning of the function. > > > > Signed-off-by: Vadim Bendebury <vbendeb@chromium.org> > > --- > > > > [Resend with the proper cc: header] > > > > > arch/x86/cpu/sc520/sc520_timer.c | 1 + > > board/eNET/eNET.c | 5 ----- > > 2 files changed, 1 insertions(+), 5 deletions(-) > > > > diff --git a/arch/x86/cpu/sc520/sc520_timer.c b/arch/x86/cpu/sc520/sc520_timer.c > > index 495a694..a7bbe92 100644 > > --- a/arch/x86/cpu/sc520/sc520_timer.c > > +++ b/arch/x86/cpu/sc520/sc520_timer.c > > @@ -87,4 +87,5 @@ void sc520_udelay(unsigned long usec) > > m += readw(&sc520_mmcr->swtmrmilli); > > u = readw(&sc520_mmcr->swtmrmicro) + (m * 1000); > > } while (u < usec); > > + (void) temp; > > } > > diff --git a/board/eNET/eNET.c b/board/eNET/eNET.c > > index 429fe1b..2f26470 100644 > > --- a/board/eNET/eNET.c > > +++ b/board/eNET/eNET.c > > @@ -178,11 +178,6 @@ void show_boot_progress(int val) > > > > int last_stage_init(void) > > { > > - int minor; > > - int major; > > - > > - major = minor = 0; > > - > > outb(0x00, LED_LATCH_ADDRESS); > > > > register_timer_isr(enet_timer_isr); > > -- > > 1.7.3.1 > > > > _______________________________________________ > > U-Boot mailing list > > U-Boot@lists.denx.de > > http://lists.denx.de/mailman/listinfo/u-boot
Hi Vadim, Oops, dropped the ML... On Dec 10, 2011 9:20 PM, "Graeme Russ" <graeme.russ@gmail.com> wrote: > > Hi Vadim, > > On Dec 10, 2011 12:14 PM, "Graeme Russ" <graeme.russ@gmail.com> wrote: > > > > Hi Vadim, > > > > On Dec 10, 2011 12:08 PM, "Vadim Bendebury" <vbendeb@chromium.org> wrote: > > > > > > On Fri, Dec 9, 2011 at 3:57 PM, Graeme Russ <graeme.russ@gmail.com> wrote: > > > > > > > > Hi Vadim, > > > > > > > > On Dec 10, 2011 10:31 AM, "Vadim Bendebury" <vbendeb@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.russ@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; /* 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
On Sat, Dec 10, 2011 at 2:42 AM, Graeme Russ <graeme.russ@gmail.com> wrote: > Hi Vadim, > > Oops, dropped the ML... > > On Dec 10, 2011 9:20 PM, "Graeme Russ" <graeme.russ@gmail.com> wrote: >> >> Hi Vadim, >> >> On Dec 10, 2011 12:14 PM, "Graeme Russ" <graeme.russ@gmail.com> wrote: >> > >> > Hi Vadim, >> > >> > On Dec 10, 2011 12:08 PM, "Vadim Bendebury" <vbendeb@chromium.org> >> > wrote: >> > > >> > > On Fri, Dec 9, 2011 at 3:57 PM, Graeme Russ <graeme.russ@gmail.com> >> > > wrote: >> > > > >> > > > Hi Vadim, >> > > > >> > > > On Dec 10, 2011 10:31 AM, "Vadim Bendebury" <vbendeb@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.russ@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
diff --git a/arch/x86/cpu/sc520/sc520_timer.c b/arch/x86/cpu/sc520/sc520_timer.c index 495a694..a7bbe92 100644 --- a/arch/x86/cpu/sc520/sc520_timer.c +++ b/arch/x86/cpu/sc520/sc520_timer.c @@ -87,4 +87,5 @@ void sc520_udelay(unsigned long usec) m += readw(&sc520_mmcr->swtmrmilli); u = readw(&sc520_mmcr->swtmrmicro) + (m * 1000); } while (u < usec); + (void) temp; } diff --git a/board/eNET/eNET.c b/board/eNET/eNET.c index 429fe1b..2f26470 100644 --- a/board/eNET/eNET.c +++ b/board/eNET/eNET.c @@ -178,11 +178,6 @@ void show_boot_progress(int val) int last_stage_init(void) { - int minor; - int major; - - major = minor = 0; - outb(0x00, LED_LATCH_ADDRESS); register_timer_isr(enet_timer_isr);
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. The source code is being modified accordingly. TEST: - the eNET target now builds cleanly - examining disassembled sc520_timer.o shows that the registers are still being accessed in the beginning of the function. Signed-off-by: Vadim Bendebury <vbendeb@chromium.org> --- [Resend with the proper cc: header] arch/x86/cpu/sc520/sc520_timer.c | 1 + board/eNET/eNET.c | 5 ----- 2 files changed, 1 insertions(+), 5 deletions(-) -- 1.7.3.1