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
