Re: [PATCH v1 2/3] riscv/sifive_u: Add a serial property to the sifive_u SoC

[Alistair Francis]

On Wed, Mar 4, 2020 at 6:47 AM Bin Meng <address@hidden> wrote:
>
> Hi Alistair,
>
> On Wed, Mar 4, 2020 at 9:37 AM Alistair Francis
> <address@hidden> wrote:
> >
> > At present the board serial number is hard-coded to 1, and passed
> > to OTP model during initialization. Firmware (FSBL, U-Boot) uses
> > the serial number to generate a unique MAC address for the on-chip
> > ethernet controller. When multiple QEMU 'sifive_u' instances are
> > created and connected to the same subnet, they all have the same
> > MAC address hence it creates a unusable network.
> >
> > A new "serial" property is introduced to the sifive_u SoC to specify
> > the board serial number. When not given, the default serial number
> > 1 is used.
> >
> > Suggested-by: Bin Meng <address@hidden>
> > Signed-off-by: Alistair Francis <address@hidden>
> > ---
> >  hw/riscv/sifive_u.c         | 8 +++++++-
> >  include/hw/riscv/sifive_u.h | 2 ++
> >  2 files changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> > index 9a0145b5b4..e52f9d0bd4 100644
> > --- a/hw/riscv/sifive_u.c
> > +++ b/hw/riscv/sifive_u.c
> > @@ -488,7 +488,7 @@ static void riscv_sifive_u_soc_init(Object *obj)
> >                            TYPE_SIFIVE_U_PRCI);
> >      sysbus_init_child_obj(obj, "otp", &s->otp, sizeof(s->otp),
> >                            TYPE_SIFIVE_U_OTP);
> > -    qdev_prop_set_uint32(DEVICE(&s->otp), "serial", OTP_SERIAL);
> > +    qdev_prop_set_uint32(DEVICE(&s->otp), "serial", s->serial);
> >      sysbus_init_child_obj(obj, "gem", &s->gem, sizeof(s->gem),
> >                            TYPE_CADENCE_GEM);
> >  }
> > @@ -607,10 +607,16 @@ static void riscv_sifive_u_soc_realize(DeviceState 
> > *dev, Error **errp)
> >          memmap[SIFIVE_U_GEM_MGMT].base, memmap[SIFIVE_U_GEM_MGMT].size);
> >  }
> >
> > +static Property riscv_sifive_u_soc_props[] = {
> > +    DEFINE_PROP_UINT32("serial", SiFiveUSoCState, serial, OTP_SERIAL),
> > +    DEFINE_PROP_END_OF_LIST()
>
> I am not sure how adding another level of property in the SoC could
> solve the 'make check' error.

The problem is that you were adding a machine property and then you
had the SoC reach up to the machine object to get the serial value.
This isn't correct and is why the tests fail.

This patch series instead adds a property to the machine and the SoC,
where the machine sets the SoC property.

>
> > +};
> > +
> >  static void riscv_sifive_u_soc_class_init(ObjectClass *oc, void *data)
> >  {
> >      DeviceClass *dc = DEVICE_CLASS(oc);
> >
> > +    device_class_set_props(dc, riscv_sifive_u_soc_props);
> >      dc->realize = riscv_sifive_u_soc_realize;
> >      /* Reason: Uses serial_hds in realize function, thus can't be used 
> > twice */
> >      dc->user_creatable = false;
> > diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
> > index 82667b5746..a2baa1de5f 100644
> > --- a/include/hw/riscv/sifive_u.h
> > +++ b/include/hw/riscv/sifive_u.h
> > @@ -42,6 +42,8 @@ typedef struct SiFiveUSoCState {
> >      SiFiveUPRCIState prci;
> >      SiFiveUOTPState otp;
> >      CadenceGEMState gem;
> > +
> > +    uint32_t serial;
> >  } SiFiveUSoCState;
> >
> >  #define TYPE_RISCV_U_MACHINE MACHINE_TYPE_NAME("sifive_u")
> > --
>
> But anyway this patch does not actually work as expected. See below:
>
> $ ./riscv64-softmmu/qemu-system-riscv64 -M sifive_u,serial=3
> -nographic -m 2G -bios opensbi_u-boot_sifive_u_64.bin
>
> OpenSBI v0.5 (Oct 31 2019 18:38:50)
>    ____                    _____ ____ _____
>   / __ \                  / ____|  _ \_   _|
>  | |  | |_ __   ___ _ __ | (___ | |_) || |
>  | |  | | '_ \ / _ \ '_ \ \___ \|  _ < | |
>  | |__| | |_) |  __/ | | |____) | |_) || |_
>   \____/| .__/ \___|_| |_|_____/|____/_____|
>         | |
>         |_|
>
> Platform Name          : SiFive Freedom U540
> Platform HART Features : RV64ACDFIMSU
> Platform Max HARTs     : 5
> Current Hart           : 1
> Firmware Base          : 0x80000000
> Firmware Size          : 96 KB
> Runtime SBI Version    : 0.2
>
> PMP0: 0x0000000080000000-0x000000008001ffff (A)
> PMP1: 0x0000000000000000-0xffffffffffffffff (A,R,W,X)
>
>
> U-Boot 2019.10 (Oct 31 2019 - 18:38:33 +0800)
>
> CPU:   rv64imafdcsu
> Model: SiFive HiFive Unleashed A00
> DRAM:  2 GiB
> MMC:
> In:    serial@10010000
> Out:   serial@10010000
> Err:   serial@10010000
> Net:
> Warning: ethernet@10090000 MAC addresses don't match:
> Address in ROM is          52:54:00:12:34:56
> Address in environment is  70:b3:d5:92:f0:01
> eth0: ethernet@10090000
> Hit any key to stop autoboot:  0
>
>
> See this line:
> Address in environment is  70:b3:d5:92:f0:01
>
> It should be: 70:b3:d5:92:f0:03 since I specified serial number as 3.

Fixed, The problem was setting the serial property too early.

Alistair

>
> Regards,
> Bin