Re: [PATCH v1 3/3] hw/riscv: opentitan: Add the flash alias

From: Alistair Francis <alistair23@gmail.com>
To: Bin Meng <bmeng.cn@gmail.com>
Cc: "open list:RISC-V" <qemu-riscv@nongnu.org>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Alistair Francis <alistair.francis@wdc.com>,
	"qemu-devel@nongnu.org Developers" <qemu-devel@nongnu.org>
Subject: Re: [PATCH v1 3/3] hw/riscv: opentitan: Add the flash alias
Date: Tue, 6 Jul 2021 14:49:32 +1000	[thread overview]
Message-ID: <CAKmqyKO4cq-zNAx4XWHFfZ0kDkjuYLur2eh9xNH1pdV8J3Qwpg@mail.gmail.com> (raw)
In-Reply-To: <CAEUhbmXTs5udNh2MQdOOXJ0GfnziRfGDJrD43QewVc+aceZrbw@mail.gmail.com>

On Mon, Jul 5, 2021 at 4:16 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Fri, Jul 2, 2021 at 1:20 PM Alistair Francis
> <alistair.francis@wdc.com> wrote:
>
> Could you add some commit message to explain this alias?

Yep, I'll add something.

>
> >
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> >  include/hw/riscv/opentitan.h | 2 ++
> >  hw/riscv/opentitan.c         | 6 ++++++
> >  2 files changed, 8 insertions(+)
> >
> > diff --git a/include/hw/riscv/opentitan.h b/include/hw/riscv/opentitan.h
> > index a488f5e8ec..9f93bebdac 100644
> > --- a/include/hw/riscv/opentitan.h
> > +++ b/include/hw/riscv/opentitan.h
> > @@ -40,6 +40,7 @@ struct LowRISCIbexSoCState {
> >
> >      MemoryRegion flash_mem;
> >      MemoryRegion rom;
> > +    MemoryRegion flash_alias;
> >  };
> >
> >  typedef struct OpenTitanState {
> > @@ -54,6 +55,7 @@ enum {
> >      IBEX_DEV_ROM,
> >      IBEX_DEV_RAM,
> >      IBEX_DEV_FLASH,
> > +    IBEX_DEV_FLASH_VIRTUAL,
>
> Is this virtual address? But it is still physical?

It's a physical address (OpenTitan has no MMU so all addresses are physical).

There is an alias region to access the flash region, so the region can
be accessed either by it's "real" address or this "virtual" address
range. It's similar to some other MCUs, like the STM range.

The virtual is just the name that they call it.

Alistair

>
> >      IBEX_DEV_UART,
> >      IBEX_DEV_GPIO,
> >      IBEX_DEV_SPI,
> > diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c
> > index 933c211b11..36a41c8b5b 100644
> > --- a/hw/riscv/opentitan.c
> > +++ b/hw/riscv/opentitan.c
> > @@ -59,6 +59,7 @@ static const MemMapEntry ibex_memmap[] = {
> >      [IBEX_DEV_NMI_GEN] =        {  0x411c0000,  0x1000  },
> >      [IBEX_DEV_OTBN] =           {  0x411d0000,  0x10000 },
> >      [IBEX_DEV_PERI] =           {  0x411f0000,  0x10000 },
> > +    [IBEX_DEV_FLASH_VIRTUAL] =  {  0x80000000,  0x80000 },
> >  };
> >
> >  static void opentitan_board_init(MachineState *machine)
> > @@ -134,8 +135,13 @@ static void lowrisc_ibex_soc_realize(DeviceState *dev_soc, Error **errp)
> >      /* Flash memory */
> >      memory_region_init_rom(&s->flash_mem, OBJECT(dev_soc), "riscv.lowrisc.ibex.flash",
> >                             memmap[IBEX_DEV_FLASH].size, &error_fatal);
> > +    memory_region_init_alias(&s->flash_alias, OBJECT(dev_soc),
> > +                             "riscv.lowrisc.ibex.flash_virtual", &s->flash_mem, 0,
> > +                             memmap[IBEX_DEV_FLASH_VIRTUAL].size);
> >      memory_region_add_subregion(sys_mem, memmap[IBEX_DEV_FLASH].base,
> >                                  &s->flash_mem);
> > +    memory_region_add_subregion(sys_mem, memmap[IBEX_DEV_FLASH_VIRTUAL].base,
> > +                                &s->flash_alias);
> >
> >      /* PLIC */
> >      if (!sysbus_realize(SYS_BUS_DEVICE(&s->plic), errp)) {
> > --
>
> Regards,
> Bin