Re: [PATCH 1/3] dmaengine: sun6i: Correct DMA support on H3

[Maxime Ripard]

On Fri, Sep 01, 2017 at 05:04:54AM +0200, Stefan Bruens wrote:
> On Donnerstag, 31. August 2017 16:51:35 CEST Maxime Ripard wrote:
> > Hi,
> > 
> > On Thu, Aug 31, 2017 at 01:36:07AM +0200, Stefan Brüns wrote:
> > > +/* Between SoC generations, there are some significant differences:
> > > + * - A23 added a clock gate register
> > > + * - the H3 burst length field has a different offset
> > > + */
> > 
> > This is not the proper comment style.
> > 
> > > +enum dmac_variant {
> > > +	DMAC_VARIANT_A31,
> > > +	DMAC_VARIANT_A23,
> > > +	DMAC_VARIANT_H3,
> > > +};
> > > +
> > 
> > And this is redundant with what we already have in our structures.
> 
> Actually, its not. For H3, there are currently at least 3 register compatible 
> SoCs: H5 is identical, R40 has 16 dma channels, A64 has 8 channels. So if the 
> current config structure is kept, we need 3 different compatible strings. Same 
> for the A23, which is register compatible to e.g. A83t and V3s, but with 
> different numbers of DMA channels.
> 
> So either you decorate the code with a cascade of
> 
> if ((of_is_compatible(..A23..) || of_is_compatible(..A83T..) || ...) {
> } else if ((of_is_compatible(..H3..) || of_is_compatible(..A64..) || ...) {
> } else { /* A31 */
> }
> 
> in a number of places, or you do it just once.

That's not how you retrieve the structures. They are already
associated to the compatible, and you need to do a single lookup to
get them. So that's nowhere near what you're suggesting. You can have
a look at the of_match_device in the probe function.

> 
> > 
> > >  /*
> > >  
> > >   * Hardware channels / ports representation
> > >   *
> > > 
> > > @@ -101,6 +116,7 @@ struct sun6i_dma_config {
> > > 
> > >  	u32 nr_max_channels;
> > >  	u32 nr_max_requests;
> > >  	u32 nr_max_vchans;
> > > 
> > > +	enum dmac_variant dmac_variant;
> > > 
> > >  };
> > >  
> > >  /*
> > > 
> > > @@ -240,8 +256,12 @@ static inline s8 convert_burst(u32 maxburst)
> > > 
> > >  	switch (maxburst) {
> > >  	
> > >  	case 1:
> > >  		return 0;
> > > 
> > > +	case 4:
> > > +		return 1;
> > > 
> > >  	case 8:
> > >  		return 2;
> > > 
> > > +	case 16:
> > > +		return 3;
> > > 
> > >  	default:
> > >  		return -EINVAL;
> > >  	
> > >  	}
> > > 
> > > @@ -249,11 +269,7 @@ static inline s8 convert_burst(u32 maxburst)
> > > 
> > >  static inline s8 convert_buswidth(enum dma_slave_buswidth addr_width)
> > >  {
> > > 
> > > -	if ((addr_width < DMA_SLAVE_BUSWIDTH_1_BYTE) ||
> > > -	    (addr_width > DMA_SLAVE_BUSWIDTH_4_BYTES))
> > > -		return -EINVAL;
> > > -
> > > -	return addr_width >> 1;
> > > +	return ilog2(addr_width);
> > > 
> > >  }
> > 
> > This isn't really the same operation. There should be some explanation
> > about why it's the right thing to do.
> 
> For 1, 2 and 4 it is the same. The correct register value for 8 bytes, 
> supported by H3, H5, A64 and R40, is 3.

That should be in a separate patch, with that in the commit log.

> > >  static size_t sun6i_get_chan_size(struct sun6i_pchan *pchan)
> > > 
> > > @@ -499,45 +515,58 @@ static int set_config(struct sun6i_dma_dev *sdev,
> > > 
> > >  			enum dma_transfer_direction direction,
> > >  			u32 *p_cfg)
> > >  
> > >  {
> > > 
> > > +	enum dma_slave_buswidth src_addr_width, dst_addr_width;
> > > +	u32 src_maxburst, dst_maxburst, supported_burst_length;
> > > 
> > >  	s8 src_width, dst_width, src_burst, dst_burst;
> > > 
> > > +	src_addr_width = sconfig->src_addr_width;
> > > +	dst_addr_width = sconfig->dst_addr_width;
> > > +	src_maxburst = sconfig->src_maxburst;
> > > +	dst_maxburst = sconfig->dst_maxburst;
> > > +
> > > +	if (sdev->cfg->dmac_variant == DMAC_VARIANT_H3)
> > > +		supported_burst_length = BIT(1) | BIT(4) | BIT(8) | BIT(16);
> > > +	else
> > > +		supported_burst_length = BIT(1) | BIT(8);
> > 
> > This could be stored in the structure for example.
> 
> Which one? sun6i_dma_dev? sun6i_dma_config?

The one associated with the compatible, so sun6i_dma_config.

>  
> > >  	switch (direction) {
> > > 
> > >  	case DMA_MEM_TO_DEV:
> > > -		src_burst = convert_burst(sconfig->src_maxburst ?
> > > -					sconfig->src_maxburst : 8);
> > > -		src_width = convert_buswidth(sconfig->src_addr_width !=
> > > -						DMA_SLAVE_BUSWIDTH_UNDEFINED ?
> > > -				sconfig->src_addr_width :
> > > -				DMA_SLAVE_BUSWIDTH_4_BYTES);
> > > -		dst_burst = convert_burst(sconfig->dst_maxburst);
> > > -		dst_width = convert_buswidth(sconfig->dst_addr_width);
> > > +		if (src_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED)
> > > +			src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> > > +		src_maxburst = src_maxburst ? src_maxburst : 8;
> > > 
> > >  		break;
> > >  	
> > >  	case DMA_DEV_TO_MEM:
> > > -		src_burst = convert_burst(sconfig->src_maxburst);
> > > -		src_width = convert_buswidth(sconfig->src_addr_width);
> > > -		dst_burst = convert_burst(sconfig->dst_maxburst ?
> > > -					sconfig->dst_maxburst : 8);
> > > -		dst_width = convert_buswidth(sconfig->dst_addr_width !=
> > > -						DMA_SLAVE_BUSWIDTH_UNDEFINED ?
> > > -				sconfig->dst_addr_width :
> > > -				DMA_SLAVE_BUSWIDTH_4_BYTES);
> > > +		if (dst_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED)
> > > +			dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> > > +		dst_maxburst = dst_maxburst ? dst_maxburst : 8;
> > > 
> > >  		break;
> > >  	
> > >  	default:
> > >  		return -EINVAL;
> > >  	
> > >  	}
> > > 
> > > -	if (src_burst < 0)
> > > -		return src_burst;
> > > -	if (src_width < 0)
> > > -		return src_width;
> > > -	if (dst_burst < 0)
> > > -		return dst_burst;
> > > -	if (dst_width < 0)
> > > -		return dst_width;
> > > -
> > > -	*p_cfg = DMA_CHAN_CFG_SRC_BURST(src_burst) |
> > > -		DMA_CHAN_CFG_SRC_WIDTH(src_width) |
> > > -		DMA_CHAN_CFG_DST_BURST(dst_burst) |
> > > +	if (!(BIT(src_addr_width) & sdev->slave.src_addr_widths))
> > > +		return -EINVAL;
> > > +	if (!(BIT(dst_addr_width) & sdev->slave.dst_addr_widths))
> > > +		return -EINVAL;
> > > +	if (!(BIT(src_maxburst) & supported_burst_length))
> > > +		return -EINVAL;
> > > +	if (!(BIT(dst_maxburst) & supported_burst_length))
> > > +		return -EINVAL;
> > > +
> > > +	src_width = convert_buswidth(src_addr_width);
> > > +	dst_width = convert_buswidth(dst_addr_width);
> > > +	dst_burst = convert_burst(dst_maxburst);
> > > +	src_burst = convert_burst(src_maxburst);
> > 
> > I'm not sure what you're trying to do here. Could you split your patch
> > by logical change, this doesn't seem related to just supporting the
> > H3, but a heavier refactoring.
> 
> Untangling 3 independent steps - handling of DMA_SLAVE_BUSWIDTH_UNDEFINED, 
> range checking, and conversion to register value. As the valid ranges depend 
> on the controller, the code is much easier to read if the range check is done 
> first, and then the conversion.

Please make separate patches as well for the splitting up of each of
those steps.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Attachment: signature.asc
Description: PGP signature