[PATCH 0/2] Juno PCI fixes

[PATCH 0/2] Juno PCI fixes

[Daniil Egranov]

The patches fixing the case where driver is using PCI DAC 
and replacing PCI DMA related calls to the ArmDmaLib with 
their Null implementations as the PCI on Juno is DMA coherent.

Daniil Egranov (2):
  ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe: Remove dependency
    on ArmDmaLib
  ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe: Fix for PCI Dual
    Address Cycle

 .../ArmJunoPkg/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf     | 2 +-
 .../ArmJunoPkg/Drivers/PciHostBridgeDxe/PciRootBridge.c          | 9 ++++++---
 2 files changed, 7 insertions(+), 4 deletions(-)

-- 
2.7.4

[PATCH 1/2] ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe: Remove calls to ArmDmaLib

[Daniil Egranov]

The PCI on Juno is DMA coherent, which means it should not be 
using ArmDmaLib for PCI DMA.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Daniil Egranov <daniil.egranov@arm.com>
---
 ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf b/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf
index de28c80..597154c 100644
--- a/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf
+++ b/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf
@@ -36,7 +36,7 @@
   MemoryAllocationLib
   DxeServicesTableLib
   CacheMaintenanceLib
-  DmaLib
+  NullDmaLib
 
 [Sources]
   PciHostBridge.c
-- 
2.7.4

[PATCH 2/2] ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe: Fix for PCI Dual Address Cycle

[Daniil Egranov]

The fix handles the PCI Dual Address Cycle Attribute case. It allows
drivers using PCI DAC run on Juno. 

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Daniil Egranov <daniil.egranov@arm.com>
---
 .../ArmJunoPkg/Drivers/PciHostBridgeDxe/PciRootBridge.c          | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciRootBridge.c b/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciRootBridge.c
index 10a4575..72d0915 100644
--- a/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciRootBridge.c
+++ b/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciRootBridge.c
@@ -518,11 +518,14 @@ PciRbMap (
 
   PCI_TRACE ("PciRbMap()");
 
-  if (Operation == EfiPciOperationBusMasterRead) {
+  if (Operation == EfiPciOperationBusMasterRead ||
+      Operation == EfiPciOperationBusMasterRead64) {
     DmaOperation = MapOperationBusMasterRead;
-  } else if (Operation == EfiPciOperationBusMasterWrite) {
+  } else if (Operation == EfiPciOperationBusMasterWrite ||
+             Operation == EfiPciOperationBusMasterWrite64) {
     DmaOperation = MapOperationBusMasterWrite;
-  } else if (Operation == EfiPciOperationBusMasterCommonBuffer) {
+  } else if (Operation == EfiPciOperationBusMasterCommonBuffer ||
+             Operation == EfiPciOperationBusMasterCommonBuffer64) {
     DmaOperation = MapOperationBusMasterCommonBuffer;
   } else {
     return EFI_INVALID_PARAMETER;
-- 
2.7.4

Re: [PATCH 1/2] ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe: Remove calls to ArmDmaLib

[Ard Biesheuvel]

Hi Daniil,

On 22 September 2016 at 23:33, Daniil Egranov <daniil.egranov@arm.com> wrote:
> The PCI on Juno is DMA coherent, which means it should not be
> using ArmDmaLib for PCI DMA.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Daniil Egranov <daniil.egranov@arm.com>
> ---
>  ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf b/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf
> index de28c80..597154c 100644
> --- a/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf
> +++ b/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf
> @@ -36,7 +36,7 @@
>    MemoryAllocationLib
>    DxeServicesTableLib
>    CacheMaintenanceLib
> -  DmaLib
> +  NullDmaLib
>

This is wrong. The module .inf lists library *classes* and the
platform .dsc decides how each class maps onto an implementation (aka
library resolution)

IOW, the other patch you sent that updates ARM Juno's .dsc is sufficient.

Re: [PATCH 2/2] ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe: Fix for PCI Dual Address Cycle

[Ard Biesheuvel]

On 22 September 2016 at 23:33, Daniil Egranov <daniil.egranov@arm.com> wrote:
> The fix handles the PCI Dual Address Cycle Attribute case. It allows
> drivers using PCI DAC run on Juno.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Daniil Egranov <daniil.egranov@arm.com>

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

> ---
>  .../ArmJunoPkg/Drivers/PciHostBridgeDxe/PciRootBridge.c          | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciRootBridge.c b/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciRootBridge.c
> index 10a4575..72d0915 100644
> --- a/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciRootBridge.c
> +++ b/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciRootBridge.c
> @@ -518,11 +518,14 @@ PciRbMap (
>
>    PCI_TRACE ("PciRbMap()");
>
> -  if (Operation == EfiPciOperationBusMasterRead) {
> +  if (Operation == EfiPciOperationBusMasterRead ||
> +      Operation == EfiPciOperationBusMasterRead64) {
>      DmaOperation = MapOperationBusMasterRead;
> -  } else if (Operation == EfiPciOperationBusMasterWrite) {
> +  } else if (Operation == EfiPciOperationBusMasterWrite ||
> +             Operation == EfiPciOperationBusMasterWrite64) {
>      DmaOperation = MapOperationBusMasterWrite;
> -  } else if (Operation == EfiPciOperationBusMasterCommonBuffer) {
> +  } else if (Operation == EfiPciOperationBusMasterCommonBuffer ||
> +             Operation == EfiPciOperationBusMasterCommonBuffer64) {
>      DmaOperation = MapOperationBusMasterCommonBuffer;
>    } else {
>      return EFI_INVALID_PARAMETER;
> --
> 2.7.4
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 1/2] ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe: Remove calls to ArmDmaLib

[Daniil Egranov]

Hi Ard,


On 09/23/2016 02:57 AM, Ard Biesheuvel wrote:
> Hi Daniil,
>
> On 22 September 2016 at 23:33, Daniil Egranov <daniil.egranov@arm.com> wrote:
>> The PCI on Juno is DMA coherent, which means it should not be
>> using ArmDmaLib for PCI DMA.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Daniil Egranov <daniil.egranov@arm.com>
>> ---
>>   ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf b/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf
>> index de28c80..597154c 100644
>> --- a/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf
>> +++ b/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf
>> @@ -36,7 +36,7 @@
>>     MemoryAllocationLib
>>     DxeServicesTableLib
>>     CacheMaintenanceLib
>> -  DmaLib
>> +  NullDmaLib
>>
> This is wrong. The module .inf lists library *classes* and the
> platform .dsc decides how each class maps onto an implementation (aka
> library resolution)
Agree. However, this is platform specific module and as i understand, 
the behavior of it will not change so, I think, having NullDmaLib here 
will be appropriate as well. We can move it to .dsc file, if it fits 
better to the platform description/module dependencies structure.

> IOW, the other patch you sent that updates ARM Juno's .dsc is sufficient.
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 1/2] ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe: Remove calls to ArmDmaLib

[Ard Biesheuvel]

On 23 September 2016 at 17:20, Daniil Egranov <daniil.egranov@arm.com> wrote:
> Hi Ard,
>
>
> On 09/23/2016 02:57 AM, Ard Biesheuvel wrote:
>>
>> Hi Daniil,
>>
>> On 22 September 2016 at 23:33, Daniil Egranov <daniil.egranov@arm.com>
>> wrote:
>>>
>>> The PCI on Juno is DMA coherent, which means it should not be
>>> using ArmDmaLib for PCI DMA.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Daniil Egranov <daniil.egranov@arm.com>
>>> ---
>>>   ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf
>>> | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git
>>> a/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf
>>> b/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf
>>> index de28c80..597154c 100644
>>> ---
>>> a/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf
>>> +++
>>> b/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf
>>> @@ -36,7 +36,7 @@
>>>     MemoryAllocationLib
>>>     DxeServicesTableLib
>>>     CacheMaintenanceLib
>>> -  DmaLib
>>> +  NullDmaLib
>>>
>> This is wrong. The module .inf lists library *classes* and the
>> platform .dsc decides how each class maps onto an implementation (aka
>> library resolution)
>
> Agree. However, this is platform specific module and as i understand, the
> behavior of it will not change so, I think, having NullDmaLib here will be
> appropriate as well. We can move it to .dsc file, if it fits better to the
> platform description/module dependencies structure.
>

What you could do is remove the DmaLib dependency altogether, and just
implement PciIo->Map and PciIo->Unmap directly, and always return the
host address.

Re: [PATCH 1/2] ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe: Remove calls to ArmDmaLib

[Daniil Egranov]

On 09/23/2016 11:21 AM, Ard Biesheuvel wrote:
> On 23 September 2016 at 17:20, Daniil Egranov <daniil.egranov@arm.com> wrote:
>> Hi Ard,
>>
>>
>> On 09/23/2016 02:57 AM, Ard Biesheuvel wrote:
>>> Hi Daniil,
>>>
>>> On 22 September 2016 at 23:33, Daniil Egranov <daniil.egranov@arm.com>
>>> wrote:
>>>> The PCI on Juno is DMA coherent, which means it should not be
>>>> using ArmDmaLib for PCI DMA.
>>>>
>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>> Signed-off-by: Daniil Egranov <daniil.egranov@arm.com>
>>>> ---
>>>>    ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf
>>>> | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git
>>>> a/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf
>>>> b/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf
>>>> index de28c80..597154c 100644
>>>> ---
>>>> a/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf
>>>> +++
>>>> b/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf
>>>> @@ -36,7 +36,7 @@
>>>>      MemoryAllocationLib
>>>>      DxeServicesTableLib
>>>>      CacheMaintenanceLib
>>>> -  DmaLib
>>>> +  NullDmaLib
>>>>
>>> This is wrong. The module .inf lists library *classes* and the
>>> platform .dsc decides how each class maps onto an implementation (aka
>>> library resolution)
>> Agree. However, this is platform specific module and as i understand, the
>> behavior of it will not change so, I think, having NullDmaLib here will be
>> appropriate as well. We can move it to .dsc file, if it fits better to the
>> platform description/module dependencies structure.
>>
> What you could do is remove the DmaLib dependency altogether, and just
> implement PciIo->Map and PciIo->Unmap directly, and always return the
> host address.
This implementation will be almost the same as NullDmaLib one :). If you 
think it will be better structure-wise to remap it in .dsc file, let's 
go this path and disregard this patch.

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

Re: [PATCH 1/2] ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe: Remove calls to ArmDmaLib

[Ard Biesheuvel]

On 23 September 2016 at 22:47, Daniil Egranov <daniil.egranov@arm.com> wrote:
>
>
> On 09/23/2016 11:21 AM, Ard Biesheuvel wrote:
>>
>> On 23 September 2016 at 17:20, Daniil Egranov <daniil.egranov@arm.com>
>> wrote:
>>>
>>> Hi Ard,
>>>
>>>
>>> On 09/23/2016 02:57 AM, Ard Biesheuvel wrote:
>>>>
>>>> Hi Daniil,
>>>>
>>>> On 22 September 2016 at 23:33, Daniil Egranov <daniil.egranov@arm.com>
>>>> wrote:
>>>>>
>>>>> The PCI on Juno is DMA coherent, which means it should not be
>>>>> using ArmDmaLib for PCI DMA.
>>>>>
>>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>>> Signed-off-by: Daniil Egranov <daniil.egranov@arm.com>
>>>>> ---
>>>>>
>>>>> ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf
>>>>> | 2 +-
>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git
>>>>>
>>>>> a/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf
>>>>>
>>>>> b/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf
>>>>> index de28c80..597154c 100644
>>>>> ---
>>>>>
>>>>> a/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf
>>>>> +++
>>>>>
>>>>> b/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf
>>>>> @@ -36,7 +36,7 @@
>>>>>      MemoryAllocationLib
>>>>>      DxeServicesTableLib
>>>>>      CacheMaintenanceLib
>>>>> -  DmaLib
>>>>> +  NullDmaLib
>>>>>
>>>> This is wrong. The module .inf lists library *classes* and the
>>>> platform .dsc decides how each class maps onto an implementation (aka
>>>> library resolution)
>>>
>>> Agree. However, this is platform specific module and as i understand, the
>>> behavior of it will not change so, I think, having NullDmaLib here will
>>> be
>>> appropriate as well. We can move it to .dsc file, if it fits better to
>>> the
>>> platform description/module dependencies structure.
>>>
>> What you could do is remove the DmaLib dependency altogether, and just
>> implement PciIo->Map and PciIo->Unmap directly, and always return the
>> host address.
>
> This implementation will be almost the same as NullDmaLib one :). If you
> think it will be better structure-wise to remap it in .dsc file, let's go
> this path and disregard this patch.
>

The [LibraryClasses] section expects library classes, and NullDmaLib
is not a library class. So putting this here is simply wrong. This is
not a matter of taste.

Also, another platform could potentially use the same PCIe IP and
integrate it in a non-coherent fashion, so it is not a property of the
device if it is coherent or not.

So forget about my suggestion to drop the DmaLib dependency. Just add
NullDmaLib to the .dsc, but for this driver only.

Re: [PATCH 2/2] ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe: Fix for PCI Dual Address Cycle

[Ryan Harkin]

On 23 September 2016 at 08:58, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 22 September 2016 at 23:33, Daniil Egranov <daniil.egranov@arm.com> wrote:
>> The fix handles the PCI Dual Address Cycle Attribute case. It allows
>> drivers using PCI DAC run on Juno.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Daniil Egranov <daniil.egranov@arm.com>
>
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>

Tested on Juno R0/1/2, where R0 doesn't use the Marvell ethernet
controller, but the SMC ethernet still works.

Tested-by: Ryan Harkin <ryan.harkin@linaro.org>

>> ---
>>  .../ArmJunoPkg/Drivers/PciHostBridgeDxe/PciRootBridge.c          | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciRootBridge.c b/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciRootBridge.c
>> index 10a4575..72d0915 100644
>> --- a/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciRootBridge.c
>> +++ b/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciRootBridge.c
>> @@ -518,11 +518,14 @@ PciRbMap (
>>
>>    PCI_TRACE ("PciRbMap()");
>>
>> -  if (Operation == EfiPciOperationBusMasterRead) {
>> +  if (Operation == EfiPciOperationBusMasterRead ||
>> +      Operation == EfiPciOperationBusMasterRead64) {
>>      DmaOperation = MapOperationBusMasterRead;
>> -  } else if (Operation == EfiPciOperationBusMasterWrite) {
>> +  } else if (Operation == EfiPciOperationBusMasterWrite ||
>> +             Operation == EfiPciOperationBusMasterWrite64) {
>>      DmaOperation = MapOperationBusMasterWrite;
>> -  } else if (Operation == EfiPciOperationBusMasterCommonBuffer) {
>> +  } else if (Operation == EfiPciOperationBusMasterCommonBuffer ||
>> +             Operation == EfiPciOperationBusMasterCommonBuffer64) {
>>      DmaOperation = MapOperationBusMasterCommonBuffer;
>>    } else {
>>      return EFI_INVALID_PARAMETER;
>> --
>> 2.7.4
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 2/2] ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe: Fix for PCI Dual Address Cycle

[Ard Biesheuvel]

On 12 October 2016 at 09:37, Ryan Harkin <ryan.harkin@linaro.org> wrote:
> On 23 September 2016 at 08:58, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> On 22 September 2016 at 23:33, Daniil Egranov <daniil.egranov@arm.com> wrote:
>>> The fix handles the PCI Dual Address Cycle Attribute case. It allows
>>> drivers using PCI DAC run on Juno.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Daniil Egranov <daniil.egranov@arm.com>
>>
>> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>
>
> Tested on Juno R0/1/2, where R0 doesn't use the Marvell ethernet
> controller, but the SMC ethernet still works.
>
> Tested-by: Ryan Harkin <ryan.harkin@linaro.org>
>

Pushed, thanks.