Date:   Tue, 21 Mar 2023 22:27:27 +0300
From:   Serge Semin <fancer.lancer@gmail.com>
To:     Joy Chakraborty <joychakr@google.com>
Cc:     Mark Brown <broonie@kernel.org>, linux-spi@vger.kernel.org,
        linux-kernel@vger.kernel.org, manugautam@google.com,
        rohitner@google.com
Subject: Re: [PATCH v2] spi: dw: Add 32 bpw support to DW DMA Controller
Message-ID: <20230321192727.2ts4oabwiktblis3@mobilestation>
References: <20230321115843.2688472-1-joychakr@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230321115843.2688472-1-joychakr@google.com>
Precedence: bulk

On Tue, Mar 21, 2023 at 11:58:43AM +0000, Joy Chakraborty wrote:
> If DW Controller is capable of a maximum of 32 bits per word then SW or
> DMA controller has to write up to 32bit or 4byte data to the FIFO at a
> time.
> 

> This Patch adds support for AxSize = 4 bytes configuration from dw dma

* sorry for referring to the newbie-doc, but please note:
https://elixir.bootlin.com/linux/v6.3-rc1/source/Documentation/process/submitting-patches.rst#L77
and
https://elixir.bootlin.com/linux/v6.3-rc1/source/Documentation/process/submitting-patches.rst#L94

> driver if n_bytes i.e. number of bytes per write to fifo is 3 or 4.
> It also checks to see if the dma controller is capable of satisfying the
> width requirement to achieve a particular bits/word requirement per
> transfer.
> 
> Signed-off-by: Joy Chakraborty <joychakr@google.com>
> ---
>  drivers/spi/spi-dw-dma.c | 37 ++++++++++++++++++++++++++++++++-----
>  drivers/spi/spi-dw.h     |  1 +
>  2 files changed, 33 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/spi/spi-dw-dma.c b/drivers/spi/spi-dw-dma.c
> index ababb910b391..9ac3a1c25e2d 100644
> --- a/drivers/spi/spi-dw-dma.c
> +++ b/drivers/spi/spi-dw-dma.c
> @@ -23,6 +23,8 @@
>  #define DW_SPI_TX_BUSY		1
>  #define DW_SPI_TX_BURST_LEVEL	16
>  
> +static inline enum dma_slave_buswidth dw_spi_dma_convert_width(u8 n_bytes);
> +
>  static bool dw_spi_dma_chan_filter(struct dma_chan *chan, void *param)
>  {
>  	struct dw_dma_slave *s = param;
> @@ -89,6 +91,16 @@ static void dw_spi_dma_sg_burst_init(struct dw_spi *dws)
>  		dws->dma_sg_burst = 0;
>  }
>  
> +static void dw_spi_dma_addr_widths_init(struct dw_spi *dws)
> +{
> +	struct dma_slave_caps tx = {0}, rx = {0};
> +

> +	dma_get_slave_caps(dws->txchan, &tx);
> +	dma_get_slave_caps(dws->rxchan, &rx);

Even though in this case any dma_get_slave_caps() failure will
effectively disable the DMA-based transfers, in general it would be
useful to have the dma_get_slave_caps() return value checked and halt
further DMA-init in case if it's not zero. In addition to that if the
Tx/Rx DMA device doesn't have the DMA_SLAVE capability or DEV2MEM and
MEM2DEV direction specified the DMA device won't be suitable for
SPI-ing. So further DMA-initialization are pointless in that case too.
It's just a general note not obligating or requesting anything since
the respective update should have been done in a separate patch
anyway.

> +

> +	dws->dma_addr_widths = tx.dst_addr_widths & rx.src_addr_widths;

Hm, in general the addr-width capabilities can mismatch. But it's very
much unlikely since both DMA channels normally belong to the same
controller. So I guess we can live with the suggested approach for now
but please add a comment above that line about the
assumption/limitation it implies.

> +}
> +
>  static int dw_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws)
>  {
>  	struct dw_dma_slave dma_tx = { .dst_id = 1 }, *tx = &dma_tx;
> @@ -128,6 +140,8 @@ static int dw_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws)
>  
>  	dw_spi_dma_sg_burst_init(dws);
>  
> +	dw_spi_dma_addr_widths_init(dws);
> +
>  	pci_dev_put(dma_dev);
>  
>  	return 0;
> @@ -167,6 +181,8 @@ static int dw_spi_dma_init_generic(struct device *dev, struct dw_spi *dws)
>  
>  	dw_spi_dma_sg_burst_init(dws);
>  
> +	dw_spi_dma_addr_widths_init(dws);
> +
>  	return 0;
>  
>  free_rxchan:
> @@ -202,18 +218,29 @@ static bool dw_spi_can_dma(struct spi_controller *master,
>  			   struct spi_device *spi, struct spi_transfer *xfer)
>  {
>  	struct dw_spi *dws = spi_controller_get_devdata(master);

> +	enum dma_slave_buswidth dma_bus_width;
>  
> -	return xfer->len > dws->fifo_len;
> +	if (xfer->len > dws->fifo_len) {
> +		dma_bus_width = dw_spi_dma_convert_width(dws->n_bytes);
> +		if (dws->dma_addr_widths & BIT(dma_bus_width))
> +			return true;
> +	}
< newline would have been nice, but...
> +	return false;

on the other hand a level of indentation could be decreased like this:

+	enum dma_slave_buswidth width;
+
+	if (xfer->len <= dws->fifo_len)
+		return false;
+
+	width = dw_spi_dma_convert_width(dws->n_bytes);
+
+	return !!(dws->dma_addr_widths & BIT(width));

-Serge(y)

>  }
>  
>  static enum dma_slave_buswidth dw_spi_dma_convert_width(u8 n_bytes)
>  {
> -	if (n_bytes == 1)
> +	switch (n_bytes) {
> +	case 1:
>  		return DMA_SLAVE_BUSWIDTH_1_BYTE;
> -	else if (n_bytes == 2)
> +	case 2:
>  		return DMA_SLAVE_BUSWIDTH_2_BYTES;
> -
> -	return DMA_SLAVE_BUSWIDTH_UNDEFINED;
> +	case 3:
> +	case 4:
> +		return DMA_SLAVE_BUSWIDTH_4_BYTES;
> +	default:
> +		return DMA_SLAVE_BUSWIDTH_UNDEFINED;
> +	}
>  }
>  
>  static int dw_spi_dma_wait(struct dw_spi *dws, unsigned int len, u32 speed)
> diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
> index 9e8eb2b52d5c..3962e6dcf880 100644
> --- a/drivers/spi/spi-dw.h
> +++ b/drivers/spi/spi-dw.h
> @@ -190,6 +190,7 @@ struct dw_spi {
>  	struct dma_chan		*rxchan;
>  	u32			rxburst;
>  	u32			dma_sg_burst;
> +	u32			dma_addr_widths;
>  	unsigned long		dma_chan_busy;
>  	dma_addr_t		dma_addr; /* phy address of the Data register */
>  	const struct dw_spi_dma_ops *dma_ops;
> -- 
> 2.40.0.rc1.284.g88254d51c5-goog
>