Re: [PATCH] Fix CLINT clock frequency for SiFive E

[Daniel Henrique Barboza]

On 11/15/23 18:06, Román Cárdenas Rodríguez wrote:
> Hi, Daniel!
>
> Sorry for that, I’m quite new to this way of contributing (you may notice that 
> I sent the same patch several times, I apologize).
>
> If you check the manual of SiFive E310 
> (https://cdn.sparkfun.com/assets/7/f/0/2/7/fe310-g002-manual-v19p05.pdf?_gl=1*w2ieef*_ga*MTcyNDI2MjM0Ny4xNjk2ODcwNTM3*_ga_T369JS7J9N*MTY5Njg3MDUzNy4xLjAuMTY5Njg3MDUzNy42MC4wLjA.
>  
> <https://cdn.sparkfun.com/assets/7/f/0/2/7/fe310-g002-manual-v19p05.pdf?_gl=1*w2ieef*_ga*MTcyNDI2MjM0Ny4xNjk2ODcwNTM3*_ga_T369JS7J9N*MTY5Njg3MDUzNy4xLjAuMTY5Njg3MDUzNy42MC4wLjA.>),
>  you can see in Figure 1 that the CLINT is connected to the real time clock, which 
> also feeds the AON peripheral (they share the same clock).
>
> In page 43, the docs also say that the timer registers of the CLINT count ticks 
> from the rtcclk.
>
> I attached a couple of screenshots to ease the process.
>
> Thank you very much for your time, let me know if I can help you with further 
> documentation
>
> PS: I am currently playing with bare metal applications both in QEMU and a 
> physical SiFive E310 board and I confirm that the CLINT clock in the physical 
> board runs at 32.768 kHz. In QEMU, the same app produces a completely different 
> outcome, as sometimes a new CLINT interrupt is triggered before finishing other 
> tasks.


I read the doc and this makes sense.

Can you please re-send the patch, same code, but with all this information in 
the commit
message? We need this context to justify changing the existing sifive-e board 
clock.

ps: It would be nice if someone from Sifive could drop an ACK in this change.


Thanks,

Daniel

> Screenshot 2023-11-15 at 21.58.17.png
>
> Screenshot 2023-11-15 at 21.57.27.png
>
> > On 15 Nov 2023, at 21:51, Daniel Henrique Barboza <dbarboza@ventanamicro.com> 
> > wrote:
> >
> > Hi Roman!
> >
> > It helps to add the maintainers/reviewers in the CC when sending the patch. You
> > can see who need to be CCed by using the get_maintainer.pl script. E.g:
> >
> > ./scripts/get_maintainer.pl \[PATCH\]\ Fix\ CLINT\ clock\ frequency\ for\ 
> > SiFive\ E\ -\ rcardenas.rod@gmail.com\ -\ 2023-11-10\ 1314.eml
> >
> > Alistair Francis <Alistair.Francis@wdc.com> (supporter:SiFive Machines)
> > Bin Meng <bin.meng@windriver.com> (supporter:SiFive Machines)
> > Palmer Dabbelt <palmer@dabbelt.com> (supporter:SiFive Machines)
> > Weiwei Li <liwei1518@gmail.com> (reviewer:RISC-V TCG CPUs)
> > Daniel Henrique Barboza <dbarboza@ventanamicro.com> (reviewer:RISC-V TCG CPUs)
> > Liu Zhiwei <zhiwei_liu@linux.alibaba.com> (reviewer:RISC-V TCG CPUs)
> > qemu-riscv@nongnu.org (open list:SiFive Machines)
> > qemu-devel@nongnu.org (open list:All patches CC here)
> >
> >
> > I'm CCing all these folks in the reply.
> >
> >
> > On 11/10/23 13:14, rcardenas.rod@gmail.com wrote:
> > > From: Román Cárdenas Rodríguez <rcardenas.rod@gmail.com>
> > > ---
> > >  hw/riscv/sifive_e.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
> > > index 0d37adc542..87d9602383 100644
> > > --- a/hw/riscv/sifive_e.c
> > > +++ b/hw/riscv/sifive_e.c
> > > @@ -225,7 +225,7 @@ static void sifive_e_soc_realize(DeviceState *dev, Error 
> > > **errp)
> > >              RISCV_ACLINT_SWI_SIZE,
> > >          RISCV_ACLINT_DEFAULT_MTIMER_SIZE, 0, ms->smp.cpus,
> > >          RISCV_ACLINT_DEFAULT_MTIMECMP, RISCV_ACLINT_DEFAULT_MTIME,
> > > -        RISCV_ACLINT_DEFAULT_TIMEBASE_FREQ, false);
> > > +        SIFIVE_E_LFCLK_DEFAULT_FREQ, false);
> >
> > I'm not sure if this is correct. The last commit that touched this line was 
> > b8fb878aa2
> > ("hw/intc: Upgrade the SiFive CLINT implementation to RISC-V ACLINT"). If you 
> > see the commit
> > diff, the previous value was:
> >
> > -            SIFIVE_CLINT_TIMEBASE_FREQ, false);
> >
> > In this same commit we can see that the existing value of that macro back then 
> > was:
> >
> > -    SIFIVE_CLINT_TIMEBASE_FREQ = 10000000
> >
> >
> > Which is the same value of RISCV_ACLINT_DEFAULT_TIMEBASE_FREQ.
> >
> >
> > The value you're changing to, SIFIVE_E_LFCLK_DEFAULT_FREQ (32768), seems to be 
> > related to the
> > SIFIVE AON watchdog implemented in hw/misc/sifive_e_aon.c:
> >
> > static void sifive_e_aon_init(Object *obj)
> > {
> >    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> >    SiFiveEAONState *r = SIFIVE_E_AON(obj);
> >
> >    memory_region_init_io(&r->mmio, OBJECT(r), &sifive_e_aon_ops, r,
> >                          TYPE_SIFIVE_E_AON, SIFIVE_E_AON_MAX);
> >    sysbus_init_mmio(sbd, &r->mmio);
> >
> >    /* watchdog timer */
> >    r->wdog_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> >                                 sifive_e_aon_wdt_expired_cb, r);
> >    r->wdogclk_freq = SIFIVE_E_LFCLK_DEFAULT_FREQ;   <==========
> >    sysbus_init_irq(sbd, &r->wdog_irq);
> > }
> >
> >
> >
> > Thanks,
> >
> >
> > Daniel
> >
> >
> >
> > >      sifive_e_prci_create(memmap[SIFIVE_E_DEV_PRCI].base);
> > >        /* AON */