Leendert, et al I added the 'PciEmulation' clause to be sure the new code was used only as a result of explicitly setting PcdAtaPassThruPciEmulation = TRUE. (And yes, I " wanted to be cautious and not break the other uses of this driver")
Leo -----Original Message----- From: Leendert van Doorn [mailto:leend...@paramecium.org] Sent: Friday, March 25, 2016 3:28 AM To: Tian, Feng; Duran, Leo; edk2-devel@lists.01.org Cc: leif.lindh...@linaro.org; ard.biesheu...@linaro.org Subject: Re: [edk2] [PATCH] MdeModulePkg: support AHCI controller using PCI emulation Wow, this is a blast from the past. One of my Friday afternoon hacking projects is seeing the light of day ☺ Not all of this patch is mine though, I didn’t add the if (PciEmulation) clauses and they aren’t needed as far as I remember. The problem that part of this code solves is that Map() can return a different device address than the input host address. The Arm implementation of DmaLib has cases where this can occur, specifically when I/O needs to use uncached memory and the source is in cached memory. The original AHCI driver however used the host address in places where it should be using the device address. This patch fixes that. On x86 this bug wasn’t a problem because the two are always the same but on ARM systems that may not be the case (See ArmPkg/…/DmaLib). My original patch fixed this driver to consistently use device address as returned by Map(). I’m not sure why the if (PciEmulation) clauses were added. Presumably someone wanted to be cautious and not break the other uses of this driver. The way I remember the driver and DmaLib logic that shouldn’t be necessary. Leendert On 3/24/16, 5:50 PM, "Tian, Feng" <feng.t...@intel.com> wrote: >Duran > >Could you explain more about this fix? > >1) What's the meaning of PCI emulation here? >2) What's the difference between normal case and this PCI emulation? >3) Why this PCI emulation case needs to update driver to directly use PCI >address for filling transfer descriptor content? > >Thanks >Feng > >-----Original Message----- >From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of >Leo Duran >Sent: Friday, March 25, 2016 4:30 AM >To: edk2-devel@lists.01.org >Cc: Leo Duran; Leendert van Doorn; leif.lindh...@linaro.org; >ard.biesheu...@linaro.org >Subject: [edk2] [PATCH] MdeModulePkg: support AHCI controller using PCI >emulation > >From: Leendert van Doorn <leend...@paramecium.org> > >Contributed-under: TianoCore Contribution Agreement 1.0 >Signed-off-by: Leo Duran <leo.du...@amd.com> >--- > MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c | 88 +++++++++++++++++----- > .../Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf | 1 + > MdeModulePkg/MdeModulePkg.dec | 6 ++ > 3 files changed, 75 insertions(+), 20 deletions(-) > >diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c >b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c >index f64a340..0c8887c 100644 >--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c >+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c >@@ -529,6 +529,7 @@ AhciBuildCommand ( > UINTN MemAddr; > DATA_64 Data64; > UINT32 Offset; >+ BOOLEAN PciEmulation = PcdGetBool (PcdAtaPassThruPciEmulation); > > // > // Filling the PRDT >@@ -542,22 +543,32 @@ AhciBuildCommand ( > // > ASSERT (PrdtNumber <= 65535); > >- Data64.Uint64 = (UINTN) (AhciRegisters->AhciRFis) + sizeof >(EFI_AHCI_RECEIVED_FIS) * Port; >+ if (PciEmulation) >+ Data64.Uint64 = (UINTN) (AhciRegisters->AhciRFisPciAddr) + sizeof >+ (EFI_AHCI_RECEIVED_FIS) * Port; else >+ Data64.Uint64 = (UINTN) (AhciRegisters->AhciRFis) + sizeof >+ (EFI_AHCI_RECEIVED_FIS) * Port; > > BaseAddr = Data64.Uint64; > > ZeroMem ((VOID *)((UINTN) BaseAddr), sizeof > (EFI_AHCI_RECEIVED_FIS)); > >- ZeroMem (AhciRegisters->AhciCommandTable, sizeof >(EFI_AHCI_COMMAND_TABLE)); >+ if (PciEmulation) >+ ZeroMem (AhciRegisters->AhciCommandTablePciAddr, sizeof >+ (EFI_AHCI_COMMAND_TABLE)); else >+ ZeroMem (AhciRegisters->AhciCommandTable, sizeof >+ (EFI_AHCI_COMMAND_TABLE)); > > CommandFis->AhciCFisPmNum = PortMultiplier; > >- CopyMem (&AhciRegisters->AhciCommandTable->CommandFis, CommandFis, >sizeof (EFI_AHCI_COMMAND_FIS)); >+ if (PciEmulation) >+ CopyMem (&AhciRegisters->AhciCommandTablePciAddr->CommandFis, >+ CommandFis, sizeof (EFI_AHCI_COMMAND_FIS)); else >+ CopyMem (&AhciRegisters->AhciCommandTable->CommandFis, CommandFis, >+ sizeof (EFI_AHCI_COMMAND_FIS)); > > Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH + > EFI_AHCI_PORT_CMD; > if (AtapiCommand != NULL) { > CopyMem ( >- &AhciRegisters->AhciCommandTable->AtapiCmd, >+ PciEmulation ? &AhciRegisters->AhciCommandTablePciAddr->AtapiCmd : >+ &AhciRegisters->AhciCommandTable->AtapiCmd, > AtapiCommand, > AtapiCommandLength > ); >@@ -576,14 +587,26 @@ AhciBuildCommand ( > > for (PrdtIndex = 0; PrdtIndex < PrdtNumber; PrdtIndex++) { > if (RemainedData < EFI_AHCI_MAX_DATA_PER_PRDT) { >- AhciRegisters->AhciCommandTable->PrdtTable[PrdtIndex].AhciPrdtDbc = >(UINT32)RemainedData - 1; >+ if (PciEmulation) >+ >AhciRegisters->AhciCommandTablePciAddr->PrdtTable[PrdtIndex].AhciPrdtDbc = >(UINT32)RemainedData - 1; >+ else >+ >+ AhciRegisters->AhciCommandTable->PrdtTable[PrdtIndex].AhciPrdtDbc = >+ (UINT32)RemainedData - 1; > } else { >- AhciRegisters->AhciCommandTable->PrdtTable[PrdtIndex].AhciPrdtDbc = >EFI_AHCI_MAX_DATA_PER_PRDT - 1; >+ if (PciEmulation) >+ >AhciRegisters->AhciCommandTablePciAddr->PrdtTable[PrdtIndex].AhciPrdtDbc = >EFI_AHCI_MAX_DATA_PER_PRDT - 1; >+ else >+ >+ AhciRegisters->AhciCommandTable->PrdtTable[PrdtIndex].AhciPrdtDbc = >+ EFI_AHCI_MAX_DATA_PER_PRDT - 1; > } > > Data64.Uint64 = (UINT64)MemAddr; >- AhciRegisters->AhciCommandTable->PrdtTable[PrdtIndex].AhciPrdtDba = >Data64.Uint32.Lower32; >- AhciRegisters->AhciCommandTable->PrdtTable[PrdtIndex].AhciPrdtDbau = >Data64.Uint32.Upper32; >+ if (PciEmulation) { >+ >AhciRegisters->AhciCommandTablePciAddr->PrdtTable[PrdtIndex].AhciPrdtDba = >Data64.Uint32.Lower32; >+ >AhciRegisters->AhciCommandTablePciAddr->PrdtTable[PrdtIndex].AhciPrdtDbau = >Data64.Uint32.Upper32; >+ } else { >+ AhciRegisters->AhciCommandTable->PrdtTable[PrdtIndex].AhciPrdtDba = >Data64.Uint32.Lower32; >+ AhciRegisters->AhciCommandTable->PrdtTable[PrdtIndex].AhciPrdtDbau = >Data64.Uint32.Upper32; >+ } >+ > RemainedData -= EFI_AHCI_MAX_DATA_PER_PRDT; > MemAddr += EFI_AHCI_MAX_DATA_PER_PRDT; > } >@@ -592,19 +615,29 @@ AhciBuildCommand ( > // Set the last PRDT to Interrupt On Complete > // > if (PrdtNumber > 0) { >- AhciRegisters->AhciCommandTable->PrdtTable[PrdtNumber - 1].AhciPrdtIoc = >1; >+ if (PciEmulation) >+ AhciRegisters->AhciCommandTablePciAddr->PrdtTable[PrdtNumber - >1].AhciPrdtIoc = 1; >+ else >+ AhciRegisters->AhciCommandTable->PrdtTable[PrdtNumber - >+ 1].AhciPrdtIoc = 1; > } > > CopyMem ( >- (VOID *) ((UINTN) AhciRegisters->AhciCmdList + (UINTN) CommandSlotNumber >* sizeof (EFI_AHCI_COMMAND_LIST)), >+ PciEmulation ? (VOID *) ((UINTN) AhciRegisters->AhciCmdListPciAddr + >(UINTN) CommandSlotNumber * sizeof (EFI_AHCI_COMMAND_LIST)) : >+ (VOID *) ((UINTN) AhciRegisters->AhciCmdList + >+ (UINTN) CommandSlotNumber * sizeof (EFI_AHCI_COMMAND_LIST)), > CommandList, > sizeof (EFI_AHCI_COMMAND_LIST) > ); > > Data64.Uint64 = (UINT64)(UINTN) >AhciRegisters->AhciCommandTablePciAddr; >- AhciRegisters->AhciCmdList[CommandSlotNumber].AhciCmdCtba = >Data64.Uint32.Lower32; >- AhciRegisters->AhciCmdList[CommandSlotNumber].AhciCmdCtbau = >Data64.Uint32.Upper32; >- AhciRegisters->AhciCmdList[CommandSlotNumber].AhciCmdPmp = PortMultiplier; >+ if (PciEmulation) { >+ AhciRegisters->AhciCmdListPciAddr[CommandSlotNumber].AhciCmdCtba = >Data64.Uint32.Lower32; >+ AhciRegisters->AhciCmdListPciAddr[CommandSlotNumber].AhciCmdCtbau = >Data64.Uint32.Upper32; >+ AhciRegisters->AhciCmdListPciAddr[CommandSlotNumber].AhciCmdPmp = >PortMultiplier; >+ } else { >+ AhciRegisters->AhciCmdList[CommandSlotNumber].AhciCmdCtba = >Data64.Uint32.Lower32; >+ AhciRegisters->AhciCmdList[CommandSlotNumber].AhciCmdCtbau = >Data64.Uint32.Upper32; >+ AhciRegisters->AhciCmdList[CommandSlotNumber].AhciCmdPmp = >PortMultiplier; >+ } > > } > >@@ -707,6 +740,7 @@ AhciPioTransfer ( > BOOLEAN InfiniteWait; > BOOLEAN PioFisReceived; > BOOLEAN D2hFisReceived; >+ BOOLEAN PciEmulation = PcdGetBool >(PcdAtaPassThruPciEmulation); > > if (Timeout == 0) { > InfiniteWait = TRUE; >@@ -774,7 +808,10 @@ AhciPioTransfer ( > // > // Check the status and wait the driver sending data > // >- FisBaseAddr = (UINTN)AhciRegisters->AhciRFis + Port * sizeof >(EFI_AHCI_RECEIVED_FIS); >+ if (PciEmulation) >+ FisBaseAddr = (UINTN)AhciRegisters->AhciRFisPciAddr + Port * >+ sizeof (EFI_AHCI_RECEIVED_FIS); else >+ FisBaseAddr = (UINTN)AhciRegisters->AhciRFis + Port * sizeof >+ (EFI_AHCI_RECEIVED_FIS); > > if (Read && (AtapiCommand == 0)) { > // >@@ -814,7 +851,10 @@ AhciPioTransfer ( > break; > } > >- PrdCount = *(volatile UINT32 *) >(&(AhciRegisters->AhciCmdList[0].AhciCmdPrdbc)); >+ if (PciEmulation) >+ PrdCount = *(volatile UINT32 *) >(&(AhciRegisters->AhciCmdListPciAddr[0].AhciCmdPrdbc)); >+ else >+ PrdCount = *(volatile UINT32 *) >+ (&(AhciRegisters->AhciCmdList[0].AhciCmdPrdbc)); > if (PrdCount == DataCount) { > Status = EFI_SUCCESS; > break; >@@ -930,7 +970,6 @@ AhciDmaTransfer ( > EFI_AHCI_COMMAND_LIST CmdList; > UINTN FisBaseAddr; > UINT32 PortTfd; >- > EFI_PCI_IO_PROTOCOL *PciIo; > EFI_TPL OldTpl; > >@@ -1025,9 +1064,12 @@ AhciDmaTransfer ( > } > > // >- // Wait for command compelte >+ // Wait for command complete > // >- FisBaseAddr = (UINTN)AhciRegisters->AhciRFis + Port * sizeof >(EFI_AHCI_RECEIVED_FIS); >+ if (PcdGetBool (PcdAtaPassThruPciEmulation)) >+ FisBaseAddr = (UINTN)AhciRegisters->AhciRFisPciAddr + Port * >+ sizeof (EFI_AHCI_RECEIVED_FIS); else >+ FisBaseAddr = (UINTN)AhciRegisters->AhciRFis + Port * sizeof >+ (EFI_AHCI_RECEIVED_FIS); > Offset = FisBaseAddr + EFI_AHCI_D2H_FIS_OFFSET; > if (Task != NULL) { > // >@@ -1175,7 +1217,10 @@ AhciNonDataTransfer ( > // > // Wait device sends the Response Fis > // >- FisBaseAddr = (UINTN)AhciRegisters->AhciRFis + Port * sizeof >(EFI_AHCI_RECEIVED_FIS); >+ if (PcdGetBool (PcdAtaPassThruPciEmulation)) >+ FisBaseAddr = (UINTN)AhciRegisters->AhciRFisPciAddr + Port * >+ sizeof (EFI_AHCI_RECEIVED_FIS); else >+ FisBaseAddr = (UINTN)AhciRegisters->AhciRFis + Port * sizeof >+ (EFI_AHCI_RECEIVED_FIS); > Offset = FisBaseAddr + EFI_AHCI_D2H_FIS_OFFSET; > Status = AhciWaitMemSet ( > Offset, >@@ -1544,7 +1589,10 @@ AhciAtaSmartReturnStatusCheck ( > (EFI_IO_BUS_ATA_ATAPI | EFI_IOB_ATA_BUS_SMART_ENABLE) > ); > >- FisBaseAddr = (UINTN)AhciRegisters->AhciRFis + Port * sizeof >(EFI_AHCI_RECEIVED_FIS); >+ if (PcdGetBool (PcdAtaPassThruPciEmulation)) >+ FisBaseAddr = (UINTN)AhciRegisters->AhciRFisPciAddr + Port * >+ sizeof (EFI_AHCI_RECEIVED_FIS); else >+ FisBaseAddr = (UINTN)AhciRegisters->AhciRFis + Port * sizeof >+ (EFI_AHCI_RECEIVED_FIS); > > Value = *(UINT32 *) (FisBaseAddr + EFI_AHCI_D2H_FIS_OFFSET); > >diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf >b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf >index 82d5f7a..e999a14 100644 >--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf >+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf >@@ -70,6 +70,7 @@ > > [Pcd] > gEfiMdeModulePkgTokenSpaceGuid.PcdAtaSmartEnable ## SOMETIMES_CONSUMES >+ gEfiMdeModulePkgTokenSpaceGuid.PcdAtaPassThruPciEmulation ## >+ CONSUMES > > # [Event] > # EVENT_TYPE_PERIODIC_TIMER ## SOMETIMES_CONSUMES diff --git >a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec index >61db352..5f96cf1 100644 >--- a/MdeModulePkg/MdeModulePkg.dec >+++ b/MdeModulePkg/MdeModulePkg.dec >@@ -1401,6 +1401,12 @@ > # @Prompt Enable ATA S.M.A.R.T feature. > >gEfiMdeModulePkgTokenSpaceGuid.PcdAtaSmartEnable|TRUE|BOOLEAN|0x0001006 >5 > >+ ## Indicates if PCI emulation is enabled for ATA PassThru.<BR><BR> >+ # TRUE - PCI emulation for ATA PassThru is enabled.<BR> >+ # FALSE - PCI emulation for ATA PassThru is disabled.<BR> >+ # @Prompt Enable PCI emulation for ATA PassThru. >+ >+ gEfiMdeModulePkgTokenSpaceGuid.PcdAtaPassThruPciEmulation|FALSE|BOOLE >+ A >+ N|0x00010080 >+ > ## Indicates if full PCI enumeration is disabled.<BR><BR> > # TRUE - Full PCI enumeration is disabled.<BR> > # FALSE - Full PCI enumeration is not disabled.<BR> >-- >1.9.1 > >_______________________________________________ >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
