LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Rameshwar Sahu <rsahu@apm.com>
To: Vinod Koul <vinod.koul@intel.com>
Cc: dan.j.williams@intel.com, dmaengine@vger.kernel.org,
	Arnd Bergmann <arnd@arndb.de>,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, ddutile@redhat.com,
	jcm@redhat.com, patches@apm.com, Loc Ho <lho@apm.com>
Subject: Re: [PATCH v7 1/3] dmaengine: Add support for APM X-Gene SoC DMA engine driver
Date: Mon, 16 Mar 2015 16:00:22 +0530	[thread overview]
Message-ID: <CAFd313y3avndJ_jDbOSWsbBOYapto_HpDUa+eR7Qy2FO2qLDYg@mail.gmail.com> (raw)
In-Reply-To: <20150316092756.GH32683@intel.com>

Hi Vinod,


On Mon, Mar 16, 2015 at 2:57 PM, Vinod Koul <vinod.koul@intel.com> wrote:
> On Thu, Mar 12, 2015 at 04:45:19PM +0530, Rameshwar Prasad Sahu wrote:
>> +/* DMA ring csr registers and bit definations */
>> +#define DMA_RING_CONFIG                      0x04
>> +#define DMA_RING_ENABLE                      BIT(31)
>> +#define DMA_RING_ID                  0x08
>> +#define DMA_RING_ID_SETUP(v)         ((v) | BIT(31))
>> +#define DMA_RING_ID_BUF                      0x0C
>> +#define DMA_RING_ID_BUF_SETUP(v)     (((v) << 9) | BIT(21))
>> +#define DMA_RING_THRESLD0_SET1               0x30
>> +#define DMA_RING_THRESLD0_SET1_VAL   0X64
>> +#define DMA_RING_THRESLD1_SET1               0x34
>> +#define DMA_RING_THRESLD1_SET1_VAL   0xC8
>> +#define DMA_RING_HYSTERESIS          0x68
>> +#define DMA_RING_HYSTERESIS_VAL              0xFFFFFFFF
>> +#define DMA_RING_STATE                       0x6C
>> +#define DMA_RING_STATE_WR_BASE               0x70
>> +#define DMA_RING_NE_INT_MODE         0x017C
>> +#define DMA_RING_NE_INT_MODE_SET(m, v)               \
>> +     ((m) = ((m) & ~BIT(31 - (v))) | BIT(31 - (v)))
>> +#define DMA_RING_NE_INT_MODE_RESET(m, v)     \
>> +     ((m) &= (~BIT(31 - (v))))
>> +#define DMA_RING_CLKEN                       0xC208
>> +#define DMA_RING_SRST                        0xC200
>> +#define DMA_RING_MEM_RAM_SHUTDOWN    0xD070
>> +#define DMA_RING_BLK_MEM_RDY         0xD074
>> +#define DMA_RING_BLK_MEM_RDY_VAL     0xFFFFFFFF
>> +#define DMA_RING_DESC_CNT(v)         (((v) & 0x0001FFFE) >> 1)
>> +#define DMA_RING_ID_GET(owner, num)  (((owner) << 6) | (num))
>> +#define DMA_RING_DST_ID(v)           ((1 << 10) | (v))
>> +#define DMA_RING_CMD_OFFSET          0x2C
>> +#define DMA_RING_CMD_BASE_OFFSET(v)  ((v) << 6)
>> +#define DMA_RING_COHERENT_SET(m)     (((u32 *)(m))[2] |= BIT(4))
>> +#define DMA_RING_ADDRL_SET(m, v)     (((u32 *)(m))[2] |= (((v) >> 8) << 5))
>> +#define DMA_RING_ADDRH_SET(m, v)     (((u32 *)(m))[3] |= ((v) >> 35))
>> +#define DMA_RING_ACCEPTLERR_SET(m)   (((u32 *)(m))[3] |= BIT(19))
>> +#define DMA_RING_SIZE_SET(m, v)              (((u32 *)(m))[3] |= ((v) << 23))
>> +#define DMA_RING_RECOMBBUF_SET(m)    (((u32 *)(m))[3] |= BIT(27))
>> +#define DMA_RING_RECOMTIMEOUTL_SET(m)        (((u32 *)(m))[3] |= (0x7 << 28))
>> +#define DMA_RING_RECOMTIMEOUTH_SET(m)        (((u32 *)(m))[4] |= 0x3)
>> +#define DMA_RING_SELTHRSH_SET(m)     (((u32 *)(m))[4] |= BIT(3))
>> +#define DMA_RING_TYPE_SET(m, v)              (((u32 *)(m))[4] |= ((v) << 19))
> These are very generic name as can conflicts, can you please namespace
> these...

Okay
>
>> +/* DMA device csr registers and bit definitions */
>> +#define DMA_IPBRR                    0x0
>> +#define DMA_DEV_ID_RD(v)             ((v) & 0x00000FFF)
>> +#define DMA_BUS_ID_RD(v)             (((v) >> 12) & 3)
>> +#define DMA_REV_NO_RD(v)             (((v) >> 14) & 3)
>> +#define DMA_GCR                              0x10
>> +#define DMA_CH_SETUP(v)                      ((v) = ((v) & ~0x000FFFFF) | 0x000AAFFF)
>> +#define DMA_ENABLE(v)                        ((v) |= BIT(31))
>> +#define DMA_DISABLE(v)                       ((v) &= ~BIT(31))
>> +#define DMA_RAID6_CONT                       0x14
>> +#define DMA_RAID6_MULTI_CTRL(v)              ((v) << 24)
>> +#define DMA_INT                              0x70
>> +#define DMA_INT_MASK                 0x74
>> +#define DMA_INT_ALL_MASK             0xFFFFFFFF
>> +#define DMA_INT_ALL_UNMASK           0x0
>> +#define DMA_INT_MASK_SHIFT           0x14
>> +#define DMA_RING_INT0_MASK           0x90A0
>> +#define DMA_RING_INT1_MASK           0x90A8
>> +#define DMA_RING_INT2_MASK           0x90B0
>> +#define DMA_RING_INT3_MASK           0x90B8
>> +#define DMA_RING_INT4_MASK           0x90C0
>> +#define DMA_CFG_RING_WQ_ASSOC                0x90E0
>> +#define DMA_ASSOC_RING_MNGR1         0xFFFFFFFF
>> +#define DMA_MEM_RAM_SHUTDOWN         0xD070
>> +#define DMA_BLK_MEM_RDY                      0xD074
>> +#define DMA_BLK_MEM_RDY_VAL          0xFFFFFFFF
> same here and throughout the driver..
>

Okay...


>> +static void xgene_dma_free_descriptor(struct xgene_dma_chan *chan,
>> +                                   struct xgene_dma_desc_sw *desc)
>> +{
>> +     list_del(&desc->node);
>> +     chan_dbg(chan, "LD %p free\n", desc);
>> +     dma_pool_free(chan->desc_pool, desc, desc->tx.phys);
> where is the descriptor freed? Perhpas we can say clean rather than free
> here to not confuse...

xgene_dma_clean_completed_descriptor() is freeing the descriptor once
descriptor acked by client.
This is in the completion path.

>
>> +}
>> +
>> +static struct xgene_dma_desc_sw *xgene_dma_alloc_descriptor(
>> +                              struct xgene_dma_chan *chan)
>> +{
>> +     struct xgene_dma_desc_sw *desc;
>> +     dma_addr_t phys;
>> +
>> +     desc = dma_pool_alloc(chan->desc_pool, GFP_NOWAIT, &phys);
>> +     if (!desc) {
>> +             chan_dbg(chan, "Failed to allocate LDs\n");
> not error?

Yes it's error only by lacking of dma memory, do I need to use dev_err
to show the error msg ??

>
>> +static void xgene_dma_free_desc_list_reverse(struct xgene_dma_chan *chan,
>> +                                          struct list_head *list)
> do we really care about free order?

Yes it start dellocation of descriptor by tail.

>
>> +{
>> +     struct xgene_dma_desc_sw *desc, *_desc;
>> +
>> +     list_for_each_entry_safe_reverse(desc, _desc, list, node)
>> +             xgene_dma_free_descriptor(chan, desc);
>> +}
>> +
>> +static void xgene_dma_free_chan_resources(struct dma_chan *dchan)
>> +{
>> +     struct xgene_dma_chan *chan = to_dma_chan(dchan);
>> +
>> +     chan_dbg(chan, "Free all resources\n");
>> +
>> +     if (!chan->desc_pool)
>> +             return;
>> +
>> +     spin_lock_bh(&chan->lock);
>> +
>> +     /* Process all running descriptor */
>> +     xgene_dma_cleanup_descriptors(chan);
>> +
>> +     /* Clean all link descriptor queues */
>> +     xgene_dma_free_desc_list(chan, &chan->ld_pending);
>> +     xgene_dma_free_desc_list(chan, &chan->ld_running);
>> +     xgene_dma_free_desc_list(chan, &chan->ld_completed);
>> +
>> +     spin_unlock_bh(&chan->lock);
>> +
>> +     /* Delete this channel DMA pool */
>> +     dma_pool_destroy(chan->desc_pool);
>> +     chan->desc_pool = NULL;
>> +}
>> +
>> +static struct dma_async_tx_descriptor *xgene_dma_prep_memcpy(
>> +     struct dma_chan *dchan, dma_addr_t dst, dma_addr_t src,
>> +     size_t len, unsigned long flags)
>> +{
>> +     struct xgene_dma_desc_sw *first = NULL, *new;
>> +     struct xgene_dma_chan *chan;
>> +     size_t copy;
>> +     u32 desc_id;
>> +
>> +     if (unlikely(!dchan || !len))
>> +             return NULL;
>> +
>> +     chan = to_dma_chan(dchan);
>> +
>> +     /* Get the id for this group of descriptors */
>> +     desc_id = atomic_inc_return(&chan->desc_id);
>> +
>> +     do {
>> +             /* Allocate the link descriptor from DMA pool */
>> +             new = xgene_dma_alloc_descriptor(chan);
>> +             if (!new)
>> +                     goto fail;
>> +
>> +             /* Create the largest transaction possible */
>> +             copy = min_t(size_t, len, DMA_MAX_64B_DESC_BYTE_CNT);
>> +
>> +             /* Prepare DMA descriptor */
>> +             xgene_dma_prep_cpy_desc(chan, new, dst, src, copy, desc_id);
>> +
>> +             if (!first)
>> +                     first = new;
>> +
>> +             new->first = first;
>> +             first->desc_cnt++;
>> +             new->desc_id = desc_id;
>> +
>> +             new->tx.cookie = 0;
>> +             async_tx_ack(&new->tx);
>> +
>> +             /* Update metadata */
>> +             len -= copy;
>> +             dst += copy;
>> +             src += copy;
>> +
>> +             /* Insert the link descriptor to the LD ring */
>> +             list_add_tail(&new->node, &first->tx_list);
>> +     } while (len);
>> +
>> +     first->tx.flags = flags; /* client is in control of this ack */
>> +     first->tx.cookie = -EBUSY;
>> +     first->flags |= DMA_FLAG_FIRST_DESC;
>> +
>> +     return &first->tx;
> where are you mapping dma buffers?

 I didn't get you here. Can you please explain me here what you mean.
As per my understanding client should map the dma buffer and give the
physical address and size to this callback prep routines.

>
>> +static enum dma_status xgene_dma_find_tx_desc_status(
>> +     struct xgene_dma_chan *chan, dma_cookie_t cookie,
>> +     struct dma_tx_state *txstate)
>> +{
>> +     struct xgene_dma_desc_sw *desc;
>> +
>> +     spin_lock_bh(&chan->lock);
>> +
>> +     /* Check if this tx descriptor is still in pending queue */
>> +     list_for_each_entry(desc, &chan->ld_pending, node) {
>> +             if (desc->tx.cookie == cookie) {
>> +                     xgene_chan_xfer_ld_pending(chan);
> why are you calling this here, status check shouldnt do this...

Okay, I will remove it.


>> +                     spin_unlock_bh(&chan->lock);
>> +                     return DMA_IN_PROGRESS;
> residue here is size of transacation.

We can't calculate here residue size. We don't have any controller
register which will tell about remaining transaction size.

>> +             }
>> +     }
>> +
>> +     /* Check if this descriptor is in running queue */
>> +     list_for_each_entry(desc, &chan->ld_running, node) {
>> +             if (desc->tx.cookie == cookie) {
>> +                     /* Cleanup any running and executed descriptors */
>> +                     xgene_dma_cleanup_descriptors(chan);
> ditto?

Okay


>> +                     spin_unlock_bh(&chan->lock);
>> +                     return dma_cookie_status(&chan->dma_chan,
>> +                                              cookie, txstate);
> and you havent touched txstate so what is the intent here...?

txstate can filled by caller, so it may be NULL or not NULL, we are
passing same.



>> +             }
>> +     }
>> +
>> +     spin_unlock_bh(&chan->lock);
>> +
>> +     return DMA_COMPLETE;
>> +}
>> +
>> +static enum dma_status xgene_dma_tx_status(struct dma_chan *dchan,
>> +                                        dma_cookie_t cookie,
>> +                                        struct dma_tx_state *txstate)
>> +{
>> +     struct xgene_dma_chan *chan = to_dma_chan(dchan);
>> +
>> +     enum dma_status status;
>> +
>> +     status = dma_cookie_status(dchan, cookie, txstate);
>> +     if (status == DMA_COMPLETE)
> you should do this if txstate in NULL, no point doing calculations..
>
>> +             return status;
>> +
>> +     return xgene_dma_find_tx_desc_status(chan, cookie, txstate);
>> +}
>> +
>> +static void xgene_dma_tasklet_cb(unsigned long data)
>> +{
>> +     struct xgene_dma_chan *chan = (struct xgene_dma_chan *)data;
>> +
>> +     spin_lock_bh(&chan->lock);
>> +
>> +     /* Run all cleanup for descriptors which have been completed */
>> +     xgene_dma_cleanup_descriptors(chan);
>> +
>> +     /* Re-enable DMA channel IRQ */
>> +     enable_irq(chan->rx_irq);
>> +
>> +     spin_unlock_bh(&chan->lock);
>> +}
>> +
>> +static irqreturn_t xgene_dma_chan_ring_isr(int irq, void *id)
>> +{
>> +     struct xgene_dma_chan *chan = (struct xgene_dma_chan *)id;
>> +
>> +     BUG_ON(!chan);
>> +
>> +     /*
>> +      * Disable DMA channel IRQ until we process completed
>> +      * descriptors
>> +      */
>> +     disable_irq_nosync(chan->rx_irq);
> and why is that?

This will reduce the interrupt overheads and will give us better performance.
we are enabling it again when we are done with tasklet function.

>
>> +
>> +     /*
>> +      * Schedule the tasklet to handle all cleanup of the current
>> +      * transaction. It will start a new transaction if there is
>> +      * one pending.
>> +      */
>> +     tasklet_schedule(&chan->tasklet);
> for better results why not schedule the next transaction here..?

I agree, I will do this.

>> +
>> +     return IRQ_HANDLED;
>> +}
>> +
>
>> +static void xgene_dma_async_unregister(struct xgene_dma *pdma)
>> +{
>> +     int i;
>> +
>> +     for (i = 0; i < DMA_MAX_CHANNEL; i++)
>> +             dma_async_device_unregister(&pdma->dma_dev[i]);
> how do you ensure irq is disabled and tasklets killed?

After un-registering async dma device we are freeing irqs and killing tasklet.
Does this make sense. and do we need to add some way to make sure
about irq disabled.

>
>> +MODULE_DESCRIPTION("APM X-Gene SoC DMA driver");
>> +MODULE_AUTHOR("Rameshwar Prasad Sahu <rsahu@apm.com>");
>> +MODULE_AUTHOR("Loc Ho <lho@apm.com>");
>> +MODULE_LICENSE("GPL");
>> +MODULE_VERSION("1.0");
> why do you need this?

I don't see really use of this, but this will give us idea which
version of dma driver support what offload functionality for later.

>> --
>> 1.8.2.1
>>
>
> --

  reply	other threads:[~2015-03-16 10:30 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-03-12 11:15 [PATCH v7 0/3] dmaengine: APM X-Gene SoC DMA engine driver support Rameshwar Prasad Sahu
2015-03-12 11:15 ` [PATCH v7 1/3] dmaengine: Add support for APM X-Gene SoC DMA engine driver Rameshwar Prasad Sahu
2015-03-16  9:27   ` Vinod Koul
2015-03-16 10:30     ` Rameshwar Sahu [this message]
2015-03-16 11:27       ` Vinod Koul
2015-03-16 11:54         ` Rameshwar Sahu
2015-03-16 16:26           ` Vinod Koul
2015-03-16 17:31             ` Rameshwar Sahu
2015-03-17  9:33               ` Rameshwar Sahu
2015-03-17 10:19                 ` Vinod Koul
2015-03-17 10:38                   ` Rameshwar Sahu
2015-03-16 11:02     ` Rameshwar Sahu
2015-03-12 11:15 ` [PATCH v7 2/3] arm64: dts: Add APM X-Gene DMA device and DMA clock DTS nodes Rameshwar Prasad Sahu
2015-03-12 11:15 ` [PATCH v7 3/3] Documentation: dma: Add APM X-Gene SoC DMA engine driver documentation Rameshwar Prasad Sahu
2015-03-16  9:29   ` Vinod Koul
2015-03-16 10:31     ` Rameshwar Sahu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAFd313y3avndJ_jDbOSWsbBOYapto_HpDUa+eR7Qy2FO2qLDYg@mail.gmail.com \
    --to=rsahu@apm.com \
    --cc=arnd@arndb.de \
    --cc=dan.j.williams@intel.com \
    --cc=ddutile@redhat.com \
    --cc=devicetree@vger.kernel.org \
    --cc=dmaengine@vger.kernel.org \
    --cc=jcm@redhat.com \
    --cc=lho@apm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=patches@apm.com \
    --cc=vinod.koul@intel.com \
    --subject='Re: [PATCH v7 1/3] dmaengine: Add support for APM X-Gene SoC DMA engine driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).