Re: [Linaro-uefi] [edk2] [PATCH 4/7] EmbeddedPkg: SiI3132: Break out FIS command submission

15 Nov 2016

HI Jeremy,
Some comments below.
On 14 November 2016 at 21:09, Jeremy Linton jeremy.linton@arm.com wrote:
...
The existing ATA pass-through routine builds the FIS and handles
submission to the hardware. Break out the FIS submission part
so that it can be utilized by the SCSI pass-through. Also,
tighten up the error handling a bit. Starting with removal
of the ASSERTs on errors. ATAPI like SCSI uses check
conditions to indicate device state changes. So these error
paths can get exercised on CD disk change/etc. Further
we want the clamp the timeouts within a range rather than
spinning forever if the port fails to become ready.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jeremy Linton jeremy.linton@arm.com

.../Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c    | 226 +++++++++++++--------
 OpenPlatformPkg                                    |   2 +-
 2 files changed, 137 insertions(+), 91 deletions(-)

diff --git a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c
index 2fb5fd6..69bbdfe 100644
--- a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c
+++ b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c
@@ -22,7 +22,8 @@ GetSataDevice (
   IN  SATA_SI3132_INSTANCE* SataInstance,
   IN  UINT16 Port,
   IN  UINT16 PortMultiplierPort
-) {

)

+{
   LIST_ENTRY              *List;
   SATA_SI3132_PORT        *SataPort;
   SATA_SI3132_DEVICE      *SataDevice;
@@ -44,6 +45,121 @@ GetSataDevice (
   return NULL;
 }
+UINT32
+SiI3231DeviceReady (

IN SATA_SI3132_PORT *SataPort,
IN EFI_PCI_IO_PROTOCOL *PciIo
)

+{

UINT32                  Value32;
UINT32                  Timeout = SI_DEFAULT_TIMEOUT;

do
{

Braces on same line please
...

SATA_PORT_READ32 (SataPort->RegBase + SII3132_PORT_STATUS_REG, &Value32);
Timeout--;
} while (Timeout && !(Value32 & SII3132_PORT_STATUS_PORTREADY));
if (Timeout == 0)
{

Braces on same line please
...

DEBUG ((DEBUG_WARN, "SiI3132AtaPassThru() Device not ready, try anyway\n"));
//Consider doing a device reset here.
}

return Timeout;

+}



+EFI_STATUS
+SiI3132IssueCommand (

IN SATA_SI3132_PORT *SataPort,
EFI_PCI_IO_PROTOCOL  *PciIo,
IN UINT32 Timeout,
VOID *StatusBlock
)

+{

CONST UINT32 IrqMask = (SII3132_PORT_INT_CMDCOMPL | SII3132_PORT_INT_CMDERR) << 16;
UINT32       Value32, Error;
CONST UINTN  EmptySlot = 0;

Assignments not initializers;
...

EFI_STATUS   Status;

SiI3231DeviceReady(SataPort, PciIo);

Space before (
...

// Clear IRQ
SATA_PORT_WRITE32 (SataPort->RegBase + SII3132_PORT_INTSTATUS_REG, IrqMask);

if (!FeaturePcdGet (PcdSataSiI3132FeatureDirectCommandIssuing)) {
// Indirect Command Issuance

//TODO: Find which slot is free (maybe use the Cmd FIFO)
//SATA_PORT_READ32 (SataPort->RegBase + SII3132_PORT_CMDEXECFIFO_REG, &EmptySlot);
SATA_PORT_WRITE32 (SataPort->RegBase + SII3132_PORT_CMDACTIV_REG + (EmptySlot * 8),
                (UINT32)(SataPort->PhysAddrHostPRB & 0xFFFFFFFF));


SATA_PORT_WRITE32 (SataPort->RegBase + SII3132_PORT_CMDACTIV_REG + (EmptySlot * 8) + 4,
                (UINT32)((SataPort->PhysAddrHostPRB >> 32) & 0xFFFFFFFF));


} else {
// Direct Command Issuance
DEBUG ((DEBUG_ERROR ,"SiI3132AtaPassThru() Untested path\n"));
Status = PciIo->Mem.Write (PciIo, EfiPciIoWidthUint32, 1, // Bar 1
   SataPort->RegBase + (EmptySlot * 0x80),


   sizeof (SATA_SI3132_PRB) / 4,


   SataPort->HostPRB);


ASSERT_EFI_ERROR (Status);

SATA_PORT_WRITE32 (SataPort->RegBase + SII3132_PORT_CMDEXECFIFO_REG, EmptySlot);
}

// Clamp the timeout range
if (Timeout < 1) {
Timeout = SI_DEFAULT_TIMEOUT;
} else if (Timeout > SI_DEFAULT_TIMEOUT) {
Timeout = SI_DEFAULT_TIMEOUT;
}

SATA_PORT_READ32 (SataPort->RegBase + SII3132_PORT_INTSTATUS_REG, &Value32);
while (Timeout && !(Value32 & IrqMask)) {
gBS->Stall (1);
SATA_PORT_READ32 (SataPort->RegBase + SII3132_PORT_INTSTATUS_REG, &Value32);
Timeout--;
}

// Fill Packet Ata Status Block
Status = PciIo->Mem.Read (PciIo, EfiPciIoWidthUint32, 1, // Bar 1
SataPort->RegBase + 0x08,
sizeof (EFI_ATA_STATUS_BLOCK) / 4,
StatusBlock);

if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_ERROR, "SiI3132AtaPassThru() status (%d) block %X %X\n",Status, SataPort->RegBase, StatusBlock));
}

if (Timeout == 0) {
DEBUG ((DEBUG_ERROR, "SiI3132AtaPassThru() Err:Timeout\n"));
// Flush the command, reinit port
SATA_PORT_WRITE32 (SataPort->RegBase + SII3132_PORT_CONTROLSET_REG, SII3132_PORT_CONTROL_INT);
SiI3231DeviceReady(SataPort, PciIo);
Status = EFI_TIMEOUT;

} else if (Value32 & (SII3132_PORT_INT_CMDERR << 16)) {
UINT32 Serror;

Newline please
...

SATA_PORT_READ32 (SataPort->RegBase + SII3132_PORT_CMDERROR_REG, &Error);
SATA_PORT_READ32 (SataPort->RegBase + SII3132_PORT_SERROR_REG, &Serror);
SATA_PORT_WRITE32 (SataPort->RegBase + SII3132_PORT_INTSTATUS_REG, Value32 & 0xFF00); //clear error bits

DEBUG ((DEBUG_INFO, "SiI3132AtaPassThru() CmdErr:0x%X (SiI3132 Err:0x%X) (STATUS: %X ERROR:%X) SERROR=%X\n", Value32, Error,SataPort->HostPRB->Fis.Command,SataPort->HostPRB->Fis.Features,Serror));

// clear port status
SATA_PORT_WRITE32 (SataPort->RegBase + SII3132_PORT_CONTROLSET_REG, SII3132_PORT_CONTROL_INT);
SiI3231DeviceReady(SataPort, PciIo);
Status = EFI_DEVICE_ERROR;

} else if (Value32 & (SII3132_PORT_INT_CMDCOMPL << 16)) {
// Clear Command Complete
SATA_PORT_WRITE32 (SataPort->RegBase + SII3132_PORT_INTSTATUS_REG, SII3132_PORT_INT_CMDCOMPL << 16);
Status = EFI_SUCCESS;

}

return Status;

+}



EFI_STATUS
 SiI3132AtaPassThruCommand (
   IN     SATA_SI3132_INSTANCE             *SataSiI3132Instance,
@@ -58,18 +174,14 @@ SiI3132AtaPassThruCommand (
   UINTN                   InDataBufferLength = 0;
   EFI_PHYSICAL_ADDRESS    PhysOutDataBuffer;
   UINTN                   OutDataBufferLength;

CONST UINTN             EmptySlot = 0;
UINTN                   Control = PRB_CTRL_ATA;
UINTN                   Protocol = 0;
UINT32                  Value32, Error, Timeout = 0;
CONST UINT32            IrqMask = (SII3132_PORT_INT_CMDCOMPL | SII3132_PORT_INT_CMDERR) << 16;


UINTN                   Protocol = PRB_PROT_DEFAULT;
EFI_STATUS              Status;
VOID*                   PciAllocMapping = NULL;
EFI_PCI_IO_PROTOCOL     *PciIo;
PciIo = SataSiI3132Instance->PciIo;
ZeroMem (SataPort->HostPRB, sizeof (SATA_SI3132_PRB));



// Construct Si3132 PRB
 switch (Packet->Protocol) {
 case EFI_ATA_PASS_THRU_PROTOCOL_ATA_HARDWARE_RESET:

@@ -92,7 +204,7 @@ SiI3132AtaPassThruCommand (
   case EFI_ATA_PASS_THRU_PROTOCOL_PIO_DATA_IN:
     // Fixup the size for block transfer. Following UEFI Specification, 'InTransferLength' should
     // be in number of bytes. But for most data transfer commands, the value is in number of blocks

if (Packet->Acb->AtaCommand == ATA_CMD_IDENTIFY_DRIVE) {


 if ( (Packet->Acb->AtaCommand == ATA_CMD_IDENTIFY_DRIVE) || (Packet->Acb->AtaCommand == ATA_CMD_IDENTIFY_DEVICE) ) {
 InDataBufferLength = Packet->InTransferLength;

} else {
     SataDevice = GetSataDevice (SataSiI3132Instance, SataPort->Index, PortMultiplierPort);

@@ -108,6 +220,7 @@ SiI3132AtaPassThruCommand (
                Packet->InDataBuffer, &InDataBufferLength, &PhysInDataBuffer, &PciAllocMapping
                );
     if (EFI_ERROR (Status)) {

   DEBUG ((DEBUG_ERROR, "SiI map() failure %d\n", Status));
 return Status;

}

@@ -121,8 +234,8 @@ SiI3132AtaPassThruCommand (
     CopyMem (&SataPort->HostPRB->Fis, Packet->Acb, sizeof (EFI_ATA_COMMAND_BLOCK));
 // Fixup the FIS


SataPort->HostPRB->Fis.FisType = 0x27; // Register - Host to Device FIS
SataPort->HostPRB->Fis.Control = 1 << 7; // Is a command


SataPort->HostPRB->Fis.FisType = SII_FIS_REGISTER_H2D; // Register - Host to Device FIS
SataPort->HostPRB->Fis.Control = SII_FIS_CONTROL_CMD; // Is a command
if (FeaturePcdGet (PcdSataSiI3132FeaturePMPSupport)) {
  SataPort->HostPRB->Fis.Control |= PortMultiplierPort & 0xFF;
}

@@ -188,83 +301,12 @@ SiI3132AtaPassThruCommand (
   SataPort->HostPRB->Control = Control;
   SataPort->HostPRB->ProtocolOverride = Protocol;

// Clear IRQ
SATA_PORT_WRITE32 (SataPort->RegBase + SII3132_PORT_INTSTATUS_REG, IrqMask);


Status = SiI3132IssueCommand(SataPort, PciIo, Packet->Timeout, Packet->Asb);


if (!FeaturePcdGet (PcdSataSiI3132FeatureDirectCommandIssuing)) {
// Indirect Command Issuance

//TODO: Find which slot is free (maybe use the Cmd FIFO)
//SATA_PORT_READ32 (SataPort->RegBase + SII3132_PORT_CMDEXECFIFO_REG, &EmptySlot);

SATA_PORT_WRITE32 (SataPort->RegBase + SII3132_PORT_CMDACTIV_REG + (EmptySlot * 8),
                (UINT32)(SataPort->PhysAddrHostPRB & 0xFFFFFFFF));


SATA_PORT_WRITE32 (SataPort->RegBase + SII3132_PORT_CMDACTIV_REG + (EmptySlot * 8) + 4,
                (UINT32)((SataPort->PhysAddrHostPRB >> 32) & 0xFFFFFFFF));


} else {
// Direct Command Issuance
Status = PciIo->Mem.Write (PciIo, EfiPciIoWidthUint32, 1, // Bar 1
   SataPort->RegBase + (EmptySlot * 0x80),


   sizeof (SATA_SI3132_PRB) / 4,


   SataPort->HostPRB);


ASSERT_EFI_ERROR (Status);

SATA_PORT_WRITE32 (SataPort->RegBase + SII3132_PORT_CMDEXECFIFO_REG, EmptySlot);
}


-#if 0

// Could need to be implemented if we run multiple command in parallel to know which slot has been completed
SATA_PORT_READ32 (SataPort->RegBase + SII3132_PORT_SLOTSTATUS_REG, &Value32);
Timeout = Packet->Timeout;
while (!Timeout && !Value32) {
gBS->Stall (1);
SATA_PORT_READ32 (SataPort->RegBase + SII3132_PORT_SLOTSTATUS_REG, &Value32);
Timeout--;
}

-#else

SATA_PORT_READ32 (SataPort->RegBase + SII3132_PORT_INTSTATUS_REG, &Value32);
if (!Packet->Timeout) {
while (!(Value32 & IrqMask)) {
 gBS->Stall (1);


 SATA_PORT_READ32 (SataPort->RegBase + SII3132_PORT_INTSTATUS_REG, &Value32);


}
} else {
Timeout = Packet->Timeout;
while (Timeout && !(Value32 & IrqMask)) {
 gBS->Stall (1);


 SATA_PORT_READ32 (SataPort->RegBase + SII3132_PORT_INTSTATUS_REG, &Value32);


 Timeout--;


}
}

-#endif

// Fill Packet Ata Status Block
Status = PciIo->Mem.Read (PciIo, EfiPciIoWidthUint32, 1, // Bar 1
 SataPort->RegBase + 0x08,


 sizeof (EFI_ATA_STATUS_BLOCK) / 4,


 Packet->Asb);


ASSERT_EFI_ERROR (Status);


if ((Packet->Timeout != 0) && (Timeout == 0)) {
DEBUG ((EFI_D_ERROR, "SiI3132AtaPassThru() Err:Timeout\n"));
//ASSERT (0);
return EFI_TIMEOUT;
} else if (Value32 & (SII3132_PORT_INT_CMDERR << 16)) {
SATA_PORT_READ32 (SataPort->RegBase + SII3132_PORT_CMDERROR_REG, &Error);
DEBUG ((EFI_D_ERROR, "SiI3132AtaPassThru() CmdErr:0x%X (SiI3132 Err:0x%X)\n", Value32, Error));
ASSERT (0);
return EFI_DEVICE_ERROR;
} else if (Value32 & (SII3132_PORT_INT_CMDCOMPL << 16)) {
// Clear Command Complete
SATA_PORT_WRITE32 (SataPort->RegBase + SII3132_PORT_INTSTATUS_REG, SII3132_PORT_INT_CMDCOMPL << 16);

if (PciAllocMapping) {
 Status = PciIo->Unmap (PciIo, PciAllocMapping);


 ASSERT (!EFI_ERROR (Status));


}

// If the command was ATA_CMD_IDENTIFY_DRIVE then we need to update the BlockSize
if (Packet->Acb->AtaCommand == ATA_CMD_IDENTIFY_DRIVE) {


if (Status == EFI_SUCCESS)
{

!EFI_ERROR (Status)
...

 // If the command was ATA_CMD_IDENTIFY_DRIVE then we need to update the BlockSize


 if (Packet->Acb->AtaCommand == ATA_CMD_IDENTIFY_DRIVE) {
 ATA_IDENTIFY_DATA *IdentifyData = (ATA_IDENTIFY_DATA*)Packet->InDataBuffer;

 // Get the corresponding Block Device



@@ -279,11 +321,15 @@ SiI3132AtaPassThruCommand (
         SataDevice->BlockSize = 0x200;
       }
     }

return EFI_SUCCESS;
} else {
ASSERT (0);
return EFI_DEVICE_ERROR;
}



if (PciAllocMapping) {
 Status = PciIo->Unmap (PciIo, PciAllocMapping);


 ASSERT (!EFI_ERROR (Status));



We have ASSERT_EFI_ERROR () for this
...

}

return Status;


}
/**
@@ -339,7 +385,7 @@ SiI3132AtaPassThru (
   }
   SataPort = SataDevice->Port;

DEBUG ((EFI_D_INFO, "SiI3132AtaPassThru(%d,%d) : AtaCmd:0x%X Prot:%d\n", Port, PortMultiplierPort,


DEBUG ((DEBUG_VERBOSE, "SiI3132AtaPassThru(%p,%d,%d) : AtaCmd:0x%X Prot:%d\n", SataPort, Port, PortMultiplierPort,
       Packet->Acb->AtaCommand, Packet->Protocol));
return SiI3132AtaPassThruCommand (SataSiI3132Instance, SataPort, PortMultiplierPort, Packet, Event);


diff --git a/OpenPlatformPkg b/OpenPlatformPkg
index 133555d..a072474 160000
--- a/OpenPlatformPkg
+++ b/OpenPlatformPkg
@@ -1 +1 @@
-Subproject commit 133555dca92c9d78fafb0944c6f28c5dab98f2ce
+Subproject commit a072474a3b05ec7f12f2d4db271cc07a0cf7a369
This does not belong in the patch
...
--
2.5.5

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

    

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

Re: [Linaro-uefi] [edk2] [PATCH 4/7] EmbeddedPkg: SiI3132: Break out FIS command submission