[U-Boot] [PATCH v2 1/9] sunxi: initial sun7i clocks and timer support.

[Hans de Goede]

Hi,

On 03/28/2014 09:20 AM, Ian Campbell wrote:
> On Thu, 2014-03-27 at 23:36 +0100, Marek Vasut wrote:
>> On Thursday, March 27, 2014 at 11:12:38 PM, Ian Campbell wrote:
>>> On Thu, 2014-03-27 at 23:00 +0100, Marek Vasut wrote:
>>>> On Thursday, March 27, 2014 at 10:29:56 PM, Ian Campbell wrote:
>>>>> On Mon, 2014-03-24 at 21:52 +0100, Marek Vasut wrote:
>>>>>>> +static struct sunxi_timer *timer_base =
>>>>>>> +     &((struct sunxi_timer_reg
>>>>>>> *)SUNXI_TIMER_BASE)->timer[TIMER_NUM]; +
>>>>>>> +/* macro to read the 32 bit timer: since it decrements, we invert
>>>>>>> read value */ +#define READ_TIMER() (~readl(&timer_base->val))
>>>>>>
>>>>>> This macro has to go, just use ~readl() in place. But still, why do
>>>>>> you use that negation in "~readl()" anyway ?
>>>>>
>>>>> The comment right above it explains why: the timer counts backwards and
>>>>> inverting it accounts for that.
>>>>>
>>>>> This is subtle enough that I don't think using ~readl() in place in the
>>>>> 3 callers would be an improvement.
>>>>
>>>> Please do it, we don't want any implementers down the line using this
>>>> "READ_TIMER()" call and getting hit by "timer_base undefined" . That
>>>> macro hides the dependency on this symbol, while if you expanded it
>>>> in-place, the dependency would be explicit. I really do want to see that
>>>> macro gone, sorry.
>>>
>>> How about a static inline instead of the macro? I'm thinking with a
>>> body:
>>> {
>>>       struct sunxi_timer *timers =
>>>               (struct sunxi_timer_reg *)SUNXI_TIMER_BASE;
>>>       return timers[TIMER_NUM]->val;
>>> }
>>> With something similar in timer_init then both the macro and the static
>>> global timer_base can be dropped.
>>
>> That's just wrapping a readl() into another function, which seems unnecessary 
>> really.
> 
> Sorry, but I think inlining the readl (and along with it the
> interesting/subtle) inverting functionality is a terrible idea, it
> should be wrapped in some sort of accessor precisely because it is not a
> raw readl.
> 
> I'm going to make it a function as I suggested.
> 
>>> BTW this macro is in arch/arm/cpu/armv7/sunxi/timer.c not a header, so
>>> I'm not sure which implementers down the line you were worried about
>>> using it in some other context where it breaks.
>>
>> People plumbing in the timer.c file who are not aware the macro has a dependency 
>> which is not passed as it's parameter.
> 
> What people? What plumbing? I've no idea what you are concerned about
> here.

I think what Marek is concerned about is people making some global u-boot change
which for some reason requires fixing up a bunch of platform specific files,
and they end up touching our timer.c without ever test-compiling it.

These kind of things happen in qemu / the kernel too. In this case they could move
a READ_TIMER() somewhere where the timer_base is not defined. Your suggestion of
making it a proper function will fix that though, and I think is the best solution.

Regards,

Hans