From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from mail-wr1-f47.google.com (mail-wr1-f47.google.com [209.85.221.47])
 by mx.groups.io with SMTP id smtpd.web12.12084.1632825278799413170
 for <devel@edk2.groups.io>;
 Tue, 28 Sep 2021 03:34:39 -0700
Authentication-Results: mx.groups.io;
 dkim=pass header.i=@nuviainc-com.20210112.gappssmtp.com header.s=20210112 header.b=7oLvyS9O;
 spf=pass (domain: nuviainc.com, ip: 209.85.221.47, mailfrom: leif@nuviainc.com)
Received: by mail-wr1-f47.google.com with SMTP id s21so17123817wra.7
        for <devel@edk2.groups.io>; Tue, 28 Sep 2021 03:34:38 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
        d=nuviainc-com.20210112.gappssmtp.com; s=20210112;
        h=date:from:to:cc:subject:message-id:references:mime-version
         :content-disposition:in-reply-to;
        bh=xRXg1uFZmjDypxKjRezFk+pqWl7mBhhgEEHy0WYdGhI=;
        b=7oLvyS9OSRG+HKN0+tu5DVngUtBFJW6cKwgLzdmEPWjKAiJVTq7Do1aIFfs+hHE+5B
         NpASZn1jCufb/LScYjrtGg/UA5wByddR0dTX3xf5a6byMLlLQPsnGaF5tRskWY+aRedX
         gsZayap1qW+4gB+kgHCECS1LwArUKyNOIRiyQpd1n68EfIc502vtwfqW2L0NrCIGkdCY
         Zrtbfox5nJtH0KVBnPpT/ZgH8ocvM9MlVVt/M3DLIZ7r018aJk+brxodIzAY8ke9dQg8
         zRwXlIEuSCf5iHwtoJ+9rEhLPtFi7VwdAe3AOyIqZY2OwFHOXqK7N3r6KU/AF39S0DZy
         T8zA==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
        d=1e100.net; s=20210112;
        h=x-gm-message-state:date:from:to:cc:subject:message-id:references
         :mime-version:content-disposition:in-reply-to;
        bh=xRXg1uFZmjDypxKjRezFk+pqWl7mBhhgEEHy0WYdGhI=;
        b=ZQpZepuFe6erPBvqqHhm9OZ8bKwTO/zgG0S8PM4WMODAiivkkO+qGLUPVf05rfU/c/
         jQDFVTV4ZerN0Pk1sRjHqYa+O4ks9olIyuKbrDciLYdBYsVWHEfe0aX/Hk6SvS3v29FH
         BxitJeKL8jQErgIIP5NT09MaVHNsxiSdtnDMCezA94rAoFUPJbd/QUmzVt6Ient90s/O
         Gvos1eYp/XBhSPznalmlVkZPBW5eLq884DrJrHSbET6EPgA9PRB237xUycvm8tf3cRM8
         sJEYG0PCDedaqrIitDgRjeDqC5F/xXVY54UlVX/8PAWD3vJJinGLeHA5V3ZSQ9hxaxVj
         tDZA==
X-Gm-Message-State: AOAM530gYXwxh4z8FdSFmVs4JY6xttdlE2TbZtkVfHft1pIhDK2XxR9H
	VmioYN9vGkg2Sq8xRH/Wc0aQOg==
X-Google-Smtp-Source: ABdhPJzTi6ZCKEMhsN6R4hCUt/Mfpa1lxMJRU7AsITuMrGcU5DBTuS5Vlyan60PB3gPuo58aGFyPaw==
X-Received: by 2002:a5d:6c6e:: with SMTP id r14mr5411742wrz.319.1632825277335;
        Tue, 28 Sep 2021 03:34:37 -0700 (PDT)
Return-Path: <leif@nuviainc.com>
Received: from leviathan (cpc92314-cmbg19-2-0-cust559.5-4.cable.virginm.net. [82.11.186.48])
        by smtp.gmail.com with ESMTPSA id k22sm20191165wrd.59.2021.09.28.03.34.36
        (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);
        Tue, 28 Sep 2021 03:34:36 -0700 (PDT)
Date: Tue, 28 Sep 2021 11:34:35 +0100
From: "Leif Lindholm" <leif@nuviainc.com>
To: Nhi Pham <nhi@os.amperecomputing.com>
Cc: devel@edk2.groups.io, patches@amperecomputing.com,
	vunguyen@os.amperecomputing.com,
	Thang Nguyen <thang@os.amperecomputing.com>,
	Chuong Tran <chuong@os.amperecomputing.com>,
	Phong Vo <phong@os.amperecomputing.com>,
	Michael D Kinney <michael.d.kinney@intel.com>,
	Ard Biesheuvel <ardb+tianocore@kernel.org>,
	Nate DeSimone <nathaniel.l.desimone@intel.com>
Subject: Re: [PATCH v3 12/28] AmpereAltraPkg: Add Ac01PcieLib library instance
Message-ID: <20210928103435.6k7hncti3j2tnicf@leviathan>
References: <20210915155527.8176-1-nhi@os.amperecomputing.com>
 <20210915155527.8176-13-nhi@os.amperecomputing.com>
MIME-Version: 1.0
In-Reply-To: <20210915155527.8176-13-nhi@os.amperecomputing.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

Apart from the request to break out Ac01PcieConfigRW and
Ac01PcieCfgIn/Out# I noticed a further thing.

On Wed, Sep 15, 2021 at 22:55:11 +0700, Nhi Pham wrote:
> +/**
> +  Get RootBridge disable status.
> +
> +  @param[in]  HBIndex               Index to identify of PCIE Host bridge.
> +  @param[in]  RBIndex               Index to identify of underneath PCIE Root bridge.
> +
> +  @retval BOOLEAN                   Return RootBridge disable status.
> +**/
> +BOOLEAN
> +Ac01PcieCheckRootBridgeDisabled (
> +  IN UINTN HBIndex,
> +  IN UINTN RBIndex
> +  )
> +{
> +  UINTN RCIndex;
> +  INT8  Ret;
> +
> +  RCIndex = HBIndex;
> +  Ret = !RCList[RCIndex].Active;
> +  if (Ret) {
> +    PciList[HBIndex] = -1;
> +  } else {
> +    PciList[HBIndex] = HBIndex;
> +  }
> +  if (HBIndex == (AC01_MAX_PCIE_ROOT_COMPLEX -1)) {
> +    SortPciList (PciList);
> +    if (!IsSlaveSocketPresent ()) {
> +      AcpiPatchPciMem32 (PciList);
> +    }
> +    AcpiInstallMcfg (PciList);
> +    AcpiInstallIort (PciList);

Should a function named "Check if RootBridge is Disabled" really have
the undocumented side effect of going off and installing ACPI tables?

Please move that logic over to PciHostBridgeReadyToBootEvent.

With that, I think this revision is fully reviewed.

/
    Leif

> +  }
> +  return Ret;
> +}