| 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
| ||
|
Message-ID:
<AS4PR04MB938646EE3BB6B8E828D6A2FAE1202@AS4PR04MB9386.eurprd04.prod.outlook.com>
Date: Thu, 7 Mar 2024 08:22:22 +0000
From: Joy Zou <joy.zou@....com>
To: Frank Li <frank.li@....com>, Alexander Stein
<alexander.stein@...tq-group.com>
CC: Vinod Koul <vkoul@...nel.org>, Shawn Guo <shawnguo@...nel.org>, Sascha
Hauer <s.hauer@...gutronix.de>, Pengutronix Kernel Team
<kernel@...gutronix.de>, Fabio Estevam <festevam@...il.com>, dl-linux-imx
<linux-imx@....com>, "linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>, "dmaengine@...r.kernel.org"
<dmaengine@...r.kernel.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "imx@...ts.linux.dev" <imx@...ts.linux.dev>,
Nicolin Chen <b42378@...escale.com>, "S.J. Wang" <shengjiu.wang@....com>
Subject: RE: [PATCH 1/4] dmaengine: imx-sdma: Support allocate memory from
internal SRAM (iram)
> -----Original Message-----
> From: Frank Li <frank.li@....com>
> Sent: 2024年3月6日 23:23
> To: Alexander Stein <alexander.stein@...tq-group.com>
> Cc: Vinod Koul <vkoul@...nel.org>; Shawn Guo <shawnguo@...nel.org>;
> Sascha Hauer <s.hauer@...gutronix.de>; Pengutronix Kernel Team
> <kernel@...gutronix.de>; Fabio Estevam <festevam@...il.com>;
> dl-linux-imx <linux-imx@....com>; linux-arm-kernel@...ts.infradead.org;
> dmaengine@...r.kernel.org; linux-kernel@...r.kernel.org;
> imx@...ts.linux.dev; Nicolin Chen <b42378@...escale.com>; S.J. Wang
> <shengjiu.wang@....com>; Joy Zou <joy.zou@....com>
> Subject: Re: [PATCH 1/4] dmaengine: imx-sdma: Support allocate memory
> from internal SRAM (iram)
>
> On Wed, Mar 06, 2024 at 10:55:00AM +0100, Alexander Stein wrote:
> > Hi Frank,
> >
> > thanks for the patch.
> >
> > Am Montag, 4. März 2024, 05:32:53 CET schrieb Frank Li:
> > > From: Nicolin Chen <b42378@...escale.com>
> > >
> > > Allocate memory from SoC internal SRAM to reduce DDR access and keep
> > > DDR in lower power state (such as self-referesh) longer.
> > >
> > > Check iram_pool before sdma_init() so that ccb/context could be
> > > allocated from iram because DDR maybe in self-referesh in lower
> > > power audio case while sdma still running.
> > >
> > > Reviewed-by: Shengjiu Wang <shengjiu.wang@....com>
> > > Signed-off-by: Nicolin Chen <b42378@...escale.com>
> > > Signed-off-by: Joy Zou <joy.zou@....com>
> > > Signed-off-by: Frank Li <Frank.Li@....com>
> > > ---
> > > drivers/dma/imx-sdma.c | 53
> > > +++++++++++++++++++++++++++++++++++++-------------
> > > 1 file changed, 40 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c index
> > > 9b42f5e96b1e0..9a6d8f1e9ff63 100644
> > > --- a/drivers/dma/imx-sdma.c
> > > +++ b/drivers/dma/imx-sdma.c
> > > @@ -24,6 +24,7 @@
> > > #include <linux/semaphore.h>
> > > #include <linux/spinlock.h>
> > > #include <linux/device.h>
> > > +#include <linux/genalloc.h>
> > > #include <linux/dma-mapping.h>
> > > #include <linux/firmware.h>
> > > #include <linux/slab.h>
> > > @@ -516,6 +517,7 @@ struct sdma_engine {
> > > void __iomem *regs;
> > > struct sdma_context_data *context;
> > > dma_addr_t context_phys;
> > > + dma_addr_t ccb_phys;
> > > struct dma_device dma_device;
> > > struct clk *clk_ipg;
> > > struct clk *clk_ahb;
> > > @@ -531,6 +533,7 @@ struct sdma_engine {
> > > /* clock ratio for AHB:SDMA core. 1:1 is 1, 2:1 is 0*/
> > > bool clk_ratio;
> > > bool fw_loaded;
> > > + struct gen_pool *iram_pool;
> > > };
> > >
> > > static int sdma_config_write(struct dma_chan *chan, @@ -1358,8
> > > +1361,14 @@ static int sdma_request_channel0(struct sdma_engine
> > > *sdma) {
> > > int ret = -EBUSY;
> > >
> > > - sdma->bd0 = dma_alloc_coherent(sdma->dev, PAGE_SIZE,
> &sdma->bd0_phys,
> > > - GFP_NOWAIT);
> > > + if (sdma->iram_pool)
> > > + sdma->bd0 = gen_pool_dma_alloc(sdma->iram_pool,
> > > + sizeof(struct sdma_buffer_descriptor),
> > > + &sdma->bd0_phys);
> > > + else
> > > + sdma->bd0 = dma_alloc_coherent(sdma->dev,
> > > + sizeof(struct sdma_buffer_descriptor),
> > > + &sdma->bd0_phys, GFP_NOWAIT);
> > > if (!sdma->bd0) {
> > > ret = -ENOMEM;
> > > goto out;
> > > @@ -1379,10 +1388,14 @@ static int sdma_request_channel0(struct
> > > sdma_engine *sdma) static int sdma_alloc_bd(struct sdma_desc *desc)
> > > {
> > > u32 bd_size = desc->num_bd * sizeof(struct
> > > sdma_buffer_descriptor);
> > > + struct sdma_engine *sdma = desc->sdmac->sdma;
> > > int ret = 0;
> > >
> > > - desc->bd = dma_alloc_coherent(desc->sdmac->sdma->dev, bd_size,
> > > - &desc->bd_phys, GFP_NOWAIT);
> > > + if (sdma->iram_pool)
> > > + desc->bd = gen_pool_dma_alloc(sdma->iram_pool, bd_size,
> &desc->bd_phys);
> > > + else
> > > + desc->bd = dma_alloc_coherent(sdma->dev, bd_size,
> &desc->bd_phys,
> > > +GFP_NOWAIT);
> > > +
> > > if (!desc->bd) {
> > > ret = -ENOMEM;
> > > goto out;
> > > @@ -1394,9 +1407,12 @@ static int sdma_alloc_bd(struct sdma_desc
> > > *desc) static void sdma_free_bd(struct sdma_desc *desc) {
> > > u32 bd_size = desc->num_bd * sizeof(struct
> > > sdma_buffer_descriptor);
> > > + struct sdma_engine *sdma = desc->sdmac->sdma;
> > >
> > > - dma_free_coherent(desc->sdmac->sdma->dev, bd_size, desc->bd,
> > > - desc->bd_phys);
> > > + if (sdma->iram_pool)
> > > + gen_pool_free(sdma->iram_pool, (unsigned long)desc->bd,
> bd_size);
> > > + else
> > > + dma_free_coherent(desc->sdmac->sdma->dev, bd_size,
> desc->bd,
> > > +desc->bd_phys);
> > > }
> > >
> > > static void sdma_desc_free(struct virt_dma_desc *vd) @@ -2066,8
> > > +2082,8 @@ static int sdma_get_firmware(struct sdma_engine *sdma,
> > >
> > > static int sdma_init(struct sdma_engine *sdma) {
> > > + int ccbsize;
> > > int i, ret;
> > > - dma_addr_t ccb_phys;
> >
> > What is the motivation to put ccb_phys to struct sdma_engine? AFAICS
> > this is only used in sdma_init. Also the following patches of this
> > series are not using the struct member.
>
> You are right. let me check other donwstream patches. If no one use it, it
> should be local variable.
Hi frank,
Yes, it should be local variable in current patch. I have checked downstream patches. We have optimized the sdma_init function in other patch, this patch need to put ccb_phys to struct sdma_engine.
BR
Joy Zou
> >
> > Best regards,
> > Alexander
> >
> > >
> > > ret = clk_enable(sdma->clk_ipg);
> > > if (ret)
> > > @@ -2083,10 +2099,15 @@ static int sdma_init(struct sdma_engine
> *sdma)
> > > /* Be sure SDMA has not started yet */
> > > writel_relaxed(0, sdma->regs + SDMA_H_C0PTR);
> > >
> > > - sdma->channel_control = dma_alloc_coherent(sdma->dev,
> > > - MAX_DMA_CHANNELS * sizeof(struct sdma_channel_control)
> +
> > > - sizeof(struct sdma_context_data),
> > > - &ccb_phys, GFP_KERNEL);
> > > + ccbsize = MAX_DMA_CHANNELS * (sizeof(struct
> sdma_channel_control)
> > > + + sizeof(struct sdma_context_data));
> > > +
> > > + if (sdma->iram_pool)
> > > + sdma->channel_control =
> gen_pool_dma_alloc(sdma->iram_pool,
> > > + ccbsize, &sdma->ccb_phys);
> > > + else
> > > + sdma->channel_control = dma_alloc_coherent(sdma->dev,
> ccbsize, &sdma->ccb_phys,
> > > + GFP_KERNEL);
> > >
> > > if (!sdma->channel_control) {
> > > ret = -ENOMEM;
> > > @@ -2095,7 +2116,7 @@ static int sdma_init(struct sdma_engine
> *sdma)
> > >
> > > sdma->context = (void *)sdma->channel_control +
> > > MAX_DMA_CHANNELS * sizeof(struct sdma_channel_control);
> > > - sdma->context_phys = ccb_phys +
> > > + sdma->context_phys = sdma->ccb_phys +
> > > MAX_DMA_CHANNELS * sizeof(struct sdma_channel_control);
> > >
> > > /* disable all channels */
> > > @@ -2121,7 +2142,7 @@ static int sdma_init(struct sdma_engine
> *sdma)
> > > else
> > > writel_relaxed(0, sdma->regs + SDMA_H_CONFIG);
> > >
> > > - writel_relaxed(ccb_phys, sdma->regs + SDMA_H_C0PTR);
> > > + writel_relaxed(sdma->ccb_phys, sdma->regs + SDMA_H_C0PTR);
> > >
> > > /* Initializes channel's priorities */
> > > sdma_set_channel_priority(&sdma->channel[0], 7); @@ -2272,6
> > > +2293,12 @@ static int sdma_probe(struct platform_device *pdev)
> > > vchan_init(&sdmac->vc, &sdma->dma_device);
> > > }
> > >
> > > + if (np) {
> > > + sdma->iram_pool = of_gen_pool_get(np, "iram", 0);
> > > + if (sdma->iram_pool)
> > > + dev_info(&pdev->dev, "alloc bd from iram.\n");
> > > + }
> > > +
> > > ret = sdma_init(sdma);
> > > if (ret)
> > > goto err_init;
> > >
> > >
> >
> >
> > --
> > TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> > Amtsgericht München, HRB 105018
> > Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> > http://www.tq-group.com/
> >
> >
Powered by blists - more mailing lists