On 2/10/22 14:32, Song Liu wrote:
> On Thu, Feb 10, 2022 at 11:52 AM Nathan Fontenot <nafonten@xxxxxxx> wrote:
>>
>> On 2/9/22 19:10, Song Liu wrote:
>>> On Wed, Feb 9, 2022 at 1:08 PM Nathan Fontenot <nafonten@xxxxxxx> wrote:
>>>>
>>> [...]
>>>>> +
>>>>> +static const struct hsmp_msg_desc msg_desc_table[] = {
>>>>> + /* num_args, response_size, type */
>>>>> + {0, 0, RSVD}, /* RESERVED */
>>>>> + {1, 1, GET}, /* HSMP_TEST */
>>>>> + {0, 1, GET}, /* HSMP_GET_SMU_VER */
>>>>> + {0, 1, GET}, /* HSMP_GET_PROTO_VER */
>>>>> + {0, 1, GET}, /* HSMP_GET_SOCKET_POWER */
>>>>> + {1, 0, SET}, /* HSMP_SET_SOCKET_POWER_LIMIT */
>>>>> + {0, 1, GET}, /* HSMP_GET_SOCKET_POWER_LIMIT */
>>>>> + {0, 1, GET}, /* HSMP_GET_SOCKET_POWER_LIMIT_MAX */
>>>>> + {1, 0, SET}, /* HSMP_SET_BOOST_LIMIT */
>>>>> + {1, 0, SET}, /* HSMP_SET_BOOST_LIMIT_SOCKET */
>>>>> + {1, 1, GET}, /* HSMP_GET_BOOST_LIMIT */
>>>>> + {0, 1, GET}, /* HSMP_GET_PROC_HOT */
>>>>> + {1, 0, SET}, /* HSMP_SET_XGMI_LINK_WIDTH */
>>>>> + {1, 0, SET}, /* HSMP_SET_DF_PSTATE */
>>>>> + {0, 0, SET}, /* HSMP_SET_AUTO_DF_PSTATE */
>>>>> + {0, 2, GET}, /* HSMP_GET_FCLK_MCLK */
>>>>> + {0, 1, GET}, /* HSMP_GET_CCLK_THROTTLE_LIMIT */
>>>>> + {0, 1, GET}, /* HSMP_GET_C0_PERCENT */
>>>>> + {1, 0, SET}, /* HSMP_SET_NBIO_DPM_LEVEL */
>>>>> + {0, 0, RSVD}, /* RESERVED */
>>>>> + {0, 1, GET}, /* HSMP_GET_DDR_BANDWIDTH */
>>>>> + {0, 1, GET}, /* HSMP_GET_TEMP_MONITOR */
>>>>> +};
>>>>
>>>> The hsmp_msg_desc, hsmp_msg_type, and msg_desc_table are used by the driver for
>>>> validating user data. These aren't part of the user API.
>>>>
>>>> Perhaps these should be defined in the driver itself instead of being a part of
>>>> the uapi header.
>>>
>>> This was my idea. While I agree it is a little weird to have these
>>> tables in a uapi
>>> header, I think it is helpful to give the user some reference about
>>> proper num_args
>>> and response_size for each message. I don't have a better idea to achieve this.
>>>
>>
>> I like the idea to give users a reference on args and responses for each HSMP function.
>> If this table is kept in the uapi header perhaps we should add a short description of
>> what the expected args and responses are for each HSMP function with a pointer to the
>> full documentation of the HSMP functions in the PPR.
>
> I guess we can use unions do give full descriptions, like:
>
> struct hsmp_message {
> __u32 msg_id; /* Message ID */
> __u16 num_args; /* Number of arguments
> in message */
> __u16 response_sz; /* Number of expected
> response words */
> union {
> struct {
> __u32 args[HSMP_MAX_MSG_LEN];
> }; /* ensure size of args */
> struct {
> __u32 test_arg1;
> } hsmp_test;
> /* args for other commands */
> } args;
> union {
> struct {
> __u32 response[HSMP_MAX_MSG_LEN];
> }; /* ensure size of response */
> struct {
> __u32 test_response1; /* or better name */
> } hsmp_test;
> /* reponse for other commands */
> } response;
> __u16 sock_ind; /* socket number */
> };
>
I was thinking of keeping the msg_desc_table as is, just provide more details
about the expected args and responses in a comment. I think creating a union
of structs for each HSMP function (and there are more functions coming) would
get a bit messy.
> btw: do we really need HSMP_MAX_MSG_LEN of 8? The list above
> have at most 2 args/responses.
The PPR spec defines the args and responses as having up to 8 so we ned to keep
the max length at 8. No current HSMP has more than 2 though.
-Nathan
>
> Thanks,
> Song
