* [PATCH v4 0/2] spi: dw: DW SPI DMA Driver updates @ 2023-03-27 4:34 Joy Chakraborty 2023-03-27 4:34 ` [PATCH v4 1/2] spi: dw: Add 32 bpw support to DW DMA Controller Joy Chakraborty 2023-03-27 4:34 ` [PATCH v4 2/2] spi: dw: Add dma controller capability checks Joy Chakraborty 0 siblings, 2 replies; 6+ messages in thread From: Joy Chakraborty @ 2023-03-27 4:34 UTC (permalink / raw) To: Serge Semin, Mark Brown Cc: linux-spi, linux-kernel, manugautam, rohitner, 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/ --- 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v4 1/2] spi: dw: Add 32 bpw support to DW DMA Controller 2023-03-27 4:34 [PATCH v4 0/2] spi: dw: DW SPI DMA Driver updates Joy Chakraborty @ 2023-03-27 4:34 ` Joy Chakraborty 2023-03-27 4:34 ` [PATCH v4 2/2] spi: dw: Add dma controller capability checks Joy Chakraborty 1 sibling, 0 replies; 6+ messages in thread From: Joy Chakraborty @ 2023-03-27 4:34 UTC (permalink / raw) To: Serge Semin, Mark Brown Cc: linux-spi, linux-kernel, manugautam, rohitner, 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.348.gf938b09366-goog ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v4 2/2] spi: dw: Add dma controller capability checks 2023-03-27 4:34 [PATCH v4 0/2] spi: dw: DW SPI DMA Driver updates Joy Chakraborty 2023-03-27 4:34 ` [PATCH v4 1/2] spi: dw: Add 32 bpw support to DW DMA Controller Joy Chakraborty @ 2023-03-27 4:34 ` Joy Chakraborty 2023-03-28 18:03 ` Serge Semin 1 sibling, 1 reply; 6+ messages in thread From: Joy Chakraborty @ 2023-03-27 4:34 UTC (permalink / raw) To: Serge Semin, Mark Brown Cc: linux-spi, linux-kernel, manugautam, rohitner, 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 | 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v4 2/2] spi: dw: Add dma controller capability checks 2023-03-27 4:34 ` [PATCH v4 2/2] spi: dw: Add dma controller capability checks Joy Chakraborty @ 2023-03-28 18:03 ` Serge Semin 2023-03-29 3:56 ` Joy Chakraborty 0 siblings, 1 reply; 6+ messages in thread From: Serge Semin @ 2023-03-28 18:03 UTC (permalink / raw) To: Joy Chakraborty; +Cc: Mark Brown, linux-spi, linux-kernel, manugautam, rohitner 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 <joychakr@google.com> > --- > 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 2/2] spi: dw: Add dma controller capability checks 2023-03-28 18:03 ` Serge Semin @ 2023-03-29 3:56 ` Joy Chakraborty 2023-03-29 13:35 ` Serge Semin 0 siblings, 1 reply; 6+ messages in thread From: Joy Chakraborty @ 2023-03-29 3:56 UTC (permalink / raw) To: Serge Semin; +Cc: Mark Brown, linux-spi, linux-kernel, manugautam, rohitner Hi Serge(y), On Tue, Mar 28, 2023 at 11:33 PM Serge Semin <fancer.lancer@gmail.com> 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 <joychakr@google.com> > > --- > > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 2/2] spi: dw: Add dma controller capability checks 2023-03-29 3:56 ` Joy Chakraborty @ 2023-03-29 13:35 ` Serge Semin 0 siblings, 0 replies; 6+ messages in thread From: Serge Semin @ 2023-03-29 13:35 UTC (permalink / raw) To: Joy Chakraborty; +Cc: Mark Brown, linux-spi, linux-kernel, manugautam, rohitner 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 <fancer.lancer@gmail.com> 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 <joychakr@google.com> > > > --- > > > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-03-29 13:35 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-03-27 4:34 [PATCH v4 0/2] spi: dw: DW SPI DMA Driver updates Joy Chakraborty 2023-03-27 4:34 ` [PATCH v4 1/2] spi: dw: Add 32 bpw support to DW DMA Controller Joy Chakraborty 2023-03-27 4:34 ` [PATCH v4 2/2] spi: dw: Add dma controller capability checks Joy Chakraborty 2023-03-28 18:03 ` Serge Semin 2023-03-29 3:56 ` Joy Chakraborty 2023-03-29 13:35 ` Serge Semin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox