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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot