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

[Bin Meng]

Hi Alistair,

On Sat, Aug 22, 2020 at 3:04 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> 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.
>

Yes, I will be working on a series to clean hw/riscv directory.

Regards,
Bin