Re: [PATCH v2 1/6] dma: Indicate residue granularity in dma_slave_caps

[Lars-Peter Clausen <lars@metafoo.de>]

On 01/09/2014 04:08 PM, Vinod Koul wrote:
> On Mon, Jan 06, 2014 at 11:45:57AM +0100, Lars-Peter Clausen wrote:
>> This patch adds a new field to the dma_slave_caps struct which indicates the
>> granularity with which the driver is able to update the residue field of the
>> dma_tx_state struct. Making this information available to dmaengine users allows
>> them to make better decisions on how to operate. E.g. for audio certain features
>> like wakeup less operation or timer based scheduling only make sense and work
>> correctly if the reported residue is fine-grained enough.
>>
>> Right now four different levels of granularity are supported:
>> 	* DESCRIPTOR: The DMA channel is only able to tell whether a descriptor has
>> 	  been completed or not, which means residue reporting is not supported by
>> 	  this channel. The residue field of the dma_tx_state field will always be
>> 	  0.
>> 	* SEGMENT: The DMA channel updates the residue field after each successfully
>> 	  completed segment of the transfer (For cyclic transfers this is after each
>> 	  period). This is typically implemented by having the hardware generate an
>> 	  interrupt after each transferred segment and then the drivers updates the
>> 	  outstanding residue by the size of the segment. Another possibility is if
>> 	  the hardware supports SG and the segment descriptor has a field which gets
>> 	  set after the segment has been completed. The driver then counts the
>> 	  number of segments without the flag set to compute the residue.
>> 	* BURST: The DMA channel updates the residue field after each transferred
>> 	  burst. This is typically only supported if the hardware has a progress
>> 	  register of some sort (E.g. a register with the current read/write address
>> 	  or a register with the amount of bursts/beats/bytes that have been
>> 	  transferred or still need to be transferred).
>>
>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>> ---
>> Changes since v1:
>> 	* Drop BYTE granularity level as there has been consent that it is not
>> 	  really useful.
>> ---
>>  include/linux/dmaengine.h | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
>> index ed92b30..74d5cb2 100644
>> --- a/include/linux/dmaengine.h
>> +++ b/include/linux/dmaengine.h
>> @@ -364,6 +364,19 @@ struct dma_slave_config {
>>  	unsigned int slave_id;
>>  };
>>  
>> +/**
>> + * enum dma_residue_granularity - Granularity of the reported transfer residue
>> + * @DMA_RESIDUE_GRANULATRITY_DESCRIPTOR: Residue reporting is not supported.
>> + * @DMA_RESIDUE_GRANULATRITY_SEGMENT: Residue is updated with each completed
>> + *  segment in the descriptor.
>> + * @DMA_RESIDUE_GRANULATRITY_BURST: Residue is updated with each transfered burst
>> + */
>> +enum dma_residue_granularity {
>> +	DMA_RESIDUE_GRANULARITY_DESCRIPTOR = 0,
>> +	DMA_RESIDUE_GRANULARITY_SEGMENT = 1,
>> +	DMA_RESIDUE_GRANULARITY_BURST = 2,
> This is too longish macro :) Cna we just have DMA_RESIDUE_XXXX. Do we really
> need granularity?

How about DMA_RSD_GRNLRTY? ;)

I'd prefer to keep the name as it is. In my opinion the intuitive meaning
changes completely if you leave out the "granularity".

> 
> Also segment is not too obvious, block is genrally used in DMA controllers for
> this, though not too relgious about it!
> 

I'd like to stick with the kernel terminology.

> Can you also add bit more detail in enum descriptions. Move a bit from patch
> description above :)

Ok.

> 
> 
>> +};
>> +
>>  /* struct dma_slave_caps - expose capabilities of a slave channel only
>>   *
>>   * @src_addr_widths: bit mask of src addr widths the channel supports
>> @@ -374,6 +387,7 @@ struct dma_slave_config {
>>   * 	should be checked by controller as well
>>   * @cmd_pause: true, if pause and thereby resume is supported
>>   * @cmd_terminate: true, if terminate cmd is supported
>> + * @residue_granularity: granularity of the reported transfer residue
>>   */
>>  struct dma_slave_caps {
>>  	u32 src_addr_widths;
>> @@ -381,6 +395,7 @@ struct dma_slave_caps {
>>  	u32 directions;
>>  	bool cmd_pause;
>>  	bool cmd_terminate;
>> +	enum dma_residue_granularity residue_granularity;
> enum dma_residue granularity; would be fine too...
> 
> --
> ~Vinod
> --
> To unsubscribe from this list: send the line "unsubscribe dmaengine" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>