Received-SPF: pass (google.com: domain of linux-kernel+bounces-149194-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Wed, 17 Apr 2024 23:35:39 +0300
From: Serge Semin <fancer.lancer@gmail.com>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Viresh Kumar <vireshk@kernel.org>, Vinod Koul <vkoul@kernel.org>, 
	Ilpo =?utf-8?B?SsOkcnZpbmVu?= <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: <tez5uqt4lg2qf5nooxuqo2rqhkqzzzbpeysdcbljokznbztkhj@j5t7cy4gd4pd>
References: <20240416162908.24180-1-fancer.lancer@gmail.com>
 <20240416162908.24180-5-fancer.lancer@gmail.com>
 <Zh7NfmffgSBSjVWv@smile.fi.intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Zh7NfmffgSBSjVWv@smile.fi.intel.com>

On Tue, Apr 16, 2024 at 10:11:58PM +0300, Andy Shevchenko wrote:
> 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
> 
> ?

Right. I'll just directly refer to the dwc_config() method here.

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

Ok. Thanks for clarification.

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

Ok.

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

Making it int won't make much sense since currently the method doesn't
imply returning an error status. IMO using "verify" was ok, but since
you don't see it suitable please suggest a better alternative. mend,
fix, align?

> 
> > +{
> > +	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.

Ok.

> 
> ...
> 
> > +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.

Ok.

-Serge(y)

> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>