diff mbox series

Fix CLINT clock frequency for SiFive E

Message ID 20231110161457.60528-1-rcardenas.rod@gmail.com
State New
Headers show
Series Fix CLINT clock frequency for SiFive E | expand

Commit Message

Román Cárdenas Nov. 10, 2023, 4:14 p.m. UTC 
From: Román Cárdenas Rodríguez <rcardenas.rod@gmail.com>

---
 hw/riscv/sifive_e.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Daniel Henrique Barboza Nov. 15, 2023, 8:51 p.m. UTC | #1 
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 */
Román Cárdenas Nov. 15, 2023, 9:06 p.m. UTC | #2 
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 */
Daniel Henrique Barboza Nov. 16, 2023, 11:57 a.m. UTC | #3 
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 */
>
Román Cárdenas Nov. 16, 2023, 12:11 p.m. UTC | #4 
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 mbox series

Patch

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