Re: [PATCH 08/41] opentitan: Rename memmap enum constants

[Alistair Francis]

On Mon, Aug 17, 2020 at 2:53 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Sat, Aug 15, 2020 at 1:56 AM Philippe Mathieu-Daudé <f4bug@amsat.org> 
> wrote:
> >
> > On 8/14/20 12:25 AM, Eduardo Habkost wrote:
> > > Some of the enum constant names conflict with the QOM type check
> > > macros.  This needs to be addressed to allow us to transform the
> > > QOM type check macros into functions generated by
> > > OBJECT_DECLARE_TYPE().
> > >
> > > Rename all the constants to IBEX_DEV_*, to avoid conflicts.
> > >
> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > ---
> > >  include/hw/riscv/opentitan.h | 38 ++++++++--------
> > >  hw/riscv/opentitan.c         | 84 ++++++++++++++++++------------------
> > >  2 files changed, 61 insertions(+), 61 deletions(-)
> > >
> > > diff --git a/include/hw/riscv/opentitan.h b/include/hw/riscv/opentitan.h
> > > index 8f29b9cbbf..835a80f896 100644
> > > --- a/include/hw/riscv/opentitan.h
> > > +++ b/include/hw/riscv/opentitan.h
> > > @@ -49,25 +49,25 @@ typedef struct OpenTitanState {
> > >  } OpenTitanState;
> > >
> > >  enum {
> > > -    IBEX_ROM,
> > > -    IBEX_RAM,
> > > -    IBEX_FLASH,
> > > -    IBEX_UART,
> > > -    IBEX_GPIO,
> > > -    IBEX_SPI,
> > > -    IBEX_FLASH_CTRL,
> > > -    IBEX_RV_TIMER,
> > > -    IBEX_AES,
> > > -    IBEX_HMAC,
> > > -    IBEX_PLIC,
> > > -    IBEX_PWRMGR,
> > > -    IBEX_RSTMGR,
> > > -    IBEX_CLKMGR,
> > > -    IBEX_PINMUX,
> > > -    IBEX_ALERT_HANDLER,
> > > -    IBEX_NMI_GEN,
> > > -    IBEX_USBDEV,
> > > -    IBEX_PADCTRL,
> > > +    IBEX_DEV_ROM,
> > > +    IBEX_DEV_RAM,
> > > +    IBEX_DEV_FLASH,
> > > +    IBEX_DEV_UART,
> > > +    IBEX_DEV_GPIO,
> > > +    IBEX_DEV_SPI,
> > > +    IBEX_DEV_FLASH_CTRL,
> > > +    IBEX_DEV_RV_TIMER,
> > > +    IBEX_DEV_AES,
> > > +    IBEX_DEV_HMAC,
> > > +    IBEX_DEV_PLIC,
> > > +    IBEX_DEV_PWRMGR,
> > > +    IBEX_DEV_RSTMGR,
> > > +    IBEX_DEV_CLKMGR,
> > > +    IBEX_DEV_PINMUX,
> > > +    IBEX_DEV_ALERT_HANDLER,
> > > +    IBEX_DEV_NMI_GEN,
> > > +    IBEX_DEV_USBDEV,
> > > +    IBEX_DEV_PADCTRL,
> >
> > Similarly, why is this enum in a public header and not local
> > to opentitan.c, only place where it is used?
> >
>
> I believe the reason is that opentitan.c implements both SoC and board
> stuff. These enums are helpful to define SoC peripherals hence
> technically another board that uses the same SoC may utilize these
> macros.
>
> Unfortunately this is the case for all RISC-V boards so far. Should we
> clean this up, for example, splitting SoC and board codes into 2
> files?

Yeah the hw/riscv directory is a bit of a mess. We need to look at
moving non machine parts out (like sifive_uart for example) and then
it's probably worth splitting SoC and machine. It does result in some
extra files (some of which are very simple) but the current setup is
pretty confusing.

Are you volunteering Bin? Otherwise I can look at it.

Alistair

>
> Regards,
> Bin
>