[Skiboot] [PATCH 58/61] P10 Cleanup special wakeup and xive stop api usage
Pratik Sampat
psampat at linux.ibm.com
Sat Jul 24 00:58:00 AEST 2021
On 21/07/21 5:02 pm, Nicholas Piggin wrote:
> Excerpts from Vasant Hegde's message of July 19, 2021 11:20 pm:
>> From: Vaidyanathan Srinivasan <svaidy at linux.ibm.com>
>>
>> Cleanup P9 code pending implementation for P10.
>>
>> P10 stop-api integration will be needed for STOP11
>> support only. STOP11 will be a restricted usage
>> for testing core re-init on P10. Only stop0,2,3
>> will be available for general usage.
>>
>> Also, do not treat gated core with SPW done as error.
>> Core gating bit is a software state updated by microcode,
>> while SPWU done bit comes from hardware logic to indicate
>> successful operation. Print a warning if the status bits
>> are out of sync, but no need to fail the special wakeup
>> operation.
> Hmm, I wonder if similar should be done with P9 code first,
> and then some of the P10 updates merged into the P10 enable
> patches.
I believe P9 didn't fail the SPW command if the core is gated, that is just the
behavior I observe on P10, is that what you mean?
>> Signed-off-by: Vaidyanathan Srinivasan <svaidy at linux.ibm.com>
>> Signed-off-by: Pratik R. Sampat <psampat at linux.ibm.com>
>> Signed-off-by: Vasant Hegde <hegdevasant at linux.vnet.ibm.com>
>> ---
>> core/direct-controls.c | 34 +++++++++++++++++++++++++++-------
>> hw/slw.c | 30 +++++++++---------------------
>> hw/xive2.c | 26 +-------------------------
>> 3 files changed, 37 insertions(+), 53 deletions(-)
>>
>> diff --git a/core/direct-controls.c b/core/direct-controls.c
>> index 879a537af..4795c19dc 100644
>> --- a/core/direct-controls.c
>> +++ b/core/direct-controls.c
>> @@ -600,15 +600,35 @@ static int p10_core_set_special_wakeup(struct cpu_thread *cpu)
>> * CORE_GATED will be unset on a successful special
>> * wakeup of the core which indicates that the core is
>> * out of stop state. If CORE_GATED is still set then
>> - * raise error.
>> + * check SPWU register and raise error only if SPWU_DONE
>> + * is not set, else print a warning and consider SPWU
>> + * operation as successful.
>> */
>> if (p10_core_is_gated(cpu)) {
>> - /* Deassert spwu for this strange error */
>> - xscom_write(chip_id, spwu_addr, 0);
>> - prlog(PR_ERR, "Failed special wakeup on %u:%u"
>> - " core remains gated.\n",
>> - chip_id, core_id);
>> - return OPAL_HARDWARE;
>> + if(xscom_read(chip_id, spwu_addr, &val)) {
>> + prlog(PR_ERR, "Core %u:%u:"
>> + " unable to read QME_SPWU_HYP\n",
>> + chip_id, core_id);
>> + return OPAL_HARDWARE;
>> + }
>> + if (val & P10_SPWU_DONE) {
>> + /*
>> + * If SPWU DONE bit is set then
>> + * SPWU operation is complete
>> + */
>> + prlog(PR_WARNING, "Special wakeup on "
>> + "%u:%u: core remains gated while"
>> + " SPWU_HYP DONE set\n",
>> + chip_id, core_id);
>> + return 0;
>> + }
>> + /* Deassert spwu for this strange error */
>> + xscom_write(chip_id, spwu_addr, 0);
>> + prlog(PR_ERR,
>> + "Failed special wakeup on %u:%u"
>> + " core remains gated.\n",
>> + chip_id, core_id);
>> + return OPAL_HARDWARE;
> Should the equivalent be done for P9 here?
I believe P9 does not fail on special wakeup when core is gated hence, it may
not be necessary.
>> } else {
>> return 0;
>> }
>> diff --git a/hw/slw.c b/hw/slw.c
>> index e22d1bdde..52536db06 100644
>> --- a/hw/slw.c
>> +++ b/hw/slw.c
>> @@ -228,32 +228,20 @@ static bool slw_set_overrides_p10(struct proc_chip *chip, struct cpu_thread *c)
>> int rc;
>> uint32_t core = pir_to_core_id(c->pir);
>>
>> - /* Clear special wakeup bits that could hold power mgt */
>> - rc = xscom_write(chip->id,
>> - XSCOM_ADDR_P10_QME_CORE(core, P10_QME_SPWU_HYP),
>> - 0);
>> - if (rc) {
>> - log_simple_error(&e_info(OPAL_RC_SLW_SET),
>> - "SLW: Failed to write P10_QME_SPWU_HYP\n");
>> - return false;
>> - }
>> - /* Read back for debug */
>> + /* Special wakeup bits that could hold power mgt */
>> rc = xscom_read(chip->id,
>> XSCOM_ADDR_P10_QME_CORE(core, P10_QME_SPWU_HYP),
>> &tmp);
>> - if (tmp)
>> + if (rc) {
>> + log_simple_error(&e_info(OPAL_RC_SLW_SET),
>> + "SLW: Failed to read P10_QME_SPWU_HYP\n");
>> + return false;
>> + }
> Ditto here -- should the p9 code be changed? I wonder if the special
> wakeup check should be made in direct controls init code rather than
> here.
P9 made those check here too and that's why I followed pattern, however if you
believe this belongs in direct controls I could maybe clean it up for both P9
and P10.
>
>> + if (tmp & P10_SPWU_REQ)
>> prlog(PR_WARNING,
>> - "SLW: core %d P10_QME_SPWU_HYP read 0x%016llx\n",
>> - core, tmp);
>> -#if 0
>> - rc = xscom_read(chip->id,
>> - XSCOM_ADDR_P10_QME_CORE(core, P10_QME_SPWU_OTR),
>> - &tmp);
>> - if (tmp)
>> - prlog(PR_WARNING,
>> - "SLW: core %d P10_QME_SPWU_OTR read 0x%016llx\n",
>> + "SLW: core %d P10_QME_SPWU_HYP requested 0x%016llx\n",
>> core, tmp);
>> -#endif
>> +
>> return true;
>> }
>>
>> diff --git a/hw/xive2.c b/hw/xive2.c
>> index b79635cc9..f559b0f9a 100644
>> --- a/hw/xive2.c
>> +++ b/hw/xive2.c
>> @@ -3022,34 +3022,10 @@ 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");
>> - 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 p9_stop_api fail detected\n");
>> - break;
>> - }
>> /*
>> - * TODO (p10): need P10 stop state engine
>> + * TODO (p10): need P10 stop state engine and fix for STOP11
>> */
>> - rc = p9_stop_save_scom((void *)chip->homer_base, xa, val,
>> - P9_STOP_SCOM_REPLACE, P9_STOP_SECTION_EQ_SCOM);
>> - if (rc) {
>> - xive_cpu_err(c, "p9_stop_api failed for NCU_SPEC_BAR rc=%lld\n",
>> - rc);
>> - wakeup_engine_state = WAKEUP_ENGINE_FAILED;
>> - }
>> - }
>> - }
> This should probably be merged withthe xive2 code that added it.
I agree, I could move this hunk within the xive2 patch.
Thanks for pointing it out!
Thanks,
Pratik
> Thanks,
> Nick
More information about the Skiboot
mailing list