From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-x234.google.com (mail-wr0-x234.google.com [IPv6:2a00:1450:400c:c0c::234]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id C62D18042B for ; Fri, 17 Mar 2017 09:31:05 -0700 (PDT) Received: by mail-wr0-x234.google.com with SMTP id l37so55538903wrc.1 for ; Fri, 17 Mar 2017 09:31:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=qvGt3+uRVsm+8Cjc2rH26gr8RFABTPpScEfSJffoLLo=; b=MtfAnvNLZ8cxT8hyzacxEkDZ8jLuu6Lm3hUwaZPw9hUwBAYUQXih7cIRMC5zBlCBs5 3CzGNyaqKBpniXsxGd3sundQrTW7kc8KNhowU1fEAJERFBb928Q4A9f0j8bXLAr8g+XH sqWBmc158CYqkCLPdLz9nJTMjU3bFKtcd47o4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=qvGt3+uRVsm+8Cjc2rH26gr8RFABTPpScEfSJffoLLo=; b=RCvSTVXtE7ptZyPN1eW0Vxi0yKVt14a+OsMmP9SyJbHfx2nLNmEC7gI9akqs2QrQjX rpFNZhSY0MGtraataz6O1U/o4TxdGpFeNEL+cJ5TsRWNeyiZqzotrPtDmKTYl6wGN0vC z7ATIO2J/z8QFdJetlZ72/lTZnTK4fn8FWo64jjbUFX2jJWPzdrCkW003lKaAqr2vlO2 kvehkCtbUn9qC5EvYlCL44bjvmFJHTPvF6+m2fQmrCVhYdCky6zScm0ZhgMF3qD7+oVm YxuaQAMFLGDwrA/+I0RHA/nz4JEDvj6AOB1bv/1taY0PuXpkr4gXHItYeqaLmrFKOfen Y5lw== X-Gm-Message-State: AFeK/H2kFHuwjw6zfbnnpW5ZntCl9If1M96qaDaD1jM6bfkTi9h+DuNiKRX8SSs+Mf47Uoih X-Received: by 10.223.165.17 with SMTP id i17mr14749887wrb.70.1489768263769; Fri, 17 Mar 2017 09:31:03 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id u184sm3183797wmb.29.2017.03.17.09.31.02 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 17 Mar 2017 09:31:02 -0700 (PDT) Date: Fri, 17 Mar 2017 16:31:00 +0000 From: Leif Lindholm To: Jeremy Linton Cc: edk2-devel@lists.01.org, ryan.harkin@linaro.org, linaro-uefi@lists.linaro.org, ard.biesheuvel@linaro.org Message-ID: <20170317163100.GH16034@bivouac.eciton.net> References: <20170307221512.5180-1-jeremy.linton@arm.com> <20170307221512.5180-5-jeremy.linton@arm.com> MIME-Version: 1.0 In-Reply-To: <20170307221512.5180-5-jeremy.linton@arm.com> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [PATCH v4 4/6] EmbeddedPkg: SiI3132: Break out FIS command submission X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 17 Mar 2017 16:31:06 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Tue, Mar 07, 2017 at 04:15:09PM -0600, Jeremy Linton 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 > --- > .../Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c | 225 +++++++++++++-------- > OpenPlatformPkg | 2 +- > 2 files changed, 138 insertions(+), 89 deletions(-) > > diff --git a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c > index 2fb5fd6..601583d 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 > -) { > + ) > +{ Whitespace-only change as part of functional patch. > LIST_ENTRY *List; > SATA_SI3132_PORT *SataPort; > SATA_SI3132_DEVICE *SataDevice; > @@ -44,6 +45,124 @@ GetSataDevice ( > return NULL; > } > > +UINT32 > +SiI3231DeviceReady ( > + IN SATA_SI3132_PORT *SataPort, > + IN EFI_PCI_IO_PROTOCOL *PciIo > + ) > +{ > + UINT32 Value32; > + UINT32 Timeout; > + Timeout = SI_DEFAULT_TIMEOUT; > + > + do { > + SATA_PORT_READ32 (SataPort->RegBase + SII3132_PORT_STATUS_REG, &Value32); > + Timeout--; > + } while (Timeout && !(Value32 & SII3132_PORT_STATUS_PORTREADY)); I would much prefer a blank line here. > + if (Timeout == 0) { > + DEBUG ((DEBUG_WARN, "SiI3132AtaPassThru() Device not ready, try anyway\n")); I guess this code this is just being moved around within the same file, so I'll refrain from commenting on a whole bunch of stuff in this function. > + } 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; > + > + 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 +177,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)); > - Whitespace-only change. > // Construct Si3132 PRB > switch (Packet->Protocol) { > case EFI_ATA_PASS_THRU_PROTOCOL_ATA_HARDWARE_RESET: > @@ -92,7 +207,8 @@ 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) || Why the redundant space after "if ("? > + (Packet->Acb->AtaCommand == ATA_CMD_IDENTIFY_DEVICE) ) { And before ") {"? > InDataBufferLength = Packet->InTransferLength; > } else { > SataDevice = GetSataDevice (SataSiI3132Instance, SataPort->Index, PortMultiplierPort); > @@ -108,6 +224,7 @@ SiI3132AtaPassThruCommand ( > Packet->InDataBuffer, &InDataBufferLength, &PhysInDataBuffer, &PciAllocMapping > ); > if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "SiI map() failure %d\n", Status)); > return Status; > } > > @@ -121,8 +238,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,81 +305,10 @@ SiI3132AtaPassThruCommand ( > SataPort->HostPRB->Control = Control; > SataPort->HostPRB->ProtocolOverride = Protocol; > > - // 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 > - 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)); > - } > + Status = SiI3132IssueCommand (SataPort, PciIo, Packet->Timeout, Packet->Asb); > > + if (!EFI_ERROR (Status)) > + { "{" on line before. > // 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; > @@ -279,11 +325,14 @@ SiI3132AtaPassThruCommand ( > SataDevice->BlockSize = 0x200; > } > } > - return EFI_SUCCESS; > - } else { > - ASSERT (0); This ASSERT is not replaced in the new program flow. Is this intentional? > - return EFI_DEVICE_ERROR; > } > + > + if (PciAllocMapping) { > + Status = PciIo->Unmap (PciIo, PciAllocMapping); > + } > + > + return Status; > + > } > > /** > @@ -339,8 +388,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); > -- > 2.9.3 >