RE: [EXT] Re: [V4 2/2] dmaengine: fsl-dpaa2-qdma: Add NXP dpaa2 qDMA controller driver for Layerscape SoCs

Peng Ma
2019-09-11T02:02:00Z

Hi Vinod,

I send those series patchs(V5) on June 25, 2019. I haven't received any comments yet. Their current state
is "Not Applicable", so please let me know what I need to do next.
Thanks very much for your comments.

Patch link:
https://patchwork.kernel.org/patch/11015035/
https://patchwork.kernel.org/patch/11015033/

Best Regards,
Peng
>-----Original Message-----
>From: Vinod Koul <vkoul@kernel.org>
>Sent: 2019年6月25日 0:46
>To: Peng Ma <peng.ma@nxp.com>
>Cc: dan.j.williams@intel.com; Leo Li <leoyang.li@nxp.com>;
>linux-kernel@vger.kernel.org; dmaengine@vger.kernel.org
>Subject: [EXT] Re: [V4 2/2] dmaengine: fsl-dpaa2-qdma: Add NXP dpaa2 qDMA
>controller driver for Layerscape SoCs
>
>Caution: EXT Email
>
>On 13-06-19, 10:13, Peng Ma wrote:
>> DPPA2(Data Path Acceleration Architecture 2) qDMA supports channel
>> virtualization by allowing DMA
>
>typo virtualization
>
>> jobs to be enqueued into different frame queues.
>> Core can initiate a DMA transaction by preparing a frame
>> descriptor(FD) for each DMA job and enqueuing this job to a frame
>> queue. through a hardware portal. The qDMA
> ^^^
>why this full stop?
>
>> +static struct dpaa2_qdma_comp *
>> +dpaa2_qdma_request_desc(struct dpaa2_qdma_chan *dpaa2_chan) {
>> + struct dpaa2_qdma_comp *comp_temp = NULL;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&dpaa2_chan->queue_lock, flags);
>> + if (list_empty(&dpaa2_chan->comp_free)) {
>> + spin_unlock_irqrestore(&dpaa2_chan->queue_lock, flags);
>> + comp_temp = kzalloc(sizeof(*comp_temp), GFP_NOWAIT);
>> + if (!comp_temp)
>> + goto err;
>> + comp_temp->fd_virt_addr =
>> + dma_pool_alloc(dpaa2_chan->fd_pool,
>GFP_NOWAIT,
>> + &comp_temp->fd_bus_addr);
>> + if (!comp_temp->fd_virt_addr)
>> + goto err_comp;
>> +
>> + comp_temp->fl_virt_addr =
>> + dma_pool_alloc(dpaa2_chan->fl_pool,
>GFP_NOWAIT,
>> + &comp_temp->fl_bus_addr);
>> + if (!comp_temp->fl_virt_addr)
>> + goto err_fd_virt;
>> +
>> + comp_temp->desc_virt_addr =
>> + dma_pool_alloc(dpaa2_chan->sdd_pool,
>GFP_NOWAIT,
>> +
>&comp_temp->desc_bus_addr);
>> + if (!comp_temp->desc_virt_addr)
>> + goto err_fl_virt;
>> +
>> + comp_temp->qchan = dpaa2_chan;
>> + return comp_temp;
>> + }
>> +
>> + comp_temp = list_first_entry(&dpaa2_chan->comp_free,
>> + struct dpaa2_qdma_comp, list);
>> + list_del(&comp_temp->list);
>> + spin_unlock_irqrestore(&dpaa2_chan->queue_lock, flags);
>> +
>> + comp_temp->qchan = dpaa2_chan;
>> +
>> + return comp_temp;
>> +
>> +err_fl_virt:
>
>no err logs? how will you know what went wrong?
>
>> +static enum
>> +dma_status dpaa2_qdma_tx_status(struct dma_chan *chan,
>> + dma_cookie_t cookie,
>> + struct dma_tx_state *txstate) {
>> + return dma_cookie_status(chan, cookie, txstate);
>
>why not set dma_cookie_status as this callback?
>
>> +static int __cold dpaa2_qdma_setup(struct fsl_mc_device *ls_dev) {
>> + struct dpaa2_qdma_priv_per_prio *ppriv;
>> + struct device *dev = &ls_dev->dev;
>> + struct dpaa2_qdma_priv *priv;
>> + u8 prio_def = DPDMAI_PRIO_NUM;
>> + int err = -EINVAL;
>> + int i;
>> +
>> + priv = dev_get_drvdata(dev);
>> +
>> + priv->dev = dev;
>> + priv->dpqdma_id = ls_dev->obj_desc.id;
>> +
>> + /* Get the handle for the DPDMAI this interface is associate with */
>> + err = dpdmai_open(priv->mc_io, 0, priv->dpqdma_id,
>&ls_dev->mc_handle);
>> + if (err) {
>> + dev_err(dev, "dpdmai_open() failed\n");
>> + return err;
>> + }
>> + dev_info(dev, "Opened dpdmai object successfully\n");
>
>this is noise in kernel, consider debug level
>
>> +static int __cold dpaa2_dpdmai_bind(struct dpaa2_qdma_priv *priv) {
>> + int err;
>> + int i, num;
>> + struct device *dev = priv->dev;
>> + struct dpaa2_qdma_priv_per_prio *ppriv;
>> + struct dpdmai_rx_queue_cfg rx_queue_cfg;
>> + struct fsl_mc_device *ls_dev = to_fsl_mc_device(dev);
>
>the order is reverse than used in other fn, please stick to one style!
>--
>~Vinod