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

[Simon Glass]

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 :-)

Regards,
Simon