| Message ID | 20200923063410.3431917-3-vkoul@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/3] dt-bindings: dmaengine: Document qcom,gpi dma binding | expand |
On 23/09/2020 9.34, Vinod Koul wrote: > 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(+) > > 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 *_pol_high ? > + * @pack_en: process tx/rx buffers as packed what does this mean? > + * @word_len: spi word length > + * @clk_div: source clock divider > + * @clk_src: serial clock we tend to use common clock framework for clock configuration? > + * @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; This is not documented and I can not really guess what it is. > + 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 Is this the same thing as with the spi? > + * @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; Not documented. Is it indicates multiple-byte read/write or multiple messages? > +}; > + > +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 Why is it part of the peripheral config? You get the buffer via prep_slave_sg(). Or is this something else? The GPI driver uses the rx_len for DMA_MEM_TO_DEV (tx) setup. > + * @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? 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; 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
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
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 -- ~Vinod
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(+) -- 2.26.2