Re: [edk2-devel] [PATCH v4 6/6] DynamicTablesPkg: AcpiSsdtPcieLibArm: Added case handling for PCI config

["Kun Qin" <kuqin12@gmail.com>]

Hi Sami,

Now that the merge window opens, could you please help us to merge in
these patch series (after dropping this specific patch) when you have a 
chance?

Please let me know if you ran into issues when doing that. Thanks in 
advance!

Regards,
Kun

On 8/17/2022 10:42 AM, Kun Qin wrote:
> Hi Sami,
>
> Thank you for the help! I agree that we can drop this patch and merge 
> the rest when the window is open.
>
> Pierre,
>
> Thanks for your input on the usage as well!
>
> Regards,
> Kun
>
> On 8/17/2022 5:06 AM, Sami Mujawar wrote:
>> Hi Kun,
>>
>> I plan to get this series merged when the merge window opens.
>>
>> If you agree, I will drop this patch before merging. Please let me 
>> know if that is ok.
>>
>> Regards,
>>
>> Sami Mujawar
>>
>> On 17/08/2022 09:53 am, Pierre Gondois wrote:
>>>
>>>
>>> On 8/17/22 02:17, Kun Qin wrote:
>>>> Hi Pierre,
>>>>
>>>> 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.
>>>>
>>>> On the other hand, just for my learning purpose, could you please 
>>>> let me know
>>>> what the use case for "PCI_SS_CONFIG" is? It does not seem to be 
>>>> used at all.
>>>
>>> I haven't seen any usecase neither so far, but it is to be used to 
>>> reference a
>>> PCI bus/device/function/register location to identify/configure a 
>>> device
>>> from what I understood.
>>>
>>>>
>>>> Thanks again for testing these patches!
>>>>
>>>> Regards,
>>>> Kun
>>>>
>>>> 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=3998
>>>>>>
>>>>>> This change added a switch case handling for PCI_SS_CONFIG during 
>>>>>> SSDT
>>>>>> generation. This will allow PCI config case return EFI_SUCCESS 
>>>>>> instead of
>>>>>> EFI_INVALID_PARAMETER.
>>>>>>
>>>>>> Cc: Sami Mujawar <Sami.Mujawar@arm.com>
>>>>>> Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>
>>>>>>
>>>>>> Co-authored-by: Joe Lopez <joelopez@microsoft.com>
>>>>>> Signed-off-by: Kun Qin <kuqin12@gmail.com>
>>>>>> Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
>>>>>> Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
>>>>>> ---
>>>>>>
>>>>>> Notes:
>>>>>>      v2:
>>>>>>      - Added Reviewed-by tag [Pierre]
>>>>>>           v3:
>>>>>>      - No change
>>>>>>           v4:
>>>>>>      - Added Reviewed-by tag [Sami]
>>>>>>
>>>>>> DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c 
>>>>>> | 5 +++++
>>>>>>   1 file changed, 5 insertions(+)
>>>>>>
>>>>>> diff --git 
>>>>>> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c 
>>>>>> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c 
>>>>>>
>>>>>> index dd75fc27e60e..c6fbd09c43f8 100644
>>>>>> --- 
>>>>>> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
>>>>>> +++ 
>>>>>> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
>>>>>> @@ -606,6 +606,11 @@ GeneratePciCrs (
>>>>>>                      );
>>>>>>           break;
>>>>>>   +      case PCI_SS_CONFIG:
>>>>>> +        // Do nothing
>>>>>> +        Status = EFI_SUCCESS;
>>>>>> +        break;
>>>>>> +
>>>>>>         default:
>>>>>>           Status = EFI_INVALID_PARAMETER;
>>>>>>       } // switch
>>
>>
>> 
>>
>>