Re: [Linaro-uefi] [PATCH v4 6/6] EmbeddedPkg: SiI3132: Enable SCSI pass-through protocol

On Tue, Mar 07, 2017 at 04:15:11PM -0600, Jeremy Linton wrote:

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 | 52 ++++++++++++++-------- .../Drivers/SataSiI3132Dxe/SataSiI3132Dxe.inf | 2 + 2 files changed, 35 insertions(+), 19 deletions(-)

diff --git a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c index f494655..b98231c 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-2017, 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 = 0x4;

Why 0x4? Could we have a #define with a descriptive name instead?

// 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,20 @@ 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 = 0x4;

And here?

• // 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,7 +178,9 @@ SataSiI3132PortInitialization ( UINT32 Signature; EFI_STATUS Status; EFI_PCI_IO_PROTOCOL* PciIo;

• BOOLEAN Atapi;

• Atapi = FALSE; Status = SiI3132HwResetPort (Port); if (EFI_ERROR (Status)) { return Status;

@@ -177,33 +192,33 @@ 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"));
  

What's the dignificance of switching from SATA_TRACE to DEBUG? Upgrading the verbosity?

     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;
   }
  

// Create Device Device = (SATA_SI3132_DEVICE*)AllocatePool (sizeof (SATA_SI3132_DEVICE));

•  Device->Index     = Port->Index; //TODO: Could need to be fixed when SATA Port Multiplier support
  

•  Device->Index     = 0; //TODO: When port multiplers are supported this is the multiplier index
   Device->Port      = Port;
   Device->BlockSize = 0;
  

•  Device->Atapi     = Atapi;
  

// Attached the device to the Sata Port InsertTailList (&Port->Devices, &Device->Link); @@ -432,13 +447,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.9.3