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
>