Hi Alistair, On Wed, Jul 5, 2017 at 11:28 PM, Alistair Francis <alistai...@gmail.com> wrote:
> On Wed, Jul 5, 2017 at 10:56 AM, 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" > > Also just noticed (this applies to the whole series) that you need the > headers in the C file. > > Move every #include that you can from the mss-timer.h files to this > file, headers should be local to where they are called. Also make sure > you include the os-dep header in every file. > Ok will fix this. Thanks, Sundeep > > Thanks, > Alistair > > >> + > >> +#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. > > > >> + 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. > > > >> + } > >> + 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. > > > >> + 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> > > > > Thanks, > > Alistair >