Re: [RFC PATCH 2/3] dmaengine: add peripheral configuration

[Peter Ujfalusi]

Hi Vinod,

On 25/08/2020 10.10, Vinod Koul wrote:
>>>  /**
>>>   * struct dma_slave_config - dma slave channel runtime config
>>>   * @direction: whether the data shall go in or out on this slave
>>> @@ -418,6 +485,10 @@ enum dma_slave_buswidth {
>>>   * @slave_id: Slave requester id. Only valid for slave channels. The dma
>>>   * slave peripheral will have unique id as dma requester which need to be
>>>   * pass as slave config.
>>> + * @peripheral: type of peripheral to DMA to/from
>>> + * @set_config: set peripheral config
>>> + * @spi: peripheral config for spi
>>> + * @:i2c peripheral config for i2c
>>>   *
>>>   * This struct is passed in as configuration data to a DMA engine
>>>   * in order to set up a certain channel for DMA transport at runtime.
>>> @@ -443,6 +514,10 @@ struct dma_slave_config {
>>>  	u32 dst_port_window_size;
>>>  	bool device_fc;
>>>  	unsigned int slave_id;
>>> +	enum dmaengine_peripheral peripheral;
>>> +	u8 set_config;
>>> +	struct dmaengine_spi_config spi;
>>> +	struct dmaengine_i2c_config i2c;
>>
>> Would it be possible to reuse one of the existing feature already
>> supported by DMAengine?
>> We have DMA_PREP_CMD to give a command instead of a real transfer:
>> dmaengine_prep_slave_single(tx_chan, config_data, config_len,
>> 			    DMA_MEM_TO_DEV, DMA_PREP_CMD);
>> dmaengine_prep_slave_single(tx_chan, tx_buff, tx_len, DMA_MEM_TO_DEV,
>> 			    DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
>> dma_async_issue_pending(tx_chan);
>>
>> or the metadata support:
>> tx = dmaengine_prep_slave_single(tx_chan, tx_buff, tx_len,
>> 				 DMA_MEM_TO_DEV,
>> 				 DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
>> dmaengine_desc_attach_metadata(tx, config_data, config_len);
>> dma_async_issue_pending(tx_chan);
>>
>> By reading the driver itself, it is not clear if you always need to send
>> the config for TX, or only when the config is changing and what happens
>> if the first transfer (for SPI, since that is the only implemented one)
>> is RX, when you don't send config at all...
> 
> So this config is sent to driver everytime before the prep call (can be
> optimized to once if we have similar transfers in queue).

I see that you queue the TREs in the prep callback.

> This config is used to create the configuration passed to dmaengine
> which is used to actually program both dmaengine as well as peripheral
> registers (i2c/spi/etc), so we need a way to pass the spi/i2c config.

But do you need to send it with each DMA_MEM_TO_DEV or only once?
DMA_DEV_TO_MEM does not set the config, so I assume you must have one TX
to initialize the peripheral as the first transfer.

> I think prep cmd can be used to send this data, I do not see any issues
> with that, it would work if we want to go that route.

The only thing which might be an issue is that with the DMA_PREP_CMD the
config_data is dma_addr_t (via dmaengine_prep_slave_single).

> I did have a prototype with metadata but didnt work very well, the
> problem is it assumes metadata for tx/rx but here i send the data
> everytime from client data.

Yes, the intended use case for metadata (per descriptor!) is for
channels where each transfer might have different metadata needed for
the given transfer (tx/rx).

In your case you have semi static peripheral configuration data, which
is not really changing between transfers.

A compromise would be to add:
void *peripheral_config;
to the dma_slave_config, move the set_config inside of the device
specific struct you are passing from a client to the core?

>> I'm concerned about the size increase of dma_slave_config (it grows by
>>> 30 bytes) and for DMAs with hundreds of channels (UDMA) it will add up
>> to a sizeable amount.
> 
> I agree that is indeed a valid concern, that is the reason I tagged this
> as a RFC patch ;-)
> 
> I see the prep_cmd is a better approach for this, anyone else has better
> suggestions?
> 
> Thanks for looking in.
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki