Re: [PATCH 5/7] RISC-V: arch/riscv/lib

[Palmer Dabbelt <palmer@dabbelt.com>]

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.