* [Qemu-devel] [PATCH] riscv: sifive_e: Correct various SoC IP block sizes
@ 2019-08-03 0:27 Bin Meng
2019-08-05 6:14 ` [Qemu-devel] [Qemu-riscv] " Chih-Min Chao
2019-08-05 17:51 ` [Qemu-devel] " Alistair Francis
0 siblings, 2 replies; 12+ messages in thread
From: Bin Meng @ 2019-08-03 0:27 UTC (permalink / raw)
To: Alistair Francis, Bastian Koppelmann, Palmer Dabbelt,
Sagar Karandikar, qemu-devel, qemu-riscv
Some of the SoC IP block sizes are wrong. Correct them according
to the FE310 manual.
Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---
hw/riscv/sifive_e.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
index 2a499d8..9655847 100644
--- a/hw/riscv/sifive_e.c
+++ b/hw/riscv/sifive_e.c
@@ -53,13 +53,13 @@ static const struct MemmapEntry {
hwaddr base;
hwaddr size;
} sifive_e_memmap[] = {
- [SIFIVE_E_DEBUG] = { 0x0, 0x100 },
+ [SIFIVE_E_DEBUG] = { 0x0, 0x1000 },
[SIFIVE_E_MROM] = { 0x1000, 0x2000 },
[SIFIVE_E_OTP] = { 0x20000, 0x2000 },
[SIFIVE_E_CLINT] = { 0x2000000, 0x10000 },
[SIFIVE_E_PLIC] = { 0xc000000, 0x4000000 },
- [SIFIVE_E_AON] = { 0x10000000, 0x8000 },
- [SIFIVE_E_PRCI] = { 0x10008000, 0x8000 },
+ [SIFIVE_E_AON] = { 0x10000000, 0x1000 },
+ [SIFIVE_E_PRCI] = { 0x10008000, 0x1000 },
[SIFIVE_E_OTP_CTRL] = { 0x10010000, 0x1000 },
[SIFIVE_E_GPIO0] = { 0x10012000, 0x1000 },
[SIFIVE_E_UART0] = { 0x10013000, 0x1000 },
--
2.7.4
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [Qemu-riscv] [PATCH] riscv: sifive_e: Correct various SoC IP block sizes
2019-08-03 0:27 [Qemu-devel] [PATCH] riscv: sifive_e: Correct various SoC IP block sizes Bin Meng
@ 2019-08-05 6:14 ` Chih-Min Chao
2019-08-05 6:43 ` Bin Meng
2019-08-05 17:51 ` [Qemu-devel] " Alistair Francis
1 sibling, 1 reply; 12+ messages in thread
From: Chih-Min Chao @ 2019-08-05 6:14 UTC (permalink / raw)
To: Bin Meng
Cc: open list:RISC-V, Sagar Karandikar, Bastian Koppelmann,
Palmer Dabbelt, qemu-devel@nongnu.org Developers,
Alistair Francis
On Sat, Aug 3, 2019 at 8:27 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> Some of the SoC IP block sizes are wrong. Correct them according
> to the FE310 manual.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
> hw/riscv/sifive_e.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
> index 2a499d8..9655847 100644
> --- a/hw/riscv/sifive_e.c
> +++ b/hw/riscv/sifive_e.c
> @@ -53,13 +53,13 @@ static const struct MemmapEntry {
> hwaddr base;
> hwaddr size;
> } sifive_e_memmap[] = {
> - [SIFIVE_E_DEBUG] = { 0x0, 0x100 },
> + [SIFIVE_E_DEBUG] = { 0x0, 0x1000 },
> [SIFIVE_E_MROM] = { 0x1000, 0x2000 },
> [SIFIVE_E_OTP] = { 0x20000, 0x2000 },
> [SIFIVE_E_CLINT] = { 0x2000000, 0x10000 },
> [SIFIVE_E_PLIC] = { 0xc000000, 0x4000000 },
> - [SIFIVE_E_AON] = { 0x10000000, 0x8000 },
> - [SIFIVE_E_PRCI] = { 0x10008000, 0x8000 },
> + [SIFIVE_E_AON] = { 0x10000000, 0x1000 },
> + [SIFIVE_E_PRCI] = { 0x10008000, 0x1000 },
> [SIFIVE_E_OTP_CTRL] = { 0x10010000, 0x1000 },
> [SIFIVE_E_GPIO0] = { 0x10012000, 0x1000 },
> [SIFIVE_E_UART0] = { 0x10013000, 0x1000 },
> --
> 2.7.4
>
>
It seems the modification follows E310-G002(Hifive1 Rev B) spec and the
origin is for E310-G000(Hifive1) spec.
There should be some way to specify different board version with different
memory map or we have policy, always support the latest spec.
chihmin
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [Qemu-riscv] [PATCH] riscv: sifive_e: Correct various SoC IP block sizes
2019-08-05 6:14 ` [Qemu-devel] [Qemu-riscv] " Chih-Min Chao
@ 2019-08-05 6:43 ` Bin Meng
2019-08-06 21:06 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 12+ messages in thread
From: Bin Meng @ 2019-08-05 6:43 UTC (permalink / raw)
To: Chih-Min Chao
Cc: open list:RISC-V, Sagar Karandikar, Bastian Koppelmann,
Palmer Dabbelt, qemu-devel@nongnu.org Developers,
Alistair Francis
On Mon, Aug 5, 2019 at 2:14 PM Chih-Min Chao <chihmin.chao@sifive.com> wrote:
>
>
>
> On Sat, Aug 3, 2019 at 8:27 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>>
>> Some of the SoC IP block sizes are wrong. Correct them according
>> to the FE310 manual.
>>
>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>> ---
>>
>> hw/riscv/sifive_e.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
>> index 2a499d8..9655847 100644
>> --- a/hw/riscv/sifive_e.c
>> +++ b/hw/riscv/sifive_e.c
>> @@ -53,13 +53,13 @@ static const struct MemmapEntry {
>> hwaddr base;
>> hwaddr size;
>> } sifive_e_memmap[] = {
>> - [SIFIVE_E_DEBUG] = { 0x0, 0x100 },
>> + [SIFIVE_E_DEBUG] = { 0x0, 0x1000 },
>> [SIFIVE_E_MROM] = { 0x1000, 0x2000 },
>> [SIFIVE_E_OTP] = { 0x20000, 0x2000 },
>> [SIFIVE_E_CLINT] = { 0x2000000, 0x10000 },
>> [SIFIVE_E_PLIC] = { 0xc000000, 0x4000000 },
>> - [SIFIVE_E_AON] = { 0x10000000, 0x8000 },
>> - [SIFIVE_E_PRCI] = { 0x10008000, 0x8000 },
>> + [SIFIVE_E_AON] = { 0x10000000, 0x1000 },
>> + [SIFIVE_E_PRCI] = { 0x10008000, 0x1000 },
>> [SIFIVE_E_OTP_CTRL] = { 0x10010000, 0x1000 },
>> [SIFIVE_E_GPIO0] = { 0x10012000, 0x1000 },
>> [SIFIVE_E_UART0] = { 0x10013000, 0x1000 },
>> --
>> 2.7.4
>>
>
> It seems the modification follows E310-G002(Hifive1 Rev B) spec and the origin is for E310-G000(Hifive1) spec.
> There should be some way to specify different board version with different memory map or we have policy, always support the latest spec.
>
Yes, I checked both specs. The older spec says these bigger sizes,
however their register sizes fit well in the smaller range as well. So
I think the modification works well for both.
Regards,
Bin
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH] riscv: sifive_e: Correct various SoC IP block sizes
2019-08-03 0:27 [Qemu-devel] [PATCH] riscv: sifive_e: Correct various SoC IP block sizes Bin Meng
2019-08-05 6:14 ` [Qemu-devel] [Qemu-riscv] " Chih-Min Chao
@ 2019-08-05 17:51 ` Alistair Francis
1 sibling, 0 replies; 12+ messages in thread
From: Alistair Francis @ 2019-08-05 17:51 UTC (permalink / raw)
To: Bin Meng
Cc: open list:RISC-V, Sagar Karandikar, Bastian Koppelmann,
Palmer Dabbelt, qemu-devel@nongnu.org Developers,
Alistair Francis
On Fri, Aug 2, 2019 at 5:27 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Some of the SoC IP block sizes are wrong. Correct them according
> to the FE310 manual.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
>
> hw/riscv/sifive_e.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
> index 2a499d8..9655847 100644
> --- a/hw/riscv/sifive_e.c
> +++ b/hw/riscv/sifive_e.c
> @@ -53,13 +53,13 @@ static const struct MemmapEntry {
> hwaddr base;
> hwaddr size;
> } sifive_e_memmap[] = {
> - [SIFIVE_E_DEBUG] = { 0x0, 0x100 },
> + [SIFIVE_E_DEBUG] = { 0x0, 0x1000 },
> [SIFIVE_E_MROM] = { 0x1000, 0x2000 },
> [SIFIVE_E_OTP] = { 0x20000, 0x2000 },
> [SIFIVE_E_CLINT] = { 0x2000000, 0x10000 },
> [SIFIVE_E_PLIC] = { 0xc000000, 0x4000000 },
> - [SIFIVE_E_AON] = { 0x10000000, 0x8000 },
> - [SIFIVE_E_PRCI] = { 0x10008000, 0x8000 },
> + [SIFIVE_E_AON] = { 0x10000000, 0x1000 },
> + [SIFIVE_E_PRCI] = { 0x10008000, 0x1000 },
> [SIFIVE_E_OTP_CTRL] = { 0x10010000, 0x1000 },
> [SIFIVE_E_GPIO0] = { 0x10012000, 0x1000 },
> [SIFIVE_E_UART0] = { 0x10013000, 0x1000 },
> --
> 2.7.4
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [Qemu-riscv] [PATCH] riscv: sifive_e: Correct various SoC IP block sizes
2019-08-05 6:43 ` Bin Meng
@ 2019-08-06 21:06 ` Philippe Mathieu-Daudé
2019-08-07 1:36 ` Bin Meng
2019-08-07 2:53 ` Bin Meng
0 siblings, 2 replies; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-08-06 21:06 UTC (permalink / raw)
To: Bin Meng, Chih-Min Chao
Cc: open list:RISC-V, Sagar Karandikar, Bastian Koppelmann,
Palmer Dabbelt, qemu-devel@nongnu.org Developers,
Alistair Francis
On 8/5/19 8:43 AM, Bin Meng wrote:
> On Mon, Aug 5, 2019 at 2:14 PM Chih-Min Chao <chihmin.chao@sifive.com> wrote:
>> On Sat, Aug 3, 2019 at 8:27 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>>>
>>> Some of the SoC IP block sizes are wrong. Correct them according
>>> to the FE310 manual.
>>>
>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>>> ---
>>>
>>> hw/riscv/sifive_e.c | 6 +++---
>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
>>> index 2a499d8..9655847 100644
>>> --- a/hw/riscv/sifive_e.c
>>> +++ b/hw/riscv/sifive_e.c
>>> @@ -53,13 +53,13 @@ static const struct MemmapEntry {
>>> hwaddr base;
>>> hwaddr size;
>>> } sifive_e_memmap[] = {
>>> - [SIFIVE_E_DEBUG] = { 0x0, 0x100 },
>>> + [SIFIVE_E_DEBUG] = { 0x0, 0x1000 },
>>> [SIFIVE_E_MROM] = { 0x1000, 0x2000 },
>>> [SIFIVE_E_OTP] = { 0x20000, 0x2000 },
>>> [SIFIVE_E_CLINT] = { 0x2000000, 0x10000 },
>>> [SIFIVE_E_PLIC] = { 0xc000000, 0x4000000 },
>>> - [SIFIVE_E_AON] = { 0x10000000, 0x8000 },
>>> - [SIFIVE_E_PRCI] = { 0x10008000, 0x8000 },
>>> + [SIFIVE_E_AON] = { 0x10000000, 0x1000 },
>>> + [SIFIVE_E_PRCI] = { 0x10008000, 0x1000 },
>>> [SIFIVE_E_OTP_CTRL] = { 0x10010000, 0x1000 },
>>> [SIFIVE_E_GPIO0] = { 0x10012000, 0x1000 },
>>> [SIFIVE_E_UART0] = { 0x10013000, 0x1000 },
>>> --
>>> 2.7.4
>>>
>>
>> It seems the modification follows E310-G002(Hifive1 Rev B) spec and the origin is for E310-G000(Hifive1) spec.
>> There should be some way to specify different board version with different memory map or we have policy, always support the latest spec.
I agree with Chao, it would be cleaner to have two different boards
(machines).
Since the SoCs are very similar, you could add a 'revision' property and
use it to select the correct map.
>>
>
> Yes, I checked both specs. The older spec says these bigger sizes,
> however their register sizes fit well in the smaller range as well. So
> I think the modification works well for both.
This is OK for the PRCI, since sifive_prci_create() does not use
memmap[SIFIVE_E_PRCI].size.
However the AON case is borderline, since you shrink it from 32KiB to 4KiB.
BTW (not related to this patch) it is odd a function named
sifive_mmio_emulate() creates a RAM region with memory_region_init_ram()
and does not use the UnimplementedDevice (see make_unimp_dev() in
hw/arm/musca.c).
>
> Regards,
> Bin
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [Qemu-riscv] [PATCH] riscv: sifive_e: Correct various SoC IP block sizes
2019-08-06 21:06 ` Philippe Mathieu-Daudé
@ 2019-08-07 1:36 ` Bin Meng
2019-08-07 2:53 ` Bin Meng
1 sibling, 0 replies; 12+ messages in thread
From: Bin Meng @ 2019-08-07 1:36 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: open list:RISC-V, Sagar Karandikar, Bastian Koppelmann,
Palmer Dabbelt, qemu-devel@nongnu.org Developers, Chih-Min Chao,
Alistair Francis
On Wed, Aug 7, 2019 at 5:06 AM Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> On 8/5/19 8:43 AM, Bin Meng wrote:
> > On Mon, Aug 5, 2019 at 2:14 PM Chih-Min Chao <chihmin.chao@sifive.com> wrote:
> >> On Sat, Aug 3, 2019 at 8:27 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> >>>
> >>> Some of the SoC IP block sizes are wrong. Correct them according
> >>> to the FE310 manual.
> >>>
> >>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> >>> ---
> >>>
> >>> hw/riscv/sifive_e.c | 6 +++---
> >>> 1 file changed, 3 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
> >>> index 2a499d8..9655847 100644
> >>> --- a/hw/riscv/sifive_e.c
> >>> +++ b/hw/riscv/sifive_e.c
> >>> @@ -53,13 +53,13 @@ static const struct MemmapEntry {
> >>> hwaddr base;
> >>> hwaddr size;
> >>> } sifive_e_memmap[] = {
> >>> - [SIFIVE_E_DEBUG] = { 0x0, 0x100 },
> >>> + [SIFIVE_E_DEBUG] = { 0x0, 0x1000 },
> >>> [SIFIVE_E_MROM] = { 0x1000, 0x2000 },
> >>> [SIFIVE_E_OTP] = { 0x20000, 0x2000 },
> >>> [SIFIVE_E_CLINT] = { 0x2000000, 0x10000 },
> >>> [SIFIVE_E_PLIC] = { 0xc000000, 0x4000000 },
> >>> - [SIFIVE_E_AON] = { 0x10000000, 0x8000 },
> >>> - [SIFIVE_E_PRCI] = { 0x10008000, 0x8000 },
> >>> + [SIFIVE_E_AON] = { 0x10000000, 0x1000 },
> >>> + [SIFIVE_E_PRCI] = { 0x10008000, 0x1000 },
> >>> [SIFIVE_E_OTP_CTRL] = { 0x10010000, 0x1000 },
> >>> [SIFIVE_E_GPIO0] = { 0x10012000, 0x1000 },
> >>> [SIFIVE_E_UART0] = { 0x10013000, 0x1000 },
> >>> --
> >>> 2.7.4
> >>>
> >>
> >> It seems the modification follows E310-G002(Hifive1 Rev B) spec and the origin is for E310-G000(Hifive1) spec.
> >> There should be some way to specify different board version with different memory map or we have policy, always support the latest spec.
>
> I agree with Chao, it would be cleaner to have two different boards
> (machines).
> Since the SoCs are very similar, you could add a 'revision' property and
> use it to select the correct map.
>
> >>
> >
> > Yes, I checked both specs. The older spec says these bigger sizes,
> > however their register sizes fit well in the smaller range as well. So
> > I think the modification works well for both.
>
> This is OK for the PRCI, since sifive_prci_create() does not use
> memmap[SIFIVE_E_PRCI].size.
>
> However the AON case is borderline, since you shrink it from 32KiB to 4KiB.
>
> BTW (not related to this patch) it is odd a function named
> sifive_mmio_emulate() creates a RAM region with memory_region_init_ram()
> and does not use the UnimplementedDevice (see make_unimp_dev() in
> hw/arm/musca.c).
Yes, this sifive_mmio_emulate() issue has been pointed out by Alistair
when reviewing the following patch:
http://patchwork.ozlabs.org/patch/1142293/
Regards,
Bin
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [Qemu-riscv] [PATCH] riscv: sifive_e: Correct various SoC IP block sizes
2019-08-06 21:06 ` Philippe Mathieu-Daudé
2019-08-07 1:36 ` Bin Meng
@ 2019-08-07 2:53 ` Bin Meng
2019-08-14 9:34 ` Bin Meng
1 sibling, 1 reply; 12+ messages in thread
From: Bin Meng @ 2019-08-07 2:53 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: open list:RISC-V, Sagar Karandikar, Bastian Koppelmann,
Palmer Dabbelt, qemu-devel@nongnu.org Developers, Chih-Min Chao,
Alistair Francis
On Wed, Aug 7, 2019 at 5:06 AM Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> On 8/5/19 8:43 AM, Bin Meng wrote:
> > On Mon, Aug 5, 2019 at 2:14 PM Chih-Min Chao <chihmin.chao@sifive.com> wrote:
> >> On Sat, Aug 3, 2019 at 8:27 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> >>>
> >>> Some of the SoC IP block sizes are wrong. Correct them according
> >>> to the FE310 manual.
> >>>
> >>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> >>> ---
> >>>
> >>> hw/riscv/sifive_e.c | 6 +++---
> >>> 1 file changed, 3 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
> >>> index 2a499d8..9655847 100644
> >>> --- a/hw/riscv/sifive_e.c
> >>> +++ b/hw/riscv/sifive_e.c
> >>> @@ -53,13 +53,13 @@ static const struct MemmapEntry {
> >>> hwaddr base;
> >>> hwaddr size;
> >>> } sifive_e_memmap[] = {
> >>> - [SIFIVE_E_DEBUG] = { 0x0, 0x100 },
> >>> + [SIFIVE_E_DEBUG] = { 0x0, 0x1000 },
> >>> [SIFIVE_E_MROM] = { 0x1000, 0x2000 },
> >>> [SIFIVE_E_OTP] = { 0x20000, 0x2000 },
> >>> [SIFIVE_E_CLINT] = { 0x2000000, 0x10000 },
> >>> [SIFIVE_E_PLIC] = { 0xc000000, 0x4000000 },
> >>> - [SIFIVE_E_AON] = { 0x10000000, 0x8000 },
> >>> - [SIFIVE_E_PRCI] = { 0x10008000, 0x8000 },
> >>> + [SIFIVE_E_AON] = { 0x10000000, 0x1000 },
> >>> + [SIFIVE_E_PRCI] = { 0x10008000, 0x1000 },
> >>> [SIFIVE_E_OTP_CTRL] = { 0x10010000, 0x1000 },
> >>> [SIFIVE_E_GPIO0] = { 0x10012000, 0x1000 },
> >>> [SIFIVE_E_UART0] = { 0x10013000, 0x1000 },
> >>> --
> >>> 2.7.4
> >>>
> >>
> >> It seems the modification follows E310-G002(Hifive1 Rev B) spec and the origin is for E310-G000(Hifive1) spec.
> >> There should be some way to specify different board version with different memory map or we have policy, always support the latest spec.
>
> I agree with Chao, it would be cleaner to have two different boards
> (machines).
> Since the SoCs are very similar, you could add a 'revision' property and
> use it to select the correct map.
>
I am not sure if adding two different machines will bring us a lot of
benefits, since the only difference is the SoC revision with different
block sizes.
> >>
> >
> > Yes, I checked both specs. The older spec says these bigger sizes,
> > however their register sizes fit well in the smaller range as well. So
> > I think the modification works well for both.
>
> This is OK for the PRCI, since sifive_prci_create() does not use
> memmap[SIFIVE_E_PRCI].size.
>
> However the AON case is borderline, since you shrink it from 32KiB to 4KiB.
>
AON is not implemented anyway currently. And I checked the FE310 old
spec, its register block size is still within the 4KiB range, so
shrinking the size should be fine for both old and new SoC.
> BTW (not related to this patch) it is odd a function named
> sifive_mmio_emulate() creates a RAM region with memory_region_init_ram()
> and does not use the UnimplementedDevice (see make_unimp_dev() in
> hw/arm/musca.c).
>
Regards,
Bin
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [Qemu-riscv] [PATCH] riscv: sifive_e: Correct various SoC IP block sizes
2019-08-07 2:53 ` Bin Meng
@ 2019-08-14 9:34 ` Bin Meng
2019-09-04 3:41 ` Bin Meng
0 siblings, 1 reply; 12+ messages in thread
From: Bin Meng @ 2019-08-14 9:34 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: open list:RISC-V, Sagar Karandikar, Bastian Koppelmann,
Palmer Dabbelt, qemu-devel@nongnu.org Developers, Chih-Min Chao,
Alistair Francis
Hi Palmer,
On Wed, Aug 7, 2019 at 10:53 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Wed, Aug 7, 2019 at 5:06 AM Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> >
> > On 8/5/19 8:43 AM, Bin Meng wrote:
> > > On Mon, Aug 5, 2019 at 2:14 PM Chih-Min Chao <chihmin.chao@sifive.com> wrote:
> > >> On Sat, Aug 3, 2019 at 8:27 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> > >>>
> > >>> Some of the SoC IP block sizes are wrong. Correct them according
> > >>> to the FE310 manual.
> > >>>
> > >>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > >>> ---
> > >>>
> > >>> hw/riscv/sifive_e.c | 6 +++---
> > >>> 1 file changed, 3 insertions(+), 3 deletions(-)
> > >>>
> > >>> diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
> > >>> index 2a499d8..9655847 100644
> > >>> --- a/hw/riscv/sifive_e.c
> > >>> +++ b/hw/riscv/sifive_e.c
> > >>> @@ -53,13 +53,13 @@ static const struct MemmapEntry {
> > >>> hwaddr base;
> > >>> hwaddr size;
> > >>> } sifive_e_memmap[] = {
> > >>> - [SIFIVE_E_DEBUG] = { 0x0, 0x100 },
> > >>> + [SIFIVE_E_DEBUG] = { 0x0, 0x1000 },
> > >>> [SIFIVE_E_MROM] = { 0x1000, 0x2000 },
> > >>> [SIFIVE_E_OTP] = { 0x20000, 0x2000 },
> > >>> [SIFIVE_E_CLINT] = { 0x2000000, 0x10000 },
> > >>> [SIFIVE_E_PLIC] = { 0xc000000, 0x4000000 },
> > >>> - [SIFIVE_E_AON] = { 0x10000000, 0x8000 },
> > >>> - [SIFIVE_E_PRCI] = { 0x10008000, 0x8000 },
> > >>> + [SIFIVE_E_AON] = { 0x10000000, 0x1000 },
> > >>> + [SIFIVE_E_PRCI] = { 0x10008000, 0x1000 },
> > >>> [SIFIVE_E_OTP_CTRL] = { 0x10010000, 0x1000 },
> > >>> [SIFIVE_E_GPIO0] = { 0x10012000, 0x1000 },
> > >>> [SIFIVE_E_UART0] = { 0x10013000, 0x1000 },
> > >>> --
> > >>> 2.7.4
> > >>>
> > >>
> > >> It seems the modification follows E310-G002(Hifive1 Rev B) spec and the origin is for E310-G000(Hifive1) spec.
> > >> There should be some way to specify different board version with different memory map or we have policy, always support the latest spec.
> >
> > I agree with Chao, it would be cleaner to have two different boards
> > (machines).
> > Since the SoCs are very similar, you could add a 'revision' property and
> > use it to select the correct map.
> >
>
> I am not sure if adding two different machines will bring us a lot of
> benefits, since the only difference is the SoC revision with different
> block sizes.
>
> > >>
> > >
> > > Yes, I checked both specs. The older spec says these bigger sizes,
> > > however their register sizes fit well in the smaller range as well. So
> > > I think the modification works well for both.
> >
> > This is OK for the PRCI, since sifive_prci_create() does not use
> > memmap[SIFIVE_E_PRCI].size.
> >
> > However the AON case is borderline, since you shrink it from 32KiB to 4KiB.
> >
>
> AON is not implemented anyway currently. And I checked the FE310 old
> spec, its register block size is still within the 4KiB range, so
> shrinking the size should be fine for both old and new SoC.
>
> > BTW (not related to this patch) it is odd a function named
> > sifive_mmio_emulate() creates a RAM region with memory_region_init_ram()
> > and does not use the UnimplementedDevice (see make_unimp_dev() in
> > hw/arm/musca.c).
> >
What's your suggestion regarding this patch?
Regards,
Bin
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [Qemu-riscv] [PATCH] riscv: sifive_e: Correct various SoC IP block sizes
2019-08-14 9:34 ` Bin Meng
@ 2019-09-04 3:41 ` Bin Meng
2019-09-04 18:34 ` Palmer Dabbelt
0 siblings, 1 reply; 12+ messages in thread
From: Bin Meng @ 2019-09-04 3:41 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: open list:RISC-V, Sagar Karandikar, Bastian Koppelmann,
Palmer Dabbelt, qemu-devel@nongnu.org Developers, Chih-Min Chao,
Alistair Francis
Palmer,
On Wed, Aug 14, 2019 at 5:34 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Palmer,
>
> On Wed, Aug 7, 2019 at 10:53 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > On Wed, Aug 7, 2019 at 5:06 AM Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> > >
> > > On 8/5/19 8:43 AM, Bin Meng wrote:
> > > > On Mon, Aug 5, 2019 at 2:14 PM Chih-Min Chao <chihmin.chao@sifive.com> wrote:
> > > >> On Sat, Aug 3, 2019 at 8:27 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> > > >>>
> > > >>> Some of the SoC IP block sizes are wrong. Correct them according
> > > >>> to the FE310 manual.
> > > >>>
> > > >>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > > >>> ---
> > > >>>
> > > >>> hw/riscv/sifive_e.c | 6 +++---
> > > >>> 1 file changed, 3 insertions(+), 3 deletions(-)
> > > >>>
> > > >>> diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
> > > >>> index 2a499d8..9655847 100644
> > > >>> --- a/hw/riscv/sifive_e.c
> > > >>> +++ b/hw/riscv/sifive_e.c
> > > >>> @@ -53,13 +53,13 @@ static const struct MemmapEntry {
> > > >>> hwaddr base;
> > > >>> hwaddr size;
> > > >>> } sifive_e_memmap[] = {
> > > >>> - [SIFIVE_E_DEBUG] = { 0x0, 0x100 },
> > > >>> + [SIFIVE_E_DEBUG] = { 0x0, 0x1000 },
> > > >>> [SIFIVE_E_MROM] = { 0x1000, 0x2000 },
> > > >>> [SIFIVE_E_OTP] = { 0x20000, 0x2000 },
> > > >>> [SIFIVE_E_CLINT] = { 0x2000000, 0x10000 },
> > > >>> [SIFIVE_E_PLIC] = { 0xc000000, 0x4000000 },
> > > >>> - [SIFIVE_E_AON] = { 0x10000000, 0x8000 },
> > > >>> - [SIFIVE_E_PRCI] = { 0x10008000, 0x8000 },
> > > >>> + [SIFIVE_E_AON] = { 0x10000000, 0x1000 },
> > > >>> + [SIFIVE_E_PRCI] = { 0x10008000, 0x1000 },
> > > >>> [SIFIVE_E_OTP_CTRL] = { 0x10010000, 0x1000 },
> > > >>> [SIFIVE_E_GPIO0] = { 0x10012000, 0x1000 },
> > > >>> [SIFIVE_E_UART0] = { 0x10013000, 0x1000 },
> > > >>> --
> > > >>> 2.7.4
> > > >>>
> > > >>
> > > >> It seems the modification follows E310-G002(Hifive1 Rev B) spec and the origin is for E310-G000(Hifive1) spec.
> > > >> There should be some way to specify different board version with different memory map or we have policy, always support the latest spec.
> > >
> > > I agree with Chao, it would be cleaner to have two different boards
> > > (machines).
> > > Since the SoCs are very similar, you could add a 'revision' property and
> > > use it to select the correct map.
> > >
> >
> > I am not sure if adding two different machines will bring us a lot of
> > benefits, since the only difference is the SoC revision with different
> > block sizes.
> >
> > > >>
> > > >
> > > > Yes, I checked both specs. The older spec says these bigger sizes,
> > > > however their register sizes fit well in the smaller range as well. So
> > > > I think the modification works well for both.
> > >
> > > This is OK for the PRCI, since sifive_prci_create() does not use
> > > memmap[SIFIVE_E_PRCI].size.
> > >
> > > However the AON case is borderline, since you shrink it from 32KiB to 4KiB.
> > >
> >
> > AON is not implemented anyway currently. And I checked the FE310 old
> > spec, its register block size is still within the 4KiB range, so
> > shrinking the size should be fine for both old and new SoC.
> >
> > > BTW (not related to this patch) it is odd a function named
> > > sifive_mmio_emulate() creates a RAM region with memory_region_init_ram()
> > > and does not use the UnimplementedDevice (see make_unimp_dev() in
> > > hw/arm/musca.c).
> > >
>
> What's your suggestion regarding this patch?
Ping?
Regards,
Bin
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [Qemu-riscv] [PATCH] riscv: sifive_e: Correct various SoC IP block sizes
2019-09-04 3:41 ` Bin Meng
@ 2019-09-04 18:34 ` Palmer Dabbelt
2020-06-23 6:35 ` Bin Meng
0 siblings, 1 reply; 12+ messages in thread
From: Palmer Dabbelt @ 2019-09-04 18:34 UTC (permalink / raw)
To: bmeng.cn, Peter Maydell
Cc: qemu-riscv, sagark, Bastian Koppelmann, qemu-devel,
Chih-Min Chao, Alistair Francis, philmd
On Tue, 03 Sep 2019 20:41:52 PDT (-0700), bmeng.cn@gmail.com wrote:
> Palmer,
>
> On Wed, Aug 14, 2019 at 5:34 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>>
>> Hi Palmer,
>>
>> On Wed, Aug 7, 2019 at 10:53 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>> >
>> > On Wed, Aug 7, 2019 at 5:06 AM Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>> > >
>> > > On 8/5/19 8:43 AM, Bin Meng wrote:
>> > > > On Mon, Aug 5, 2019 at 2:14 PM Chih-Min Chao <chihmin.chao@sifive.com> wrote:
>> > > >> On Sat, Aug 3, 2019 at 8:27 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>> > > >>>
>> > > >>> Some of the SoC IP block sizes are wrong. Correct them according
>> > > >>> to the FE310 manual.
>> > > >>>
>> > > >>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>> > > >>> ---
>> > > >>>
>> > > >>> hw/riscv/sifive_e.c | 6 +++---
>> > > >>> 1 file changed, 3 insertions(+), 3 deletions(-)
>> > > >>>
>> > > >>> diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
>> > > >>> index 2a499d8..9655847 100644
>> > > >>> --- a/hw/riscv/sifive_e.c
>> > > >>> +++ b/hw/riscv/sifive_e.c
>> > > >>> @@ -53,13 +53,13 @@ static const struct MemmapEntry {
>> > > >>> hwaddr base;
>> > > >>> hwaddr size;
>> > > >>> } sifive_e_memmap[] = {
>> > > >>> - [SIFIVE_E_DEBUG] = { 0x0, 0x100 },
>> > > >>> + [SIFIVE_E_DEBUG] = { 0x0, 0x1000 },
>> > > >>> [SIFIVE_E_MROM] = { 0x1000, 0x2000 },
>> > > >>> [SIFIVE_E_OTP] = { 0x20000, 0x2000 },
>> > > >>> [SIFIVE_E_CLINT] = { 0x2000000, 0x10000 },
>> > > >>> [SIFIVE_E_PLIC] = { 0xc000000, 0x4000000 },
>> > > >>> - [SIFIVE_E_AON] = { 0x10000000, 0x8000 },
>> > > >>> - [SIFIVE_E_PRCI] = { 0x10008000, 0x8000 },
>> > > >>> + [SIFIVE_E_AON] = { 0x10000000, 0x1000 },
>> > > >>> + [SIFIVE_E_PRCI] = { 0x10008000, 0x1000 },
>> > > >>> [SIFIVE_E_OTP_CTRL] = { 0x10010000, 0x1000 },
>> > > >>> [SIFIVE_E_GPIO0] = { 0x10012000, 0x1000 },
>> > > >>> [SIFIVE_E_UART0] = { 0x10013000, 0x1000 },
>> > > >>> --
>> > > >>> 2.7.4
>> > > >>>
>> > > >>
>> > > >> It seems the modification follows E310-G002(Hifive1 Rev B) spec and the origin is for E310-G000(Hifive1) spec.
>> > > >> There should be some way to specify different board version with different memory map or we have policy, always support the latest spec.
>> > >
>> > > I agree with Chao, it would be cleaner to have two different boards
>> > > (machines).
>> > > Since the SoCs are very similar, you could add a 'revision' property and
>> > > use it to select the correct map.
>> > >
>> >
>> > I am not sure if adding two different machines will bring us a lot of
>> > benefits, since the only difference is the SoC revision with different
>> > block sizes.
>> >
>> > > >>
>> > > >
>> > > > Yes, I checked both specs. The older spec says these bigger sizes,
>> > > > however their register sizes fit well in the smaller range as well. So
>> > > > I think the modification works well for both.
>> > >
>> > > This is OK for the PRCI, since sifive_prci_create() does not use
>> > > memmap[SIFIVE_E_PRCI].size.
>> > >
>> > > However the AON case is borderline, since you shrink it from 32KiB to 4KiB.
>> > >
>> >
>> > AON is not implemented anyway currently. And I checked the FE310 old
>> > spec, its register block size is still within the 4KiB range, so
>> > shrinking the size should be fine for both old and new SoC.
>> >
>> > > BTW (not related to this patch) it is odd a function named
>> > > sifive_mmio_emulate() creates a RAM region with memory_region_init_ram()
>> > > and does not use the UnimplementedDevice (see make_unimp_dev() in
>> > > hw/arm/musca.c).
>> > >
>>
>> What's your suggestion regarding this patch?
>
> Ping?
Sorry, I missed this the first time around. In retrospect, it looks like we
ended up with the wrong naming scheme for boards: sifive_e is very ambiguous,
as there are many boards that look like this. We'd originally chosen a more
explicit scheme (something like "sifive-fe310-g000"), but that was NAK'd as
resulting in too many machine types.
Peter: would you be OK deprecating "sifive_e" and adding "sifive-fe310-g000"
and "sifive-fe310-g002" targets? We'll end up with a lot of machines this way,
but I don't see another way to closely match what's out there. In embedded
land there isn't really any runtime portability, so if the memory maps don't
match exactly then it's not a useful target for users.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [Qemu-riscv] [PATCH] riscv: sifive_e: Correct various SoC IP block sizes
2019-09-04 18:34 ` Palmer Dabbelt
@ 2020-06-23 6:35 ` Bin Meng
2020-06-23 16:07 ` Alistair Francis
0 siblings, 1 reply; 12+ messages in thread
From: Bin Meng @ 2020-06-23 6:35 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: Peter Maydell, open list:RISC-V, Sagar Karandikar,
Bastian Koppelmann, qemu-devel@nongnu.org Developers,
Chih-Min Chao, Alistair Francis, Philippe Mathieu-Daudé
Hi,
On Thu, Sep 5, 2019 at 2:34 AM Palmer Dabbelt <palmer@sifive.com> wrote:
>
> On Tue, 03 Sep 2019 20:41:52 PDT (-0700), bmeng.cn@gmail.com wrote:
> > Palmer,
> >
> > On Wed, Aug 14, 2019 at 5:34 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >>
> >> Hi Palmer,
> >>
> >> On Wed, Aug 7, 2019 at 10:53 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> >> >
> >> > On Wed, Aug 7, 2019 at 5:06 AM Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> >> > >
> >> > > On 8/5/19 8:43 AM, Bin Meng wrote:
> >> > > > On Mon, Aug 5, 2019 at 2:14 PM Chih-Min Chao <chihmin.chao@sifive.com> wrote:
> >> > > >> On Sat, Aug 3, 2019 at 8:27 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> >> > > >>>
> >> > > >>> Some of the SoC IP block sizes are wrong. Correct them according
> >> > > >>> to the FE310 manual.
> >> > > >>>
> >> > > >>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> >> > > >>> ---
> >> > > >>>
> >> > > >>> hw/riscv/sifive_e.c | 6 +++---
> >> > > >>> 1 file changed, 3 insertions(+), 3 deletions(-)
> >> > > >>>
> >> > > >>> diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
> >> > > >>> index 2a499d8..9655847 100644
> >> > > >>> --- a/hw/riscv/sifive_e.c
> >> > > >>> +++ b/hw/riscv/sifive_e.c
> >> > > >>> @@ -53,13 +53,13 @@ static const struct MemmapEntry {
> >> > > >>> hwaddr base;
> >> > > >>> hwaddr size;
> >> > > >>> } sifive_e_memmap[] = {
> >> > > >>> - [SIFIVE_E_DEBUG] = { 0x0, 0x100 },
> >> > > >>> + [SIFIVE_E_DEBUG] = { 0x0, 0x1000 },
> >> > > >>> [SIFIVE_E_MROM] = { 0x1000, 0x2000 },
> >> > > >>> [SIFIVE_E_OTP] = { 0x20000, 0x2000 },
> >> > > >>> [SIFIVE_E_CLINT] = { 0x2000000, 0x10000 },
> >> > > >>> [SIFIVE_E_PLIC] = { 0xc000000, 0x4000000 },
> >> > > >>> - [SIFIVE_E_AON] = { 0x10000000, 0x8000 },
> >> > > >>> - [SIFIVE_E_PRCI] = { 0x10008000, 0x8000 },
> >> > > >>> + [SIFIVE_E_AON] = { 0x10000000, 0x1000 },
> >> > > >>> + [SIFIVE_E_PRCI] = { 0x10008000, 0x1000 },
> >> > > >>> [SIFIVE_E_OTP_CTRL] = { 0x10010000, 0x1000 },
> >> > > >>> [SIFIVE_E_GPIO0] = { 0x10012000, 0x1000 },
> >> > > >>> [SIFIVE_E_UART0] = { 0x10013000, 0x1000 },
> >> > > >>> --
> >> > > >>> 2.7.4
> >> > > >>>
> >> > > >>
> >> > > >> It seems the modification follows E310-G002(Hifive1 Rev B) spec and the origin is for E310-G000(Hifive1) spec.
> >> > > >> There should be some way to specify different board version with different memory map or we have policy, always support the latest spec.
> >> > >
> >> > > I agree with Chao, it would be cleaner to have two different boards
> >> > > (machines).
> >> > > Since the SoCs are very similar, you could add a 'revision' property and
> >> > > use it to select the correct map.
> >> > >
> >> >
> >> > I am not sure if adding two different machines will bring us a lot of
> >> > benefits, since the only difference is the SoC revision with different
> >> > block sizes.
> >> >
> >> > > >>
> >> > > >
> >> > > > Yes, I checked both specs. The older spec says these bigger sizes,
> >> > > > however their register sizes fit well in the smaller range as well. So
> >> > > > I think the modification works well for both.
> >> > >
> >> > > This is OK for the PRCI, since sifive_prci_create() does not use
> >> > > memmap[SIFIVE_E_PRCI].size.
> >> > >
> >> > > However the AON case is borderline, since you shrink it from 32KiB to 4KiB.
> >> > >
> >> >
> >> > AON is not implemented anyway currently. And I checked the FE310 old
> >> > spec, its register block size is still within the 4KiB range, so
> >> > shrinking the size should be fine for both old and new SoC.
> >> >
> >> > > BTW (not related to this patch) it is odd a function named
> >> > > sifive_mmio_emulate() creates a RAM region with memory_region_init_ram()
> >> > > and does not use the UnimplementedDevice (see make_unimp_dev() in
> >> > > hw/arm/musca.c).
> >> > >
> >>
> >> What's your suggestion regarding this patch?
> >
> > Ping?
>
> Sorry, I missed this the first time around. In retrospect, it looks like we
> ended up with the wrong naming scheme for boards: sifive_e is very ambiguous,
> as there are many boards that look like this. We'd originally chosen a more
> explicit scheme (something like "sifive-fe310-g000"), but that was NAK'd as
> resulting in too many machine types.
>
> Peter: would you be OK deprecating "sifive_e" and adding "sifive-fe310-g000"
> and "sifive-fe310-g002" targets? We'll end up with a lot of machines this way,
> but I don't see another way to closely match what's out there. In embedded
> land there isn't really any runtime portability, so if the memory maps don't
> match exactly then it's not a useful target for users.
Just want to restart the discussion for this patch. Now that we have
"revB" support for sifive_e machine, I guess we can do something?
But renaming the sifive_e machine to something like sifive-fe31-g000
is another topic .. Thoughts?
Regards,
Bin
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [Qemu-riscv] [PATCH] riscv: sifive_e: Correct various SoC IP block sizes
2020-06-23 6:35 ` Bin Meng
@ 2020-06-23 16:07 ` Alistair Francis
0 siblings, 0 replies; 12+ messages in thread
From: Alistair Francis @ 2020-06-23 16:07 UTC (permalink / raw)
To: Bin Meng
Cc: Peter Maydell, open list:RISC-V, Sagar Karandikar,
Bastian Koppelmann, Palmer Dabbelt,
qemu-devel@nongnu.org Developers, Chih-Min Chao,
Alistair Francis, Philippe Mathieu-Daudé
On Mon, Jun 22, 2020 at 11:36 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi,
>
> On Thu, Sep 5, 2019 at 2:34 AM Palmer Dabbelt <palmer@sifive.com> wrote:
> >
> > On Tue, 03 Sep 2019 20:41:52 PDT (-0700), bmeng.cn@gmail.com wrote:
> > > Palmer,
> > >
> > > On Wed, Aug 14, 2019 at 5:34 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> > >>
> > >> Hi Palmer,
> > >>
> > >> On Wed, Aug 7, 2019 at 10:53 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> > >> >
> > >> > On Wed, Aug 7, 2019 at 5:06 AM Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> > >> > >
> > >> > > On 8/5/19 8:43 AM, Bin Meng wrote:
> > >> > > > On Mon, Aug 5, 2019 at 2:14 PM Chih-Min Chao <chihmin.chao@sifive.com> wrote:
> > >> > > >> On Sat, Aug 3, 2019 at 8:27 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> > >> > > >>>
> > >> > > >>> Some of the SoC IP block sizes are wrong. Correct them according
> > >> > > >>> to the FE310 manual.
> > >> > > >>>
> > >> > > >>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > >> > > >>> ---
> > >> > > >>>
> > >> > > >>> hw/riscv/sifive_e.c | 6 +++---
> > >> > > >>> 1 file changed, 3 insertions(+), 3 deletions(-)
> > >> > > >>>
> > >> > > >>> diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
> > >> > > >>> index 2a499d8..9655847 100644
> > >> > > >>> --- a/hw/riscv/sifive_e.c
> > >> > > >>> +++ b/hw/riscv/sifive_e.c
> > >> > > >>> @@ -53,13 +53,13 @@ static const struct MemmapEntry {
> > >> > > >>> hwaddr base;
> > >> > > >>> hwaddr size;
> > >> > > >>> } sifive_e_memmap[] = {
> > >> > > >>> - [SIFIVE_E_DEBUG] = { 0x0, 0x100 },
> > >> > > >>> + [SIFIVE_E_DEBUG] = { 0x0, 0x1000 },
> > >> > > >>> [SIFIVE_E_MROM] = { 0x1000, 0x2000 },
> > >> > > >>> [SIFIVE_E_OTP] = { 0x20000, 0x2000 },
> > >> > > >>> [SIFIVE_E_CLINT] = { 0x2000000, 0x10000 },
> > >> > > >>> [SIFIVE_E_PLIC] = { 0xc000000, 0x4000000 },
> > >> > > >>> - [SIFIVE_E_AON] = { 0x10000000, 0x8000 },
> > >> > > >>> - [SIFIVE_E_PRCI] = { 0x10008000, 0x8000 },
> > >> > > >>> + [SIFIVE_E_AON] = { 0x10000000, 0x1000 },
> > >> > > >>> + [SIFIVE_E_PRCI] = { 0x10008000, 0x1000 },
> > >> > > >>> [SIFIVE_E_OTP_CTRL] = { 0x10010000, 0x1000 },
> > >> > > >>> [SIFIVE_E_GPIO0] = { 0x10012000, 0x1000 },
> > >> > > >>> [SIFIVE_E_UART0] = { 0x10013000, 0x1000 },
> > >> > > >>> --
> > >> > > >>> 2.7.4
> > >> > > >>>
> > >> > > >>
> > >> > > >> It seems the modification follows E310-G002(Hifive1 Rev B) spec and the origin is for E310-G000(Hifive1) spec.
> > >> > > >> There should be some way to specify different board version with different memory map or we have policy, always support the latest spec.
> > >> > >
> > >> > > I agree with Chao, it would be cleaner to have two different boards
> > >> > > (machines).
> > >> > > Since the SoCs are very similar, you could add a 'revision' property and
> > >> > > use it to select the correct map.
> > >> > >
> > >> >
> > >> > I am not sure if adding two different machines will bring us a lot of
> > >> > benefits, since the only difference is the SoC revision with different
> > >> > block sizes.
> > >> >
> > >> > > >>
> > >> > > >
> > >> > > > Yes, I checked both specs. The older spec says these bigger sizes,
> > >> > > > however their register sizes fit well in the smaller range as well. So
> > >> > > > I think the modification works well for both.
> > >> > >
> > >> > > This is OK for the PRCI, since sifive_prci_create() does not use
> > >> > > memmap[SIFIVE_E_PRCI].size.
> > >> > >
> > >> > > However the AON case is borderline, since you shrink it from 32KiB to 4KiB.
> > >> > >
> > >> >
> > >> > AON is not implemented anyway currently. And I checked the FE310 old
> > >> > spec, its register block size is still within the 4KiB range, so
> > >> > shrinking the size should be fine for both old and new SoC.
> > >> >
> > >> > > BTW (not related to this patch) it is odd a function named
> > >> > > sifive_mmio_emulate() creates a RAM region with memory_region_init_ram()
> > >> > > and does not use the UnimplementedDevice (see make_unimp_dev() in
> > >> > > hw/arm/musca.c).
> > >> > >
> > >>
> > >> What's your suggestion regarding this patch?
> > >
> > > Ping?
> >
> > Sorry, I missed this the first time around. In retrospect, it looks like we
> > ended up with the wrong naming scheme for boards: sifive_e is very ambiguous,
> > as there are many boards that look like this. We'd originally chosen a more
> > explicit scheme (something like "sifive-fe310-g000"), but that was NAK'd as
> > resulting in too many machine types.
> >
> > Peter: would you be OK deprecating "sifive_e" and adding "sifive-fe310-g000"
> > and "sifive-fe310-g002" targets? We'll end up with a lot of machines this way,
> > but I don't see another way to closely match what's out there. In embedded
> > land there isn't really any runtime portability, so if the memory maps don't
> > match exactly then it's not a useful target for users.
>
> Just want to restart the discussion for this patch. Now that we have
> "revB" support for sifive_e machine, I guess we can do something?
>
> But renaming the sifive_e machine to something like sifive-fe31-g000
> is another topic .. Thoughts?
I would prefer not to have "sifive-fe310-g000" and "sifive-fe310-g002"
boards as that seems like it might lead to way too many boards in the
future.
In saying that board properties aren't that much better if there are
lots of boards as well.
One option would be something like what the ARM virt board does. Where
"sifive_e" is always the latest and you can specify different
versions. We would then have some deprecation scheme to remove older
boards.
I think for now properties seem to work, we can have revB=true to
change anything required to match revB and the default is revA (or
whatever they call it).
If a revC comes out it should be easy to handle that via a property.
In the future we can re-evaluate what to do if that gets too hard to
maintain. Eventually we probably want the default to be the revB, but
that will break users so let's try to avoid that for now.
It's also possible that a revC won't break compatibility. For example
if revC just adds a new device, we can just expose that for all revs.
Alistair
>
> Regards,
> Bin
>
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-06-23 16:19 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-03 0:27 [Qemu-devel] [PATCH] riscv: sifive_e: Correct various SoC IP block sizes Bin Meng
2019-08-05 6:14 ` [Qemu-devel] [Qemu-riscv] " Chih-Min Chao
2019-08-05 6:43 ` Bin Meng
2019-08-06 21:06 ` Philippe Mathieu-Daudé
2019-08-07 1:36 ` Bin Meng
2019-08-07 2:53 ` Bin Meng
2019-08-14 9:34 ` Bin Meng
2019-09-04 3:41 ` Bin Meng
2019-09-04 18:34 ` Palmer Dabbelt
2020-06-23 6:35 ` Bin Meng
2020-06-23 16:07 ` Alistair Francis
2019-08-05 17:51 ` [Qemu-devel] " Alistair Francis
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).