Re: [Qemu-devel] [PATCH v4 2/5] fw_cfg: add DMA register

[Gabriel L. Somlo]

On Tue, Oct 31, 2017 at 04:19:35PM +0100, Marc-André Lureau wrote:
> Add an optional <dma_off> kernel module (or command line) parameter
> using the following syntax:
> 
>       [qemu_fw_cfg.]ioport=<size>@<base>[:<ctrl_off>:<data_off>[:<dma_off>]]
>  or
>       [qemu_fw_cfg.]mmio=<size>@<base>[:<ctrl_off>:<data_off>[:<dma_off>]]
> 
> and initializes the register address using given or default offset.
> 
> Signed-off-by: Marc-André Lureau <address@hidden>

Reviewed-by: Gabriel Somlo <address@hidden>

> ---
>  drivers/firmware/qemu_fw_cfg.c | 53 
> ++++++++++++++++++++++++++++++++----------
>  1 file changed, 41 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c
> index 5cfe39f7a45f..1f3e8545dab7 100644
> --- a/drivers/firmware/qemu_fw_cfg.c
> +++ b/drivers/firmware/qemu_fw_cfg.c
> @@ -10,20 +10,21 @@
>   * and select subsets of aarch64), a Device Tree node (on arm), or using
>   * a kernel module (or command line) parameter with the following syntax:
>   *
> - *      [qemu_fw_cfg.]ioport=<size>@<base>[:<ctrl_off>:<data_off>]
> + *      
> [qemu_fw_cfg.]ioport=<size>@<base>[:<ctrl_off>:<data_off>[:<dma_off>]]
>   * or
> - *      [qemu_fw_cfg.]mmio=<size>@<base>[:<ctrl_off>:<data_off>]
> + *      [qemu_fw_cfg.]mmio=<size>@<base>[:<ctrl_off>:<data_off>[:<dma_off>]]
>   *
>   * where:
>   *      <size>     := size of ioport or mmio range
>   *      <base>     := physical base address of ioport or mmio range
>   *      <ctrl_off> := (optional) offset of control register
>   *      <data_off> := (optional) offset of data register
> + *      <dma_off> := (optional) offset of dma register
>   *
>   * e.g.:
> - *      address@hidden:0:1           (the default on x86)
> + *      address@hidden:0:1:4 (the default on x86)
>   * or
> - *      address@hidden:8:0   (the default on arm)
> + *      address@hidden:8:0:16        (the default on arm)
>   */
>  
>  #include <linux/module.h>
> @@ -63,6 +64,7 @@ static resource_size_t fw_cfg_p_size;
>  static void __iomem *fw_cfg_dev_base;
>  static void __iomem *fw_cfg_reg_ctrl;
>  static void __iomem *fw_cfg_reg_data;
> +static void __iomem *fw_cfg_reg_dma;
>  
>  /* atomic access to fw_cfg device (potentially slow i/o, so using mutex) */
>  static DEFINE_MUTEX(fw_cfg_dev_lock);
> @@ -118,12 +120,14 @@ static void fw_cfg_io_cleanup(void)
>  # if (defined(CONFIG_ARM) || defined(CONFIG_ARM64))
>  #  define FW_CFG_CTRL_OFF 0x08
>  #  define FW_CFG_DATA_OFF 0x00
> +#  define FW_CFG_DMA_OFF 0x10
>  # elif (defined(CONFIG_PPC_PMAC) || defined(CONFIG_SPARC32)) /* 
> ppc/mac,sun4m */
>  #  define FW_CFG_CTRL_OFF 0x00
>  #  define FW_CFG_DATA_OFF 0x02
>  # elif (defined(CONFIG_X86) || defined(CONFIG_SPARC64)) /* x86, sun4u */
>  #  define FW_CFG_CTRL_OFF 0x00
>  #  define FW_CFG_DATA_OFF 0x01
> +#  define FW_CFG_DMA_OFF 0x04
>  # else
>  #  error "QEMU FW_CFG not available on this architecture!"
>  # endif
> @@ -133,7 +137,7 @@ static void fw_cfg_io_cleanup(void)
>  static int fw_cfg_do_platform_probe(struct platform_device *pdev)
>  {
>       char sig[FW_CFG_SIG_SIZE];
> -     struct resource *range, *ctrl, *data;
> +     struct resource *range, *ctrl, *data, *dma;
>  
>       /* acquire i/o range details */
>       fw_cfg_is_mmio = false;
> @@ -170,6 +174,7 @@ static int fw_cfg_do_platform_probe(struct 
> platform_device *pdev)
>       /* were custom register offsets provided (e.g. on the command line)? */
>       ctrl = platform_get_resource_byname(pdev, IORESOURCE_REG, "ctrl");
>       data = platform_get_resource_byname(pdev, IORESOURCE_REG, "data");
> +     dma = platform_get_resource_byname(pdev, IORESOURCE_REG, "dma");
>       if (ctrl && data) {
>               fw_cfg_reg_ctrl = fw_cfg_dev_base + ctrl->start;
>               fw_cfg_reg_data = fw_cfg_dev_base + data->start;
> @@ -179,6 +184,13 @@ static int fw_cfg_do_platform_probe(struct 
> platform_device *pdev)
>               fw_cfg_reg_data = fw_cfg_dev_base + FW_CFG_DATA_OFF;
>       }
>  
> +     if (dma)
> +             fw_cfg_reg_dma = fw_cfg_dev_base + dma->start;
> +#ifdef FW_CFG_DMA_OFF
> +     else
> +             fw_cfg_reg_dma = fw_cfg_dev_base + FW_CFG_DMA_OFF;
> +#endif
> +
>       /* verify fw_cfg device signature */
>       fw_cfg_read_blob(FW_CFG_SIGNATURE, sig, 0, FW_CFG_SIG_SIZE);
>       if (memcmp(sig, "QEMU", FW_CFG_SIG_SIZE) != 0) {
> @@ -628,6 +640,7 @@ static struct platform_device *fw_cfg_cmdline_dev;
>  /* use special scanf/printf modifier for phys_addr_t, resource_size_t */
>  #define PH_ADDR_SCAN_FMT "@%" __PHYS_ADDR_PREFIX "i%n" \
>                        ":%" __PHYS_ADDR_PREFIX "i" \
> +                      ":%" __PHYS_ADDR_PREFIX "i%n" \
>                        ":%" __PHYS_ADDR_PREFIX "i%n"
>  
>  #define PH_ADDR_PR_1_FMT "0x%" __PHYS_ADDR_PREFIX "x@" \
> @@ -637,12 +650,15 @@ static struct platform_device *fw_cfg_cmdline_dev;
>                        ":%" __PHYS_ADDR_PREFIX "u" \
>                        ":%" __PHYS_ADDR_PREFIX "u"
>  
> +#define PH_ADDR_PR_4_FMT PH_ADDR_PR_3_FMT \
> +                      ":%" __PHYS_ADDR_PREFIX "u"
> +
>  static int fw_cfg_cmdline_set(const char *arg, const struct kernel_param *kp)
>  {
> -     struct resource res[3] = {};
> +     struct resource res[4] = {};
>       char *str;
>       phys_addr_t base;
> -     resource_size_t size, ctrl_off, data_off;
> +     resource_size_t size, ctrl_off, data_off, dma_off;
>       int processed, consumed = 0;
>  
>       /* only one fw_cfg device can exist system-wide, so if one
> @@ -658,19 +674,20 @@ static int fw_cfg_cmdline_set(const char *arg, const 
> struct kernel_param *kp)
>       /* consume "<size>" portion of command line argument */
>       size = memparse(arg, &str);
>  
> -     /* get "@<base>[:<ctrl_off>:<data_off>]" chunks */
> +     /* get "@<base>[:<ctrl_off>:<data_off>[:<dma_off>]]" chunks */
>       processed = sscanf(str, PH_ADDR_SCAN_FMT,
>                          &base, &consumed,
> -                        &ctrl_off, &data_off, &consumed);
> +                        &ctrl_off, &data_off, &consumed,
> +                        &dma_off, &consumed);
>  
> -     /* sscanf() must process precisely 1 or 3 chunks:
> +     /* sscanf() must process precisely 1, 3 or 4 chunks:
>        * <base> is mandatory, optionally followed by <ctrl_off>
> -      * and <data_off>;
> +      * and <data_off>, and <dma_off>;
>        * there must be no extra characters after the last chunk,
>        * so str[consumed] must be '\0'.
>        */
>       if (str[consumed] ||
> -         (processed != 1 && processed != 3))
> +         (processed != 1 && processed != 3 && processed != 4))
>               return -EINVAL;
>  
>       res[0].start = base;
> @@ -687,6 +704,11 @@ static int fw_cfg_cmdline_set(const char *arg, const 
> struct kernel_param *kp)
>               res[2].start = data_off;
>               res[2].flags = IORESOURCE_REG;
>       }
> +     if (processed > 3) {
> +             res[3].name = "dma";
> +             res[3].start = dma_off;
> +             res[3].flags = IORESOURCE_REG;
> +     }
>  
>       /* "processed" happens to nicely match the number of resources
>        * we need to pass in to this platform device.
> @@ -721,6 +743,13 @@ static int fw_cfg_cmdline_get(char *buf, const struct 
> kernel_param *kp)
>                               fw_cfg_cmdline_dev->resource[0].start,
>                               fw_cfg_cmdline_dev->resource[1].start,
>                               fw_cfg_cmdline_dev->resource[2].start);
> +     case 4:
> +             return snprintf(buf, PAGE_SIZE, PH_ADDR_PR_4_FMT,
> +                             resource_size(&fw_cfg_cmdline_dev->resource[0]),
> +                             fw_cfg_cmdline_dev->resource[0].start,
> +                             fw_cfg_cmdline_dev->resource[1].start,
> +                             fw_cfg_cmdline_dev->resource[2].start,
> +                             fw_cfg_cmdline_dev->resource[3].start);
>       }
>  
>       /* Should never get here */
> -- 
> 2.15.0.rc0.40.gaefcc5f6f
>