On Mon, Oct 30, 2017 at 03:46:54PM +0200, Horia Geantă wrote:
> @@ -600,6 +600,23 @@ config ZX_DMA
> help
> Support the DMA engine for ZTE ZX family platform devices.
>
> +config CRYPTO_DEV_FSL_CAAM_DMA
File is sorted alphabetically
> + tristate "CAAM DMA engine support"
> + depends on CRYPTO_DEV_FSL_CAAM_JR
> + default y
why should it be default?
> --- /dev/null
> +++ b/drivers/dma/caam_dma.c
> @@ -0,0 +1,444 @@
> +/*
> + * caam support for SG DMA
> + *
> + * Copyright 2016 Freescale Semiconductor, Inc
> + * Copyright 2017 NXP
> + */
that is interesting, no license text?
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/interrupt.h>
why do you need this
> +#include <linux/slab.h>
> +#include <linux/debugfs.h>
i didn't see any debugfs code, why do you need this
alphabetical sort pls
> +
> +#include <linux/dmaengine.h>
> +#include "dmaengine.h"
> +
> +#include "../crypto/caam/regs.h"
> +#include "../crypto/caam/jr.h"
> +#include "../crypto/caam/error.h"
> +#include "../crypto/caam/intern.h"
> +#include "../crypto/caam/desc_constr.h"
ah that puts very hard assumptions on locations of the subsystems. Please do
not do that and move the required stuff into common header location in
include/
> +/* This is max chunk size of a DMA transfer. If a buffer is larger than this
> + * value it is internally broken into chunks of max CAAM_DMA_CHUNK_SIZE bytes
> + * and for each chunk a DMA transfer request is issued.
> + * This value is the largest number on 16 bits that is a multiple of 256
> bytes
> + * (the largest configurable CAAM DMA burst size).
> + */
Kernel comments style we follow is:
/*
* this is for
* multi line comment
*/
Pls fix this in the file
> +#define CAAM_DMA_CHUNK_SIZE 65280
> +
> +struct caam_dma_sh_desc {
sh?
> +struct caam_dma_ctx {
> + struct dma_chan chan;
> + struct list_head node;
> + struct device *jrdev;
> + struct list_head submit_q;
call it pending
> +static struct dma_device *dma_dev;
> +static struct caam_dma_sh_desc *dma_sh_desc;
> +static LIST_HEAD(dma_ctx_list);
why do you need so many globals?
> +static dma_cookie_t caam_dma_tx_submit(struct dma_async_tx_descriptor *tx)
> +{
> + struct caam_dma_edesc *edesc = NULL;
> + struct caam_dma_ctx *ctx = NULL;
> + dma_cookie_t cookie;
> +
> + edesc = container_of(tx, struct caam_dma_edesc, async_tx);
> + ctx = container_of(tx->chan, struct caam_dma_ctx, chan);
> +
> + spin_lock_bh(&ctx->edesc_lock);
why _bh, i didnt see any irqs or tasklets here which is actually odd, so
what is going on
> +
> + cookie = dma_cookie_assign(tx);
> + list_add_tail(&edesc->node, &ctx->submit_q);
> +
> + spin_unlock_bh(&ctx->edesc_lock);
> +
> + return cookie;
> +}
we have a virtual channel wrapper where we do the same stuff as above, so
consider reusing that
> +static void caam_dma_memcpy_init_job_desc(struct caam_dma_edesc *edesc)
> +{
> + u32 *jd = edesc->jd;
> + u32 *sh_desc = dma_sh_desc->desc;
> + dma_addr_t desc_dma = dma_sh_desc->desc_dma;
> +
> + /* init the job descriptor */
> + init_job_desc_shared(jd, desc_dma, desc_len(sh_desc), HDR_REVERSE);
> +
> + /* set SEQIN PTR */
> + append_seq_in_ptr(jd, edesc->src_dma, edesc->src_len, 0);
> +
> + /* set SEQOUT PTR */
> + append_seq_out_ptr(jd, edesc->dst_dma, edesc->dst_len, 0);
> +
> +#ifdef DEBUG
> + print_hex_dump(KERN_ERR, "caam dma desc@" __stringify(__LINE__) ": ",
> + DUMP_PREFIX_ADDRESS, 16, 4, jd, desc_bytes(jd), 1);
ah this make you compile kernel. Consider the dynamic printk helpers for
printing
> +/* This function can be called in an interrupt context */
> +static void caam_dma_issue_pending(struct dma_chan *chan)
> +{
> + struct caam_dma_ctx *ctx = container_of(chan, struct caam_dma_ctx,
> + chan);
> + struct caam_dma_edesc *edesc, *_edesc;
> +
> + spin_lock_bh(&ctx->edesc_lock);
> + list_for_each_entry_safe(edesc, _edesc, &ctx->submit_q, node) {
> + if (caam_jr_enqueue(ctx->jrdev, edesc->jd,
> + caam_dma_done, edesc) < 0)
what does the caam_jr_enqueue() do?
> +static int caam_dma_jr_chan_bind(void)
> +{
> + struct device *jrdev;
> + struct caam_dma_ctx *ctx;
> + int bonds = 0;
> + int i;
> +
> + for (i = 0; i < caam_jr_driver_probed(); i++) {
> + jrdev = caam_jridx_alloc(i);
> + if (IS_ERR(jrdev)) {
> + pr_err("job ring device %d allocation failed\n", i);
> + continue;
> + }
> +
> + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> + if (!ctx) {
> + caam_jr_free(jrdev);
> + continue;
you want to continue?
> + dma_dev->dev = dev;
> + dma_dev->residue_granularity = DMA_RESIDUE_GRANULARITY_DESCRIPTOR;
> + dma_cap_set(DMA_MEMCPY, dma_dev->cap_mask);
> + dma_cap_set(DMA_PRIVATE, dma_dev->cap_mask);
> + dma_dev->device_tx_status = dma_cookie_status;
> + dma_dev->device_issue_pending = caam_dma_issue_pending;
> + dma_dev->device_prep_dma_memcpy = caam_dma_prep_memcpy;
> + dma_dev->device_free_chan_resources = caam_dma_free_chan_resources;
> +
> + err = dma_async_device_register(dma_dev);
> + if (err) {
> + dev_err(dev, "Failed to register CAAM DMA engine\n");
> + goto jr_bind_err;
> + }
> +
> + dev_info(dev, "caam dma support with %d job rings\n", bonds);
that is very noisy
> +MODULE_LICENSE("Dual BSD/GPL");
> +MODULE_DESCRIPTION("NXP CAAM support for SG DMA");
> +MODULE_AUTHOR("NXP Semiconductors");
No MODULE_ALIAS, how did you load the driver
--
~Vinod
