| Message ID | 20200923063410.3431917-3-vkoul@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dmaengine: Add support for QCOM GSI dma controller | expand |
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 > 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 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.. -- ~Vinod
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
Hi Vinod, On 07/10/2020 14.28, Vinod Koul wrote: > Hi Peter, > > On 02-10-20, 11:48, Peter Ujfalusi wrote: > >> 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. > > Ok I am going to drop the dmaengine_peripheral and make > peripheral_config as as you proposed. > > So will add following to dma_slave_config: > void *peripheral_config; > > Driver can define the config they would like and use. > > We can eventually look at common implementations and try to unify once > we have more users Sound good to me! - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h index 6fbd5c99e30c..bbc32271ad7f 100644 --- a/include/linux/dmaengine.h +++ b/include/linux/dmaengine.h @@ -380,6 +380,94 @@ enum dma_slave_buswidth { DMA_SLAVE_BUSWIDTH_64_BYTES = 64, }; +/** + * enum spi_transfer_cmd - spi transfer commands + */ +enum spi_transfer_cmd { + SPI_TX = 1, + SPI_RX, + SPI_DUPLEX, +}; + +/** + * struct dmaengine_spi_config - spi config for peripheral + * + * @loopback_en: spi loopback enable when set + * @clock_pol: clock polarity + * @data_pol: data polarity + * @pack_en: process tx/rx buffers as packed + * @word_len: spi word length + * @clk_div: source clock divider + * @clk_src: serial clock + * @cmd: spi cmd + * @cs: chip select toggle + */ +struct dmaengine_spi_config { + u8 loopback_en; + u8 clock_pol; + u8 data_pol; + u8 pack_en; + u8 word_len; + u32 clk_div; + u32 clk_src; + u8 fragmentation; + enum spi_transfer_cmd cmd; + u8 cs; +}; + +enum i2c_op { + I2C_WRITE = 1, + I2C_READ, +}; + +/** + * struct dmaengine_i2c_config - i2c config for peripheral + * + * @pack_enable: process tx/rx buffers as packed + * @cycle_count: clock cycles to be sent + * @high_count: high period of clock + * @low_count: low period of clock + * @clk_div: source clock divider + * @addr: i2c bus address + * @stretch: stretch the clock at eot + * @op: i2c cmd + */ +struct dmaengine_i2c_config { + u8 pack_enable; + u8 cycle_count; + u8 high_count; + u8 low_count; + u16 clk_div; + u8 addr; + u8 stretch; + enum i2c_op op; + bool multi_msg; +}; + +enum dmaengine_peripheral { + DMAENGINE_PERIPHERAL_SPI = 1, + DMAENGINE_PERIPHERAL_UART = 2, + DMAENGINE_PERIPHERAL_I2C = 3, + DMAENGINE_PERIPHERAL_LAST = DMAENGINE_PERIPHERAL_I2C, +}; + +/** + * struct dmaengine_peripheral_config - peripheral configuration for + * dmaengine peripherals + * + * @peripheral: type of peripheral to DMA to/from + * @set_config: set peripheral config + * @rx_len: receive length for buffer + * @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; +}; /** * 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; }; /**
Some complex dmaengine controllers have capability to program the peripheral device, so pass on the peripheral configuration as part of dma_slave_config Signed-off-by: Vinod Koul <vkoul@kernel.org> --- include/linux/dmaengine.h | 91 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 91 insertions(+)