From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-x22a.google.com (mail-wr0-x22a.google.com [IPv6:2a00:1450:400c:c0c::22a]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 9EDD58042F for ; Fri, 17 Mar 2017 09:40:53 -0700 (PDT) Received: by mail-wr0-x22a.google.com with SMTP id u108so55544227wrb.3 for ; Fri, 17 Mar 2017 09:40:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=Nd+/KpKWhA5nUE+xjxVVOpQM/FRAqAYIX+cgq8DAQHg=; b=OXVABSznf/mDw7wbh1EVRdD7HB5NCVu65kFaG+JX9mez4+CoQVCBbb73ZjhTOEyGUf FUSgyaDRTzNxYm/Ay7fu4mCUjr6v+DrL2aTRJuHTDcdSiCT0TaXEvZRBOsH5OY9pg7Tt zOR3kpQnacrqh3qlEexlMVtVB/RiUXvf5KFBc= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=Nd+/KpKWhA5nUE+xjxVVOpQM/FRAqAYIX+cgq8DAQHg=; b=VrojPYxJ5C3DjseB3DU8jVf0KBHpX5EQL3d609KNC+u330AswMMGio5/AVnCn5BoHj 2Ctivh6L9nA7A4Hdbsi2Xbcghbrwj88cz2IJIFQ5iLQVCxEdbB4dWRqIIsn1UsHs70ye tQIMeE88gcYZ9CMU9EENCZ1AWWtZ9vcF+dTAODTNdO/CearZK2tcuaLu15LgxMy8MP2H iH7IsBYOrFuKv50UgpIvqHkVDF1942tKQQ29jNGxitCZvOYAjjFMyM1LYAmC5aUiFSsQ ZvMCfNfgGcbfjOkHHOFMGRlDvqwdH21vZgvumsYKLsZrwg6oiWSqb9kZ6hTtqeu7tjc8 zaIQ== X-Gm-Message-State: AFeK/H3VTE/lhOF8fEL+lWHEzMTdBLOCDSl+xOMQgl44YVVBMuls9I2LCmERd2HhFcgLIuy6 X-Received: by 10.223.130.101 with SMTP id 92mr14382964wrb.192.1489768852086; Fri, 17 Mar 2017 09:40:52 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id 127sm3229892wmt.20.2017.03.17.09.40.51 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 17 Mar 2017 09:40:51 -0700 (PDT) Date: Fri, 17 Mar 2017 16:40:49 +0000 From: Leif Lindholm To: Jeremy Linton Cc: edk2-devel@lists.01.org, ryan.harkin@linaro.org, linaro-uefi@lists.linaro.org, ard.biesheuvel@linaro.org Message-ID: <20170317164049.GJ16034@bivouac.eciton.net> References: <20170307221512.5180-1-jeremy.linton@arm.com> <20170307221512.5180-7-jeremy.linton@arm.com> MIME-Version: 1.0 In-Reply-To: <20170307221512.5180-7-jeremy.linton@arm.com> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [PATCH v4 6/6] EmbeddedPkg: SiI3132: Enable SCSI pass-through protocol X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 17 Mar 2017 16:40:54 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 > --- > 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 > +#include > > #include > #include > @@ -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 >