[PATCH v5 0/2] spi: dw: DW SPI DMA Driver updates

[PATCH v5 0/2] 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.
---

Joy Chakraborty (2):
  spi: dw: Add 32 bpw support to DW DMA Controller
  spi: dw: Add dma controller capability checks

 drivers/spi/spi-dw-dma.c | 70 ++++++++++++++++++++++++++++++++--------
 drivers/spi/spi-dw.h     |  1 +
 2 files changed, 57 insertions(+), 14 deletions(-)

-- 
2.40.0.423.gd6c402a77b-goog

[PATCH v5 1/2] spi: dw: Add 32 bpw support to DW DMA Controller

[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 3 or 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.

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

diff --git a/drivers/spi/spi-dw-dma.c b/drivers/spi/spi-dw-dma.c
index ababb910b391..b3a88bb75907 100644
--- a/drivers/spi/spi-dw-dma.c
+++ b/drivers/spi/spi-dw-dma.c
@@ -208,12 +208,17 @@ 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 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)
-- 
2.40.0.423.gd6c402a77b-goog

Re: [PATCH v5 1/2] spi: dw: Add 32 bpw support to DW DMA Controller

[Andy Shevchenko]

On Thu, Mar 30, 2023 at 06:34:49AM +0000, Joy Chakraborty wrote:

First of all the Subject is wrong. You are not touching DMA controller.
Needs to be rephrased.

> Add Support for AxSize = 4 bytes configuration from dw dma driver if

SPI DMA driver

(or something like this, note capital letters for acronyms).

> n_bytes i.e. number of bytes per write to fifo is 3 or 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.

...

>  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:

I'm not sure about this.

> +	case 4:
> +		return DMA_SLAVE_BUSWIDTH_4_BYTES;
> +	default:
> +		return DMA_SLAVE_BUSWIDTH_UNDEFINED;
> +	}
>  }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 1/2] spi: dw: Add 32 bpw support to DW DMA Controller

[Serge Semin]

On Tue, Apr 11, 2023 at 03:13:49PM +0300, Andy Shevchenko wrote:
> On Thu, Mar 30, 2023 at 06:34:49AM +0000, Joy Chakraborty wrote:
> 
> First of all the Subject is wrong. You are not touching DMA controller.
> Needs to be rephrased.
> 
> > Add Support for AxSize = 4 bytes configuration from dw dma driver if
> 
> SPI DMA driver
> 
> (or something like this, note capital letters for acronyms).
> 
> > n_bytes i.e. number of bytes per write to fifo is 3 or 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.
> 
> ...
> 
> >  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:
> 
> I'm not sure about this.

This actually makes sense seeing the function argument can have values
1, 2, _3_ and 4:
dws->n_bytes = DIV_ROUND_UP(transfer->bits_per_word, BITS_PER_BYTE);
transfer->bits_per_word = __F__(master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32));
...
dw_spi_dma_convert_width(dws->n_bytes)

The spi_transfer.bits_per_word field value depends on the
SPI peripheral device communication protocol requirements which may
imply the 3-bytes word xfers (even though it's indeed unluckily).

This semantic will also match to what we currently have in the
IRQ-based SPI-transfer implementation (see dw_writer() and
dw_reader()).

-Serge(y)

> 
> > +	case 4:
> > +		return DMA_SLAVE_BUSWIDTH_4_BYTES;
> > +	default:
> > +		return DMA_SLAVE_BUSWIDTH_UNDEFINED;
> > +	}
> >  }
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH v5 1/2] spi: dw: Add 32 bpw support to DW DMA Controller

[Joy Chakraborty]

Hello Andy,

On Tue, Apr 11, 2023 at 7:41 PM Serge Semin <fancer.lancer@gmail.com> wrote:
>
> On Tue, Apr 11, 2023 at 03:13:49PM +0300, Andy Shevchenko wrote:
> > On Thu, Mar 30, 2023 at 06:34:49AM +0000, Joy Chakraborty wrote:
> >
> > First of all the Subject is wrong. You are not touching DMA controller.
> > Needs to be rephrased.

Sure, will rephrase this to "SPI DMA Driver" instead of controller.

> >
> > > Add Support for AxSize = 4 bytes configuration from dw dma driver if
> >
> > SPI DMA driver
> >
> > (or something like this, note capital letters for acronyms).
> >
> > > n_bytes i.e. number of bytes per write to fifo is 3 or 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.
> >
> > ...
> >
> > >  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:
> >
> > I'm not sure about this.
>
> This actually makes sense seeing the function argument can have values
> 1, 2, _3_ and 4:
> dws->n_bytes = DIV_ROUND_UP(transfer->bits_per_word, BITS_PER_BYTE);
> transfer->bits_per_word = __F__(master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32));
> ...
> dw_spi_dma_convert_width(dws->n_bytes)
>
> The spi_transfer.bits_per_word field value depends on the
> SPI peripheral device communication protocol requirements which may
> imply the 3-bytes word xfers (even though it's indeed unluckily).
>
> This semantic will also match to what we currently have in the
> IRQ-based SPI-transfer implementation (see dw_writer() and
> dw_reader()).
>
> -Serge(y)
>

Will keep this as is to be similar to dw_writer() / dw_reader() as
explained by Serge(y).

> >
> > > +   case 4:
> > > +           return DMA_SLAVE_BUSWIDTH_4_BYTES;
> > > +   default:
> > > +           return DMA_SLAVE_BUSWIDTH_UNDEFINED;
> > > +   }
> > >  }
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
> >

I shall create another patch series with the same.

Thanks
Joy

Re: [PATCH v5 1/2] spi: dw: Add 32 bpw support to DW DMA Controller

[Serge Semin]

On Tue, Apr 11, 2023 at 08:00:23PM +0530, Joy Chakraborty wrote:
> Hello Andy,
> 
> On Tue, Apr 11, 2023 at 7:41 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> >
> > On Tue, Apr 11, 2023 at 03:13:49PM +0300, Andy Shevchenko wrote:
> > > On Thu, Mar 30, 2023 at 06:34:49AM +0000, Joy Chakraborty wrote:
> > >
> > > First of all the Subject is wrong. You are not touching DMA controller.
> > > Needs to be rephrased.
> 
> Sure, will rephrase this to "SPI DMA Driver" instead of controller.
> 
> > >
> > > > Add Support for AxSize = 4 bytes configuration from dw dma driver if
> > >
> > > SPI DMA driver
> > >
> > > (or something like this, note capital letters for acronyms).
> > >
> > > > n_bytes i.e. number of bytes per write to fifo is 3 or 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.
> > >
> > > ...
> > >
> > > >  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:
> > >
> > > I'm not sure about this.
> >
> > This actually makes sense seeing the function argument can have values
> > 1, 2, _3_ and 4:
> > dws->n_bytes = DIV_ROUND_UP(transfer->bits_per_word, BITS_PER_BYTE);
> > transfer->bits_per_word = __F__(master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32));
> > ...
> > dw_spi_dma_convert_width(dws->n_bytes)
> >
> > The spi_transfer.bits_per_word field value depends on the
> > SPI peripheral device communication protocol requirements which may
> > imply the 3-bytes word xfers (even though it's indeed unluckily).
> >
> > This semantic will also match to what we currently have in the
> > IRQ-based SPI-transfer implementation (see dw_writer() and
> > dw_reader()).
> >
> > -Serge(y)
> >
> 
> Will keep this as is to be similar to dw_writer() / dw_reader() as
> explained by Serge(y).
> 
> > >
> > > > +   case 4:
> > > > +           return DMA_SLAVE_BUSWIDTH_4_BYTES;
> > > > +   default:
> > > > +           return DMA_SLAVE_BUSWIDTH_UNDEFINED;
> > > > +   }
> > > >  }
> > >
> > > --
> > > With Best Regards,
> > > Andy Shevchenko
> > >
> > >
> 
> I shall create another patch series with the same.

Hold on for Andy to respond please.

-Serge(y)

> 
> Thanks
> Joy

Re: [PATCH v5 1/2] spi: dw: Add 32 bpw support to DW DMA Controller

[Andy Shevchenko]

On Tue, Apr 11, 2023 at 05:11:15PM +0300, Serge Semin wrote:
> On Tue, Apr 11, 2023 at 03:13:49PM +0300, Andy Shevchenko wrote:
> > On Thu, Mar 30, 2023 at 06:34:49AM +0000, Joy Chakraborty wrote:

...

> > > -	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:
> > 
> > I'm not sure about this.
> 
> This actually makes sense seeing the function argument can have values
> 1, 2, _3_ and 4:
> dws->n_bytes = DIV_ROUND_UP(transfer->bits_per_word, BITS_PER_BYTE);
> transfer->bits_per_word = __F__(master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32));
> ...
> dw_spi_dma_convert_width(dws->n_bytes)
> 
> The spi_transfer.bits_per_word field value depends on the
> SPI peripheral device communication protocol requirements which may
> imply the 3-bytes word xfers (even though it's indeed unluckily).
> 
> This semantic will also match to what we currently have in the
> IRQ-based SPI-transfer implementation (see dw_writer() and
> dw_reader()).

Nice, but we have DMA_SLAVE_BUSWIDTH_3_BYTES definition for that. Why we don't
use it?

> > > +	case 4:
> > > +		return DMA_SLAVE_BUSWIDTH_4_BYTES;
> > > +	default:
> > > +		return DMA_SLAVE_BUSWIDTH_UNDEFINED;
> > > +	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 1/2] spi: dw: Add 32 bpw support to DW DMA Controller

[Serge Semin]

On Tue, Apr 11, 2023 at 05:46:34PM +0300, Andy Shevchenko wrote:
> On Tue, Apr 11, 2023 at 05:11:15PM +0300, Serge Semin wrote:
> > On Tue, Apr 11, 2023 at 03:13:49PM +0300, Andy Shevchenko wrote:
> > > On Thu, Mar 30, 2023 at 06:34:49AM +0000, Joy Chakraborty wrote:
> 
> ...
> 
> > > > -	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:
> > > 
> > > I'm not sure about this.
> > 
> > This actually makes sense seeing the function argument can have values
> > 1, 2, _3_ and 4:
> > dws->n_bytes = DIV_ROUND_UP(transfer->bits_per_word, BITS_PER_BYTE);
> > transfer->bits_per_word = __F__(master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32));
> > ...
> > dw_spi_dma_convert_width(dws->n_bytes)
> > 
> > The spi_transfer.bits_per_word field value depends on the
> > SPI peripheral device communication protocol requirements which may
> > imply the 3-bytes word xfers (even though it's indeed unluckily).
> > 
> > This semantic will also match to what we currently have in the
> > IRQ-based SPI-transfer implementation (see dw_writer() and
> > dw_reader()).
> 

> Nice, but we have DMA_SLAVE_BUSWIDTH_3_BYTES definition for that. Why we don't
> use it?

We could but there are two more-or-less firm reasons not to do
that:
1. There aren't that much DMA-engines with the
DMA_SLAVE_BUSWIDTH_3_BYTES capability meanwhile the DW APB SSI just
ignores the upper bits if CTRLR0.DFS is less than the value actual
written to the DR registers. Note DW DMAC engine isn't one of such
controllers. So if we get to meet a peripheral SPI-device with 3-bytes
word protocol transfers and the DMA-engine doesn't support it the
DMA-based transfers may fail (depending on the DMA-engine driver
implementation).
2. The DW APB SSIs (3.x and 4.x) can be synthesized with the APB Data
Bus Width of 8, 16 and 32. So no matter whether DMA-engine supports
the 3-bytes bus width the system bus most likely will either convert
the transfers to the proper sized bus-transactions or fail.

So taking all of the above into account not using the
DMA_SLAVE_BUSWIDTH_3_BYTES macro here seems better than using it with
a risk to fail some of the platform setups especially seeing the DW
APB SSI ignores the upper bits anyway.

-Serge(y)

> 
> > > > +	case 4:
> > > > +		return DMA_SLAVE_BUSWIDTH_4_BYTES;
> > > > +	default:
> > > > +		return DMA_SLAVE_BUSWIDTH_UNDEFINED;
> > > > +	}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH v5 1/2] spi: dw: Add 32 bpw support to DW DMA Controller

[Andy Shevchenko]

On Tue, Apr 11, 2023 at 06:09:16PM +0300, Serge Semin wrote:
> On Tue, Apr 11, 2023 at 05:46:34PM +0300, Andy Shevchenko wrote:
> > On Tue, Apr 11, 2023 at 05:11:15PM +0300, Serge Semin wrote:
> > > On Tue, Apr 11, 2023 at 03:13:49PM +0300, Andy Shevchenko wrote:
> > > > On Thu, Mar 30, 2023 at 06:34:49AM +0000, Joy Chakraborty wrote:

...

> > > > > -	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:
> > > > 
> > > > I'm not sure about this.
> > > 
> > > This actually makes sense seeing the function argument can have values
> > > 1, 2, _3_ and 4:
> > > dws->n_bytes = DIV_ROUND_UP(transfer->bits_per_word, BITS_PER_BYTE);
> > > transfer->bits_per_word = __F__(master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32));
> > > ...
> > > dw_spi_dma_convert_width(dws->n_bytes)
> > > 
> > > The spi_transfer.bits_per_word field value depends on the
> > > SPI peripheral device communication protocol requirements which may
> > > imply the 3-bytes word xfers (even though it's indeed unluckily).
> > > 
> > > This semantic will also match to what we currently have in the
> > > IRQ-based SPI-transfer implementation (see dw_writer() and
> > > dw_reader()).
> 
> > Nice, but we have DMA_SLAVE_BUSWIDTH_3_BYTES definition for that. Why we don't
> > use it?
> 
> We could but there are two more-or-less firm reasons not to do
> that:
> 1. There aren't that much DMA-engines with the
> DMA_SLAVE_BUSWIDTH_3_BYTES capability meanwhile the DW APB SSI just
> ignores the upper bits if CTRLR0.DFS is less than the value actual
> written to the DR registers. Note DW DMAC engine isn't one of such
> controllers. So if we get to meet a peripheral SPI-device with 3-bytes
> word protocol transfers and the DMA-engine doesn't support it the
> DMA-based transfers may fail (depending on the DMA-engine driver
> implementation).
> 2. The DW APB SSIs (3.x and 4.x) can be synthesized with the APB Data
> Bus Width of 8, 16 and 32. So no matter whether DMA-engine supports
> the 3-bytes bus width the system bus most likely will either convert
> the transfers to the proper sized bus-transactions or fail.
> 
> So taking all of the above into account not using the
> DMA_SLAVE_BUSWIDTH_3_BYTES macro here seems better than using it with
> a risk to fail some of the platform setups especially seeing the DW
> APB SSI ignores the upper bits anyway.

But this is not about SPI host hardware, it's about the consumers.
They should know about supported sizes. Either we add the corresponding support
to the driver or remove 3 case as I suggested. I don't think it's correct to
use 3 as 4.

> > > > > +	case 4:
> > > > > +		return DMA_SLAVE_BUSWIDTH_4_BYTES;
> > > > > +	default:
> > > > > +		return DMA_SLAVE_BUSWIDTH_UNDEFINED;
> > > > > +	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 1/2] spi: dw: Add 32 bpw support to DW DMA Controller

[Joy Chakraborty]

On Tue, Apr 11, 2023 at 8:47 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Tue, Apr 11, 2023 at 06:09:16PM +0300, Serge Semin wrote:
> > On Tue, Apr 11, 2023 at 05:46:34PM +0300, Andy Shevchenko wrote:
> > > On Tue, Apr 11, 2023 at 05:11:15PM +0300, Serge Semin wrote:
> > > > On Tue, Apr 11, 2023 at 03:13:49PM +0300, Andy Shevchenko wrote:
> > > > > On Thu, Mar 30, 2023 at 06:34:49AM +0000, Joy Chakraborty wrote:
>
> ...
>
> > > > > > -     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:
> > > > >
> > > > > I'm not sure about this.
> > > >
> > > > This actually makes sense seeing the function argument can have values
> > > > 1, 2, _3_ and 4:
> > > > dws->n_bytes = DIV_ROUND_UP(transfer->bits_per_word, BITS_PER_BYTE);
> > > > transfer->bits_per_word = __F__(master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32));
> > > > ...
> > > > dw_spi_dma_convert_width(dws->n_bytes)
> > > >
> > > > The spi_transfer.bits_per_word field value depends on the
> > > > SPI peripheral device communication protocol requirements which may
> > > > imply the 3-bytes word xfers (even though it's indeed unluckily).
> > > >
> > > > This semantic will also match to what we currently have in the
> > > > IRQ-based SPI-transfer implementation (see dw_writer() and
> > > > dw_reader()).
> >
> > > Nice, but we have DMA_SLAVE_BUSWIDTH_3_BYTES definition for that. Why we don't
> > > use it?
> >
> > We could but there are two more-or-less firm reasons not to do
> > that:
> > 1. There aren't that much DMA-engines with the
> > DMA_SLAVE_BUSWIDTH_3_BYTES capability meanwhile the DW APB SSI just
> > ignores the upper bits if CTRLR0.DFS is less than the value actual
> > written to the DR registers. Note DW DMAC engine isn't one of such
> > controllers. So if we get to meet a peripheral SPI-device with 3-bytes
> > word protocol transfers and the DMA-engine doesn't support it the
> > DMA-based transfers may fail (depending on the DMA-engine driver
> > implementation).
> > 2. The DW APB SSIs (3.x and 4.x) can be synthesized with the APB Data
> > Bus Width of 8, 16 and 32. So no matter whether DMA-engine supports
> > the 3-bytes bus width the system bus most likely will either convert
> > the transfers to the proper sized bus-transactions or fail.
> >
> > So taking all of the above into account not using the
> > DMA_SLAVE_BUSWIDTH_3_BYTES macro here seems better than using it with
> > a risk to fail some of the platform setups especially seeing the DW
> > APB SSI ignores the upper bits anyway.
>
> But this is not about SPI host hardware, it's about the consumers.
> They should know about supported sizes. Either we add the corresponding support
> to the driver or remove 3 case as I suggested. I don't think it's correct to
> use 3 as 4.

Another thing to add is that as per spi.h even if bits per word is a
not a power of 2 the buffer should be formatted in memory as a power
of 2
...
 * @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 for n_bytes = 3 or 24 bits/per word we expect the client SW to
format it to 4 byte buffers hence the transaction generated should
also be of size 4 from the DMA.

>
> > > > > > +     case 4:
> > > > > > +             return DMA_SLAVE_BUSWIDTH_4_BYTES;
> > > > > > +     default:
> > > > > > +             return DMA_SLAVE_BUSWIDTH_UNDEFINED;
> > > > > > +     }
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Thanks
Joy

Re: [PATCH v5 1/2] spi: dw: Add 32 bpw support to DW DMA Controller

[Andy Shevchenko]

On Tue, Apr 11, 2023 at 08:48:52PM +0530, Joy Chakraborty wrote:
> On Tue, Apr 11, 2023 at 8:47 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Tue, Apr 11, 2023 at 06:09:16PM +0300, Serge Semin wrote:
> > > On Tue, Apr 11, 2023 at 05:46:34PM +0300, Andy Shevchenko wrote:
> > > > On Tue, Apr 11, 2023 at 05:11:15PM +0300, Serge Semin wrote:
> > > > > On Tue, Apr 11, 2023 at 03:13:49PM +0300, Andy Shevchenko wrote:
> > > > > > On Thu, Mar 30, 2023 at 06:34:49AM +0000, Joy Chakraborty wrote:

...

> > > > > > > -     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:
> > > > > >
> > > > > > I'm not sure about this.
> > > > >
> > > > > This actually makes sense seeing the function argument can have values
> > > > > 1, 2, _3_ and 4:
> > > > > dws->n_bytes = DIV_ROUND_UP(transfer->bits_per_word, BITS_PER_BYTE);
> > > > > transfer->bits_per_word = __F__(master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32));
> > > > > ...
> > > > > dw_spi_dma_convert_width(dws->n_bytes)
> > > > >
> > > > > The spi_transfer.bits_per_word field value depends on the
> > > > > SPI peripheral device communication protocol requirements which may
> > > > > imply the 3-bytes word xfers (even though it's indeed unluckily).
> > > > >
> > > > > This semantic will also match to what we currently have in the
> > > > > IRQ-based SPI-transfer implementation (see dw_writer() and
> > > > > dw_reader()).
> > >
> > > > Nice, but we have DMA_SLAVE_BUSWIDTH_3_BYTES definition for that. Why we don't
> > > > use it?
> > >
> > > We could but there are two more-or-less firm reasons not to do
> > > that:
> > > 1. There aren't that much DMA-engines with the
> > > DMA_SLAVE_BUSWIDTH_3_BYTES capability meanwhile the DW APB SSI just
> > > ignores the upper bits if CTRLR0.DFS is less than the value actual
> > > written to the DR registers. Note DW DMAC engine isn't one of such
> > > controllers. So if we get to meet a peripheral SPI-device with 3-bytes
> > > word protocol transfers and the DMA-engine doesn't support it the
> > > DMA-based transfers may fail (depending on the DMA-engine driver
> > > implementation).
> > > 2. The DW APB SSIs (3.x and 4.x) can be synthesized with the APB Data
> > > Bus Width of 8, 16 and 32. So no matter whether DMA-engine supports
> > > the 3-bytes bus width the system bus most likely will either convert
> > > the transfers to the proper sized bus-transactions or fail.
> > >
> > > So taking all of the above into account not using the
> > > DMA_SLAVE_BUSWIDTH_3_BYTES macro here seems better than using it with
> > > a risk to fail some of the platform setups especially seeing the DW
> > > APB SSI ignores the upper bits anyway.
> >
> > But this is not about SPI host hardware, it's about the consumers.
> > They should know about supported sizes. Either we add the corresponding support
> > to the driver or remove 3 case as I suggested. I don't think it's correct to
> > use 3 as 4.
> 
> Another thing to add is that as per spi.h even if bits per word is a
> not a power of 2 the buffer should be formatted in memory as a power
> of 2
> ...
>  * @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 for n_bytes = 3 or 24 bits/per word we expect the client SW to
> format it to 4 byte buffers hence the transaction generated should
> also be of size 4 from the DMA.

You didn't get my point. The consumer wants to know if the 3 bytes is supported
or not, that's should be part of the DMA related thing, right?

It's incorrectly to say 4 for 3 if the backend DMA controller behaves differently
on this. How do you know that (any) DMA controller integrated with this hardware
has no side effects for this change.

So, I don't think the case 3 is correct in this patch.

> > > > > > > +     case 4:
> > > > > > > +             return DMA_SLAVE_BUSWIDTH_4_BYTES;
> > > > > > > +     default:
> > > > > > > +             return DMA_SLAVE_BUSWIDTH_UNDEFINED;
> > > > > > > +     }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 1/2] spi: dw: Add 32 bpw support to DW DMA Controller

[Joy Chakraborty]

On Tue, Apr 11, 2023 at 9:52 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Tue, Apr 11, 2023 at 08:48:52PM +0530, Joy Chakraborty wrote:
> > On Tue, Apr 11, 2023 at 8:47 PM Andy Shevchenko
> > <andriy.shevchenko@intel.com> wrote:
> > > On Tue, Apr 11, 2023 at 06:09:16PM +0300, Serge Semin wrote:
> > > > On Tue, Apr 11, 2023 at 05:46:34PM +0300, Andy Shevchenko wrote:
> > > > > On Tue, Apr 11, 2023 at 05:11:15PM +0300, Serge Semin wrote:
> > > > > > On Tue, Apr 11, 2023 at 03:13:49PM +0300, Andy Shevchenko wrote:
> > > > > > > On Thu, Mar 30, 2023 at 06:34:49AM +0000, Joy Chakraborty wrote:
>
> ...
>
> > > > > > > > -     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:
> > > > > > >
> > > > > > > I'm not sure about this.
> > > > > >
> > > > > > This actually makes sense seeing the function argument can have values
> > > > > > 1, 2, _3_ and 4:
> > > > > > dws->n_bytes = DIV_ROUND_UP(transfer->bits_per_word, BITS_PER_BYTE);
> > > > > > transfer->bits_per_word = __F__(master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32));
> > > > > > ...
> > > > > > dw_spi_dma_convert_width(dws->n_bytes)
> > > > > >
> > > > > > The spi_transfer.bits_per_word field value depends on the
> > > > > > SPI peripheral device communication protocol requirements which may
> > > > > > imply the 3-bytes word xfers (even though it's indeed unluckily).
> > > > > >
> > > > > > This semantic will also match to what we currently have in the
> > > > > > IRQ-based SPI-transfer implementation (see dw_writer() and
> > > > > > dw_reader()).
> > > >
> > > > > Nice, but we have DMA_SLAVE_BUSWIDTH_3_BYTES definition for that. Why we don't
> > > > > use it?
> > > >
> > > > We could but there are two more-or-less firm reasons not to do
> > > > that:
> > > > 1. There aren't that much DMA-engines with the
> > > > DMA_SLAVE_BUSWIDTH_3_BYTES capability meanwhile the DW APB SSI just
> > > > ignores the upper bits if CTRLR0.DFS is less than the value actual
> > > > written to the DR registers. Note DW DMAC engine isn't one of such
> > > > controllers. So if we get to meet a peripheral SPI-device with 3-bytes
> > > > word protocol transfers and the DMA-engine doesn't support it the
> > > > DMA-based transfers may fail (depending on the DMA-engine driver
> > > > implementation).
> > > > 2. The DW APB SSIs (3.x and 4.x) can be synthesized with the APB Data
> > > > Bus Width of 8, 16 and 32. So no matter whether DMA-engine supports
> > > > the 3-bytes bus width the system bus most likely will either convert
> > > > the transfers to the proper sized bus-transactions or fail.
> > > >
> > > > So taking all of the above into account not using the
> > > > DMA_SLAVE_BUSWIDTH_3_BYTES macro here seems better than using it with
> > > > a risk to fail some of the platform setups especially seeing the DW
> > > > APB SSI ignores the upper bits anyway.
> > >
> > > But this is not about SPI host hardware, it's about the consumers.
> > > They should know about supported sizes. Either we add the corresponding support
> > > to the driver or remove 3 case as I suggested. I don't think it's correct to
> > > use 3 as 4.
> >
> > Another thing to add is that as per spi.h even if bits per word is a
> > not a power of 2 the buffer should be formatted in memory as a power
> > of 2
> > ...
> >  * @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 for n_bytes = 3 or 24 bits/per word we expect the client SW to
> > format it to 4 byte buffers hence the transaction generated should
> > also be of size 4 from the DMA.
>
> You didn't get my point. The consumer wants to know if the 3 bytes is supported
> or not, that's should be part of the DMA related thing, right?
>
> It's incorrectly to say 4 for 3 if the backend DMA controller behaves differently
> on this. How do you know that (any) DMA controller integrated with this hardware
> has no side effects for this change.
>
> So, I don't think the case 3 is correct in this patch.

I see, I am of the opposite opinion in this case.

Other then Serge(y)'s points,
I was trying to say that irrespective of what the underlying DMA
controller supports we should use DMA_SLAVE_BUSWIDTH_4_BYTES when
n_bytes = 3 from SPI perspective as we get n_bytes from bits per word
passed by the client / spi framework " dws->n_bytes =
DIV_ROUND_UP(transfer->bits_per_word, BITS_PER_BYTE) ".
Based on the spi header what I perceive is that for bits/word between
17 and 32 the data has to be stored in 32bit words in memory as per
the example in the header " (e.g. 20 bit samples use 32 bits) ".

Hence, taking an example to transfer 6 bytes (say 0xAA 0xBB 0xCC 0xDD
0xEE 0xFF) with bits per word as 24 (n_bytes = 3) i.e. a total of 2
words I expect the buffer to look as follows which is coming from the
client:
_ _____address|__________0________4________8________C
    SD:00000000|>00CCBBAA 00FFEEDD 00000000 00000000
Hence to transfer this successfully the DMA controller would need to
copy 4 bytes per word .

Please correct me if my understanding of this is incorrect.

>
> > > > > > > > +     case 4:
> > > > > > > > +             return DMA_SLAVE_BUSWIDTH_4_BYTES;
> > > > > > > > +     default:
> > > > > > > > +             return DMA_SLAVE_BUSWIDTH_UNDEFINED;
> > > > > > > > +     }
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Thanks
Joy

Re: [PATCH v5 1/2] spi: dw: Add 32 bpw support to DW DMA Controller

[Joy Chakraborty]

On Wed, Apr 12, 2023 at 1:11 AM Joy Chakraborty <joychakr@google.com> wrote:
>
> On Tue, Apr 11, 2023 at 9:52 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> >
> > On Tue, Apr 11, 2023 at 08:48:52PM +0530, Joy Chakraborty wrote:
> > > On Tue, Apr 11, 2023 at 8:47 PM Andy Shevchenko
> > > <andriy.shevchenko@intel.com> wrote:
> > > > On Tue, Apr 11, 2023 at 06:09:16PM +0300, Serge Semin wrote:
> > > > > On Tue, Apr 11, 2023 at 05:46:34PM +0300, Andy Shevchenko wrote:
> > > > > > On Tue, Apr 11, 2023 at 05:11:15PM +0300, Serge Semin wrote:
> > > > > > > On Tue, Apr 11, 2023 at 03:13:49PM +0300, Andy Shevchenko wrote:
> > > > > > > > On Thu, Mar 30, 2023 at 06:34:49AM +0000, Joy Chakraborty wrote:
> >
> > ...
> >
> > > > > > > > > -     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:
> > > > > > > >
> > > > > > > > I'm not sure about this.
> > > > > > >
> > > > > > > This actually makes sense seeing the function argument can have values
> > > > > > > 1, 2, _3_ and 4:
> > > > > > > dws->n_bytes = DIV_ROUND_UP(transfer->bits_per_word, BITS_PER_BYTE);
> > > > > > > transfer->bits_per_word = __F__(master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32));
> > > > > > > ...
> > > > > > > dw_spi_dma_convert_width(dws->n_bytes)
> > > > > > >
> > > > > > > The spi_transfer.bits_per_word field value depends on the
> > > > > > > SPI peripheral device communication protocol requirements which may
> > > > > > > imply the 3-bytes word xfers (even though it's indeed unluckily).
> > > > > > >
> > > > > > > This semantic will also match to what we currently have in the
> > > > > > > IRQ-based SPI-transfer implementation (see dw_writer() and
> > > > > > > dw_reader()).
> > > > >
> > > > > > Nice, but we have DMA_SLAVE_BUSWIDTH_3_BYTES definition for that. Why we don't
> > > > > > use it?
> > > > >
> > > > > We could but there are two more-or-less firm reasons not to do
> > > > > that:
> > > > > 1. There aren't that much DMA-engines with the
> > > > > DMA_SLAVE_BUSWIDTH_3_BYTES capability meanwhile the DW APB SSI just
> > > > > ignores the upper bits if CTRLR0.DFS is less than the value actual
> > > > > written to the DR registers. Note DW DMAC engine isn't one of such
> > > > > controllers. So if we get to meet a peripheral SPI-device with 3-bytes
> > > > > word protocol transfers and the DMA-engine doesn't support it the
> > > > > DMA-based transfers may fail (depending on the DMA-engine driver
> > > > > implementation).
> > > > > 2. The DW APB SSIs (3.x and 4.x) can be synthesized with the APB Data
> > > > > Bus Width of 8, 16 and 32. So no matter whether DMA-engine supports
> > > > > the 3-bytes bus width the system bus most likely will either convert
> > > > > the transfers to the proper sized bus-transactions or fail.
> > > > >
> > > > > So taking all of the above into account not using the
> > > > > DMA_SLAVE_BUSWIDTH_3_BYTES macro here seems better than using it with
> > > > > a risk to fail some of the platform setups especially seeing the DW
> > > > > APB SSI ignores the upper bits anyway.
> > > >
> > > > But this is not about SPI host hardware, it's about the consumers.
> > > > They should know about supported sizes. Either we add the corresponding support
> > > > to the driver or remove 3 case as I suggested. I don't think it's correct to
> > > > use 3 as 4.
> > >
> > > Another thing to add is that as per spi.h even if bits per word is a
> > > not a power of 2 the buffer should be formatted in memory as a power
> > > of 2
> > > ...
> > >  * @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 for n_bytes = 3 or 24 bits/per word we expect the client SW to
> > > format it to 4 byte buffers hence the transaction generated should
> > > also be of size 4 from the DMA.
> >
> > You didn't get my point. The consumer wants to know if the 3 bytes is supported
> > or not, that's should be part of the DMA related thing, right?
> >
> > It's incorrectly to say 4 for 3 if the backend DMA controller behaves differently
> > on this. How do you know that (any) DMA controller integrated with this hardware
> > has no side effects for this change.
> >
> > So, I don't think the case 3 is correct in this patch.
>
> I see, I am of the opposite opinion in this case.
>
> Other then Serge(y)'s points,
> I was trying to say that irrespective of what the underlying DMA
> controller supports we should use DMA_SLAVE_BUSWIDTH_4_BYTES when
> n_bytes = 3 from SPI perspective as we get n_bytes from bits per word
> passed by the client / spi framework " dws->n_bytes =
> DIV_ROUND_UP(transfer->bits_per_word, BITS_PER_BYTE) ".
> Based on the spi header what I perceive is that for bits/word between
> 17 and 32 the data has to be stored in 32bit words in memory as per
> the example in the header " (e.g. 20 bit samples use 32 bits) ".
>
> Hence, taking an example to transfer 6 bytes (say 0xAA 0xBB 0xCC 0xDD
> 0xEE 0xFF) with bits per word as 24 (n_bytes = 3) i.e. a total of 2
> words I expect the buffer to look as follows which is coming from the
> client:
> _ _____address|__________0________4________8________C
>     SD:00000000|>00CCBBAA 00FFEEDD 00000000 00000000
> Hence to transfer this successfully the DMA controller would need to
> copy 4 bytes per word .
>
> Please correct me if my understanding of this is incorrect.

On the other hand I do see that in the fifo driver dw_writer() /
dw_reader() increments the pointer with 3 incase n_bytes = 3 even
though it copies 4 bytes.
...
   if (dws->n_bytes == 1)
      txw = *(u8 *)(dws->tx);
   else if (dws->n_bytes == 2)
      txw = *(u16 *)(dws->tx);
   else
      txw = *(u32 *)(dws->tx);

dws->tx += dws->n_bytes;
...
This will not behave as using DMA_SLAVE_BUSWIDTH_4_BYTES in the DMA so
maybe I am not correct in interpretting the spi.h header file.
Can CPU's in general access u32 from unaligned odd addresses ?

From Serge(y)'s comment regarding this, the APB interface writing data
to the FIFO register definitely cannot handle
DMA_SLAVE_BUSWIDTH_3_BYTES since it handles a power of 2 only.
Hence we can possibly remove "case 3:" completely and also mask out
DMA_SLAVE_BUSWIDTH_3_BYTES from dma_addr_width capabilities so that
can_dma api does not allow n_bytes = 3 to use DMA.

Would that be correct ?

>
> >
> > > > > > > > > +     case 4:
> > > > > > > > > +             return DMA_SLAVE_BUSWIDTH_4_BYTES;
> > > > > > > > > +     default:
> > > > > > > > > +             return DMA_SLAVE_BUSWIDTH_UNDEFINED;
> > > > > > > > > +     }
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
> >
> Thanks
> Joy

Re: [PATCH v5 1/2] spi: dw: Add 32 bpw support to DW DMA Controller

[Andy Shevchenko]

On Wed, Apr 12, 2023 at 02:51:44PM +0530, Joy Chakraborty wrote:
> On Wed, Apr 12, 2023 at 1:11 AM Joy Chakraborty <joychakr@google.com> wrote:
> > On Tue, Apr 11, 2023 at 9:52 PM Andy Shevchenko
> > <andriy.shevchenko@intel.com> wrote:
> > > On Tue, Apr 11, 2023 at 08:48:52PM +0530, Joy Chakraborty wrote:
> > > > On Tue, Apr 11, 2023 at 8:47 PM Andy Shevchenko
> > > > <andriy.shevchenko@intel.com> wrote:
> > > > > On Tue, Apr 11, 2023 at 06:09:16PM +0300, Serge Semin wrote:
> > > > > > On Tue, Apr 11, 2023 at 05:46:34PM +0300, Andy Shevchenko wrote:
> > > > > > > On Tue, Apr 11, 2023 at 05:11:15PM +0300, Serge Semin wrote:
> > > > > > > > On Tue, Apr 11, 2023 at 03:13:49PM +0300, Andy Shevchenko wrote:
> > > > > > > > > On Thu, Mar 30, 2023 at 06:34:49AM +0000, Joy Chakraborty wrote:

...

> > > > > > > > > > -     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:
> > > > > > > > >
> > > > > > > > > I'm not sure about this.
> > > > > > > >
> > > > > > > > This actually makes sense seeing the function argument can have values
> > > > > > > > 1, 2, _3_ and 4:
> > > > > > > > dws->n_bytes = DIV_ROUND_UP(transfer->bits_per_word, BITS_PER_BYTE);
> > > > > > > > transfer->bits_per_word = __F__(master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32));
> > > > > > > > ...
> > > > > > > > dw_spi_dma_convert_width(dws->n_bytes)
> > > > > > > >
> > > > > > > > The spi_transfer.bits_per_word field value depends on the
> > > > > > > > SPI peripheral device communication protocol requirements which may
> > > > > > > > imply the 3-bytes word xfers (even though it's indeed unluckily).
> > > > > > > >
> > > > > > > > This semantic will also match to what we currently have in the
> > > > > > > > IRQ-based SPI-transfer implementation (see dw_writer() and
> > > > > > > > dw_reader()).
> > > > > >
> > > > > > > Nice, but we have DMA_SLAVE_BUSWIDTH_3_BYTES definition for that. Why we don't
> > > > > > > use it?
> > > > > >
> > > > > > We could but there are two more-or-less firm reasons not to do
> > > > > > that:
> > > > > > 1. There aren't that much DMA-engines with the
> > > > > > DMA_SLAVE_BUSWIDTH_3_BYTES capability meanwhile the DW APB SSI just
> > > > > > ignores the upper bits if CTRLR0.DFS is less than the value actual
> > > > > > written to the DR registers. Note DW DMAC engine isn't one of such
> > > > > > controllers. So if we get to meet a peripheral SPI-device with 3-bytes
> > > > > > word protocol transfers and the DMA-engine doesn't support it the
> > > > > > DMA-based transfers may fail (depending on the DMA-engine driver
> > > > > > implementation).
> > > > > > 2. The DW APB SSIs (3.x and 4.x) can be synthesized with the APB Data
> > > > > > Bus Width of 8, 16 and 32. So no matter whether DMA-engine supports
> > > > > > the 3-bytes bus width the system bus most likely will either convert
> > > > > > the transfers to the proper sized bus-transactions or fail.
> > > > > >
> > > > > > So taking all of the above into account not using the
> > > > > > DMA_SLAVE_BUSWIDTH_3_BYTES macro here seems better than using it with
> > > > > > a risk to fail some of the platform setups especially seeing the DW
> > > > > > APB SSI ignores the upper bits anyway.
> > > > >
> > > > > But this is not about SPI host hardware, it's about the consumers.
> > > > > They should know about supported sizes. Either we add the corresponding support
> > > > > to the driver or remove 3 case as I suggested. I don't think it's correct to
> > > > > use 3 as 4.
> > > >
> > > > Another thing to add is that as per spi.h even if bits per word is a
> > > > not a power of 2 the buffer should be formatted in memory as a power
> > > > of 2
> > > > ...
> > > >  * @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 for n_bytes = 3 or 24 bits/per word we expect the client SW to
> > > > format it to 4 byte buffers hence the transaction generated should
> > > > also be of size 4 from the DMA.
> > >
> > > You didn't get my point. The consumer wants to know if the 3 bytes is supported
> > > or not, that's should be part of the DMA related thing, right?
> > >
> > > It's incorrectly to say 4 for 3 if the backend DMA controller behaves differently
> > > on this. How do you know that (any) DMA controller integrated with this hardware
> > > has no side effects for this change.
> > >
> > > So, I don't think the case 3 is correct in this patch.
> >
> > I see, I am of the opposite opinion in this case.
> >
> > Other then Serge(y)'s points,
> > I was trying to say that irrespective of what the underlying DMA
> > controller supports we should use DMA_SLAVE_BUSWIDTH_4_BYTES when
> > n_bytes = 3 from SPI perspective as we get n_bytes from bits per word
> > passed by the client / spi framework " dws->n_bytes =
> > DIV_ROUND_UP(transfer->bits_per_word, BITS_PER_BYTE) ".
> > Based on the spi header what I perceive is that for bits/word between
> > 17 and 32 the data has to be stored in 32bit words in memory as per
> > the example in the header " (e.g. 20 bit samples use 32 bits) ".
> >
> > Hence, taking an example to transfer 6 bytes (say 0xAA 0xBB 0xCC 0xDD
> > 0xEE 0xFF) with bits per word as 24 (n_bytes = 3) i.e. a total of 2
> > words I expect the buffer to look as follows which is coming from the
> > client:
> > _ _____address|__________0________4________8________C
> >     SD:00000000|>00CCBBAA 00FFEEDD 00000000 00000000
> > Hence to transfer this successfully the DMA controller would need to
> > copy 4 bytes per word .
> >
> > Please correct me if my understanding of this is incorrect.

Thank you for finding the answer for me by yourself!

> On the other hand I do see that in the fifo driver dw_writer() /
> dw_reader() increments the pointer with 3 incase n_bytes = 3 even
> though it copies 4 bytes.
> ...
>    if (dws->n_bytes == 1)
>       txw = *(u8 *)(dws->tx);
>    else if (dws->n_bytes == 2)
>       txw = *(u16 *)(dws->tx);
>    else
>       txw = *(u32 *)(dws->tx);
> 
> dws->tx += dws->n_bytes;
> ...
> This will not behave as using DMA_SLAVE_BUSWIDTH_4_BYTES in the DMA so
> maybe I am not correct in interpretting the spi.h header file.
> Can CPU's in general access u32 from unaligned odd addresses ?

Generally speaking the above code must check number of bytes for being 4.

> From Serge(y)'s comment regarding this, the APB interface writing data
> to the FIFO register definitely cannot handle
> DMA_SLAVE_BUSWIDTH_3_BYTES since it handles a power of 2 only.
> Hence we can possibly remove "case 3:" completely and also mask out
> DMA_SLAVE_BUSWIDTH_3_BYTES from dma_addr_width capabilities so that
> can_dma api does not allow n_bytes = 3 to use DMA.
> 
> Would that be correct ?

We have to fix the above and the DIV_ROUND_UP(transfer->bits_per_word,
BITS_PER_BYTE) one to be something like

roundup_pow_of_two(round_up(..., BITS_PER_BYTE))

> > > > > > > > > > +     case 4:
> > > > > > > > > > +             return DMA_SLAVE_BUSWIDTH_4_BYTES;
> > > > > > > > > > +     default:
> > > > > > > > > > +             return DMA_SLAVE_BUSWIDTH_UNDEFINED;
> > > > > > > > > > +     }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 1/2] spi: dw: Add 32 bpw support to DW DMA Controller

[Joy Chakraborty]

On Wed, Apr 12, 2023 at 6:55 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Wed, Apr 12, 2023 at 02:51:44PM +0530, Joy Chakraborty wrote:
> > On Wed, Apr 12, 2023 at 1:11 AM Joy Chakraborty <joychakr@google.com> wrote:
> > > On Tue, Apr 11, 2023 at 9:52 PM Andy Shevchenko
> > > <andriy.shevchenko@intel.com> wrote:
> > > > On Tue, Apr 11, 2023 at 08:48:52PM +0530, Joy Chakraborty wrote:
> > > > > On Tue, Apr 11, 2023 at 8:47 PM Andy Shevchenko
> > > > > <andriy.shevchenko@intel.com> wrote:
> > > > > > On Tue, Apr 11, 2023 at 06:09:16PM +0300, Serge Semin wrote:
> > > > > > > On Tue, Apr 11, 2023 at 05:46:34PM +0300, Andy Shevchenko wrote:
> > > > > > > > On Tue, Apr 11, 2023 at 05:11:15PM +0300, Serge Semin wrote:
> > > > > > > > > On Tue, Apr 11, 2023 at 03:13:49PM +0300, Andy Shevchenko wrote:
> > > > > > > > > > On Thu, Mar 30, 2023 at 06:34:49AM +0000, Joy Chakraborty wrote:
>
> ...
>
> > > > > > > > > > > -     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:
> > > > > > > > > >
> > > > > > > > > > I'm not sure about this.
> > > > > > > > >
> > > > > > > > > This actually makes sense seeing the function argument can have values
> > > > > > > > > 1, 2, _3_ and 4:
> > > > > > > > > dws->n_bytes = DIV_ROUND_UP(transfer->bits_per_word, BITS_PER_BYTE);
> > > > > > > > > transfer->bits_per_word = __F__(master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32));
> > > > > > > > > ...
> > > > > > > > > dw_spi_dma_convert_width(dws->n_bytes)
> > > > > > > > >
> > > > > > > > > The spi_transfer.bits_per_word field value depends on the
> > > > > > > > > SPI peripheral device communication protocol requirements which may
> > > > > > > > > imply the 3-bytes word xfers (even though it's indeed unluckily).
> > > > > > > > >
> > > > > > > > > This semantic will also match to what we currently have in the
> > > > > > > > > IRQ-based SPI-transfer implementation (see dw_writer() and
> > > > > > > > > dw_reader()).
> > > > > > >
> > > > > > > > Nice, but we have DMA_SLAVE_BUSWIDTH_3_BYTES definition for that. Why we don't
> > > > > > > > use it?
> > > > > > >
> > > > > > > We could but there are two more-or-less firm reasons not to do
> > > > > > > that:
> > > > > > > 1. There aren't that much DMA-engines with the
> > > > > > > DMA_SLAVE_BUSWIDTH_3_BYTES capability meanwhile the DW APB SSI just
> > > > > > > ignores the upper bits if CTRLR0.DFS is less than the value actual
> > > > > > > written to the DR registers. Note DW DMAC engine isn't one of such
> > > > > > > controllers. So if we get to meet a peripheral SPI-device with 3-bytes
> > > > > > > word protocol transfers and the DMA-engine doesn't support it the
> > > > > > > DMA-based transfers may fail (depending on the DMA-engine driver
> > > > > > > implementation).
> > > > > > > 2. The DW APB SSIs (3.x and 4.x) can be synthesized with the APB Data
> > > > > > > Bus Width of 8, 16 and 32. So no matter whether DMA-engine supports
> > > > > > > the 3-bytes bus width the system bus most likely will either convert
> > > > > > > the transfers to the proper sized bus-transactions or fail.
> > > > > > >
> > > > > > > So taking all of the above into account not using the
> > > > > > > DMA_SLAVE_BUSWIDTH_3_BYTES macro here seems better than using it with
> > > > > > > a risk to fail some of the platform setups especially seeing the DW
> > > > > > > APB SSI ignores the upper bits anyway.
> > > > > >
> > > > > > But this is not about SPI host hardware, it's about the consumers.
> > > > > > They should know about supported sizes. Either we add the corresponding support
> > > > > > to the driver or remove 3 case as I suggested. I don't think it's correct to
> > > > > > use 3 as 4.
> > > > >
> > > > > Another thing to add is that as per spi.h even if bits per word is a
> > > > > not a power of 2 the buffer should be formatted in memory as a power
> > > > > of 2
> > > > > ...
> > > > >  * @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 for n_bytes = 3 or 24 bits/per word we expect the client SW to
> > > > > format it to 4 byte buffers hence the transaction generated should
> > > > > also be of size 4 from the DMA.
> > > >
> > > > You didn't get my point. The consumer wants to know if the 3 bytes is supported
> > > > or not, that's should be part of the DMA related thing, right?
> > > >
> > > > It's incorrectly to say 4 for 3 if the backend DMA controller behaves differently
> > > > on this. How do you know that (any) DMA controller integrated with this hardware
> > > > has no side effects for this change.
> > > >
> > > > So, I don't think the case 3 is correct in this patch.
> > >
> > > I see, I am of the opposite opinion in this case.
> > >
> > > Other then Serge(y)'s points,
> > > I was trying to say that irrespective of what the underlying DMA
> > > controller supports we should use DMA_SLAVE_BUSWIDTH_4_BYTES when
> > > n_bytes = 3 from SPI perspective as we get n_bytes from bits per word
> > > passed by the client / spi framework " dws->n_bytes =
> > > DIV_ROUND_UP(transfer->bits_per_word, BITS_PER_BYTE) ".
> > > Based on the spi header what I perceive is that for bits/word between
> > > 17 and 32 the data has to be stored in 32bit words in memory as per
> > > the example in the header " (e.g. 20 bit samples use 32 bits) ".
> > >
> > > Hence, taking an example to transfer 6 bytes (say 0xAA 0xBB 0xCC 0xDD
> > > 0xEE 0xFF) with bits per word as 24 (n_bytes = 3) i.e. a total of 2
> > > words I expect the buffer to look as follows which is coming from the
> > > client:
> > > _ _____address|__________0________4________8________C
> > >     SD:00000000|>00CCBBAA 00FFEEDD 00000000 00000000
> > > Hence to transfer this successfully the DMA controller would need to
> > > copy 4 bytes per word .
> > >
> > > Please correct me if my understanding of this is incorrect.
>
> Thank you for finding the answer for me by yourself!
>
> > On the other hand I do see that in the fifo driver dw_writer() /
> > dw_reader() increments the pointer with 3 incase n_bytes = 3 even
> > though it copies 4 bytes.
> > ...
> >    if (dws->n_bytes == 1)
> >       txw = *(u8 *)(dws->tx);
> >    else if (dws->n_bytes == 2)
> >       txw = *(u16 *)(dws->tx);
> >    else
> >       txw = *(u32 *)(dws->tx);
> >
> > dws->tx += dws->n_bytes;
> > ...
> > This will not behave as using DMA_SLAVE_BUSWIDTH_4_BYTES in the DMA so
> > maybe I am not correct in interpretting the spi.h header file.
> > Can CPU's in general access u32 from unaligned odd addresses ?
>
> Generally speaking the above code must check number of bytes for being 4.
>
> > From Serge(y)'s comment regarding this, the APB interface writing data
> > to the FIFO register definitely cannot handle
> > DMA_SLAVE_BUSWIDTH_3_BYTES since it handles a power of 2 only.
> > Hence we can possibly remove "case 3:" completely and also mask out
> > DMA_SLAVE_BUSWIDTH_3_BYTES from dma_addr_width capabilities so that
> > can_dma api does not allow n_bytes = 3 to use DMA.
> >
> > Would that be correct ?
>
> We have to fix the above and the DIV_ROUND_UP(transfer->bits_per_word,
> BITS_PER_BYTE) one to be something like
>
> roundup_pow_of_two(round_up(..., BITS_PER_BYTE))
>

Hello Serge(y),

Are we okay in removing n_bytes=3 support from the dma driver switch case ?
This will also lead to can_dma returning false for n_bytes=3 which
will essentially make it fall back to fifo mode.

> > > > > > > > > > > +     case 4:
> > > > > > > > > > > +             return DMA_SLAVE_BUSWIDTH_4_BYTES;
> > > > > > > > > > > +     default:
> > > > > > > > > > > +             return DMA_SLAVE_BUSWIDTH_UNDEFINED;
> > > > > > > > > > > +     }
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v5 1/2] spi: dw: Add 32 bpw support to DW DMA Controller

[Serge Semin]

Hi Joy

On Thu, Apr 13, 2023 at 09:37:35AM +0530, Joy Chakraborty wrote:
> On Wed, Apr 12, 2023 at 6:55 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> >
> > On Wed, Apr 12, 2023 at 02:51:44PM +0530, Joy Chakraborty wrote:
> > > On Wed, Apr 12, 2023 at 1:11 AM Joy Chakraborty <joychakr@google.com> wrote:
> > > > On Tue, Apr 11, 2023 at 9:52 PM Andy Shevchenko
> > > > <andriy.shevchenko@intel.com> wrote:
> > > > > On Tue, Apr 11, 2023 at 08:48:52PM +0530, Joy Chakraborty wrote:
> > > > > > On Tue, Apr 11, 2023 at 8:47 PM Andy Shevchenko
> > > > > > <andriy.shevchenko@intel.com> wrote:
> > > > > > > On Tue, Apr 11, 2023 at 06:09:16PM +0300, Serge Semin wrote:
> > > > > > > > On Tue, Apr 11, 2023 at 05:46:34PM +0300, Andy Shevchenko wrote:
> > > > > > > > > On Tue, Apr 11, 2023 at 05:11:15PM +0300, Serge Semin wrote:
> > > > > > > > > > On Tue, Apr 11, 2023 at 03:13:49PM +0300, Andy Shevchenko wrote:
> > > > > > > > > > > On Thu, Mar 30, 2023 at 06:34:49AM +0000, Joy Chakraborty wrote:
> >
> > ...
> >
> > > > > > > > > > > > -     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:
> > > > > > > > > > >
> > > > > > > > > > > I'm not sure about this.
> > > > > > > > > >
> > > > > > > > > > This actually makes sense seeing the function argument can have values
> > > > > > > > > > 1, 2, _3_ and 4:
> > > > > > > > > > dws->n_bytes = DIV_ROUND_UP(transfer->bits_per_word, BITS_PER_BYTE);
> > > > > > > > > > transfer->bits_per_word = __F__(master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32));
> > > > > > > > > > ...
> > > > > > > > > > dw_spi_dma_convert_width(dws->n_bytes)
> > > > > > > > > >
> > > > > > > > > > The spi_transfer.bits_per_word field value depends on the
> > > > > > > > > > SPI peripheral device communication protocol requirements which may
> > > > > > > > > > imply the 3-bytes word xfers (even though it's indeed unluckily).
> > > > > > > > > >
> > > > > > > > > > This semantic will also match to what we currently have in the
> > > > > > > > > > IRQ-based SPI-transfer implementation (see dw_writer() and
> > > > > > > > > > dw_reader()).
> > > > > > > >
> > > > > > > > > Nice, but we have DMA_SLAVE_BUSWIDTH_3_BYTES definition for that. Why we don't
> > > > > > > > > use it?
> > > > > > > >
> > > > > > > > We could but there are two more-or-less firm reasons not to do
> > > > > > > > that:
> > > > > > > > 1. There aren't that much DMA-engines with the
> > > > > > > > DMA_SLAVE_BUSWIDTH_3_BYTES capability meanwhile the DW APB SSI just
> > > > > > > > ignores the upper bits if CTRLR0.DFS is less than the value actual
> > > > > > > > written to the DR registers. Note DW DMAC engine isn't one of such
> > > > > > > > controllers. So if we get to meet a peripheral SPI-device with 3-bytes
> > > > > > > > word protocol transfers and the DMA-engine doesn't support it the
> > > > > > > > DMA-based transfers may fail (depending on the DMA-engine driver
> > > > > > > > implementation).
> > > > > > > > 2. The DW APB SSIs (3.x and 4.x) can be synthesized with the APB Data
> > > > > > > > Bus Width of 8, 16 and 32. So no matter whether DMA-engine supports
> > > > > > > > the 3-bytes bus width the system bus most likely will either convert
> > > > > > > > the transfers to the proper sized bus-transactions or fail.
> > > > > > > >
> > > > > > > > So taking all of the above into account not using the
> > > > > > > > DMA_SLAVE_BUSWIDTH_3_BYTES macro here seems better than using it with
> > > > > > > > a risk to fail some of the platform setups especially seeing the DW
> > > > > > > > APB SSI ignores the upper bits anyway.
> > > > > > >
> > > > > > > But this is not about SPI host hardware, it's about the consumers.
> > > > > > > They should know about supported sizes. Either we add the corresponding support
> > > > > > > to the driver or remove 3 case as I suggested. I don't think it's correct to
> > > > > > > use 3 as 4.
> > > > > >
> > > > > > Another thing to add is that as per spi.h even if bits per word is a
> > > > > > not a power of 2 the buffer should be formatted in memory as a power
> > > > > > of 2
> > > > > > ...
> > > > > >  * @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 for n_bytes = 3 or 24 bits/per word we expect the client SW to
> > > > > > format it to 4 byte buffers hence the transaction generated should
> > > > > > also be of size 4 from the DMA.
> > > > >
> > > > > You didn't get my point. The consumer wants to know if the 3 bytes is supported
> > > > > or not, that's should be part of the DMA related thing, right?
> > > > >
> > > > > It's incorrectly to say 4 for 3 if the backend DMA controller behaves differently
> > > > > on this. How do you know that (any) DMA controller integrated with this hardware
> > > > > has no side effects for this change.
> > > > >
> > > > > So, I don't think the case 3 is correct in this patch.
> > > >
> > > > I see, I am of the opposite opinion in this case.
> > > >
> > > > Other then Serge(y)'s points,
> > > > I was trying to say that irrespective of what the underlying DMA
> > > > controller supports we should use DMA_SLAVE_BUSWIDTH_4_BYTES when
> > > > n_bytes = 3 from SPI perspective as we get n_bytes from bits per word
> > > > passed by the client / spi framework " dws->n_bytes =
> > > > DIV_ROUND_UP(transfer->bits_per_word, BITS_PER_BYTE) ".
> > > > Based on the spi header what I perceive is that for bits/word between
> > > > 17 and 32 the data has to be stored in 32bit words in memory as per
> > > > the example in the header " (e.g. 20 bit samples use 32 bits) ".
> > > >
> > > > Hence, taking an example to transfer 6 bytes (say 0xAA 0xBB 0xCC 0xDD
> > > > 0xEE 0xFF) with bits per word as 24 (n_bytes = 3) i.e. a total of 2
> > > > words I expect the buffer to look as follows which is coming from the
> > > > client:
> > > > _ _____address|__________0________4________8________C
> > > >     SD:00000000|>00CCBBAA 00FFEEDD 00000000 00000000
> > > > Hence to transfer this successfully the DMA controller would need to
> > > > copy 4 bytes per word .
> > > >
> > > > Please correct me if my understanding of this is incorrect.
> >
> > Thank you for finding the answer for me by yourself!
> >
> > > On the other hand I do see that in the fifo driver dw_writer() /
> > > dw_reader() increments the pointer with 3 incase n_bytes = 3 even
> > > though it copies 4 bytes.
> > > ...
> > >    if (dws->n_bytes == 1)
> > >       txw = *(u8 *)(dws->tx);
> > >    else if (dws->n_bytes == 2)
> > >       txw = *(u16 *)(dws->tx);
> > >    else
> > >       txw = *(u32 *)(dws->tx);
> > >
> > > dws->tx += dws->n_bytes;
> > > ...
> > > This will not behave as using DMA_SLAVE_BUSWIDTH_4_BYTES in the DMA so
> > > maybe I am not correct in interpretting the spi.h header file.
> > > Can CPU's in general access u32 from unaligned odd addresses ?
> >
> > Generally speaking the above code must check number of bytes for being 4.
> >
> > > From Serge(y)'s comment regarding this, the APB interface writing data
> > > to the FIFO register definitely cannot handle
> > > DMA_SLAVE_BUSWIDTH_3_BYTES since it handles a power of 2 only.
> > > Hence we can possibly remove "case 3:" completely and also mask out
> > > DMA_SLAVE_BUSWIDTH_3_BYTES from dma_addr_width capabilities so that
> > > can_dma api does not allow n_bytes = 3 to use DMA.
> > >
> > > Would that be correct ?
> >
> > We have to fix the above and the DIV_ROUND_UP(transfer->bits_per_word,
> > BITS_PER_BYTE) one to be something like
> >
> > roundup_pow_of_two(round_up(..., BITS_PER_BYTE))
> >
> 
> Hello Serge(y),
> 
> Are we okay in removing n_bytes=3 support from the dma driver switch case ?
> This will also lead to can_dma returning false for n_bytes=3 which
> will essentially make it fall back to fifo mode.

Yes, I am ok with dropping the "3"-case from
dw_spi_dma_convert_width() in your patch. Could you also provide an
additional patch in your series which would replace
DIV_ROUND_UP(transfer->bits_per_word, BITS_PER_BYTE) with
roundup_pow_of_two() as @Andy suggests? It is supposed to be a bug-fix
AFAICS to the commit a51acc2400d4 ("spi: dw: Add support for 32-bits
max xfer size").

-Serge(y)

> 
> > > > > > > > > > > > +     case 4:
> > > > > > > > > > > > +             return DMA_SLAVE_BUSWIDTH_4_BYTES;
> > > > > > > > > > > > +     default:
> > > > > > > > > > > > +             return DMA_SLAVE_BUSWIDTH_UNDEFINED;
> > > > > > > > > > > > +     }
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
> >

Re: [PATCH v5 1/2] spi: dw: Add 32 bpw support to DW DMA Controller

[Joy Chakraborty]

On Thu, Apr 13, 2023 at 7:17 PM Serge Semin <fancer.lancer@gmail.com> wrote:
>
> Hi Joy
>
> On Thu, Apr 13, 2023 at 09:37:35AM +0530, Joy Chakraborty wrote:
> > On Wed, Apr 12, 2023 at 6:55 PM Andy Shevchenko
> > <andriy.shevchenko@intel.com> wrote:
> > >
> > > On Wed, Apr 12, 2023 at 02:51:44PM +0530, Joy Chakraborty wrote:
> > > > On Wed, Apr 12, 2023 at 1:11 AM Joy Chakraborty <joychakr@google.com> wrote:
> > > > > On Tue, Apr 11, 2023 at 9:52 PM Andy Shevchenko
> > > > > <andriy.shevchenko@intel.com> wrote:
> > > > > > On Tue, Apr 11, 2023 at 08:48:52PM +0530, Joy Chakraborty wrote:
> > > > > > > On Tue, Apr 11, 2023 at 8:47 PM Andy Shevchenko
> > > > > > > <andriy.shevchenko@intel.com> wrote:
> > > > > > > > On Tue, Apr 11, 2023 at 06:09:16PM +0300, Serge Semin wrote:
> > > > > > > > > On Tue, Apr 11, 2023 at 05:46:34PM +0300, Andy Shevchenko wrote:
> > > > > > > > > > On Tue, Apr 11, 2023 at 05:11:15PM +0300, Serge Semin wrote:
> > > > > > > > > > > On Tue, Apr 11, 2023 at 03:13:49PM +0300, Andy Shevchenko wrote:
> > > > > > > > > > > > On Thu, Mar 30, 2023 at 06:34:49AM +0000, Joy Chakraborty wrote:
> > >
> > > ...
> > >
> > > > > > > > > > > > > -     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:
> > > > > > > > > > > >
> > > > > > > > > > > > I'm not sure about this.
> > > > > > > > > > >
> > > > > > > > > > > This actually makes sense seeing the function argument can have values
> > > > > > > > > > > 1, 2, _3_ and 4:
> > > > > > > > > > > dws->n_bytes = DIV_ROUND_UP(transfer->bits_per_word, BITS_PER_BYTE);
> > > > > > > > > > > transfer->bits_per_word = __F__(master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32));
> > > > > > > > > > > ...
> > > > > > > > > > > dw_spi_dma_convert_width(dws->n_bytes)
> > > > > > > > > > >
> > > > > > > > > > > The spi_transfer.bits_per_word field value depends on the
> > > > > > > > > > > SPI peripheral device communication protocol requirements which may
> > > > > > > > > > > imply the 3-bytes word xfers (even though it's indeed unluckily).
> > > > > > > > > > >
> > > > > > > > > > > This semantic will also match to what we currently have in the
> > > > > > > > > > > IRQ-based SPI-transfer implementation (see dw_writer() and
> > > > > > > > > > > dw_reader()).
> > > > > > > > >
> > > > > > > > > > Nice, but we have DMA_SLAVE_BUSWIDTH_3_BYTES definition for that. Why we don't
> > > > > > > > > > use it?
> > > > > > > > >
> > > > > > > > > We could but there are two more-or-less firm reasons not to do
> > > > > > > > > that:
> > > > > > > > > 1. There aren't that much DMA-engines with the
> > > > > > > > > DMA_SLAVE_BUSWIDTH_3_BYTES capability meanwhile the DW APB SSI just
> > > > > > > > > ignores the upper bits if CTRLR0.DFS is less than the value actual
> > > > > > > > > written to the DR registers. Note DW DMAC engine isn't one of such
> > > > > > > > > controllers. So if we get to meet a peripheral SPI-device with 3-bytes
> > > > > > > > > word protocol transfers and the DMA-engine doesn't support it the
> > > > > > > > > DMA-based transfers may fail (depending on the DMA-engine driver
> > > > > > > > > implementation).
> > > > > > > > > 2. The DW APB SSIs (3.x and 4.x) can be synthesized with the APB Data
> > > > > > > > > Bus Width of 8, 16 and 32. So no matter whether DMA-engine supports
> > > > > > > > > the 3-bytes bus width the system bus most likely will either convert
> > > > > > > > > the transfers to the proper sized bus-transactions or fail.
> > > > > > > > >
> > > > > > > > > So taking all of the above into account not using the
> > > > > > > > > DMA_SLAVE_BUSWIDTH_3_BYTES macro here seems better than using it with
> > > > > > > > > a risk to fail some of the platform setups especially seeing the DW
> > > > > > > > > APB SSI ignores the upper bits anyway.
> > > > > > > >
> > > > > > > > But this is not about SPI host hardware, it's about the consumers.
> > > > > > > > They should know about supported sizes. Either we add the corresponding support
> > > > > > > > to the driver or remove 3 case as I suggested. I don't think it's correct to
> > > > > > > > use 3 as 4.
> > > > > > >
> > > > > > > Another thing to add is that as per spi.h even if bits per word is a
> > > > > > > not a power of 2 the buffer should be formatted in memory as a power
> > > > > > > of 2
> > > > > > > ...
> > > > > > >  * @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 for n_bytes = 3 or 24 bits/per word we expect the client SW to
> > > > > > > format it to 4 byte buffers hence the transaction generated should
> > > > > > > also be of size 4 from the DMA.
> > > > > >
> > > > > > You didn't get my point. The consumer wants to know if the 3 bytes is supported
> > > > > > or not, that's should be part of the DMA related thing, right?
> > > > > >
> > > > > > It's incorrectly to say 4 for 3 if the backend DMA controller behaves differently
> > > > > > on this. How do you know that (any) DMA controller integrated with this hardware
> > > > > > has no side effects for this change.
> > > > > >
> > > > > > So, I don't think the case 3 is correct in this patch.
> > > > >
> > > > > I see, I am of the opposite opinion in this case.
> > > > >
> > > > > Other then Serge(y)'s points,
> > > > > I was trying to say that irrespective of what the underlying DMA
> > > > > controller supports we should use DMA_SLAVE_BUSWIDTH_4_BYTES when
> > > > > n_bytes = 3 from SPI perspective as we get n_bytes from bits per word
> > > > > passed by the client / spi framework " dws->n_bytes =
> > > > > DIV_ROUND_UP(transfer->bits_per_word, BITS_PER_BYTE) ".
> > > > > Based on the spi header what I perceive is that for bits/word between
> > > > > 17 and 32 the data has to be stored in 32bit words in memory as per
> > > > > the example in the header " (e.g. 20 bit samples use 32 bits) ".
> > > > >
> > > > > Hence, taking an example to transfer 6 bytes (say 0xAA 0xBB 0xCC 0xDD
> > > > > 0xEE 0xFF) with bits per word as 24 (n_bytes = 3) i.e. a total of 2
> > > > > words I expect the buffer to look as follows which is coming from the
> > > > > client:
> > > > > _ _____address|__________0________4________8________C
> > > > >     SD:00000000|>00CCBBAA 00FFEEDD 00000000 00000000
> > > > > Hence to transfer this successfully the DMA controller would need to
> > > > > copy 4 bytes per word .
> > > > >
> > > > > Please correct me if my understanding of this is incorrect.
> > >
> > > Thank you for finding the answer for me by yourself!
> > >
> > > > On the other hand I do see that in the fifo driver dw_writer() /
> > > > dw_reader() increments the pointer with 3 incase n_bytes = 3 even
> > > > though it copies 4 bytes.
> > > > ...
> > > >    if (dws->n_bytes == 1)
> > > >       txw = *(u8 *)(dws->tx);
> > > >    else if (dws->n_bytes == 2)
> > > >       txw = *(u16 *)(dws->tx);
> > > >    else
> > > >       txw = *(u32 *)(dws->tx);
> > > >
> > > > dws->tx += dws->n_bytes;
> > > > ...
> > > > This will not behave as using DMA_SLAVE_BUSWIDTH_4_BYTES in the DMA so
> > > > maybe I am not correct in interpretting the spi.h header file.
> > > > Can CPU's in general access u32 from unaligned odd addresses ?
> > >
> > > Generally speaking the above code must check number of bytes for being 4.
> > >
> > > > From Serge(y)'s comment regarding this, the APB interface writing data
> > > > to the FIFO register definitely cannot handle
> > > > DMA_SLAVE_BUSWIDTH_3_BYTES since it handles a power of 2 only.
> > > > Hence we can possibly remove "case 3:" completely and also mask out
> > > > DMA_SLAVE_BUSWIDTH_3_BYTES from dma_addr_width capabilities so that
> > > > can_dma api does not allow n_bytes = 3 to use DMA.
> > > >
> > > > Would that be correct ?
> > >
> > > We have to fix the above and the DIV_ROUND_UP(transfer->bits_per_word,
> > > BITS_PER_BYTE) one to be something like
> > >
> > > roundup_pow_of_two(round_up(..., BITS_PER_BYTE))
> > >
> >
> > Hello Serge(y),
> >
> > Are we okay in removing n_bytes=3 support from the dma driver switch case ?
> > This will also lead to can_dma returning false for n_bytes=3 which
> > will essentially make it fall back to fifo mode.
>
> Yes, I am ok with dropping the "3"-case from
> dw_spi_dma_convert_width() in your patch. Could you also provide an
> additional patch in your series which would replace
> DIV_ROUND_UP(transfer->bits_per_word, BITS_PER_BYTE) with
> roundup_pow_of_two() as @Andy suggests? It is supposed to be a bug-fix
> AFAICS to the commit a51acc2400d4 ("spi: dw: Add support for 32-bits
> max xfer size").
>
> -Serge(y)
>

Sure, I will update this patch series and resend.

Thanks
Joy
> >
> > > > > > > > > > > > > +     case 4:
> > > > > > > > > > > > > +             return DMA_SLAVE_BUSWIDTH_4_BYTES;
> > > > > > > > > > > > > +     default:
> > > > > > > > > > > > > +             return DMA_SLAVE_BUSWIDTH_UNDEFINED;
> > > > > > > > > > > > > +     }
> > >
> > > --
> > > With Best Regards,
> > > Andy Shevchenko
> > >
> > >

[PATCH v5 2/2] spi: dw: Add dma controller capability checks

[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
and store addr_width capabilities to check per transfer to make sure the
bits/word requirement can be met for that transfer.

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

diff --git a/drivers/spi/spi-dw-dma.c b/drivers/spi/spi-dw-dma.c
index b3a88bb75907..31e40b054c82 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 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;
@@ -72,12 +74,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);
@@ -87,6 +99,14 @@ static void dw_spi_dma_sg_burst_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
+	 * 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)
@@ -95,6 +115,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 +145,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 +189,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;
@@ -202,8 +233,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 enum dma_slave_buswidth dw_spi_dma_convert_width(u8 n_bytes)
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.423.gd6c402a77b-goog

Re: [PATCH v5 2/2] spi: dw: Add dma controller capability checks

[Andy Shevchenko]

On Thu, Mar 30, 2023 at 06:34:50AM +0000, Joy Chakraborty wrote:
> Check capabilities of DMA controller during init to make sure it is
> capable of handling MEM2DEV for tx channel, DEV2MEM for rx channel
> and store addr_width capabilities to check per transfer to make sure the
> bits/word requirement can be met for that transfer.

...

> +static enum dma_slave_buswidth dw_spi_dma_convert_width(u8 n_bytes);

Can we avoid forward declarations please?

...

> +	if (!(tx.directions & BIT(DMA_MEM_TO_DEV) &&
> +	      rx.directions & BIT(DMA_DEV_TO_MEM)))
> +		return -ENXIO;

What about simplex transfers where we only care about sending or receiving data
and using dummy data for the other channel? Doesn't this make a regression for
that types of transfers? (Or, if we don't support such, this should be explained
in the commit message at least.)

...

> +	/*
> +	 * Assuming both channels belong to the same DMA controller hence the
> +	 * address width capabilities most likely would be the same.
> +	 */
> +	dws->dma_addr_widths = tx.dst_addr_widths & rx.src_addr_widths;

I don't think so this is correct.

Theoretically it's possible to have simplex transfers on which the one of
the channel is simply ignored / not used. See above.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 2/2] spi: dw: Add dma controller capability checks

[Serge Semin]

On Tue, Apr 11, 2023 at 03:18:34PM +0300, Andy Shevchenko wrote:
> On Thu, Mar 30, 2023 at 06:34:50AM +0000, Joy Chakraborty wrote:
> > Check capabilities of DMA controller during init to make sure it is
> > capable of handling MEM2DEV for tx channel, DEV2MEM for rx channel
> > and store addr_width capabilities to check per transfer to make sure the
> > bits/word requirement can be met for that transfer.
> 
> ...
> 
> > +static enum dma_slave_buswidth dw_spi_dma_convert_width(u8 n_bytes);
> 
> Can we avoid forward declarations please?
> 
> ...
> 
> > +	if (!(tx.directions & BIT(DMA_MEM_TO_DEV) &&
> > +	      rx.directions & BIT(DMA_DEV_TO_MEM)))
> > +		return -ENXIO;
> 

> What about simplex transfers where we only care about sending or receiving data
> and using dummy data for the other channel? Doesn't this make a regression for
> that types of transfers? (Or, if we don't support such, this should be explained
> in the commit message at least.)

I don't think the code above is that much relevant for the half-duplex
transfers. The DW APB SSI-DMA driver requires both Tx and Rx channels
being specified thus supporting the Full-duplex transfers at least in
case of the TxRx and Rx-only SPI-transfers (the later case relies on
having the dummy buffers supplied by the SPI-core). Thus the channels
must support the corresponding DMA-directions.

Indeed the Tx-only DMA-based SPI-transfers implementation in the
driver implies not using the Rx DMA-channel, but even in that case the
Rx-channel still needs to be specified otherwise the DW APB SSI-DMA
setup methods will halt with error returned. So unless there are cases
with dummy Rx DMA-channels (which I very much doubt there is) I don't
see the suggested update causing a regression. Am I missing something?

> 
> ...
> 
> > +	/*
> > +	 * Assuming both channels belong to the same DMA controller hence the
> > +	 * address width capabilities most likely would be the same.
> > +	 */
> > +	dws->dma_addr_widths = tx.dst_addr_widths & rx.src_addr_widths;
> 

> I don't think so this is correct.
> 
> Theoretically it's possible to have simplex transfers on which the one of
> the channel is simply ignored / not used. See above.

Please see my explanation above. To cut it short even in case of the
half-duplex SPI-xfers both channels need to be specified with the
respective capabilities. It's implied by the DW APB SSI-DMA setup
methods design (see dw_spi_dma_init_mfld() and
dw_spi_dma_init_generic()).

So until the DW APB SSI-DMA driver is re-developed to supporting true
Tx-only and Rx-only transfers with no requirement one of the channels
being specified I don't see any problem with the code above. Do you
still think otherwise?

-Serge(y)

> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH v5 2/2] spi: dw: Add dma controller capability checks

[Serge Semin]

On Tue, Apr 11, 2023 at 05:38:01PM +0300, Serge Semin wrote:
> On Tue, Apr 11, 2023 at 03:18:34PM +0300, Andy Shevchenko wrote:
> > On Thu, Mar 30, 2023 at 06:34:50AM +0000, Joy Chakraborty wrote:
> > > Check capabilities of DMA controller during init to make sure it is
> > > capable of handling MEM2DEV for tx channel, DEV2MEM for rx channel
> > > and store addr_width capabilities to check per transfer to make sure the
> > > bits/word requirement can be met for that transfer.
> > 
> > ...
> > 
> > > +static enum dma_slave_buswidth dw_spi_dma_convert_width(u8 n_bytes);
> > 
> > Can we avoid forward declarations please?
> > 
> > ...
> > 
> > > +	if (!(tx.directions & BIT(DMA_MEM_TO_DEV) &&
> > > +	      rx.directions & BIT(DMA_DEV_TO_MEM)))
> > > +		return -ENXIO;
> > 
> 
> > What about simplex transfers where we only care about sending or receiving data
> > and using dummy data for the other channel? Doesn't this make a regression for
> > that types of transfers? (Or, if we don't support such, this should be explained
> > in the commit message at least.)
> 
> I don't think the code above is that much relevant for the half-duplex
> transfers. The DW APB SSI-DMA driver requires both Tx and Rx channels
> being specified thus supporting the Full-duplex transfers at least in
> case of the TxRx and Rx-only SPI-transfers (the later case relies on
> having the dummy buffers supplied by the SPI-core). Thus the channels
> must support the corresponding DMA-directions.
> 
> Indeed the Tx-only DMA-based SPI-transfers implementation in the
> driver implies not using the Rx DMA-channel, but even in that case the
> Rx-channel still needs to be specified otherwise the DW APB SSI-DMA
> setup methods will halt with error returned. So unless there are cases
> with dummy Rx DMA-channels (which I very much doubt there is) I don't
> see the suggested update causing a regression. Am I missing something?
> 
> > 
> > ...
> > 
> > > +	/*
> > > +	 * Assuming both channels belong to the same DMA controller hence the
> > > +	 * address width capabilities most likely would be the same.
> > > +	 */
> > > +	dws->dma_addr_widths = tx.dst_addr_widths & rx.src_addr_widths;
> > 
> 
> > I don't think so this is correct.
> > 
> > Theoretically it's possible to have simplex transfers on which the one of
> > the channel is simply ignored / not used. See above.
> 
> Please see my explanation above. To cut it short even in case of the
> half-duplex SPI-xfers both channels need to be specified with the
> respective capabilities. It's implied by the DW APB SSI-DMA setup
> methods design (see dw_spi_dma_init_mfld() and
> dw_spi_dma_init_generic()).
> 
> So until the DW APB SSI-DMA driver is re-developed to supporting true

> Tx-only and Rx-only transfers with no requirement one of the channels

s/with no requirement one of the channels/with no requirement of both the channels

-Serge(y)

> being specified I don't see any problem with the code above. Do you
> still think otherwise?
> 
> -Serge(y)
> 
> > 
> > -- 
> > With Best Regards,
> > Andy Shevchenko
> > 
> > 

Re: [PATCH v5 2/2] spi: dw: Add dma controller capability checks

[Andy Shevchenko]

On Tue, Apr 11, 2023 at 05:37:58PM +0300, Serge Semin wrote:
> On Tue, Apr 11, 2023 at 03:18:34PM +0300, Andy Shevchenko wrote:
> > On Thu, Mar 30, 2023 at 06:34:50AM +0000, Joy Chakraborty wrote:

...

> > > +	if (!(tx.directions & BIT(DMA_MEM_TO_DEV) &&
> > > +	      rx.directions & BIT(DMA_DEV_TO_MEM)))
> > > +		return -ENXIO;
> > 
> 
> > What about simplex transfers where we only care about sending or receiving data
> > and using dummy data for the other channel? Doesn't this make a regression for
> > that types of transfers? (Or, if we don't support such, this should be explained
> > in the commit message at least.)
> 
> I don't think the code above is that much relevant for the half-duplex
> transfers. The DW APB SSI-DMA driver requires both Tx and Rx channels
> being specified thus supporting the Full-duplex transfers at least in
> case of the TxRx and Rx-only SPI-transfers (the later case relies on
> having the dummy buffers supplied by the SPI-core). Thus the channels
> must support the corresponding DMA-directions.
> 
> Indeed the Tx-only DMA-based SPI-transfers implementation in the
> driver implies not using the Rx DMA-channel, but even in that case the
> Rx-channel still needs to be specified otherwise the DW APB SSI-DMA
> setup methods will halt with error returned. So unless there are cases
> with dummy Rx DMA-channels (which I very much doubt there is) I don't
> see the suggested update causing a regression. Am I missing something?

Okay, so since it's not a problem, can we explain this in the commit message
then? A summary of the above, perhaps?

...

> Do you still think otherwise?

Answered above.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 2/2] spi: dw: Add dma controller capability checks

[Joy Chakraborty]

Hello Andy,

On Tue, Apr 11, 2023 at 5:48 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Thu, Mar 30, 2023 at 06:34:50AM +0000, Joy Chakraborty wrote:
> > Check capabilities of DMA controller during init to make sure it is
> > capable of handling MEM2DEV for tx channel, DEV2MEM for rx channel
> > and store addr_width capabilities to check per transfer to make sure the
> > bits/word requirement can be met for that transfer.
>
> ...
>
> > +static enum dma_slave_buswidth dw_spi_dma_convert_width(u8 n_bytes);
>
> Can we avoid forward declarations please?

We can, but for that I would have to move this api somewhere else in
the file is that acceptable?

>
> ...
>
> > +     if (!(tx.directions & BIT(DMA_MEM_TO_DEV) &&
> > +           rx.directions & BIT(DMA_DEV_TO_MEM)))
> > +             return -ENXIO;
>
> What about simplex transfers where we only care about sending or receiving data
> and using dummy data for the other channel? Doesn't this make a regression for
> that types of transfers? (Or, if we don't support such, this should be explained
> in the commit message at least.)
>

Yes we can have simplex transfers for TX only, but for RX only there
is dummy data added by the SPI core as the dw-core driver adds
"SPI_CONTROLLER_MUST_TX".

But transfers aside, as per the current driver design the dw dma
driver needs both valid tx and rx channels to exist and be functional
as per implementation of api "dw_spi_dma_init_generic" :
...
dws->rxchan = dma_request_chan(dev, "rx");
if (IS_ERR(dws->rxchan)) {
ret = PTR_ERR(dws->rxchan);
dws->rxchan = NULL;
goto err_exit;
}

dws->txchan = dma_request_chan(dev, "tx");
if (IS_ERR(dws->txchan)) {
ret = PTR_ERR(dws->txchan);
dws->txchan = NULL;
goto free_rxchan;
}
...

Hence in terms of capability check we should check for directional
capability of both TX and RX is what I understand.
Please let me know if you think otherwise.

> ...
>
> > +     /*
> > +      * Assuming both channels belong to the same DMA controller hence the
> > +      * address width capabilities most likely would be the same.
> > +      */
> > +     dws->dma_addr_widths = tx.dst_addr_widths & rx.src_addr_widths;
>
> I don't think so this is correct.
>
> Theoretically it's possible to have simplex transfers on which the one of
> the channel is simply ignored / not used. See above.
>

Yes, it is possible to have tx only transfers which would still be
possible to do with this implementation. What we have assumed here is
since the tx and rx channel both are a requirement for the dw dma
driver to be functional it would most likely have the same address
width capability.

But we can make this more elaborate by checking it for both tx and rx
separately.
Something like this
...
 dws->tx_dma_addr_widths = tx.dst_addr_widths;
 dws->rx_dma_addr_widths = rx.src_addr_widths;
...
...
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);

if (xfer->rx_buf && !(dws->rx_dma_addr_widths & BIT(dma_bus_width)))
return false;

return dws->tx_dma_addr_widths & BIT(dma_bus_width);
}
...

@Serge Semin Please share your thoughts on the same.

> --
> With Best Regards,
> Andy Shevchenko
>
>

I shall break this into 2 patches as per Serge(y)'s recommendation and
make changes as per replies.

Thanks
Joy

Re: [PATCH v5 2/2] spi: dw: Add dma controller capability checks

[Joy Chakraborty]

Sorry I think the emails crossed.

So as per the discussion, are these the possible changes:
1> Move "dw_spi_dma_convert_width" to avoid forward declaration .
2> Update the commit text to include more explanation.
3> Divide this into 2 patches?

Thanks
Joy

Joy

On Tue, Apr 11, 2023 at 8:30 PM Joy Chakraborty <joychakr@google.com> wrote:
>
> Hello Andy,
>
> On Tue, Apr 11, 2023 at 5:48 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> >
> > On Thu, Mar 30, 2023 at 06:34:50AM +0000, Joy Chakraborty wrote:
> > > Check capabilities of DMA controller during init to make sure it is
> > > capable of handling MEM2DEV for tx channel, DEV2MEM for rx channel
> > > and store addr_width capabilities to check per transfer to make sure the
> > > bits/word requirement can be met for that transfer.
> >
> > ...
> >
> > > +static enum dma_slave_buswidth dw_spi_dma_convert_width(u8 n_bytes);
> >
> > Can we avoid forward declarations please?
>
> We can, but for that I would have to move this api somewhere else in
> the file is that acceptable?
>
> >
> > ...
> >
> > > +     if (!(tx.directions & BIT(DMA_MEM_TO_DEV) &&
> > > +           rx.directions & BIT(DMA_DEV_TO_MEM)))
> > > +             return -ENXIO;
> >
> > What about simplex transfers where we only care about sending or receiving data
> > and using dummy data for the other channel? Doesn't this make a regression for
> > that types of transfers? (Or, if we don't support such, this should be explained
> > in the commit message at least.)
> >
>
> Yes we can have simplex transfers for TX only, but for RX only there
> is dummy data added by the SPI core as the dw-core driver adds
> "SPI_CONTROLLER_MUST_TX".
>
> But transfers aside, as per the current driver design the dw dma
> driver needs both valid tx and rx channels to exist and be functional
> as per implementation of api "dw_spi_dma_init_generic" :
> ...
> dws->rxchan = dma_request_chan(dev, "rx");
> if (IS_ERR(dws->rxchan)) {
> ret = PTR_ERR(dws->rxchan);
> dws->rxchan = NULL;
> goto err_exit;
> }
>
> dws->txchan = dma_request_chan(dev, "tx");
> if (IS_ERR(dws->txchan)) {
> ret = PTR_ERR(dws->txchan);
> dws->txchan = NULL;
> goto free_rxchan;
> }
> ...
>
> Hence in terms of capability check we should check for directional
> capability of both TX and RX is what I understand.
> Please let me know if you think otherwise.
>
> > ...
> >
> > > +     /*
> > > +      * Assuming both channels belong to the same DMA controller hence the
> > > +      * address width capabilities most likely would be the same.
> > > +      */
> > > +     dws->dma_addr_widths = tx.dst_addr_widths & rx.src_addr_widths;
> >
> > I don't think so this is correct.
> >
> > Theoretically it's possible to have simplex transfers on which the one of
> > the channel is simply ignored / not used. See above.
> >
>
> Yes, it is possible to have tx only transfers which would still be
> possible to do with this implementation. What we have assumed here is
> since the tx and rx channel both are a requirement for the dw dma
> driver to be functional it would most likely have the same address
> width capability.
>
> But we can make this more elaborate by checking it for both tx and rx
> separately.
> Something like this
> ...
>  dws->tx_dma_addr_widths = tx.dst_addr_widths;
>  dws->rx_dma_addr_widths = rx.src_addr_widths;
> ...
> ...
> 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);
>
> if (xfer->rx_buf && !(dws->rx_dma_addr_widths & BIT(dma_bus_width)))
> return false;
>
> return dws->tx_dma_addr_widths & BIT(dma_bus_width);
> }
> ...
>
> @Serge Semin Please share your thoughts on the same.
>
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
> >
>
> I shall break this into 2 patches as per Serge(y)'s recommendation and
> make changes as per replies.
>
> Thanks
> Joy

Re: [PATCH v5 2/2] spi: dw: Add dma controller capability checks

[Andy Shevchenko]

On Tue, Apr 11, 2023 at 08:37:33PM +0530, Joy Chakraborty wrote:
> Sorry I think the emails crossed.
> 
> So as per the discussion, are these the possible changes:
> 1> Move "dw_spi_dma_convert_width" to avoid forward declaration .
> 2> Update the commit text to include more explanation.
> 3> Divide this into 2 patches?

Seems okay to me.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 2/2] spi: dw: Add dma controller capability checks

[Serge Semin]

On Tue, Apr 11, 2023 at 06:15:22PM +0300, Andy Shevchenko wrote:
> On Tue, Apr 11, 2023 at 08:37:33PM +0530, Joy Chakraborty wrote:
> > Sorry I think the emails crossed.
> > 
> > So as per the discussion, are these the possible changes:
> > 1> Move "dw_spi_dma_convert_width" to avoid forward declaration .
> > 2> Update the commit text to include more explanation.
> > 3> Divide this into 2 patches?
> 
> Seems okay to me.

To me too.

-Serge(y)

> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH v5 2/2] spi: dw: Add dma controller capability checks

[Andy Shevchenko]

On Tue, Apr 11, 2023 at 08:30:41PM +0530, Joy Chakraborty wrote:
> On Tue, Apr 11, 2023 at 5:48 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Thu, Mar 30, 2023 at 06:34:50AM +0000, Joy Chakraborty wrote:

...

> > > +static enum dma_slave_buswidth dw_spi_dma_convert_width(u8 n_bytes);
> >
> > Can we avoid forward declarations please?
> 
> We can, but for that I would have to move this api somewhere else in
> the file is that acceptable?

Yes, but make it in a separate change as a preparation for this one.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 0/2] spi: dw: DW SPI DMA Driver updates

[Serge Semin]

Cc+=Andy

On Thu, Mar 30, 2023 at 06:34:48AM +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.
> ---
> 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.

The series looks good to me now. Though if I were you I would have
split up the last patch into two ones.

Anyway I tested the patchset on Baikal-T1 SoC with DW APB SSI 3.22b +
DW DMAC 2.18b and looped back SPI-interface. So feel free to add:
Tested-by: Serge Semin <fancer.lancer@gmail.com>
Reviewed-by: Serge Semin <fancer.lancer@gmail.com>

@Andy, anything to add from your side?
@Mark, if you are ok with the series content please merge in.

-Serge(y)

> ---
> 
> Joy Chakraborty (2):
>   spi: dw: Add 32 bpw support to DW DMA Controller
>   spi: dw: Add dma controller capability checks
> 
>  drivers/spi/spi-dw-dma.c | 70 ++++++++++++++++++++++++++++++++--------
>  drivers/spi/spi-dw.h     |  1 +
>  2 files changed, 57 insertions(+), 14 deletions(-)
> 
> -- 
> 2.40.0.423.gd6c402a77b-goog
> 

Re: [PATCH v5 0/2] spi: dw: DW SPI DMA Driver updates

[Andy Shevchenko]

On Tue, Apr 11, 2023 at 08:12:33AM +0300, Serge Semin wrote:
> Cc+=Andy

Thank for Cc'ing me. I'll go through individual patches and give my comments
if any.

> On Thu, Mar 30, 2023 at 06:34:48AM +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.
> > ---
> > 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.
> 
> The series looks good to me now. Though if I were you I would have
> split up the last patch into two ones.
> 
> Anyway I tested the patchset on Baikal-T1 SoC with DW APB SSI 3.22b +
> DW DMAC 2.18b and looped back SPI-interface. So feel free to add:
> Tested-by: Serge Semin <fancer.lancer@gmail.com>
> Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
> 
> @Andy, anything to add from your side?
> @Mark, if you are ok with the series content please merge in.

-- 
With Best Regards,
Andy Shevchenko