Messages in this thread | | | From | "Verma, Vishal L" <> | Subject | Re: [PATCH 3/4] cxl: add a firmware update mechanism using the sysfs firmware loader | Date | Fri, 19 May 2023 20:24:13 +0000 |
| |
On Thu, 2023-05-18 at 19:58 -0700, Alison Schofield wrote: > > > > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h > > index 8c3302fc7738..0ecee5b558f4 100644 > > --- a/drivers/cxl/cxlmem.h > > +++ b/drivers/cxl/cxlmem.h > > snip > > > + * Get Firmware Info > > + * CXL rev 3.0 section 8.2.9.3.1; Table 8-56 > > + */ > > +struct cxl_mbox_get_fw_info { > > + u8 num_slots; > > + u8 slot_info; > > + u8 activation_cap; > > + u8 reserved[13]; > > + char slot_1_revision[0x10]; > > + char slot_2_revision[0x10]; > > + char slot_3_revision[0x10]; > > + char slot_4_revision[0x10]; > > The practice here is to use decimals [16]
I looked around, and see cxl_identify, cxl_event_mem_module, and cxl_event_dram all have a few hex ones thrown in each. So I looked to the spec to see if there was any consistency there, and unfortunately it isn't either. The spec does say 'Length in bytes' 16 for these fw revision fields though, so I think it makes sense changing these to decimal..
> > > +} __packed; > > + > > snip > > > + * Transfer Firmware Input Payload > > + * CXL rev 3.0 section 8.2.9.3.2; Table 8-57 > > + */ > > +struct cxl_mbox_transfer_fw { > > + u8 action; > > + u8 slot; > > + u8 reserved[2]; > > + __le32 offset; > > + u8 reserved2[0x78]; > > Here too.
..however for this one the spec has '78h'. I think we should just match the spec in these cases so anyone cross referencing the header and spec is saved from doing a bunch of math for these.
> > That's all for now. > Thanks for taking a look!
| |