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

[Ninad Palsule <ninad@linux.ibm.com>]

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>
>