[PATCH v2 2/2] dmaengine: Add hisilicon k3 DMA engine driver

[zhangfei gao]

On Tue, Aug 13, 2013 at 7:20 PM, Vinod Koul <vinod.koul at intel.com> wrote:
> On Fri, Jun 28, 2013 at 08:39:13PM +0800, Zhangfei Gao wrote:
>> Add dmaengine driver for hisilicon k3 platform based on virt_dma
>>
>> Signed-off-by: Zhangfei Gao <zhangfei.gao at linaro.org>
>> Tested-by: Kai Yang <jean.yangkai at huawei.com>
>> Acked-by: Arnd Bergmann <arnd at arndb.de>
>> ---
>
>> +#define DRIVER_NAME          "k3-dma"
>> +#define DMA_ALIGN            3
>> +#define DMA_MAX_SIZE         0x1ffc
>> +
>> +#define INT_STAT             0x00
>> +#define INT_TC1                      0x04
>> +#define INT_ERR1             0x0c
>> +#define INT_ERR2             0x10
>> +#define INT_TC1_MASK         0x18
>> +#define INT_ERR1_MASK                0x20
>> +#define INT_ERR2_MASK                0x24
>> +#define INT_TC1_RAW          0x600
>> +#define INT_ERR1_RAW         0x608
>> +#define INT_ERR2_RAW         0x610
>> +#define CH_PRI                       0x688
>> +#define CH_STAT                      0x690
>> +#define CX_CUR_CNT           0x704
>> +#define CX_LLI                       0x800
>> +#define CX_CNT                       0x810
>> +#define CX_SRC                       0x814
>> +#define CX_DST                       0x818
>> +#define CX_CONFIG            0x81c
>> +#define AXI_CONFIG           0x820
>> +#define DEF_AXI_CONFIG               0x201201
>> +
>> +#define CX_LLI_CHAIN_EN              0x2
>> +#define CCFG_EN                      0x1
>> +#define CCFG_MEM2PER         (0x1 << 2)
>> +#define CCFG_PER2MEM         (0x2 << 2)
>> +#define CCFG_SRCINCR         (0x1 << 31)
>> +#define CCFG_DSTINCR         (0x1 << 30)
> I see these are not namespace aptly and can collide..
OK,
How about change name to
#define CX_CFG                 0x81c

#define CX_CFG_EN              0x1
#define CX_CFG_MEM2PER         (0x1 << 2)
~

>> +             switch (width) {
>> +             case DMA_SLAVE_BUSWIDTH_1_BYTE:
>> +                     val = 0;
>> +                     break;
>> +             case DMA_SLAVE_BUSWIDTH_2_BYTES:
>> +                     val = 1;
>> +                     break;
>> +             case DMA_SLAVE_BUSWIDTH_4_BYTES:
>> +                     val = 2;
>> +                     break;
>> +             case DMA_SLAVE_BUSWIDTH_8_BYTES:
>> +                     val = 3;
> DMA_SLAVE_BUSWIDTHS are 1, 2, 4, 8...
> so if you can do val = ffs(width) as well?

Good idea, will use __ffs(width) here.

>
>
>> +     case DMA_PAUSE:
>> +             dev_dbg(d->slave.dev, "vchan %p: pause\n", &c->vc);
>> +             if (c->status == DMA_IN_PROGRESS) {
>> +                     c->status = DMA_PAUSED;
>> +                     if (p) {
>> +                             k3_dma_pause_dma(p, false);
>> +                     } else {
>> +                             spin_lock(&d->lock);
>> +                             list_del_init(&c->node);
>> +                             spin_unlock(&d->lock);
>> +                     }
> why do we need the else part here?
Since asynchronous mode is supported.
Desc is submitted to list but may not get physical channel to run.

>
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static int k3_dma_pltfm_suspend(struct device *dev)
>> +{
>> +     struct k3_dma_dev *d = dev_get_drvdata(dev);
>> +     u32 stat = 0;
>> +
>> +     stat = k3_dma_get_chan_stat(d);
>> +     if (stat) {
>> +             dev_warn(d->slave.dev,
>> +                     "chan %d is running fail to suspend\n", stat);
>> +             return -1;
>> +     }
>> +     k3_dma_enable_dma(d, false);
>> +     clk_disable_unprepare(d->clk);
>> +     return 0;
>> +}
>> +
>> +static int k3_dma_pltfm_resume(struct device *dev)
>> +{
>> +     struct k3_dma_dev *d = dev_get_drvdata(dev);
>> +     int ret = 0;
>> +
>> +     ret = clk_prepare_enable(d->clk);
>> +     if (ret < 0) {
>> +             dev_err(d->slave.dev, "clk_prepare_enable failed: %d\n", ret);
>> +             return -EINVAL;
>> +     }
>> +     k3_dma_enable_dma(d, true);
>> +     return 0;
>> +}
>> +#else
>> +#define k3_dma_pltfm_suspend NULL
>> +#define k3_dma_pltfm_resume  NULL
>> +#endif /* CONFIG_PM_SLEEP */
> you dont need #ifdef here, then whats the use of SIMPLE_DEV_PM_OPS below?
Yes.

>> +
>> +SIMPLE_DEV_PM_OPS(k3_dma_pltfm_pmops, k3_dma_pltfm_suspend, k3_dma_pltfm_resume);
> pltfm... can we skip this or use platform, plat...
Got it.

>> +
>> +static struct platform_driver k3_pdma_driver = {
>> +     .driver         = {
>> +             .name   = DRIVER_NAME,
>> +             .owner  = THIS_MODULE,
>> +             .pm     = &k3_dma_pltfm_pmops,
>> +             .of_match_table = k3_pdma_dt_ids,
>> +     },
>> +     .probe          = k3_dma_probe,
>> +     .remove         = k3_dma_remove,
>> +};
>> +
>> +module_platform_driver(k3_pdma_driver);
>> +
>> +MODULE_DESCRIPTION("Hisilicon k3 DMA Driver");
>> +MODULE_LICENSE("GPL v2");
> MODULE_ALIAS?

OK.
>
> ~Vinod
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel