On Friday, March 28, 2014 at 09:25:40 AM, Hans de Goede wrote: > 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.
We don't do such changes without test-compiling those (most of us don't and those who do ... well ... tsk tsk tsk ! ;-) ). > 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. Yeah. > Your suggestion of making it a proper function will fix that though, and I > think is the best solution. I think wrapping a plain readl() into a function is not necessary, but I will wait for V3 to see it in action and then I will decide . _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot