Re: [PATCH 04/10] platform/x86/intel/ifs: Scan test for new generations

[Joseph, Jithu]

On 9/19/2023 12:44 AM, Ilpo Järvinen wrote:
> On Fri, 15 Sep 2023, Joseph, Jithu wrote:
>> On 9/15/2023 9:51 AM, Ilpo Järvinen wrote:
>>> On Wed, 13 Sep 2023, Jithu Joseph wrote:
>>>
>>>> Make changes to scan test flow such that MSRs are populated
>>>> appropriately based on the generation supported by hardware.
>>>>
>>>> Width of chunk related bitfields is ACTIVATE_SCAN and SCAN_STATUS MSRs
>>>> are different in newer IFS generation compared to gen0.
>>>>
>>>> Signed-off-by: Jithu Joseph <jithu.joseph@xxxxxxxxx>
>>>> Reviewed-by: Tony Luck <tony.luck@xxxxxxxxx>
>>>> Tested-by: Pengfei Xu <pengfei.xu@xxxxxxxxx>
>>>> ---
>>>>  drivers/platform/x86/intel/ifs/ifs.h     | 14 ++++++++++++++
>>>>  drivers/platform/x86/intel/ifs/runtest.c | 23 ++++++++++++++++++-----
>>>>  2 files changed, 32 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
>>>> index 886dc74de57d..3265a6d8a6f3 100644
>>>> --- a/drivers/platform/x86/intel/ifs/ifs.h
>>>> +++ b/drivers/platform/x86/intel/ifs/ifs.h
>>>> @@ -205,6 +205,12 @@ union ifs_scan {
>>>>  		u32	delay	:31;
>>>>  		u32	sigmce	:1;
>>>>  	};
>>>> +	struct {
>>>> +		u16	start;
>>>> +		u16	stop;
>>>> +		u32	delay	:31;
>>>> +		u32	sigmce	:1;
>>>> +	} gen2;
>>>
>>> I don't like the way old struct is left without genx naming. It makes the 
>>> code below more confusing as is.
>>>
>>
>> Given that less than half the fields (2/4 in ifs_scan and 2/5 in ifs_status ) are changing across
>> generations(and rest are common) , I felt the code would be more readable if the common fields are
>> accessed without generation as is done now. 
>>
>> That said I don’t mind changing if you feel strongly about this
> 
> I would certainly prefer the generation dependent fields to marked as 
> such. However, it does not say you couldn't have the other fields remain 
> w/o gen.
> 
> How about this definition (it comes with the added benefit that you 
> cannot accidently use start/stop without specifying gen which guards 
> against one type of bugs):
> 

Yes this looks better. I will adopt this in v2.

> union ifs_scan {
>         u64     data;
>         struct {
> 		union {
> 			struct {
> 	        	        u8      start;
>         	        	u8      stop;
> 	        	        u16     rsvd;
> 			} gen0;
> 		        struct {
>                 		u16     start;
> 		                u16     stop;
> 			} gen2;
> 		};
>                 u32     delay   :31;
>                 u32     sigmce  :1;
>         };
> };
> 
> Note that I used start and stop in gen0 without the bitfield that
> seems unnecessary.
> 

Thanks
Jithu