Re: [PATCH v1 4/6] riscv/sifive_u: Add the start-in-flash property

[Alistair Francis]

On Thu, Sep 19, 2019 at 10:15 PM Bin Meng <address@hidden> wrote:
>
> On Fri, Sep 20, 2019 at 6:32 AM Alistair Francis
> <address@hidden> wrote:
> >
> > Add a property that when set to true QEMU will jump from the ROM code to
> > the start of flash memory instead of DRAM which is the default
> > behaviour.
> >
> > Signed-off-by: Alistair Francis <address@hidden>
> > ---
> >  hw/riscv/sifive_u.c         | 27 +++++++++++++++++++++++++++
> >  include/hw/riscv/sifive_u.h |  2 ++
> >  2 files changed, 29 insertions(+)
> >
> > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> > index c3949fb316..b7cd3631cd 100644
> > --- a/hw/riscv/sifive_u.c
> > +++ b/hw/riscv/sifive_u.c
> > @@ -373,6 +373,10 @@ static void riscv_sifive_u_init(MachineState *machine)
> >                                         /* dtb: */
> >      };
> >
> > +    if (s->start_in_flash) {
> > +        reset_vec[6] = memmap[SIFIVE_U_FLASH0].base; /* start: .dword 
> > FLASH0_BASE */
> > +    }
> > +
> >      /* copy in the reset vector in little_endian byte order */
> >      for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> >          reset_vec[i] = cpu_to_le32(reset_vec[i]);
> > @@ -544,8 +548,31 @@ static void riscv_sifive_u_soc_realize(DeviceState 
> > *dev, Error **errp)
> >          memmap[SIFIVE_U_GEM_MGMT].base, memmap[SIFIVE_U_GEM_MGMT].size);
> >  }
> >
> > +static bool virt_get_start_in_flash(Object *obj, Error **errp)
> > +{
> > +    SiFiveUState *s = RISCV_U_MACHINE(obj);
> > +
> > +    return s->start_in_flash;
> > +}
> > +
> > +static void virt_set_start_in_flash(Object *obj, bool value, Error **errp)
> > +{
> > +    SiFiveUState *s = RISCV_U_MACHINE(obj);
> > +
> > +    s->start_in_flash = value;
> > +}
> > +
> >  static void riscv_sifive_u_machine_instance_init(Object *obj)
> >  {
> > +    SiFiveUState *s = RISCV_U_MACHINE(obj);
> > +
> > +    s->start_in_flash = false;
> > +    object_property_add_bool(obj, "start-in-flash", 
> > virt_get_start_in_flash,
> > +                             virt_set_start_in_flash, NULL);
> > +    object_property_set_description(obj, "start-in-flash",
> > +                                    "Set on to tell QEMU's ROM to jump to 
> > " \
> > +                                    "flash. Otherwise QEMU will jump to 
> > DRAM",
> > +                                    NULL);
> >
> >  }
> >
> > diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
> > index a921079fbe..2656b43c58 100644
> > --- a/include/hw/riscv/sifive_u.h
> > +++ b/include/hw/riscv/sifive_u.h
> > @@ -57,6 +57,8 @@ typedef struct SiFiveUState {
> >
> >      void *fdt;
> >      int fdt_size;
> > +
> > +    bool start_in_flash;
> >  } SiFiveUState;
> >
> >  enum {
>
> This patch chose a different way from the one used in patch "[v1,6/6]
> riscv/virt: Jump to pflash if specified":
>
> - this patch uses reset_vec[6] while patch [6/6] defines a variable start_addr
> - this patch adds a "start-in-flash" property to the machine, while
> patch [6/6] tests against drive IF_PFLASH

Yes, we do it differently for the sifive_u board as the sifive_u board
doesn't use pflash so there is no way to know if the user has loaded
anything into the SPI memory.

>
> We should be consistent and I would prefer to use the patch [6/6] way.
> On Unleashed an SPI flash is mounted so we cannot add a PFlash to
> sifive_u machine like what was done on virt machine, so we should test
> IF_MTD instead. Thoughts?

How would we test that?

Right now I am loading the binary in SPI with the -device loader
option. The machine can't really know what is/isn't loaded there.

It's not ideal, but I don't see a nicer way.

Alistair

>
> Regards,
> Bin