From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexey Brodkin Date: Mon, 6 Feb 2017 16:46:33 +0000 Subject: [U-Boot] [PATCH 1/3] drivers: timer: Introduce ARC timer driver In-Reply-To: References: <1484574565-27160-1-git-send-email-vzakhar@synopsys.com> <1484574565-27160-2-git-send-email-vzakhar@synopsys.com> <1485873879.11876.38.camel@synopsys.com> <1485876908.2706.28.camel@synopsys.com> Message-ID: <1486399592.2857.3.camel@synopsys.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 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