HI Jeremy, Some comments below.
On 14 November 2016 at 21:09, Jeremy Linton <[email protected]> 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 <[email protected]> > --- > .../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 > [email protected] > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
