[Skiboot] [PATCH v2 4/5] skiboot: Enable opal calls to init/start/stop IMC Trace mode
Anju T Sudhakar
anju at linux.vnet.ibm.com
Mon Mar 11 16:36:29 AEDT 2019
Hi,
Thank you for reviewing the patch.
On 2/25/19 11:07 AM, Stewart Smith wrote:
> Anju T Sudhakar<anju at linux.vnet.ibm.com> writes:
>> Patch to enhance the imc opal call to support and handle trace_imc mode.
>>
>> To initialize the trace-mode, TRACE_IMC_SCOM value is written to
>> TRACE_IMC_ADDR of the respective core.
>>
>> TRACE_IMC_SCOM is a 64bit value, and each bit represent the following:
>> 0:1 : SAMPSEL
>> 2:33 : CPMC_LOAD
>> 34:40 : CPMC1SEL
>> 41:47 : CPMC2SEL
>> 48:50 : BUFFERSIZE
>> 51:63 : RESERVED
>> For the nonce the value for TRACE_IMC_SCOM is hard coded.
> nonce?
>
> Do you mean we initialize the trace to a default value?
yes. Right now we initialize this to a default value.
>> During initialization htm_mode is disabled, and enabled only at start.
>>
>> The opal calls to start/stop the counters, will write CORE_IMC_HTM_MODE_ENABLE/
>> CORE_IMC_HTM_MODE_DISABLE respectively to the htm_scom_index of the desired
>> cores.
>>
>> Additional switch cases are added to the current opal calls to start/stop
>> the counters for trace-mode.
>>
>> Signed-off-by: Anju T Sudhakar<anju at linux.vnet.ibm.com>
>> Reviewed-by: Madhavan Srinivasan<maddy at linux.vnet.ibm.com>
>> Cc: Akshay Adiga<akshay.adiga at linux.vnet.ibm.com>
>> ---
>> hw/imc.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 103 insertions(+)
>>
>> diff --git a/hw/imc.c b/hw/imc.c
>> index 3392eaf1..40c64d58 100644
>> --- a/hw/imc.c
>> +++ b/hw/imc.c
>> @@ -22,6 +22,15 @@
>> #include <device.h>
>> #include <p9_stop_api.H>
>>
>> +/*
>> + * IMC trace scom values
>> + */
> for these defines, please have them CAPITALIZED so we know they're
> #define and not variables, and also please prefix with something IMC related.
Sure, will do.
>> +#define samplesel 1 /* select cpmc2 */
>> +#define cpmcload 0xfa /* Value to be loaded into cpmc2 at sampling start */
>> +#define cpmc1sel 2 /* Event: CPM_CCYC */
>> +#define cpmc2sel 2 /* Event: CPM_32MHZ_CYC */
> What do these two things mean? Are there any other options?
>> +#define buffersize 0 /* b’000’- 4K entries * 64 per entry =
>> 256K buffersize */
> I feel this should be documented somewhere in the device tree details
> for the trace nodes.
>
> Indeed, I see that documentation missing from doc/device-tree/imc.rst -
> can you please add it?
Sure, I will document this.
>> +
>> /*
>> * Nest IMC PMU names along with their bit values as represented in the
>> * imc_chip_avl_vector(in struct imc_chip_cb, look at include/imc.h).
>> @@ -264,6 +273,8 @@ static int get_imc_device_type(struct dt_node *node)
>> return IMC_COUNTER_CORE;
>> case IMC_COUNTER_THREAD:
>> return IMC_COUNTER_THREAD;
>> + case IMC_COUNTER_TRACE:
>> + return IMC_COUNTER_TRACE;
>> default:
>> break;
>> }
>> @@ -283,11 +294,23 @@ static bool is_nest_node(struct dt_node *node)
>> static bool is_imc_device_type_supported(struct dt_node *node)
>> {
>> u32 val = get_imc_device_type(node);
>> + struct proc_chip *chip = get_chip(this_cpu()->chip_id);
>> + uint64_t pvr;
>>
>> if ((val == IMC_COUNTER_CHIP) || (val == IMC_COUNTER_CORE) ||
>> (val == IMC_COUNTER_THREAD))
>> return true;
>>
>> + if (val == IMC_COUNTER_TRACE) {
>> + pvr = mfspr(SPR_PVR);
>> + /*
>> + * Trace mode is supported in Nimbus DD2.2
>> + * and later versions.
>> + */
>> + if ((chip->type == PROC_CHIP_P9_NIMBUS) &&
>> + (PVR_VERS_MAJ(pvr) == 2) && (PVR_VERS_MIN(pvr) >= 2))
>> + return true;
>> + }
>> return false;
> What about Cumulus? Or do we just not know currently?
>
IIUC Cumulus is powervm, so we don't support this.
>> }
>>
>> @@ -644,6 +667,8 @@ static int64_t opal_imc_counters_init(uint32_t type, uint64_t addr, uint64_t cpu
>> int port_id, phys_core_id;
>> int ret;
>> uint32_t scoms;
>> + uint64_t trace_scom_val = TRACE_IMC_SCOM(samplesel, cpmcload,
>> + cpmc1sel, cpmc2sel, buffersize);
>>
>> switch (type) {
>> case OPAL_IMC_COUNTERS_NEST:
>> @@ -738,6 +763,53 @@ static int64_t opal_imc_counters_init(uint32_t type, uint64_t addr, uint64_t cpu
>> return OPAL_HARDWARE;
>> }
>> return OPAL_SUCCESS;
>> + case OPAL_IMC_COUNTERS_TRACE:
>> + if (!c)
>> + return OPAL_PARAMETER;
>> +
>> + phys_core_id = cpu_get_core_index(c);
>> + port_id = phys_core_id % 4;
>> +
>> + if (proc_chip_quirks & QUIRK_MAMBO_CALLOUTS)
>> + return OPAL_SUCCESS;
>> +
>> + if (has_deep_states) {
>> + if (wakeup_engine_state == WAKEUP_ENGINE_PRESENT) {
>> + struct proc_chip *chip = get_chip(c->chip_id);
>> +
>> + prlog(PR_INFO, "Configuring stopapi for
>> IMC trace-mode\n");
> should probably be PR_DEBUG.
OK.
>> + scoms = XSCOM_ADDR_P9_EC(phys_core_id, TRACE_IMC_ADDR);
>> + ret = p9_stop_save_scom((void *)chip->homer_base, scoms,
>> + trace_scom_val,
>> + P9_STOP_SCOM_REPLACE,
>> + P9_STOP_SECTION_CORE_SCOM);
>> + if (ret) {
>> + prerror("IMC trace_mode stopapi ret = %d, scoms = %x (core id = %x)\n", ret, scoms, phys_core_id);
>> + if (ret != STOP_SAVE_SCOM_ENTRY_UPDATE_FAILED)
>> + wakeup_engine_state = WAKEUP_ENGINE_FAILED;
>> + else
>> + prerror("SCOM entries are full\n");
>> + return OPAL_HARDWARE;
>> + }
> getting longer than 80 columns here, maybe split up the trace init into its own function?
>
>> + } else {
>> + prerror("IMC: TRACE: Wakeup engine in error state!");
>> + return OPAL_HARDWARE;
> This error isn't strictly true, we have WAKEUP_ENGINE_(NOT_PRESENT|PRESENT|FAILED).
ok. I will change the error message here to something say 'Wakeup engine
not present', is that sounds good?
>> + }
>> + }
>> + if (xscom_write(c->chip_id,
>> + XSCOM_ADDR_P9_EP(phys_core_id, htm_scom_index[port_id]),
>> + (u64)CORE_IMC_HTM_MODE_DISABLE)) {
>> + prerror("IMC: error in xscom_write for htm mode\n");
>> + return OPAL_HARDWARE;
>> + }
>> + if (xscom_write(c->chip_id,
>> + XSCOM_ADDR_P9_EC(phys_core_id,
>> + TRACE_IMC_ADDR), trace_scom_val)) {
>> + prerror("IMC: error in xscom_write for trace mode\n");
>> + return OPAL_HARDWARE;
>> + }
>> + return OPAL_SUCCESS;
>> +
>> }
>>
>> return OPAL_SUCCESS;
>> @@ -797,6 +869,21 @@ static int64_t opal_imc_counters_start(uint32_t type, uint64_t cpu_pir)
>> return OPAL_HARDWARE;
>> }
>>
>> + return OPAL_SUCCESS;
>> + case OPAL_IMC_COUNTERS_TRACE:
>> + phys_core_id = cpu_get_core_index(c);
>> + port_id = phys_core_id % 4;
>> +
>> + if (proc_chip_quirks & QUIRK_MAMBO_CALLOUTS)
>> + return OPAL_SUCCESS;
>> +
>> + if (xscom_write(c->chip_id,
>> + XSCOM_ADDR_P9_EP(phys_core_id,
>> + htm_scom_index[port_id]),
>> + (u64)CORE_IMC_HTM_MODE_ENABLE)) {
>> + prerror("IMC OPAL_start: error in xscom_write for htm_mode\n");
>> + return OPAL_HARDWARE;
>> + }
> Should we also return OPAL_PARAMETER if start is called on a CPU
> revision that doesn't support it?
Do we really need this? Because is_imc_device_type_supported() function
verifies the cpu version
before adding the trace node to the device tree.
>
>> return OPAL_SUCCESS;
>> }
>>
>> @@ -857,6 +944,22 @@ static int64_t opal_imc_counters_stop(uint32_t type, uint64_t cpu_pir)
>> }
>>
>> return OPAL_SUCCESS;
>> + case OPAL_IMC_COUNTERS_TRACE:
>> + phys_core_id = cpu_get_core_index(c);
>> + port_id = phys_core_id % 4;
>> +
>> + if (proc_chip_quirks & QUIRK_MAMBO_CALLOUTS)
>> + return OPAL_SUCCESS;
>> +
>> + if (xscom_write(c->chip_id,
>> + XSCOM_ADDR_P9_EP(phys_core_id,
>> + htm_scom_index[port_id]),
>> + (u64)CORE_IMC_HTM_MODE_DISABLE)) {
>> + prerror("IMC: error in xscom_write for htm_mode\n");
>> + return OPAL_HARDWARE;
>> + }
>> + return OPAL_SUCCESS;
>> +
>> }
>>
>> return OPAL_SUCCESS;
>> --
>> 2.17.1
>>
More information about the Skiboot
mailing list