Re: [PATCH 0/8] ATAPI support on SiI SATA adapter

[Jeremy Linton <Jeremy.Linton@arm.com>]

|-----Original Message-----
|From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
|Sent: Tuesday, November 15, 2016 8:58 AM
|To: Jeremy Linton
|Cc: edk2-devel-01; Steve Capper; Leif Lindholm; ryan.harkin@linaro.org;
|linaro-uefi
|Subject: Re: [edk2] [PATCH 0/8] ATAPI support on SiI SATA adapter
|
|On 15 November 2016 at 14:54, Jeremy Linton <jeremy.linton@arm.com>
|wrote:
|> On 11/15/2016 01:43 AM, Ard Biesheuvel wrote:
|>>
|>> Hi Jeremy,
|>>
|>> On 14 November 2016 at 21:09, Jeremy Linton <jeremy.linton@arm.com>
|wrote:
|>>>
|>>> The SiI isn't an AHCI compatible adapter so it implements the EFI
|>>> ATA pass-through protocol directly. This works for fixed hard
|>>> drives, but not ATAPI attached devices (CDROM, DVDROM, TAPE, etc).
|>>>
|>>> This patch adds read only ATAPI support via the EFI SCSI
|>>> pass-through protocol, allowing boot from attached CD/DVD. This
|>>> patch also cleans up, and tweaks recovery paths/etc in the original driver.
|>>
|>>
|>> Very nice! Thanks for getting to the bottom of this.
|>>
|>> However, looking at the patches, they are riddled with coding style
|>> violations. I am usually less strict than Leif when it comes to
|>> upholding those, but these patches really need to be cleaned up to be
|>> considered for merging.
|>
|>
|>
|> Is there a tool which can correct or at least point out the formatting
|> errors?
|>
|
|Yes, BaseTools/Scripts/PatchCheck.py

I did use that before submission, the only thing it complained about was an error caused by the git submodule commit id not having a CR (which AFAIK is bogus)

[jlinton@mammon-v1 edk2]$ python BaseTools/Scripts/PatchCheck.py -8 |more
Checking git commit: 4a9a9d7
The commit message format passed all checks.
The code passed all checks.

Checking git commit: 8537b6c
The commit message format passed all checks.
The code passed all checks.

Checking git commit: 458452e
The commit message format passed all checks.
The code passed all checks.

Checking git commit: ca0606f
The commit message format passed all checks.
Code format is not valid:
 * Line ending ('\n') is not CRLF
   File: OpenPlatformPkg
   Line: Subproject commit a072474a3b05ec7f12f2d4db271cc07a0cf7a369

Checking git commit: 0d9942f
The commit message format passed all checks.
The code passed all checks.

Checking git commit: b516f7a
The commit message format passed all checks.
The code passed all checks.

Checking git commit: 2259641
The commit message format passed all checks.
The code passed all checks.

Checking git commit: 61be9c2
The commit message format passed all checks.
The code passed all checks.

|
|>
|>>
|>>> When
|>>> combined with the ARM/PCI dma lib changes this allows us to relax
|>>> the IO alignment requirement that caused grub failures.
|>>>
|>>
|>> What changes are you referring to here?
|>
|>
|> I believe on juno the PCI changed from the ArmDmaLib to the null lib
|> or some such, which removed the bounce buffering on unaligned
|map/unmap.
|>
|
|Indeed, it removed the use of uncached memory, which does not work for
|coherent masters.
|I only realize now that the Sil3132 is a coherent PCI device, and that you were
|using 4 KB IoAlign to work around the non-coherent nature of ArmDmaLib
|
|
|>
|>>
|>>> Finally, the OpenPlatformPkg/Juno must be updated, with another
|>>> patch to avoid build breaks now that the SiI has a dependency on the
|>>> SCSI libraries.
|>>>
|>>> Contributed-under: TianoCore Contribution Agreement 1.0
|>>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
|>>>
|>>> Jeremy Linton (7):
|>>>   MdePkg IndustryStandard/Scsi.h: Add sense code macro
|>>>   EmbeddedPkg: SiI3132: Add ScsiProtocol callbacks
|>>>   EmbeddedPkg: SiI3132: Add SCSI protocol support to header
|>>>   EmbeddedPkg: SiI3132: Break out FIS command submission
|>>>   EmbeddedPkg: SiI3132: Cleanup device node creation
|>>>   EmbeddedPkg: SiI3132: Enable SCSI pass-through protocol
|>>>   EmbeddedPkg: SiI3132: Correct the IoAlign
|>>>
|>>>  EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c   |  48 ++-
|>>>  EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h   |  89 ++++-
|>>>  .../Drivers/SataSiI3132Dxe/SataSiI3132Dxe.inf      |   2 +
|>>>  .../Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c    | 270 ++++++++------
|>>>  .../Drivers/SataSiI3132Dxe/SiI3132ScsiPassThru.c   | 401
|>>> +++++++++++++++++++++
|>>>  MdePkg/Include/IndustryStandard/Scsi.h             |   2 +
|>>>  OpenPlatformPkg                                    |   2 +-
|>>>  7 files changed, 688 insertions(+), 126 deletions(-)  create mode
|>>> 100644 EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132ScsiPassThru.c
|>>>
|>>> --
|>>> 2.5.5
|>>>
|>>> _______________________________________________
|>>> edk2-devel mailing list
|>>> edk2-devel@lists.01.org
|>>> https://lists.01.org/mailman/listinfo/edk2-devel
|>
|>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.