Hi Vinod,

On 01/10/2020 14.23, Vinod Koul wrote:

Hi Peter,

On 29-09-20, 11:06, Peter Ujfalusi wrote:

quoted

quoted

+ * @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 ;-)

quoted

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'.

quoted

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