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

[Serge Semin]

On Thu, Apr 18, 2024 at 02:47:18PM +0300, Andy Shevchenko wrote:
> 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.

Ok. I see. Thanks for clarification. I normally do that in another
order: group the functionally related fields together - all
source-related configs first, then all destination-related configs.
Honestly I don't have strong opinion about this part, it's just my
personal preference. Am I right to think that from your experience in
kernel it's normally done in the order you described?

> 
> > > > +	} 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)
> 

Ok. Let's follow your way in this case. After your how-to-read-it
comment your version no longer look less readable than what is
implemented by me. Thanks for clarification.

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

Interestingly, but asm-code in both cases match.) So the redundant
store operation in your C-code gets to be optimized away.

-Serge(y)

> 
> > Do you think it still would be better to use your version despite of
> > my reasoning?
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>