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

[Alexey Brodkin]

Hi Simon,

On Mon, 2017-02-06 at 07:33 -0800, Simon Glass wrote:
> Hi Alexey,
> 
> On 31 January 2017 at 07:35, Alexey Brodkin <Alexey.Brodkin at synopsys.com> wrote:
> > 
> > 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.
> 
> OK that's fine. I don't want to interfere in how ARC does things. It
> just stood out as odd :-)

That was the whole point so people get it right :)
Maybe there's some sense in adding more comments to this section
to justify that solution.

-Alexey