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
