[Qemu-devel] [PATCH] riscv: sifive_e: Correct various SoC IP block sizes

[Qemu-devel] [PATCH] riscv: sifive_e: Correct various SoC IP block sizes

[Bin Meng]

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

Re: [Qemu-devel] [Qemu-riscv] [PATCH] riscv: sifive_e: Correct various SoC IP block sizes

[Chih-Min Chao]

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

Re: [Qemu-devel] [Qemu-riscv] [PATCH] riscv: sifive_e: Correct various SoC IP block sizes

[Bin Meng]

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

Re: [Qemu-devel] [PATCH] riscv: sifive_e: Correct various SoC IP block sizes

[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
>
>

Re: [Qemu-devel] [Qemu-riscv] [PATCH] riscv: sifive_e: Correct various SoC IP block sizes

[Philippe Mathieu-Daudé]

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
> 

Re: [Qemu-devel] [Qemu-riscv] [PATCH] riscv: sifive_e: Correct various SoC IP block sizes

[Bin Meng]

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

Re: [Qemu-devel] [Qemu-riscv] [PATCH] riscv: sifive_e: Correct various SoC IP block sizes

[Bin Meng]

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

Re: [Qemu-devel] [Qemu-riscv] [PATCH] riscv: sifive_e: Correct various SoC IP block sizes

[Bin Meng]

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

Re: [Qemu-devel] [Qemu-riscv] [PATCH] riscv: sifive_e: Correct various SoC IP block sizes

[Bin Meng]

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

Re: [Qemu-devel] [Qemu-riscv] [PATCH] riscv: sifive_e: Correct various SoC IP block sizes

[Palmer Dabbelt]

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.

Re: [Qemu-devel] [Qemu-riscv] [PATCH] riscv: sifive_e: Correct various SoC IP block sizes

[Bin Meng]

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

Re: [Qemu-devel] [Qemu-riscv] [PATCH] riscv: sifive_e: Correct various SoC IP block sizes

[Alistair Francis]

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
>