public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Sami Mujawar" <sami.mujawar@arm.com>
To: Pranav Madhu <pranav.madhu@arm.com>, devel@edk2.groups.io
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>,
	Pierre Gondois <pierre.gondois@arm.com>
Subject: Re: [edk2-platforms][PATCH V2 2/8] Platform/Sgi: ACPI PPTT table for SGI-575 platform
Date: Mon, 10 May 2021 09:43:32 +0100	[thread overview]
Message-ID: <8ef1359e-b8be-76b5-82b6-4feded7bbcd5@arm.com> (raw)
In-Reply-To: <20210428121229.32674-3-pranav.madhu@arm.com>

Hi Pranav,

Please see my response inline marked [SAMI].

Regards,

Sami Mujawar


On 28/04/2021 01:12 PM, Pranav Madhu wrote:
> From: Pranav Madhu <Pranav.Madhu@arm.com>
>
> The SGI-575 platform includes two clusters with four single-thread CPUs.
> Each of the CPUs include 64KB L1 Data cache, 64KB L1 Instruction cache
> and 512KB L2 cache. Each cluster includes a 2MB L3 cache. Add PPTT table
> for SGI-575 platform with this information.
>
> Signed-off-by: Pranav Madhu <pranav.madhu@arm.com>
> ---
>   Platform/ARM/SgiPkg/AcpiTables/Sgi575AcpiTables.inf |   3 +-
>   Platform/ARM/SgiPkg/AcpiTables/Sgi575/Pptt.aslc     | 161 ++++++++++++++++++++
>   2 files changed, 163 insertions(+), 1 deletion(-)
>
> diff --git a/Platform/ARM/SgiPkg/AcpiTables/Sgi575AcpiTables.inf b/Platform/ARM/SgiPkg/AcpiTables/Sgi575AcpiTables.inf
> index 2121fd39f2f0..b1ee16e98ea3 100644
> --- a/Platform/ARM/SgiPkg/AcpiTables/Sgi575AcpiTables.inf
> +++ b/Platform/ARM/SgiPkg/AcpiTables/Sgi575AcpiTables.inf
> @@ -1,7 +1,7 @@
>   ## @file
>   #  ACPI table data and ASL sources required to boot the platform.
>   #
> -#  Copyright (c) 2018, ARM Ltd. All rights reserved.
> +#  Copyright (c) 2018 - 2021, ARM Ltd. All rights reserved.
>   #
>   #  SPDX-License-Identifier: BSD-2-Clause-Patent
>   #
> @@ -22,6 +22,7 @@
>     Mcfg.aslc
>     Sgi575/Dsdt.asl
>     Sgi575/Madt.aslc
> +  Sgi575/Pptt.aslc
>     Spcr.aslc
>     Ssdt.asl
>
> diff --git a/Platform/ARM/SgiPkg/AcpiTables/Sgi575/Pptt.aslc b/Platform/ARM/SgiPkg/AcpiTables/Sgi575/Pptt.aslc
> new file mode 100644
> index 000000000000..3388a012dd55
> --- /dev/null
> +++ b/Platform/ARM/SgiPkg/AcpiTables/Sgi575/Pptt.aslc
> @@ -0,0 +1,161 @@
> +/** @file
> +* Processor Properties Topology Table (PPTT) for SGI-575 platform
> +*
> +* This file describes the topological structure of the processor block on the
> +* SGI-575 platform in the form as defined by ACPI PPTT table. The SGI-575
> +* platform includes two clusters with four single-thread CPUS. Each of the CPUs
> +* include 64KB L1 Data cache, 64KB L1 Instruction cache and 512KB L2 cache.
> +* Each cluster includes a 2MB L3 cache.
> +*
> +* Copyright (c) 2021, ARM Limited. All rights reserved.
> +*
> +* SPDX-License-Identifier: BSD-2-Clause-Patent
> +*
> +* @par Specification Reference:
> +*   - ACPI 6.3, Chapter 5, Section 5.2.29, Processor Properties Topology Table
> +**/
> +
> +#include <IndustryStandard/Acpi.h>
> +#include <Library/AcpiLib.h>
> +#include <Library/ArmLib.h>
> +#include <Library/PcdLib.h>
> +
> +#include "SgiPlatform.h"
> +#include "SgiAcpiHeader.h"
> +
> +#define PPTT_CORE_INIT(pid, cid, cpuid)                                        \
[SAMI] Can you add a doxygen style header describing the parameters. As
it stands the meaning of the macro parameter is not intutive.
I can see the difficulty if PPTT_CORE_INIT() were to be a generic macro,
in which case it may be better to describe the parameters as affinity
levels.
However, in the current case this macro is local, so
  -  the macro could be modified to suit parameters specific to this file
OR
- Describe the macro parameters as affinity levels and add a Doxygen
function style comment header clarifying the parameter usage.

  [/SAMI]
> +  {                                                                            \
> +    /* Parameters for CPU Core */                                              \
> +    EFI_ACPI_6_3_PPTT_STRUCTURE_PROCESSOR_INIT (                               \
> +      OFFSET_OF (RD_PPTT_CORE, DCache),     /* Length */                       \
> +      PPTT_PROCESSOR_CORE_FLAGS,            /* Flag */                         \
> +      OFFSET_OF (EFI_ACPI_6_3_PROCESSOR_PROPERTIES_TOPOLOGY_TABLE,             \
> +        Package.Cluster[cid]),              /* Parent */                       \
> +      ((pid << 3) | (cid << 2) | cpuid),    /* ACPI Id */                      \
> +      2                                     /* Num of private resource */      \
> +    ),                                                                         \
> +                                                                               \
> +    /* Offsets of the private resources */                                     \
> +    {                                                                          \
> +      OFFSET_OF (EFI_ACPI_6_3_PROCESSOR_PROPERTIES_TOPOLOGY_TABLE,             \
> +        Package.Cluster[cid].Core[cpuid].DCache),                              \
> +      OFFSET_OF (EFI_ACPI_6_3_PROCESSOR_PROPERTIES_TOPOLOGY_TABLE,             \
> +        Package.Cluster[cid].Core[cpuid].ICache)                               \
> +    },                                                                         \
> +                                                                               \
> +    /* L1 data cache parameters */                                             \
> +    EFI_ACPI_6_3_PPTT_STRUCTURE_CACHE_INIT (                                   \
> +      PPTT_CACHE_STRUCTURE_FLAGS,           /* Flag */                         \
> +      OFFSET_OF (EFI_ACPI_6_3_PROCESSOR_PROPERTIES_TOPOLOGY_TABLE,             \
> +        Package.Cluster[cid].Core[cpuid].L2Cache),                             \
> +                                            /* Next level of cache */          \
> +      SIZE_64KB,                            /* Size */                         \
> +      64,                                   /* Num of sets */                  \
> +      16,                                   /* Associativity */                \
> +      PPTT_DATA_CACHE_ATTR,                 /* Attributes */                   \
> +      64                                    /* Line size */                    \
> +    ),                                                                         \
> +                                                                               \
> +    /* L1 instruction cache parameters */                                      \
> +    EFI_ACPI_6_3_PPTT_STRUCTURE_CACHE_INIT (                                   \
> +      PPTT_CACHE_STRUCTURE_FLAGS,           /* Flag */                         \
> +      OFFSET_OF (EFI_ACPI_6_3_PROCESSOR_PROPERTIES_TOPOLOGY_TABLE,             \
> +        Package.Cluster[cid].Core[cpuid].L2Cache),                             \
> +                                            /* Next level of cache */          \
> +      SIZE_64KB,                            /* Size */                         \
> +      256,                                  /* Num of sets */                  \
> +      4,                                    /* Associativity */                \
> +      PPTT_INST_CACHE_ATTR,                 /* Attributes */                   \
> +      64                                    /* Line size */                    \
> +    ),                                                                         \
> +                                                                               \
> +    /* L2 cache parameters */                                                  \
> +    EFI_ACPI_6_3_PPTT_STRUCTURE_CACHE_INIT (                                   \
> +      PPTT_CACHE_STRUCTURE_FLAGS,           /* Flag */                         \
> +      0,                                    /* Next level of cache */          \
> +      SIZE_512KB,                           /* Size */                         \
> +      1024,                                 /* Num of sets */                  \
> +      8,                                    /* Associativity */                \
> +      PPTT_UNIFIED_CACHE_ATTR,              /* Attributes */                   \
> +      64                                    /* Line size */                    \
> +    ),                                                                         \
> +  }
> +
> +#define PPTT_CLUSTER_INIT(pid, cid)                                            \
> +  {                                                                            \
> +    /* Parameters for Cluster */                                               \
> +    EFI_ACPI_6_3_PPTT_STRUCTURE_PROCESSOR_INIT (                               \
> +      OFFSET_OF (RD_PPTT_CLUSTER, L3Cache),                                    \
> +                                            /* Length */                       \
> +      PPTT_PROCESSOR_CLUSTER_FLAGS,         /* Flag */                         \
> +      OFFSET_OF (EFI_ACPI_6_3_PROCESSOR_PROPERTIES_TOPOLOGY_TABLE,             \
> +        Package),                           /* Parent */                       \
> +      0,                                    /* ACPI Id */                      \
> +      1                                     /* Num of private resource */      \
> +    ),                                                                         \
> +                                                                               \
> +    /* Offsets of the private resources */                                     \
> +    OFFSET_OF (EFI_ACPI_6_3_PROCESSOR_PROPERTIES_TOPOLOGY_TABLE,               \
> +      Package.Cluster[cid].L3Cache),                                           \
> +                                                                               \
> +    /* L3 cache parameters */                                                  \
> +    EFI_ACPI_6_3_PPTT_STRUCTURE_CACHE_INIT (                                   \
> +      PPTT_CACHE_STRUCTURE_FLAGS,           /* Flag */                         \
> +      0,                                    /* Next level of cache */          \
> +      SIZE_2MB,                             /* Size */                         \
> +      2048,                                 /* Num of sets */                  \
> +      16,                                   /* Associativity */                \
> +      PPTT_UNIFIED_CACHE_ATTR,              /* Attributes */                   \
> +      64                                    /* Line size */                    \
> +    ),                                                                         \
> +                                                                               \
> +    /* Initialize child cores */                                               \
> +    {                                                                          \
> +      PPTT_CORE_INIT (pid, cid, 0),                                            \
> +      PPTT_CORE_INIT (pid, cid, 1),                                            \
> +      PPTT_CORE_INIT (pid, cid, 2),                                            \
> +      PPTT_CORE_INIT (pid, cid, 3)                                             \
> +    }                                                                          \
> +  }
> +
> +#pragma pack(1)
> +typedef struct {
> +  EFI_ACPI_6_3_PPTT_STRUCTURE_PROCESSOR  Package;
> +  RD_PPTT_CLUSTER                        Cluster[CLUSTER_COUNT];
> +} SGI575_PPTT_PACKAGE;
> +
> +/*
> + * Processor Properties Topology Table
> + */
> +typedef struct {
> +  EFI_ACPI_6_3_PROCESSOR_PROPERTIES_TOPOLOGY_TABLE_HEADER  Header;
> +  SGI575_PPTT_PACKAGE                                      Package;
> +} EFI_ACPI_6_3_PROCESSOR_PROPERTIES_TOPOLOGY_TABLE;
> +#pragma pack ()
> +
> +STATIC EFI_ACPI_6_3_PROCESSOR_PROPERTIES_TOPOLOGY_TABLE Pptt = {
> +  {
> +    ARM_ACPI_HEADER (
> +      EFI_ACPI_6_3_PROCESSOR_PROPERTIES_TOPOLOGY_TABLE_STRUCTURE_SIGNATURE,
> +      EFI_ACPI_6_3_PROCESSOR_PROPERTIES_TOPOLOGY_TABLE,
> +      EFI_ACPI_6_3_PROCESSOR_PROPERTIES_TOPOLOGY_TABLE_REVISION
> +    )
> +  },
> +
> +  {
> +    EFI_ACPI_6_3_PPTT_STRUCTURE_PROCESSOR_INIT (
> +      OFFSET_OF (SGI575_PPTT_PACKAGE, Cluster[0]),
> +      PPTT_PROCESSOR_PACKAGE_FLAGS, 0, 0, 0
> +    ),
> +    {
> +      PPTT_CLUSTER_INIT (0, 0),
> +      PPTT_CLUSTER_INIT (0, 1)
> +    }
> +  }
> +};
> +
> +/*
> + * Reference the table being generated to prevent the optimizer from removing
> + * the data structure from the executable
> + */
> +VOID* CONST ReferenceAcpiTable = &Pptt;

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.

  reply	other threads:[~2021-05-10  8:43 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-28 12:12 [edk2-platforms][PATCH V2 0/8] Platform/Sgi: Add PPTT table for Neoverse Reference Design platforms Pranav Madhu
2021-04-28 12:12 ` [edk2-platforms][PATCH V2 1/8] Platform/Sgi: Helper macros for PPTT Table Pranav Madhu
2021-05-10  8:43   ` Sami Mujawar
2021-04-28 12:12 ` [edk2-platforms][PATCH V2 2/8] Platform/Sgi: ACPI PPTT table for SGI-575 platform Pranav Madhu
2021-05-10  8:43   ` Sami Mujawar [this message]
2021-04-28 12:12 ` [edk2-platforms][PATCH V2 3/8] Platform/Sgi: ACPI PPTT table for RD-N1-Edge platform Pranav Madhu
2021-04-28 12:12 ` [edk2-platforms][PATCH V2 4/8] Platform/Sgi: ACPI PPTT table for RD-N1-Edge dual-chip Pranav Madhu
2021-04-28 12:12 ` [edk2-platforms][PATCH V2 5/8] Platform/Sgi: ACPI PPTT table for RD-E1-Edge platform Pranav Madhu
2021-04-28 12:12 ` [edk2-platforms][PATCH V2 6/8] Platform/Sgi: ACPI PPTT Table for RD-V1 platform Pranav Madhu
2021-04-28 12:12 ` [edk2-platforms][PATCH V2 7/8] Platform/Sgi: ACPI PPTT Table for RD-V1 quad-chip platform Pranav Madhu
2021-04-28 12:12 ` [edk2-platforms][PATCH V2 8/8] Platform/Sgi: ACPI PPTT table for RD-N2 platform Pranav Madhu
2021-05-03 14:25 ` [edk2-devel] [edk2-platforms][PATCH V2 0/8] Platform/Sgi: Add PPTT table for Neoverse Reference Design platforms Thomas Abraham
2021-05-04  9:44   ` PierreGondois
2021-05-10  9:22 ` Sami Mujawar

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=8ef1359e-b8be-76b5-82b6-4feded7bbcd5@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