diff mbox series

[5/7] bcm-vk: add bcm_vk UAPI

Message ID 20190822192451.5983-6-scott.branden@broadcom.com (mailing list archive)
State New, archived
Headers show
Series firmware: add partial read support in request_firmware_into_buf | expand

Commit Message

Scott Branden Aug. 22, 2019, 7:24 p.m. UTC 
Add user space api for bcm-vk driver.

Signed-off-by: Scott Branden <scott.branden@broadcom.com>
---
 include/uapi/linux/misc/bcm_vk.h | 88 ++++++++++++++++++++++++++++++++
 1 file changed, 88 insertions(+)
 create mode 100644 include/uapi/linux/misc/bcm_vk.h

Comments

Arnd Bergmann Aug. 27, 2019, 1:54 p.m. UTC | #1 
On Thu, Aug 22, 2019 at 9:25 PM Scott Branden
<scott.branden@broadcom.com> wrote:
>
> Add user space api for bcm-vk driver.

> +
> +struct vk_metadata {
> +       /* struct version, always backwards compatible */
> +       __u32 version;
> +
> +       /* Version 0 fields */
> +       __u32 card_status;
> +#define VK_CARD_STATUS_FASTBOOT_READY BIT(0)
> +#define VK_CARD_STATUS_FWLOADER_READY BIT(1)
> +
> +       __u32 firmware_version;
> +       __u32 fw_status;
> +       /* End version 0 fields */
> +
> +       __u64 reserved[14];
> +       /* Total of 16*u64 for all versions */
> +};

I'd suggest getting rid of the API version fields, just leave the version 0
fields here and add a new structure + ioctl if you need other
fields

Versioning usually just adds complexity and is hard to get right.

> +struct vk_access {
> +       __u8 barno;     /* BAR number to use */
> +       __u8 type;      /* Type of access */
> +#define VK_ACCESS_READ 0
> +#define VK_ACCESS_WRITE 1
> +       __u32 len;      /* length of data */
> +       __u64 offset;   /* offset in BAR */
> +       __u32 *data;    /* where to read/write data to */
> +};

The pointer in the last member makes the structure incompatible between
32-bit and 64-bit user space. You could work around that using a __u64
member and turning that into a pointer using the u64_to_user_ptr()
macro in the driver in a portable way.

However, since this seems to be a read/write type interface, maybe
it's better to just use read/write file operations.

I also wonder if the interface should be on a higher abstraction level
here.

      Arnd
Kieran Bingham Aug. 27, 2019, 2:49 p.m. UTC | #2 
Hi Scott,

On 22/08/2019 20:24, Scott Branden wrote:
> Add user space api for bcm-vk driver.
> 
> Signed-off-by: Scott Branden <scott.branden@broadcom.com>
> ---
>  include/uapi/linux/misc/bcm_vk.h | 88 ++++++++++++++++++++++++++++++++
>  1 file changed, 88 insertions(+)
>  create mode 100644 include/uapi/linux/misc/bcm_vk.h
> 
> diff --git a/include/uapi/linux/misc/bcm_vk.h b/include/uapi/linux/misc/bcm_vk.h
> new file mode 100644
> index 000000000000..df7dfd7f0702
> --- /dev/null
> +++ b/include/uapi/linux/misc/bcm_vk.h
> @@ -0,0 +1,88 @@
> +/* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-2-Clause) */
> +/*
> + * Copyright(c) 2018 Broadcom
> + */
> +
> +#ifndef __UAPI_LINUX_MISC_BCM_VK_H
> +#define __UAPI_LINUX_MISC_BCM_VK_H
> +
> +#include <linux/ioctl.h>
> +#include <linux/types.h>
> +
> +struct vk_metadata {
> +	/* struct version, always backwards compatible */
> +	__u32 version;
> +
> +	/* Version 0 fields */
> +	__u32 card_status;
> +#define VK_CARD_STATUS_FASTBOOT_READY BIT(0)
> +#define VK_CARD_STATUS_FWLOADER_READY BIT(1)
> +
> +	__u32 firmware_version;
> +	__u32 fw_status;
> +	/* End version 0 fields */
> +
> +	__u64 reserved[14];
> +	/* Total of 16*u64 for all versions */
> +};
> +
> +struct vk_image {
> +	__u32 type;     /* Type of image */
> +#define VK_IMAGE_TYPE_BOOT1 1 /* 1st stage (load to SRAM) */
> +#define VK_IMAGE_TYPE_BOOT2 2 /* 2nd stage (load to DDR) */
> +	char filename[64]; /* Filename of image */
> +};
> +
> +/* default firmware images names */
> +#define VK_BOOT1_DEF_FILENAME	    "vk-boot1.bin"
> +#define VK_BOOT2_DEF_FILENAME	    "vk-boot2.bin"
> +
> +struct vk_access {
> +	__u8 barno;     /* BAR number to use */
> +	__u8 type;      /* Type of access */
> +#define VK_ACCESS_READ 0
> +#define VK_ACCESS_WRITE 1
> +	__u32 len;      /* length of data */
> +	__u64 offset;   /* offset in BAR */
> +	__u32 *data;    /* where to read/write data to */
> +};
> +
> +struct vk_reset {
> +	__u32 arg1;
> +	__u32 arg2;
> +};
> +
> +#define VK_MAGIC              0x5E
> +
> +/* Get metadata from Valkyrie (firmware version, card status, etc) */
> +#define VK_IOCTL_GET_METADATA _IOR(VK_MAGIC, 0x1, struct vk_metadata)
> +
> +/* Load image to Valkyrie */
> +#define VK_IOCTL_LOAD_IMAGE   _IOW(VK_MAGIC, 0x2, struct vk_image)
> +
> +/* Read data from Valkyrie */
> +#define VK_IOCTL_ACCESS_BAR   _IOWR(VK_MAGIC, 0x3, struct vk_access)
> +
> +/* Send Reset to Valkyrie */
> +#define VK_IOCTL_RESET        _IOW(VK_MAGIC, 0x4, struct vk_reset)

It sounds a bit like the valkyrie is a generic asynchronous coprocessor,
does it merit using the remoteproc interfaces to control it ?

Or is it really just a single purpose cell doing video operations ?

--
Kieran

> +
> +/*
> + * message block - basic unit in the message where a message's size is always
> + *		   N x sizeof(basic_block)
> + */
> +struct vk_msg_blk {
> +	__u8 function_id;
> +#define VK_FID_TRANS_BUF 5
> +#define VK_FID_SHUTDOWN  8
> +	__u8 size;
> +	__u16 queue_id:4;
> +	__u16 msg_id:12;
> +	__u32 context_id;
> +	__u32 args[2];
> +#define VK_CMD_PLANES_MASK 0x000F /* number of planes to up/download */
> +#define VK_CMD_UPLOAD      0x0400 /* memory transfer to vk */
> +#define VK_CMD_DOWNLOAD    0x0500 /* memory transfer from vk */
> +#define VK_CMD_MASK        0x0F00 /* command mask */
> +};
> +
> +#endif /* __UAPI_LINUX_MISC_BCM_VK_H */
>
Olof Johansson Oct. 8, 2019, 3:59 p.m. UTC | #3 
On Tue, Aug 27, 2019 at 7:49 AM Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Hi Scott,
>
> On 22/08/2019 20:24, Scott Branden wrote:
> > Add user space api for bcm-vk driver.
> >
> > Signed-off-by: Scott Branden <scott.branden@broadcom.com>
> > ---
> >  include/uapi/linux/misc/bcm_vk.h | 88 ++++++++++++++++++++++++++++++++
> >  1 file changed, 88 insertions(+)
> >  create mode 100644 include/uapi/linux/misc/bcm_vk.h
> >
> > diff --git a/include/uapi/linux/misc/bcm_vk.h b/include/uapi/linux/misc/bcm_vk.h
> > new file mode 100644
> > index 000000000000..df7dfd7f0702
> > --- /dev/null
> > +++ b/include/uapi/linux/misc/bcm_vk.h
> > @@ -0,0 +1,88 @@
> > +/* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-2-Clause) */
> > +/*
> > + * Copyright(c) 2018 Broadcom
> > + */
> > +
> > +#ifndef __UAPI_LINUX_MISC_BCM_VK_H
> > +#define __UAPI_LINUX_MISC_BCM_VK_H
> > +
> > +#include <linux/ioctl.h>
> > +#include <linux/types.h>
> > +
> > +struct vk_metadata {
> > +     /* struct version, always backwards compatible */
> > +     __u32 version;
> > +
> > +     /* Version 0 fields */
> > +     __u32 card_status;
> > +#define VK_CARD_STATUS_FASTBOOT_READY BIT(0)
> > +#define VK_CARD_STATUS_FWLOADER_READY BIT(1)
> > +
> > +     __u32 firmware_version;
> > +     __u32 fw_status;
> > +     /* End version 0 fields */
> > +
> > +     __u64 reserved[14];
> > +     /* Total of 16*u64 for all versions */
> > +};
> > +
> > +struct vk_image {
> > +     __u32 type;     /* Type of image */
> > +#define VK_IMAGE_TYPE_BOOT1 1 /* 1st stage (load to SRAM) */
> > +#define VK_IMAGE_TYPE_BOOT2 2 /* 2nd stage (load to DDR) */
> > +     char filename[64]; /* Filename of image */
> > +};
> > +
> > +/* default firmware images names */
> > +#define VK_BOOT1_DEF_FILENAME            "vk-boot1.bin"
> > +#define VK_BOOT2_DEF_FILENAME            "vk-boot2.bin"
> > +
> > +struct vk_access {
> > +     __u8 barno;     /* BAR number to use */
> > +     __u8 type;      /* Type of access */
> > +#define VK_ACCESS_READ 0
> > +#define VK_ACCESS_WRITE 1
> > +     __u32 len;      /* length of data */
> > +     __u64 offset;   /* offset in BAR */
> > +     __u32 *data;    /* where to read/write data to */
> > +};
> > +
> > +struct vk_reset {
> > +     __u32 arg1;
> > +     __u32 arg2;
> > +};
> > +
> > +#define VK_MAGIC              0x5E
> > +
> > +/* Get metadata from Valkyrie (firmware version, card status, etc) */
> > +#define VK_IOCTL_GET_METADATA _IOR(VK_MAGIC, 0x1, struct vk_metadata)
> > +
> > +/* Load image to Valkyrie */
> > +#define VK_IOCTL_LOAD_IMAGE   _IOW(VK_MAGIC, 0x2, struct vk_image)
> > +
> > +/* Read data from Valkyrie */
> > +#define VK_IOCTL_ACCESS_BAR   _IOWR(VK_MAGIC, 0x3, struct vk_access)
> > +
> > +/* Send Reset to Valkyrie */
> > +#define VK_IOCTL_RESET        _IOW(VK_MAGIC, 0x4, struct vk_reset)
>
> It sounds a bit like the valkyrie is a generic asynchronous coprocessor,
> does it merit using the remoteproc interfaces to control it ?
>
> Or is it really just a single purpose cell doing video operations ?

Remoteproc brings some useful shared functionality, in particular
around loading and parsing firmware formats for platforms where the
remote processor uses system carved out memory to run, etc.

For something like a PCIe device, it *can* be used but it really
doesn't bring any immediate benefit, especially if there aren't
multiple in-kernel drivers that need to talk to the hardware in an
abstracted way.


-Olof



-Olof
diff mbox series

Patch

diff --git a/include/uapi/linux/misc/bcm_vk.h b/include/uapi/linux/misc/bcm_vk.h
new file mode 100644
index 000000000000..df7dfd7f0702
--- /dev/null
+++ b/include/uapi/linux/misc/bcm_vk.h
@@ -0,0 +1,88 @@ 
+/* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-2-Clause) */
+/*
+ * Copyright(c) 2018 Broadcom
+ */
+
+#ifndef __UAPI_LINUX_MISC_BCM_VK_H
+#define __UAPI_LINUX_MISC_BCM_VK_H
+
+#include <linux/ioctl.h>
+#include <linux/types.h>
+
+struct vk_metadata {
+	/* struct version, always backwards compatible */
+	__u32 version;
+
+	/* Version 0 fields */
+	__u32 card_status;
+#define VK_CARD_STATUS_FASTBOOT_READY BIT(0)
+#define VK_CARD_STATUS_FWLOADER_READY BIT(1)
+
+	__u32 firmware_version;
+	__u32 fw_status;
+	/* End version 0 fields */
+
+	__u64 reserved[14];
+	/* Total of 16*u64 for all versions */
+};
+
+struct vk_image {
+	__u32 type;     /* Type of image */
+#define VK_IMAGE_TYPE_BOOT1 1 /* 1st stage (load to SRAM) */
+#define VK_IMAGE_TYPE_BOOT2 2 /* 2nd stage (load to DDR) */
+	char filename[64]; /* Filename of image */
+};
+
+/* default firmware images names */
+#define VK_BOOT1_DEF_FILENAME	    "vk-boot1.bin"
+#define VK_BOOT2_DEF_FILENAME	    "vk-boot2.bin"
+
+struct vk_access {
+	__u8 barno;     /* BAR number to use */
+	__u8 type;      /* Type of access */
+#define VK_ACCESS_READ 0
+#define VK_ACCESS_WRITE 1
+	__u32 len;      /* length of data */
+	__u64 offset;   /* offset in BAR */
+	__u32 *data;    /* where to read/write data to */
+};
+
+struct vk_reset {
+	__u32 arg1;
+	__u32 arg2;
+};
+
+#define VK_MAGIC              0x5E
+
+/* Get metadata from Valkyrie (firmware version, card status, etc) */
+#define VK_IOCTL_GET_METADATA _IOR(VK_MAGIC, 0x1, struct vk_metadata)
+
+/* Load image to Valkyrie */
+#define VK_IOCTL_LOAD_IMAGE   _IOW(VK_MAGIC, 0x2, struct vk_image)
+
+/* Read data from Valkyrie */
+#define VK_IOCTL_ACCESS_BAR   _IOWR(VK_MAGIC, 0x3, struct vk_access)
+
+/* Send Reset to Valkyrie */
+#define VK_IOCTL_RESET        _IOW(VK_MAGIC, 0x4, struct vk_reset)
+
+/*
+ * message block - basic unit in the message where a message's size is always
+ *		   N x sizeof(basic_block)
+ */
+struct vk_msg_blk {
+	__u8 function_id;
+#define VK_FID_TRANS_BUF 5
+#define VK_FID_SHUTDOWN  8
+	__u8 size;
+	__u16 queue_id:4;
+	__u16 msg_id:12;
+	__u32 context_id;
+	__u32 args[2];
+#define VK_CMD_PLANES_MASK 0x000F /* number of planes to up/download */
+#define VK_CMD_UPLOAD      0x0400 /* memory transfer to vk */
+#define VK_CMD_DOWNLOAD    0x0500 /* memory transfer from vk */
+#define VK_CMD_MASK        0x0F00 /* command mask */
+};
+
+#endif /* __UAPI_LINUX_MISC_BCM_VK_H */