Re: [PATCH v4 40/45] Add mailbox property tests. Part 1

[Peter Maydell]

On Fri, 8 Dec 2023 at 02:35, Sergey Kambalin <serg.oker@gmail.com> wrote:
>
> Signed-off-by: Sergey Kambalin <sergey.kambalin@auriga.com>
> ---
>  tests/qtest/bcm2838-mailbox.c            |  34 ++--
>  tests/qtest/bcm2838-mailbox.h            |  18 +-
>  tests/qtest/bcm2838-mbox-property-test.c | 206 +++++++++++++++++++++++
>  tests/qtest/meson.build                  |   2 +-
>  4 files changed, 220 insertions(+), 40 deletions(-)
>  create mode 100644 tests/qtest/bcm2838-mbox-property-test.c
>
> diff --git a/tests/qtest/bcm2838-mailbox.c b/tests/qtest/bcm2838-mailbox.c
> index 2edc24e15e..4b160cd56c 100644
> --- a/tests/qtest/bcm2838-mailbox.c
> +++ b/tests/qtest/bcm2838-mailbox.c
> @@ -12,6 +12,10 @@
>  #include "libqtest-single.h"
>  #include "bcm2838-mailbox.h"
>
> +REG32(MBOX_EXCHNG_REG,          0)
> +FIELD(MBOX_EXCHNG_REG, CHANNEL, 0, 4)
> +FIELD(MBOX_EXCHNG_REG, DATA,    4, 28)
> +
>
>  static uint32_t qtest_mbox0_read_reg32(QTestState *s, uint32_t offset)
>  {
> @@ -25,47 +29,33 @@ static void qtest_mbox1_write_reg32(QTestState *s, 
> uint32_t offset, uint32_t val
>
>  static void qtest_mbox1_write(QTestState *s, uint8_t channel, uint32_t data)
>  {
> -    uint32_t reg;
> +    uint32_t mbox_reg = 0;
>
> -    reg = FIELD_DP32(reg, MBOX_WRITE_REG, CHANNEL, channel);
> -    reg = FIELD_DP32(reg, MBOX_WRITE_REG, DATA, data);
> -    qtest_mbox1_write_reg32(s, MBOX_REG_WRITE, reg);
> +    mbox_reg = FIELD_DP32(mbox_reg, MBOX_EXCHNG_REG, CHANNEL, channel);
> +    mbox_reg = FIELD_DP32(mbox_reg, MBOX_EXCHNG_REG, DATA, data);
> +    qtest_mbox1_write_reg32(s, MBOX_REG_WRITE, mbox_reg);

Why change the variable name? I don't mind which one you use,
but please pick something and stick to it.

>  }
>
>  int qtest_mbox0_has_data(QTestState *s) {
>      return !(qtest_mbox0_read_reg32(s, MBOX_REG_STATUS) & MBOX_READ_EMPTY);
>  }
>
> -int mbox0_has_data(void) {
> -    return qtest_mbox0_has_data(global_qtest);
> -}

Why did we add this if we're now deleting it?

> -
>  void qtest_mbox0_read_message(QTestState *s,
>                                uint8_t channel,
>                                void *msgbuf,
>                                size_t msgbuf_size)
>  {
> -    uint32_t reg;
> +    uint32_t mbox_reg;
>      uint32_t msgaddr;
>
>      g_assert(qtest_mbox0_has_data(s));
> -    reg = qtest_mbox0_read_reg32(s, MBOX_REG_READ);
> -    g_assert_cmphex(FIELD_EX32(reg, MBOX_WRITE_REG, CHANNEL), ==, channel);
> -    msgaddr = FIELD_EX32(reg, MBOX_WRITE_REG, DATA) << 4;
> +    mbox_reg = qtest_mbox0_read_reg32(s, MBOX_REG_READ);
> +    g_assert_cmphex(FIELD_EX32(mbox_reg, MBOX_EXCHNG_REG, CHANNEL), ==, 
> channel);
> +    msgaddr = FIELD_EX32(mbox_reg, MBOX_EXCHNG_REG, DATA) << 4;
>      qtest_memread(s, msgaddr, msgbuf, msgbuf_size);
>  }
>
> -void mbox0_read_message(uint8_t channel, void *msgbuf, size_t msgbuf_size) {
> -    qtest_mbox0_read_message(global_qtest, channel, msgbuf, msgbuf_size);
> -}
> -
>  void qtest_mbox1_write_message(QTestState *s, uint8_t channel, uint32_t 
> msg_addr)
>  {
>      qtest_mbox1_write(s, channel, msg_addr >> 4);
>  }
> -
> -
> -void mbox1_write_message(uint8_t channel, uint32_t msg_addr)
> -{
> -    qtest_mbox1_write_message(global_qtest, channel, msg_addr);
> -}
> diff --git a/tests/qtest/bcm2838-mailbox.h b/tests/qtest/bcm2838-mailbox.h
> index 2b140a5d32..7e660e65a7 100644
> --- a/tests/qtest/bcm2838-mailbox.h
> +++ b/tests/qtest/bcm2838-mailbox.h
> @@ -77,7 +77,7 @@
>  #define TAG_SET_GPIO_STATE         0x00038041
>  #define TAG_INITIALIZE_VCHIQ       0x00048010
>
> -#define BOARD_REVISION    11546898
> +#define BOARD_REVISION    0xB03115

This doesn't look like it should be here ?

>  #define FIRMWARE_REVISION 346337
>  #define FIRMWARE_VARIANT  0x77777777 /* TODO: Find the real value */
>
> @@ -147,22 +147,6 @@
>  /* Used to test stubs that don't perform actual work */
>  #define DUMMY_VALUE 0x12345678
>
> -REG32(MBOX_WRITE_REG,          0)
> -FIELD(MBOX_WRITE_REG, CHANNEL, 0, 4)
> -FIELD(MBOX_WRITE_REG, DATA,    4, 28)
> -
> -REG32(MBOX_SIZE_STAT,          0)
> -FIELD(MBOX_SIZE_STAT, SIZE,    0, 30)
> -FIELD(MBOX_SIZE_STAT, SUCCESS, 30, 1)
> -
> -REG32(SET_DEVICE_POWER_STATE_CMD,        0)
> -FIELD(SET_DEVICE_POWER_STATE_CMD, EN,    0, 1)
> -FIELD(SET_DEVICE_POWER_STATE_CMD, WAIT,  1, 1)
> -
> -REG32(GET_CLOCK_STATE_CMD,        0)
> -FIELD(GET_CLOCK_STATE_CMD, EN,    0, 1)
> -FIELD(GET_CLOCK_STATE_CMD, NPRES, 1, 1)

We only just added these in the previous patch ?

> -
>  typedef struct {
>      uint32_t size;
>      uint32_t req_resp_code;

thanks
-- PMM