On Fri, 8 Dec 2023 at 02:35, Sergey Kambalin <serg.o...@gmail.com> wrote:
>
> Signed-off-by: Sergey Kambalin <sergey.kamba...@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
