Re: [PATCH v2 2/2] dma: sprd: Add Spreadtrum DMA driver

[Baolin Wang <baolin.wang@linaro.org>]

Hi Vinod,

On 7 September 2017 at 00:22, Vinod Koul <vinod.koul@intel.com> wrote:
> On Tue, Sep 05, 2017 at 07:48:43PM +0800, Baolin Wang wrote:
>
>> > > +/* DMA global registers definition */
>> > > +#define DMA_GLB_PAUSE                    0x0
>> > > +#define DMA_GLB_FRAG_WAIT                0x4
>> > > +#define DMA_GLB_REQ_PEND0_EN             0x8
>> > > +#define DMA_GLB_REQ_PEND1_EN             0xc
>> > > +#define DMA_GLB_INT_RAW_STS              0x10
>> > > +#define DMA_GLB_INT_MSK_STS              0x14
>> > > +#define DMA_GLB_REQ_STS                  0x18
>> > > +#define DMA_GLB_CHN_EN_STS               0x1c
>> > > +#define DMA_GLB_DEBUG_STS                0x20
>> > > +#define DMA_GLB_ARB_SEL_STS              0x24
>> > > +#define DMA_GLB_CHN_START_CHN_CFG1       0x28
>> > > +#define DMA_GLB_CHN_START_CHN_CFG2       0x2c
>> > > +#define DMA_CHN_LLIST_OFFSET             0x10
>> > > +#define DMA_GLB_REQ_CID_OFFSET           0x2000
>> > > +#define DMA_GLB_REQ_CID(uid)             (0x4 * ((uid) - 1))
>> >
>> > namespaced properly please, here and rest..
>>
>> Could you elaborate which name need to named properly?
>> I guess DMA_GLB_REQ_CID is not very clear, can I change
>> to DMA_GLB_REQ_UID(uid) which is used to define the UID
>> registers?
>
> I meant something like:
>
> SPRD_DMA_xxxx
>
> DMA_GLB is very generic term and might cause collisions in future so lets
> make these future proof..

Make sense.

>
>> > > +static int sprd_dma_enable(struct sprd_dma_dev *sdev)
>> > > +{
>> > > + int ret;
>> > > +
>> > > + ret = clk_prepare_enable(sdev->clk);
>> > > + if (ret)
>> > > +         return ret;
>> > > +
>> > > + if (!IS_ERR(sdev->ashb_clk))
>> >
>> > that looks odd, can you explain this?
>>
>> Since the ashb_clk is optional and only for AGCP DMA controller,
>> so here we add one condition to check if the ashb_clk need enable.
>
> it be worth documenting this..

OK.

>
>> > > +static void sprd_dma_free_chan_resources(struct dma_chan *chan)
>> > > +{
>> > > + struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
>> > > + unsigned long flags;
>> > > +
>> > > + spin_lock_irqsave(&schan->vc.lock, flags);
>> > > + sprd_dma_stop(schan);
>> > > + spin_unlock_irqrestore(&schan->vc.lock, flags);
>> > > +
>> > > + vchan_free_chan_resources(&schan->vc);
>> > > + pm_runtime_put_sync(chan->device->dev);
>> >
>> > why put_sync()
>>
>> Since we will get pm counter to resume DMA when allocating resources, then
>> we should put pm counter in sprd_dma_free_chan_resources() to try to suspend
>> DMA if the pm counter is 0.
>
> runtime_pm part is fine, you could have used pm_runtime_put(), why the
> sync() variant here...

After checking again, I agree pm_runtime_put() is enough here and I
will fix it in next version.

>>
>> >
>> > > +static enum dma_status sprd_dma_tx_status(struct dma_chan *chan,
>> > > +                                   dma_cookie_t cookie,
>> > > +                                   struct dma_tx_state *txstate)
>> > > +{
>> > > + struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
>> > > + unsigned long flags;
>> > > + enum dma_status ret;
>> > > +
>> > > + ret = dma_cookie_status(chan, cookie, txstate);
>> > > +
>> > > + spin_lock_irqsave(&schan->vc.lock, flags);
>> > > + txstate->residue = sprd_dma_get_dst_addr(schan);
>> >
>> > I dont think this is correct, the residue needs to be read only for current
>> > cookie and the query might be for one which is not even submitted
>>
>> We have one scenario for our audio driver, the audio driver need to get
>> the destination address to check if their transfer is done in no irq mode.
>
> Yes but for the descriptor requested but not any. Audio maybe working as
> period count maybe lesser...
>
>> > > +static int sprd_dma_probe(struct platform_device *pdev)
>> > > +{
>> > > + struct device_node *np = pdev->dev.of_node;
>> > > + struct sprd_dma_dev *sdev;
>> > > + struct sprd_dma_chn *dma_chn;
>> > > + struct resource *res;
>> > > + u32 chn_count;
>> > > + int ret, i;
>> > > +
>> > > + ret = of_property_read_u32(np, "#dma-channels", &chn_count);
>> > > + if (ret) {
>> > > +         dev_err(&pdev->dev, "get dma channels count failed\n");
>> > > +         return ret;
>> > > + }
>> > > +
>> > > + sdev = devm_kzalloc(&pdev->dev, (sizeof(*sdev) +
>> > > +                     (sizeof(struct sprd_dma_chn) * chn_count)),
>> > > +                     GFP_KERNEL);
>> > > + if (!sdev)
>> > > +         return -ENOMEM;
>> > > +
>> > > + sdev->clk = devm_clk_get(&pdev->dev, "enable");
>> > > + if (IS_ERR(sdev->clk)) {
>> > > +         dev_err(&pdev->dev, "get enable clock failed\n");
>> > > +         return PTR_ERR(sdev->clk);
>> > > + }
>> > > +
>> > > + /* ashb clock is optional for AGCP DMA */
>> > > + sdev->ashb_clk = devm_clk_get(&pdev->dev, "ashb_eb");
>> > > + if (IS_ERR(sdev->ashb_clk))
>> > > +         dev_warn(&pdev->dev, "no optional ashb eb clock\n");
>> > > +
>> > > + sdev->irq = platform_get_irq(pdev, 0);
>> > > + if (sdev->irq > 0) {
>> > > +         ret = devm_request_irq(&pdev->dev, sdev->irq, dma_irq_handle,
>> > > +                                0, "sprd_dma", (void *)sdev);
>> >
>> > so after this your driver should be able to handle the urq, are you ready
>> > for that, I think not
>>
>> We have one no irq mode for audio driver, our DMA driver do not need do handle
>> the irq, and audio driver can get the destination address to check if their
>> transfer is done.
>
> ?? you are adding interrupt handler!

Sorry for confusing. This driver is not only for one DMA controller,
we can have 3 DMA controllers for different usage, moreover we can
have irq mode or no irq mode for different cases, in irq mode the DMA
interrupt handler need handle the descriptors when transfer is done.
But in no irq mode for our audio driver special case, which is used to
save system power without DMA interrupts resume system, in this
scenario we do not need complete descriptors by DMA handler.

>
>> > > +static int sprd_dma_remove(struct platform_device *pdev)
>> > > +{
>> > > + struct sprd_dma_dev *sdev = platform_get_drvdata(pdev);
>> > > + int ret;
>> > > +
>> > > + ret = pm_runtime_get_sync(&pdev->dev);
>> > > + if (ret < 0)
>> > > +         return ret;
>> > > +
>> > > + dma_async_device_unregister(&sdev->dma_dev);
>> > > + sprd_dma_disable(sdev);
>> >
>> > and irq is not freed, how do you guarantee that irqs are not scheduled
>> > anymore and all tasklets running have completed and cant be further
>> > scheduled?
>>
>> Since we request irq by devm_xxx() API, which means the irq can be freed
>> automatically when removing the DMA driver.
>
> no, thats buggy. you should explictly, a) kill the tasklet b) free irq

OK.

>
>> > > +static int __init sprd_dma_init(void)
>> > > +{
>> > > + return platform_driver_register(&sprd_dma_driver);
>> > > +}
>> > > +arch_initcall_sync(sprd_dma_init);
>> > > +
>> > > +static void __exit sprd_dma_exit(void)
>> > > +{
>> > > + platform_driver_unregister(&sprd_dma_driver);
>> > > +}
>> > > +module_exit(sprd_dma_exit);
>> >
>> > module_platform_driver() pls
>>
>> Since our SPI will depend on DMA driver, but I will try to set module_init
>> level.
>
> ah okay, i missed that. Btw with defer probe I don't think we should have
> that issue

Yes. Will fix that. Thanks for your comments.

-- 
Baolin.wang
Best Regards