public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Sami Mujawar" <sami.mujawar@arm.com>
To: Abdul Lateef Attar <abdattar@amd.com>, devel@edk2.groups.io
Cc: Ray Ni <ray.ni@intel.com>, Zhichao Gao <zhichao.gao@intel.com>,
	nd <nd@arm.com>, Chris Jones <Christopher.Jones@arm.com>
Subject: Re: [edk2-devel] [PATCH v1] ShellPkg/AcpiView: PrintFormatter for FADT Flags field
Date: Mon, 15 Nov 2021 17:59:09 +0000	[thread overview]
Message-ID: <3837483b-cca3-8f37-a30e-018820a85520@arm.com> (raw)
In-Reply-To: <20211112132725.13810-1-abdattar@amd.com>

[-- Attachment #1: Type: text/plain, Size: 8101 bytes --]

Hi Abdul,

Thank you for contributing this patch.

Is it possible to introduce a new functionParseAcpiFieldBits () that can 
trace the bit fields, please? It may also be possible to reuse the 
ACPI_PARSER structure to specify the Field bits.

e.g. Keeping the meaning of the existing ACPI_PARSER field the same, the 
meaning of a few fields can be extended to also include the following.

i.e. when ACPI_PARSER is used to describe a Bit Field, then the 
following would apply.

ACPI_PARSER.Length => Length of the field bits in bits.

ACPI_PARSER.Offset => Field bit start position.

With this the FADT Flag bits can be described in an ACPI_PARSER array 
(similar to how the ACPI table fields are described).

e.g.

STATIC CONST ACPI_PARSER FadtFlagBitsParser[]= {
   {L"WBINVD", 1, 0, L"%d", NULL, NULL, NULL, NULL},
   {L"WBINVD_FLUSH", 1, 1, L"%d", NULL, NULL, NULL, NULL},
   {L"PROC_C1", 1, 2,L"%d", NULL, NULL, NULL, NULL},
...
};

The DumpFadtFlags() function could call ParseAcpiFieldBits 
(...,PARSER_PARAMS (FadtFlagBitsParser)).

ParseAcpiFieldBits() could then extract (mask and shift) the bits and 
print using the print formatter.

I think introducing ParseAcpiFieldBits() would allow the possibility of 
reusing the code as well as adding validations for the field bits.

Please do let me know if you have any queries and I would be happy to help.

Regards,

Sami Mujawar


On 12/11/2021 01:27 PM, Abdul Lateef Attar wrote:
> Adds PrintFormatter function to the FADT flags field.
> Prints indivisual flag name along with flag value.
>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Zhichao Gao <zhichao.gao@intel.com>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Signed-off-by: Abdul Lateef Attar <abdattar@amd.com>
> ---
>   ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c | 208 +++++++++++++++++++-
>   1 file changed, 207 insertions(+), 1 deletion(-)
>
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c
> index d86718bab67d..7f01412f1a9b 100644
> --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c
> @@ -2,6 +2,7 @@
>     FADT table parser
>
>   
>
>     Copyright (c) 2016 - 2020, ARM Limited. All rights reserved.
>
> +  Copyright (c) 2021, AMD Incorporated. All rights reserved.<BR>
>
>     SPDX-License-Identifier: BSD-2-Clause-Patent
>
>   
>
>     @par Reference(s):
>
> @@ -127,6 +128,211 @@ ValidateFlags (
>   #endif
>
>   }
>
>   
>
> +/**
>
> +  This function traces FADT Flags field.
>
> +
>
> +  @param [in] Format  Optional format string for tracing the data.
>
> +  @param [in] Ptr     Pointer to the start of the buffer.
>
> +**/
>
> +VOID
>
> +EFIAPI
>
> +DumpFadtFlags (
>
> +  IN CONST CHAR16* Format OPTIONAL,
>
> +  IN UINT8*        Ptr
>
> +  )
>
> +{
>
> +  if (Format != NULL) {
>
> +    Print (Format, *(UINT32*)Ptr);
>
> +    return;
>
> +  }
>
> +
>
> +  Print (L"0x%X\n", *(UINT32*)Ptr);
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +     L"WBINVD",
>
> +    *(UINT32*)Ptr & BIT0 ? 1 : 0
>
> +    );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"WBINVD_FLUSH",
>
> +    *(UINT32*)Ptr & BIT1 ? 1 : 0
>
> +    );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"PROC_C1",
>
> +    *(UINT32*)Ptr & BIT2 ? 1 : 0
>
> +    );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"P_LVL2_UP",
>
> +    *(UINT32*)Ptr & BIT3 ? 1 : 0
>
> +    );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"PWR_BUTTON",
>
> +    *(UINT32*)Ptr & BIT4 ? 1 : 0
>
> +    );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"SLP_BUTTON",
>
> +    *(UINT32*)Ptr & BIT5 ? 1 : 0
>
> +   );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"FIX_RTC",
>
> +    *(UINT32*)Ptr & BIT6 ? 1 : 0
>
> +   );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"RTC_S4",
>
> +    *(UINT32*)Ptr & BIT7 ? 1 : 0
>
> +   );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"TMR_VAL_EXT",
>
> +    *(UINT32*)Ptr & BIT8 ? 1 : 0
>
> +    );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"DCK_CAP",
>
> +    *(UINT32*)Ptr & BIT9 ? 1 : 0
>
> +    );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"RESET_REG_SUP",
>
> +    *(UINT32*)Ptr & BIT10 ? 1 : 0
>
> +    );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"SEALED_CASE",
>
> +    *(UINT32*)Ptr & BIT11 ? 1 : 0
>
> +    );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"HEADLESS",
>
> +    *(UINT32*)Ptr & BIT12 ? 1 : 0
>
> +    );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"CPU_SW_SLP",
>
> +    *(UINT32*)Ptr & BIT13 ? 1 : 0
>
> +    );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"PCI_EXP_WAK",
>
> +    *(UINT32*)Ptr & BIT14 ? 1 : 0
>
> +    );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"USE_PLATFORM_CLOCK",
>
> +    *(UINT32*)Ptr & BIT15 ? 1 : 0
>
> +    );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"S4_RTC_STS_VALID",
>
> +    *(UINT32*)Ptr & BIT16 ? 1 : 0
>
> +    );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"REMOTE_POWER_ON_CAPABLE",
>
> +    *(UINT32*)Ptr & BIT17 ? 1 : 0
>
> +    );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"FORCE_APIC_CLUSTER_MODEL",
>
> +    *(UINT32*)Ptr & BIT18 ? 1 : 0
>
> +    );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"FORCE_APIC_PHYSICAL_DESTINATION_MODE",
>
> +    *(UINT32*)Ptr & BIT19 ? 1 : 0
>
> +    );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"HW_REDUCED_ACPI",
>
> +    *(UINT32*)Ptr & BIT20 ? 1 : 0
>
> +    );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"LOW_POWER_S0_IDLE_CAPABLE",
>
> +    *(UINT32*)Ptr & BIT21 ? 1 : 0
>
> +    );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"Reserved",
>
> +    *(UINT32*)Ptr & BIT22 ? 1 : 0
>
> +    );
>
> +}
>
> +
>
>   /**
>
>     An ACPI_PARSER array describing the ACPI FADT Table.
>
>   **/
>
> @@ -170,7 +376,7 @@ STATIC CONST ACPI_PARSER FadtParser[] = {
>     {L"CENTURY", 1, 108, L"0x%x", NULL, NULL, NULL, NULL},
>
>     {L"IAPC_BOOT_ARCH", 2, 109, L"0x%x", NULL, NULL, NULL, NULL},
>
>     {L"Reserved", 1, 111, L"0x%x", NULL, NULL, NULL, NULL},
>
> -  {L"Flags", 4, 112, L"0x%x", NULL, (VOID**)&Flags, ValidateFlags, NULL},
>
> +  {L"Flags", 4, 112, NULL, DumpFadtFlags, (VOID**)&Flags, ValidateFlags, NULL},
>
>     {L"RESET_REG", 12, 116, NULL, DumpGas, NULL, NULL, NULL},
>
>     {L"RESET_VALUE", 1, 128, L"0x%x", NULL, NULL, NULL, NULL},
>
>     {L"ARM_BOOT_ARCH", 2, 129, L"0x%x", NULL, NULL, NULL, NULL},
>


[-- Attachment #2: Type: text/html, Size: 13215 bytes --]

      parent reply	other threads:[~2021-11-15 17:59 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-12 13:27 [edk2-devel] [PATCH v1] ShellPkg/AcpiView: PrintFormatter for FADT Flags field Abdul Lateef Attar
2021-11-15  5:49 ` Gao, Zhichao
2021-11-15 17:59 ` Sami Mujawar [this message]

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=3837483b-cca3-8f37-a30e-018820a85520@arm.com \
    --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