Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [RFC PATCH 2/3] dmaengine: add peripheral configuration
To:     Vinod Koul <vkoul@kernel.org>, <dmaengine@vger.kernel.org>
CC:     Rob Herring <robh+dt@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        <linux-arm-msm@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
References: <20200824084712.2526079-1-vkoul@kernel.org>
 <20200824084712.2526079-3-vkoul@kernel.org>
From:   Peter Ujfalusi <peter.ujfalusi@ti.com>
Message-ID: <50ed780f-4c1a-2da2-71e4-423f3b224e25@ti.com>
Date:   Tue, 25 Aug 2020 09:52:07 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.11.0
MIME-Version: 1.0
In-Reply-To: <20200824084712.2526079-3-vkoul@kernel.org>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Vinod,

On 24/08/2020 11.47, 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
>=20
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>  include/linux/dmaengine.h | 75 +++++++++++++++++++++++++++++++++++++++=

>  1 file changed, 75 insertions(+)
>=20
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index 6fbd5c99e30c..264643ca9209 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -380,6 +380,73 @@ enum dma_slave_buswidth {
>  	DMA_SLAVE_BUSWIDTH_64_BYTES =3D 64,
>  };
> =20
> +/**
> + * enum spi_transfer_cmd - spi transfer commands
> + */
> +enum spi_transfer_cmd {
> +	SPI_TX =3D 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
> + * @rx_len: receive length for spi buffer
> + */
> +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;
> +	u32 rx_len;
> +};
> +
> +/**
> + * 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
> + * @strech: strech 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 strech;
> +	u8 op;
> +};
> +
> +enum dmaengine_peripheral {
> +	DMAENGINE_PERIPHERAL_SPI,
> +	DMAENGINE_PERIPHERAL_I2C,
> +	DMAENGINE_PERIPHERAL_UART,
> +	DMAENGINE_PERIPHERAL_LAST =3D DMAENGINE_PERIPHERAL_UART,
> +};
> +
>  /**
>   * struct dma_slave_config - dma slave channel runtime config
>   * @direction: whether the data shall go in or out on this slave
> @@ -418,6 +485,10 @@ enum dma_slave_buswidth {
>   * @slave_id: Slave requester id. Only valid for slave channels. The d=
ma
>   * slave peripheral will have unique id as dma requester which need to=
 be
>   * pass as slave config.
> + * @peripheral: type of peripheral to DMA to/from
> + * @set_config: set peripheral config
> + * @spi: peripheral config for spi
> + * @:i2c peripheral config for i2c
>   *
>   * 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 +514,10 @@ struct dma_slave_config {
>  	u32 dst_port_window_size;
>  	bool device_fc;
>  	unsigned int slave_id;
> +	enum dmaengine_peripheral peripheral;
> +	u8 set_config;
> +	struct dmaengine_spi_config spi;
> +	struct dmaengine_i2c_config i2c;

Would it be possible to reuse one of the existing feature already
supported by DMAengine?
We have DMA_PREP_CMD to give a command instead of a real transfer:
dmaengine_prep_slave_single(tx_chan, config_data, config_len,
			    DMA_MEM_TO_DEV, DMA_PREP_CMD);
dmaengine_prep_slave_single(tx_chan, tx_buff, tx_len, DMA_MEM_TO_DEV,
			    DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
dma_async_issue_pending(tx_chan);

or the metadata support:
tx =3D dmaengine_prep_slave_single(tx_chan, tx_buff, tx_len,
				 DMA_MEM_TO_DEV,
				 DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
dmaengine_desc_attach_metadata(tx, config_data, config_len);
dma_async_issue_pending(tx_chan);

By reading the driver itself, it is not clear if you always need to send
the config for TX, or only when the config is changing and what happens
if the first transfer (for SPI, since that is the only implemented one)
is RX, when you don't send config at all...

I'm concerned about the size increase of dma_slave_config (it grows by
>30 bytes) and for DMAs with hundreds of channels (UDMA) it will add up
to a sizeable amount.

>  };
> =20
>  /**
>=20

- P=C3=A9ter

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