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

[Thomas Huth <thuth@redhat.com>]

On 30/09/2024 18.48, Cédric Le Goater wrote:
> On 9/30/24 18:36, Thomas Huth wrote:
>> 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?
> 
> Because it is related to the Aspeed GPIO controllers. Unless we
> want to introduce a new set of test files for 64-bit Aspeed SoC
> controllers ? why not.
> 
> I am ok with both options. Option 1 is simpler to implement, but
> there may be reasons to separate the tests based on architecture,
> although I'm not aware of any at the moment. Would you rather prefer
> option 2 ? How should we name the test file ?

Since all arm 32-bit tests are currently completely separate from the 
aarch64 tests, I think a separate file would be better, indeed.
Simply call it ast2700-gpio-test.c ?

  Thomas