On Wed, Aug 14, 2024, at 11:59, Akshay Gupta wrote:
> +/* input for bulk write to CPUID protocol */
> +struct cpu_msr_indata {
> + u8 wr_len; /* const value */
> + u8 rd_len; /* const value */
> + u8 proto_cmd; /* const value */
> + u8 thread; /* thread number */
> + union {
> + u8 reg_offset[4]; /* input value */
> + u32 value;
> + };
> + u8 ext; /* extended function */
> +} __packed;
You cannot have a fully aligned union inside of a misaligned
struct. Since the only member is the inner 'u32 value',
I think it would make more sense to make that one
__packed instead of the structure.
> +/* output for bulk read from CPUID protocol */
> +struct cpu_msr_outdata {
> + u8 num_bytes; /* number of bytes return */
> + u8 status; /* Protocol status code */
> + union {
> + u64 value;
> + u8 reg_data[8];
> + };
> +} __packed;
Same here.
> +#define prepare_cpuid_input_message(input, thread_id, func, ext_func) \
> + input.rd_len = CPUID_RD_DATA_LEN, \
> + input.wr_len = CPUID_WR_DATA_LEN, \
> + input.proto_cmd = RD_CPUID_CMD, \
> + input.thread = thread_id << 1, \
> + input.value = func, \
> + input.ext = ext_func
This can be an inline function.
> +/*
> + * For Mailbox command software alert status bit is set by firmware
> + * to indicate command completion
> + * For RMI Rev 0x20, new h/w status bit is introduced. which is used
> + * by firmware to indicate completion of commands (0x71, 0x72, 0x73).
> + * wait for the status bit to be set by the firmware before
> + * reading the data out.
> + */
> +static int sbrmi_wait_status(struct sbrmi_data *data,
> + int *status, int mask)
> +{
> + int ret, retry = 100;
> +
> + do {
> + ret = regmap_read(data->regmap, SBRMI_STATUS, status);
> + if (ret < 0)
> + return ret;
> +
> + if (*status & mask)
> + break;
> +
> + /* Wait 1~2 second for firmware to return data out */
> + if (retry > 95)
> + usleep_range(50, 100);
> + else
> + usleep_range(10000, 20000);
> + } while (retry--);
This loop is likely to take much longer than 2 seconds if it
times out because of all the rounding etc.
You can probably change this to regmap_read_poll_timeout(),
which handles timeouts correctly.
> +/* command ID to identify CPUID protocol */
> +#define APML_CPUID 0x1000
> /* These are byte indexes into data_in and data_out arrays */
> #define AMD_SBI_RD_WR_DATA_INDEX 0
> #define AMD_SBI_REG_OFF_INDEX 0
> #define AMD_SBI_REG_VAL_INDEX 4
> #define AMD_SBI_RD_FLAG_INDEX 7
> +#define AMD_SBI_THREAD_LOW_INDEX 4
> +#define AMD_SBI_THREAD_HI_INDEX 5
> +#define AMD_SBI_EXT_FUNC_INDEX 6
>
> #define AMD_SBI_MB_DATA_SIZE 4
>
> struct apml_message {
> /* message ids:
> * Mailbox Messages: 0x0 ... 0x999
> + * APML_CPUID: 0x1000
> */
> __u32 cmd;
>
> /*
> * 8 bit data for reg read,
> * 32 bit data in case of mailbox,
> + * up to 64 bit in case of cpuid
> */
> union {
> + __u64 cpu_msr_out;
> __u32 mb_out[2];
> __u8 reg_out[8];
> } data_out;
>
> /*
> * [0]...[3] mailbox 32bit input
> + * cpuid,
> + * [4][5] cpuid: thread
> + * [6] cpuid: ext function & read eax/ebx or ecx/edx
> + * [7:0] -> bits [7:4] -> ext function &
> + * bit [0] read eax/ebx or ecx/edx
> * [7] read/write functionality
> */
> union {
> + __u64 cpu_msr_in;
> __u32 mb_in[2];
> __u8 reg_in[8];
> } data_in;
> /*
> + * Status code is returned in case of CPUID access
> * Error code is returned in case of soft mailbox
> */
> __u32 fw_ret_code;
Low-level protocols like this are rarely a good idea to be
exported to userspace. I can't see what the exact data is
that you can get in and out, but you probably want higher-level
interfaces for the individual things the platform interface
can do, either hooking up to existing kernel subsystems or
as separate user space interfaces.
Arnd
