| Message ID | 20230330063450.2289058-3-joychakr@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | spi: dw: DW SPI DMA Driver updates | expand |
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.
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 > >
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 > > > >
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.
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
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
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.
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.
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 > >
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;
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(-)