Re: [PATCH 1/4] dmaengine: imx-sdma: Support allocate memory from internal SRAM (iram)

[Frank Li]

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@xxxxxxxxxxxxx>
> > 
> > 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@xxxxxxx>
> > Signed-off-by: Nicolin Chen <b42378@xxxxxxxxxxxxx>
> > Signed-off-by: Joy Zou <joy.zou@xxxxxxx>
> > Signed-off-by: Frank Li <Frank.Li@xxxxxxx>
> > ---
> >  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.

> 
> 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/
> 
>