On Tue, Mar 8, 2022 at 1:33 PM Alistair Francis <alistai...@gmail.com> wrote:
>
> On Fri, Mar 4, 2022 at 2:08 PM Anup Patel <a...@brainfault.org> wrote:
> >
> > On Fri, Mar 4, 2022 at 8:50 AM Atish Patra <ati...@rivosinc.com> wrote:
> > >
> > > Currently, the aclint and ibex timer devices uses the "timer" &
> > > "timecmp" to generate the m-mode timer interrupt. In future,
> > > we will have timer interrupt injected to S/VS mode directly.
> > > No functionality change introduced in this patch.
> >
> > s/have timer/have a timer/
> >
> > >
> > > Add a prefix "m" these enviornment variables to indicate its
> > > true purpose.
> >
> > s/enviornment/environment/
> >
> > >
> > > Signed-off-by: Atish Patra <ati...@rivosinc.com>
> >
> > I suggest we should remove mtimecmp and mtimer from
> > target/riscv because mtimer is a memory mapped device.
>
> I agree. I guess this is in the CPU as it's per hart, and it's easy to
> store here instead of in the timer, but we could convert aclint to use
> an array of timers (Ibex only ever has one hart).
>
> That would probably be a closer match to hardware, as the timer is
> external to the CPU
>
ok sure. I will revise that.
> Alistair
>
> >
> > Regards,
> > Anup
> >
> > > ---
> > > hw/intc/riscv_aclint.c | 20 ++++++++++----------
> > > hw/timer/ibex_timer.c | 14 +++++++-------
> > > target/riscv/cpu.h | 4 ++--
> > > target/riscv/machine.c | 2 +-
> > > 4 files changed, 20 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/hw/intc/riscv_aclint.c b/hw/intc/riscv_aclint.c
> > > index f1a5d3d284fd..642794a06865 100644
> > > --- a/hw/intc/riscv_aclint.c
> > > +++ b/hw/intc/riscv_aclint.c
> > > @@ -59,8 +59,8 @@ static void
> > > riscv_aclint_mtimer_write_timecmp(RISCVAclintMTimerState *mtimer,
> > >
> > > uint64_t rtc_r = cpu_riscv_read_rtc(timebase_freq);
> > >
> > > - cpu->env.timecmp = value;
> > > - if (cpu->env.timecmp <= rtc_r) {
> > > + cpu->env.mtimecmp = value;
> > > + if (cpu->env.mtimecmp <= rtc_r) {
> > > /*
> > > * If we're setting an MTIMECMP value in the "past",
> > > * immediately raise the timer interrupt
> > > @@ -71,7 +71,7 @@ static void
> > > riscv_aclint_mtimer_write_timecmp(RISCVAclintMTimerState *mtimer,
> > >
> > > /* otherwise, set up the future timer interrupt */
> > > qemu_irq_lower(mtimer->timer_irqs[hartid - mtimer->hartid_base]);
> > > - diff = cpu->env.timecmp - rtc_r;
> > > + diff = cpu->env.mtimecmp - rtc_r;
> > > /* back to ns (note args switched in muldiv64) */
> > > uint64_t ns_diff = muldiv64(diff, NANOSECONDS_PER_SECOND,
> > > timebase_freq);
> > >
> > > @@ -96,7 +96,7 @@ static void
> > > riscv_aclint_mtimer_write_timecmp(RISCVAclintMTimerState *mtimer,
> > > next = MIN(next, INT64_MAX);
> > > }
> > >
> > > - timer_mod(cpu->env.timer, next);
> > > + timer_mod(cpu->env.mtimer, next);
> > > }
> > >
> > > /*
> > > @@ -127,11 +127,11 @@ static uint64_t riscv_aclint_mtimer_read(void
> > > *opaque, hwaddr addr,
> > > "aclint-mtimer: invalid hartid: %zu", hartid);
> > > } else if ((addr & 0x7) == 0) {
> > > /* timecmp_lo */
> > > - uint64_t timecmp = env->timecmp;
> > > + uint64_t timecmp = env->mtimecmp;
> > > return timecmp & 0xFFFFFFFF;
> > > } else if ((addr & 0x7) == 4) {
> > > /* timecmp_hi */
> > > - uint64_t timecmp = env->timecmp;
> > > + uint64_t timecmp = env->mtimecmp;
> > > return (timecmp >> 32) & 0xFFFFFFFF;
> > > } else {
> > > qemu_log_mask(LOG_UNIMP,
> > > @@ -168,14 +168,14 @@ static void riscv_aclint_mtimer_write(void *opaque,
> > > hwaddr addr,
> > > "aclint-mtimer: invalid hartid: %zu", hartid);
> > > } else if ((addr & 0x7) == 0) {
> > > /* timecmp_lo */
> > > - uint64_t timecmp_hi = env->timecmp >> 32;
> > > + uint64_t timecmp_hi = env->mtimecmp >> 32;
> > > riscv_aclint_mtimer_write_timecmp(mtimer, RISCV_CPU(cpu),
> > > hartid,
> > > timecmp_hi << 32 | (value & 0xFFFFFFFF),
> > > mtimer->timebase_freq);
> > > return;
> > > } else if ((addr & 0x7) == 4) {
> > > /* timecmp_hi */
> > > - uint64_t timecmp_lo = env->timecmp;
> > > + uint64_t timecmp_lo = env->mtimecmp;
> > > riscv_aclint_mtimer_write_timecmp(mtimer, RISCV_CPU(cpu),
> > > hartid,
> > > value << 32 | (timecmp_lo & 0xFFFFFFFF),
> > > mtimer->timebase_freq);
> > > @@ -304,9 +304,9 @@ DeviceState *riscv_aclint_mtimer_create(hwaddr addr,
> > > hwaddr size,
> > >
> > > cb->s = RISCV_ACLINT_MTIMER(dev);
> > > cb->num = i;
> > > - env->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> > > + env->mtimer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> > > &riscv_aclint_mtimer_cb, cb);
> > > - env->timecmp = 0;
> > > + env->mtimecmp = 0;
> > >
> > > qdev_connect_gpio_out(dev, i,
> > > qdev_get_gpio_in(DEVICE(rvcpu),
> > > IRQ_M_TIMER));
> > > diff --git a/hw/timer/ibex_timer.c b/hw/timer/ibex_timer.c
> > > index 8c2ca364daab..4c34f9e08282 100644
> > > --- a/hw/timer/ibex_timer.c
> > > +++ b/hw/timer/ibex_timer.c
> > > @@ -73,9 +73,9 @@ static void ibex_timer_update_irqs(IbexTimerState *s)
> > > }
> > >
> > > /* Update the CPUs mtimecmp */
> > > - cpu->env.timecmp = value;
> > > + cpu->env.mtimecmp = value;
> > >
> > > - if (cpu->env.timecmp <= now) {
> > > + if (cpu->env.mtimecmp <= now) {
> > > /*
> > > * If the mtimecmp was in the past raise the interrupt now.
> > > */
> > > @@ -91,7 +91,7 @@ static void ibex_timer_update_irqs(IbexTimerState *s)
> > > qemu_irq_lower(s->m_timer_irq);
> > > qemu_set_irq(s->irq, false);
> > >
> > > - diff = cpu->env.timecmp - now;
> > > + diff = cpu->env.mtimecmp - now;
> > > next = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> > > muldiv64(diff,
> > > NANOSECONDS_PER_SECOND,
> > > @@ -99,9 +99,9 @@ static void ibex_timer_update_irqs(IbexTimerState *s)
> > >
> > > if (next < qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)) {
> > > /* We overflowed the timer, just set it as large as we can */
> > > - timer_mod(cpu->env.timer, 0x7FFFFFFFFFFFFFFF);
> > > + timer_mod(cpu->env.mtimer, 0x7FFFFFFFFFFFFFFF);
> > > } else {
> > > - timer_mod(cpu->env.timer, next);
> > > + timer_mod(cpu->env.mtimer, next);
> > > }
> > > }
> > >
> > > @@ -122,9 +122,9 @@ static void ibex_timer_reset(DeviceState *dev)
> > >
> > > CPUState *cpu = qemu_get_cpu(0);
> > > CPURISCVState *env = cpu->env_ptr;
> > > - env->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> > > + env->mtimer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> > > &ibex_timer_cb, s);
> > > - env->timecmp = 0;
> > > + env->mtimecmp = 0;
> > >
> > > s->timer_ctrl = 0x00000000;
> > > s->timer_cfg0 = 0x00010000;
> > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > > index 5d914bd34550..94234c59ffa8 100644
> > > --- a/target/riscv/cpu.h
> > > +++ b/target/riscv/cpu.h
> > > @@ -265,7 +265,7 @@ struct CPURISCVState {
> > > /* temporary htif regs */
> > > uint64_t mfromhost;
> > > uint64_t mtohost;
> > > - uint64_t timecmp;
> > > + uint64_t mtimecmp;
> > >
> > > /* physical memory protection */
> > > pmp_table_t pmp_state;
> > > @@ -316,7 +316,7 @@ struct CPURISCVState {
> > > float_status fp_status;
> > >
> > > /* Fields from here on are preserved across CPU reset. */
> > > - QEMUTimer *timer; /* Internal timer */
> > > + QEMUTimer *mtimer; /* Internal timer for M-mode interrupt */
> > >
> > > hwaddr kernel_addr;
> > > hwaddr fdt_addr;
> > > diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> > > index ebc33c9e2781..be3022082a46 100644
> > > --- a/target/riscv/machine.c
> > > +++ b/target/riscv/machine.c
> > > @@ -303,7 +303,7 @@ const VMStateDescription vmstate_riscv_cpu = {
> > > VMSTATE_UINTTL(env.mscratch, RISCVCPU),
> > > VMSTATE_UINT64(env.mfromhost, RISCVCPU),
> > > VMSTATE_UINT64(env.mtohost, RISCVCPU),
> > > - VMSTATE_UINT64(env.timecmp, RISCVCPU),
> > > + VMSTATE_UINT64(env.mtimecmp, RISCVCPU),
> > >
> > > VMSTATE_END_OF_LIST()
> > > },
> > > --
> > > 2.30.2
> > >
> > >
> >
>
--
Regards,
Atish
