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

[Leif Lindholm <leif.lindholm@linaro.org>]

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
>