Message ID | 20231110161457.60528-1-rcardenas.rod@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Fix CLINT clock frequency for SiFive E | expand |
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 */
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.), 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.   > 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 */
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 */ >
Done! Let me know if I missed something Kind regards, Román > On 16 Nov 2023, at 12:57, Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote: > > > > 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 */
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); sifive_e_prci_create(memmap[SIFIVE_E_DEV_PRCI].base); /* AON */
From: Román Cárdenas Rodríguez <rcardenas.rod@gmail.com> --- hw/riscv/sifive_e.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)