| Message ID | 20230913183348.1349409-5-jithu.joseph@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | IFS support for GNR and SRF | expand |
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. > }; > > /* 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. > + } gen2; > }; > > /* MSR_ARRAY_BIST bit fields */ > diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c > index 1061eb7ec399..4bbab6be2fa2 100644 > --- a/drivers/platform/x86/intel/ifs/runtest.c > +++ b/drivers/platform/x86/intel/ifs/runtest.c > @@ -171,6 +171,8 @@ static void ifs_test_core(int cpu, struct device *dev) > union ifs_status status; > unsigned long timeout; > struct ifs_data *ifsd; > + int to_start, to_stop; > + int status_chunk; > u64 msrvals[2]; > int retries; > > @@ -179,13 +181,21 @@ static void ifs_test_core(int cpu, struct device *dev) > activate.rsvd = 0; > activate.delay = IFS_THREAD_WAIT; > activate.sigmce = 0; > - activate.start = 0; > - activate.stop = ifsd->valid_chunks - 1; > + to_start = 0; > + to_stop = ifsd->valid_chunks - 1; > + > + if (ifsd->generation) { > + activate.gen2.start = to_start; > + activate.gen2.stop = to_stop; > + } else { > + activate.start = to_start; > + activate.stop = to_stop; > + } > > timeout = jiffies + HZ / 2; > retries = MAX_IFS_RETRIES; > > - while (activate.start <= activate.stop) { > + while (to_start <= to_stop) { > if (time_after(jiffies, timeout)) { > status.error_code = IFS_SW_TIMEOUT; > break; > @@ -202,7 +212,8 @@ static void ifs_test_core(int cpu, struct device *dev) > if (!can_restart(status)) > break; > > - if (status.chunk_num == activate.start) { > + status_chunk = ifsd->generation ? status.gen2.chunk_num : status.chunk_num; > + if (status_chunk == to_start) { > /* Check for forward progress */ > if (--retries == 0) { > if (status.error_code == IFS_NO_ERROR) > @@ -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. > + to_start = status_chunk; > } > } > >
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 >> }; >> >> /* 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
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.
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@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): > 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
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; }; /* 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; + } gen2; }; /* MSR_ARRAY_BIST bit fields */ diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c index 1061eb7ec399..4bbab6be2fa2 100644 --- a/drivers/platform/x86/intel/ifs/runtest.c +++ b/drivers/platform/x86/intel/ifs/runtest.c @@ -171,6 +171,8 @@ static void ifs_test_core(int cpu, struct device *dev) union ifs_status status; unsigned long timeout; struct ifs_data *ifsd; + int to_start, to_stop; + int status_chunk; u64 msrvals[2]; int retries; @@ -179,13 +181,21 @@ static void ifs_test_core(int cpu, struct device *dev) activate.rsvd = 0; activate.delay = IFS_THREAD_WAIT; activate.sigmce = 0; - activate.start = 0; - activate.stop = ifsd->valid_chunks - 1; + to_start = 0; + to_stop = ifsd->valid_chunks - 1; + + if (ifsd->generation) { + activate.gen2.start = to_start; + activate.gen2.stop = to_stop; + } else { + activate.start = to_start; + activate.stop = to_stop; + } timeout = jiffies + HZ / 2; retries = MAX_IFS_RETRIES; - while (activate.start <= activate.stop) { + while (to_start <= to_stop) { if (time_after(jiffies, timeout)) { status.error_code = IFS_SW_TIMEOUT; break; @@ -202,7 +212,8 @@ static void ifs_test_core(int cpu, struct device *dev) if (!can_restart(status)) break; - if (status.chunk_num == activate.start) { + status_chunk = ifsd->generation ? status.gen2.chunk_num : status.chunk_num; + if (status_chunk == to_start) { /* Check for forward progress */ if (--retries == 0) { if (status.error_code == IFS_NO_ERROR) @@ -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); + to_start = status_chunk; } }