[PATCH v8 0/5] spi: dw: DW SPI DMA Driver updates

[PATCH v8 0/5] spi: dw: DW SPI DMA Driver updates

[Joy Chakraborty]

This Patch series adds support for 32 bits per word trasfers using DMA
and some defensive checks around dma controller capabilities.
---
V1 Changes : Add support for AxSize=4 bytes to support 32bits/word.
---
V1->V2 Changes : Add dma capability check to make sure address widths
are supported.
---
V2->V3 Changes : Split changes , add DMA direction check and other
cosmetic chnages.
---
V3->V4 Changes : Fix Sparce Warning
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303270715.w9sMJhIh-lkp@intel.com/
---
V4->V5 Changes : Preserve reverse xmas Tree order, move direction
check before initalisation of further capabilities, remove zero
initialisations, remove error OR'ing.
---
V5->V6 Changes :
	-Remove case of n_bytes=3 using 4_bytes buswidth
	-Avoid forward decaration
	-Break capability check patch into 2
	-round n_bytes to power of 2 ( Bug Fix)
	-Add more explanation in commit text.
---
V6->V7 Changes : Remove extra spaces, refer to functions in commit as
func()
---

Joy Chakraborty (5):
  spi: dw: Add 32 bpw support to SPI DW DMA driver
  spi: dw: Move dw_spi_can_dma()
  spi: dw: Add DMA directional capability check
  spi: dw: Add DMA address widths capability check
  spi: dw: Round of n_bytes to power of 2

 drivers/spi/spi-dw-core.c |  2 +-
 drivers/spi/spi-dw-dma.c  | 76 +++++++++++++++++++++++++++++----------
 drivers/spi/spi-dw.h      |  1 +
 3 files changed, 60 insertions(+), 19 deletions(-)

-- 
2.40.0.634.g4ca3ef3211-goog

[PATCH v8 1/5] spi: dw: Add 32 bpw support to SPI DW DMA driver

[Joy Chakraborty]

Add Support for AxSize = 4 bytes configuration from dw dma driver if
n_bytes i.e. number of bytes per write to fifo is 4.

Number of bytes written to fifo per write is depended on the bits/word
configuration being used which the DW core driver translates to n_bytes.
Hence, for bits per word values between 17 and 32 n_bytes should be
equal to 4.

Signed-off-by: Joy Chakraborty <joychakr@google.com>
---
 drivers/spi/spi-dw-dma.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-dw-dma.c b/drivers/spi/spi-dw-dma.c
index ababb910b391..c1b42cb59965 100644
--- a/drivers/spi/spi-dw-dma.c
+++ b/drivers/spi/spi-dw-dma.c
@@ -208,12 +208,16 @@ static bool dw_spi_can_dma(struct spi_controller *master,
 
 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 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)
-- 
2.40.0.634.g4ca3ef3211-goog

[PATCH v8 2/5] spi: dw: Move dw_spi_can_dma()

[Joy Chakraborty]

Move dw_spi_can_dma() implementation below dw_spi_dma_convert_width()
for handing compile dependency in future patches.

Signed-off-by: Joy Chakraborty <joychakr@google.com>
---
 drivers/spi/spi-dw-dma.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/spi-dw-dma.c b/drivers/spi/spi-dw-dma.c
index c1b42cb59965..f19c092920a1 100644
--- a/drivers/spi/spi-dw-dma.c
+++ b/drivers/spi/spi-dw-dma.c
@@ -198,14 +198,6 @@ static irqreturn_t dw_spi_dma_transfer_handler(struct dw_spi *dws)
 	return IRQ_HANDLED;
 }
 
-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);
-
-	return xfer->len > dws->fifo_len;
-}
-
 static enum dma_slave_buswidth dw_spi_dma_convert_width(u8 n_bytes)
 {
 	switch (n_bytes) {
@@ -220,6 +212,14 @@ static enum dma_slave_buswidth dw_spi_dma_convert_width(u8 n_bytes)
 	}
 }
 
+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);
+
+	return xfer->len > dws->fifo_len;
+}
+
 static int dw_spi_dma_wait(struct dw_spi *dws, unsigned int len, u32 speed)
 {
 	unsigned long long ms;
-- 
2.40.0.634.g4ca3ef3211-goog

[PATCH v8 3/5] spi: dw: Add DMA directional capability check

[Joy Chakraborty]

Check capabilities of DMA controller during init to make sure it is
capable of handling MEM2DEV for tx channel, DEV2MEM for rx channel.

Current DW DMA driver requires both tx and rx channel to be configured
and functional for any kind of transfers to take effect including
half duplex. Hence, check for both tx and rx direction and fail on
unavailbility of either.

Signed-off-by: Joy Chakraborty <joychakr@google.com>
---
 drivers/spi/spi-dw-dma.c | 39 ++++++++++++++++++++++++++++++---------
 1 file changed, 30 insertions(+), 9 deletions(-)

diff --git a/drivers/spi/spi-dw-dma.c b/drivers/spi/spi-dw-dma.c
index f19c092920a1..22d0727a3789 100644
--- a/drivers/spi/spi-dw-dma.c
+++ b/drivers/spi/spi-dw-dma.c
@@ -72,12 +72,22 @@ static void dw_spi_dma_maxburst_init(struct dw_spi *dws)
 	dw_writel(dws, DW_SPI_DMATDLR, dws->txburst);
 }
 
-static void dw_spi_dma_sg_burst_init(struct dw_spi *dws)
+static int dw_spi_dma_caps_init(struct dw_spi *dws)
 {
-	struct dma_slave_caps tx = {0}, rx = {0};
+	struct dma_slave_caps tx, rx;
+	int ret;
+
+	ret = dma_get_slave_caps(dws->txchan, &tx);
+	if (ret)
+		return ret;
+
+	ret = dma_get_slave_caps(dws->rxchan, &rx);
+	if (ret)
+		return ret;
 
-	dma_get_slave_caps(dws->txchan, &tx);
-	dma_get_slave_caps(dws->rxchan, &rx);
+	if (!(tx.directions & BIT(DMA_MEM_TO_DEV) &&
+	      rx.directions & BIT(DMA_DEV_TO_MEM)))
+		return -ENXIO;
 
 	if (tx.max_sg_burst > 0 && rx.max_sg_burst > 0)
 		dws->dma_sg_burst = min(tx.max_sg_burst, rx.max_sg_burst);
@@ -95,6 +105,7 @@ static int dw_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws)
 	struct dw_dma_slave dma_rx = { .src_id = 0 }, *rx = &dma_rx;
 	struct pci_dev *dma_dev;
 	dma_cap_mask_t mask;
+	int ret = -EBUSY;
 
 	/*
 	 * Get pci device for DMA controller, currently it could only
@@ -124,20 +135,25 @@ static int dw_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws)
 
 	init_completion(&dws->dma_completion);
 
-	dw_spi_dma_maxburst_init(dws);
+	ret = dw_spi_dma_caps_init(dws);
+	if (ret)
+		goto free_txchan;
 
-	dw_spi_dma_sg_burst_init(dws);
+	dw_spi_dma_maxburst_init(dws);
 
 	pci_dev_put(dma_dev);
 
 	return 0;
 
+free_txchan:
+	dma_release_channel(dws->txchan);
+	dws->txchan = NULL;
 free_rxchan:
 	dma_release_channel(dws->rxchan);
 	dws->rxchan = NULL;
 err_exit:
 	pci_dev_put(dma_dev);
-	return -EBUSY;
+	return ret;
 }
 
 static int dw_spi_dma_init_generic(struct device *dev, struct dw_spi *dws)
@@ -163,12 +179,17 @@ static int dw_spi_dma_init_generic(struct device *dev, struct dw_spi *dws)
 
 	init_completion(&dws->dma_completion);
 
-	dw_spi_dma_maxburst_init(dws);
+	ret = dw_spi_dma_caps_init(dws);
+	if (ret)
+		goto free_txchan;
 
-	dw_spi_dma_sg_burst_init(dws);
+	dw_spi_dma_maxburst_init(dws);
 
 	return 0;
 
+free_txchan:
+	dma_release_channel(dws->txchan);
+	dws->txchan = NULL;
 free_rxchan:
 	dma_release_channel(dws->rxchan);
 	dws->rxchan = NULL;
-- 
2.40.0.634.g4ca3ef3211-goog

[PATCH v8 4/5] spi: dw: Add DMA address widths capability check

[Joy Chakraborty]

Store address width capabilities of DMA controller during init and check
the same per transfer to make sure the bits/word requirement can be met.

Current DW DMA driver requires both tx and rx channel to be configured
and functional hence a subset of both tx and rx channel address width
capability is checked with the width requirement(n_bytes) for a
transfer.

Signed-off-by: Joy Chakraborty <joychakr@google.com>
---
 drivers/spi/spi-dw-dma.c | 17 ++++++++++++++++-
 drivers/spi/spi-dw.h     |  1 +
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-dw-dma.c b/drivers/spi/spi-dw-dma.c
index 22d0727a3789..df819652901a 100644
--- a/drivers/spi/spi-dw-dma.c
+++ b/drivers/spi/spi-dw-dma.c
@@ -97,6 +97,15 @@ static int dw_spi_dma_caps_init(struct dw_spi *dws)
 		dws->dma_sg_burst = rx.max_sg_burst;
 	else
 		dws->dma_sg_burst = 0;
+
+	/*
+	 * Assuming both channels belong to the same DMA controller hence the
+	 * peripheral side address width capabilities most likely would be
+	 * the same.
+	 */
+	dws->dma_addr_widths = tx.dst_addr_widths & rx.src_addr_widths;
+
+	return 0;
 }
 
 static int dw_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws)
@@ -237,8 +246,14 @@ 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;
+
+	if (xfer->len <= dws->fifo_len)
+		return false;
+
+	dma_bus_width = dw_spi_dma_convert_width(dws->n_bytes);
 
-	return xfer->len > dws->fifo_len;
+	return dws->dma_addr_widths & BIT(dma_bus_width);
 }
 
 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.634.g4ca3ef3211-goog

[PATCH v8 5/5] spi: dw: Round of n_bytes to power of 2

[Joy Chakraborty]

n_bytes variable in the driver represents the number of bytes per word
that needs to be sent/copied to fifo. Bits/word can be between 8 and 32
bits from the client but in memory they are a power of 2, same is mentioned
in spi.h header:
"
 * @bits_per_word: Data transfers involve one or more words; word sizes
 *	like eight or 12 bits are common.  In-memory wordsizes are
 *	powers of two bytes (e.g. 20 bit samples use 32 bits).
 *	This may be changed by the device's driver, or left at the
 *	default (0) indicating protocol words are eight bit bytes.
 *	The spi_transfer.bits_per_word can override this for each transfer.
"

Hence, round of n_bytes to a power of 2 to avoid values like 3 which
would generate unalligned/odd accesses to memory/fifo.

Fixes: a51acc2400d4 ("spi: dw: Add support for 32-bits max xfer size")
Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Joy Chakraborty <joychakr@google.com>
---
 drivers/spi/spi-dw-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
index c3bfb6c84cab..a6486db46c61 100644
--- a/drivers/spi/spi-dw-core.c
+++ b/drivers/spi/spi-dw-core.c
@@ -426,7 +426,7 @@ static int dw_spi_transfer_one(struct spi_controller *master,
 	int ret;
 
 	dws->dma_mapped = 0;
-	dws->n_bytes = DIV_ROUND_UP(transfer->bits_per_word, BITS_PER_BYTE);
+	dws->n_bytes = roundup_pow_of_two(DIV_ROUND_UP(transfer->bits_per_word, BITS_PER_BYTE));
 	dws->tx = (void *)transfer->tx_buf;
 	dws->tx_len = transfer->len / dws->n_bytes;
 	dws->rx = transfer->rx_buf;
-- 
2.40.0.634.g4ca3ef3211-goog

Re: [PATCH v8 5/5] spi: dw: Round of n_bytes to power of 2

[Serge Semin]

On Thu, Apr 20, 2023 at 05:51:31AM +0000, Joy Chakraborty wrote:
> n_bytes variable in the driver represents the number of bytes per word
> that needs to be sent/copied to fifo. Bits/word can be between 8 and 32
> bits from the client but in memory they are a power of 2, same is mentioned
> in spi.h header:
> "
>  * @bits_per_word: Data transfers involve one or more words; word sizes
>  *	like eight or 12 bits are common.  In-memory wordsizes are
>  *	powers of two bytes (e.g. 20 bit samples use 32 bits).
>  *	This may be changed by the device's driver, or left at the
>  *	default (0) indicating protocol words are eight bit bytes.
>  *	The spi_transfer.bits_per_word can override this for each transfer.
> "
> 
> Hence, round of n_bytes to a power of 2 to avoid values like 3 which
> would generate unalligned/odd accesses to memory/fifo.
> 
> Fixes: a51acc2400d4 ("spi: dw: Add support for 32-bits max xfer size")
> Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> Signed-off-by: Joy Chakraborty <joychakr@google.com>
> ---
>  drivers/spi/spi-dw-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> index c3bfb6c84cab..a6486db46c61 100644
> --- a/drivers/spi/spi-dw-core.c
> +++ b/drivers/spi/spi-dw-core.c
> @@ -426,7 +426,7 @@ static int dw_spi_transfer_one(struct spi_controller *master,
>  	int ret;
>  
>  	dws->dma_mapped = 0;

> -	dws->n_bytes = DIV_ROUND_UP(transfer->bits_per_word, BITS_PER_BYTE);
> +	dws->n_bytes = roundup_pow_of_two(DIV_ROUND_UP(transfer->bits_per_word, BITS_PER_BYTE));

Almost 100 symbols looks too bulky. Moreover single-lined nested call
makes things a bit harder to read. What about formatting it up like
this?

	dws->n_bytes =
		roundup_pow_of_two(DIV_ROUND_UP(transfer->bits_per_word,
						BITS_PER_BYTE));

	or like this:

	dws->n_bytes = roundup_pow_of_two(DIV_ROUND_UP(transfer->bits_per_word,
						       BITS_PER_BYTE));

Splitting the line into chunks will simplify the visual
differentiation between the outer and inner calls.

* Note even though the 80-char columns limit isn't that strict rule
now, but it's still preferable unless exceeding the limit significantly
increases readability. The update you suggest doesn't seem like the case
which would improve the readability.

-Serge(y)

>  	dws->tx = (void *)transfer->tx_buf;
>  	dws->tx_len = transfer->len / dws->n_bytes;
>  	dws->rx = transfer->rx_buf;
> -- 
> 2.40.0.634.g4ca3ef3211-goog
> 

Re: [PATCH v8 0/5] spi: dw: DW SPI DMA Driver updates

[Serge Semin]

On Thu, Apr 20, 2023 at 05:51:26AM +0000, Joy Chakraborty wrote:
> This Patch series adds support for 32 bits per word trasfers using DMA
> and some defensive checks around dma controller capabilities.

The series looks good to me except a small nitpick described in the
patch 5. Just a note for future patchset it's preferable to have the
fixes-patches placed at the head of the series thus it would minimize
a possible to catch merge-conflicts on the patches backporting. In
case of your fixes patch it won't be relevant since the change is
independent from the rest of the series updates.

So feel free to add the tags:
Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
Tested-by: Serge Semin <fancer.lancer@gmail.com>
* tested on Baikal-T1 based system with DW SPI-looped back interface
transferring a chunk of data with DFS:8,12,16.

Note before moving further we'll need to wait for @Andy response.

-Serge(y)

> ---
> V1 Changes : Add support for AxSize=4 bytes to support 32bits/word.
> ---
> V1->V2 Changes : Add dma capability check to make sure address widths
> are supported.
> ---
> V2->V3 Changes : Split changes , add DMA direction check and other
> cosmetic chnages.
> ---
> V3->V4 Changes : Fix Sparce Warning
> | Reported-by: kernel test robot <lkp@intel.com>
> | Link: https://lore.kernel.org/oe-kbuild-all/202303270715.w9sMJhIh-lkp@intel.com/
> ---
> V4->V5 Changes : Preserve reverse xmas Tree order, move direction
> check before initalisation of further capabilities, remove zero
> initialisations, remove error OR'ing.
> ---
> V5->V6 Changes :
> 	-Remove case of n_bytes=3 using 4_bytes buswidth
> 	-Avoid forward decaration
> 	-Break capability check patch into 2
> 	-round n_bytes to power of 2 ( Bug Fix)
> 	-Add more explanation in commit text.
> ---
> V6->V7 Changes : Remove extra spaces, refer to functions in commit as
> func()
> ---
> 
> Joy Chakraborty (5):
>   spi: dw: Add 32 bpw support to SPI DW DMA driver
>   spi: dw: Move dw_spi_can_dma()
>   spi: dw: Add DMA directional capability check
>   spi: dw: Add DMA address widths capability check
>   spi: dw: Round of n_bytes to power of 2
> 
>  drivers/spi/spi-dw-core.c |  2 +-
>  drivers/spi/spi-dw-dma.c  | 76 +++++++++++++++++++++++++++++----------
>  drivers/spi/spi-dw.h      |  1 +
>  3 files changed, 60 insertions(+), 19 deletions(-)
> 
> -- 
> 2.40.0.634.g4ca3ef3211-goog
> 

Re: [PATCH v8 5/5] spi: dw: Round of n_bytes to power of 2

[Joy Chakraborty]

On Fri, Apr 21, 2023 at 2:23 PM Serge Semin <fancer.lancer@gmail.com> wrote:
>
> On Thu, Apr 20, 2023 at 05:51:31AM +0000, Joy Chakraborty wrote:
> > n_bytes variable in the driver represents the number of bytes per word
> > that needs to be sent/copied to fifo. Bits/word can be between 8 and 32
> > bits from the client but in memory they are a power of 2, same is mentioned
> > in spi.h header:
> > "
> >  * @bits_per_word: Data transfers involve one or more words; word sizes
> >  *    like eight or 12 bits are common.  In-memory wordsizes are
> >  *    powers of two bytes (e.g. 20 bit samples use 32 bits).
> >  *    This may be changed by the device's driver, or left at the
> >  *    default (0) indicating protocol words are eight bit bytes.
> >  *    The spi_transfer.bits_per_word can override this for each transfer.
> > "
> >
> > Hence, round of n_bytes to a power of 2 to avoid values like 3 which
> > would generate unalligned/odd accesses to memory/fifo.
> >
> > Fixes: a51acc2400d4 ("spi: dw: Add support for 32-bits max xfer size")
> > Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> > Signed-off-by: Joy Chakraborty <joychakr@google.com>
> > ---
> >  drivers/spi/spi-dw-core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> > index c3bfb6c84cab..a6486db46c61 100644
> > --- a/drivers/spi/spi-dw-core.c
> > +++ b/drivers/spi/spi-dw-core.c
> > @@ -426,7 +426,7 @@ static int dw_spi_transfer_one(struct spi_controller *master,
> >       int ret;
> >
> >       dws->dma_mapped = 0;
>
> > -     dws->n_bytes = DIV_ROUND_UP(transfer->bits_per_word, BITS_PER_BYTE);
> > +     dws->n_bytes = roundup_pow_of_two(DIV_ROUND_UP(transfer->bits_per_word, BITS_PER_BYTE));
>
> Almost 100 symbols looks too bulky. Moreover single-lined nested call
> makes things a bit harder to read. What about formatting it up like
> this?
>
>         dws->n_bytes =
>                 roundup_pow_of_two(DIV_ROUND_UP(transfer->bits_per_word,
>                                                 BITS_PER_BYTE));
>
>         or like this:
>
>         dws->n_bytes = roundup_pow_of_two(DIV_ROUND_UP(transfer->bits_per_word,
>                                                        BITS_PER_BYTE));
>
> Splitting the line into chunks will simplify the visual
> differentiation between the outer and inner calls.
>
> * Note even though the 80-char columns limit isn't that strict rule
> now, but it's still preferable unless exceeding the limit significantly
> increases readability. The update you suggest doesn't seem like the case
> which would improve the readability.
>

Sure, I can make the following change in the formatting and send the
patch series:
         dws->n_bytes =
                 roundup_pow_of_two(DIV_ROUND_UP(transfer->bits_per_word,
                                                 BITS_PER_BYTE));

Thanks
Joy
> -Serge(y)
>
> >       dws->tx = (void *)transfer->tx_buf;
> >       dws->tx_len = transfer->len / dws->n_bytes;
> >       dws->rx = transfer->rx_buf;
> > --
> > 2.40.0.634.g4ca3ef3211-goog
> >

Re: [PATCH v8 5/5] spi: dw: Round of n_bytes to power of 2

[Serge Semin]

On Fri, Apr 21, 2023 at 02:51:58PM +0530, Joy Chakraborty wrote:
> On Fri, Apr 21, 2023 at 2:23 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> >
> > On Thu, Apr 20, 2023 at 05:51:31AM +0000, Joy Chakraborty wrote:
> > > n_bytes variable in the driver represents the number of bytes per word
> > > that needs to be sent/copied to fifo. Bits/word can be between 8 and 32
> > > bits from the client but in memory they are a power of 2, same is mentioned
> > > in spi.h header:
> > > "
> > >  * @bits_per_word: Data transfers involve one or more words; word sizes
> > >  *    like eight or 12 bits are common.  In-memory wordsizes are
> > >  *    powers of two bytes (e.g. 20 bit samples use 32 bits).
> > >  *    This may be changed by the device's driver, or left at the
> > >  *    default (0) indicating protocol words are eight bit bytes.
> > >  *    The spi_transfer.bits_per_word can override this for each transfer.
> > > "
> > >
> > > Hence, round of n_bytes to a power of 2 to avoid values like 3 which
> > > would generate unalligned/odd accesses to memory/fifo.
> > >
> > > Fixes: a51acc2400d4 ("spi: dw: Add support for 32-bits max xfer size")
> > > Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> > > Signed-off-by: Joy Chakraborty <joychakr@google.com>
> > > ---
> > >  drivers/spi/spi-dw-core.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> > > index c3bfb6c84cab..a6486db46c61 100644
> > > --- a/drivers/spi/spi-dw-core.c
> > > +++ b/drivers/spi/spi-dw-core.c
> > > @@ -426,7 +426,7 @@ static int dw_spi_transfer_one(struct spi_controller *master,
> > >       int ret;
> > >
> > >       dws->dma_mapped = 0;
> >
> > > -     dws->n_bytes = DIV_ROUND_UP(transfer->bits_per_word, BITS_PER_BYTE);
> > > +     dws->n_bytes = roundup_pow_of_two(DIV_ROUND_UP(transfer->bits_per_word, BITS_PER_BYTE));
> >
> > Almost 100 symbols looks too bulky. Moreover single-lined nested call
> > makes things a bit harder to read. What about formatting it up like
> > this?
> >
> >         dws->n_bytes =
> >                 roundup_pow_of_two(DIV_ROUND_UP(transfer->bits_per_word,
> >                                                 BITS_PER_BYTE));
> >
> >         or like this:
> >
> >         dws->n_bytes = roundup_pow_of_two(DIV_ROUND_UP(transfer->bits_per_word,
> >                                                        BITS_PER_BYTE));
> >
> > Splitting the line into chunks will simplify the visual
> > differentiation between the outer and inner calls.
> >
> > * Note even though the 80-char columns limit isn't that strict rule
> > now, but it's still preferable unless exceeding the limit significantly
> > increases readability. The update you suggest doesn't seem like the case
> > which would improve the readability.
> >
> 
> Sure, I can make the following change in the formatting and send the
> patch series:
>          dws->n_bytes =
>                  roundup_pow_of_two(DIV_ROUND_UP(transfer->bits_per_word,
>                                                  BITS_PER_BYTE));

Ok. Thanks in advance.

-Serge(y)

> 
> Thanks
> Joy
> > -Serge(y)
> >
> > >       dws->tx = (void *)transfer->tx_buf;
> > >       dws->tx_len = transfer->len / dws->n_bytes;
> > >       dws->rx = transfer->rx_buf;
> > > --
> > > 2.40.0.634.g4ca3ef3211-goog
> > >

RE: [PATCH v8 5/5] spi: dw: Round of n_bytes to power of 2

[David Laight]

From: Joy Chakraborty
> Sent: 21 April 2023 10:22
...
> Sure, I can make the following change in the formatting and send the
> patch series:
>          dws->n_bytes =
>                  roundup_pow_of_two(DIV_ROUND_UP(transfer->bits_per_word,
>                                                  BITS_PER_BYTE));

Won't checkpatch bleat about that?

Is it ever actually valid for the caller to provide a
value that isn't 8, 16 or 32 ?

I'm sure it looked as though some other lengths/counts
where likely to go badly wrong.

I know there are times when it is useful to bit-bang 'odd'
numbers of bits - like command+address+delay for fast reads
but that is a sub-32bit transfer so (at least somewhere)
is 1 word but not all the bits.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v8 5/5] spi: dw: Round of n_bytes to power of 2

[Serge Semin]

On Fri, Apr 21, 2023 at 04:39:30PM +0000, David Laight wrote:
> From: Joy Chakraborty
> > Sent: 21 April 2023 10:22
> ...
> > Sure, I can make the following change in the formatting and send the
> > patch series:
> >          dws->n_bytes =
> >                  roundup_pow_of_two(DIV_ROUND_UP(transfer->bits_per_word,
> >                                                  BITS_PER_BYTE));
>
 
> Won't checkpatch bleat about that?

Why would it?

> 
> Is it ever actually valid for the caller to provide a
> value that isn't 8, 16 or 32 ?

Judging by this
https://elixir.bootlin.com/linux/v6.3-rc7/source/drivers/spi/spi.c#L3630
it is. SPI-controller also supports word lengths within the
pre-synthesized range. So it's up to the SPI-peripherals and their
protocols what word length to select.

-Serge(y)

> 
> I'm sure it looked as though some other lengths/counts
> where likely to go badly wrong.
> 
> I know there are times when it is useful to bit-bang 'odd'
> numbers of bits - like command+address+delay for fast reads
> but that is a sub-32bit transfer so (at least somewhere)
> is 1 word but not all the bits.
> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)

Re: [PATCH v8 5/5] spi: dw: Round of n_bytes to power of 2

[Joy Chakraborty]

On Fri, Apr 21, 2023 at 10:18 PM Serge Semin <fancer.lancer@gmail.com> wrote:
>
> On Fri, Apr 21, 2023 at 04:39:30PM +0000, David Laight wrote:
> > From: Joy Chakraborty
> > > Sent: 21 April 2023 10:22
> > ...
> > > Sure, I can make the following change in the formatting and send the
> > > patch series:
> > >          dws->n_bytes =
> > >                  roundup_pow_of_two(DIV_ROUND_UP(transfer->bits_per_word,
> > >                                                  BITS_PER_BYTE));
> >
>
> > Won't checkpatch bleat about that?
>
> Why would it?

I ran checkpatch on this and it seems to be fine with minor spacing changes.

>
> >
> > Is it ever actually valid for the caller to provide a
> > value that isn't 8, 16 or 32 ?
>
> Judging by this
> https://elixir.bootlin.com/linux/v6.3-rc7/source/drivers/spi/spi.c#L3630
> it is. SPI-controller also supports word lengths within the
> pre-synthesized range. So it's up to the SPI-peripherals and their
> protocols what word length to select.
>
> -Serge(y)
>
> >
> > I'm sure it looked as though some other lengths/counts
> > where likely to go badly wrong.
> >
> > I know there are times when it is useful to bit-bang 'odd'
> > numbers of bits - like command+address+delay for fast reads
> > but that is a sub-32bit transfer so (at least somewhere)
> > is 1 word but not all the bits.
> >
> >       David
> >
> > -
> > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> > Registration No: 1397386 (Wales)

Re: [PATCH v8 5/5] spi: dw: Round of n_bytes to power of 2

[Serge Semin]

On Fri, Apr 21, 2023 at 10:40:44PM +0530, Joy Chakraborty wrote:
> On Fri, Apr 21, 2023 at 10:18 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> >
> > On Fri, Apr 21, 2023 at 04:39:30PM +0000, David Laight wrote:
> > > From: Joy Chakraborty
> > > > Sent: 21 April 2023 10:22
> > > ...
> > > > Sure, I can make the following change in the formatting and send the
> > > > patch series:
> > > >          dws->n_bytes =
> > > >                  roundup_pow_of_two(DIV_ROUND_UP(transfer->bits_per_word,
> > > >                                                  BITS_PER_BYTE));
> > >
> >
> > > Won't checkpatch bleat about that?
> >
> > Why would it?
> 
> I ran checkpatch on this and it seems to be fine with minor spacing changes.

What spacing do you mean? No problem with the change as is:
[fancer@mobilestation] kernel $ git show HEAD | grep -A1 -B2 roundup_pow_of_two
-	dws->n_bytes = DIV_ROUND_UP(transfer->bits_per_word, BITS_PER_BYTE);
+	dws->n_bytes =
+		roundup_pow_of_two(DIV_ROUND_UP(transfer->bits_per_word,
+						BITS_PER_BYTE));
[fancer@mobilestation] kernel $ ./scripts/checkpatch.pl --git HEAD
total: 0 errors, 0 warnings, 10 lines checked

Commit e18b699257db ("spi: dw: Round of n_bytes to power of 2") has no obvious style problems and is ready for submission.

-Serge(y)

> 
> >
> > >
> > > Is it ever actually valid for the caller to provide a
> > > value that isn't 8, 16 or 32 ?
> >
> > Judging by this
> > https://elixir.bootlin.com/linux/v6.3-rc7/source/drivers/spi/spi.c#L3630
> > it is. SPI-controller also supports word lengths within the
> > pre-synthesized range. So it's up to the SPI-peripherals and their
> > protocols what word length to select.
> >
> > -Serge(y)
> >
> > >
> > > I'm sure it looked as though some other lengths/counts
> > > where likely to go badly wrong.
> > >
> > > I know there are times when it is useful to bit-bang 'odd'
> > > numbers of bits - like command+address+delay for fast reads
> > > but that is a sub-32bit transfer so (at least somewhere)
> > > is 1 word but not all the bits.
> > >
> > >       David
> > >
> > > -
> > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> > > Registration No: 1397386 (Wales)

Re: [PATCH v8 5/5] spi: dw: Round of n_bytes to power of 2

[Joy Chakraborty]

On Fri, Apr 21, 2023 at 10:45 PM Serge Semin <fancer.lancer@gmail.com> wrote:
>
> On Fri, Apr 21, 2023 at 10:40:44PM +0530, Joy Chakraborty wrote:
> > On Fri, Apr 21, 2023 at 10:18 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> > >
> > > On Fri, Apr 21, 2023 at 04:39:30PM +0000, David Laight wrote:
> > > > From: Joy Chakraborty
> > > > > Sent: 21 April 2023 10:22
> > > > ...
> > > > > Sure, I can make the following change in the formatting and send the
> > > > > patch series:
> > > > >          dws->n_bytes =
> > > > >                  roundup_pow_of_two(DIV_ROUND_UP(transfer->bits_per_word,
> > > > >                                                  BITS_PER_BYTE));
> > > >
> > >
> > > > Won't checkpatch bleat about that?
> > >
> > > Why would it?
> >
> > I ran checkpatch on this and it seems to be fine with minor spacing changes.
>
> What spacing do you mean? No problem with the change as is:
> [fancer@mobilestation] kernel $ git show HEAD | grep -A1 -B2 roundup_pow_of_two
> -       dws->n_bytes = DIV_ROUND_UP(transfer->bits_per_word, BITS_PER_BYTE);
> +       dws->n_bytes =
> +               roundup_pow_of_two(DIV_ROUND_UP(transfer->bits_per_word,
> +                                               BITS_PER_BYTE));
> [fancer@mobilestation] kernel $ ./scripts/checkpatch.pl --git HEAD
> total: 0 errors, 0 warnings, 10 lines checked
>
> Commit e18b699257db ("spi: dw: Round of n_bytes to power of 2") has no obvious style problems and is ready for submission.
>
> -Serge(y)
>

Sorry for my error, it looks like my email client does not show it correctly.
What I was going to upload in V9 is the same as you mentioned.

Thanks
Joy
> >
> > >
> > > >
> > > > Is it ever actually valid for the caller to provide a
> > > > value that isn't 8, 16 or 32 ?
> > >
> > > Judging by this
> > > https://elixir.bootlin.com/linux/v6.3-rc7/source/drivers/spi/spi.c#L3630
> > > it is. SPI-controller also supports word lengths within the
> > > pre-synthesized range. So it's up to the SPI-peripherals and their
> > > protocols what word length to select.
> > >
> > > -Serge(y)
> > >
> > > >
> > > > I'm sure it looked as though some other lengths/counts
> > > > where likely to go badly wrong.
> > > >
> > > > I know there are times when it is useful to bit-bang 'odd'
> > > > numbers of bits - like command+address+delay for fast reads
> > > > but that is a sub-32bit transfer so (at least somewhere)
> > > > is 1 word but not all the bits.
> > > >
> > > >       David
> > > >
> > > > -
> > > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> > > > Registration No: 1397386 (Wales)