A "known issue" during implementation of SE DMA for spi geni driver was
that it does DMA map/unmap internally instead of in spi framework.
Current patches remove this hiccup and also clean up code a bit.
Testing revealed no regressions and results with 1000 iterations of
reading from EC showed no loss of performance.
Results
=======
Before - Iteration 999, min=5.10, max=5.17, avg=5.14, ints=25129
After - Iteration 999, min=5.10, max=5.20, avg=5.15, ints=25153
Vijaya Krishna Nivarthi (2):
soc: qcom: geni-se: Add interfaces geni_se_tx_init_dma() and
geni_se_rx_init_dma()
spi: spi-geni-qcom: Do not do DMA map/unmap inside driver, use
framework instead
---
v1 -> v2:
- Modified interfaces arguments and accordingly calls to them
- Added dma_max_len to driver
drivers/soc/qcom/qcom-geni-se.c | 67 ++++++++++++++++++-------
drivers/spi/spi-geni-qcom.c | 103 +++++++++++++++++++--------------------
include/linux/soc/qcom/geni-se.h | 4 ++
3 files changed, 103 insertions(+), 71 deletions(-)
--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by the Linux Foundation.
The geni_se_xx_dma_prep() interfaces necessarily do DMA mapping before
initiating DMA transfers. This is not suitable for spi where framework
is expected to handle map/unmap.
Expose new interfaces geni_se_xx_init_dma() which do only DMA transfer.
Signed-off-by: Vijaya Krishna Nivarthi <[email protected]>
---
v1 -> v2:
- interfaces to take dma address as argument instead of its pointer
---
drivers/soc/qcom/qcom-geni-se.c | 67 +++++++++++++++++++++++++++++-----------
include/linux/soc/qcom/geni-se.h | 4 +++
2 files changed, 53 insertions(+), 18 deletions(-)
diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
index 795a2e1..dd50a25 100644
--- a/drivers/soc/qcom/qcom-geni-se.c
+++ b/drivers/soc/qcom/qcom-geni-se.c
@@ -682,6 +682,30 @@ EXPORT_SYMBOL(geni_se_clk_freq_match);
#define GENI_SE_DMA_EOT_EN BIT(1)
#define GENI_SE_DMA_AHB_ERR_EN BIT(2)
#define GENI_SE_DMA_EOT_BUF BIT(0)
+
+/**
+ * geni_se_tx_init_dma() - Initiate TX DMA transfer on the serial engine
+ * @se: Pointer to the concerned serial engine.
+ * @iova: Mapped DMA address.
+ * @len: Length of the TX buffer.
+ *
+ * This function is used to initiate DMA TX transfer.
+ */
+void geni_se_tx_init_dma(struct geni_se *se, dma_addr_t iova, size_t len)
+{
+ u32 val;
+
+ val = GENI_SE_DMA_DONE_EN;
+ val |= GENI_SE_DMA_EOT_EN;
+ val |= GENI_SE_DMA_AHB_ERR_EN;
+ writel_relaxed(val, se->base + SE_DMA_TX_IRQ_EN_SET);
+ writel_relaxed(lower_32_bits(iova), se->base + SE_DMA_TX_PTR_L);
+ writel_relaxed(upper_32_bits(iova), se->base + SE_DMA_TX_PTR_H);
+ writel_relaxed(GENI_SE_DMA_EOT_BUF, se->base + SE_DMA_TX_ATTR);
+ writel(len, se->base + SE_DMA_TX_LEN);
+}
+EXPORT_SYMBOL(geni_se_tx_init_dma);
+
/**
* geni_se_tx_dma_prep() - Prepare the serial engine for TX DMA transfer
* @se: Pointer to the concerned serial engine.
@@ -697,7 +721,6 @@ int geni_se_tx_dma_prep(struct geni_se *se, void *buf, size_t len,
dma_addr_t *iova)
{
struct geni_wrapper *wrapper = se->wrapper;
- u32 val;
if (!wrapper)
return -EINVAL;
@@ -706,17 +729,34 @@ int geni_se_tx_dma_prep(struct geni_se *se, void *buf, size_t len,
if (dma_mapping_error(wrapper->dev, *iova))
return -EIO;
+ geni_se_tx_init_dma(se, *iova, len);
+ return 0;
+}
+EXPORT_SYMBOL(geni_se_tx_dma_prep);
+
+/**
+ * geni_se_rx_init_dma() - Initiate RX DMA transfer on the serial engine
+ * @se: Pointer to the concerned serial engine.
+ * @iova: Mapped DMA address.
+ * @len: Length of the RX buffer.
+ *
+ * This function is used to initiate DMA RX transfer.
+ */
+void geni_se_rx_init_dma(struct geni_se *se, dma_addr_t iova, size_t len)
+{
+ u32 val;
+
val = GENI_SE_DMA_DONE_EN;
val |= GENI_SE_DMA_EOT_EN;
val |= GENI_SE_DMA_AHB_ERR_EN;
- writel_relaxed(val, se->base + SE_DMA_TX_IRQ_EN_SET);
- writel_relaxed(lower_32_bits(*iova), se->base + SE_DMA_TX_PTR_L);
- writel_relaxed(upper_32_bits(*iova), se->base + SE_DMA_TX_PTR_H);
- writel_relaxed(GENI_SE_DMA_EOT_BUF, se->base + SE_DMA_TX_ATTR);
- writel(len, se->base + SE_DMA_TX_LEN);
- return 0;
+ writel_relaxed(val, se->base + SE_DMA_RX_IRQ_EN_SET);
+ writel_relaxed(lower_32_bits(iova), se->base + SE_DMA_RX_PTR_L);
+ writel_relaxed(upper_32_bits(iova), se->base + SE_DMA_RX_PTR_H);
+ /* RX does not have EOT buffer type bit. So just reset RX_ATTR */
+ writel_relaxed(0, se->base + SE_DMA_RX_ATTR);
+ writel(len, se->base + SE_DMA_RX_LEN);
}
-EXPORT_SYMBOL(geni_se_tx_dma_prep);
+EXPORT_SYMBOL(geni_se_rx_init_dma);
/**
* geni_se_rx_dma_prep() - Prepare the serial engine for RX DMA transfer
@@ -733,7 +773,6 @@ int geni_se_rx_dma_prep(struct geni_se *se, void *buf, size_t len,
dma_addr_t *iova)
{
struct geni_wrapper *wrapper = se->wrapper;
- u32 val;
if (!wrapper)
return -EINVAL;
@@ -742,15 +781,7 @@ int geni_se_rx_dma_prep(struct geni_se *se, void *buf, size_t len,
if (dma_mapping_error(wrapper->dev, *iova))
return -EIO;
- val = GENI_SE_DMA_DONE_EN;
- val |= GENI_SE_DMA_EOT_EN;
- val |= GENI_SE_DMA_AHB_ERR_EN;
- writel_relaxed(val, se->base + SE_DMA_RX_IRQ_EN_SET);
- writel_relaxed(lower_32_bits(*iova), se->base + SE_DMA_RX_PTR_L);
- writel_relaxed(upper_32_bits(*iova), se->base + SE_DMA_RX_PTR_H);
- /* RX does not have EOT buffer type bit. So just reset RX_ATTR */
- writel_relaxed(0, se->base + SE_DMA_RX_ATTR);
- writel(len, se->base + SE_DMA_RX_LEN);
+ geni_se_rx_init_dma(se, *iova, len);
return 0;
}
EXPORT_SYMBOL(geni_se_rx_dma_prep);
diff --git a/include/linux/soc/qcom/geni-se.h b/include/linux/soc/qcom/geni-se.h
index c55a0bc..821a191 100644
--- a/include/linux/soc/qcom/geni-se.h
+++ b/include/linux/soc/qcom/geni-se.h
@@ -490,9 +490,13 @@ int geni_se_clk_freq_match(struct geni_se *se, unsigned long req_freq,
unsigned int *index, unsigned long *res_freq,
bool exact);
+void geni_se_tx_init_dma(struct geni_se *se, dma_addr_t iova, size_t len);
+
int geni_se_tx_dma_prep(struct geni_se *se, void *buf, size_t len,
dma_addr_t *iova);
+void geni_se_rx_init_dma(struct geni_se *se, dma_addr_t iova, size_t len);
+
int geni_se_rx_dma_prep(struct geni_se *se, void *buf, size_t len,
dma_addr_t *iova);
--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by the Linux Foundation.
The spi geni driver in SE DMA mode, unlike GSI DMA, is not making use of
DMA mapping functionality available in the framework.
The driver does mapping internally which makes dma buffer fields available
in spi_transfer struct superfluous while requiring additional members in
spi_geni_master struct.
Conform to the design by having framework handle map/unmap and do only
DMA transfer in the driver; this also simplifies code a bit.
Fixes: e5f0dfa78ac7 ("spi: spi-geni-qcom: Add support for SE DMA mode")
Suggested-by: Douglas Anderson <[email protected]>
Signed-off-by: Vijaya Krishna Nivarthi <[email protected]>
---
v1 -> v2:
- pass dma address to new geni interfaces instead of pointer
- set max_dma_len as per HPG
- remove expendable local variables
---
drivers/spi/spi-geni-qcom.c | 103 +++++++++++++++++++++-----------------------
1 file changed, 50 insertions(+), 53 deletions(-)
diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index e423efc..206cc04 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -97,8 +97,6 @@ struct spi_geni_master {
struct dma_chan *tx;
struct dma_chan *rx;
int cur_xfer_mode;
- dma_addr_t tx_se_dma;
- dma_addr_t rx_se_dma;
};
static int get_spi_clk_cfg(unsigned int speed_hz,
@@ -174,7 +172,7 @@ static void handle_se_timeout(struct spi_master *spi,
unmap_if_dma:
if (mas->cur_xfer_mode == GENI_SE_DMA) {
if (xfer) {
- if (xfer->tx_buf && mas->tx_se_dma) {
+ if (xfer->tx_buf) {
spin_lock_irq(&mas->lock);
reinit_completion(&mas->tx_reset_done);
writel(1, se->base + SE_DMA_TX_FSM_RST);
@@ -182,9 +180,8 @@ static void handle_se_timeout(struct spi_master *spi,
time_left = wait_for_completion_timeout(&mas->tx_reset_done, HZ);
if (!time_left)
dev_err(mas->dev, "DMA TX RESET failed\n");
- geni_se_tx_dma_unprep(se, mas->tx_se_dma, xfer->len);
}
- if (xfer->rx_buf && mas->rx_se_dma) {
+ if (xfer->rx_buf) {
spin_lock_irq(&mas->lock);
reinit_completion(&mas->rx_reset_done);
writel(1, se->base + SE_DMA_RX_FSM_RST);
@@ -192,7 +189,6 @@ static void handle_se_timeout(struct spi_master *spi,
time_left = wait_for_completion_timeout(&mas->rx_reset_done, HZ);
if (!time_left)
dev_err(mas->dev, "DMA RX RESET failed\n");
- geni_se_rx_dma_unprep(se, mas->rx_se_dma, xfer->len);
}
} else {
/*
@@ -523,17 +519,36 @@ static int setup_gsi_xfer(struct spi_transfer *xfer, struct spi_geni_master *mas
return 1;
}
+static u32 get_xfer_len_in_words(struct spi_transfer *xfer,
+ struct spi_geni_master *mas)
+{
+ u32 len;
+
+ if (!(mas->cur_bits_per_word % MIN_WORD_LEN))
+ len = xfer->len * BITS_PER_BYTE / mas->cur_bits_per_word;
+ else
+ len = xfer->len / (mas->cur_bits_per_word / BITS_PER_BYTE + 1);
+ len &= TRANS_LEN_MSK;
+
+ return len;
+}
+
static bool geni_can_dma(struct spi_controller *ctlr,
struct spi_device *slv, struct spi_transfer *xfer)
{
struct spi_geni_master *mas = spi_master_get_devdata(slv->master);
+ u32 len, fifo_size;
- /*
- * Return true if transfer needs to be mapped prior to
- * calling transfer_one which is the case only for GPI_DMA.
- * For SE_DMA mode, map/unmap is done in geni_se_*x_dma_prep.
- */
- return mas->cur_xfer_mode == GENI_GPI_DMA;
+ if (mas->cur_xfer_mode == GENI_GPI_DMA)
+ return true;
+
+ len = get_xfer_len_in_words(xfer, mas);
+ fifo_size = mas->tx_fifo_depth * mas->fifo_width_bits / mas->cur_bits_per_word;
+
+ if (len > fifo_size)
+ return true;
+ else
+ return false;
}
static int spi_geni_prepare_message(struct spi_master *spi,
@@ -772,7 +787,7 @@ static int setup_se_xfer(struct spi_transfer *xfer,
u16 mode, struct spi_master *spi)
{
u32 m_cmd = 0;
- u32 len, fifo_size;
+ u32 len;
struct geni_se *se = &mas->se;
int ret;
@@ -804,11 +819,7 @@ static int setup_se_xfer(struct spi_transfer *xfer,
mas->tx_rem_bytes = 0;
mas->rx_rem_bytes = 0;
- if (!(mas->cur_bits_per_word % MIN_WORD_LEN))
- len = xfer->len * BITS_PER_BYTE / mas->cur_bits_per_word;
- else
- len = xfer->len / (mas->cur_bits_per_word / BITS_PER_BYTE + 1);
- len &= TRANS_LEN_MSK;
+ len = get_xfer_len_in_words(xfer, mas);
mas->cur_xfer = xfer;
if (xfer->tx_buf) {
@@ -823,9 +834,20 @@ static int setup_se_xfer(struct spi_transfer *xfer,
mas->rx_rem_bytes = xfer->len;
}
- /* Select transfer mode based on transfer length */
- fifo_size = mas->tx_fifo_depth * mas->fifo_width_bits / mas->cur_bits_per_word;
- mas->cur_xfer_mode = (len <= fifo_size) ? GENI_SE_FIFO : GENI_SE_DMA;
+ /*
+ * Select DMA mode if sgt are present; and with only 1 entry
+ * This is not a serious limitation because the xfer buffers are
+ * expected to fit into in 1 entry almost always, and if any
+ * doesn't for any reason we fall back to FIFO mode anyway
+ */
+ if (!xfer->tx_sg.nents && !xfer->rx_sg.nents)
+ mas->cur_xfer_mode = GENI_SE_FIFO;
+ else if (xfer->tx_sg.nents > 1 || xfer->rx_sg.nents > 1) {
+ dev_warn_once(mas->dev, "Doing FIFO, cannot handle tx_nents-%d, rx_nents-%d\n",
+ xfer->tx_sg.nents, xfer->rx_sg.nents);
+ mas->cur_xfer_mode = GENI_SE_FIFO;
+ } else
+ mas->cur_xfer_mode = GENI_SE_DMA;
geni_se_select_mode(se, mas->cur_xfer_mode);
/*
@@ -836,35 +858,17 @@ static int setup_se_xfer(struct spi_transfer *xfer,
geni_se_setup_m_cmd(se, m_cmd, FRAGMENTATION);
if (mas->cur_xfer_mode == GENI_SE_DMA) {
- if (m_cmd & SPI_RX_ONLY) {
- ret = geni_se_rx_dma_prep(se, xfer->rx_buf,
- xfer->len, &mas->rx_se_dma);
- if (ret) {
- dev_err(mas->dev, "Failed to setup Rx dma %d\n", ret);
- mas->rx_se_dma = 0;
- goto unlock_and_return;
- }
- }
- if (m_cmd & SPI_TX_ONLY) {
- ret = geni_se_tx_dma_prep(se, (void *)xfer->tx_buf,
- xfer->len, &mas->tx_se_dma);
- if (ret) {
- dev_err(mas->dev, "Failed to setup Tx dma %d\n", ret);
- mas->tx_se_dma = 0;
- if (m_cmd & SPI_RX_ONLY) {
- /* Unmap rx buffer if duplex transfer */
- geni_se_rx_dma_unprep(se, mas->rx_se_dma, xfer->len);
- mas->rx_se_dma = 0;
- }
- goto unlock_and_return;
- }
- }
+ if (m_cmd & SPI_RX_ONLY)
+ geni_se_rx_init_dma(se, sg_dma_address(xfer->rx_sg.sgl),
+ sg_dma_len(xfer->rx_sg.sgl));
+ if (m_cmd & SPI_TX_ONLY)
+ geni_se_tx_init_dma(se, sg_dma_address(xfer->tx_sg.sgl),
+ sg_dma_len(xfer->tx_sg.sgl));
} else if (m_cmd & SPI_TX_ONLY) {
if (geni_spi_handle_tx(mas))
writel(mas->tx_wm, se->base + SE_GENI_TX_WATERMARK_REG);
}
-unlock_and_return:
spin_unlock_irq(&mas->lock);
return ret;
}
@@ -965,14 +969,6 @@ static irqreturn_t geni_spi_isr(int irq, void *data)
if (dma_rx_status & RX_RESET_DONE)
complete(&mas->rx_reset_done);
if (!mas->tx_rem_bytes && !mas->rx_rem_bytes && xfer) {
- if (xfer->tx_buf && mas->tx_se_dma) {
- geni_se_tx_dma_unprep(se, mas->tx_se_dma, xfer->len);
- mas->tx_se_dma = 0;
- }
- if (xfer->rx_buf && mas->rx_se_dma) {
- geni_se_rx_dma_unprep(se, mas->rx_se_dma, xfer->len);
- mas->rx_se_dma = 0;
- }
spi_finalize_current_transfer(spi);
mas->cur_xfer = NULL;
}
@@ -1057,6 +1053,7 @@ static int spi_geni_probe(struct platform_device *pdev)
spi->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32);
spi->num_chipselect = 4;
spi->max_speed_hz = 50000000;
+ spi->max_dma_len = 0xffff0; /* 24 bits for tx/rx dma length */
spi->prepare_message = spi_geni_prepare_message;
spi->transfer_one = spi_geni_transfer_one;
spi->can_dma = geni_can_dma;
--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by the Linux Foundation.
Hi,
On Wed, May 17, 2023 at 5:18 AM Vijaya Krishna Nivarthi
<[email protected]> wrote:
>
> The geni_se_xx_dma_prep() interfaces necessarily do DMA mapping before
> initiating DMA transfers. This is not suitable for spi where framework
> is expected to handle map/unmap.
>
> Expose new interfaces geni_se_xx_init_dma() which do only DMA transfer.
>
> Signed-off-by: Vijaya Krishna Nivarthi <[email protected]>
> ---
> v1 -> v2:
> - interfaces to take dma address as argument instead of its pointer
> ---
> drivers/soc/qcom/qcom-geni-se.c | 67 +++++++++++++++++++++++++++++-----------
> include/linux/soc/qcom/geni-se.h | 4 +++
> 2 files changed, 53 insertions(+), 18 deletions(-)
Mark and Bjorn will have to coordinate how they want to land this,
since normally patch #1 would go through the Qualcomm tree and patch
#2 through the SPI tree. In any case:
Reviewed-by: Douglas Anderson <[email protected]>
Hi,
On Wed, May 17, 2023 at 5:18 AM Vijaya Krishna Nivarthi
<[email protected]> wrote:
>
> The spi geni driver in SE DMA mode, unlike GSI DMA, is not making use of
> DMA mapping functionality available in the framework.
> The driver does mapping internally which makes dma buffer fields available
> in spi_transfer struct superfluous while requiring additional members in
> spi_geni_master struct.
>
> Conform to the design by having framework handle map/unmap and do only
> DMA transfer in the driver; this also simplifies code a bit.
>
> Fixes: e5f0dfa78ac7 ("spi: spi-geni-qcom: Add support for SE DMA mode")
I'm not 100% sure I'd tag it as a fix. It's certainly a cleanup that
was asked for when thuat patch was landed, but technically it doesn't
fix any known problems. In any case, I'll leave it to Mark to decide
if he's happy with the fixes tag and to remove it if he sees fit. IMO
no need to re-post the patch either way.
> Suggested-by: Douglas Anderson <[email protected]>
> Signed-off-by: Vijaya Krishna Nivarthi <[email protected]>
> ---
> v1 -> v2:
> - pass dma address to new geni interfaces instead of pointer
> - set max_dma_len as per HPG
> - remove expendable local variables
> ---
> drivers/spi/spi-geni-qcom.c | 103 +++++++++++++++++++++-----------------------
> 1 file changed, 50 insertions(+), 53 deletions(-)
Mark and Bjorn will have to coordinate how they want to land this,
since normally patch #1 would go through the Qualcomm tree and patch
#2 through the SPI tree. In any case:
Reviewed-by: Douglas Anderson <[email protected]>
On Wed, May 17, 2023 at 07:18:17AM -0700, Doug Anderson wrote:
> On Wed, May 17, 2023 at 5:18 AM Vijaya Krishna Nivarthi
> > The geni_se_xx_dma_prep() interfaces necessarily do DMA mapping before
> > initiating DMA transfers. This is not suitable for spi where framework
> > is expected to handle map/unmap.
> Mark and Bjorn will have to coordinate how they want to land this,
> since normally patch #1 would go through the Qualcomm tree and patch
> #2 through the SPI tree. In any case:
Bjorn?
On 17.05.2023 14:18, Vijaya Krishna Nivarthi wrote:
> The geni_se_xx_dma_prep() interfaces necessarily do DMA mapping before
> initiating DMA transfers. This is not suitable for spi where framework
> is expected to handle map/unmap.
>
> Expose new interfaces geni_se_xx_init_dma() which do only DMA transfer.
>
> Signed-off-by: Vijaya Krishna Nivarthi <[email protected]>
> ---
Reviewed-by: Konrad Dybcio <[email protected]>
Konrad
> v1 -> v2:
> - interfaces to take dma address as argument instead of its pointer
> ---
> drivers/soc/qcom/qcom-geni-se.c | 67 +++++++++++++++++++++++++++++-----------
> include/linux/soc/qcom/geni-se.h | 4 +++
> 2 files changed, 53 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> index 795a2e1..dd50a25 100644
> --- a/drivers/soc/qcom/qcom-geni-se.c
> +++ b/drivers/soc/qcom/qcom-geni-se.c
> @@ -682,6 +682,30 @@ EXPORT_SYMBOL(geni_se_clk_freq_match);
> #define GENI_SE_DMA_EOT_EN BIT(1)
> #define GENI_SE_DMA_AHB_ERR_EN BIT(2)
> #define GENI_SE_DMA_EOT_BUF BIT(0)
> +
> +/**
> + * geni_se_tx_init_dma() - Initiate TX DMA transfer on the serial engine
> + * @se: Pointer to the concerned serial engine.
> + * @iova: Mapped DMA address.
> + * @len: Length of the TX buffer.
> + *
> + * This function is used to initiate DMA TX transfer.
> + */
> +void geni_se_tx_init_dma(struct geni_se *se, dma_addr_t iova, size_t len)
> +{
> + u32 val;
> +
> + val = GENI_SE_DMA_DONE_EN;
> + val |= GENI_SE_DMA_EOT_EN;
> + val |= GENI_SE_DMA_AHB_ERR_EN;
> + writel_relaxed(val, se->base + SE_DMA_TX_IRQ_EN_SET);
> + writel_relaxed(lower_32_bits(iova), se->base + SE_DMA_TX_PTR_L);
> + writel_relaxed(upper_32_bits(iova), se->base + SE_DMA_TX_PTR_H);
> + writel_relaxed(GENI_SE_DMA_EOT_BUF, se->base + SE_DMA_TX_ATTR);
> + writel(len, se->base + SE_DMA_TX_LEN);
> +}
> +EXPORT_SYMBOL(geni_se_tx_init_dma);
> +
> /**
> * geni_se_tx_dma_prep() - Prepare the serial engine for TX DMA transfer
> * @se: Pointer to the concerned serial engine.
> @@ -697,7 +721,6 @@ int geni_se_tx_dma_prep(struct geni_se *se, void *buf, size_t len,
> dma_addr_t *iova)
> {
> struct geni_wrapper *wrapper = se->wrapper;
> - u32 val;
>
> if (!wrapper)
> return -EINVAL;
> @@ -706,17 +729,34 @@ int geni_se_tx_dma_prep(struct geni_se *se, void *buf, size_t len,
> if (dma_mapping_error(wrapper->dev, *iova))
> return -EIO;
>
> + geni_se_tx_init_dma(se, *iova, len);
> + return 0;
> +}
> +EXPORT_SYMBOL(geni_se_tx_dma_prep);
> +
> +/**
> + * geni_se_rx_init_dma() - Initiate RX DMA transfer on the serial engine
> + * @se: Pointer to the concerned serial engine.
> + * @iova: Mapped DMA address.
> + * @len: Length of the RX buffer.
> + *
> + * This function is used to initiate DMA RX transfer.
> + */
> +void geni_se_rx_init_dma(struct geni_se *se, dma_addr_t iova, size_t len)
> +{
> + u32 val;
> +
> val = GENI_SE_DMA_DONE_EN;
> val |= GENI_SE_DMA_EOT_EN;
> val |= GENI_SE_DMA_AHB_ERR_EN;
> - writel_relaxed(val, se->base + SE_DMA_TX_IRQ_EN_SET);
> - writel_relaxed(lower_32_bits(*iova), se->base + SE_DMA_TX_PTR_L);
> - writel_relaxed(upper_32_bits(*iova), se->base + SE_DMA_TX_PTR_H);
> - writel_relaxed(GENI_SE_DMA_EOT_BUF, se->base + SE_DMA_TX_ATTR);
> - writel(len, se->base + SE_DMA_TX_LEN);
> - return 0;
> + writel_relaxed(val, se->base + SE_DMA_RX_IRQ_EN_SET);
> + writel_relaxed(lower_32_bits(iova), se->base + SE_DMA_RX_PTR_L);
> + writel_relaxed(upper_32_bits(iova), se->base + SE_DMA_RX_PTR_H);
> + /* RX does not have EOT buffer type bit. So just reset RX_ATTR */
> + writel_relaxed(0, se->base + SE_DMA_RX_ATTR);
> + writel(len, se->base + SE_DMA_RX_LEN);
> }
> -EXPORT_SYMBOL(geni_se_tx_dma_prep);
> +EXPORT_SYMBOL(geni_se_rx_init_dma);
>
> /**
> * geni_se_rx_dma_prep() - Prepare the serial engine for RX DMA transfer
> @@ -733,7 +773,6 @@ int geni_se_rx_dma_prep(struct geni_se *se, void *buf, size_t len,
> dma_addr_t *iova)
> {
> struct geni_wrapper *wrapper = se->wrapper;
> - u32 val;
>
> if (!wrapper)
> return -EINVAL;
> @@ -742,15 +781,7 @@ int geni_se_rx_dma_prep(struct geni_se *se, void *buf, size_t len,
> if (dma_mapping_error(wrapper->dev, *iova))
> return -EIO;
>
> - val = GENI_SE_DMA_DONE_EN;
> - val |= GENI_SE_DMA_EOT_EN;
> - val |= GENI_SE_DMA_AHB_ERR_EN;
> - writel_relaxed(val, se->base + SE_DMA_RX_IRQ_EN_SET);
> - writel_relaxed(lower_32_bits(*iova), se->base + SE_DMA_RX_PTR_L);
> - writel_relaxed(upper_32_bits(*iova), se->base + SE_DMA_RX_PTR_H);
> - /* RX does not have EOT buffer type bit. So just reset RX_ATTR */
> - writel_relaxed(0, se->base + SE_DMA_RX_ATTR);
> - writel(len, se->base + SE_DMA_RX_LEN);
> + geni_se_rx_init_dma(se, *iova, len);
> return 0;
> }
> EXPORT_SYMBOL(geni_se_rx_dma_prep);
> diff --git a/include/linux/soc/qcom/geni-se.h b/include/linux/soc/qcom/geni-se.h
> index c55a0bc..821a191 100644
> --- a/include/linux/soc/qcom/geni-se.h
> +++ b/include/linux/soc/qcom/geni-se.h
> @@ -490,9 +490,13 @@ int geni_se_clk_freq_match(struct geni_se *se, unsigned long req_freq,
> unsigned int *index, unsigned long *res_freq,
> bool exact);
>
> +void geni_se_tx_init_dma(struct geni_se *se, dma_addr_t iova, size_t len);
> +
> int geni_se_tx_dma_prep(struct geni_se *se, void *buf, size_t len,
> dma_addr_t *iova);
>
> +void geni_se_rx_init_dma(struct geni_se *se, dma_addr_t iova, size_t len);
> +
> int geni_se_rx_dma_prep(struct geni_se *se, void *buf, size_t len,
> dma_addr_t *iova);
>
On 17.05.2023 14:18, Vijaya Krishna Nivarthi wrote:
> The spi geni driver in SE DMA mode, unlike GSI DMA, is not making use of
> DMA mapping functionality available in the framework.
> The driver does mapping internally which makes dma buffer fields available
> in spi_transfer struct superfluous while requiring additional members in
> spi_geni_master struct.
>
> Conform to the design by having framework handle map/unmap and do only
> DMA transfer in the driver; this also simplifies code a bit.
>
> Fixes: e5f0dfa78ac7 ("spi: spi-geni-qcom: Add support for SE DMA mode")
> Suggested-by: Douglas Anderson <[email protected]>
> Signed-off-by: Vijaya Krishna Nivarthi <[email protected]>
> ---
I don't really have a good insight in this code, but these changes
seem sane.
Acked-by: Konrad Dybcio <[email protected]>
Konrad
> v1 -> v2:
> - pass dma address to new geni interfaces instead of pointer
> - set max_dma_len as per HPG
> - remove expendable local variables
> ---
> drivers/spi/spi-geni-qcom.c | 103 +++++++++++++++++++++-----------------------
> 1 file changed, 50 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> index e423efc..206cc04 100644
> --- a/drivers/spi/spi-geni-qcom.c
> +++ b/drivers/spi/spi-geni-qcom.c
> @@ -97,8 +97,6 @@ struct spi_geni_master {
> struct dma_chan *tx;
> struct dma_chan *rx;
> int cur_xfer_mode;
> - dma_addr_t tx_se_dma;
> - dma_addr_t rx_se_dma;
> };
>
> static int get_spi_clk_cfg(unsigned int speed_hz,
> @@ -174,7 +172,7 @@ static void handle_se_timeout(struct spi_master *spi,
> unmap_if_dma:
> if (mas->cur_xfer_mode == GENI_SE_DMA) {
> if (xfer) {
> - if (xfer->tx_buf && mas->tx_se_dma) {
> + if (xfer->tx_buf) {
> spin_lock_irq(&mas->lock);
> reinit_completion(&mas->tx_reset_done);
> writel(1, se->base + SE_DMA_TX_FSM_RST);
> @@ -182,9 +180,8 @@ static void handle_se_timeout(struct spi_master *spi,
> time_left = wait_for_completion_timeout(&mas->tx_reset_done, HZ);
> if (!time_left)
> dev_err(mas->dev, "DMA TX RESET failed\n");
> - geni_se_tx_dma_unprep(se, mas->tx_se_dma, xfer->len);
> }
> - if (xfer->rx_buf && mas->rx_se_dma) {
> + if (xfer->rx_buf) {
> spin_lock_irq(&mas->lock);
> reinit_completion(&mas->rx_reset_done);
> writel(1, se->base + SE_DMA_RX_FSM_RST);
> @@ -192,7 +189,6 @@ static void handle_se_timeout(struct spi_master *spi,
> time_left = wait_for_completion_timeout(&mas->rx_reset_done, HZ);
> if (!time_left)
> dev_err(mas->dev, "DMA RX RESET failed\n");
> - geni_se_rx_dma_unprep(se, mas->rx_se_dma, xfer->len);
> }
> } else {
> /*
> @@ -523,17 +519,36 @@ static int setup_gsi_xfer(struct spi_transfer *xfer, struct spi_geni_master *mas
> return 1;
> }
>
> +static u32 get_xfer_len_in_words(struct spi_transfer *xfer,
> + struct spi_geni_master *mas)
> +{
> + u32 len;
> +
> + if (!(mas->cur_bits_per_word % MIN_WORD_LEN))
> + len = xfer->len * BITS_PER_BYTE / mas->cur_bits_per_word;
> + else
> + len = xfer->len / (mas->cur_bits_per_word / BITS_PER_BYTE + 1);
> + len &= TRANS_LEN_MSK;
> +
> + return len;
> +}
> +
> static bool geni_can_dma(struct spi_controller *ctlr,
> struct spi_device *slv, struct spi_transfer *xfer)
> {
> struct spi_geni_master *mas = spi_master_get_devdata(slv->master);
> + u32 len, fifo_size;
>
> - /*
> - * Return true if transfer needs to be mapped prior to
> - * calling transfer_one which is the case only for GPI_DMA.
> - * For SE_DMA mode, map/unmap is done in geni_se_*x_dma_prep.
> - */
> - return mas->cur_xfer_mode == GENI_GPI_DMA;
> + if (mas->cur_xfer_mode == GENI_GPI_DMA)
> + return true;
> +
> + len = get_xfer_len_in_words(xfer, mas);
> + fifo_size = mas->tx_fifo_depth * mas->fifo_width_bits / mas->cur_bits_per_word;
> +
> + if (len > fifo_size)
> + return true;
> + else
> + return false;
> }
>
> static int spi_geni_prepare_message(struct spi_master *spi,
> @@ -772,7 +787,7 @@ static int setup_se_xfer(struct spi_transfer *xfer,
> u16 mode, struct spi_master *spi)
> {
> u32 m_cmd = 0;
> - u32 len, fifo_size;
> + u32 len;
> struct geni_se *se = &mas->se;
> int ret;
>
> @@ -804,11 +819,7 @@ static int setup_se_xfer(struct spi_transfer *xfer,
> mas->tx_rem_bytes = 0;
> mas->rx_rem_bytes = 0;
>
> - if (!(mas->cur_bits_per_word % MIN_WORD_LEN))
> - len = xfer->len * BITS_PER_BYTE / mas->cur_bits_per_word;
> - else
> - len = xfer->len / (mas->cur_bits_per_word / BITS_PER_BYTE + 1);
> - len &= TRANS_LEN_MSK;
> + len = get_xfer_len_in_words(xfer, mas);
>
> mas->cur_xfer = xfer;
> if (xfer->tx_buf) {
> @@ -823,9 +834,20 @@ static int setup_se_xfer(struct spi_transfer *xfer,
> mas->rx_rem_bytes = xfer->len;
> }
>
> - /* Select transfer mode based on transfer length */
> - fifo_size = mas->tx_fifo_depth * mas->fifo_width_bits / mas->cur_bits_per_word;
> - mas->cur_xfer_mode = (len <= fifo_size) ? GENI_SE_FIFO : GENI_SE_DMA;
> + /*
> + * Select DMA mode if sgt are present; and with only 1 entry
> + * This is not a serious limitation because the xfer buffers are
> + * expected to fit into in 1 entry almost always, and if any
> + * doesn't for any reason we fall back to FIFO mode anyway
> + */
> + if (!xfer->tx_sg.nents && !xfer->rx_sg.nents)
> + mas->cur_xfer_mode = GENI_SE_FIFO;
> + else if (xfer->tx_sg.nents > 1 || xfer->rx_sg.nents > 1) {
> + dev_warn_once(mas->dev, "Doing FIFO, cannot handle tx_nents-%d, rx_nents-%d\n",
> + xfer->tx_sg.nents, xfer->rx_sg.nents);
> + mas->cur_xfer_mode = GENI_SE_FIFO;
> + } else
> + mas->cur_xfer_mode = GENI_SE_DMA;
> geni_se_select_mode(se, mas->cur_xfer_mode);
>
> /*
> @@ -836,35 +858,17 @@ static int setup_se_xfer(struct spi_transfer *xfer,
> geni_se_setup_m_cmd(se, m_cmd, FRAGMENTATION);
>
> if (mas->cur_xfer_mode == GENI_SE_DMA) {
> - if (m_cmd & SPI_RX_ONLY) {
> - ret = geni_se_rx_dma_prep(se, xfer->rx_buf,
> - xfer->len, &mas->rx_se_dma);
> - if (ret) {
> - dev_err(mas->dev, "Failed to setup Rx dma %d\n", ret);
> - mas->rx_se_dma = 0;
> - goto unlock_and_return;
> - }
> - }
> - if (m_cmd & SPI_TX_ONLY) {
> - ret = geni_se_tx_dma_prep(se, (void *)xfer->tx_buf,
> - xfer->len, &mas->tx_se_dma);
> - if (ret) {
> - dev_err(mas->dev, "Failed to setup Tx dma %d\n", ret);
> - mas->tx_se_dma = 0;
> - if (m_cmd & SPI_RX_ONLY) {
> - /* Unmap rx buffer if duplex transfer */
> - geni_se_rx_dma_unprep(se, mas->rx_se_dma, xfer->len);
> - mas->rx_se_dma = 0;
> - }
> - goto unlock_and_return;
> - }
> - }
> + if (m_cmd & SPI_RX_ONLY)
> + geni_se_rx_init_dma(se, sg_dma_address(xfer->rx_sg.sgl),
> + sg_dma_len(xfer->rx_sg.sgl));
> + if (m_cmd & SPI_TX_ONLY)
> + geni_se_tx_init_dma(se, sg_dma_address(xfer->tx_sg.sgl),
> + sg_dma_len(xfer->tx_sg.sgl));
> } else if (m_cmd & SPI_TX_ONLY) {
> if (geni_spi_handle_tx(mas))
> writel(mas->tx_wm, se->base + SE_GENI_TX_WATERMARK_REG);
> }
>
> -unlock_and_return:
> spin_unlock_irq(&mas->lock);
> return ret;
> }
> @@ -965,14 +969,6 @@ static irqreturn_t geni_spi_isr(int irq, void *data)
> if (dma_rx_status & RX_RESET_DONE)
> complete(&mas->rx_reset_done);
> if (!mas->tx_rem_bytes && !mas->rx_rem_bytes && xfer) {
> - if (xfer->tx_buf && mas->tx_se_dma) {
> - geni_se_tx_dma_unprep(se, mas->tx_se_dma, xfer->len);
> - mas->tx_se_dma = 0;
> - }
> - if (xfer->rx_buf && mas->rx_se_dma) {
> - geni_se_rx_dma_unprep(se, mas->rx_se_dma, xfer->len);
> - mas->rx_se_dma = 0;
> - }
> spi_finalize_current_transfer(spi);
> mas->cur_xfer = NULL;
> }
> @@ -1057,6 +1053,7 @@ static int spi_geni_probe(struct platform_device *pdev)
> spi->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32);
> spi->num_chipselect = 4;
> spi->max_speed_hz = 50000000;
> + spi->max_dma_len = 0xffff0; /* 24 bits for tx/rx dma length */
> spi->prepare_message = spi_geni_prepare_message;
> spi->transfer_one = spi_geni_transfer_one;
> spi->can_dma = geni_can_dma;
On Wed, May 17, 2023 at 05:48:12PM +0530, Vijaya Krishna Nivarthi wrote:
> A "known issue" during implementation of SE DMA for spi geni driver was
> that it does DMA map/unmap internally instead of in spi framework.
> Current patches remove this hiccup and also clean up code a bit.
Given Konrad's review I'll go ahead and apply these on a branch
(assuming my CI is happy), if there's a need to merge them into the qcom
tree I can sign a pull request (or revert the commits). Hopefully
that's OK with everyone.
On 6/6/2023 11:19 PM, Mark Brown wrote:
> On Wed, May 17, 2023 at 05:48:12PM +0530, Vijaya Krishna Nivarthi wrote:
>> A "known issue" during implementation of SE DMA for spi geni driver was
>> that it does DMA map/unmap internally instead of in spi framework.
>> Current patches remove this hiccup and also clean up code a bit.
> Given Konrad's review I'll go ahead and apply these on a branch
> (assuming my CI is happy), if there's a need to merge them into the qcom
> tree I can sign a pull request (or revert the commits). Hopefully
> that's OK with everyone.
Sounds ok to me given Bjorn seems not available until 9th.
Thank you everyone for review and time.
-Vijay/
On Wed, 17 May 2023 17:48:12 +0530, Vijaya Krishna Nivarthi wrote:
> A "known issue" during implementation of SE DMA for spi geni driver was
> that it does DMA map/unmap internally instead of in spi framework.
> Current patches remove this hiccup and also clean up code a bit.
>
> Testing revealed no regressions and results with 1000 iterations of
> reading from EC showed no loss of performance.
> Results
> =======
> Before - Iteration 999, min=5.10, max=5.17, avg=5.14, ints=25129
> After - Iteration 999, min=5.10, max=5.20, avg=5.15, ints=25153
>
> [...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
Thanks!
[1/2] soc: qcom: geni-se: Add interfaces geni_se_tx_init_dma() and geni_se_rx_init_dma()
commit: 6d6e57594957ee9131bc3802dfc8657ca6f78fee
[2/2] spi: spi-geni-qcom: Do not do DMA map/unmap inside driver, use framework instead
commit: 3a76c7ca9e77269dd10cf21465a055274cfa40c6
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark