[U-Boot] [PATCH 1/3] drivers: timer: Introduce ARC timer driver

[Vlad Zakharov <Vladislav.Zakharov@synopsys.com>]

Hi Simon,

On Fri, 2017-01-20 at 20:51 -0700, Simon Glass wrote:
> > +?????? switch (priv->timer_id) {
> > +?????? case 0:
> > +?????????????? /* Disable timer if CPU is halted */
> > +?????????????? write_aux_reg(ARC_AUX_TIMER0_CTRL, NH_MODE);
> > +?????????????? /* Set max value for counter/timer */
> > +?????????????? write_aux_reg(ARC_AUX_TIMER0_LIMIT, 0xffffffff);
> > +?????????????? /* Set initial count value and restart counter/timer */
> > +?????????????? write_aux_reg(ARC_AUX_TIMER0_CNT, 0);
> > +?????????????? break;
> > +?????? case 1:
> > +?????????????? /* Disable timer if CPU is halted */
> > +?????????????? write_aux_reg(ARC_AUX_TIMER1_CTRL, NH_MODE);
> > +?????????????? /* Set max value for counter/timer */
> > +?????????????? write_aux_reg(ARC_AUX_TIMER1_LIMIT, 0xffffffff);
> > +?????????????? /* Set initial count value and restart counter/timer */
> > +?????????????? write_aux_reg(ARC_AUX_TIMER1_CNT, 0);
> 
> You are writing the same values in each case. Can you set a value to
> either ARC_AUX_TIMER0 or ARC_AUX_TIMER1? and then just have the code
> once?

Yes, here we have some code duplication. But in fact we have a reason for this. ARC timers are controlled by auxiliary
registers that are not mapped anywhere. They have their fixed numbers and are being accessed using special ARC asm
commands. Of course I can write something like this:
---------------------------------->8------------------------------------
timer_base = priv->timer_id ? ARC_AUX_TIMER1_BASE : ARC_AUX_TIMER0_BASE;

write_aux_reg(timer_base + ARC_TIMER_CTRL, NH_MODE);
write_aux_reg(timer_base + ARC_TIMER_LIMIT, 0xffffffff);
write_aux_reg(timer_base + ARC_TIMER_CNT, 0);
---------------------------------->8------------------------------------
But unfortunately it will not reflect the real life. We don't have any ARC timer base addresses, only fixed register
numbers.

If you insist I will use the latter code snippet. But from our point of view the code is being duplicated in this patch
is very small but helps to understand what is really happening with ARC timers much better.

Thanks.

-- 
Best regards,
Vlad Zakharov <vzakhar@synopsys.com>