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

[Peter Ujfalusi]

Hi Vinod,

On 01/10/2020 14.23, Vinod Koul wrote:
> Hi Peter,
> 
> On 29-09-20, 11:06, Peter Ujfalusi wrote:
> 
>>> + * @spi: peripheral config for spi
>>> + * @i2c: peripheral config for i2c
>>> + */
>>> +struct dmaengine_peripheral_config {
>>> +	enum dmaengine_peripheral peripheral;
>>> +	u8 set_config;
>>> +	u32 rx_len;
>>> +	struct dmaengine_spi_config spi;
>>> +	struct dmaengine_i2c_config i2c;
>>
>> I know that you want this to be as generic as much as it is possible,
>> but do we really want to?
> 
> That is really a good question ;-)
> 
>> GPIv2 will also handle I2S peripheral, other vendor's similar solution
> 
> Not I2S, but yes but additional peripherals is always a question

Never underestimate the 'creativity'.

>> would require different sets of parameters unique to their IPs?
>>
>> How we are going to handle similar setups for DMA which is used for
>> networking, SPI/I2C/I2S/NAND/display/capture, etc?
>>
>> Imho these settings are really part of the peripheral's domain and not
>> the DMA. It is just a small detail that instead of direct register
>> writes, your setup is using the DMA descriptors to write.
>> It is similar to what I use as metadata (part of the descriptor belongs
>> and owned by the client driver).
>>
>> I think it would be better to have:
>>
>> enum dmaengine_peripheral {
>> 	DMAENGINE_PERIPHERAL_GPI_SPI = 1,
>> 	DMAENGINE_PERIPHERAL_GPI_UART,
>> 	DMAENGINE_PERIPHERAL_GPI_I2C,
>> 	DMAENGINE_PERIPHERAL_XYZ_SPI,
>> 	DMAENGINE_PERIPHERAL_XYZ_AASRC,
>> 	DMAENGINE_PERIPHERAL_ABC_CAM,
>> 	...
>> 	DMAENGINE_PERIPHERAL_LAST,
>> };
>>
>> enum dmaengine_peripheral peripheral_type;
>> void *peripheral_config;
>>
>>
>> and that's it. The set_config is specific to GPI.
>> It can be debated where the structs should be defined, in the generic
>> dmaengine.h or in include/linux/dma/ as controller specific
>> (gpi_peripheral.h) or a generic one, like dmaengine_peripheral.h
>>
>> The SPI/I2C/UART client of yours would pass the GPI specific struct as
>> in any case it has to know what is the DMA it is serviced by.
> 
> If we want to take that approach, I can actually move the whole logic of
> creating the specific TREs from DMA to clients and they pass on TRE
> values and driver adds to ring after appending DMA TREs

The drawback is that you are tying the driver to a specific DMA with
directly giving the TREs. If the TRE (or other method) is used by a
newer device you need to work on both sides.

> Question is how should this interface look like? reuse metadata or add a
> new API which sets the txn specific data (void pointer and size) to the
> txn.. 

It depends which is best for the use case.
I see the metadata useful when you need to send different
metadata/configuration with each transfer.
It can be also useful when you need it seldom, but for your use case and
setup the dma_slave_config extended with

enum dmaengine_peripheral peripheral_type;
void *peripheral_config;

would be a bit more explicit.

I would then deal with the peripheral config in this way:
when the DMA driver's device_config is called, I would take the
parameters and set a flag that the config needs to be processed as it
has changed.
In the next prep_slave_sg() then I would prepare the TREs with the
config and clear the flag that the next transfer does not need the
configuration anymore.

In this way each dmaengine_slave_config() will trigger at the next
prep_slave_sg time configuration update for the peripheral to be
included in the TREs.
The set_config would be internal to the DMA driver, clients just need to
update the configuration when they need to and everything is taken care of.

- Péter

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