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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

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.