From: Leif Lindholm <leif.lindholm@linaro.org>
To: Jeremy Linton <jeremy.linton@arm.com>
Cc: edk2-devel@lists.01.org, ryan.harkin@linaro.org,
linaro-uefi@lists.linaro.org, ard.biesheuvel@linaro.org
Subject: Re: [PATCH v4 4/6] EmbeddedPkg: SiI3132: Break out FIS command submission
Date: Fri, 17 Mar 2017 16:31:00 +0000 [thread overview]
Message-ID: <20170317163100.GH16034@bivouac.eciton.net> (raw)
In-Reply-To: <20170307221512.5180-5-jeremy.linton@arm.com>
On Tue, Mar 07, 2017 at 04:15:09PM -0600, Jeremy Linton 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 | 225 +++++++++++++--------
> OpenPlatformPkg | 2 +-
> 2 files changed, 138 insertions(+), 89 deletions(-)
>
> diff --git a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c
> index 2fb5fd6..601583d 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
> -) {
> + )
> +{
Whitespace-only change as part of functional patch.
> LIST_ENTRY *List;
> SATA_SI3132_PORT *SataPort;
> SATA_SI3132_DEVICE *SataDevice;
> @@ -44,6 +45,124 @@ GetSataDevice (
> return NULL;
> }
>
> +UINT32
> +SiI3231DeviceReady (
> + IN SATA_SI3132_PORT *SataPort,
> + IN EFI_PCI_IO_PROTOCOL *PciIo
> + )
> +{
> + UINT32 Value32;
> + UINT32 Timeout;
> + Timeout = SI_DEFAULT_TIMEOUT;
> +
> + do {
> + SATA_PORT_READ32 (SataPort->RegBase + SII3132_PORT_STATUS_REG, &Value32);
> + Timeout--;
> + } while (Timeout && !(Value32 & SII3132_PORT_STATUS_PORTREADY));
I would much prefer a blank line here.
> + if (Timeout == 0) {
> + DEBUG ((DEBUG_WARN, "SiI3132AtaPassThru() Device not ready, try anyway\n"));
I guess this code this is just being moved around within the same
file, so I'll refrain from commenting on a whole bunch of stuff in
this function.
> + } 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 +177,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));
> -
Whitespace-only change.
> // Construct Si3132 PRB
> switch (Packet->Protocol) {
> case EFI_ATA_PASS_THRU_PROTOCOL_ATA_HARDWARE_RESET:
> @@ -92,7 +207,8 @@ 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) ||
Why the redundant space after "if ("?
> + (Packet->Acb->AtaCommand == ATA_CMD_IDENTIFY_DEVICE) ) {
And before ") {"?
> InDataBufferLength = Packet->InTransferLength;
> } else {
> SataDevice = GetSataDevice (SataSiI3132Instance, SataPort->Index, PortMultiplierPort);
> @@ -108,6 +224,7 @@ SiI3132AtaPassThruCommand (
> Packet->InDataBuffer, &InDataBufferLength, &PhysInDataBuffer, &PciAllocMapping
> );
> if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "SiI map() failure %d\n", Status));
> return Status;
> }
>
> @@ -121,8 +238,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,81 +305,10 @@ SiI3132AtaPassThruCommand (
> SataPort->HostPRB->Control = Control;
> SataPort->HostPRB->ProtocolOverride = Protocol;
>
> - // 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
> - 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));
> - }
> + Status = SiI3132IssueCommand (SataPort, PciIo, Packet->Timeout, Packet->Asb);
>
> + if (!EFI_ERROR (Status))
> + {
"{" on line before.
> // 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;
> @@ -279,11 +325,14 @@ SiI3132AtaPassThruCommand (
> SataDevice->BlockSize = 0x200;
> }
> }
> - return EFI_SUCCESS;
> - } else {
> - ASSERT (0);
This ASSERT is not replaced in the new program flow.
Is this intentional?
> - return EFI_DEVICE_ERROR;
> }
> +
> + if (PciAllocMapping) {
> + Status = PciIo->Unmap (PciIo, PciAllocMapping);
> + }
> +
> + return Status;
> +
> }
>
> /**
> @@ -339,8 +388,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);
> --
> 2.9.3
>
next prev parent reply other threads:[~2017-03-17 16:31 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-03-07 22:15 [PATCH v4 0/6] ATAPI support on SiI SATA adapter Jeremy Linton
2017-03-07 22:15 ` [PATCH v4 1/6] EmbeddedPkg: SiI3132: Note that ARM is using this Dxe Jeremy Linton
2017-03-16 13:34 ` Leif Lindholm
2017-03-07 22:15 ` [PATCH v4 2/6] EmbeddedPkg: SiI3132: Add ScsiProtocol callbacks Jeremy Linton
2017-03-16 14:09 ` Leif Lindholm
2017-03-07 22:15 ` [PATCH v4 3/6] EmbeddedPkg: SiI3132: Add SCSI protocol support to header Jeremy Linton
2017-03-16 14:19 ` Leif Lindholm
2017-03-07 22:15 ` [PATCH v4 4/6] EmbeddedPkg: SiI3132: Break out FIS command submission Jeremy Linton
2017-03-17 16:31 ` Leif Lindholm [this message]
2017-03-07 22:15 ` [PATCH v4 5/6] EmbeddedPkg: SiI3132: Cleanup device node creation Jeremy Linton
2017-03-17 16:37 ` Leif Lindholm
2017-03-07 22:15 ` [PATCH v4 6/6] EmbeddedPkg: SiI3132: Enable SCSI pass-through protocol Jeremy Linton
2017-03-17 16:40 ` Leif Lindholm
2017-03-07 22:15 ` [PATCH] Platforms/ARM/Juno: Add " Jeremy Linton
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170317163100.GH16034@bivouac.eciton.net \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox