Received-SPF: pass (google.com: domain of linux-kernel+bounces-94149-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Wed, 6 Mar 2024 10:23:29 -0500
From: Frank Li <Frank.li@nxp.com>
To: Alexander Stein <alexander.stein@ew.tq-group.com>
Cc: Vinod Koul <vkoul@kernel.org>, Shawn Guo <shawnguo@kernel.org>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	Pengutronix Kernel Team <kernel@pengutronix.de>,
	Fabio Estevam <festevam@gmail.com>,
	NXP Linux Team <linux-imx@nxp.com>,
	linux-arm-kernel@lists.infradead.org, dmaengine@vger.kernel.org,
	linux-kernel@vger.kernel.org, imx@lists.linux.dev,
	Nicolin Chen <b42378@freescale.com>,
	Shengjiu Wang <shengjiu.wang@nxp.com>, Joy Zou <joy.zou@nxp.com>
Subject: Re: [PATCH 1/4] dmaengine: imx-sdma: Support allocate memory from
 internal SRAM (iram)
Message-ID: <ZeiKcYdlQ4S1oWCW@lizhi-Precision-Tower-5810>
References: <20240303-sdma_upstream-v1-0-869cd0165b09@nxp.com>
 <20240303-sdma_upstream-v1-1-869cd0165b09@nxp.com>
 <3280558.aeNJFYEL58@steina-w>
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <3280558.aeNJFYEL58@steina-w>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?iso-8859-1?Q?iWN1bIYBLF/sAHGHs1uwxxJobrPm5NzRj8sZ6lg82DZIYOKlqEs/WDn6uF?=
 =?iso-8859-1?Q?AB4Upw5nlsZ3LsXG0lvH56SB8pP4NcvMxeN/mPcV7Bd3hPQai+6QV0EuGL?=
 =?iso-8859-1?Q?uPKwalCepP1HMsVPCtBwHA+VOIacZtsycP63K9Unx/ethOZIWUbrzTUUHo?=
 =?iso-8859-1?Q?gyJ56G0dvquswQvDTI4clQykZudYODDXX5Fcv9tlvYK6WIGD84AaWPQcTg?=
 =?iso-8859-1?Q?4QjxVGiVtIDZVjxyYwPVvFI7TuMYQUb9egofkfYuZcvFPUELSwMWTMMwbu?=
 =?iso-8859-1?Q?AygrbCUQKOTSKjlt7vLhVZolloF81XXSvU5YcuR6Guwyuz34w/LzV4oyjs?=
 =?iso-8859-1?Q?MZbB2ISJUosNTBDf1wPE8HnXMAqKcLLTTsIWlSUl38raD7mayzRmd/5vlo?=
 =?iso-8859-1?Q?ZXiF9iwlMGd7lndIQ5SuWmoeaNGtXU8rhEnTQfP4ystwXXcyp5VEGGYLdM?=
 =?iso-8859-1?Q?41+3aScJnhfUwQmw2POzpiM/mF06Nllwfgseu48muadI8/0MUNbnpzYD3A?=
 =?iso-8859-1?Q?yMQkhLqW8qzslrCrF3xw+7zBTLPXt4pXA2NSD3+EGk5625/yFsygREbUnS?=
 =?iso-8859-1?Q?K05BouvSVMq5iFb1yAStLnYeLewc0STeCi5KR1DSRG3JmASzKVOl+rsHJz?=
 =?iso-8859-1?Q?u0kCSRg7TOVeToJLYl4fuAj8w0xW40NHe1TYmDMebCUYGipGeJ6qdynd3i?=
 =?iso-8859-1?Q?mkaz5u5NIi0/7rM1H+4Rx3Pt1eP/Tsxzm6Vs0D2HRTGpWVZdxGK6g8ouen?=
 =?iso-8859-1?Q?Q7lzpiwKomnMyYhkOAf2OWKHOH3fLAqoU0oI/veTKUiSYKVtMgPx34X9t/?=
 =?iso-8859-1?Q?ERlL6N4JsIVEzGVA6aKopnTsQ5X6EckMiV14lJDjc/5YlxsKAFN76Zsrzv?=
 =?iso-8859-1?Q?b7B1GNX+ayOnMVHW3NAjF2q0IEf5xCXuRbV8ixAYMWQJWGieMi+r1cEs+E?=
 =?iso-8859-1?Q?YEg2/ivzQdurM2YyW/dNsxmyyq1syPo6lQxvETipiAH5FmMz7nG1Wei722?=
 =?iso-8859-1?Q?IdOWfMBZAUMcyWGwShH/wIw9RIohCm3lD8Bk4Igbf8l5Z8l06SGh5ArdIn?=
 =?iso-8859-1?Q?mJiJyd1XjS1EQq3wW5Ug4ypN8ZgrP9YI8IIPcDCKX6pddABWRjw+sei28M?=
 =?iso-8859-1?Q?qQ6BhM9mQ6knmtizDIEkS8SzN+1bmzGGz6Gq5gCV/7d5yvHiuFGS2wCx/T?=
 =?iso-8859-1?Q?sT4qyYOAzKn603rBbY2WoyHMZO4Z75sWVw6dzxem+GlXfNsJIB8C6CJJnT?=
 =?iso-8859-1?Q?OUpaESnLNVaYcGE77Akyoo7bBqk9Q042Wqsc5Eqde3mdMwEKaBdlEY0UAE?=
 =?iso-8859-1?Q?K5RZLTlZ3FTF3r64bwVayFQGOK4mtVVGnwcJQnpgHcq82eN62s8YW9D2K9?=
 =?iso-8859-1?Q?rfHXikba2BZO560clEvaBRJwCC0zNFLtdNaec8rWFocdnplXATdD/pUMiT?=
 =?iso-8859-1?Q?k6/9S+JyZyBLAbVWjf+Nmb0Ln70ZUtSkcLz64dnTLAy0aOMbuztUsuUSRI?=
 =?iso-8859-1?Q?YUGs6sQH2iv7JAsq4s4+H90fiZm5GEgmDIx9DN39DJzPmBYkcW8JQLSSIk?=
 =?iso-8859-1?Q?Rx6R2HjkUZVHscmoVyh1dsZnE/EHcaglvfTkSZ2jd5S5RlrkXNhH4icY/f?=
 =?iso-8859-1?Q?wP5m4TKasL8bvEgA2LrQmUq27n9Zl7Rs9r?=
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 08d51758-b0d6-4a96-efc4-08dc3df1652b
X-MS-Exchange-CrossTenant-AuthSource: PAXPR04MB9642.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Mar 2024 15:23:37.4924
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 7Cpx4jSdo/8mCrWsnfQasRUDHYyzFEehRUdLhefFkZ+3jcfpW+XGD0ZC1ACstsdpjDeDQjrc8aLgD5nbQUn8zQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AS1PR04MB9238

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

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