QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [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; 10+ 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	[flat|nested] 10+ 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; 10+ 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] 10+ 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; 10+ 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] 10+ 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; 10+ 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] 10+ 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; 10+ 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] 10+ 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; 10+ 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] 10+ 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; 10+ 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] 10+ 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; 10+ 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] 10+ 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; 10+ 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] 10+ 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
  0 siblings, 0 replies; 10+ 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] 10+ messages in thread

end of thread, back to index

Thread overview: 10+ 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
2019-08-05 17:51 ` [Qemu-devel] " Alistair Francis

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org qemu-devel@archiver.kernel.org
	public-inbox-index qemu-devel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox