On Thu, Feb 10, 2022 at 12:42 PM Nathan Fontenot <nafonten@xxxxxxx> wrote:
>
> 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.
Yeah, I think msg_desc_table with detailed comments also works.
>
> > 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.
Got it. Thanks for the explanation.
Song
