On 06-11-19, 20:22, Alexander Gordeev wrote:
> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> index 7af874b69ffb..f6f43480a4a4 100644
> --- a/drivers/dma/Kconfig
> +++ b/drivers/dma/Kconfig
> @@ -669,6 +669,8 @@ source "drivers/dma/sh/Kconfig"
>
> source "drivers/dma/ti/Kconfig"
>
> +source "drivers/dma/avalon/Kconfig"
Sort this alphabetically please
> +
> # clients
> comment "DMA Clients"
> depends on DMA_ENGINE
> diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
> index f5ce8665e944..fd7e11417b73 100644
> --- a/drivers/dma/Makefile
> +++ b/drivers/dma/Makefile
> @@ -75,6 +75,7 @@ obj-$(CONFIG_UNIPHIER_MDMAC) += uniphier-mdmac.o
> obj-$(CONFIG_XGENE_DMA) += xgene-dma.o
> obj-$(CONFIG_ZX_DMA) += zx_dma.o
> obj-$(CONFIG_ST_FDMA) += st_fdma.o
> +obj-$(CONFIG_AVALON_DMA) += avalon/
This one as well
> +config AVALON_DMA
> + tristate "Intel Avalon-MM DMA Interface for PCIe"
> + depends on PCI
> + select DMA_ENGINE
> + select DMA_VIRTUAL_CHANNELS
> + help
> + This selects a driver for Avalon-MM DMA Interface for PCIe
> + hard IP block used in Intel Arria, Cyclone or Stratix FPGAs.
If it is just a single kconfig block, why not move it into dmaengine
Kconfig?
> +static unsigned int dma_mask_width = 64;
> +module_param(dma_mask_width, uint, 0644);
> +MODULE_PARM_DESC(dma_mask_width, "Avalon DMA bitmask width (default: 64)");
> +
> +unsigned long ctrl_base;
> +module_param(ctrl_base, ulong, 0644);
> +MODULE_PARM_DESC(ctrl_base, "Avalon DMA controller base (default: 0)");
> +
> +static unsigned int rd_ep_dst_lo = 0x80000000;
> +module_param(rd_ep_dst_lo, uint, 0644);
> +MODULE_PARM_DESC(rd_ep_dst_lo,
> + "Read status and desc table low (default: 0x80000000)");
> +
> +static unsigned int rd_ep_dst_hi = 0;
> +module_param(rd_ep_dst_hi, uint, 0644);
> +MODULE_PARM_DESC(rd_ep_dst_hi,
> + "Read status and desc table hi (default: 0)");
> +
> +static unsigned int wr_ep_dst_lo = 0x80002000;
> +module_param(wr_ep_dst_lo, uint, 0644);
> +MODULE_PARM_DESC(wr_ep_dst_lo,
> + "Write status and desc table low (default: 0x80002000)");
> +
> +static unsigned int wr_ep_dst_hi = 0;
> +module_param(wr_ep_dst_hi, uint, 0644);
> +MODULE_PARM_DESC(wr_ep_dst_hi,
> + "Write status and desc table hi (default: 0)");
these are resources, do you not have any other way, DT/ACPI/something
else to find these!
> +static void avalon_dma_term(struct avalon_dma *adma)
> +{
> + struct avalon_dma_chan *chan = &adma->chan;
> + struct avalon_dma_hw *hw = &chan->hw;
> + struct device *dev = adma->dev;
> +
> + free_irq(adma->irq, adma);
please also kill the vchan tasklet
> +static int avalon_dma_device_config(struct dma_chan *dma_chan,
> + struct dma_slave_config *config)
> +{
> + struct avalon_dma_chan *chan = to_avalon_dma_chan(dma_chan);
> +
> + if (!IS_ALIGNED(config->src_addr, sizeof(u32)) ||
> + !IS_ALIGNED(config->dst_addr, sizeof(u32)))
> + return -EINVAL;
> +
> + chan->src_addr = config->src_addr;
> + chan->dst_addr = config->dst_addr;
hmmm you dont care about widths and burst sizes?
> +static struct dma_async_tx_descriptor *
> +avalon_dma_prep_slave_sg(struct dma_chan *dma_chan,
> + struct scatterlist *sg, unsigned int sg_len,
> + enum dma_transfer_direction direction,
> + unsigned long flags, void *context)
> +{
> + struct avalon_dma_chan *chan = to_avalon_dma_chan(dma_chan);
> + struct avalon_dma_desc *desc;
> + dma_addr_t dev_addr;
> + int i;
> +
> + if (direction == DMA_MEM_TO_DEV)
> + dev_addr = chan->dst_addr;
> + else if (direction == DMA_DEV_TO_MEM)
> + dev_addr = chan->src_addr;
the dst_addr/src_addr is initialized to -1 so dont you want to check you
have a valid address?
> + else
> + return NULL;
> +
> + desc = kzalloc(struct_size(desc, seg, sg_len), GFP_NOWAIT);
> + if (!desc)
> + return NULL;
> +
> + desc->direction = direction;
> + desc->dev_addr = dev_addr;
> + desc->seg_curr = 0;
> + desc->seg_off = 0;
> + desc->nr_segs = sg_len;
> +
> + for (i = 0; i < sg_len; i++) {
> + struct dma_segment *seg = &desc->seg[i];
> + dma_addr_t dma_addr = sg_dma_address(sg);
> + unsigned int dma_len = sg_dma_len(sg);
> +
> + if (!IS_ALIGNED(dma_addr, sizeof(u32)) ||
> + !IS_ALIGNED(dma_len, sizeof(u32)))
you are leaking desc here
> +struct avalon_dma *avalon_dma_register(struct device *dev,
> + void __iomem *regs,
> + unsigned int irq)
> +{
> + struct avalon_dma *adma;
> + struct avalon_dma_chan *chan;
> + struct dma_device *dma_dev;
> + int ret;
> +
> + adma = kzalloc(sizeof(*adma), GFP_KERNEL);
> + if (!adma)
> + return ERR_PTR(-ENOMEM);
any reason for not using device managed API for this?
> +static unsigned int pci_bar = 0;
> +module_param(pci_bar, uint, 0644);
> +MODULE_PARM_DESC(pci_bar,
> + "PCI BAR number the controller is mapped to (default: 0)");
> +
> +static unsigned int pci_msi_vector = 0;
> +module_param(pci_msi_vector, uint, 0644);
> +MODULE_PARM_DESC(pci_msi_vector,
> + "MSI vector number used for the controller (default: 0)");
> +
> +static unsigned int pci_msi_count_order = 5;
> +module_param(pci_msi_count_order, uint, 0644);
> +MODULE_PARM_DESC(pci_msi_count_order,
> + "Number of MSI vectors (order) device uses (default: 5)");
and still not convinced these should be module params
--
~Vinod
