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

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