From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: Jeremy Linton <jeremy.linton@arm.com>
Cc: edk2-devel-01 <edk2-devel@lists.01.org>,
Steve Capper <steve.capper@arm.com>,
Leif Lindholm <leif.lindholm@linaro.org>,
Ryan Harkin <ryan.harkin@linaro.org>,
linaro-uefi <linaro-uefi@lists.linaro.org>
Subject: Re: [PATCH 4/7] EmbeddedPkg: SiI3132: Break out FIS command submission
Date: Tue, 15 Nov 2016 17:10:02 +0000 [thread overview]
Message-ID: <CAKv+Gu_XLdnw5S+hMzycugkiVXmAWHFpH5ZijaaGntfenfQQ7g@mail.gmail.com> (raw)
In-Reply-To: <1479157789-14674-5-git-send-email-jeremy.linton@arm.com>
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
next prev parent reply other threads:[~2016-11-15 17:09 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-11-14 21:09 [PATCH 0/8] ATAPI support on SiI SATA adapter Jeremy Linton
2016-11-14 21:09 ` [PATCH 1/7] MdePkg IndustryStandard/Scsi.h: Add sense code macro Jeremy Linton
2016-11-15 7:38 ` Ard Biesheuvel
2016-11-15 10:51 ` Gao, Liming
2016-11-14 21:09 ` [PATCH 2/7] EmbeddedPkg: SiI3132: Add ScsiProtocol callbacks Jeremy Linton
2016-11-15 16:04 ` Ard Biesheuvel
2016-11-14 21:09 ` [PATCH 3/7] EmbeddedPkg: SiI3132: Add SCSI protocol support to header Jeremy Linton
2016-11-14 21:24 ` Jeremy Linton
2016-11-15 17:02 ` Ard Biesheuvel
2016-11-16 15:44 ` Ryan Harkin
2016-11-14 21:09 ` [PATCH 4/7] EmbeddedPkg: SiI3132: Break out FIS command submission Jeremy Linton
2016-11-15 17:10 ` Ard Biesheuvel [this message]
2016-11-14 21:09 ` [PATCH 5/7] EmbeddedPkg: SiI3132: Cleanup device node creation Jeremy Linton
2016-11-14 21:09 ` [PATCH 6/7] EmbeddedPkg: SiI3132: Enable SCSI pass-through protocol Jeremy Linton
2016-11-14 21:09 ` [PATCH 7/7] EmbeddedPkg: SiI3132: Correct the IoAlign Jeremy Linton
2016-11-14 21:09 ` [PATCH] Platforms/ARM/Juno: Add SCSI pass-through protocol Jeremy Linton
2016-11-15 7:43 ` [PATCH 0/8] ATAPI support on SiI SATA adapter Ard Biesheuvel
2016-11-15 14:54 ` Jeremy Linton
2016-11-15 14:57 ` Ard Biesheuvel
2016-11-15 15:05 ` Jeremy Linton
2016-11-15 15:10 ` Ard Biesheuvel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAKv+Gu_XLdnw5S+hMzycugkiVXmAWHFpH5ZijaaGntfenfQQ7g@mail.gmail.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox