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

[Alexey Brodkin]

Hi Simon,

On Tue, 2017-01-31 at 14:44 +0000, Vlad Zakharov wrote:
> 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.

Just a subtle clarification.

In ARC world we have addition address space for controlling built-in features of the core.
These are so-called AUX[iliary] registers. We access them via special commands like LR/SR
(load/store AUX reg) and each AUX reg has its own pre-defined index.

In other words ARC_AUX_TIMER0_CTRL and ARC_AUX_TIMER1_CTRL are 2 very special values/indexes (in terms they are always
the same within all ARC cores with the same ISA). Compared to common MMIO peripherals which might be mapped to different
memory location these are constant. And IMHO for ARC user it is much more natural to see a particular AUX reg number and
then just get all the info about it from ARC core documentation. Otherwise if we start using fake bases it may just
mislead a reader.

-Alexey