| Message ID | 20210719132012.150948-60-hegdevasant@linux.vnet.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | P10 Enablement | expand |
On 7/19/21 3:20 PM, Vasant Hegde wrote: > From: Vaidyanathan Srinivasan <svaidy@linux.ibm.com> > > P10 Stop engines have apis similar to P9 to set xscom restores > after wakeup from deep-sleep states. > > This xscom restore will be used to support STOP11 on P10. > > Signed-off-by: Vaidyanathan Srinivasan <svaidy@linux.ibm.com> > Signed-off-by: Pratik Rajesh Sampat <psampat@linux.ibm.com> > Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com> > --- > hw/xive2.c | 31 ++++++++++++++++++++++++++----- > 1 file changed, 26 insertions(+), 5 deletions(-) > > diff --git a/hw/xive2.c b/hw/xive2.c > index f559b0f9a..d2fc2ad69 100644 > --- a/hw/xive2.c > +++ b/hw/xive2.c > @@ -20,8 +20,8 @@ > #include <bitmap.h> > #include <buddy.h> > #include <phys-map.h> > -#include <p9_stop_api.H> /* TODO (p10): need P10 stop state engine */ > - > +#include <p9_stop_api.H> > +#include <p10_stop_api.H> Why do we need both ? C. > > /* Verbose debug */ > #undef XIVE_VERBOSE_DEBUG > @@ -3022,11 +3022,32 @@ static void xive_configure_ex_special_bar(struct xive *x, struct cpu_thread *c) > > void xive2_late_init(void) > { > + struct cpu_thread *c; > + > prlog(PR_INFO, "SLW: Configuring self-restore for NCU_SPEC_BAR\n"); > - /* > - * TODO (p10): need P10 stop state engine and fix for STOP11 > - */ > + for_each_present_cpu(c) { > + if(cpu_is_thread0(c)) { > + struct proc_chip *chip = get_chip(c->chip_id); > + struct xive *x = chip->xive; > + uint64_t xa, val, rc; > + xa = XSCOM_ADDR_P10_NCU(pir_to_core_id(c->pir), P10_NCU_SPEC_BAR); > + val = (uint64_t)x->tm_base | P10_NCU_SPEC_BAR_ENABLE; > + /* Bail out if wakeup engine has already failed */ > + if (wakeup_engine_state != WAKEUP_ENGINE_PRESENT) { > + prlog(PR_ERR, "XIVE proc_stop_api fail detected\n"); > + break; > + } > + rc = proc_stop_save_scom((void *)chip->homer_base, xa, val, > + PROC_STOP_SCOM_REPLACE, PROC_STOP_SECTION_L3); > + if (rc) { > + xive_cpu_err(c, "proc_stop_save_scom failed for NCU_SPEC_BAR rc=%lld\n", > + rc); > + wakeup_engine_state = WAKEUP_ENGINE_FAILED; > + } > + } > + } > } > + > static void xive_provision_cpu(struct xive_cpu_state *xs, struct cpu_thread *c) > { > struct xive *x; >
On 7/21/21 7:05 PM, Cédric Le Goater wrote: > On 7/19/21 3:20 PM, Vasant Hegde wrote: >> From: Vaidyanathan Srinivasan <svaidy@linux.ibm.com> >> >> P10 Stop engines have apis similar to P9 to set xscom restores >> after wakeup from deep-sleep states. >> >> This xscom restore will be used to support STOP11 on P10. >> >> Signed-off-by: Vaidyanathan Srinivasan <svaidy@linux.ibm.com> >> Signed-off-by: Pratik Rajesh Sampat <psampat@linux.ibm.com> >> Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com> >> --- >> hw/xive2.c | 31 ++++++++++++++++++++++++++----- >> 1 file changed, 26 insertions(+), 5 deletions(-) >> >> diff --git a/hw/xive2.c b/hw/xive2.c >> index f559b0f9a..d2fc2ad69 100644 >> --- a/hw/xive2.c >> +++ b/hw/xive2.c >> @@ -20,8 +20,8 @@ >> #include <bitmap.h> >> #include <buddy.h> >> #include <phys-map.h> >> -#include <p9_stop_api.H> /* TODO (p10): need P10 stop state engine */ >> - >> +#include <p9_stop_api.H> >> +#include <p10_stop_api.H> > > Why do we need both ? > Because p10_stop_api.H uses definitions from p9_stop_api.H. May be we should move common definitions to separate header file (say stop_api.h ) and then include that in P[9/10]_stop_api.H file? -Vasant
On 22/07/21 3:01 pm, Vasant Hegde wrote: > On 7/21/21 7:05 PM, Cédric Le Goater wrote: >> On 7/19/21 3:20 PM, Vasant Hegde wrote: >>> From: Vaidyanathan Srinivasan <svaidy@linux.ibm.com> >>> >>> P10 Stop engines have apis similar to P9 to set xscom restores >>> after wakeup from deep-sleep states. >>> >>> This xscom restore will be used to support STOP11 on P10. >>> >>> Signed-off-by: Vaidyanathan Srinivasan <svaidy@linux.ibm.com> >>> Signed-off-by: Pratik Rajesh Sampat <psampat@linux.ibm.com> >>> Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com> >>> --- >>> hw/xive2.c | 31 ++++++++++++++++++++++++++----- >>> 1 file changed, 26 insertions(+), 5 deletions(-) >>> >>> diff --git a/hw/xive2.c b/hw/xive2.c >>> index f559b0f9a..d2fc2ad69 100644 >>> --- a/hw/xive2.c >>> +++ b/hw/xive2.c >>> @@ -20,8 +20,8 @@ >>> #include <bitmap.h> >>> #include <buddy.h> >>> #include <phys-map.h> >>> -#include <p9_stop_api.H> /* TODO (p10): need P10 stop state engine */ >>> - >>> +#include <p9_stop_api.H> >>> +#include <p10_stop_api.H> >> >> Why do we need both ? >> > > Because p10_stop_api.H uses definitions from p9_stop_api.H. May be we > should move common > definitions to separate header file (say stop_api.h ) and then include > that in P[9/10]_stop_api.H file? > I agree that there are a lot of common definitions around both P9 and P10 stop API header, and could definitely be cleaned up and merged. However the headers are direct mirror from hostboot and they maintain different code-bases for P9 and P10. My cause of concern stems from the fact that if we refactor the libraries here and if hostboot updates their libraries we would manually each time have to code those changes up in our refactored library too. Thanks, Pratik > -Vasant >
On 7/23/21 8:33 PM, Pratik Sampat wrote: > > > On 22/07/21 3:01 pm, Vasant Hegde wrote: >> On 7/21/21 7:05 PM, Cédric Le Goater wrote: >>> On 7/19/21 3:20 PM, Vasant Hegde wrote: >>>> From: Vaidyanathan Srinivasan <svaidy@linux.ibm.com> >>>> >>>> P10 Stop engines have apis similar to P9 to set xscom restores >>>> after wakeup from deep-sleep states. >>>> >>>> This xscom restore will be used to support STOP11 on P10. >>>> >>>> Signed-off-by: Vaidyanathan Srinivasan <svaidy@linux.ibm.com> >>>> Signed-off-by: Pratik Rajesh Sampat <psampat@linux.ibm.com> >>>> Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com> >>>> --- >>>> hw/xive2.c | 31 ++++++++++++++++++++++++++----- >>>> 1 file changed, 26 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/hw/xive2.c b/hw/xive2.c >>>> index f559b0f9a..d2fc2ad69 100644 >>>> --- a/hw/xive2.c >>>> +++ b/hw/xive2.c >>>> @@ -20,8 +20,8 @@ >>>> #include <bitmap.h> >>>> #include <buddy.h> >>>> #include <phys-map.h> >>>> -#include <p9_stop_api.H> /* TODO (p10): need P10 stop state engine */ >>>> - >>>> +#include <p9_stop_api.H> >>>> +#include <p10_stop_api.H> >>> >>> Why do we need both ? >>> >> >> Because p10_stop_api.H uses definitions from p9_stop_api.H. May be we should >> move common >> definitions to separate header file (say stop_api.h ) and then include that in >> P[9/10]_stop_api.H file? >> > I agree that there are a lot of common definitions around both P9 and P10 stop > API header, and could definitely be cleaned up and merged. However the headers > are direct mirror from hostboot and they maintain different code-bases for > P9 and P10. > > My cause of concern stems from the fact that if we refactor the libraries here > and if hostboot updates their libraries we would manually each time have > to code those changes up in our refactored library too. I'm referring to include/p10_stop_api.H here. AFAIC this file is already modified. Something like below works? diff --git a/hw/xive2.c b/hw/xive2.c index 674005895..aece99a0d 100644 --- a/hw/xive2.c +++ b/hw/xive2.c @@ -20,7 +20,6 @@ #include <bitmap.h> #include <buddy.h> #include <phys-map.h> -#include <p9_stop_api.H> #include <p10_stop_api.H> /* Verbose debug */ diff --git a/include/p10_stop_api.H b/include/p10_stop_api.H index f3854fefc..3a8772195 100644 --- a/include/p10_stop_api.H +++ b/include/p10_stop_api.H @@ -23,6 +23,7 @@ #ifdef __SKIBOOT__ #include <skiboot.h> + #include <p9_stop_api.H> #endif -Vasant
On 7/27/21 12:50 PM, Vasant Hegde wrote: > On 7/23/21 8:33 PM, Pratik Sampat wrote: >> >> >> On 22/07/21 3:01 pm, Vasant Hegde wrote: >>> On 7/21/21 7:05 PM, Cédric Le Goater wrote: >>>> On 7/19/21 3:20 PM, Vasant Hegde wrote: >>>>> From: Vaidyanathan Srinivasan <svaidy@linux.ibm.com> >>>>> >>>>> P10 Stop engines have apis similar to P9 to set xscom restores >>>>> after wakeup from deep-sleep states. >>>>> >>>>> This xscom restore will be used to support STOP11 on P10. >>>>> >>>>> Signed-off-by: Vaidyanathan Srinivasan <svaidy@linux.ibm.com> >>>>> Signed-off-by: Pratik Rajesh Sampat <psampat@linux.ibm.com> >>>>> Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com> >>>>> --- >>>>> hw/xive2.c | 31 ++++++++++++++++++++++++++----- >>>>> 1 file changed, 26 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/hw/xive2.c b/hw/xive2.c >>>>> index f559b0f9a..d2fc2ad69 100644 >>>>> --- a/hw/xive2.c >>>>> +++ b/hw/xive2.c >>>>> @@ -20,8 +20,8 @@ >>>>> #include <bitmap.h> >>>>> #include <buddy.h> >>>>> #include <phys-map.h> >>>>> -#include <p9_stop_api.H> /* TODO (p10): need P10 stop state engine */ >>>>> - >>>>> +#include <p9_stop_api.H> >>>>> +#include <p10_stop_api.H> >>>> >>>> Why do we need both ? >>>> >>> >>> Because p10_stop_api.H uses definitions from p9_stop_api.H. May be we should move common >>> definitions to separate header file (say stop_api.h ) and then include that in P[9/10]_stop_api.H file? >>> >> I agree that there are a lot of common definitions around both P9 and P10 stop >> API header, and could definitely be cleaned up and merged. However the headers >> are direct mirror from hostboot and they maintain different code-bases for >> P9 and P10. >> >> My cause of concern stems from the fact that if we refactor the libraries here >> and if hostboot updates their libraries we would manually each time have >> to code those changes up in our refactored library too. > > I'm referring to include/p10_stop_api.H here. AFAIC this file is already modified. > > Something like below works? Looks good to me. Thanks, C. > > > diff --git a/hw/xive2.c b/hw/xive2.c > index 674005895..aece99a0d 100644 > --- a/hw/xive2.c > +++ b/hw/xive2.c > @@ -20,7 +20,6 @@ > #include <bitmap.h> > #include <buddy.h> > #include <phys-map.h> > -#include <p9_stop_api.H> > #include <p10_stop_api.H> > > /* Verbose debug */ > diff --git a/include/p10_stop_api.H b/include/p10_stop_api.H > index f3854fefc..3a8772195 100644 > --- a/include/p10_stop_api.H > +++ b/include/p10_stop_api.H > @@ -23,6 +23,7 @@ > > #ifdef __SKIBOOT__ > #include <skiboot.h> > + #include <p9_stop_api.H> > #endif > > > -Vasant
On 28/07/21 11:40 am, Cédric Le Goater wrote: > On 7/27/21 12:50 PM, Vasant Hegde wrote: >> On 7/23/21 8:33 PM, Pratik Sampat wrote: >>> >>> On 22/07/21 3:01 pm, Vasant Hegde wrote: >>>> On 7/21/21 7:05 PM, Cédric Le Goater wrote: >>>>> On 7/19/21 3:20 PM, Vasant Hegde wrote: >>>>>> From: Vaidyanathan Srinivasan <svaidy@linux.ibm.com> >>>>>> >>>>>> P10 Stop engines have apis similar to P9 to set xscom restores >>>>>> after wakeup from deep-sleep states. >>>>>> >>>>>> This xscom restore will be used to support STOP11 on P10. >>>>>> >>>>>> Signed-off-by: Vaidyanathan Srinivasan <svaidy@linux.ibm.com> >>>>>> Signed-off-by: Pratik Rajesh Sampat <psampat@linux.ibm.com> >>>>>> Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com> >>>>>> --- >>>>>> hw/xive2.c | 31 ++++++++++++++++++++++++++----- >>>>>> 1 file changed, 26 insertions(+), 5 deletions(-) >>>>>> >>>>>> diff --git a/hw/xive2.c b/hw/xive2.c >>>>>> index f559b0f9a..d2fc2ad69 100644 >>>>>> --- a/hw/xive2.c >>>>>> +++ b/hw/xive2.c >>>>>> @@ -20,8 +20,8 @@ >>>>>> #include <bitmap.h> >>>>>> #include <buddy.h> >>>>>> #include <phys-map.h> >>>>>> -#include <p9_stop_api.H> /* TODO (p10): need P10 stop state engine */ >>>>>> - >>>>>> +#include <p9_stop_api.H> >>>>>> +#include <p10_stop_api.H> >>>>> Why do we need both ? >>>>> >>>> Because p10_stop_api.H uses definitions from p9_stop_api.H. May be we should move common >>>> definitions to separate header file (say stop_api.h ) and then include that in P[9/10]_stop_api.H file? >>>> >>> I agree that there are a lot of common definitions around both P9 and P10 stop >>> API header, and could definitely be cleaned up and merged. However the headers >>> are direct mirror from hostboot and they maintain different code-bases for >>> P9 and P10. >>> >>> My cause of concern stems from the fact that if we refactor the libraries here >>> and if hostboot updates their libraries we would manually each time have >>> to code those changes up in our refactored library too. >> I'm referring to include/p10_stop_api.H here. AFAIC this file is already modified. >> >> Something like below works? > Looks good to me. Sure, I can call the P9 headers in P10 that way. Thanks! > Thanks, > > C. > >> >> diff --git a/hw/xive2.c b/hw/xive2.c >> index 674005895..aece99a0d 100644 >> --- a/hw/xive2.c >> +++ b/hw/xive2.c >> @@ -20,7 +20,6 @@ >> #include <bitmap.h> >> #include <buddy.h> >> #include <phys-map.h> >> -#include <p9_stop_api.H> >> #include <p10_stop_api.H> >> >> /* Verbose debug */ >> diff --git a/include/p10_stop_api.H b/include/p10_stop_api.H >> index f3854fefc..3a8772195 100644 >> --- a/include/p10_stop_api.H >> +++ b/include/p10_stop_api.H >> @@ -23,6 +23,7 @@ >> >> #ifdef __SKIBOOT__ >> #include <skiboot.h> >> + #include <p9_stop_api.H> >> #endif >> >> >> -Vasant
diff --git a/hw/xive2.c b/hw/xive2.c index f559b0f9a..d2fc2ad69 100644 --- a/hw/xive2.c +++ b/hw/xive2.c @@ -20,8 +20,8 @@ #include <bitmap.h> #include <buddy.h> #include <phys-map.h> -#include <p9_stop_api.H> /* TODO (p10): need P10 stop state engine */ - +#include <p9_stop_api.H> +#include <p10_stop_api.H> /* Verbose debug */ #undef XIVE_VERBOSE_DEBUG @@ -3022,11 +3022,32 @@ static void xive_configure_ex_special_bar(struct xive *x, struct cpu_thread *c) void xive2_late_init(void) { + struct cpu_thread *c; + prlog(PR_INFO, "SLW: Configuring self-restore for NCU_SPEC_BAR\n"); - /* - * TODO (p10): need P10 stop state engine and fix for STOP11 - */ + for_each_present_cpu(c) { + if(cpu_is_thread0(c)) { + struct proc_chip *chip = get_chip(c->chip_id); + struct xive *x = chip->xive; + uint64_t xa, val, rc; + xa = XSCOM_ADDR_P10_NCU(pir_to_core_id(c->pir), P10_NCU_SPEC_BAR); + val = (uint64_t)x->tm_base | P10_NCU_SPEC_BAR_ENABLE; + /* Bail out if wakeup engine has already failed */ + if (wakeup_engine_state != WAKEUP_ENGINE_PRESENT) { + prlog(PR_ERR, "XIVE proc_stop_api fail detected\n"); + break; + } + rc = proc_stop_save_scom((void *)chip->homer_base, xa, val, + PROC_STOP_SCOM_REPLACE, PROC_STOP_SECTION_L3); + if (rc) { + xive_cpu_err(c, "proc_stop_save_scom failed for NCU_SPEC_BAR rc=%lld\n", + rc); + wakeup_engine_state = WAKEUP_ENGINE_FAILED; + } + } + } } + static void xive_provision_cpu(struct xive_cpu_state *xs, struct cpu_thread *c) { struct xive *x;