Re: [PATCH v3 5/8] misc: amd-sbi: Add support for CPUID protocol

[Gupta, Akshay]

On 8/19/2024 3:57 PM, Arnd Bergmann wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> 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.
Thank you for suggestion, will update this.
> > +/* 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.
Thank you for suggestion, will update this.
> > +#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.
Will change to 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.
Thank you for suggestion, will use the regmap_read_poll_timeout()
> > +/* 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

we have opensource C library and user space tool.
All the platform level support is handled by the library. IOCTL is the 
single entry point to the Linux kernel which
communicates with the SMU, using the protocols and handles the 
synchronization.
Library is hosted at: https://github.com/amd/esmi_oob_library