| Message ID | 1564792052-6469-1-git-send-email-bmeng.cn@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | riscv: sifive_e: Correct various SoC IP block sizes | expand |
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
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
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 > >
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 >
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
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
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
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
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.
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
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 >
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 },
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(-)