public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
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


  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