Re: [Qemu-devel] [PATCH v2 11/28] riscv: sifive: Rename sifive_prci.{c, h} to sifive_e_prci.{c, h}

[Alistair Francis]

On Sun, Aug 11, 2019 at 1:00 AM Bin Meng <address@hidden> wrote:
>
> Hi Alistair,
>
> On Sat, Aug 10, 2019 at 9:51 AM Alistair Francis <address@hidden> wrote:
> >
> > On Wed, Aug 7, 2019 at 12:49 AM Bin Meng <address@hidden> wrote:
> > >
> > > Current SiFive PRCI model only works with sifive_e machine, as it
> > > only emulates registers or PRCI block in the FE310 SoC.
> > >
> > > Rename the file name to make it clear that it is for sifive_e.
> > >
> > > Signed-off-by: Bin Meng <address@hidden>
> > > ---
> > >
> > > Changes in v2: None
> > >
> > >  hw/riscv/Makefile.objs                              |  2 +-
> > >  hw/riscv/sifive_e.c                                 |  4 ++--
> > >  hw/riscv/{sifive_prci.c => sifive_e_prci.c}         | 14 +++++++-------
> > >  include/hw/riscv/{sifive_prci.h => sifive_e_prci.h} | 14 +++++++-------
> > >  4 files changed, 17 insertions(+), 17 deletions(-)
> > >  rename hw/riscv/{sifive_prci.c => sifive_e_prci.c} (90%)
> > >  rename include/hw/riscv/{sifive_prci.h => sifive_e_prci.h} (82%)
> > >
> > > diff --git a/hw/riscv/Makefile.objs b/hw/riscv/Makefile.objs
> > > index eb9d4f9..c859697 100644
> > > --- a/hw/riscv/Makefile.objs
> > > +++ b/hw/riscv/Makefile.objs
> > > @@ -2,9 +2,9 @@ obj-y += boot.o
> > >  obj-$(CONFIG_SPIKE) += riscv_htif.o
> > >  obj-$(CONFIG_HART) += riscv_hart.o
> > >  obj-$(CONFIG_SIFIVE_E) += sifive_e.o
> > > +obj-$(CONFIG_SIFIVE_E) += sifive_e_prci.o
> > >  obj-$(CONFIG_SIFIVE) += sifive_clint.o
> > >  obj-$(CONFIG_SIFIVE) += sifive_gpio.o
> > > -obj-$(CONFIG_SIFIVE) += sifive_prci.o
> > >  obj-$(CONFIG_SIFIVE) += sifive_plic.o
> > >  obj-$(CONFIG_SIFIVE) += sifive_test.o
> > >  obj-$(CONFIG_SIFIVE_U) += sifive_u.o
> > > diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
> > > index 2a499d8..2d67670 100644
> > > --- a/hw/riscv/sifive_e.c
> > > +++ b/hw/riscv/sifive_e.c
> > > @@ -41,9 +41,9 @@
> > >  #include "hw/riscv/riscv_hart.h"
> > >  #include "hw/riscv/sifive_plic.h"
> > >  #include "hw/riscv/sifive_clint.h"
> > > -#include "hw/riscv/sifive_prci.h"
> > >  #include "hw/riscv/sifive_uart.h"
> > >  #include "hw/riscv/sifive_e.h"
> > > +#include "hw/riscv/sifive_e_prci.h"
> > >  #include "hw/riscv/boot.h"
> > >  #include "chardev/char.h"
> > >  #include "sysemu/arch_init.h"
> > > @@ -174,7 +174,7 @@ static void riscv_sifive_e_soc_realize(DeviceState 
> > > *dev, Error **errp)
> > >          SIFIVE_SIP_BASE, SIFIVE_TIMECMP_BASE, SIFIVE_TIME_BASE);
> > >      sifive_mmio_emulate(sys_mem, "riscv.sifive.e.aon",
> > >          memmap[SIFIVE_E_AON].base, memmap[SIFIVE_E_AON].size);
> > > -    sifive_prci_create(memmap[SIFIVE_E_PRCI].base);
> > > +    sifive_e_prci_create(memmap[SIFIVE_E_PRCI].base);
> > >
> > >      /* GPIO */
> > >
> > > diff --git a/hw/riscv/sifive_prci.c b/hw/riscv/sifive_e_prci.c
> > > similarity index 90%
> > > rename from hw/riscv/sifive_prci.c
> > > rename to hw/riscv/sifive_e_prci.c
> > > index f406682..acb914d 100644
> > > --- a/hw/riscv/sifive_prci.c
> > > +++ b/hw/riscv/sifive_e_prci.c
> > > @@ -1,5 +1,5 @@
> > >  /*
> > > - * QEMU SiFive PRCI (Power, Reset, Clock, Interrupt)
> > > + * QEMU SiFive E PRCI (Power, Reset, Clock, Interrupt)
> > >   *
> > >   * Copyright (c) 2017 SiFive, Inc.
> > >   *
> > > @@ -22,7 +22,7 @@
> > >  #include "hw/sysbus.h"
> > >  #include "qemu/module.h"
> > >  #include "target/riscv/cpu.h"
> > > -#include "hw/riscv/sifive_prci.h"
> > > +#include "hw/riscv/sifive_e_prci.h"
> > >
> > >  static uint64_t sifive_prci_read(void *opaque, hwaddr addr, unsigned int 
> > > size)
> > >  {
> > > @@ -82,10 +82,10 @@ static const MemoryRegionOps sifive_prci_ops = {
> > >
> > >  static void sifive_prci_init(Object *obj)
> > >  {
> > > -    SiFivePRCIState *s = SIFIVE_PRCI(obj);
> > > +    SiFivePRCIState *s = SIFIVE_E_PRCI(obj);
> >
> > Should we not rename the struct as well?
> >
>
> I think this is OK given it's only used by sifive_e machine and will
> not be mixed with sifive_u.

Structs can be public though, so this seems risky. I feel like it
should be renamed with the file.

Alistair

>
> Regards,
> Bin