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

[Peter Ujfalusi <peter.ujfalusi@ti.com>]

Hi Vinod,

On 29/09/2020 11.06, Peter Ujfalusi wrote:
> 
> I know that you want this to be as generic as much as it is possible,
> but do we really want to?
> GPIv2 will also handle I2S peripheral, other vendor's similar solution
> 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;

TI have an AASRC (Audio Asynchronous Sample Rate Converted) in j721e and
to configure the DMA side (AASRC_PDMA) we need special configuration
parameters passed from the AASRC driver to the DMA channel.
This peripheral config extension would be perfect for it, but the
parameters I would need is not generic in any ways.

The other thing which might need to be considered is to have src/dst
pair of this. When we do DMA_DEV_TO_DEV, it would help to figure out
which side we should apply which config (if you have the same type of
device on both ends with different 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.
> 
>> +};
>>  /**
>>   * struct dma_slave_config - dma slave channel runtime config
>>   * @direction: whether the data shall go in or out on this slave
>> @@ -418,6 +506,8 @@ 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: peripheral configuration for programming peripheral for
>> + * dmaengine transfer
>>   *
>>   * 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 +533,7 @@ struct dma_slave_config {
>>  	u32 dst_port_window_size;
>>  	bool device_fc;
>>  	unsigned int slave_id;
>> +	struct dmaengine_peripheral_config *peripheral;
>>  };
>>  
>>  /**
>>
> 
> - Péter
> 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 

- Péter

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