Re: [PATCH v3 1/8] dma: sun4i: Add support for the DMA engine on sun[457]i SoCs

[Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3838 bytes --]

Hi,

Overall it looks very nice, thanks.

On Mon, Aug 04, 2014 at 05:09:55PM -0300, Emilio López wrote:
> +static struct sun4i_dma_pchan *find_and_use_pchan(struct sun4i_dma_dev *priv,
> +                                                 struct sun4i_dma_vchan *vchan)
> +{
> +       struct sun4i_dma_pchan *pchan = NULL, *pchans = priv->pchans;
> +       unsigned long flags;
> +       int i, max;
> +
> +       /*
> +        * pchans 0-NDMA_NR_MAX_CHANNELS are normal, and
> +        * NDMA_NR_MAX_CHANNELS+ are dedicated ones
> +        */
> +       if (vchan->is_dedicated) {
> +               i = NDMA_NR_MAX_CHANNELS;
> +               max = DMA_NR_MAX_CHANNELS;
> +       } else {
> +               i = 0;
> +               max = NDMA_NR_MAX_CHANNELS;
> +       }
> +
> +       spin_lock_irqsave(&priv->lock, flags);
> +       for_each_clear_bit_from(i, &priv->pchans_used, max) {
> +               pchan = &pchans[i];
> +               pchan->vchan = vchan;
> +               set_bit(i, priv->pchans_used);
> +               break;

ffz instead?

> +static int execute_vchan_pending(struct sun4i_dma_dev *priv,
> +				 struct sun4i_dma_vchan *vchan)
> +{
> +	struct sun4i_dma_promise *promise = NULL;
> +	struct sun4i_dma_contract *contract = NULL;
> +	struct sun4i_dma_pchan *pchan;
> +	struct virt_dma_desc *vd;
> +	int ret;
> +
> +	lockdep_assert_held(&vchan->vc.lock);

So this has to be called with a lock taken?

You should mention that somewhere.
Usually, such fonctions are also prefixed by "__"

> +/**
> + * Generate a promise, to be used in a normal DMA contract.
> + *
> + * A NDMA promise contains all the information required to program the
> + * normal part of the DMA Engine and get data copied. A non-executed
> + * promise will live in the demands list on a contract. Once it has been
> + * completed, it will be moved to the completed demands list for later freeing.
> + * All linked promises will be freed when the corresponding contract is freed
> + */
> +static struct sun4i_dma_promise *
> +generate_ndma_promise(struct dma_chan *chan, dma_addr_t src, dma_addr_t dest,
> +		      size_t len, struct dma_slave_config *sconfig)
> +{
> +	struct sun4i_dma_promise *promise;
> +	int ret;
> +
> +	promise = kzalloc(sizeof(*promise), GFP_NOWAIT);
> +	if (!promise)
> +		return NULL;
> +
> +	promise->src = src;
> +	promise->dst = dest;
> +	promise->len = len;
> +	promise->cfg = NDMA_CFG_LOADING | NDMA_CFG_BYTE_COUNT_MODE_REMAIN;
> +
> +	/* Use sensible default values if client is using undefined ones */
> +	if (sconfig->src_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED)
> +		sconfig->src_addr_width = sconfig->dst_addr_width;
> +	if (sconfig->dst_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED)
> +		sconfig->dst_addr_width = sconfig->src_addr_width;
> +	if (sconfig->src_maxburst == 0)
> +		sconfig->src_maxburst = sconfig->dst_maxburst;
> +	if (sconfig->dst_maxburst == 0)
> +		sconfig->dst_maxburst = sconfig->src_maxburst;

I'm not sure what's the default policy on that. Vinod?

> +static irqreturn_t sun4i_dma_submit_work(int irq, void *dev_id)
> +{
> +	struct sun4i_dma_dev *priv = dev_id;
> +	struct sun4i_dma_vchan *vchan;
> +	unsigned long flags;
> +	int i;
> +
> +	for (i = 0; i < DMA_NR_MAX_VCHANS; i++) {
> +		vchan = &priv->vchans[i];
> +		spin_lock_irqsave(&vchan->vc.lock, flags);
> +		execute_vchan_pending(priv, vchan);
> +		spin_unlock_irqrestore(&vchan->vc.lock, flags);
> +	}
> +
> +	return IRQ_HANDLED;
> +}

Judging from Russell's comment here, that should be in the interrupt
handler

http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/277737.html

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]