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 */
};
btw: do we really need HSMP_MAX_MSG_LEN of 8? The list above
have at most 2 args/responses.
Thanks,
Song
