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
>> };
>>
>> /* MSR_SCAN_STATUS bit fields */
>> @@ -219,6 +225,14 @@ union ifs_status {
>> u32 control_error :1;
>> u32 signature_error :1;
>> };
>> + struct {
>> + u16 chunk_num;
>> + u16 chunk_stop_index;
>> + u8 error_code;
>> + u32 rsvd1 :22;
>> + u32 control_error :1;
>> + u32 signature_error :1;
>
> Again, I don't think the alignment will be correct in this case.
>
I hope this is clarified in the reply in patch03/10
...
>> @@ -211,7 +222,9 @@ static void ifs_test_core(int cpu, struct device *dev)
>> }
>> } else {
>> retries = MAX_IFS_RETRIES;
>> - activate.start = status.chunk_num;
>> + ifsd->generation ? (activate.gen2.start = status_chunk) :
>> + (activate.start = status_chunk);
>
> Misaligned.
Will align it to start
Jithu
