Re: [PATCH 3/4] dmaengine: dw: Simplify prepare CTL_LO methods

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, Apr 17, 2024 at 11:11:46PM +0300, Serge Semin wrote:
> On Tue, Apr 16, 2024 at 10:04:42PM +0300, Andy Shevchenko wrote:
> > On Tue, Apr 16, 2024 at 07:28:57PM +0300, Serge Semin wrote:

...

> > > +	if (dwc->direction == DMA_MEM_TO_DEV) {
> > > +		sms = dwc->dws.m_master;
> > > +		smsize = 0;
> > > +		dms = dwc->dws.p_master;
> > > +		dmsize = sconfig->dst_maxburst;
> > 
> 
> > I would group it differently, i.e.
> > 
> > 		sms = dwc->dws.m_master;
> > 		dms = dwc->dws.p_master;
> > 		smsize = 0;
> > 		dmsize = sconfig->dst_maxburst;
> 
> Could you please clarify, why? From my point of view it was better to
> group the source master ID and the source master burst size inits
> together.

Sure. The point here is that when you look at the DMA channel configuration
usually you operate with the semantically tied fields for source and
destination. At least this is my experience, I always check both sides
of the transfer for the same field, e.g., master setting, hence I want to
have them coupled.

> > > +	} else if (dwc->direction == DMA_DEV_TO_MEM) {
> > > +		sms = dwc->dws.p_master;
> > > +		smsize = sconfig->src_maxburst;
> > > +		dms = dwc->dws.m_master;
> > > +		dmsize = 0;
> > > +	} else /* DMA_MEM_TO_MEM */ {
> > > +		sms = dwc->dws.m_master;
> > > +		smsize = 0;
> > > +		dms = dwc->dws.m_master;
> > > +		dmsize = 0;
> > > +	}
> > 
> > Ditto for two above cases.

...

> > >  static u32 idma32_prepare_ctllo(struct dw_dma_chan *dwc)
> > >  {
> > >  	struct dma_slave_config	*sconfig = &dwc->dma_sconfig;
> > > -	u8 smsize = (dwc->direction == DMA_DEV_TO_MEM) ? sconfig->src_maxburst : 0;
> > > -	u8 dmsize = (dwc->direction == DMA_MEM_TO_DEV) ? sconfig->dst_maxburst : 0;

> > > +	u8 smsize, dmsize;
> > > +
> > > +	if (dwc->direction == DMA_MEM_TO_DEV) {
> > > +		smsize = 0;
> > > +		dmsize = sconfig->dst_maxburst;
> > > +	} else if (dwc->direction == DMA_DEV_TO_MEM) {
> > > +		smsize = sconfig->src_maxburst;
> > > +		dmsize = 0;
> > > +	} else /* DMA_MEM_TO_MEM */ {
> > > +		smsize = 0;
> > > +		dmsize = 0;
> > > +	}
> > 
> > 	u8 smsize = 0, dmsize = 0;
> > 
> > 	if (dwc->direction == DMA_MEM_TO_DEV)
> > 		dmsize = sconfig->dst_maxburst;
> > 	else if (dwc->direction == DMA_DEV_TO_MEM)
> > 		smsize = sconfig->src_maxburst;
> > 
> > ?
> > 
> > Something similar also can be done in the Synopsys case above, no?
> 
> As in case of the patch #1 the if-else statement here was designed
> like that intentionally: to signify that the else clause implies the
> DMA_MEM_TO_MEM transfer. Any other one (like DMA_DEV_TO_DEV) would
> need to have the statement alteration.

My version as I read it:
- for M2D the dmsize is important
- for D2M the smsize is important
- for anything else use defaults (which are 0)

> Moreover even though your
> version looks smaller, but it causes one redundant store operation.

Most likely not. Any assembler here? I can check on x86_64, but I believe it
simply assigns 0 for both u8 at once using xor r16,r16 or so.

Maybe ARM or MIPS (what do you use?) sucks? :-)

> Do you think it still would be better to use your version despite of
> my reasoning?

-- 
With Best Regards,
Andy Shevchenko