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

[Marek Vasut]

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 .