On Mon, Aug 12, 2024 at 8:27 AM Peter Maydell <peter.mayd...@linaro.org> wrote:
>
Hi Peter,
Thanks for the review!
> On Mon, 5 Aug 2024 at 21:17, Octavian Purdila <ta...@google.com> wrote:
> >
> > From: Stefan Stanacar <stefa...@google.com>
> >
> > From: Stefan Stanacar <stefa...@google.com>
> >
> > The CMSIS System View Description format(CMSIS-SVD) is an XML based
> > description of Arm Cortex-M microcontrollers provided and maintained
> > by sillicon vendors. It includes details such as peripherals registers
> > (down to bitfields), peripheral register block addresses, reset
> > values, etc.
> >
> > This script uses this information to create header files that makes it
> > easier to emulate peripherals.
> >
> > The script can be used to create either peripheral specific headers or
> > board / system specific information. The script generated headers are
> > similar to the SVDConv utility.
> >
> > Peripheral specific headers contains information such as register
> > layout, register names and reset values for registers:
> >
> > typedef struct {
> > ...
> > union {
> > uint32_t PSELID; /* 0x00000FF8 Peripheral Select and
> > * Flexcomm module ID */
> > struct {
> > uint32_t PERSEL : 3; /* [2..0] Peripheral Select */
> > uint32_t LOCK : 1; /* [3..3] Lock the peripheral select */
> > uint32_t USARTPRESENT : 1; /* [4..4] USART present indicator */
> > uint32_t SPIPRESENT : 1; /* [5..5] SPI present indicator */
> > uint32_t I2CPRESENT : 1; /* [6..6] I2C present indicator */
> > uint32_t I2SPRESENT : 1; /* [7..7] I2S Present */
> > uint32_t : 4;
> > uint32_t ID : 20; /* [31..12] Flexcomm ID */
> > } PSELID_b;
> > };
>
> Bitfield layout in C isn't portable, so don't generate this kind
> of union-of-a-integer-and-some-bitfields, please. You can
> generate FIELD() macro invocations (see include/hw/registerfields.h)
> which define shift/mask/length macros that can be used with
> FIELD_EX*/FIELD_DP* to do extract/deposit operations.
>
I see that C bitfields are already used in a few places in qemu. Could
you please elaborate on the portability issue?
> > ...
> > } FLEXCOMM_Type; /* Size = 4096 (0x1000) */
> >
> > #define FLEXCOMM_PSELID_PERSEL_Pos (0UL)
> > #define FLEXCOMM_PSELID_PERSEL_Msk (0x7UL)
> > #define FLEXCOMM_PSELID_LOCK_Pos (3UL)
> > #define FLEXCOMM_PSELID_LOCK_Msk (0x8UL)
> > ...
> >
> > typedef enum { /* FLEXCOMM_PSELID_LOCK */
> > /* Peripheral select can be changed by software. */
> > FLEXCOMM_PSELID_LOCK_UNLOCKED = 0,
> > /* Peripheral select is locked and cannot be changed until this
> > * Flexcomm module or the entire device is reset. */
> > FLEXCOMM_PSELID_LOCK_LOCKED = 1,
> > } FLEXCOMM_PSELID_LOCK_Enum;
> > ...
> >
> > #define FLEXCOMM_REGISTER_NAMES_ARRAY(_name) \
> > const char *_name[sizeof(FLEXCOMM_Type)] = { \
> > [4088 ... 4091] = "PSELID", \
> > [4092 ... 4095] = "PID", \
> > }
> >
> > Board specific headers contains information about peripheral base
> > register addresses.
>
> thanks
> -- PMM
