Re: [PATCH] ArmPkg/ArmDmaLib: add support for fixed host-to-device DMA offset

[Leif Lindholm <leif.lindholm@linaro.org>]

On Thu, Nov 10, 2016 at 01:31:38AM +0400, Ard Biesheuvel wrote:
> 
> 
> > On 10 Nov 2016, at 01:13, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > 
> >> On Tue, Nov 08, 2016 at 01:04:55PM +0000, Ard Biesheuvel wrote:
> >> Some devices, such as the Raspberry Pi3, have a fixed offset between memory
> >> addresses as seen by the host and as seen by the other bus masters. So add
> >> a new PCD that allows this fixed offset to be recorded, and to be used when
> >> returning device addresses from the DmaLib mapping routines.
> >> 
> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> ---
> >> ArmPkg/ArmPkg.dec                      |  6 ++++++
> >> ArmPkg/Library/ArmDmaLib/ArmDmaLib.c   | 13 +++++++++++--
> >> ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf |  1 +
> >> 3 files changed, 18 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
> >> index 3cdb5da3d4f3..b2d59a168337 100644
> >> --- a/ArmPkg/ArmPkg.dec
> >> +++ b/ArmPkg/ArmPkg.dec
> >> @@ -134,6 +134,12 @@ [PcdsFixedAtBuild.common]
> >>   gArmTokenSpaceGuid.PcdFdSize|0|UINT32|0x0000002C
> >>   gArmTokenSpaceGuid.PcdFvSize|0|UINT32|0x0000002E
> >> 
> >> +  #
> >> +  # Value to add to a host address to obtain a device address
> >> +  # (using unsigned 64-bit integer arithmetic on both ARM and AArch64)
> >> +  #
> >> +  gArmTokenSpaceGuid.PcdArmDmaDeviceOffset|0x0|UINT64|0x0000044
> >> +
> > 
> > Is the offset always positive?
> > 
> 
> No, hence the comment (which apparently deserves some clarification)
> 
> The idea is that, even on a pure 32-bit system, a negative offset
> can be expressed by wrapping unsigned integer arithmetic, e.g.,
> 0xffffffff_fffff000 for -4 kB

Ah, ok. Yes, I can use something more obvious in the comment anyway :)
Although I think the word 'wrap' making an appearance would have been
sufficient.

/
    Leif

> >> [PcdsFixedAtBuild.common, PcdsPatchableInModule.common]
> >>   gArmTokenSpaceGuid.PcdFdBaseAddress|0|UINT64|0x0000002B
> >>   gArmTokenSpaceGuid.PcdFvBaseAddress|0|UINT64|0x0000002D
> >> diff --git a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
> >> index d48d6ff6dbbb..d6dee78a254d 100644
> >> --- a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
> >> +++ b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
> >> @@ -39,6 +39,15 @@ typedef struct {
> >> EFI_CPU_ARCH_PROTOCOL      *gCpu;
> >> UINTN                      gCacheAlignment = 0;
> >> 
> >> +STATIC
> >> +PHYSICAL_ADDRESS
> >> +HostToDeviceAddress (
> >> +  IN  PHYSICAL_ADDRESS  HostAddress
> >> +  )
> >> +{
> >> +  return HostAddress + PcdGet64 (PcdArmDmaDeviceOffset);
> >> +}
> >> +
> >> /**
> >>   Provides the DMA controller-specific addresses needed to access system memory.
> >> 
> >> @@ -82,7 +91,7 @@ DmaMap (
> >>     return EFI_INVALID_PARAMETER;
> >>   }
> >> 
> >> -  *DeviceAddress = ConvertToPhysicalAddress (HostAddress);
> >> +  *DeviceAddress = HostToDeviceAddress (ConvertToPhysicalAddress (HostAddress));
> >> 
> >>   // Remember range so we can flush on the other side
> >>   Map = AllocatePool (sizeof (MAP_INFO_INSTANCE));
> >> @@ -129,7 +138,7 @@ DmaMap (
> >>         CopyMem (Buffer, HostAddress, *NumberOfBytes);
> >>       }
> >> 
> >> -      *DeviceAddress = (PHYSICAL_ADDRESS)(UINTN)Buffer;
> >> +      *DeviceAddress = HostToDeviceAddress ((PHYSICAL_ADDRESS)(UINTN)Buffer);
> >>     } else {
> >>       Map->DoubleBuffer  = FALSE;
> >>     }
> >> diff --git a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf
> >> index 95c13006eaac..36c23f486974 100644
> >> --- a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf
> >> +++ b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf
> >> @@ -46,6 +46,7 @@ [Protocols]
> >> [Guids]
> >> 
> >> [Pcd]
> >> +  gArmTokenSpaceGuid.PcdArmDmaDeviceOffset
> >> 
> >> [Depex]
> >>   gEfiCpuArchProtocolGuid
> >> -- 
> >> 2.7.4
> >>