[Skiboot] [PATCH v2 4/5] external/opal-prd: Support manufacturing command HTMGT and attribute override

[Neelesh Gupta]

Hi Jeremy,

Thanks for the review.

On 08/25/2015 06:43 AM, Jeremy Kerr wrote:
> Hi Neelesh,
>
>
>> diff --git a/external/opal-prd/hostboot-interface.h b/external/opal-prd/hostboot-interface.h
>> index a518f50..7ab928f 100644
>> --- a/external/opal-prd/hostboot-interface.h
>> +++ b/external/opal-prd/hostboot-interface.h
>> @@ -421,6 +421,41 @@ struct runtime_interfaces {
>>   	 */
>>   	int (*enable_occ_actuation)(bool i_occActivation);
>>   
>> +	/**
>> +	 * @brief	Apply a set of attribute overrides
>> +	 *
>> +	 * @param[in]	pointer to binary override data
>> +	 * @param[in]	length of override data (bytes)
>> +	 * @returns	0 on success, or return code if the command failed
>> +	 *
>> +	 * @platform	OpenPower
>> +	 */
>> +	int (*apply_attr_override)(uint8_t *i_data, size_t size);
>> +
>> +	/**
>> +	 * @brief	Send a pass-through command to HTMGT
>> +	 *
>> +	 * @details	This is a blocking call that will send a command to
>> +	 *		HTMGT.
>> +	 *
>> +	 * @note	If o_rspLength is returned with a non-zero value, the
>> +	 *		data at the o_rspData should be dumped to stdout in a
>> +	 *		hex dump format.
>> +	 * @note	The maximum response data returned will be 4096 bytes
>> +	 *
>> +	 * @param[in]	i_cmdLength	number of bytes in pass-thru command data
>> +	 * @param[in]	*i_cmdData	pointer to pass-thru command data
>> +	 * @param[out]	*o_rspLength	pointer to number of bytes returned in
>> +	 *				o_rspData
>> +	 * @param[out]	*o_rspData	pointer to a 4096 byte buffer that will
>> +	 *				contain the response data from the command
>> +	 *
>> +	 * @returns	0 on success, or return code if the command failed
>> +	 * @platform	OpenPower
>> +	 */
>> +	int (*htmgt_pass_thru)(uint16_t i_cmdLength, uint8_t *i_cmdData,
>> +			       uint16_t *o_rspLength, uint8_t *o_rspData);
>> +
>>   	/* Reserve some space for future growth. */
>>   	void (*reserved[32])(void);
> Since we don't have interface version changes here, you'll need to
> consume part of this reserved[] array if you add functions to the
> runtime_interfaces struct.

Correct, I should make use of the reserve pool..

>
>> diff --git a/external/opal-prd/opal-prd.c b/external/opal-prd/opal-prd.c
>> index 8f1d6cb..9b9e213 100644
>> --- a/external/opal-prd/opal-prd.c
>> +++ b/external/opal-prd/opal-prd.c
>> @@ -75,6 +75,7 @@ struct opal_prd_ctx {
>>   	struct pnor		pnor;
>>   	char			*hbrt_file_name;
>>   	bool			use_syslog;
>> +	bool			mfg_mode;
>>   	void			(*vlog)(int, const char *, va_list);
>>   };
>>   
>> @@ -83,13 +84,20 @@ enum control_msg_type {
>>   	CONTROL_MSG_DISABLE_OCCS	= 0x01,
>>   	CONTROL_MSG_TEMP_OCC_RESET	= 0x02,
>>   	CONTROL_MSG_TEMP_OCC_ERROR	= 0x03,
>> +	CONTROL_MSG_ATTR_OVERRIDE	= 0x10,
>> +	CONTROL_MSG_HTMGT_PASSTHRU	= 0x20,
>>   };
> Why the gap in numbering here?

I was also not completely sure.. :) .. thought of grouping them using 
1st nibble,
in case we add any new commands to a particular group ..
Yes, it can be sequential.. will change it.

>
>>   
>>   struct control_msg {
>>   	enum control_msg_type	type;
>> -	uint64_t		response;
>> +	int			response;
>> +	uint32_t		argc;
>> +	uint32_t		data_len;
>> +	uint8_t			data[];
>>   };
>>   
>> +#define MAX_CONTROL_MSG_BUF	4096
>> +
>>   static struct opal_prd_ctx *ctx;
>>   
>>   static const char *opal_prd_devnode = "/dev/opal-prd";
>> @@ -179,6 +187,22 @@ static void pr_log_nocall(const char *name)
>>   	pr_log(LOG_WARNING, "HBRT: Call %s not provided", name);
>>   }
>>   
>> +static void pr_log_stream(const uint8_t *data, uint32_t len)
> Not sure this function name matches what the function does. How about
> just hexdump() ?
>
>
>
>> +	printf("\t%s htmgt-passthru <1st-byte> <2nd byte>..<nth byte>\n",
>> +			progname);
>
> Just:
>          printf("\t%s htmgt-passthru <bytes...>\n", progname);
>
>
>> +	printf("\t%s override <binary-blob>\n", progname);
> and:
>
>          printf("\t%s override <FILE>\n", progname);
>
> (binary-blob doesn't tell the user that this is supposed to refer to a file)
>
>> +	{"mfg-mode", no_argument, NULL, 'm'},
> You listed --manufacting-mode in the commit header here. I prefer that,
> but either is fine.
>
>
>> @@ -1483,8 +1723,29 @@ int main(int argc, char *argv[])
>>   		}
>>   
>>   		rc = send_occ_control(ctx, argv[optind + 1]);
>> +		break;
>> +	case ACTION_ATTR_OVERRIDE:
>> +		if (optind + 1 >= argc) {
>> +			pr_log(LOG_ERR, "CTRL: attribute override command "
>> +					"requires an argument");
>> +			return EXIT_FAILURE;
>> +		}
>> +
>> +		rc = send_attr_override(ctx, argc - optind - 1, &argv[optind + 1]);
>> +		break;
>> +	case ACTION_HTMGT_PASSTHRU:
>> +		if (optind + 1 >= argc) {
>> +			pr_log(LOG_ERR, "CTRL: htmgt passthru requires minimum "
>> +					"an argument");
> English nitpick: s/minimum an/at least one/

Will also take care of rest of the comment in next revision.

Thanks,
Neelesh.

>
> Cheers,
>
>
> Jeremy
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/skiboot/attachments/20150825/df0249ca/attachment.html>