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

2021-02-25 Thread Asherah Connor
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


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

2021-02-25 Thread Asherah Connor
On 21/02/25 02:02:p, Simon Glass wrote:
> On Tue, 23 Feb 2021 at 22:24, Asherah Connor  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


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

2021-02-25 Thread Heinrich Schuchardt

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 
---

(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, );
  }

-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 

+/*
+ * 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 

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

2021-02-25 Thread Simon Glass
Hi Asherah,

On Tue, 23 Feb 2021 at 22:24, Asherah Connor  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 
> ---
>
> (no changes since v1)
>
>  drivers/misc/qfw.c |  6 ++--
>  include/qfw.h  | 86 +++---
>  2 files changed, 84 insertions(+), 8 deletions(-)
>

Reviewed-by: Simon Glass 


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

2021-02-23 Thread Asherah Connor
Also rename a "length" to "size" for consistency with the rest of qfw.

Signed-off-by: Asherah Connor 
---

(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, );
 }
 
-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 
 
+/*
+ * 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
+ *
+ * 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.
+ */
 int qfw_read_firmware_list(struct udevice *dev);
+
+/**
+ * Finds a file by name in the QEMU firmware config file list.
+ *
+ * @dev: Device to use
+ * @name: Name of file to locate (e.g. "etc/table-loader")
+ *
+ *