[U-Boot] [PATCH v3 05/11] dm: timer: Support 64-bit counter

[Simon Glass]

Hi,

On 22 November 2015 at 09:21, Simon Glass <sjg at chromium.org> wrote:
> Hi,
>
> On 20 November 2015 at 17:41, Thomas Chou <thomas at wytron.com.tw> wrote:
>> Hi Simon,
>>
>>
>> On 2015年11月21日 05:10, Simon Glass wrote:
>>>>>>
>>>>>> @@ -35,10 +52,10 @@ struct timer_ops {
>>>>>>           * Get the current timer count
>>>>>>           *
>>>>>>           * @dev: The timer device
>>>>>> -        * @count: pointer that returns the current timer count
>>>>>> +        * @count: pointer that returns the current 64-bit timer count
>>>>>>           * @return: 0 if OK, -ve on error
>>>>>>           */
>>>>>> -       int (*get_count)(struct udevice *dev, unsigned long *count);
>>>>>> +       int (*get_count)(struct udevice *dev, u64 *count);
>>>>>
>>>>>
>>>>> Why do we need to change the API to 64-bit?
>>>>
>>>>
>>>> IMHO, this API should be created to be accept a 64-bit value in the
>>>> first place. As you see the U-Boot time APIs in lib/time.c like
>>>> get_ticks() has been using 64-bit value.
>>>
>>>
>>> Right, but the point of this timer is for time delays. Making everyone
>>> one of those 64-bit on a 32-bit machine does not seem like a win to
>>> me.
>>>
>>>>
>>>>>
>>>>> My only concern is that we are pretty happy with the 32-bit timer and
>>>>> I'm not sure it should change. At present unsigned long can be 32-bit
>>>>> on 32-bit machines.
>>>>
>>>>
>>>> 32-bit timer has to be converted to 64-bit value as get_ticks()
>>>> requires 64-bit. As for 'unsigned long can be 32-bit on 32-bit
>>>> machines', I think we should explicitly declare its width as this is
>>>> hardware limitation (32-bit timer can only produce 32-bit value, even
>>>> if it is on a 64-bit machine, where long on 64-bit means 64-bit, which
>>>> is wrong for this 32-bit timer hardware).
>>>
>>>
>>> This is a bit messy, but for now I think we should follow what
>>> get_timer() does. It would be worth getting more input on the list I
>>> think.
>>>
>>
>> I would agree with Bin that the API should have been created as 64 bits. I
>> considered this option before, because most part of lib/time.c uses 64 bits.
>> It was only that I didn't have much confidence to change so many global code
>> (without buildman). I realized this mistake when I wrote the sandbox timer.
>> It is wasteful to truncate the 64 bits count to 32 bits, and then
>> reconstruct 32 bits to 64 bits. This is what Bin wanted to resolve. There is
>> no overhead for 32 bits timer, as the patch only shift the 64 bits
>> conversion from lib/time.c to driver. Yet for 64 bits timer, this is a good
>> saving.
>
> OK thank you both for your explanation. This makes sense to me now.
>
> Acked-by: Simon Glass <sjg at chromium.org>

Unfortunately this causes build errors for a few avr32 boards - e.g.
grasshopper.

I think the problem is the gd declaration in the timer.h header file.
I don't think that is a good idea. Can we move it to the C file?

Regards,
Simon