Received-SPF: pass (google.com: domain of linux-kernel+bounces-147452-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Tue, 16 Apr 2024 22:11:58 +0300
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Serge Semin <fancer.lancer@gmail.com>
Cc: Viresh Kumar <vireshk@kernel.org>, Vinod Koul <vkoul@kernel.org>,
	Ilpo =?iso-8859-1?Q?J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jiri Slaby <jirislaby@kernel.org>, dmaengine@vger.kernel.org,
	linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 4/4] dmaengine: dw: Simplify max-burst calculation
 procedure
Message-ID: <Zh7NfmffgSBSjVWv@smile.fi.intel.com>
References: <20240416162908.24180-1-fancer.lancer@gmail.com>
 <20240416162908.24180-5-fancer.lancer@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240416162908.24180-5-fancer.lancer@gmail.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo

On Tue, Apr 16, 2024 at 07:28:58PM +0300, Serge Semin wrote:
> In order to have a more coherent DW AHB DMA slave configuration method
> let's simplify the source and destination channel max-burst calculation
> procedure:
> 
> 1. Create the max-burst verification method as it has been just done for
> the memory and peripheral address widths. Thus the DWC DMA slave config

dwc_config() method

?

> method will turn to a set of the verification methods execution.
> 
> 2. Since both the generic DW AHB DMA and Intel DMA32 engines support the

"i" in iDMA 32-bit stands for "integrated", so 'Intel iDMA 32-bit'

> power-of-2 bursts only, then the specified by the client driver max-burst
> values can be converted to being power-of-2 right in the max-burst
> verification method.
> 
> 3. Since max-burst encoded value is required on the CTL_LO fields
> calculation stage, the encode_maxburst() callback can be easily dropped
> from the dw_dma structure meanwhile the encoding procedure will be
> executed right in the CTL_LO register value calculation.
> 
> Thus the update will provide the next positive effects: the internal
> DMA-slave config structure will contain only the real DMA-transfer config
> value, which will be encoded to the DMA-controller register fields only
> when it's required on the buffer mapping; the redundant encode_maxburst()
> callback will be dropped simplifying the internal HW-abstraction API;
> DWC-config method will look more readable executing the verification

dwc_config() method

?

> functions one-by-one.

..

> +static void dwc_verify_maxburst(struct dma_chan *chan)

It's inconsistent to the rest of _verify methods. It doesn't verify as it
doesn't return anything. Make it int or rename the function.

> +{
> +	struct dw_dma_chan *dwc = to_dw_dma_chan(chan);
> +
> +	dwc->dma_sconfig.src_maxburst =
> +		clamp(dwc->dma_sconfig.src_maxburst, 1U, dwc->max_burst);
> +	dwc->dma_sconfig.dst_maxburst =
> +		clamp(dwc->dma_sconfig.dst_maxburst, 1U, dwc->max_burst);
> +
> +	dwc->dma_sconfig.src_maxburst =
> +		rounddown_pow_of_two(dwc->dma_sconfig.src_maxburst);
> +	dwc->dma_sconfig.dst_maxburst =
> +		rounddown_pow_of_two(dwc->dma_sconfig.dst_maxburst);
> +}

..

>  static int dwc_verify_p_buswidth(struct dma_chan *chan)
> -		reg_burst = rounddown_pow_of_two(dwc->dma_sconfig.src_maxburst);
> +		reg_burst = dwc->dma_sconfig.src_maxburst;

Seems you have a dependency, need a comment below that maxburst has to be
"verified" [whatever] first.

..

> +static inline u8 dw_dma_encode_maxburst(u32 maxburst)
> +{
> +	/*
> +	 * Fix burst size according to dw_dmac. We need to convert them as:
> +	 * 1 -> 0, 4 -> 1, 8 -> 2, 16 -> 3.
> +	 */
> +	return maxburst > 1 ? fls(maxburst) - 2 : 0;
> +}

Split these moves to another preparatory patch.

-- 
With Best Regards,
Andy Shevchenko