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 <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
---
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 | 67 +++++++++++++++++++++++++++++++---------
drivers/spi/spi-dw.h | 1 +
2 files changed, 53 insertions(+), 15 deletions(-)
--
2.40.0.348.gf938b09366-goog
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 <[email protected]>
---
drivers/spi/spi-dw-dma.c | 54 ++++++++++++++++++++++++++++++++--------
drivers/spi/spi-dw.h | 1 +
2 files changed, 44 insertions(+), 11 deletions(-)
diff --git a/drivers/spi/spi-dw-dma.c b/drivers/spi/spi-dw-dma.c
index b3a88bb75907..f47483ec369f 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,15 @@ 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)
{
+ int ret;
struct dma_slave_caps tx = {0}, rx = {0};
- dma_get_slave_caps(dws->txchan, &tx);
- dma_get_slave_caps(dws->rxchan, &rx);
+ ret = dma_get_slave_caps(dws->txchan, &tx);
+ ret |= dma_get_slave_caps(dws->rxchan, &rx);
+ if (ret)
+ return ret;
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 +92,18 @@ 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;
+
+ if (!(tx.directions & BIT(DMA_MEM_TO_DEV) &&
+ rx.directions & BIT(DMA_DEV_TO_MEM)))
+ return -ENXIO;
+
+ return 0;
}
static int dw_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws)
@@ -95,6 +112,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 +142,24 @@ 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 +185,16 @@ 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 +228,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.348.gf938b09366-goog
On Mon, Mar 27, 2023 at 04:34:05AM +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.
>
> Signed-off-by: Joy Chakraborty <[email protected]>
> ---
> drivers/spi/spi-dw-dma.c | 54 ++++++++++++++++++++++++++++++++--------
> drivers/spi/spi-dw.h | 1 +
> 2 files changed, 44 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/spi/spi-dw-dma.c b/drivers/spi/spi-dw-dma.c
> index b3a88bb75907..f47483ec369f 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,15 @@ 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)
> {
> + int ret;
> struct dma_slave_caps tx = {0}, rx = {0};
1. Preserve the reverse xmas tree order please (driver local convention).
2. The zero-initialization can be dropped since the function will halt
on further procedures if any of the dma_get_slave_caps() calls fail.
Meanwhile if all of them are successfully executed the capability
structures will be sanely initialized.
>
> - dma_get_slave_caps(dws->txchan, &tx);
> - dma_get_slave_caps(dws->rxchan, &rx);
> + ret = dma_get_slave_caps(dws->txchan, &tx);
if (ret)
return ret;
< newline
> + ret |= dma_get_slave_caps(dws->rxchan, &rx);
> + if (ret)
> + return ret;
No OR-ing the errnos please. They aren't bitfields.
>
> 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 +92,18 @@ 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;
> +
> + if (!(tx.directions & BIT(DMA_MEM_TO_DEV) &&
> + rx.directions & BIT(DMA_DEV_TO_MEM)))
> + return -ENXIO;
Please move this to be right after the capabilities are retrieved.
There is no point in the SG-burst and addr-width data initializations
if a DMA-controller isn't suitable for the Tx/Rx DMAs.
-Serge(y)
> +
> + return 0;
> }
>
> static int dw_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws)
> @@ -95,6 +112,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 +142,24 @@ 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 +185,16 @@ 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 +228,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.348.gf938b09366-goog
>
Hi Serge(y),
On Tue, Mar 28, 2023 at 11:33 PM Serge Semin <[email protected]> wrote:
>
> On Mon, Mar 27, 2023 at 04:34:05AM +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.
> >
> > Signed-off-by: Joy Chakraborty <[email protected]>
> > ---
> > drivers/spi/spi-dw-dma.c | 54 ++++++++++++++++++++++++++++++++--------
> > drivers/spi/spi-dw.h | 1 +
> > 2 files changed, 44 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/spi/spi-dw-dma.c b/drivers/spi/spi-dw-dma.c
> > index b3a88bb75907..f47483ec369f 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,15 @@ 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)
> > {
>
> > + int ret;
> > struct dma_slave_caps tx = {0}, rx = {0};
>
> 1. Preserve the reverse xmas tree order please (driver local convention).
> 2. The zero-initialization can be dropped since the function will halt
> on further procedures if any of the dma_get_slave_caps() calls fail.
> Meanwhile if all of them are successfully executed the capability
> structures will be sanely initialized.
>
Sure, will update
> >
> > - dma_get_slave_caps(dws->txchan, &tx);
> > - dma_get_slave_caps(dws->rxchan, &rx);
>
> > + ret = dma_get_slave_caps(dws->txchan, &tx);
> if (ret)
> return ret;
> < newline
> > + ret |= dma_get_slave_caps(dws->rxchan, &rx);
> > + if (ret)
> > + return ret;
>
> No OR-ing the errnos please. They aren't bitfields.
Sure, I will update this.
In this case do you think we need an error print here to differentiate
which slave caps failed ?
>
> >
> > 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 +92,18 @@ 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;
> > +
>
> > + if (!(tx.directions & BIT(DMA_MEM_TO_DEV) &&
> > + rx.directions & BIT(DMA_DEV_TO_MEM)))
> > + return -ENXIO;
>
> Please move this to be right after the capabilities are retrieved.
> There is no point in the SG-burst and addr-width data initializations
> if a DMA-controller isn't suitable for the Tx/Rx DMAs.
On second thought I see that dma_get_slave_caps already checks if
direction exists in dmaengine.c:
...
int dma_get_slave_caps(struct dma_chan *chan, struct dma_slave_caps *caps)
...
if (!device->directions)
return -ENXIO;
...
But it does not check the capability w.r.t the type of channel i.e.
tx/rx , so we can keep this check as well.
Either Way, in case we keep it I shall move this as you suggested.
>
> -Serge(y)
>
> > +
> > + return 0;
> > }
> >
> > static int dw_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws)
> > @@ -95,6 +112,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 +142,24 @@ 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 +185,16 @@ 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 +228,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.348.gf938b09366-goog
> >
Thanks
Joy
On Wed, Mar 29, 2023 at 09:26:47AM +0530, Joy Chakraborty wrote:
> Hi Serge(y),
>
> On Tue, Mar 28, 2023 at 11:33 PM Serge Semin <[email protected]> wrote:
> >
> > On Mon, Mar 27, 2023 at 04:34:05AM +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.
> > >
> > > Signed-off-by: Joy Chakraborty <[email protected]>
> > > ---
> > > drivers/spi/spi-dw-dma.c | 54 ++++++++++++++++++++++++++++++++--------
> > > drivers/spi/spi-dw.h | 1 +
> > > 2 files changed, 44 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/spi/spi-dw-dma.c b/drivers/spi/spi-dw-dma.c
> > > index b3a88bb75907..f47483ec369f 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,15 @@ 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)
> > > {
> >
> > > + int ret;
> > > struct dma_slave_caps tx = {0}, rx = {0};
> >
> > 1. Preserve the reverse xmas tree order please (driver local convention).
> > 2. The zero-initialization can be dropped since the function will halt
> > on further procedures if any of the dma_get_slave_caps() calls fail.
> > Meanwhile if all of them are successfully executed the capability
> > structures will be sanely initialized.
> >
>
> Sure, will update
>
> > >
> > > - dma_get_slave_caps(dws->txchan, &tx);
> > > - dma_get_slave_caps(dws->rxchan, &rx);
> >
> > > + ret = dma_get_slave_caps(dws->txchan, &tx);
> > if (ret)
> > return ret;
> > < newline
> > > + ret |= dma_get_slave_caps(dws->rxchan, &rx);
> > > + if (ret)
> > > + return ret;
> >
> > No OR-ing the errnos please. They aren't bitfields.
>
> Sure, I will update this.
> In this case do you think we need an error print here to differentiate
> which slave caps failed ?
No. dw_spi_add_host() will inform (dev_warn()) about the
DMA-initialization failures. Printing something besides of that
won't help much since in any case an additional debugging would be
necessary.
>
> >
> > >
> > > 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 +92,18 @@ 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;
> > > +
> >
> > > + if (!(tx.directions & BIT(DMA_MEM_TO_DEV) &&
> > > + rx.directions & BIT(DMA_DEV_TO_MEM)))
> > > + return -ENXIO;
> >
> > Please move this to be right after the capabilities are retrieved.
> > There is no point in the SG-burst and addr-width data initializations
> > if a DMA-controller isn't suitable for the Tx/Rx DMAs.
>
> On second thought I see that dma_get_slave_caps already checks if
> direction exists in dmaengine.c:
> ...
> int dma_get_slave_caps(struct dma_chan *chan, struct dma_slave_caps *caps)
> ...
> if (!device->directions)
> return -ENXIO;
> ...
> But it does not check the capability w.r.t the type of channel i.e.
> tx/rx , so we can keep this check as well.
Right. dma_get_slave_caps() doesn't check the particular channel
dir capability which is essential for us in this case. So please keep the
directions check here.
> Either Way, in case we keep it I shall move this as you suggested.
Ok. Thanks.
-Serge(y)
>
> >
> > -Serge(y)
> >
> > > +
> > > + return 0;
> > > }
> > >
> > > static int dw_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws)
> > > @@ -95,6 +112,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 +142,24 @@ 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 +185,16 @@ 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 +228,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.348.gf938b09366-goog
> > >
>
> Thanks
> Joy