Messages in this thread | | | Date | Tue, 19 Sep 2023 10:44:14 +0300 (EEST) | From | Ilpo Järvinen <> | Subject | Re: [PATCH 04/10] platform/x86/intel/ifs: Scan test for new generations |
| |
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@intel.com> > >> Reviewed-by: Tony Luck <tony.luck@intel.com> > >> Tested-by: Pengfei Xu <pengfei.xu@intel.com> > >> --- > >> 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.
| |