Patchwork [v3,15/16] boot-order-test: Support fw_cfg in I/O space

login
register
mail settings
Submitter Markus Armbruster
Date June 14, 2013, 11:15 a.m.
Message ID <1371208516-7857-16-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/251376/
State New
Headers show

Comments

Markus Armbruster - June 14, 2013, 11:15 a.m.
Next commit needs it.

Cc: Blue Swirl <blauwirbel@gmail.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/boot-order-test.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)
Anthony Liguori - June 14, 2013, 1:53 p.m.
Markus Armbruster <armbru@redhat.com> writes:

> Next commit needs it.
>
> Cc: Blue Swirl <blauwirbel@gmail.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/boot-order-test.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
> index 7b1edc1..d1d99f8 100644
> --- a/tests/boot-order-test.c
> +++ b/tests/boot-order-test.c
> @@ -133,23 +133,31 @@ static void test_prep_boot_order(void)
>      test_boot_orders("prep", read_boot_order_prep, test_cases_prep);
>  }
>  
> -static void read_fw_cfg(uint64_t cfg_addr, uint16_t cmd,
> +static void read_fw_cfg(uint64_t cfg_addr, bool addr_is_io, uint16_t cmd,
>                          void *buf, size_t len)

I missed it earlier, but you can use libqos/fw_cfg.h for this.

Regards,

Anthony Liguori

>  {
>      uint8_t *p = buf;
>      size_t i;
>  
> -    writew(cfg_addr, cmd);
> -    for (i = 0; i < len; i++) {
> -        p[i] = readb(cfg_addr + 2);
> +    if (addr_is_io) {
> +        outw(cfg_addr, cmd);
> +        for (i = 0; i < len; i++) {
> +            p[i] = inb(cfg_addr + 1);
> +        }
> +    } else {
> +        writew(cfg_addr, cmd);
> +        for (i = 0; i < len; i++) {
> +            p[i] = readb(cfg_addr + 2);
> +        }
>      }
>  }
>  
> -static uint16_t read_fw_cfg_i16(uint64_t cfg_addr, uint16_t cmd)
> +static uint16_t read_fw_cfg_i16(uint64_t cfg_addr, bool addr_is_io,
> +                                    uint16_t cmd)
>  {
>      uint16_t value;
>  
> -    read_fw_cfg(cfg_addr, cmd, &value, sizeof(value));
> +    read_fw_cfg(cfg_addr, addr_is_io, cmd, &value, sizeof(value));
>      return le16_to_cpu(value);
>  }
>  
> @@ -157,7 +165,7 @@ static uint16_t read_fw_cfg_i16(uint64_t cfg_addr, uint16_t cmd)
>  
>  static uint64_t read_boot_order_pmac(void)
>  {
> -    return read_fw_cfg_i16(PMAC_CFG_ADDR, FW_CFG_BOOT_DEVICE);
> +    return read_fw_cfg_i16(PMAC_CFG_ADDR, false, FW_CFG_BOOT_DEVICE);
>  }
>  
>  static const boot_order_test test_cases_fw_cfg[] = {
> @@ -182,7 +190,7 @@ static void test_pmac_newworld_boot_order(void)
>  
>  static uint64_t read_boot_order_sun4m(void)
>  {
> -    return read_fw_cfg_i16(SUN4M_CFG_ADDR, FW_CFG_BOOT_DEVICE);
> +    return read_fw_cfg_i16(SUN4M_CFG_ADDR, false, FW_CFG_BOOT_DEVICE);
>  }
>  
>  static void test_sun4m_boot_order(void)
> -- 
> 1.7.11.7
Andreas Färber - June 14, 2013, 2:04 p.m.
Am 14.06.2013 15:53, schrieb Anthony Liguori:
> Markus Armbruster <armbru@redhat.com> writes:
> 
>> Next commit needs it.
>>
>> Cc: Blue Swirl <blauwirbel@gmail.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  tests/boot-order-test.c | 24 ++++++++++++++++--------
>>  1 file changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
>> index 7b1edc1..d1d99f8 100644
>> --- a/tests/boot-order-test.c
>> +++ b/tests/boot-order-test.c
>> @@ -133,23 +133,31 @@ static void test_prep_boot_order(void)
>>      test_boot_orders("prep", read_boot_order_prep, test_cases_prep);
>>  }
>>  
>> -static void read_fw_cfg(uint64_t cfg_addr, uint16_t cmd,
>> +static void read_fw_cfg(uint64_t cfg_addr, bool addr_is_io, uint16_t cmd,
>>                          void *buf, size_t len)
> 
> I missed it earlier, but you can use libqos/fw_cfg.h for this.

Agree, but that didn't exist at the time. :-)

Regards,
Andreas
Markus Armbruster - June 19, 2013, 6:49 a.m.
Anthony Liguori <aliguori@us.ibm.com> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> Next commit needs it.
>>
>> Cc: Blue Swirl <blauwirbel@gmail.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  tests/boot-order-test.c | 24 ++++++++++++++++--------
>>  1 file changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
>> index 7b1edc1..d1d99f8 100644
>> --- a/tests/boot-order-test.c
>> +++ b/tests/boot-order-test.c
>> @@ -133,23 +133,31 @@ static void test_prep_boot_order(void)
>>      test_boot_orders("prep", read_boot_order_prep, test_cases_prep);
>>  }
>>  
>> -static void read_fw_cfg(uint64_t cfg_addr, uint16_t cmd,
>> +static void read_fw_cfg(uint64_t cfg_addr, bool addr_is_io, uint16_t cmd,
>>                          void *buf, size_t len)
>
> I missed it earlier, but you can use libqos/fw_cfg.h for this.

Missed on rebase, thanks for pointing it out.

Two options:

(1) You commit this minor code duplication now, and I promise to clean
    it up in a follow-up series.

(2) You tell me to rework this series so it uses libqos/fw_cfg.h from
    the start.

I'd prefer (1).
Markus Armbruster - June 19, 2013, 6:47 p.m.
Markus Armbruster <armbru@redhat.com> writes:

> Anthony Liguori <aliguori@us.ibm.com> writes:
>
>> Markus Armbruster <armbru@redhat.com> writes:
>>
>>> Next commit needs it.
>>>
>>> Cc: Blue Swirl <blauwirbel@gmail.com>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>  tests/boot-order-test.c | 24 ++++++++++++++++--------
>>>  1 file changed, 16 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
>>> index 7b1edc1..d1d99f8 100644
>>> --- a/tests/boot-order-test.c
>>> +++ b/tests/boot-order-test.c
>>> @@ -133,23 +133,31 @@ static void test_prep_boot_order(void)
>>>      test_boot_orders("prep", read_boot_order_prep, test_cases_prep);
>>>  }
>>>  
>>> -static void read_fw_cfg(uint64_t cfg_addr, uint16_t cmd,
>>> +static void read_fw_cfg(uint64_t cfg_addr, bool addr_is_io, uint16_t cmd,
>>>                          void *buf, size_t len)
>>
>> I missed it earlier, but you can use libqos/fw_cfg.h for this.
>
> Missed on rebase, thanks for pointing it out.
>
> Two options:
>
> (1) You commit this minor code duplication now, and I promise to clean
>     it up in a follow-up series.
>
> (2) You tell me to rework this series so it uses libqos/fw_cfg.h from
>     the start.
>
> I'd prefer (1).

(3) Commit PATCH 01-06/16 now (which passed your review), bounce the
rest back to me.

Still prefer (1), though :)

Patch

diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
index 7b1edc1..d1d99f8 100644
--- a/tests/boot-order-test.c
+++ b/tests/boot-order-test.c
@@ -133,23 +133,31 @@  static void test_prep_boot_order(void)
     test_boot_orders("prep", read_boot_order_prep, test_cases_prep);
 }
 
-static void read_fw_cfg(uint64_t cfg_addr, uint16_t cmd,
+static void read_fw_cfg(uint64_t cfg_addr, bool addr_is_io, uint16_t cmd,
                         void *buf, size_t len)
 {
     uint8_t *p = buf;
     size_t i;
 
-    writew(cfg_addr, cmd);
-    for (i = 0; i < len; i++) {
-        p[i] = readb(cfg_addr + 2);
+    if (addr_is_io) {
+        outw(cfg_addr, cmd);
+        for (i = 0; i < len; i++) {
+            p[i] = inb(cfg_addr + 1);
+        }
+    } else {
+        writew(cfg_addr, cmd);
+        for (i = 0; i < len; i++) {
+            p[i] = readb(cfg_addr + 2);
+        }
     }
 }
 
-static uint16_t read_fw_cfg_i16(uint64_t cfg_addr, uint16_t cmd)
+static uint16_t read_fw_cfg_i16(uint64_t cfg_addr, bool addr_is_io,
+                                    uint16_t cmd)
 {
     uint16_t value;
 
-    read_fw_cfg(cfg_addr, cmd, &value, sizeof(value));
+    read_fw_cfg(cfg_addr, addr_is_io, cmd, &value, sizeof(value));
     return le16_to_cpu(value);
 }
 
@@ -157,7 +165,7 @@  static uint16_t read_fw_cfg_i16(uint64_t cfg_addr, uint16_t cmd)
 
 static uint64_t read_boot_order_pmac(void)
 {
-    return read_fw_cfg_i16(PMAC_CFG_ADDR, FW_CFG_BOOT_DEVICE);
+    return read_fw_cfg_i16(PMAC_CFG_ADDR, false, FW_CFG_BOOT_DEVICE);
 }
 
 static const boot_order_test test_cases_fw_cfg[] = {
@@ -182,7 +190,7 @@  static void test_pmac_newworld_boot_order(void)
 
 static uint64_t read_boot_order_sun4m(void)
 {
-    return read_fw_cfg_i16(SUN4M_CFG_ADDR, FW_CFG_BOOT_DEVICE);
+    return read_fw_cfg_i16(SUN4M_CFG_ADDR, false, FW_CFG_BOOT_DEVICE);
 }
 
 static void test_sun4m_boot_order(void)