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

[Román Cárdenas Rodríguez]

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 */