[PATCH 0/8] ATAPI support on SiI SATA adapter

[PATCH 0/8] ATAPI support on SiI SATA adapter

[Jeremy Linton]

The SiI isn't an AHCI compatible adapter so it implements the EFI ATA
pass-through protocol directly. This works for fixed hard drives, but
not ATAPI attached devices (CDROM, DVDROM, TAPE, etc).

This patch adds read only ATAPI support via the EFI SCSI pass-through
protocol, allowing boot from attached CD/DVD. This patch also cleans
up, and tweaks recovery paths/etc in the original driver. When
combined with the ARM/PCI dma lib changes this allows us to relax the
IO alignment requirement that caused grub failures.

Finally, the OpenPlatformPkg/Juno must be updated, with another patch
to avoid build breaks now that the SiI has a dependency on the SCSI
libraries.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>

Jeremy Linton (7):
  MdePkg IndustryStandard/Scsi.h: Add sense code macro
  EmbeddedPkg: SiI3132: Add ScsiProtocol callbacks
  EmbeddedPkg: SiI3132: Add SCSI protocol support to header
  EmbeddedPkg: SiI3132: Break out FIS command submission
  EmbeddedPkg: SiI3132: Cleanup device node creation
  EmbeddedPkg: SiI3132: Enable SCSI pass-through protocol
  EmbeddedPkg: SiI3132: Correct the IoAlign

 EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c   |  48 ++-
 EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h   |  89 ++++-
 .../Drivers/SataSiI3132Dxe/SataSiI3132Dxe.inf      |   2 +
 .../Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c    | 270 ++++++++------
 .../Drivers/SataSiI3132Dxe/SiI3132ScsiPassThru.c   | 401 +++++++++++++++++++++
 MdePkg/Include/IndustryStandard/Scsi.h             |   2 +
 OpenPlatformPkg                                    |   2 +-
 7 files changed, 688 insertions(+), 126 deletions(-)
 create mode 100644 EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132ScsiPassThru.c

-- 
2.5.5

[PATCH 1/7] MdePkg IndustryStandard/Scsi.h: Add sense code macro

[Jeremy Linton]

Add some definitions to mask the sense key from sense data,
and check the validity of the returned sense data.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 MdePkg/Include/IndustryStandard/Scsi.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MdePkg/Include/IndustryStandard/Scsi.h b/MdePkg/Include/IndustryStandard/Scsi.h
index 0d81314..802479e 100644
--- a/MdePkg/Include/IndustryStandard/Scsi.h
+++ b/MdePkg/Include/IndustryStandard/Scsi.h
@@ -369,6 +369,8 @@ typedef struct {
 //
 // Sense Key
 //
+#define EFI_SCSI_REQUEST_SENSE_ERROR  (0x70)
+#define EFI_SCSI_SK_VALUE(byte)       (byte&0x0F)
 #define EFI_SCSI_SK_NO_SENSE          (0x0)
 #define EFI_SCSI_SK_RECOVERY_ERROR    (0x1)
 #define EFI_SCSI_SK_NOT_READY         (0x2)
-- 
2.5.5

Re: [PATCH 1/7] MdePkg IndustryStandard/Scsi.h: Add sense code macro

[Ard Biesheuvel]

(+ Liming)

On 14 November 2016 at 21:09, Jeremy Linton <jeremy.linton@arm.com> wrote:
> Add some definitions to mask the sense key from sense data,
> and check the validity of the returned sense data.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
>  MdePkg/Include/IndustryStandard/Scsi.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/MdePkg/Include/IndustryStandard/Scsi.h b/MdePkg/Include/IndustryStandard/Scsi.h
> index 0d81314..802479e 100644
> --- a/MdePkg/Include/IndustryStandard/Scsi.h
> +++ b/MdePkg/Include/IndustryStandard/Scsi.h
> @@ -369,6 +369,8 @@ typedef struct {
>  //
>  // Sense Key
>  //
> +#define EFI_SCSI_REQUEST_SENSE_ERROR  (0x70)
> +#define EFI_SCSI_SK_VALUE(byte)       (byte&0x0F)

If you fix the white space

Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>  #define EFI_SCSI_SK_NO_SENSE          (0x0)
>  #define EFI_SCSI_SK_RECOVERY_ERROR    (0x1)
>  #define EFI_SCSI_SK_NOT_READY         (0x2)
> --
> 2.5.5
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 1/7] MdePkg IndustryStandard/Scsi.h: Add sense code macro

[Gao, Liming]

Reviewed-by: Liming Gao <liming.gao@intel.com>

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Jeremy Linton
Sent: Tuesday, November 15, 2016 5:10 AM
To: edk2-devel@lists.01.org
Cc: steve.capper@arm.com; leif.lindholm@linaro.org; ryan.harkin@linaro.org; linaro-uefi@lists.linaro.org
Subject: [edk2] [PATCH 1/7] MdePkg IndustryStandard/Scsi.h: Add sense code macro

Add some definitions to mask the sense key from sense data,
and check the validity of the returned sense data.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 MdePkg/Include/IndustryStandard/Scsi.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MdePkg/Include/IndustryStandard/Scsi.h b/MdePkg/Include/IndustryStandard/Scsi.h
index 0d81314..802479e 100644
--- a/MdePkg/Include/IndustryStandard/Scsi.h
+++ b/MdePkg/Include/IndustryStandard/Scsi.h
@@ -369,6 +369,8 @@ typedef struct {
 //
 // Sense Key
 //
+#define EFI_SCSI_REQUEST_SENSE_ERROR  (0x70)
+#define EFI_SCSI_SK_VALUE(byte)       (byte&0x0F)
 #define EFI_SCSI_SK_NO_SENSE          (0x0)
 #define EFI_SCSI_SK_RECOVERY_ERROR    (0x1)
 #define EFI_SCSI_SK_NOT_READY         (0x2)
-- 
2.5.5

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[PATCH 2/7] EmbeddedPkg: SiI3132: Add ScsiProtocol callbacks

[Jeremy Linton]

Create a new module that adds the callbacks to support
the EFI SCSI pass-through protocol. These callbacks
wrap around the existing ATA pass-through callbacks.
In particular the SCSI command submission routine takes
the SCSI command and wraps it with an SATA FIS and
sets the protocol to ATAPI. It then forwards the FIS to
a new routine we will break out of the ATA pass-through
callback that manages the FIS submission to the adapter.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 .../Drivers/SataSiI3132Dxe/SiI3132ScsiPassThru.c   | 401 +++++++++++++++++++++
 1 file changed, 401 insertions(+)
 create mode 100644 EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132ScsiPassThru.c

diff --git a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132ScsiPassThru.c b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132ScsiPassThru.c
new file mode 100644
index 0000000..131b0af
--- /dev/null
+++ b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132ScsiPassThru.c
@@ -0,0 +1,401 @@
+/** @file
+*  ATAPI support for the Silicon Image I3132
+*
+*  Copyright (c) 2016, ARM Limited. All rights reserved.
+*
+*  This program and the accompanying materials
+*  are licensed and made available under the terms and conditions of the BSD License
+*  which accompanies this distribution.  The full text of the license may be found at
+*  http://opensource.org/licenses/bsd-license.php
+*
+*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+*
+**/
+
+
+#include "SataSiI3132.h"
+
+#include <IndustryStandard/Atapi.h>
+#include <IndustryStandard/Scsi.h>
+#include <Library/MemoryAllocationLib.h>
+
+EFI_STATUS
+SiI3132IDiscoverAtapi (
+  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This,
+  IN int Port
+  )
+{
+  SATA_SI3132_INSTANCE  *SataInstance = INSTANCE_FROM_SCSIPASSTHRU_THIS(This);
+  SATA_SI3132_PORT      *SataPort = &SataInstance->Ports[Port];
+  EFI_ATA_PASS_THRU_COMMAND_PACKET Packet;
+  ATA_IDENTIFY_DATA     *Data;
+  EFI_STATUS            Status;
+  EFI_ATA_STATUS_BLOCK  *Asb;
+  EFI_ATA_COMMAND_BLOCK *Acb;
+
+  Asb=AllocateAlignedPages(EFI_SIZE_TO_PAGES(sizeof(EFI_ATA_STATUS_BLOCK)), EFI_PAGE_SIZE);
+  Acb=AllocateAlignedPages(EFI_SIZE_TO_PAGES(sizeof(EFI_ATA_COMMAND_BLOCK)), EFI_PAGE_SIZE);
+  Data=AllocateAlignedPages(EFI_SIZE_TO_PAGES(sizeof(ATA_IDENTIFY_DATA)), EFI_PAGE_SIZE);
+  ZeroMem (Acb, sizeof (EFI_ATA_COMMAND_BLOCK));
+  Acb->AtaCommand = ATA_CMD_IDENTIFY_DEVICE;
+  Acb->AtaDeviceHead = (UINT8) (BIT7 | BIT6 | BIT5 );
+
+  ZeroMem (&Packet, sizeof (EFI_ATA_PASS_THRU_COMMAND_PACKET));
+  Packet.Acb=Acb;
+  Packet.Asb=Asb;
+  Packet.InDataBuffer = Data;
+  Packet.InTransferLength = sizeof (ATA_IDENTIFY_DATA);
+  Packet.Protocol = EFI_ATA_PASS_THRU_PROTOCOL_PIO_DATA_IN;
+  Packet.Length   = EFI_ATA_PASS_THRU_LENGTH_BYTES | EFI_ATA_PASS_THRU_LENGTH_SECTOR_COUNT;
+
+  Status = SiI3132AtaPassThruCommand(SataInstance, SataPort, 0, &Packet, NULL);
+  if (EFI_ERROR (Status)) {
+    DEBUG((DEBUG_WARN, "SiI ATAPI IDENTIFY DEVICE FAILURE %d\n", Status));
+  }
+  else
+  {
+    Data->ModelName[39]=0;
+  }
+
+  FreeAlignedPages(Data, EFI_SIZE_TO_PAGES(sizeof(ATA_IDENTIFY_DATA)));
+  FreeAlignedPages(Acb, EFI_SIZE_TO_PAGES(sizeof(EFI_ATA_COMMAND_BLOCK)));
+  FreeAlignedPages(Asb, EFI_SIZE_TO_PAGES(sizeof(EFI_ATA_STATUS_BLOCK)));
+
+  return Status;
+}
+
+EFI_STATUS
+SiI3132ScsiPassRead (
+  IN SATA_SI3132_INSTANCE *SataInstance,
+  IN SATA_SI3132_PORT *SataPort,
+  IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet)
+{
+  EFI_STATUS              Status = EFI_SUCCESS;
+  EFI_PCI_IO_PROTOCOL     *PciIo = SataInstance->PciIo;
+  VOID*                   PciAllocMapping = NULL;
+  EFI_PHYSICAL_ADDRESS    PhysBuffer;
+  UINTN InDataBufferLength = Packet->InTransferLength;
+  VOID *ata_sense=AllocateAlignedPages(EFI_SIZE_TO_PAGES(sizeof(EFI_ATA_STATUS_BLOCK)),
+                                       EFI_PAGE_SIZE);;
+
+  BOOLEAN request_sense=FALSE;
+
+  DEBUG ((DEBUG_VERBOSE, "SiI3132ScsiPassRead() CDB[0]:%X len=%d\n",
+          ((UINT8*)Packet->Cdb)[0], Packet->InTransferLength));
+
+  if (ata_sense) {
+    if (Packet->InTransferLength) {
+      Status = PciIo->Map (SataInstance->PciIo, EfiPciIoOperationBusMasterRead,
+                           Packet->InDataBuffer, &InDataBufferLength,
+                           &PhysBuffer, &PciAllocMapping);
+
+      if (EFI_ERROR (Status)) {
+        DEBUG((DEBUG_ERROR, "SiI map() failure %d\n", Status));
+        return Status;
+      }
+    }
+    else {
+      PhysBuffer=0;
+    }
+    do {
+      // SI "The host driver must populate the area normaly used for the first SGE
+      // with the desired ATAPI command". AKA, put the SCSI CDB itself (not the address)
+      // in the 12 bytes comprising the SGE[0].
+      ZeroMem (&SataPort->HostPRB->Sge[0], sizeof(SATA_SI3132_SGE));
+      CopyMem(&SataPort->HostPRB->Sge[0], (UINT8*)Packet->Cdb, Packet->CdbLength);
+
+      // The SGE for the data buffer
+      SataPort->HostPRB->Sge[1].DataAddressLow = (UINT64)PhysBuffer;
+      SataPort->HostPRB->Sge[1].DataAddressHigh = ((UINT64)(PhysBuffer) >> 32);
+      SataPort->HostPRB->Sge[1].Attributes = SGE_TRM;
+      SataPort->HostPRB->Sge[1].DataCount = Packet->InTransferLength;
+
+      // Create the ATA FIS
+      SataPort->HostPRB->Control = (Packet->InTransferLength ? PRB_CTRL_PKT_READ:0);
+      SataPort->HostPRB->ProtocolOverride = 0;
+
+      // This is an ATA PACKET command, which encapuslates the ATAPI(sorta SCSI) 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
+      SataPort->HostPRB->Fis.Command  = ATA_CMD_PACKET;
+      SataPort->HostPRB->Fis.Features = 1;
+      SataPort->HostPRB->Fis.Fis[0]   = 0;
+      SataPort->HostPRB->Fis.Fis[1]   = (UINT8) (((UINT64)PhysBuffer) & 0x00ff);
+      SataPort->HostPRB->Fis.Fis[2]   = (UINT8) (((UINT64)PhysBuffer) >> 8);
+      SataPort->HostPRB->Fis.Fis[3]   = 0x40;
+
+      // Issue this as an ATA command
+      Status = SiI3132IssueCommand(SataPort, PciIo, Packet->Timeout, ata_sense);
+
+      if (request_sense) {
+        // If we were trying to send a request sense in response to a failure
+        // Check conditions are a normal part of SCSI operation, its expected
+        // that most devices will do a 6/2900 (reset) and 2/2800 (media change)
+        // at startup.
+        UINT8 *sense = (UINT8 *)Packet->SenseData;
+        request_sense=FALSE;
+        Packet->InTransferLength = 0;
+        Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK;
+        Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_CHECK_CONDITION;
+        if ((Packet->SenseDataLength > 13) &&
+            (sense[0] & EFI_SCSI_REQUEST_SENSE_ERROR)) {
+            DEBUG ((DEBUG_INFO, "SiI3132ScsiPassRead() Key %X ASC(Q) %02X%02X\n",
+                    EFI_SCSI_SK_VALUE(sense[2]), sense[12], sense[13]));
+        }
+        Status = EFI_SUCCESS;
+      } else if (Status == EFI_SUCCESS) {
+        Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK;
+        Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
+        ZeroMem (Packet->SenseData, Packet->SenseDataLength);
+        Packet->SenseDataLength = 0;
+      } else if (Status == EFI_TIMEOUT) {
+        SiI3132HwResetPort (SataPort);
+        Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
+        Packet->SenseDataLength = 0;
+        Packet->InTransferLength = 0;
+      }
+      else {
+        // Assume for now, that if we didn't succeed that it was a check condition.
+        // ATAPI can't autosense on SiI, so lets emulate it with an explicit
+        // request sense into the sense buffer.
+        Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
+        // Packet->SenseData
+        if ((request_sense == FALSE) && (Packet->SenseDataLength)) {
+          if (Packet->SenseDataLength >= SI_MAX_SENSE)
+            Packet->SenseDataLength = SI_MAX_SENSE - 1;
+          Packet->CdbLength = 6; //Request Sense is a 6 byte CDB
+          ZeroMem (Packet->Cdb, SI_MAX_CDB);
+          ((char*)Packet->Cdb)[0] = EFI_SCSI_OP_REQUEST_SENSE;
+          ((char*)Packet->Cdb)[4] = Packet->SenseDataLength;
+
+          ZeroMem (SataPort->HostPRB, sizeof (SATA_SI3132_PRB));
+          if (PciAllocMapping) {
+            Status = PciIo->Unmap (PciIo, PciAllocMapping);
+          }
+
+          Packet->InTransferLength = Packet->SenseDataLength;
+          InDataBufferLength = Packet->SenseDataLength;
+          Status = PciIo->Map (SataInstance->PciIo, EfiPciIoOperationBusMasterRead,
+                               Packet->SenseData, &InDataBufferLength, &PhysBuffer, &PciAllocMapping);
+          if (EFI_ERROR (Status)) {
+            DEBUG((DEBUG_ERROR, "SiI map() sense failure %d\n", Status));
+            Packet->SenseDataLength = 0;
+          } else {
+            // Everything seems ok, lets issue a SCSI sense
+            request_sense = TRUE;
+          }
+        }
+      }
+    } while (request_sense);
+
+    if (PciAllocMapping) {
+      PciIo->Unmap (PciIo, PciAllocMapping);
+    }
+  } else {
+    DEBUG ((DEBUG_ERROR, "SCSI PassThru Unable to allocate sense buffer\n"));
+    Status = EFI_OUT_OF_RESOURCES;
+  }
+  return Status;
+}
+
+EFI_STATUS
+SiI3132ScsiPassThru (
+  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This,
+  IN UINT8 *Target,
+  IN UINT64 Lun,
+  IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet,
+  IN EFI_EVENT  Event OPTIONAL
+  )
+{
+  EFI_STATUS              Status = EFI_SUCCESS;
+  SATA_SI3132_INSTANCE    *SataInstance = INSTANCE_FROM_SCSIPASSTHRU_THIS(This);
+  SATA_SI3132_PORT        *SataPort = &SataInstance->Ports[Target[0]];
+
+  ZeroMem (SataPort->HostPRB, sizeof (SATA_SI3132_PRB));
+
+  switch (Packet->DataDirection) {
+    case EFI_EXT_SCSI_DATA_DIRECTION_READ:  {
+        Status = SiI3132ScsiPassRead(SataInstance,SataPort, Packet);
+    }
+    break;
+    case EFI_EXT_SCSI_DATA_DIRECTION_WRITE:
+      // TODO, fill this in if we ever want to connect something
+      // besides a simple CDROM/DVDROM
+    default:
+      DEBUG ((DEBUG_ERROR, "SCSI PassThru Unsupported direction\n"));
+  }
+
+  return Status;
+}
+
+UINT8 mScsiId[TARGET_MAX_BYTES] = {
+  0xFF, 0xFF, 0xFF, 0xFF,
+  0xFF, 0xFF, 0xFF, 0xFF,
+  0xFF, 0xFF, 0xFF, 0xFF,
+  0xFF, 0xFF, 0xFF, 0xFF
+};
+
+// With ATA, there are potentially three levels of addressing: port, portmultiplier, lun
+// although I'm not sure there are any actual multilun devices anymore (cd autoloaders/etc)
+// on ATA the common disk/cdroms generally are single lun devices. So, lets skip the LUN
+// scanning/addressing. Futher, the standard ATAPI spec doesn't support lun's.
+EFI_STATUS
+SiI3132GetNextTargetLun (
+  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This,
+  IN OUT UINT8 **Target,
+  IN OUT UINT64 *Lun
+  )
+{
+  EFI_STATUS Status = EFI_NOT_FOUND;
+  SATA_SI3132_INSTANCE *SataInstance = INSTANCE_FROM_SCSIPASSTHRU_THIS(This);
+  UINT8                 *Target8;
+
+  DEBUG ((DEBUG_INFO, "SCSI GetNextTargetLun L:%d\n",*Lun));
+  if (!SataInstance) {
+    DEBUG ((DEBUG_ERROR, "SCSI GetNextTargetLun no instance\n",Lun));
+    return EFI_INVALID_PARAMETER;
+  }
+
+  if (Target == NULL || Lun == NULL) {
+    return EFI_INVALID_PARAMETER;
+  }
+  if (*Target == NULL) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  if (*Lun > 0) {
+    DEBUG ((DEBUG_ERROR, "SCSI GetNextTargetLun Only supports lun0 at the moment\n",Lun));
+    return EFI_INVALID_PARAMETER;
+  }
+
+  Target8 = *Target;
+
+  // look for first device
+  if (CompareMem(Target8, mScsiId, TARGET_MAX_BYTES) == 0) {
+    BOOLEAN found = FALSE;
+    int Index;
+    Target8  = *Target;
+    for (Index = 0; ((Index < SATA_SII3132_MAXPORT) && (!found)); Index++) {
+      SATA_SI3132_PORT        *SataPort;
+      SataPort = &(SataInstance->Ports[Index]);
+      LIST_ENTRY              *List = SataPort->Devices.ForwardLink;
+      int Multiplier = 0;
+      while ((List != &SataPort->Devices) && (!found)) {
+        SATA_SI3132_DEVICE* SataDevice = (SATA_SI3132_DEVICE*)List;
+        if (SataDevice->Atapi) {
+          found = TRUE;
+          Target8[0] = Index;
+          Target8[1] = Multiplier; //the device on this port (for port multipliers)
+          DEBUG ((DEBUG_INFO, "SCSI GetNextTargetLun found device at %d %d\n",Index,Multiplier));
+          SiI3132IDiscoverAtapi(This, Index);
+
+          Status = EFI_SUCCESS;
+          break;
+        }
+        List = List->ForwardLink;
+        Multiplier++;
+      }
+    }
+  }
+  else {
+      //todo find the next device,
+  }
+
+  return Status;
+}
+
+EFI_STATUS
+SiI3132GetNextTargetLun2 (
+  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This,
+  IN UINT8 *Target,
+  IN UINT64 Lun,
+  IN OUT EFI_DEVICE_PATH_PROTOCOL **DevicePath
+  )
+{
+  EFI_STATUS Status = EFI_SUCCESS;
+  DEBUG ((DEBUG_INFO, "SCSI GetNextTargetLun2\n"));
+  return Status;
+}
+
+EFI_STATUS
+SiI3132ScsiBuildDevicePath (
+  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL               *This,
+  IN UINT8                                         *Target,
+  IN UINT64                                        Lun,
+  IN OUT EFI_DEVICE_PATH_PROTOCOL                  **DevicePath
+  )
+{
+  SATA_SI3132_INSTANCE *SataInstance=INSTANCE_FROM_SCSIPASSTHRU_THIS(This);
+
+  DEBUG ((DEBUG_INFO, "SCSI BuildDevicePath T:%d L:%d\n",*Target,Lun));
+
+  if (Lun<1)
+    return SiI3132BuildDevicePath (&SataInstance->AtaPassThruProtocol,Target[0],Target[1],DevicePath);
+  return EFI_NOT_FOUND;
+}
+
+EFI_STATUS
+SiI3132GetTargetLun (
+  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL               *This,
+  IN EFI_DEVICE_PATH_PROTOCOL                      *DevicePath,
+  OUT UINT8                                        **Target,
+  OUT UINT64                                       *Lun
+  )
+{
+  EFI_STATUS Status = EFI_SUCCESS;
+
+  DEBUG ((DEBUG_INFO, "SCSI GetNextTarget T:%d L:%d\n",*Target,Lun));
+  return Status;
+}
+
+// So normally we would want to do a ATA port reset here (which is generally
+// frowned on with modern SCSI transports (sas, fc, etc) unless all the
+// attached devices are in an error state). But the EFI SCSI protocol isn't
+// specific enough to specify a device for which we want to reset the port.
+// This means we are basically stuck simulating it by resetting all the ports
+// which is bad karma. So lets just claim its unsupported and if we discover
+// that port resets are needed as part of the target/lun reset then consider
+// doing it automatically as part of that path.
+EFI_STATUS
+SiI3132ResetChannel (
+  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This
+  )
+{
+  EFI_STATUS Status = EFI_UNSUPPORTED;
+
+  DEBUG ((DEBUG_ERROR, "SCSI ResetChannel\n"));
+  return Status;
+}
+
+// Just do a device reset here, in the future if we find out that is insufficient
+// try to just reset the SATA port the device is attached to as well.
+EFI_STATUS
+SiI3132ResetTargetLun (
+  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL               *This,
+  IN UINT8                                         *Target,
+  IN UINT64                                        Lun
+  )
+{
+  EFI_STATUS Status = EFI_NOT_FOUND;
+  SATA_SI3132_INSTANCE *SataInstance = INSTANCE_FROM_SCSIPASSTHRU_THIS(This);
+
+  DEBUG ((DEBUG_ERROR, "SCSI ResetTargetLun\n"));
+
+  if (Lun<1)
+    Status = SiI3132ResetDevice (&SataInstance->AtaPassThruProtocol,
+                                 Target[0], Target[1]);
+
+  return Status;
+}
+
+EFI_STATUS
+SiI3132GetNextTarget (
+  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL               *This,
+  IN OUT UINT8                                     **Target
+  )
+{
+  EFI_STATUS Status = EFI_SUCCESS;
+  DEBUG ((DEBUG_VERBOSE, "SCSI GetNextTarget\n"));
+  return Status;
+}
-- 
2.5.5

Re: [PATCH 2/7] EmbeddedPkg: SiI3132: Add ScsiProtocol callbacks

[Ard Biesheuvel]

On 14 November 2016 at 21:09, Jeremy Linton <jeremy.linton@arm.com> wrote:
> Create a new module that adds the callbacks to support
> the EFI SCSI pass-through protocol. These callbacks
> wrap around the existing ATA pass-through callbacks.
> In particular the SCSI command submission routine takes
> the SCSI command and wraps it with an SATA FIS and
> sets the protocol to ATAPI. It then forwards the FIS to
> a new routine we will break out of the ATA pass-through
> callback that manages the FIS submission to the adapter.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
>  .../Drivers/SataSiI3132Dxe/SiI3132ScsiPassThru.c   | 401 +++++++++++++++++++++
>  1 file changed, 401 insertions(+)
>  create mode 100644 EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132ScsiPassThru.c
>
> diff --git a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132ScsiPassThru.c b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132ScsiPassThru.c
> new file mode 100644
> index 0000000..131b0af
> --- /dev/null
> +++ b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132ScsiPassThru.c
> @@ -0,0 +1,401 @@
> +/** @file
> +*  ATAPI support for the Silicon Image I3132
> +*
> +*  Copyright (c) 2016, ARM Limited. All rights reserved.
> +*
> +*  This program and the accompanying materials
> +*  are licensed and made available under the terms and conditions of the BSD License
> +*  which accompanies this distribution.  The full text of the license may be found at
> +*  http://opensource.org/licenses/bsd-license.php
> +*
> +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +*
> +**/
> +
> +
> +#include "SataSiI3132.h"
> +
> +#include <IndustryStandard/Atapi.h>
> +#include <IndustryStandard/Scsi.h>
> +#include <Library/MemoryAllocationLib.h>
> +
> +EFI_STATUS
> +SiI3132IDiscoverAtapi (
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This,
> +  IN int Port

INT32 not int, please

> +  )
> +{
> +  SATA_SI3132_INSTANCE  *SataInstance = INSTANCE_FROM_SCSIPASSTHRU_THIS(This);
> +  SATA_SI3132_PORT      *SataPort = &SataInstance->Ports[Port];

Please replace with assignments.

> +  EFI_ATA_PASS_THRU_COMMAND_PACKET Packet;
> +  ATA_IDENTIFY_DATA     *Data;
> +  EFI_STATUS            Status;
> +  EFI_ATA_STATUS_BLOCK  *Asb;
> +  EFI_ATA_COMMAND_BLOCK *Acb;
> +
> +  Asb=AllocateAlignedPages(EFI_SIZE_TO_PAGES(sizeof(EFI_ATA_STATUS_BLOCK)), EFI_PAGE_SIZE);
> +  Acb=AllocateAlignedPages(EFI_SIZE_TO_PAGES(sizeof(EFI_ATA_COMMAND_BLOCK)), EFI_PAGE_SIZE);
> +  Data=AllocateAlignedPages(EFI_SIZE_TO_PAGES(sizeof(ATA_IDENTIFY_DATA)), EFI_PAGE_SIZE);

Spaces around '='

> +  ZeroMem (Acb, sizeof (EFI_ATA_COMMAND_BLOCK));
> +  Acb->AtaCommand = ATA_CMD_IDENTIFY_DEVICE;
> +  Acb->AtaDeviceHead = (UINT8) (BIT7 | BIT6 | BIT5 );
> +
> +  ZeroMem (&Packet, sizeof (EFI_ATA_PASS_THRU_COMMAND_PACKET));
> +  Packet.Acb=Acb;
> +  Packet.Asb=Asb;

Spaces

> +  Packet.InDataBuffer = Data;
> +  Packet.InTransferLength = sizeof (ATA_IDENTIFY_DATA);
> +  Packet.Protocol = EFI_ATA_PASS_THRU_PROTOCOL_PIO_DATA_IN;
> +  Packet.Length   = EFI_ATA_PASS_THRU_LENGTH_BYTES | EFI_ATA_PASS_THRU_LENGTH_SECTOR_COUNT;
> +
> +  Status = SiI3132AtaPassThruCommand(SataInstance, SataPort, 0, &Packet, NULL);

Space before (

> +  if (EFI_ERROR (Status)) {
> +    DEBUG((DEBUG_WARN, "SiI ATAPI IDENTIFY DEVICE FAILURE %d\n", Status));
> +  }
> +  else
> +  {
> +    Data->ModelName[39]=0;
> +  }
> +
> +  FreeAlignedPages(Data, EFI_SIZE_TO_PAGES(sizeof(ATA_IDENTIFY_DATA)));
> +  FreeAlignedPages(Acb, EFI_SIZE_TO_PAGES(sizeof(EFI_ATA_COMMAND_BLOCK)));
> +  FreeAlignedPages(Asb, EFI_SIZE_TO_PAGES(sizeof(EFI_ATA_STATUS_BLOCK)));

Space before (

> +
> +  return Status;
> +}
> +
> +EFI_STATUS
> +SiI3132ScsiPassRead (
> +  IN SATA_SI3132_INSTANCE *SataInstance,
> +  IN SATA_SI3132_PORT *SataPort,
> +  IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet)
> +{
> +  EFI_STATUS              Status = EFI_SUCCESS;
> +  EFI_PCI_IO_PROTOCOL     *PciIo = SataInstance->PciIo;
> +  VOID*                   PciAllocMapping = NULL;

Assignments not initializers

> +  EFI_PHYSICAL_ADDRESS    PhysBuffer;
> +  UINTN InDataBufferLength = Packet->InTransferLength;
> +  VOID *ata_sense=AllocateAlignedPages(EFI_SIZE_TO_PAGES(sizeof(EFI_ATA_STATUS_BLOCK)),
> +                                       EFI_PAGE_SIZE);;
> +
> +  BOOLEAN request_sense=FALSE;
> +

EDK2 uses CamelCase, not whatever_the_other_thing_is_called

> +  DEBUG ((DEBUG_VERBOSE, "SiI3132ScsiPassRead() CDB[0]:%X len=%d\n",
> +          ((UINT8*)Packet->Cdb)[0], Packet->InTransferLength));
> +
> +  if (ata_sense) {
> +    if (Packet->InTransferLength) {
> +      Status = PciIo->Map (SataInstance->PciIo, EfiPciIoOperationBusMasterRead,
> +                           Packet->InDataBuffer, &InDataBufferLength,
> +                           &PhysBuffer, &PciAllocMapping);
> +
> +      if (EFI_ERROR (Status)) {
> +        DEBUG((DEBUG_ERROR, "SiI map() failure %d\n", Status));

Space before ((

> +        return Status;
> +      }
> +    }
> +    else {
> +      PhysBuffer=0;

Spaces around =

> +    }
> +    do {
> +      // SI "The host driver must populate the area normaly used for the first SGE
> +      // with the desired ATAPI command". AKA, put the SCSI CDB itself (not the address)
> +      // in the 12 bytes comprising the SGE[0].
> +      ZeroMem (&SataPort->HostPRB->Sge[0], sizeof(SATA_SI3132_SGE));
> +      CopyMem(&SataPort->HostPRB->Sge[0], (UINT8*)Packet->Cdb, Packet->CdbLength);
> +
> +      // The SGE for the data buffer
> +      SataPort->HostPRB->Sge[1].DataAddressLow = (UINT64)PhysBuffer;
> +      SataPort->HostPRB->Sge[1].DataAddressHigh = ((UINT64)(PhysBuffer) >> 32);
> +      SataPort->HostPRB->Sge[1].Attributes = SGE_TRM;
> +      SataPort->HostPRB->Sge[1].DataCount = Packet->InTransferLength;
> +
> +      // Create the ATA FIS
> +      SataPort->HostPRB->Control = (Packet->InTransferLength ? PRB_CTRL_PKT_READ:0);
> +      SataPort->HostPRB->ProtocolOverride = 0;
> +
> +      // This is an ATA PACKET command, which encapuslates the ATAPI(sorta SCSI) 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
> +      SataPort->HostPRB->Fis.Command  = ATA_CMD_PACKET;
> +      SataPort->HostPRB->Fis.Features = 1;
> +      SataPort->HostPRB->Fis.Fis[0]   = 0;
> +      SataPort->HostPRB->Fis.Fis[1]   = (UINT8) (((UINT64)PhysBuffer) & 0x00ff);
> +      SataPort->HostPRB->Fis.Fis[2]   = (UINT8) (((UINT64)PhysBuffer) >> 8);
> +      SataPort->HostPRB->Fis.Fis[3]   = 0x40;
> +
> +      // Issue this as an ATA command
> +      Status = SiI3132IssueCommand(SataPort, PciIo, Packet->Timeout, ata_sense);
> +
> +      if (request_sense) {
> +        // If we were trying to send a request sense in response to a failure
> +        // Check conditions are a normal part of SCSI operation, its expected
> +        // that most devices will do a 6/2900 (reset) and 2/2800 (media change)
> +        // at startup.
> +        UINT8 *sense = (UINT8 *)Packet->SenseData;
> +        request_sense=FALSE;
> +        Packet->InTransferLength = 0;
> +        Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK;
> +        Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_CHECK_CONDITION;
> +        if ((Packet->SenseDataLength > 13) &&
> +            (sense[0] & EFI_SCSI_REQUEST_SENSE_ERROR)) {
> +            DEBUG ((DEBUG_INFO, "SiI3132ScsiPassRead() Key %X ASC(Q) %02X%02X\n",
> +                    EFI_SCSI_SK_VALUE(sense[2]), sense[12], sense[13]));
> +        }
> +        Status = EFI_SUCCESS;
> +      } else if (Status == EFI_SUCCESS) {

Please use !EFI_ERROR (Status). I know it means the same thing *now*
but that may change in the future.

> +        Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK;
> +        Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
> +        ZeroMem (Packet->SenseData, Packet->SenseDataLength);
> +        Packet->SenseDataLength = 0;
> +      } else if (Status == EFI_TIMEOUT) {
> +        SiI3132HwResetPort (SataPort);
> +        Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
> +        Packet->SenseDataLength = 0;
> +        Packet->InTransferLength = 0;
> +      }
> +      else {

Same line please

> +        // Assume for now, that if we didn't succeed that it was a check condition.
> +        // ATAPI can't autosense on SiI, so lets emulate it with an explicit
> +        // request sense into the sense buffer.
> +        Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
> +        // Packet->SenseData
> +        if ((request_sense == FALSE) && (Packet->SenseDataLength)) {
> +          if (Packet->SenseDataLength >= SI_MAX_SENSE)
> +            Packet->SenseDataLength = SI_MAX_SENSE - 1;
> +          Packet->CdbLength = 6; //Request Sense is a 6 byte CDB
> +          ZeroMem (Packet->Cdb, SI_MAX_CDB);
> +          ((char*)Packet->Cdb)[0] = EFI_SCSI_OP_REQUEST_SENSE;
> +          ((char*)Packet->Cdb)[4] = Packet->SenseDataLength;

Yuck. Please use a separate UINT8* variable

> +
> +          ZeroMem (SataPort->HostPRB, sizeof (SATA_SI3132_PRB));
> +          if (PciAllocMapping) {
> +            Status = PciIo->Unmap (PciIo, PciAllocMapping);

Why is it OK to ignore Status. And if it is, why do you need to assign
it in the first place?

> +          }
> +
> +          Packet->InTransferLength = Packet->SenseDataLength;
> +          InDataBufferLength = Packet->SenseDataLength;
> +          Status = PciIo->Map (SataInstance->PciIo, EfiPciIoOperationBusMasterRead,
> +                               Packet->SenseData, &InDataBufferLength, &PhysBuffer, &PciAllocMapping);
> +          if (EFI_ERROR (Status)) {
> +            DEBUG((DEBUG_ERROR, "SiI map() sense failure %d\n", Status));
> +            Packet->SenseDataLength = 0;
> +          } else {
> +            // Everything seems ok, lets issue a SCSI sense
> +            request_sense = TRUE;
> +          }
> +        }
> +      }
> +    } while (request_sense);
> +
> +    if (PciAllocMapping) {
> +      PciIo->Unmap (PciIo, PciAllocMapping);
> +    }
> +  } else {
> +    DEBUG ((DEBUG_ERROR, "SCSI PassThru Unable to allocate sense buffer\n"));
> +    Status = EFI_OUT_OF_RESOURCES;
> +  }
> +  return Status;
> +}
> +
> +EFI_STATUS
> +SiI3132ScsiPassThru (
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This,
> +  IN UINT8 *Target,
> +  IN UINT64 Lun,
> +  IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet,
> +  IN EFI_EVENT  Event OPTIONAL
> +  )
> +{
> +  EFI_STATUS              Status = EFI_SUCCESS;
> +  SATA_SI3132_INSTANCE    *SataInstance = INSTANCE_FROM_SCSIPASSTHRU_THIS(This);
> +  SATA_SI3132_PORT        *SataPort = &SataInstance->Ports[Target[0]];
> +
> +  ZeroMem (SataPort->HostPRB, sizeof (SATA_SI3132_PRB));
> +
> +  switch (Packet->DataDirection) {
> +    case EFI_EXT_SCSI_DATA_DIRECTION_READ:  {
> +        Status = SiI3132ScsiPassRead(SataInstance,SataPort, Packet);
> +    }
> +    break;
> +    case EFI_EXT_SCSI_DATA_DIRECTION_WRITE:
> +      // TODO, fill this in if we ever want to connect something
> +      // besides a simple CDROM/DVDROM
> +    default:
> +      DEBUG ((DEBUG_ERROR, "SCSI PassThru Unsupported direction\n"));
> +  }
> +
> +  return Status;
> +}
> +
> +UINT8 mScsiId[TARGET_MAX_BYTES] = {
> +  0xFF, 0xFF, 0xFF, 0xFF,
> +  0xFF, 0xFF, 0xFF, 0xFF,
> +  0xFF, 0xFF, 0xFF, 0xFF,
> +  0xFF, 0xFF, 0xFF, 0xFF
> +};
> +
> +// With ATA, there are potentially three levels of addressing: port, portmultiplier, lun
> +// although I'm not sure there are any actual multilun devices anymore (cd autoloaders/etc)
> +// on ATA the common disk/cdroms generally are single lun devices. So, lets skip the LUN
> +// scanning/addressing. Futher, the standard ATAPI spec doesn't support lun's.
> +EFI_STATUS
> +SiI3132GetNextTargetLun (
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This,
> +  IN OUT UINT8 **Target,
> +  IN OUT UINT64 *Lun
> +  )
> +{
> +  EFI_STATUS Status = EFI_NOT_FOUND;
> +  SATA_SI3132_INSTANCE *SataInstance = INSTANCE_FROM_SCSIPASSTHRU_THIS(This);
> +  UINT8                 *Target8;
> +
> +  DEBUG ((DEBUG_INFO, "SCSI GetNextTargetLun L:%d\n",*Lun));
> +  if (!SataInstance) {
> +    DEBUG ((DEBUG_ERROR, "SCSI GetNextTargetLun no instance\n",Lun));
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if (Target == NULL || Lun == NULL) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +  if (*Target == NULL) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if (*Lun > 0) {
> +    DEBUG ((DEBUG_ERROR, "SCSI GetNextTargetLun Only supports lun0 at the moment\n",Lun));
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  Target8 = *Target;
> +
> +  // look for first device
> +  if (CompareMem(Target8, mScsiId, TARGET_MAX_BYTES) == 0) {
> +    BOOLEAN found = FALSE;
> +    int Index;
> +    Target8  = *Target;
> +    for (Index = 0; ((Index < SATA_SII3132_MAXPORT) && (!found)); Index++) {
> +      SATA_SI3132_PORT        *SataPort;
> +      SataPort = &(SataInstance->Ports[Index]);
> +      LIST_ENTRY              *List = SataPort->Devices.ForwardLink;
> +      int Multiplier = 0;
> +      while ((List != &SataPort->Devices) && (!found)) {
> +        SATA_SI3132_DEVICE* SataDevice = (SATA_SI3132_DEVICE*)List;
> +        if (SataDevice->Atapi) {
> +          found = TRUE;
> +          Target8[0] = Index;
> +          Target8[1] = Multiplier; //the device on this port (for port multipliers)
> +          DEBUG ((DEBUG_INFO, "SCSI GetNextTargetLun found device at %d %d\n",Index,Multiplier));
> +          SiI3132IDiscoverAtapi(This, Index);
> +
> +          Status = EFI_SUCCESS;
> +          break;
> +        }
> +        List = List->ForwardLink;
> +        Multiplier++;
> +      }
> +    }
> +  }
> +  else {
> +      //todo find the next device,
> +  }
> +
> +  return Status;
> +}
> +
> +EFI_STATUS
> +SiI3132GetNextTargetLun2 (
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This,
> +  IN UINT8 *Target,
> +  IN UINT64 Lun,
> +  IN OUT EFI_DEVICE_PATH_PROTOCOL **DevicePath
> +  )
> +{
> +  EFI_STATUS Status = EFI_SUCCESS;
> +  DEBUG ((DEBUG_INFO, "SCSI GetNextTargetLun2\n"));
> +  return Status;
> +}
> +
> +EFI_STATUS
> +SiI3132ScsiBuildDevicePath (
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL               *This,
> +  IN UINT8                                         *Target,
> +  IN UINT64                                        Lun,
> +  IN OUT EFI_DEVICE_PATH_PROTOCOL                  **DevicePath
> +  )
> +{
> +  SATA_SI3132_INSTANCE *SataInstance=INSTANCE_FROM_SCSIPASSTHRU_THIS(This);
> +
> +  DEBUG ((DEBUG_INFO, "SCSI BuildDevicePath T:%d L:%d\n",*Target,Lun));
> +
> +  if (Lun<1)
> +    return SiI3132BuildDevicePath (&SataInstance->AtaPassThruProtocol,Target[0],Target[1],DevicePath);
> +  return EFI_NOT_FOUND;
> +}
> +
> +EFI_STATUS
> +SiI3132GetTargetLun (
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL               *This,
> +  IN EFI_DEVICE_PATH_PROTOCOL                      *DevicePath,
> +  OUT UINT8                                        **Target,
> +  OUT UINT64                                       *Lun
> +  )
> +{
> +  EFI_STATUS Status = EFI_SUCCESS;
> +
> +  DEBUG ((DEBUG_INFO, "SCSI GetNextTarget T:%d L:%d\n",*Target,Lun));
> +  return Status;
> +}
> +
> +// So normally we would want to do a ATA port reset here (which is generally
> +// frowned on with modern SCSI transports (sas, fc, etc) unless all the
> +// attached devices are in an error state). But the EFI SCSI protocol isn't
> +// specific enough to specify a device for which we want to reset the port.
> +// This means we are basically stuck simulating it by resetting all the ports
> +// which is bad karma. So lets just claim its unsupported and if we discover
> +// that port resets are needed as part of the target/lun reset then consider
> +// doing it automatically as part of that path.
> +EFI_STATUS
> +SiI3132ResetChannel (
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This
> +  )
> +{
> +  EFI_STATUS Status = EFI_UNSUPPORTED;
> +
> +  DEBUG ((DEBUG_ERROR, "SCSI ResetChannel\n"));
> +  return Status;
> +}
> +
> +// Just do a device reset here, in the future if we find out that is insufficient
> +// try to just reset the SATA port the device is attached to as well.
> +EFI_STATUS
> +SiI3132ResetTargetLun (
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL               *This,
> +  IN UINT8                                         *Target,
> +  IN UINT64                                        Lun
> +  )
> +{
> +  EFI_STATUS Status = EFI_NOT_FOUND;
> +  SATA_SI3132_INSTANCE *SataInstance = INSTANCE_FROM_SCSIPASSTHRU_THIS(This);
> +
> +  DEBUG ((DEBUG_ERROR, "SCSI ResetTargetLun\n"));
> +
> +  if (Lun<1)
> +    Status = SiI3132ResetDevice (&SataInstance->AtaPassThruProtocol,
> +                                 Target[0], Target[1]);
> +
> +  return Status;
> +}
> +
> +EFI_STATUS
> +SiI3132GetNextTarget (
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL               *This,
> +  IN OUT UINT8                                     **Target
> +  )
> +{
> +  EFI_STATUS Status = EFI_SUCCESS;
> +  DEBUG ((DEBUG_VERBOSE, "SCSI GetNextTarget\n"));
> +  return Status;
> +}
> --
> 2.5.5
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

[PATCH 3/7] EmbeddedPkg: SiI3132: Add SCSI protocol support to header

[Jeremy Linton]

Add EXT_SCSI_PASS_THRU structures to SI3132_PORT structure,
along with helpers and new entry points.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h | 89 +++++++++++++++++++++++-
 1 file changed, 87 insertions(+), 2 deletions(-)

diff --git a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h
index f23446a..91f9448 100644
--- a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h
+++ b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h
@@ -20,6 +20,7 @@
 
 #include <Protocol/AtaPassThru.h>
 #include <Protocol/PciIo.h>
+#include <Protocol/ScsiPassThruExt.h>
 
 #include <Library/UefiLib.h>
 #include <Library/DebugLib.h>
@@ -57,6 +58,7 @@
 #define SII3132_PORT_SLOTSTATUS_REG             0x1800
 #define SII3132_PORT_CMDACTIV_REG               0x1C00
 #define SII3132_PORT_SSTATUS_REG                0x1F04
+#define SII3132_PORT_SERROR_REG                 0x1F08
 
 #define SII3132_PORT_CONTROL_RESET              (1 << 0)
 #define SII3132_PORT_DEVICE_RESET               (1 << 1)
@@ -81,6 +83,7 @@
 #define PRB_CTRL_INT_MASK       0x40
 #define PRB_CTRL_SRST           0x80
 
+#define PRB_PROT_DEFAULT        0x00
 #define PRB_PROT_PACKET         0x01
 #define PRB_PROT_LEGACY_QUEUE   0x02
 #define PRB_PROT_NATIVE_QUEUE   0x04
@@ -88,6 +91,9 @@
 #define PRB_PROT_WRITE          0x10
 #define PRB_PROT_TRANSPARENT    0x20
 
+#define SII_FIS_REGISTER_H2D    0x27      //Register FIS - Host to Device
+#define SII_FIS_CONTROL_CMD     (1 << 7)  //Indicate FIS is a command
+
 #define SGE_XCF     (1 << 28)
 #define SGE_DRD     (1 << 29)
 #define SGE_LNK     (1 << 30)
@@ -95,7 +101,7 @@
 
 #define SI_MAX_CDB         12  //MAX supported CDB
 #define SI_MAX_SENSE       256
-#define SI_DEFAULT_TIMEOUT 20000
+#define SI_DEFAULT_TIMEOUT 50000
 
 
 typedef struct _SATA_SI3132_SGE {
@@ -126,6 +132,8 @@ typedef struct _SATA_SI3132_DEVICE {
     UINTN                       Index;
     struct _SATA_SI3132_PORT    *Port;  //Parent Port
     UINT32                      BlockSize;
+    BOOLEAN                     Atapi; //ATAPI device
+    BOOLEAN                     Cdb16; //Uses 16byte CDB transfers (or 12)
 } SATA_SI3132_DEVICE;
 
 typedef struct _SATA_SI3132_PORT {
@@ -146,13 +154,18 @@ typedef struct _SATA_SI3132_INSTANCE {
 
     SATA_SI3132_PORT            Ports[SATA_SII3132_MAXPORT];
 
-    EFI_ATA_PASS_THRU_PROTOCOL  AtaPassThruProtocol;
+    EFI_ATA_PASS_THRU_MODE            AtaPassThruMode;
+    EFI_ATA_PASS_THRU_PROTOCOL        AtaPassThruProtocol;
+    EFI_EXT_SCSI_PASS_THRU_MODE       ExtScsiPassThruMode;
+    EFI_EXT_SCSI_PASS_THRU_PROTOCOL   ExtScsiPassThru;
+
 
     EFI_PCI_IO_PROTOCOL         *PciIo;
 } SATA_SI3132_INSTANCE;
 
 #define SATA_SII3132_SIGNATURE              SIGNATURE_32('s', 'i', '3', '2')
 #define INSTANCE_FROM_ATAPASSTHRU_THIS(a)   CR(a, SATA_SI3132_INSTANCE, AtaPassThruProtocol, SATA_SII3132_SIGNATURE)
+#define INSTANCE_FROM_SCSIPASSTHRU_THIS(a)  CR(a, SATA_SI3132_INSTANCE, ExtScsiPassThru, SATA_SII3132_SIGNATURE)
 
 #define SATA_GLOBAL_READ32(Offset, Value)  PciIo->Mem.Read (PciIo, EfiPciIoWidthUint32, 0, Offset, 1, Value)
 #define SATA_GLOBAL_WRITE32(Offset, Value) { UINT32 Value32 = Value; PciIo->Mem.Write (PciIo, EfiPciIoWidthUint32, 0, Offset, 1, &Value32); }
@@ -271,4 +284,76 @@ EFI_STATUS SiI3132ResetDevice (
   IN UINT16                     PortMultiplierPort
   );
 
+/**
+ * EFI ATA Pass Thru Entry points for SCSI Protocol
+ */
+SATA_SI3132_DEVICE* GetSataDevice (
+  IN  SATA_SI3132_INSTANCE *SataInstance,
+  IN  UINT16                Port,
+  IN  UINT16                PortMultiplierPort
+  );
+
+
+EFI_STATUS SiI3132IssueCommand(
+  IN SATA_SI3132_PORT *SataPort,
+  EFI_PCI_IO_PROTOCOL *PciIo,
+  IN UINT32            Timeout,
+  VOID                *StatusBlock
+  );
+
+
+
+/**
+ * EFI SCSI Pass Thru Protocol
+ */
+EFI_STATUS SiI3132ScsiPassThru(
+  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This,
+  IN UINT8 *Target,
+  IN UINT64 Lun,
+  IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet,
+  IN EFI_EVENT  Event OPTIONAL
+  );
+
+EFI_STATUS SiI3132GetNextTargetLun(
+  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This,
+  IN OUT UINT8 **Target,
+  IN OUT UINT64 *Lun
+);
+
+EFI_STATUS SiI3132GetNextTargetLun2(
+  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This,
+  IN UINT8 *Target,
+  IN UINT64 Lun,
+  IN OUT EFI_DEVICE_PATH_PROTOCOL **DevicePath
+  );
+
+EFI_STATUS SiI3132ScsiBuildDevicePath(
+  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL               *This,
+  IN UINT8                                         *Target,
+  IN UINT64                                        Lun,
+  IN OUT EFI_DEVICE_PATH_PROTOCOL                  **DevicePath
+  );
+
+EFI_STATUS SiI3132GetTargetLun (
+  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL               *This,
+  IN EFI_DEVICE_PATH_PROTOCOL                      *DevicePath,
+  OUT UINT8                                        **Target,
+  OUT UINT64                                       *Lun
+  );
+
+EFI_STATUS SiI3132ResetChannel(
+  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This
+  );
+
+EFI_STATUS SiI3132ResetTargetLun(
+  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL               *This,
+  IN UINT8                                         *Target,
+  IN UINT64                                        Lun
+  );
+
+EFI_STATUS SiI3132GetNextTarget(
+  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL               *This,
+  IN OUT UINT8                                     **Target
+  );
+
 #endif
-- 
2.5.5

Re: [PATCH 3/7] EmbeddedPkg: SiI3132: Add SCSI protocol support to header

[Jeremy Linton]

On 11/14/2016 03:09 PM, Jeremy Linton wrote:
> Add EXT_SCSI_PASS_THRU structures to SI3132_PORT structure,
> along with helpers and new entry points.

Of course, I noticed after posting that this patch is missing a prereq, 
that should have been squashed into it.

(see below)

>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
>  EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h | 89 +++++++++++++++++++++++-
>  1 file changed, 87 insertions(+), 2 deletions(-)
>
> diff --git a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h
> index f23446a..91f9448 100644
> --- a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h
> +++ b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h
> @@ -20,6 +20,7 @@
>
>  #include <Protocol/AtaPassThru.h>
>  #include <Protocol/PciIo.h>
> +#include <Protocol/ScsiPassThruExt.h>
>
>  #include <Library/UefiLib.h>
>  #include <Library/DebugLib.h>
> @@ -57,6 +58,7 @@
>  #define SII3132_PORT_SLOTSTATUS_REG             0x1800
>  #define SII3132_PORT_CMDACTIV_REG               0x1C00
>  #define SII3132_PORT_SSTATUS_REG                0x1F04
> +#define SII3132_PORT_SERROR_REG                 0x1F08
>
>  #define SII3132_PORT_CONTROL_RESET              (1 << 0)
>  #define SII3132_PORT_DEVICE_RESET               (1 << 1)
> @@ -81,6 +83,7 @@
>  #define PRB_CTRL_INT_MASK       0x40
>  #define PRB_CTRL_SRST           0x80
>
> +#define PRB_PROT_DEFAULT        0x00
>  #define PRB_PROT_PACKET         0x01
>  #define PRB_PROT_LEGACY_QUEUE   0x02
>  #define PRB_PROT_NATIVE_QUEUE   0x04
> @@ -88,6 +91,9 @@
>  #define PRB_PROT_WRITE          0x10
>  #define PRB_PROT_TRANSPARENT    0x20
>
> +#define SII_FIS_REGISTER_H2D    0x27      //Register FIS - Host to Device
> +#define SII_FIS_CONTROL_CMD     (1 << 7)  //Indicate FIS is a command
> +
>  #define SGE_XCF     (1 << 28)
>  #define SGE_DRD     (1 << 29)
>  #define SGE_LNK     (1 << 30)
> @@ -95,7 +101,7 @@
>
>  #define SI_MAX_CDB         12  //MAX supported CDB
>  #define SI_MAX_SENSE       256
> -#define SI_DEFAULT_TIMEOUT 20000
> +#define SI_DEFAULT_TIMEOUT 50000


Right here this is wrong, all three of these lines should be new

SI_MAX_CDB, SI_MAX_SENSE and SI_DEFAULT_TIMEOUT all need to be declared.

>
>
>  typedef struct _SATA_SI3132_SGE {
> @@ -126,6 +132,8 @@ typedef struct _SATA_SI3132_DEVICE {
>      UINTN                       Index;
>      struct _SATA_SI3132_PORT    *Port;  //Parent Port
>      UINT32                      BlockSize;
> +    BOOLEAN                     Atapi; //ATAPI device
> +    BOOLEAN                     Cdb16; //Uses 16byte CDB transfers (or 12)
>  } SATA_SI3132_DEVICE;
>
>  typedef struct _SATA_SI3132_PORT {
> @@ -146,13 +154,18 @@ typedef struct _SATA_SI3132_INSTANCE {
>
>      SATA_SI3132_PORT            Ports[SATA_SII3132_MAXPORT];
>
> -    EFI_ATA_PASS_THRU_PROTOCOL  AtaPassThruProtocol;
> +    EFI_ATA_PASS_THRU_MODE            AtaPassThruMode;
> +    EFI_ATA_PASS_THRU_PROTOCOL        AtaPassThruProtocol;
> +    EFI_EXT_SCSI_PASS_THRU_MODE       ExtScsiPassThruMode;
> +    EFI_EXT_SCSI_PASS_THRU_PROTOCOL   ExtScsiPassThru;
> +
>
>      EFI_PCI_IO_PROTOCOL         *PciIo;
>  } SATA_SI3132_INSTANCE;
>
>  #define SATA_SII3132_SIGNATURE              SIGNATURE_32('s', 'i', '3', '2')
>  #define INSTANCE_FROM_ATAPASSTHRU_THIS(a)   CR(a, SATA_SI3132_INSTANCE, AtaPassThruProtocol, SATA_SII3132_SIGNATURE)
> +#define INSTANCE_FROM_SCSIPASSTHRU_THIS(a)  CR(a, SATA_SI3132_INSTANCE, ExtScsiPassThru, SATA_SII3132_SIGNATURE)
>
>  #define SATA_GLOBAL_READ32(Offset, Value)  PciIo->Mem.Read (PciIo, EfiPciIoWidthUint32, 0, Offset, 1, Value)
>  #define SATA_GLOBAL_WRITE32(Offset, Value) { UINT32 Value32 = Value; PciIo->Mem.Write (PciIo, EfiPciIoWidthUint32, 0, Offset, 1, &Value32); }
> @@ -271,4 +284,76 @@ EFI_STATUS SiI3132ResetDevice (
>    IN UINT16                     PortMultiplierPort
>    );
>
> +/**
> + * EFI ATA Pass Thru Entry points for SCSI Protocol
> + */
> +SATA_SI3132_DEVICE* GetSataDevice (
> +  IN  SATA_SI3132_INSTANCE *SataInstance,
> +  IN  UINT16                Port,
> +  IN  UINT16                PortMultiplierPort
> +  );
> +
> +
> +EFI_STATUS SiI3132IssueCommand(
> +  IN SATA_SI3132_PORT *SataPort,
> +  EFI_PCI_IO_PROTOCOL *PciIo,
> +  IN UINT32            Timeout,
> +  VOID                *StatusBlock
> +  );
> +
> +
> +
> +/**
> + * EFI SCSI Pass Thru Protocol
> + */
> +EFI_STATUS SiI3132ScsiPassThru(
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This,
> +  IN UINT8 *Target,
> +  IN UINT64 Lun,
> +  IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet,
> +  IN EFI_EVENT  Event OPTIONAL
> +  );
> +
> +EFI_STATUS SiI3132GetNextTargetLun(
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This,
> +  IN OUT UINT8 **Target,
> +  IN OUT UINT64 *Lun
> +);
> +
> +EFI_STATUS SiI3132GetNextTargetLun2(
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This,
> +  IN UINT8 *Target,
> +  IN UINT64 Lun,
> +  IN OUT EFI_DEVICE_PATH_PROTOCOL **DevicePath
> +  );
> +
> +EFI_STATUS SiI3132ScsiBuildDevicePath(
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL               *This,
> +  IN UINT8                                         *Target,
> +  IN UINT64                                        Lun,
> +  IN OUT EFI_DEVICE_PATH_PROTOCOL                  **DevicePath
> +  );
> +
> +EFI_STATUS SiI3132GetTargetLun (
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL               *This,
> +  IN EFI_DEVICE_PATH_PROTOCOL                      *DevicePath,
> +  OUT UINT8                                        **Target,
> +  OUT UINT64                                       *Lun
> +  );
> +
> +EFI_STATUS SiI3132ResetChannel(
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This
> +  );
> +
> +EFI_STATUS SiI3132ResetTargetLun(
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL               *This,
> +  IN UINT8                                         *Target,
> +  IN UINT64                                        Lun
> +  );
> +
> +EFI_STATUS SiI3132GetNextTarget(
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL               *This,
> +  IN OUT UINT8                                     **Target
> +  );
> +
>  #endif
>

Re: [PATCH 3/7] EmbeddedPkg: SiI3132: Add SCSI protocol support to header

[Ard Biesheuvel]

On 14 November 2016 at 21:24, Jeremy Linton <jeremy.linton@arm.com> wrote:
> On 11/14/2016 03:09 PM, Jeremy Linton wrote:
>>
>> Add EXT_SCSI_PASS_THRU structures to SI3132_PORT structure,
>> along with helpers and new entry points.
>
>
> Of course, I noticed after posting that this patch is missing a prereq, that
> should have been squashed into it.
>
> (see below)
>
>
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>> ---
>>  EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h | 89
>> +++++++++++++++++++++++-
>>  1 file changed, 87 insertions(+), 2 deletions(-)
>>
>> diff --git a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h
>> b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h
>> index f23446a..91f9448 100644
>> --- a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h
>> +++ b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h
>> @@ -20,6 +20,7 @@
>>
>>  #include <Protocol/AtaPassThru.h>
>>  #include <Protocol/PciIo.h>
>> +#include <Protocol/ScsiPassThruExt.h>
>>
>>  #include <Library/UefiLib.h>
>>  #include <Library/DebugLib.h>
>> @@ -57,6 +58,7 @@
>>  #define SII3132_PORT_SLOTSTATUS_REG             0x1800
>>  #define SII3132_PORT_CMDACTIV_REG               0x1C00
>>  #define SII3132_PORT_SSTATUS_REG                0x1F04
>> +#define SII3132_PORT_SERROR_REG                 0x1F08
>>
>>  #define SII3132_PORT_CONTROL_RESET              (1 << 0)
>>  #define SII3132_PORT_DEVICE_RESET               (1 << 1)
>> @@ -81,6 +83,7 @@
>>  #define PRB_CTRL_INT_MASK       0x40
>>  #define PRB_CTRL_SRST           0x80
>>
>> +#define PRB_PROT_DEFAULT        0x00
>>  #define PRB_PROT_PACKET         0x01
>>  #define PRB_PROT_LEGACY_QUEUE   0x02
>>  #define PRB_PROT_NATIVE_QUEUE   0x04
>> @@ -88,6 +91,9 @@
>>  #define PRB_PROT_WRITE          0x10
>>  #define PRB_PROT_TRANSPARENT    0x20
>>
>> +#define SII_FIS_REGISTER_H2D    0x27      //Register FIS - Host to Device
>> +#define SII_FIS_CONTROL_CMD     (1 << 7)  //Indicate FIS is a command
>> +
>>  #define SGE_XCF     (1 << 28)
>>  #define SGE_DRD     (1 << 29)
>>  #define SGE_LNK     (1 << 30)
>> @@ -95,7 +101,7 @@
>>
>>  #define SI_MAX_CDB         12  //MAX supported CDB
>>  #define SI_MAX_SENSE       256
>> -#define SI_DEFAULT_TIMEOUT 20000
>> +#define SI_DEFAULT_TIMEOUT 50000
>
>
>
> Right here this is wrong, all three of these lines should be new
>
> SI_MAX_CDB, SI_MAX_SENSE and SI_DEFAULT_TIMEOUT all need to be declared.
>

WIth this fixed (and the redundant newlines removed, especially inside
struct definitions)

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>
>>
>>
>>  typedef struct _SATA_SI3132_SGE {
>> @@ -126,6 +132,8 @@ typedef struct _SATA_SI3132_DEVICE {
>>      UINTN                       Index;
>>      struct _SATA_SI3132_PORT    *Port;  //Parent Port
>>      UINT32                      BlockSize;
>> +    BOOLEAN                     Atapi; //ATAPI device
>> +    BOOLEAN                     Cdb16; //Uses 16byte CDB transfers (or
>> 12)
>>  } SATA_SI3132_DEVICE;
>>
>>  typedef struct _SATA_SI3132_PORT {
>> @@ -146,13 +154,18 @@ typedef struct _SATA_SI3132_INSTANCE {
>>
>>      SATA_SI3132_PORT            Ports[SATA_SII3132_MAXPORT];
>>
>> -    EFI_ATA_PASS_THRU_PROTOCOL  AtaPassThruProtocol;
>> +    EFI_ATA_PASS_THRU_MODE            AtaPassThruMode;
>> +    EFI_ATA_PASS_THRU_PROTOCOL        AtaPassThruProtocol;
>> +    EFI_EXT_SCSI_PASS_THRU_MODE       ExtScsiPassThruMode;
>> +    EFI_EXT_SCSI_PASS_THRU_PROTOCOL   ExtScsiPassThru;
>> +
>>
>>      EFI_PCI_IO_PROTOCOL         *PciIo;
>>  } SATA_SI3132_INSTANCE;
>>
>>  #define SATA_SII3132_SIGNATURE              SIGNATURE_32('s', 'i', '3',
>> '2')
>>  #define INSTANCE_FROM_ATAPASSTHRU_THIS(a)   CR(a, SATA_SI3132_INSTANCE,
>> AtaPassThruProtocol, SATA_SII3132_SIGNATURE)
>> +#define INSTANCE_FROM_SCSIPASSTHRU_THIS(a)  CR(a, SATA_SI3132_INSTANCE,
>> ExtScsiPassThru, SATA_SII3132_SIGNATURE)
>>
>>  #define SATA_GLOBAL_READ32(Offset, Value)  PciIo->Mem.Read (PciIo,
>> EfiPciIoWidthUint32, 0, Offset, 1, Value)
>>  #define SATA_GLOBAL_WRITE32(Offset, Value) { UINT32 Value32 = Value;
>> PciIo->Mem.Write (PciIo, EfiPciIoWidthUint32, 0, Offset, 1, &Value32); }
>> @@ -271,4 +284,76 @@ EFI_STATUS SiI3132ResetDevice (
>>    IN UINT16                     PortMultiplierPort
>>    );
>>
>> +/**
>> + * EFI ATA Pass Thru Entry points for SCSI Protocol
>> + */
>> +SATA_SI3132_DEVICE* GetSataDevice (
>> +  IN  SATA_SI3132_INSTANCE *SataInstance,
>> +  IN  UINT16                Port,
>> +  IN  UINT16                PortMultiplierPort
>> +  );
>> +
>> +
>> +EFI_STATUS SiI3132IssueCommand(
>> +  IN SATA_SI3132_PORT *SataPort,
>> +  EFI_PCI_IO_PROTOCOL *PciIo,
>> +  IN UINT32            Timeout,
>> +  VOID                *StatusBlock
>> +  );
>> +
>> +
>> +
>> +/**
>> + * EFI SCSI Pass Thru Protocol
>> + */
>> +EFI_STATUS SiI3132ScsiPassThru(
>> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This,
>> +  IN UINT8 *Target,
>> +  IN UINT64 Lun,
>> +  IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet,
>> +  IN EFI_EVENT  Event OPTIONAL
>> +  );
>> +
>> +EFI_STATUS SiI3132GetNextTargetLun(
>> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This,
>> +  IN OUT UINT8 **Target,
>> +  IN OUT UINT64 *Lun
>> +);
>> +
>> +EFI_STATUS SiI3132GetNextTargetLun2(
>> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This,
>> +  IN UINT8 *Target,
>> +  IN UINT64 Lun,
>> +  IN OUT EFI_DEVICE_PATH_PROTOCOL **DevicePath
>> +  );
>> +
>> +EFI_STATUS SiI3132ScsiBuildDevicePath(
>> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL               *This,
>> +  IN UINT8                                         *Target,
>> +  IN UINT64                                        Lun,
>> +  IN OUT EFI_DEVICE_PATH_PROTOCOL                  **DevicePath
>> +  );
>> +
>> +EFI_STATUS SiI3132GetTargetLun (
>> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL               *This,
>> +  IN EFI_DEVICE_PATH_PROTOCOL                      *DevicePath,
>> +  OUT UINT8                                        **Target,
>> +  OUT UINT64                                       *Lun
>> +  );
>> +
>> +EFI_STATUS SiI3132ResetChannel(
>> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This
>> +  );
>> +
>> +EFI_STATUS SiI3132ResetTargetLun(
>> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL               *This,
>> +  IN UINT8                                         *Target,
>> +  IN UINT64                                        Lun
>> +  );
>> +
>> +EFI_STATUS SiI3132GetNextTarget(
>> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL               *This,
>> +  IN OUT UINT8                                     **Target
>> +  );
>> +
>>  #endif
>>
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 3/7] EmbeddedPkg: SiI3132: Add SCSI protocol support to header

[Ryan Harkin]

Hi Jeremy,

On 14 November 2016 at 21:09, Jeremy Linton <jeremy.linton@arm.com> wrote:
> Add EXT_SCSI_PASS_THRU structures to SI3132_PORT structure,
> along with helpers and new entry points.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
>  EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h | 89 +++++++++++++++++++++++-
>  1 file changed, 87 insertions(+), 2 deletions(-)
>
> diff --git a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h
> index f23446a..91f9448 100644
> --- a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h
> +++ b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h
> @@ -20,6 +20,7 @@
>
>  #include <Protocol/AtaPassThru.h>
>  #include <Protocol/PciIo.h>
> +#include <Protocol/ScsiPassThruExt.h>
>
>  #include <Library/UefiLib.h>
>  #include <Library/DebugLib.h>
> @@ -57,6 +58,7 @@
>  #define SII3132_PORT_SLOTSTATUS_REG             0x1800
>  #define SII3132_PORT_CMDACTIV_REG               0x1C00
>  #define SII3132_PORT_SSTATUS_REG                0x1F04
> +#define SII3132_PORT_SERROR_REG                 0x1F08
>
>  #define SII3132_PORT_CONTROL_RESET              (1 << 0)
>  #define SII3132_PORT_DEVICE_RESET               (1 << 1)
> @@ -81,6 +83,7 @@
>  #define PRB_CTRL_INT_MASK       0x40
>  #define PRB_CTRL_SRST           0x80
>
> +#define PRB_PROT_DEFAULT        0x00
>  #define PRB_PROT_PACKET         0x01
>  #define PRB_PROT_LEGACY_QUEUE   0x02
>  #define PRB_PROT_NATIVE_QUEUE   0x04
> @@ -88,6 +91,9 @@
>  #define PRB_PROT_WRITE          0x10
>  #define PRB_PROT_TRANSPARENT    0x20
>
> +#define SII_FIS_REGISTER_H2D    0x27      //Register FIS - Host to Device
> +#define SII_FIS_CONTROL_CMD     (1 << 7)  //Indicate FIS is a command
> +
>  #define SGE_XCF     (1 << 28)
>  #define SGE_DRD     (1 << 29)
>  #define SGE_LNK     (1 << 30)
> @@ -95,7 +101,7 @@
>
>  #define SI_MAX_CDB         12  //MAX supported CDB
>  #define SI_MAX_SENSE       256
> -#define SI_DEFAULT_TIMEOUT 20000
> +#define SI_DEFAULT_TIMEOUT 50000
>

This hunk doesn't apply.  The line being removed doesn't exist in the
upstream version of the file.  Have I missed something?

>
>  typedef struct _SATA_SI3132_SGE {
> @@ -126,6 +132,8 @@ typedef struct _SATA_SI3132_DEVICE {
>      UINTN                       Index;
>      struct _SATA_SI3132_PORT    *Port;  //Parent Port
>      UINT32                      BlockSize;
> +    BOOLEAN                     Atapi; //ATAPI device
> +    BOOLEAN                     Cdb16; //Uses 16byte CDB transfers (or 12)
>  } SATA_SI3132_DEVICE;
>
>  typedef struct _SATA_SI3132_PORT {
> @@ -146,13 +154,18 @@ typedef struct _SATA_SI3132_INSTANCE {
>
>      SATA_SI3132_PORT            Ports[SATA_SII3132_MAXPORT];
>
> -    EFI_ATA_PASS_THRU_PROTOCOL  AtaPassThruProtocol;
> +    EFI_ATA_PASS_THRU_MODE            AtaPassThruMode;
> +    EFI_ATA_PASS_THRU_PROTOCOL        AtaPassThruProtocol;
> +    EFI_EXT_SCSI_PASS_THRU_MODE       ExtScsiPassThruMode;
> +    EFI_EXT_SCSI_PASS_THRU_PROTOCOL   ExtScsiPassThru;
> +
>
>      EFI_PCI_IO_PROTOCOL         *PciIo;
>  } SATA_SI3132_INSTANCE;
>
>  #define SATA_SII3132_SIGNATURE              SIGNATURE_32('s', 'i', '3', '2')
>  #define INSTANCE_FROM_ATAPASSTHRU_THIS(a)   CR(a, SATA_SI3132_INSTANCE, AtaPassThruProtocol, SATA_SII3132_SIGNATURE)
> +#define INSTANCE_FROM_SCSIPASSTHRU_THIS(a)  CR(a, SATA_SI3132_INSTANCE, ExtScsiPassThru, SATA_SII3132_SIGNATURE)
>
>  #define SATA_GLOBAL_READ32(Offset, Value)  PciIo->Mem.Read (PciIo, EfiPciIoWidthUint32, 0, Offset, 1, Value)
>  #define SATA_GLOBAL_WRITE32(Offset, Value) { UINT32 Value32 = Value; PciIo->Mem.Write (PciIo, EfiPciIoWidthUint32, 0, Offset, 1, &Value32); }
> @@ -271,4 +284,76 @@ EFI_STATUS SiI3132ResetDevice (
>    IN UINT16                     PortMultiplierPort
>    );
>
> +/**
> + * EFI ATA Pass Thru Entry points for SCSI Protocol
> + */
> +SATA_SI3132_DEVICE* GetSataDevice (
> +  IN  SATA_SI3132_INSTANCE *SataInstance,
> +  IN  UINT16                Port,
> +  IN  UINT16                PortMultiplierPort
> +  );
> +
> +
> +EFI_STATUS SiI3132IssueCommand(
> +  IN SATA_SI3132_PORT *SataPort,
> +  EFI_PCI_IO_PROTOCOL *PciIo,
> +  IN UINT32            Timeout,
> +  VOID                *StatusBlock
> +  );
> +
> +
> +
> +/**
> + * EFI SCSI Pass Thru Protocol
> + */
> +EFI_STATUS SiI3132ScsiPassThru(
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This,
> +  IN UINT8 *Target,
> +  IN UINT64 Lun,
> +  IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet,
> +  IN EFI_EVENT  Event OPTIONAL
> +  );
> +
> +EFI_STATUS SiI3132GetNextTargetLun(
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This,
> +  IN OUT UINT8 **Target,
> +  IN OUT UINT64 *Lun
> +);
> +
> +EFI_STATUS SiI3132GetNextTargetLun2(
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This,
> +  IN UINT8 *Target,
> +  IN UINT64 Lun,
> +  IN OUT EFI_DEVICE_PATH_PROTOCOL **DevicePath
> +  );
> +
> +EFI_STATUS SiI3132ScsiBuildDevicePath(
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL               *This,
> +  IN UINT8                                         *Target,
> +  IN UINT64                                        Lun,
> +  IN OUT EFI_DEVICE_PATH_PROTOCOL                  **DevicePath
> +  );
> +
> +EFI_STATUS SiI3132GetTargetLun (
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL               *This,
> +  IN EFI_DEVICE_PATH_PROTOCOL                      *DevicePath,
> +  OUT UINT8                                        **Target,
> +  OUT UINT64                                       *Lun
> +  );
> +
> +EFI_STATUS SiI3132ResetChannel(
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This
> +  );
> +
> +EFI_STATUS SiI3132ResetTargetLun(
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL               *This,
> +  IN UINT8                                         *Target,
> +  IN UINT64                                        Lun
> +  );
> +
> +EFI_STATUS SiI3132GetNextTarget(
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL               *This,
> +  IN OUT UINT8                                     **Target
> +  );
> +
>  #endif
> --
> 2.5.5
>

[PATCH 4/7] EmbeddedPkg: SiI3132: Break out FIS command submission

[Jeremy Linton]

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
+  {
+    SATA_PORT_READ32 (SataPort->RegBase + SII3132_PORT_STATUS_REG, &Value32);
+    Timeout--;
+  } while (Timeout && !(Value32 & SII3132_PORT_STATUS_PORTREADY));
+  if (Timeout == 0)
+  {
+    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;
+  EFI_STATUS   Status;
+
+  SiI3231DeviceReady(SataPort, PciIo);
+  // 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;
+    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)
+  {
+      // 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));
+  }
+
+  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
-- 
2.5.5

Re: [PATCH 4/7] EmbeddedPkg: SiI3132: Break out FIS command submission

[Ard Biesheuvel]

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

[PATCH 5/7] EmbeddedPkg: SiI3132: Cleanup device node creation

[Jeremy Linton]

There can be either ATA or ATAPI devices connected to
each SATA port. We want to detect if the device is ATA
and create a SATA_DP path or a SCSI_DP for ATAPI devices.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 .../Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c    | 44 ++++++++++++++--------
 1 file changed, 29 insertions(+), 15 deletions(-)

diff --git a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c
index 69bbdfe..d613aee 100644
--- a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c
+++ b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c
@@ -518,16 +518,19 @@ SiI3132GetNextDevice (
   SataPort = &(SataSiI3132Instance->Ports[Port]);
 
   if (*PortMultiplierPort == 0xFFFF) {
+    SATA_TRACE ("SiI3132GetNextDevice() PortMultiplier");
     List = SataPort->Devices.ForwardLink;
-    if (List != &SataPort->Devices) {
+    if ((List != &SataPort->Devices) &&
+        (((SATA_SI3132_DEVICE*)List)->Atapi == FALSE)) {
       // The list is not empty, return the first device
       *PortMultiplierPort = ((SATA_SI3132_DEVICE*)List)->Index;
     } else {
       Status = EFI_NOT_FOUND;
     }
   } else {
+    SATA_TRACE ("SiI3132GetNextDevice()");
     SataDevice = GetSataDevice (SataSiI3132Instance, Port, *PortMultiplierPort);
-    if (SataDevice != NULL) {
+    if ((SataDevice != NULL) && (SataDevice->Atapi == FALSE)) {
       // We have found the previous port multiplier, return the next one
       List = SataDevice->Link.ForwardLink;
       if (List != &SataPort->Devices) {
@@ -598,20 +601,31 @@ SiI3132BuildDevicePath (
     return EFI_NOT_FOUND;
   }
 
-  SiI3132DevicePath = CreateDeviceNode (MESSAGING_DEVICE_PATH, MSG_SATA_DP, sizeof (SATA_DEVICE_PATH));
-  if (SiI3132DevicePath == NULL) {
-    return EFI_OUT_OF_RESOURCES;
-  }
+  if (SataDevice->Atapi) {
+    SiI3132DevicePath = CreateDeviceNode (MESSAGING_DEVICE_PATH, MSG_SCSI_DP, sizeof (SCSI_DEVICE_PATH));
+    if (SiI3132DevicePath == NULL) {
+        return EFI_OUT_OF_RESOURCES;
+    }
+    ((SCSI_DEVICE_PATH*)SiI3132DevicePath)->Pun = Port;
+    ((SCSI_DEVICE_PATH*)SiI3132DevicePath)->Lun = 0;
 
-  ((SATA_DEVICE_PATH*)SiI3132DevicePath)->HBAPortNumber = Port;
-  if (FeaturePcdGet (PcdSataSiI3132FeaturePMPSupport)) {
-    ((SATA_DEVICE_PATH*)SiI3132DevicePath)->PortMultiplierPortNumber = PortMultiplierPort;
-  } else {
-    //Temp:((SATA_DEVICE_PATH*)SiI3132DevicePath)->PortMultiplierPortNumber = SATA_HBA_DIRECT_CONNECT_FLAG;
-    ((SATA_DEVICE_PATH*)SiI3132DevicePath)->PortMultiplierPortNumber = 0;
   }
-  ((SATA_DEVICE_PATH*)SiI3132DevicePath)->Lun = Port; //TODO: Search information how to define properly LUN (Logical Unit Number)
+  else {
+    SiI3132DevicePath = CreateDeviceNode (MESSAGING_DEVICE_PATH, MSG_SATA_DP, sizeof (SATA_DEVICE_PATH));
+    if (SiI3132DevicePath == NULL) {
+        return EFI_OUT_OF_RESOURCES;
+    }
 
+    ((SATA_DEVICE_PATH*)SiI3132DevicePath)->HBAPortNumber = Port;
+    if (FeaturePcdGet (PcdSataSiI3132FeaturePMPSupport)) {
+        ((SATA_DEVICE_PATH*)SiI3132DevicePath)->PortMultiplierPortNumber = PortMultiplierPort;
+    } else {
+        //Temp:((SATA_DEVICE_PATH*)SiI3132DevicePath)->PortMultiplierPortNumber = SATA_HBA_DIRECT_CONNECT_FLAG;
+        ((SATA_DEVICE_PATH*)SiI3132DevicePath)->PortMultiplierPortNumber = 0;
+    }
+//  ((SATA_DEVICE_PATH*)SiI3132DevicePath)->Lun = Port; //TODO: Search information how to define properly LUN (Logical Unit Number)
+    ((SATA_DEVICE_PATH*)SiI3132DevicePath)->Lun = 0; // Only support lun0 on ATA
+  }
   *DevicePath = SiI3132DevicePath;
   return EFI_SUCCESS;
 }
@@ -677,7 +691,7 @@ SiI3132GetDevice (
     return EFI_INVALID_PARAMETER;
   }
 
-  if (((SATA_DEVICE_PATH*)DevicePath)->Lun >= SATA_SII3132_MAXPORT) {
+  if (((SATA_DEVICE_PATH*)DevicePath)->HBAPortNumber >= SATA_SII3132_MAXPORT) {
     return EFI_NOT_FOUND;
   }
 
@@ -685,7 +699,7 @@ SiI3132GetDevice (
     ASSERT (0); //TODO: Implement me!
     return EFI_UNSUPPORTED;
   } else {
-    *Port = ((SATA_DEVICE_PATH*)DevicePath)->Lun;
+    *Port = ((SATA_DEVICE_PATH*)DevicePath)->HBAPortNumber;
     // Return the first Sata Sevice as there should be only one directly connected
     *PortMultiplierPort = ((SATA_SI3132_DEVICE*)SataSiI3132Instance->Ports[*Port].Devices.ForwardLink)->Index;
     return EFI_SUCCESS;
-- 
2.5.5

[PATCH 6/7] EmbeddedPkg: SiI3132: Enable SCSI pass-through protocol

[Jeremy Linton]

Now that everything is in place, lets export the protocol,
build the module, and remove the ATAPI unsupported flags.
Now when we detect an ATAPI device on a port we flag it
as such.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c   | 48 ++++++++++++++--------
 .../Drivers/SataSiI3132Dxe/SataSiI3132Dxe.inf      |  2 +
 2 files changed, 32 insertions(+), 18 deletions(-)

diff --git a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c
index f494655..52b79fd 100644
--- a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c
+++ b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c
@@ -1,7 +1,7 @@
 /** @file
 *  PCIe Sata support for the Silicon Image I3132
 *
-*  Copyright (c) 2011-2015, ARM Limited. All rights reserved.
+*  Copyright (c) 2011-2016, ARM Limited. All rights reserved.
 *
 *  This program and the accompanying materials
 *  are licensed and made available under the terms and conditions of the BSD License
@@ -16,6 +16,7 @@
 #include "SataSiI3132.h"
 
 #include <IndustryStandard/Acpi10.h>
+#include <IndustryStandard/Atapi.h>
 
 #include <Library/MemoryAllocationLib.h>
 #include <Library/UefiBootServicesTableLib.h>
@@ -88,7 +89,6 @@ SataSiI3132Constructor (
   )
 {
   SATA_SI3132_INSTANCE    *Instance;
-  EFI_ATA_PASS_THRU_MODE  *AtaPassThruMode;
 
   if (!SataSiI3132Instance) {
     return EFI_INVALID_PARAMETER;
@@ -102,16 +102,15 @@ SataSiI3132Constructor (
   Instance->Signature           = SATA_SII3132_SIGNATURE;
   Instance->PciIo               = PciIo;
 
-  AtaPassThruMode = (EFI_ATA_PASS_THRU_MODE*)AllocatePool (sizeof (EFI_ATA_PASS_THRU_MODE));
-  AtaPassThruMode->Attributes = EFI_ATA_PASS_THRU_ATTRIBUTES_PHYSICAL | EFI_ATA_PASS_THRU_ATTRIBUTES_LOGICAL;
-  AtaPassThruMode->IoAlign = 0x1000;
+  Instance->AtaPassThruMode.Attributes = EFI_ATA_PASS_THRU_ATTRIBUTES_PHYSICAL | EFI_ATA_PASS_THRU_ATTRIBUTES_LOGICAL;
+  Instance->AtaPassThruMode.IoAlign = 0x1000;
 
   // Initialize SiI3132 ports
   SataSiI3132PortConstructor (Instance, 0);
   SataSiI3132PortConstructor (Instance, 1);
 
   // Set ATA Pass Thru Protocol
-  Instance->AtaPassThruProtocol.Mode            = AtaPassThruMode;
+  Instance->AtaPassThruProtocol.Mode            = &Instance->AtaPassThruMode;
   Instance->AtaPassThruProtocol.PassThru        = SiI3132AtaPassThru;
   Instance->AtaPassThruProtocol.GetNextPort     = SiI3132GetNextPort;
   Instance->AtaPassThruProtocol.GetNextDevice   = SiI3132GetNextDevice;
@@ -120,6 +119,19 @@ SataSiI3132Constructor (
   Instance->AtaPassThruProtocol.ResetPort       = SiI3132ResetPort;
   Instance->AtaPassThruProtocol.ResetDevice     = SiI3132ResetDevice;
 
+  Instance->ExtScsiPassThruMode.Attributes = EFI_EXT_SCSI_PASS_THRU_ATTRIBUTES_PHYSICAL | EFI_EXT_SCSI_PASS_THRU_ATTRIBUTES_LOGICAL;
+  Instance->ExtScsiPassThruMode.IoAlign = 0x1000;
+
+  // Set SCSI Pass Thru Protocol
+  Instance->ExtScsiPassThru.Mode                = &Instance->ExtScsiPassThruMode;
+  Instance->ExtScsiPassThru.PassThru            = SiI3132ScsiPassThru;
+  Instance->ExtScsiPassThru.GetNextTargetLun    = SiI3132GetNextTargetLun;
+  Instance->ExtScsiPassThru.BuildDevicePath     = SiI3132ScsiBuildDevicePath;
+  Instance->ExtScsiPassThru.GetTargetLun        = SiI3132GetTargetLun;
+  Instance->ExtScsiPassThru.ResetChannel        = SiI3132ResetChannel;
+  Instance->ExtScsiPassThru.ResetTargetLun      = SiI3132ResetTargetLun;
+  Instance->ExtScsiPassThru.GetNextTarget       = SiI3132GetNextTarget;
+
   *SataSiI3132Instance = Instance;
 
   return EFI_SUCCESS;
@@ -165,6 +177,7 @@ SataSiI3132PortInitialization (
   UINT32                  Signature;
   EFI_STATUS              Status;
   EFI_PCI_IO_PROTOCOL*    PciIo;
+  BOOLEAN                 Atapi = FALSE;
 
   Status = SiI3132HwResetPort (Port);
   if (EFI_ERROR (Status)) {
@@ -177,24 +190,23 @@ SataSiI3132PortInitialization (
   Status = SATA_PORT_READ32 (Port->RegBase + SII3132_PORT_SSTATUS_REG, &Value32);
   if (!EFI_ERROR (Status) && (Value32 & 0x3)) {
     // Do a soft reset to see if it is a port multiplier
-    SATA_TRACE ("SataSiI3132PortInitialization: soft reset - it is a port multiplier\n");
+    SATA_TRACE ("SataSiI3132PortInitialization: soft reset - is it a port multiplier?\n");
     Status = SiI3132SoftResetCommand (Port, &Signature);
     if (!EFI_ERROR (Status)) {
       if (Signature == SII3132_PORT_SIGNATURE_PMP) {
-        SATA_TRACE ("SataSiI3132PortInitialization(): a Port Multiplier is present");
+        DEBUG ((DEBUG_ERROR, "SataSiI3132PortInitialization(): a Port Multiplier is present"));
         if (FeaturePcdGet (PcdSataSiI3132FeaturePMPSupport)) {
           ASSERT (0); // Not supported yet
         } else {
           return EFI_UNSUPPORTED;
         }
       } else if (Signature == SII3132_PORT_SIGNATURE_ATAPI) {
-        ASSERT (0); // Not supported yet
         SATA_TRACE ("SataSiI3132PortInitialization(): an ATAPI device is present");
-        return EFI_UNSUPPORTED;
+        Atapi = TRUE;
       } else if (Signature == SII3132_PORT_SIGNATURE_ATA) {
         SATA_TRACE ("SataSiI3132PortInitialization(): an ATA device is present");
       } else {
-        SATA_TRACE ("SataSiI3132PortInitialization(): Present device unknown!");
+        DEBUG ((DEBUG_ERROR, "SataSiI3132PortInitialization(): Present device unknown!"));
         ASSERT (0); // Not supported
         return EFI_UNSUPPORTED;
       }
@@ -204,6 +216,7 @@ SataSiI3132PortInitialization (
       Device->Index     = Port->Index; //TODO: Could need to be fixed when SATA Port Multiplier support
       Device->Port      = Port;
       Device->BlockSize = 0;
+      Device->Atapi     = Atapi;
 
       // Attached the device to the Sata Port
       InsertTailList (&Port->Devices, &Device->Link);
@@ -432,13 +445,12 @@ SataSiI3132DriverBindingStart (
     return Status;
   }
 
-  // Install Ata Pass Thru Protocol
-  Status = gBS->InstallProtocolInterface (
-              &Controller,
-              &gEfiAtaPassThruProtocolGuid,
-              EFI_NATIVE_INTERFACE,
-              &(SataSiI3132Instance->AtaPassThruProtocol)
-              );
+  Status = gBS->InstallMultipleProtocolInterfaces(&Controller,
+                                                  &gEfiAtaPassThruProtocolGuid,
+                                                  &(SataSiI3132Instance->AtaPassThruProtocol),
+                                                  &gEfiExtScsiPassThruProtocolGuid,
+                                                  &(SataSiI3132Instance->ExtScsiPassThru),
+                                                  NULL);
   if (EFI_ERROR (Status)) {
     goto FREE_POOL;
   }
diff --git a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132Dxe.inf b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132Dxe.inf
index 69aaab3..eb6e2bd 100644
--- a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132Dxe.inf
+++ b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132Dxe.inf
@@ -35,10 +35,12 @@
   SataSiI3132.c
   ComponentName.c
   SiI3132AtaPassThru.c
+  SiI3132ScsiPassThru.c
 
 [Protocols]
   gEfiPciIoProtocolGuid                         # Consumed
   gEfiAtaPassThruProtocolGuid                   # Produced
+  gEfiExtScsiPassThruProtocolGuid               # Produced
 
 [Pcd]
   gEmbeddedTokenSpaceGuid.PcdSataSiI3132FeaturePMPSupport
-- 
2.5.5

[PATCH 7/7] EmbeddedPkg: SiI3132: Correct the IoAlign

[Jeremy Linton]

With the update to the DMA lib, and the FIS submission
cleanups the SiI driver now works fine with alignments
less than a full page. Large alignment requirements
cause problems with grub. Decrease it to a sane value.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c
index 52b79fd..737e398 100644
--- a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c
+++ b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c
@@ -103,7 +103,7 @@ SataSiI3132Constructor (
   Instance->PciIo               = PciIo;
 
   Instance->AtaPassThruMode.Attributes = EFI_ATA_PASS_THRU_ATTRIBUTES_PHYSICAL | EFI_ATA_PASS_THRU_ATTRIBUTES_LOGICAL;
-  Instance->AtaPassThruMode.IoAlign = 0x1000;
+  Instance->AtaPassThruMode.IoAlign = 0x4;
 
   // Initialize SiI3132 ports
   SataSiI3132PortConstructor (Instance, 0);
@@ -120,7 +120,7 @@ SataSiI3132Constructor (
   Instance->AtaPassThruProtocol.ResetDevice     = SiI3132ResetDevice;
 
   Instance->ExtScsiPassThruMode.Attributes = EFI_EXT_SCSI_PASS_THRU_ATTRIBUTES_PHYSICAL | EFI_EXT_SCSI_PASS_THRU_ATTRIBUTES_LOGICAL;
-  Instance->ExtScsiPassThruMode.IoAlign = 0x1000;
+  Instance->ExtScsiPassThruMode.IoAlign = 0x4;
 
   // Set SCSI Pass Thru Protocol
   Instance->ExtScsiPassThru.Mode                = &Instance->ExtScsiPassThruMode;
-- 
2.5.5

[PATCH] Platforms/ARM/Juno: Add SCSI pass-through protocol

[Jeremy Linton]

Now that the SiI adapter supports ATAPI add the SCSI
pass-through protocol.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 Platforms/ARM/Juno/ArmJuno.dsc | 3 +++
 Platforms/ARM/Juno/ArmJuno.fdf | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/Platforms/ARM/Juno/ArmJuno.dsc b/Platforms/ARM/Juno/ArmJuno.dsc
index 4080c0b..1e3e551 100644
--- a/Platforms/ARM/Juno/ArmJuno.dsc
+++ b/Platforms/ARM/Juno/ArmJuno.dsc
@@ -49,6 +49,7 @@
 
   # USB Requirements
   UefiUsbLib|MdePkg/Library/UefiUsbLib/UefiUsbLib.inf
+  UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf
 
 [LibraryClasses.common.SEC]
   PrePiLib|EmbeddedPkg/Library/PrePiLib/PrePiLib.inf
@@ -285,6 +286,8 @@
   # SATA Controller
   #
   MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf
+  MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
+  MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
   EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132Dxe.inf
 
   #
diff --git a/Platforms/ARM/Juno/ArmJuno.fdf b/Platforms/ARM/Juno/ArmJuno.fdf
index beee7af..b20367e 100644
--- a/Platforms/ARM/Juno/ArmJuno.fdf
+++ b/Platforms/ARM/Juno/ArmJuno.fdf
@@ -166,6 +166,8 @@ FvNameGuid         = B73FE497-B92E-416e-8326-45AD0D270092
   # SATA Controller
   #
   INF MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf
+  INF MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
+  INF MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
   INF EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132Dxe.inf
 
   #
-- 
2.5.5

Re: [PATCH 0/8] ATAPI support on SiI SATA adapter

[Ard Biesheuvel]

Hi Jeremy,

On 14 November 2016 at 21:09, Jeremy Linton <jeremy.linton@arm.com> wrote:
> The SiI isn't an AHCI compatible adapter so it implements the EFI ATA
> pass-through protocol directly. This works for fixed hard drives, but
> not ATAPI attached devices (CDROM, DVDROM, TAPE, etc).
>
> This patch adds read only ATAPI support via the EFI SCSI pass-through
> protocol, allowing boot from attached CD/DVD. This patch also cleans
> up, and tweaks recovery paths/etc in the original driver.

Very nice! Thanks for getting to the bottom of this.

However, looking at the patches, they are riddled with coding style
violations. I am usually less strict than Leif when it comes to
upholding those, but these patches really need to be cleaned up to be
considered for merging.

> When
> combined with the ARM/PCI dma lib changes this allows us to relax the
> IO alignment requirement that caused grub failures.
>

What changes are you referring to here?

> Finally, the OpenPlatformPkg/Juno must be updated, with another patch
> to avoid build breaks now that the SiI has a dependency on the SCSI
> libraries.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>
> Jeremy Linton (7):
>   MdePkg IndustryStandard/Scsi.h: Add sense code macro
>   EmbeddedPkg: SiI3132: Add ScsiProtocol callbacks
>   EmbeddedPkg: SiI3132: Add SCSI protocol support to header
>   EmbeddedPkg: SiI3132: Break out FIS command submission
>   EmbeddedPkg: SiI3132: Cleanup device node creation
>   EmbeddedPkg: SiI3132: Enable SCSI pass-through protocol
>   EmbeddedPkg: SiI3132: Correct the IoAlign
>
>  EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c   |  48 ++-
>  EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h   |  89 ++++-
>  .../Drivers/SataSiI3132Dxe/SataSiI3132Dxe.inf      |   2 +
>  .../Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c    | 270 ++++++++------
>  .../Drivers/SataSiI3132Dxe/SiI3132ScsiPassThru.c   | 401 +++++++++++++++++++++
>  MdePkg/Include/IndustryStandard/Scsi.h             |   2 +
>  OpenPlatformPkg                                    |   2 +-
>  7 files changed, 688 insertions(+), 126 deletions(-)
>  create mode 100644 EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132ScsiPassThru.c
>
> --
> 2.5.5
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 0/8] ATAPI support on SiI SATA adapter

[Jeremy Linton]

On 11/15/2016 01:43 AM, Ard Biesheuvel wrote:
> Hi Jeremy,
>
> On 14 November 2016 at 21:09, Jeremy Linton <jeremy.linton@arm.com> wrote:
>> The SiI isn't an AHCI compatible adapter so it implements the EFI ATA
>> pass-through protocol directly. This works for fixed hard drives, but
>> not ATAPI attached devices (CDROM, DVDROM, TAPE, etc).
>>
>> This patch adds read only ATAPI support via the EFI SCSI pass-through
>> protocol, allowing boot from attached CD/DVD. This patch also cleans
>> up, and tweaks recovery paths/etc in the original driver.
>
> Very nice! Thanks for getting to the bottom of this.
>
> However, looking at the patches, they are riddled with coding style
> violations. I am usually less strict than Leif when it comes to
> upholding those, but these patches really need to be cleaned up to be
> considered for merging.


Is there a tool which can correct or at least point out the formatting 
errors?


>
>> When
>> combined with the ARM/PCI dma lib changes this allows us to relax the
>> IO alignment requirement that caused grub failures.
>>
>
> What changes are you referring to here?

I believe on juno the PCI changed from the ArmDmaLib to the null lib or 
some such, which removed the bounce buffering on unaligned map/unmap.


>
>> Finally, the OpenPlatformPkg/Juno must be updated, with another patch
>> to avoid build breaks now that the SiI has a dependency on the SCSI
>> libraries.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>>
>> Jeremy Linton (7):
>>   MdePkg IndustryStandard/Scsi.h: Add sense code macro
>>   EmbeddedPkg: SiI3132: Add ScsiProtocol callbacks
>>   EmbeddedPkg: SiI3132: Add SCSI protocol support to header
>>   EmbeddedPkg: SiI3132: Break out FIS command submission
>>   EmbeddedPkg: SiI3132: Cleanup device node creation
>>   EmbeddedPkg: SiI3132: Enable SCSI pass-through protocol
>>   EmbeddedPkg: SiI3132: Correct the IoAlign
>>
>>  EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c   |  48 ++-
>>  EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h   |  89 ++++-
>>  .../Drivers/SataSiI3132Dxe/SataSiI3132Dxe.inf      |   2 +
>>  .../Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c    | 270 ++++++++------
>>  .../Drivers/SataSiI3132Dxe/SiI3132ScsiPassThru.c   | 401 +++++++++++++++++++++
>>  MdePkg/Include/IndustryStandard/Scsi.h             |   2 +
>>  OpenPlatformPkg                                    |   2 +-
>>  7 files changed, 688 insertions(+), 126 deletions(-)
>>  create mode 100644 EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132ScsiPassThru.c
>>
>> --
>> 2.5.5
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 0/8] ATAPI support on SiI SATA adapter

[Ard Biesheuvel]

On 15 November 2016 at 14:54, Jeremy Linton <jeremy.linton@arm.com> wrote:
> On 11/15/2016 01:43 AM, Ard Biesheuvel wrote:
>>
>> Hi Jeremy,
>>
>> On 14 November 2016 at 21:09, Jeremy Linton <jeremy.linton@arm.com> wrote:
>>>
>>> The SiI isn't an AHCI compatible adapter so it implements the EFI ATA
>>> pass-through protocol directly. This works for fixed hard drives, but
>>> not ATAPI attached devices (CDROM, DVDROM, TAPE, etc).
>>>
>>> This patch adds read only ATAPI support via the EFI SCSI pass-through
>>> protocol, allowing boot from attached CD/DVD. This patch also cleans
>>> up, and tweaks recovery paths/etc in the original driver.
>>
>>
>> Very nice! Thanks for getting to the bottom of this.
>>
>> However, looking at the patches, they are riddled with coding style
>> violations. I am usually less strict than Leif when it comes to
>> upholding those, but these patches really need to be cleaned up to be
>> considered for merging.
>
>
>
> Is there a tool which can correct or at least point out the formatting
> errors?
>

Yes, BaseTools/Scripts/PatchCheck.py

>
>>
>>> When
>>> combined with the ARM/PCI dma lib changes this allows us to relax the
>>> IO alignment requirement that caused grub failures.
>>>
>>
>> What changes are you referring to here?
>
>
> I believe on juno the PCI changed from the ArmDmaLib to the null lib or some
> such, which removed the bounce buffering on unaligned map/unmap.
>

Indeed, it removed the use of uncached memory, which does not work for
coherent masters.
I only realize now that the Sil3132 is a coherent PCI device, and that
you were using 4 KB IoAlign to work around the non-coherent nature of
ArmDmaLib


>
>>
>>> Finally, the OpenPlatformPkg/Juno must be updated, with another patch
>>> to avoid build breaks now that the SiI has a dependency on the SCSI
>>> libraries.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>>>
>>> Jeremy Linton (7):
>>>   MdePkg IndustryStandard/Scsi.h: Add sense code macro
>>>   EmbeddedPkg: SiI3132: Add ScsiProtocol callbacks
>>>   EmbeddedPkg: SiI3132: Add SCSI protocol support to header
>>>   EmbeddedPkg: SiI3132: Break out FIS command submission
>>>   EmbeddedPkg: SiI3132: Cleanup device node creation
>>>   EmbeddedPkg: SiI3132: Enable SCSI pass-through protocol
>>>   EmbeddedPkg: SiI3132: Correct the IoAlign
>>>
>>>  EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c   |  48 ++-
>>>  EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h   |  89 ++++-
>>>  .../Drivers/SataSiI3132Dxe/SataSiI3132Dxe.inf      |   2 +
>>>  .../Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c    | 270 ++++++++------
>>>  .../Drivers/SataSiI3132Dxe/SiI3132ScsiPassThru.c   | 401
>>> +++++++++++++++++++++
>>>  MdePkg/Include/IndustryStandard/Scsi.h             |   2 +
>>>  OpenPlatformPkg                                    |   2 +-
>>>  7 files changed, 688 insertions(+), 126 deletions(-)
>>>  create mode 100644
>>> EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132ScsiPassThru.c
>>>
>>> --
>>> 2.5.5
>>>
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org
>>> https://lists.01.org/mailman/listinfo/edk2-devel
>
>

Re: [PATCH 0/8] ATAPI support on SiI SATA adapter

[Jeremy Linton]

|-----Original Message-----
|From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
|Sent: Tuesday, November 15, 2016 8:58 AM
|To: Jeremy Linton
|Cc: edk2-devel-01; Steve Capper; Leif Lindholm; ryan.harkin@linaro.org;
|linaro-uefi
|Subject: Re: [edk2] [PATCH 0/8] ATAPI support on SiI SATA adapter
|
|On 15 November 2016 at 14:54, Jeremy Linton <jeremy.linton@arm.com>
|wrote:
|> On 11/15/2016 01:43 AM, Ard Biesheuvel wrote:
|>>
|>> Hi Jeremy,
|>>
|>> On 14 November 2016 at 21:09, Jeremy Linton <jeremy.linton@arm.com>
|wrote:
|>>>
|>>> The SiI isn't an AHCI compatible adapter so it implements the EFI
|>>> ATA pass-through protocol directly. This works for fixed hard
|>>> drives, but not ATAPI attached devices (CDROM, DVDROM, TAPE, etc).
|>>>
|>>> This patch adds read only ATAPI support via the EFI SCSI
|>>> pass-through protocol, allowing boot from attached CD/DVD. This
|>>> patch also cleans up, and tweaks recovery paths/etc in the original driver.
|>>
|>>
|>> Very nice! Thanks for getting to the bottom of this.
|>>
|>> However, looking at the patches, they are riddled with coding style
|>> violations. I am usually less strict than Leif when it comes to
|>> upholding those, but these patches really need to be cleaned up to be
|>> considered for merging.
|>
|>
|>
|> Is there a tool which can correct or at least point out the formatting
|> errors?
|>
|
|Yes, BaseTools/Scripts/PatchCheck.py

I did use that before submission, the only thing it complained about was an error caused by the git submodule commit id not having a CR (which AFAIK is bogus)

[jlinton@mammon-v1 edk2]$ python BaseTools/Scripts/PatchCheck.py -8 |more
Checking git commit: 4a9a9d7
The commit message format passed all checks.
The code passed all checks.

Checking git commit: 8537b6c
The commit message format passed all checks.
The code passed all checks.

Checking git commit: 458452e
The commit message format passed all checks.
The code passed all checks.

Checking git commit: ca0606f
The commit message format passed all checks.
Code format is not valid:
 * Line ending ('\n') is not CRLF
   File: OpenPlatformPkg
   Line: Subproject commit a072474a3b05ec7f12f2d4db271cc07a0cf7a369

Checking git commit: 0d9942f
The commit message format passed all checks.
The code passed all checks.

Checking git commit: b516f7a
The commit message format passed all checks.
The code passed all checks.

Checking git commit: 2259641
The commit message format passed all checks.
The code passed all checks.

Checking git commit: 61be9c2
The commit message format passed all checks.
The code passed all checks.

|
|>
|>>
|>>> When
|>>> combined with the ARM/PCI dma lib changes this allows us to relax
|>>> the IO alignment requirement that caused grub failures.
|>>>
|>>
|>> What changes are you referring to here?
|>
|>
|> I believe on juno the PCI changed from the ArmDmaLib to the null lib
|> or some such, which removed the bounce buffering on unaligned
|map/unmap.
|>
|
|Indeed, it removed the use of uncached memory, which does not work for
|coherent masters.
|I only realize now that the Sil3132 is a coherent PCI device, and that you were
|using 4 KB IoAlign to work around the non-coherent nature of ArmDmaLib
|
|
|>
|>>
|>>> Finally, the OpenPlatformPkg/Juno must be updated, with another
|>>> patch to avoid build breaks now that the SiI has a dependency on the
|>>> SCSI libraries.
|>>>
|>>> Contributed-under: TianoCore Contribution Agreement 1.0
|>>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
|>>>
|>>> Jeremy Linton (7):
|>>>   MdePkg IndustryStandard/Scsi.h: Add sense code macro
|>>>   EmbeddedPkg: SiI3132: Add ScsiProtocol callbacks
|>>>   EmbeddedPkg: SiI3132: Add SCSI protocol support to header
|>>>   EmbeddedPkg: SiI3132: Break out FIS command submission
|>>>   EmbeddedPkg: SiI3132: Cleanup device node creation
|>>>   EmbeddedPkg: SiI3132: Enable SCSI pass-through protocol
|>>>   EmbeddedPkg: SiI3132: Correct the IoAlign
|>>>
|>>>  EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c   |  48 ++-
|>>>  EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h   |  89 ++++-
|>>>  .../Drivers/SataSiI3132Dxe/SataSiI3132Dxe.inf      |   2 +
|>>>  .../Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c    | 270 ++++++++------
|>>>  .../Drivers/SataSiI3132Dxe/SiI3132ScsiPassThru.c   | 401
|>>> +++++++++++++++++++++
|>>>  MdePkg/Include/IndustryStandard/Scsi.h             |   2 +
|>>>  OpenPlatformPkg                                    |   2 +-
|>>>  7 files changed, 688 insertions(+), 126 deletions(-)  create mode
|>>> 100644 EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132ScsiPassThru.c
|>>>
|>>> --
|>>> 2.5.5
|>>>
|>>> _______________________________________________
|>>> edk2-devel mailing list
|>>> edk2-devel@lists.01.org
|>>> https://lists.01.org/mailman/listinfo/edk2-devel
|>
|>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH 0/8] ATAPI support on SiI SATA adapter

[Ard Biesheuvel]

On 15 November 2016 at 15:05, Jeremy Linton <Jeremy.Linton@arm.com> wrote:
> |-----Original Message-----
> |From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> |Sent: Tuesday, November 15, 2016 8:58 AM
> |To: Jeremy Linton
> |Cc: edk2-devel-01; Steve Capper; Leif Lindholm; ryan.harkin@linaro.org;
> |linaro-uefi
> |Subject: Re: [edk2] [PATCH 0/8] ATAPI support on SiI SATA adapter
> |
> |On 15 November 2016 at 14:54, Jeremy Linton <jeremy.linton@arm.com>
> |wrote:
> |> On 11/15/2016 01:43 AM, Ard Biesheuvel wrote:
> |>>
> |>> Hi Jeremy,
> |>>
> |>> On 14 November 2016 at 21:09, Jeremy Linton <jeremy.linton@arm.com>
> |wrote:
> |>>>
> |>>> The SiI isn't an AHCI compatible adapter so it implements the EFI
> |>>> ATA pass-through protocol directly. This works for fixed hard
> |>>> drives, but not ATAPI attached devices (CDROM, DVDROM, TAPE, etc).
> |>>>
> |>>> This patch adds read only ATAPI support via the EFI SCSI
> |>>> pass-through protocol, allowing boot from attached CD/DVD. This
> |>>> patch also cleans up, and tweaks recovery paths/etc in the original driver.
> |>>
> |>>
> |>> Very nice! Thanks for getting to the bottom of this.
> |>>
> |>> However, looking at the patches, they are riddled with coding style
> |>> violations. I am usually less strict than Leif when it comes to
> |>> upholding those, but these patches really need to be cleaned up to be
> |>> considered for merging.
> |>
> |>
> |>
> |> Is there a tool which can correct or at least point out the formatting
> |> errors?
> |>
> |
> |Yes, BaseTools/Scripts/PatchCheck.py
>
> I did use that before submission, the only thing it complained about was an error caused by the git submodule commit id not having a CR (which AFAIK is bogus)
>
> [jlinton@mammon-v1 edk2]$ python BaseTools/Scripts/PatchCheck.py -8 |more
> Checking git commit: 4a9a9d7
> The commit message format passed all checks.
> The code passed all checks.
>
> Checking git commit: 8537b6c
> The commit message format passed all checks.
> The code passed all checks.
>
> Checking git commit: 458452e
> The commit message format passed all checks.
> The code passed all checks.
>
> Checking git commit: ca0606f
> The commit message format passed all checks.
> Code format is not valid:
>  * Line ending ('\n') is not CRLF
>    File: OpenPlatformPkg
>    Line: Subproject commit a072474a3b05ec7f12f2d4db271cc07a0cf7a369
>
> Checking git commit: 0d9942f
> The commit message format passed all checks.
> The code passed all checks.
>
> Checking git commit: b516f7a
> The commit message format passed all checks.
> The code passed all checks.
>
> Checking git commit: 2259641
> The commit message format passed all checks.
> The code passed all checks.
>
> Checking git commit: 61be9c2
> The commit message format passed all checks.
> The code passed all checks.
>

Hmm, ok, that is strange.

What I spotted when going over the patches was lots of initialized
stack variables, which the EDK2 coding style does not allow. Local
variables should be initialized using assignments not initializers
(for some reason, which I think may have to do with generated code
size on some toolchains)

Other things to check for is spaces around '=' and the use of STATIC
for things that are only referenced locally.

In any case, I will go through the patches again to comment in some more detail.

Thanks,
Ard.