Hi Alistair, On Wed, Jul 5, 2017 at 11:26 PM, Alistair Francis <alistai...@gmail.com> wrote:
> On Sun, Jul 2, 2017 at 9:45 PM, Subbaraya Sundeep > <sundeep.l...@gmail.com> wrote: > > Modelled System Timer in Microsemi's Smartfusion2 Soc. > > Timer has two 32bit down counters and two interrupts. > > > > Signed-off-by: Subbaraya Sundeep <sundeep.l...@gmail.com> > > --- > > hw/timer/Makefile.objs | 1 + > > hw/timer/mss-timer.c | 261 ++++++++++++++++++++++++++++++ > +++++++++++++ > > include/hw/timer/mss-timer.h | 67 +++++++++++ > > 3 files changed, 329 insertions(+) > > create mode 100644 hw/timer/mss-timer.c > > create mode 100644 include/hw/timer/mss-timer.h > > > > diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs > > index dd6f27e..fc4d2da 100644 > > --- a/hw/timer/Makefile.objs > > +++ b/hw/timer/Makefile.objs > > @@ -41,3 +41,4 @@ common-obj-$(CONFIG_STM32F2XX_TIMER) += > stm32f2xx_timer.o > > common-obj-$(CONFIG_ASPEED_SOC) += aspeed_timer.o > > > > common-obj-$(CONFIG_SUN4V_RTC) += sun4v-rtc.o > > +common-obj-$(CONFIG_MSF2) += mss-timer.o > > diff --git a/hw/timer/mss-timer.c b/hw/timer/mss-timer.c > > new file mode 100644 > > index 0000000..e46d118 > > --- /dev/null > > +++ b/hw/timer/mss-timer.c > > @@ -0,0 +1,261 @@ > > +/* > > + * Block model of System timer present in > > + * Microsemi's SmartFusion2 and SmartFusion SoCs. > > + * > > + * Copyright (c) 2017 Subbaraya Sundeep <sundeep.l...@gmail.com>. > > + * > > + * Permission is hereby granted, free of charge, to any person > obtaining a copy > > + * of this software and associated documentation files (the > "Software"), to deal > > + * in the Software without restriction, including without limitation > the rights > > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or > sell > > + * copies of the Software, and to permit persons to whom the Software is > > + * furnished to do so, subject to the following conditions: > > + * > > + * The above copyright notice and this permission notice shall be > included in > > + * all copies or substantial portions of the Software. > > + * > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > EXPRESS OR > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > MERCHANTABILITY, > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT > SHALL > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR > OTHER > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, > ARISING FROM, > > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > DEALINGS IN > > + * THE SOFTWARE. > > + */ > > + > > +#include "hw/timer/mss-timer.h" > > + > > +#ifndef MSS_TIMER_ERR_DEBUG > > +#define MSS_TIMER_ERR_DEBUG 0 > > +#endif > > + > > +#define DB_PRINT_L(lvl, fmt, args...) do { \ > > + if (MSS_TIMER_ERR_DEBUG >= lvl) { \ > > + qemu_log("%s: " fmt "\n", __func__, ## args); \ > > + } \ > > +} while (0); > > + > > +#define DB_PRINT(fmt, args...) DB_PRINT_L(1, fmt, ## args) > > + > > +#define R_TIM_VAL 0 > > +#define R_TIM_LOADVAL 1 > > +#define R_TIM_BGLOADVAL 2 > > +#define R_TIM_CTRL 3 > > +#define R_TIM_RIS 4 > > +#define R_TIM_MIS 5 > > + > > +#define TIMER_CTRL_ENBL (1 << 0) > > +#define TIMER_CTRL_ONESHOT (1 << 1) > > +#define TIMER_CTRL_INTR (1 << 2) > > +#define TIMER_RIS_ACK (1 << 0) > > +#define TIMER_RST_CLR (1 << 6) > > +#define TIMER_MODE (1 << 0) > > + > > +static void timer_update_irq(struct Msf2Timer *st) > > +{ > > + bool isr, ier; > > + > > + isr = !!(st->regs[R_TIM_RIS] & TIMER_RIS_ACK); > > + ier = !!(st->regs[R_TIM_CTRL] & TIMER_CTRL_INTR); > > + qemu_set_irq(st->irq, (ier && isr)); > > +} > > + > > +static void timer_update(struct Msf2Timer *st) > > +{ > > + uint64_t count; > > + > > + if (!(st->regs[R_TIM_CTRL] & TIMER_CTRL_ENBL)) { > > + ptimer_stop(st->ptimer); > > + return; > > + } > > + > > + count = st->regs[R_TIM_LOADVAL]; > > + ptimer_set_limit(st->ptimer, count, 1); > > + ptimer_run(st->ptimer, 1); > > +} > > + > > +static uint64_t > > +timer_read(void *opaque, hwaddr offset, unsigned int size) > > +{ > > + MSSTimerState *t = opaque; > > + hwaddr addr; > > + struct Msf2Timer *st; > > + uint32_t ret = 0; > > + int timer = 0; > > + int isr; > > + int ier; > > + > > + addr = offset >> 2; > > + /* > > + * Two independent timers has same base address. > > + * Based on address passed figure out which timer is being used. > > + */ > > + if ((addr >= R_TIM1_MAX) && (addr < NUM_TIMERS * R_TIM1_MAX)) { > > + timer = 1; > > + addr -= R_TIM1_MAX; > > + } > > + > > + st = &t->timers[timer]; > > + > > + switch (addr) { > > + case R_TIM_VAL: > > + ret = ptimer_get_count(st->ptimer); > > + break; > > + > > + case R_TIM_MIS: > > + isr = !!(st->regs[R_TIM_RIS] & TIMER_RIS_ACK); > > + ier = !!(st->regs[R_TIM_CTRL] & TIMER_CTRL_INTR); > > + ret = ier & isr; > > + break; > > + > > + default: > > + if (addr < NUM_TIMERS * R_TIM1_MAX) { > > Shouldn't this just be: addr < R_TIM1_MAX? > At this point you have already subjected the offset for multiple timers. > Yes. You are right. NUM_TIMERS * R_TIM1_MAX will also work but offsets are already changed so I will change to addr < R_TIM1_MAX. > > + ret = st->regs[addr]; > > + } else { > > + qemu_log_mask(LOG_GUEST_ERROR, > > + TYPE_MSS_TIMER": 64-bit mode not supported\n"); > > Maybe add a return here so you don't print the other debug message after > this. > > Ok I will change. > > + } > > + break; > > + } > > + > > + DB_PRINT("timer=%d 0x%" HWADDR_PRIx "=0x%" PRIx32, timer, offset, > > + ret); > > + return ret; > > +} > > + > > +static void > > +timer_write(void *opaque, hwaddr offset, > > + uint64_t val64, unsigned int size) > > +{ > > + MSSTimerState *t = opaque; > > + hwaddr addr; > > + struct Msf2Timer *st; > > + int timer = 0; > > + uint32_t value = val64; > > + > > + addr = offset >> 2; > > + /* > > + * Two independent timers has same base address. > > + * Based on addr passed figure out which timer is being used. > > + */ > > + if ((addr >= R_TIM1_MAX) && (addr < NUM_TIMERS * R_TIM1_MAX)) { > > + timer = 1; > > + addr -= R_TIM1_MAX; > > + } > > + > > + st = &t->timers[timer]; > > + > > + DB_PRINT("addr=0x%" HWADDR_PRIx " val=0x%" PRIx32 " (timer=%d)", > offset, > > + value, timer); > > + > > + switch (addr) { > > + case R_TIM_CTRL: > > + st->regs[R_TIM_CTRL] = value; > > + timer_update(st); > > + break; > > + > > + case R_TIM_RIS: > > + if (value & TIMER_RIS_ACK) { > > + st->regs[R_TIM_RIS] &= ~TIMER_RIS_ACK; > > + } > > + break; > > + > > + case R_TIM_LOADVAL: > > + st->regs[R_TIM_LOADVAL] = value; > > + if (st->regs[R_TIM_CTRL] & TIMER_CTRL_ENBL) { > > + timer_update(st); > > + } > > + break; > > + > > + case R_TIM_BGLOADVAL: > > + st->regs[R_TIM_BGLOADVAL] = value; > > + st->regs[R_TIM_LOADVAL] = value; > > + break; > > + > > + case R_TIM_VAL: > > + case R_TIM_MIS: > > + break; > > + > > + default: > > + if (addr < NUM_TIMERS * R_TIM1_MAX) { > > Same comment as above. > Ok. > > > + st->regs[addr] = value; > > + } else { > > + qemu_log_mask(LOG_GUEST_ERROR, > > + TYPE_MSS_TIMER": 64-bit mode not supported\n"); > > + return; > > + } > > + break; > > + } > > + timer_update_irq(st); > > +} > > Once those comments above are addressed: > > Reviewed-by: Alistair Francis <alistair.fran...@xilinx.com> > Thank you. Sundeep > > Thanks, > Alistair >