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.
> + 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?
> +
> + 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.
> + * @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.
> + * @version: Interface version
> + * @rsvd: Word boundary padding
word size seems to be varying between structures. I'd just document
it as "reserved"
> + * @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
