From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web09.26937.1660726400273186284 for ; Wed, 17 Aug 2022 01:53:20 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: pierre.gondois@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 8D7A4106F; Wed, 17 Aug 2022 01:53:20 -0700 (PDT) Received: from [10.34.100.114] (pierre123.nice.arm.com [10.34.100.114]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 09FFA3F67D; Wed, 17 Aug 2022 01:53:18 -0700 (PDT) Message-ID: <46f86936-9ec3-58a4-a0f6-47e5bedbf630@arm.com> Date: Wed, 17 Aug 2022 10:53:23 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [PATCH v4 6/6] DynamicTablesPkg: AcpiSsdtPcieLibArm: Added case handling for PCI config To: Kun Qin , devel@edk2.groups.io Cc: Sami Mujawar , Alexei Fedorov , Joe Lopez References: <20220810222853.1916-1-kuqin12@gmail.com> <20220810222853.1916-7-kuqin12@gmail.com> From: "PierreGondois" In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable On 8/17/22 02:17, Kun Qin wrote: > Hi Pierre, >=20 > You are correct that if CM_ARM_PCI_ADDRESS_MAP_INFO.PCI_SS_CONFIG > is no longer being used, this patch is not needed. Thanks for catching = this. >=20 > On the other hand, just for my learning purpose, could you please let m= e know > what the use case for "PCI_SS_CONFIG" is? It does not seem to be used a= t all. I haven't seen any usecase neither so far, but it is to be used to refere= nce a PCI bus/device/function/register location to identify/configure a device from what I understood. >=20 > Thanks again for testing these patches! >=20 > Regards, > Kun >=20 > On 8/16/2022 8:33 AM, Pierre Gondois wrote: >> Hello Kun, >> >> Is this patch still required ? >> Cf: https://edk2.groups.io/g/devel/message/92204 >> >> The CM_ARM_PCI_CONFIG_SPACE_INFO struct should be enough to describe >> the PCI ECAM, so CM_ARM_PCI_ADDRESS_MAP_INFO.SpaceCode being set to >> PCI_SS_CONFIG should be an invalid case. >> If not I don't think a v5 should be necessary. >> >> Also I ran the patchset on KvmTool and everything was working. >> >> Regards, >> Pierre >> >> On 8/11/22 00:28, Kun Qin wrote: >>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D3998 >>> >>> This change added a switch case handling for PCI_SS_CONFIG during SSD= T >>> generation. This will allow PCI config case return EFI_SUCCESS instea= d of >>> EFI_INVALID_PARAMETER. >>> >>> Cc: Sami Mujawar >>> Cc: Alexei Fedorov >>> >>> Co-authored-by: Joe Lopez >>> Signed-off-by: Kun Qin >>> Reviewed-by: Pierre Gondois >>> Reviewed-by: Sami Mujawar >>> --- >>> >>> Notes: >>> =C2=A0=C2=A0=C2=A0=C2=A0 v2: >>> =C2=A0=C2=A0=C2=A0=C2=A0 - Added Reviewed-by tag [Pierre] >>> =C2=A0=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0 v3: >>> =C2=A0=C2=A0=C2=A0=C2=A0 - No change >>> =C2=A0=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0 v4: >>> =C2=A0=C2=A0=C2=A0=C2=A0 - Added Reviewed-by tag [Sami] >>> >>> DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerato= r.c | 5 +++++ >>> =C2=A0 1 file changed, 5 insertions(+) >>> >>> diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/Ssd= tPcieGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/S= sdtPcieGenerator.c >>> index dd75fc27e60e..c6fbd09c43f8 100644 >>> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGe= nerator.c >>> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGe= nerator.c >>> @@ -606,6 +606,11 @@ GeneratePciCrs ( >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ); >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 break; >>> =C2=A0 +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 case PCI_SS_CONFIG: >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // Do nothing >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Status =3D EFI_SUCCESS; >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 break; >>> + >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 default: >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Status =3D EFI= _INVALID_PARAMETER; >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } // switch