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

[Ninad Palsule]

Hello Thomas,

On 10/23/23 01:51, Thomas Huth wrote:
> 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.

Fixed it. I will update it in version 7.

Thanks for the review.

Regards,

Ninad

> > +        /* 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>