Re: [PATCH v6 08/10] hw/fsi: Added qtest

[Thomas Huth <thuth@redhat.com>]

On 21/10/2023 23.17, Ninad Palsule wrote:
> Added basic qtests for FSI model.
> 
> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> ---
> v3:
>   - Added new qtest as per Cedric's comment.
> V4:
>   - Remove MAINTAINER and documentation changes from this commit
> v6:
>   - Incorporated review comments by Thomas Huth.
> ---
>   tests/qtest/fsi-test.c  | 207 ++++++++++++++++++++++++++++++++++++++++
>   tests/qtest/meson.build |   1 +
>   2 files changed, 208 insertions(+)
>   create mode 100644 tests/qtest/fsi-test.c
> 
> diff --git a/tests/qtest/fsi-test.c b/tests/qtest/fsi-test.c
> new file mode 100644
> index 0000000000..01a0739092
> --- /dev/null
> +++ b/tests/qtest/fsi-test.c
> @@ -0,0 +1,207 @@
> +/*
> + * QTest testcases for IBM's Flexible Service Interface (FSI)
> + *
> + * Copyright (c) 2023 IBM Corporation
> + *
> + * Authors:
> + *   Ninad Palsule <ninad@linux.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include <glib/gstdio.h>
> +
> +#include "qemu/module.h"
> +#include "libqtest-single.h"
> +
> +/* Registers from ast2600 specifications */
> +#define ASPEED_FSI_ENGINER_TRIGGER   0x04
> +#define ASPEED_FSI_OPB0_BUS_SELECT   0x10
> +#define ASPEED_FSI_OPB1_BUS_SELECT   0x28
> +#define ASPEED_FSI_OPB0_RW_DIRECTION 0x14
> +#define ASPEED_FSI_OPB1_RW_DIRECTION 0x2c
> +#define ASPEED_FSI_OPB0_XFER_SIZE    0x18
> +#define ASPEED_FSI_OPB1_XFER_SIZE    0x30
> +#define ASPEED_FSI_OPB0_BUS_ADDR     0x1c
> +#define ASPEED_FSI_OPB1_BUS_ADDR     0x34
> +#define ASPEED_FSI_INTRRUPT_CLEAR    0x40
> +#define ASPEED_FSI_INTRRUPT_STATUS   0x48
> +#define ASPEED_FSI_OPB0_BUS_STATUS   0x80
> +#define ASPEED_FSI_OPB1_BUS_STATUS   0x8c
> +#define ASPEED_FSI_OPB0_READ_DATA    0x84
> +#define ASPEED_FSI_OPB1_READ_DATA    0x90
> +
> +/*
> + * FSI Base addresses from the ast2600 specifications.
> + */
> +#define AST2600_OPB_FSI0_BASE_ADDR 0x1e79b000
> +#define AST2600_OPB_FSI1_BASE_ADDR 0x1e79b100
> +
> +static uint32_t aspeed_fsi_base_addr;
> +
> +static uint32_t aspeed_fsi_readl(QTestState *s, uint32_t reg)
> +{
> +    return qtest_readl(s, aspeed_fsi_base_addr + reg);
> +}
> +
> +static void aspeed_fsi_writel(QTestState *s, uint32_t reg, uint32_t val)
> +{
> +    qtest_writel(s, aspeed_fsi_base_addr + reg, val);
> +}
> +
> +/* Setup base address and select register */
> +static void test_fsi_setup(QTestState *s, uint32_t base_addr)
> +{
> +    uint32_t curval;
> +
> +    /* Set the base select register */
> +    if (base_addr == AST2600_OPB_FSI0_BASE_ADDR) {
> +        aspeed_fsi_base_addr = base_addr;
> +
> +        /* Unselect FSI1 */
> +        aspeed_fsi_writel(s, ASPEED_FSI_OPB1_BUS_SELECT, 0x0);
> +        curval = aspeed_fsi_readl(s, ASPEED_FSI_OPB1_BUS_SELECT);
> +        g_assert_cmpuint(curval, ==, 0x0);
> +
> +        /* Select FSI0 */
> +        aspeed_fsi_writel(s, ASPEED_FSI_OPB0_BUS_SELECT, 0x1);
> +        curval = aspeed_fsi_readl(s, ASPEED_FSI_OPB0_BUS_SELECT);
> +        g_assert_cmpuint(curval, ==, 0x1);
> +    } else if (base_addr == AST2600_OPB_FSI1_BASE_ADDR) {
> +        aspeed_fsi_base_addr = base_addr;

You could move "aspeed_fsi_base_addr = base_addr" before the if-statement, 
so that you don't have to repeat it in both cases.

> +        /* Unselect FSI0 */
> +        aspeed_fsi_writel(s, ASPEED_FSI_OPB0_BUS_SELECT, 0x0);
> +        curval = aspeed_fsi_readl(s, ASPEED_FSI_OPB0_BUS_SELECT);
> +        g_assert_cmpuint(curval, ==, 0x0);
> +
> +        /* Select FSI1 */
> +        aspeed_fsi_writel(s, ASPEED_FSI_OPB1_BUS_SELECT, 0x1);
> +        curval = aspeed_fsi_readl(s, ASPEED_FSI_OPB1_BUS_SELECT);
> +        g_assert_cmpuint(curval, ==, 0x1);
> +    } else {
> +        g_assert_not_reached();
> +    }
> +}

Anyway:
Acked-by: Thomas Huth <thuth@redhat.com>