diff mbox series

[v7,4/5] spi: dw: Add DMA address widths capability check

Message ID 20230418052902.1336866-5-joychakr@google.com
State New
Headers
Series spi: dw: DW SPI DMA Driver updates |

Commit Message

Joy Chakraborty April 18, 2023, 5:29 a.m. UTC 
  Store address width capabilities of DMA controller during init and check
the same per transfer to make sure the bits/word requirement can be met.

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

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

Comments

Andy Shevchenko April 18, 2023, 7:38 a.m. UTC | #1 
On Tue, Apr 18, 2023 at 05:29:01AM +0000, Joy Chakraborty wrote:
> Store address width capabilities of DMA controller during init and check
> the same per transfer to make sure the bits/word requirement can be met.
> 
> Current DW DMA driver requires both tx and rx channel to be configured
> and functional hence a subset of both tx and rx channel address width
> capability is checked with the width requirement(n_bytes) for a
> transfer.

...

> +	/*
> +	 * Assuming both channels belong to the same DMA controller hence the
> +	 * address width capabilities most likely would be the same.
> +	 */

I had a small comment on this In v6 thread.
Joy Chakraborty April 19, 2023, 5:48 a.m. UTC | #2 
On Tue, Apr 18, 2023 at 1:08 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Tue, Apr 18, 2023 at 05:29:01AM +0000, Joy Chakraborty wrote:
> > Store address width capabilities of DMA controller during init and check
> > the same per transfer to make sure the bits/word requirement can be met.
> >
> > Current DW DMA driver requires both tx and rx channel to be configured
> > and functional hence a subset of both tx and rx channel address width
> > capability is checked with the width requirement(n_bytes) for a
> > transfer.
>
> ...
>
> > +     /*
> > +      * Assuming both channels belong to the same DMA controller hence the
> > +      * address width capabilities most likely would be the same.
> > +      */
>
> I had a small comment on this In v6 thread.

Sure,

Your comment in V6 thread:
"
I would add something to explain the side of these address width, like

         * Assuming both channels belong to the same DMA controller hence
         * the peripheral side address width capabilities most likely would
         * be the same.
"

I do not think the address width capabilities are dependent on the
side of generation like memory or peripheral. From what I understand,
address width capabilities are solely dependent on the transaction
generation capability of the DMA controller towards the system bus.
What we intend to highlight here is the assumption that both tx and rx
channel would belong to the same DMA controller hence the transaction
generation capabilities would be the same both for read and write
(e.g. if the DMA controller is able to generate 32 bit sized reads
then it should also be able to generate 32 bit sized writes).
With this assumption we are doing a bitwise and of both tx and rx capabilities.

Please let me know if you think otherwise.

Thanks
Joy

>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Andy Shevchenko April 19, 2023, 11:49 a.m. UTC | #3 
On Wed, Apr 19, 2023 at 11:18:25AM +0530, Joy Chakraborty wrote:
> On Tue, Apr 18, 2023 at 1:08 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Tue, Apr 18, 2023 at 05:29:01AM +0000, Joy Chakraborty wrote:

...

> > > +     /*
> > > +      * Assuming both channels belong to the same DMA controller hence the
> > > +      * address width capabilities most likely would be the same.
> > > +      */
> >
> > I had a small comment on this In v6 thread.
> 
> Sure,
> 
> Your comment in V6 thread:
> "
> I would add something to explain the side of these address width, like
> 
>          * Assuming both channels belong to the same DMA controller hence
>          * the peripheral side address width capabilities most likely would
>          * be the same.
> "
> 
> I do not think the address width capabilities are dependent on the
> side of generation like memory or peripheral.

Yes, they are independent. Memory could do with 4 bytes, while peripheral with
1 byte and so on.

> From what I understand,
> address width capabilities are solely dependent on the transaction
> generation capability of the DMA controller towards the system bus.

What do you mean by a SB in the above? Memory? Peripheral?

> What we intend to highlight here is the assumption that both tx and rx
> channel would belong to the same DMA controller hence the transaction
> generation capabilities would be the same both for read and write
> (e.g. if the DMA controller is able to generate 32 bit sized reads
> then it should also be able to generate 32 bit sized writes).
> With this assumption we are doing a bitwise and of both tx and rx capabilities.
> 
> Please let me know if you think otherwise.
Joy Chakraborty April 19, 2023, 12:48 p.m. UTC | #4 
On Wed, Apr 19, 2023 at 5:19 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Wed, Apr 19, 2023 at 11:18:25AM +0530, Joy Chakraborty wrote:
> > On Tue, Apr 18, 2023 at 1:08 PM Andy Shevchenko
> > <andriy.shevchenko@intel.com> wrote:
> > > On Tue, Apr 18, 2023 at 05:29:01AM +0000, Joy Chakraborty wrote:
>
> ...
>
> > > > +     /*
> > > > +      * Assuming both channels belong to the same DMA controller hence the
> > > > +      * address width capabilities most likely would be the same.
> > > > +      */
> > >
> > > I had a small comment on this In v6 thread.
> >
> > Sure,
> >
> > Your comment in V6 thread:
> > "
> > I would add something to explain the side of these address width, like
> >
> >          * Assuming both channels belong to the same DMA controller hence
> >          * the peripheral side address width capabilities most likely would
> >          * be the same.
> > "
> >
> > I do not think the address width capabilities are dependent on the
> > side of generation like memory or peripheral.
>
> Yes, they are independent. Memory could do with 4 bytes, while peripheral with
> 1 byte and so on.
>
> > From what I understand,
> > address width capabilities are solely dependent on the transaction
> > generation capability of the DMA controller towards the system bus.
>
> What do you mean by a SB in the above? Memory? Peripheral?

By system bus I mean anything that is connecting the Memory, DMA and
the peripheral.
Something like :

          +-----------+          +-------------------+
          |               |           |                        |
          |   DMA    |           | PERIPHERAL |
          |               |           |                         |
          +----^-+---+          +-----+--^---------+
        *** -->| |                         |    |
                  | |                         |    |
<------------+-v--------------------v---+------------->
                    SYSTEM BUS
<---------------------+--^----------------------------->
                            |   |
                            |   |
                     +----v--+-----+
                     |                   |
                     |  MEMORY |
                     |                   |
                     +--------------+
*** : Address width capabilities should be the capability of the DMA
to generate transactions to the system bus on the marked interface
irrespective of whether it is destined for Peripheral or memory is
what I understand.

>
> > What we intend to highlight here is the assumption that both tx and rx
> > channel would belong to the same DMA controller hence the transaction
> > generation capabilities would be the same both for read and write
> > (e.g. if the DMA controller is able to generate 32 bit sized reads
> > then it should also be able to generate 32 bit sized writes).
> > With this assumption we are doing a bitwise and of both tx and rx capabilities.
> >
> > Please let me know if you think otherwise.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Joy Chakraborty April 19, 2023, 2:55 p.m. UTC | #5 
On Wed, Apr 19, 2023 at 6:18 PM Joy Chakraborty <joychakr@google.com> wrote:
>
> On Wed, Apr 19, 2023 at 5:19 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> >
> > On Wed, Apr 19, 2023 at 11:18:25AM +0530, Joy Chakraborty wrote:
> > > On Tue, Apr 18, 2023 at 1:08 PM Andy Shevchenko
> > > <andriy.shevchenko@intel.com> wrote:
> > > > On Tue, Apr 18, 2023 at 05:29:01AM +0000, Joy Chakraborty wrote:
> >
> > ...
> >
> > > > > +     /*
> > > > > +      * Assuming both channels belong to the same DMA controller hence the
> > > > > +      * address width capabilities most likely would be the same.
> > > > > +      */
> > > >
> > > > I had a small comment on this In v6 thread.
> > >
> > > Sure,
> > >
> > > Your comment in V6 thread:
> > > "
> > > I would add something to explain the side of these address width, like
> > >
> > >          * Assuming both channels belong to the same DMA controller hence
> > >          * the peripheral side address width capabilities most likely would
> > >          * be the same.
> > > "
> > >
> > > I do not think the address width capabilities are dependent on the
> > > side of generation like memory or peripheral.
> >
> > Yes, they are independent. Memory could do with 4 bytes, while peripheral with
> > 1 byte and so on.
> >
> > > From what I understand,
> > > address width capabilities are solely dependent on the transaction
> > > generation capability of the DMA controller towards the system bus.
> >
> > What do you mean by a SB in the above? Memory? Peripheral?
>
> By system bus I mean anything that is connecting the Memory, DMA and
> the peripheral.
> Something like :
>
>           +-----------+          +-------------------+
>           |               |           |                        |
>           |   DMA    |           | PERIPHERAL |
>           |               |           |                         |
>           +----^-+---+          +-----+--^---------+
>         *** -->| |                         |    |
>                   | |                         |    |
> <------------+-v--------------------v---+------------->
>                     SYSTEM BUS
> <---------------------+--^----------------------------->
>                             |   |
>                             |   |
>                      +----v--+-----+
>                      |                   |
>                      |  MEMORY |
>                      |                   |
>                      +--------------+
> *** : Address width capabilities should be the capability of the DMA
> to generate transactions to the system bus on the marked interface
> irrespective of whether it is destined for Peripheral or memory is
> what I understand.
>
Looks like the diagram did not come correctly, repasting:
        +----------+          +---------------+
        |          |          |               |
        |   DMA    |          |  PERIPHERAL   |
        |          |          |               |
        +----^-+---+          +-----+--^------+
       ***-->| |                    |  |
             | |                    |  |
             | |                    |  |
<------------+-v--------------------v--+--------------->
                    SYSTEM BUS
<---------------------+--^----------------------------->
                      |  |
                      |  |
                      |  |
                 +----v--+-----+
                 |   MEMORY    |
                 |             |
                 +-------------+

> >
> > > What we intend to highlight here is the assumption that both tx and rx
> > > channel would belong to the same DMA controller hence the transaction
> > > generation capabilities would be the same both for read and write
> > > (e.g. if the DMA controller is able to generate 32 bit sized reads
> > > then it should also be able to generate 32 bit sized writes).
> > > With this assumption we are doing a bitwise and of both tx and rx capabilities.
> > >
> > > Please let me know if you think otherwise.
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
> >
Andy Shevchenko April 19, 2023, 5:35 p.m. UTC | #6 
On Wed, Apr 19, 2023 at 06:18:04PM +0530, Joy Chakraborty wrote:
> On Wed, Apr 19, 2023 at 5:19 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Wed, Apr 19, 2023 at 11:18:25AM +0530, Joy Chakraborty wrote:
> > > On Tue, Apr 18, 2023 at 1:08 PM Andy Shevchenko
> > > <andriy.shevchenko@intel.com> wrote:
> > > > On Tue, Apr 18, 2023 at 05:29:01AM +0000, Joy Chakraborty wrote:

...

> > > > > +     /*
> > > > > +      * Assuming both channels belong to the same DMA controller hence the
> > > > > +      * address width capabilities most likely would be the same.
> > > > > +      */
> > > >
> > > > I had a small comment on this In v6 thread.
> > >
> > > Sure,
> > >
> > > Your comment in V6 thread:
> > > "
> > > I would add something to explain the side of these address width, like
> > >
> > >          * Assuming both channels belong to the same DMA controller hence
> > >          * the peripheral side address width capabilities most likely would
> > >          * be the same.
> > > "
> > >
> > > I do not think the address width capabilities are dependent on the
> > > side of generation like memory or peripheral.
> >
> > Yes, they are independent. Memory could do with 4 bytes, while peripheral with
> > 1 byte and so on.
> >
> > > From what I understand,
> > > address width capabilities are solely dependent on the transaction
> > > generation capability of the DMA controller towards the system bus.
> >
> > What do you mean by a SB in the above? Memory? Peripheral?
> 
> By system bus I mean anything that is connecting the Memory, DMA and
> the peripheral.
> Something like :
> 
>           +-----------+          +-------------------+
>           |               |           |                        |
>           |   DMA    |           | PERIPHERAL |
>           |               |           |                         |
>           +----^-+---+          +-----+--^---------+
>         *** -->| |                         |    |
>                   | |                         |    |
> <------------+-v--------------------v---+------------->
>                     SYSTEM BUS
> <---------------------+--^----------------------------->
>                             |   |
>                             |   |
>                      +----v--+-----+
>                      |                   |
>                      |  MEMORY |
>                      |                   |
>                      +--------------+
> *** : Address width capabilities should be the capability of the DMA
> to generate transactions to the system bus on the marked interface
> irrespective of whether it is destined for Peripheral or memory is
> what I understand.

That's misunderstanding. You used only one possible HW design, there may be
more. For example we have Synopsys DesignWare DMA that has a lot of parameters
to configure bus mastering. One of such a case, where it makes a lot of sense,
is DesignWare SATA with the above mentioned DMA controller where it has two
masters and they are connected towards memory and towards peripheral "buses".
They have _different_ configurations.

So, generally speaking what you are saying is not true.

> > > What we intend to highlight here is the assumption that both tx and rx
> > > channel would belong to the same DMA controller hence the transaction
> > > generation capabilities would be the same both for read and write
> > > (e.g. if the DMA controller is able to generate 32 bit sized reads
> > > then it should also be able to generate 32 bit sized writes).
> > > With this assumption we are doing a bitwise and of both tx and rx capabilities.
> > >
> > > Please let me know if you think otherwise.
Joy Chakraborty April 19, 2023, 9:03 p.m. UTC | #7 
On Wed, Apr 19, 2023 at 11:05 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Wed, Apr 19, 2023 at 06:18:04PM +0530, Joy Chakraborty wrote:
> > On Wed, Apr 19, 2023 at 5:19 PM Andy Shevchenko
> > <andriy.shevchenko@intel.com> wrote:
> > > On Wed, Apr 19, 2023 at 11:18:25AM +0530, Joy Chakraborty wrote:
> > > > On Tue, Apr 18, 2023 at 1:08 PM Andy Shevchenko
> > > > <andriy.shevchenko@intel.com> wrote:
> > > > > On Tue, Apr 18, 2023 at 05:29:01AM +0000, Joy Chakraborty wrote:
>
> ...
>
> > > > > > +     /*
> > > > > > +      * Assuming both channels belong to the same DMA controller hence the
> > > > > > +      * address width capabilities most likely would be the same.
> > > > > > +      */
> > > > >
> > > > > I had a small comment on this In v6 thread.
> > > >
> > > > Sure,
> > > >
> > > > Your comment in V6 thread:
> > > > "
> > > > I would add something to explain the side of these address width, like
> > > >
> > > >          * Assuming both channels belong to the same DMA controller hence
> > > >          * the peripheral side address width capabilities most likely would
> > > >          * be the same.
> > > > "
> > > >
> > > > I do not think the address width capabilities are dependent on the
> > > > side of generation like memory or peripheral.
> > >
> > > Yes, they are independent. Memory could do with 4 bytes, while peripheral with
> > > 1 byte and so on.
> > >
> > > > From what I understand,
> > > > address width capabilities are solely dependent on the transaction
> > > > generation capability of the DMA controller towards the system bus.
> > >
> > > What do you mean by a SB in the above? Memory? Peripheral?
> >
> > By system bus I mean anything that is connecting the Memory, DMA and
> > the peripheral.
> > Something like :
> >
> >           +-----------+          +-------------------+
> >           |               |           |                        |
> >           |   DMA    |           | PERIPHERAL |
> >           |               |           |                         |
> >           +----^-+---+          +-----+--^---------+
> >         *** -->| |                         |    |
> >                   | |                         |    |
> > <------------+-v--------------------v---+------------->
> >                     SYSTEM BUS
> > <---------------------+--^----------------------------->
> >                             |   |
> >                             |   |
> >                      +----v--+-----+
> >                      |                   |
> >                      |  MEMORY |
> >                      |                   |
> >                      +--------------+
> > *** : Address width capabilities should be the capability of the DMA
> > to generate transactions to the system bus on the marked interface
> > irrespective of whether it is destined for Peripheral or memory is
> > what I understand.
>
> That's misunderstanding. You used only one possible HW design, there may be
> more. For example we have Synopsys DesignWare DMA that has a lot of parameters
> to configure bus mastering. One of such a case, where it makes a lot of sense,
> is DesignWare SATA with the above mentioned DMA controller where it has two
> masters and they are connected towards memory and towards peripheral "buses".
> They have _different_ configurations.
>
> So, generally speaking what you are saying is not true.

Got it, thank you for the clarification.
I misunderstood what you meant, that peripheral access can be from a
different path.

I shall add to the comment as you suggested and send another patch.

>
> > > > What we intend to highlight here is the assumption that both tx and rx
> > > > channel would belong to the same DMA controller hence the transaction
> > > > generation capabilities would be the same both for read and write
> > > > (e.g. if the DMA controller is able to generate 32 bit sized reads
> > > > then it should also be able to generate 32 bit sized writes).
> > > > With this assumption we are doing a bitwise and of both tx and rx capabilities.
> > > >
> > > > Please let me know if you think otherwise.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
diff mbox series

Patch

diff --git a/drivers/spi/spi-dw-dma.c b/drivers/spi/spi-dw-dma.c
index 22d0727a3789..45980c46946d 100644
--- a/drivers/spi/spi-dw-dma.c
+++ b/drivers/spi/spi-dw-dma.c
@@ -97,6 +97,14 @@  static int dw_spi_dma_caps_init(struct dw_spi *dws)
 		dws->dma_sg_burst = rx.max_sg_burst;
 	else
 		dws->dma_sg_burst = 0;
+
+	/*
+	 * Assuming both channels belong to the same DMA controller hence the
+	 * address width capabilities most likely would be the same.
+	 */
+	dws->dma_addr_widths = tx.dst_addr_widths & rx.src_addr_widths;
+
+	return 0;
 }
 
 static int dw_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws)
@@ -237,8 +245,14 @@  static bool dw_spi_can_dma(struct spi_controller *master,
 			   struct spi_device *spi, struct spi_transfer *xfer)
 {
 	struct dw_spi *dws = spi_controller_get_devdata(master);
+	enum dma_slave_buswidth dma_bus_width;
+
+	if (xfer->len <= dws->fifo_len)
+		return false;
+
+	dma_bus_width = dw_spi_dma_convert_width(dws->n_bytes);
 
-	return xfer->len > dws->fifo_len;
+	return dws->dma_addr_widths & BIT(dma_bus_width);
 }
 
 static int dw_spi_dma_wait(struct dw_spi *dws, unsigned int len, u32 speed)
diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
index 9e8eb2b52d5c..3962e6dcf880 100644
--- a/drivers/spi/spi-dw.h
+++ b/drivers/spi/spi-dw.h
@@ -190,6 +190,7 @@  struct dw_spi {
 	struct dma_chan		*rxchan;
 	u32			rxburst;
 	u32			dma_sg_burst;
+	u32			dma_addr_widths;
 	unsigned long		dma_chan_busy;
 	dma_addr_t		dma_addr; /* phy address of the Data register */
 	const struct dw_spi_dma_ops *dma_ops;