Re: [Qemu-devel] [Qemu-devel RFC v3 1/5] msf2: Add Smartfusion2 System timer
On Tue, May 2, 2017 at 10:11 PM, sundeep subbarayawrote: > Hi Alistair, > > On Wed, May 3, 2017 at 3:25 AM, Alistair Francis wrote: >> On Fri, Apr 28, 2017 at 9:51 AM, Subbaraya Sundeep >> wrote: >>> Modelled System Timer in Microsemi's Smartfusion2 Soc. >>> Timer has two 32bit down counters and two interrupts. >>> >>> Signed-off-by: Subbaraya Sundeep >>> --- >>> hw/timer/Makefile.objs| 1 + >>> hw/timer/msf2_timer.c | 250 >>> ++ >>> include/hw/timer/msf2_timer.h | 82 ++ >>> 3 files changed, 333 insertions(+) >>> create mode 100644 hw/timer/msf2_timer.c >>> create mode 100644 include/hw/timer/msf2_timer.h >>> >>> diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs >>> index dd6f27e..0bdf1e1 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) += msf2_timer.o >>> diff --git a/hw/timer/msf2_timer.c b/hw/timer/msf2_timer.c >>> new file mode 100644 >>> index 000..d1dbde5 >>> --- /dev/null >>> +++ b/hw/timer/msf2_timer.c >> >> Hey Sundeep, >> >> File names should use '-' instead of '_'. > > Ok will change. >> >>> @@ -0,0 +1,250 @@ >>> +/* >>> + * Timer block model of Microsemi SmartFusion2. >>> + * >>> + * Copyright (c) 2017 Subbaraya Sundeep . >>> + * >>> + * 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 "qemu/osdep.h" >>> +#include "hw/timer/msf2_timer.h" >>> +#include "hw/sysbus.h" >>> +#include "qemu/log.h" >>> +#include "qemu/main-loop.h" >> >> Do you need to include the main-loop.h? This doesn't look right. >> > Ok I will remove it. > >>> + >>> +#ifndef MSF2_TIMER_ERR_DEBUG >>> +#define MSF2_TIMER_ERR_DEBUG 0 >>> +#endif >>> + >>> +#define DB_PRINT_L(lvl, fmt, args...) do { \ >>> +if (MSF2_TIMER_ERR_DEBUG >= lvl) { \ >>> +qemu_log("%s: " fmt, __func__, ## args); \ >>> +} \ >>> +} while (0); >>> + >>> +#define DB_PRINT(fmt, args...) DB_PRINT_L(1, fmt, ## args) >>> + >>> +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; >>> + >>> +DB_PRINT("timer=%d\n", st->nr); >> >> Maybe make this a little more explict. Something like "Updating timer: %d"? >> > Yeah will change that. >>> + >>> +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 addr, unsigned int size) >>> +{ >>> +MSF2TimerState *t = opaque; >>> +struct Msf2Timer *st; >>> +uint32_t r = 0; >> >> Call the ret instead of r and it should be a uint64_t. >> > I will change to ret. I guess uint32_t is fine because same is used > for returning values in 32 bit registers. > >>> +unsigned int timer = 0; >> >> Isn't the timer normally an int? Try to use the same type throughout the >> code. >> > Ok will use int. > >>> +int isr; >>> +int ier; >>> + >>> +addr >>= 2; >>> +/* >>> + * Two independent timers has same base address. >>> + * Based on addr passed figure out which timer is being used. >>> + */ >>> +if (addr >= R_TIM1_MAX)
Re: [Qemu-devel] [Qemu-devel RFC v3 1/5] msf2: Add Smartfusion2 System timer
Hi Alistair, On Wed, May 3, 2017 at 3:25 AM, Alistair Franciswrote: > On Fri, Apr 28, 2017 at 9:51 AM, Subbaraya Sundeep > wrote: >> Modelled System Timer in Microsemi's Smartfusion2 Soc. >> Timer has two 32bit down counters and two interrupts. >> >> Signed-off-by: Subbaraya Sundeep >> --- >> hw/timer/Makefile.objs| 1 + >> hw/timer/msf2_timer.c | 250 >> ++ >> include/hw/timer/msf2_timer.h | 82 ++ >> 3 files changed, 333 insertions(+) >> create mode 100644 hw/timer/msf2_timer.c >> create mode 100644 include/hw/timer/msf2_timer.h >> >> diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs >> index dd6f27e..0bdf1e1 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) += msf2_timer.o >> diff --git a/hw/timer/msf2_timer.c b/hw/timer/msf2_timer.c >> new file mode 100644 >> index 000..d1dbde5 >> --- /dev/null >> +++ b/hw/timer/msf2_timer.c > > Hey Sundeep, > > File names should use '-' instead of '_'. Ok will change. > >> @@ -0,0 +1,250 @@ >> +/* >> + * Timer block model of Microsemi SmartFusion2. >> + * >> + * Copyright (c) 2017 Subbaraya Sundeep . >> + * >> + * 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 "qemu/osdep.h" >> +#include "hw/timer/msf2_timer.h" >> +#include "hw/sysbus.h" >> +#include "qemu/log.h" >> +#include "qemu/main-loop.h" > > Do you need to include the main-loop.h? This doesn't look right. > Ok I will remove it. >> + >> +#ifndef MSF2_TIMER_ERR_DEBUG >> +#define MSF2_TIMER_ERR_DEBUG 0 >> +#endif >> + >> +#define DB_PRINT_L(lvl, fmt, args...) do { \ >> +if (MSF2_TIMER_ERR_DEBUG >= lvl) { \ >> +qemu_log("%s: " fmt, __func__, ## args); \ >> +} \ >> +} while (0); >> + >> +#define DB_PRINT(fmt, args...) DB_PRINT_L(1, fmt, ## args) >> + >> +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; >> + >> +DB_PRINT("timer=%d\n", st->nr); > > Maybe make this a little more explict. Something like "Updating timer: %d"? > Yeah will change that. >> + >> +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 addr, unsigned int size) >> +{ >> +MSF2TimerState *t = opaque; >> +struct Msf2Timer *st; >> +uint32_t r = 0; > > Call the ret instead of r and it should be a uint64_t. > I will change to ret. I guess uint32_t is fine because same is used for returning values in 32 bit registers. >> +unsigned int timer = 0; > > Isn't the timer normally an int? Try to use the same type throughout the code. > Ok will use int. >> +int isr; >> +int ier; >> + >> +addr >>= 2; >> +/* >> + * Two independent timers has same base address. >> + * Based on addr passed figure out which timer is being used. >> + */ >> +if (addr >= R_TIM1_MAX) { >> +timer = 1; >> +addr -= R_TIM1_MAX; >> +} >> + >> +st = >timers[timer]; >> + >> +switch (addr) { >> +case R_TIM_VAL: >> +r = ptimer_get_count(st->ptimer); >> +DB_PRINT("msf2_timer t=%d read
Re: [Qemu-devel] [Qemu-devel RFC v3 1/5] msf2: Add Smartfusion2 System timer
On Fri, Apr 28, 2017 at 9:51 AM, Subbaraya Sundeepwrote: > Modelled System Timer in Microsemi's Smartfusion2 Soc. > Timer has two 32bit down counters and two interrupts. > > Signed-off-by: Subbaraya Sundeep > --- > hw/timer/Makefile.objs| 1 + > hw/timer/msf2_timer.c | 250 > ++ > include/hw/timer/msf2_timer.h | 82 ++ > 3 files changed, 333 insertions(+) > create mode 100644 hw/timer/msf2_timer.c > create mode 100644 include/hw/timer/msf2_timer.h > > diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs > index dd6f27e..0bdf1e1 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) += msf2_timer.o > diff --git a/hw/timer/msf2_timer.c b/hw/timer/msf2_timer.c > new file mode 100644 > index 000..d1dbde5 > --- /dev/null > +++ b/hw/timer/msf2_timer.c Hey Sundeep, File names should use '-' instead of '_'. > @@ -0,0 +1,250 @@ > +/* > + * Timer block model of Microsemi SmartFusion2. > + * > + * Copyright (c) 2017 Subbaraya Sundeep . > + * > + * 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 "qemu/osdep.h" > +#include "hw/timer/msf2_timer.h" > +#include "hw/sysbus.h" > +#include "qemu/log.h" > +#include "qemu/main-loop.h" Do you need to include the main-loop.h? This doesn't look right. > + > +#ifndef MSF2_TIMER_ERR_DEBUG > +#define MSF2_TIMER_ERR_DEBUG 0 > +#endif > + > +#define DB_PRINT_L(lvl, fmt, args...) do { \ > +if (MSF2_TIMER_ERR_DEBUG >= lvl) { \ > +qemu_log("%s: " fmt, __func__, ## args); \ > +} \ > +} while (0); > + > +#define DB_PRINT(fmt, args...) DB_PRINT_L(1, fmt, ## args) > + > +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; > + > +DB_PRINT("timer=%d\n", st->nr); Maybe make this a little more explict. Something like "Updating timer: %d"? > + > +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 addr, unsigned int size) > +{ > +MSF2TimerState *t = opaque; > +struct Msf2Timer *st; > +uint32_t r = 0; Call the ret instead of r and it should be a uint64_t. > +unsigned int timer = 0; Isn't the timer normally an int? Try to use the same type throughout the code. > +int isr; > +int ier; > + > +addr >>= 2; > +/* > + * Two independent timers has same base address. > + * Based on addr passed figure out which timer is being used. > + */ > +if (addr >= R_TIM1_MAX) { > +timer = 1; > +addr -= R_TIM1_MAX; > +} > + > +st = >timers[timer]; > + > +switch (addr) { > +case R_TIM_VAL: > +r = ptimer_get_count(st->ptimer); > +DB_PRINT("msf2_timer t=%d read counter=%x\n", timer, r); Put a 0x in front of hex values. Also %x isn't the correct type for uint64_t, it's something similar to PRIx. > +break; > + > +case R_TIM_MIS: > +isr = !!(st->regs[R_TIM_RIS] & TIMER_RIS_ACK); > +ier = !!(st->regs[R_TIM_CTRL] & TIMER_CTRL_INTR); > +r = ier && isr; This should be a bitwise AND. > +break; > + > +default: > +if (addr < ARRAY_SIZE(st->regs)) { > +r
[Qemu-devel] [Qemu-devel RFC v3 1/5] msf2: Add Smartfusion2 System timer
Modelled System Timer in Microsemi's Smartfusion2 Soc. Timer has two 32bit down counters and two interrupts. Signed-off-by: Subbaraya Sundeep--- hw/timer/Makefile.objs| 1 + hw/timer/msf2_timer.c | 250 ++ include/hw/timer/msf2_timer.h | 82 ++ 3 files changed, 333 insertions(+) create mode 100644 hw/timer/msf2_timer.c create mode 100644 include/hw/timer/msf2_timer.h diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs index dd6f27e..0bdf1e1 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) += msf2_timer.o diff --git a/hw/timer/msf2_timer.c b/hw/timer/msf2_timer.c new file mode 100644 index 000..d1dbde5 --- /dev/null +++ b/hw/timer/msf2_timer.c @@ -0,0 +1,250 @@ +/* + * Timer block model of Microsemi SmartFusion2. + * + * Copyright (c) 2017 Subbaraya Sundeep . + * + * 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 "qemu/osdep.h" +#include "hw/timer/msf2_timer.h" +#include "hw/sysbus.h" +#include "qemu/log.h" +#include "qemu/main-loop.h" + +#ifndef MSF2_TIMER_ERR_DEBUG +#define MSF2_TIMER_ERR_DEBUG 0 +#endif + +#define DB_PRINT_L(lvl, fmt, args...) do { \ +if (MSF2_TIMER_ERR_DEBUG >= lvl) { \ +qemu_log("%s: " fmt, __func__, ## args); \ +} \ +} while (0); + +#define DB_PRINT(fmt, args...) DB_PRINT_L(1, fmt, ## args) + +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; + +DB_PRINT("timer=%d\n", st->nr); + +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 addr, unsigned int size) +{ +MSF2TimerState *t = opaque; +struct Msf2Timer *st; +uint32_t r = 0; +unsigned int timer = 0; +int isr; +int ier; + +addr >>= 2; +/* + * Two independent timers has same base address. + * Based on addr passed figure out which timer is being used. + */ +if (addr >= R_TIM1_MAX) { +timer = 1; +addr -= R_TIM1_MAX; +} + +st = >timers[timer]; + +switch (addr) { +case R_TIM_VAL: +r = ptimer_get_count(st->ptimer); +DB_PRINT("msf2_timer t=%d read counter=%x\n", timer, r); +break; + +case R_TIM_MIS: +isr = !!(st->regs[R_TIM_RIS] & TIMER_RIS_ACK); +ier = !!(st->regs[R_TIM_CTRL] & TIMER_CTRL_INTR); +r = ier && isr; +break; + +default: +if (addr < ARRAY_SIZE(st->regs)) { +r = st->regs[addr]; +} +break; +} +DB_PRINT("timer=%d %lu=%x\n", timer, addr * 4, r); +return r; +} + +static void +timer_write(void *opaque, hwaddr addr, +uint64_t val64, unsigned int size) +{ +MSF2TimerState *t = opaque; +struct Msf2Timer *st; +unsigned int timer = 0; +uint32_t value = val64; + +addr >>= 2; +/* + * Two independent timers has same base address. + * Based on addr passed figure out which timer is being used. + */ +if (addr >= R_TIM1_MAX) { +timer = 1; +addr -= R_TIM1_MAX; +} + +st = >timers[timer]; + +DB_PRINT("addr=%lu val=%x (timer=%d)\n", addr * 4, value, timer); + +switch (addr) { +case R_TIM_CTRL: +st->regs[R_TIM_CTRL] = value; +timer_update(st); +break; + +case