[v4,5/5] qemu: add documentation to qfw.h

Also rename a "length" to "size" for consistency with the rest of qfw.

Signed-off-by: Asherah Connor <ashe@kivikakk.ee>
---

(no changes since v1)

 drivers/misc/qfw.c |  6 ++--
 include/qfw.h      | 86 +++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 84 insertions(+), 8 deletions(-)

Hi Asherah,

On Tue, 23 Feb 2021 at 22:24, Asherah Connor <ashe@kivikakk.ee> wrote:
>
> Also rename a "length" to "size" for consistency with the rest of qfw.

Here also you add comments so should mention that.

>
> Signed-off-by: Asherah Connor <ashe@kivikakk.ee>
> ---
>
> (no changes since v1)
>
>  drivers/misc/qfw.c |  6 ++--
>  include/qfw.h      | 86 +++++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 84 insertions(+), 8 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

On 2/24/21 4:23 AM, Asherah Connor wrote:
> Also rename a "length" to "size" for consistency with the rest of qfw.
>
> Signed-off-by: Asherah Connor <ashe@kivikakk.ee>
> ---
>
> (no changes since v1)
>
>   drivers/misc/qfw.c |  6 ++--
>   include/qfw.h      | 86 +++++++++++++++++++++++++++++++++++++++++++---
>   2 files changed, 84 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/misc/qfw.c b/drivers/misc/qfw.c
> index b0c82e2098..3e8c1a9cda 100644
> --- a/drivers/misc/qfw.c
> +++ b/drivers/misc/qfw.c
> @@ -283,14 +283,14 @@ static void qfw_read_entry_dma(struct qfw_dev *qdev, u16 entry, u32 size,
>   	ops->read_entry_dma(qdev->dev, &dma);
>   }
>
> -void qfw_read_entry(struct udevice *dev, u16 entry, u32 length, void *address)
> +void qfw_read_entry(struct udevice *dev, u16 entry, u32 size, void *address)
>   {
>   	struct qfw_dev *qdev = dev_get_uclass_priv(dev);
>
>   	if (qdev->dma_present)
> -		qfw_read_entry_dma(qdev, entry, length, address);
> +		qfw_read_entry_dma(qdev, entry, size, address);
>   	else
> -		qfw_read_entry_io(qdev, entry, length, address);
> +		qfw_read_entry_io(qdev, entry, size, address);
>   }
>
>   int qfw_register(struct udevice *dev)
> diff --git a/include/qfw.h b/include/qfw.h
> index 41d4db08d6..d59c71a5dd 100644
> --- a/include/qfw.h
> +++ b/include/qfw.h
> @@ -8,6 +8,11 @@
>
>   #include <linux/list.h>
>
> +/*
> + * List of firmware configuration item selectors. The official source of truth
> + * for these is the QEMU source itself; see
> + * https://github.com/qemu/qemu/blob/master/hw/nvram/fw_cfg.c
> + */
>   enum {
>   	FW_CFG_SIGNATURE	= 0x00,
>   	FW_CFG_ID		= 0x01,
> @@ -66,8 +71,10 @@ enum {
>   #define FW_CFG_DMA_SKIP	(1 << 2)
>   #define FW_CFG_DMA_SELECT	(1 << 3)
>
> +/* Bit set in FW_CFG_ID response to indicate DMA interface availability. */
>   #define FW_CFG_DMA_ENABLED	(1 << 1)
>
> +/* Structs read from FW_CFG_FILE_DIR. */
>   struct fw_cfg_file {
>   	__be32 size;
>   	__be16 select;
> @@ -134,27 +141,56 @@ struct bios_linker_entry {
>   	};
>   } __packed;
>
> +/* DMA transfer control data between UCLASS_QFW and QEMU. */
>   struct qfw_dma {
>   	__be32 control;
>   	__be32 length;
>   	__be64 address;
>   };
>
> +/* uclass per-device configuration information */
>   struct qfw_dev {
> -	struct udevice *dev;
> -	bool dma_present;
> -	struct list_head fw_list;
> +	struct udevice *dev;		/* Transport device */
> +	bool dma_present;		/* DMA interface usable? */
> +	struct list_head fw_list;	/* Cached firmware file list */
>   };
>
> +/* Ops used internally between UCLASS_QFW and its driver implementations. */
>   struct dm_qfw_ops {
> +	/**
> +	 * read_entry_io() - Read a firmware config entry using the regular
> +	 * IO interface for the platform (either PIO or MMIO)
> +	 *
> +	 * Supply FW_CFG_INVALID as the entry to continue a previous read.  In
> +	 * this case, no selector will be issued before reading.
> +	 *
> +	 * @dev: Device to use
> +	 * @entry: Firmware config entry number (e.g. FW_CFG_SIGNATURE)
> +	 * @size: Number of bytes to read
> +	 * @address: Target location for read
> +	 */
>   	void (*read_entry_io)(struct udevice *dev, u16 entry, u32 size,
>   			      void *address);
> +
> +	/**
> +	 * read_entry_dma() - Read a firmware config entry using the DMA
> +	 * interface
> +	 *
> +	 * Supply FW_CFG_INVALID as the entry to continue a previous read.  In
> +	 * this case, no selector will be issued before reading.
> +	 *
> +	 * This method assumes DMA availability has already been confirmed.
> +	 *
> +	 * @dev: Device to use
> +	 * @dma: DMA transfer control struct
> +	 */
>   	void (*read_entry_dma)(struct udevice *dev, struct qfw_dma *dma);
>   };
>
>   #define dm_qfw_get_ops(dev) \
>   		((struct dm_qfw_ops *)(dev)->driver->ops)
>
> +/* Used internally by driver implementations on successful probe. */
>   int qfw_register(struct udevice *dev);
>
>   struct udevice;
> @@ -168,8 +204,37 @@ struct udevice;
>    */
>   int qfw_get_dev(struct udevice **devp);
>
> -void qfw_read_entry(struct udevice *dev, u16 entry, u32 length, void *address);
> +/**
> + * Read a QEMU firmware config entry

This will not generate documentation for qfw_read_entry() with Sphinx.

See
https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation

> + *
> + * The appropriate transport and interface will be selected automatically.
> + *
> + * @dev: Device to use
> + * @entry: Firmware config entry number (e.g. FW_CFG_SIGNATURE)
> + * @size: Number of bytes to read
> + * @address: Target location for read
> + */
> +void qfw_read_entry(struct udevice *dev, u16 entry, u32 size, void *address);
> +
> +/**
> + * Reads the QEMU firmware config file list, for later use with qfw_find_file.
> + *
> + * If the list has already been read, returns 0 (success).
> + *
> + * @dev: Device to use
> + *
> + * @return 0 on success, -ENOMEM if unable to allocate.

This is not valid Sphinx syntax.

> + */
>   int qfw_read_firmware_list(struct udevice *dev);
> +
> +/**
> + * Finds a file by name in the QEMU firmware config file list.

Function name missing.

> + *
> + * @dev: Device to use
> + * @name: Name of file to locate (e.g. "etc/table-loader")
> + *
> + * @return pointer to fw_file struct if found, NULL if not present.

Invalid syntax.

> + */
>   struct fw_file *qfw_find_file(struct udevice *dev, const char *name);
>
>   /**
> @@ -179,7 +244,18 @@ struct fw_file *qfw_find_file(struct udevice *dev, const char *name);
>    */
>   int qfw_online_cpus(struct udevice *dev);
>
> -/* helper functions to iterate firmware file list */
> +/**
> + * Helper functions to iterate firmware file list. Suggested use:

Please, describe each function individually in Sphinx style.

Best regards

Heinrich

> + *
> + * struct fw_cfg_file_iter iter;
> + * struct fw_file *file;
> + *
> + * for (file = qfw_file_iter_init(dev, &iter);
> + *      !qfw_file_iter_end(&iter);
> + *      file = qfw_file_iter_next(&iter)) {
> + *         // use `file'
> + * }
> + */
>   struct fw_file *qfw_file_iter_init(struct udevice *dev,
>   				   struct fw_cfg_file_iter *iter);
>   struct fw_file *qfw_file_iter_next(struct fw_cfg_file_iter *iter);
>

On 21/02/25 02:02:p, Simon Glass wrote:
> On Tue, 23 Feb 2021 at 22:24, Asherah Connor <ashe@kivikakk.ee> wrote:
> >
> > Also rename a "length" to "size" for consistency with the rest of qfw.
> 
> Here also you add comments so should mention that.

I thought that may be considered redundant with the subject; will add to
both.

Best,

Asherah

On 21/02/25 09:02:p, Heinrich Schuchardt wrote:
> > -void qfw_read_entry(struct udevice *dev, u16 entry, u32 length, void *address);
> > +/**
> > + * Read a QEMU firmware config entry
> 
> This will not generate documentation for qfw_read_entry() with Sphinx.
> 
> See
> https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation

Thank you!  I am now testing documentation produced using

scripts/kernel-doc -man include/qfw.h | man --local-file -

If there's a better way, please let me know.

> > + * @return 0 on success, -ENOMEM if unable to allocate.
> 
> This is not valid Sphinx syntax.
[...]
> function name missing.
[...]
> invalid syntax.
[...]
> Please, describe each function individually in Sphinx style.

I'll correct these in the next version.  Thank you!

Best,

Asherah