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):
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.
--
i.
