[Skiboot] [PATCH v5 5/8] skiboot: Add core IMC related counter configuration OPAL call

[Hemant Kumar]

On 02/14/2017 10:01 AM, Oliver O'Halloran wrote:
> On Wed, 2017-02-08 at 02:41 +0530, Hemant Kumar wrote:
>> This patch adds macro definitions related to Core IMC related macro
>> definitions for the configuration of the required SCOM ports. A new
>> OPAL
>> call opal_core_imc_counters_countrol() is added to initialize, enable
[SNIP]
>> +
>> +/*
>> + * opal_core_imc_counters_control : Controls the Core IMC counters.
>> + *
>> + * operation : For now, this call supports only OPAL_CORE_IMC_INIT,
>> + *             OPAL_CORE_IMC_DISABLE and OPAL_CORE_IMC_ENABLE
>> operations.
>> + *
>> + *             OPAL_CORE_IMC_INIT initializes core IMC Engine for
>> the
>> + *             current core, by initializing the pdbars, htm_mode,
>> + *             and the event_mask. "addr" must be non-zero for this
>> operation.
>> + *
>> + *             OPAL_CORE_IMC_ENABLE enables the core imc engine by
>> + *             appropriately setting bits 4-9 of the HTM_MODE scom
>> port. No
>> + *             initialization is done in this call. This just
>> enables the
>> + *             the counters to count with the previous
>> initialization.
>> + *
>> + *             OPAL_CORE_IMC_DISABLE disables the core imc engine by
>> clearing
>> + *             bits 4-9 of the HTM_MODE scom port.
>> + *
>> + * addr      : Contains per-core physical address. This is the
>> memory
>> + *	       address where the core IMC engine writes the
>> counter values.
>> + *             Must be non-zero for CORE_IMC_INIT and zero for
>> + *             CORE_IMC_DISABLE and CORE_IMC_ENABLE operations.
>> + *
>> + * This call can be extended to include other operations in
>> "operation" and
>> + * the  other two parameters value_1 and value_2 are provided in
>> case, they
>> + * are needed in future. For now, they are unused and must be zero.
>> + */
>> +static int64_t opal_core_imc_counters_control(uint64_t operation,
>> +					      uint64_t addr,
>> +					      uint64_t value_1,
>> +					      uint64_t value_2)
>> +{
>> +	int ret = OPAL_PARAMETER;
>> +
>> +	if (value_1 || value_2)
>> +		return ret;
>> +
>> +	switch (operation) {
>> +	case OPAL_CORE_IMC_DISABLE:
>> +		if (!addr)
>> +			ret =
>> opal_core_imc_counters_switch(CORE_IMC_OP_DISABLE);
>> +		break;
>> +	case OPAL_CORE_IMC_ENABLE:
>> +		if (!addr)
>> +			ret =
>> opal_core_imc_counters_switch(CORE_IMC_OP_ENABLE);
> I'm a bit skeptical of this interface. I imagine perf or some other
> performance monitoring tool would want to be able to change the IMC
> state for *any* core rather than just the core it's running on. Given
> the XSCOM interface can be used from any core it would be good if we
> had some way of selecting which core.
>
> Otherwise we would need to do a system-wide IPI and have each core call
> into OPAL independently. This might work, but it's kinda nasty.

Agreed. But this system-wide IPI that we do from the kernel side
to make this opal call is done only once during boot up. And, this
(enabling the IMC) is done for all the cores in the system.
Other cases where this call would be made will be from the
hotplug code (when a core comes online or goes offline). In this
case, there won't be an IPI, rather, that core will make a call to
this function in the event of online'ing or offline'ing.

>> +		break;
>> +	case OPAL_CORE_IMC_INIT:
>> +		if (addr)
>> +			ret = opal_core_imc_counters_init(addr);
>> +		break;
>> +	default:
>> +		ret = OPAL_PARAMETER;
>> +	}
>> +
>> +	return ret;
>> +}
>> +opal_call(OPAL_CORE_IMC_COUNTERS_CONTROL,
>> opal_core_imc_counters_control, 4);
>> diff --git a/include/imc.h b/include/imc.h
>> index b44c0c2..f6c6856 100644
>> --- a/include/imc.h
>> +++ b/include/imc.h
>> @@ -112,6 +112,16 @@ struct imc_chip_cb
>>   #define NEST_IMC_ENABLE			0x1
>>   #define NEST_IMC_DISABLE		0x2
>>   
>> +/*
>> + * Core IMC SCOMs
>> + */
>> +#define CORE_IMC_EVENT_MASK_ADDR        0x20010AA8
>> +#define CORE_IMC_EVENT_MASK 		0x0001020000000000
>> +#define CORE_IMC_PDBAR_MASK 		0x0003ffffffffe000
>> +#define CORE_IMC_NCU_MODE 		0x0800000000000000
>> +#define CORE_IMC_HTM_MODE_ENABLE 	0xE800000000000000
>> +#define CORE_IMC_HTM_MODE_DISABLE 	0xE000000000000000
> You can suffix the number with 'ul' to make it an unsigned long
> literal:
> 	E.g #define CORE_IMC_HTM_MODE_DISABLE 	0xE00000000000000
> 0ul

Right, will change this.

>> +
>>   void imc_init(void);
>>   
>>   #endif /* __IMC_H */
>> diff --git a/include/opal-api.h b/include/opal-api.h
>> index 9668758..bb5d26b 100644
>> --- a/include/opal-api.h
>> +++ b/include/opal-api.h
>> @@ -182,7 +182,8 @@
>>   #define OPAL_PCI_TCE_KILL			126
>>   #define OPAL_NMMU_SET_PTCR			127
>>   #define OPAL_NEST_IMC_COUNTERS_CONTROL		128
>> -#define OPAL_LAST				128
>> +#define OPAL_CORE_IMC_COUNTERS_CONTROL		129
>> +#define OPAL_LAST				129
>>   
>>   /* Device tree flags */
>>   
>> @@ -1063,6 +1064,13 @@ enum {
>>   	OPAL_NEST_IMC_START,
>>   };
>>   
>> +/* Operation argument to Core IMC */
>> +enum {
>> +	OPAL_CORE_IMC_DISABLE,
>> +	OPAL_CORE_IMC_ENABLE,
>> +	OPAL_CORE_IMC_INIT,
>> +};
>> +
>>   #endif /* __ASSEMBLY__ */
>>   
>>   #endif /* __OPAL_API_H */

--
Thanks,
Hemant Kumar