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

[Alistair Francis]

On Sat, Sep 21, 2019 at 7:19 PM Bin Meng <address@hidden> wrote:
>
> On Sat, Sep 21, 2019 at 6:12 AM Alistair Francis <address@hidden> wrote:
> >
> > 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.
> >
>
> OK.
>
> > >
> > > 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.
>
> I think we need write a SiFive SPI model to support this in a clean
> way. Ideally we should simulate the hardware boot workflow as
> documented in the FU540 manual chapter 6 "Boot Process".

I really didn't want to do this. For me it's low priority and there
are enough other things to work on rather then adding SiFive device
models. Maybe someone who works at SiFive would be able to do this?

My hope with this series is that we could unblock firmware developers
(oreboot and coreboot) while the SPI model is written.

Alistair

>
> Regards,
> Bin