[edk2,edk2-platforms] Silicon/SynQuacerPciCpuIo2Dxe: fix PCIe I/O translation

Message ID 20181019104826.23073-1-ard.biesheuvel@linaro.org
State New
Headers show
Series
  • [edk2,edk2-platforms] Silicon/SynQuacerPciCpuIo2Dxe: fix PCIe I/O translation
Related show

Commit Message

Ard Biesheuvel Oct. 19, 2018, 10:48 a.m.
Commit 9dd8190e4995 ("Silicon/SynQuacer: tweak PCI I/O windows for
ACPI/Linux support") updated the min/max/offset definitions for the
PCIe I/O resource windows on SynQuacer, and updated the read path of
the platform's EfiCpuIo2 protocol implementation, but failed to update
the write path as well, resulting in spurious errors if when attempting
to write to PCIe I/O ports on PCIe RC #1, which uses translation for the
I/O BAR window.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 Silicon/Socionext/SynQuacer/Drivers/SynQuacerPciCpuIo2Dxe/SynQuacerPciCpuIo2Dxe.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

-- 
2.17.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Leif Lindholm Oct. 20, 2018, 6:52 a.m. | #1
Looks good functionality-wise, but is a bit of a handful to look at (and
not just because I'm code reviewing on a phone).
Could you do it with a couple of temp vars?

/
    Leif

On Fri, 19 Oct 2018, 18:48 Ard Biesheuvel, <ard.biesheuvel@linaro.org>
wrote:

> Commit 9dd8190e4995 ("Silicon/SynQuacer: tweak PCI I/O windows for

> ACPI/Linux support") updated the min/max/offset definitions for the

> PCIe I/O resource windows on SynQuacer, and updated the read path of

> the platform's EfiCpuIo2 protocol implementation, but failed to update

> the write path as well, resulting in spurious errors if when attempting

> to write to PCIe I/O ports on PCIe RC #1, which uses translation for the

> I/O BAR window.

>

> Contributed-under: TianoCore Contribution Agreement 1.1

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

>  Silicon/Socionext/SynQuacer/Drivers/SynQuacerPciCpuIo2Dxe/SynQuacerPciCpuIo2Dxe.c

> | 18 ++++++++++++------

>  1 file changed, 12 insertions(+), 6 deletions(-)

>

> diff --git

> a/Silicon/Socionext/SynQuacer/Drivers/SynQuacerPciCpuIo2Dxe/SynQuacerPciCpuIo2Dxe.c

> b/Silicon/Socionext/SynQuacer/Drivers/SynQuacerPciCpuIo2Dxe/SynQuacerPciCpuIo2Dxe.c

> index 736b20cd5129..e5cc3aef908d 100644

> ---

> a/Silicon/Socionext/SynQuacer/Drivers/SynQuacerPciCpuIo2Dxe/SynQuacerPciCpuIo2Dxe.c

> +++

> b/Silicon/Socionext/SynQuacer/Drivers/SynQuacerPciCpuIo2Dxe/SynQuacerPciCpuIo2Dxe.c

> @@ -518,12 +518,18 @@ CpuIoServiceWrite (

>      return Status;

>    }

>

> -  if ((Address >= SYNQUACER_PCI_SEG0_PORTIO_MIN) &&

> -      (Address <= SYNQUACER_PCI_SEG0_PORTIO_MAX)) {

> -    Address += SYNQUACER_PCI_SEG0_PORTIO_MEMBASE;

> -  } else if ((Address >= SYNQUACER_PCI_SEG1_PORTIO_MIN) &&

> -             (Address <= SYNQUACER_PCI_SEG1_PORTIO_MAX)) {

> -    Address += SYNQUACER_PCI_SEG1_PORTIO_MEMBASE;

> +  if ((Address >= (SYNQUACER_PCI_SEG0_PORTIO_MIN +

> +                   SYNQUACER_PCI_SEG0_PORTIO_OFFSET)) &&

> +      (Address <= (SYNQUACER_PCI_SEG0_PORTIO_MAX +

> +                   SYNQUACER_PCI_SEG0_PORTIO_OFFSET))) {

> +    Address += SYNQUACER_PCI_SEG0_PORTIO_MEMBASE -

> +               SYNQUACER_PCI_SEG0_PORTIO_OFFSET;

> +  } else if ((Address >= (SYNQUACER_PCI_SEG1_PORTIO_MIN +

> +                          SYNQUACER_PCI_SEG1_PORTIO_OFFSET)) &&

> +             (Address <= (SYNQUACER_PCI_SEG1_PORTIO_MAX +

> +                          SYNQUACER_PCI_SEG1_PORTIO_OFFSET))) {

> +    Address += SYNQUACER_PCI_SEG1_PORTIO_MEMBASE -

> +               SYNQUACER_PCI_SEG1_PORTIO_OFFSET;

>

>    } else {

>      ASSERT (FALSE);

> --

> 2.17.1

>

>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Oct. 20, 2018, 2:06 p.m. | #2
On 20 October 2018 at 14:52, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> Looks good functionality-wise, but is a bit of a handful to look at (and not

> just because I'm code reviewing on a phone).

> Could you do it with a couple of temp vars?

>


Yes, but then CpuIoServiceWrite would deviate from CpuIoServiceRead,
so I should probably break this out into a helper function as well.

> On Fri, 19 Oct 2018, 18:48 Ard Biesheuvel, <ard.biesheuvel@linaro.org>

> wrote:

>>

>> Commit 9dd8190e4995 ("Silicon/SynQuacer: tweak PCI I/O windows for

>> ACPI/Linux support") updated the min/max/offset definitions for the

>> PCIe I/O resource windows on SynQuacer, and updated the read path of

>> the platform's EfiCpuIo2 protocol implementation, but failed to update

>> the write path as well, resulting in spurious errors if when attempting

>> to write to PCIe I/O ports on PCIe RC #1, which uses translation for the

>> I/O BAR window.

>>

>> Contributed-under: TianoCore Contribution Agreement 1.1

>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>> ---

>>

>> Silicon/Socionext/SynQuacer/Drivers/SynQuacerPciCpuIo2Dxe/SynQuacerPciCpuIo2Dxe.c

>> | 18 ++++++++++++------

>>  1 file changed, 12 insertions(+), 6 deletions(-)

>>

>> diff --git

>> a/Silicon/Socionext/SynQuacer/Drivers/SynQuacerPciCpuIo2Dxe/SynQuacerPciCpuIo2Dxe.c

>> b/Silicon/Socionext/SynQuacer/Drivers/SynQuacerPciCpuIo2Dxe/SynQuacerPciCpuIo2Dxe.c

>> index 736b20cd5129..e5cc3aef908d 100644

>> ---

>> a/Silicon/Socionext/SynQuacer/Drivers/SynQuacerPciCpuIo2Dxe/SynQuacerPciCpuIo2Dxe.c

>> +++

>> b/Silicon/Socionext/SynQuacer/Drivers/SynQuacerPciCpuIo2Dxe/SynQuacerPciCpuIo2Dxe.c

>> @@ -518,12 +518,18 @@ CpuIoServiceWrite (

>>      return Status;

>>    }

>>

>> -  if ((Address >= SYNQUACER_PCI_SEG0_PORTIO_MIN) &&

>> -      (Address <= SYNQUACER_PCI_SEG0_PORTIO_MAX)) {

>> -    Address += SYNQUACER_PCI_SEG0_PORTIO_MEMBASE;

>> -  } else if ((Address >= SYNQUACER_PCI_SEG1_PORTIO_MIN) &&

>> -             (Address <= SYNQUACER_PCI_SEG1_PORTIO_MAX)) {

>> -    Address += SYNQUACER_PCI_SEG1_PORTIO_MEMBASE;

>> +  if ((Address >= (SYNQUACER_PCI_SEG0_PORTIO_MIN +

>> +                   SYNQUACER_PCI_SEG0_PORTIO_OFFSET)) &&

>> +      (Address <= (SYNQUACER_PCI_SEG0_PORTIO_MAX +

>> +                   SYNQUACER_PCI_SEG0_PORTIO_OFFSET))) {

>> +    Address += SYNQUACER_PCI_SEG0_PORTIO_MEMBASE -

>> +               SYNQUACER_PCI_SEG0_PORTIO_OFFSET;

>> +  } else if ((Address >= (SYNQUACER_PCI_SEG1_PORTIO_MIN +

>> +                          SYNQUACER_PCI_SEG1_PORTIO_OFFSET)) &&

>> +             (Address <= (SYNQUACER_PCI_SEG1_PORTIO_MAX +

>> +                          SYNQUACER_PCI_SEG1_PORTIO_OFFSET))) {

>> +    Address += SYNQUACER_PCI_SEG1_PORTIO_MEMBASE -

>> +               SYNQUACER_PCI_SEG1_PORTIO_OFFSET;

>>

>>    } else {

>>      ASSERT (FALSE);

>> --

>> 2.17.1

>>

>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patch

diff --git a/Silicon/Socionext/SynQuacer/Drivers/SynQuacerPciCpuIo2Dxe/SynQuacerPciCpuIo2Dxe.c b/Silicon/Socionext/SynQuacer/Drivers/SynQuacerPciCpuIo2Dxe/SynQuacerPciCpuIo2Dxe.c
index 736b20cd5129..e5cc3aef908d 100644
--- a/Silicon/Socionext/SynQuacer/Drivers/SynQuacerPciCpuIo2Dxe/SynQuacerPciCpuIo2Dxe.c
+++ b/Silicon/Socionext/SynQuacer/Drivers/SynQuacerPciCpuIo2Dxe/SynQuacerPciCpuIo2Dxe.c
@@ -518,12 +518,18 @@  CpuIoServiceWrite (
     return Status;
   }
 
-  if ((Address >= SYNQUACER_PCI_SEG0_PORTIO_MIN) &&
-      (Address <= SYNQUACER_PCI_SEG0_PORTIO_MAX)) {
-    Address += SYNQUACER_PCI_SEG0_PORTIO_MEMBASE;
-  } else if ((Address >= SYNQUACER_PCI_SEG1_PORTIO_MIN) &&
-             (Address <= SYNQUACER_PCI_SEG1_PORTIO_MAX)) {
-    Address += SYNQUACER_PCI_SEG1_PORTIO_MEMBASE;
+  if ((Address >= (SYNQUACER_PCI_SEG0_PORTIO_MIN +
+                   SYNQUACER_PCI_SEG0_PORTIO_OFFSET)) &&
+      (Address <= (SYNQUACER_PCI_SEG0_PORTIO_MAX +
+                   SYNQUACER_PCI_SEG0_PORTIO_OFFSET))) {
+    Address += SYNQUACER_PCI_SEG0_PORTIO_MEMBASE -
+               SYNQUACER_PCI_SEG0_PORTIO_OFFSET;
+  } else if ((Address >= (SYNQUACER_PCI_SEG1_PORTIO_MIN +
+                          SYNQUACER_PCI_SEG1_PORTIO_OFFSET)) &&
+             (Address <= (SYNQUACER_PCI_SEG1_PORTIO_MAX +
+                          SYNQUACER_PCI_SEG1_PORTIO_OFFSET))) {
+    Address += SYNQUACER_PCI_SEG1_PORTIO_MEMBASE -
+               SYNQUACER_PCI_SEG1_PORTIO_OFFSET;
 
   } else {
     ASSERT (FALSE);