lkml.org 
[lkml]   [2017]   [Jun]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 5/7] RISC-V: arch/riscv/lib
On Tue, 06 Jun 2017 02:31:02 PDT (-0700), Arnd Bergmann wrote:
> On Tue, Jun 6, 2017 at 6:56 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>> On Fri, 26 May 2017 02:06:58 PDT (-0700), Arnd Bergmann wrote:
>>> On Thu, May 25, 2017 at 3:59 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>>>> On Tue, 23 May 2017 04:19:42 PDT (-0700), Arnd Bergmann wrote:
>>>>> On Tue, May 23, 2017 at 2:41 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>>>
>>>>> Also, it would be good to replace the multiply+div64
>>>>> with a single multiplication here, see how x86 and arm do it
>>>>> (for the tsc/__timer_delay case).
>>>>
>>>> Makes sense. I think this should do it
>>>>
>>>> https://github.com/riscv/riscv-linux/commit/d397332f6ebff42f3ecb385e9cf3284fdeda6776
>>>>
>>>> but I'm finding this hard to test as this only works for 2ms sleeps. It seems
>>>> at least in the right ballpark
>>>
>>> + if (usecs > MAX_UDELAY_US) {
>>> + __delay((u64)usecs * riscv_timebase / 1000000ULL);
>>> + return;
>>> + }
>>>
>>> You still do the 64-bit division here. What I meant is to completely
>>> avoid the division and use a multiply+shift.
>>
>> The goal here was to avoid the error case that ARM has on overflow and instead
>> just delay for the requested time. This should only divide when the delay is
>>>=2ms, so the division won't cost much in comparison.
>>
>> The normal case should have no division in it.
>>
>> I can copy ARM's error handling if you think that's better, but it seemed more
>> complicated than just computing the correct answer.
>
> I think the intention originally was to avoid overflowing the 32-bit
> argument in
>
> void __delay(unsigned long cycles)
>
> If you need to delay for more than 4 billion clocksource cycles,
> your code is still broken.

Maybe I'm crazy, but I thought the goal was to avoid overflowing on the
multiply. Specifically, the code looks like

udelay(long input) {
long a = input * MUL_VAL;
long b = a >> SHIFT_VAL;
__delay(b);
}

so the place there's extra overflow is at computing a, not b (the input to
__delay). When I modified the ARM code I went and recalculated the point at
which the multiply would overflow and it matched the value from the ARM code,
which is 2000us.

While I can buy the argument that 2000us is still too long, the real reason I
wrote the code this way is because I thought it was easier than having an error
case. If you think the error is better then I'll do it that way.

\
 
 \ /
  Last update: 2017-06-12 00:55    [W:0.400 / U:0.484 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site