Re: [PATCH v6 8/8] hw/gpio/aspeed: Add test case for AST2700

[Thomas Huth <thuth@redhat.com>]

On 30/09/2024 10.52, Jamin Lin wrote:
> Add test case to test GPIO output and input pins from A0 to D7 for AST2700.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
>   tests/qtest/aspeed_gpio-test.c | 77 ++++++++++++++++++++++++++++++++--
>   tests/qtest/meson.build        |  3 ++
>   2 files changed, 76 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/qtest/aspeed_gpio-test.c b/tests/qtest/aspeed_gpio-test.c
> index d38f51d719..03b3b1c2b2 100644
> --- a/tests/qtest/aspeed_gpio-test.c
> +++ b/tests/qtest/aspeed_gpio-test.c
> @@ -33,6 +33,10 @@
>   #define GPIO_ABCD_DATA_VALUE 0x000
>   #define GPIO_ABCD_DIRECTION  0x004
>   
> +/* AST2700 */
> +#define AST2700_GPIO_BASE 0x14C0B000
> +#define GPIOA0_CONTROL 0x180
> +
>   static void test_set_colocated_pins(const void *data)
>   {
>       QTestState *s = (QTestState *)data;
> @@ -72,17 +76,82 @@ static void test_set_input_pins(const void *data)
>       g_assert_cmphex(value, ==, 0xffffffff);
>   }
>   
> +static void test_2700_output_pins(const void *data)
> +{
> +    QTestState *s = (QTestState *)data;
> +    uint32_t offset = 0;
> +    uint32_t value = 0;
> +    uint32_t pin = 0;
> +
> +    for (char c = 'A'; c <= 'D'; c++) {
> +        for (int i = 0; i < 8; i++) {
> +            offset = AST2700_GPIO_BASE + GPIOA0_CONTROL + (pin * 4);
> +
> +            /* output direction and output hi */
> +            qtest_writel(s, offset, 0x00000003);
> +            value = qtest_readl(s, offset);
> +            g_assert_cmphex(value, ==, 0x00000003);
> +
> +            /* output direction and output low */
> +            qtest_writel(s, offset, 0x00000002);
> +            value = qtest_readl(s, offset);
> +            g_assert_cmphex(value, ==, 0x00000002);
> +            pin++;
> +        }
> +    }
> +}
> +
> +static void test_2700_input_pins(const void *data)
> +{
> +    QTestState *s = (QTestState *)data;
> +    char name[16];
> +    uint32_t offset = 0;
> +    uint32_t value = 0;
> +    uint32_t pin = 0;
> +
> +    for (char c = 'A'; c <= 'D'; c++) {
> +        for (int i = 0; i < 8; i++) {
> +            sprintf(name, "gpio%c%d", c, i);
> +            offset = AST2700_GPIO_BASE + GPIOA0_CONTROL + (pin * 4);
> +            /* input direction */
> +            qtest_writel(s, offset, 0);
> +
> +            /* set input */
> +            qtest_qom_set_bool(s, "/machine/soc/gpio", name, true);
> +            value = qtest_readl(s, offset);
> +            g_assert_cmphex(value, ==, 0x00002000);
> +
> +            /* clear input */
> +            qtest_qom_set_bool(s, "/machine/soc/gpio", name, false);
> +            value = qtest_readl(s, offset);
> +            g_assert_cmphex(value, ==, 0);
> +            pin++;
> +        }
> +    }
> +}

As far as I can see, there is nothing in these two functions that requires 
any of the other code in this file ...

> +
>   int main(int argc, char **argv)
>   {
> +    const char *arch = qtest_get_arch();
>       QTestState *s;
>       int r;
>   
>       g_test_init(&argc, &argv, NULL);
>   
> -    s = qtest_init("-machine ast2600-evb");
> -    qtest_add_data_func("/ast2600/gpio/set_colocated_pins", s,
> -                        test_set_colocated_pins);
> -    qtest_add_data_func("/ast2600/gpio/set_input_pins", s, test_set_input_pins);
> +    if (strcmp(arch, "aarch64") == 0) {
> +        s = qtest_init("-machine ast2700-evb");
> +        qtest_add_data_func("/ast2700/gpio/input_pins",
> +                            s, test_2700_input_pins);
> +        qtest_add_data_func("/ast2700/gpio/out_pins", s, test_2700_output_pins);
> +    } else {
> +        s = qtest_init("-machine ast2600-evb");
> +        qtest_add_data_func("/ast2600/gpio/set_colocated_pins", s,
> +                            test_set_colocated_pins);
> +        qtest_add_data_func("/ast2600/gpio/set_input_pins", s,
> +                            test_set_input_pins);
> +    }

... so the more I look at this, the more I think your new test should reside 
in a separate file that only gets executed for aarch64, while this file here 
should stay for arm 32-bit. Or is there a real compelling reason for putting 
your code in this file here?

  Thomas


>       r = g_test_run();
>       qtest_quit(s);
>   
> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> index 310865e49c..292980e3ad 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -209,6 +209,8 @@ qtests_aspeed = \
>     ['aspeed_hace-test',
>      'aspeed_smc-test',
>      'aspeed_gpio-test']
> +qtests_aspeed64 = \
> +  ['aspeed_gpio-test']
>   
>   qtests_stm32l4x5 = \
>     ['stm32l4x5_exti-test',
> @@ -247,6 +249,7 @@ qtests_aarch64 = \
>     (config_all_devices.has_key('CONFIG_RASPI') ? ['bcm2835-dma-test', 'bcm2835-i2c-test'] : []) +  \
>     (config_all_accel.has_key('CONFIG_TCG') and                                            \
>      config_all_devices.has_key('CONFIG_TPM_TIS_I2C') ? ['tpm-tis-i2c-test'] : []) + \
> +  (config_all_devices.has_key('CONFIG_ASPEED_SOC') ? qtests_aspeed64 : []) + \
>     ['arm-cpu-features',
>      'numa-test',
>      'boot-serial-test',