[edk2-devel] [PATCH v1 1/1] MdePkg: Correcting EFI_ACPI_DMA_TRANSFER_TYPE_16_BIT definition
Paul
paul.grimes at amd.com
Fri Aug 28 18:39:35 UTC 2020
[AMD Public Use]
Thanks for the feedback, Lazlo. I agree with your point on the optimal format for these #defines. I think it would be best to submit the current patch as is given that the same feedback could (should?) be applied to various other #defines in the file,
eg:
EFI_ACPI_DMA_BUS_MASTER_MASK 0x04, which only applies to Bit 2 and
EFI_ACPI_IRQ_POLARITY_MASK 0x08, ... Bit 3 and
EFI_ACPI_IRQ_MODE 0x01, ... bit 0
IMO if these defines were to be updated for clarity, it should probably be done for the whole file in a separate commit.
Thanks,
Paul
-----Original Message-----
From: Laszlo Ersek <lersek at redhat.com>
Sent: Friday, August 28, 2020 10:06 AM
To: devel at edk2.groups.io; Grimes, Paul <Paul.Grimes at amd.com>
Cc: Michael D Kinney <michael.d.kinney at intel.com>; Liming Gao <gaoliming at byosoft.com.cn>; Zhiguang Liu <zhiguang.liu at intel.com>
Subject: Re: [edk2-devel] [PATCH v1 1/1] MdePkg: Correcting EFI_ACPI_DMA_TRANSFER_TYPE_16_BIT definition
[CAUTION: External Email]
On 08/27/20 22:40, Paul wrote:
> In Acpi10.h, EFI_ACPI_DMA_TRANSFER_TYPE_16_BIT is defined as 0x10, but
> should be 0x02 per the ACPI Specification.
>
> REF:https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> bugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2937&data=02%7C01%7Cp
> aul.grimes%40amd.com%7C82b28bb6544a4612fc1108d84b749dc6%7C3dd8961fe488
> 4e608e11a82d994e183d%7C0%7C0%7C637342311528396385&sdata=7vHYIHHaJU
> 4yrXzAWtv5xTf%2BQfclAUBusz278%2F6I%2BRY%3D&reserved=0
>
> Cc: Michael D Kinney <michael.d.kinney at intel.com>
> Cc: Liming Gao <gaoliming at byosoft.com.cn>
> Cc: Zhiguang Liu <zhiguang.liu at intel.com>
> Signed-off-by: Paul G <paul.grimes at amd.com>
> ---
> MdePkg/Include/IndustryStandard/Acpi10.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/MdePkg/Include/IndustryStandard/Acpi10.h
> b/MdePkg/Include/IndustryStandard/Acpi10.h
> index fa06eefbb6e6..adeb5ae8c219 100644
> --- a/MdePkg/Include/IndustryStandard/Acpi10.h
> +++ b/MdePkg/Include/IndustryStandard/Acpi10.h
> @@ -358,7 +358,7 @@ typedef struct {
> #define EFI_ACPI_DMA_TRANSFER_TYPE_MASK 0x03
>
> #define EFI_ACPI_DMA_TRANSFER_TYPE_8_BIT 0x00
>
> #define EFI_ACPI_DMA_TRANSFER_TYPE_8_BIT_AND_16_BIT 0x01
>
> -#define EFI_ACPI_DMA_TRANSFER_TYPE_16_BIT 0x10
>
> +#define EFI_ACPI_DMA_TRANSFER_TYPE_16_BIT 0x02
>
>
>
> //
>
> // IO Information
>
Good catch. The ACPI spec text was likely cut n' pasted into the edk2 source, and then prefixed with "0x". The spec says,
"""
Bits [1:0] DMA transfer type preference, _SIZ
00 8-bit only
01 8- and 16-bit
10 16-bit only
11 Reserved
"""
but that's in binary, not in hexadecimal.
In fact, the leading zero on *all four* macros (including
EFI_ACPI_DMA_TRANSFER_TYPE_MASK) is misleading. In hex, the leading zero in the current macros stands for bits [7:4], which are completely irrelevant for the _SIZ bit-field in the DMA Descriptor. So optimally we'd have
#define EFI_ACPI_DMA_TRANSFER_TYPE_MASK 0x3
#define EFI_ACPI_DMA_TRANSFER_TYPE_8_BIT 0x0
#define EFI_ACPI_DMA_TRANSFER_TYPE_8_BIT_AND_16_BIT 0x1
#define EFI_ACPI_DMA_TRANSFER_TYPE_16_BIT 0x2
But I agree the current patch is OK too:
Reviewed-by: Laszlo Ersek <lersek at redhat.com>
I also agree it's a bugfix and should be merged now.
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#64766): https://edk2.groups.io/g/devel/message/64766
Mute This Topic: https://groups.io/mt/76462757/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list