On 3/4/2025 12:30 AM, Jonathan Cameron wrote:
On Fri, 28 Feb 2025 17:35:52 -0800
Shannon Nelson <shannon.nelson@xxxxxxx> wrote:
Initial files for adding a new fwctl driver for the AMD/Pensando PDS
devices. This sets up a simple auxiliary_bus driver that registers
with fwctl subsystem. It expects that a pds_core device has set up
the auxiliary_device pds_core.fwctl
Signed-off-by: Shannon Nelson <shannon.nelson@xxxxxxx>
A few minor comments inline,
diff --git a/drivers/fwctl/pds/main.c b/drivers/fwctl/pds/main.c
new file mode 100644
index 000000000000..afc7dc283ad5
--- /dev/null
+++ b/drivers/fwctl/pds/main.c
@@ -0,0 +1,169 @@
+struct pdsfc_dev {
+ struct fwctl_device fwctl;
+ struct pds_auxiliary_dev *padev;
+ struct pdsc *pdsc;
Not used in this patch that I can see. I was curious why it is
here as I would expect that to match with the padev parent.
I'll take it out.
+ u32 caps;
+ struct pds_fwctl_ident ident;
+};
+DEFINE_FREE(pdsfc_dev, struct pdsfc_dev *, if (_T) fwctl_put(&_T->fwctl));
+static void *pdsfc_info(struct fwctl_uctx *uctx, size_t *length)
+{
+ struct pdsfc_uctx *pdsfc_uctx = container_of(uctx, struct pdsfc_uctx, uctx);
+ struct fwctl_info_pds *info;
+
+ info = kzalloc(sizeof(*info), GFP_KERNEL);
+ if (!info)
+ return ERR_PTR(-ENOMEM);
+
+ info->uctx_caps = pdsfc_uctx->uctx_caps;
What about the UID? If it is always zero, what is it for?
Early code, thought we'd use it. I'll take it out.
+
+ return info;
+}
+/**
+ * struct pds_fwctl_comp - Firmware control completion structure
+ * @status: Status of the firmware control operation
+ * @rsvd: Word boundary padding
+ * @comp_index: Completion index in little-endian format
+ * @rsvd2: Word boundary padding
11 bytes is some extreme word padding. I'd just call it reserved
space.
This is the same wording we've used in the other interface files, so at
least we're consistently odd.
I'll tweak these.
+ * @color: Color bit indicating the state of the completion
+ */
+struct pds_fwctl_comp {
+ u8 status;
+ u8 rsvd;
+ __le16 comp_index;
+ u8 rsvd2[11];
+ u8 color;
+} __packed;
...
+/**
+ * struct pds_fwctl_ident - Firmware control identification structure
+ * @features: Supported features
Nice to have some defines or similar that give meaning to these
features, but maybe that comes in later patches.
We don't have any defined yet, but we have found it is useful to define
this field for future growth without breaking APIs. I'll make that more
clear.
+ * @version: Interface version
+ * @rsvd: Word boundary padding
word size seems to be varying between structures. I'd just document
it as "reserved"
Sure
Thanks,
sln
+ * @max_req_sz: Maximum request size
+ * @max_resp_sz: Maximum response size
+ * @max_req_sg_elems: Maximum number of request SGs
+ * @max_resp_sg_elems: Maximum number of response SGs
+ */
+struct pds_fwctl_ident {
+ __le64 features;
+ u8 version;
+ u8 rsvd[3];
+ __le32 max_req_sz;
+ __le32 max_resp_sz;
+ u8 max_req_sg_elems;
+ u8 max_resp_sg_elems;
+} __packed;
Jonathan
