Hi,

On 01/08/2015 08:56 AM, Andy Gross wrote:
Signed-off-by: Andy Gross <agr...@codeaurora.org>
<snip>

+static struct dma_async_tx_descriptor *adm_prep_slave_sg(struct dma_chan *chan,
+       struct scatterlist *sgl, unsigned int sg_len,
+       enum dma_transfer_direction direction, unsigned long flags,
+       void *context)
+{
+       struct adm_chan *achan = to_adm_chan(chan);
+       struct adm_device *adev = achan->adev;
+       struct adm_async_desc *async_desc;
+       struct scatterlist *sg;
+       u32 i;
+       u32 single_count = 0, box_count = 0, desc_offset = 0, crci = 0;
+       struct adm_desc_hw_box *box_desc;
+       struct adm_desc_hw_single *single_desc;
+       void *desc;
+       u32 *cple, *last_cmd;
+       u32 burst;
+       int blk_size = -EINVAL;

blk_size should be set to 0 here. Otherwise async_desc->blk_size takes -EINVAL when no flow control.

+
+
+       if (!is_slave_direction(direction)) {
+               dev_err(adev->dev, "invalid dma direction\n");
+               return NULL;
+       }
+
+       /*
+        * get burst value from slave configuration
+        * If zero, default to maximum burst size
+        * If larger than the max transfer size, set to ADM_MAX_XFER
+        */
+       burst = (direction == DMA_MEM_TO_DEV) ?
+               achan->slave.dst_maxburst :
+               achan->slave.src_maxburst;
+
+       if (!burst || burst > ADM_MAX_XFER)
+               burst = ADM_MAX_XFER;
+
+       /* if using flow control, validate burst and crci values */
+       if (achan->slave.device_fc) {
+
+               blk_size = adm_get_blksize(burst);
+               if (blk_size < 0) {
+                       dev_err(adev->dev, "invalid burst value w/ crci: %d\n",
+                               burst);
+                       return ERR_PTR(-EINVAL);
+               }
+
+               crci = achan->slave.slave_id;
+               if (!(crci & 0xf)) {

This check isn't sufficient for the crci to lie between 1 and 15. slave_id passed as, say, 0xf5, will be valid.

<snip>


+/**
+ * adm_dma_irq - irq handler for ADM controller
+ * @irq: IRQ of interrupt
+ * @data: callback data
+ *
+ * IRQ handler for the bam controller
+ */
+static irqreturn_t adm_dma_irq(int irq, void *data)
+{
+       struct adm_device *adev = data;
+       u32 srcs, i;
+       struct adm_async_desc *async_desc;
+       unsigned long flags;
+
+       srcs = readl_relaxed(adev->regs +
+                       HI_SEC_DOMAIN_IRQ_STATUS(adev->ee));
+
+       for (i = 0; i < 16; i++) {

we could use adev->num_channels here instead of 16.

+               struct adm_chan *achan = &adev->channels[i];
+               u32 status, result;
+
+               if (srcs & BIT(i)) {
+                       status = readl_relaxed(adev->regs +
+                               HI_CH_STATUS_SD(i, adev->ee));
+
+                       /* if no result present, skip */
+                       if (!(status & CH_STATUS_VALID))
+                               continue;
+
+                       result = readl_relaxed(adev->regs +
+                               HI_CH_RSLT(i, adev->ee));
+
+                       /* no valid results, skip */
+                       if (!(result & CH_RSLT_VALID))
+                               continue;
+
+                       /* flag error if transaction was flushed or failed */
+                       if (result & (CH_RSLT_ERR | CH_RSLT_FLUSH))
+                               achan->error = 1;
+
+                       spin_lock_irqsave(&achan->vc.lock, flags);
+                       async_desc = achan->curr_txd;
+
+                       achan->curr_txd = NULL;
+
+                       if (async_desc) {
+                               vchan_cookie_complete(&async_desc->vd);
+
+                               /* kick off next DMA */
+                               adm_start_dma(achan);
+                       }
+
+                       spin_unlock_irqrestore(&achan->vc.lock, flags);
+               }
+       }
+
+       return IRQ_HANDLED;
+}

<snip>

+
+static int adm_dma_probe(struct platform_device *pdev)
+{
+       struct adm_device *adev;
+       struct resource *iores;
+       int ret;
+       u32 i;
+
+       adev = devm_kzalloc(&pdev->dev, sizeof(*adev), GFP_KERNEL);
+       if (!adev)
+               return -ENOMEM;
+
+       adev->dev = &pdev->dev;
+
+       iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+       adev->regs = devm_ioremap_resource(&pdev->dev, iores);
+       if (IS_ERR(adev->regs))
+               return PTR_ERR(adev->regs);
+
+       adev->irq = platform_get_irq(pdev, 0);
+       if (adev->irq < 0)
+               return adev->irq;
+
+       ret = of_property_read_u32(pdev->dev.of_node, "qcom,ee", &adev->ee);
+       if (ret) {
+               dev_err(adev->dev, "Execution environment unspecified\n");
+               return ret;
+       }
+
+       adev->core_clk = devm_clk_get(adev->dev, "core");
+       if (IS_ERR(adev->core_clk))
+               return PTR_ERR(adev->core_clk);
+
+       ret = clk_prepare_enable(adev->core_clk);
+       if (ret) {
+               dev_err(adev->dev, "failed to prepare/enable core clock\n");
+               return ret;
+       }
+
+       adev->iface_clk = devm_clk_get(adev->dev, "iface");
+       if (IS_ERR(adev->iface_clk))
+               return PTR_ERR(adev->iface_clk);
+

An error here or below would leave core_clk on.

+       ret = clk_prepare_enable(adev->iface_clk);
+       if (ret) {
+               dev_err(adev->dev, "failed to prepare/enable iface clock\n");
+               return ret;
+       }
+
+       adev->clk_reset = devm_reset_control_get(&pdev->dev, "clk");
+       if (IS_ERR(adev->clk_reset)) {
+               dev_err(adev->dev, "failed to get ADM0 reset\n");
+               return PTR_ERR(adev->clk_reset);
+       }
+
+       adev->c0_reset = devm_reset_control_get(&pdev->dev, "c0");
+       if (IS_ERR(adev->c0_reset)) {
+               dev_err(adev->dev, "failed to get ADM0 C0 reset\n");
+               return PTR_ERR(adev->c0_reset);
+       }
+
+       adev->c1_reset = devm_reset_control_get(&pdev->dev, "c1");
+       if (IS_ERR(adev->c1_reset)) {
+               dev_err(adev->dev, "failed to get ADM0 C1 reset\n");
+               return PTR_ERR(adev->c1_reset);
+       }
+
+       adev->c2_reset = devm_reset_control_get(&pdev->dev, "c2");
+       if (IS_ERR(adev->c2_reset)) {
+               dev_err(adev->dev, "failed to get ADM0 C2 reset\n");
+               return PTR_ERR(adev->c2_reset);
+       }

All the devm_reset_control_get() failures should disable the clocks too.

Looks good otherwise.

Thanks,
Archit
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to