From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x231.google.com (mail-wm0-x231.google.com [IPv6:2a00:1450:400c:c09::231]) (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 2AF078042B for ; Fri, 17 Mar 2017 09:37:23 -0700 (PDT) Received: by mail-wm0-x231.google.com with SMTP id t189so20335371wmt.1 for ; Fri, 17 Mar 2017 09:37:23 -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=m7VvgJ8OwqPj7ZW4cZOhlkTmabQRcJHdmHqitriC7+o=; b=ajhd+ip/UbG9LIcYCz4KrTUB/teBEB3zCtMYIh01IiZ1BC048nHTBAYpeT6c/4+j9K /L6DOYPh3cEMWDVlgPWExLli4irs1NzISPKRoTck0Gvbkv78hhczsAyn5zjhncET/Thk mnaCkoidMRwfh4funE6mFH+IhqzGO3lgk/VG4= 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=m7VvgJ8OwqPj7ZW4cZOhlkTmabQRcJHdmHqitriC7+o=; b=nu8TbW+uSz4v9tps6ziHYRfKcQLB6LmCYfbOoZwHMHRhAS3reQqgjblG4dIO2nRf74 2npTN75HLgS7HfY+3EeYwTmyHjKnt+XlHT59R7mulcEpym7l/wi7viZ3JpR/chIcOtiO aSd3JWzHwlAyEa72T0nX5jzwqey/ZMP8SK1DCl7rbjNge71K56JSrhmlsBJEMSsA/ErT 3bGDdB013zuGJV8YhJeXPSzmrWDO9zi4nfXQBW2INNyYzTVCSxAsI6u5E6u9h39+IN24 1mvTcFjDMZ+YO75AlegY5x+jZHzeJlBSF6xnKgzvVAMBK/pMvwaDJYmlb7Si1VQRU8U9 JJ5A== X-Gm-Message-State: AFeK/H34ujWX8h14duML1ez0RI63myrd4sPImJWFJ1WtFnW0PTpN70MaejmFssSmN86QzPaJ X-Received: by 10.28.216.141 with SMTP id p135mr3774321wmg.71.1489768641151; Fri, 17 Mar 2017 09:37:21 -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 60sm10495321wrg.60.2017.03.17.09.37.20 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 17 Mar 2017 09:37:20 -0700 (PDT) Date: Fri, 17 Mar 2017 16:37:18 +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: <20170317163718.GI16034@bivouac.eciton.net> References: <20170307221512.5180-1-jeremy.linton@arm.com> <20170307221512.5180-6-jeremy.linton@arm.com> MIME-Version: 1.0 In-Reply-To: <20170307221512.5180-6-jeremy.linton@arm.com> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [PATCH v4 5/6] EmbeddedPkg: SiI3132: Cleanup device node creation 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:37:23 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Tue, Mar 07, 2017 at 04:15:10PM -0600, Jeremy Linton wrote: > 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 > --- > .../Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c | 69 +++++++++++++--------- > 1 file changed, 41 insertions(+), 28 deletions(-) > > diff --git a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c > index 601583d..ce33438 100644 > --- a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c > +++ b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c > @@ -1,6 +1,6 @@ > /** @file > * > -* 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 > @@ -61,7 +61,7 @@ SiI3231DeviceReady ( > } 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. > + // Consider doing a device reset here. Whitespace-only change. > } > > return Timeout; > @@ -76,7 +76,8 @@ SiI3132IssueCommand ( > ) > { > UINT32 IrqMask; > - UINT32 Value32, Error; > + UINT32 Value32; > + UINT32 Error; Style-only change. > UINTN EmptySlot; > EFI_STATUS Status; > > @@ -89,15 +90,15 @@ SiI3132IssueCommand ( > 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); > + // TODO: Find which slot is free (maybe use the Cmd FIFO) > + // SATA_PORT_READ32 (SataPort->RegBase + SII3132_PORT_CMDEXECFIFO_REG, &EmptySlot); Whitespace-only changes. > 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")); > + DEBUG ((DEBUG_ERROR, "SiI3132AtaPassThru() Untested path\n")); Whitespace-only change. > Status = PciIo->Mem.Write (PciIo, EfiPciIoWidthUint32, 1, // Bar 1 > SataPort->RegBase + (EmptySlot * 0x80), > sizeof (SATA_SI3132_PRB) / 4, > @@ -128,7 +129,7 @@ SiI3132IssueCommand ( > StatusBlock); > > if (EFI_ERROR (Status)) { > - DEBUG ((DEBUG_ERROR, "SiI3132AtaPassThru() status (%d) block %X %X\n",Status, SataPort->RegBase, StatusBlock)); > + DEBUG ((DEBUG_ERROR, "SiI3132AtaPassThru() status (%d) block %X %X\n", Status, SataPort->RegBase, StatusBlock)); Whitespace-only change. > } > > if (Timeout == 0) { > @@ -146,7 +147,7 @@ SiI3132IssueCommand ( > 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)); > + Value32, Error, SataPort->HostPRB->Fis.Command, SataPort->HostPRB->Fis.Features, Serror)); Whitespace-only change. > > // clear port status > SATA_PORT_WRITE32 (SataPort->RegBase + SII3132_PORT_CONTROLSET_REG, SII3132_PORT_CONTROL_INT); > @@ -182,6 +183,7 @@ SiI3132AtaPassThruCommand ( > EFI_STATUS Status; > VOID* PciAllocMapping = NULL; > EFI_PCI_IO_PROTOCOL *PciIo; > + ATA_IDENTIFY_DATA *IdentifyData; > > PciIo = SataSiI3132Instance->PciIo; > ZeroMem (SataPort->HostPRB, sizeof (SATA_SI3132_PRB)); > @@ -311,7 +313,7 @@ SiI3132AtaPassThruCommand ( > { > // 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; > + IdentifyData = (ATA_IDENTIFY_DATA*)Packet->InDataBuffer; > > // Get the corresponding Block Device > SataDevice = GetSataDevice (SataSiI3132Instance, SataPort->Index, PortMultiplierPort); > @@ -446,9 +448,9 @@ SiI3132GetNextPort ( > *Port = 0; > } else { > if (PrevPort < SATA_SII3132_MAXPORT) { > - *Port = PrevPort + 1; > + *Port = PrevPort + 1; > } else { > - Status = EFI_NOT_FOUND; > + Status = EFI_NOT_FOUND; Whitespace-only changes. > } > } > return Status; > @@ -521,16 +523,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) { > @@ -589,8 +594,6 @@ SiI3132BuildDevicePath ( > SATA_SI3132_DEVICE *SataDevice; > EFI_DEVICE_PATH_PROTOCOL *SiI3132DevicePath; > > - SATA_TRACE ("SiI3132BuildDevicePath()"); > - > SataSiI3132Instance = INSTANCE_FROM_ATAPASSTHRU_THIS (This); > if (!SataSiI3132Instance) { > return EFI_INVALID_PARAMETER; > @@ -601,20 +604,30 @@ 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) { > + DEBUG ((DEBUG_INFO, "SiI3132BuildDevicePath() Atapi %d:%d\n", Port, PortMultiplierPort)); > + 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) > + DEBUG ((DEBUG_VERBOSE, "SiI3132BuildDevicePath() ATA %d:%d\n", Port, PortMultiplierPort)); > + 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 { > + ((SATA_DEVICE_PATH*)SiI3132DevicePath)->PortMultiplierPortNumber = 0; > + } > + ((SATA_DEVICE_PATH*)SiI3132DevicePath)->Lun = 0; // Only support lun0 on ATA > + } > *DevicePath = SiI3132DevicePath; > return EFI_SUCCESS; > } > @@ -680,7 +693,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; > } > > @@ -688,7 +701,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.9.3 >