linux-kernel - Re: [PATCH v3 2/6] dmaengine: mxs: support i.MX7D and deep sleep mode

Open Source and information security mailing list archives

Date:	Tue, 20 Oct 2015 10:52:58 -0500
From:	Han Xu <xhnjupt@...il.com>
To:	Vinod Koul <vinod.koul@...el.com>
Cc:	Han Xu <b45815@...escale.com>,
	Fabio Estevam <fabio.estevam@...escale.com>,
	boris.brezillon@...e-electrons.com, linux-kernel@...r.kernel.org,
	dmaengine@...r.kernel.org,
	"linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>,
	Nicholas Mc Guire <hofrat@...dl.org>,
	Huang Shijie <shijie.huang@....com>, dan.j.williams@...el.com,
	Brian Norris <computersforpeace@...il.com>,
	David Woodhouse <dwmw2@...radead.org>
Subject: Re: [PATCH v3 2/6] dmaengine: mxs: support i.MX7D and deep sleep mode

On Mon, Sep 21, 2015 at 12:02 PM, Vinod Koul <vinod.koul@...el.com> wrote:
> On Fri, Aug 28, 2015 at 02:32:41PM -0500, Han Xu wrote:
>> @@ -28,7 +28,6 @@
>>  #include <linux/of_device.h>
>>  #include <linux/of_dma.h>
>>  #include <linux/list.h>
>> -
>
> Pl dont change at random places

It's not a random place, i.MX7D need an extra clock clk_io for APBH
DMA, I will add more
comments here

>
>> +     if (mxs_dma->dev_id == IMX7D_DMA) {
>> +             ret = clk_prepare_enable(mxs_dma->clk_io);
>> +             if (ret)
>> +                     goto err_clk_unprepare;
>> +     }
>> +
>>       mxs_dma_reset_chan(chan);
>>
>>       dma_async_tx_descriptor_init(&mxs_chan->desc, chan);
>> @@ -450,6 +464,8 @@ static int mxs_dma_alloc_chan_resources(struct dma_chan *chan)
>>
>>       return 0;
>>
>> +err_clk_unprepare:
>> +     clk_disable_unprepare(mxs_dma->clk);
>
> and this doesn't look right. You are calling this for failure on
> clk_prepare_enable() so if clock prepare failed you are still going to
> disable and unprepare??

This is also for i.MX7D, if enable mxs_dma->clk_io failed, disable
mxs_dma->clk as well.

>
>> -static int __init mxs_dma_init(struct mxs_dma_engine *mxs_dma)
>> +static int mxs_dma_init(struct mxs_dma_engine *mxs_dma)
>
> this should be separate change explaining why

I will separate the clock change and PM change to two patches.

>
>> +static int mxs_dma_pm_resume(struct device *dev)
>> +{
>> +     struct mxs_dma_engine *mxs_dma = dev_get_drvdata(dev);
>> +     int ret;
>> +
>> +     ret = mxs_dma_init(mxs_dma);
>> +     if (ret)
>> +             return ret;
>> +     return 0;
>
> Aren't you supposed to prepare and unprepare clock in PM handlers too?

APBH DMA was dedicate for GPMI NAND driver, NAND driver will release
all DMA resources
when suspend and acquire DMA resources when resume. All clock
enable/disable were handled
in these functions. Please refer to the patch serial [PATCH v3 1/6]
mtd: nand: gpmi: add gpmi
dsm supend/resume support

>
> --
> ~Vinod
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives