Re: [edk2-devel] [PATCH v1 1/1] MdePkg: Correcting EFI_ACPI_DMA_TRANSFER_TYPE_16_BIT definition

["Laszlo Ersek" <lersek@redhat.com>]

On 08/28/20 20:39, Grimes, Paul wrote:
> [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.    

Sure, I'm OK with the patch as posted.
Laszlo

> 
> Thanks,
> Paul
> 
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com> 
> Sent: Friday, August 28, 2020 10:06 AM
> To: devel@edk2.groups.io; Grimes, Paul <Paul.Grimes@amd.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Zhiguang Liu <zhiguang.liu@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&amp;data=02%7C01%7Cp
>> aul.grimes%40amd.com%7C82b28bb6544a4612fc1108d84b749dc6%7C3dd8961fe488
>> 4e608e11a82d994e183d%7C0%7C0%7C637342311528396385&amp;sdata=7vHYIHHaJU
>> 4yrXzAWtv5xTf%2BQfclAUBusz278%2F6I%2BRY%3D&amp;reserved=0
>>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
>> Signed-off-by: Paul G <paul.grimes@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@redhat.com>
> 
> I also agree it's a bugfix and should be merged now.
> 
> Thanks
> Laszlo
>