Hi Thomas, Cedric
> Subject: Re: [PATCH v6 8/8] hw/gpio/aspeed: Add test case for AST2700
>
> 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_...@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 ?
>
Thanks for suggestion.
Will move it into new file, ast2700-gptio-test.c.
Jamin
> Thomas
